Re: [RFC PATCH 01/10] iommu/vt-d: Get iommu device for a mediated device

[Alex Williamson <alex.williamson@redhat.com>]

On Sun, 22 Jul 2018 14:09:24 +0800
Lu Baolu <baolu.lu@linux.intel.com> wrote:

> This patch adds the support to get the iommu device for a mediated
> device. The assumption is that each mediated device is a minimal
> assignable set of a physical PCI device. Hence, we should use the
> iommu device of the parent PCI device to manage the translation.

Hmm, is this absolutely a valid assumption?  I'm afraid there's an
assumption throughout this series that the only way an mdev device
could be interacting with the IOMMU is via S-IOV, but we could choose
today to make an mdev wrapper for any device, which might provide basic
RID granularity to the IOMMU.  So if I make an mdev wrapper for a PF
such that I can implement migration for that device, is it valid for
the IOMMU driver to flag me as an mdev device and base mappings on my
parent device?  Perhaps in this patch we can assume that the parent of
such an mdev device would be the PF backing it and that results in the
correct drhd, but in the next patch we start imposing the assumption
that because the device is an mdev, the only valid association is via
pasid, which I'd say is incorrect.
 
> Cc: Ashok Raj <ashok.raj@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Liu Yi L <yi.l.liu@intel.com>
> Signed-off-by: Sanjay Kumar <sanjay.k.kumar@intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel-iommu.c |  6 ++++++
>  drivers/iommu/intel-pasid.h | 16 ++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 7d198ea..fc3ac1c 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -767,6 +767,12 @@ static struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devf
>  	if (iommu_dummy(dev))
>  		return NULL;
>  
> +	if (dev_is_mdev(dev)) {
> +		dev = dev_mdev_parent(dev);
> +		if (WARN_ON(!dev_is_pci(dev)))
> +			return NULL;
> +	}
> +
>  	if (dev_is_pci(dev)) {
>  		struct pci_dev *pf_pdev;
>  
> diff --git a/drivers/iommu/intel-pasid.h b/drivers/iommu/intel-pasid.h
> index 518df72..46cde66 100644
> --- a/drivers/iommu/intel-pasid.h
> +++ b/drivers/iommu/intel-pasid.h
> @@ -35,6 +35,22 @@ struct pasid_table {
>  	struct list_head	dev;		/* device list */
>  };
>  
> +static inline bool dev_is_mdev(struct device *dev)
> +{
> +	if (!dev)
> +		return false;
> +
> +	return !strcmp(dev->bus->name, "mdev");
> +}

I assume we're not using mdev_bus_type because mdev is a loadable
module and that symbol doesn't exist in this statically loaded driver,
but strcmp is a pretty ugly alternative.  Could we use symbol_get() so
that we can use mdev_bus_type?  Thanks,

Alex

> +
> +static inline struct device *dev_mdev_parent(struct device *dev)
> +{
> +	if (WARN_ON(!dev_is_mdev(dev)))
> +		return NULL;
> +
> +	return dev->parent;
> +}
> +
>  extern u32 intel_pasid_max_id;
>  int intel_pasid_alloc_id(void *ptr, int start, int end, gfp_t gfp);
>  void intel_pasid_free_id(int pasid);