From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v4 2/5] xen/arm: vgic-v2: Don't ignore a write in ITARGETSR if one field is 0 Date: Fri, 23 Oct 2015 10:30:15 +0100 Message-ID: <1445592615.2374.80.camel@citrix.com> References: <1444659760-24123-1-git-send-email-julien.grall@citrix.com> <1444659760-24123-3-git-send-email-julien.grall@citrix.com> <1445530071.2374.40.camel@citrix.com> <56291414.8030007@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZpYg4-0000vu-Tk for xen-devel@lists.xenproject.org; Fri, 23 Oct 2015 09:30:25 +0000 In-Reply-To: <56291414.8030007@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: Julien Grall , xen-devel@lists.xenproject.org Cc: stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On Thu, 2015-10-22 at 17:51 +0100, Julien Grall wrote: > On 22/10/15 17:07, Ian Campbell wrote: > > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c > > > index 665afeb..6b7eab3 100644 > > > --- a/xen/arch/arm/vgic-v2.c > > > +++ b/xen/arch/arm/vgic-v2.c > > > @@ -50,6 +50,94 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t > > > cbase, > > > paddr_t vbase) > > > vgic_v2_hw.vbase = vbase; > > > } > > > > > > +#define NR_TARGET_PER_REG 4U > > > +#define NR_BIT_PER_TARGET 8U > > > > NR_TARGETS_ and NR_BITS_... > > > > "REG" there is a bit generic, given this only works for registers with > > 8 > > -bit fields, _ITARGETSR instead? > > Well this is within the vgic-v2.c and only one register contains target. > So I found pointless to add ITARGETSR to the name. It's the use of the generic "REG" when there is only one relevant register (which could be named) which I found confusing, since the current name implies it has wider relevance than it actually does. > > This is really a part of the for() iteration expression, but oddly > > place > > here. > > If you turn the "((1 << NR_BIT_PER_TARGET) - 1)" thing into a #define > > or > > constant, then you may find that extracting the relevant byte from the > > unshifted itargetsr using (itargetsr >> (i*NR_BITS_PER_TARGET) and then > > applying the mask is clean enough to use here instead. > > I placed this shift here because I didn't want to use ... >> (i * > NR_BIT_..) which require a multiplication rather than a shift in the > resulting code. FWIW given a constant NR_BITS which is a power of two I think i*NR_BITS would end up a shift with any reasonable compiler. > > > + * guest). > > > + */ > > > + if ( !new_target || (new_target > d->max_vcpus) ) > > > + { > > > + printk(XENLOG_G_DEBUG > > > > gdprintk? > > I would prefer to keep this printk in non-debug to help us catching any > OS potentially using this trick. > > Based on that I would even use XENLOG_G_WARNING because this is not > really compliant to the spec and we are meant to fix it. Assuming I remember correctly that XENLOG_G_WARNING is ratelimited in default configurations, then OK. > > Regards, >