| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aBr-cScjbkhyoBFA@liuwe-devbox-ubuntu-v2.tail21d00.ts.net>
Date: Wed, 7 May 2025 06:32:17 +0000
From: Wei Liu <wei.liu@...nel.org>
To: Roman Kisel <romank@...ux.microsoft.com>
Cc: bp@...en8.de, dave.hansen@...ux.intel.com, decui@...rosoft.com,
haiyangz@...rosoft.com, hpa@...or.com, kys@...rosoft.com,
mikelley@...rosoft.com, mingo@...hat.com, tglx@...utronix.de,
tiala@...rosoft.com, wei.liu@...nel.org,
linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org,
x86@...nel.org, apais@...rosoft.com, benhill@...rosoft.com,
bperkins@...rosoft.com, sunilmut@...rosoft.com
Subject: Re: [PATCH hyperv-next v4] x86/hyperv: Fix CPU number and VP index
confusion in hv_snp_boot_ap()
On Tue, May 06, 2025 at 10:42:49AM -0700, Roman Kisel wrote:
> To start an application processor in SNP-isolated guest, a hypercall
> is used that takes a virtual processor index. The hv_snp_boot_ap()
> function uses that START_VP hypercall but passes the CPU number to it
> instead of the VP index.
>
> As those two aren't generally interchangeable, that may lead to hung
> APs if the VP index and the APIC ID don't match up.
>
> Use the APIC ID to the VP index conversion to provide the correct
> input to the hypercall.
>
> Cc: stable@...r.kernel.org
> Fixes: 44676bb9d566 ("x86/hyperv: Add smp support for SEV-SNP guest")
> Signed-off-by: Roman Kisel <romank@...ux.microsoft.com>
> Reviewed-by: Michael Kelley <mhklinux@...look.com>
> ---
> When merging this change to other trees
> 2d2d4d8bb00 ("arch/x86: Provide the CPU number in the wakeup AP callback")
> is needed.
This is dropped from the tree because it breaks builds.
Since this needs to be backported, it makes more sense to apply this
patch first before the other one (assuming that one doesn't need to be
backported).
Thanks,
Wei.
>
> [V4]
> - Rebased on the latest hyperv-next branch, and updated the
> title and the commit message as some changes were superceeded
> by 2d2d4d8bb00 ("arch/x86: Provide the CPU number in the wakeup AP callback").
> ** Thank you, Wei! **
>
> [V3]
> https://lore.kernel.org/linux-hyperv/20250428182705.132755-1-romank@linux.microsoft.com/
> - Removed the misleading comment about the APIC ID and VP indices.
> - Removed the not sufficiently founded if statement that was added
> to the previous version of the patch to avoid the O(n) time complexity.
> I'll follow up with a separate patch to address that as that pattern
> has crept into other places in the code in the AP wakeup path.
> Fixed the logging message to use the "VP index" terminology
> consistently.
> ** Thank you, Michael! **
>
> [V2]
> https://lore.kernel.org/linux-hyperv/20250425213512.1837061-1-romank@linux.microsoft.com/
> - Fixed the terminology in the patch and other code to use
> the term "VP index" consistently
> ** Thank you, Michael! **
>
> - Missed not enabling the SNP-SEV options in the local testing,
> and sent a patch that breaks the build.
> ** Thank you, Saurabh! **
>
> - Added comments and getting the Linux kernel CPU number from
> the available data.
>
> [V1]
> https://lore.kernel.org/linux-hyperv/20250424215746.467281-1-romank@linux.microsoft.com/
> ---
> arch/x86/hyperv/hv_init.c | 33 +++++++++++++++++++++++++
> arch/x86/hyperv/hv_vtl.c | 44 +++++----------------------------
> arch/x86/hyperv/ivm.c | 8 +++++-
> arch/x86/include/asm/mshyperv.h | 2 ++
> include/hyperv/hvgdk_mini.h | 2 +-
> 5 files changed, 49 insertions(+), 40 deletions(-)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 3b569291dfed..9a8fc144e195 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -672,3 +672,36 @@ bool hv_is_hyperv_initialized(void)
> return hypercall_msr.enable;
> }
> EXPORT_SYMBOL_GPL(hv_is_hyperv_initialized);
> +
> +int hv_apicid_to_vp_index(u32 apic_id)
> +{
> + u64 control;
> + u64 status;
> + unsigned long irq_flags;
> + struct hv_get_vp_from_apic_id_in *input;
> + u32 *output, ret;
> +
> + local_irq_save(irq_flags);
> +
> + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> + memset(input, 0, sizeof(*input));
> + input->partition_id = HV_PARTITION_ID_SELF;
> + input->apic_ids[0] = apic_id;
> +
> + output = *this_cpu_ptr(hyperv_pcpu_output_arg);
> +
> + control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_INDEX_FROM_APIC_ID;
> + status = hv_do_hypercall(control, input, output);
> + ret = output[0];
> +
> + local_irq_restore(irq_flags);
> +
> + if (!hv_result_success(status)) {
> + pr_err("failed to get vp index from apic id %d, status %#llx\n",
> + apic_id, status);
> + return -EINVAL;
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(hv_apicid_to_vp_index);
> diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> index cc8c6817f704..3d149a2ca4c8 100644
> --- a/arch/x86/hyperv/hv_vtl.c
> +++ b/arch/x86/hyperv/hv_vtl.c
> @@ -211,55 +211,23 @@ static int hv_vtl_bringup_vcpu(u32 target_vp_index, int cpu, u64 eip_ignored)
> return ret;
> }
>
> -static int hv_vtl_apicid_to_vp_id(u32 apic_id)
> -{
> - u64 control;
> - u64 status;
> - unsigned long irq_flags;
> - struct hv_get_vp_from_apic_id_in *input;
> - u32 *output, ret;
> -
> - local_irq_save(irq_flags);
> -
> - input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> - memset(input, 0, sizeof(*input));
> - input->partition_id = HV_PARTITION_ID_SELF;
> - input->apic_ids[0] = apic_id;
> -
> - output = *this_cpu_ptr(hyperv_pcpu_output_arg);
> -
> - control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_ID_FROM_APIC_ID;
> - status = hv_do_hypercall(control, input, output);
> - ret = output[0];
> -
> - local_irq_restore(irq_flags);
> -
> - if (!hv_result_success(status)) {
> - pr_err("failed to get vp id from apic id %d, status %#llx\n",
> - apic_id, status);
> - return -EINVAL;
> - }
> -
> - return ret;
> -}
> -
> static int hv_vtl_wakeup_secondary_cpu(u32 apicid, unsigned long start_eip, unsigned int cpu)
> {
> - int vp_id;
> + int vp_index;
>
> pr_debug("Bringing up CPU with APIC ID %d in VTL2...\n", apicid);
> - vp_id = hv_vtl_apicid_to_vp_id(apicid);
> + vp_index = hv_apicid_to_vp_index(apicid);
>
> - if (vp_id < 0) {
> + if (vp_index < 0) {
> pr_err("Couldn't find CPU with APIC ID %d\n", apicid);
> return -EINVAL;
> }
> - if (vp_id > ms_hyperv.max_vp_index) {
> - pr_err("Invalid CPU id %d for APIC ID %d\n", vp_id, apicid);
> + if (vp_index > ms_hyperv.max_vp_index) {
> + pr_err("Invalid CPU id %d for APIC ID %d\n", vp_index, apicid);
> return -EINVAL;
> }
>
> - return hv_vtl_bringup_vcpu(vp_id, cpu, start_eip);
> + return hv_vtl_bringup_vcpu(vp_index, cpu, start_eip);
> }
>
> int __init hv_vtl_early_init(void)
> diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
> index 30aa76974a3a..71918817129e 100644
> --- a/arch/x86/hyperv/ivm.c
> +++ b/arch/x86/hyperv/ivm.c
> @@ -297,10 +297,16 @@ int hv_snp_boot_ap(u32 apic_id, unsigned long start_ip, unsigned int cpu)
> u64 ret, retry = 5;
> struct hv_enable_vp_vtl *start_vp_input;
> unsigned long flags;
> + int vp_index;
>
> if (!vmsa)
> return -ENOMEM;
>
> + /* Find the Hyper-V VP index which might be not the same as APIC ID */
> + vp_index = hv_apicid_to_vp_index(apic_id);
> + if (vp_index < 0 || vp_index > ms_hyperv.max_vp_index)
> + return -EINVAL;
> +
> native_store_gdt(&gdtr);
>
> vmsa->gdtr.base = gdtr.address;
> @@ -348,7 +354,7 @@ int hv_snp_boot_ap(u32 apic_id, unsigned long start_ip, unsigned int cpu)
> start_vp_input = (struct hv_enable_vp_vtl *)ap_start_input_arg;
> memset(start_vp_input, 0, sizeof(*start_vp_input));
> start_vp_input->partition_id = -1;
> - start_vp_input->vp_index = cpu;
> + start_vp_input->vp_index = vp_index;
> start_vp_input->target_vtl.target_vtl = ms_hyperv.vtl;
> *(u64 *)&start_vp_input->vp_context = __pa(vmsa) | 1;
>
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 3bfc066aeccc..5ec92e3e2e37 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -307,6 +307,7 @@ static __always_inline u64 hv_raw_get_msr(unsigned int reg)
> {
> return __rdmsr(reg);
> }
> +int hv_apicid_to_vp_index(u32 apic_id);
>
> #else /* CONFIG_HYPERV */
> static inline void hyperv_init(void) {}
> @@ -328,6 +329,7 @@ static inline void hv_set_msr(unsigned int reg, u64 value) { }
> static inline u64 hv_get_msr(unsigned int reg) { return 0; }
> static inline void hv_set_non_nested_msr(unsigned int reg, u64 value) { }
> static inline u64 hv_get_non_nested_msr(unsigned int reg) { return 0; }
> +static inline int hv_apicid_to_vp_index(u32 apic_id) { return -EINVAL; }
> #endif /* CONFIG_HYPERV */
>
>
> diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
> index cf0923dc727d..2d431b53f587 100644
> --- a/include/hyperv/hvgdk_mini.h
> +++ b/include/hyperv/hvgdk_mini.h
> @@ -475,7 +475,7 @@ union hv_vp_assist_msr_contents { /* HV_REGISTER_VP_ASSIST_PAGE */
> #define HVCALL_CREATE_PORT 0x0095
> #define HVCALL_CONNECT_PORT 0x0096
> #define HVCALL_START_VP 0x0099
> -#define HVCALL_GET_VP_ID_FROM_APIC_ID 0x009a
> +#define HVCALL_GET_VP_INDEX_FROM_APIC_ID 0x009a
> #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE 0x00af
> #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_LIST 0x00b0
> #define HVCALL_SIGNAL_EVENT_DIRECT 0x00c0
>
> base-commit: 1b019573c9662c17a33419367a217abe1f5e8060
> --
> 2.43.0
>
Powered by blists - more mailing lists