Re: [PATCH v4 3/3 gnumach] apic: Add extended feature registers for local apic unit

[Samuel Thibault]

Applied, thanks!

Damien Zammit via Bug reports for the GNU Hurd, le dim. 22 déc. 2024 01:43:44 
+0000, a ecrit:
> Add workaround for broken systems that advertise 8 bit APIC ids
> but only match IPIs on 4 bits of the APIC id.
> ---
>  i386/i386/apic.c              | 28 +++++++++++++++++++++++++++-
>  i386/i386/apic.h              | 15 ++++++++++++++-
>  i386/i386/cpu_number.h        |  3 +++
>  i386/i386/cpuboot.S           |  1 +
>  i386/i386at/acpi_parse_apic.c |  5 ++++-
>  i386/i386at/ioapic.c          |  8 ++++++--
>  6 files changed, 55 insertions(+), 5 deletions(-)
> 
> diff --git a/i386/i386/apic.c b/i386/i386/apic.c
> index e0941c6a..77d555b5 100644
> --- a/i386/i386/apic.c
> +++ b/i386/i386/apic.c
> @@ -46,6 +46,13 @@ int cpu_id_lut[UINT8_MAX + 1] = {0};
>  
>  ApicInfo apic_data;
>  
> +/* xAPIC: This is supposed to be an 8 bit mask.  On some platforms it needs 
> to be a
> + * lower 4 bit mask because the chipset only matches on 4 bits of the id 
> when doing IPIs.
> + * The cpuid accessor and lapic may report full 8 bit id so always & with 
> this mask when
> + * reading the APIC id.  Increases to 8 bits if no workaround is required.
> +*/
> +uint8_t apic_id_mask = 0xf;
> +
>  /*
>   * apic_data_init: initialize the apic_data structures to preliminary values.
>   * Reserve memory to the lapic list dynamic vector.
> @@ -206,7 +213,7 @@ apic_get_current_cpu(void)
>      eax = 1;
>      ecx = 0;
>      cpuid(eax, ebx, ecx, edx);
> -    return (ebx >> 24);
> +    return (ebx >> 24) & apic_id_mask;
>  }
>  
>  
> @@ -322,6 +329,25 @@ lapic_disable(void)
>      lapic->spurious_vector.r &= ~LAPIC_ENABLE;
>  }
>  
> +void
> +fix_apic_id_mask(void)
> +{
> +    if (lapic->version.r & APIC_VERSION_HAS_EXT_APIC_SPACE) {
> +        /* Extended registers beyond 0x3f0 are present */
> +        if (lapic->extended_feature.r & APIC_EXT_FEATURE_HAS_8BITID) {
> +            /* 8 bit APIC ids are supported on this local APIC */
> +            if (!(lapic->extended_control.r & APIC_EXT_CTRL_ENABLE_8BITID)) {
> +                printf("WARNING: Only 4 bit APIC ids\n");
> +                apic_id_mask = 0xf;
> +                return;
> +            }
> +        }
> +    }
> +
> +    printf("8 bit APIC ids\n");
> +    apic_id_mask = 0xff;
> +}
> +
>  void
>  lapic_setup(void)
>  {
> diff --git a/i386/i386/apic.h b/i386/i386/apic.h
> index 5b38bfba..92fb900a 100644
> --- a/i386/i386/apic.h
> +++ b/i386/i386/apic.h
> @@ -189,8 +189,20 @@ typedef struct ApicLocalUnit {
>          ApicReg reserved3d;              /* 0x3d0 */
>          ApicReg divider_config;          /* 0x3e0 */
>          ApicReg reserved3f;              /* 0x3f0 */
> +        ApicReg extended_feature;        /* 0x400  Present if version 
> extended apic space bit is set */
> +        ApicReg extended_control;        /* 0x410 */
> +        ApicReg specific_eoi;            /* 0x420 */
>  } ApicLocalUnit;
>  
> +#define APIC_VERSION_HAS_EXT_APIC_SPACE      (1 << 31)
> +#define APIC_VERSION_HAS_DIRECTED_EOI        (1 << 24)
> +
> +#define APIC_EXT_FEATURE_HAS_SEOI    (1 << 1)
> +#define APIC_EXT_FEATURE_HAS_8BITID  (1 << 2)
> +
> +#define APIC_EXT_CTRL_ENABLE_SEOI    (1 << 1)
> +#define APIC_EXT_CTRL_ENABLE_8BITID  (1 << 2)
> +
>  typedef struct IoApicData {
>          uint8_t  apic_id;
>          uint8_t  ngsis;
> @@ -244,6 +256,7 @@ int apic_get_total_gsis(void);
>  void picdisable(void);
>  void lapic_eoi(void);
>  void ioapic_irq_eoi(int pin);
> +void fix_apic_id_mask(void);
>  void lapic_setup(void);
>  void lapic_disable(void);
>  void lapic_enable(void);
> @@ -261,6 +274,7 @@ extern void intnull(int unit);
>  extern volatile ApicLocalUnit* lapic;
>  extern int cpu_id_lut[];
>  extern uint32_t *hpet_addr;
> +extern uint8_t apic_id_mask;
>  
>  #endif
>  
> @@ -292,7 +306,6 @@ extern uint32_t *hpet_addr;
>  #define LAPIC_TIMER_DIVIDE_8           2
>  #define LAPIC_TIMER_DIVIDE_16          3
>  #define LAPIC_TIMER_BASEDIV            0x100000
> -#define LAPIC_HAS_DIRECTED_EOI         0x1000000
>  
>  #define NINTR                          64 /* Max 32 GSIs on each of two 
> IOAPICs */
>  #define IOAPIC_FIXED                   0
> diff --git a/i386/i386/cpu_number.h b/i386/i386/cpu_number.h
> index 547e0498..0c0ec189 100644
> --- a/i386/i386/cpu_number.h
> +++ b/i386/i386/cpu_number.h
> @@ -46,6 +46,7 @@
>       movl    %cs:lapic, reg          ;\
>       movl    %cs:APIC_ID(reg), reg   ;\
>       shrl    $24, reg                ;\
> +     andl    %cs:apic_id_mask, reg   ;\
>       movl    %cs:CX(cpu_id_lut, reg), reg    ;\
>  
>  /* Fast version, requires a stack */
> @@ -60,6 +61,7 @@
>       movl    $1, %eax        ;\
>       cpuid                   ;\
>       shrl    $24, %ebx       ;\
> +     andl    %cs:apic_id_mask, %ebx  ;\
>       movl    %cs:CX(cpu_id_lut, %ebx), %esi  ;\
>       popl    %edx            ;\
>       popl    %ecx            ;\
> @@ -79,6 +81,7 @@
>       movl    $1, %eax        ;\
>       cpuid                   ;\
>       shrl    $24, %ebx       ;\
> +     andl    %cs:apic_id_mask, %ebx  ;\
>       movl    %cs:CX(cpu_id_lut, %ebx), %esi  ;\
>       popq    %rdx            ;\
>       popq    %rcx            ;\
> diff --git a/i386/i386/cpuboot.S b/i386/i386/cpuboot.S
> index 6ba7aa42..76de8714 100644
> --- a/i386/i386/cpuboot.S
> +++ b/i386/i386/cpuboot.S
> @@ -171,6 +171,7 @@ apboot_jmp_offset:
>       movl    $1, %eax
>       cpuid
>       shrl    $24, %ebx
> +     andl    %cs:apic_id_mask, %ebx
>       movl    %cs:CX(cpu_id_lut, %ebx), %ebp
>  
>       /* Copy first gdt descriptor and gdt to cpu-th area */
> diff --git a/i386/i386at/acpi_parse_apic.c b/i386/i386at/acpi_parse_apic.c
> index ae92ee2e..bac5d04d 100644
> --- a/i386/i386at/acpi_parse_apic.c
> +++ b/i386/i386at/acpi_parse_apic.c
> @@ -378,7 +378,7 @@ acpi_apic_add_lapic(struct acpi_apic_lapic *lapic_entry)
>      /* If cpu flag is correct */
>      if (lapic_entry->flags & (ACPI_LAPIC_FLAG_ENABLED | 
> ACPI_LAPIC_FLAG_CAPABLE)) {
>          /* Add cpu to processors' list. */
> -        apic_add_cpu(lapic_entry->apic_id);
> +        apic_add_cpu(lapic_entry->apic_id & apic_id_mask);
>      }
>  
>  }
> @@ -541,6 +541,9 @@ acpi_apic_setup(struct acpi_apic *apic)
>          return ACPI_NO_LAPIC;
>  
>      apic_lapic_init(lapic_unit);
> +
> +    fix_apic_id_mask();
> +
>      acpi_apic_parse_table(apic);
>  
>      ncpus = apic_get_numcpus();
> diff --git a/i386/i386at/ioapic.c b/i386/i386at/ioapic.c
> index 845d1249..5dd2de2e 100644
> --- a/i386/i386at/ioapic.c
> +++ b/i386/i386at/ioapic.c
> @@ -366,7 +366,7 @@ void
>  ioapic_configure(void)
>  {
>      /* Assume first IO APIC maps to GSI base 0 */
> -    int gsi, apic = 0, bsp = 0, pin;
> +    int gsi, apic = 0, pin;
>      IrqOverrideData *irq_over;
>      int timer_gsi;
>      int version = ioapic_version(apic);
> @@ -387,7 +387,11 @@ ioapic_configure(void)
>      entry.both.delvmode = IOAPIC_FIXED;
>      entry.both.destmode = IOAPIC_PHYSICAL;
>      entry.both.mask = IOAPIC_MASK_DISABLED;
> -    entry.both.dest = apic_get_cpu_apic_id(bsp);
> +    /* This does not use apic_id_mask because
> +     * the IOAPIC seems to use what is actually set
> +     * in lapic's apic_id register.
> +     */
> +    entry.both.dest = lapic->apic_id.r >> 24;
>  
>      for (pin = 0; pin < 16; pin++) {
>          gsi = pin;
> -- 
> 2.45.2
> 
> 
> 

-- 
Samuel
 PS> Salut ! J'ai un sujet de philo à vous soumettre : "Suffit-il
 PS> d'observer pour connaître" Idées + plan Merçi
 Oui, ya qu'a t'observer pour connaître le fait que tu es une feignasse.
 -+- FF in: Guide du Neuneu d'Usenet - Neuneu fait de la philo -+-