| 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: <29153c28-bf32-43b3-8c5b-b93c0881dad9@linux.microsoft.com>
Date: Wed, 30 Apr 2025 12:52:01 -0700
From: Roman Kisel <romank@...ux.microsoft.com>
To: Tom Lendacky <thomas.lendacky@....com>, ardb@...nel.org, bp@...en8.de,
dave.hansen@...ux.intel.com, decui@...rosoft.com, dimitri.sivanich@....com,
haiyangz@...rosoft.com, hpa@...or.com, imran.f.khan@...cle.com,
jacob.jun.pan@...ux.intel.com, jgross@...e.com, justin.ernst@....com,
kprateek.nayak@....com, kyle.meyer@....com, kys@...rosoft.com,
lenb@...nel.org, mingo@...hat.com, nikunj@....com, papaluri@....com,
perry.yuan@....com, peterz@...radead.org, rafael@...nel.org,
russ.anderson@....com, steve.wahl@....com, tglx@...utronix.de,
tim.c.chen@...ux.intel.com, tony.luck@...el.com, wei.liu@...nel.org,
xin@...or.com, yuehaibing@...wei.com, linux-acpi@...r.kernel.org,
linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org, x86@...nel.org
Cc: apais@...rosoft.com, benhill@...rosoft.com, bperkins@...rosoft.com,
sunilmut@...rosoft.com
Subject: Re: [PATCH hyperv-next v2] arch/x86: Provide the CPU number in the
wakeup AP callback
On 4/30/2025 11:04 AM, Tom Lendacky wrote:
> On 4/30/25 11:14, Roman Kisel wrote:
>> When starting APs, confidential guests and paravisor guests
>> need to know the CPU number, and the pattern of using the linear
>> search has emerged in several places. With N processors that leads
>> to the O(N^2) time complexity.
>>
>> Provide the CPU number in the AP wake up callback so that one can
>> get the CPU number in constant time.
>>
>> Suggested-by: Michael Kelley <mhklinux@...look.com>
>> Signed-off-by: Roman Kisel <romank@...ux.microsoft.com>
>> ---
>> The diff in ivm.c might catch your eye but that code mixes up the
>> APIC ID and the CPU number anyway. That is fixed in another patch:
>> https://lore.kernel.org/linux-hyperv/20250428182705.132755-1-romank@linux.microsoft.com/
>> independently of this one (being an optimization).
>> I separated the two as this one might be more disputatious due to
>> the change in the API (although it is a tiny one and comes with
>> the benefits).
>>
>> [V2]
>> - Remove the struct used in v1 in favor of passing the CPU number
>> directly to the callback not to increase complexity.
>> ** Thank you, Michael! **
>> [V1]
>> https://lore.kernel.org/linux-hyperv/20250428225948.810147-1-romank@linux.microsoft.com/
>> ---
>> arch/x86/coco/sev/core.c | 13 ++-----------
>> arch/x86/hyperv/hv_vtl.c | 12 ++----------
>> arch/x86/hyperv/ivm.c | 2 +-
>> arch/x86/include/asm/apic.h | 8 ++++----
>> arch/x86/include/asm/mshyperv.h | 4 ++--
>> arch/x86/kernel/acpi/madt_wakeup.c | 2 +-
>> arch/x86/kernel/apic/apic_noop.c | 2 +-
>> arch/x86/kernel/apic/x2apic_uv_x.c | 2 +-
>> arch/x86/kernel/smpboot.c | 8 ++++----
>> 9 files changed, 18 insertions(+), 35 deletions(-)
>>
>
> The change to wakeup_secondary_cpu_via_init() isn't needed but does
> provide consistency. I'll leave that to the maintainers to decide if that
> function should remain as is.
>
> Otherwise,
>
> Reviewed-by: Tom Lendacky <thomas.lendacky@....com>
Tom,
appreciate takling the time to review, thank you!
>
>> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
>> index 82492efc5d94..e7b6dba30a0a 100644
>> --- a/arch/x86/coco/sev/core.c
>> +++ b/arch/x86/coco/sev/core.c
>> @@ -1179,7 +1179,7 @@ static void snp_cleanup_vmsa(struct sev_es_save_area *vmsa, int apic_id)
>> free_page((unsigned long)vmsa);
>> }
>>
>> -static int wakeup_cpu_via_vmgexit(u32 apic_id, unsigned long start_ip)
>> +static int wakeup_cpu_via_vmgexit(u32 apic_id, unsigned long start_ip, int cpu)
>> {
>> struct sev_es_save_area *cur_vmsa, *vmsa;
>> struct ghcb_state state;
>> @@ -1187,7 +1187,7 @@ static int wakeup_cpu_via_vmgexit(u32 apic_id, unsigned long start_ip)
>> unsigned long flags;
>> struct ghcb *ghcb;
>> u8 sipi_vector;
>> - int cpu, ret;
>> + int ret;
>> u64 cr4;
>>
>> /*
>> @@ -1208,15 +1208,6 @@ static int wakeup_cpu_via_vmgexit(u32 apic_id, unsigned long start_ip)
>>
>> /* Override start_ip with known protected guest start IP */
>> start_ip = real_mode_header->sev_es_trampoline_start;
>> -
>> - /* Find the logical CPU for the APIC ID */
>> - for_each_present_cpu(cpu) {
>> - if (arch_match_cpu_phys_id(cpu, apic_id))
>> - break;
>> - }
>> - if (cpu >= nr_cpu_ids)
>> - return -EINVAL;
>> -
>> cur_vmsa = per_cpu(sev_vmsa, cpu);
>>
>> /*
>> diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
>> index 582fe820e29c..5784b6c56ca4 100644
>> --- a/arch/x86/hyperv/hv_vtl.c
>> +++ b/arch/x86/hyperv/hv_vtl.c
>> @@ -237,17 +237,9 @@ static int hv_vtl_apicid_to_vp_id(u32 apic_id)
>> return ret;
>> }
>>
>> -static int hv_vtl_wakeup_secondary_cpu(u32 apicid, unsigned long start_eip)
>> +static int hv_vtl_wakeup_secondary_cpu(u32 apicid, unsigned long start_eip, int cpu)
>> {
>> - int vp_id, cpu;
>> -
>> - /* Find the logical CPU for the APIC ID */
>> - for_each_present_cpu(cpu) {
>> - if (arch_match_cpu_phys_id(cpu, apicid))
>> - break;
>> - }
>> - if (cpu >= nr_cpu_ids)
>> - return -EINVAL;
>> + int vp_id;
>>
>> pr_debug("Bringing up CPU with APIC ID %d in VTL2...\n", apicid);
>> vp_id = hv_vtl_apicid_to_vp_id(apicid);
>> diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
>> index c0039a90e9e0..ba744dbc22bb 100644
>> --- a/arch/x86/hyperv/ivm.c
>> +++ b/arch/x86/hyperv/ivm.c
>> @@ -288,7 +288,7 @@ static void snp_cleanup_vmsa(struct sev_es_save_area *vmsa)
>> free_page((unsigned long)vmsa);
>> }
>>
>> -int hv_snp_boot_ap(u32 cpu, unsigned long start_ip)
>> +int hv_snp_boot_ap(u32 apic_id, unsigned long start_ip, int cpu)
>> {
>> struct sev_es_save_area *vmsa = (struct sev_es_save_area *)
>> __get_free_page(GFP_KERNEL | __GFP_ZERO);
>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>> index f21ff1932699..a480f7626847 100644
>> --- a/arch/x86/include/asm/apic.h
>> +++ b/arch/x86/include/asm/apic.h
>> @@ -313,9 +313,9 @@ struct apic {
>> u32 (*get_apic_id)(u32 id);
>>
>> /* wakeup_secondary_cpu */
>> - int (*wakeup_secondary_cpu)(u32 apicid, unsigned long start_eip);
>> + int (*wakeup_secondary_cpu)(u32 apicid, unsigned long start_eip, int cpu);
>> /* wakeup secondary CPU using 64-bit wakeup point */
>> - int (*wakeup_secondary_cpu_64)(u32 apicid, unsigned long start_eip);
>> + int (*wakeup_secondary_cpu_64)(u32 apicid, unsigned long start_eip, int cpu);
>>
>> char *name;
>> };
>> @@ -333,8 +333,8 @@ struct apic_override {
>> void (*send_IPI_self)(int vector);
>> u64 (*icr_read)(void);
>> void (*icr_write)(u32 low, u32 high);
>> - int (*wakeup_secondary_cpu)(u32 apicid, unsigned long start_eip);
>> - int (*wakeup_secondary_cpu_64)(u32 apicid, unsigned long start_eip);
>> + int (*wakeup_secondary_cpu)(u32 apicid, unsigned long start_eip, int cpu);
>> + int (*wakeup_secondary_cpu_64)(u32 apicid, unsigned long start_eip, int cpu);
>> };
>>
>> /*
>> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
>> index 07aadf0e839f..abca9d8d4a82 100644
>> --- a/arch/x86/include/asm/mshyperv.h
>> +++ b/arch/x86/include/asm/mshyperv.h
>> @@ -268,11 +268,11 @@ int hv_unmap_ioapic_interrupt(int ioapic_id, struct hv_interrupt_entry *entry);
>> #ifdef CONFIG_AMD_MEM_ENCRYPT
>> bool hv_ghcb_negotiate_protocol(void);
>> void __noreturn hv_ghcb_terminate(unsigned int set, unsigned int reason);
>> -int hv_snp_boot_ap(u32 cpu, unsigned long start_ip);
>> +int hv_snp_boot_ap(u32 apic_id, unsigned long start_ip, int cpu);
>> #else
>> static inline bool hv_ghcb_negotiate_protocol(void) { return false; }
>> static inline void hv_ghcb_terminate(unsigned int set, unsigned int reason) {}
>> -static inline int hv_snp_boot_ap(u32 cpu, unsigned long start_ip) { return 0; }
>> +static inline int hv_snp_boot_ap(u32 apic_id, unsigned long start_ip, int cpu) { return 0; }
>> #endif
>>
>> #if defined(CONFIG_AMD_MEM_ENCRYPT) || defined(CONFIG_INTEL_TDX_GUEST)
>> diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
>> index d5ef6215583b..d95c806f0e93 100644
>> --- a/arch/x86/kernel/acpi/madt_wakeup.c
>> +++ b/arch/x86/kernel/acpi/madt_wakeup.c
>> @@ -169,7 +169,7 @@ static int __init acpi_mp_setup_reset(u64 reset_vector)
>> return 0;
>> }
>>
>> -static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
>> +static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip, int cpu)
>> {
>> if (!acpi_mp_wake_mailbox_paddr) {
>> pr_warn_once("No MADT mailbox: cannot bringup secondary CPUs. Booting with kexec?\n");
>> diff --git a/arch/x86/kernel/apic/apic_noop.c b/arch/x86/kernel/apic/apic_noop.c
>> index b5bb7a2e8340..cab7cac16f53 100644
>> --- a/arch/x86/kernel/apic/apic_noop.c
>> +++ b/arch/x86/kernel/apic/apic_noop.c
>> @@ -27,7 +27,7 @@ static void noop_send_IPI_allbutself(int vector) { }
>> static void noop_send_IPI_all(int vector) { }
>> static void noop_send_IPI_self(int vector) { }
>> static void noop_apic_icr_write(u32 low, u32 id) { }
>> -static int noop_wakeup_secondary_cpu(u32 apicid, unsigned long start_eip) { return -1; }
>> +static int noop_wakeup_secondary_cpu(u32 apicid, unsigned long start_eip, int cpu) { return -1; }
>> static u64 noop_apic_icr_read(void) { return 0; }
>> static u32 noop_get_apic_id(u32 apicid) { return 0; }
>> static void noop_apic_eoi(void) { }
>> diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c
>> index 7fef504ca508..f8000555127b 100644
>> --- a/arch/x86/kernel/apic/x2apic_uv_x.c
>> +++ b/arch/x86/kernel/apic/x2apic_uv_x.c
>> @@ -667,7 +667,7 @@ static __init void build_uv_gr_table(void)
>> }
>> }
>>
>> -static int uv_wakeup_secondary(u32 phys_apicid, unsigned long start_rip)
>> +static int uv_wakeup_secondary(u32 phys_apicid, unsigned long start_rip, int cpu)
>> {
>> unsigned long val;
>> int pnode;
>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>> index c10850ae6f09..4b514e485f9c 100644
>> --- a/arch/x86/kernel/smpboot.c
>> +++ b/arch/x86/kernel/smpboot.c
>> @@ -715,7 +715,7 @@ static void send_init_sequence(u32 phys_apicid)
>> /*
>> * Wake up AP by INIT, INIT, STARTUP sequence.
>> */
>> -static int wakeup_secondary_cpu_via_init(u32 phys_apicid, unsigned long start_eip)
>> +static int wakeup_secondary_cpu_via_init(u32 phys_apicid, unsigned long start_eip, int cpu)
>> {
>> unsigned long send_status = 0, accept_status = 0;
>> int num_starts, j, maxlvt;
>> @@ -916,11 +916,11 @@ static int do_boot_cpu(u32 apicid, int cpu, struct task_struct *idle)
>> * - Use an INIT boot APIC message
>> */
>> if (apic->wakeup_secondary_cpu_64)
>> - ret = apic->wakeup_secondary_cpu_64(apicid, start_ip);
>> + ret = apic->wakeup_secondary_cpu_64(apicid, start_ip, cpu);
>> else if (apic->wakeup_secondary_cpu)
>> - ret = apic->wakeup_secondary_cpu(apicid, start_ip);
>> + ret = apic->wakeup_secondary_cpu(apicid, start_ip, cpu);
>> else
>> - ret = wakeup_secondary_cpu_via_init(apicid, start_ip);
>> + ret = wakeup_secondary_cpu_via_init(apicid, start_ip, cpu);
>>
>> /* If the wakeup mechanism failed, cleanup the warm reset vector */
>> if (ret)
>>
>> base-commit: 628cc040b3a2980df6032766e8ef0688e981ab95
--
Thank you,
Roman
Powered by blists - more mailing lists