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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v1 09/11] hw/pci: Introduce
>pci_device_set/unset_iommu_device()
>
>Hi Zhenzhong,
>
>On 2/28/24 04:58, 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.
>@errp is missing

Will add.

>> +     *
>> +     */
>> +    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;
>>
>>  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)
>>      }
>>  }
>>
>I would write some comments describing the output params and also
>explicitly saying some are optional

Sure.

>> -AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>> +static void pci_device_get_iommu_bus_devfn(PCIDevice *dev,
>> +                                           PCIBus **aliased_bus,
>> +                                           PCIBus **piommu_bus,
>
>piommu_bus is not an optional parameter. I would put it before aliased_bus.

Good suggestion, will do.

>
>> +                                           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);
>I would add a comment explaining why you don't care about aliased bus
>and devfn

Sure.

Thanks
Zhenzhong

>> +    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)
>>  {
>>      /*
>Thanks
>
>Eric