>-----Original Message----- >From: Cédric Le Goater <c...@redhat.com> >Subject: Re: [PATCH rfcv1 2/6] hw/pci: introduce >pci_device_set/unset_iommu_device() > >On 1/15/24 11:13, Zhenzhong Duan wrote: >> From: Yi Liu <yi.l....@intel.com> >> >> 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 <yi.l....@intel.com> >> Signed-off-by: Yi Sun <yi.y....@linux.intel.com> >> Signed-off-by: Nicolin Chen <nicol...@nvidia.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com> >> --- >> 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) >> { >> 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 && > >Why do we test iommu_bus in pci_device_un/set_iommu_device routines >and >not in pci_device_iommu_address_space() ?
iommu_bus check in pci_device_iommu_address_space() is dropped in below commit, I didn't find related discussion in mail history, maybe by accident? I can add it back if it's not intentional. ba7d12eb8c hw/pci: modify pci_setup_iommu() to set PCIIOMMUOps Thanks Zhenzhong