| Message ID | 20181105073408.21815-7-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/mdev: IOMMU aware mediated device | expand |
Please use EXPORT_SYMBOL_GPL like most of the vfio code.
Hi, On 11/5/18 10:51 PM, Christoph Hellwig wrote: > Please use EXPORT_SYMBOL_GPL like most of the vfio code. > Sure. Will use this in the next version. Best regards, Lu Baolu
On Mon, 5 Nov 2018 15:34:06 +0800 Lu Baolu <baolu.lu@linux.intel.com> wrote: > A parent device might create different types of mediated > devices. For example, a mediated device could be created > by the parent device with full isolation and protection > provided by the IOMMU. One usage case could be found on > Intel platforms where a mediated device is an assignable > subset of a PCI, the DMA requests on behalf of it are all > tagged with a PASID. Since IOMMU supports PASID-granular > translations (scalable mode in vt-d 3.0), this mediated > device could be individually protected and isolated by an > IOMMU. > > This patch adds two new members in struct mdev_device: > * iommu_device > - This, if set, indicates that the mediated device could > be fully isolated and protected by IOMMU via attaching > an iommu domain to this device. If empty, it indicates > using vendor defined isolation. > > * iommu_domain > - This is a place holder for an iommu domain. A domain > could be store here for later use once it has been > attached to the iommu_device of this mdev. > > Below helpers are added to set and get above iommu device > and iommu domain pointers. > > * mdev_set/get_iommu_device(dev, iommu_device) > - Set or get the iommu device which represents this mdev > in IOMMU's device scope. Drivers don't need to set the > iommu device if it uses vendor defined isolation. > > * mdev_set/get_iommu_domain(domain) > - A iommu domain which has been attached to the iommu > device in order to protect and isolate the mediated > device will be kept in the mdev data structure and > could be retrieved later. > > 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> > Suggested-by: Kevin Tian <kevin.tian@intel.com> > Suggested-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > drivers/vfio/mdev/mdev_core.c | 36 ++++++++++++++++++++++++++++++++ > drivers/vfio/mdev/mdev_private.h | 2 ++ > include/linux/mdev.h | 23 ++++++++++++++++++++ > 3 files changed, 61 insertions(+) > > diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c > index 0212f0ee8aea..5119809225c5 100644 > --- a/drivers/vfio/mdev/mdev_core.c > +++ b/drivers/vfio/mdev/mdev_core.c > @@ -390,6 +390,42 @@ int mdev_device_remove(struct device *dev, bool force_remove) > return 0; > } > > +int mdev_set_iommu_device(struct device *dev, struct device *iommu_device) > +{ > + struct mdev_device *mdev = to_mdev_device(dev); > + > + mdev->iommu_device = iommu_device; > + > + return 0; > +} > +EXPORT_SYMBOL(mdev_set_iommu_device); > + > +struct device *mdev_get_iommu_device(struct device *dev) > +{ > + struct mdev_device *mdev = to_mdev_device(dev); > + > + return mdev->iommu_device; > +} > +EXPORT_SYMBOL(mdev_get_iommu_device); > + > +int mdev_set_iommu_domain(struct device *dev, void *domain) > +{ > + struct mdev_device *mdev = to_mdev_device(dev); > + > + mdev->iommu_domain = domain; > + > + return 0; > +} > +EXPORT_SYMBOL(mdev_set_iommu_domain); > + > +void *mdev_get_iommu_domain(struct device *dev) > +{ > + struct mdev_device *mdev = to_mdev_device(dev); > + > + return mdev->iommu_domain; > +} > +EXPORT_SYMBOL(mdev_get_iommu_domain); > + > static int __init mdev_init(void) > { > return mdev_bus_register(); > diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h > index b5819b7d7ef7..c01518068e84 100644 > --- a/drivers/vfio/mdev/mdev_private.h > +++ b/drivers/vfio/mdev/mdev_private.h > @@ -34,6 +34,8 @@ struct mdev_device { > struct list_head next; > struct kobject *type_kobj; > bool active; > + struct device *iommu_device; > + void *iommu_domain; > }; > > #define to_mdev_device(dev) container_of(dev, struct mdev_device, dev) > diff --git a/include/linux/mdev.h b/include/linux/mdev.h > index b6e048e1045f..c46777d3e568 100644 > --- a/include/linux/mdev.h > +++ b/include/linux/mdev.h > @@ -14,6 +14,29 @@ > #define MDEV_H > > struct mdev_device; > +struct iommu_domain; > + > +/* > + * Called by the parent device driver to set the PCI device which represents s/PCI // There is no requirement or expectation that the device is PCI. > + * this mdev in iommu protection scope. By default, the iommu device is NULL, > + * that indicates using vendor defined isolation. > + * > + * @dev: the mediated device that iommu will isolate. > + * @iommu_device: a pci device which represents the iommu for @dev. > + * > + * Return 0 for success, otherwise negative error value. > + */ > +int mdev_set_iommu_device(struct device *dev, struct device *iommu_device); > + > +struct device *mdev_get_iommu_device(struct device *dev); > + > +/* > + * Called by vfio iommu modules to save the iommu domain after a domain being > + * attached to the mediated device. > + */ > +int mdev_set_iommu_domain(struct device *dev, void *domain); > + > +void *mdev_get_iommu_domain(struct device *dev); I can't say I really understand the purpose of this, the cover letter indicates this is a placeholder, should we add it separately when we have a requirement for it? Thanks, Alex
Hi Alex, On 11/7/18 7:53 AM, Alex Williamson wrote: > On Mon, 5 Nov 2018 15:34:06 +0800 > Lu Baolu <baolu.lu@linux.intel.com> wrote: > >> A parent device might create different types of mediated >> devices. For example, a mediated device could be created >> by the parent device with full isolation and protection >> provided by the IOMMU. One usage case could be found on >> Intel platforms where a mediated device is an assignable >> subset of a PCI, the DMA requests on behalf of it are all >> tagged with a PASID. Since IOMMU supports PASID-granular >> translations (scalable mode in vt-d 3.0), this mediated >> device could be individually protected and isolated by an >> IOMMU. >> >> This patch adds two new members in struct mdev_device: >> * iommu_device >> - This, if set, indicates that the mediated device could >> be fully isolated and protected by IOMMU via attaching >> an iommu domain to this device. If empty, it indicates >> using vendor defined isolation. >> >> * iommu_domain >> - This is a place holder for an iommu domain. A domain >> could be store here for later use once it has been >> attached to the iommu_device of this mdev. >> >> Below helpers are added to set and get above iommu device >> and iommu domain pointers. >> >> * mdev_set/get_iommu_device(dev, iommu_device) >> - Set or get the iommu device which represents this mdev >> in IOMMU's device scope. Drivers don't need to set the >> iommu device if it uses vendor defined isolation. >> >> * mdev_set/get_iommu_domain(domain) >> - A iommu domain which has been attached to the iommu >> device in order to protect and isolate the mediated >> device will be kept in the mdev data structure and >> could be retrieved later. >> >> 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> >> Suggested-by: Kevin Tian <kevin.tian@intel.com> >> Suggested-by: Alex Williamson <alex.williamson@redhat.com> >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >> --- >> drivers/vfio/mdev/mdev_core.c | 36 ++++++++++++++++++++++++++++++++ >> drivers/vfio/mdev/mdev_private.h | 2 ++ >> include/linux/mdev.h | 23 ++++++++++++++++++++ >> 3 files changed, 61 insertions(+) >> >> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c >> index 0212f0ee8aea..5119809225c5 100644 >> --- a/drivers/vfio/mdev/mdev_core.c >> +++ b/drivers/vfio/mdev/mdev_core.c >> @@ -390,6 +390,42 @@ int mdev_device_remove(struct device *dev, bool force_remove) >> return 0; >> } >> >> +int mdev_set_iommu_device(struct device *dev, struct device *iommu_device) >> +{ >> + struct mdev_device *mdev = to_mdev_device(dev); >> + >> + mdev->iommu_device = iommu_device; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(mdev_set_iommu_device); >> + >> +struct device *mdev_get_iommu_device(struct device *dev) >> +{ >> + struct mdev_device *mdev = to_mdev_device(dev); >> + >> + return mdev->iommu_device; >> +} >> +EXPORT_SYMBOL(mdev_get_iommu_device); >> + >> +int mdev_set_iommu_domain(struct device *dev, void *domain) >> +{ >> + struct mdev_device *mdev = to_mdev_device(dev); >> + >> + mdev->iommu_domain = domain; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(mdev_set_iommu_domain); >> + >> +void *mdev_get_iommu_domain(struct device *dev) >> +{ >> + struct mdev_device *mdev = to_mdev_device(dev); >> + >> + return mdev->iommu_domain; >> +} >> +EXPORT_SYMBOL(mdev_get_iommu_domain); >> + >> static int __init mdev_init(void) >> { >> return mdev_bus_register(); >> diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h >> index b5819b7d7ef7..c01518068e84 100644 >> --- a/drivers/vfio/mdev/mdev_private.h >> +++ b/drivers/vfio/mdev/mdev_private.h >> @@ -34,6 +34,8 @@ struct mdev_device { >> struct list_head next; >> struct kobject *type_kobj; >> bool active; >> + struct device *iommu_device; >> + void *iommu_domain; >> }; >> >> #define to_mdev_device(dev) container_of(dev, struct mdev_device, dev) >> diff --git a/include/linux/mdev.h b/include/linux/mdev.h >> index b6e048e1045f..c46777d3e568 100644 >> --- a/include/linux/mdev.h >> +++ b/include/linux/mdev.h >> @@ -14,6 +14,29 @@ >> #define MDEV_H >> >> struct mdev_device; >> +struct iommu_domain; >> + >> +/* >> + * Called by the parent device driver to set the PCI device which represents > > s/PCI // > > There is no requirement or expectation that the device is PCI. > Fair enough. >> + * this mdev in iommu protection scope. By default, the iommu device is NULL, >> + * that indicates using vendor defined isolation. >> + * >> + * @dev: the mediated device that iommu will isolate. >> + * @iommu_device: a pci device which represents the iommu for @dev. >> + * >> + * Return 0 for success, otherwise negative error value. >> + */ >> +int mdev_set_iommu_device(struct device *dev, struct device *iommu_device); >> + >> +struct device *mdev_get_iommu_device(struct device *dev); >> + >> +/* >> + * Called by vfio iommu modules to save the iommu domain after a domain being >> + * attached to the mediated device. >> + */ >> +int mdev_set_iommu_domain(struct device *dev, void *domain); >> + >> +void *mdev_get_iommu_domain(struct device *dev); > > I can't say I really understand the purpose of this, the cover letter > indicates this is a placeholder, should we add it separately when we > have a requirement for it? Oh, I am sorry that I used a wrong word. It's not a placeholder for something designed for future, but adding two members that will be used in the following patches. Since they will be used in anther modules (like vfio_iommu), we need function interfaces to get and set them. mdev->iommu_device: - This, if set, indicates that the mediated device could be fully isolated and protected by IOMMU via attaching an iommu domain to this device. If empty, it indicates using vendor defined isolation. mdev->iommu_domain: - This is used to save the pointer of an iommu domain. Once a domain has been attached to the iommu_device, it should be stored here. Sorry for the confusion. Best regards, Lu Baolu
On 11/7/2018 7:18 AM, Lu Baolu wrote: > Hi Alex, > > On 11/7/18 7:53 AM, Alex Williamson wrote: >> On Mon, 5 Nov 2018 15:34:06 +0800 >> Lu Baolu <baolu.lu@linux.intel.com> wrote: >> >>> A parent device might create different types of mediated >>> devices. For example, a mediated device could be created >>> by the parent device with full isolation and protection >>> provided by the IOMMU. One usage case could be found on >>> Intel platforms where a mediated device is an assignable >>> subset of a PCI, the DMA requests on behalf of it are all >>> tagged with a PASID. Since IOMMU supports PASID-granular >>> translations (scalable mode in vt-d 3.0), this mediated >>> device could be individually protected and isolated by an >>> IOMMU. >>> >>> This patch adds two new members in struct mdev_device: >>> * iommu_device >>> - This, if set, indicates that the mediated device could >>> be fully isolated and protected by IOMMU via attaching >>> an iommu domain to this device. If empty, it indicates >>> using vendor defined isolation. >>> >>> * iommu_domain >>> - This is a place holder for an iommu domain. A domain >>> could be store here for later use once it has been >>> attached to the iommu_device of this mdev. >>> >>> Below helpers are added to set and get above iommu device >>> and iommu domain pointers. >>> >>> * mdev_set/get_iommu_device(dev, iommu_device) >>> - Set or get the iommu device which represents this mdev >>> in IOMMU's device scope. Drivers don't need to set the >>> iommu device if it uses vendor defined isolation. >>> >>> * mdev_set/get_iommu_domain(domain) >>> - A iommu domain which has been attached to the iommu >>> device in order to protect and isolate the mediated >>> device will be kept in the mdev data structure and >>> could be retrieved later. >>> >>> 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> >>> Suggested-by: Kevin Tian <kevin.tian@intel.com> >>> Suggested-by: Alex Williamson <alex.williamson@redhat.com> >>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >>> --- >>> drivers/vfio/mdev/mdev_core.c | 36 ++++++++++++++++++++++++++++++++ >>> drivers/vfio/mdev/mdev_private.h | 2 ++ >>> include/linux/mdev.h | 23 ++++++++++++++++++++ >>> 3 files changed, 61 insertions(+) >>> >>> diff --git a/drivers/vfio/mdev/mdev_core.c >>> b/drivers/vfio/mdev/mdev_core.c >>> index 0212f0ee8aea..5119809225c5 100644 >>> --- a/drivers/vfio/mdev/mdev_core.c >>> +++ b/drivers/vfio/mdev/mdev_core.c >>> @@ -390,6 +390,42 @@ int mdev_device_remove(struct device *dev, bool >>> force_remove) >>> return 0; >>> } >>> +int mdev_set_iommu_device(struct device *dev, struct device >>> *iommu_device) >>> +{ >>> + struct mdev_device *mdev = to_mdev_device(dev); >>> + >>> + mdev->iommu_device = iommu_device; >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL(mdev_set_iommu_device); >>> + >>> +struct device *mdev_get_iommu_device(struct device *dev) >>> +{ >>> + struct mdev_device *mdev = to_mdev_device(dev); >>> + >>> + return mdev->iommu_device; >>> +} >>> +EXPORT_SYMBOL(mdev_get_iommu_device); >>> + >>> +int mdev_set_iommu_domain(struct device *dev, void *domain) >>> +{ >>> + struct mdev_device *mdev = to_mdev_device(dev); >>> + >>> + mdev->iommu_domain = domain; >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL(mdev_set_iommu_domain); >>> + >>> +void *mdev_get_iommu_domain(struct device *dev) >>> +{ >>> + struct mdev_device *mdev = to_mdev_device(dev); >>> + >>> + return mdev->iommu_domain; >>> +} >>> +EXPORT_SYMBOL(mdev_get_iommu_domain); >>> + >>> static int __init mdev_init(void) >>> { >>> return mdev_bus_register(); >>> diff --git a/drivers/vfio/mdev/mdev_private.h >>> b/drivers/vfio/mdev/mdev_private.h >>> index b5819b7d7ef7..c01518068e84 100644 >>> --- a/drivers/vfio/mdev/mdev_private.h >>> +++ b/drivers/vfio/mdev/mdev_private.h >>> @@ -34,6 +34,8 @@ struct mdev_device { >>> struct list_head next; >>> struct kobject *type_kobj; >>> bool active; >>> + struct device *iommu_device; >>> + void *iommu_domain; >>> }; >>> #define to_mdev_device(dev) container_of(dev, struct >>> mdev_device, dev) >>> diff --git a/include/linux/mdev.h b/include/linux/mdev.h >>> index b6e048e1045f..c46777d3e568 100644 >>> --- a/include/linux/mdev.h >>> +++ b/include/linux/mdev.h >>> @@ -14,6 +14,29 @@ >>> #define MDEV_H >>> struct mdev_device; >>> +struct iommu_domain; >>> + >>> +/* >>> + * Called by the parent device driver to set the PCI device which >>> represents >> >> s/PCI // >> >> There is no requirement or expectation that the device is PCI. >> > > Fair enough. > >>> + * this mdev in iommu protection scope. By default, the iommu device >>> is NULL, >>> + * that indicates using vendor defined isolation. >>> + * >>> + * @dev: the mediated device that iommu will isolate. >>> + * @iommu_device: a pci device which represents the iommu for @dev. >>> + * >>> + * Return 0 for success, otherwise negative error value. >>> + */ >>> +int mdev_set_iommu_device(struct device *dev, struct device >>> *iommu_device); >>> + >>> +struct device *mdev_get_iommu_device(struct device *dev); >>> + >>> +/* >>> + * Called by vfio iommu modules to save the iommu domain after a >>> domain being >>> + * attached to the mediated device. >>> + */ >>> +int mdev_set_iommu_domain(struct device *dev, void *domain); >>> + >>> +void *mdev_get_iommu_domain(struct device *dev); >> >> I can't say I really understand the purpose of this, the cover letter >> indicates this is a placeholder, should we add it separately when we >> have a requirement for it? > > Oh, I am sorry that I used a wrong word. It's not a placeholder for > something designed for future, but adding two members that will be used > in the following patches. Since they will be used in anther modules > (like vfio_iommu), we need function interfaces to get and set them. > > mdev->iommu_device: > - This, if set, indicates that the mediated device could > be fully isolated and protected by IOMMU via attaching > an iommu domain to this device. If empty, it indicates > using vendor defined isolation. > > mdev->iommu_domain: > - This is used to save the pointer of an iommu domain. Once > a domain has been attached to the iommu_device, it should > be stored here. > I don't see mdev->iommu_domain is used anywhere in this series of patch. If this is not being used, then no need to save it. With that symbols mdev_set/get_iommu_domain(domain) are not required. Please keep symbols mdev_set/get_iommu_device(dev, iommu_device) non-GPL same as other exported symbols from mdev_core module. Thanks, Kirti
Hi, On 11/16/18 5:31 AM, Kirti Wankhede wrote: > > > On 11/7/2018 7:18 AM, Lu Baolu wrote: >> Hi Alex, >> >> On 11/7/18 7:53 AM, Alex Williamson wrote: >>> On Mon, 5 Nov 2018 15:34:06 +0800 >>> Lu Baolu <baolu.lu@linux.intel.com> wrote: >>> >>>> A parent device might create different types of mediated >>>> devices. For example, a mediated device could be created >>>> by the parent device with full isolation and protection >>>> provided by the IOMMU. One usage case could be found on >>>> Intel platforms where a mediated device is an assignable >>>> subset of a PCI, the DMA requests on behalf of it are all >>>> tagged with a PASID. Since IOMMU supports PASID-granular >>>> translations (scalable mode in vt-d 3.0), this mediated >>>> device could be individually protected and isolated by an >>>> IOMMU. >>>> >>>> This patch adds two new members in struct mdev_device: >>>> * iommu_device >>>> - This, if set, indicates that the mediated device could >>>> be fully isolated and protected by IOMMU via attaching >>>> an iommu domain to this device. If empty, it indicates >>>> using vendor defined isolation. >>>> >>>> * iommu_domain >>>> - This is a place holder for an iommu domain. A domain >>>> could be store here for later use once it has been >>>> attached to the iommu_device of this mdev. >>>> >>>> Below helpers are added to set and get above iommu device >>>> and iommu domain pointers. >>>> >>>> * mdev_set/get_iommu_device(dev, iommu_device) >>>> - Set or get the iommu device which represents this mdev >>>> in IOMMU's device scope. Drivers don't need to set the >>>> iommu device if it uses vendor defined isolation. >>>> >>>> * mdev_set/get_iommu_domain(domain) >>>> - A iommu domain which has been attached to the iommu >>>> device in order to protect and isolate the mediated >>>> device will be kept in the mdev data structure and >>>> could be retrieved later. >>>> >>>> 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> >>>> Suggested-by: Kevin Tian <kevin.tian@intel.com> >>>> Suggested-by: Alex Williamson <alex.williamson@redhat.com> >>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >>>> --- >>>> drivers/vfio/mdev/mdev_core.c | 36 ++++++++++++++++++++++++++++++++ >>>> drivers/vfio/mdev/mdev_private.h | 2 ++ >>>> include/linux/mdev.h | 23 ++++++++++++++++++++ >>>> 3 files changed, 61 insertions(+) >>>> >>>> diff --git a/drivers/vfio/mdev/mdev_core.c >>>> b/drivers/vfio/mdev/mdev_core.c >>>> index 0212f0ee8aea..5119809225c5 100644 >>>> --- a/drivers/vfio/mdev/mdev_core.c >>>> +++ b/drivers/vfio/mdev/mdev_core.c >>>> @@ -390,6 +390,42 @@ int mdev_device_remove(struct device *dev, bool >>>> force_remove) >>>> return 0; >>>> } >>>> +int mdev_set_iommu_device(struct device *dev, struct device >>>> *iommu_device) >>>> +{ >>>> + struct mdev_device *mdev = to_mdev_device(dev); >>>> + >>>> + mdev->iommu_device = iommu_device; >>>> + >>>> + return 0; >>>> +} >>>> +EXPORT_SYMBOL(mdev_set_iommu_device); >>>> + >>>> +struct device *mdev_get_iommu_device(struct device *dev) >>>> +{ >>>> + struct mdev_device *mdev = to_mdev_device(dev); >>>> + >>>> + return mdev->iommu_device; >>>> +} >>>> +EXPORT_SYMBOL(mdev_get_iommu_device); >>>> + >>>> +int mdev_set_iommu_domain(struct device *dev, void *domain) >>>> +{ >>>> + struct mdev_device *mdev = to_mdev_device(dev); >>>> + >>>> + mdev->iommu_domain = domain; >>>> + >>>> + return 0; >>>> +} >>>> +EXPORT_SYMBOL(mdev_set_iommu_domain); >>>> + >>>> +void *mdev_get_iommu_domain(struct device *dev) >>>> +{ >>>> + struct mdev_device *mdev = to_mdev_device(dev); >>>> + >>>> + return mdev->iommu_domain; >>>> +} >>>> +EXPORT_SYMBOL(mdev_get_iommu_domain); >>>> + >>>> static int __init mdev_init(void) >>>> { >>>> return mdev_bus_register(); >>>> diff --git a/drivers/vfio/mdev/mdev_private.h >>>> b/drivers/vfio/mdev/mdev_private.h >>>> index b5819b7d7ef7..c01518068e84 100644 >>>> --- a/drivers/vfio/mdev/mdev_private.h >>>> +++ b/drivers/vfio/mdev/mdev_private.h >>>> @@ -34,6 +34,8 @@ struct mdev_device { >>>> struct list_head next; >>>> struct kobject *type_kobj; >>>> bool active; >>>> + struct device *iommu_device; >>>> + void *iommu_domain; >>>> }; >>>> #define to_mdev_device(dev) container_of(dev, struct >>>> mdev_device, dev) >>>> diff --git a/include/linux/mdev.h b/include/linux/mdev.h >>>> index b6e048e1045f..c46777d3e568 100644 >>>> --- a/include/linux/mdev.h >>>> +++ b/include/linux/mdev.h >>>> @@ -14,6 +14,29 @@ >>>> #define MDEV_H >>>> struct mdev_device; >>>> +struct iommu_domain; >>>> + >>>> +/* >>>> + * Called by the parent device driver to set the PCI device which >>>> represents >>> >>> s/PCI // >>> >>> There is no requirement or expectation that the device is PCI. >>> >> >> Fair enough. >> >>>> + * this mdev in iommu protection scope. By default, the iommu device >>>> is NULL, >>>> + * that indicates using vendor defined isolation. >>>> + * >>>> + * @dev: the mediated device that iommu will isolate. >>>> + * @iommu_device: a pci device which represents the iommu for @dev. >>>> + * >>>> + * Return 0 for success, otherwise negative error value. >>>> + */ >>>> +int mdev_set_iommu_device(struct device *dev, struct device >>>> *iommu_device); >>>> + >>>> +struct device *mdev_get_iommu_device(struct device *dev); >>>> + >>>> +/* >>>> + * Called by vfio iommu modules to save the iommu domain after a >>>> domain being >>>> + * attached to the mediated device. >>>> + */ >>>> +int mdev_set_iommu_domain(struct device *dev, void *domain); >>>> + >>>> +void *mdev_get_iommu_domain(struct device *dev); >>> >>> I can't say I really understand the purpose of this, the cover letter >>> indicates this is a placeholder, should we add it separately when we >>> have a requirement for it? >> >> Oh, I am sorry that I used a wrong word. It's not a placeholder for >> something designed for future, but adding two members that will be used >> in the following patches. Since they will be used in anther modules >> (like vfio_iommu), we need function interfaces to get and set them. >> >> mdev->iommu_device: >> - This, if set, indicates that the mediated device could >> be fully isolated and protected by IOMMU via attaching >> an iommu domain to this device. If empty, it indicates >> using vendor defined isolation. >> >> mdev->iommu_domain: >> - This is used to save the pointer of an iommu domain. Once >> a domain has been attached to the iommu_device, it should >> be stored here. >> > > I don't see mdev->iommu_domain is used anywhere in this series of patch. > If this is not being used, then no need to save it. With that symbols > mdev_set/get_iommu_domain(domain) are not required. Yes. We won't use mdev->iommu_domain in this patch series. It should be used by mdev parent driver to retrieve the default pasid of the domain. Something like: domain = mdev_get_iommu_domain(dev) pasid = iommu_domain_get_attr(domain, DOMAIN_ATTR_AUXD_ID); reg_write(pasid_reg, pasid); I am okay if we remove mdev_set/get_iommu_domain from this patch series and add it later when the parent driver comes. > > Please keep symbols mdev_set/get_iommu_device(dev, iommu_device) non-GPL > same as other exported symbols from mdev_core module. Yes. It will be fixed in the next version. > > Thanks, > Kirti > Best regards, Lu Baolu
On Fri, Nov 16, 2018 at 09:20:48AM +0800, Lu Baolu wrote: > > Please keep symbols mdev_set/get_iommu_device(dev, iommu_device) non-GPL > > same as other exported symbols from mdev_core module. > > Yes. It will be fixed in the next version. No. mdev shall not be used to circumvent the exports in the generic vfio code.
Hi, On 11/16/18 4:57 PM, Christoph Hellwig wrote: > On Fri, Nov 16, 2018 at 09:20:48AM +0800, Lu Baolu wrote: >>> Please keep symbols mdev_set/get_iommu_device(dev, iommu_device) non-GPL >>> same as other exported symbols from mdev_core module. >> >> Yes. It will be fixed in the next version. > > No. mdev shall not be used to circumvent the exports in the generic > vfio code. Get it now. Thanks a lot. Best regards, Lu Baolu
On 11/16/2018 2:27 PM, Christoph Hellwig wrote: > On Fri, Nov 16, 2018 at 09:20:48AM +0800, Lu Baolu wrote: >>> Please keep symbols mdev_set/get_iommu_device(dev, iommu_device) non-GPL >>> same as other exported symbols from mdev_core module. >> >> Yes. It will be fixed in the next version. > > No. mdev shall not be used to circumvent the exports in the generic > vfio code. > It is about how mdev framework can be used by existing drivers. These symbols doesn't use any other exported symbols. Thanks, Kirti
On Wed, Nov 21, 2018 at 02:22:08AM +0530, Kirti Wankhede wrote: > It is about how mdev framework can be used by existing drivers. These > symbols doesn't use any other exported symbols. That is an unfortunate accident of history, but doesn't extent to new ones. It also is another inidicator those drivers probably are derived works of the Linux kernel and might be in legal trouble one way or another.
On 11/21/2018 2:15 PM, Christoph Hellwig wrote: > On Wed, Nov 21, 2018 at 02:22:08AM +0530, Kirti Wankhede wrote: >> It is about how mdev framework can be used by existing drivers. These >> symbols doesn't use any other exported symbols. > > That is an unfortunate accident of history, but doesn't extent to new > ones. It also is another inidicator those drivers probably are derived > works of the Linux kernel and might be in legal trouble one way or > another. > These symbols are just to associate iommu properties of a physical device with a mdev device, doesn't include low-level information. Thanks, Kirti
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index 0212f0ee8aea..5119809225c5 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -390,6 +390,42 @@ int mdev_device_remove(struct device *dev, bool force_remove) return 0; } +int mdev_set_iommu_device(struct device *dev, struct device *iommu_device) +{ + struct mdev_device *mdev = to_mdev_device(dev); + + mdev->iommu_device = iommu_device; + + return 0; +} +EXPORT_SYMBOL(mdev_set_iommu_device); + +struct device *mdev_get_iommu_device(struct device *dev) +{ + struct mdev_device *mdev = to_mdev_device(dev); + + return mdev->iommu_device; +} +EXPORT_SYMBOL(mdev_get_iommu_device); + +int mdev_set_iommu_domain(struct device *dev, void *domain) +{ + struct mdev_device *mdev = to_mdev_device(dev); + + mdev->iommu_domain = domain; + + return 0; +} +EXPORT_SYMBOL(mdev_set_iommu_domain); + +void *mdev_get_iommu_domain(struct device *dev) +{ + struct mdev_device *mdev = to_mdev_device(dev); + + return mdev->iommu_domain; +} +EXPORT_SYMBOL(mdev_get_iommu_domain); + static int __init mdev_init(void) { return mdev_bus_register(); diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h index b5819b7d7ef7..c01518068e84 100644 --- a/drivers/vfio/mdev/mdev_private.h +++ b/drivers/vfio/mdev/mdev_private.h @@ -34,6 +34,8 @@ struct mdev_device { struct list_head next; struct kobject *type_kobj; bool active; + struct device *iommu_device; + void *iommu_domain; }; #define to_mdev_device(dev) container_of(dev, struct mdev_device, dev) diff --git a/include/linux/mdev.h b/include/linux/mdev.h index b6e048e1045f..c46777d3e568 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -14,6 +14,29 @@ #define MDEV_H struct mdev_device; +struct iommu_domain; + +/* + * Called by the parent device driver to set the PCI device which represents + * this mdev in iommu protection scope. By default, the iommu device is NULL, + * that indicates using vendor defined isolation. + * + * @dev: the mediated device that iommu will isolate. + * @iommu_device: a pci device which represents the iommu for @dev. + * + * Return 0 for success, otherwise negative error value. + */ +int mdev_set_iommu_device(struct device *dev, struct device *iommu_device); + +struct device *mdev_get_iommu_device(struct device *dev); + +/* + * Called by vfio iommu modules to save the iommu domain after a domain being + * attached to the mediated device. + */ +int mdev_set_iommu_domain(struct device *dev, void *domain); + +void *mdev_get_iommu_domain(struct device *dev); /** * struct mdev_parent_ops - Structure to be registered for each parent device to
A parent device might create different types of mediated devices. For example, a mediated device could be created by the parent device with full isolation and protection provided by the IOMMU. One usage case could be found on Intel platforms where a mediated device is an assignable subset of a PCI, the DMA requests on behalf of it are all tagged with a PASID. Since IOMMU supports PASID-granular translations (scalable mode in vt-d 3.0), this mediated device could be individually protected and isolated by an IOMMU. This patch adds two new members in struct mdev_device: * iommu_device - This, if set, indicates that the mediated device could be fully isolated and protected by IOMMU via attaching an iommu domain to this device. If empty, it indicates using vendor defined isolation. * iommu_domain - This is a place holder for an iommu domain. A domain could be store here for later use once it has been attached to the iommu_device of this mdev. Below helpers are added to set and get above iommu device and iommu domain pointers. * mdev_set/get_iommu_device(dev, iommu_device) - Set or get the iommu device which represents this mdev in IOMMU's device scope. Drivers don't need to set the iommu device if it uses vendor defined isolation. * mdev_set/get_iommu_domain(domain) - A iommu domain which has been attached to the iommu device in order to protect and isolate the mediated device will be kept in the mdev data structure and could be retrieved later. 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> Suggested-by: Kevin Tian <kevin.tian@intel.com> Suggested-by: Alex Williamson <alex.williamson@redhat.com> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> --- drivers/vfio/mdev/mdev_core.c | 36 ++++++++++++++++++++++++++++++++ drivers/vfio/mdev/mdev_private.h | 2 ++ include/linux/mdev.h | 23 ++++++++++++++++++++ 3 files changed, 61 insertions(+)