From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH 04/10] xen/arm: vgic-v3: Don't check the size when we ignore the write/read as zero Date: Tue, 20 Jan 2015 17:57:50 +0000 Message-ID: <1421776670.10440.330.camel@citrix.com> References: <1421684957-29884-1-git-send-email-julien.grall@linaro.org> <1421684957-29884-5-git-send-email-julien.grall@linaro.org> <1421769455.10440.306.camel@citrix.com> <54BE9341.7010307@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YDdec-0003st-86 for xen-devel@lists.xenproject.org; Tue, 20 Jan 2015 18:35:54 +0000 In-Reply-To: <54BE9341.7010307@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: xen-devel@lists.xenproject.org, tim@xen.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org 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.