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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 20/01/15 17:57, Ian Campbell wrote:
> On Tue, 2015-01-20 at 17:41 +0000, Julien Grall wrote:
>> 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.
> 
> 5.1.3 of the GICv3 spec defines it as UNPREDICATABLE (and also lists all
> the valid options for each register).

I saw it after sending the mail.

>> 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.
> 
> If the driver is making a legitimate access (i.e. correct size etc) then
> we shouldn't be logging, irrespective of whether we are implementing as
> RAZ/WI or anything else.

I agree for RAZ, but WI would mean something will goes wrong. For
instance if the guest is trying to set a bit to 1, while the bit should
be 0.

So I'd like to keep at least a debug message for WI. It's could be very
helpful for the developper.

> Given the contents of 5.1.3 I could perhaps be convinced accept making
> the bad_width: behaviour less catastrophic to the guest, but killing the
> guest meets the defintion of UNPREDICTABLE I think and in general not
> letting guests take unexpect advantage of odd corner cases is a good
> idea IMHO, lest they come to rely on something.
> 
>>> 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.
> 
> Right, so the decoding switch would need to use the right goto for its
> case.

As the spec defines the access to be UNPREDICATABLE, I would rather
prefer to not decode the size and directly read as zero/write ignore.

I would end up to a smaller code and more comprehensible one.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel