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 <imamm...@redhat.com> > > Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
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