Re: [PATCH 09/12] i386: Fix lapic and ioapic for smp

[Samuel Thibault]

Damien Zammit, le mar. 25 oct. 2022 10:56:20 +0000, a ecrit:
> diff --git a/i386/i386/apic.c b/i386/i386/apic.c
> index 2e0c1776..b31162fe 100644
> --- a/i386/i386/apic.c
> +++ b/i386/i386/apic.c
> @@ -28,6 +30,7 @@
>  volatile ApicLocalUnit* lapic = NULL;
> 
>  ApicInfo apic_data;
> +ApicInfo *apic_data_ptr;

What for? We can just expose the apic_data variable itself.

> @@ -116,11 +120,27 @@ uint16_t
>  apic_get_cpu_apic_id(int kernel_id)
>  {
>      if (kernel_id >= NCPUS)
> -        return -1;
> +        return 0;

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?

> +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;

>  /* apic_get_lapic: returns a reference to the common memory address for 
> Local APIC. */
>  volatile ApicLocalUnit*
>  apic_get_lapic(void)
> @@ -161,14 +181,7 @@ apic_get_num_ioapics(void)
>  uint16_t
>  apic_get_current_cpu(void)
>  {
> -    uint16_t apic_id;
> -
> -    if(lapic == NULL)
> -        apic_id = 0;
> -    else
> -        apic_id = lapic->apic_id.r;
> -
> -    return apic_id;
> +    return (lapic->apic_id.r >> 24) & 0xff;

I guess we may have get_current_cpu() calls hidden in macros that would
be called very early, and thus better do check for lapic being
initialized yet or not.

> @@ -234,3 +247,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;
> +
> +    //printf("ICR Low: %08x\n", icrl_values.r);
> +    //printf("ICR High: %08x\n", icrh_values.r);
> +
> +    lapic->icr_high = icrh_values;
> +    lapic->icr_low = icrl_values;

I'd guess we need some barrier, to make sure the compiler emits the
eventual triggering write after all other writes have been done?

> diff --git a/i386/i386/apic.h b/i386/i386/apic.h
> index add1b8cf..d45780d2 100644
> --- a/i386/i386/apic.h
> +++ b/i386/i386/apic.h
> @@ -61,10 +61,99 @@ union ioapic_route_entry_union {
>      struct ioapic_route_entry both;
>  };
> 
> +
> +/* Grateful to trasterlabs for this snippet */

What is the copyright of this code?

> diff --git a/i386/i386/smp.c b/i386/i386/smp.c
> index d7523a73..e32b8f6a 100644
> --- a/i386/i386/smp.c
> +++ b/i386/i386/smp.c
> +void smp_startup_cpu(unsigned apic_id, unsigned vector)
> +{
> +    /* Clear APIC errors */
> +    lapic->error_status.r = 0;
> +
> +    printf("Sending IPIs to APIC ID %u...", apic_id);
> +
> +    /* Assert INIT IPI */
> +    apic_send_ipi(NO_SHORTHAND, INIT, PHYSICAL, ASSERT, LEVEL, 0, apic_id);
> +
> +    /* Wait for delivery */
> +    do {
> +        pause_memory;
> +    } while(lapic->icr_low.delivery_status == SEND_PENDING);
> +
> +    /* Deassert INIT IPI */
> +    apic_send_ipi(NO_SHORTHAND, INIT, PHYSICAL, DE_ASSERT, LEVEL, 0, 
> apic_id);
> +
> +    /* Wait for delivery */
> +    do {
> +        pause_memory;
> +    } while(lapic->icr_low.delivery_status == SEND_PENDING);
> +
> +    /* Wait 10 msec */
> +    pit_mdelay(10);

Are these delays specified by Intel or something like that?

> +
> +    /* Clear APIC errors */
> +    lapic->error_status.r = 0;
> +
> +    /* First StartUp IPI */
> +    apic_send_ipi(NO_SHORTHAND, STARTUP, PHYSICAL, ASSERT, LEVEL, vector >> 
> 12, apic_id);
> +
> +    /* Wait 200 usec */
> +    pit_udelay(200);
> +
> +    /* Wait for delivery */
> +    do {
> +        pause_memory;
> +    } while(lapic->icr_low.delivery_status == SEND_PENDING);
> +
> +    /* Second StartUp IPI */
> +    apic_send_ipi(NO_SHORTHAND, STARTUP, PHYSICAL, ASSERT, LEVEL, vector >> 
> 12, apic_id);
> +
> +    /* Wait 200 usec */
> +    pit_udelay(200);
> +
> +    /* Wait for delivery */
> +    do {
> +        pause_memory;
> +    } while(lapic->icr_low.delivery_status == SEND_PENDING);
> +
> +    printf("done\n");