Now that we're using hypercalls to start APs, we can replace the 'ap_cpuid'
global with a regular function parameter.  This requires telling the compiler
that we'd like the parameter in a register rather than on the stack.

While adjusting, rename to cpu_setup().  It's always been used on the BSP,
making the name ap_start() specifically misleading.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>

This is a trick I found for XTF, not that I've completed the SMP support yet.

I realise it's cheating slightly WRT 4.19, but it came out of the middle of a
series targetted for 4.19.
---
 tools/firmware/hvmloader/smp.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/tools/firmware/hvmloader/smp.c b/tools/firmware/hvmloader/smp.c
index 6ebf0b60faab..5d46eee1c5f4 100644
--- a/tools/firmware/hvmloader/smp.c
+++ b/tools/firmware/hvmloader/smp.c
@@ -29,15 +29,15 @@
 
 #include <xen/vcpu.h>
 
-static int ap_callin, ap_cpuid;
+static int ap_callin;
 
-static void ap_start(void)
+static void __attribute__((regparm(1))) cpu_setup(unsigned int cpu)
 {
-    printf(" - CPU%d ... ", ap_cpuid);
+    printf(" - CPU%d ... ", cpu);
     cacheattr_init();
     printf("done.\n");
 
-    if ( !ap_cpuid ) /* Used on the BSP too */
+    if ( !cpu ) /* Used on the BSP too */
         return;
 
     wmb();
@@ -55,7 +55,6 @@ static void boot_cpu(unsigned int cpu)
     static struct vcpu_hvm_context ap;
 
     /* Initialise shared variables. */
-    ap_cpuid = cpu;
     ap_callin = 0;
     wmb();
 
@@ -63,9 +62,11 @@ static void boot_cpu(unsigned int cpu)
     ap = (struct vcpu_hvm_context) {
         .mode = VCPU_HVM_MODE_32B,
         .cpu_regs.x86_32 = {
-            .eip = (unsigned long)ap_start,
+            .eip = (unsigned long)cpu_setup,
             .esp = (unsigned long)ap_stack + ARRAY_SIZE(ap_stack),
 
+            .eax = cpu,
+
             /* Protected Mode, no paging. */
             .cr0 = X86_CR0_PE,
 
@@ -105,7 +106,7 @@ void smp_initialise(void)
     unsigned int i, nr_cpus = hvm_info->nr_vcpus;
 
     printf("Multiprocessor initialisation:\n");
-    ap_start();
+    cpu_setup(0);
     for ( i = 1; i < nr_cpus; i++ )
         boot_cpu(i);
 }

base-commit: 53959cb8309919fc2f157a1c99e0af2ce280cb84
-- 
2.30.2

On Thu, May 09, 2024 at 06:50:57PM +0100, Andrew Cooper wrote:
> Now that we're using hypercalls to start APs, we can replace the 'ap_cpuid'
> global with a regular function parameter.  This requires telling the compiler
> that we'd like the parameter in a register rather than on the stack.
> 
> While adjusting, rename to cpu_setup().  It's always been used on the BSP,
> making the name ap_start() specifically misleading.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

On 09/05/2024 18:50, Andrew Cooper wrote:
> Now that we're using hypercalls to start APs, we can replace the 'ap_cpuid'
> global with a regular function parameter.  This requires telling the compiler
> that we'd like the parameter in a register rather than on the stack.
> 
> While adjusting, rename to cpu_setup().  It's always been used on the BSP,
> making the name ap_start() specifically misleading.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> 
> This is a trick I found for XTF, not that I've completed the SMP support yet.
> 
> I realise it's cheating slightly WRT 4.19, but it came out of the middle of a
> series targetted for 4.19.
> ---
>  tools/firmware/hvmloader/smp.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/smp.c b/tools/firmware/hvmloader/smp.c
> index 6ebf0b60faab..5d46eee1c5f4 100644
> --- a/tools/firmware/hvmloader/smp.c
> +++ b/tools/firmware/hvmloader/smp.c
> @@ -29,15 +29,15 @@
>  
>  #include <xen/vcpu.h>
>  
> -static int ap_callin, ap_cpuid;
> +static int ap_callin;
>  
> -static void ap_start(void)
> +static void __attribute__((regparm(1))) cpu_setup(unsigned int cpu)

I like it, but I'm not a fan of compiler attributes when there's sane
alternatives. We could pre-push the argument onto ap_stack to achieve
the same thing. As in, add a -4 offset to esp, and write "cpu" there.

  *(uint32_t*)ap.cpu_regs.x86_32.esp) = cpu;

That said, this is a solution as good as any other and it's definitely
better than a global, so take it or leave it.

With or without the proposed alternative...

Reviewed-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

>  {
> -    printf(" - CPU%d ... ", ap_cpuid);
> +    printf(" - CPU%d ... ", cpu);
>      cacheattr_init();
>      printf("done.\n");
>  
> -    if ( !ap_cpuid ) /* Used on the BSP too */
> +    if ( !cpu ) /* Used on the BSP too */
>          return;
>  
>      wmb();
> @@ -55,7 +55,6 @@ static void boot_cpu(unsigned int cpu)
>      static struct vcpu_hvm_context ap;
>  
>      /* Initialise shared variables. */
> -    ap_cpuid = cpu;
>      ap_callin = 0;
>      wmb();
>  
> @@ -63,9 +62,11 @@ static void boot_cpu(unsigned int cpu)
>      ap = (struct vcpu_hvm_context) {
>          .mode = VCPU_HVM_MODE_32B,
>          .cpu_regs.x86_32 = {
> -            .eip = (unsigned long)ap_start,
> +            .eip = (unsigned long)cpu_setup,
>              .esp = (unsigned long)ap_stack + ARRAY_SIZE(ap_stack),
>  
> +            .eax = cpu,
> +
>              /* Protected Mode, no paging. */
>              .cr0 = X86_CR0_PE,
>  
> @@ -105,7 +106,7 @@ void smp_initialise(void)
>      unsigned int i, nr_cpus = hvm_info->nr_vcpus;
>  
>      printf("Multiprocessor initialisation:\n");
> -    ap_start();
> +    cpu_setup(0);
>      for ( i = 1; i < nr_cpus; i++ )
>          boot_cpu(i);
>  }
> 
> base-commit: 53959cb8309919fc2f157a1c99e0af2ce280cb84