Re: [PATCH 1/4] i386: Add AP variants of descriptor tables

[Samuel Thibault]

Hello,

We're converging :)

Samuel

Damien Zammit, le ven. 11 nov. 2022 23:21:15 +0000, a ecrit:
> -void idt_init(void)
> +static void
> +idt_fill(struct real_gate *myidt)
>  {
>  #ifdef       MACH_PV_DESCRIPTORS
>       if (hyp_set_trap_table(kvtolin(idt_inittab)))
> @@ -47,18 +49,29 @@ void idt_init(void)
>       /* Initialize the exception vectors from the idt_inittab.  */
>       while (iie->entrypoint)
>       {
> -             fill_idt_gate(iie->vector, iie->entrypoint, KERNEL_CS, 
> iie->type, 0);
> +             fill_idt_gate(myidt, iie->vector, iie->entrypoint, KERNEL_CS, 
> iie->type, 0);
>               iie++;
>       }
> +#endif       /* MACH_PV_DESCRIPTORS */

? No, keep #endif where it was. In the PV case there is just one idt,
set with hyp_set_trap_table, and we don't use lidt.

> 
> -     /* Load the IDT pointer into the processor.  */
> +     /* Load the IDT pointer into the processor */

Leave as it is: English wants a period, and says there are two spaces
after the period :)

> diff --git a/i386/i386/ktss.c b/i386/i386/ktss.c
> index 0d21d3eb..5cc260fc 100644
> --- a/i386/i386/ktss.c
> +++ b/i386/i386/ktss.c
> @@ -35,12 +35,13 @@
>  #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;
> 
>  void
> -ktss_init(void)
> +ktss_fill(struct task_tss *myktss, struct real_descriptor *mygdt)
>  {
>       /* XXX temporary exception stack */
>       static int exception_stack[1024];

Mmm, will that not be a problem to use the same exception stack for all
processors? I guess a [NCPUS] could be needed here.

>       /* Initialize the master TSS.  */
>  #ifdef __x86_64__
> -     ktss.tss.rsp0 = (unsigned long)(exception_stack+1024);
> +     myktss->tss.rsp0 = (unsigned long)(exception_stack+1024);
>  #else /* ! __x86_64__ */
> -     ktss.tss.ss0 = KERNEL_DS;
> -     ktss.tss.esp0 = (unsigned long)(exception_stack+1024);
> +     myktss->tss.ss0 = KERNEL_DS;
> +     myktss->tss.esp0 = (unsigned long)(exception_stack+1024);
>  #endif /* __x86_64__ */
> 

> diff --git a/i386/i386at/int_init.c b/i386/i386at/int_init.c
> index 6da627dd..da552106 100644
> --- a/i386/i386at/int_init.c
> +++ b/i386/i386at/int_init.c
> @@ -22,29 +22,48 @@
> +     fill_idt_gate(myidt, CALL_SINGLE_FUNCTION_BASE,
>                             int_entry_table[i], KERNEL_CS,
>                             ACC_PL_K|ACC_INTR_GATE, 0);

Use i++, so that

> +#ifdef APIC
> +     fill_idt_gate(myidt, IOAPIC_SPURIOUS_BASE,
> +                           int_entry_table[25], KERNEL_CS,
>                             ACC_PL_K|ACC_INTR_GATE, 0);
>  #endif

Here you won't hardcode a magic 25.

> diff --git a/i386/i386at/interrupt.S b/i386/i386at/interrupt.S
> index 7480fba9..9eccf022 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? */

Use CALL_SINGLE_FUNCTION_BASE here as well.

> +     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,17 @@ _isa_eoi:
>       addl    $28,%esp                /* pop local variables */
>  _no_eoi:
>       ret

Add a newline here, to separate this new path.

> +_call_single:
> +     subl    $8,%esp                 /* One param */
> +     movl    %eax,S_ARG0             /* save irq number */

No, S_ARG0 is valid only inside the callee. #define some new S2_IRQ and
S2_IPL macros to 0(%esp) and 4(%esp).

> +     call    spl7                    /* set ipl */
> +     movl    %eax,S_ARG1             /* save previous ipl */
> +     call    EXT(pmap_update_interrupt) /* TODO: Allow other functions */
> +     movl    S_ARG1,%eax             /* restore previous ipl */
> +     call    splx_cli                /* restore previous ipl */
> +     cli
> +     movl    S_ARG0,%eax             /* restore irq number */

The irq number doesn't seem to be used?

> +     call    EXT(lapic_eoi)          /* lapic EOI */
> +     addl    $8,%esp
> +     ret
>  END(interrupt)

While at it, it would be useful to update ./x86_64/interrupt.S and
x86_64/locore.S too, so they remain coherent.

Samuel