| Message ID | 20230530175937.24202-2-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: VFIO migration support with vIOMMU | expand |
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. > 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? > +} 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,
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. >> +} 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, >
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. > >> +} 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, > >
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, >>> >
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) { PCIBus *bus = pci_get_bus(dev); PCIBus *iommu_bus = bus; uint8_t devfn = dev->devfn; + AddressSpace *as = NULL; while (iommu_bus && !iommu_bus->iommu_fn && iommu_bus->parent_dev) { PCIBus *parent_bus = pci_get_bus(iommu_bus->parent_dev); @@ -2678,10 +2679,12 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev) iommu_bus = parent_bus; } + + as = &address_space_memory; if (!pci_bus_bypass_iommu(bus) && iommu_bus && iommu_bus->iommu_fn) { - return iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque, devfn); + as = iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque, devfn); } - return &address_space_memory; + return as_to_pci_as(as); } 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; +} 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,
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 ++++++++++++++++++++- 2 files changed, 26 insertions(+), 4 deletions(-)