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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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).

> 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.

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.

Ian.


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