Re: [PATCH 4.5/8] tools/hvmloader: Further simplify SMP setup
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 Reviewed-by: Roger Pau Monné Thanks, Roger.
Re: [PATCH 4.5/8] tools/hvmloader: Further simplify SMP setup
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
> ---
> CC: Jan Beulich
> CC: Roger Pau Monné
> CC: Alejandro Vallejo
>
> 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
>
> -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
> {
> -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
[PATCH 4.5/8] tools/hvmloader: Further simplify SMP setup
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
---
CC: Jan Beulich
CC: Roger Pau Monné
CC: Alejandro Vallejo
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
-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
