Re: [PATCH v2 gnumach] percpu area using gs segment

[Samuel Thibault]

Hello,

Thanks for this! Just a few comments.

Damien Zammit, le dim. 17 sept. 2023 01:15:14 +0000, a ecrit:
> +#define SMP_COMPLETE (-1)
> +#define MY(stm)              %gs:PERCPU_##stm
> +
>  #if  NCPUS > 1
>  
>  #ifdef __i386__
> @@ -63,14 +66,27 @@
>       movl    %esi, reg       ;\
>       popl    %esi            ;\
>  
> +/* Never call CPU_NUMBER_GS(%esi) */
> +#define CPU_NUMBER_GS(reg)   \
> +     movl    %cs:bspdone, reg        ;\
> +     cmpl    $SMP_COMPLETE, reg      ;\
> +     je      8f              ;\
> +     CPU_NUMBER(reg)         ;\
> +     jmp     9f              ;\

Is the CPU_NUMBER assembly macro really possibly used before we set up
segments in gdt_init?

> diff --git a/i386/i386/cpuboot.S b/i386/i386/cpuboot.S
> index 4a5823be..7d1e815c 100644
> --- a/i386/i386/cpuboot.S
> +++ b/i386/i386/cpuboot.S
> @@ -119,7 +119,7 @@ _apboot:
>       wrmsr
>  
>       /* Load int_stack_top[cpu] -> esp */
> -     CPU_NUMBER(%edx)
> +     CPU_NUMBER_NO_STACK(%edx)
>       movl    CX(EXT(int_stack_top), %edx), %esp
>  
>       /* Ensure stack alignment */

This deserves a separate commit.

> diff --git a/i386/i386/gdt.c b/i386/i386/gdt.c
> index ddda603b..e335de50 100644
> --- a/i386/i386/gdt.c
> +++ b/i386/i386/gdt.c
> @@ -73,6 +74,11 @@ gdt_fill(struct real_descriptor *mygdt)
>                           0xffffffff,
>                           ACC_PL_K|ACC_DATA_W, SZ_32);
>  #endif       /* MACH_PV_DESCRIPTORS */
> +     vm_offset_t thiscpu = kvtolin(&percpu_array[cpu_number_slow()]);

Rather just get the cpu number through an additional parameter
for gdt_fill, we do have it at hand in gdt_init (it's 0), and in
ap_gdt_init.

> diff --git a/i386/i386/i386asm.sym b/i386/i386/i386asm.sym
> index 436e296a..620e8f37 100644
> --- a/i386/i386/i386asm.sym
> +++ b/i386/i386/i386asm.sym
> @@ -154,17 +156,10 @@ expr    NPTES                                           
> PTES_PER_PAGE
>  expr INTEL_PTE_VALID|INTEL_PTE_WRITE                 INTEL_PTE_KERNEL
>  
>  expr IDTSZ
> -expr GDTSZ
> -expr LDTSZ
>  
>  expr KERNEL_RING
> -
>  expr KERNEL_CS
>  expr KERNEL_DS
> -expr KERNEL_TSS
> -#ifndef      MACH_PV_DESCRIPTORS
> -expr KERNEL_LDT
> -#endif       /* MACH_PV_DESCRIPTORS */

> diff --git a/i386/i386/locore.S b/i386/i386/locore.S
> index 55aa9d60..ff59615b 100644
> --- a/i386/i386/locore.S
> +++ b/i386/i386/locore.S
> @@ -33,6 +33,7 @@
>  #include <i386/proc_reg.h>
>  #include <i386/trap.h>
>  #include <i386/seg.h>
> +#include <i386/gdt.h>

Rather make these a separate commit.

> @@ -479,7 +481,7 @@ trap_set_segs:
>       jz      trap_from_kernel        /* kernel trap if not */
>  trap_from_user:
>  
> -     CPU_NUMBER(%edx)
> +     CPU_NUMBER_GS(%edx)

Can't we make almost all CPU_NUMBER calls use gs? Having gs set up is
the normal situation, so better make CPU_NUMBER use gs, and have a
CPU_NUMBER_NOGS for the few situations where gs is not set up.

> diff --git a/i386/i386/percpu.c b/i386/i386/percpu.c
> new file mode 100644
> index 00000000..b2b8afa7
> --- /dev/null
> +++ b/i386/i386/percpu.c
> +#include <i386/smp.h>
> +#include <i386/apic.h>
> +#include <i386/percpu.h>
> +
> +struct percpu percpu_array[NCPUS] __aligned(0x8000) = {0};

Why aligning?

> +#define percpu_assign(stm, val)     \
> +    asm("mov %[src], %%gs:%c[offs]" \
> +         : /* No outputs */         \
> +         : [src] "r" (val), [offs] "e" (__builtin_offsetof(struct percpu, 
> stm)) \
> +         : );
> +
> +#define percpu_ptr(typ, stm)        \
> +MACRO_BEGIN                         \
> +    typ *ptr_ = (typ *)__builtin_offsetof(struct percpu, stm); \
> +                                    \
> +    asm("add %%gs:0, %[pointer]"    \
> +         : [pointer] "+r" (ptr_)    \
> +         : /* No inputs */          \
> +         : );                       \
> +                                    \
> +    ptr_;                           \
> +MACRO_END

Also introduce a percpu_get() that reads through %%gs:%c[offs], that
can be used in cpu_number, which will be more efficient than reading
%%gs:0 first. So that percpu_ptr will only be called when we really need
a pointer.

BTW, the cpu_number() functions really deserve being rather made
inlines.

Samuel