[Qemu-devel] [PATCH RESEND v2 0/3] sPAPR: Support EEH Error Injection

[Qemu-devel] [PATCH RESEND v2 0/3] 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
=========
v1 -> 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 (3):
  linux-headers: Add eeh.h
  sPAPR: Support RTAS call ibm, {open, close}-errinjct
  sPAPR: Support RTAS call ibm,errinjct

 hw/ppc/spapr_pci.c              |  42 +++++++++++
 hw/ppc/spapr_pci_vfio.c         |  56 +++++++++++++++
 hw/ppc/spapr_rtas.c             | 156 ++++++++++++++++++++++++++++++++++++++++
 include/hw/pci-host/spapr.h     |   2 +
 include/hw/ppc/spapr.h          |  35 ++++++++-
 linux-headers/asm-powerpc/eeh.h |  56 +++++++++++++++
 6 files changed, 346 insertions(+), 1 deletion(-)
 create mode 100644 linux-headers/asm-powerpc/eeh.h

-- 
2.1.0

[Qemu-devel] [PATCH RESEND v2 1/3] 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>
---
 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 RESEND v2 2/3] 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_rtas.c    | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  9 ++++++-
 2 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index e99e25f..0a9c904 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -604,6 +604,73 @@ 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->errinjct_token) {
+        ret = RTAS_OUT_TOKEN_OPENED;
+        goto out;
+    }
+
+    /* Grab random number as token */
+    spapr->errinjct_token = random();
+    if (spapr->errinjct_token == 0) {
+        ret = RTAS_OUT_BUSY;
+        goto out;
+    }
+
+    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->errinjct_token) {
+        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->errinjct_token = 0;
+    ret = RTAS_OUT_SUCCESS;
+out:
+    rtas_st(rets, 0, ret);
+}
+
 static struct rtas_call {
     const char *name;
     spapr_rtas_fn fn;
@@ -754,6 +821,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 b6cb0d0..30d9854 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -73,6 +73,9 @@ struct sPAPRMachineState {
     int htab_fd;
     bool htab_fd_stale;
 
+    /* Error injection token */
+    uint32_t errinjct_token;
+
     /* RTAS state */
     QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
 
@@ -412,6 +415,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 +460,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

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

[David Gibson]

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

On Mon, Aug 03, 2015 at 09:23:19AM +1000, 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.
> 
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_rtas.c    | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  9 ++++++-
>  2 files changed, 79 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index e99e25f..0a9c904 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -604,6 +604,73 @@ 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->errinjct_token) {
> +        ret = RTAS_OUT_TOKEN_OPENED;
> +        goto out;
> +    }
> +
> +    /* Grab random number as token */
> +    spapr->errinjct_token = random();

I don't quite understand the function of this token.   Using random()
seems a very, very odd way of doing things.  Is it supposed to be a
security thing?

> +    if (spapr->errinjct_token == 0) {
> +        ret = RTAS_OUT_BUSY;

AFAICT, this gives a 1 in RAND_MAX chance of returning RTAS_OUT_BUSY
for no particular reason.

> +        goto out;
> +    }
> +
> +    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->errinjct_token) {
> +        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->errinjct_token = 0;
> +    ret = RTAS_OUT_SUCCESS;
> +out:
> +    rtas_st(rets, 0, ret);
> +}
> +
>  static struct rtas_call {
>      const char *name;
>      spapr_rtas_fn fn;
> @@ -754,6 +821,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 b6cb0d0..30d9854 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -73,6 +73,9 @@ struct sPAPRMachineState {
>      int htab_fd;
>      bool htab_fd_stale;
>  
> +    /* Error injection token */
> +    uint32_t errinjct_token;
> +
>      /* RTAS state */
>      QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
>  
> @@ -412,6 +415,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 +460,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

-- 
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 RESEND v2 2/3] sPAPR: Support RTAS call ibm, {open, close}-errinjct

[Gavin Shan]

On Mon, Aug 03, 2015 at 12:51:09PM +1000, David Gibson wrote:
>On Mon, Aug 03, 2015 at 09:23:19AM +1000, 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.
>> 
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/spapr_rtas.c    | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/spapr.h |  9 ++++++-
>>  2 files changed, 79 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index e99e25f..0a9c904 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -604,6 +604,73 @@ 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->errinjct_token) {
>> +        ret = RTAS_OUT_TOKEN_OPENED;
>> +        goto out;
>> +    }
>> +
>> +    /* Grab random number as token */
>> +    spapr->errinjct_token = random();
>
>I don't quite understand the function of this token.   Using random()
>seems a very, very odd way of doing things.  Is it supposed to be a
>security thing?
>

Yes, the token is allocated by "ibm,open-errinjct". The token will be
passed to subsequent "ibm,errinjct" and "ibm,close-errinjct". From this
perspecitve, the token owner is allowed to do error injection and it's
for security. Apart from having random number as the token, is there
better (fast) way to produce it?

>> +    if (spapr->errinjct_token == 0) {
>> +        ret = RTAS_OUT_BUSY;
>
>AFAICT, this gives a 1 in RAND_MAX chance of returning RTAS_OUT_BUSY
>for no particular reason.
>

Yes, "0" represents invalid token (not opened). Maybe here we can retry
for a bit more like below. 0 returned from 10 successive random() would
be rare.

    uint32_t retries;

    while (!spapr->errinjct_token && retries++ < 10)
        spapr->errinjct_token = random();
    if (!spapr->errinjct_token) {
        ret = RTAS_OUT_BUSY;
        goto out;
    }


>> +        goto out;
>> +    }
>> +
>> +    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->errinjct_token) {
>> +        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->errinjct_token = 0;
>> +    ret = RTAS_OUT_SUCCESS;
>> +out:
>> +    rtas_st(rets, 0, ret);
>> +}
>> +
>>  static struct rtas_call {
>>      const char *name;
>>      spapr_rtas_fn fn;
>> @@ -754,6 +821,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 b6cb0d0..30d9854 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -73,6 +73,9 @@ struct sPAPRMachineState {
>>      int htab_fd;
>>      bool htab_fd_stale;
>>  
>> +    /* Error injection token */
>> +    uint32_t errinjct_token;
>> +
>>      /* RTAS state */
>>      QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
>>  
>> @@ -412,6 +415,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 +460,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

Thanks,
Gavin

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

[Alexey Kardashevskiy]

On 08/03/2015 01:32 PM, Gavin Shan wrote:
> On Mon, Aug 03, 2015 at 12:51:09PM +1000, David Gibson wrote:
>> On Mon, Aug 03, 2015 at 09:23:19AM +1000, 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.
>>>
>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>> ---
>>>   hw/ppc/spapr_rtas.c    | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   include/hw/ppc/spapr.h |  9 ++++++-
>>>   2 files changed, 79 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>> index e99e25f..0a9c904 100644
>>> --- a/hw/ppc/spapr_rtas.c
>>> +++ b/hw/ppc/spapr_rtas.c
>>> @@ -604,6 +604,73 @@ 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->errinjct_token) {
>>> +        ret = RTAS_OUT_TOKEN_OPENED;
>>> +        goto out;
>>> +    }
>>> +
>>> +    /* Grab random number as token */
>>> +    spapr->errinjct_token = random();
>>
>> I don't quite understand the function of this token.   Using random()
>> seems a very, very odd way of doing things.  Is it supposed to be a
>> security thing?
>>
>
> Yes, the token is allocated by "ibm,open-errinjct". The token will be
> passed to subsequent "ibm,errinjct" and "ibm,close-errinjct". From this
> perspecitve, the token owner is allowed to do error injection and it's
> for security. Apart from having random number as the token, is there
> better (fast) way to produce it?
>
>>> +    if (spapr->errinjct_token == 0) {
>>> +        ret = RTAS_OUT_BUSY;
>>
>> AFAICT, this gives a 1 in RAND_MAX chance of returning RTAS_OUT_BUSY
>> for no particular reason.
>>
>
> Yes, "0" represents invalid token (not opened). Maybe here we can retry
> for a bit more like below. 0 returned from 10 successive random() would
> be rare.
>
>      uint32_t retries;
>
>      while (!spapr->errinjct_token && retries++ < 10)
>          spapr->errinjct_token = random();
>      if (!spapr->errinjct_token) {
>          ret = RTAS_OUT_BUSY;
>          goto out;
>      }


No. QEMU is using rand() (not random()) and since it returns up to RAND_MAX 
which is 0x7fffffff, you could do something simple like this:

spapr->errinjct_token = (rand % 32767) + 1


But for debugging purposes it makes more sense just to initialize it to 1 
and then increment it in every call of rtas_ibm_open_errinjct().


>
>>> +        goto out;
>>> +    }
>>> +
>>> +    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->errinjct_token) {
>>> +        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->errinjct_token = 0;
>>> +    ret = RTAS_OUT_SUCCESS;
>>> +out:
>>> +    rtas_st(rets, 0, ret);
>>> +}
>>> +
>>>   static struct rtas_call {
>>>       const char *name;
>>>       spapr_rtas_fn fn;
>>> @@ -754,6 +821,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 b6cb0d0..30d9854 100644
>>> --- a/include/hw/ppc/spapr.h
>>> +++ b/include/hw/ppc/spapr.h
>>> @@ -73,6 +73,9 @@ struct sPAPRMachineState {
>>>       int htab_fd;
>>>       bool htab_fd_stale;
>>>
>>> +    /* Error injection token */
>>> +    uint32_t errinjct_token;
>>> +
>>>       /* RTAS state */
>>>       QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
>>>
>>> @@ -412,6 +415,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 +460,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
>
> Thanks,
> Gavin
>


-- 
Alexey

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

[Gavin Shan]

On Tue, Aug 04, 2015 at 02:49:14PM +1000, Alexey Kardashevskiy wrote:
>On 08/03/2015 01:32 PM, Gavin Shan wrote:
>>On Mon, Aug 03, 2015 at 12:51:09PM +1000, David Gibson wrote:
>>>On Mon, Aug 03, 2015 at 09:23:19AM +1000, 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.
>>>>
>>>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>>---
>>>>  hw/ppc/spapr_rtas.c    | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/hw/ppc/spapr.h |  9 ++++++-
>>>>  2 files changed, 79 insertions(+), 1 deletion(-)
>>>>
>>>>diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>>index e99e25f..0a9c904 100644
>>>>--- a/hw/ppc/spapr_rtas.c
>>>>+++ b/hw/ppc/spapr_rtas.c
>>>>@@ -604,6 +604,73 @@ 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->errinjct_token) {
>>>>+        ret = RTAS_OUT_TOKEN_OPENED;
>>>>+        goto out;
>>>>+    }
>>>>+
>>>>+    /* Grab random number as token */
>>>>+    spapr->errinjct_token = random();
>>>
>>>I don't quite understand the function of this token.   Using random()
>>>seems a very, very odd way of doing things.  Is it supposed to be a
>>>security thing?
>>>
>>
>>Yes, the token is allocated by "ibm,open-errinjct". The token will be
>>passed to subsequent "ibm,errinjct" and "ibm,close-errinjct". From this
>>perspecitve, the token owner is allowed to do error injection and it's
>>for security. Apart from having random number as the token, is there
>>better (fast) way to produce it?
>>
>>>>+    if (spapr->errinjct_token == 0) {
>>>>+        ret = RTAS_OUT_BUSY;
>>>
>>>AFAICT, this gives a 1 in RAND_MAX chance of returning RTAS_OUT_BUSY
>>>for no particular reason.
>>>
>>
>>Yes, "0" represents invalid token (not opened). Maybe here we can retry
>>for a bit more like below. 0 returned from 10 successive random() would
>>be rare.
>>
>>     uint32_t retries;
>>
>>     while (!spapr->errinjct_token && retries++ < 10)
>>         spapr->errinjct_token = random();
>>     if (!spapr->errinjct_token) {
>>         ret = RTAS_OUT_BUSY;
>>         goto out;
>>     }
>
>
>No. QEMU is using rand() (not random()) and since it returns up to RAND_MAX
>which is 0x7fffffff, you could do something simple like this:
>
>spapr->errinjct_token = (rand % 32767) + 1
>

Good idea. I'll have it in next revision.

Thanks,
Gavin

>
>But for debugging purposes it makes more sense just to initialize it to 1 and
>then increment it in every call of rtas_ibm_open_errinjct().
>
>
>>
>>>>+        goto out;
>>>>+    }
>>>>+
>>>>+    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->errinjct_token) {
>>>>+        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->errinjct_token = 0;
>>>>+    ret = RTAS_OUT_SUCCESS;
>>>>+out:
>>>>+    rtas_st(rets, 0, ret);
>>>>+}
>>>>+
>>>>  static struct rtas_call {
>>>>      const char *name;
>>>>      spapr_rtas_fn fn;
>>>>@@ -754,6 +821,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 b6cb0d0..30d9854 100644
>>>>--- a/include/hw/ppc/spapr.h
>>>>+++ b/include/hw/ppc/spapr.h
>>>>@@ -73,6 +73,9 @@ struct sPAPRMachineState {
>>>>      int htab_fd;
>>>>      bool htab_fd_stale;
>>>>
>>>>+    /* Error injection token */
>>>>+    uint32_t errinjct_token;
>>>>+
>>>>      /* RTAS state */
>>>>      QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
>>>>
>>>>@@ -412,6 +415,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 +460,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
>>
>>Thanks,
>>Gavin
>>
>
>
>-- 
>Alexey
>

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

[Alexey Kardashevskiy]

On 08/04/2015 05:16 PM, Gavin Shan wrote:
> On Tue, Aug 04, 2015 at 02:49:14PM +1000, Alexey Kardashevskiy wrote:
>> On 08/03/2015 01:32 PM, Gavin Shan wrote:
>>> On Mon, Aug 03, 2015 at 12:51:09PM +1000, David Gibson wrote:
>>>> On Mon, Aug 03, 2015 at 09:23:19AM +1000, 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.
>>>>>
>>>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>>> ---
>>>>>   hw/ppc/spapr_rtas.c    | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   include/hw/ppc/spapr.h |  9 ++++++-
>>>>>   2 files changed, 79 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>>> index e99e25f..0a9c904 100644
>>>>> --- a/hw/ppc/spapr_rtas.c
>>>>> +++ b/hw/ppc/spapr_rtas.c
>>>>> @@ -604,6 +604,73 @@ 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->errinjct_token) {
>>>>> +        ret = RTAS_OUT_TOKEN_OPENED;
>>>>> +        goto out;
>>>>> +    }
>>>>> +
>>>>> +    /* Grab random number as token */
>>>>> +    spapr->errinjct_token = random();
>>>>
>>>> I don't quite understand the function of this token.   Using random()
>>>> seems a very, very odd way of doing things.  Is it supposed to be a
>>>> security thing?
>>>>
>>>
>>> Yes, the token is allocated by "ibm,open-errinjct". The token will be
>>> passed to subsequent "ibm,errinjct" and "ibm,close-errinjct". From this
>>> perspecitve, the token owner is allowed to do error injection and it's
>>> for security. Apart from having random number as the token, is there
>>> better (fast) way to produce it?
>>>
>>>>> +    if (spapr->errinjct_token == 0) {
>>>>> +        ret = RTAS_OUT_BUSY;
>>>>
>>>> AFAICT, this gives a 1 in RAND_MAX chance of returning RTAS_OUT_BUSY
>>>> for no particular reason.
>>>>
>>>
>>> Yes, "0" represents invalid token (not opened). Maybe here we can retry
>>> for a bit more like below. 0 returned from 10 successive random() would
>>> be rare.
>>>
>>>      uint32_t retries;
>>>
>>>      while (!spapr->errinjct_token && retries++ < 10)
>>>          spapr->errinjct_token = random();
>>>      if (!spapr->errinjct_token) {
>>>          ret = RTAS_OUT_BUSY;
>>>          goto out;
>>>      }
>>
>>
>> No. QEMU is using rand() (not random()) and since it returns up to RAND_MAX
>> which is 0x7fffffff, you could do something simple like this:
>>
>> spapr->errinjct_token = (rand % 32767) + 1
>>
>
> Good idea. I'll have it in next revision.
>
> Thanks,
> Gavin
>
>>
>> But for debugging purposes it makes more sense just to initialize it to 1 and
>> then increment it in every call of rtas_ibm_open_errinjct().


Why rand() and not this? You do not protect against a guest attack by 
limiting a number of the rtas calls so the token just needs to be unique 
and that's it, and later in gdb is is going to be easier to trace these 
tokens if need for this ever arises.



-- 
Alexey

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

[Gavin Shan]

On Tue, Aug 04, 2015 at 05:23:30PM +1000, Alexey Kardashevskiy wrote:
>On 08/04/2015 05:16 PM, Gavin Shan wrote:
>>On Tue, Aug 04, 2015 at 02:49:14PM +1000, Alexey Kardashevskiy wrote:
>>>On 08/03/2015 01:32 PM, Gavin Shan wrote:
>>>>On Mon, Aug 03, 2015 at 12:51:09PM +1000, David Gibson wrote:
>>>>>On Mon, Aug 03, 2015 at 09:23:19AM +1000, 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.
>>>>>>
>>>>>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>>>>---
>>>>>>  hw/ppc/spapr_rtas.c    | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  include/hw/ppc/spapr.h |  9 ++++++-
>>>>>>  2 files changed, 79 insertions(+), 1 deletion(-)
>>>>>>
>>>>>>diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>>>>index e99e25f..0a9c904 100644
>>>>>>--- a/hw/ppc/spapr_rtas.c
>>>>>>+++ b/hw/ppc/spapr_rtas.c
>>>>>>@@ -604,6 +604,73 @@ 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->errinjct_token) {
>>>>>>+        ret = RTAS_OUT_TOKEN_OPENED;
>>>>>>+        goto out;
>>>>>>+    }
>>>>>>+
>>>>>>+    /* Grab random number as token */
>>>>>>+    spapr->errinjct_token = random();
>>>>>
>>>>>I don't quite understand the function of this token.   Using random()
>>>>>seems a very, very odd way of doing things.  Is it supposed to be a
>>>>>security thing?
>>>>>
>>>>
>>>>Yes, the token is allocated by "ibm,open-errinjct". The token will be
>>>>passed to subsequent "ibm,errinjct" and "ibm,close-errinjct". From this
>>>>perspecitve, the token owner is allowed to do error injection and it's
>>>>for security. Apart from having random number as the token, is there
>>>>better (fast) way to produce it?
>>>>
>>>>>>+    if (spapr->errinjct_token == 0) {
>>>>>>+        ret = RTAS_OUT_BUSY;
>>>>>
>>>>>AFAICT, this gives a 1 in RAND_MAX chance of returning RTAS_OUT_BUSY
>>>>>for no particular reason.
>>>>>
>>>>
>>>>Yes, "0" represents invalid token (not opened). Maybe here we can retry
>>>>for a bit more like below. 0 returned from 10 successive random() would
>>>>be rare.
>>>>
>>>>     uint32_t retries;
>>>>
>>>>     while (!spapr->errinjct_token && retries++ < 10)
>>>>         spapr->errinjct_token = random();
>>>>     if (!spapr->errinjct_token) {
>>>>         ret = RTAS_OUT_BUSY;
>>>>         goto out;
>>>>     }
>>>
>>>
>>>No. QEMU is using rand() (not random()) and since it returns up to RAND_MAX
>>>which is 0x7fffffff, you could do something simple like this:
>>>
>>>spapr->errinjct_token = (rand % 32767) + 1
>>>
>>
>>Good idea. I'll have it in next revision.
>>
>>Thanks,
>>Gavin
>>
>>>
>>>But for debugging purposes it makes more sense just to initialize it to 1 and
>>>then increment it in every call of rtas_ibm_open_errinjct().
>
>
>Why rand() and not this? You do not protect against a guest attack by
>limiting a number of the rtas calls so the token just needs to be unique and
>that's it, and later in gdb is is going to be easier to trace these tokens if
>need for this ever arises.
>

When calling rtas_ibm_close_errinjct(), the token (spapr->errinjct_token)
will be zero'ed to indicate: the token has been closed. Alternatively, one
statistics can be added if it's not expensive. However, I don't understand
why we need trace the number of error injections that was ever raised. Could
you please share the purpose about that?

Thanks,
Gavin

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

[David Gibson]

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

On Tue, Aug 04, 2015 at 08:55:29PM +1000, Gavin Shan wrote:
> On Tue, Aug 04, 2015 at 05:23:30PM +1000, Alexey Kardashevskiy wrote:
> >On 08/04/2015 05:16 PM, Gavin Shan wrote:
> >>On Tue, Aug 04, 2015 at 02:49:14PM +1000, Alexey Kardashevskiy wrote:
> >>>On 08/03/2015 01:32 PM, Gavin Shan wrote:
> >>>>On Mon, Aug 03, 2015 at 12:51:09PM +1000, David Gibson wrote:
> >>>>>On Mon, Aug 03, 2015 at 09:23:19AM +1000, 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.
> >>>>>>
> >>>>>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> >>>>>>---
> >>>>>>  hw/ppc/spapr_rtas.c    | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>  include/hw/ppc/spapr.h |  9 ++++++-
> >>>>>>  2 files changed, 79 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>>diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> >>>>>>index e99e25f..0a9c904 100644
> >>>>>>--- a/hw/ppc/spapr_rtas.c
> >>>>>>+++ b/hw/ppc/spapr_rtas.c
> >>>>>>@@ -604,6 +604,73 @@ 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->errinjct_token) {
> >>>>>>+        ret = RTAS_OUT_TOKEN_OPENED;
> >>>>>>+        goto out;
> >>>>>>+    }
> >>>>>>+
> >>>>>>+    /* Grab random number as token */
> >>>>>>+    spapr->errinjct_token = random();
> >>>>>
> >>>>>I don't quite understand the function of this token.   Using random()
> >>>>>seems a very, very odd way of doing things.  Is it supposed to be a
> >>>>>security thing?
> >>>>>
> >>>>
> >>>>Yes, the token is allocated by "ibm,open-errinjct". The token will be
> >>>>passed to subsequent "ibm,errinjct" and "ibm,close-errinjct". From this
> >>>>perspecitve, the token owner is allowed to do error injection and it's
> >>>>for security. Apart from having random number as the token, is there
> >>>>better (fast) way to produce it?
> >>>>
> >>>>>>+    if (spapr->errinjct_token == 0) {
> >>>>>>+        ret = RTAS_OUT_BUSY;
> >>>>>
> >>>>>AFAICT, this gives a 1 in RAND_MAX chance of returning RTAS_OUT_BUSY
> >>>>>for no particular reason.
> >>>>>
> >>>>
> >>>>Yes, "0" represents invalid token (not opened). Maybe here we can retry
> >>>>for a bit more like below. 0 returned from 10 successive random() would
> >>>>be rare.
> >>>>
> >>>>     uint32_t retries;
> >>>>
> >>>>     while (!spapr->errinjct_token && retries++ < 10)
> >>>>         spapr->errinjct_token = random();
> >>>>     if (!spapr->errinjct_token) {
> >>>>         ret = RTAS_OUT_BUSY;
> >>>>         goto out;
> >>>>     }
> >>>
> >>>
> >>>No. QEMU is using rand() (not random()) and since it returns up to RAND_MAX
> >>>which is 0x7fffffff, you could do something simple like this:
> >>>
> >>>spapr->errinjct_token = (rand % 32767) + 1
> >>>
> >>
> >>Good idea. I'll have it in next revision.
> >>
> >>Thanks,
> >>Gavin
> >>
> >>>
> >>>But for debugging purposes it makes more sense just to initialize it to 1 and
> >>>then increment it in every call of rtas_ibm_open_errinjct().
> >
> >
> >Why rand() and not this? You do not protect against a guest attack by
> >limiting a number of the rtas calls so the token just needs to be unique and
> >that's it, and later in gdb is is going to be easier to trace these tokens if
> >need for this ever arises.
> >
> 
> When calling rtas_ibm_close_errinjct(), the token (spapr->errinjct_token)
> will be zero'ed to indicate: the token has been closed. Alternatively, one
> statistics can be added if it's not expensive. However, I don't understand
> why we need trace the number of error injections that was ever raised. Could
> you please share the purpose about that?

I understand that, but using a token from random() just doesn't make
sense.

1) If this is just to prevent accidental multiple users of the device,
then an incrementing counter is simpler, easier to understand and just
as good.

2) If this is supposed to securely prevent other users from
controlling device then a) there's no point, it requires privilege in
the guest anyway, and b) you'd have to use a secure random number
source, not a pseudo-rng like random().

Oh, also, you haven't added the token to the migration stream, which
means a migration would silently close the token.

-- 
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 --]

[Qemu-devel] [PATCH RESEND v2 3/3] 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          | 42 ++++++++++++++++++++++
 hw/ppc/spapr_pci_vfio.c     | 56 +++++++++++++++++++++++++++++
 hw/ppc/spapr_rtas.c         | 85 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/pci-host/spapr.h |  2 ++
 include/hw/ppc/spapr.h      | 28 ++++++++++++++-
 5 files changed, 212 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index cfd3b7b..fb03c3a 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -682,6 +682,48 @@ 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;
+    int ret;
+
+    if (is_64bits) {
+        addr = rtas_ldq(param_buf, 0);
+        mask = rtas_ldq(param_buf, 1);
+        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 */
+    ret = spc->eeh_inject_error(sphb, func, addr, mask, is_64bits);
+    if (ret < 0) {
+        return RTAS_OUT_HW_ERROR;
+    }
+
+    return RTAS_OUT_SUCCESS;
+}
+
 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 0a9c904..d6894ee 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -637,6 +637,53 @@ 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->errinjct_token != open_token) {
+        ret = RTAS_OUT_TOKEN_OPENED;
+        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,
@@ -728,6 +775,42 @@ 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 char *tokens[] = {
+        "fatal",
+        "recovered-random-event",
+        "recovered-special-event",
+        "corrupted-page",
+        "corrupted-slb",
+        "translator-failure",
+        "ioa-bus-error",
+        "ioa-bus-error-64",
+        "platform-specific",
+        "corrupted-dcache-start",
+        "corrupted-dcache-end",
+        "corrupted-icache-start",
+        "corrupted-icache-end",
+        "corrupted-tlb-start",
+        "corrupted-tlb-end"
+    };
+
+    /* ibm,errinjct-tokens */
+    offset = 0;
+    for (i = 0; i < ARRAY_SIZE(tokens); i++) {
+        offset += sprintf(errinjct_tokens + offset, "%s", tokens[i]);
+        errinjct_tokens[offset++] = '\0';
+        *(int *)(&errinjct_tokens[offset]) = i+1;
+        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) {
@@ -823,6 +906,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 30d9854..e3135e3 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -408,6 +408,24 @@ 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_FATAL                1
+#define RTAS_ERRINJCT_TYPE_RANDOM_EVENT         2
+#define RTAS_ERRINJCT_TYPE_SPECIAL_EVENT        3
+#define RTAS_ERRINJCT_TYPE_CORRUPTED_PAGE       4
+#define RTAS_ERRINJCT_TYPE_CORRUPTED_SLB        5
+#define RTAS_ERRINJCT_TYPE_TRANSLATOR_FAILURE   6
+#define RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR        7
+#define RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64      8
+#define RTAS_ERRINJCT_TYPE_PLATFORM_SPECIFIC    9
+#define RTAS_ERRINJCT_TYPE_DCACHE_START         10
+#define RTAS_ERRINJCT_TYPE_DCACHE_END           11
+#define RTAS_ERRINJCT_TYPE_ICACHE_START         12
+#define RTAS_ERRINJCT_TYPE_ICACHE_END           13
+#define RTAS_ERRINJCT_TYPE_TLB_START            14
+#define RTAS_ERRINJCT_TYPE_TLB_END              15
+#define RTAS_ERRINJCT_TYPE_UPSTREAM_IO_ERROR    16
+
 /* RTAS return codes */
 #define RTAS_OUT_SUCCESS            0
 #define RTAS_OUT_NO_ERRORS_FOUND    1
@@ -462,8 +480,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
@@ -499,6 +518,11 @@ static inline uint32_t rtas_ld(target_ulong phys, int n)
     return ldl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4*n));
 }
 
+static inline uint64_t rtas_ldq(target_ulong phys, int n)
+{
+    return ldq_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 8*n));
+}
+
 static inline void rtas_st(target_ulong phys, int n, uint32_t val)
 {
     stl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4*n), val);
@@ -595,6 +619,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_event(sPAPRDRConnector *drc);
 void spapr_hotplug_req_remove_event(sPAPRDRConnector *drc);
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH RESEND v2 3/3] sPAPR: Support RTAS call ibm, errinjct

[David Gibson]

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

On Mon, Aug 03, 2015 at 09:23:20AM +1000, 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          | 42 ++++++++++++++++++++++
>  hw/ppc/spapr_pci_vfio.c     | 56 +++++++++++++++++++++++++++++
>  hw/ppc/spapr_rtas.c         | 85 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci-host/spapr.h |  2 ++
>  include/hw/ppc/spapr.h      | 28 ++++++++++++++-
>  5 files changed, 212 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index cfd3b7b..fb03c3a 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -682,6 +682,48 @@ 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;
> +    int ret;
> +
> +    if (is_64bits) {
> +        addr = rtas_ldq(param_buf, 0);
> +        mask = rtas_ldq(param_buf, 1);
> +        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 */
> +    ret = spc->eeh_inject_error(sphb, func, addr, mask, is_64bits);
> +    if (ret < 0) {
> +        return RTAS_OUT_HW_ERROR;

Your eeh_inject_error callback below returns rtas error codes, which
here you ignore and always return RTAS_OUT_HW_ERROR.

> +    }
> +
> +    return RTAS_OUT_SUCCESS;
> +}
> +
>  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 0a9c904..d6894ee 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -637,6 +637,53 @@ 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->errinjct_token != open_token) {
> +        ret = RTAS_OUT_TOKEN_OPENED;

In the open function this error code indicates that the token is
already opened, here it could indicate that it's not opened (or that
you have the wrong one).

> +        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,
> @@ -728,6 +775,42 @@ 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 char *tokens[] = {
> +        "fatal",
> +        "recovered-random-event",
> +        "recovered-special-event",
> +        "corrupted-page",
> +        "corrupted-slb",
> +        "translator-failure",
> +        "ioa-bus-error",
> +        "ioa-bus-error-64",
> +        "platform-specific",
> +        "corrupted-dcache-start",
> +        "corrupted-dcache-end",
> +        "corrupted-icache-start",
> +        "corrupted-icache-end",
> +        "corrupted-tlb-start",
> +        "corrupted-tlb-end"

You list all these tokens despite the fact you only support two of them.

This also silently depends on the fact that this list appears int the
same order as your #defined token values, which is a non-obvious
dependency I'd prefer to avoid.

> +    };
> +
> +    /* ibm,errinjct-tokens */
> +    offset = 0;
> +    for (i = 0; i < ARRAY_SIZE(tokens); i++) {
> +        offset += sprintf(errinjct_tokens + offset, "%s", tokens[i]);
> +        errinjct_tokens[offset++] = '\0';
> +        *(int *)(&errinjct_tokens[offset]) = i+1;
> +        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) {
> @@ -823,6 +906,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 30d9854..e3135e3 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -408,6 +408,24 @@ 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_FATAL                1
> +#define RTAS_ERRINJCT_TYPE_RANDOM_EVENT         2
> +#define RTAS_ERRINJCT_TYPE_SPECIAL_EVENT        3
> +#define RTAS_ERRINJCT_TYPE_CORRUPTED_PAGE       4
> +#define RTAS_ERRINJCT_TYPE_CORRUPTED_SLB        5
> +#define RTAS_ERRINJCT_TYPE_TRANSLATOR_FAILURE   6
> +#define RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR        7
> +#define RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64      8
> +#define RTAS_ERRINJCT_TYPE_PLATFORM_SPECIFIC    9
> +#define RTAS_ERRINJCT_TYPE_DCACHE_START         10
> +#define RTAS_ERRINJCT_TYPE_DCACHE_END           11
> +#define RTAS_ERRINJCT_TYPE_ICACHE_START         12
> +#define RTAS_ERRINJCT_TYPE_ICACHE_END           13
> +#define RTAS_ERRINJCT_TYPE_TLB_START            14
> +#define RTAS_ERRINJCT_TYPE_TLB_END              15
> +#define RTAS_ERRINJCT_TYPE_UPSTREAM_IO_ERROR    16
> +
>  /* RTAS return codes */
>  #define RTAS_OUT_SUCCESS            0
>  #define RTAS_OUT_NO_ERRORS_FOUND    1
> @@ -462,8 +480,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
> @@ -499,6 +518,11 @@ static inline uint32_t rtas_ld(target_ulong phys, int n)
>      return ldl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4*n));
>  }
>  
> +static inline uint64_t rtas_ldq(target_ulong phys, int n)
> +{
> +    return ldq_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 8*n));
> +}
> +

I don't much like this.  The rtas_ld() function is really designed to
be used for direct rtas parameters, nothing else.  This is only used
for another buffer.  Further, the index used here is different from
rtas_ld() because it's counted in 8-byte rather than 4-byte
increments, which is potentially confusing.

It might make more sense to do the conversion from a guest physical
addr to a qemu memory pointer in the top level rtas function, and just
pass the pointer through to the other layers.

>  static inline void rtas_st(target_ulong phys, int n, uint32_t val)
>  {
>      stl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4*n), val);
> @@ -595,6 +619,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_event(sPAPRDRConnector *drc);
>  void spapr_hotplug_req_remove_event(sPAPRDRConnector *drc);
>  

-- 
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 RESEND v2 3/3] sPAPR: Support RTAS call ibm, errinjct

[Gavin Shan]

On Mon, Aug 03, 2015 at 01:01:50PM +1000, David Gibson wrote:
>On Mon, Aug 03, 2015 at 09:23:20AM +1000, 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          | 42 ++++++++++++++++++++++
>>  hw/ppc/spapr_pci_vfio.c     | 56 +++++++++++++++++++++++++++++
>>  hw/ppc/spapr_rtas.c         | 85 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/pci-host/spapr.h |  2 ++
>>  include/hw/ppc/spapr.h      | 28 ++++++++++++++-
>>  5 files changed, 212 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index cfd3b7b..fb03c3a 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -682,6 +682,48 @@ 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;
>> +    int ret;
>> +
>> +    if (is_64bits) {
>> +        addr = rtas_ldq(param_buf, 0);
>> +        mask = rtas_ldq(param_buf, 1);
>> +        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 */
>> +    ret = spc->eeh_inject_error(sphb, func, addr, mask, is_64bits);
>> +    if (ret < 0) {
>> +        return RTAS_OUT_HW_ERROR;
>
>Your eeh_inject_error callback below returns rtas error codes, which
>here you ignore and always return RTAS_OUT_HW_ERROR.
>

Yeah, I'll fix in next revision.

>> +    }
>> +
>> +    return RTAS_OUT_SUCCESS;
>> +}
>> +
>>  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 0a9c904..d6894ee 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -637,6 +637,53 @@ 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->errinjct_token != open_token) {
>> +        ret = RTAS_OUT_TOKEN_OPENED;
>
>In the open function this error code indicates that the token is
>already opened, here it could indicate that it's not opened (or that
>you have the wrong one).
>

Yes, the correct errcode here should be 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,
>> @@ -728,6 +775,42 @@ 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 char *tokens[] = {
>> +        "fatal",
>> +        "recovered-random-event",
>> +        "recovered-special-event",
>> +        "corrupted-page",
>> +        "corrupted-slb",
>> +        "translator-failure",
>> +        "ioa-bus-error",
>> +        "ioa-bus-error-64",
>> +        "platform-specific",
>> +        "corrupted-dcache-start",
>> +        "corrupted-dcache-end",
>> +        "corrupted-icache-start",
>> +        "corrupted-icache-end",
>> +        "corrupted-tlb-start",
>> +        "corrupted-tlb-end"
>
>You list all these tokens despite the fact you only support two of them.
>
>This also silently depends on the fact that this list appears int the
>same order as your #defined token values, which is a non-obvious
>dependency I'd prefer to avoid.
>

Ok. I'll introduce two arrays as below. Then I don't need expose
those options that are not supported yet and make the dependencies
obvious:

    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", tokens[i]);
>> +        errinjct_tokens[offset++] = '\0';
>> +        *(int *)(&errinjct_tokens[offset]) = i+1;
>> +        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) {
>> @@ -823,6 +906,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 30d9854..e3135e3 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -408,6 +408,24 @@ 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_FATAL                1
>> +#define RTAS_ERRINJCT_TYPE_RANDOM_EVENT         2
>> +#define RTAS_ERRINJCT_TYPE_SPECIAL_EVENT        3
>> +#define RTAS_ERRINJCT_TYPE_CORRUPTED_PAGE       4
>> +#define RTAS_ERRINJCT_TYPE_CORRUPTED_SLB        5
>> +#define RTAS_ERRINJCT_TYPE_TRANSLATOR_FAILURE   6
>> +#define RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR        7
>> +#define RTAS_ERRINJCT_TYPE_IOA_BUS_ERROR64      8
>> +#define RTAS_ERRINJCT_TYPE_PLATFORM_SPECIFIC    9
>> +#define RTAS_ERRINJCT_TYPE_DCACHE_START         10
>> +#define RTAS_ERRINJCT_TYPE_DCACHE_END           11
>> +#define RTAS_ERRINJCT_TYPE_ICACHE_START         12
>> +#define RTAS_ERRINJCT_TYPE_ICACHE_END           13
>> +#define RTAS_ERRINJCT_TYPE_TLB_START            14
>> +#define RTAS_ERRINJCT_TYPE_TLB_END              15
>> +#define RTAS_ERRINJCT_TYPE_UPSTREAM_IO_ERROR    16
>> +

I'll drop those options that aren't supported by this patchset.

>>  /* RTAS return codes */
>>  #define RTAS_OUT_SUCCESS            0
>>  #define RTAS_OUT_NO_ERRORS_FOUND    1
>> @@ -462,8 +480,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
>> @@ -499,6 +518,11 @@ static inline uint32_t rtas_ld(target_ulong phys, int n)
>>      return ldl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4*n));
>>  }
>>  
>> +static inline uint64_t rtas_ldq(target_ulong phys, int n)
>> +{
>> +    return ldq_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 8*n));
>> +}
>> +
>
>I don't much like this.  The rtas_ld() function is really designed to
>be used for direct rtas parameters, nothing else.  This is only used
>for another buffer.  Further, the index used here is different from
>rtas_ld() because it's counted in 8-byte rather than 4-byte
>increments, which is potentially confusing.
>
>It might make more sense to do the conversion from a guest physical
>addr to a qemu memory pointer in the top level rtas function, and just
>pass the pointer through to the other layers.
>

Ok. I'll drop rtas_ldq() and use rtas_ld() instead. Note rtas_ldq() was
used to grab the 64-bits address and mask, which are part of the PCI
error injection paramters in spapr_rtas_errinjct_ioa(). After replacing
rtas_ldq() with rtas_ld(), the code will be something like below:

    uint64_t addr, mask;

    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);
    }

>>  static inline void rtas_st(target_ulong phys, int n, uint32_t val)
>>  {
>>      stl_be_phys(&address_space_memory, ppc64_phys_to_real(phys + 4*n), val);
>> @@ -595,6 +619,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_event(sPAPRDRConnector *drc);
>>  void spapr_hotplug_req_remove_event(sPAPRDRConnector *drc);
>>  

Thanks,
Gavin