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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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