[PATCH 0/2] VFIO: Accept IOMMU group (PE) ID

[PATCH 0/2] VFIO: Accept IOMMU group (PE) ID

[Gavin Shan]

This allows to accept IOMMU group (PE) ID from the parameter from userland
when handling EEH operation so that the operation only affects the target
IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from userland
is invalid, all IOMMU groups (PEs) attached to the specified container are
affected as before.

Gavin Shan (2):
  drivers/vfio: Support EEH API revision
  drivers/vfio: Support IOMMU group for EEH operations

 drivers/vfio/vfio_iommu_spapr_tce.c | 50 ++++++++++++++++++++++++++++++++++---
 drivers/vfio/vfio_spapr_eeh.c       | 46 ++++++++++++++++++++++------------
 include/linux/vfio.h                | 13 +++++++---
 include/uapi/linux/vfio.h           |  6 +++++
 4 files changed, 93 insertions(+), 22 deletions(-)

-- 
2.1.0

[PATCH 1/2] drivers/vfio: Support EEH API revision

[Gavin Shan]

This extends the return value from container's IOCTL command
(VFIO_CHECK_EXTENSION + VFIO_EEH) to EEH API revision. Also,
extra check is applied to return -ENOTTY if EEH functionality
is disabled in vfio_spapr_iommu_eeh_ioctl().

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 drivers/vfio/vfio_iommu_spapr_tce.c | 5 +++++
 drivers/vfio/vfio_spapr_eeh.c       | 9 ++++++++-
 include/linux/vfio.h                | 6 ++++++
 include/uapi/linux/vfio.h           | 3 +++
 4 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index 0582b72..812b43b 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -954,6 +954,11 @@ static long tce_iommu_ioctl(void *iommu_data,
 
 	case VFIO_EEH_PE_OP: {
 		struct tce_iommu_group *tcegrp;
+		int eeh_enabled;
+
+		eeh_enabled = vfio_spapr_pci_eeh_enabled();
+		if (eeh_enabled == VFIO_EEH_DISABLED)
+			return -ENOTTY;
 
 		ret = 0;
 		list_for_each_entry(tcegrp, &container->group_list, next) {
diff --git a/drivers/vfio/vfio_spapr_eeh.c b/drivers/vfio/vfio_spapr_eeh.c
index 38edeb4..d208d77 100644
--- a/drivers/vfio/vfio_spapr_eeh.c
+++ b/drivers/vfio/vfio_spapr_eeh.c
@@ -18,6 +18,12 @@
 #define DRIVER_AUTHOR	"Gavin Shan, IBM Corporation"
 #define DRIVER_DESC	"VFIO IOMMU SPAPR EEH"
 
+int vfio_spapr_pci_eeh_enabled(void)
+{
+	return VFIO_EEH_ENABLED_V1;
+}
+EXPORT_SYMBOL_GPL(vfio_spapr_pci_eeh_enabled);
+
 /* We might build address mapping here for "fast" path later */
 void vfio_spapr_pci_eeh_open(struct pci_dev *pdev)
 {
@@ -42,7 +48,8 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
 	switch (cmd) {
 	case VFIO_CHECK_EXTENSION:
 		if (arg == VFIO_EEH)
-			ret = eeh_enabled() ? 1 : 0;
+			ret = eeh_enabled() ? vfio_spapr_pci_eeh_enabled() :
+					      VFIO_EEH_DISABLED;
 		else
 			ret = 0;
 		break;
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index ddb4409..ff036ca 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -91,12 +91,18 @@ extern long vfio_external_check_extension(struct vfio_group *group,
 
 struct pci_dev;
 #ifdef CONFIG_EEH
+extern int vfio_spapr_pci_eeh_enabled(void);
 extern void vfio_spapr_pci_eeh_open(struct pci_dev *pdev);
 extern void vfio_spapr_pci_eeh_release(struct pci_dev *pdev);
 extern long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
 				       unsigned int cmd,
 				       unsigned long arg);
 #else
+static inline int vfio_spapr_pci_eeh_enabled(void)
+{
+	return VFIO_EEH_DISABLED;
+}
+
 static inline void vfio_spapr_pci_eeh_open(struct pci_dev *pdev)
 {
 }
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 9fd7b5d..74f5b8b 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -495,6 +495,9 @@ struct vfio_iommu_spapr_tce_info {
  * - configure PE;
  * - inject EEH error.
  */
+#define VFIO_EEH_DISABLED	0
+#define VFIO_EEH_ENABLED_V1	1
+
 struct vfio_eeh_pe_err {
 	__u32 type;
 	__u32 func;
-- 
2.1.0

[PATCH 2/2] drivers/vfio: Support IOMMU group for EEH operations

[Gavin Shan]

Currently, EEH module works based on the assumption that every
container has only one attached IOMMU group. It's not true any
more. So the userland has to specify the IOMMU group (PE) to
which the requested EEH operation is applied.

This exposes "v2" interface for the userland to specify IOMMU
group (PE) ID when requesting EEH operation.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 drivers/vfio/vfio_iommu_spapr_tce.c | 51 ++++++++++++++++++++++++++++++++-----
 drivers/vfio/vfio_spapr_eeh.c       | 39 ++++++++++++++++------------
 include/linux/vfio.h                |  7 ++---
 include/uapi/linux/vfio.h           |  3 +++
 4 files changed, 75 insertions(+), 25 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index 812b43b..f85bde7 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -724,7 +724,8 @@ static long tce_iommu_ioctl(void *iommu_data,
 			ret = 1;
 			break;
 		default:
-			ret = vfio_spapr_iommu_eeh_ioctl(NULL, cmd, arg);
+			ret = vfio_spapr_iommu_eeh_ioctl(NULL, cmd,
+							 arg, NULL, 0);
 			break;
 		}
 
@@ -953,17 +954,55 @@ static long tce_iommu_ioctl(void *iommu_data,
 		return 0;
 
 	case VFIO_EEH_PE_OP: {
-		struct tce_iommu_group *tcegrp;
-		int eeh_enabled;
+		struct tce_iommu_group *tmp, *tcegrp;
+		struct vfio_eeh_pe_op op;
+		int enabled, flag;
+
+		enabled = vfio_spapr_pci_eeh_enabled();
+		if (enabled == VFIO_EEH_DISABLED)
+			return -ENOTTY;
 
-		eeh_enabled = vfio_spapr_pci_eeh_enabled();
-		if (eeh_enabled == VFIO_EEH_DISABLED)
+		/* Get the specified version */
+		minsz = offsetofend(struct vfio_eeh_pe_op, flags);
+		if (copy_from_user(&op, (void __user *)arg, minsz))
+			return -EFAULT;
+		flag = (op.flags & VFIO_EEH_ENABLED_MASK);
+		if (flag > enabled)
 			return -ENOTTY;
+		else if (flag == VFIO_EEH_DISABLED)
+			flag = VFIO_EEH_ENABLED_V1;
+
+		if (flag == VFIO_EEH_ENABLED_V1)
+			minsz = offsetofend(struct vfio_eeh_pe_op, op);
+		else if (flag == VFIO_EEH_ENABLED_V2)
+			minsz = offsetofend(struct vfio_eeh_pe_op, groupid);
+		if (copy_from_user(&op, (void __user *)arg, minsz))
+			return -EFAULT;
+
+		if (op.argsz < minsz)
+			return -EINVAL;
+
+		if (flag == VFIO_EEH_ENABLED_V2) {
+			tcegrp = NULL;
+			list_for_each_entry(tmp, &container->group_list, next) {
+				if (tmp->grp &&
+				    iommu_group_id(tmp->grp) == op.groupid) {
+					tcegrp = tmp;
+					break;
+				}
+			}
+
+			if (!tcegrp)
+				return -ENODEV;
+
+			return vfio_spapr_iommu_eeh_ioctl(tcegrp->grp,
+					cmd, arg, &op, flag);
+		}
 
 		ret = 0;
 		list_for_each_entry(tcegrp, &container->group_list, next) {
 			ret = vfio_spapr_iommu_eeh_ioctl(tcegrp->grp,
-					cmd, arg);
+					cmd, arg, &op, flag);
 			if (ret)
 				return ret;
 		}
diff --git a/drivers/vfio/vfio_spapr_eeh.c b/drivers/vfio/vfio_spapr_eeh.c
index d208d77..e77dcb8 100644
--- a/drivers/vfio/vfio_spapr_eeh.c
+++ b/drivers/vfio/vfio_spapr_eeh.c
@@ -20,7 +20,7 @@
 
 int vfio_spapr_pci_eeh_enabled(void)
 {
-	return VFIO_EEH_ENABLED_V1;
+	return VFIO_EEH_ENABLED_V2;
 }
 EXPORT_SYMBOL_GPL(vfio_spapr_pci_eeh_enabled);
 
@@ -38,11 +38,12 @@ void vfio_spapr_pci_eeh_release(struct pci_dev *pdev)
 EXPORT_SYMBOL_GPL(vfio_spapr_pci_eeh_release);
 
 long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
-				unsigned int cmd, unsigned long arg)
+				unsigned int cmd, unsigned long arg,
+				void *parm, int flag)
 {
 	struct eeh_pe *pe;
-	struct vfio_eeh_pe_op op;
-	unsigned long minsz;
+	struct vfio_eeh_pe_op *op;
+	unsigned long src, dst, len;
 	long ret = -EINVAL;
 
 	switch (cmd) {
@@ -54,17 +55,12 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
 			ret = 0;
 		break;
 	case VFIO_EEH_PE_OP:
+		op = (struct vfio_eeh_pe_op *)parm;
 		pe = eeh_iommu_group_to_pe(group);
 		if (!pe)
 			return -ENODEV;
 
-		minsz = offsetofend(struct vfio_eeh_pe_op, op);
-		if (copy_from_user(&op, (void __user *)arg, minsz))
-			return -EFAULT;
-		if (op.argsz < minsz || op.flags)
-			return -EINVAL;
-
-		switch (op.op) {
+		switch (op->op) {
 		case VFIO_EEH_PE_DISABLE:
 			ret = eeh_pe_set_option(pe, EEH_OPT_DISABLE);
 			break;
@@ -93,14 +89,25 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
 			ret = eeh_pe_configure(pe);
 			break;
 		case VFIO_EEH_PE_INJECT_ERR:
-			minsz = offsetofend(struct vfio_eeh_pe_op, err.mask);
-			if (op.argsz < minsz)
+			if (flag == VFIO_EEH_ENABLED_V1)
+				src = offsetofend(struct vfio_eeh_pe_op, op);
+			else if (flag == VFIO_EEH_ENABLED_V2)
+				src = offsetofend(struct vfio_eeh_pe_op,
+						  groupid);
+			else
+				return -ENOTTY;
+
+			len = sizeof(struct vfio_eeh_pe_err);
+			if (op->argsz < src + len)
 				return -EINVAL;
-			if (copy_from_user(&op, (void __user *)arg, minsz))
+
+			dst = offsetofend(struct vfio_eeh_pe_op, groupid);
+			if (copy_from_user(parm + dst,
+				(void __user *)(arg + src), len))
 				return -EFAULT;
 
-			ret = eeh_pe_inject_err(pe, op.err.type, op.err.func,
-						op.err.addr, op.err.mask);
+			ret = eeh_pe_inject_err(pe, op->err.type, op->err.func,
+						op->err.addr, op->err.mask);
 			break;
 		default:
 			ret = -EINVAL;
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index ff036ca..c004307 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -95,8 +95,8 @@ extern int vfio_spapr_pci_eeh_enabled(void);
 extern void vfio_spapr_pci_eeh_open(struct pci_dev *pdev);
 extern void vfio_spapr_pci_eeh_release(struct pci_dev *pdev);
 extern long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
-				       unsigned int cmd,
-				       unsigned long arg);
+				       unsigned int cmd, unsigned long arg,
+				       void *param, int flag);
 #else
 static inline int vfio_spapr_pci_eeh_enabled(void)
 {
@@ -113,7 +113,8 @@ static inline void vfio_spapr_pci_eeh_release(struct pci_dev *pdev)
 
 static inline long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
 					      unsigned int cmd,
-					      unsigned long arg)
+					      unsigned long arg,
+					      void *param, int flag)
 {
 	return -ENOTTY;
 }
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 74f5b8b..66ded6b 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -497,6 +497,7 @@ struct vfio_iommu_spapr_tce_info {
  */
 #define VFIO_EEH_DISABLED	0
 #define VFIO_EEH_ENABLED_V1	1
+#define VFIO_EEH_ENABLED_V2	2
 
 struct vfio_eeh_pe_err {
 	__u32 type;
@@ -508,7 +509,9 @@ struct vfio_eeh_pe_err {
 struct vfio_eeh_pe_op {
 	__u32 argsz;
 	__u32 flags;
+#define VFIO_EEH_ENABLED_MASK	0xFF
 	__u32 op;
+	__u32 groupid;
 	union {
 		struct vfio_eeh_pe_err err;
 	};
-- 
2.1.0

Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID

[Alex Williamson]

On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote:
> This allows to accept IOMMU group (PE) ID from the parameter from userland
> when handling EEH operation so that the operation only affects the target
> IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from userland
> is invalid, all IOMMU groups (PEs) attached to the specified container are
> affected as before.
> 
> Gavin Shan (2):
>   drivers/vfio: Support EEH API revision
>   drivers/vfio: Support IOMMU group for EEH operations
> 
>  drivers/vfio/vfio_iommu_spapr_tce.c | 50 ++++++++++++++++++++++++++++++++++---
>  drivers/vfio/vfio_spapr_eeh.c       | 46 ++++++++++++++++++++++------------
>  include/linux/vfio.h                | 13 +++++++---
>  include/uapi/linux/vfio.h           |  6 +++++
>  4 files changed, 93 insertions(+), 22 deletions(-)

This interface is terrible.  A function named foo_enabled() should
return a bool, yes or no, don't try to overload it to also return a
version.  AFAICT, patch 2/2 breaks current users by changing the offset
of the union in struct vfio_eeh_pe_err.  Also, we generally pass group
file descriptors rather than a group ID because we can prove the
ownership of the group through the file descriptor and we don't need to
worry about races with the group because we can hold a reference to it.

Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1999 bytes --]

On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote:
> On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote:
> > This allows to accept IOMMU group (PE) ID from the parameter from userland
> > when handling EEH operation so that the operation only affects the target
> > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from userland
> > is invalid, all IOMMU groups (PEs) attached to the specified container are
> > affected as before.
> > 
> > Gavin Shan (2):
> >   drivers/vfio: Support EEH API revision
> >   drivers/vfio: Support IOMMU group for EEH operations
> > 
> >  drivers/vfio/vfio_iommu_spapr_tce.c | 50 ++++++++++++++++++++++++++++++++++---
> >  drivers/vfio/vfio_spapr_eeh.c       | 46 ++++++++++++++++++++++------------
> >  include/linux/vfio.h                | 13 +++++++---
> >  include/uapi/linux/vfio.h           |  6 +++++
> >  4 files changed, 93 insertions(+), 22 deletions(-)
> 
> This interface is terrible.  A function named foo_enabled() should
> return a bool, yes or no, don't try to overload it to also return a
> version.

Sorry, that one's my fault.  I suggested that approach to Gavin
without really thinking it through.


> AFAICT, patch 2/2 breaks current users by changing the offset
> of the union in struct vfio_eeh_pe_err.

Yeah, this one's ugly.  We have to preserve the offset, but that means
putting the group in a very awkward place.  Especially since I'm not
sure if there even are any existing users of the single extant union
branch.

Sigh.

> Also, we generally pass group
> file descriptors rather than a group ID because we can prove the
> ownership of the group through the file descriptor and we don't need to
> worry about races with the group because we can hold a reference to it.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2582 bytes --]

On Sat, Sep 19, 2015 at 04:22:47PM +1000, David Gibson wrote:
> On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote:
> > On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote:
> > > This allows to accept IOMMU group (PE) ID from the parameter from userland
> > > when handling EEH operation so that the operation only affects the target
> > > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from userland
> > > is invalid, all IOMMU groups (PEs) attached to the specified container are
> > > affected as before.
> > > 
> > > Gavin Shan (2):
> > >   drivers/vfio: Support EEH API revision
> > >   drivers/vfio: Support IOMMU group for EEH operations
> > > 
> > >  drivers/vfio/vfio_iommu_spapr_tce.c | 50 ++++++++++++++++++++++++++++++++++---
> > >  drivers/vfio/vfio_spapr_eeh.c       | 46 ++++++++++++++++++++++------------
> > >  include/linux/vfio.h                | 13 +++++++---
> > >  include/uapi/linux/vfio.h           |  6 +++++
> > >  4 files changed, 93 insertions(+), 22 deletions(-)
> > 
> > This interface is terrible.  A function named foo_enabled() should
> > return a bool, yes or no, don't try to overload it to also return a
> > version.
> 
> Sorry, that one's my fault.  I suggested that approach to Gavin
> without really thinking it through.
> 
> 
> > AFAICT, patch 2/2 breaks current users by changing the offset
> > of the union in struct vfio_eeh_pe_err.
> 
> Yeah, this one's ugly.  We have to preserve the offset, but that means
> putting the group in a very awkward place.  Especially since I'm not
> sure if there even are any existing users of the single extant union
> branch.
> 
> Sigh.
> 
> > Also, we generally pass group
> > file descriptors rather than a group ID because we can prove the
> > ownership of the group through the file descriptor and we don't need to
> > worry about races with the group because we can hold a reference to it.

Duh.  I finally realised the better, simpler, obvious solution.

Rather than changing the parameter structure, we should move the
ioctl()s so they're on the group fd instead of the container fd.

Obviously we need to keep it on the container fd for backwards compat,
but I think we should just error out if there is more than one group
in the container there.

We will need a new capability too, obviously.  VFIO_EEH_GROUPFD maybe?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID

[Gavin Shan]

On Mon, Sep 21, 2015 at 11:42:28AM +1000, David Gibson wrote:
>On Sat, Sep 19, 2015 at 04:22:47PM +1000, David Gibson wrote:
>> On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote:
>> > On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote:
>> > > This allows to accept IOMMU group (PE) ID from the parameter from userland
>> > > when handling EEH operation so that the operation only affects the target
>> > > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from userland
>> > > is invalid, all IOMMU groups (PEs) attached to the specified container are
>> > > affected as before.
>> > > 
>> > > Gavin Shan (2):
>> > >   drivers/vfio: Support EEH API revision
>> > >   drivers/vfio: Support IOMMU group for EEH operations
>> > > 
>> > >  drivers/vfio/vfio_iommu_spapr_tce.c | 50 ++++++++++++++++++++++++++++++++++---
>> > >  drivers/vfio/vfio_spapr_eeh.c       | 46 ++++++++++++++++++++++------------
>> > >  include/linux/vfio.h                | 13 +++++++---
>> > >  include/uapi/linux/vfio.h           |  6 +++++
>> > >  4 files changed, 93 insertions(+), 22 deletions(-)
>> > 
>> > This interface is terrible.  A function named foo_enabled() should
>> > return a bool, yes or no, don't try to overload it to also return a
>> > version.
>> 
>> Sorry, that one's my fault.  I suggested that approach to Gavin
>> without really thinking it through.
>> 
>> 
>> > AFAICT, patch 2/2 breaks current users by changing the offset
>> > of the union in struct vfio_eeh_pe_err.
>> 
>> Yeah, this one's ugly.  We have to preserve the offset, but that means
>> putting the group in a very awkward place.  Especially since I'm not
>> sure if there even are any existing users of the single extant union
>> branch.
>> 
>> Sigh.
>> 

Yeah, Perhaps, I should have put "RFC" on the subjects because those
patches are really pre-mature and just intend to bring more discussion
on it.

>> > Also, we generally pass group
>> > file descriptors rather than a group ID because we can prove the
>> > ownership of the group through the file descriptor and we don't need to
>> > worry about races with the group because we can hold a reference to it.
>
>Duh.  I finally realised the better, simpler, obvious solution.
>
>Rather than changing the parameter structure, we should move the
>ioctl()s so they're on the group fd instead of the container fd.
>
>Obviously we need to keep it on the container fd for backwards compat,
>but I think we should just error out if there is more than one group
>in the container there.
>
>We will need a new capability too, obviously.  VFIO_EEH_GROUPFD maybe?
>

Yes, I agree to route EEH ioctl commands to group fd since EEH ioctl
commands operate on granularity of PE (IOMMU group). However, it requires
to extend current code to support that. I'm not sure if it's good idea as
I explained to David through IRC. Waiting for Alex to judge:

- Adding a callback to "struct vfio_group": platform_ioctl();
- When attaching the group to platform, this function is initialized;
- The EEH ioctl commands are routed to platform_ioctl() in vfio_group_fops_unl_ioctl()

Thanks,
Gavin

>-- 
>David Gibson			| I'll have my music baroque, and my code
>david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
>				| _way_ _around_!
>http://www.ozlabs.org/~dgibson

Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID

[Gavin Shan]

On Mon, Sep 21, 2015 at 11:42:28AM +1000, David Gibson wrote:
>On Sat, Sep 19, 2015 at 04:22:47PM +1000, David Gibson wrote:
>> On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote:
>> > On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote:
>> > > This allows to accept IOMMU group (PE) ID from the parameter from userland
>> > > when handling EEH operation so that the operation only affects the target
>> > > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from userland
>> > > is invalid, all IOMMU groups (PEs) attached to the specified container are
>> > > affected as before.
>> > > 
>> > > Gavin Shan (2):
>> > >   drivers/vfio: Support EEH API revision
>> > >   drivers/vfio: Support IOMMU group for EEH operations
>> > > 
>> > >  drivers/vfio/vfio_iommu_spapr_tce.c | 50 ++++++++++++++++++++++++++++++++++---
>> > >  drivers/vfio/vfio_spapr_eeh.c       | 46 ++++++++++++++++++++++------------
>> > >  include/linux/vfio.h                | 13 +++++++---
>> > >  include/uapi/linux/vfio.h           |  6 +++++
>> > >  4 files changed, 93 insertions(+), 22 deletions(-)
>> > 
>> > This interface is terrible.  A function named foo_enabled() should
>> > return a bool, yes or no, don't try to overload it to also return a
>> > version.
>> 
>> Sorry, that one's my fault.  I suggested that approach to Gavin
>> without really thinking it through.
>> 
>> 
>> > AFAICT, patch 2/2 breaks current users by changing the offset
>> > of the union in struct vfio_eeh_pe_err.
>> 
>> Yeah, this one's ugly.  We have to preserve the offset, but that means
>> putting the group in a very awkward place.  Especially since I'm not
>> sure if there even are any existing users of the single extant union
>> branch.
>> 
>> Sigh.
>> 
>> > Also, we generally pass group
>> > file descriptors rather than a group ID because we can prove the
>> > ownership of the group through the file descriptor and we don't need to
>> > worry about races with the group because we can hold a reference to it.
>
>Duh.  I finally realised the better, simpler, obvious solution.
>
>Rather than changing the parameter structure, we should move the
>ioctl()s so they're on the group fd instead of the container fd.
>
>Obviously we need to keep it on the container fd for backwards compat,
>but I think we should just error out if there is more than one group
>in the container there.
>
>We will need a new capability too, obviously.  VFIO_EEH_GROUPFD maybe?
>

Yeah, the patches should be marked as "RFC" actually as they're actually
prototypes. I agree with David that the EEH ioctl commands should be routed
through IOMMU group as I proposed long time ago. However, if we're going
to do it now, we have to maintain two set the interfaces: one handled by
container's ioctl() and another one is handled by IOMMU group's ioctl().
Would it be a problem?

Actually, the code change is made based on the fact: nobody is using
the union (struct vfio_eeh_pe_err) yet before the QEMU changes to do
error injection gets merged by David. So I think it's fine to introduce
another field in struct vfio_eeh_pe_op though there is gap?

Thanks,
Gavin

Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID

[Alex Williamson]

On Mon, 2015-09-21 at 22:11 +1000, Gavin Shan wrote:
> On Mon, Sep 21, 2015 at 11:42:28AM +1000, David Gibson wrote:
> >On Sat, Sep 19, 2015 at 04:22:47PM +1000, David Gibson wrote:
> >> On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote:
> >> > On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote:
> >> > > This allows to accept IOMMU group (PE) ID from the parameter from userland
> >> > > when handling EEH operation so that the operation only affects the target
> >> > > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from userland
> >> > > is invalid, all IOMMU groups (PEs) attached to the specified container are
> >> > > affected as before.
> >> > > 
> >> > > Gavin Shan (2):
> >> > >   drivers/vfio: Support EEH API revision
> >> > >   drivers/vfio: Support IOMMU group for EEH operations
> >> > > 
> >> > >  drivers/vfio/vfio_iommu_spapr_tce.c | 50 ++++++++++++++++++++++++++++++++++---
> >> > >  drivers/vfio/vfio_spapr_eeh.c       | 46 ++++++++++++++++++++++------------
> >> > >  include/linux/vfio.h                | 13 +++++++---
> >> > >  include/uapi/linux/vfio.h           |  6 +++++
> >> > >  4 files changed, 93 insertions(+), 22 deletions(-)
> >> > 
> >> > This interface is terrible.  A function named foo_enabled() should
> >> > return a bool, yes or no, don't try to overload it to also return a
> >> > version.
> >> 
> >> Sorry, that one's my fault.  I suggested that approach to Gavin
> >> without really thinking it through.
> >> 
> >> 
> >> > AFAICT, patch 2/2 breaks current users by changing the offset
> >> > of the union in struct vfio_eeh_pe_err.
> >> 
> >> Yeah, this one's ugly.  We have to preserve the offset, but that means
> >> putting the group in a very awkward place.  Especially since I'm not
> >> sure if there even are any existing users of the single extant union
> >> branch.
> >> 
> >> Sigh.
> >> 
> >> > Also, we generally pass group
> >> > file descriptors rather than a group ID because we can prove the
> >> > ownership of the group through the file descriptor and we don't need to
> >> > worry about races with the group because we can hold a reference to it.
> >
> >Duh.  I finally realised the better, simpler, obvious solution.
> >
> >Rather than changing the parameter structure, we should move the
> >ioctl()s so they're on the group fd instead of the container fd.
> >
> >Obviously we need to keep it on the container fd for backwards compat,
> >but I think we should just error out if there is more than one group
> >in the container there.
> >
> >We will need a new capability too, obviously.  VFIO_EEH_GROUPFD maybe?
> >
> 
> Yeah, the patches should be marked as "RFC" actually as they're actually
> prototypes. I agree with David that the EEH ioctl commands should be routed
> through IOMMU group as I proposed long time ago. However, if we're going
> to do it now, we have to maintain two set the interfaces: one handled by
> container's ioctl() and another one is handled by IOMMU group's ioctl().
> Would it be a problem?
> 
> Actually, the code change is made based on the fact: nobody is using
> the union (struct vfio_eeh_pe_err) yet before the QEMU changes to do
> error injection gets merged by David. So I think it's fine to introduce
> another field in struct vfio_eeh_pe_op though there is gap?

We really need to get away from this mindset of assuming that we know
every user of the code and every dependency it may have.  The reality is
that this is an exposed ABI and we shouldn't break it just because we
don't know of any users.  Thanks,

Alex