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)
> +{
