* [PATCH v3 0/2] EEH Error Injection Support for VFIO Devices
@ 2015-03-20 19:58 ` Gavin Shan
0 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-20 19:58 UTC (permalink / raw)
To: linuxppc-dev; +Cc: kvm, alex.williamson, agraf, aik, david, Gavin Shan
Those two 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
=========
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 (2):
powerpc/eeh: Introduce eeh_pe_inject_err()
drivers/vfio: Support EEH error injection
Documentation/vfio.txt | 12 ++++++++++++
arch/powerpc/include/asm/eeh.h | 26 ++++++++++++++++++++++++++
arch/powerpc/kernel/eeh.c | 35 +++++++++++++++++++++++++++++++++++
drivers/vfio/vfio_spapr_eeh.c | 10 ++++++++++
include/uapi/linux/vfio.h | 36 +++++++++++++++++++++++++++++++++++-
5 files changed, 118 insertions(+), 1 deletion(-)
--
1.8.3.2
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 0/2] EEH Error Injection Support for VFIO Devices
@ 2015-03-20 19:58 ` Gavin Shan
0 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-20 19:58 UTC (permalink / raw)
To: linuxppc-dev; +Cc: kvm, aik, Gavin Shan, agraf, alex.williamson, david
Those two 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
=========
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 (2):
powerpc/eeh: Introduce eeh_pe_inject_err()
drivers/vfio: Support EEH error injection
Documentation/vfio.txt | 12 ++++++++++++
arch/powerpc/include/asm/eeh.h | 26 ++++++++++++++++++++++++++
arch/powerpc/kernel/eeh.c | 35 +++++++++++++++++++++++++++++++++++
drivers/vfio/vfio_spapr_eeh.c | 10 ++++++++++
include/uapi/linux/vfio.h | 36 +++++++++++++++++++++++++++++++++++-
5 files changed, 118 insertions(+), 1 deletion(-)
--
1.8.3.2
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 1/2] powerpc/eeh: Introduce eeh_pe_inject_err()
2015-03-20 19:58 ` Gavin Shan
@ 2015-03-20 19:58 ` Gavin Shan
-1 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-20 19:58 UTC (permalink / raw)
To: linuxppc-dev; +Cc: kvm, alex.williamson, agraf, aik, david, Gavin Shan
The patch defines PCI error types and functions in 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>
---
arch/powerpc/include/asm/eeh.h | 26 ++++++++++++++++++++++++++
arch/powerpc/kernel/eeh.c | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 61912fc..85a17de 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -194,6 +194,30 @@ enum {
#define EEH_RESET_FUNDAMENTAL 3 /* Fundamental reset */
#define EEH_LOG_TEMP 1 /* EEH temporary error log */
#define EEH_LOG_PERM 2 /* EEH permanent error log */
+#define EEH_ERR_TYPE_32 0 /* 32-bits PCI error */
+#define EEH_ERR_TYPE_64 1 /* 64-bits PCI 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
struct eeh_ops {
char *name;
@@ -293,6 +317,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/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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 1/2] powerpc/eeh: Introduce eeh_pe_inject_err()
@ 2015-03-20 19:58 ` Gavin Shan
0 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-20 19:58 UTC (permalink / raw)
To: linuxppc-dev; +Cc: kvm, aik, Gavin Shan, agraf, alex.williamson, david
The patch defines PCI error types and functions in 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>
---
arch/powerpc/include/asm/eeh.h | 26 ++++++++++++++++++++++++++
arch/powerpc/kernel/eeh.c | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 61912fc..85a17de 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -194,6 +194,30 @@ enum {
#define EEH_RESET_FUNDAMENTAL 3 /* Fundamental reset */
#define EEH_LOG_TEMP 1 /* EEH temporary error log */
#define EEH_LOG_PERM 2 /* EEH permanent error log */
+#define EEH_ERR_TYPE_32 0 /* 32-bits PCI error */
+#define EEH_ERR_TYPE_64 1 /* 64-bits PCI 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
struct eeh_ops {
char *name;
@@ -293,6 +317,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/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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 2/2] drivers/vfio: Support EEH error injection
2015-03-20 19:58 ` Gavin Shan
@ 2015-03-20 19:58 ` Gavin Shan
-1 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-20 19:58 UTC (permalink / raw)
To: linuxppc-dev; +Cc: kvm, alex.williamson, agraf, aik, david, 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>
---
Documentation/vfio.txt | 12 ++++++++++++
drivers/vfio/vfio_spapr_eeh.c | 10 ++++++++++
include/uapi/linux/vfio.h | 36 +++++++++++++++++++++++++++++++++++-
3 files changed, 57 insertions(+), 1 deletion(-)
diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
+ pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
+#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
+#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
+#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
+#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
+#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
+#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
+#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
+#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
+#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
+#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
+#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
+#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
+#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
+#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
+#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
+#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
+#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
+#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
+#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
+#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
+#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
#define VFIO_EEH_PE_OP _IO(VFIO_TYPE, VFIO_BASE + 21)
--
1.8.3.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v3 2/2] drivers/vfio: Support EEH error injection
@ 2015-03-20 19:58 ` Gavin Shan
0 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-20 19:58 UTC (permalink / raw)
To: linuxppc-dev; +Cc: kvm, aik, Gavin Shan, agraf, alex.williamson, david
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>
---
Documentation/vfio.txt | 12 ++++++++++++
drivers/vfio/vfio_spapr_eeh.c | 10 ++++++++++
include/uapi/linux/vfio.h | 36 +++++++++++++++++++++++++++++++++++-
3 files changed, 57 insertions(+), 1 deletion(-)
diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
+ pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
+#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
+#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
+#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
+#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
+#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
+#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
+#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
+#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
+#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
+#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
+#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
+#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
+#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
+#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
+#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
+#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
+#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
+#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
+#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
+#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
+#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
#define VFIO_EEH_PE_OP _IO(VFIO_TYPE, VFIO_BASE + 21)
--
1.8.3.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v3 1/2] powerpc/eeh: Introduce eeh_pe_inject_err()
2015-03-20 19:58 ` Gavin Shan
@ 2015-03-23 1:33 ` David Gibson
-1 siblings, 0 replies; 28+ messages in thread
From: David Gibson @ 2015-03-23 1:33 UTC (permalink / raw)
To: Gavin Shan; +Cc: linuxppc-dev, kvm, alex.williamson, agraf, aik
[-- Attachment #1: Type: text/plain, Size: 599 bytes --]
On Sat, Mar 21, 2015 at 06:58:44AM +1100, Gavin Shan wrote:
> The patch defines PCI error types and functions in 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>
--
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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 1/2] powerpc/eeh: Introduce eeh_pe_inject_err()
@ 2015-03-23 1:33 ` David Gibson
0 siblings, 0 replies; 28+ messages in thread
From: David Gibson @ 2015-03-23 1:33 UTC (permalink / raw)
To: Gavin Shan; +Cc: aik, linuxppc-dev, alex.williamson, agraf, kvm
[-- Attachment #1: Type: text/plain, Size: 599 bytes --]
On Sat, Mar 21, 2015 at 06:58:44AM +1100, Gavin Shan wrote:
> The patch defines PCI error types and functions in 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>
--
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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
2015-03-20 19:58 ` Gavin Shan
@ 2015-03-23 1:39 ` David Gibson
-1 siblings, 0 replies; 28+ messages in thread
From: David Gibson @ 2015-03-23 1:39 UTC (permalink / raw)
To: Gavin Shan; +Cc: linuxppc-dev, kvm, alex.williamson, agraf, aik
[-- Attachment #1: Type: text/plain, Size: 4813 bytes --]
On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
> 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 | 36 +++++++++++++++++++++++++++++++++++-
> 3 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
These all seem to duplicate (under slightly different names) the
defines you just added in arch/powerpc/include/asm/eeh.h. It would
make more sense to only have the uapi copy, surely.
--
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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
@ 2015-03-23 1:39 ` David Gibson
0 siblings, 0 replies; 28+ messages in thread
From: David Gibson @ 2015-03-23 1:39 UTC (permalink / raw)
To: Gavin Shan; +Cc: aik, linuxppc-dev, alex.williamson, agraf, kvm
[-- Attachment #1: Type: text/plain, Size: 4813 bytes --]
On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
> 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 | 36 +++++++++++++++++++++++++++++++++++-
> 3 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
These all seem to duplicate (under slightly different names) the
defines you just added in arch/powerpc/include/asm/eeh.h. It would
make more sense to only have the uapi copy, surely.
--
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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
2015-03-23 1:39 ` David Gibson
@ 2015-03-23 1:56 ` Gavin Shan
-1 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-23 1:56 UTC (permalink / raw)
To: David Gibson; +Cc: Gavin Shan, linuxppc-dev, kvm, alex.williamson, agraf, aik
On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
>On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
>> 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 | 36 +++++++++++++++++++++++++++++++++++-
>> 3 files changed, 57 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
>> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
>> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
>> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
>> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
>> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
>> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
>> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
>> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
>> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
>> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
>> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
>> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
>> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
>> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
>> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
>> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
>> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
>> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
>> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
>> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
>> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
>> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
>
>These all seem to duplicate (under slightly different names) the
>defines you just added in arch/powerpc/include/asm/eeh.h. It would
>make more sense to only have the uapi copy, surely.
>
David, Thanks for your time. The defines in eeh.h is used not only
for mapping those constants defined in vfio.h, but also for validating
arguments from debugfs entry "err_injct". So it'd better to keep
them separate: one set is used by userspace, another set is used
by host kernel.
By the way, Alex Williamson, could you please ACK this if you don't
find obvious errors in the patch. I really want this to be merged
to 4.1.
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
@ 2015-03-23 1:56 ` Gavin Shan
0 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-23 1:56 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, aik, Gavin Shan, agraf, linuxppc-dev, alex.williamson
On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
>On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
>> 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 | 36 +++++++++++++++++++++++++++++++++++-
>> 3 files changed, 57 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
>> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
>> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
>> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
>> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
>> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
>> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
>> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
>> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
>> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
>> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
>> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
>> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
>> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
>> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
>> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
>> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
>> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
>> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
>> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
>> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
>> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
>> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
>
>These all seem to duplicate (under slightly different names) the
>defines you just added in arch/powerpc/include/asm/eeh.h. It would
>make more sense to only have the uapi copy, surely.
>
David, Thanks for your time. The defines in eeh.h is used not only
for mapping those constants defined in vfio.h, but also for validating
arguments from debugfs entry "err_injct". So it'd better to keep
them separate: one set is used by userspace, another set is used
by host kernel.
By the way, Alex Williamson, could you please ACK this if you don't
find obvious errors in the patch. I really want this to be merged
to 4.1.
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
2015-03-23 1:56 ` Gavin Shan
@ 2015-03-23 3:43 ` David Gibson
-1 siblings, 0 replies; 28+ messages in thread
From: David Gibson @ 2015-03-23 3:43 UTC (permalink / raw)
To: Gavin Shan; +Cc: linuxppc-dev, kvm, alex.williamson, agraf, aik
[-- Attachment #1: Type: text/plain, Size: 5953 bytes --]
On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
> >> 3 files changed, 57 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
> >
> >These all seem to duplicate (under slightly different names) the
> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
> >make more sense to only have the uapi copy, surely.
> >
>
> David, Thanks for your time. The defines in eeh.h is used not only
> for mapping those constants defined in vfio.h, but also for validating
> arguments from debugfs entry "err_injct". So it'd better to keep
> them separate: one set is used by userspace, another set is used
> by host kernel.
I don't understand why this is an argument for duplicating them. They
have the same meanings and values, why can't you use the uapi versions
everywhere?
> By the way, Alex Williamson, could you please ACK this if you don't
> find obvious errors in the patch. I really want this to be merged
> to 4.1.
>
> 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
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
@ 2015-03-23 3:43 ` David Gibson
0 siblings, 0 replies; 28+ messages in thread
From: David Gibson @ 2015-03-23 3:43 UTC (permalink / raw)
To: Gavin Shan; +Cc: aik, linuxppc-dev, alex.williamson, agraf, kvm
[-- Attachment #1: Type: text/plain, Size: 5953 bytes --]
On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
> >> 3 files changed, 57 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
> >
> >These all seem to duplicate (under slightly different names) the
> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
> >make more sense to only have the uapi copy, surely.
> >
>
> David, Thanks for your time. The defines in eeh.h is used not only
> for mapping those constants defined in vfio.h, but also for validating
> arguments from debugfs entry "err_injct". So it'd better to keep
> them separate: one set is used by userspace, another set is used
> by host kernel.
I don't understand why this is an argument for duplicating them. They
have the same meanings and values, why can't you use the uapi versions
everywhere?
> By the way, Alex Williamson, could you please ACK this if you don't
> find obvious errors in the patch. I really want this to be merged
> to 4.1.
>
> 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
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
2015-03-23 3:43 ` David Gibson
@ 2015-03-23 5:03 ` Gavin Shan
-1 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-23 5:03 UTC (permalink / raw)
To: David Gibson; +Cc: Gavin Shan, linuxppc-dev, kvm, alex.williamson, agraf, aik
On Mon, Mar 23, 2015 at 02:43:03PM +1100, David Gibson wrote:
>On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
>> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
>> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
>> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
>> >> 3 files changed, 57 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
>> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
>> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
>> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
>> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
>> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
>> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
>> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
>> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
>> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
>> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
>> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
>> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
>> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
>> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
>> >
>> >These all seem to duplicate (under slightly different names) the
>> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
>> >make more sense to only have the uapi copy, surely.
>> >
>>
>> David, Thanks for your time. The defines in eeh.h is used not only
>> for mapping those constants defined in vfio.h, but also for validating
>> arguments from debugfs entry "err_injct". So it'd better to keep
>> them separate: one set is used by userspace, another set is used
>> by host kernel.
>
>I don't understand why this is an argument for duplicating them. They
>have the same meanings and values, why can't you use the uapi versions
>everywhere?
>
We can drop the constants defined in eeh.h and just use those defines
in vfio.h. But I prefer separate constants for some ovbious reasons
except the one I mentioned above :)
- We already had separate sets of constants for EEH and VFIO. For example,
VFIO_EEH_PE_STATE_* in include/uapi/vfio.h always have counter part in
arch/powerpc/asm/include/eeh.h
- The UAPI interface is expected to be stable, or back-compatible when
introducing changes to it. The defines (at least their names) in eeh.h
doesn't have to be stable.
- I would like keep the code between EEH/VFIO independent to each other
enough.
Thanks,
Gavin
>> By the way, Alex Williamson, could you please ACK this if you don't
>> find obvious errors in the patch. I really want this to be merged
>> to 4.1.
>>
>> 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
@ 2015-03-23 5:03 ` Gavin Shan
0 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-23 5:03 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, aik, Gavin Shan, agraf, linuxppc-dev, alex.williamson
On Mon, Mar 23, 2015 at 02:43:03PM +1100, David Gibson wrote:
>On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
>> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
>> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
>> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
>> >> 3 files changed, 57 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
>> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
>> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
>> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
>> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
>> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
>> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
>> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
>> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
>> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
>> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
>> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
>> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
>> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
>> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
>> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
>> >
>> >These all seem to duplicate (under slightly different names) the
>> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
>> >make more sense to only have the uapi copy, surely.
>> >
>>
>> David, Thanks for your time. The defines in eeh.h is used not only
>> for mapping those constants defined in vfio.h, but also for validating
>> arguments from debugfs entry "err_injct". So it'd better to keep
>> them separate: one set is used by userspace, another set is used
>> by host kernel.
>
>I don't understand why this is an argument for duplicating them. They
>have the same meanings and values, why can't you use the uapi versions
>everywhere?
>
We can drop the constants defined in eeh.h and just use those defines
in vfio.h. But I prefer separate constants for some ovbious reasons
except the one I mentioned above :)
- We already had separate sets of constants for EEH and VFIO. For example,
VFIO_EEH_PE_STATE_* in include/uapi/vfio.h always have counter part in
arch/powerpc/asm/include/eeh.h
- The UAPI interface is expected to be stable, or back-compatible when
introducing changes to it. The defines (at least their names) in eeh.h
doesn't have to be stable.
- I would like keep the code between EEH/VFIO independent to each other
enough.
Thanks,
Gavin
>> By the way, Alex Williamson, could you please ACK this if you don't
>> find obvious errors in the patch. I really want this to be merged
>> to 4.1.
>>
>> 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
2015-03-23 5:03 ` Gavin Shan
@ 2015-03-23 5:10 ` David Gibson
-1 siblings, 0 replies; 28+ messages in thread
From: David Gibson @ 2015-03-23 5:10 UTC (permalink / raw)
To: Gavin Shan; +Cc: linuxppc-dev, kvm, alex.williamson, agraf, aik
[-- Attachment #1: Type: text/plain, Size: 7688 bytes --]
On Mon, Mar 23, 2015 at 04:03:59PM +1100, Gavin Shan wrote:
> On Mon, Mar 23, 2015 at 02:43:03PM +1100, David Gibson wrote:
> >On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
> >> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
> >> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
> >> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
> >> >> 3 files changed, 57 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> >> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
> >> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
> >> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
> >> >
> >> >These all seem to duplicate (under slightly different names) the
> >> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
> >> >make more sense to only have the uapi copy, surely.
> >> >
> >>
> >> David, Thanks for your time. The defines in eeh.h is used not only
> >> for mapping those constants defined in vfio.h, but also for validating
> >> arguments from debugfs entry "err_injct". So it'd better to keep
> >> them separate: one set is used by userspace, another set is used
> >> by host kernel.
> >
> >I don't understand why this is an argument for duplicating them. They
> >have the same meanings and values, why can't you use the uapi versions
> >everywhere?
> >
>
> We can drop the constants defined in eeh.h and just use those defines
> in vfio.h. But I prefer separate constants for some ovbious reasons
> except the one I mentioned above :)
>
> - We already had separate sets of constants for EEH and VFIO. For example,
> VFIO_EEH_PE_STATE_* in include/uapi/vfio.h always have counter part in
> arch/powerpc/asm/include/eeh.h
Oh.. I see your point.
Yes, the EEH internal code shouldn't need to use the VFIO defined
constants.
But it's still silly to duplicate, I think the correct think would be
to expose the EEH constants in uapi, and use those for VFIO as well.
> - The UAPI interface is expected to be stable, or back-compatible when
> introducing changes to it. The defines (at least their names) in eeh.h
> doesn't have to be stable.
But is there any reason not to make them stable. The VFIO case shows
that we need to lock down an interface somewhere. I don't see that
there's an advantage to having a not locked down interface in addition
to a locked down one.
> - I would like keep the code between EEH/VFIO independent to each other
> enough.
>
> Thanks,
> Gavin
>
> >> By the way, Alex Williamson, could you please ACK this if you don't
> >> find obvious errors in the patch. I really want this to be merged
> >> to 4.1.
> >>
> >> 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
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
@ 2015-03-23 5:10 ` David Gibson
0 siblings, 0 replies; 28+ messages in thread
From: David Gibson @ 2015-03-23 5:10 UTC (permalink / raw)
To: Gavin Shan; +Cc: aik, linuxppc-dev, alex.williamson, agraf, kvm
[-- Attachment #1: Type: text/plain, Size: 7688 bytes --]
On Mon, Mar 23, 2015 at 04:03:59PM +1100, Gavin Shan wrote:
> On Mon, Mar 23, 2015 at 02:43:03PM +1100, David Gibson wrote:
> >On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
> >> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
> >> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
> >> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
> >> >> 3 files changed, 57 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> >> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
> >> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
> >> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
> >> >
> >> >These all seem to duplicate (under slightly different names) the
> >> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
> >> >make more sense to only have the uapi copy, surely.
> >> >
> >>
> >> David, Thanks for your time. The defines in eeh.h is used not only
> >> for mapping those constants defined in vfio.h, but also for validating
> >> arguments from debugfs entry "err_injct". So it'd better to keep
> >> them separate: one set is used by userspace, another set is used
> >> by host kernel.
> >
> >I don't understand why this is an argument for duplicating them. They
> >have the same meanings and values, why can't you use the uapi versions
> >everywhere?
> >
>
> We can drop the constants defined in eeh.h and just use those defines
> in vfio.h. But I prefer separate constants for some ovbious reasons
> except the one I mentioned above :)
>
> - We already had separate sets of constants for EEH and VFIO. For example,
> VFIO_EEH_PE_STATE_* in include/uapi/vfio.h always have counter part in
> arch/powerpc/asm/include/eeh.h
Oh.. I see your point.
Yes, the EEH internal code shouldn't need to use the VFIO defined
constants.
But it's still silly to duplicate, I think the correct think would be
to expose the EEH constants in uapi, and use those for VFIO as well.
> - The UAPI interface is expected to be stable, or back-compatible when
> introducing changes to it. The defines (at least their names) in eeh.h
> doesn't have to be stable.
But is there any reason not to make them stable. The VFIO case shows
that we need to lock down an interface somewhere. I don't see that
there's an advantage to having a not locked down interface in addition
to a locked down one.
> - I would like keep the code between EEH/VFIO independent to each other
> enough.
>
> Thanks,
> Gavin
>
> >> By the way, Alex Williamson, could you please ACK this if you don't
> >> find obvious errors in the patch. I really want this to be merged
> >> to 4.1.
> >>
> >> 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
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
2015-03-23 5:10 ` David Gibson
@ 2015-03-23 5:20 ` Gavin Shan
-1 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-23 5:20 UTC (permalink / raw)
To: David Gibson; +Cc: Gavin Shan, linuxppc-dev, kvm, alex.williamson, agraf, aik
On Mon, Mar 23, 2015 at 04:10:20PM +1100, David Gibson wrote:
>On Mon, Mar 23, 2015 at 04:03:59PM +1100, Gavin Shan wrote:
>> On Mon, Mar 23, 2015 at 02:43:03PM +1100, David Gibson wrote:
>> >On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
>> >> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
>> >> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
>> >> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
>> >> >> 3 files changed, 57 insertions(+), 1 deletion(-)
>> >> >>
>> >> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>> >> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
>> >> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
>> >> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
>> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
>> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
>> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
>> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
>> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
>> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
>> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
>> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
>> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
>> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
>> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
>> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
>> >> >
>> >> >These all seem to duplicate (under slightly different names) the
>> >> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
>> >> >make more sense to only have the uapi copy, surely.
>> >> >
>> >>
>> >> David, Thanks for your time. The defines in eeh.h is used not only
>> >> for mapping those constants defined in vfio.h, but also for validating
>> >> arguments from debugfs entry "err_injct". So it'd better to keep
>> >> them separate: one set is used by userspace, another set is used
>> >> by host kernel.
>> >
>> >I don't understand why this is an argument for duplicating them. They
>> >have the same meanings and values, why can't you use the uapi versions
>> >everywhere?
>> >
>>
>> We can drop the constants defined in eeh.h and just use those defines
>> in vfio.h. But I prefer separate constants for some ovbious reasons
>> except the one I mentioned above :)
>>
>> - We already had separate sets of constants for EEH and VFIO. For example,
>> VFIO_EEH_PE_STATE_* in include/uapi/vfio.h always have counter part in
>> arch/powerpc/asm/include/eeh.h
>
>Oh.. I see your point.
>
>Yes, the EEH internal code shouldn't need to use the VFIO defined
>constants.
>
>But it's still silly to duplicate, I think the correct think would be
>to expose the EEH constants in uapi, and use those for VFIO as well.
>
I'm not sure. If Alex.W agree on this, I can have one patch to address
it in future :-)
>> - The UAPI interface is expected to be stable, or back-compatible when
>> introducing changes to it. The defines (at least their names) in eeh.h
>> doesn't have to be stable.
>
>But is there any reason not to make them stable. The VFIO case shows
>that we need to lock down an interface somewhere. I don't see that
>there's an advantage to having a not locked down interface in addition
>to a locked down one.
>
I was saying that the UAPI interace should be stable. I don't see
there're any reasons the UAPI interface will be changed in near
future :)
Thanks,
Gavin
>> - I would like keep the code between EEH/VFIO independent to each other
>> enough.
>>
>> Thanks,
>> Gavin
>>
>> >> By the way, Alex Williamson, could you please ACK this if you don't
>> >> find obvious errors in the patch. I really want this to be merged
>> >> to 4.1.
>> >>
>> >> 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
@ 2015-03-23 5:20 ` Gavin Shan
0 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-23 5:20 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, aik, Gavin Shan, agraf, linuxppc-dev, alex.williamson
On Mon, Mar 23, 2015 at 04:10:20PM +1100, David Gibson wrote:
>On Mon, Mar 23, 2015 at 04:03:59PM +1100, Gavin Shan wrote:
>> On Mon, Mar 23, 2015 at 02:43:03PM +1100, David Gibson wrote:
>> >On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
>> >> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
>> >> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
>> >> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
>> >> >> 3 files changed, 57 insertions(+), 1 deletion(-)
>> >> >>
>> >> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>> >> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
>> >> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
>> >> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
>> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
>> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
>> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
>> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
>> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
>> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
>> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
>> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
>> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
>> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
>> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
>> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
>> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
>> >> >
>> >> >These all seem to duplicate (under slightly different names) the
>> >> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
>> >> >make more sense to only have the uapi copy, surely.
>> >> >
>> >>
>> >> David, Thanks for your time. The defines in eeh.h is used not only
>> >> for mapping those constants defined in vfio.h, but also for validating
>> >> arguments from debugfs entry "err_injct". So it'd better to keep
>> >> them separate: one set is used by userspace, another set is used
>> >> by host kernel.
>> >
>> >I don't understand why this is an argument for duplicating them. They
>> >have the same meanings and values, why can't you use the uapi versions
>> >everywhere?
>> >
>>
>> We can drop the constants defined in eeh.h and just use those defines
>> in vfio.h. But I prefer separate constants for some ovbious reasons
>> except the one I mentioned above :)
>>
>> - We already had separate sets of constants for EEH and VFIO. For example,
>> VFIO_EEH_PE_STATE_* in include/uapi/vfio.h always have counter part in
>> arch/powerpc/asm/include/eeh.h
>
>Oh.. I see your point.
>
>Yes, the EEH internal code shouldn't need to use the VFIO defined
>constants.
>
>But it's still silly to duplicate, I think the correct think would be
>to expose the EEH constants in uapi, and use those for VFIO as well.
>
I'm not sure. If Alex.W agree on this, I can have one patch to address
it in future :-)
>> - The UAPI interface is expected to be stable, or back-compatible when
>> introducing changes to it. The defines (at least their names) in eeh.h
>> doesn't have to be stable.
>
>But is there any reason not to make them stable. The VFIO case shows
>that we need to lock down an interface somewhere. I don't see that
>there's an advantage to having a not locked down interface in addition
>to a locked down one.
>
I was saying that the UAPI interace should be stable. I don't see
there're any reasons the UAPI interface will be changed in near
future :)
Thanks,
Gavin
>> - I would like keep the code between EEH/VFIO independent to each other
>> enough.
>>
>> Thanks,
>> Gavin
>>
>> >> By the way, Alex Williamson, could you please ACK this if you don't
>> >> find obvious errors in the patch. I really want this to be merged
>> >> to 4.1.
>> >>
>> >> 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
2015-03-23 5:20 ` Gavin Shan
@ 2015-03-23 16:14 ` Alex Williamson
-1 siblings, 0 replies; 28+ messages in thread
From: Alex Williamson @ 2015-03-23 16:14 UTC (permalink / raw)
To: Gavin Shan; +Cc: David Gibson, linuxppc-dev, kvm, agraf, aik
On Mon, 2015-03-23 at 16:20 +1100, Gavin Shan wrote:
> On Mon, Mar 23, 2015 at 04:10:20PM +1100, David Gibson wrote:
> >On Mon, Mar 23, 2015 at 04:03:59PM +1100, Gavin Shan wrote:
> >> On Mon, Mar 23, 2015 at 02:43:03PM +1100, David Gibson wrote:
> >> >On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
> >> >> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
> >> >> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
> >> >> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
> >> >> >> 3 files changed, 57 insertions(+), 1 deletion(-)
> >> >> >>
> >> >> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> >> >> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
> >> >> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
> >> >> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
> >> >> >
> >> >> >These all seem to duplicate (under slightly different names) the
> >> >> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
> >> >> >make more sense to only have the uapi copy, surely.
> >> >> >
> >> >>
> >> >> David, Thanks for your time. The defines in eeh.h is used not only
> >> >> for mapping those constants defined in vfio.h, but also for validating
> >> >> arguments from debugfs entry "err_injct". So it'd better to keep
> >> >> them separate: one set is used by userspace, another set is used
> >> >> by host kernel.
> >> >
> >> >I don't understand why this is an argument for duplicating them. They
> >> >have the same meanings and values, why can't you use the uapi versions
> >> >everywhere?
> >> >
> >>
> >> We can drop the constants defined in eeh.h and just use those defines
> >> in vfio.h. But I prefer separate constants for some ovbious reasons
> >> except the one I mentioned above :)
> >>
> >> - We already had separate sets of constants for EEH and VFIO. For example,
> >> VFIO_EEH_PE_STATE_* in include/uapi/vfio.h always have counter part in
> >> arch/powerpc/asm/include/eeh.h
> >
> >Oh.. I see your point.
> >
> >Yes, the EEH internal code shouldn't need to use the VFIO defined
> >constants.
> >
> >But it's still silly to duplicate, I think the correct think would be
> >to expose the EEH constants in uapi, and use those for VFIO as well.
> >
>
> I'm not sure. If Alex.W agree on this, I can have one patch to address
> it in future :-)
I agree with David, there should be a uapi eeh header and the vfio
documentation should define that as the valid set of functions.
> >> - The UAPI interface is expected to be stable, or back-compatible when
> >> introducing changes to it. The defines (at least their names) in eeh.h
> >> doesn't have to be stable.
> >
> >But is there any reason not to make them stable. The VFIO case shows
> >that we need to lock down an interface somewhere. I don't see that
> >there's an advantage to having a not locked down interface in addition
> >to a locked down one.
> >
>
> I was saying that the UAPI interace should be stable. I don't see
> there're any reasons the UAPI interface will be changed in near
> future :)
When I asked previously whether a translation layer was needed between
the vfio uapi and kernel internal eeh api I was told that the values
were defined by the spapr spec and therefore not changeable. If that's
true, we shouldn't need to worry about exporting them via uapi. Thanks,
Alex
> >> - I would like keep the code between EEH/VFIO independent to each other
> >> enough.
> >>
> >> Thanks,
> >> Gavin
> >>
> >> >> By the way, Alex Williamson, could you please ACK this if you don't
> >> >> find obvious errors in the patch. I really want this to be merged
> >> >> to 4.1.
> >> >>
> >> >> 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
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
@ 2015-03-23 16:14 ` Alex Williamson
0 siblings, 0 replies; 28+ messages in thread
From: Alex Williamson @ 2015-03-23 16:14 UTC (permalink / raw)
To: Gavin Shan; +Cc: aik, linuxppc-dev, agraf, kvm, David Gibson
On Mon, 2015-03-23 at 16:20 +1100, Gavin Shan wrote:
> On Mon, Mar 23, 2015 at 04:10:20PM +1100, David Gibson wrote:
> >On Mon, Mar 23, 2015 at 04:03:59PM +1100, Gavin Shan wrote:
> >> On Mon, Mar 23, 2015 at 02:43:03PM +1100, David Gibson wrote:
> >> >On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
> >> >> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
> >> >> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
> >> >> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
> >> >> >> 3 files changed, 57 insertions(+), 1 deletion(-)
> >> >> >>
> >> >> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> >> >> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
> >> >> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
> >> >> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
> >> >> >
> >> >> >These all seem to duplicate (under slightly different names) the
> >> >> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
> >> >> >make more sense to only have the uapi copy, surely.
> >> >> >
> >> >>
> >> >> David, Thanks for your time. The defines in eeh.h is used not only
> >> >> for mapping those constants defined in vfio.h, but also for validating
> >> >> arguments from debugfs entry "err_injct". So it'd better to keep
> >> >> them separate: one set is used by userspace, another set is used
> >> >> by host kernel.
> >> >
> >> >I don't understand why this is an argument for duplicating them. They
> >> >have the same meanings and values, why can't you use the uapi versions
> >> >everywhere?
> >> >
> >>
> >> We can drop the constants defined in eeh.h and just use those defines
> >> in vfio.h. But I prefer separate constants for some ovbious reasons
> >> except the one I mentioned above :)
> >>
> >> - We already had separate sets of constants for EEH and VFIO. For example,
> >> VFIO_EEH_PE_STATE_* in include/uapi/vfio.h always have counter part in
> >> arch/powerpc/asm/include/eeh.h
> >
> >Oh.. I see your point.
> >
> >Yes, the EEH internal code shouldn't need to use the VFIO defined
> >constants.
> >
> >But it's still silly to duplicate, I think the correct think would be
> >to expose the EEH constants in uapi, and use those for VFIO as well.
> >
>
> I'm not sure. If Alex.W agree on this, I can have one patch to address
> it in future :-)
I agree with David, there should be a uapi eeh header and the vfio
documentation should define that as the valid set of functions.
> >> - The UAPI interface is expected to be stable, or back-compatible when
> >> introducing changes to it. The defines (at least their names) in eeh.h
> >> doesn't have to be stable.
> >
> >But is there any reason not to make them stable. The VFIO case shows
> >that we need to lock down an interface somewhere. I don't see that
> >there's an advantage to having a not locked down interface in addition
> >to a locked down one.
> >
>
> I was saying that the UAPI interace should be stable. I don't see
> there're any reasons the UAPI interface will be changed in near
> future :)
When I asked previously whether a translation layer was needed between
the vfio uapi and kernel internal eeh api I was told that the values
were defined by the spapr spec and therefore not changeable. If that's
true, we shouldn't need to worry about exporting them via uapi. Thanks,
Alex
> >> - I would like keep the code between EEH/VFIO independent to each other
> >> enough.
> >>
> >> Thanks,
> >> Gavin
> >>
> >> >> By the way, Alex Williamson, could you please ACK this if you don't
> >> >> find obvious errors in the patch. I really want this to be merged
> >> >> to 4.1.
> >> >>
> >> >> 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
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
2015-03-23 16:14 ` Alex Williamson
@ 2015-03-23 22:47 ` Gavin Shan
-1 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-23 22:47 UTC (permalink / raw)
To: Alex Williamson; +Cc: Gavin Shan, aik, linuxppc-dev, agraf, kvm, David Gibson
On Mon, Mar 23, 2015 at 10:14:59AM -0600, Alex Williamson wrote:
>On Mon, 2015-03-23 at 16:20 +1100, Gavin Shan wrote:
>> On Mon, Mar 23, 2015 at 04:10:20PM +1100, David Gibson wrote:
>> >On Mon, Mar 23, 2015 at 04:03:59PM +1100, Gavin Shan wrote:
>> >> On Mon, Mar 23, 2015 at 02:43:03PM +1100, David Gibson wrote:
>> >> >On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
>> >> >> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
>> >> >> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
>> >> >> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
>> >> >> >> 3 files changed, 57 insertions(+), 1 deletion(-)
>> >> >> >>
>> >> >> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>> >> >> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
>> >> >> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
>> >> >> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
>> >> >> >
>> >> >> >These all seem to duplicate (under slightly different names) the
>> >> >> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
>> >> >> >make more sense to only have the uapi copy, surely.
>> >> >> >
>> >> >>
>> >> >> David, Thanks for your time. The defines in eeh.h is used not only
>> >> >> for mapping those constants defined in vfio.h, but also for validating
>> >> >> arguments from debugfs entry "err_injct". So it'd better to keep
>> >> >> them separate: one set is used by userspace, another set is used
>> >> >> by host kernel.
>> >> >
>> >> >I don't understand why this is an argument for duplicating them. They
>> >> >have the same meanings and values, why can't you use the uapi versions
>> >> >everywhere?
>> >> >
>> >>
>> >> We can drop the constants defined in eeh.h and just use those defines
>> >> in vfio.h. But I prefer separate constants for some ovbious reasons
>> >> except the one I mentioned above :)
>> >>
>> >> - We already had separate sets of constants for EEH and VFIO. For example,
>> >> VFIO_EEH_PE_STATE_* in include/uapi/vfio.h always have counter part in
>> >> arch/powerpc/asm/include/eeh.h
>> >
>> >Oh.. I see your point.
>> >
>> >Yes, the EEH internal code shouldn't need to use the VFIO defined
>> >constants.
>> >
>> >But it's still silly to duplicate, I think the correct think would be
>> >to expose the EEH constants in uapi, and use those for VFIO as well.
>> >
>>
>> I'm not sure. If Alex.W agree on this, I can have one patch to address
>> it in future :-)
>
>I agree with David, there should be a uapi eeh header and the vfio
>documentation should define that as the valid set of functions.
>
>> >> - The UAPI interface is expected to be stable, or back-compatible when
>> >> introducing changes to it. The defines (at least their names) in eeh.h
>> >> doesn't have to be stable.
>> >
>> >But is there any reason not to make them stable. The VFIO case shows
>> >that we need to lock down an interface somewhere. I don't see that
>> >there's an advantage to having a not locked down interface in addition
>> >to a locked down one.
>> >
>>
>> I was saying that the UAPI interace should be stable. I don't see
>> there're any reasons the UAPI interface will be changed in near
>> future :)
>
>When I asked previously whether a translation layer was needed between
>the vfio uapi and kernel internal eeh api I was told that the values
>were defined by the spapr spec and therefore not changeable. If that's
>true, we shouldn't need to worry about exporting them via uapi. Thanks,
>
Well. It sounds a good idea. I'll add one separate patch, which would be
PATCH[3/3] in next revision to address it. Currently, there are not
too much constants, which can be put into arch/powerpc/include/uapi/asm/eeh.h.
The following list are the candidates from include/api/vfio.h, which can be
moved to uapi/asm/eeh.h. The names of those constants will be changed from
"VFIO_EEH_*" to "EEH_*" and then user space will have some code as folows
to use them. That looks a bit weird. Do we really want moving those constants
to uapi/asm/eeh.h and rename them accordingly?
#include <stdio.h>
#include <sys/ioctl.h>
#include <eeh.h>
#include <vfio.h>
struct vfio_eeh_pe_op pe_op = { .argsz = sizeof(pe_op), .flags = 0 };
pe_op.op = VFIO_EEH_PE_INJECT_ERR; /* Its prefix is "VFIO_EEH_* */
pe_op.err.type = EEH_ERR_TYPE_32; /* It starts with "EEH_*" */
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);
---
#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_ERR_TYPE_32 0 /* 32-bits EEH error type */
#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
Thanks,
Gavin
>Alex
>
>> >> - I would like keep the code between EEH/VFIO independent to each other
>> >> enough.
>> >>
>> >> Thanks,
>> >> Gavin
>> >>
>> >> >> By the way, Alex Williamson, could you please ACK this if you don't
>> >> >> find obvious errors in the patch. I really want this to be merged
>> >> >> to 4.1.
>> >> >>
>> >> >> 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
>>
>>
>
>
>
>_______________________________________________
>Linuxppc-dev mailing list
>Linuxppc-dev@lists.ozlabs.org
>https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
@ 2015-03-23 22:47 ` Gavin Shan
0 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-23 22:47 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm, aik, Gavin Shan, agraf, linuxppc-dev, David Gibson
On Mon, Mar 23, 2015 at 10:14:59AM -0600, Alex Williamson wrote:
>On Mon, 2015-03-23 at 16:20 +1100, Gavin Shan wrote:
>> On Mon, Mar 23, 2015 at 04:10:20PM +1100, David Gibson wrote:
>> >On Mon, Mar 23, 2015 at 04:03:59PM +1100, Gavin Shan wrote:
>> >> On Mon, Mar 23, 2015 at 02:43:03PM +1100, David Gibson wrote:
>> >> >On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
>> >> >> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
>> >> >> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
>> >> >> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
>> >> >> >> 3 files changed, 57 insertions(+), 1 deletion(-)
>> >> >> >>
>> >> >> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>> >> >> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
>> >> >> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
>> >> >> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
>> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
>> >> >> >
>> >> >> >These all seem to duplicate (under slightly different names) the
>> >> >> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
>> >> >> >make more sense to only have the uapi copy, surely.
>> >> >> >
>> >> >>
>> >> >> David, Thanks for your time. The defines in eeh.h is used not only
>> >> >> for mapping those constants defined in vfio.h, but also for validating
>> >> >> arguments from debugfs entry "err_injct". So it'd better to keep
>> >> >> them separate: one set is used by userspace, another set is used
>> >> >> by host kernel.
>> >> >
>> >> >I don't understand why this is an argument for duplicating them. They
>> >> >have the same meanings and values, why can't you use the uapi versions
>> >> >everywhere?
>> >> >
>> >>
>> >> We can drop the constants defined in eeh.h and just use those defines
>> >> in vfio.h. But I prefer separate constants for some ovbious reasons
>> >> except the one I mentioned above :)
>> >>
>> >> - We already had separate sets of constants for EEH and VFIO. For example,
>> >> VFIO_EEH_PE_STATE_* in include/uapi/vfio.h always have counter part in
>> >> arch/powerpc/asm/include/eeh.h
>> >
>> >Oh.. I see your point.
>> >
>> >Yes, the EEH internal code shouldn't need to use the VFIO defined
>> >constants.
>> >
>> >But it's still silly to duplicate, I think the correct think would be
>> >to expose the EEH constants in uapi, and use those for VFIO as well.
>> >
>>
>> I'm not sure. If Alex.W agree on this, I can have one patch to address
>> it in future :-)
>
>I agree with David, there should be a uapi eeh header and the vfio
>documentation should define that as the valid set of functions.
>
>> >> - The UAPI interface is expected to be stable, or back-compatible when
>> >> introducing changes to it. The defines (at least their names) in eeh.h
>> >> doesn't have to be stable.
>> >
>> >But is there any reason not to make them stable. The VFIO case shows
>> >that we need to lock down an interface somewhere. I don't see that
>> >there's an advantage to having a not locked down interface in addition
>> >to a locked down one.
>> >
>>
>> I was saying that the UAPI interace should be stable. I don't see
>> there're any reasons the UAPI interface will be changed in near
>> future :)
>
>When I asked previously whether a translation layer was needed between
>the vfio uapi and kernel internal eeh api I was told that the values
>were defined by the spapr spec and therefore not changeable. If that's
>true, we shouldn't need to worry about exporting them via uapi. Thanks,
>
Well. It sounds a good idea. I'll add one separate patch, which would be
PATCH[3/3] in next revision to address it. Currently, there are not
too much constants, which can be put into arch/powerpc/include/uapi/asm/eeh.h.
The following list are the candidates from include/api/vfio.h, which can be
moved to uapi/asm/eeh.h. The names of those constants will be changed from
"VFIO_EEH_*" to "EEH_*" and then user space will have some code as folows
to use them. That looks a bit weird. Do we really want moving those constants
to uapi/asm/eeh.h and rename them accordingly?
#include <stdio.h>
#include <sys/ioctl.h>
#include <eeh.h>
#include <vfio.h>
struct vfio_eeh_pe_op pe_op = { .argsz = sizeof(pe_op), .flags = 0 };
pe_op.op = VFIO_EEH_PE_INJECT_ERR; /* Its prefix is "VFIO_EEH_* */
pe_op.err.type = EEH_ERR_TYPE_32; /* It starts with "EEH_*" */
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);
---
#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_ERR_TYPE_32 0 /* 32-bits EEH error type */
#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
Thanks,
Gavin
>Alex
>
>> >> - I would like keep the code between EEH/VFIO independent to each other
>> >> enough.
>> >>
>> >> Thanks,
>> >> Gavin
>> >>
>> >> >> By the way, Alex Williamson, could you please ACK this if you don't
>> >> >> find obvious errors in the patch. I really want this to be merged
>> >> >> to 4.1.
>> >> >>
>> >> >> 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
>>
>>
>
>
>
>_______________________________________________
>Linuxppc-dev mailing list
>Linuxppc-dev@lists.ozlabs.org
>https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
2015-03-23 22:47 ` Gavin Shan
@ 2015-03-24 1:22 ` David Gibson
-1 siblings, 0 replies; 28+ messages in thread
From: David Gibson @ 2015-03-24 1:22 UTC (permalink / raw)
To: Gavin Shan; +Cc: Alex Williamson, aik, linuxppc-dev, agraf, kvm
[-- Attachment #1: Type: text/plain, Size: 13107 bytes --]
On Tue, Mar 24, 2015 at 09:47:54AM +1100, Gavin Shan wrote:
> On Mon, Mar 23, 2015 at 10:14:59AM -0600, Alex Williamson wrote:
> >On Mon, 2015-03-23 at 16:20 +1100, Gavin Shan wrote:
> >> On Mon, Mar 23, 2015 at 04:10:20PM +1100, David Gibson wrote:
> >> >On Mon, Mar 23, 2015 at 04:03:59PM +1100, Gavin Shan wrote:
> >> >> On Mon, Mar 23, 2015 at 02:43:03PM +1100, David Gibson wrote:
> >> >> >On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
> >> >> >> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
> >> >> >> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
> >> >> >> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
> >> >> >> >> 3 files changed, 57 insertions(+), 1 deletion(-)
> >> >> >> >>
> >> >> >> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> >> >> >> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
> >> >> >> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
> >> >> >> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
> >> >> >> >
> >> >> >> >These all seem to duplicate (under slightly different names) the
> >> >> >> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
> >> >> >> >make more sense to only have the uapi copy, surely.
> >> >> >> >
> >> >> >>
> >> >> >> David, Thanks for your time. The defines in eeh.h is used not only
> >> >> >> for mapping those constants defined in vfio.h, but also for validating
> >> >> >> arguments from debugfs entry "err_injct". So it'd better to keep
> >> >> >> them separate: one set is used by userspace, another set is used
> >> >> >> by host kernel.
> >> >> >
> >> >> >I don't understand why this is an argument for duplicating them. They
> >> >> >have the same meanings and values, why can't you use the uapi versions
> >> >> >everywhere?
> >> >> >
> >> >>
> >> >> We can drop the constants defined in eeh.h and just use those defines
> >> >> in vfio.h. But I prefer separate constants for some ovbious reasons
> >> >> except the one I mentioned above :)
> >> >>
> >> >> - We already had separate sets of constants for EEH and VFIO. For example,
> >> >> VFIO_EEH_PE_STATE_* in include/uapi/vfio.h always have counter part in
> >> >> arch/powerpc/asm/include/eeh.h
> >> >
> >> >Oh.. I see your point.
> >> >
> >> >Yes, the EEH internal code shouldn't need to use the VFIO defined
> >> >constants.
> >> >
> >> >But it's still silly to duplicate, I think the correct think would be
> >> >to expose the EEH constants in uapi, and use those for VFIO as well.
> >> >
> >>
> >> I'm not sure. If Alex.W agree on this, I can have one patch to address
> >> it in future :-)
> >
> >I agree with David, there should be a uapi eeh header and the vfio
> >documentation should define that as the valid set of functions.
> >
> >> >> - The UAPI interface is expected to be stable, or back-compatible when
> >> >> introducing changes to it. The defines (at least their names) in eeh.h
> >> >> doesn't have to be stable.
> >> >
> >> >But is there any reason not to make them stable. The VFIO case shows
> >> >that we need to lock down an interface somewhere. I don't see that
> >> >there's an advantage to having a not locked down interface in addition
> >> >to a locked down one.
> >> >
> >>
> >> I was saying that the UAPI interace should be stable. I don't see
> >> there're any reasons the UAPI interface will be changed in near
> >> future :)
> >
> >When I asked previously whether a translation layer was needed between
> >the vfio uapi and kernel internal eeh api I was told that the values
> >were defined by the spapr spec and therefore not changeable. If that's
> >true, we shouldn't need to worry about exporting them via uapi. Thanks,
> >
>
> Well. It sounds a good idea. I'll add one separate patch, which would be
> PATCH[3/3] in next revision to address it. Currently, there are not
> too much constants, which can be put into arch/powerpc/include/uapi/asm/eeh.h.
> The following list are the candidates from include/api/vfio.h, which can be
> moved to uapi/asm/eeh.h. The names of those constants will be changed from
> "VFIO_EEH_*" to "EEH_*" and then user space will have some code as folows
> to use them. That looks a bit weird. Do we really want moving those constants
> to uapi/asm/eeh.h and rename them accordingly?
I think so, yes. I think the patch series will be cleaner if you do
the exporting of the EEH constants in 1/3 then build your other
patches on top of that.
> #include <stdio.h>
> #include <sys/ioctl.h>
> #include <eeh.h>
> #include <vfio.h>
>
> struct vfio_eeh_pe_op pe_op = { .argsz = sizeof(pe_op), .flags = 0 };
>
> pe_op.op = VFIO_EEH_PE_INJECT_ERR; /* Its prefix is "VFIO_EEH_* */
> pe_op.err.type = EEH_ERR_TYPE_32; /* It starts with "EEH_*" */
I think that's fine.
> 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);
>
> ---
>
> #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_ERR_TYPE_32 0 /* 32-bits EEH error type */
> #define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
> #define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
> #define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
> #define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
> #define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
> #define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
> #define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
> #define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
> #define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
> #define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
> #define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
> #define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
> #define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
> #define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
> #define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
> #define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
> #define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
> #define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
> #define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
> #define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
> #define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
>
> Thanks,
> Gavin
>
> >Alex
> >
> >> >> - I would like keep the code between EEH/VFIO independent to each other
> >> >> enough.
> >> >>
> >> >> Thanks,
> >> >> Gavin
> >> >>
> >> >> >> By the way, Alex Williamson, could you please ACK this if you don't
> >> >> >> find obvious errors in the patch. I really want this to be merged
> >> >> >> to 4.1.
> >> >> >>
> >> >> >> Thanks,
> >> >> >> Gavin
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >
> >> >>
> >> >>
> >> >
> >>
> >>
> >
> >
> >
> >_______________________________________________
> >Linuxppc-dev mailing list
> >Linuxppc-dev@lists.ozlabs.org
> >https://lists.ozlabs.org/listinfo/linuxppc-dev
>
--
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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
@ 2015-03-24 1:22 ` David Gibson
0 siblings, 0 replies; 28+ messages in thread
From: David Gibson @ 2015-03-24 1:22 UTC (permalink / raw)
To: Gavin Shan; +Cc: aik, linuxppc-dev, Alex Williamson, agraf, kvm
[-- Attachment #1: Type: text/plain, Size: 13107 bytes --]
On Tue, Mar 24, 2015 at 09:47:54AM +1100, Gavin Shan wrote:
> On Mon, Mar 23, 2015 at 10:14:59AM -0600, Alex Williamson wrote:
> >On Mon, 2015-03-23 at 16:20 +1100, Gavin Shan wrote:
> >> On Mon, Mar 23, 2015 at 04:10:20PM +1100, David Gibson wrote:
> >> >On Mon, Mar 23, 2015 at 04:03:59PM +1100, Gavin Shan wrote:
> >> >> On Mon, Mar 23, 2015 at 02:43:03PM +1100, David Gibson wrote:
> >> >> >On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
> >> >> >> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
> >> >> >> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
> >> >> >> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
> >> >> >> >> 3 files changed, 57 insertions(+), 1 deletion(-)
> >> >> >> >>
> >> >> >> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> >> >> >> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
> >> >> >> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
> >> >> >> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
> >> >> >> >
> >> >> >> >These all seem to duplicate (under slightly different names) the
> >> >> >> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
> >> >> >> >make more sense to only have the uapi copy, surely.
> >> >> >> >
> >> >> >>
> >> >> >> David, Thanks for your time. The defines in eeh.h is used not only
> >> >> >> for mapping those constants defined in vfio.h, but also for validating
> >> >> >> arguments from debugfs entry "err_injct". So it'd better to keep
> >> >> >> them separate: one set is used by userspace, another set is used
> >> >> >> by host kernel.
> >> >> >
> >> >> >I don't understand why this is an argument for duplicating them. They
> >> >> >have the same meanings and values, why can't you use the uapi versions
> >> >> >everywhere?
> >> >> >
> >> >>
> >> >> We can drop the constants defined in eeh.h and just use those defines
> >> >> in vfio.h. But I prefer separate constants for some ovbious reasons
> >> >> except the one I mentioned above :)
> >> >>
> >> >> - We already had separate sets of constants for EEH and VFIO. For example,
> >> >> VFIO_EEH_PE_STATE_* in include/uapi/vfio.h always have counter part in
> >> >> arch/powerpc/asm/include/eeh.h
> >> >
> >> >Oh.. I see your point.
> >> >
> >> >Yes, the EEH internal code shouldn't need to use the VFIO defined
> >> >constants.
> >> >
> >> >But it's still silly to duplicate, I think the correct think would be
> >> >to expose the EEH constants in uapi, and use those for VFIO as well.
> >> >
> >>
> >> I'm not sure. If Alex.W agree on this, I can have one patch to address
> >> it in future :-)
> >
> >I agree with David, there should be a uapi eeh header and the vfio
> >documentation should define that as the valid set of functions.
> >
> >> >> - The UAPI interface is expected to be stable, or back-compatible when
> >> >> introducing changes to it. The defines (at least their names) in eeh.h
> >> >> doesn't have to be stable.
> >> >
> >> >But is there any reason not to make them stable. The VFIO case shows
> >> >that we need to lock down an interface somewhere. I don't see that
> >> >there's an advantage to having a not locked down interface in addition
> >> >to a locked down one.
> >> >
> >>
> >> I was saying that the UAPI interace should be stable. I don't see
> >> there're any reasons the UAPI interface will be changed in near
> >> future :)
> >
> >When I asked previously whether a translation layer was needed between
> >the vfio uapi and kernel internal eeh api I was told that the values
> >were defined by the spapr spec and therefore not changeable. If that's
> >true, we shouldn't need to worry about exporting them via uapi. Thanks,
> >
>
> Well. It sounds a good idea. I'll add one separate patch, which would be
> PATCH[3/3] in next revision to address it. Currently, there are not
> too much constants, which can be put into arch/powerpc/include/uapi/asm/eeh.h.
> The following list are the candidates from include/api/vfio.h, which can be
> moved to uapi/asm/eeh.h. The names of those constants will be changed from
> "VFIO_EEH_*" to "EEH_*" and then user space will have some code as folows
> to use them. That looks a bit weird. Do we really want moving those constants
> to uapi/asm/eeh.h and rename them accordingly?
I think so, yes. I think the patch series will be cleaner if you do
the exporting of the EEH constants in 1/3 then build your other
patches on top of that.
> #include <stdio.h>
> #include <sys/ioctl.h>
> #include <eeh.h>
> #include <vfio.h>
>
> struct vfio_eeh_pe_op pe_op = { .argsz = sizeof(pe_op), .flags = 0 };
>
> pe_op.op = VFIO_EEH_PE_INJECT_ERR; /* Its prefix is "VFIO_EEH_* */
> pe_op.err.type = EEH_ERR_TYPE_32; /* It starts with "EEH_*" */
I think that's fine.
> 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);
>
> ---
>
> #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_ERR_TYPE_32 0 /* 32-bits EEH error type */
> #define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
> #define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
> #define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
> #define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
> #define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
> #define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
> #define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
> #define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
> #define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
> #define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
> #define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
> #define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
> #define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
> #define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
> #define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
> #define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
> #define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
> #define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
> #define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
> #define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
> #define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
>
> Thanks,
> Gavin
>
> >Alex
> >
> >> >> - I would like keep the code between EEH/VFIO independent to each other
> >> >> enough.
> >> >>
> >> >> Thanks,
> >> >> Gavin
> >> >>
> >> >> >> By the way, Alex Williamson, could you please ACK this if you don't
> >> >> >> find obvious errors in the patch. I really want this to be merged
> >> >> >> to 4.1.
> >> >> >>
> >> >> >> Thanks,
> >> >> >> Gavin
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >
> >> >>
> >> >>
> >> >
> >>
> >>
> >
> >
> >
> >_______________________________________________
> >Linuxppc-dev mailing list
> >Linuxppc-dev@lists.ozlabs.org
> >https://lists.ozlabs.org/listinfo/linuxppc-dev
>
--
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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
2015-03-24 1:22 ` David Gibson
@ 2015-03-24 2:41 ` Gavin Shan
-1 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-24 2:41 UTC (permalink / raw)
To: David Gibson; +Cc: Gavin Shan, Alex Williamson, aik, linuxppc-dev, agraf, kvm
On Tue, Mar 24, 2015 at 12:22:25PM +1100, David Gibson wrote:
>On Tue, Mar 24, 2015 at 09:47:54AM +1100, Gavin Shan wrote:
>> On Mon, Mar 23, 2015 at 10:14:59AM -0600, Alex Williamson wrote:
>> >On Mon, 2015-03-23 at 16:20 +1100, Gavin Shan wrote:
>> >> On Mon, Mar 23, 2015 at 04:10:20PM +1100, David Gibson wrote:
>> >> >On Mon, Mar 23, 2015 at 04:03:59PM +1100, Gavin Shan wrote:
>> >> >> On Mon, Mar 23, 2015 at 02:43:03PM +1100, David Gibson wrote:
>> >> >> >On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
>> >> >> >> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
>> >> >> >> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
>> >> >> >> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
>> >> >> >> >> 3 files changed, 57 insertions(+), 1 deletion(-)
>> >> >> >> >>
>> >> >> >> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>> >> >> >> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
>> >> >> >> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
>> >> >> >> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
>> >> >> >> >
>> >> >> >> >These all seem to duplicate (under slightly different names) the
>> >> >> >> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
>> >> >> >> >make more sense to only have the uapi copy, surely.
>> >> >> >> >
>> >> >> >>
>> >> >> >> David, Thanks for your time. The defines in eeh.h is used not only
>> >> >> >> for mapping those constants defined in vfio.h, but also for validating
>> >> >> >> arguments from debugfs entry "err_injct". So it'd better to keep
>> >> >> >> them separate: one set is used by userspace, another set is used
>> >> >> >> by host kernel.
>> >> >> >
>> >> >> >I don't understand why this is an argument for duplicating them. They
>> >> >> >have the same meanings and values, why can't you use the uapi versions
>> >> >> >everywhere?
>> >> >> >
>> >> >>
>> >> >> We can drop the constants defined in eeh.h and just use those defines
>> >> >> in vfio.h. But I prefer separate constants for some ovbious reasons
>> >> >> except the one I mentioned above :)
>> >> >>
>> >> >> - We already had separate sets of constants for EEH and VFIO. For example,
>> >> >> VFIO_EEH_PE_STATE_* in include/uapi/vfio.h always have counter part in
>> >> >> arch/powerpc/asm/include/eeh.h
>> >> >
>> >> >Oh.. I see your point.
>> >> >
>> >> >Yes, the EEH internal code shouldn't need to use the VFIO defined
>> >> >constants.
>> >> >
>> >> >But it's still silly to duplicate, I think the correct think would be
>> >> >to expose the EEH constants in uapi, and use those for VFIO as well.
>> >> >
>> >>
>> >> I'm not sure. If Alex.W agree on this, I can have one patch to address
>> >> it in future :-)
>> >
>> >I agree with David, there should be a uapi eeh header and the vfio
>> >documentation should define that as the valid set of functions.
>> >
>> >> >> - The UAPI interface is expected to be stable, or back-compatible when
>> >> >> introducing changes to it. The defines (at least their names) in eeh.h
>> >> >> doesn't have to be stable.
>> >> >
>> >> >But is there any reason not to make them stable. The VFIO case shows
>> >> >that we need to lock down an interface somewhere. I don't see that
>> >> >there's an advantage to having a not locked down interface in addition
>> >> >to a locked down one.
>> >> >
>> >>
>> >> I was saying that the UAPI interace should be stable. I don't see
>> >> there're any reasons the UAPI interface will be changed in near
>> >> future :)
>> >
>> >When I asked previously whether a translation layer was needed between
>> >the vfio uapi and kernel internal eeh api I was told that the values
>> >were defined by the spapr spec and therefore not changeable. If that's
>> >true, we shouldn't need to worry about exporting them via uapi. Thanks,
>> >
>>
>> Well. It sounds a good idea. I'll add one separate patch, which would be
>> PATCH[3/3] in next revision to address it. Currently, there are not
>> too much constants, which can be put into arch/powerpc/include/uapi/asm/eeh.h.
>> The following list are the candidates from include/api/vfio.h, which can be
>> moved to uapi/asm/eeh.h. The names of those constants will be changed from
>> "VFIO_EEH_*" to "EEH_*" and then user space will have some code as folows
>> to use them. That looks a bit weird. Do we really want moving those constants
>> to uapi/asm/eeh.h and rename them accordingly?
>
>I think so, yes. I think the patch series will be cleaner if you do
>the exporting of the EEH constants in 1/3 then build your other
>patches on top of that.
>
Thanks for confirm, David. I'll have one patch (1/3) to address this
in next revision.
Thanks,
Gavin
>> #include <stdio.h>
>> #include <sys/ioctl.h>
>> #include <eeh.h>
>> #include <vfio.h>
>>
>> struct vfio_eeh_pe_op pe_op = { .argsz = sizeof(pe_op), .flags = 0 };
>>
>> pe_op.op = VFIO_EEH_PE_INJECT_ERR; /* Its prefix is "VFIO_EEH_* */
>> pe_op.err.type = EEH_ERR_TYPE_32; /* It starts with "EEH_*" */
>
>I think that's fine.
>
>> 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);
>>
>> ---
>>
>> #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_ERR_TYPE_32 0 /* 32-bits EEH error type */
>> #define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
>> #define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
>> #define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
>> #define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
>> #define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
>> #define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
>> #define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
>> #define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
>> #define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
>> #define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
>> #define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
>> #define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
>> #define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
>> #define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
>> #define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
>> #define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
>> #define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
>> #define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
>> #define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
>> #define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
>> #define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
>>
>> Thanks,
>> Gavin
>>
>> >Alex
>> >
>> >> >> - I would like keep the code between EEH/VFIO independent to each other
>> >> >> enough.
>> >> >>
>> >> >> Thanks,
>> >> >> Gavin
>> >> >>
>> >> >> >> By the way, Alex Williamson, could you please ACK this if you don't
>> >> >> >> find obvious errors in the patch. I really want this to be merged
>> >> >> >> to 4.1.
>> >> >> >>
>> >> >> >> Thanks,
>> >> >> >> Gavin
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >
>> >> >>
>> >> >>
>> >> >
>> >>
>> >>
>> >
>> >
>> >
>> >_______________________________________________
>> >Linuxppc-dev mailing list
>> >Linuxppc-dev@lists.ozlabs.org
>> >https://lists.ozlabs.org/listinfo/linuxppc-dev
>>
>
>--
>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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/2] drivers/vfio: Support EEH error injection
@ 2015-03-24 2:41 ` Gavin Shan
0 siblings, 0 replies; 28+ messages in thread
From: Gavin Shan @ 2015-03-24 2:41 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, aik, Gavin Shan, agraf, linuxppc-dev, Alex Williamson
On Tue, Mar 24, 2015 at 12:22:25PM +1100, David Gibson wrote:
>On Tue, Mar 24, 2015 at 09:47:54AM +1100, Gavin Shan wrote:
>> On Mon, Mar 23, 2015 at 10:14:59AM -0600, Alex Williamson wrote:
>> >On Mon, 2015-03-23 at 16:20 +1100, Gavin Shan wrote:
>> >> On Mon, Mar 23, 2015 at 04:10:20PM +1100, David Gibson wrote:
>> >> >On Mon, Mar 23, 2015 at 04:03:59PM +1100, Gavin Shan wrote:
>> >> >> On Mon, Mar 23, 2015 at 02:43:03PM +1100, David Gibson wrote:
>> >> >> >On Mon, Mar 23, 2015 at 12:56:36PM +1100, Gavin Shan wrote:
>> >> >> >> On Mon, Mar 23, 2015 at 12:39:45PM +1100, David Gibson wrote:
>> >> >> >> >On Sat, Mar 21, 2015 at 06:58:45AM +1100, Gavin Shan wrote:
>> >> >> >> >> 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 | 36 +++++++++++++++++++++++++++++++++++-
>> >> >> >> >> 3 files changed, 57 insertions(+), 1 deletion(-)
>> >> >> >> >>
>> >> >> >> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>> >> >> >> >> index 96978ec..c6e11a3 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 = VFIO_EEH_ERR_TYPE_32;
>> >> >> >> >> + pe_op.err.func = VFIO_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..f68e962 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,29 @@ 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_ERR_TYPE_32 0 /* 32-bits EEH error type */
>> >> >> >> >> +#define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
>> >> >> >> >> +#define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
>> >> >> >> >
>> >> >> >> >These all seem to duplicate (under slightly different names) the
>> >> >> >> >defines you just added in arch/powerpc/include/asm/eeh.h. It would
>> >> >> >> >make more sense to only have the uapi copy, surely.
>> >> >> >> >
>> >> >> >>
>> >> >> >> David, Thanks for your time. The defines in eeh.h is used not only
>> >> >> >> for mapping those constants defined in vfio.h, but also for validating
>> >> >> >> arguments from debugfs entry "err_injct". So it'd better to keep
>> >> >> >> them separate: one set is used by userspace, another set is used
>> >> >> >> by host kernel.
>> >> >> >
>> >> >> >I don't understand why this is an argument for duplicating them. They
>> >> >> >have the same meanings and values, why can't you use the uapi versions
>> >> >> >everywhere?
>> >> >> >
>> >> >>
>> >> >> We can drop the constants defined in eeh.h and just use those defines
>> >> >> in vfio.h. But I prefer separate constants for some ovbious reasons
>> >> >> except the one I mentioned above :)
>> >> >>
>> >> >> - We already had separate sets of constants for EEH and VFIO. For example,
>> >> >> VFIO_EEH_PE_STATE_* in include/uapi/vfio.h always have counter part in
>> >> >> arch/powerpc/asm/include/eeh.h
>> >> >
>> >> >Oh.. I see your point.
>> >> >
>> >> >Yes, the EEH internal code shouldn't need to use the VFIO defined
>> >> >constants.
>> >> >
>> >> >But it's still silly to duplicate, I think the correct think would be
>> >> >to expose the EEH constants in uapi, and use those for VFIO as well.
>> >> >
>> >>
>> >> I'm not sure. If Alex.W agree on this, I can have one patch to address
>> >> it in future :-)
>> >
>> >I agree with David, there should be a uapi eeh header and the vfio
>> >documentation should define that as the valid set of functions.
>> >
>> >> >> - The UAPI interface is expected to be stable, or back-compatible when
>> >> >> introducing changes to it. The defines (at least their names) in eeh.h
>> >> >> doesn't have to be stable.
>> >> >
>> >> >But is there any reason not to make them stable. The VFIO case shows
>> >> >that we need to lock down an interface somewhere. I don't see that
>> >> >there's an advantage to having a not locked down interface in addition
>> >> >to a locked down one.
>> >> >
>> >>
>> >> I was saying that the UAPI interace should be stable. I don't see
>> >> there're any reasons the UAPI interface will be changed in near
>> >> future :)
>> >
>> >When I asked previously whether a translation layer was needed between
>> >the vfio uapi and kernel internal eeh api I was told that the values
>> >were defined by the spapr spec and therefore not changeable. If that's
>> >true, we shouldn't need to worry about exporting them via uapi. Thanks,
>> >
>>
>> Well. It sounds a good idea. I'll add one separate patch, which would be
>> PATCH[3/3] in next revision to address it. Currently, there are not
>> too much constants, which can be put into arch/powerpc/include/uapi/asm/eeh.h.
>> The following list are the candidates from include/api/vfio.h, which can be
>> moved to uapi/asm/eeh.h. The names of those constants will be changed from
>> "VFIO_EEH_*" to "EEH_*" and then user space will have some code as folows
>> to use them. That looks a bit weird. Do we really want moving those constants
>> to uapi/asm/eeh.h and rename them accordingly?
>
>I think so, yes. I think the patch series will be cleaner if you do
>the exporting of the EEH constants in 1/3 then build your other
>patches on top of that.
>
Thanks for confirm, David. I'll have one patch (1/3) to address this
in next revision.
Thanks,
Gavin
>> #include <stdio.h>
>> #include <sys/ioctl.h>
>> #include <eeh.h>
>> #include <vfio.h>
>>
>> struct vfio_eeh_pe_op pe_op = { .argsz = sizeof(pe_op), .flags = 0 };
>>
>> pe_op.op = VFIO_EEH_PE_INJECT_ERR; /* Its prefix is "VFIO_EEH_* */
>> pe_op.err.type = EEH_ERR_TYPE_32; /* It starts with "EEH_*" */
>
>I think that's fine.
>
>> 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);
>>
>> ---
>>
>> #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_ERR_TYPE_32 0 /* 32-bits EEH error type */
>> #define VFIO_EEH_ERR_TYPE_64 1 /* 64-bits EEH error type */
>> #define VFIO_EEH_ERR_FUNC_LD_MEM_ADDR 0 /* Memory load */
>> #define VFIO_EEH_ERR_FUNC_LD_MEM_DATA 1
>> #define VFIO_EEH_ERR_FUNC_LD_IO_ADDR 2 /* IO load */
>> #define VFIO_EEH_ERR_FUNC_LD_IO_DATA 3
>> #define VFIO_EEH_ERR_FUNC_LD_CFG_ADDR 4 /* Config load */
>> #define VFIO_EEH_ERR_FUNC_LD_CFG_DATA 5
>> #define VFIO_EEH_ERR_FUNC_ST_MEM_ADDR 6 /* Memory store */
>> #define VFIO_EEH_ERR_FUNC_ST_MEM_DATA 7
>> #define VFIO_EEH_ERR_FUNC_ST_IO_ADDR 8 /* IO store */
>> #define VFIO_EEH_ERR_FUNC_ST_IO_DATA 9
>> #define VFIO_EEH_ERR_FUNC_ST_CFG_ADDR 10 /* Config store */
>> #define VFIO_EEH_ERR_FUNC_ST_CFG_DATA 11
>> #define VFIO_EEH_ERR_FUNC_DMA_RD_ADDR 12 /* DMA read */
>> #define VFIO_EEH_ERR_FUNC_DMA_RD_DATA 13
>> #define VFIO_EEH_ERR_FUNC_DMA_RD_MASTER 14
>> #define VFIO_EEH_ERR_FUNC_DMA_RD_TARGET 15
>> #define VFIO_EEH_ERR_FUNC_DMA_WR_ADDR 16 /* DMA write */
>> #define VFIO_EEH_ERR_FUNC_DMA_WR_DATA 17
>> #define VFIO_EEH_ERR_FUNC_DMA_WR_MASTER 18
>> #define VFIO_EEH_ERR_FUNC_DMA_WR_TARGET 19
>>
>> Thanks,
>> Gavin
>>
>> >Alex
>> >
>> >> >> - I would like keep the code between EEH/VFIO independent to each other
>> >> >> enough.
>> >> >>
>> >> >> Thanks,
>> >> >> Gavin
>> >> >>
>> >> >> >> By the way, Alex Williamson, could you please ACK this if you don't
>> >> >> >> find obvious errors in the patch. I really want this to be merged
>> >> >> >> to 4.1.
>> >> >> >>
>> >> >> >> Thanks,
>> >> >> >> Gavin
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >
>> >> >>
>> >> >>
>> >> >
>> >>
>> >>
>> >
>> >
>> >
>> >_______________________________________________
>> >Linuxppc-dev mailing list
>> >Linuxppc-dev@lists.ozlabs.org
>> >https://lists.ozlabs.org/listinfo/linuxppc-dev
>>
>
>--
>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
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2015-03-24 2:42 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-20 19:58 [PATCH v3 0/2] EEH Error Injection Support for VFIO Devices Gavin Shan
2015-03-20 19:58 ` Gavin Shan
2015-03-20 19:58 ` [PATCH v3 1/2] powerpc/eeh: Introduce eeh_pe_inject_err() Gavin Shan
2015-03-20 19:58 ` Gavin Shan
2015-03-23 1:33 ` David Gibson
2015-03-23 1:33 ` David Gibson
2015-03-20 19:58 ` [PATCH v3 2/2] drivers/vfio: Support EEH error injection Gavin Shan
2015-03-20 19:58 ` Gavin Shan
2015-03-23 1:39 ` David Gibson
2015-03-23 1:39 ` David Gibson
2015-03-23 1:56 ` Gavin Shan
2015-03-23 1:56 ` Gavin Shan
2015-03-23 3:43 ` David Gibson
2015-03-23 3:43 ` David Gibson
2015-03-23 5:03 ` Gavin Shan
2015-03-23 5:03 ` Gavin Shan
2015-03-23 5:10 ` David Gibson
2015-03-23 5:10 ` David Gibson
2015-03-23 5:20 ` Gavin Shan
2015-03-23 5:20 ` Gavin Shan
2015-03-23 16:14 ` Alex Williamson
2015-03-23 16:14 ` Alex Williamson
2015-03-23 22:47 ` Gavin Shan
2015-03-23 22:47 ` Gavin Shan
2015-03-24 1:22 ` David Gibson
2015-03-24 1:22 ` David Gibson
2015-03-24 2:41 ` Gavin Shan
2015-03-24 2:41 ` Gavin Shan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.