| Message ID | 20250430204720.108962-1-romank@linux.microsoft.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [hyperv-next,v3] arch/x86: Provide the CPU number in the wakeup AP callback | expand |
From: Roman Kisel <romank@linux.microsoft.com> Sent: Wednesday, April 30, 2025 1:47 PM > > 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@outlook.com> > Signed-off-by: Roman Kisel <romank@linux.microsoft.com> > Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> LGTM. Reviewed-by: Michael Kelley <mhklinux@outlook.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). > > [V3] > - Fixed the cpu nummber to be unsigned int within the patch and > in the do_boot_cpu() function. > ** Thank you, Thomas! ** > > [V2] > https://lore.kernel.org/linux-hyperv/20250430161413.276759-1-romank@linux.microsoft.com/ > - 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 | 5 +++-- > arch/x86/kernel/acpi/madt_wakeup.c | 2 +- > arch/x86/kernel/apic/apic_noop.c | 8 +++++++- > arch/x86/kernel/apic/x2apic_uv_x.c | 2 +- > arch/x86/kernel/smpboot.c | 10 +++++----- > 9 files changed, 26 insertions(+), 36 deletions(-) > > diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c > index 82492efc5d94..8b6d310b61b9 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, unsigned 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..4d6e0e198041 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, unsigned 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..6025da891a83 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, unsigned 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..33f677e2db75 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, unsigned 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, unsigned 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, unsigned int cpu); > + int (*wakeup_secondary_cpu_64)(u32 apicid, unsigned long start_eip, unsigned int cpu); > }; > > /* > diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h > index 07aadf0e839f..cab952f722e4 100644 > --- a/arch/x86/include/asm/mshyperv.h > +++ b/arch/x86/include/asm/mshyperv.h > @@ -268,11 +268,12 @@ 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, unsigned 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, > + unsigned 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..f48581888d53 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, unsigned 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..58abb941c45b 100644 > --- a/arch/x86/kernel/apic/apic_noop.c > +++ b/arch/x86/kernel/apic/apic_noop.c > @@ -27,7 +27,13 @@ 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, > + unsigned 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..15209f220e1f 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, unsigned int cpu) > { > unsigned long val; > int pnode; > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > index c10850ae6f09..b013296b100f 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, unsigned int cpu) > { > unsigned long send_status = 0, accept_status = 0; > int num_starts, j, maxlvt; > @@ -862,7 +862,7 @@ int common_cpu_up(unsigned int cpu, struct task_struct *idle) > * Returns zero if startup was successfully sent, else error code from > * ->wakeup_secondary_cpu. > */ > -static int do_boot_cpu(u32 apicid, int cpu, struct task_struct *idle) > +static int do_boot_cpu(u32 apicid, unsigned int cpu, struct task_struct *idle) > { > unsigned long start_ip = real_mode_header->trampoline_start; > int ret; > @@ -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 > -- > 2.43.0 >
On Wed, Apr 30 2025 at 13:47, 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@outlook.com> > Signed-off-by: Roman Kisel <romank@linux.microsoft.com> > Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
On Wed, Apr 30, 2025 at 01:47:20PM -0700, 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@outlook.com> > Signed-off-by: Roman Kisel <romank@linux.microsoft.com> > Reviewed-by: Tom Lendacky <thomas.lendacky@amd.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). > > [V3] > - Fixed the cpu nummber to be unsigned int within the patch and > in the do_boot_cpu() function. > ** Thank you, Thomas! ** > > [V2] > https://lore.kernel.org/linux-hyperv/20250430161413.276759-1-romank@linux.microsoft.com/ > - 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 | 5 +++-- > arch/x86/kernel/acpi/madt_wakeup.c | 2 +- > arch/x86/kernel/apic/apic_noop.c | 8 +++++++- > arch/x86/kernel/apic/x2apic_uv_x.c | 2 +- > arch/x86/kernel/smpboot.c | 10 +++++----- Since this is tagged as a hyperv-next patch, I'm happy to pick this up. Some changes should be acked by x86 maintainers. Thanks, Wei. > 9 files changed, 26 insertions(+), 36 deletions(-) > > diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c > index 82492efc5d94..8b6d310b61b9 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, unsigned 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..4d6e0e198041 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, unsigned 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..6025da891a83 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, unsigned 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..33f677e2db75 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, unsigned 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, unsigned 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, unsigned int cpu); > + int (*wakeup_secondary_cpu_64)(u32 apicid, unsigned long start_eip, unsigned int cpu); > }; > > /* > diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h > index 07aadf0e839f..cab952f722e4 100644 > --- a/arch/x86/include/asm/mshyperv.h > +++ b/arch/x86/include/asm/mshyperv.h > @@ -268,11 +268,12 @@ 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, unsigned 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, > + unsigned 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..f48581888d53 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, unsigned 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..58abb941c45b 100644 > --- a/arch/x86/kernel/apic/apic_noop.c > +++ b/arch/x86/kernel/apic/apic_noop.c > @@ -27,7 +27,13 @@ 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, > + unsigned 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..15209f220e1f 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, unsigned int cpu) > { > unsigned long val; > int pnode; > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > index c10850ae6f09..b013296b100f 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, unsigned int cpu) > { > unsigned long send_status = 0, accept_status = 0; > int num_starts, j, maxlvt; > @@ -862,7 +862,7 @@ int common_cpu_up(unsigned int cpu, struct task_struct *idle) > * Returns zero if startup was successfully sent, else error code from > * ->wakeup_secondary_cpu. > */ > -static int do_boot_cpu(u32 apicid, int cpu, struct task_struct *idle) > +static int do_boot_cpu(u32 apicid, unsigned int cpu, struct task_struct *idle) > { > unsigned long start_ip = real_mode_header->trampoline_start; > int ret; > @@ -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 > -- > 2.43.0 >
On 5/2/2025 10:32 AM, Wei Liu wrote: > On Wed, Apr 30, 2025 at 01:47:20PM -0700, Roman Kisel wrote: [...] >> 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 | 5 +++-- >> arch/x86/kernel/acpi/madt_wakeup.c | 2 +- >> arch/x86/kernel/apic/apic_noop.c | 8 +++++++- >> arch/x86/kernel/apic/x2apic_uv_x.c | 2 +- >> arch/x86/kernel/smpboot.c | 10 +++++----- > > Since this is tagged as a hyperv-next patch, I'm happy to pick this up. > Thank you very much, Wei! > Some changes should be acked by x86 maintainers. Tom and Thomas reviewed the patch, and the `scripts/get_maintainer.pl` prints them as x86 maintainers. If I understand correctly what you're saying, someone who sends patches from the x86 tree to Linus should add Acked-by to the patch. Likely I should just wait until such person gets to this patch. If I'm misunderstanding, I'd appreciate a quick note to help me navigate this :) > > Thanks, > Wei. >
On Mon, May 05, 2025 at 10:22:47AM -0700, Roman Kisel wrote: > > > On 5/2/2025 10:32 AM, Wei Liu wrote: > > On Wed, Apr 30, 2025 at 01:47:20PM -0700, Roman Kisel wrote: > > [...] > > > > 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 | 5 +++-- > > > arch/x86/kernel/acpi/madt_wakeup.c | 2 +- > > > arch/x86/kernel/apic/apic_noop.c | 8 +++++++- > > > arch/x86/kernel/apic/x2apic_uv_x.c | 2 +- > > > arch/x86/kernel/smpboot.c | 10 +++++----- > > > > Since this is tagged as a hyperv-next patch, I'm happy to pick this up. > > > > Thank you very much, Wei! > > > Some changes should be acked by x86 maintainers. > > Tom and Thomas reviewed the patch, and the `scripts/get_maintainer.pl` > prints them as x86 maintainers. If I understand correctly what you're > saying, someone who sends patches from the x86 tree to Linus should add > Acked-by to the patch. Likely I should just wait until such person gets > to this patch. > > If I'm misunderstanding, I'd appreciate a quick note to help me navigate > this :) > $ ./scripts/get_maintainer.pl -f arch/x86/kernel Thomas Gleixner <tglx@linutronix.de> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),commit_signer:89/608=15%) Ingo Molnar <mingo@redhat.com> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),commit_signer:180/608=30%) Borislav Petkov <bp@alien8.de> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),commit_signer:245/608=40%) Dave Hansen <dave.hansen@linux.intel.com> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),commit_signer:70/608=12%) x86@kernel.org (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)) "H. Peter Anvin" <hpa@zytor.com> (reviewer:X86 ARCHITECTURE (32-BIT AND 64-BIT)) Reinette Chatre <reinette.chatre@intel.com> (commit_signer:65/608=11%) Tony Luck <tony.luck@intel.com> (authored:31/608=5%) James Morse <james.morse@arm.com> (authored:31/608=5%) linux-kernel@vger.kernel.org (open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)) I'm looking for acks / reviews from Thomas, Ingo, Borislav, or Dave. If Thomas had given an ack or a review before, you should've added that to the patch here. You don't need to do that for this patch. Please point me to Thomas' reply to the previous version and I can add the missing tag to patch while I queue it. Thanks, Wei.
On 5/5/25 11:01, Wei Liu wrote: > You don't need to do that for this patch. Please point me to Thomas' > reply to the previous version and I can add the missing tag to patch > while I queue it. It's right here: https://lore.kernel.org/all/8734dnouq6.ffs@tglx/ It's pretty darn trivial to find if you do any of the following: 1. Read this thread in your mail reader that understands threads, or 2. Look at this thread on lore, or 3. Run "b4 am" on the thread Sure, it can be kinda hard to do #1/#2 on gigantic threads if there are a ton of replies from the maintainer. But there were 5 messages in this thread and only one from Thomas. I'd highly suggest adding one or more of those tools to your maintainer toolbox! b4, especially, does all the work for you.
On Mon, May 05, 2025 at 11:12:10AM -0700, Dave Hansen wrote: > On 5/5/25 11:01, Wei Liu wrote: > > You don't need to do that for this patch. Please point me to Thomas' > > reply to the previous version and I can add the missing tag to patch > > while I queue it. > > It's right here: > > https://lore.kernel.org/all/8734dnouq6.ffs@tglx/ Thanks for the pointer. This is appreciated. > > It's pretty darn trivial to find if you do any of the following: > > 1. Read this thread in your mail reader that understands threads, or > 2. Look at this thread on lore, or > 3. Run "b4 am" on the thread > > Sure, it can be kinda hard to do #1/#2 on gigantic threads if there are > a ton of replies from the maintainer. But there were 5 messages in this > thread and only one from Thomas. > > I'd highly suggest adding one or more of those tools to your maintainer > toolbox! b4, especially, does all the work for you. I use b4, too. I wished I had more time to go over every things in my inbox. That's why I asked Roman (and other contributors) to include whatever tags were given during the review process while they submit new versions of their patches. Thanks, Wei.
On Tue, May 06, 2025 at 02:52:54AM +0000, Wei Liu wrote: > On Mon, May 05, 2025 at 11:12:10AM -0700, Dave Hansen wrote: > > On 5/5/25 11:01, Wei Liu wrote: > > > You don't need to do that for this patch. Please point me to Thomas' > > > reply to the previous version and I can add the missing tag to patch > > > while I queue it. > > > > It's right here: > > > > https://lore.kernel.org/all/8734dnouq6.ffs@tglx/ > > Thanks for the pointer. This is appreciated. Oh, it is right there in this same exact thread. I'm dumb. Wei.
On Wed, Apr 30, 2025 at 01:47:20PM -0700, 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@outlook.com> > Signed-off-by: Roman Kisel <romank@linux.microsoft.com> > Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> Queued to hyperv-next. Thanks.
> On Wed, Apr 30, 2025 at 01:47:20PM -0700, 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@outlook.com> > > Signed-off-by: Roman Kisel <romank@linux.microsoft.com> > > Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> > > Queued to hyperv-next. Thanks. We need to update numachip_wakeup_secondary as well, otherwise this will break the builds. Regards, Saurabh
On 5/6/2025 11:23 PM, Wei Liu wrote: > On Tue, May 06, 2025 at 02:58:34AM +0000, Wei Liu wrote: >> On Wed, Apr 30, 2025 at 01:47:20PM -0700, 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@outlook.com> >>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com> >>> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> >> >> Queued to hyperv-next. Thanks. > > This patch broke linux-next. I have dropped it. Please change > numachip_wakeup_secondary. > Thanks for your help! Working on that. > Wei.
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c index 82492efc5d94..8b6d310b61b9 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, unsigned 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..4d6e0e198041 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, unsigned 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..6025da891a83 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, unsigned 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..33f677e2db75 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, unsigned 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, unsigned 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, unsigned int cpu); + int (*wakeup_secondary_cpu_64)(u32 apicid, unsigned long start_eip, unsigned int cpu); }; /* diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h index 07aadf0e839f..cab952f722e4 100644 --- a/arch/x86/include/asm/mshyperv.h +++ b/arch/x86/include/asm/mshyperv.h @@ -268,11 +268,12 @@ 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, unsigned 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, + unsigned 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..f48581888d53 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, unsigned 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..58abb941c45b 100644 --- a/arch/x86/kernel/apic/apic_noop.c +++ b/arch/x86/kernel/apic/apic_noop.c @@ -27,7 +27,13 @@ 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, + unsigned 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..15209f220e1f 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, unsigned int cpu) { unsigned long val; int pnode; diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c index c10850ae6f09..b013296b100f 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, unsigned int cpu) { unsigned long send_status = 0, accept_status = 0; int num_starts, j, maxlvt; @@ -862,7 +862,7 @@ int common_cpu_up(unsigned int cpu, struct task_struct *idle) * Returns zero if startup was successfully sent, else error code from * ->wakeup_secondary_cpu. */ -static int do_boot_cpu(u32 apicid, int cpu, struct task_struct *idle) +static int do_boot_cpu(u32 apicid, unsigned int cpu, struct task_struct *idle) { unsigned long start_ip = real_mode_header->trampoline_start; int ret; @@ -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)