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

[Eduardo Habkost]

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?

> +    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()?

> +        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
> 

-- 
Eduardo