Re: [PATCH v2 06/22] hw/pci: introduce pci_device_set/unset_iommu_context()

[Auger Eric <eric.auger@redhat.com>]

Yi,
On 3/30/20 6:24 AM, Liu Yi L wrote:
> This patch adds pci_device_set/unset_iommu_context() to set/unset
> host_iommu_context for a given device. New callback is added in
> PCIIOMMUOps. As such, vIOMMU could make use of host IOMMU capability.
> e.g setup nested translation.

I think you need to explain what this practically is supposed to do.
such as: by attaching such context to a PCI device (for example VFIO
assigned?), you tell the host that this PCIe device is protected by a FL
stage controlled by the guest or something like that - if this is
correct understanding (?) -
> 
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Yi Sun <yi.y.sun@linux.intel.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> ---
>  hw/pci/pci.c         | 49 ++++++++++++++++++++++++++++++++++++++++++++-----
>  include/hw/pci/pci.h | 10 ++++++++++
>  2 files changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index aa9025c..af3c1a1 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2638,7 +2638,8 @@ 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 **pbus, uint8_t *pdevfn)
>  {
>      PCIBus *bus = pci_get_bus(dev);
>      PCIBus *iommu_bus = bus;
> @@ -2683,14 +2684,52 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>  
>          iommu_bus = parent_bus;
>      }
> -    if (iommu_bus && iommu_bus->iommu_ops &&
> -                     iommu_bus->iommu_ops->get_address_space) {
> -        return iommu_bus->iommu_ops->get_address_space(bus,
> -                                 iommu_bus->iommu_opaque, devfn);
> +    *pbus = iommu_bus;
> +    *pdevfn = devfn;
> +}
> +
> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> +{
> +    PCIBus *bus;
> +    uint8_t devfn;
> +
> +    pci_device_get_iommu_bus_devfn(dev, &bus, &devfn);
> +    if (bus && bus->iommu_ops &&
> +        bus->iommu_ops->get_address_space) {
> +        return bus->iommu_ops->get_address_space(bus,
> +                                bus->iommu_opaque, devfn);
>      }
>      return &address_space_memory;
>  }
>  
> +int pci_device_set_iommu_context(PCIDevice *dev,
> +                                 HostIOMMUContext *iommu_ctx)
> +{
> +    PCIBus *bus;
> +    uint8_t devfn;
> +
> +    pci_device_get_iommu_bus_devfn(dev, &bus, &devfn);
> +    if (bus && bus->iommu_ops &&
> +        bus->iommu_ops->set_iommu_context) {
> +        return bus->iommu_ops->set_iommu_context(bus,
> +                              bus->iommu_opaque, devfn, iommu_ctx);
> +    }
> +    return -ENOENT;
> +}
> +
> +void pci_device_unset_iommu_context(PCIDevice *dev)
> +{
> +    PCIBus *bus;
> +    uint8_t devfn;
> +
> +    pci_device_get_iommu_bus_devfn(dev, &bus, &devfn);
> +    if (bus && bus->iommu_ops &&
> +        bus->iommu_ops->unset_iommu_context) {
> +        bus->iommu_ops->unset_iommu_context(bus,
> +                                 bus->iommu_opaque, devfn);
> +    }
> +}
> +
>  void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
>  {
>      bus->iommu_ops = ops;
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index ffe192d..0ec5680 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -9,6 +9,8 @@
>  
>  #include "hw/pci/pcie.h"
>  
> +#include "hw/iommu/host_iommu_context.h"
> +
>  extern bool pci_available;
>  
>  /* PCI bus */
> @@ -489,9 +491,17 @@ typedef struct PCIIOMMUOps PCIIOMMUOps;
>  struct PCIIOMMUOps {
>      AddressSpace * (*get_address_space)(PCIBus *bus,
>                                  void *opaque, int32_t devfn);
> +    int (*set_iommu_context)(PCIBus *bus, void *opaque,
> +                             int32_t devfn,
> +                             HostIOMMUContext *iommu_ctx);
> +    void (*unset_iommu_context)(PCIBus *bus, void *opaque,
> +                                int32_t devfn);
>  };
>  
>  AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
> +int pci_device_set_iommu_context(PCIDevice *dev,
> +                                 HostIOMMUContext *iommu_ctx);
> +void pci_device_unset_iommu_context(PCIDevice *dev);
>  void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *iommu_ops, void *opaque);
>  
>  static inline void
> 
Thanks

Eric