On Tue, 18 Oct 2016 11:05:47 -0200
Eduardo Habkost <ehabk...@redhat.com> 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 <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:
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);
> > > + }
> > > }
> > > }
> [...]
>
