From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall 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 18:50:59 +0000 Message-ID: <54BEA393.5060207@linaro.org> 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> <1421776670.10440.330.camel@citrix.com> 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 1YDdtg-000529-SB for xen-devel@lists.xenproject.org; Tue, 20 Jan 2015 18:51:29 +0000 Received: by mail-wi0-f182.google.com with SMTP id n3so25043726wiv.3 for ; Tue, 20 Jan 2015 10:51:27 -0800 (PST) In-Reply-To: <1421776670.10440.330.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: xen-devel@lists.xenproject.org, tim@xen.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org 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