| Message ID | 20190317172232.1068-8-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMMUv3 Nested Stage Setup | expand |
On Sun, 17 Mar 2019 18:22:17 +0100 Eric Auger <eric.auger@redhat.com> wrote: > From: "Liu, Yi L" <yi.l.liu@linux.intel.com> > > This patch adds VFIO_IOMMU_ATTACH/DETACH_PASID_TABLE ioctl > which aims to pass/withdraw the virtual iommu guest configuration > to/from the VFIO driver downto to the iommu subsystem. > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> > Signed-off-by: Liu, Yi L <yi.l.liu@linux.intel.com> > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > v3 -> v4: > - restore ATTACH/DETACH > - add unwind on failure > > v2 -> v3: > - s/BIND_PASID_TABLE/SET_PASID_TABLE > > v1 -> v2: > - s/BIND_GUEST_STAGE/BIND_PASID_TABLE > - remove the struct device arg > --- > drivers/vfio/vfio_iommu_type1.c | 53 +++++++++++++++++++++++++++++++++ > include/uapi/linux/vfio.h | 17 +++++++++++ > 2 files changed, 70 insertions(+) > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 73652e21efec..222e9199edbf 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -1644,6 +1644,43 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu) > return ret; > } > > +static void > +vfio_detach_pasid_table(struct vfio_iommu *iommu) > +{ > + struct vfio_domain *d; > + > + mutex_lock(&iommu->lock); > + > + list_for_each_entry(d, &iommu->domain_list, next) { > + iommu_detach_pasid_table(d->domain); > + } > + mutex_unlock(&iommu->lock); > +} > + > +static int > +vfio_attach_pasid_table(struct vfio_iommu *iommu, > + struct vfio_iommu_type1_attach_pasid_table *ustruct) > +{ > + struct vfio_domain *d; > + int ret = 0; > + > + mutex_lock(&iommu->lock); > + > + list_for_each_entry(d, &iommu->domain_list, next) { > + ret = iommu_attach_pasid_table(d->domain, &ustruct->config); > + if (ret) > + goto unwind; > + } > + goto unlock; > +unwind: > + list_for_each_entry_continue_reverse(d, &iommu->domain_list, next) { > + iommu_detach_pasid_table(d->domain); > + } > +unlock: > + mutex_unlock(&iommu->lock); > + return ret; > +} > + > static long vfio_iommu_type1_ioctl(void *iommu_data, > unsigned int cmd, unsigned long arg) > { > @@ -1714,6 +1751,22 @@ static long vfio_iommu_type1_ioctl(void *iommu_data, > > return copy_to_user((void __user *)arg, &unmap, minsz) ? > -EFAULT : 0; > + } else if (cmd == VFIO_IOMMU_ATTACH_PASID_TABLE) { > + struct vfio_iommu_type1_attach_pasid_table ustruct; > + > + minsz = offsetofend(struct vfio_iommu_type1_attach_pasid_table, > + config); > + > + if (copy_from_user(&ustruct, (void __user *)arg, minsz)) > + return -EFAULT; > + > + if (ustruct.argsz < minsz || ustruct.flags) > + return -EINVAL; Who is responsible for validating the ustruct.config? arm_smmu_attach_pasid_table() only looks at the format, ignoring the version field :-\ In fact, where is struct iommu_pasid_smmuv3 ever used from the config? Should the padding fields be forced to zero? We don't have flags to incorporate use of them with future extensions, so maybe we don't care? > + > + return vfio_attach_pasid_table(iommu, &ustruct); > + } else if (cmd == VFIO_IOMMU_DETACH_PASID_TABLE) { > + vfio_detach_pasid_table(iommu); > + return 0; > } > > return -ENOTTY; > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index 02bb7ad6e986..329d378565d9 100644 > --- a/include/uapi/linux/vfio.h > +++ b/include/uapi/linux/vfio.h > @@ -14,6 +14,7 @@ > > #include <linux/types.h> > #include <linux/ioctl.h> > +#include <linux/iommu.h> > > #define VFIO_API_VERSION 0 > > @@ -759,6 +760,22 @@ struct vfio_iommu_type1_dma_unmap { > #define VFIO_IOMMU_ENABLE _IO(VFIO_TYPE, VFIO_BASE + 15) > #define VFIO_IOMMU_DISABLE _IO(VFIO_TYPE, VFIO_BASE + 16) > > +/** > + * VFIO_IOMMU_ATTACH_PASID_TABLE - _IOWR(VFIO_TYPE, VFIO_BASE + 22, > + * struct vfio_iommu_type1_attach_pasid_table) > + * > + * Passes the PASID table to the host. Calling ATTACH_PASID_TABLE > + * while a table is already installed is allowed: it replaces the old > + * table. DETACH does a comprehensive tear down of the nested mode. > + */ > +struct vfio_iommu_type1_attach_pasid_table { > + __u32 argsz; > + __u32 flags; > + struct iommu_pasid_table_config config; > +}; > +#define VFIO_IOMMU_ATTACH_PASID_TABLE _IO(VFIO_TYPE, VFIO_BASE + 22) > +#define VFIO_IOMMU_DETACH_PASID_TABLE _IO(VFIO_TYPE, VFIO_BASE + 23) > + DETACH should also be documented, it's not clear from the uapi that it requires no parameters. Thanks, Alex
Hi Alex, On 3/21/19 11:19 PM, Alex Williamson wrote: > On Sun, 17 Mar 2019 18:22:17 +0100 > Eric Auger <eric.auger@redhat.com> wrote: > >> From: "Liu, Yi L" <yi.l.liu@linux.intel.com> >> >> This patch adds VFIO_IOMMU_ATTACH/DETACH_PASID_TABLE ioctl >> which aims to pass/withdraw the virtual iommu guest configuration >> to/from the VFIO driver downto to the iommu subsystem. >> >> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> >> Signed-off-by: Liu, Yi L <yi.l.liu@linux.intel.com> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> --- >> v3 -> v4: >> - restore ATTACH/DETACH >> - add unwind on failure >> >> v2 -> v3: >> - s/BIND_PASID_TABLE/SET_PASID_TABLE >> >> v1 -> v2: >> - s/BIND_GUEST_STAGE/BIND_PASID_TABLE >> - remove the struct device arg >> --- >> drivers/vfio/vfio_iommu_type1.c | 53 +++++++++++++++++++++++++++++++++ >> include/uapi/linux/vfio.h | 17 +++++++++++ >> 2 files changed, 70 insertions(+) >> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c >> index 73652e21efec..222e9199edbf 100644 >> --- a/drivers/vfio/vfio_iommu_type1.c >> +++ b/drivers/vfio/vfio_iommu_type1.c >> @@ -1644,6 +1644,43 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu) >> return ret; >> } >> >> +static void >> +vfio_detach_pasid_table(struct vfio_iommu *iommu) >> +{ >> + struct vfio_domain *d; >> + >> + mutex_lock(&iommu->lock); >> + >> + list_for_each_entry(d, &iommu->domain_list, next) { >> + iommu_detach_pasid_table(d->domain); >> + } >> + mutex_unlock(&iommu->lock); >> +} >> + >> +static int >> +vfio_attach_pasid_table(struct vfio_iommu *iommu, >> + struct vfio_iommu_type1_attach_pasid_table *ustruct) >> +{ >> + struct vfio_domain *d; >> + int ret = 0; >> + >> + mutex_lock(&iommu->lock); >> + >> + list_for_each_entry(d, &iommu->domain_list, next) { >> + ret = iommu_attach_pasid_table(d->domain, &ustruct->config); >> + if (ret) >> + goto unwind; >> + } >> + goto unlock; >> +unwind: >> + list_for_each_entry_continue_reverse(d, &iommu->domain_list, next) { >> + iommu_detach_pasid_table(d->domain); >> + } >> +unlock: >> + mutex_unlock(&iommu->lock); >> + return ret; >> +} >> + >> static long vfio_iommu_type1_ioctl(void *iommu_data, >> unsigned int cmd, unsigned long arg) >> { >> @@ -1714,6 +1751,22 @@ static long vfio_iommu_type1_ioctl(void *iommu_data, >> >> return copy_to_user((void __user *)arg, &unmap, minsz) ? >> -EFAULT : 0; >> + } else if (cmd == VFIO_IOMMU_ATTACH_PASID_TABLE) { >> + struct vfio_iommu_type1_attach_pasid_table ustruct; >> + >> + minsz = offsetofend(struct vfio_iommu_type1_attach_pasid_table, >> + config); >> + >> + if (copy_from_user(&ustruct, (void __user *)arg, minsz)) >> + return -EFAULT; >> + >> + if (ustruct.argsz < minsz || ustruct.flags) >> + return -EINVAL; > > Who is responsible for validating the ustruct.config? > arm_smmu_attach_pasid_table() only looks at the format, ignoring the > version field :-\ In fact, where is struct iommu_pasid_smmuv3 ever used > from the config? This is something missing and to be fixed in smmuv3 arm_smmu_attach_pasid_table(). At the moment the virtual SMMUv3 only supports a single context descriptor hence the shortcut. Should the padding fields be forced to zero? We > don't have flags to incorporate use of them with future extensions, so > maybe we don't care? My guess is if we were to add new fields in iommu_pasid_smmuv3, we would both increment iommu_pasid_smmuv3.version and iommu_pasid_table_config.version. I don't think padding fields are meant to be filled here (ie. no flag needed). > >> + >> + return vfio_attach_pasid_table(iommu, &ustruct); >> + } else if (cmd == VFIO_IOMMU_DETACH_PASID_TABLE) { >> + vfio_detach_pasid_table(iommu); >> + return 0; >> } >> >> return -ENOTTY; >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h >> index 02bb7ad6e986..329d378565d9 100644 >> --- a/include/uapi/linux/vfio.h >> +++ b/include/uapi/linux/vfio.h >> @@ -14,6 +14,7 @@ >> >> #include <linux/types.h> >> #include <linux/ioctl.h> >> +#include <linux/iommu.h> >> >> #define VFIO_API_VERSION 0 >> >> @@ -759,6 +760,22 @@ struct vfio_iommu_type1_dma_unmap { >> #define VFIO_IOMMU_ENABLE _IO(VFIO_TYPE, VFIO_BASE + 15) >> #define VFIO_IOMMU_DISABLE _IO(VFIO_TYPE, VFIO_BASE + 16) >> >> +/** >> + * VFIO_IOMMU_ATTACH_PASID_TABLE - _IOWR(VFIO_TYPE, VFIO_BASE + 22, >> + * struct vfio_iommu_type1_attach_pasid_table) >> + * >> + * Passes the PASID table to the host. Calling ATTACH_PASID_TABLE >> + * while a table is already installed is allowed: it replaces the old >> + * table. DETACH does a comprehensive tear down of the nested mode. >> + */ >> +struct vfio_iommu_type1_attach_pasid_table { >> + __u32 argsz; >> + __u32 flags; >> + struct iommu_pasid_table_config config; >> +}; >> +#define VFIO_IOMMU_ATTACH_PASID_TABLE _IO(VFIO_TYPE, VFIO_BASE + 22) >> +#define VFIO_IOMMU_DETACH_PASID_TABLE _IO(VFIO_TYPE, VFIO_BASE + 23) >> + > > DETACH should also be documented, it's not clear from the uapi that it > requires no parameters. Thanks, sure Thanks Eric > > Alex >
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 73652e21efec..222e9199edbf 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -1644,6 +1644,43 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu) return ret; } +static void +vfio_detach_pasid_table(struct vfio_iommu *iommu) +{ + struct vfio_domain *d; + + mutex_lock(&iommu->lock); + + list_for_each_entry(d, &iommu->domain_list, next) { + iommu_detach_pasid_table(d->domain); + } + mutex_unlock(&iommu->lock); +} + +static int +vfio_attach_pasid_table(struct vfio_iommu *iommu, + struct vfio_iommu_type1_attach_pasid_table *ustruct) +{ + struct vfio_domain *d; + int ret = 0; + + mutex_lock(&iommu->lock); + + list_for_each_entry(d, &iommu->domain_list, next) { + ret = iommu_attach_pasid_table(d->domain, &ustruct->config); + if (ret) + goto unwind; + } + goto unlock; +unwind: + list_for_each_entry_continue_reverse(d, &iommu->domain_list, next) { + iommu_detach_pasid_table(d->domain); + } +unlock: + mutex_unlock(&iommu->lock); + return ret; +} + static long vfio_iommu_type1_ioctl(void *iommu_data, unsigned int cmd, unsigned long arg) { @@ -1714,6 +1751,22 @@ static long vfio_iommu_type1_ioctl(void *iommu_data, return copy_to_user((void __user *)arg, &unmap, minsz) ? -EFAULT : 0; + } else if (cmd == VFIO_IOMMU_ATTACH_PASID_TABLE) { + struct vfio_iommu_type1_attach_pasid_table ustruct; + + minsz = offsetofend(struct vfio_iommu_type1_attach_pasid_table, + config); + + if (copy_from_user(&ustruct, (void __user *)arg, minsz)) + return -EFAULT; + + if (ustruct.argsz < minsz || ustruct.flags) + return -EINVAL; + + return vfio_attach_pasid_table(iommu, &ustruct); + } else if (cmd == VFIO_IOMMU_DETACH_PASID_TABLE) { + vfio_detach_pasid_table(iommu); + return 0; } return -ENOTTY; diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index 02bb7ad6e986..329d378565d9 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -14,6 +14,7 @@ #include <linux/types.h> #include <linux/ioctl.h> +#include <linux/iommu.h> #define VFIO_API_VERSION 0 @@ -759,6 +760,22 @@ struct vfio_iommu_type1_dma_unmap { #define VFIO_IOMMU_ENABLE _IO(VFIO_TYPE, VFIO_BASE + 15) #define VFIO_IOMMU_DISABLE _IO(VFIO_TYPE, VFIO_BASE + 16) +/** + * VFIO_IOMMU_ATTACH_PASID_TABLE - _IOWR(VFIO_TYPE, VFIO_BASE + 22, + * struct vfio_iommu_type1_attach_pasid_table) + * + * Passes the PASID table to the host. Calling ATTACH_PASID_TABLE + * while a table is already installed is allowed: it replaces the old + * table. DETACH does a comprehensive tear down of the nested mode. + */ +struct vfio_iommu_type1_attach_pasid_table { + __u32 argsz; + __u32 flags; + struct iommu_pasid_table_config config; +}; +#define VFIO_IOMMU_ATTACH_PASID_TABLE _IO(VFIO_TYPE, VFIO_BASE + 22) +#define VFIO_IOMMU_DETACH_PASID_TABLE _IO(VFIO_TYPE, VFIO_BASE + 23) + /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */ /*