| Message ID | 1532239773-15325-2-git-send-email-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/mdev: IOMMU aware mediated device | expand |
Hi Baolu, Per my understanding, the subject is not accurate. I think this patch is to get parent device structure for a mediate device instead of iommu device. This may be misleading for reviewers. Thanks, Yi Liu > From: Lu Baolu [mailto:baolu.lu@linux.intel.com] > Sent: Sunday, July 22, 2018 2:09 PM > > 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. > > 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"); } > + > +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); > -- > 2.7.4
Hi Yi, Thank you for reviewing my patch. On 07/23/2018 12:44 PM, Liu, Yi L wrote: > Hi Baolu, > > Per my understanding, the subject is not accurate. I think this patch is > to get parent device structure for a mediate device instead of iommu > device. This may be misleading for reviewers. Please check below. > > Thanks, > Yi Liu > >> From: Lu Baolu [mailto:baolu.lu@linux.intel.com] >> Sent: Sunday, July 22, 2018 2:09 PM >> >> 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. >> >> 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; >> + } >> + This gets the parent of a mediated device. And an iommu device will be found which services the parent device. A mediated device should in the same iommu scope as its parent. Best regards, Lu Baolu >> 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"); } >> + >> +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); >> -- >> 2.7.4 >
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);
Hi Alex, On 07/25/2018 02:50 AM, Alex Williamson wrote: > 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? You are right. We should not make it SIOV centric. Let me look into the patches and identify/fix the unreasonable assumptions. > 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, Okay. > 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. You are right. I will find and fix it. > >> 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, Yes. > but strcmp is a pretty ugly alternative. Could we use symbol_get() so > that we can use mdev_bus_type? Thanks, Sure. Best regards, Lu Baolu
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"); +} + +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);