| Message ID | 20190108102633.17482-5-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMMUv3 Nested Stage Setup | expand |
On Tue, 8 Jan 2019 11:26:16 +0100 Eric Auger <eric.auger@redhat.com> wrote: > From: "Liu, Yi L" <yi.l.liu@linux.intel.com> > > This patch adds VFIO_IOMMU_SET_PASID_TABLE ioctl which aims at > passing the virtual iommu guest configuration to 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> > > --- > 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 | 31 +++++++++++++++++++++++++++++++ > include/uapi/linux/vfio.h | 8 ++++++++ > 2 files changed, 39 insertions(+) > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 7651cfb14836..d9dd23f64f00 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -1644,6 +1644,24 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu) > return ret; > } > > +static int > +vfio_set_pasid_table(struct vfio_iommu *iommu, > + struct vfio_iommu_type1_set_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_set_pasid_table(d->domain, &ustruct->config); > + if (ret) > + break; > + } There's no unwind on failure here, leaves us in an inconsistent state should something go wrong or domains don't have homogeneous PASID support. What's expected to happen if a PASID table is already set for a domain, does it replace the old one or return -EBUSY? > + mutex_unlock(&iommu->lock); > + return ret; > +} > + > static long vfio_iommu_type1_ioctl(void *iommu_data, > unsigned int cmd, unsigned long arg) > { > @@ -1714,6 +1732,19 @@ 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_SET_PASID_TABLE) { > + struct vfio_iommu_type1_set_pasid_table ustruct; > + > + minsz = offsetofend(struct vfio_iommu_type1_set_pasid_table, > + config); > + > + if (copy_from_user(&ustruct, (void __user *)arg, minsz)) > + return -EFAULT; > + > + if (ustruct.argsz < minsz || ustruct.flags) > + return -EINVAL; > + > + return vfio_set_pasid_table(iommu, &ustruct); > } > > return -ENOTTY; > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index 02bb7ad6e986..0d9f4090c95d 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,13 @@ 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) > > +struct vfio_iommu_type1_set_pasid_table { > + __u32 argsz; > + __u32 flags; > + struct iommu_pasid_table_config config; > +}; > +#define VFIO_IOMMU_SET_PASID_TABLE _IO(VFIO_TYPE, VFIO_BASE + 22) -ENOCOMMENTS Thanks, Alex > + > /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */ > > /*
Hi Alex, On 1/11/19 11:50 PM, Alex Williamson wrote: > On Tue, 8 Jan 2019 11:26:16 +0100 > Eric Auger <eric.auger@redhat.com> wrote: > >> From: "Liu, Yi L" <yi.l.liu@linux.intel.com> >> >> This patch adds VFIO_IOMMU_SET_PASID_TABLE ioctl which aims at >> passing the virtual iommu guest configuration to 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> >> >> --- >> 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 | 31 +++++++++++++++++++++++++++++++ >> include/uapi/linux/vfio.h | 8 ++++++++ >> 2 files changed, 39 insertions(+) >> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c >> index 7651cfb14836..d9dd23f64f00 100644 >> --- a/drivers/vfio/vfio_iommu_type1.c >> +++ b/drivers/vfio/vfio_iommu_type1.c >> @@ -1644,6 +1644,24 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu) >> return ret; >> } >> >> +static int >> +vfio_set_pasid_table(struct vfio_iommu *iommu, >> + struct vfio_iommu_type1_set_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_set_pasid_table(d->domain, &ustruct->config); >> + if (ret) >> + break; >> + } > > There's no unwind on failure here, leaves us in an inconsistent state > should something go wrong or domains don't have homogeneous PASID > support. What's expected to happen if a PASID table is already set for > a domain, does it replace the old one or return -EBUSY? Effectively the setting can succeed on one domain and fail on another one, typically if one underlying SMMU does not support nested while the others support it. At the moment setting a new PASID table whereas one is installed does not return any error in the SMMU code, just overwrite the associated fields in the stream table entry. This is also the way we turn nested off (cfg->bypass == true). Needs to be clarified anyway. > >> + mutex_unlock(&iommu->lock); >> + return ret; >> +} >> + >> static long vfio_iommu_type1_ioctl(void *iommu_data, >> unsigned int cmd, unsigned long arg) >> { >> @@ -1714,6 +1732,19 @@ 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_SET_PASID_TABLE) { >> + struct vfio_iommu_type1_set_pasid_table ustruct; >> + >> + minsz = offsetofend(struct vfio_iommu_type1_set_pasid_table, >> + config); >> + >> + if (copy_from_user(&ustruct, (void __user *)arg, minsz)) >> + return -EFAULT; >> + >> + if (ustruct.argsz < minsz || ustruct.flags) >> + return -EINVAL; >> + >> + return vfio_set_pasid_table(iommu, &ustruct); >> } >> >> return -ENOTTY; >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h >> index 02bb7ad6e986..0d9f4090c95d 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,13 @@ 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) >> >> +struct vfio_iommu_type1_set_pasid_table { >> + __u32 argsz; >> + __u32 flags; >> + struct iommu_pasid_table_config config; >> +}; >> +#define VFIO_IOMMU_SET_PASID_TABLE _IO(VFIO_TYPE, VFIO_BASE + 22) > > -ENOCOMMENTS Thanks, sure Thanks Eric > > Alex > >> + >> /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */ >> >> /* >
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 7651cfb14836..d9dd23f64f00 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -1644,6 +1644,24 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu) return ret; } +static int +vfio_set_pasid_table(struct vfio_iommu *iommu, + struct vfio_iommu_type1_set_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_set_pasid_table(d->domain, &ustruct->config); + if (ret) + break; + } + mutex_unlock(&iommu->lock); + return ret; +} + static long vfio_iommu_type1_ioctl(void *iommu_data, unsigned int cmd, unsigned long arg) { @@ -1714,6 +1732,19 @@ 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_SET_PASID_TABLE) { + struct vfio_iommu_type1_set_pasid_table ustruct; + + minsz = offsetofend(struct vfio_iommu_type1_set_pasid_table, + config); + + if (copy_from_user(&ustruct, (void __user *)arg, minsz)) + return -EFAULT; + + if (ustruct.argsz < minsz || ustruct.flags) + return -EINVAL; + + return vfio_set_pasid_table(iommu, &ustruct); } return -ENOTTY; diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index 02bb7ad6e986..0d9f4090c95d 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,13 @@ 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) +struct vfio_iommu_type1_set_pasid_table { + __u32 argsz; + __u32 flags; + struct iommu_pasid_table_config config; +}; +#define VFIO_IOMMU_SET_PASID_TABLE _IO(VFIO_TYPE, VFIO_BASE + 22) + /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */ /*