Re: [PATCH v7 04/10] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.10.2024 17:45, Alejandro Vallejo wrote:
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -224,7 +224,7 @@ static void apic_setup(void)
>  
>      /* 8259A ExtInts are delivered through IOAPIC pin 0 (Virtual Wire Mode). 
> */
>      ioapic_write(0x10, APIC_DM_EXTINT);
> -    ioapic_write(0x11, SET_APIC_ID(LAPIC_ID(0)));
> +    ioapic_write(0x11, SET_APIC_ID(cpu_to_x2apicid[0]));
>  }

In uses like this or ...

> --- a/tools/firmware/hvmloader/mp_tables.c
> +++ b/tools/firmware/hvmloader/mp_tables.c
> @@ -198,8 +198,10 @@ static void fill_mp_config_table(struct mp_config_table 
> *mpct, int length)
>  /* fills in an MP processor entry for VCPU 'vcpu_id' */
>  static void fill_mp_proc_entry(struct mp_proc_entry *mppe, int vcpu_id)
>  {
> +    ASSERT(cpu_to_x2apicid[vcpu_id] < 0xFF );
> +
>      mppe->type = ENTRY_TYPE_PROCESSOR;
> -    mppe->lapic_id = LAPIC_ID(vcpu_id);
> +    mppe->lapic_id = cpu_to_x2apicid[vcpu_id];

... this one (and also in acpi_lapic_id()), I consider the "x2" in the name
actively confusing, despite ...

> --- a/tools/firmware/hvmloader/smp.c
> +++ b/tools/firmware/hvmloader/smp.c
> @@ -29,7 +29,37 @@
>  
>  #include <xen/vcpu.h>
>  
> -static int ap_callin;
> +/**
> + * Lookup table of (x2)APIC IDs.
> + *
> + * Each entry is populated its respective CPU as they come online. This is 
> required
> + * for generating the MADT with minimal assumptions about ID relationships.
> + *
> + * While the name makes "x2" explicit, these may actually be xAPIC IDs if no
> + * x2APIC is present. "x2" merely highlights that each entry is 32 bits wide.
> + */
> +uint32_t cpu_to_x2apicid[HVM_MAX_VCPUS];

... the commentary here.

> +/** Tristate about x2apic being supported. -1=unknown */
> +static int has_x2apic = -1;

Why is this a tristate? Prior to the variable having been set, ...

> +static uint32_t read_apic_id(void)
> +{
> +    uint32_t apic_id;
> +
> +    if ( has_x2apic )
> +        cpuid(0xb, NULL, NULL, NULL, &apic_id);

... this is bogus anyway.

Jan