Re: [PATCH v4 01/15] hw/pci: Add a pci_setup_iommu_ops() helper

[Cédric Le Goater]

On 10/6/23 10:38, Joao Martins wrote:
> On 02/10/2023 16:12, Cédric Le Goater wrote:
> > Hello Joao,
> >
> > On 6/22/23 23:48, Joao Martins wrote:
> > > From: Yi Liu <yi.l.liu@intel.com>
> > >
> > > Add a pci_setup_iommu_ops() that uses a newly added structure
> > > (PCIIOMMUOps) instead of using PCIIOMMUFunc. The old pci_setup_iommu()
> > > that uses PCIIOMMUFunc is still kept for other IOMMUs to get an
> > > an address space for a PCI device in vendor specific way.
> > >
> > > In preparation to expand to supplying vIOMMU attributes, add a
> > > alternate helper pci_setup_iommu_ops() to setup the PCI device IOMMU.
> > > For now the PCIIOMMUOps just defines the address_space, but it will
> > > be extended to have another callback.
> > >
> > > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > > [joao: Massage commit message and subject, and make it a complementary
> > > rather than changing every single consumer of pci_setup_iommu()]
> > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> > > ---
> > > v1: https://lore.kernel.org/all/20210302203827.437645-5-yi.l.liu@intel.com/
> > > ---
> > >    include/hw/pci/pci.h     |  7 +++++++
> > >    include/hw/pci/pci_bus.h |  1 +
> > >    hw/pci/pci.c             | 26 +++++++++++++++++++++++---
> > >    3 files changed, 31 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > index e6d0574a2999..f59aef5a329a 100644
> > > --- a/include/hw/pci/pci.h
> > > +++ b/include/hw/pci/pci.h
> > > @@ -368,6 +368,13 @@ typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *, void *,
> > > int);
> > >    AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
> > >    void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
> > >    +typedef struct PCIIOMMUOps PCIIOMMUOps;
> > > +struct PCIIOMMUOps {
> > > +    AddressSpace * (*get_address_space)(PCIBus *bus,
> > > +                                void *opaque, int32_t devfn);
> > > +};
> > > +void pci_setup_iommu_ops(PCIBus *bus, const PCIIOMMUOps *iommu_ops, void
> > > *opaque);
> > > +
> >
> > I think you should first convert all PHBs to PCIIOMMUOps to avoid all the
> > tests as below and adapt pci_setup_iommu_ops() with the new parameter.
>
> OK, that's Yi's original patch:
>
> https://lore.kernel.org/all/20210302203827.437645-5-yi.l.liu@intel.com/
>
> I went with this one is that 1) it might take eons to get every single IOMMU
> maintainer ack; and 2) it would allow each IOMMU to move at its own speed
> specially as I can't test most of the other ones. essentially iterative, rather
> than invasive change? Does that make sense?

I think it is ok to make global changes to replace a function by a struct
of ops. This is not major (unless the extra indirection has a major perf
impact on some platforms). Getting acks from everyone will be difficult
since some PHBs are orphans.

C.