Thanks for looking at it, we definitely need to fix this before enabling
SMP :D
Damien Zammit, le mar. 25 oct. 2022 10:56:32 +0000, a ecrit:
> ---
> i386/i386/io_perm.c | 5 ---
> i386/i386/pcb.c | 79 ++++++++++++++++++++++++---------------------
> 2 files changed, 43 insertions(+), 41 deletions(-)
>
> diff --git a/i386/i386/io_perm.c b/i386/i386/io_perm.c
> index 6db60f73..d9c646fe 100644
> --- a/i386/i386/io_perm.c
> +++ b/i386/i386/io_perm.c
> @@ -266,9 +266,7 @@ i386_io_perm_modify (task_t target_task, io_perm_t
> io_perm, boolean_t enable)
>
> if (!iopb)
> {
> - simple_unlock (&target_task->machine.iopb_lock);
> iopb = (unsigned char *) kmem_cache_alloc (&machine_task_iopb_cache);
> - simple_lock (&target_task->machine.iopb_lock);
? why ?
kmem_cache_alloc can be a long operation, better not keep a simple lock
during it. The safety is properly provided with the tests below with the
simple lock reacquired: if in the end an iopb was allocated by another
thread in the meanwhile, it's used and the just-allocated one freed.
> @@ -308,9 +306,6 @@ i386_io_perm_modify (task_t target_task, io_perm_t
> io_perm, boolean_t enable)
> target_task->machine.iopb_size = iopb_size;
> }
>
> -#if NCPUS>1
> -#warning SMP support missing (notify all CPUs running threads in that of the
> I/O bitmap change).
> -#endif
> if (target_task == current_task())
> update_ktss_iopb (iopb, target_task->machine.iopb_size);
>
> diff --git a/i386/i386/pcb.c b/i386/i386/pcb.c
> index 5ac487b7..4ff7309c 100644
> --- a/i386/i386/pcb.c
> +++ b/i386/i386/pcb.c
> @@ -48,6 +48,7 @@
> #include <i386/user_ldt.h>
> #include <i386/db_interface.h>
> #include <i386/fpu.h>
> +#include <i386/spl.h>
> #include "eflags.h"
> #include "gdt.h"
> #include "ldt.h"
> @@ -122,8 +123,8 @@ vm_offset_t stack_detach(thread_t thread)
> #define curr_gdt(mycpu) (mp_gdt[mycpu])
> #define curr_ktss(mycpu) (mp_ktss[mycpu])
> #else
> -#define curr_gdt(mycpu) ((void)(mycpu), gdt)
> -#define curr_ktss(mycpu) ((void)(mycpu), (struct task_tss
> *)&ktss)
> +#define curr_gdt(mycpu) (gdt)
> +#define curr_ktss(mycpu) ((struct task_tss *)&ktss)
Why? We don't want warnings about the mycpu variable being unused when
building with NCPUS == 1.
> #endif
>
> #define gdt_desc_p(mycpu,sel) \
> @@ -131,6 +132,8 @@ vm_offset_t stack_detach(thread_t thread)
>
> void switch_ktss(pcb_t pcb)
> {
> + spl_t s = splhigh();
What scenario does this protect against?
> int mycpu = cpu_number();
> {
> vm_offset_t pcb_stack_top;
> @@ -222,21 +225,20 @@ void switch_ktss(pcb_t pcb)
> pcb->ims.user_gdt, sizeof pcb->ims.user_gdt);
> #endif /* MACH_PV_DESCRIPTORS */
>
> - db_load_context(pcb);
> + db_set_debug_state(pcb, &pcb->ims.ids)
Why? We do want to load the dr registers.
>
> /*
> * Load the floating-point context, if necessary.
> */
> fpu_load_context(pcb);
>
> + splx(s);
> }
>
> -/* If NEW_IOPB is not null, the SIZE denotes the number of bytes in
> - the new bitmap. Expects iopb_lock to be held. */
> -void
> -update_ktss_iopb (unsigned char *new_iopb, io_port_t size)
> +static void
> +update_ktss_iopb_per_cpu (unsigned char *new_iopb, io_port_t size, int cpu)
Better keep the comment on this function too.
> {
> - struct task_tss *tss = curr_ktss (cpu_number ());
> + struct task_tss *tss = curr_ktss (cpu);
>
> if (new_iopb && size > 0)
> {
> @@ -249,6 +251,24 @@ update_ktss_iopb (unsigned char *new_iopb, io_port_t
> size)
> tss->tss.io_bit_map_offset = IOPB_INVAL;
> }
>
> +/* If NEW_IOPB is not null, the SIZE denotes the number of bytes in
> + the new bitmap. Expects iopb_lock to be held per TASK. */
> +void
> +update_ktss_iopb (unsigned char *new_iopb, io_port_t size)
> +{
> +#if NCPUS > 1
> + int slot;
> +
> + for (slot = 0; slot < NCPUS; slot++)
> + {
> + if (machine_slot[slot].is_cpu)
> + update_ktss_iopb_per_cpu (new_iopb, size, slot);
There is a misunderstanding here.
There are two kinds of update_ktss_iopb calls:
- calls from stack_handoff and switch_context only matter for the
current CPU: they just update the ioperm bitmap.
- calls from i386_io_perm_modify matter for all cpus which are running
the calling task.
So these two cases should be split: we don't want to change ioperms on
all cpus just because one cpu made a thread context switch.
Also, we do not want to change ioperms on all cpus, only on cpus that
happen to be running a thread of the calling task.
(and of course we have to protect both from a cpu that happens to be
switching threads, and another thread that happens to be calling
i386_io_perm_modify too).
> + }
> +#else
> + update_ktss_iopb_per_cpu (new_iopb, size, cpu_number());
> +#endif
> +}
> +
> /*
> * stack_handoff:
> *
> @@ -259,8 +279,12 @@ void stack_handoff(
> thread_t old,
> thread_t new)
> {
> - int mycpu = cpu_number();
> vm_offset_t stack;
> + task_t old_task, new_task;
> + int mycpu;
> + spl_t s;
> +
> + mycpu = cpu_number();
>
> /*
> * Save FP registers if in use.
> @@ -270,8 +294,6 @@ void stack_handoff(
> /*
> * Switch address maps if switching tasks.
> */
> - {
> - task_t old_task, new_task;
>
> if ((old_task = old->task) != (new_task = new->task)) {
> PMAP_DEACTIVATE_USER(vm_map_pmap(old_task->map),
> @@ -279,20 +301,12 @@ void stack_handoff(
> PMAP_ACTIVATE_USER(vm_map_pmap(new_task->map),
> new, mycpu);
>
> + s = splhigh();
What scenario does this protect against?
> simple_lock (&new_task->machine.iopb_lock);
> -#if NCPUS>1
> -#warning SMP support missing (avoid races with io_perm_modify).
> -#else
> - /* This optimization only works on a single processor
> - machine, where old_task's iopb can not change while
> - we are switching. */
> - if (old_task->machine.iopb || new_task->machine.iopb)
> -#endif
> - update_ktss_iopb (new_task->machine.iopb,
> - new_task->machine.iopb_size);
> + update_ktss_iopb (new_task->machine.iopb,
> new_task->machine.iopb_size);
> simple_unlock (&new_task->machine.iopb_lock);
> + splx(s);
> }
> - }
>
> /*
> * Load the rest of the user state for the new thread
> @@ -335,6 +349,10 @@ thread_t switch_context(
> void (*continuation)(),
> thread_t new)
> {
> + task_t old_task, new_task;
> + int mycpu = cpu_number();
> + spl_t s;
> +
> /*
> * Save FP registers if in use.
> */
> @@ -343,9 +361,6 @@ thread_t switch_context(
> /*
> * Switch address maps if switching tasks.
> */
> - {
> - task_t old_task, new_task;
> - int mycpu = cpu_number();
>
> if ((old_task = old->task) != (new_task = new->task)) {
> PMAP_DEACTIVATE_USER(vm_map_pmap(old_task->map),
> @@ -353,20 +368,12 @@ thread_t switch_context(
> PMAP_ACTIVATE_USER(vm_map_pmap(new_task->map),
> new, mycpu);
>
> + s = splhigh();
> simple_lock (&new_task->machine.iopb_lock);
> -#if NCPUS>1
> -#warning SMP support missing (avoid races with io_perm_modify).
> -#else
> - /* This optimization only works on a single processor
> - machine, where old_task's iopb can not change while
> - we are switching. */
> - if (old_task->machine.iopb || new_task->machine.iopb)
> -#endif
> - update_ktss_iopb (new_task->machine.iopb,
> - new_task->machine.iopb_size);
> + update_ktss_iopb (new_task->machine.iopb,
> new_task->machine.iopb_size);
> simple_unlock (&new_task->machine.iopb_lock);
> + splx(s);
> }
> - }
>
> /*
> * Load the rest of the user state for the new thread
> --
> 2.34.1
>
>
