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

[Matthew Barnes <matthew.barnes@xxxxxxxxx>]

> > 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.

In the ACPI 6.4 specification, section 5.2.12.12, the note says the following:

[Compatibility note] On some legacy OSes, Logical processors with APIC ID
values less than 255 (whether in XAPIC or X2APIC mode) must use the Processor
Local APIC structure to convey their APIC information to OSPM, and those
processors must be declared in the DSDT using the Processor() keyword.

Therefore, even in X2APIC mode, it's better to represent processors with APIC
ID values less than 255 with APIC entries for legacy reasons.

> > @@ -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).

I will add this in patch V2

> > +        {
> > +            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.

I will add this in patch V2

Matt