Re: [Qemu-devel] [PATCH 05/13] acpi: add DMAR scope definition for root IOAPIC

[Marcel Apfelbaum]

On 02/21/2016 03:40 PM, Jan Kiszka wrote:
> On 2016-02-21 13:08, Marcel Apfelbaum wrote:
> > On 02/21/2016 01:38 PM, Marcel Apfelbaum wrote:
> > > On 02/19/2016 05:30 AM, Peter Xu wrote:
> > > > To enable interrupt remapping for intel IOMMU device, each IOAPIC device
> > > > in the system reported via ACPI MADT must be explicitly enumerated under
> > > > one specific remapping hardware unit. This patch adds the root-complex
> > > > IOAPIC into the default DMAR device.
> > > >
> > > > Please refer to VT-d spec 8.3.1.1 for more information.
> > > >
> > > > Signed-off-by: Peter Xu <address@hidden>
> > > > ---
> > > >    hw/i386/acpi-build.c        | 23 +++++++++++++++++++++--
> > > >    include/hw/acpi/acpi-defs.h | 15 +++++++++++++++
> > > >    2 files changed, 36 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > index d9e4f91..1cefe43 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -76,6 +76,9 @@
> > > >    #define ACPI_BUILD_DPRINTF(fmt, ...)
> > > >    #endif
> > > >
> > > > +/* Default IOAPIC ID */
> > > > +#define ACPI_BUILD_IOAPIC_ID 0x0
> > > > +
> > > >    typedef struct AcpiCpuInfo {
> > > >        DECLARE_BITMAP(found_cpus, ACPI_CPU_HOTPLUG_ID_LIMIT);
> > > >    } AcpiCpuInfo;
> > > > @@ -392,7 +395,6 @@ build_madt(GArray *table_data, GArray *linker,
> > > > AcpiCpuInfo *cpu)
> > > >        io_apic = acpi_data_push(table_data, sizeof *io_apic);
> > > >        io_apic->type = ACPI_APIC_IO;
> > > >        io_apic->length = sizeof(*io_apic);
> > > > -#define ACPI_BUILD_IOAPIC_ID 0x0
> > > >        io_apic->io_apic_id = ACPI_BUILD_IOAPIC_ID;
> > > >        io_apic->address = cpu_to_le32(IO_APIC_DEFAULT_ADDRESS);
> > > >        io_apic->interrupt = cpu_to_le32(0);
> > > > @@ -2511,6 +2513,9 @@ build_dmar_q35(GArray *table_data, GArray *linker)
> > > >        AcpiDmarHardwareUnit *drhd;
> > > >        uint8_t dmar_flags = 0;
> > > >        IntelIOMMUState *intel_iommu = acpi_get_iommu();
> > > > +    AcpiDmarDeviceScope *scope = NULL;
> > > > +    /* Root complex IOAPIC use one path[0] only */
> > > > +    uint16_t scope_size = sizeof(*scope) + sizeof(uint16_t);
> > > >
> > > >        assert(intel_iommu);
> > > >
> > > > @@ -2526,11 +2531,25 @@ build_dmar_q35(GArray *table_data, GArray
> > > > *linker)
> > > >        /* DMAR Remapping Hardware Unit Definition structure */
> > > >        drhd = acpi_data_push(table_data, sizeof(*drhd));
> > > >        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);
> > > > +    scope->entry_type = cpu_to_le16(ACPI_DMAR_DEV_SCOPE_TYPE_IOAPIC);
> > > > +    scope->length = scope_size;
> > > > +    /*
> > > > +     * An arbitary but unique bus number, to be used to generate
> > > > +     * source ID for IOAPIC device in BDF format.
> > > > +     */
> > > > +#define ACPI_IOAPIC_BUS_IR         (0xf0)
> > > > +#define ACPI_IOAPIC_DEVFN_IR       (0x00)
> > >
> > > Hi,
> > >
> > > How do you know for sure there is no bus (or bus & device) having the
> > > number 0xf0 in the system?
> > > Now that we support multiple Root Complexes, using the  pxb-pcie
> > > device we can simply add a PCI root bus like this:
> > >       -device pxb-pcie,bus_nr=0xf0
> > > or we can add enough switches to get to this number.
> > >
> > > You could dynamically query for an unused PCI bus, but the number
> > > would change between the runs.
> > > Or, I suppose you can reserve a slot on bus 0 for that. It is
> > > interesting how it works on a real machine.
> >
> > thinking about it more, maybe we should let the firmware to assign the
> > bus/dev/fun for the IO APIC?
>
> We have the same problem over with VT-d and IR.
>
> I don't think the firmware is not the right place, otherwise there would
> be an interface in hw to adjust that parameters. I think we should
> simply make sure that the qemu user cannot assign devices to those
> addresses as they are reserved for the platform devices (the HPET
> requires another ID), or even reserve the hole bus for the platform.


I understand, but it is the firmware that assign addresses, not QEMU/user.
We need a way to tell the firmware not to use a certain address range, we can 
do that
with fw_config I suppose.

Reserving a bus might be easier, we take the bus number out from host bridges 
CRS
ranges and each platform device can be assigned a slot. We would need to select 
the bus carefully
to not collide with other PCI hierarchies. Maybe bus 0xFF.

Thanks,
Marcel

> Jan