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

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

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

Yes, it's what Alexey suggested.

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

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

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

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

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

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

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

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

Thanks,
Gavin