Re: [PATCH 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers

[Julien Grall <julien.grall.oss@gmail.com>]

Hi Ian,

On 16/04/2015 17:22, Ian Campbell wrote:
> On Fri, 2015-04-03 at 14:51 +0200, Julien Grall wrote:
>> On 31/03/2015 12:07, Ian Campbell wrote:
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>>    xen/arch/arm/traps.c |   52 ++++++++++++++++++++++++++++++++------------------
>>>    1 file changed, 33 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index 8b1846a..ebc09f9 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -1587,6 +1587,34 @@ static void handle_raz_wi(struct cpu_user_regs *regs,
>>>        advance_pc(regs, hsr);
>>>    }
>>>
>>> +/* Write only + write ignore */
>>> +static void handle_wo_wi(struct cpu_user_regs *regs,
>>> +                         register_t *reg,
>>
>> This helper handles WO and WI which doesn't require to modify the register.
>>
>> I would pass a register_t rather than register_t* in order to make clear
>> that the register of the guest won't change.
>
> I deliberately made handle_* all have the same prototype for
> consistency.

Ok.


>>> +/* Read only + read as zero */
>>
>> This comment may confuse developer who wants to implement RO register
>> which another value than 0.
>>
>> I got confuse too. It would be nice to expand the comment for the RO case.
>
> I'm afraid I don't understand the confusion so I'm not sure how to
> clarify. What did you think this comment was saying?

When I read the comment I though you were implemented two distinct part:
RO and RAZ.

As the value return by RO may not always be 0 (we have a handful of 
cases in traps.c), I didn't understand why you were implementing both 
within the same helper.

Although this helper choose to implement RO as RAZ. So I think it would 
be good to mention it in order to avoid confusion later.

Regards,

-- 
Julien Grall