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  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   
> > 
> > Reviewed-by: Eduardo Habkost   
> 
> 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);
> > > +}
> > >  }
> > >  }  
> [...]
> 

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

[Eduardo Habkost]

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 
> 
> Reviewed-by: Eduardo Habkost 

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

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  wrote:

> On Thu, Oct 13, 2016 at 11:52:35AM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov   
> 
> Reviewed-by: Eduardo Habkost 
> 
> 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, 
> 

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 

Reviewed-by: Eduardo Habkost 

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