[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 01/15] hw/pci: Add a pci_setup_iommu_ops() helper
From: |
Joao Martins |
Subject: |
Re: [PATCH v4 01/15] hw/pci: Add a pci_setup_iommu_ops() helper |
Date: |
Fri, 6 Oct 2023 12:06:08 +0100 |
User-agent: |
Mozilla Thunderbird |
On 06/10/2023 09:50, Cédric Le Goater wrote:
> 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).
It should be a mechanical change. As the pci_setup_iommu_ops() should be
functionally equivalent to pci_setup_iommu() [...]
> Getting acks from everyone will be difficult
> since some PHBs are orphans.
[...] This is what gets me a bit hesitant