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 <address@hidden>
>>>>---
>>>>  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
>