Re: [PATCH 08/12] i386: Add AP variants of descriptor tables

[Samuel Thibault]

Hello,

Damien Zammit, le mar. 25 oct. 2022 10:56:13 +0000, a ecrit:
> diff --git a/i386/i386/gdt.c b/i386/i386/gdt.c
> index fb18360e..44bcd29c 100644
> --- a/i386/i386/gdt.c
> +++ b/i386/i386/gdt.c
> @@ -128,3 +135,39 @@ gdt_init(void)
>  #endif       /* MACH_PV_PAGETABLES */
>  }
> 
> +#if NCPUS > 1
> +void
> +ap_gdt_init(int cpu)
> +{
> +     gdt_fill(mp_gdt[cpu]);
> +
> +#ifndef      MACH_PV_DESCRIPTORS
> +     /* Load the new GDT.  */
> +     {
> +             struct pseudo_descriptor pdesc;
> +
> +             pdesc.limit = sizeof(gdt)-1;
> +             pdesc.linear_base = kvtolin(mp_gdt[cpu]);
> +             lgdt(&pdesc);
> +     }
> +#endif       /* MACH_PV_DESCRIPTORS */
> +
> +     /* Reload all the segment registers from the new GDT.
> +        We must load ds and es with 0 before loading them with KERNEL_DS
> +        because some processors will "optimize out" the loads
> +        if the previous selector values happen to be the same.  */
> +#ifndef __x86_64__
> +     asm volatile("ljmp      %0,$1f\n"
> +                  "1:\n"
> +                  "movw      %w2,%%ds\n"
> +                  "movw      %w2,%%es\n"
> +                  "movw      %w2,%%fs\n"
> +                  "movw      %w2,%%gs\n"
> +
> +                  "movw      %w1,%%ds\n"
> +                  "movw      %w1,%%es\n"
> +                  "movw      %w1,%%ss\n"
> +                  : : "i" (KERNEL_CS), "r" (KERNEL_DS), "r" (0));
> +#endif

Please factorize this part, it's exactly the same, except gdt vs
mp_gdt[cpu]. Otherwise people will wonder whether there is to be a
difference There is just one trick: sizeof(*(mp_gdt[cpu])) won't
work, but since sizeof(gdt) would assume that ap gdt has the same
size as bsp gdt, better make the size explicit by using sizeof(struct
real_descriptor) * GDTSZ.

> diff --git a/i386/i386/idt-gen.h b/i386/i386/idt-gen.h
> index f86afb41..a94b39c0 100644
> --- a/i386/i386/idt-gen.h
> +++ b/i386/i386/idt-gen.h
> @@ -44,4 +44,8 @@ extern struct real_gate idt[IDTSZ];
>  #define fill_idt_gate(int_num, entry, selector, access, dword_count) \
>       fill_gate(&idt[int_num], entry, selector, access, dword_count)
> 
> +/* Fill a gate in a custom IDT.  */
> +#define _fill_idt_gate(_idt, int_num, entry, selector, access, dword_count) \
> +     fill_gate(&_idt[int_num], entry, selector, access, dword_count)
> +
>  #endif /* _I386_IDT_ */

Rather make fill_idt_gate explicitly take the idt.

> diff --git a/i386/i386/idt.c b/i386/i386/idt.c
> index c6a778f1..8513e158 100644
> --- a/i386/i386/idt.c
> +++ b/i386/i386/idt.c
> @@ -36,7 +37,7 @@ struct idt_init_entry
>  };
>  extern struct idt_init_entry idt_inittab[];
> 
> -void idt_init(void)
> +static void idt_fill(void)

I don't see idt_fill used separately from idt_init?

That said, here again idt_init and ap_idt_init are almost exactly the
same except idt vs mp_desc_table[cpu]->idt, so please factorize (which
will be trivial with fill_idt_gate taking the idt).

> diff --git a/i386/i386/ktss.c b/i386/i386/ktss.c
> index 0d21d3eb..33b084b7 100644
> --- a/i386/i386/ktss.c
> +++ b/i386/i386/ktss.c
> @@ -35,6 +35,7 @@
>  #include "seg.h"
>  #include "gdt.h"
>  #include "ktss.h"
> +#include "mp_desc.h"
> 
>  /* A kernel TSS with a complete I/O bitmap.  */
>  struct task_tss ktss;
> @@ -73,3 +74,38 @@ ktss_init(void)
>  #endif       /* MACH_RING1 */
>  }
> 
> +#if NCPUS > 1
> +void
> +ap_ktss_init(int cpu)

Again, it's basically exactly the same except gdt vs mp_gdt[cpu] and
&ktss vs &mp_desc_table[cpu]->ktss, please factorize.

> diff --git a/i386/i386/ldt.h b/i386/i386/ldt.h
> index 1f0d7014..55edc396 100644
> --- a/i386/i386/ldt.h
> +++ b/i386/i386/ldt.h
> @@ -64,7 +64,16 @@ extern struct real_descriptor ldt[LDTSZ];
>       fill_gate((struct real_gate*)&ldt[sel_idx(selector)], \
>                 offset, dest_selector, access, word_count)
> 
> +/* Fill a 32bit segment descriptor in a custom LDT.  */
> +#define _fill_ldt_descriptor(_ldt, selector, base, limit, access, sizebits) \
> +     fill_descriptor(&_ldt[sel_idx(selector)], base, limit, access, sizebits)
> +
> +#define _fill_ldt_gate(_ldt, selector, offset, dest_selector, access, 
> word_count) \
> +     fill_gate((struct real_gate*)&_ldt[sel_idx(selector)], \
> +               offset, dest_selector, access, word_count)

Also here, just change fill_ldt_descriptor and fill_ldt_gate to take the
ldt:

> diff --git a/i386/i386/ldt.c b/i386/i386/ldt.c
> index 261df93a..4bbc8e80 100644
> --- a/i386/i386/ldt.c
> +++ b/i386/i386/ldt.c
> @@ -37,6 +37,7 @@
>  #include "gdt.h"
>  #include "ldt.h"
>  #include "locore.h"
> +#include "mp_desc.h"
> 
>  #ifdef       MACH_PV_DESCRIPTORS
>  /* It is actually defined in xen_boothdr.S */
> @@ -79,3 +80,41 @@ ldt_init(void)
>       lldt(KERNEL_LDT);
>  #endif       /* MACH_PV_DESCRIPTORS */
>  }
> +
> +#if NCPUS > 1
> +void
> +ap_ldt_init(int cpu)

Again :)

> diff --git a/i386/i386/locore.S b/i386/i386/locore.S
> index 162bb13a..8c2f57ea 100644
> --- a/i386/i386/locore.S
> +++ b/i386/i386/locore.S
> @@ -649,6 +649,10 @@ INTERRUPT(20)
>  INTERRUPT(21)
>  INTERRUPT(22)
>  INTERRUPT(23)
> +#endif
> +/* Invalidate TLB IPI to call pmap_update_interrupt() on a specific cpu */
> +INTERRUPT(251)

Better make it use the CALL_SINGLE_FUNCTION_BASE macro. You can use
i386/i386/i386asm.sym to get it.

> diff --git a/i386/i386at/int_init.c b/i386/i386at/int_init.c
> index 6da627dd..6c242557 100644
> --- a/i386/i386at/int_init.c
> +++ b/i386/i386at/int_init.c
> @@ -23,6 +23,7 @@
> 
>  #include <i386at/idt.h>
>  #include <i386/gdt.h>
> +#include <i386/mp_desc.h>
> 
>  /* defined in locore.S */
>  extern vm_offset_t int_entry_table[];
> @@ -36,15 +37,38 @@ void int_init(void)
>                             int_entry_table[i], KERNEL_CS,
>                             ACC_PL_K|ACC_INTR_GATE, 0);
>       }
> +     fill_idt_gate(CALL_SINGLE_FUNCTION_BASE,
> +                           int_entry_table[16], KERNEL_CS,
> +                           ACC_PL_K|ACC_INTR_GATE, 0);
>  #else
>       for (i = 0; i < 24; i++) {
>               fill_idt_gate(IOAPIC_INT_BASE + i,
>                             int_entry_table[i], KERNEL_CS,
>                             ACC_PL_K|ACC_INTR_GATE, 0);
>       }
> -     fill_idt_gate(IOAPIC_SPURIOUS_BASE,
> +     fill_idt_gate(CALL_SINGLE_FUNCTION_BASE,
>                             int_entry_table[24], KERNEL_CS,
>                             ACC_PL_K|ACC_INTR_GATE, 0);
> +     fill_idt_gate(IOAPIC_SPURIOUS_BASE,
> +                           int_entry_table[25], KERNEL_CS,
> +                           ACC_PL_K|ACC_INTR_GATE, 0);
>  #endif

I'd rather see it factorized, with

#ifndef APIC
        int cur = PIC_INT_BASE;
        int nirq = 16;
#else
        int cur = IOAPIC_INT_BASE;
        int nirq = 24;
#endif

and then use cur and cur++ in the loop, and simply put the
IOAPIC_SPURIOUS_BASE addition in #ifdef APIC.

>  }
> 
> +#if NCPUS > 1
> +void ap_int_init(int cpu)
> +{

Why not setting gates for the hardware IRQs? At some point we'll
probably want to distribute IRQs over cpus. And then we can again just
factorize both functions.

> +#ifndef APIC
> +     _fill_idt_gate(mp_desc_table[cpu]->idt, CALL_SINGLE_FUNCTION_BASE,
> +                           int_entry_table[16], KERNEL_CS,
> +                           ACC_PL_K|ACC_INTR_GATE, 0);
> +#else
> +     _fill_idt_gate(mp_desc_table[cpu]->idt, CALL_SINGLE_FUNCTION_BASE,
> +                           int_entry_table[24], KERNEL_CS,
> +                           ACC_PL_K|ACC_INTR_GATE, 0);
> +     _fill_idt_gate(mp_desc_table[cpu]->idt, IOAPIC_SPURIOUS_BASE,
> +                           int_entry_table[25], KERNEL_CS,
> +                           ACC_PL_K|ACC_INTR_GATE, 0);
> +#endif
> +}
> +#endif

> diff --git a/i386/i386at/interrupt.S b/i386/i386at/interrupt.S
> index 8fd18392..a5aac966 100644
> --- a/i386/i386at/interrupt.S
> +++ b/i386/i386at/interrupt.S
> @@ -41,6 +41,9 @@ ENTRY(interrupt)
>       cmpl    $255,%eax               /* was this a spurious intr? */
>       je      _no_eoi                 /* if so, just return */
>  #endif
> +     cmpl    $251,%eax               /* was this a SMP call single function 
> request? */
> +     je      _call_single
> +
>       subl    $28,%esp                /* Two local variables + 5 parameters */
>       movl    %eax,S_IRQ              /* save irq number */
>       call    spl7                    /* set ipl */
> @@ -118,4 +121,7 @@ _isa_eoi:
>       addl    $28,%esp                /* pop local variables */
>  _no_eoi:
>       ret
> +_call_single:
> +     call    EXT(pmap_update_interrupt) /* TODO: Allow other functions */

I believe you still want to call pmap_update_interrupt under spl7, so
add calling spl7 and splx_cli. Do we not need an eoi notification?

> +     ret
>  END(interrupt)
> --
> 2.34.1