Re: [Qemu-devel] [PATCH v8 04/25] acpi: add DMAR scope definition for root IOAPIC

[Peter Xu <peterx@redhat.com>]

On Wed, Jun 01, 2016 at 02:56:38PM +0200, Igor Mammedov wrote:

[...]

> > @@ -2561,6 +2563,9 @@ build_dmar_q35(MachineState *ms, GArray *table_data, GArray *linker)
> >      AcpiTableDmar *dmar;
> >      AcpiDmarHardwareUnit *drhd;
> >      uint8_t dmar_flags = 0;
> > +    AcpiDmarDeviceScope *scope = NULL;
> > +    /* Root complex IOAPIC use one path[0] only */
> > +    uint16_t scope_size = sizeof(*scope) + sizeof(uint16_t);
> ioapic_scope_size
> sizeof(scope->path[0]) /* space for IOxAPIC path */

Agree on both.

> 
> >      if (ms->iommu_intr) {
> >          /* enable INTR for the IOMMU device */
> > @@ -2574,11 +2579,19 @@ build_dmar_q35(MachineState *ms, GArray *table_data, GArray *linker)
> >      /* DMAR Remapping Hardware Unit Definition structure */
> >      drhd = acpi_data_push(table_data, sizeof(*drhd));
> sizeof(*drhd) + ioapic_scope_size
> 
> >      drhd->type = cpu_to_le16(ACPI_DMAR_TYPE_HARDWARE_UNIT);
> > -    drhd->length = cpu_to_le16(sizeof(*drhd));   /* No device scope now */
> > +    drhd->length = cpu_to_le16(sizeof(*drhd) + scope_size);
> >      drhd->flags = ACPI_DMAR_INCLUDE_PCI_ALL;
> >      drhd->pci_segment = cpu_to_le16(0);
> >      drhd->address = cpu_to_le64(Q35_HOST_BRIDGE_IOMMU_ADDR);
> >  
> > +    /* Scope definition for the root-complex IOAPIC */
> > +    scope = acpi_data_push(table_data, scope_size);
> no need for this as previous push took care about all space we need for scope
>        ioapic_scope = &drhd->scope[0];

Both work for me. I can do a switch.

> 
> > +    scope->entry_type = cpu_to_le16(ACPI_DMAR_DEV_SCOPE_TYPE_IOAPIC);
> entry_type is 1 byte long, doing cpu_to_le16 on bigendian host
> will always result in 0 being written there
> 
> > +    scope->length = scope_size;
> > +    scope->enumeration_id = cpu_to_le16(ACPI_BUILD_IOAPIC_ID);
> > +    scope->bus = cpu_to_le16(Q35_PSEUDO_BUS_PLATFORM);
> ditto for above 2 fields

Ah, didn't notice these lines after it worked. For the three places, I
should remove cpu_to_le16().

[...]

> > +/*
> > + * Arbitary but unique BNF number for IOAPIC device. This is only
> > + * used when interrupt remapping is enabled.
> you encode it in DMAR unconditionally so not "only"

Okay. Removing the 2nd sentence.

Thanks for the review comments, Igor!

-- peterx