[Qemu-devel] [for-4.2 PATCH 0/2] PCI DMA alias support

[Qemu-devel] [for-4.2 PATCH 0/2] PCI DMA alias support

[Alex Williamson]

Please see patch 1/ for the motivation and utility of this series.
This v1 submission improves on the previous RFC with revised commit
logs, comments, and more testing, and the missing IVRS support for DMA
alias ranges is now included.  Testing has been done with Linux guests
with both SeaBIOS and OVMF with configurations of intel-iommu and
amd-iommu.  Intel-iommu testing includes device assignment, amd-iommu
is necessarily limited to emulated devices with interrupt remapping
disabled and iommu=pt in the guest (enabling interrupt remapping or
disabling guest passthrough mode fails to work regardless of this
series).  This series is NOT intended for QEMU v4.1.  Thanks,

Alex

---

Alex Williamson (2):
      pci: Use PCI aliases when determining device IOMMU address space
      hw/i386: AMD-Vi IVRS DMA alias support


 hw/i386/acpi-build.c |  127 +++++++++++++++++++++++++++++++++++++++++++++++---
 hw/pci/pci.c         |   43 ++++++++++++++++-
 2 files changed, 160 insertions(+), 10 deletions(-)

[Qemu-devel] [for-4.2 PATCH 1/2] pci: Use PCI aliases when determining device IOMMU address space

[Alex Williamson]

PCIe requester IDs are used by modern IOMMUs to differentiate devices
in order to provide a unique IOVA address space per device.  These
requester IDs are composed of the bus/device/function (BDF) of the
requesting device.  Conventional PCI pre-dates this concept and is
simply a shared parallel bus where transactions are claimed by
decoding target ranges rather than the packetized, point-to-point
mechanisms of PCI-express.  In order to interface conventional PCI
to PCIe, the PCIe-to-PCI bridge creates and accepts packetized
transactions on behalf of all downstream devices, using one of two
potential forms of a requester ID relating to the bridge itself or its
subordinate bus.  All downstream devices are therefore aliased by the
bridge's requester ID and it's not possible for the IOMMU to create
unique IOVA spaces for devices downstream of such buses.

At least that's how it works on bare metal.  Until now point we've
ignored this nuance of vIOMMU support in QEMU, creating a unique
AddressSpace per device regardless of the virtual bus topology.

Aside from simply being true to bare metal behavior, there are aspects
of a shared address space that we can use to our advantage when
designing a VM.  For instance, a PCI device assignment scenario where
we have the following IOMMU group on the host system:

  $ ls  /sys/kernel/iommu_groups/1/devices/
  0000:00:01.0  0000:01:00.0  0000:01:00.1

An IOMMU group is considered the smallest set of devices which are
fully DMA isolated from other devices by the IOMMU.  In this case the
root port at 00:01.0 does not guarantee that it prevents peer to peer
traffic between the endpoints on bus 01: and the devices are therefore
grouped together.  VFIO considers an IOMMU group to be the smallest
unit of device ownership and allows only a single shared IOVA space
per group due to the limitations of the isolation.

Therefore, if we attempt to create the following VM, we get an error:

qemu-system-x86_64 -machine q35... \
  -device intel-iommu,intremap=on \
  -device pcie-root-port,addr=1e.0,id=pcie.1 \
  -device vfio-pci,host=1:00.0,bus=pcie.1,addr=0.0,multifunction=on \
  -device vfio-pci,host=1:00.1,bus=pcie.1,addr=0.1

qemu-system-x86_64: -device vfio-pci,host=1:00.1,bus=pcie.1,addr=0.1: vfio \
0000:01:00.1: group 1 used in multiple address spaces

VFIO only allows a single IOVA space (AddressSpace) for both devices,
but we've placed them into a topology where the vIOMMU expects a
separate AddressSpace for each device.  On bare metal we know that
a conventional PCI bus would provide the sort of aliasing we need
here, forcing the IOMMU to consider these devices to be part of a
single shared IOVA space.  The support provided here does the same
for QEMU, such that we can create a conventional PCI topology to
expose equivalent AddressSpace sharing requirements to the VM:

qemu-system-x86_64 -machine q35... \
  -device intel-iommu,intremap=on \
  -device pcie-pci-bridge,addr=1e.0,id=pci.1 \
  -device vfio-pci,host=1:00.0,bus=pci.1,addr=1.0,multifunction=on \
  -device vfio-pci,host=1:00.1,bus=pci.1,addr=1.1

There are pros and cons to this configuration; it's not necessarily
recommended, it's simply a tool we can use to create configurations
which may provide additional functionality in spite of host hardware
limitations or as a benefit to the guest configuration or resource
usage.  An incomplete list of pros and cons:

Cons:
 a) Extended PCI configuration space is unavailable to devices
    downstream of a conventional PCI bus.  The degree to which this
    is a drawback depends on the device and guest drivers.
 b) Applying this topology to devices which are already isolated by
    the host IOMMU (singleton IOMMU groups) will result in devices
    which appear to be non-isolated to the VM (non-singleton groups).
    This can limit configurations within the guest, such as userspace
    drivers or nested device assignment.

Pros:
 a) QEMU better emulates bare metal.
 b) Configurations as above are now possible.
 c) Host IOMMU resources and VM locked memory requirements are reduced
    in vIOMMU configurations due to shared IOMMU domains on the host
    and avoidance of duplicate locked memory accounting.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/pci/pci.c |   43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 8076a80ab3a9..b550741e88a3 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2610,12 +2610,49 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
 {
     PCIBus *bus = pci_get_bus(dev);
     PCIBus *iommu_bus = bus;
+    uint8_t devfn = dev->devfn;
 
-    while(iommu_bus && !iommu_bus->iommu_fn && iommu_bus->parent_dev) {
-        iommu_bus = pci_get_bus(iommu_bus->parent_dev);
+    while (iommu_bus && !iommu_bus->iommu_fn && iommu_bus->parent_dev) {
+        PCIBus *parent_bus = pci_get_bus(iommu_bus->parent_dev);
+
+        /*
+         * The requester ID of the provided device may be aliased, as seen from
+         * the IOMMU, due to topology limitations.  The IOMMU relies on a
+         * requester ID to provide a unique AddressSpace for devices, but
+         * conventional PCI buses pre-date such concepts.  Instead, the PCIe-
+         * to-PCI bridge creates and accepts transactions on behalf of down-
+         * stream devices.  When doing so, all downstream devices are masked
+         * (aliased) behind a single requester ID.  The requester ID used
+         * depends on the format of the bridge devices.  Proper PCIe-to-PCI
+         * bridges, with a PCIe capability indicating such, follow the
+         * guidelines of chapter 2.3 of the PCIe-to-PCI/X bridge specification,
+         * where the bridge uses the seconary bus as the bridge portion of the
+         * requester ID and devfn of 00.0.  For other bridges, typically those
+         * found on the root complex such as the dmi-to-pci-bridge, we follow
+         * the convention of typical bare-metal hardware, which uses the
+         * requester ID of the bridge itself.  There are device specific
+         * exceptions to these rules, but these are the defaults that the
+         * Linux kernel uses when determining DMA aliases itself and believed
+         * to be true for the bare metal equivalents of the devices emulated
+         * in QEMU.
+         */
+        if (!pci_bus_is_express(iommu_bus)) {
+            PCIDevice *parent = iommu_bus->parent_dev;
+
+            if (pci_is_express(parent) &&
+                pcie_cap_get_type(parent) == PCI_EXP_TYPE_PCI_BRIDGE) {
+                devfn = PCI_DEVFN(0, 0);
+                bus = iommu_bus;
+            } else {
+                devfn = parent->devfn;
+                bus = parent_bus;
+            }
+        }
+
+        iommu_bus = parent_bus;
     }
     if (iommu_bus && iommu_bus->iommu_fn) {
-        return iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque, dev->devfn);
+        return iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque, devfn);
     }
     return &address_space_memory;
 }

[Qemu-devel] [for-4.2 PATCH 2/2] hw/i386: AMD-Vi IVRS DMA alias support

[Alex Williamson]

When we account for DMA aliases in the PCI address space, we can no
longer use a single IVHD entry in the IVRS covering all devices.  We
instead need to walk the PCI bus and create alias ranges when we find
a conventional bus.  These alias ranges cannot overlap with a "Select
All" range (as currently implemented), so we also need to enumerate
each device with IVHD entries.

Importantly, the IVHD entries used here include a Device ID, which is
simply the PCI BDF (Bus/Device/Function).  The guest firmware is
responsible for programming bus numbers, so the final revision of this
table depends on the update mechanism (acpi_build_update) to be called
after guest PCI enumeration.

For an example guest configuration of:

-+-[0000:40]---00.0-[41]----00.0  Intel Corporation 82574L Gigabit Network Connection
 \-[0000:00]-+-00.0  Intel Corporation 82G33/G31/P35/P31 Express DRAM Controller
             +-01.0  Device 1234:1111
             +-02.0-[01]----00.0  Intel Corporation 82574L Gigabit Network Connection
             +-02.1-[02]----00.0  Red Hat, Inc. QEMU XHCI Host Controller
             +-02.2-[03]--
             +-02.3-[04]--
             +-02.4-[05]--
             +-02.5-[06-09]----00.0-[07-09]--+-00.0-[08]--
             |                               \-01.0-[09]----00.0  Intel Corporation 82574L Gigabit Network Connection
             +-02.6-[0a-0c]----00.0-[0b-0c]--+-01.0-[0c]--
             |                               \-03.0  Intel Corporation 82540EM Gigabit Ethernet Controller
             +-02.7-[0d]----0e.0  Intel Corporation 82540EM Gigabit Ethernet Controller
             +-03.0  Red Hat, Inc. QEMU PCIe Expander bridge
             +-04.0  Advanced Micro Devices, Inc. [AMD] Device 0020
             +-1f.0  Intel Corporation 82801IB (ICH9) LPC Interface Controller
             +-1f.2  Intel Corporation 82801IR/IO/IH (ICH9R/DO/DH) 6 port SATA Controller [AHCI mode]
             \-1f.3  Intel Corporation 82801I (ICH9 Family) SMBus Controller

Where we have:

00:02.7 PCI bridge: Intel Corporation 82801 PCI Bridge
 (dmi-to-pci-bridge)
00:03.0 Host bridge: Red Hat, Inc. QEMU PCIe Expander bridge
 (pcie-expander-bus)
06:00.0 PCI bridge: Texas Instruments XIO3130 PCI Express Switch (Upstream)
 (pcie-switch-upstream-port)
07:00.0 PCI bridge: Texas Instruments XIO3130 PCI Express Switch (Downstream)
 (pcie-switch-downstream-port)
07:01.0 PCI bridge: Texas Instruments XIO3130 PCI Express Switch (Downstream)
 (pcie-switch-downstream-port)
0a:00.0 PCI bridge: Red Hat, Inc. Device 000e
 (pcie-to-pci-bridge)

The following IVRS table is produced:

AMD-Vi: Using IVHD type 0x10
AMD-Vi: device: 00:04.0 cap: 0040 seg: 0 flags: d1 info 0000
AMD-Vi:        mmio-addr: 00000000fed80000
AMD-Vi:   DEV_SELECT			 devid: 40:00.0 flags: 00
AMD-Vi:   DEV_SELECT_RANGE_START	 devid: 41:00.0 flags: 00
AMD-Vi:   DEV_RANGE_END		 devid: 41:1f.7
AMD-Vi:   DEV_SELECT			 devid: 00:00.0 flags: 00
AMD-Vi:   DEV_SELECT			 devid: 00:01.0 flags: 00
AMD-Vi:   DEV_SELECT			 devid: 00:02.0 flags: 00
AMD-Vi:   DEV_SELECT_RANGE_START	 devid: 01:00.0 flags: 00
AMD-Vi:   DEV_RANGE_END		 devid: 01:1f.7
AMD-Vi:   DEV_SELECT			 devid: 00:02.1 flags: 00
AMD-Vi:   DEV_SELECT_RANGE_START	 devid: 02:00.0 flags: 00
AMD-Vi:   DEV_RANGE_END		 devid: 02:1f.7
AMD-Vi:   DEV_SELECT			 devid: 00:02.2 flags: 00
AMD-Vi:   DEV_SELECT_RANGE_START	 devid: 03:00.0 flags: 00
AMD-Vi:   DEV_RANGE_END		 devid: 03:1f.7
AMD-Vi:   DEV_SELECT			 devid: 00:02.3 flags: 00
AMD-Vi:   DEV_SELECT_RANGE_START	 devid: 04:00.0 flags: 00
AMD-Vi:   DEV_RANGE_END		 devid: 04:1f.7
AMD-Vi:   DEV_SELECT			 devid: 00:02.4 flags: 00
AMD-Vi:   DEV_SELECT_RANGE_START	 devid: 05:00.0 flags: 00
AMD-Vi:   DEV_RANGE_END		 devid: 05:1f.7
AMD-Vi:   DEV_SELECT			 devid: 00:02.5 flags: 00
AMD-Vi:   DEV_SELECT			 devid: 06:00.0 flags: 00
AMD-Vi:   DEV_SELECT			 devid: 07:00.0 flags: 00
AMD-Vi:   DEV_SELECT_RANGE_START	 devid: 08:00.0 flags: 00
AMD-Vi:   DEV_RANGE_END		 devid: 08:1f.7
AMD-Vi:   DEV_SELECT			 devid: 07:01.0 flags: 00
AMD-Vi:   DEV_SELECT_RANGE_START	 devid: 09:00.0 flags: 00
AMD-Vi:   DEV_RANGE_END		 devid: 09:1f.7
AMD-Vi:   DEV_SELECT			 devid: 00:02.6 flags: 00
AMD-Vi:   DEV_SELECT			 devid: 0a:00.0 flags: 00
AMD-Vi:   DEV_ALIAS_RANGE		 devid: 0b:00.0 flags: 00 devid_to: 0b:00.0
AMD-Vi:   DEV_RANGE_END		 devid: 0c:1f.7
AMD-Vi:   DEV_SELECT			 devid: 00:02.7 flags: 00
AMD-Vi:   DEV_ALIAS_RANGE		 devid: 0d:00.0 flags: 00 devid_to: 00:02.7
AMD-Vi:   DEV_RANGE_END		 devid: 0d:1f.7
AMD-Vi:   DEV_SELECT			 devid: 00:03.0 flags: 00
AMD-Vi:   DEV_SELECT			 devid: 00:04.0 flags: 00
AMD-Vi:   DEV_SELECT			 devid: 00:1f.0 flags: 00
AMD-Vi:   DEV_SELECT			 devid: 00:1f.2 flags: 00
AMD-Vi:   DEV_SELECT			 devid: 00:1f.3 flags: 00

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/i386/acpi-build.c |  127 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 120 insertions(+), 7 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index d281ffa89e3a..7aab00ac470d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2484,12 +2484,105 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker)
  */
 #define IOAPIC_SB_DEVID   (uint64_t)PCI_BUILD_BDF(0, PCI_DEVFN(0x14, 0))
 
+/*
+ * Insert IVHD entry for device and recurse, insert alias, or insert range as
+ * necessary for the PCI topology.
+ */
+static void
+insert_ivhd(PCIBus *bus, PCIDevice *dev, void *opaque)
+{
+    GArray *table_data = opaque;
+    uint32_t entry;
+
+    /* "Select" IVHD entry, type 0x2 */
+    entry = PCI_BUILD_BDF(pci_bus_num(bus), dev->devfn) << 8 | 0x2;
+    build_append_int_noprefix(table_data, entry, 4);
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
+        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
+        uint8_t sec = pci_bus_num(sec_bus);
+        uint8_t sub = dev->config[PCI_SUBORDINATE_BUS];
+
+        if (pci_bus_is_express(sec_bus)) {
+            /*
+             * Walk the bus if there are subordinates, otherwise use a range
+             * to cover an entire leaf bus.  We could potentially also use a
+             * range for traversed buses, but we'd need to take care not to
+             * create both Select and Range entries covering the same device.
+             * This is easier and potentially more compact.
+             *
+             * An example bare metal system seems to use Select entries for
+             * root ports without a slot (ie. built-ins) and Range entries
+             * when there is a slot.  The same system also only hard-codes
+             * the alias range for an onboard PCIe-to-PCI bridge, apparently
+             * making no effort to support nested bridges.  We attempt to
+             * be more thorough here.
+             */
+            if (sec == sub) { /* leaf bus */
+                /* "Start of Range" IVHD entry, type 0x3 */
+                entry = PCI_BUILD_BDF(sec, PCI_DEVFN(0, 0)) << 8 | 0x3;
+                build_append_int_noprefix(table_data, entry, 4);
+                /* "End of Range" IVHD entry, type 0x4 */
+                entry = PCI_BUILD_BDF(sub, PCI_DEVFN(31, 7)) << 8 | 0x4;
+                build_append_int_noprefix(table_data, entry, 4);
+            } else {
+                pci_for_each_device(sec_bus, sec, insert_ivhd, table_data);
+            }
+        } else {
+            /*
+             * If the secondary bus is conventional, then we need to create an
+             * Alias range for everything downstream.  The range covers the
+             * first devfn on the secondary bus to the last devfn on the
+             * subordinate bus.  The alias target depends on legacy versus
+             * express bridges, just as in pci_device_iommu_address_space().
+             * DeviceIDa vs DeviceIDb as per the AMD IOMMU spec.
+             */
+            uint16_t dev_id_a, dev_id_b;
+
+            dev_id_a = PCI_BUILD_BDF(sec, PCI_DEVFN(0, 0));
+
+            if (pci_is_express(dev) &&
+                pcie_cap_get_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE) {
+                dev_id_b = dev_id_a;
+            } else {
+                dev_id_b = PCI_BUILD_BDF(pci_bus_num(bus), dev->devfn);
+            }
+
+            /* "Alias Start of Range" IVHD entry, type 0x43, 8 bytes */
+            build_append_int_noprefix(table_data, dev_id_a << 8 | 0x43, 4);
+            build_append_int_noprefix(table_data, dev_id_b << 8 | 0x0, 4);
+
+            /* "End of Range" IVHD entry, type 0x4 */
+            entry = PCI_BUILD_BDF(sub, PCI_DEVFN(31, 7)) << 8 | 0x4;
+            build_append_int_noprefix(table_data, entry, 4);
+        }
+    }
+}
+
+/* For all PCI host bridges, walk and insert IVHD entries */
+static int
+ivrs_host_bridges(Object *obj, void *opaque)
+{
+    GArray *ivhd_blob = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
+        PCIBus *bus = PCI_HOST_BRIDGE(obj)->bus;
+
+        if (bus) {
+            pci_for_each_device(bus, pci_bus_num(bus), insert_ivhd, ivhd_blob);
+        }
+    }
+
+    return 0;
+}
+
 static void
 build_amd_iommu(GArray *table_data, BIOSLinker *linker)
 {
-    int ivhd_table_len = 28;
+    int ivhd_table_len = 24;
     int iommu_start = table_data->len;
     AMDVIState *s = AMD_IOMMU_DEVICE(x86_iommu_get_default());
+    GArray *ivhd_blob = g_array_new(false, true, 1);
 
     /* IVRS header */
     acpi_data_push(table_data, sizeof(AcpiTableHeader));
@@ -2510,6 +2603,27 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
                              (1UL << 7),  /* PPRSup       */
                              1);
 
+    /*
+     * A PCI bus walk, for each PCI host bridge, is necessary to create a
+     * complete set of IVHD entries.  Do this into a separate blob so that we
+     * can calculate the total IVRS table length here and then append the new
+     * blob further below.  Fall back to an entry covering all devices, which
+     * is sufficient when no aliases are present.
+     */
+    object_child_foreach_recursive(object_get_root(),
+                                   ivrs_host_bridges, ivhd_blob);
+
+    if (!ivhd_blob->len) {
+        /*
+         *   Type 1 device entry reporting all devices
+         *   These are 4-byte device entries currently reporting the range of
+         *   Refer to Spec - Table 95:IVHD Device Entry Type Codes(4-byte)
+         */
+        build_append_int_noprefix(ivhd_blob, 0x0000001, 4);
+    }
+
+    ivhd_table_len += ivhd_blob->len;
+
     /*
      * When interrupt remapping is supported, we add a special IVHD device
      * for type IO-APIC.
@@ -2517,6 +2631,7 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
     if (x86_iommu_ir_supported(x86_iommu_get_default())) {
         ivhd_table_len += 8;
     }
+
     /* IVHD length */
     build_append_int_noprefix(table_data, ivhd_table_len, 2);
     /* DeviceID */
@@ -2536,12 +2651,10 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
                              (1UL << 2)   | /* GTSup  */
                              (1UL << 6),    /* GASup  */
                              4);
-    /*
-     *   Type 1 device entry reporting all devices
-     *   These are 4-byte device entries currently reporting the range of
-     *   Refer to Spec - Table 95:IVHD Device Entry Type Codes(4-byte)
-     */
-    build_append_int_noprefix(table_data, 0x0000001, 4);
+
+    /* IVHD entries as found above */
+    g_array_append_vals(table_data, ivhd_blob->data, ivhd_blob->len);
+    g_array_free(ivhd_blob, TRUE);
 
     /*
      * Add a special IVHD device type.

Re: [Qemu-devel] [for-4.2 PATCH 2/2] hw/i386: AMD-Vi IVRS DMA alias support

[Peter Xu]

On Fri, Jul 26, 2019 at 06:55:53PM -0600, Alex Williamson wrote:
> When we account for DMA aliases in the PCI address space, we can no
> longer use a single IVHD entry in the IVRS covering all devices.  We
> instead need to walk the PCI bus and create alias ranges when we find
> a conventional bus.  These alias ranges cannot overlap with a "Select
> All" range (as currently implemented), so we also need to enumerate
> each device with IVHD entries.
> 
> Importantly, the IVHD entries used here include a Device ID, which is
> simply the PCI BDF (Bus/Device/Function).  The guest firmware is
> responsible for programming bus numbers, so the final revision of this
> table depends on the update mechanism (acpi_build_update) to be called
> after guest PCI enumeration.

Ouch... so the ACPI build procedure is after those guest PCI code!
Could I ask how do you find this? :) It seems much easier for sure
this way...

This looks very nice to me already, though I still have got a few
questions, please see below.

[...]

> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
> +        uint8_t sec = pci_bus_num(sec_bus);
> +        uint8_t sub = dev->config[PCI_SUBORDINATE_BUS];
> +
> +        if (pci_bus_is_express(sec_bus)) {
> +            /*
> +             * Walk the bus if there are subordinates, otherwise use a range
> +             * to cover an entire leaf bus.  We could potentially also use a
> +             * range for traversed buses, but we'd need to take care not to
> +             * create both Select and Range entries covering the same device.
> +             * This is easier and potentially more compact.
> +             *
> +             * An example bare metal system seems to use Select entries for
> +             * root ports without a slot (ie. built-ins) and Range entries
> +             * when there is a slot.  The same system also only hard-codes
> +             * the alias range for an onboard PCIe-to-PCI bridge, apparently
> +             * making no effort to support nested bridges.  We attempt to
> +             * be more thorough here.
> +             */
> +            if (sec == sub) { /* leaf bus */
> +                /* "Start of Range" IVHD entry, type 0x3 */
> +                entry = PCI_BUILD_BDF(sec, PCI_DEVFN(0, 0)) << 8 | 0x3;
> +                build_append_int_noprefix(table_data, entry, 4);
> +                /* "End of Range" IVHD entry, type 0x4 */
> +                entry = PCI_BUILD_BDF(sub, PCI_DEVFN(31, 7)) << 8 | 0x4;
> +                build_append_int_noprefix(table_data, entry, 4);
> +            } else {
> +                pci_for_each_device(sec_bus, sec, insert_ivhd, table_data);
> +            }
> +        } else {
> +            /*
> +             * If the secondary bus is conventional, then we need to create an
> +             * Alias range for everything downstream.  The range covers the
> +             * first devfn on the secondary bus to the last devfn on the
> +             * subordinate bus.  The alias target depends on legacy versus
> +             * express bridges, just as in pci_device_iommu_address_space().
> +             * DeviceIDa vs DeviceIDb as per the AMD IOMMU spec.
> +             */
> +            uint16_t dev_id_a, dev_id_b;
> +
> +            dev_id_a = PCI_BUILD_BDF(sec, PCI_DEVFN(0, 0));
> +
> +            if (pci_is_express(dev) &&
> +                pcie_cap_get_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE) {
> +                dev_id_b = dev_id_a;
> +            } else {
> +                dev_id_b = PCI_BUILD_BDF(pci_bus_num(bus), dev->devfn);
> +            }
> +
> +            /* "Alias Start of Range" IVHD entry, type 0x43, 8 bytes */
> +            build_append_int_noprefix(table_data, dev_id_a << 8 | 0x43, 4);
> +            build_append_int_noprefix(table_data, dev_id_b << 8 | 0x0, 4);
> +
> +            /* "End of Range" IVHD entry, type 0x4 */
> +            entry = PCI_BUILD_BDF(sub, PCI_DEVFN(31, 7)) << 8 | 0x4;
> +            build_append_int_noprefix(table_data, entry, 4);
> +        }

We've implmented the similar logic for multiple times:

  - When we want to do DMA (pci_requester_id)
  - When we want to fetch the DMA address space (the previous patch)
  - When we fill in the AMD ACPI table (this patch)

Do you think we can generalize them somehow?  I'm thinking how about
we directly fetch RID in the 2nd/3rd use case using pci_requester_id()
(which existed already) and simply use it?

[...]

> +    /*
> +     * A PCI bus walk, for each PCI host bridge, is necessary to create a
> +     * complete set of IVHD entries.  Do this into a separate blob so that we
> +     * can calculate the total IVRS table length here and then append the new
> +     * blob further below.  Fall back to an entry covering all devices, which
> +     * is sufficient when no aliases are present.
> +     */
> +    object_child_foreach_recursive(object_get_root(),
> +                                   ivrs_host_bridges, ivhd_blob);
> +
> +    if (!ivhd_blob->len) {
> +        /*
> +         *   Type 1 device entry reporting all devices
> +         *   These are 4-byte device entries currently reporting the range of
> +         *   Refer to Spec - Table 95:IVHD Device Entry Type Codes(4-byte)
> +         */
> +        build_append_int_noprefix(ivhd_blob, 0x0000001, 4);
> +    }

Is there a real use case for ivhd_blob->len==0?

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [for-4.2 PATCH 2/2] hw/i386: AMD-Vi IVRS DMA alias support

[Alex Williamson]

On Mon, 29 Jul 2019 16:26:46 +0800
Peter Xu <zhexu@redhat.com> wrote:

> On Fri, Jul 26, 2019 at 06:55:53PM -0600, Alex Williamson wrote:
> > When we account for DMA aliases in the PCI address space, we can no
> > longer use a single IVHD entry in the IVRS covering all devices.  We
> > instead need to walk the PCI bus and create alias ranges when we find
> > a conventional bus.  These alias ranges cannot overlap with a "Select
> > All" range (as currently implemented), so we also need to enumerate
> > each device with IVHD entries.
> > 
> > Importantly, the IVHD entries used here include a Device ID, which is
> > simply the PCI BDF (Bus/Device/Function).  The guest firmware is
> > responsible for programming bus numbers, so the final revision of this
> > table depends on the update mechanism (acpi_build_update) to be called
> > after guest PCI enumeration.  
> 
> Ouch... so the ACPI build procedure is after those guest PCI code!
> Could I ask how do you find this? :) It seems much easier for sure
> this way...

I believe this is what MST was referring to with the MCFG update,
acpi_build() is called from both acpi_setup() and acpi_build_update(),
the latter is setup in numerous places to be called via a mechanism
that re-writes the table on certain guest actions.  For instance
acpi_add_rom_blob() passes this function as a callback which turns into
a select_cb in fw_cfg, such that (aiui) the tables are updated before
the guest reads them.  I added some fprintfs in the PCI config write
path to confirm that the update callback occurs after PCI enumeration
for both SeaBIOS and OVMF.  Since we seem to have other dependencies on
this ordering elsewhere, I don't think that the IVRS update is unique
in this regard.
 
> This looks very nice to me already, though I still have got a few
> questions, please see below.
> 
> [...]
> 
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
> > +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
> > +        uint8_t sec = pci_bus_num(sec_bus);
> > +        uint8_t sub = dev->config[PCI_SUBORDINATE_BUS];
> > +
> > +        if (pci_bus_is_express(sec_bus)) {
> > +            /*
> > +             * Walk the bus if there are subordinates, otherwise use a range
> > +             * to cover an entire leaf bus.  We could potentially also use a
> > +             * range for traversed buses, but we'd need to take care not to
> > +             * create both Select and Range entries covering the same device.
> > +             * This is easier and potentially more compact.
> > +             *
> > +             * An example bare metal system seems to use Select entries for
> > +             * root ports without a slot (ie. built-ins) and Range entries
> > +             * when there is a slot.  The same system also only hard-codes
> > +             * the alias range for an onboard PCIe-to-PCI bridge, apparently
> > +             * making no effort to support nested bridges.  We attempt to
> > +             * be more thorough here.
> > +             */
> > +            if (sec == sub) { /* leaf bus */
> > +                /* "Start of Range" IVHD entry, type 0x3 */
> > +                entry = PCI_BUILD_BDF(sec, PCI_DEVFN(0, 0)) << 8 | 0x3;
> > +                build_append_int_noprefix(table_data, entry, 4);
> > +                /* "End of Range" IVHD entry, type 0x4 */
> > +                entry = PCI_BUILD_BDF(sub, PCI_DEVFN(31, 7)) << 8 | 0x4;
> > +                build_append_int_noprefix(table_data, entry, 4);
> > +            } else {
> > +                pci_for_each_device(sec_bus, sec, insert_ivhd, table_data);
> > +            }
> > +        } else {
> > +            /*
> > +             * If the secondary bus is conventional, then we need to create an
> > +             * Alias range for everything downstream.  The range covers the
> > +             * first devfn on the secondary bus to the last devfn on the
> > +             * subordinate bus.  The alias target depends on legacy versus
> > +             * express bridges, just as in pci_device_iommu_address_space().
> > +             * DeviceIDa vs DeviceIDb as per the AMD IOMMU spec.
> > +             */
> > +            uint16_t dev_id_a, dev_id_b;
> > +
> > +            dev_id_a = PCI_BUILD_BDF(sec, PCI_DEVFN(0, 0));
> > +
> > +            if (pci_is_express(dev) &&
> > +                pcie_cap_get_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE) {
> > +                dev_id_b = dev_id_a;
> > +            } else {
> > +                dev_id_b = PCI_BUILD_BDF(pci_bus_num(bus), dev->devfn);
> > +            }
> > +
> > +            /* "Alias Start of Range" IVHD entry, type 0x43, 8 bytes */
> > +            build_append_int_noprefix(table_data, dev_id_a << 8 | 0x43, 4);
> > +            build_append_int_noprefix(table_data, dev_id_b << 8 | 0x0, 4);
> > +
> > +            /* "End of Range" IVHD entry, type 0x4 */
> > +            entry = PCI_BUILD_BDF(sub, PCI_DEVFN(31, 7)) << 8 | 0x4;
> > +            build_append_int_noprefix(table_data, entry, 4);
> > +        }  
> 
> We've implmented the similar logic for multiple times:
> 
>   - When we want to do DMA (pci_requester_id)
>   - When we want to fetch the DMA address space (the previous patch)
>   - When we fill in the AMD ACPI table (this patch)
> 
> Do you think we can generalize them somehow?  I'm thinking how about
> we directly fetch RID in the 2nd/3rd use case using pci_requester_id()
> (which existed already) and simply use it?

For this patch, I think we could use pci_requester_id() for dev_id_b
above, but we still need to walk the buses and devices, so it really
only simplifies the 'if (pci_is_express...' code block above, and
barely at that since we're at the point in the topology where such a
decision is made.  For the previous patch, pci_requester_id() returns a
BDF and that code is executed before bus numbers are programmed.  We
might still make use of requester_id_cache, but a different interface
would be necessary.  Also note how pci_req_id_cache_get() assumes we're
looking for the requester ID as seen from the root bus while
pci_device_iommu_address_space() is from the bus hosting iommu_fn.
While these are generally the same in practice, it's not required.  I'd
therefore tend to leave that to some future consolidation.  I can
update the comment to include that justification in the previous patch.

> > +    /*
> > +     * A PCI bus walk, for each PCI host bridge, is necessary to create a
> > +     * complete set of IVHD entries.  Do this into a separate blob so that we
> > +     * can calculate the total IVRS table length here and then append the new
> > +     * blob further below.  Fall back to an entry covering all devices, which
> > +     * is sufficient when no aliases are present.
> > +     */
> > +    object_child_foreach_recursive(object_get_root(),
> > +                                   ivrs_host_bridges, ivhd_blob);
> > +
> > +    if (!ivhd_blob->len) {
> > +        /*
> > +         *   Type 1 device entry reporting all devices
> > +         *   These are 4-byte device entries currently reporting the range of
> > +         *   Refer to Spec - Table 95:IVHD Device Entry Type Codes(4-byte)
> > +         */
> > +        build_append_int_noprefix(ivhd_blob, 0x0000001, 4);
> > +    }  
> 
> Is there a real use case for ivhd_blob->len==0?

It was mostly paranoia, but I believe it's really only an Intel
convention that the PCI host bridge appears as a device on the bus.  It
seems possible that we could have a host bridge with no devices, in
which case we'd insert this select-all entry to make the IVRS valid.
Perhaps in combination with AMD exposing their IOMMU as a device on the
PCI bus this is not really an issue, but it's a trivial safety net.
Thanks,

Alex

Re: [Qemu-devel] [for-4.2 PATCH 2/2] hw/i386: AMD-Vi IVRS DMA alias support

[Peter Xu]

On Mon, Jul 29, 2019 at 01:04:41PM -0600, Alex Williamson wrote:
> On Mon, 29 Jul 2019 16:26:46 +0800
> Peter Xu <zhexu@redhat.com> wrote:
> 
> > On Fri, Jul 26, 2019 at 06:55:53PM -0600, Alex Williamson wrote:
> > > When we account for DMA aliases in the PCI address space, we can no
> > > longer use a single IVHD entry in the IVRS covering all devices.  We
> > > instead need to walk the PCI bus and create alias ranges when we find
> > > a conventional bus.  These alias ranges cannot overlap with a "Select
> > > All" range (as currently implemented), so we also need to enumerate
> > > each device with IVHD entries.
> > > 
> > > Importantly, the IVHD entries used here include a Device ID, which is
> > > simply the PCI BDF (Bus/Device/Function).  The guest firmware is
> > > responsible for programming bus numbers, so the final revision of this
> > > table depends on the update mechanism (acpi_build_update) to be called
> > > after guest PCI enumeration.  
> > 
> > Ouch... so the ACPI build procedure is after those guest PCI code!
> > Could I ask how do you find this? :) It seems much easier for sure
> > this way...
> 
> I believe this is what MST was referring to with the MCFG update,
> acpi_build() is called from both acpi_setup() and acpi_build_update(),
> the latter is setup in numerous places to be called via a mechanism
> that re-writes the table on certain guest actions.  For instance
> acpi_add_rom_blob() passes this function as a callback which turns into
> a select_cb in fw_cfg, such that (aiui) the tables are updated before
> the guest reads them.  I added some fprintfs in the PCI config write
> path to confirm that the update callback occurs after PCI enumeration
> for both SeaBIOS and OVMF.  Since we seem to have other dependencies on
> this ordering elsewhere, I don't think that the IVRS update is unique
> in this regard.

Agreed.

[...]

> > We've implmented the similar logic for multiple times:
> > 
> >   - When we want to do DMA (pci_requester_id)
> >   - When we want to fetch the DMA address space (the previous patch)
> >   - When we fill in the AMD ACPI table (this patch)
> > 
> > Do you think we can generalize them somehow?  I'm thinking how about
> > we directly fetch RID in the 2nd/3rd use case using pci_requester_id()
> > (which existed already) and simply use it?
> 
> For this patch, I think we could use pci_requester_id() for dev_id_b
> above, but we still need to walk the buses and devices, so it really
> only simplifies the 'if (pci_is_express...' code block above, and
> barely at that since we're at the point in the topology where such a
> decision is made.  For the previous patch, pci_requester_id() returns a
> BDF and that code is executed before bus numbers are programmed.  We
> might still make use of requester_id_cache, but a different interface
> would be necessary.  Also note how pci_req_id_cache_get() assumes we're
> looking for the requester ID as seen from the root bus while
> pci_device_iommu_address_space() is from the bus hosting iommu_fn.
> While these are generally the same in practice, it's not required.  I'd
> therefore tend to leave that to some future consolidation.  I can
> update the comment to include that justification in the previous patch.

Yes, we can work on top in the future if needed.  I see that Michael
already plan to merge this version, then it may not worth a repost for
the comment (unless there will be a repost, we could mark a TODO).

> 
> > > +    /*
> > > +     * A PCI bus walk, for each PCI host bridge, is necessary to create a
> > > +     * complete set of IVHD entries.  Do this into a separate blob so that we
> > > +     * can calculate the total IVRS table length here and then append the new
> > > +     * blob further below.  Fall back to an entry covering all devices, which
> > > +     * is sufficient when no aliases are present.
> > > +     */
> > > +    object_child_foreach_recursive(object_get_root(),
> > > +                                   ivrs_host_bridges, ivhd_blob);
> > > +
> > > +    if (!ivhd_blob->len) {
> > > +        /*
> > > +         *   Type 1 device entry reporting all devices
> > > +         *   These are 4-byte device entries currently reporting the range of
> > > +         *   Refer to Spec - Table 95:IVHD Device Entry Type Codes(4-byte)
> > > +         */
> > > +        build_append_int_noprefix(ivhd_blob, 0x0000001, 4);
> > > +    }  
> > 
> > Is there a real use case for ivhd_blob->len==0?
> 
> It was mostly paranoia, but I believe it's really only an Intel
> convention that the PCI host bridge appears as a device on the bus.  It
> seems possible that we could have a host bridge with no devices, in
> which case we'd insert this select-all entry to make the IVRS valid.
> Perhaps in combination with AMD exposing their IOMMU as a device on the
> PCI bus this is not really an issue, but it's a trivial safety net.
> Thanks,

That question was only for curiousity.  This code path will only
trigger when AMD vIOMMU is detected so I assume the IOMMU device
should always be there, but of course it won't hurt as a safety net.

Thanks for doing this!

-- 
Peter Xu

Re: [Qemu-devel] [for-4.2 PATCH 0/2] PCI DMA alias support

[Michael S. Tsirkin]

On Fri, Jul 26, 2019 at 06:55:27PM -0600, Alex Williamson wrote:
> Please see patch 1/ for the motivation and utility of this series.
> This v1 submission improves on the previous RFC with revised commit
> logs, comments, and more testing, and the missing IVRS support for DMA
> alias ranges is now included.  Testing has been done with Linux guests
> with both SeaBIOS and OVMF with configurations of intel-iommu and
> amd-iommu.  Intel-iommu testing includes device assignment, amd-iommu
> is necessarily limited to emulated devices with interrupt remapping
> disabled and iommu=pt in the guest (enabling interrupt remapping or
> disabling guest passthrough mode fails to work regardless of this
> series).  This series is NOT intended for QEMU v4.1.  Thanks,
> 
> Alex


series looks good to me.
pls ping when 4.1 is out and I'll queue it.

> ---
> 
> Alex Williamson (2):
>       pci: Use PCI aliases when determining device IOMMU address space
>       hw/i386: AMD-Vi IVRS DMA alias support
> 
> 
>  hw/i386/acpi-build.c |  127 +++++++++++++++++++++++++++++++++++++++++++++++---
>  hw/pci/pci.c         |   43 ++++++++++++++++-
>  2 files changed, 160 insertions(+), 10 deletions(-)

Re: [Qemu-devel] [for-4.2 PATCH 0/2] PCI DMA alias support

[Alex Williamson]

On Mon, 29 Jul 2019 15:15:29 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jul 26, 2019 at 06:55:27PM -0600, Alex Williamson wrote:
> > Please see patch 1/ for the motivation and utility of this series.
> > This v1 submission improves on the previous RFC with revised commit
> > logs, comments, and more testing, and the missing IVRS support for DMA
> > alias ranges is now included.  Testing has been done with Linux guests
> > with both SeaBIOS and OVMF with configurations of intel-iommu and
> > amd-iommu.  Intel-iommu testing includes device assignment, amd-iommu
> > is necessarily limited to emulated devices with interrupt remapping
> > disabled and iommu=pt in the guest (enabling interrupt remapping or
> > disabling guest passthrough mode fails to work regardless of this
> > series).  This series is NOT intended for QEMU v4.1.  Thanks,
> > 
> > Alex  
> 
> 
> series looks good to me.
> pls ping when 4.1 is out and I'll queue it.

Here's the requested ping :)  If you'd like a re-posting or comment
update, just say so.  I think Peter was ultimately satisfied enough to
not request a re-spin for comments alone.  Thanks,

Alex

> > ---
> > 
> > Alex Williamson (2):
> >       pci: Use PCI aliases when determining device IOMMU address space
> >       hw/i386: AMD-Vi IVRS DMA alias support
> > 
> > 
> >  hw/i386/acpi-build.c |  127 +++++++++++++++++++++++++++++++++++++++++++++++---
> >  hw/pci/pci.c         |   43 ++++++++++++++++-
> >  2 files changed, 160 insertions(+), 10 deletions(-)  

Re: [for-4.2 PATCH 0/2] PCI DMA alias support

[Peter Xu]

On Mon, Aug 19, 2019 at 03:23:16PM -0600, Alex Williamson wrote:
> On Mon, 29 Jul 2019 15:15:29 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Jul 26, 2019 at 06:55:27PM -0600, Alex Williamson wrote:
> > > Please see patch 1/ for the motivation and utility of this series.
> > > This v1 submission improves on the previous RFC with revised commit
> > > logs, comments, and more testing, and the missing IVRS support for DMA
> > > alias ranges is now included.  Testing has been done with Linux guests
> > > with both SeaBIOS and OVMF with configurations of intel-iommu and
> > > amd-iommu.  Intel-iommu testing includes device assignment, amd-iommu
> > > is necessarily limited to emulated devices with interrupt remapping
> > > disabled and iommu=pt in the guest (enabling interrupt remapping or
> > > disabling guest passthrough mode fails to work regardless of this
> > > series).  This series is NOT intended for QEMU v4.1.  Thanks,
> > > 
> > > Alex  
> > 
> > 
> > series looks good to me.
> > pls ping when 4.1 is out and I'll queue it.
> 
> Here's the requested ping :)  If you'd like a re-posting or comment
> update, just say so.  I think Peter was ultimately satisfied enough to
> not request a re-spin for comments alone.  Thanks,

Hi, Michael,

Do you plan to merge this series?  Or do you still wait for an ack?

In all cases:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu

Re: [for-4.2 PATCH 0/2] PCI DMA alias support

[Alex Williamson]

On Mon, 14 Oct 2019 10:59:57 +0800
Peter Xu <peterx@redhat.com> wrote:

> On Mon, Aug 19, 2019 at 03:23:16PM -0600, Alex Williamson wrote:
> > On Mon, 29 Jul 2019 15:15:29 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Fri, Jul 26, 2019 at 06:55:27PM -0600, Alex Williamson wrote:  
> > > > Please see patch 1/ for the motivation and utility of this series.
> > > > This v1 submission improves on the previous RFC with revised commit
> > > > logs, comments, and more testing, and the missing IVRS support for DMA
> > > > alias ranges is now included.  Testing has been done with Linux guests
> > > > with both SeaBIOS and OVMF with configurations of intel-iommu and
> > > > amd-iommu.  Intel-iommu testing includes device assignment, amd-iommu
> > > > is necessarily limited to emulated devices with interrupt remapping
> > > > disabled and iommu=pt in the guest (enabling interrupt remapping or
> > > > disabling guest passthrough mode fails to work regardless of this
> > > > series).  This series is NOT intended for QEMU v4.1.  Thanks,
> > > > 
> > > > Alex    
> > > 
> > > 
> > > series looks good to me.
> > > pls ping when 4.1 is out and I'll queue it.  
> > 
> > Here's the requested ping :)  If you'd like a re-posting or comment
> > update, just say so.  I think Peter was ultimately satisfied enough to
> > not request a re-spin for comments alone.  Thanks,  
> 
> Hi, Michael,
> 
> Do you plan to merge this series?  Or do you still wait for an ack?
> 
> In all cases:
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks Peter, somehow I had it in my head that MST has already queued
these, but it seems not.  There's been hardly any churn in these files
upstream, so please consider these patches in play.  Thanks,

Alex