Re: [Minios-devel] [UNIKRAFT early RFC PATCH 09/11] plat/kvm/arm: Add simple percpu variable support

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 21/06/2019 07:57, Jia He wrote:

I think you want to explain in your commit message how the percpu is meant to 
work and what is added so far.
> Signed-off-by: Jia He <justin.he@xxxxxxx>
> ---
>   plat/common/include/smp.h |  7 +++++++
>   plat/kvm/arm/setup.c      | 22 ++++++++++++++++++++++
>   2 files changed, 29 insertions(+)
>
> diff --git a/plat/common/include/smp.h b/plat/common/include/smp.h
> index 38f801d..15bb92d 100644
> --- a/plat/common/include/smp.h
> +++ b/plat/common/include/smp.h
> @@ -34,8 +34,15 @@
>   #ifndef __PLAT_CMN_SMP_H__
>   #define __PLAT_CMN_SMP_H__
>   
> +#include <inttypes.h>
> +struct pcpu {
pcpu is not very SMP specific. You may still need it in non-SMP to avoid 
#ifdefery all over the code.
> +       uint32_t        pc_cpuid;       /* This cpu number */
> +};
> +
>   #define       MAXCPU          8 /* hard limitation for CPU number */
>   extern int cpu_possible_map[MAXCPU];
> +struct pcpu __pcpu[MAXCPU];
> +struct pcpu *pcpup = &__pcpu[0];
Hmmm, this is an header. I am surprised the compiler didn't complain about 
redefinition.
>   
>   void release_aps(void);
>   int start_cpu(uint64_t target_cpu);
> diff --git a/plat/kvm/arm/setup.c b/plat/kvm/arm/setup.c
> index 4035202..b262d94 100644
> --- a/plat/kvm/arm/setup.c
> +++ b/plat/kvm/arm/setup.c
> @@ -330,6 +330,17 @@ void init_secondary(uint64_t cpu)
>   
>   	uk_pr_info("init secondary cpu=%lu\n", cpu);
>   
> +	/*
> +        * Set the pcpu pointer with a backup in tpidr_el1 to be
> +        * loaded when entering the kernel from userland.
> +        */
I don't understand this.

> +       __asm __volatile(
> +               "mov x18, %0\n"
The compiler is free to use x18. So why setting it?


> +               "msr tpidr_el1, %0" :: "r"(pcpup));
Something does not add up here. You say per-cpu, but effectively this points to 
the beginning of the percpu. How does the CPU is supposed to know which percpu 
to use it?
Overall, I think you want to provide helper to make easier for the user to use 
percpu.
> +
> +       pcpup[cpu].pc_cpuid = cpu;
> +       uk_pr_info("pcpup[%lu]=%u\n", cpu, pcpup[cpu].pc_cpuid);
> +
>         /* Spin until the BSP releases the APs */
>         while (!aps_ready)
>                 __asm __volatile("wfe");
> @@ -408,6 +419,17 @@ void _libkvmplat_start(void *dtb_pointer)
>         uk_pr_info("Switch from bootstrap stack to stack @%p\n",
>                    _libkvmplat_stack_top);
>   
> +	/* Set the pcpu data, this is needed by pmap_bootstrap */
I can't find such function in the code.

> +       pcpup = &__pcpu[0];
> +
> +       /*
> +        * Set the pcpu pointer with a backup in tpidr_el1 to be
> +        * loaded when entering the kernel from userland.
> +        */
> +       __asm __volatile(
> +               "mov x18, %0\n"
> +               "msr tpidr_el1, %0" :: "r"(pcpup));
See all my comments above. Also, you probably want to factor the setup in a 
separate helper to avoid duplication.
> +
>         _libkvmplat_newstack((uint64_t) _libkvmplat_stack_top,
>                                 _libkvmplat_entry2, NULL);
>   }
Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel