[Qemu-devel] [PATCH v5 0/4] sPAPR: Support EEH Error Injection

[Qemu-devel] [PATCH v5 0/4] sPAPR: Support EEH Error Injection

[Gavin Shan]

The patchset depends on below Linux upstream commits:

  commit ed3e81f ("powerpc/eeh: Move PE state constants around")
  commit ec33d36 ("powerpc/eeh: Introduce eeh_pe_inject_err()")

According to PAPR specification 2.7, there're 3 RTAS calls relevent to error
injection: "ibm,open-errinjct", "ibm,close-errinjct", "ibm,errinjct". The
userland utility "errinjct" running on guest utilizes those 3 RTAS calls like
this way: Call "ibm,open-errinjct" that returns open-token, which is passed to
"ibm,errinjct" together with error specific arguments to do error injection.
Finally, to return the open-token by calling "ibm,close-errinject".

"ibm,errinjct" can be used to inject various errors, not limited to EEH errors.
However, this patchset is going to support injecting EEH errors only for VFIO
PCI devices.

=========
Changelog
=========
v5:
   * Put "errinjct_token" to migration stream disregarding it's opened or
     not. Also, it starts to be supported from v4 vmstate_spapr.
   * Include powerpc/include/uapi/asm/eeh.h in scripts/update_linux_headers.sh
v4:
   * To record currently opened token, not next one as suggested by Alexey.
v3:
   * Replace random token number with incremental counter. Another boolean
     variable to track if it's opened. Both of them are added to migration
     stream.
   * The return value from sPAPRPHBClass::eeh_inject_error() can be passed
     to user directly. No need to do conversion.
   * Corrected error code to RTAS_OUT_CLOSE_ERROR in rtas_ibm_errinjct().
   * Don't expose error injection tokens for unsupported types.
v2:
   * Rebased to git://github.com/dgibson/qemu.git (branch: spapr-next)
   * Remove specific PCI error types in hw/ppc/spapr.h. Use those macros
     asm-powerpc/eeh.h instead.

Gavin Shan (4):
  scripts: Include arch/powerpc/include/uapi/asm/eeh.h
  linux-headers: Add eeh.h
  sPAPR: Support RTAS call ibm, {open, close}-errinjct
  sPAPR: Support RTAS call ibm,errinjct

 hw/ppc/spapr.c                  |   6 +-
 hw/ppc/spapr_pci.c              |  36 ++++++++++
 hw/ppc/spapr_pci_vfio.c         |  56 ++++++++++++++++
 hw/ppc/spapr_rtas.c             | 143 ++++++++++++++++++++++++++++++++++++++++
 include/hw/pci-host/spapr.h     |   2 +
 include/hw/ppc/spapr.h          |  17 ++++-
 linux-headers/asm-powerpc/eeh.h |  56 ++++++++++++++++
 scripts/update-linux-headers.sh |   1 +
 8 files changed, 315 insertions(+), 2 deletions(-)
 create mode 100644 linux-headers/asm-powerpc/eeh.h

-- 
2.1.0

[Qemu-devel] [PATCH v5 1/4] scripts: Include arch/powerpc/include/uapi/asm/eeh.h

[Gavin Shan]

This includes linux/arch/powerpc/include/uapi/asm/eeh.h while
updating linux header files. The specific header file, introduced
by Linux upstream commit ed3e81f ("powerpc/eeh: Move PE state
constants around"), is used by EEH on sPAPR platform.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 scripts/update-linux-headers.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
index f0e830c..bcfecd3 100755
--- a/scripts/update-linux-headers.sh
+++ b/scripts/update-linux-headers.sh
@@ -89,6 +89,7 @@ for arch in $ARCHLIST; do
         cp "$tmpdir/include/asm/hyperv.h" "$output/linux-headers/asm-x86"
     fi
     if [ $arch = powerpc ]; then
+        cp "$tmpdir/include/asm/eeh.h" "$output/linux-headers/asm-powerpc/"
         cp "$tmpdir/include/asm/epapr_hcalls.h" "$output/linux-headers/asm-powerpc/"
     fi
 
-- 
2.1.0

[Qemu-devel] [PATCH v5 2/4] linux-headers: Add eeh.h

[Gavin Shan]

The header file was introduced by following Linux upstream commits:

    commit ed3e81f ("powerpc/eeh: Move PE state constants around")
    commit ec33d36 ("powerpc/eeh: Introduce eeh_pe_inject_err()")

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
I didn't include all changes from scripts/update-linux-headers.sh
as too many unrelated changes introduced by the script.
---
 linux-headers/asm-powerpc/eeh.h | 56 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 linux-headers/asm-powerpc/eeh.h

diff --git a/linux-headers/asm-powerpc/eeh.h b/linux-headers/asm-powerpc/eeh.h
new file mode 100644
index 0000000..291b7d1
--- /dev/null
+++ b/linux-headers/asm-powerpc/eeh.h
@@ -0,0 +1,56 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ * Copyright IBM Corp. 2015
+ *
+ * Authors: Gavin Shan <gwshan@linux.vnet.ibm.com>
+ */
+
+#ifndef _ASM_POWERPC_EEH_H
+#define _ASM_POWERPC_EEH_H
+
+/* PE states */
+#define EEH_PE_STATE_NORMAL		0	/* Normal state		*/
+#define EEH_PE_STATE_RESET		1	/* PE reset asserted	*/
+#define EEH_PE_STATE_STOPPED_IO_DMA	2	/* Frozen PE		*/
+#define EEH_PE_STATE_STOPPED_DMA	4	/* Stopped DMA only	*/
+#define EEH_PE_STATE_UNAVAIL		5	/* Unavailable		*/
+
+/* EEH error types and functions */
+#define EEH_ERR_TYPE_32			0       /* 32-bits error	*/
+#define EEH_ERR_TYPE_64			1       /* 64-bits error	*/
+#define EEH_ERR_FUNC_MIN		0
+#define EEH_ERR_FUNC_LD_MEM_ADDR	0	/* Memory load	*/
+#define EEH_ERR_FUNC_LD_MEM_DATA	1
+#define EEH_ERR_FUNC_LD_IO_ADDR		2	/* IO load	*/
+#define EEH_ERR_FUNC_LD_IO_DATA		3
+#define EEH_ERR_FUNC_LD_CFG_ADDR	4	/* Config load	*/
+#define EEH_ERR_FUNC_LD_CFG_DATA	5
+#define EEH_ERR_FUNC_ST_MEM_ADDR	6	/* Memory store	*/
+#define EEH_ERR_FUNC_ST_MEM_DATA	7
+#define EEH_ERR_FUNC_ST_IO_ADDR		8	/* IO store	*/
+#define EEH_ERR_FUNC_ST_IO_DATA		9
+#define EEH_ERR_FUNC_ST_CFG_ADDR	10	/* Config store	*/
+#define EEH_ERR_FUNC_ST_CFG_DATA	11
+#define EEH_ERR_FUNC_DMA_RD_ADDR	12	/* DMA read	*/
+#define EEH_ERR_FUNC_DMA_RD_DATA	13
+#define EEH_ERR_FUNC_DMA_RD_MASTER	14
+#define EEH_ERR_FUNC_DMA_RD_TARGET	15
+#define EEH_ERR_FUNC_DMA_WR_ADDR	16	/* DMA write	*/
+#define EEH_ERR_FUNC_DMA_WR_DATA	17
+#define EEH_ERR_FUNC_DMA_WR_MASTER	18
+#define EEH_ERR_FUNC_DMA_WR_TARGET	19
+#define EEH_ERR_FUNC_MAX		19
+
+#endif /* _ASM_POWERPC_EEH_H */
-- 
2.1.0

[Qemu-devel] [PATCH v5 3/4] sPAPR: Support RTAS call ibm, {open, close}-errinjct

[Gavin Shan]

The patch supports RTAS calls "ibm,{open,close}-errinjct" to
manupliate the token, which is passed to RTAS call "ibm,errinjct"
to indicate the valid context for error injection. Each VM is
permitted to have only one token at once and we simply have one
random number for that.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c         |  6 ++++-
 hw/ppc/spapr_rtas.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h | 10 +++++++-
 3 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 06d000d..591a1a7 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1191,7 +1191,7 @@ static bool version_before_3(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_spapr = {
     .name = "spapr",
-    .version_id = 3,
+    .version_id = 4,
     .minimum_version_id = 1,
     .post_load = spapr_post_load,
     .fields = (VMStateField[]) {
@@ -1202,6 +1202,10 @@ static const VMStateDescription vmstate_spapr = {
         VMSTATE_UINT64_TEST(rtc_offset, sPAPRMachineState, version_before_3),
 
         VMSTATE_PPC_TIMEBASE_V(tb, sPAPRMachineState, 2),
+
+        /* Error injection token */
+        VMSTATE_UINT32_V(errinjct_token, sPAPRMachineState, 4),
+
         VMSTATE_END_OF_LIST()
     },
 };
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index e99e25f..8405056 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -604,6 +604,68 @@ out:
     rtas_st(rets, 0, rc);
 }
 
+static void rtas_ibm_open_errinjct(PowerPCCPU *cpu,
+                                   sPAPRMachineState *spapr,
+                                   uint32_t token, uint32_t nargs,
+                                   target_ulong args, uint32_t nret,
+                                   target_ulong rets)
+{
+    int32_t ret;
+
+    /* Sanity check on number of arguments */
+    if ((nargs != 0) || (nret != 2)) {
+        ret = RTAS_OUT_PARAM_ERROR;
+        goto out;
+    }
+
+    /* Check if we already had token */
+    if (spapr->is_errinjct_opened) {
+        ret = RTAS_OUT_TOKEN_OPENED;
+        goto out;
+    }
+
+    /* Grab the token */
+    spapr->is_errinjct_opened = true;
+    rtas_st(rets, 0, ++spapr->errinjct_token);
+    ret = RTAS_OUT_SUCCESS;
+out:
+    rtas_st(rets, 1, ret);
+}
+
+static void rtas_ibm_close_errinjct(PowerPCCPU *cpu,
+                                    sPAPRMachineState *spapr,
+                                    uint32_t token, uint32_t nargs,
+                                    target_ulong args, uint32_t nret,
+                                    target_ulong rets)
+{
+    uint32_t open_token;
+    int32_t ret;
+
+    /* Sanity check on number of arguments */
+    if ((nargs != 1) || (nret != 1)) {
+        ret = RTAS_OUT_PARAM_ERROR;
+        goto out;
+    }
+
+    /* Check if we had opened token */
+    if (!spapr->is_errinjct_opened) {
+        ret = RTAS_OUT_CLOSE_ERROR;
+        goto out;
+    }
+
+    /* Match with the passed token */
+    open_token = rtas_ld(args, 0);
+    if (spapr->errinjct_token != open_token) {
+        ret = RTAS_OUT_CLOSE_ERROR;
+        goto out;
+    }
+
+    spapr->is_errinjct_opened = false;
+    ret = RTAS_OUT_SUCCESS;
+out:
+    rtas_st(rets, 0, ret);
+}
+
 static struct rtas_call {
     const char *name;
     spapr_rtas_fn fn;
@@ -754,6 +816,10 @@ static void core_rtas_register_types(void)
                         rtas_get_sensor_state);
     spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR, "ibm,configure-connector",
                         rtas_ibm_configure_connector);
+    spapr_rtas_register(RTAS_IBM_OPEN_ERRINJCT, "ibm,open-errinjct",
+                        rtas_ibm_open_errinjct);
+    spapr_rtas_register(RTAS_IBM_CLOSE_ERRINJCT, "ibm,close-errinjct",
+                        rtas_ibm_close_errinjct);
 }
 
 type_init(core_rtas_register_types)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index d87c6d4..a3ec01d 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -73,6 +73,10 @@ struct sPAPRMachineState {
     int htab_fd;
     bool htab_fd_stale;
 
+    /* Error injection token */
+    bool is_errinjct_opened;
+    uint32_t errinjct_token;
+
     /* RTAS state */
     QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
 
@@ -412,6 +416,8 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_OUT_BUSY               -2
 #define RTAS_OUT_PARAM_ERROR        -3
 #define RTAS_OUT_NOT_SUPPORTED      -3
+#define RTAS_OUT_TOKEN_OPENED       -4
+#define RTAS_OUT_CLOSE_ERROR        -4
 #define RTAS_OUT_NOT_AUTHORIZED     -9002
 
 /* RTAS tokens */
@@ -455,8 +461,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_IBM_SET_SLOT_RESET                 (RTAS_TOKEN_BASE + 0x23)
 #define RTAS_IBM_CONFIGURE_PE                   (RTAS_TOKEN_BASE + 0x24)
 #define RTAS_IBM_SLOT_ERROR_DETAIL              (RTAS_TOKEN_BASE + 0x25)
+#define RTAS_IBM_OPEN_ERRINJCT                  (RTAS_TOKEN_BASE + 0x26)
+#define RTAS_IBM_CLOSE_ERRINJCT                 (RTAS_TOKEN_BASE + 0x27)
 
-#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x26)
+#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x28)
 
 /* RTAS ibm,get-system-parameter token values */
 #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
-- 
2.1.0

[Qemu-devel] [PATCH v5 4/4] sPAPR: Support RTAS call ibm,errinjct

[Gavin Shan]

The patch supports RTAS call "ibm,errinjct" to allow injecting
EEH errors to VFIO PCI devices. The implementation is similiar
to EEH support for VFIO PCI devices: The RTAS request is captured
by QEMU and routed to sPAPRPHBClass::eeh_inject_error() where the
request is translated to VFIO container IOCTL command to be handled
by the host.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c          | 36 +++++++++++++++++++++
 hw/ppc/spapr_pci_vfio.c     | 56 +++++++++++++++++++++++++++++++++
 hw/ppc/spapr_rtas.c         | 77 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/pci-host/spapr.h |  2 ++
 include/hw/ppc/spapr.h      |  9 +++++-
 5 files changed, 179 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 9d41060..f6223ce 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -682,6 +682,42 @@ param_error_exit:
     rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
 }
 
+int spapr_rtas_errinjct_ioa(sPAPRMachineState *spapr,
+                            target_ulong param_buf,
+                            bool is_64bits)
+{
+    sPAPRPHBState *sphb;
+    sPAPRPHBClass *spc;
+    uint64_t buid, addr, mask;
+    uint32_t func;
+
+    if (is_64bits) {
+        addr = ((uint64_t)rtas_ld(param_buf, 0) << 32) | rtas_ld(param_buf, 1);
+        mask = ((uint64_t)rtas_ld(param_buf, 2) << 32) | rtas_ld(param_buf, 3);
+        buid = ((uint64_t)rtas_ld(param_buf, 5) << 32) | rtas_ld(param_buf, 6);
+        func = rtas_ld(param_buf, 7);
+    } else {
+        addr = rtas_ld(param_buf, 0);
+        mask = rtas_ld(param_buf, 1);
+        buid = ((uint64_t)rtas_ld(param_buf, 3) << 32) | rtas_ld(param_buf, 4);
+        func = rtas_ld(param_buf, 5);
+    }
+
+    /* Find PHB */
+    sphb = spapr_pci_find_phb(spapr, buid);
+    if (!sphb) {
+        return RTAS_OUT_PARAM_ERROR;
+    }
+
+    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+    if (!spc->eeh_inject_error) {
+        return RTAS_OUT_PARAM_ERROR;
+    }
+
+    /* Handle the request */
+    return spc->eeh_inject_error(sphb, func, addr, mask, is_64bits);
+}
+
 static int pci_spapr_swizzle(int slot, int pin)
 {
     return (slot + pin) % PCI_NUM_PINS;
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index cca45ed..a3674ee 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -17,6 +17,8 @@
  *  along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <asm/eeh.h>
+
 #include "hw/ppc/spapr.h"
 #include "hw/pci-host/spapr.h"
 #include "hw/pci/msix.h"
@@ -250,6 +252,59 @@ static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
     return RTAS_OUT_SUCCESS;
 }
 
+static int spapr_phb_vfio_eeh_inject_error(sPAPRPHBState *sphb,
+                                           uint32_t func, uint64_t addr,
+                                           uint64_t mask, bool is_64bits)
+{
+    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
+    struct vfio_eeh_pe_op op = {
+        .op = VFIO_EEH_PE_INJECT_ERR,
+        .argsz = sizeof(op)
+    };
+    int ret = RTAS_OUT_SUCCESS;
+
+    op.err.type = is_64bits ? EEH_ERR_TYPE_64 : EEH_ERR_TYPE_32;
+    op.err.addr = addr;
+    op.err.mask = mask;
+
+    switch (func) {
+    case EEH_ERR_FUNC_LD_MEM_ADDR:
+    case EEH_ERR_FUNC_LD_MEM_DATA:
+    case EEH_ERR_FUNC_LD_IO_ADDR:
+    case EEH_ERR_FUNC_LD_IO_DATA:
+    case EEH_ERR_FUNC_LD_CFG_ADDR:
+    case EEH_ERR_FUNC_LD_CFG_DATA:
+    case EEH_ERR_FUNC_ST_MEM_ADDR:
+    case EEH_ERR_FUNC_ST_MEM_DATA:
+    case EEH_ERR_FUNC_ST_IO_ADDR:
+    case EEH_ERR_FUNC_ST_IO_DATA:
+    case EEH_ERR_FUNC_ST_CFG_ADDR:
+    case EEH_ERR_FUNC_ST_CFG_DATA:
+    case EEH_ERR_FUNC_DMA_RD_ADDR:
+    case EEH_ERR_FUNC_DMA_RD_DATA:
+    case EEH_ERR_FUNC_DMA_RD_MASTER:
+    case EEH_ERR_FUNC_DMA_RD_TARGET:
+    case EEH_ERR_FUNC_DMA_WR_ADDR:
+    case EEH_ERR_FUNC_DMA_WR_DATA:
+    case EEH_ERR_FUNC_DMA_WR_MASTER:
+        op.err.func = func;
+        break;
+    default:
+        ret = RTAS_OUT_PARAM_ERROR;
+        goto out;
+    }
+
+    if (vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
+                             VFIO_EEH_PE_OP, &op) < 0) {
+        ret = RTAS_OUT_HW_ERROR;
+        goto out;
+    }
+
+    ret = RTAS_OUT_SUCCESS;
+out:
+    return ret;
+}
+
 static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -262,6 +317,7 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
     spc->eeh_get_state = spapr_phb_vfio_eeh_get_state;
     spc->eeh_reset = spapr_phb_vfio_eeh_reset;
     spc->eeh_configure = spapr_phb_vfio_eeh_configure;
+    spc->eeh_inject_error = spapr_phb_vfio_eeh_inject_error;
 }
 
 static const TypeInfo spapr_phb_vfio_info = {
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 8405056..5645f43 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -632,6 +632,54 @@ out:
     rtas_st(rets, 1, ret);
 }
 
+static void rtas_ibm_errinjct(PowerPCCPU *cpu,
+                              sPAPRMachineState *spapr,
+                              uint32_t token, uint32_t nargs,
+                              target_ulong args, uint32_t nret,
+                              target_ulong rets)
+{
+    target_ulong param_buf;
+    uint32_t type, open_token;
+    int32_t ret;
+
+    /* Sanity check on number of arguments */
+    if ((nargs != 3) || (nret != 1)) {
+        ret = RTAS_OUT_PARAM_ERROR;
+        goto out;
+    }
+
+    /* Check if we have opened token */
+    open_token = rtas_ld(args, 1);
+    if (!spapr->is_errinjct_opened ||
+        spapr->errinjct_token != open_token) {
+        ret = RTAS_OUT_CLOSE_ERROR;
+        goto out;
+    }
+
+    /* The parameter buffer should be 1KB aligned */
+    param_buf = rtas_ld(args, 2);
+    if (param_buf & 0x3ff) {
+        ret = RTAS_OUT_PARAM_ERROR;
+        goto out;
+    }
+
+    /* Check the error type */
+    type = rtas_ld(args, 0);
+    switch (type) {
+    case RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR:
+        ret = spapr_rtas_errinjct_ioa(spapr, param_buf, false);
+        break;
+    case RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64:
+        ret = spapr_rtas_errinjct_ioa(spapr, param_buf, true);
+        break;
+    default:
+        ret = RTAS_OUT_PARAM_ERROR;
+    }
+
+out:
+    rtas_st(rets, 0, ret);
+}
+
 static void rtas_ibm_close_errinjct(PowerPCCPU *cpu,
                                     sPAPRMachineState *spapr,
                                     uint32_t token, uint32_t nargs,
@@ -723,6 +771,33 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
     int i;
     uint32_t lrdr_capacity[5];
     MachineState *machine = MACHINE(qdev_get_machine());
+    char errinjct_tokens[1024];
+    int fdt_offset, offset;
+    const int tokens[] = {
+        RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR,
+        RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64
+    };
+    const char *token_strings[] = {
+        "ioa-bus-error",
+        "ioa-bus-error-64"
+    };
+
+    /* ibm,errinjct-tokens */
+    offset = 0;
+    for (i = 0; i < ARRAY_SIZE(tokens); i++) {
+        offset += sprintf(errinjct_tokens + offset, "%s", token_strings[i]);
+        errinjct_tokens[offset++] = '\0';
+        *(int *)(&errinjct_tokens[offset]) = tokens[i];
+        offset += sizeof(int);
+    }
+
+    fdt_offset = fdt_path_offset(fdt, "/rtas");
+    ret = fdt_setprop(fdt, fdt_offset, "ibm,errinjct-tokens",
+                      errinjct_tokens, offset);
+    if (ret < 0) {
+        fprintf(stderr, "Couldn't add ibm,errinjct-tokens\n");
+        return ret;
+    }
 
     ret = fdt_add_mem_rsv(fdt, rtas_addr, rtas_size);
     if (ret < 0) {
@@ -818,6 +893,8 @@ static void core_rtas_register_types(void)
                         rtas_ibm_configure_connector);
     spapr_rtas_register(RTAS_IBM_OPEN_ERRINJCT, "ibm,open-errinjct",
                         rtas_ibm_open_errinjct);
+    spapr_rtas_register(RTAS_IBM_ERRINJCT, "ibm,errinjct",
+                        rtas_ibm_errinjct);
     spapr_rtas_register(RTAS_IBM_CLOSE_ERRINJCT, "ibm,close-errinjct",
                         rtas_ibm_close_errinjct);
 }
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 5322b56..069300d 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -53,6 +53,8 @@ struct sPAPRPHBClass {
     int (*eeh_get_state)(sPAPRPHBState *sphb, int *state);
     int (*eeh_reset)(sPAPRPHBState *sphb, int option);
     int (*eeh_configure)(sPAPRPHBState *sphb);
+    int (*eeh_inject_error)(sPAPRPHBState *sphb, uint32_t func,
+                            uint64_t addr, uint64_t mask, bool is_64bits);
 };
 
 typedef struct spapr_pci_msi {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index a3ec01d..7a856a8 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -409,6 +409,10 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_SLOT_TEMP_ERR_LOG           1
 #define RTAS_SLOT_PERM_ERR_LOG           2
 
+/* ibm,errinjct */
+#define RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR        7
+#define RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64      8
+
 /* RTAS return codes */
 #define RTAS_OUT_SUCCESS            0
 #define RTAS_OUT_NO_ERRORS_FOUND    1
@@ -463,8 +467,9 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_IBM_SLOT_ERROR_DETAIL              (RTAS_TOKEN_BASE + 0x25)
 #define RTAS_IBM_OPEN_ERRINJCT                  (RTAS_TOKEN_BASE + 0x26)
 #define RTAS_IBM_CLOSE_ERRINJCT                 (RTAS_TOKEN_BASE + 0x27)
+#define RTAS_IBM_ERRINJCT                       (RTAS_TOKEN_BASE + 0x28)
 
-#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x28)
+#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x29)
 
 /* RTAS ibm,get-system-parameter token values */
 #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
@@ -596,6 +601,8 @@ int spapr_dma_dt(void *fdt, int node_off, const char *propname,
 int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
                       sPAPRTCETable *tcet);
 void spapr_pci_switch_vga(bool big_endian);
+int spapr_rtas_errinjct_ioa(sPAPRMachineState *spapr,
+                            target_ulong param_buf, bool is_64bits);
 void spapr_hotplug_req_add_by_index(sPAPRDRConnector *drc);
 void spapr_hotplug_req_remove_by_index(sPAPRDRConnector *drc);
 void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
-- 
2.1.0

Re: [Qemu-devel] [PATCH v5 2/4] linux-headers: Add eeh.h

[Peter Maydell]

On 18 August 2015 at 02:47, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
> The header file was introduced by following Linux upstream commits:
>
>     commit ed3e81f ("powerpc/eeh: Move PE state constants around")
>     commit ec33d36 ("powerpc/eeh: Introduce eeh_pe_inject_err()")
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
> I didn't include all changes from scripts/update-linux-headers.sh
> as too many unrelated changes introduced by the script.

Please just have a commit which does a "sync linux headers
from kernel version $WHATEVER". The whole point of the script
is to keep our headers completely synchronized with the kernel.

(The only exception to this is RFC patchsets where the kernel
header changes haven't yet hit mainline.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 3/4] sPAPR: Support RTAS call ibm, {open, close}-errinjct

[Thomas Huth]

On 17/08/15 18:47, Gavin Shan wrote:
> The patch supports RTAS calls "ibm,{open,close}-errinjct" to
> manupliate the token, which is passed to RTAS call "ibm,errinjct"
> to indicate the valid context for error injection. Each VM is
> permitted to have only one token at once and we simply have one
> random number for that.

Looking at the code, you're using a sequence number now instead of a
random number?

> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c         |  6 ++++-
>  hw/ppc/spapr_rtas.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h | 10 +++++++-
>  3 files changed, 80 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 06d000d..591a1a7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1191,7 +1191,7 @@ static bool version_before_3(void *opaque, int version_id)
>  
>  static const VMStateDescription vmstate_spapr = {
>      .name = "spapr",
> -    .version_id = 3,
> +    .version_id = 4,
>      .minimum_version_id = 1,
>      .post_load = spapr_post_load,
>      .fields = (VMStateField[]) {
> @@ -1202,6 +1202,10 @@ static const VMStateDescription vmstate_spapr = {
>          VMSTATE_UINT64_TEST(rtc_offset, sPAPRMachineState, version_before_3),
>  
>          VMSTATE_PPC_TIMEBASE_V(tb, sPAPRMachineState, 2),
> +
> +        /* Error injection token */
> +        VMSTATE_UINT32_V(errinjct_token, sPAPRMachineState, 4),

Ok, so you're only saving the errinjct_token here, but not
is_errinjct_opened?

>          VMSTATE_END_OF_LIST()
>      },
>  };
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index e99e25f..8405056 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -604,6 +604,68 @@ out:
>      rtas_st(rets, 0, rc);
>  }
>  
> +static void rtas_ibm_open_errinjct(PowerPCCPU *cpu,
> +                                   sPAPRMachineState *spapr,
> +                                   uint32_t token, uint32_t nargs,
> +                                   target_ulong args, uint32_t nret,
> +                                   target_ulong rets)
> +{
> +    int32_t ret;
> +
> +    /* Sanity check on number of arguments */
> +    if ((nargs != 0) || (nret != 2)) {

Uh, did Alexey infect you with paranthesitis?

> +        ret = RTAS_OUT_PARAM_ERROR;
> +        goto out;
> +    }
> +
> +    /* Check if we already had token */
> +    if (spapr->is_errinjct_opened) {
> +        ret = RTAS_OUT_TOKEN_OPENED;
> +        goto out;
> +    }
> +
> +    /* Grab the token */
> +    spapr->is_errinjct_opened = true;
> +    rtas_st(rets, 0, ++spapr->errinjct_token);
> +    ret = RTAS_OUT_SUCCESS;
> +out:
> +    rtas_st(rets, 1, ret);
> +}
> +
> +static void rtas_ibm_close_errinjct(PowerPCCPU *cpu,
> +                                    sPAPRMachineState *spapr,
> +                                    uint32_t token, uint32_t nargs,
> +                                    target_ulong args, uint32_t nret,
> +                                    target_ulong rets)
> +{
> +    uint32_t open_token;
> +    int32_t ret;
> +
> +    /* Sanity check on number of arguments */
> +    if ((nargs != 1) || (nret != 1)) {
> +        ret = RTAS_OUT_PARAM_ERROR;
> +        goto out;
> +    }
> +
> +    /* Check if we had opened token */
> +    if (!spapr->is_errinjct_opened) {
> +        ret = RTAS_OUT_CLOSE_ERROR;
> +        goto out;
> +    }

... and here you check that another status variable "is_errinjct_opened"
which is not saved in the VMStateDescription and thus this information
will get lost during migration, I think. That looks like a bug to me.

Can you do your code completely without "is_errinjct_opened"? I.e. by
using errinjct_token == 0 for signalling that no injection progress is
currently taking place?

> +    /* Match with the passed token */
> +    open_token = rtas_ld(args, 0);
> +    if (spapr->errinjct_token != open_token) {
> +        ret = RTAS_OUT_CLOSE_ERROR;
> +        goto out;
> +    }
> +
> +    spapr->is_errinjct_opened = false;
> +    ret = RTAS_OUT_SUCCESS;
> +out:
> +    rtas_st(rets, 0, ret);
> +}

 Thomas

Re: [Qemu-devel] [PATCH v5 4/4] sPAPR: Support RTAS call ibm, errinjct

[Thomas Huth]

On 17/08/15 18:47, Gavin Shan wrote:
> The patch supports RTAS call "ibm,errinjct" to allow injecting
> EEH errors to VFIO PCI devices. The implementation is similiar
> to EEH support for VFIO PCI devices: The RTAS request is captured
> by QEMU and routed to sPAPRPHBClass::eeh_inject_error() where the
> request is translated to VFIO container IOCTL command to be handled
> by the host.
> 
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_pci.c          | 36 +++++++++++++++++++++
>  hw/ppc/spapr_pci_vfio.c     | 56 +++++++++++++++++++++++++++++++++
>  hw/ppc/spapr_rtas.c         | 77 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci-host/spapr.h |  2 ++
>  include/hw/ppc/spapr.h      |  9 +++++-
>  5 files changed, 179 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 9d41060..f6223ce 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -682,6 +682,42 @@ param_error_exit:
>      rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>  }
>  
> +int spapr_rtas_errinjct_ioa(sPAPRMachineState *spapr,
> +                            target_ulong param_buf,
> +                            bool is_64bits)
> +{
> +    sPAPRPHBState *sphb;
> +    sPAPRPHBClass *spc;
> +    uint64_t buid, addr, mask;
> +    uint32_t func;
> +
> +    if (is_64bits) {
> +        addr = ((uint64_t)rtas_ld(param_buf, 0) << 32) | rtas_ld(param_buf, 1);
> +        mask = ((uint64_t)rtas_ld(param_buf, 2) << 32) | rtas_ld(param_buf, 3);
> +        buid = ((uint64_t)rtas_ld(param_buf, 5) << 32) | rtas_ld(param_buf, 6);

You might want to consider to introduce a helper function (e.g
"ras_ld64"?) that loads the two 32 bit values and combines them.

> +        func = rtas_ld(param_buf, 7);
> +    } else {
> +        addr = rtas_ld(param_buf, 0);
> +        mask = rtas_ld(param_buf, 1);
> +        buid = ((uint64_t)rtas_ld(param_buf, 3) << 32) | rtas_ld(param_buf, 4);
> +        func = rtas_ld(param_buf, 5);
> +    }
> +
> +    /* Find PHB */
> +    sphb = spapr_pci_find_phb(spapr, buid);
> +    if (!sphb) {
> +        return RTAS_OUT_PARAM_ERROR;
> +    }
> +
> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> +    if (!spc->eeh_inject_error) {
> +        return RTAS_OUT_PARAM_ERROR;
> +    }
> +
> +    /* Handle the request */
> +    return spc->eeh_inject_error(sphb, func, addr, mask, is_64bits);
> +}
> +
>  static int pci_spapr_swizzle(int slot, int pin)
>  {
>      return (slot + pin) % PCI_NUM_PINS;
> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> index cca45ed..a3674ee 100644
> --- a/hw/ppc/spapr_pci_vfio.c
> +++ b/hw/ppc/spapr_pci_vfio.c
> @@ -17,6 +17,8 @@
>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <asm/eeh.h>

This does not work when building on non-powerpc systems. I think you
have to use something like this instead:

#include "asm-powerpc/eeh.h"

>  #include "hw/ppc/spapr.h"
>  #include "hw/pci-host/spapr.h"
>  #include "hw/pci/msix.h"
> @@ -250,6 +252,59 @@ static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
>      return RTAS_OUT_SUCCESS;
>  }
>  
> +static int spapr_phb_vfio_eeh_inject_error(sPAPRPHBState *sphb,
> +                                           uint32_t func, uint64_t addr,
> +                                           uint64_t mask, bool is_64bits)
> +{
> +    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> +    struct vfio_eeh_pe_op op = {
> +        .op = VFIO_EEH_PE_INJECT_ERR,
> +        .argsz = sizeof(op)
> +    };
> +    int ret = RTAS_OUT_SUCCESS;
> +
> +    op.err.type = is_64bits ? EEH_ERR_TYPE_64 : EEH_ERR_TYPE_32;
> +    op.err.addr = addr;
> +    op.err.mask = mask;
> +
> +    switch (func) {
> +    case EEH_ERR_FUNC_LD_MEM_ADDR:
> +    case EEH_ERR_FUNC_LD_MEM_DATA:
> +    case EEH_ERR_FUNC_LD_IO_ADDR:
> +    case EEH_ERR_FUNC_LD_IO_DATA:
> +    case EEH_ERR_FUNC_LD_CFG_ADDR:
> +    case EEH_ERR_FUNC_LD_CFG_DATA:
> +    case EEH_ERR_FUNC_ST_MEM_ADDR:
> +    case EEH_ERR_FUNC_ST_MEM_DATA:
> +    case EEH_ERR_FUNC_ST_IO_ADDR:
> +    case EEH_ERR_FUNC_ST_IO_DATA:
> +    case EEH_ERR_FUNC_ST_CFG_ADDR:
> +    case EEH_ERR_FUNC_ST_CFG_DATA:
> +    case EEH_ERR_FUNC_DMA_RD_ADDR:
> +    case EEH_ERR_FUNC_DMA_RD_DATA:
> +    case EEH_ERR_FUNC_DMA_RD_MASTER:
> +    case EEH_ERR_FUNC_DMA_RD_TARGET:
> +    case EEH_ERR_FUNC_DMA_WR_ADDR:
> +    case EEH_ERR_FUNC_DMA_WR_DATA:
> +    case EEH_ERR_FUNC_DMA_WR_MASTER:

EEH_ERR_FUNC_DMA_WR_TARGET is missing in the list ... I assume this is
on purpose?

> +        op.err.func = func;
> +        break;
> +    default:
> +        ret = RTAS_OUT_PARAM_ERROR;
> +        goto out;
> +    }
> +
> +    if (vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
> +                             VFIO_EEH_PE_OP, &op) < 0) {
> +        ret = RTAS_OUT_HW_ERROR;
> +        goto out;
> +    }
> +
> +    ret = RTAS_OUT_SUCCESS;
> +out:
> +    return ret;
> +}
> +
>  static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -262,6 +317,7 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
>      spc->eeh_get_state = spapr_phb_vfio_eeh_get_state;
>      spc->eeh_reset = spapr_phb_vfio_eeh_reset;
>      spc->eeh_configure = spapr_phb_vfio_eeh_configure;
> +    spc->eeh_inject_error = spapr_phb_vfio_eeh_inject_error;
>  }
>  
>  static const TypeInfo spapr_phb_vfio_info = {
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 8405056..5645f43 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -632,6 +632,54 @@ out:
>      rtas_st(rets, 1, ret);
>  }
>  
> +static void rtas_ibm_errinjct(PowerPCCPU *cpu,
> +                              sPAPRMachineState *spapr,
> +                              uint32_t token, uint32_t nargs,
> +                              target_ulong args, uint32_t nret,
> +                              target_ulong rets)
> +{
> +    target_ulong param_buf;
> +    uint32_t type, open_token;
> +    int32_t ret;
> +
> +    /* Sanity check on number of arguments */
> +    if ((nargs != 3) || (nret != 1)) {

Less paranthesis, please?

> +        ret = RTAS_OUT_PARAM_ERROR;
> +        goto out;
> +    }
> +
> +    /* Check if we have opened token */
> +    open_token = rtas_ld(args, 1);
> +    if (!spapr->is_errinjct_opened ||
> +        spapr->errinjct_token != open_token) {
> +        ret = RTAS_OUT_CLOSE_ERROR;
> +        goto out;
> +    }
> +
> +    /* The parameter buffer should be 1KB aligned */
> +    param_buf = rtas_ld(args, 2);
> +    if (param_buf & 0x3ff) {
> +        ret = RTAS_OUT_PARAM_ERROR;
> +        goto out;
> +    }
> +
> +    /* Check the error type */
> +    type = rtas_ld(args, 0);
> +    switch (type) {
> +    case RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR:
> +        ret = spapr_rtas_errinjct_ioa(spapr, param_buf, false);
> +        break;
> +    case RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64:
> +        ret = spapr_rtas_errinjct_ioa(spapr, param_buf, true);
> +        break;
> +    default:
> +        ret = RTAS_OUT_PARAM_ERROR;
> +    }
> +
> +out:
> +    rtas_st(rets, 0, ret);
> +}
> +
>  static void rtas_ibm_close_errinjct(PowerPCCPU *cpu,
>                                      sPAPRMachineState *spapr,
>                                      uint32_t token, uint32_t nargs,
> @@ -723,6 +771,33 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
>      int i;
>      uint32_t lrdr_capacity[5];
>      MachineState *machine = MACHINE(qdev_get_machine());
> +    char errinjct_tokens[1024];
> +    int fdt_offset, offset;
> +    const int tokens[] = {
> +        RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR,
> +        RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64
> +    };
> +    const char *token_strings[] = {
> +        "ioa-bus-error",
> +        "ioa-bus-error-64"
> +    };
> +
> +    /* ibm,errinjct-tokens */
> +    offset = 0;
> +    for (i = 0; i < ARRAY_SIZE(tokens); i++) {
> +        offset += sprintf(errinjct_tokens + offset, "%s", token_strings[i]);
> +        errinjct_tokens[offset++] = '\0';
> +        *(int *)(&errinjct_tokens[offset]) = tokens[i];

You can also get rid of some paranthesis here. Also I am not sure, but I
think you have to take care of the endianess here? ==> Use stl_be_p instead?

> +        offset += sizeof(int);
> +    }
> +
> +    fdt_offset = fdt_path_offset(fdt, "/rtas");
> +    ret = fdt_setprop(fdt, fdt_offset, "ibm,errinjct-tokens",
> +                      errinjct_tokens, offset);
> +    if (ret < 0) {
> +        fprintf(stderr, "Couldn't add ibm,errinjct-tokens\n");
> +        return ret;
> +    }

 Thomas

Re: [Qemu-devel] [PATCH v5 2/4] linux-headers: Add eeh.h

[David Gibson]

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

On Tue, Aug 18, 2015 at 01:26:39PM +0100, Peter Maydell wrote:
> On 18 August 2015 at 02:47, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
> > The header file was introduced by following Linux upstream commits:
> >
> >     commit ed3e81f ("powerpc/eeh: Move PE state constants around")
> >     commit ec33d36 ("powerpc/eeh: Introduce eeh_pe_inject_err()")
> >
> > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> > ---
> > I didn't include all changes from scripts/update-linux-headers.sh
> > as too many unrelated changes introduced by the script.
> 
> Please just have a commit which does a "sync linux headers
> from kernel version $WHATEVER". The whole point of the script
> is to keep our headers completely synchronized with the kernel.
> 
> (The only exception to this is RFC patchsets where the kernel
> header changes haven't yet hit mainline.)

Seconded.  Please include the SHA ID of the upstream kernel you
updated from in your commit message, too.

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

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

Re: [Qemu-devel] [PATCH v5 2/4] linux-headers: Add eeh.h

[Gavin Shan]

On Tue, Aug 18, 2015 at 03:53:25PM -0700, David Gibson wrote:
>On Tue, Aug 18, 2015 at 01:26:39PM +0100, Peter Maydell wrote:
>> On 18 August 2015 at 02:47, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>> > The header file was introduced by following Linux upstream commits:
>> >
>> >     commit ed3e81f ("powerpc/eeh: Move PE state constants around")
>> >     commit ec33d36 ("powerpc/eeh: Introduce eeh_pe_inject_err()")
>> >
>> > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> > ---
>> > I didn't include all changes from scripts/update-linux-headers.sh
>> > as too many unrelated changes introduced by the script.
>> 
>> Please just have a commit which does a "sync linux headers
>> from kernel version $WHATEVER". The whole point of the script
>> is to keep our headers completely synchronized with the kernel.
>> 
>> (The only exception to this is RFC patchsets where the kernel
>> header changes haven't yet hit mainline.)
>
>Seconded.  Please include the SHA ID of the upstream kernel you
>updated from in your commit message, too.
>

Ok. I'll do. Thanks!

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

Re: [Qemu-devel] [PATCH v5 2/4] linux-headers: Add eeh.h

[Peter Maydell]

On 18 August 2015 at 23:53, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Tue, Aug 18, 2015 at 01:26:39PM +0100, Peter Maydell wrote:
>> Please just have a commit which does a "sync linux headers
>> from kernel version $WHATEVER". The whole point of the script
>> is to keep our headers completely synchronized with the kernel.
>>
>> (The only exception to this is RFC patchsets where the kernel
>> header changes haven't yet hit mainline.)
>
> Seconded.  Please include the SHA ID of the upstream kernel you
> updated from in your commit message, too.

Maybe we should enhance the script to have an option for
"and create the git commit with appropriate commit message
after updating the headers" :-)

-- PMM

Re: [Qemu-devel] [PATCH v5 3/4] sPAPR: Support RTAS call ibm, {open, close}-errinjct

[Gavin Shan]

On Tue, Aug 18, 2015 at 10:32:13AM -0700, Thomas Huth wrote:
>On 17/08/15 18:47, Gavin Shan wrote:
>> The patch supports RTAS calls "ibm,{open,close}-errinjct" to
>> manupliate the token, which is passed to RTAS call "ibm,errinjct"
>> to indicate the valid context for error injection. Each VM is
>> permitted to have only one token at once and we simply have one
>> random number for that.
>
>Looking at the code, you're using a sequence number now instead of a
>random number?
>

Yes, it's what Alexey suggested.

>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/spapr.c         |  6 ++++-
>>  hw/ppc/spapr_rtas.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/spapr.h | 10 +++++++-
>>  3 files changed, 80 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 06d000d..591a1a7 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1191,7 +1191,7 @@ static bool version_before_3(void *opaque, int version_id)
>>  
>>  static const VMStateDescription vmstate_spapr = {
>>      .name = "spapr",
>> -    .version_id = 3,
>> +    .version_id = 4,
>>      .minimum_version_id = 1,
>>      .post_load = spapr_post_load,
>>      .fields = (VMStateField[]) {
>> @@ -1202,6 +1202,10 @@ static const VMStateDescription vmstate_spapr = {
>>          VMSTATE_UINT64_TEST(rtc_offset, sPAPRMachineState, version_before_3),
>>  
>>          VMSTATE_PPC_TIMEBASE_V(tb, sPAPRMachineState, 2),
>> +
>> +        /* Error injection token */
>> +        VMSTATE_UINT32_V(errinjct_token, sPAPRMachineState, 4),
>
>Ok, so you're only saving the errinjct_token here, but not
>is_errinjct_opened?
>

Yes, It's also something that Alexey suggested in last round of review.

>>          VMSTATE_END_OF_LIST()
>>      },
>>  };
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index e99e25f..8405056 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -604,6 +604,68 @@ out:
>>      rtas_st(rets, 0, rc);
>>  }
>>  
>> +static void rtas_ibm_open_errinjct(PowerPCCPU *cpu,
>> +                                   sPAPRMachineState *spapr,
>> +                                   uint32_t token, uint32_t nargs,
>> +                                   target_ulong args, uint32_t nret,
>> +                                   target_ulong rets)
>> +{
>> +    int32_t ret;
>> +
>> +    /* Sanity check on number of arguments */
>> +    if ((nargs != 0) || (nret != 2)) {
>
>Uh, did Alexey infect you with paranthesitis?
>

hehe~, nope. I'll drop those unnecessary paranthesitis :-)

>> +        ret = RTAS_OUT_PARAM_ERROR;
>> +        goto out;
>> +    }
>> +
>> +    /* Check if we already had token */
>> +    if (spapr->is_errinjct_opened) {
>> +        ret = RTAS_OUT_TOKEN_OPENED;
>> +        goto out;
>> +    }
>> +
>> +    /* Grab the token */
>> +    spapr->is_errinjct_opened = true;
>> +    rtas_st(rets, 0, ++spapr->errinjct_token);
>> +    ret = RTAS_OUT_SUCCESS;
>> +out:
>> +    rtas_st(rets, 1, ret);
>> +}
>> +
>> +static void rtas_ibm_close_errinjct(PowerPCCPU *cpu,
>> +                                    sPAPRMachineState *spapr,
>> +                                    uint32_t token, uint32_t nargs,
>> +                                    target_ulong args, uint32_t nret,
>> +                                    target_ulong rets)
>> +{
>> +    uint32_t open_token;
>> +    int32_t ret;
>> +
>> +    /* Sanity check on number of arguments */
>> +    if ((nargs != 1) || (nret != 1)) {
>> +        ret = RTAS_OUT_PARAM_ERROR;
>> +        goto out;
>> +    }
>> +
>> +    /* Check if we had opened token */
>> +    if (!spapr->is_errinjct_opened) {
>> +        ret = RTAS_OUT_CLOSE_ERROR;
>> +        goto out;
>> +    }
>
>... and here you check that another status variable "is_errinjct_opened"
>which is not saved in the VMStateDescription and thus this information
>will get lost during migration, I think. That looks like a bug to me.
>
>Can you do your code completely without "is_errinjct_opened"? I.e. by
>using errinjct_token == 0 for signalling that no injection progress is
>currently taking place?
>

It's fine to lose "is_errinjct_opened" after migration. The user needs
another attempt to do error injection after migration.

umm, In v1, I used the condition "errinjct_token == 0" to indicate there
is no injection in progress. After that, I received the suggestion to
change the code to have two variables: one boolean for error injection
token opening state and another one for error injection (sequential)
token. I don't see any problem with it. 

>> +    /* Match with the passed token */
>> +    open_token = rtas_ld(args, 0);
>> +    if (spapr->errinjct_token != open_token) {
>> +        ret = RTAS_OUT_CLOSE_ERROR;
>> +        goto out;
>> +    }
>> +
>> +    spapr->is_errinjct_opened = false;
>> +    ret = RTAS_OUT_SUCCESS;
>> +out:
>> +    rtas_st(rets, 0, ret);
>> +}

Thanks,
Gavin

Re: [Qemu-devel] [PATCH v5 2/4] linux-headers: Add eeh.h

[David Gibson]

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

On Wed, Aug 19, 2015 at 12:46:26AM +0100, Peter Maydell wrote:
> On 18 August 2015 at 23:53, David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Tue, Aug 18, 2015 at 01:26:39PM +0100, Peter Maydell wrote:
> >> Please just have a commit which does a "sync linux headers
> >> from kernel version $WHATEVER". The whole point of the script
> >> is to keep our headers completely synchronized with the kernel.
> >>
> >> (The only exception to this is RFC patchsets where the kernel
> >> header changes haven't yet hit mainline.)
> >
> > Seconded.  Please include the SHA ID of the upstream kernel you
> > updated from in your commit message, too.
> 
> Maybe we should enhance the script to have an option for
> "and create the git commit with appropriate commit message
> after updating the headers" :-)

That would be nice.

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

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

Re: [Qemu-devel] [PATCH v5 4/4] sPAPR: Support RTAS call ibm, errinjct

[Gavin Shan]

On Tue, Aug 18, 2015 at 11:04:59AM -0700, Thomas Huth wrote:
>On 17/08/15 18:47, Gavin Shan wrote:
>> The patch supports RTAS call "ibm,errinjct" to allow injecting
>> EEH errors to VFIO PCI devices. The implementation is similiar
>> to EEH support for VFIO PCI devices: The RTAS request is captured
>> by QEMU and routed to sPAPRPHBClass::eeh_inject_error() where the
>> request is translated to VFIO container IOCTL command to be handled
>> by the host.
>> 
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/spapr_pci.c          | 36 +++++++++++++++++++++
>>  hw/ppc/spapr_pci_vfio.c     | 56 +++++++++++++++++++++++++++++++++
>>  hw/ppc/spapr_rtas.c         | 77 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/pci-host/spapr.h |  2 ++
>>  include/hw/ppc/spapr.h      |  9 +++++-
>>  5 files changed, 179 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 9d41060..f6223ce 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -682,6 +682,42 @@ param_error_exit:
>>      rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>>  }
>>  
>> +int spapr_rtas_errinjct_ioa(sPAPRMachineState *spapr,
>> +                            target_ulong param_buf,
>> +                            bool is_64bits)
>> +{
>> +    sPAPRPHBState *sphb;
>> +    sPAPRPHBClass *spc;
>> +    uint64_t buid, addr, mask;
>> +    uint32_t func;
>> +
>> +    if (is_64bits) {
>> +        addr = ((uint64_t)rtas_ld(param_buf, 0) << 32) | rtas_ld(param_buf, 1);
>> +        mask = ((uint64_t)rtas_ld(param_buf, 2) << 32) | rtas_ld(param_buf, 3);
>> +        buid = ((uint64_t)rtas_ld(param_buf, 5) << 32) | rtas_ld(param_buf, 6);
>
>You might want to consider to introduce a helper function (e.g
>"ras_ld64"?) that loads the two 32 bit values and combines them.
>

In v1, I had rtas_ldq() for 64-bits values. David suggested to drop that and
use rtas_ld() directly. I agree with David that we don't have to maintain
another function, which is rarely used.

>> +        func = rtas_ld(param_buf, 7);
>> +    } else {
>> +        addr = rtas_ld(param_buf, 0);
>> +        mask = rtas_ld(param_buf, 1);
>> +        buid = ((uint64_t)rtas_ld(param_buf, 3) << 32) | rtas_ld(param_buf, 4);
>> +        func = rtas_ld(param_buf, 5);
>> +    }
>> +
>> +    /* Find PHB */
>> +    sphb = spapr_pci_find_phb(spapr, buid);
>> +    if (!sphb) {
>> +        return RTAS_OUT_PARAM_ERROR;
>> +    }
>> +
>> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> +    if (!spc->eeh_inject_error) {
>> +        return RTAS_OUT_PARAM_ERROR;
>> +    }
>> +
>> +    /* Handle the request */
>> +    return spc->eeh_inject_error(sphb, func, addr, mask, is_64bits);
>> +}
>> +
>>  static int pci_spapr_swizzle(int slot, int pin)
>>  {
>>      return (slot + pin) % PCI_NUM_PINS;
>> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
>> index cca45ed..a3674ee 100644
>> --- a/hw/ppc/spapr_pci_vfio.c
>> +++ b/hw/ppc/spapr_pci_vfio.c
>> @@ -17,6 +17,8 @@
>>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>>   */
>>  
>> +#include <asm/eeh.h>
>
>This does not work when building on non-powerpc systems. I think you
>have to use something like this instead:
>
>#include "asm-powerpc/eeh.h"
>

The question is how hw/ppc/spapr_pci_vfio.c is built on non-powerpc systems? If
some one tries to build this source file for non-powerpc systems, it will throw
error and force users to check, which isn't bad actually.

>>  #include "hw/ppc/spapr.h"
>>  #include "hw/pci-host/spapr.h"
>>  #include "hw/pci/msix.h"
>> @@ -250,6 +252,59 @@ static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
>>      return RTAS_OUT_SUCCESS;
>>  }
>>  
>> +static int spapr_phb_vfio_eeh_inject_error(sPAPRPHBState *sphb,
>> +                                           uint32_t func, uint64_t addr,
>> +                                           uint64_t mask, bool is_64bits)
>> +{
>> +    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
>> +    struct vfio_eeh_pe_op op = {
>> +        .op = VFIO_EEH_PE_INJECT_ERR,
>> +        .argsz = sizeof(op)
>> +    };
>> +    int ret = RTAS_OUT_SUCCESS;
>> +
>> +    op.err.type = is_64bits ? EEH_ERR_TYPE_64 : EEH_ERR_TYPE_32;
>> +    op.err.addr = addr;
>> +    op.err.mask = mask;
>> +
>> +    switch (func) {
>> +    case EEH_ERR_FUNC_LD_MEM_ADDR:
>> +    case EEH_ERR_FUNC_LD_MEM_DATA:
>> +    case EEH_ERR_FUNC_LD_IO_ADDR:
>> +    case EEH_ERR_FUNC_LD_IO_DATA:
>> +    case EEH_ERR_FUNC_LD_CFG_ADDR:
>> +    case EEH_ERR_FUNC_LD_CFG_DATA:
>> +    case EEH_ERR_FUNC_ST_MEM_ADDR:
>> +    case EEH_ERR_FUNC_ST_MEM_DATA:
>> +    case EEH_ERR_FUNC_ST_IO_ADDR:
>> +    case EEH_ERR_FUNC_ST_IO_DATA:
>> +    case EEH_ERR_FUNC_ST_CFG_ADDR:
>> +    case EEH_ERR_FUNC_ST_CFG_DATA:
>> +    case EEH_ERR_FUNC_DMA_RD_ADDR:
>> +    case EEH_ERR_FUNC_DMA_RD_DATA:
>> +    case EEH_ERR_FUNC_DMA_RD_MASTER:
>> +    case EEH_ERR_FUNC_DMA_RD_TARGET:
>> +    case EEH_ERR_FUNC_DMA_WR_ADDR:
>> +    case EEH_ERR_FUNC_DMA_WR_DATA:
>> +    case EEH_ERR_FUNC_DMA_WR_MASTER:
>
>EEH_ERR_FUNC_DMA_WR_TARGET is missing in the list ... I assume this is
>on purpose?
>

Good catch!

>> +        op.err.func = func;
>> +        break;
>> +    default:
>> +        ret = RTAS_OUT_PARAM_ERROR;
>> +        goto out;
>> +    }
>> +
>> +    if (vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
>> +                             VFIO_EEH_PE_OP, &op) < 0) {
>> +        ret = RTAS_OUT_HW_ERROR;
>> +        goto out;
>> +    }
>> +
>> +    ret = RTAS_OUT_SUCCESS;
>> +out:
>> +    return ret;
>> +}
>> +
>>  static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -262,6 +317,7 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
>>      spc->eeh_get_state = spapr_phb_vfio_eeh_get_state;
>>      spc->eeh_reset = spapr_phb_vfio_eeh_reset;
>>      spc->eeh_configure = spapr_phb_vfio_eeh_configure;
>> +    spc->eeh_inject_error = spapr_phb_vfio_eeh_inject_error;
>>  }
>>  
>>  static const TypeInfo spapr_phb_vfio_info = {
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index 8405056..5645f43 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -632,6 +632,54 @@ out:
>>      rtas_st(rets, 1, ret);
>>  }
>>  
>> +static void rtas_ibm_errinjct(PowerPCCPU *cpu,
>> +                              sPAPRMachineState *spapr,
>> +                              uint32_t token, uint32_t nargs,
>> +                              target_ulong args, uint32_t nret,
>> +                              target_ulong rets)
>> +{
>> +    target_ulong param_buf;
>> +    uint32_t type, open_token;
>> +    int32_t ret;
>> +
>> +    /* Sanity check on number of arguments */
>> +    if ((nargs != 3) || (nret != 1)) {
>
>Less paranthesis, please?
>

Sure.

>> +        ret = RTAS_OUT_PARAM_ERROR;
>> +        goto out;
>> +    }
>> +
>> +    /* Check if we have opened token */
>> +    open_token = rtas_ld(args, 1);
>> +    if (!spapr->is_errinjct_opened ||
>> +        spapr->errinjct_token != open_token) {
>> +        ret = RTAS_OUT_CLOSE_ERROR;
>> +        goto out;
>> +    }
>> +
>> +    /* The parameter buffer should be 1KB aligned */
>> +    param_buf = rtas_ld(args, 2);
>> +    if (param_buf & 0x3ff) {
>> +        ret = RTAS_OUT_PARAM_ERROR;
>> +        goto out;
>> +    }
>> +
>> +    /* Check the error type */
>> +    type = rtas_ld(args, 0);
>> +    switch (type) {
>> +    case RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR:
>> +        ret = spapr_rtas_errinjct_ioa(spapr, param_buf, false);
>> +        break;
>> +    case RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64:
>> +        ret = spapr_rtas_errinjct_ioa(spapr, param_buf, true);
>> +        break;
>> +    default:
>> +        ret = RTAS_OUT_PARAM_ERROR;
>> +    }
>> +
>> +out:
>> +    rtas_st(rets, 0, ret);
>> +}
>> +
>>  static void rtas_ibm_close_errinjct(PowerPCCPU *cpu,
>>                                      sPAPRMachineState *spapr,
>>                                      uint32_t token, uint32_t nargs,
>> @@ -723,6 +771,33 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
>>      int i;
>>      uint32_t lrdr_capacity[5];
>>      MachineState *machine = MACHINE(qdev_get_machine());
>> +    char errinjct_tokens[1024];
>> +    int fdt_offset, offset;
>> +    const int tokens[] = {
>> +        RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR,
>> +        RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64
>> +    };
>> +    const char *token_strings[] = {
>> +        "ioa-bus-error",
>> +        "ioa-bus-error-64"
>> +    };
>> +
>> +    /* ibm,errinjct-tokens */
>> +    offset = 0;
>> +    for (i = 0; i < ARRAY_SIZE(tokens); i++) {
>> +        offset += sprintf(errinjct_tokens + offset, "%s", token_strings[i]);
>> +        errinjct_tokens[offset++] = '\0';
>> +        *(int *)(&errinjct_tokens[offset]) = tokens[i];
>
>You can also get rid of some paranthesis here. Also I am not sure, but I
>think you have to take care of the endianess here? ==> Use stl_be_p instead?
>

Good question! Currently, the property (/rtas/ibm,errinjct-tokens) is used by
userland utility "errinjct" like below. So I think qemu needs pass BE tokens
and the utility also needs do conversion if necessary.

    ei_funcs[i].rtas_token = *(int *)tmp_ptr;  /* tmp_ptr is pointing to data tream
						* read from /rtas/ibm,errinjct-tokens */

>> +        offset += sizeof(int);
>> +    }
>> +
>> +    fdt_offset = fdt_path_offset(fdt, "/rtas");
>> +    ret = fdt_setprop(fdt, fdt_offset, "ibm,errinjct-tokens",
>> +                      errinjct_tokens, offset);
>> +    if (ret < 0) {
>> +        fprintf(stderr, "Couldn't add ibm,errinjct-tokens\n");
>> +        return ret;
>> +    }

Thanks,
Gavin

Re: [Qemu-devel] [PATCH v5 3/4] sPAPR: Support RTAS call ibm, {open, close}-errinjct

[David Gibson]

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

On Wed, Aug 19, 2015 at 09:52:00AM +1000, Gavin Shan wrote:
> On Tue, Aug 18, 2015 at 10:32:13AM -0700, Thomas Huth wrote:
> >On 17/08/15 18:47, Gavin Shan wrote:
> >> The patch supports RTAS calls "ibm,{open,close}-errinjct" to
> >> manupliate the token, which is passed to RTAS call "ibm,errinjct"
> >> to indicate the valid context for error injection. Each VM is
> >> permitted to have only one token at once and we simply have one
> >> random number for that.
> >
> >Looking at the code, you're using a sequence number now instead of a
> >random number?
> >
> 
> Yes, it's what Alexey suggested.

> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> >> ---
> >>  hw/ppc/spapr.c         |  6 ++++-
> >>  hw/ppc/spapr_rtas.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/ppc/spapr.h | 10 +++++++-
> >>  3 files changed, 80 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 06d000d..591a1a7 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -1191,7 +1191,7 @@ static bool version_before_3(void *opaque, int version_id)
> >>  
> >>  static const VMStateDescription vmstate_spapr = {
> >>      .name = "spapr",
> >> -    .version_id = 3,
> >> +    .version_id = 4,
> >>      .minimum_version_id = 1,
> >>      .post_load = spapr_post_load,
> >>      .fields = (VMStateField[]) {
> >> @@ -1202,6 +1202,10 @@ static const VMStateDescription vmstate_spapr = {
> >>          VMSTATE_UINT64_TEST(rtc_offset, sPAPRMachineState, version_before_3),
> >>  
> >>          VMSTATE_PPC_TIMEBASE_V(tb, sPAPRMachineState, 2),
> >> +
> >> +        /* Error injection token */
> >> +        VMSTATE_UINT32_V(errinjct_token, sPAPRMachineState, 4),
> >
> >Ok, so you're only saving the errinjct_token here, but not
> >is_errinjct_opened?
> >
> 
> Yes, It's also something that Alexey suggested in last round of review.

Um.. this doesn't look right.  You absolutely need to record the
opened bit.  You might be able to encode it in the token, but that
would require pre_save and post_load hooks which you haven't added.

The other approach would be to only have the token, advance it on both
open and close, then use
	opened == (token & 1)

> >>          VMSTATE_END_OF_LIST()
> >>      },
> >>  };
> >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> >> index e99e25f..8405056 100644
> >> --- a/hw/ppc/spapr_rtas.c
> >> +++ b/hw/ppc/spapr_rtas.c
> >> @@ -604,6 +604,68 @@ out:
> >>      rtas_st(rets, 0, rc);
> >>  }
> >>  
> >> +static void rtas_ibm_open_errinjct(PowerPCCPU *cpu,
> >> +                                   sPAPRMachineState *spapr,
> >> +                                   uint32_t token, uint32_t nargs,
> >> +                                   target_ulong args, uint32_t nret,
> >> +                                   target_ulong rets)
> >> +{
> >> +    int32_t ret;
> >> +
> >> +    /* Sanity check on number of arguments */
> >> +    if ((nargs != 0) || (nret != 2)) {
> >
> >Uh, did Alexey infect you with paranthesitis?
> >
> 
> hehe~, nope. I'll drop those unnecessary paranthesitis :-)

I'd prefer you didn't.  Unlike Thomas, I also don't remember C order
of ops that well and would prefer the clarity.

> >> +        ret = RTAS_OUT_PARAM_ERROR;
> >> +        goto out;
> >> +    }
> >> +
> >> +    /* Check if we already had token */
> >> +    if (spapr->is_errinjct_opened) {
> >> +        ret = RTAS_OUT_TOKEN_OPENED;
> >> +        goto out;
> >> +    }
> >> +
> >> +    /* Grab the token */
> >> +    spapr->is_errinjct_opened = true;
> >> +    rtas_st(rets, 0, ++spapr->errinjct_token);
> >> +    ret = RTAS_OUT_SUCCESS;
> >> +out:
> >> +    rtas_st(rets, 1, ret);
> >> +}
> >> +
> >> +static void rtas_ibm_close_errinjct(PowerPCCPU *cpu,
> >> +                                    sPAPRMachineState *spapr,
> >> +                                    uint32_t token, uint32_t nargs,
> >> +                                    target_ulong args, uint32_t nret,
> >> +                                    target_ulong rets)
> >> +{
> >> +    uint32_t open_token;
> >> +    int32_t ret;
> >> +
> >> +    /* Sanity check on number of arguments */
> >> +    if ((nargs != 1) || (nret != 1)) {
> >> +        ret = RTAS_OUT_PARAM_ERROR;
> >> +        goto out;
> >> +    }
> >> +
> >> +    /* Check if we had opened token */
> >> +    if (!spapr->is_errinjct_opened) {
> >> +        ret = RTAS_OUT_CLOSE_ERROR;
> >> +        goto out;
> >> +    }
> >
> >... and here you check that another status variable "is_errinjct_opened"
> >which is not saved in the VMStateDescription and thus this information
> >will get lost during migration, I think. That looks like a bug to me.
> >
> >Can you do your code completely without "is_errinjct_opened"? I.e. by
> >using errinjct_token == 0 for signalling that no injection progress is
> >currently taking place?
> >
> 
> It's fine to lose "is_errinjct_opened" after migration. The user needs
> another attempt to do error injection after migration.

It's really not.  This means error injection will be implicitly closed
on migration, which the guest shouldn't be able to see.  If you don't
preserve this, there's no point preserving the token value.

> umm, In v1, I used the condition "errinjct_token == 0" to indicate there
> is no injection in progress. After that, I received the suggestion to
> change the code to have two variables: one boolean for error injection
> token opening state and another one for error injection (sequential)
> token. I don't see any problem with it. 
> 
> >> +    /* Match with the passed token */
> >> +    open_token = rtas_ld(args, 0);
> >> +    if (spapr->errinjct_token != open_token) {
> >> +        ret = RTAS_OUT_CLOSE_ERROR;
> >> +        goto out;
> >> +    }
> >> +
> >> +    spapr->is_errinjct_opened = false;
> >> +    ret = RTAS_OUT_SUCCESS;
> >> +out:
> >> +    rtas_st(rets, 0, ret);
> >> +}
> 
> 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 --]

Re: [Qemu-devel] [PATCH v5 4/4] sPAPR: Support RTAS call ibm, errinjct

[David Gibson]

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

On Wed, Aug 19, 2015 at 10:26:04AM +1000, Gavin Shan wrote:
> On Tue, Aug 18, 2015 at 11:04:59AM -0700, Thomas Huth wrote:
> >On 17/08/15 18:47, Gavin Shan wrote:
> >> The patch supports RTAS call "ibm,errinjct" to allow injecting
> >> EEH errors to VFIO PCI devices. The implementation is similiar
> >> to EEH support for VFIO PCI devices: The RTAS request is captured
> >> by QEMU and routed to sPAPRPHBClass::eeh_inject_error() where the
> >> request is translated to VFIO container IOCTL command to be handled
> >> by the host.
> >> 
> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> >> ---
> >>  hw/ppc/spapr_pci.c          | 36 +++++++++++++++++++++
> >>  hw/ppc/spapr_pci_vfio.c     | 56 +++++++++++++++++++++++++++++++++
> >>  hw/ppc/spapr_rtas.c         | 77 +++++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/pci-host/spapr.h |  2 ++
> >>  include/hw/ppc/spapr.h      |  9 +++++-
> >>  5 files changed, 179 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> >> index 9d41060..f6223ce 100644
> >> --- a/hw/ppc/spapr_pci.c
> >> +++ b/hw/ppc/spapr_pci.c
> >> @@ -682,6 +682,42 @@ param_error_exit:
> >>      rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> >>  }
> >>  
> >> +int spapr_rtas_errinjct_ioa(sPAPRMachineState *spapr,
> >> +                            target_ulong param_buf,
> >> +                            bool is_64bits)
> >> +{
> >> +    sPAPRPHBState *sphb;
> >> +    sPAPRPHBClass *spc;
> >> +    uint64_t buid, addr, mask;
> >> +    uint32_t func;
> >> +
> >> +    if (is_64bits) {
> >> +        addr = ((uint64_t)rtas_ld(param_buf, 0) << 32) | rtas_ld(param_buf, 1);
> >> +        mask = ((uint64_t)rtas_ld(param_buf, 2) << 32) | rtas_ld(param_buf, 3);
> >> +        buid = ((uint64_t)rtas_ld(param_buf, 5) << 32) | rtas_ld(param_buf, 6);
> >
> >You might want to consider to introduce a helper function (e.g
> >"ras_ld64"?) that loads the two 32 bit values and combines them.
> >
> 
> In v1, I had rtas_ldq() for 64-bits values. David suggested to drop that and
> use rtas_ld() directly. I agree with David that we don't have to maintain
> another function, which is rarely used.

I'm not necessarily opposed to an rtas_ldq() or similar, but I'd
prefer to see it done as a separate cleanup patch.

> >> +        func = rtas_ld(param_buf, 7);
> >> +    } else {
> >> +        addr = rtas_ld(param_buf, 0);
> >> +        mask = rtas_ld(param_buf, 1);
> >> +        buid = ((uint64_t)rtas_ld(param_buf, 3) << 32) | rtas_ld(param_buf, 4);
> >> +        func = rtas_ld(param_buf, 5);
> >> +    }
> >> +
> >> +    /* Find PHB */
> >> +    sphb = spapr_pci_find_phb(spapr, buid);
> >> +    if (!sphb) {
> >> +        return RTAS_OUT_PARAM_ERROR;
> >> +    }
> >> +
> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> >> +    if (!spc->eeh_inject_error) {
> >> +        return RTAS_OUT_PARAM_ERROR;
> >> +    }
> >> +
> >> +    /* Handle the request */
> >> +    return spc->eeh_inject_error(sphb, func, addr, mask, is_64bits);
> >> +}
> >> +
> >>  static int pci_spapr_swizzle(int slot, int pin)
> >>  {
> >>      return (slot + pin) % PCI_NUM_PINS;
> >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> >> index cca45ed..a3674ee 100644
> >> --- a/hw/ppc/spapr_pci_vfio.c
> >> +++ b/hw/ppc/spapr_pci_vfio.c
> >> @@ -17,6 +17,8 @@
> >>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
> >>   */
> >>  
> >> +#include <asm/eeh.h>
> >
> >This does not work when building on non-powerpc systems. I think you
> >have to use something like this instead:
> >
> >#include "asm-powerpc/eeh.h"
> >
> 
> The question is how hw/ppc/spapr_pci_vfio.c is built on non-powerpc systems? If
> some one tries to build this source file for non-powerpc systems, it will throw
> error and force users to check, which isn't bad actually.

Using a VFIO device with a TCG guest is possible, at least
theoretically.  So this needs to be fixed.

> >>  #include "hw/ppc/spapr.h"
> >>  #include "hw/pci-host/spapr.h"
> >>  #include "hw/pci/msix.h"
> >> @@ -250,6 +252,59 @@ static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
> >>      return RTAS_OUT_SUCCESS;
> >>  }
> >>  
> >> +static int spapr_phb_vfio_eeh_inject_error(sPAPRPHBState *sphb,
> >> +                                           uint32_t func, uint64_t addr,
> >> +                                           uint64_t mask, bool is_64bits)
> >> +{
> >> +    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> >> +    struct vfio_eeh_pe_op op = {
> >> +        .op = VFIO_EEH_PE_INJECT_ERR,
> >> +        .argsz = sizeof(op)
> >> +    };
> >> +    int ret = RTAS_OUT_SUCCESS;
> >> +
> >> +    op.err.type = is_64bits ? EEH_ERR_TYPE_64 : EEH_ERR_TYPE_32;
> >> +    op.err.addr = addr;
> >> +    op.err.mask = mask;
> >> +
> >> +    switch (func) {
> >> +    case EEH_ERR_FUNC_LD_MEM_ADDR:
> >> +    case EEH_ERR_FUNC_LD_MEM_DATA:
> >> +    case EEH_ERR_FUNC_LD_IO_ADDR:
> >> +    case EEH_ERR_FUNC_LD_IO_DATA:
> >> +    case EEH_ERR_FUNC_LD_CFG_ADDR:
> >> +    case EEH_ERR_FUNC_LD_CFG_DATA:
> >> +    case EEH_ERR_FUNC_ST_MEM_ADDR:
> >> +    case EEH_ERR_FUNC_ST_MEM_DATA:
> >> +    case EEH_ERR_FUNC_ST_IO_ADDR:
> >> +    case EEH_ERR_FUNC_ST_IO_DATA:
> >> +    case EEH_ERR_FUNC_ST_CFG_ADDR:
> >> +    case EEH_ERR_FUNC_ST_CFG_DATA:
> >> +    case EEH_ERR_FUNC_DMA_RD_ADDR:
> >> +    case EEH_ERR_FUNC_DMA_RD_DATA:
> >> +    case EEH_ERR_FUNC_DMA_RD_MASTER:
> >> +    case EEH_ERR_FUNC_DMA_RD_TARGET:
> >> +    case EEH_ERR_FUNC_DMA_WR_ADDR:
> >> +    case EEH_ERR_FUNC_DMA_WR_DATA:
> >> +    case EEH_ERR_FUNC_DMA_WR_MASTER:
> >
> >EEH_ERR_FUNC_DMA_WR_TARGET is missing in the list ... I assume this is
> >on purpose?
> >
> 
> Good catch!
> 
> >> +        op.err.func = func;
> >> +        break;
> >> +    default:
> >> +        ret = RTAS_OUT_PARAM_ERROR;
> >> +        goto out;
> >> +    }
> >> +
> >> +    if (vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
> >> +                             VFIO_EEH_PE_OP, &op) < 0) {
> >> +        ret = RTAS_OUT_HW_ERROR;
> >> +        goto out;
> >> +    }
> >> +
> >> +    ret = RTAS_OUT_SUCCESS;
> >> +out:
> >> +    return ret;
> >> +}
> >> +
> >>  static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
> >>  {
> >>      DeviceClass *dc = DEVICE_CLASS(klass);
> >> @@ -262,6 +317,7 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
> >>      spc->eeh_get_state = spapr_phb_vfio_eeh_get_state;
> >>      spc->eeh_reset = spapr_phb_vfio_eeh_reset;
> >>      spc->eeh_configure = spapr_phb_vfio_eeh_configure;
> >> +    spc->eeh_inject_error = spapr_phb_vfio_eeh_inject_error;
> >>  }
> >>  
> >>  static const TypeInfo spapr_phb_vfio_info = {
> >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> >> index 8405056..5645f43 100644
> >> --- a/hw/ppc/spapr_rtas.c
> >> +++ b/hw/ppc/spapr_rtas.c
> >> @@ -632,6 +632,54 @@ out:
> >>      rtas_st(rets, 1, ret);
> >>  }
> >>  
> >> +static void rtas_ibm_errinjct(PowerPCCPU *cpu,
> >> +                              sPAPRMachineState *spapr,
> >> +                              uint32_t token, uint32_t nargs,
> >> +                              target_ulong args, uint32_t nret,
> >> +                              target_ulong rets)
> >> +{
> >> +    target_ulong param_buf;
> >> +    uint32_t type, open_token;
> >> +    int32_t ret;
> >> +
> >> +    /* Sanity check on number of arguments */
> >> +    if ((nargs != 3) || (nret != 1)) {
> >
> >Less paranthesis, please?
> >
> 
> Sure.
> 
> >> +        ret = RTAS_OUT_PARAM_ERROR;
> >> +        goto out;
> >> +    }
> >> +
> >> +    /* Check if we have opened token */
> >> +    open_token = rtas_ld(args, 1);
> >> +    if (!spapr->is_errinjct_opened ||
> >> +        spapr->errinjct_token != open_token) {
> >> +        ret = RTAS_OUT_CLOSE_ERROR;
> >> +        goto out;
> >> +    }
> >> +
> >> +    /* The parameter buffer should be 1KB aligned */
> >> +    param_buf = rtas_ld(args, 2);
> >> +    if (param_buf & 0x3ff) {
> >> +        ret = RTAS_OUT_PARAM_ERROR;
> >> +        goto out;
> >> +    }
> >> +
> >> +    /* Check the error type */
> >> +    type = rtas_ld(args, 0);
> >> +    switch (type) {
> >> +    case RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR:
> >> +        ret = spapr_rtas_errinjct_ioa(spapr, param_buf, false);
> >> +        break;
> >> +    case RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64:
> >> +        ret = spapr_rtas_errinjct_ioa(spapr, param_buf, true);
> >> +        break;
> >> +    default:
> >> +        ret = RTAS_OUT_PARAM_ERROR;
> >> +    }
> >> +
> >> +out:
> >> +    rtas_st(rets, 0, ret);
> >> +}
> >> +
> >>  static void rtas_ibm_close_errinjct(PowerPCCPU *cpu,
> >>                                      sPAPRMachineState *spapr,
> >>                                      uint32_t token, uint32_t nargs,
> >> @@ -723,6 +771,33 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
> >>      int i;
> >>      uint32_t lrdr_capacity[5];
> >>      MachineState *machine = MACHINE(qdev_get_machine());
> >> +    char errinjct_tokens[1024];
> >> +    int fdt_offset, offset;
> >> +    const int tokens[] = {
> >> +        RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR,
> >> +        RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64
> >> +    };
> >> +    const char *token_strings[] = {
> >> +        "ioa-bus-error",
> >> +        "ioa-bus-error-64"
> >> +    };
> >> +
> >> +    /* ibm,errinjct-tokens */
> >> +    offset = 0;
> >> +    for (i = 0; i < ARRAY_SIZE(tokens); i++) {
> >> +        offset += sprintf(errinjct_tokens + offset, "%s", token_strings[i]);
> >> +        errinjct_tokens[offset++] = '\0';
> >> +        *(int *)(&errinjct_tokens[offset]) = tokens[i];
> >
> >You can also get rid of some paranthesis here. Also I am not sure, but I
> >think you have to take care of the endianess here? ==> Use stl_be_p instead?
> >
> 
> Good question! Currently, the property (/rtas/ibm,errinjct-tokens) is used by
> userland utility "errinjct" like below. So I think qemu needs pass BE tokens
> and the utility also needs do conversion if necessary.
> 
>     ei_funcs[i].rtas_token = *(int *)tmp_ptr;  /* tmp_ptr is pointing to data tream
> 						* read from /rtas/ibm,errinjct-tokens */

Yes, values you put into the device tree should definitely be BE.

> >> +        offset += sizeof(int);
> >> +    }
> >> +
> >> +    fdt_offset = fdt_path_offset(fdt, "/rtas");
> >> +    ret = fdt_setprop(fdt, fdt_offset, "ibm,errinjct-tokens",
> >> +                      errinjct_tokens, offset);
> >> +    if (ret < 0) {
> >> +        fprintf(stderr, "Couldn't add ibm,errinjct-tokens\n");
> >> +        return ret;
> >> +    }
> 
> 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 --]

Re: [Qemu-devel] [PATCH v5 4/4] sPAPR: Support RTAS call ibm, errinjct

[Thomas Huth]

On 18/08/15 17:26, Gavin Shan wrote:
> On Tue, Aug 18, 2015 at 11:04:59AM -0700, Thomas Huth wrote:
>> On 17/08/15 18:47, Gavin Shan wrote:
>>> The patch supports RTAS call "ibm,errinjct" to allow injecting
>>> EEH errors to VFIO PCI devices. The implementation is similiar
>>> to EEH support for VFIO PCI devices: The RTAS request is captured
>>> by QEMU and routed to sPAPRPHBClass::eeh_inject_error() where the
>>> request is translated to VFIO container IOCTL command to be handled
>>> by the host.
>>>
>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>> ---
>>>  hw/ppc/spapr_pci.c          | 36 +++++++++++++++++++++
>>>  hw/ppc/spapr_pci_vfio.c     | 56 +++++++++++++++++++++++++++++++++
>>>  hw/ppc/spapr_rtas.c         | 77 +++++++++++++++++++++++++++++++++++++++++++++
>>>  include/hw/pci-host/spapr.h |  2 ++
>>>  include/hw/ppc/spapr.h      |  9 +++++-
>>>  5 files changed, 179 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index 9d41060..f6223ce 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -682,6 +682,42 @@ param_error_exit:
>>>      rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>>>  }
>>>  
>>> +int spapr_rtas_errinjct_ioa(sPAPRMachineState *spapr,
>>> +                            target_ulong param_buf,
>>> +                            bool is_64bits)
>>> +{
>>> +    sPAPRPHBState *sphb;
>>> +    sPAPRPHBClass *spc;
>>> +    uint64_t buid, addr, mask;
>>> +    uint32_t func;
>>> +
>>> +    if (is_64bits) {
>>> +        addr = ((uint64_t)rtas_ld(param_buf, 0) << 32) | rtas_ld(param_buf, 1);
>>> +        mask = ((uint64_t)rtas_ld(param_buf, 2) << 32) | rtas_ld(param_buf, 3);
>>> +        buid = ((uint64_t)rtas_ld(param_buf, 5) << 32) | rtas_ld(param_buf, 6);
>>
>> You might want to consider to introduce a helper function (e.g
>> "ras_ld64"?) that loads the two 32 bit values and combines them.
>>
> 
> In v1, I had rtas_ldq() for 64-bits values. David suggested to drop that and
> use rtas_ld() directly. I agree with David that we don't have to maintain
> another function, which is rarely used.

There are also other spots in the code that load a 64-bit value that
way, so they could be reworked, too...
Anyway, if you and David don't like this idea, simply never mind, it's
not that important.

>>> +        func = rtas_ld(param_buf, 7);
>>> +    } else {
>>> +        addr = rtas_ld(param_buf, 0);
>>> +        mask = rtas_ld(param_buf, 1);
>>> +        buid = ((uint64_t)rtas_ld(param_buf, 3) << 32) | rtas_ld(param_buf, 4);
>>> +        func = rtas_ld(param_buf, 5);
>>> +    }
>>> +
>>> +    /* Find PHB */
>>> +    sphb = spapr_pci_find_phb(spapr, buid);
>>> +    if (!sphb) {
>>> +        return RTAS_OUT_PARAM_ERROR;
>>> +    }
>>> +
>>> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>>> +    if (!spc->eeh_inject_error) {
>>> +        return RTAS_OUT_PARAM_ERROR;
>>> +    }
>>> +
>>> +    /* Handle the request */
>>> +    return spc->eeh_inject_error(sphb, func, addr, mask, is_64bits);
>>> +}
>>> +
>>>  static int pci_spapr_swizzle(int slot, int pin)
>>>  {
>>>      return (slot + pin) % PCI_NUM_PINS;
>>> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
>>> index cca45ed..a3674ee 100644
>>> --- a/hw/ppc/spapr_pci_vfio.c
>>> +++ b/hw/ppc/spapr_pci_vfio.c
>>> @@ -17,6 +17,8 @@
>>>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>>>   */
>>>  
>>> +#include <asm/eeh.h>
>>
>> This does not work when building on non-powerpc systems. I think you
>> have to use something like this instead:
>>
>> #include "asm-powerpc/eeh.h"
>>
> 
> The question is how hw/ppc/spapr_pci_vfio.c is built on non-powerpc systems? If
> some one tries to build this source file for non-powerpc systems, it will throw
> error and force users to check, which isn't bad actually.

Simply try to compile qemu-softmmu-pp64 on your x86 laptop (with TCG)!
The spapr_pci_vfio.c file is also compiled there, and if you use
"<asm/eeh.h>", you break the build!

>>>  #include "hw/ppc/spapr.h"
>>>  #include "hw/pci-host/spapr.h"
>>>  #include "hw/pci/msix.h"
>>> @@ -250,6 +252,59 @@ static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
>>>      return RTAS_OUT_SUCCESS;
>>>  }
>>>  
>>> +static int spapr_phb_vfio_eeh_inject_error(sPAPRPHBState *sphb,
>>> +                                           uint32_t func, uint64_t addr,
>>> +                                           uint64_t mask, bool is_64bits)
>>> +{
>>> +    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
>>> +    struct vfio_eeh_pe_op op = {
>>> +        .op = VFIO_EEH_PE_INJECT_ERR,
>>> +        .argsz = sizeof(op)
>>> +    };
>>> +    int ret = RTAS_OUT_SUCCESS;
>>> +
>>> +    op.err.type = is_64bits ? EEH_ERR_TYPE_64 : EEH_ERR_TYPE_32;
>>> +    op.err.addr = addr;
>>> +    op.err.mask = mask;
>>> +
>>> +    switch (func) {
>>> +    case EEH_ERR_FUNC_LD_MEM_ADDR:
>>> +    case EEH_ERR_FUNC_LD_MEM_DATA:
>>> +    case EEH_ERR_FUNC_LD_IO_ADDR:
>>> +    case EEH_ERR_FUNC_LD_IO_DATA:
>>> +    case EEH_ERR_FUNC_LD_CFG_ADDR:
>>> +    case EEH_ERR_FUNC_LD_CFG_DATA:
>>> +    case EEH_ERR_FUNC_ST_MEM_ADDR:
>>> +    case EEH_ERR_FUNC_ST_MEM_DATA:
>>> +    case EEH_ERR_FUNC_ST_IO_ADDR:
>>> +    case EEH_ERR_FUNC_ST_IO_DATA:
>>> +    case EEH_ERR_FUNC_ST_CFG_ADDR:
>>> +    case EEH_ERR_FUNC_ST_CFG_DATA:
>>> +    case EEH_ERR_FUNC_DMA_RD_ADDR:
>>> +    case EEH_ERR_FUNC_DMA_RD_DATA:
>>> +    case EEH_ERR_FUNC_DMA_RD_MASTER:
>>> +    case EEH_ERR_FUNC_DMA_RD_TARGET:
>>> +    case EEH_ERR_FUNC_DMA_WR_ADDR:
>>> +    case EEH_ERR_FUNC_DMA_WR_DATA:
>>> +    case EEH_ERR_FUNC_DMA_WR_MASTER:
>>
>> EEH_ERR_FUNC_DMA_WR_TARGET is missing in the list ... I assume this is
>> on purpose?
>>
> 
> Good catch!

Ok, so if you want to test for all the defined cases from eeh.h,
wouldn't it be easier to simply check

	if (func > EEH_ERR_FUNC_MAX) {
		ret = RTAS_OUT_PARAM_ERROR;
		goto out;
	}

?

[...]
>>> @@ -723,6 +771,33 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
>>>      int i;
>>>      uint32_t lrdr_capacity[5];
>>>      MachineState *machine = MACHINE(qdev_get_machine());
>>> +    char errinjct_tokens[1024];
>>> +    int fdt_offset, offset;
>>> +    const int tokens[] = {
>>> +        RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR,
>>> +        RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64
>>> +    };
>>> +    const char *token_strings[] = {
>>> +        "ioa-bus-error",
>>> +        "ioa-bus-error-64"
>>> +    };
>>> +
>>> +    /* ibm,errinjct-tokens */
>>> +    offset = 0;
>>> +    for (i = 0; i < ARRAY_SIZE(tokens); i++) {
>>> +        offset += sprintf(errinjct_tokens + offset, "%s", token_strings[i]);
>>> +        errinjct_tokens[offset++] = '\0';
>>> +        *(int *)(&errinjct_tokens[offset]) = tokens[i];
>>
>> You can also get rid of some paranthesis here. Also I am not sure, but I
>> think you have to take care of the endianess here? ==> Use stl_be_p instead?
> 
> Good question! Currently, the property (/rtas/ibm,errinjct-tokens) is used by
> userland utility "errinjct" like below. So I think qemu needs pass BE tokens
> and the utility also needs do conversion if necessary.

Right, otherwise cross-endian setup won't work.

 Thomas

Re: [Qemu-devel] [PATCH v5 3/4] sPAPR: Support RTAS call ibm, {open, close}-errinjct

[Thomas Huth]

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

On 18/08/15 18:15, David Gibson wrote:
> On Wed, Aug 19, 2015 at 09:52:00AM +1000, Gavin Shan wrote:
>> On Tue, Aug 18, 2015 at 10:32:13AM -0700, Thomas Huth wrote:
>>> On 17/08/15 18:47, Gavin Shan wrote:
>>>> The patch supports RTAS calls "ibm,{open,close}-errinjct" to
>>>> manupliate the token, which is passed to RTAS call "ibm,errinjct"
>>>> to indicate the valid context for error injection. Each VM is
>>>> permitted to have only one token at once and we simply have one
>>>> random number for that.
>>>
>>> Looking at the code, you're using a sequence number now instead of a
>>> random number?
>>>
>>
>> Yes, it's what Alexey suggested.

Then please update the commit message accordingly.

>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>> index e99e25f..8405056 100644
>>>> --- a/hw/ppc/spapr_rtas.c
>>>> +++ b/hw/ppc/spapr_rtas.c
>>>> @@ -604,6 +604,68 @@ out:
>>>>      rtas_st(rets, 0, rc);
>>>>  }
>>>>  
>>>> +static void rtas_ibm_open_errinjct(PowerPCCPU *cpu,
>>>> +                                   sPAPRMachineState *spapr,
>>>> +                                   uint32_t token, uint32_t nargs,
>>>> +                                   target_ulong args, uint32_t nret,
>>>> +                                   target_ulong rets)
>>>> +{
>>>> +    int32_t ret;
>>>> +
>>>> +    /* Sanity check on number of arguments */
>>>> +    if ((nargs != 0) || (nret != 2)) {
>>>
>>> Uh, did Alexey infect you with paranthesitis?
>>>
>>
>> hehe~, nope. I'll drop those unnecessary paranthesitis :-)
> 
> I'd prefer you didn't.  Unlike Thomas, I also don't remember C order
> of ops that well and would prefer the clarity.

You can always look it up if you're unsure, e.g.:

http://en.cppreference.com/w/c/language/operator_precedence

And once you've learnt it, the additional paranthesis just look
cumbersome. So please remove them!

 Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v5 3/4] sPAPR: Support RTAS call ibm, {open, close}-errinjct

[Gavin Shan]

On Wed, Aug 19, 2015 at 09:15:26AM -0700, Thomas Huth wrote:
>On 18/08/15 18:15, David Gibson wrote:
>> On Wed, Aug 19, 2015 at 09:52:00AM +1000, Gavin Shan wrote:
>>> On Tue, Aug 18, 2015 at 10:32:13AM -0700, Thomas Huth wrote:
>>>> On 17/08/15 18:47, Gavin Shan wrote:
>>>>> The patch supports RTAS calls "ibm,{open,close}-errinjct" to
>>>>> manupliate the token, which is passed to RTAS call "ibm,errinjct"
>>>>> to indicate the valid context for error injection. Each VM is
>>>>> permitted to have only one token at once and we simply have one
>>>>> random number for that.
>>>>
>>>> Looking at the code, you're using a sequence number now instead of a
>>>> random number?
>>>>
>>>
>>> Yes, it's what Alexey suggested.
>
>Then please update the commit message accordingly.
>

Yes, I'll update changelog accordingly.

>>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>>> index e99e25f..8405056 100644
>>>>> --- a/hw/ppc/spapr_rtas.c
>>>>> +++ b/hw/ppc/spapr_rtas.c
>>>>> @@ -604,6 +604,68 @@ out:
>>>>>      rtas_st(rets, 0, rc);
>>>>>  }
>>>>>  
>>>>> +static void rtas_ibm_open_errinjct(PowerPCCPU *cpu,
>>>>> +                                   sPAPRMachineState *spapr,
>>>>> +                                   uint32_t token, uint32_t nargs,
>>>>> +                                   target_ulong args, uint32_t nret,
>>>>> +                                   target_ulong rets)
>>>>> +{
>>>>> +    int32_t ret;
>>>>> +
>>>>> +    /* Sanity check on number of arguments */
>>>>> +    if ((nargs != 0) || (nret != 2)) {
>>>>
>>>> Uh, did Alexey infect you with paranthesitis?
>>>>
>>>
>>> hehe~, nope. I'll drop those unnecessary paranthesitis :-)
>> 
>> I'd prefer you didn't.  Unlike Thomas, I also don't remember C order
>> of ops that well and would prefer the clarity.
>
>You can always look it up if you're unsure, e.g.:
>
>http://en.cppreference.com/w/c/language/operator_precedence
>
>And once you've learnt it, the additional paranthesis just look
>cumbersome. So please remove them!
>

Ok. I'll check the code and remove unnecessary paranthesis in next revision.

Thanks,
Gavin

Re: [Qemu-devel] [PATCH v5 4/4] sPAPR: Support RTAS call ibm, errinjct

[Gavin Shan]

On Wed, Aug 19, 2015 at 08:48:20AM -0700, Thomas Huth wrote:
>On 18/08/15 17:26, Gavin Shan wrote:
>> On Tue, Aug 18, 2015 at 11:04:59AM -0700, Thomas Huth wrote:
>>> On 17/08/15 18:47, Gavin Shan wrote:
>>>> The patch supports RTAS call "ibm,errinjct" to allow injecting
>>>> EEH errors to VFIO PCI devices. The implementation is similiar
>>>> to EEH support for VFIO PCI devices: The RTAS request is captured
>>>> by QEMU and routed to sPAPRPHBClass::eeh_inject_error() where the
>>>> request is translated to VFIO container IOCTL command to be handled
>>>> by the host.
>>>>
>>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>> ---
>>>>  hw/ppc/spapr_pci.c          | 36 +++++++++++++++++++++
>>>>  hw/ppc/spapr_pci_vfio.c     | 56 +++++++++++++++++++++++++++++++++
>>>>  hw/ppc/spapr_rtas.c         | 77 +++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/hw/pci-host/spapr.h |  2 ++
>>>>  include/hw/ppc/spapr.h      |  9 +++++-
>>>>  5 files changed, 179 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>>> index 9d41060..f6223ce 100644
>>>> --- a/hw/ppc/spapr_pci.c
>>>> +++ b/hw/ppc/spapr_pci.c
>>>> @@ -682,6 +682,42 @@ param_error_exit:
>>>>      rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>>>>  }
>>>>  
>>>> +int spapr_rtas_errinjct_ioa(sPAPRMachineState *spapr,
>>>> +                            target_ulong param_buf,
>>>> +                            bool is_64bits)
>>>> +{
>>>> +    sPAPRPHBState *sphb;
>>>> +    sPAPRPHBClass *spc;
>>>> +    uint64_t buid, addr, mask;
>>>> +    uint32_t func;
>>>> +
>>>> +    if (is_64bits) {
>>>> +        addr = ((uint64_t)rtas_ld(param_buf, 0) << 32) | rtas_ld(param_buf, 1);
>>>> +        mask = ((uint64_t)rtas_ld(param_buf, 2) << 32) | rtas_ld(param_buf, 3);
>>>> +        buid = ((uint64_t)rtas_ld(param_buf, 5) << 32) | rtas_ld(param_buf, 6);
>>>
>>> You might want to consider to introduce a helper function (e.g
>>> "ras_ld64"?) that loads the two 32 bit values and combines them.
>>>
>> 
>> In v1, I had rtas_ldq() for 64-bits values. David suggested to drop that and
>> use rtas_ld() directly. I agree with David that we don't have to maintain
>> another function, which is rarely used.
>
>There are also other spots in the code that load a 64-bit value that
>way, so they could be reworked, too...
>Anyway, if you and David don't like this idea, simply never mind, it's
>not that important.
>

Ok. I'll pick rtas_ldq() that was dropped in v2 in separate patch and try
to replace rtas_ld() with the new function.

>>>> +        func = rtas_ld(param_buf, 7);
>>>> +    } else {
>>>> +        addr = rtas_ld(param_buf, 0);
>>>> +        mask = rtas_ld(param_buf, 1);
>>>> +        buid = ((uint64_t)rtas_ld(param_buf, 3) << 32) | rtas_ld(param_buf, 4);
>>>> +        func = rtas_ld(param_buf, 5);
>>>> +    }
>>>> +
>>>> +    /* Find PHB */
>>>> +    sphb = spapr_pci_find_phb(spapr, buid);
>>>> +    if (!sphb) {
>>>> +        return RTAS_OUT_PARAM_ERROR;
>>>> +    }
>>>> +
>>>> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>>>> +    if (!spc->eeh_inject_error) {
>>>> +        return RTAS_OUT_PARAM_ERROR;
>>>> +    }
>>>> +
>>>> +    /* Handle the request */
>>>> +    return spc->eeh_inject_error(sphb, func, addr, mask, is_64bits);
>>>> +}
>>>> +
>>>>  static int pci_spapr_swizzle(int slot, int pin)
>>>>  {
>>>>      return (slot + pin) % PCI_NUM_PINS;
>>>> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
>>>> index cca45ed..a3674ee 100644
>>>> --- a/hw/ppc/spapr_pci_vfio.c
>>>> +++ b/hw/ppc/spapr_pci_vfio.c
>>>> @@ -17,6 +17,8 @@
>>>>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>>>>   */
>>>>  
>>>> +#include <asm/eeh.h>
>>>
>>> This does not work when building on non-powerpc systems. I think you
>>> have to use something like this instead:
>>>
>>> #include "asm-powerpc/eeh.h"
>>>
>> 
>> The question is how hw/ppc/spapr_pci_vfio.c is built on non-powerpc systems? If
>> some one tries to build this source file for non-powerpc systems, it will throw
>> error and force users to check, which isn't bad actually.
>
>Simply try to compile qemu-softmmu-pp64 on your x86 laptop (with TCG)!
>The spapr_pci_vfio.c file is also compiled there, and if you use
>"<asm/eeh.h>", you break the build!
>

Yes, Thanks for the details!

>>>>  #include "hw/ppc/spapr.h"
>>>>  #include "hw/pci-host/spapr.h"
>>>>  #include "hw/pci/msix.h"
>>>> @@ -250,6 +252,59 @@ static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
>>>>      return RTAS_OUT_SUCCESS;
>>>>  }
>>>>  
>>>> +static int spapr_phb_vfio_eeh_inject_error(sPAPRPHBState *sphb,
>>>> +                                           uint32_t func, uint64_t addr,
>>>> +                                           uint64_t mask, bool is_64bits)
>>>> +{
>>>> +    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
>>>> +    struct vfio_eeh_pe_op op = {
>>>> +        .op = VFIO_EEH_PE_INJECT_ERR,
>>>> +        .argsz = sizeof(op)
>>>> +    };
>>>> +    int ret = RTAS_OUT_SUCCESS;
>>>> +
>>>> +    op.err.type = is_64bits ? EEH_ERR_TYPE_64 : EEH_ERR_TYPE_32;
>>>> +    op.err.addr = addr;
>>>> +    op.err.mask = mask;
>>>> +
>>>> +    switch (func) {
>>>> +    case EEH_ERR_FUNC_LD_MEM_ADDR:
>>>> +    case EEH_ERR_FUNC_LD_MEM_DATA:
>>>> +    case EEH_ERR_FUNC_LD_IO_ADDR:
>>>> +    case EEH_ERR_FUNC_LD_IO_DATA:
>>>> +    case EEH_ERR_FUNC_LD_CFG_ADDR:
>>>> +    case EEH_ERR_FUNC_LD_CFG_DATA:
>>>> +    case EEH_ERR_FUNC_ST_MEM_ADDR:
>>>> +    case EEH_ERR_FUNC_ST_MEM_DATA:
>>>> +    case EEH_ERR_FUNC_ST_IO_ADDR:
>>>> +    case EEH_ERR_FUNC_ST_IO_DATA:
>>>> +    case EEH_ERR_FUNC_ST_CFG_ADDR:
>>>> +    case EEH_ERR_FUNC_ST_CFG_DATA:
>>>> +    case EEH_ERR_FUNC_DMA_RD_ADDR:
>>>> +    case EEH_ERR_FUNC_DMA_RD_DATA:
>>>> +    case EEH_ERR_FUNC_DMA_RD_MASTER:
>>>> +    case EEH_ERR_FUNC_DMA_RD_TARGET:
>>>> +    case EEH_ERR_FUNC_DMA_WR_ADDR:
>>>> +    case EEH_ERR_FUNC_DMA_WR_DATA:
>>>> +    case EEH_ERR_FUNC_DMA_WR_MASTER:
>>>
>>> EEH_ERR_FUNC_DMA_WR_TARGET is missing in the list ... I assume this is
>>> on purpose?
>>>
>> 
>> Good catch!
>
>Ok, so if you want to test for all the defined cases from eeh.h,
>wouldn't it be easier to simply check
>
>	if (func > EEH_ERR_FUNC_MAX) {
>		ret = RTAS_OUT_PARAM_ERROR;
>		goto out;
>	}
>
>?
>

Yes, absolutely.

>[...]
>>>> @@ -723,6 +771,33 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
>>>>      int i;
>>>>      uint32_t lrdr_capacity[5];
>>>>      MachineState *machine = MACHINE(qdev_get_machine());
>>>> +    char errinjct_tokens[1024];
>>>> +    int fdt_offset, offset;
>>>> +    const int tokens[] = {
>>>> +        RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR,
>>>> +        RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64
>>>> +    };
>>>> +    const char *token_strings[] = {
>>>> +        "ioa-bus-error",
>>>> +        "ioa-bus-error-64"
>>>> +    };
>>>> +
>>>> +    /* ibm,errinjct-tokens */
>>>> +    offset = 0;
>>>> +    for (i = 0; i < ARRAY_SIZE(tokens); i++) {
>>>> +        offset += sprintf(errinjct_tokens + offset, "%s", token_strings[i]);
>>>> +        errinjct_tokens[offset++] = '\0';
>>>> +        *(int *)(&errinjct_tokens[offset]) = tokens[i];
>>>
>>> You can also get rid of some paranthesis here. Also I am not sure, but I
>>> think you have to take care of the endianess here? ==> Use stl_be_p instead?
>> 
>> Good question! Currently, the property (/rtas/ibm,errinjct-tokens) is used by
>> userland utility "errinjct" like below. So I think qemu needs pass BE tokens
>> and the utility also needs do conversion if necessary.
>
>Right, otherwise cross-endian setup won't work.
>

Thanks,
Gavin

Re: [Qemu-devel] [PATCH v5 3/4] sPAPR: Support RTAS call ibm, {open, close}-errinjct

[Alexey Kardashevskiy]

On 08/20/2015 02:15 AM, Thomas Huth wrote:
> On 18/08/15 18:15, David Gibson wrote:
>> On Wed, Aug 19, 2015 at 09:52:00AM +1000, Gavin Shan wrote:
>>> On Tue, Aug 18, 2015 at 10:32:13AM -0700, Thomas Huth wrote:
>>>> On 17/08/15 18:47, Gavin Shan wrote:
>>>>> The patch supports RTAS calls "ibm,{open,close}-errinjct" to
>>>>> manupliate the token, which is passed to RTAS call "ibm,errinjct"
>>>>> to indicate the valid context for error injection. Each VM is
>>>>> permitted to have only one token at once and we simply have one
>>>>> random number for that.
>>>>
>>>> Looking at the code, you're using a sequence number now instead of a
>>>> random number?
>>>>
>>>
>>> Yes, it's what Alexey suggested.
>
> Then please update the commit message accordingly.
>
>>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>>> index e99e25f..8405056 100644
>>>>> --- a/hw/ppc/spapr_rtas.c
>>>>> +++ b/hw/ppc/spapr_rtas.c
>>>>> @@ -604,6 +604,68 @@ out:
>>>>>       rtas_st(rets, 0, rc);
>>>>>   }
>>>>>
>>>>> +static void rtas_ibm_open_errinjct(PowerPCCPU *cpu,
>>>>> +                                   sPAPRMachineState *spapr,
>>>>> +                                   uint32_t token, uint32_t nargs,
>>>>> +                                   target_ulong args, uint32_t nret,
>>>>> +                                   target_ulong rets)
>>>>> +{
>>>>> +    int32_t ret;
>>>>> +
>>>>> +    /* Sanity check on number of arguments */
>>>>> +    if ((nargs != 0) || (nret != 2)) {
>>>>
>>>> Uh, did Alexey infect you with paranthesitis?
>>>>
>>>
>>> hehe~, nope. I'll drop those unnecessary paranthesitis :-)
>>
>> I'd prefer you didn't.  Unlike Thomas, I also don't remember C order
>> of ops that well and would prefer the clarity.
>
> You can always look it up if you're unsure, e.g.:
>
> http://en.cppreference.com/w/c/language/operator_precedence
>
> And once you've learnt it, the additional paranthesis just look
> cumbersome. So please remove them!

I still prefer the clarity and therefore more braces which make no harm 
whatsoever.



-- 
Alexey