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

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Sat, Aug 08, 2015 at 02:04:17AM +1000, Alexey Kardashevskiy wrote:
>On 08/07/2015 01:33 PM, 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.c         |  5 ++++
>>  hw/ppc/spapr_rtas.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/spapr.h | 10 +++++++-
>>  3 files changed, 80 insertions(+), 1 deletion(-)
>>
>>diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>index dfd808f..1ebd0b2 100644
>>--- a/hw/ppc/spapr.c
>>+++ b/hw/ppc/spapr.c
>>@@ -1225,6 +1225,11 @@ 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_BOOL(is_errinjct_opened, sPAPRMachineState),
>>+        VMSTATE_UINT32(errinjct_next_token, sPAPRMachineState),
>>+
>>          VMSTATE_END_OF_LIST()
>>      },
>>  };
>>diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>index e99e25f..f1b92ca 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_next_token++);
>
>
>If you did "++spapr->errinjct_next_token" (or just moved to a separate line
>before rtas_st())...
>
>
>
>>+    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_next_token != (open_token + 1)) {
>
>
>... you would not need "+ 1" here.
>

Ok. Then it should be renamed to "errinject_current_token", or just
"errinjct_token". I prefer later one. In that case, the token will
start from 1, not 0.

Thanks,
Gavin

>
>>+        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 b6cb0d0..8ecf127 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_next_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
>>
>
>
>-- 
>Alexey
>