| Message ID | 20240509175057.1921538-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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
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.
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); }
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(-) base-commit: 53959cb8309919fc2f157a1c99e0af2ce280cb84