Re: [PATCH 4.5/8] tools/hvmloader: Further simplify SMP setup

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> 
> 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@xxxxxxxxx>

>  {
> -    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