Re: [Qemu-devel] [PATCH v3 01/13] pc: acpi: x2APIC support for MADT table

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 10:47:02AM -0200, Eduardo Habkost wrote:
> On Thu, Oct 13, 2016 at 11:52:35AM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <address@hidden>
> 
> Reviewed-by: Eduardo Habkost <address@hidden>

Reviewed-by withdrawn. See below:

[...]
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index e999654..702d254 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -340,24 +340,38 @@ build_fadt(GArray *table_data, BIOSLinker *linker, 
> > AcpiPmInfo *pm,
> >  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> >                         CPUArchIdList *apic_ids, GArray *entry)
> >  {
> > -    int apic_id;
> > -    AcpiMadtProcessorApic *apic = acpi_data_push(entry, sizeof *apic);
> > -
> > -    apic_id = apic_ids->cpus[uid].arch_id;
> > -    apic->type = ACPI_APIC_PROCESSOR;
> > -    apic->length = sizeof(*apic);
> > -    apic->processor_id = uid;
> > -    apic->local_apic_id = apic_id;
> > -    if (apic_ids->cpus[uid].cpu != NULL) {
> > -        apic->flags = cpu_to_le32(1);
> 
> Shouldn't length, processor_id, and local_apic_id be converted to
> little-endian too?

Erm, those fields are all 8-bit. Nevermind. But that means the
new x2apic code below seems wrong:

> 
> > +    uint32_t apic_id = apic_ids->cpus[uid].arch_id;
> > +
> > +    /* ACPI spec says that LAPIC entry for non present
> > +     * CPU may be omitted from MADT or it must be marked
> > +     * as disabled. However omitting non present CPU from
> > +     * MADT breaks hotplug on linux. So possible CPUs
> > +     * should be put in MADT but kept disabled.
> > +     */
> > +    if (apic_id < 255) {
> > +        AcpiMadtProcessorApic *apic = acpi_data_push(entry, sizeof *apic);
> > +
> > +        apic->type = ACPI_APIC_PROCESSOR;
> > +        apic->length = sizeof(*apic);
> > +        apic->processor_id = uid;
> > +        apic->local_apic_id = apic_id;
> > +        if (apic_ids->cpus[uid].cpu != NULL) {
> > +            apic->flags = cpu_to_le32(1);
> > +        } else {
> > +            apic->flags = cpu_to_le32(0);
> > +        }
> >      } else {
> > -        /* ACPI spec says that LAPIC entry for non present
> > -         * CPU may be omitted from MADT or it must be marked
> > -         * as disabled. However omitting non present CPU from
> > -         * MADT breaks hotplug on linux. So possible CPUs
> > -         * should be put in MADT but kept disabled.
> > -         */
> > -        apic->flags = cpu_to_le32(0);
> > +        AcpiMadtProcessorX2Apic *apic = acpi_data_push(entry, sizeof 
> > *apic);
> > +
> > +        apic->type = ACPI_APIC_LOCAL_X2APIC;
> > +        apic->length = sizeof(*apic);
> > +        apic->uid = uid;

cpu_to_le32()?

> > +        apic->x2apic_id = apic_id;

cpu_to_le32()?

> > +        if (apic_ids->cpus[uid].cpu != NULL) {
> > +            apic->flags = cpu_to_le32(1);
> > +        } else {
> > +            apic->flags = cpu_to_le32(0);
> > +        }
> >      }
> >  }
[...]

-- 
Eduardo