RE: [PATCH v1 09/11] hw/pci: Introduce pci_device_set/unset_iommu_device()
>-Original Message- >From: Eric Auger >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 >> >> 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 >> Signed-off-by: Yi Sun >> Signed-off-by: Nicolin Chen >> Signed-off-by: Zhenzhong Duan >> --- >> 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)
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 > > 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 > Signed-off-by: Yi Sun > Signed-off-by: Nicolin Chen > Signed-off-by: Zhenzhong Duan > --- > 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 > + * > + */ > +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 > -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. > + 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, ); > +if (iommu_bus) { > return iommu_bus->iommu_ops->get_address_space(bus, > iommu_bus->iommu_opaque, devfn); > } > return _space_memory; > } > > +int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *base_dev, > +Error **errp) > +{ > +
Re: [PATCH v1 09/11] hw/pci: Introduce pci_device_set/unset_iommu_device()
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 > > > > 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 > > Signed-off-by: Yi Sun > > Signed-off-by: Nicolin Chen > > Signed-off-by: Zhenzhong Duan > > --- > > 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) > > +{ > > +
Re: [PATCH v1 09/11] hw/pci: Introduce pci_device_set/unset_iommu_device()
On Wed, Feb 28, 2024 at 11:58:58AM +0800, Zhenzhong Duan wrote: > From: Yi Liu > > 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 > Signed-off-by: Yi Sun > Signed-off-by: Nicolin Chen > Signed-off-by: Zhenzhong Duan > --- > 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 ... > 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, ); > +if (iommu_bus) { > return iommu_bus->iommu_ops->get_address_space(bus, > iommu_bus->iommu_opaque, devfn); > } > return _space_memory; > } > > +int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *base_dev, > +Error **errp) > +{