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

[Jan Kiszka]

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.

Jan

signature.asc
Description: OpenPGP digital signature