Re: [PATCH v3 01/15] hw/pci: Refactor pci_device_iommu_address_space()

[Joao Martins]

On 22/06/2023 21:50, Michael S. Tsirkin wrote:
> On Wed, May 31, 2023 at 11:03:23AM +0100, Joao Martins wrote:
>> On 30/05/2023 23:04, Philippe Mathieu-Daudé wrote:
>>> Hi Joao,
>>>
>>> On 30/5/23 19:59, Joao Martins wrote:
>>>> Rename pci_device_iommu_address_space() into pci_device_iommu_info().
>>>> In the new function return a new type PCIAddressSpace that encapsulates
>>>> the AddressSpace pointer that originally was returned.
>>>>
>>>> The new type is added in preparation to expanding it to include the IOMMU
>>>> memory region as a new field, such that we are able to fetch attributes of
>>>> the vIOMMU e.g. at vfio migration setup.
>>>>
>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>> ---
>>>>   hw/pci/pci.c         |  9 ++++++---
>>>>   include/hw/pci/pci.h | 21 ++++++++++++++++++++-
>>>
>>> Please consider using scripts/git.orderfile.
>>>
>> Will do -- wasn't aware of that script.
>>
>>>>   2 files changed, 26 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index 1cc7c89036b5..ecf8a543aa77 100644
>>>> --- a/hw/pci/pci.c
>>>> +++ b/hw/pci/pci.c
>>>> @@ -2633,11 +2633,12 @@ static void pci_device_class_base_init(ObjectClass
>>>> *klass, void *data)
>>>>       }
>>>>   }
>>>>   -AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>>>> +PCIAddressSpace pci_device_iommu_info(PCIDevice *dev)
>>>>   {
>>>
>>> This function is PCI specific, ...
>>>
>>>>   }
>>>>     void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque)
>>>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>>>> index e6d0574a2999..9ffaf47fe2ab 100644
>>>> --- a/include/hw/pci/pci.h
>>>> +++ b/include/hw/pci/pci.h
>>>> @@ -363,9 +363,28 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range);
>>>>     void pci_device_deassert_intx(PCIDevice *dev);
>>>>   +typedef struct PCIAddressSpace {
>>>> +    AddressSpace *as;
>>>
>>> ... but here I fail to understand what is PCI specific in this
>>> structure. You are just trying to an AS with a IOMMU MR, right?
>>>
>> Right. The patch is trying to better split the changes to use one function to
>> return everything (via pci_device_iommu_info) with the PCIAddressSpace
>> intermediate structure as retval, such that patch 3 just adds a
>> IOMMUMemoryRegion* in the latter for usage with the
>> pci_device_iommu_memory_region().
>>
>> I've named the structure with a 'PCI' prefix, because it seemed to me that 
>> it is
>> the only case (AIUI) that cares about whether a PCI has a different address
>> space that the memory map.
> 
> 
> yea keep that pls. It should be possible to figure out the header
> from the name.
> 

OK.

I am about to respin v4 series. It mainly reworks the first four patch enterily.
Essentially I'm following Peter's suggestion of picking Yi's old patches[0][1]
and avoid the direct manipulation of an IOMMU MR. The structure is very similar,
but the difference is avoid the direct manipulation of an IOMMU MR[2]. The end
goal in hw/pci is similar, fetching the backing IOMMU attribute from a PCI 
device.

[0] https://lore.kernel.org/all/20210302203827.437645-5-yi.l.liu@intel.com/
[1] https://lore.kernel.org/all/20210302203827.437645-6-yi.l.liu@intel.com/
[2] https://lore.kernel.org/qemu-devel/ZH9Kr6mrKNqUgcYs@x1n/

>>>> +} PCIAddressSpace;
>>>> +
>>>>   typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *, void *, int);
>>>> +static inline PCIAddressSpace as_to_pci_as(AddressSpace *as)
>>>> +{
>>>> +    PCIAddressSpace ret = { .as = as };
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +static inline AddressSpace *pci_as_to_as(PCIAddressSpace pci_as)
>>>> +{
>>>> +    return pci_as.as;
>>>> +}
>>>> +
>>>> +PCIAddressSpace pci_device_iommu_info(PCIDevice *dev);
>>>> +static inline AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>>>> +{
>>>> +    return pci_as_to_as(pci_device_iommu_info(dev));
>>>> +}
>>>>   -AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
>>>>   void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
>>>>     pcibus_t pci_bar_address(PCIDevice *d,
>>>
>