diff mbox series

[v1,09/11] hw/pci: Introduce pci_device_set/unset_iommu_device()

Message ID 20240228035900.1085727-10-zhenzhong.duan@intel.com (mailing list archive)
State New, archived
Headers show
Series Add a host IOMMU device abstraction | expand

Commit Message

Duan, Zhenzhong Feb. 28, 2024, 3:58 a.m. UTC 
From: Yi Liu <yi.l.liu@intel.com>

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 <yi.l.liu@intel.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/pci/pci.h | 38 ++++++++++++++++++++++++++-
 hw/pci/pci.c         | 62 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 96 insertions(+), 4 deletions(-)

Comments

Michael S. Tsirkin March 12, 2024, 4:55 p.m. UTC | #1 
On Wed, Feb 28, 2024 at 11:58:58AM +0800, Zhenzhong Duan wrote:
> From: Yi Liu <yi.l.liu@intel.com>
> 
> 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 <yi.l.liu@intel.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  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, &iommu_bus, &devfn);
> +    if (iommu_bus) {
>          return iommu_bus->iommu_ops->get_address_space(bus,
>                                   iommu_bus->iommu_opaque, devfn);
>      }
>      return &address_space_memory;
>  }
>  
> +int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *base_dev,
> +                                Error **errp)
> +{
> +    PCIBus *iommu_bus;
> +
> +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, NULL);
> +    if (iommu_bus && iommu_bus->iommu_ops->set_iommu_device) {
> +        return iommu_bus->iommu_ops->set_iommu_device(pci_get_bus(dev),
> +                                                      iommu_bus->iommu_opaque,
> +                                                      dev->devfn, base_dev,
> +                                                      errp);
> +    }
> +    return 0;
> +}
> +
> +void pci_device_unset_iommu_device(PCIDevice *dev)
> +{
> +    PCIBus *iommu_bus;
> +
> +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, NULL);
> +    if (iommu_bus && iommu_bus->iommu_ops->unset_iommu_device) {
> +        return iommu_bus->iommu_ops->unset_iommu_device(pci_get_bus(dev),
> +                                                        iommu_bus->iommu_opaque,
> +                                                        dev->devfn);
> +    }
> +}
> +
>  void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
>  {
>      /*
> -- 
> 2.34.1
Michael S. Tsirkin March 12, 2024, 5:10 p.m. UTC | #2 
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 <yi.l.liu@intel.com>
> > 
> > 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 <yi.l.liu@intel.com>
> > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> > ---
> >  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)
> > +{
> > +    PCIBus *bus;
> > +    PCIBus *iommu_bus;
> > +    int devfn;
> > +
> > +    pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
> > +    if (iommu_bus) {
> >          return iommu_bus->iommu_ops->get_address_space(bus,
> >                                   iommu_bus->iommu_opaque, devfn);
> >      }
> >      return &address_space_memory;
> >  }
> >  
> > +int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *base_dev,
> > +                                Error **errp)
> > +{
> > +    PCIBus *iommu_bus;
> > +
> > +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, NULL);
> > +    if (iommu_bus && iommu_bus->iommu_ops->set_iommu_device) {
> > +        return iommu_bus->iommu_ops->set_iommu_device(pci_get_bus(dev),
> > +                                                      iommu_bus->iommu_opaque,
> > +                                                      dev->devfn, base_dev,
> > +                                                      errp);
> > +    }
> > +    return 0;
> > +}
> > +
> > +void pci_device_unset_iommu_device(PCIDevice *dev)
> > +{
> > +    PCIBus *iommu_bus;
> > +
> > +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, NULL);
> > +    if (iommu_bus && iommu_bus->iommu_ops->unset_iommu_device) {
> > +        return iommu_bus->iommu_ops->unset_iommu_device(pci_get_bus(dev),
> > +                                                        iommu_bus->iommu_opaque,
> > +                                                        dev->devfn);
> > +    }
> > +}
> > +
> >  void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
> >  {
> >      /*
> > -- 
> > 2.34.1
Eric Auger March 18, 2024, 2:49 p.m. UTC | #3 
Hi Zhenzhong,

On 2/28/24 04:58, Zhenzhong Duan wrote:
> From: Yi Liu <yi.l.liu@intel.com>
>
> 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 <yi.l.liu@intel.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  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, &iommu_bus, &devfn);
> +    if (iommu_bus) {
>          return iommu_bus->iommu_ops->get_address_space(bus,
>                                   iommu_bus->iommu_opaque, devfn);
>      }
>      return &address_space_memory;
>  }
>  
> +int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *base_dev,
> +                                Error **errp)
> +{
> +    PCIBus *iommu_bus;
> +
> +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, NULL);
I would add a comment explaining why you don't care about aliased bus
and devfn
> +    if (iommu_bus && iommu_bus->iommu_ops->set_iommu_device) {
> +        return iommu_bus->iommu_ops->set_iommu_device(pci_get_bus(dev),
> +                                                      iommu_bus->iommu_opaque,
> +                                                      dev->devfn, base_dev,
> +                                                      errp);
> +    }
> +    return 0;
> +}
> +
> +void pci_device_unset_iommu_device(PCIDevice *dev)
> +{
> +    PCIBus *iommu_bus;
> +
> +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, NULL);
> +    if (iommu_bus && iommu_bus->iommu_ops->unset_iommu_device) {
> +        return iommu_bus->iommu_ops->unset_iommu_device(pci_get_bus(dev),
> +                                                        iommu_bus->iommu_opaque,
> +                                                        dev->devfn);
> +    }
> +}
> +
>  void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
>  {
>      /*
Thanks

Eric
Duan, Zhenzhong March 19, 2024, 6:16 a.m. UTC | #4 
>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>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 <yi.l.liu@intel.com>
>>
>> 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 <yi.l.liu@intel.com>
>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  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)
>>
>>          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, &iommu_bus, &devfn);
>> +    if (iommu_bus) {
>>          return iommu_bus->iommu_ops->get_address_space(bus,
>>                                   iommu_bus->iommu_opaque, devfn);
>>      }
>>      return &address_space_memory;
>>  }
>>
>> +int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice
>*base_dev,
>> +                                Error **errp)
>> +{
>> +    PCIBus *iommu_bus;
>> +
>> +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, NULL);
>I would add a comment explaining why you don't care about aliased bus
>and devfn

Sure.

Thanks
Zhenzhong

>> +    if (iommu_bus && iommu_bus->iommu_ops->set_iommu_device) {
>> +        return iommu_bus->iommu_ops-
>>set_iommu_device(pci_get_bus(dev),
>> +                                                      iommu_bus->iommu_opaque,
>> +                                                      dev->devfn, base_dev,
>> +                                                      errp);
>> +    }
>> +    return 0;
>> +}
>> +
>> +void pci_device_unset_iommu_device(PCIDevice *dev)
>> +{
>> +    PCIBus *iommu_bus;
>> +
>> +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, NULL);
>> +    if (iommu_bus && iommu_bus->iommu_ops->unset_iommu_device) {
>> +        return iommu_bus->iommu_ops-
>>unset_iommu_device(pci_get_bus(dev),
>> +                                                        iommu_bus->iommu_opaque,
>> +                                                        dev->devfn);
>> +    }
>> +}
>> +
>>  void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void
>*opaque)
>>  {
>>      /*
>Thanks
>
>Eric
diff mbox series

Patch

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;
 
 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, &iommu_bus, &devfn);
+    if (iommu_bus) {
         return iommu_bus->iommu_ops->get_address_space(bus,
                                  iommu_bus->iommu_opaque, devfn);
     }
     return &address_space_memory;
 }
 
+int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *base_dev,
+                                Error **errp)
+{
+    PCIBus *iommu_bus;
+
+    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, NULL);
+    if (iommu_bus && iommu_bus->iommu_ops->set_iommu_device) {
+        return iommu_bus->iommu_ops->set_iommu_device(pci_get_bus(dev),
+                                                      iommu_bus->iommu_opaque,
+                                                      dev->devfn, base_dev,
+                                                      errp);
+    }
+    return 0;
+}
+
+void pci_device_unset_iommu_device(PCIDevice *dev)
+{
+    PCIBus *iommu_bus;
+
+    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, NULL);
+    if (iommu_bus && iommu_bus->iommu_ops->unset_iommu_device) {
+        return iommu_bus->iommu_ops->unset_iommu_device(pci_get_bus(dev),
+                                                        iommu_bus->iommu_opaque,
+                                                        dev->devfn);
+    }
+}
+
 void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
 {
     /*