Re: [PATCH v1 09/11] hw/pci: Introduce pci_device_set/unset_iommu_device()

[Michael S. Tsirkin]

On Tue, Mar 12, 2024 at 12:55:30PM -0400, Michael S. Tsirkin wrote:
> On Wed, Feb 28, 2024 at 11:58:58AM +0800, Zhenzhong Duan wrote:
> > From: Yi Liu <yi.l.liu@intel.com>
> > 
> > This adds pci_device_set/unset_iommu_device() to set/unset
> > HostIOMMUDevice for a given PCIe device. Caller of set
> > should fail if set operation fails.
> > 
> > Extract out pci_device_get_iommu_bus_devfn() to facilitate
> > implementation of pci_device_set/unset_iommu_device().
> > 
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> > ---
> >  include/hw/pci/pci.h | 38 ++++++++++++++++++++++++++-
> >  hw/pci/pci.c         | 62 +++++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 96 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index fa6313aabc..8fe6f746d7 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -3,6 +3,7 @@
> >  
> >  #include "exec/memory.h"
> >  #include "sysemu/dma.h"
> > +#include "sysemu/host_iommu_device.h"
> >  
> >  /* PCI includes legacy ISA access.  */
> >  #include "hw/isa/isa.h"
> > @@ -384,10 +385,45 @@ typedef struct PCIIOMMUOps {
> >       *
> >       * @devfn: device and function number
> >       */
> > -   AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int 
> > devfn);
> > +    AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int 
> > devfn);
> > +    /**
> > +     * @set_iommu_device: attach a HostIOMMUDevice to a vIOMMU
> > +     *
> > +     * Optional callback, if not implemented in vIOMMU, then vIOMMU can't
> > +     * retrieve host information from the associated HostIOMMUDevice.
> > +     *
> > +     * Return true if HostIOMMUDevice is attached, or else return false
> > +     * with errp set.
> > +     *
> > +     * @bus: the #PCIBus of the PCI device.
> > +     *
> > +     * @opaque: the data passed to pci_setup_iommu().
> > +     *
> > +     * @devfn: device and function number of the PCI device.
> > +     *
> > +     * @dev: the data structure representing host IOMMU device.
> > +     *
> > +     */
> > +    int (*set_iommu_device)(PCIBus *bus, void *opaque, int devfn,
> > +                            HostIOMMUDevice *dev, Error **errp);
> > +    /**
> > +     * @unset_iommu_device: detach a HostIOMMUDevice from a vIOMMU
> > +     *
> > +     * Optional callback.
> > +     *
> > +     * @bus: the #PCIBus of the PCI device.
> > +     *
> > +     * @opaque: the data passed to pci_setup_iommu().
> > +     *
> > +     * @devfn: device and function number of the PCI device.
> > +     */
> > +    void (*unset_iommu_device)(PCIBus *bus, void *opaque, int devfn);
> >  } PCIIOMMUOps;
> 
> 
> So I expected someone to implement these new callbacks but I see
> no implementation in this patchset.
> 
> Is this just infrastructure that will be used later?
> A bit hard to judge without a user ...
> 

Looked at the second part of the patchset now (dealing with VTD).
Let's continue the discussion there.

> 
> >  AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
> > +int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *base_dev,
> > +                                Error **errp);
> > +void pci_device_unset_iommu_device(PCIDevice *dev);
> >  
> >  /**
> >   * pci_setup_iommu: Initialize specific IOMMU handlers for a PCIBus
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 76080af580..8078307963 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -2672,11 +2672,14 @@ static void pci_device_class_base_init(ObjectClass 
> > *klass, void *data)
> >      }
> >  }
> >  
> > -AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> > +static void pci_device_get_iommu_bus_devfn(PCIDevice *dev,
> > +                                           PCIBus **aliased_bus,
> > +                                           PCIBus **piommu_bus,
> > +                                           int *aliased_devfn)
> >  {
> >      PCIBus *bus = pci_get_bus(dev);
> >      PCIBus *iommu_bus = bus;
> > -    uint8_t devfn = dev->devfn;
> > +    int devfn = dev->devfn;
> >  
> >      while (iommu_bus && !iommu_bus->iommu_ops && iommu_bus->parent_dev) {
> >          PCIBus *parent_bus = pci_get_bus(iommu_bus->parent_dev);
> > @@ -2717,13 +2720,66 @@ AddressSpace 
> > *pci_device_iommu_address_space(PCIDevice *dev)
> >  
> >          iommu_bus = parent_bus;
> >      }
> > -    if (!pci_bus_bypass_iommu(bus) && iommu_bus->iommu_ops) {
> > +
> > +    assert(0 <= devfn && devfn < PCI_DEVFN_MAX);
> > +    assert(iommu_bus);
> > +
> > +    if (pci_bus_bypass_iommu(bus) || !iommu_bus->iommu_ops) {
> > +        iommu_bus = NULL;
> > +    }
> > +
> > +    *piommu_bus = iommu_bus;
> > +
> > +    if (aliased_bus) {
> > +        *aliased_bus = bus;
> > +    }
> > +
> > +    if (aliased_devfn) {
> > +        *aliased_devfn = devfn;
> > +    }
> > +}
> > +
> > +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> > +{
> > +    PCIBus *bus;
> > +    PCIBus *iommu_bus;
> > +    int devfn;
> > +
> > +    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
> > +    if (iommu_bus) {
> >          return iommu_bus->iommu_ops->get_address_space(bus,
> >                                   iommu_bus->iommu_opaque, devfn);
> >      }
> >      return &address_space_memory;
> >  }
> >  
> > +int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *base_dev,
> > +                                Error **errp)
> > +{
> > +    PCIBus *iommu_bus;
> > +
> > +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, NULL);
> > +    if (iommu_bus && iommu_bus->iommu_ops->set_iommu_device) {
> > +        return iommu_bus->iommu_ops->set_iommu_device(pci_get_bus(dev),
> > +                                                      
> > iommu_bus->iommu_opaque,
> > +                                                      dev->devfn, base_dev,
> > +                                                      errp);
> > +    }
> > +    return 0;
> > +}
> > +
> > +void pci_device_unset_iommu_device(PCIDevice *dev)
> > +{
> > +    PCIBus *iommu_bus;
> > +
> > +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, NULL);
> > +    if (iommu_bus && iommu_bus->iommu_ops->unset_iommu_device) {
> > +        return iommu_bus->iommu_ops->unset_iommu_device(pci_get_bus(dev),
> > +                                                        
> > iommu_bus->iommu_opaque,
> > +                                                        dev->devfn);
> > +    }
> > +}
> > +
> >  void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
> >  {
> >      /*
> > -- 
> > 2.34.1