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

[Gavin Shan]

On Wed, Aug 19, 2015 at 09:15:26AM -0700, Thomas Huth wrote:
>On 18/08/15 18:15, David Gibson wrote:
>> On Wed, Aug 19, 2015 at 09:52:00AM +1000, Gavin Shan wrote:
>>> 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.
>
>Then please update the commit message accordingly.
>

Yes, I'll update changelog accordingly.

>>>>> 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 :-)
>> 
>> I'd prefer you didn't.  Unlike Thomas, I also don't remember C order
>> of ops that well and would prefer the clarity.
>
>You can always look it up if you're unsure, e.g.:
>
>http://en.cppreference.com/w/c/language/operator_precedence
>
>And once you've learnt it, the additional paranthesis just look
>cumbersome. So please remove them!
>

Ok. I'll check the code and remove unnecessary paranthesis in next revision.

Thanks,
Gavin