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
