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

[Igor Mammedov]

On Tue, 18 Oct 2016 10:47:02 -0200
Eduardo Habkost <address@hidden> 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>
> 
> But I have a few questions below that are not directly related to
> this patch:
> 
> > ---
> >  include/hw/acpi/acpi-defs.h | 18 +++++++++++
> >  hw/i386/acpi-build.c        | 78 
> > +++++++++++++++++++++++++++++++--------------
> >  2 files changed, 72 insertions(+), 24 deletions(-)
> > 
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index 9c1b7cb..e94123c 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -343,6 +343,24 @@ struct AcpiMadtLocalNmi {
> >  } QEMU_PACKED;
> >  typedef struct AcpiMadtLocalNmi AcpiMadtLocalNmi;
> >  
> > +struct AcpiMadtProcessorX2Apic {
> > +    ACPI_SUB_HEADER_DEF
> > +    uint16_t reserved;
> > +    uint32_t x2apic_id;              /* Processor's local x2APIC ID */
> > +    uint32_t flags;
> > +    uint32_t uid;                    /* Processor object _UID */
> > +} QEMU_PACKED;
> > +typedef struct AcpiMadtProcessorX2Apic AcpiMadtProcessorX2Apic;
> > +
> > +struct AcpiMadtLocalX2ApicNmi {
> > +    ACPI_SUB_HEADER_DEF
> > +    uint16_t flags;                  /* MPS INTI flags */
> > +    uint32_t uid;                    /* Processor object _UID */
> > +    uint8_t  lint;                   /* Local APIC LINT# */
> > +    uint8_t  reserved[3];            /* Local APIC LINT# */
> > +} QEMU_PACKED;
> > +typedef struct AcpiMadtLocalX2ApicNmi AcpiMadtLocalX2ApicNmi;
> > +
> >  struct AcpiMadtGenericInterrupt {
> >      ACPI_SUB_HEADER_DEF
> >      uint16_t reserved;
> > 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?
it's 1 byte fields only.

> 
> > +    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;
> > +        apic->x2apic_id = apic_id;
> > +        if (apic_ids->cpus[uid].cpu != NULL) {
> > +            apic->flags = cpu_to_le32(1);
> > +        } else {
> > +            apic->flags = cpu_to_le32(0);
> > +        }
> >      }
> >  }
> >  
> > @@ -369,11 +383,11 @@ build_madt(GArray *table_data, BIOSLinker *linker, 
> > PCMachineState *pcms)
> >      int madt_start = table_data->len;
> >      AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(pcms->acpi_dev);
> >      AcpiDeviceIf *adev = ACPI_DEVICE_IF(pcms->acpi_dev);
> > +    bool x2apic_mode = false;
> >  
> >      AcpiMultipleApicTable *madt;
> >      AcpiMadtIoApic *io_apic;
> >      AcpiMadtIntsrcovr *intsrcovr;
> > -    AcpiMadtLocalNmi *local_nmi;
> >      int i;
> >  
> >      madt = acpi_data_push(table_data, sizeof *madt);
> > @@ -382,6 +396,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, 
> > PCMachineState *pcms)
> >  
> >      for (i = 0; i < apic_ids->len; i++) {
> >          adevc->madt_cpu(adev, i, apic_ids, table_data);  
> 
> Why adevc->madt_cpu exists if all devices use
> pc_madt_cpu_entry()?
it's been added with ARM target in mind, so that it could be
possible to have generic hotplug code which uses this callback as well
and maybe some day to generalize build_madt() as well.

 
> > +        if (apic_ids->cpus[i].arch_id > 254) {
> > +            x2apic_mode = true;
> > +        }
> >      }
> >      g_free(apic_ids);
> >  
> > @@ -414,12 +431,25 @@ build_madt(GArray *table_data, BIOSLinker *linker, 
> > PCMachineState *pcms)
> >          intsrcovr->flags  = cpu_to_le16(0xd); /* active high, level 
> > triggered */
> >      }
> >  
> > -    local_nmi = acpi_data_push(table_data, sizeof *local_nmi);
> > -    local_nmi->type         = ACPI_APIC_LOCAL_NMI;
> > -    local_nmi->length       = sizeof(*local_nmi);
> > -    local_nmi->processor_id = 0xff; /* all processors */
> > -    local_nmi->flags        = cpu_to_le16(0);
> > -    local_nmi->lint         = 1; /* ACPI_LINT1 */
> > +    if (x2apic_mode) {
> > +        AcpiMadtLocalX2ApicNmi *local_nmi;
> > +
> > +        local_nmi = acpi_data_push(table_data, sizeof *local_nmi);
> > +        local_nmi->type   = ACPI_APIC_LOCAL_X2APIC_NMI;
> > +        local_nmi->length = sizeof(*local_nmi);
> > +        local_nmi->uid    = 0xFFFFFFFF; /* all processors */
> > +        local_nmi->flags  = cpu_to_le16(0);
> > +        local_nmi->lint   = 1; /* ACPI_LINT1 */
> > +    } else {
> > +        AcpiMadtLocalNmi *local_nmi;
> > +
> > +        local_nmi = acpi_data_push(table_data, sizeof *local_nmi);
> > +        local_nmi->type         = ACPI_APIC_LOCAL_NMI;
> > +        local_nmi->length       = sizeof(*local_nmi);
> > +        local_nmi->processor_id = 0xff; /* all processors */
> > +        local_nmi->flags        = cpu_to_le16(0);
> > +        local_nmi->lint         = 1; /* ACPI_LINT1 */
> > +    }
> >  
> >      build_header(linker, table_data,
> >                   (void *)(table_data->data + madt_start), "APIC",
> > -- 
> > 2.7.4
> >   
>