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

[Igor Mammedov]

On Tue, 18 Oct 2016 11:05:47 -0200
Eduardo Habkost <address@hidden> wrote:

> 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:
Thanks for noticing,
 it needs cpu_to_le32() at some places.
I'll fix it and post v4 here.

> 
> >   
> > > +    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);
> > > +        }
> > >      }
> > >  }  
> [...]
>