[PATCH] x86/hyperv: Use per cpu initial stack for vtl context

[PATCH] x86/hyperv: Use per cpu initial stack for vtl context

[Saurabh Sengar]

Currently, the secondary vCPUs in Hyper-V VTL context lack support for
parallel startup. Therefore, relying on the single initial_stack fetched
from the current task structure suffices for all vCPUs.

However, common initial_stack risks stack corruption when parallel startup
is enabled. In order to facilitate parallel startup, use the initial_stack
from the per CPU idle thread instead of the current task.

Fixes: 18415f33e2ac ("cpu/hotplug: Allow "parallel" bringup up to CPUHP_BP_KICK_AP_STATE")
Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
 arch/x86/hyperv/hv_vtl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
index 96e6c51515f5..a54b46b673de 100644
--- a/arch/x86/hyperv/hv_vtl.c
+++ b/arch/x86/hyperv/hv_vtl.c
@@ -12,6 +12,7 @@
 #include <asm/i8259.h>
 #include <asm/mshyperv.h>
 #include <asm/realmode.h>
+#include <../kernel/smpboot.h>
 
 extern struct boot_params boot_params;
 static struct real_mode_header hv_vtl_real_mode_header;
@@ -71,7 +72,8 @@ static int hv_vtl_bringup_vcpu(u32 target_vp_index, u64 eip_ignored)
 	struct ldttss_desc *ldt;
 	struct desc_struct *gdt;
 
-	u64 rsp = current->thread.sp;
+	struct task_struct *idle = idle_thread_get(target_vp_index);
+	u64 rsp = (unsigned long)idle->thread.sp;
 	u64 rip = (u64)&hv_vtl_ap_entry;
 
 	native_store_gdt(&gdt_ptr);
-- 
2.34.1

RE: [PATCH] x86/hyperv: Use per cpu initial stack for vtl context

[Michael Kelley]

From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Thursday, February 1, 2024 10:26 AM
> 
> Currently, the secondary vCPUs in Hyper-V VTL context lack support for
> parallel startup. Therefore, relying on the single initial_stack fetched
> from the current task structure suffices for all vCPUs.
> 
> However, common initial_stack risks stack corruption when parallel startup
> is enabled. In order to facilitate parallel startup, use the initial_stack
> from the per CPU idle thread instead of the current task.
> 
> Fixes: 18415f33e2ac ("cpu/hotplug: Allow "parallel" bringup up to CPUHP_BP_KICK_AP_STATE")
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  arch/x86/hyperv/hv_vtl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> index 96e6c51515f5..a54b46b673de 100644
> --- a/arch/x86/hyperv/hv_vtl.c
> +++ b/arch/x86/hyperv/hv_vtl.c
> @@ -12,6 +12,7 @@
>  #include <asm/i8259.h>
>  #include <asm/mshyperv.h>
>  #include <asm/realmode.h>
> +#include <../kernel/smpboot.h>
> 
>  extern struct boot_params boot_params;
>  static struct real_mode_header hv_vtl_real_mode_header;
> @@ -71,7 +72,8 @@ static int hv_vtl_bringup_vcpu(u32 target_vp_index, u64 eip_ignored)
>  	struct ldttss_desc *ldt;
>  	struct desc_struct *gdt;
> 
> -	u64 rsp = current->thread.sp;
> +	struct task_struct *idle = idle_thread_get(target_vp_index);

The "VP index" of a vCPU is a Hyper-V concept, and may not match
the Linux concept of a CPU number.   In most cases, they *do* match,
so your testing of this patch probably worked.  But there's no guarantee
that they match.  The Hyper-V TLFS does not even guarantee that VP
indices are dense or that they start with 0 (even though they do in
current versions of Hyper-V).

As a different kind of example, in a kdump kernel, Linux labels the
booting CPU as CPU 0, but it may not be the 0th CPU in the guest
VM, and hence may not have VP index of 0.  Of course, in a kdump
kernel nr_cpus is typically 1, so you aren't bringing up secondary
CPUs.  But sometimes kdump kernels boot with nr_cpus=2 or greater,
in which case the mismatch would occur.

This conceptual difference in VP index and Linux CPU numbers is why
the hv_vp_index array exists -- to map from a Linux CPU number to a
Hyper-V VP index, and thereby avoid assuming they are equal.

So before hv_vtl_wakeup_secondary_cpu() calls this function, it needs
to separately map the apicid to a Linux CPU number, which can then
be passed to idle_thread_get().

Michael

> +	u64 rsp = (unsigned long)idle->thread.sp;
>  	u64 rip = (u64)&hv_vtl_ap_entry;
> 
>  	native_store_gdt(&gdt_ptr);
> --
> 2.34.1
> 

Re: [PATCH] x86/hyperv: Use per cpu initial stack for vtl context

[Saurabh Singh Sengar]

On Thu, Feb 01, 2024 at 07:53:59PM +0000, Michael Kelley wrote:
> From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Thursday, February 1, 2024 10:26 AM
> > 
> > Currently, the secondary vCPUs in Hyper-V VTL context lack support for
> > parallel startup. Therefore, relying on the single initial_stack fetched
> > from the current task structure suffices for all vCPUs.
> > 
> > However, common initial_stack risks stack corruption when parallel startup
> > is enabled. In order to facilitate parallel startup, use the initial_stack
> > from the per CPU idle thread instead of the current task.
> > 
> > Fixes: 18415f33e2ac ("cpu/hotplug: Allow "parallel" bringup up to CPUHP_BP_KICK_AP_STATE")
> > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > ---
> >  arch/x86/hyperv/hv_vtl.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> > index 96e6c51515f5..a54b46b673de 100644
> > --- a/arch/x86/hyperv/hv_vtl.c
> > +++ b/arch/x86/hyperv/hv_vtl.c
> > @@ -12,6 +12,7 @@
> >  #include <asm/i8259.h>
> >  #include <asm/mshyperv.h>
> >  #include <asm/realmode.h>
> > +#include <../kernel/smpboot.h>
> > 
> >  extern struct boot_params boot_params;
> >  static struct real_mode_header hv_vtl_real_mode_header;
> > @@ -71,7 +72,8 @@ static int hv_vtl_bringup_vcpu(u32 target_vp_index, u64 eip_ignored)
> >  	struct ldttss_desc *ldt;
> >  	struct desc_struct *gdt;
> > 
> > -	u64 rsp = current->thread.sp;
> > +	struct task_struct *idle = idle_thread_get(target_vp_index);
> 
> The "VP index" of a vCPU is a Hyper-V concept, and may not match
> the Linux concept of a CPU number.   In most cases, they *do* match,
> so your testing of this patch probably worked.  But there's no guarantee
> that they match.  The Hyper-V TLFS does not even guarantee that VP
> indices are dense or that they start with 0 (even though they do in
> current versions of Hyper-V).
> 
> As a different kind of example, in a kdump kernel, Linux labels the
> booting CPU as CPU 0, but it may not be the 0th CPU in the guest
> VM, and hence may not have VP index of 0.  Of course, in a kdump
> kernel nr_cpus is typically 1, so you aren't bringing up secondary
> CPUs.  But sometimes kdump kernels boot with nr_cpus=2 or greater,
> in which case the mismatch would occur.
> 
> This conceptual difference in VP index and Linux CPU numbers is why
> the hv_vp_index array exists -- to map from a Linux CPU number to a
> Hyper-V VP index, and thereby avoid assuming they are equal.
> 
> So before hv_vtl_wakeup_secondary_cpu() calls this function, it needs
> to separately map the apicid to a Linux CPU number, which can then
> be passed to idle_thread_get().
> 
> Michael

Thanks for the review. I will fix this in V2.

> 
> > +	u64 rsp = (unsigned long)idle->thread.sp;
> >  	u64 rip = (u64)&hv_vtl_ap_entry;
> > 
> >  	native_store_gdt(&gdt_ptr);
> > --
> > 2.34.1
> > 
>