Re: [PATCH 2/4] i386: Fix lapic and ioapic for smp

[Almudena Garcia]

> Again, we need a memory barrier, to prevent the compiler from emitting
these writes in the wrong order.

Really, these writes are using a temporary struct. There are a only write to the real register, which is done assigning the temporary struct to the real register through the HAL 

El mar., 15 nov. 2022 1:40, Samuel Thibault <samuel.thibault@gnu.org> escribió:

Damien Zammit, le ven. 11 nov. 2022 23:21:22 +0000, a ecrit:
> diff --git a/i386/i386/apic.c b/i386/i386/apic.c
> index d30084e2..6a5fa754 100644
> --- a/i386/i386/apic.c
> +++ b/i386/i386/apic.c
> @@ -19,6 +19,8 @@
>     Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111, USA. */
>
>  #include <i386/apic.h>
> +#include <i386/cpu.h>
> +#include <i386at/idt.h>
>  #include <string.h>
>  #include <vm/vm_kern.h>
>  #include <kern/printf.h>
> @@ -116,11 +118,27 @@ uint16_t
>  apic_get_cpu_apic_id(int kernel_id)
>  {
>      if (kernel_id >= NCPUS)
> -        return -1;
> +        return 0;

As already mentioned:

Better make it return an int16_t to be able to return -1? Otherwise I
guess we can confuse that 0 with acpi_id 0?

>
>      return apic_data.cpu_lapic_list[kernel_id];
>  }
>
> +
> +/*
> + * apic_get_cpu_kernel_id: returns the kernel_id of a cpu.
> + * Receives as input the APIC ID of a CPU.
> + */
> +int
> +apic_get_cpu_kernel_id(uint16_t apic_id)
> +{
> +    int i = 0;
> +
> +    while(apic_data.cpu_lapic_list[i] != apic_id && i < apic_data.ncpus)
> +        i++;

Rather just use

    for (i = 0; i < apic_data.ncpus; i++)
      if (apic_data.cpu_lapic_list[i] == apic_id)
        return i;

which is much more canonical and thus more maintainable.

> @@ -235,6 +249,61 @@ void apic_print_info(void)
>      }
>  }
>
> +void apic_send_ipi(unsigned dest_shorthand, unsigned deliv_mode, unsigned dest_mode, unsigned level, unsigned trig_mode, unsigned vector, unsigned dest_id)
> +{
> +    IcrLReg icrl_values;
> +    IcrHReg icrh_values;
> +
> +    icrl_values.destination_shorthand = dest_shorthand;
> +    icrl_values.delivery_mode = deliv_mode;
> +    icrl_values.destination_mode = dest_mode;
> +    icrl_values.level = level;
> +    icrl_values.trigger_mode = trig_mode;
> +    icrl_values.vector = vector;
> +    icrh_values.destination_field = dest_id;
> +
> +    lapic->icr_high = icrh_values;
> +    lapic->icr_low = icrl_values;

Again, we need a memory barrier, to prevent the compiler from emitting
these writes in the wrong order.

> --- a/i386/i386at/ioapic.c
> +++ b/i386/i386at/ioapic.c
> -    splon(s);
> +    /* Enable interrupts for the first time on BSP */
> +    asm("sti");

cpu_intr_enable is available for this.

Samuel