>-----Original Message-----
>From: Eric Auger <[email protected]>
>Subject: Re: [PATCH rfcv1 2/6] hw/pci: introduce
>pci_device_set/unset_iommu_device()
>
>Hi Zhenzhong,
>
>On 1/15/24 11:13, Zhenzhong Duan wrote:
>> From: Yi Liu <[email protected]>
>>
>> This adds pci_device_set/unset_iommu_device() to set/unset
>> IOMMUFDDevice 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 <[email protected]>
>> Signed-off-by: Yi Sun <[email protected]>
>> Signed-off-by: Nicolin Chen <[email protected]>
>> Signed-off-by: Zhenzhong Duan <[email protected]>
>> ---
>> include/hw/pci/pci.h | 39 ++++++++++++++++++++++++++++++++++-
>> hw/pci/pci.c | 49
>+++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 86 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index fa6313aabc..a810c0ec74 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -7,6 +7,8 @@
>> /* PCI includes legacy ISA access. */
>> #include "hw/isa/isa.h"
>>
>> +#include "sysemu/iommufd_device.h"
>> +
>> extern bool pci_available;
>>
>> /* PCI bus */
>> @@ -384,10 +386,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: set iommufd device for a PCI device to
>vIOMMU
>> + *
>> + * Optional callback, if not implemented in vIOMMU, then vIOMMU
>can't
>> + * utilize iommufd specific features.
>> + *
>> + * Return true if iommufd device is accepted, 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.
>> + *
>> + * @idev: the data structure representing iommufd device.
>> + *
>> + */
>> + int (*set_iommu_device)(PCIBus *bus, void *opaque, int32_t devfn,
>> + IOMMUFDDevice *idev, Error **errp);
>> + /**
>> + * @unset_iommu_device: unset iommufd device for a PCI device from
>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, int32_t
>devfn);
>> } PCIIOMMUOps;
>>
>> AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
>> +int pci_device_set_iommu_device(PCIDevice *dev, IOMMUFDDevice
>*idev,
>> + 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..3848662f95 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -2672,7 +2672,10 @@ 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_pbus,
>> + PCIBus **piommu_bus,
>> + uint8_t *aliased_pdevfn)
>nit: I would drop the p in aliased_pbus andaliased_pdevfn. Maybe you
>should allow the caller to pass NUL for aliased_pbus and aliased_pdevfn
>as it is the case for pci_device_set_iommu_device() I may resue that
>helper in [RFC 2/7] hw/pci: Introduce pci_device_iommu_bus
Good suggestion, will do.
Thanks
Zhenzhong
>> {
>> PCIBus *bus = pci_get_bus(dev);
>> PCIBus *iommu_bus = bus;
>> @@ -2717,6 +2720,18 @@ AddressSpace
>*pci_device_iommu_address_space(PCIDevice *dev)
>>
>> iommu_bus = parent_bus;
>> }
>> + *aliased_pbus = bus;
>> + *piommu_bus = iommu_bus;
>> + *aliased_pdevfn = devfn;
>> +}
>> +
>> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>> +{
>> + PCIBus *bus;
>> + PCIBus *iommu_bus;
>> + uint8_t devfn;
>> +
>> + pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
>> if (!pci_bus_bypass_iommu(bus) && iommu_bus->iommu_ops) {
>> return iommu_bus->iommu_ops->get_address_space(bus,
>> iommu_bus->iommu_opaque, devfn);
>> @@ -2724,6 +2739,38 @@ AddressSpace
>*pci_device_iommu_address_space(PCIDevice *dev)
>> return &address_space_memory;
>> }
>>
>> +int pci_device_set_iommu_device(PCIDevice *dev, IOMMUFDDevice
>*idev,
>> + Error **errp)
>> +{
>> + PCIBus *bus;
>> + PCIBus *iommu_bus;
>> + uint8_t devfn;
>> +
>> + pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
>> + if (!pci_bus_bypass_iommu(bus) && iommu_bus &&
>> + iommu_bus->iommu_ops && 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, idev,
>> errp);
>> + }
>> + return 0;
>> +}
>> +
>> +void pci_device_unset_iommu_device(PCIDevice *dev)
>> +{
>> + PCIBus *bus;
>> + PCIBus *iommu_bus;
>> + uint8_t devfn;
>> +
>> + pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
>> + if (!pci_bus_bypass_iommu(bus) && iommu_bus &&
>> + iommu_bus->iommu_ops && 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
