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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <SN6PR02MB415780229941BD1668547945D4832@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Wed, 30 Apr 2025 22:39:38 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Roman Kisel <romank@...ux.microsoft.com>, "ardb@...nel.org"
	<ardb@...nel.org>, "bp@...en8.de" <bp@...en8.de>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"decui@...rosoft.com" <decui@...rosoft.com>, "dimitri.sivanich@....com"
	<dimitri.sivanich@....com>, "haiyangz@...rosoft.com"
	<haiyangz@...rosoft.com>, "hpa@...or.com" <hpa@...or.com>,
	"imran.f.khan@...cle.com" <imran.f.khan@...cle.com>,
	"jacob.jun.pan@...ux.intel.com" <jacob.jun.pan@...ux.intel.com>,
	"jgross@...e.com" <jgross@...e.com>, "justin.ernst@....com"
	<justin.ernst@....com>, "kprateek.nayak@....com" <kprateek.nayak@....com>,
	"kyle.meyer@....com" <kyle.meyer@....com>, "kys@...rosoft.com"
	<kys@...rosoft.com>, "lenb@...nel.org" <lenb@...nel.org>, "mingo@...hat.com"
	<mingo@...hat.com>, "nikunj@....com" <nikunj@....com>, "papaluri@....com"
	<papaluri@....com>, "perry.yuan@....com" <perry.yuan@....com>,
	"peterz@...radead.org" <peterz@...radead.org>, "rafael@...nel.org"
	<rafael@...nel.org>, "russ.anderson@....com" <russ.anderson@....com>,
	"steve.wahl@....com" <steve.wahl@....com>, "tglx@...utronix.de"
	<tglx@...utronix.de>, "thomas.lendacky@....com" <thomas.lendacky@....com>,
	"tim.c.chen@...ux.intel.com" <tim.c.chen@...ux.intel.com>,
	"tony.luck@...el.com" <tony.luck@...el.com>, "wei.liu@...nel.org"
	<wei.liu@...nel.org>, "xin@...or.com" <xin@...or.com>,
	"yuehaibing@...wei.com" <yuehaibing@...wei.com>, "linux-acpi@...r.kernel.org"
	<linux-acpi@...r.kernel.org>, "linux-hyperv@...r.kernel.org"
	<linux-hyperv@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>
CC: "apais@...rosoft.com" <apais@...rosoft.com>, "benhill@...rosoft.com"
	<benhill@...rosoft.com>, "bperkins@...rosoft.com" <bperkins@...rosoft.com>,
	"sunilmut@...rosoft.com" <sunilmut@...rosoft.com>
Subject: RE: [PATCH hyperv-next v3] arch/x86: Provide the CPU number in the
 wakeup AP callback

From: Roman Kisel <romank@...ux.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@...look.com>
> Signed-off-by: Roman Kisel <romank@...ux.microsoft.com>
> Reviewed-by: Tom Lendacky <thomas.lendacky@....com>

LGTM.

Reviewed-by: Michael Kelley <mhklinux@...look.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
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ