Re: [PATCH 04/10] xen/arm: vgic-v3: Don't check the size when we ignore the write/read as zero

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 20/01/15 15:57, Ian Campbell wrote:
> On Mon, 2015-01-19 at 16:29 +0000, Julien Grall wrote:
>> In general, it's not necessary/important to check the size.
> 
> Only if the docs say this register can be accessed by a partial
> read/write, or if it is implementation defined what the result would be
> (and RAZ/WI is within the set of allowable actions).
> 
> Do you have a reference for the behaviour of GICR accesses which aren't
> of the register's natural size?

It's clearly specify in the spec if the register can be accessed with a
non-natural size.

AFAICT, the spec doesn't give a specific behavior if the register
doesn't support byte/word/double word access.

IHMO, for RAZ register we can safely accept any kind of access as the
final register will in fine always contains 0.

That will also any issue with WI/RAZ register because we may have miss a
line in the spec stating a register is byte and word accessible. (see
the case of vgic-v2).

>>  It's better
>> to log it to let know the guest that its access will have no effect.
>>
>> Note: On debug build it may happen to see some of these messages during
>> domain boot.
> 
> We should only print if the guest has done something wrong, and reading
> a RAZ register (or one which we have implemented that way) is not
> inherently wrong.

That's why it's log in DEBUG and not in ERR. Although, I agree that on
read it's not important. But on write it's help the developer to
understand which his GIC driver is not working.

> IOW read_as_zero* should be silent, and a different code path used for
> "guest did something wrong".
>
> IOW I think the current distinction between bad_width and read_as_zero*
> is correct currently, although perhaps the goto's which target them need
> adjusting in some cases.
> 
> Perhaps you want to add a read_as_zero_32 which has the check, making
> read_as_zero accept either 32- or 64-bit accesses, and goto the correct
> label for each register?

It's register defined which access is allowed for a register.

Regards,

-- 
Julien Grall