[PATCH v4 0/4] EEH Error Injection Support for VFIO Devices

[PATCH v4 0/4] EEH Error Injection Support for VFIO Devices

[Gavin Shan]

The series of patches are extention to EEH support for VFIO PCI devices,
which allows to inject EEH errors to VFIO PCI devices from userspace
for testing purpose.

Changelog
=========
v3 -> v4:
        * Move constants for EEH PE states defined in uapi/linux/vfio.h
          to uapi/asm/eeh.h.
v2 -> v3:
        * Use offsetofend(), instead of sizeof(struct vfio_eeh_pe_op)
          to calculate argument buffer size.
v1 -> v2:
        * Use EEH_ERR_FUNC_{MIN,MAX} to validate PCI error function.
        * Put additional arguments for error injection to union in
          struct vfio_eeh_pe_op.

Gavin Shan (4):
  powerpc/eeh: Eliminate duplicated PE states
  powerpc/eeh: Introduce eeh_pe_inject_err()
  drivers/vfio: Support EEH error injection
  drivers/vfio: Remove duplicated PE states

 Documentation/vfio.txt              | 12 ++++++++
 arch/powerpc/include/asm/eeh.h      |  9 +++---
 arch/powerpc/include/uapi/asm/eeh.h | 56 +++++++++++++++++++++++++++++++++++++
 arch/powerpc/kernel/eeh.c           | 35 +++++++++++++++++++++++
 drivers/vfio/vfio_spapr_eeh.c       | 10 +++++++
 include/uapi/linux/vfio.h           | 19 +++++++++----
 6 files changed, 130 insertions(+), 11 deletions(-)
 create mode 100644 arch/powerpc/include/uapi/asm/eeh.h

-- 
1.8.3.2

[PATCH v4 1/4] powerpc/eeh: Eliminate duplicated PE states

[Gavin Shan]

There are two equivalent sets of constants for PE states, defined
in arch/powerpc/include/asm/eeh.h and include/uapi/linux/vfio.h.
The former is used by EEH core and the latter is used by userspace.
The patch moves those constants from arch/powerpc/include/asm/eeh.h
to arch/powerpc/include/uapi/asm/eeh.h to avoid maintaining two
equivalent sets of constants.

Suggested-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/eeh.h      |  7 ++-----
 arch/powerpc/include/uapi/asm/eeh.h | 30 ++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 5 deletions(-)
 create mode 100644 arch/powerpc/include/uapi/asm/eeh.h

diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index d777c0c..08c4042 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -27,6 +27,8 @@
 #include <linux/time.h>
 #include <linux/atomic.h>
 
+#include <uapi/asm/eeh.h>
+
 struct pci_dev;
 struct pci_bus;
 
@@ -184,11 +186,6 @@ enum {
 #define EEH_STATE_DMA_ACTIVE	(1 << 4)	/* Active DMA		*/
 #define EEH_STATE_MMIO_ENABLED	(1 << 5)	/* MMIO enabled		*/
 #define EEH_STATE_DMA_ENABLED	(1 << 6)	/* DMA enabled		*/
-#define EEH_PE_STATE_NORMAL		0	/* Normal state		*/
-#define EEH_PE_STATE_RESET		1	/* PE reset asserted	*/
-#define EEH_PE_STATE_STOPPED_IO_DMA	2	/* Frozen PE		*/
-#define EEH_PE_STATE_STOPPED_DMA	4	/* Stopped DMA, Enabled IO */
-#define EEH_PE_STATE_UNAVAIL		5	/* Unavailable		*/
 #define EEH_RESET_DEACTIVATE	0	/* Deactivate the PE reset	*/
 #define EEH_RESET_HOT		1	/* Hot reset			*/
 #define EEH_RESET_FUNDAMENTAL	3	/* Fundamental reset		*/
diff --git a/arch/powerpc/include/uapi/asm/eeh.h b/arch/powerpc/include/uapi/asm/eeh.h
new file mode 100644
index 0000000..8bb34b0
--- /dev/null
+++ b/arch/powerpc/include/uapi/asm/eeh.h
@@ -0,0 +1,30 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ * Copyright IBM Corp. 2015
+ *
+ * Authors: Gavin Shan <gwshan@linux.vnet.ibm.com>
+ */
+
+#ifndef _ASM_POWERPC_EEH_H
+#define _ASM_POWERPC_EEH_H
+
+/* PE states */
+#define EEH_PE_STATE_NORMAL		0	/* Normal state		*/
+#define EEH_PE_STATE_RESET		1	/* PE reset asserted	*/
+#define EEH_PE_STATE_STOPPED_IO_DMA	2	/* Frozen PE		*/
+#define EEH_PE_STATE_STOPPED_DMA	4	/* Stopped DMA only	*/
+#define EEH_PE_STATE_UNAVAIL		5	/* Unavailable		*/
+
+#endif /* _ASM_POWERPC_EEH_H */
-- 
1.8.3.2

[PATCH v4 2/4] powerpc/eeh: Introduce eeh_pe_inject_err()

[Gavin Shan]

The patch defines PCI error types and functions in uapi/asm/eeh.h
and exports function eeh_pe_inject_err(), which will be called by
VFIO driver to inject the specified PCI error to the indicated
PE for testing purpose.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 arch/powerpc/include/asm/eeh.h      |  2 ++
 arch/powerpc/include/uapi/asm/eeh.h | 26 ++++++++++++++++++++++++++
 arch/powerpc/kernel/eeh.c           | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)

diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 08c4042..cd6003b 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -291,6 +291,8 @@ int eeh_pe_set_option(struct eeh_pe *pe, int option);
 int eeh_pe_get_state(struct eeh_pe *pe);
 int eeh_pe_reset(struct eeh_pe *pe, int option);
 int eeh_pe_configure(struct eeh_pe *pe);
+int eeh_pe_inject_err(struct eeh_pe *pe, int type, int func,
+		      unsigned long addr, unsigned long mask);
 
 /**
  * EEH_POSSIBLE_ERROR() -- test for possible MMIO failure.
diff --git a/arch/powerpc/include/uapi/asm/eeh.h b/arch/powerpc/include/uapi/asm/eeh.h
index 8bb34b0..291b7d1 100644
--- a/arch/powerpc/include/uapi/asm/eeh.h
+++ b/arch/powerpc/include/uapi/asm/eeh.h
@@ -27,4 +27,30 @@
 #define EEH_PE_STATE_STOPPED_DMA	4	/* Stopped DMA only	*/
 #define EEH_PE_STATE_UNAVAIL		5	/* Unavailable		*/
 
+/* EEH error types and functions */
+#define EEH_ERR_TYPE_32			0       /* 32-bits error	*/
+#define EEH_ERR_TYPE_64			1       /* 64-bits error	*/
+#define EEH_ERR_FUNC_MIN		0
+#define EEH_ERR_FUNC_LD_MEM_ADDR	0	/* Memory load	*/
+#define EEH_ERR_FUNC_LD_MEM_DATA	1
+#define EEH_ERR_FUNC_LD_IO_ADDR		2	/* IO load	*/
+#define EEH_ERR_FUNC_LD_IO_DATA		3
+#define EEH_ERR_FUNC_LD_CFG_ADDR	4	/* Config load	*/
+#define EEH_ERR_FUNC_LD_CFG_DATA	5
+#define EEH_ERR_FUNC_ST_MEM_ADDR	6	/* Memory store	*/
+#define EEH_ERR_FUNC_ST_MEM_DATA	7
+#define EEH_ERR_FUNC_ST_IO_ADDR		8	/* IO store	*/
+#define EEH_ERR_FUNC_ST_IO_DATA		9
+#define EEH_ERR_FUNC_ST_CFG_ADDR	10	/* Config store	*/
+#define EEH_ERR_FUNC_ST_CFG_DATA	11
+#define EEH_ERR_FUNC_DMA_RD_ADDR	12	/* DMA read	*/
+#define EEH_ERR_FUNC_DMA_RD_DATA	13
+#define EEH_ERR_FUNC_DMA_RD_MASTER	14
+#define EEH_ERR_FUNC_DMA_RD_TARGET	15
+#define EEH_ERR_FUNC_DMA_WR_ADDR	16	/* DMA write	*/
+#define EEH_ERR_FUNC_DMA_WR_DATA	17
+#define EEH_ERR_FUNC_DMA_WR_MASTER	18
+#define EEH_ERR_FUNC_DMA_WR_TARGET	19
+#define EEH_ERR_FUNC_MAX		19
+
 #endif /* _ASM_POWERPC_EEH_H */
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 76253eb..daa68a1 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1636,6 +1636,41 @@ int eeh_pe_configure(struct eeh_pe *pe)
 }
 EXPORT_SYMBOL_GPL(eeh_pe_configure);
 
+/**
+ * eeh_pe_inject_err - Injecting the specified PCI error to the indicated PE
+ * @pe: the indicated PE
+ * @type: error type
+ * @function: error function
+ * @addr: address
+ * @mask: address mask
+ *
+ * The routine is called to inject the specified PCI error, which
+ * is determined by @type and @function, to the indicated PE for
+ * testing purpose.
+ */
+int eeh_pe_inject_err(struct eeh_pe *pe, int type, int func,
+		      unsigned long addr, unsigned long mask)
+{
+	/* Invalid PE ? */
+	if (!pe)
+		return -ENODEV;
+
+	/* Unsupported operation ? */
+	if (!eeh_ops || !eeh_ops->err_inject)
+		return -ENOENT;
+
+	/* Check on PCI error type */
+	if (type != EEH_ERR_TYPE_32 && type != EEH_ERR_TYPE_64)
+		return -EINVAL;
+
+	/* Check on PCI error function */
+	if (func < EEH_ERR_FUNC_MIN || func > EEH_ERR_FUNC_MAX)
+		return -EINVAL;
+
+	return eeh_ops->err_inject(pe, type, func, addr, mask);
+}
+EXPORT_SYMBOL_GPL(eeh_pe_inject_err);
+
 static int proc_eeh_show(struct seq_file *m, void *v)
 {
 	if (!eeh_enabled()) {
-- 
1.8.3.2

[PATCH v4 3/4] drivers/vfio: Support EEH error injection

[Gavin Shan]

The patch adds one more EEH sub-command (VFIO_EEH_PE_INJECT_ERR)
to inject the specified EEH error, which is represented by
(struct vfio_eeh_pe_err), to the indicated PE for testing purpose.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 Documentation/vfio.txt        | 12 ++++++++++++
 drivers/vfio/vfio_spapr_eeh.c | 10 ++++++++++
 include/uapi/linux/vfio.h     | 14 +++++++++++++-
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
index 96978ec..4c746a7 100644
--- a/Documentation/vfio.txt
+++ b/Documentation/vfio.txt
@@ -385,6 +385,18 @@ The code flow from the example above should be slightly changed:
 
 	....
 
+	/* Inject EEH error, which is expected to be caused by 32-bits
+	 * config load.
+	 */
+	pe_op.op = VFIO_EEH_PE_INJECT_ERR;
+	pe_op.err.type = EEH_ERR_TYPE_32;
+	pe_op.err.func = EEH_ERR_FUNC_LD_CFG_ADDR;
+	pe_op.err.addr = 0ul;
+	pe_op.err.mask = 0ul;
+	ioctl(container, VFIO_EEH_PE_OP, &pe_op);
+
+	....
+
 	/* When 0xFF's returned from reading PCI config space or IO BARs
 	 * of the PCI device. Check the PE's state to see if that has been
 	 * frozen.
diff --git a/drivers/vfio/vfio_spapr_eeh.c b/drivers/vfio/vfio_spapr_eeh.c
index 5fa42db..38edeb4 100644
--- a/drivers/vfio/vfio_spapr_eeh.c
+++ b/drivers/vfio/vfio_spapr_eeh.c
@@ -85,6 +85,16 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
 		case VFIO_EEH_PE_CONFIGURE:
 			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)
+				return -EINVAL;
+			if (copy_from_user(&op, (void __user *)arg, minsz))
+				return -EFAULT;
+
+			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/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 82889c3..d81c17f 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -468,12 +468,23 @@ struct vfio_iommu_spapr_tce_info {
  * - unfreeze IO/DMA for frozen PE;
  * - read PE state;
  * - reset PE;
- * - configure PE.
+ * - configure PE;
+ * - inject EEH error.
  */
+struct vfio_eeh_pe_err {
+	__u32 type;
+	__u32 func;
+	__u64 addr;
+	__u64 mask;
+};
+
 struct vfio_eeh_pe_op {
 	__u32 argsz;
 	__u32 flags;
 	__u32 op;
+	union {
+		struct vfio_eeh_pe_err err;
+	};
 };
 
 #define VFIO_EEH_PE_DISABLE		0	/* Disable EEH functionality */
@@ -490,6 +501,7 @@ struct vfio_eeh_pe_op {
 #define VFIO_EEH_PE_RESET_HOT		6	/* Assert hot reset          */
 #define VFIO_EEH_PE_RESET_FUNDAMENTAL	7	/* Assert fundamental reset  */
 #define VFIO_EEH_PE_CONFIGURE		8	/* PE configuration          */
+#define VFIO_EEH_PE_INJECT_ERR		9	/* Inject EEH error          */
 
 #define VFIO_EEH_PE_OP			_IO(VFIO_TYPE, VFIO_BASE + 21)
 
-- 
1.8.3.2

[PATCH v4 4/4] drivers/vfio: Remove duplicated PE states

[Gavin Shan]

The set of constants for PE states defined in uapi/linux/vfio.h is
duplicated to uapi/asm/eeh.h. The patch removes the set from the
former.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 include/uapi/linux/vfio.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index d81c17f..3fd1e86 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -492,11 +492,6 @@ struct vfio_eeh_pe_op {
 #define VFIO_EEH_PE_UNFREEZE_IO		2	/* Enable IO for frozen PE   */
 #define VFIO_EEH_PE_UNFREEZE_DMA	3	/* Enable DMA for frozen PE  */
 #define VFIO_EEH_PE_GET_STATE		4	/* PE state retrieval        */
-#define  VFIO_EEH_PE_STATE_NORMAL	0	/* PE in functional state    */
-#define  VFIO_EEH_PE_STATE_RESET	1	/* PE reset in progress      */
-#define  VFIO_EEH_PE_STATE_STOPPED	2	/* Stopped DMA and IO        */
-#define  VFIO_EEH_PE_STATE_STOPPED_DMA	4	/* Stopped DMA only          */
-#define  VFIO_EEH_PE_STATE_UNAVAIL	5	/* State unavailable         */
 #define VFIO_EEH_PE_RESET_DEACTIVATE	5	/* Deassert PE reset         */
 #define VFIO_EEH_PE_RESET_HOT		6	/* Assert hot reset          */
 #define VFIO_EEH_PE_RESET_FUNDAMENTAL	7	/* Assert fundamental reset  */
-- 
1.8.3.2

Re: [PATCH v4 4/4] drivers/vfio: Remove duplicated PE states

[Alex Williamson]

On Thu, 2015-03-26 at 10:20 +1100, Gavin Shan wrote:
> The set of constants for PE states defined in uapi/linux/vfio.h is
> duplicated to uapi/asm/eeh.h. The patch removes the set from the
> former.
> 
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>  include/uapi/linux/vfio.h | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index d81c17f..3fd1e86 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -492,11 +492,6 @@ struct vfio_eeh_pe_op {
>  #define VFIO_EEH_PE_UNFREEZE_IO		2	/* Enable IO for frozen PE   */
>  #define VFIO_EEH_PE_UNFREEZE_DMA	3	/* Enable DMA for frozen PE  */
>  #define VFIO_EEH_PE_GET_STATE		4	/* PE state retrieval        */
> -#define  VFIO_EEH_PE_STATE_NORMAL	0	/* PE in functional state    */
> -#define  VFIO_EEH_PE_STATE_RESET	1	/* PE reset in progress      */
> -#define  VFIO_EEH_PE_STATE_STOPPED	2	/* Stopped DMA and IO        */
> -#define  VFIO_EEH_PE_STATE_STOPPED_DMA	4	/* Stopped DMA only          */
> -#define  VFIO_EEH_PE_STATE_UNAVAIL	5	/* State unavailable         */
>  #define VFIO_EEH_PE_RESET_DEACTIVATE	5	/* Deassert PE reset         */
>  #define VFIO_EEH_PE_RESET_HOT		6	/* Assert hot reset          */
>  #define VFIO_EEH_PE_RESET_FUNDAMENTAL	7	/* Assert fundamental reset  */

How do you know that nobody depends on these defines?  I thought the
suggestion was to use the EEH_* defines for error injection, not to
remove existing VFIO_EEH_* defines.  You could certainly redefine these
in terms of EEH_* defines instead.  Thanks,

Alex

Re: [PATCH v4 4/4] drivers/vfio: Remove duplicated PE states

[Gavin Shan]

On Wed, Mar 25, 2015 at 06:46:28PM -0600, Alex Williamson wrote:
>On Thu, 2015-03-26 at 10:20 +1100, Gavin Shan wrote:
>> The set of constants for PE states defined in uapi/linux/vfio.h is
>> duplicated to uapi/asm/eeh.h. The patch removes the set from the
>> former.
>> 
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>>  include/uapi/linux/vfio.h | 5 -----
>>  1 file changed, 5 deletions(-)
>> 
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index d81c17f..3fd1e86 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -492,11 +492,6 @@ struct vfio_eeh_pe_op {
>>  #define VFIO_EEH_PE_UNFREEZE_IO		2	/* Enable IO for frozen PE   */
>>  #define VFIO_EEH_PE_UNFREEZE_DMA	3	/* Enable DMA for frozen PE  */
>>  #define VFIO_EEH_PE_GET_STATE		4	/* PE state retrieval        */
>> -#define  VFIO_EEH_PE_STATE_NORMAL	0	/* PE in functional state    */
>> -#define  VFIO_EEH_PE_STATE_RESET	1	/* PE reset in progress      */
>> -#define  VFIO_EEH_PE_STATE_STOPPED	2	/* Stopped DMA and IO        */
>> -#define  VFIO_EEH_PE_STATE_STOPPED_DMA	4	/* Stopped DMA only          */
>> -#define  VFIO_EEH_PE_STATE_UNAVAIL	5	/* State unavailable         */
>>  #define VFIO_EEH_PE_RESET_DEACTIVATE	5	/* Deassert PE reset         */
>>  #define VFIO_EEH_PE_RESET_HOT		6	/* Assert hot reset          */
>>  #define VFIO_EEH_PE_RESET_FUNDAMENTAL	7	/* Assert fundamental reset  */
>
>How do you know that nobody depends on these defines?  I thought the
>suggestion was to use the EEH_* defines for error injection, not to
>remove existing VFIO_EEH_* defines.  You could certainly redefine these
>in terms of EEH_* defines instead.  Thanks,
>

QEMU should be the first user to utilize the EEH capability exposed by
the host kernel, and I believe QEMU doesn't use those constants yet.
So it's right time to move those constants to uapi/asm/eeh.h. Once some
one starts to use them, it's impossible to do so.

Thanks,
Gavin

>Alex
>

Re: [PATCH v4 4/4] drivers/vfio: Remove duplicated PE states

[David Gibson]

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

On Thu, Mar 26, 2015 at 11:59:03AM +1100, Gavin Shan wrote:
> On Wed, Mar 25, 2015 at 06:46:28PM -0600, Alex Williamson wrote:
> >On Thu, 2015-03-26 at 10:20 +1100, Gavin Shan wrote:
> >> The set of constants for PE states defined in uapi/linux/vfio.h is
> >> duplicated to uapi/asm/eeh.h. The patch removes the set from the
> >> former.
> >> 
> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> >> ---
> >>  include/uapi/linux/vfio.h | 5 -----
> >>  1 file changed, 5 deletions(-)
> >> 
> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >> index d81c17f..3fd1e86 100644
> >> --- a/include/uapi/linux/vfio.h
> >> +++ b/include/uapi/linux/vfio.h
> >> @@ -492,11 +492,6 @@ struct vfio_eeh_pe_op {
> >>  #define VFIO_EEH_PE_UNFREEZE_IO		2	/* Enable IO for frozen PE   */
> >>  #define VFIO_EEH_PE_UNFREEZE_DMA	3	/* Enable DMA for frozen PE  */
> >>  #define VFIO_EEH_PE_GET_STATE		4	/* PE state retrieval        */
> >> -#define  VFIO_EEH_PE_STATE_NORMAL	0	/* PE in functional state    */
> >> -#define  VFIO_EEH_PE_STATE_RESET	1	/* PE reset in progress      */
> >> -#define  VFIO_EEH_PE_STATE_STOPPED	2	/* Stopped DMA and IO        */
> >> -#define  VFIO_EEH_PE_STATE_STOPPED_DMA	4	/* Stopped DMA only          */
> >> -#define  VFIO_EEH_PE_STATE_UNAVAIL	5	/* State unavailable         */
> >>  #define VFIO_EEH_PE_RESET_DEACTIVATE	5	/* Deassert PE reset         */
> >>  #define VFIO_EEH_PE_RESET_HOT		6	/* Assert hot reset          */
> >>  #define VFIO_EEH_PE_RESET_FUNDAMENTAL	7	/* Assert fundamental reset  */
> >
> >How do you know that nobody depends on these defines?  I thought the
> >suggestion was to use the EEH_* defines for error injection, not to
> >remove existing VFIO_EEH_* defines.  You could certainly redefine these
> >in terms of EEH_* defines instead.  Thanks,
> >
> 
> QEMU should be the first user to utilize the EEH capability exposed by
> the host kernel, and I believe QEMU doesn't use those constants yet.
> So it's right time to move those constants to uapi/asm/eeh.h. Once some
> one starts to use them, it's impossible to do so.

That's a good point, actually.  This seems like a reasonable risk
under the cirucmstances to me.

-- 
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 v4 4/4] drivers/vfio: Remove duplicated PE states

[Alex Williamson]

On Thu, 2015-03-26 at 11:59 +1100, Gavin Shan wrote:
> On Wed, Mar 25, 2015 at 06:46:28PM -0600, Alex Williamson wrote:
> >On Thu, 2015-03-26 at 10:20 +1100, Gavin Shan wrote:
> >> The set of constants for PE states defined in uapi/linux/vfio.h is
> >> duplicated to uapi/asm/eeh.h. The patch removes the set from the
> >> former.
> >> 
> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> >> ---
> >>  include/uapi/linux/vfio.h | 5 -----
> >>  1 file changed, 5 deletions(-)
> >> 
> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >> index d81c17f..3fd1e86 100644
> >> --- a/include/uapi/linux/vfio.h
> >> +++ b/include/uapi/linux/vfio.h
> >> @@ -492,11 +492,6 @@ struct vfio_eeh_pe_op {
> >>  #define VFIO_EEH_PE_UNFREEZE_IO		2	/* Enable IO for frozen PE   */
> >>  #define VFIO_EEH_PE_UNFREEZE_DMA	3	/* Enable DMA for frozen PE  */
> >>  #define VFIO_EEH_PE_GET_STATE		4	/* PE state retrieval        */
> >> -#define  VFIO_EEH_PE_STATE_NORMAL	0	/* PE in functional state    */
> >> -#define  VFIO_EEH_PE_STATE_RESET	1	/* PE reset in progress      */
> >> -#define  VFIO_EEH_PE_STATE_STOPPED	2	/* Stopped DMA and IO        */
> >> -#define  VFIO_EEH_PE_STATE_STOPPED_DMA	4	/* Stopped DMA only          */
> >> -#define  VFIO_EEH_PE_STATE_UNAVAIL	5	/* State unavailable         */
> >>  #define VFIO_EEH_PE_RESET_DEACTIVATE	5	/* Deassert PE reset         */
> >>  #define VFIO_EEH_PE_RESET_HOT		6	/* Assert hot reset          */
> >>  #define VFIO_EEH_PE_RESET_FUNDAMENTAL	7	/* Assert fundamental reset  */
> >
> >How do you know that nobody depends on these defines?  I thought the
> >suggestion was to use the EEH_* defines for error injection, not to
> >remove existing VFIO_EEH_* defines.  You could certainly redefine these
> >in terms of EEH_* defines instead.  Thanks,
> >
> 
> QEMU should be the first user to utilize the EEH capability exposed by
> the host kernel, and I believe QEMU doesn't use those constants yet.
> So it's right time to move those constants to uapi/asm/eeh.h. Once some
> one starts to use them, it's impossible to do so.

There are soon to be four kernel versions out there with these defines,
you can't be sure that nobody has already or won't in the future do
VFIO/EEH development on those kernels.  The defines need to stay IMHO.
Thanks,

Alex

Re: [PATCH v4 4/4] drivers/vfio: Remove duplicated PE states

[Gavin Shan]

On Wed, Mar 25, 2015 at 07:55:41PM -0600, Alex Williamson wrote:
>On Thu, 2015-03-26 at 11:59 +1100, Gavin Shan wrote:
>> On Wed, Mar 25, 2015 at 06:46:28PM -0600, Alex Williamson wrote:
>> >On Thu, 2015-03-26 at 10:20 +1100, Gavin Shan wrote:
>> >> The set of constants for PE states defined in uapi/linux/vfio.h is
>> >> duplicated to uapi/asm/eeh.h. The patch removes the set from the
>> >> former.
>> >> 
>> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> >> ---
>> >>  include/uapi/linux/vfio.h | 5 -----
>> >>  1 file changed, 5 deletions(-)
>> >> 
>> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> >> index d81c17f..3fd1e86 100644
>> >> --- a/include/uapi/linux/vfio.h
>> >> +++ b/include/uapi/linux/vfio.h
>> >> @@ -492,11 +492,6 @@ struct vfio_eeh_pe_op {
>> >>  #define VFIO_EEH_PE_UNFREEZE_IO		2	/* Enable IO for frozen PE   */
>> >>  #define VFIO_EEH_PE_UNFREEZE_DMA	3	/* Enable DMA for frozen PE  */
>> >>  #define VFIO_EEH_PE_GET_STATE		4	/* PE state retrieval        */
>> >> -#define  VFIO_EEH_PE_STATE_NORMAL	0	/* PE in functional state    */
>> >> -#define  VFIO_EEH_PE_STATE_RESET	1	/* PE reset in progress      */
>> >> -#define  VFIO_EEH_PE_STATE_STOPPED	2	/* Stopped DMA and IO        */
>> >> -#define  VFIO_EEH_PE_STATE_STOPPED_DMA	4	/* Stopped DMA only          */
>> >> -#define  VFIO_EEH_PE_STATE_UNAVAIL	5	/* State unavailable         */
>> >>  #define VFIO_EEH_PE_RESET_DEACTIVATE	5	/* Deassert PE reset         */
>> >>  #define VFIO_EEH_PE_RESET_HOT		6	/* Assert hot reset          */
>> >>  #define VFIO_EEH_PE_RESET_FUNDAMENTAL	7	/* Assert fundamental reset  */
>> >
>> >How do you know that nobody depends on these defines?  I thought the
>> >suggestion was to use the EEH_* defines for error injection, not to
>> >remove existing VFIO_EEH_* defines.  You could certainly redefine these
>> >in terms of EEH_* defines instead.  Thanks,
>> >
>> 
>> QEMU should be the first user to utilize the EEH capability exposed by
>> the host kernel, and I believe QEMU doesn't use those constants yet.
>> So it's right time to move those constants to uapi/asm/eeh.h. Once some
>> one starts to use them, it's impossible to do so.
>
>There are soon to be four kernel versions out there with these defines,
>you can't be sure that nobody has already or won't in the future do
>VFIO/EEH development on those kernels.  The defines need to stay IMHO.
>Thanks,
>

Right, it's the point to keep them. I'll simply drop PATCH[4/4] and
amend the commit log of PATCH[1/4] in next revision, which will be
sent out soon.

Thanks,
Gavin

>Alex
>

Re: [PATCH v4 4/4] drivers/vfio: Remove duplicated PE states

[David Gibson]

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

On Wed, Mar 25, 2015 at 06:46:28PM -0600, Alex Williamson wrote:
> On Thu, 2015-03-26 at 10:20 +1100, Gavin Shan wrote:
> > The set of constants for PE states defined in uapi/linux/vfio.h is
> > duplicated to uapi/asm/eeh.h. The patch removes the set from the
> > former.
> > 
> > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> > ---
> >  include/uapi/linux/vfio.h | 5 -----
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index d81c17f..3fd1e86 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -492,11 +492,6 @@ struct vfio_eeh_pe_op {
> >  #define VFIO_EEH_PE_UNFREEZE_IO		2	/* Enable IO for frozen PE   */
> >  #define VFIO_EEH_PE_UNFREEZE_DMA	3	/* Enable DMA for frozen PE  */
> >  #define VFIO_EEH_PE_GET_STATE		4	/* PE state retrieval        */
> > -#define  VFIO_EEH_PE_STATE_NORMAL	0	/* PE in functional state    */
> > -#define  VFIO_EEH_PE_STATE_RESET	1	/* PE reset in progress      */
> > -#define  VFIO_EEH_PE_STATE_STOPPED	2	/* Stopped DMA and IO        */
> > -#define  VFIO_EEH_PE_STATE_STOPPED_DMA	4	/* Stopped DMA only          */
> > -#define  VFIO_EEH_PE_STATE_UNAVAIL	5	/* State unavailable         */
> >  #define VFIO_EEH_PE_RESET_DEACTIVATE	5	/* Deassert PE reset         */
> >  #define VFIO_EEH_PE_RESET_HOT		6	/* Assert hot reset          */
> >  #define VFIO_EEH_PE_RESET_FUNDAMENTAL	7	/* Assert fundamental reset  */
> 
> How do you know that nobody depends on these defines?  I thought the
> suggestion was to use the EEH_* defines for error injection, not to
> remove existing VFIO_EEH_* defines.  You could certainly redefine these
> in terms of EEH_* defines instead.  Thanks,

Yeah, since they're already exported, these can't be just removed, but
should be redefined in terms of the new exported EEH defines.

I also think this should be folded into 1/1.

-- 
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 v4 4/4] drivers/vfio: Remove duplicated PE states

[Gavin Shan]

On Thu, Mar 26, 2015 at 12:01:57PM +1100, David Gibson wrote:
>On Wed, Mar 25, 2015 at 06:46:28PM -0600, Alex Williamson wrote:
>> On Thu, 2015-03-26 at 10:20 +1100, Gavin Shan wrote:
>> > The set of constants for PE states defined in uapi/linux/vfio.h is
>> > duplicated to uapi/asm/eeh.h. The patch removes the set from the
>> > former.
>> > 
>> > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> > ---
>> >  include/uapi/linux/vfio.h | 5 -----
>> >  1 file changed, 5 deletions(-)
>> > 
>> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> > index d81c17f..3fd1e86 100644
>> > --- a/include/uapi/linux/vfio.h
>> > +++ b/include/uapi/linux/vfio.h
>> > @@ -492,11 +492,6 @@ struct vfio_eeh_pe_op {
>> >  #define VFIO_EEH_PE_UNFREEZE_IO		2	/* Enable IO for frozen PE   */
>> >  #define VFIO_EEH_PE_UNFREEZE_DMA	3	/* Enable DMA for frozen PE  */
>> >  #define VFIO_EEH_PE_GET_STATE		4	/* PE state retrieval        */
>> > -#define  VFIO_EEH_PE_STATE_NORMAL	0	/* PE in functional state    */
>> > -#define  VFIO_EEH_PE_STATE_RESET	1	/* PE reset in progress      */
>> > -#define  VFIO_EEH_PE_STATE_STOPPED	2	/* Stopped DMA and IO        */
>> > -#define  VFIO_EEH_PE_STATE_STOPPED_DMA	4	/* Stopped DMA only          */
>> > -#define  VFIO_EEH_PE_STATE_UNAVAIL	5	/* State unavailable         */
>> >  #define VFIO_EEH_PE_RESET_DEACTIVATE	5	/* Deassert PE reset         */
>> >  #define VFIO_EEH_PE_RESET_HOT		6	/* Assert hot reset          */
>> >  #define VFIO_EEH_PE_RESET_FUNDAMENTAL	7	/* Assert fundamental reset  */
>> 
>> How do you know that nobody depends on these defines?  I thought the
>> suggestion was to use the EEH_* defines for error injection, not to
>> remove existing VFIO_EEH_* defines.  You could certainly redefine these
>> in terms of EEH_* defines instead.  Thanks,
>
>Yeah, since they're already exported, these can't be just removed, but
>should be redefined in terms of the new exported EEH defines.
>

I just explained to Alex.W with something as follows. Are you sure to
keep this set of defines in vfio.h? That way, the EEH error constants
are all defined in uapi/asm/eeh.h, but the EEH PE state constatns will
be distributed in vfio.h and uapi/asm/eeh.h at the same time. Actually,
I believe it's safe to move the PE state defines from vfio.h to
uapi/asm/eeh.h and now is the right time to do so :)

---

QEMU should be the first user to utilize the EEH capability exposed by
the host kernel, and I believe QEMU doesn't use those constants yet.
So it's right time to move those constants to uapi/asm/eeh.h. Once some
one starts to use them, it's impossible to do so.

>I also think this should be folded into 1/1.
>

The reason I didn't fold it to PATCH[1/4]: I was afraid the changs
will be taken via different trees (ppc-next and vfio-next).

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