Re: [Qemu-devel] [RFC PATCH] pci: Use PCI aliases when determining device IOMMU address space

[Michael S. Tsirkin]

On Tue, Jul 23, 2019 at 11:26:18AM -0600, Alex Williamson wrote:
> On Mon, 1 Apr 2019 13:41:39 +0000
> "Singh, Brijesh" <address@hidden> wrote:
> 
> > Thanks for adding Alex.
> > 
> > Adding Suravee.
> > 
> > 
> > On 3/29/19 11:49 AM, Alex Williamson wrote:
> > > [Cc +Brijesh]
> > > 
> > > Hi Brijesh, will the change below require the IVRS to be updated to
> > > include aliases for all BDF ranges behind a conventional bridge?  I
> > > think the Linux code handles this regardless of the firmware provided
> > > aliases, but is it required per spec for the ACPI tables to include
> > > bridge aliases?  Thanks,
> > >   
> > 
> > We do need to includes aliases in ACPI table. We need to populate the
> > IVHD type 0x43 and 0x4 for alias range start and end. I believe host
> > IVRS would contain similar information.
> > 
> > Suravee, please correct me if I am missing something?
> 
> I finally found some time to investigate this a little further, yes the
> types mentioned are correct for defining start and end of an alias
> range.  The challenge here is that these entries require a DeviceID,
> which is defined as a BDF, AIUI.  The IVRS is created in QEMU, but bus
> numbers are defined by the guest firmware, and potentially redefined by
> the guest OS.  This makes it non-trivial to insert a few IVHDs into the
> IVRS to describe alias ranges.  I'm wondering if the solution here is
> to define a new linker-loader command that would instruct the guest to
> write a bus number byte to a given offset for a described device.
> These commands would be inserted before the checksum command, such that
> these bus number updates are calculated as part of the checksum.
> 
> I'm imagining the command format would need to be able to distinguish
> between the actual bus number of a described device, the secondary bus
> number of the device, and the subordinate bus number of the device.
> For describing the device, I'm envisioning stealing from the DMAR
> definition, which already includes a bus number invariant mechanism to
> describe a device, starting with a segment and root bus, follow a chain
> of devfns to get to the target device.  Therefore the guest firmware
> would follow the path to the described device, pick the desired bus
> number, and write it to the indicated table offset.
> 
> Does this seem like a reasonable approach?  Better ideas?  I'm not
> thrilled with the increased scope demanded by IVRS support, but so long
> as we have an AMD IOMMU model, I don't see how to avoid it.  Thanks,
> 
> Alex

We generally just got the bus #s as programmed into virtual
bridges/devices and had qemu program them into acpi
tables.

If guest os changes bus#s it's responsible for updating acpi
tables :)


> > > On Tue, 26 Mar 2019 16:55:19 -0600
> > > Alex Williamson <address@hidden> wrote:
> > >   
> > >> Conventional PCI buses pre-date requester IDs.  An IOMMU cannot
> > >> distinguish by devfn & bus between devices in a conventional PCI
> > >> topology and therefore we cannot assign them separate AddressSpaces.
> > >> By taking this requester ID aliasing into account, QEMU better matches
> > >> the bare metal behavior and restrictions, and enables shared
> > >> AddressSpace configurations that are otherwise not possible with
> > >> guest IOMMU support.
> > >>
> > >> For the latter case, given any example where an IOMMU group on the
> > >> host includes multiple devices:
> > >>
> > >>    $ ls  /sys/kernel/iommu_groups/1/devices/
> > >>    0000:00:01.0  0000:01:00.0  0000:01:00.1
> > >>
> > >> If we incorporate a vIOMMU into the VM configuration, we're restricted
> > >> that we can only assign one of the endpoints to the guest because a
> > >> second endpoint will attempt to use a different AddressSpace.  VFIO
> > >> only supports IOMMU group level granularity at the container level,
> > >> preventing this second endpoint from being assigned:
> > >>
> > >> 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
> > >>
> > >> However, when QEMU incorporates proper aliasing, we can make use of a
> > >> PCIe-to-PCI bridge to mask the requester ID, resulting in a hack that
> > >> provides the downstream devices with the same AddressSpace, ex:
> > >>
> > >> 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
> > >>
> > >> While the utility of this hack may be limited, this AddressSpace
> > >> aliasing is the correct behavior for QEMU to emulate bare metal.
> > >>
> > >> Signed-off-by: Alex Williamson <address@hidden>
> > >> ---
> > >>   hw/pci/pci.c |   33 +++++++++++++++++++++++++++++++--
> > >>   1 file changed, 31 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > >> index 35451c1e9987..38467e676f1f 100644
> > >> --- a/hw/pci/pci.c
> > >> +++ b/hw/pci/pci.c
> > >> @@ -2594,12 +2594,41 @@ 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);
> > >> +        PCIBus *parent_bus = pci_get_bus(iommu_bus->parent_dev);
> > >> +
> > >> +        /*
> > >> +         * Determine which requester ID alias should be used for the 
> > >> device
> > >> +         * based on the PCI topology.  There are no requester IDs on 
> > >> convetional
> > >> +         * PCI buses, therefore we push the alias up to the parent on 
> > >> each non-
> > >> +         * express bus.  Which alias we use depends on whether this is 
> > >> a legacy
> > >> +         * PCI bridge or PCIe-to-PCI/X bridge as in chapter 2.3 of the 
> > >> PCIe-to-
> > >> +         * PCI bridge spec.  Note that we cannot use pci_requester_id() 
> > >> here
> > >> +         * because the resulting BDF depends on the secondary bridge 
> > >> register
> > >> +         * programming.  We also cannot lookup the PCIBus from the bus 
> > >> number
> > >> +         * at this point for the iommu_fn.  Also, requester_id_cache is 
> > >> the
> > >> +         * alias to the root bus, which is usually, but not necessarily 
> > >> always
> > >> +         * where we'll find our iommu_fn.
> > >> +         */
> > >> +        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;
> > >>   }
> > >>  
> > >