Re: [XEN PATCH] tools: add x2APIC entries in MADT based on APIC ID

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.03.2024 16:35, Matthew Barnes wrote:
> libacpi is a tool that is used by libxl (for PVH guests) and hvmloader
> (for HVM guests) to construct ACPI tables for guests.
> 
> Currently, libacpi only uses APIC entries to enumerate processors for
> guests in the MADT.
> 
> The APIC ID field in APIC entries is an octet big, which is fine for
> xAPIC IDs, but not so for sufficiently large x2APIC IDs.

Yet where would those come from? I can see that down the road we will
have such, but right now I don't think we do. Without saying so, this
change could be mistaken for a fix of an active bug.

> This patch scans each APIC ID before constructing the MADT, and uses the
> x2APIC entry for each vCPU whose APIC ID exceeds the size limit imposed
> by regular APIC entries.

It is my understanding that if you use any x2APIC entry, every CPU needs
to have one.

> @@ -134,27 +151,45 @@ static struct acpi_20_madt *construct_madt(struct 
> acpi_ctxt *ctxt,
>          io_apic->ioapic_id   = config->ioapic_id;
>          io_apic->ioapic_addr = config->ioapic_base_address;
>  
> -        lapic = (struct acpi_20_madt_lapic *)(io_apic + 1);
> +        apicid_entry = io_apic + 1;
>      }
>      else
> -        lapic = (struct acpi_20_madt_lapic *)(madt + 1);
> +        apicid_entry = madt + 1;
>  
>      info->nr_cpus = hvminfo->nr_vcpus;
> -    info->madt_lapic0_addr = ctxt->mem_ops.v2p(ctxt, lapic);
> +    info->madt_lapic0_addr = ctxt->mem_ops.v2p(ctxt, apicid_entry);
>      for ( i = 0; i < hvminfo->nr_vcpus; i++ )
>      {
> -        memset(lapic, 0, sizeof(*lapic));
> -        lapic->type    = ACPI_PROCESSOR_LOCAL_APIC;
> -        lapic->length  = sizeof(*lapic);
> -        /* Processor ID must match processor-object IDs in the DSDT. */
> -        lapic->acpi_processor_id = i;
> -        lapic->apic_id = config->lapic_id(i);
> -        lapic->flags = (test_bit(i, hvminfo->vcpu_online)
> -                        ? ACPI_LOCAL_APIC_ENABLED : 0);
> -        lapic++;
> +        uint32_t apic_id = config->lapic_id(i);
> +        if ( apic_id < 255 )

Nit (here and below): This file uses hypervisor coding style. and hence a
blank line is wanted between declaration(s) and statement(s).

> +        {
> +            struct acpi_20_madt_lapic *lapic = apicid_entry;
> +            memset(lapic, 0, sizeof(*lapic));
> +            lapic->type    = ACPI_PROCESSOR_LOCAL_APIC;
> +            lapic->length  = sizeof(*lapic);
> +            /* Processor ID must match processor-object IDs in the DSDT. */
> +            lapic->acpi_processor_id = i;
> +            lapic->apic_id = apic_id;
> +            lapic->flags = (test_bit(i, hvminfo->vcpu_online)
> +                            ? ACPI_LOCAL_APIC_ENABLED : 0);
> +            apicid_entry = lapic + 1;
> +        }
> +        else
> +        {
> +            struct acpi_20_madt_x2apic *x2apic = apicid_entry;
> +            memset(x2apic, 0, sizeof(*x2apic));
> +            x2apic->type    = ACPI_PROCESSOR_LOCAL_X2APIC;
> +            x2apic->length  = sizeof(*x2apic);
> +            x2apic->apic_id = apic_id;
> +            x2apic->flags   = (test_bit(i, hvminfo->vcpu_online)
> +                                ? ACPI_LOCAL_APIC_ENABLED : 0);

Nit: Indentation off by 1.

Jan