Re: [Qemu-devel] [Qemu-ppc] [PATCH v6 7/8] sPAPR: Support RTAS call ibm, {open, close}-errinjct

[Gavin Shan]

On Tue, Aug 25, 2015 at 04:07:35PM +0200, Thomas Huth wrote:
>On 24/08/15 14:03, Gavin Shan wrote:
>> This 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 sequential number for that.
>> It's notable that the least bit of the token is reserved to indicate
>> if the token has been opened, meaning the valid token should be always
>> odd.
>> 
>> Signed-off-by: Gavin Shan <address@hidden>
>> ---
>>  hw/ppc/spapr.c         |  6 ++++-
>>  hw/ppc/spapr_rtas.c    | 60 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/spapr.h |  9 +++++++-
>>  3 files changed, 73 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),
>> +
>>          VMSTATE_END_OF_LIST()
>>      },
>>  };
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index e99e25f..64924c6 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -604,6 +604,62 @@ 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 & 1) {
>> +        ret = RTAS_OUT_TOKEN_OPENED;
>> +        goto out;
>> +    }
>> +
>> +    /* Grab the token */
>> +    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;
>> +    }
>> +
>> +    /* Match with the passed token */
>> +    open_token = rtas_ld(args, 0);
>> +    if (!(spapr->errinjct_token & 1) ||
>> +        spapr->errinjct_token != open_token) {
>> +        ret = RTAS_OUT_CLOSE_ERROR;
>> +        goto out;
>> +    }
>> +
>> +    spapr->errinjct_token++;
>> +    ret = RTAS_OUT_SUCCESS;
>> +out:
>> +    rtas_st(rets, 0, ret);
>> +}
>
>This basically now looks fine to me! I am just wondering what happens
>when the guest opens a token, but then resets the system before it
>closes it again?
>I.e. shouldn't the errinjct_token be set to 0 back again during the
>reset handler?
>

Good point! It should be resetted to 0. If the token is already opened
and then go through guest reboot, the user won't get valid token again
after the system is up. I'll fix it in hw/ppc/spapr.c::ppc_spapr_reset()
in next revision.

Thanks,
Gavin