From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v5 16/21] xen/arm: split vgic driver into generic and vgic-v2 driver Date: Tue, 17 Jun 2014 14:40:54 +0100 Message-ID: <53A04566.4000106@linaro.org> References: <1402580192-13937-1-git-send-email-vijay.kilari@gmail.com> <1402580192-13937-17-git-send-email-vijay.kilari@gmail.com> <539DD21A.2030108@linaro.org> <1403011141.16844.98.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1403011141.16844.98.camel@kazak.uk.xensource.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: vijay.kilari@gmail.com, stefano.stabellini@eu.citrix.com, Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, tim@xen.org, xen-devel@lists.xen.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org On 06/17/2014 02:19 PM, Ian Campbell wrote: > On Sun, 2014-06-15 at 18:04 +0100, Julien Grall wrote: >> You've reintroduced the XSA-94 here (see bf70db7 vgic: Check rank in >> GICD_ICFGR* emulation before locking). When you send a new version of a >> serie, please *check* there is no update on this code which may fix error. > > One technique I use here is, given the patch in file "x": > grep ^- x | cut -c2- > A > grep ^\+ x | cut -c2- > B > diff -u A B > > It's far from perfect and relies on the code order not changing too > drastically over the movement, but it would have caught this. > >> I saw you shared a part of the emulation between the distributor and the >> redistributor in GICv3. I think you can also share with GICv2, this >> could avoid fix in 2 places the same bug (or worst only fixing in 1 place). > > I'm not convinced that sharing vgic-2 and vgic-3 code wouldn't end up > being more confusing in the long run. There is about 200 lines common between v2 and v3 (I{C,S}ENABLER, I{C,S}ACTIVER, IPRIORITYR, ICFGR...). Some of theses registers implementation contains non-obvious code (when we will implement correctly I*ACTIVER). Duplicating them will be very hard to maintain. While it's not so important for this series. I think we should merge them sooner or later. >>> -static int vgic_to_sgi(struct vcpu *v, register_t sgir) >>> +int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int virq, >>> + unsigned long vcpu_mask) >> >> >> You can't assume that all the VCPU bits will fit in an unsigned long. We >> will have to use cpumask_t at some point. >> >> I'm fine if you don't handle it for now, but you need to *write down* >> somewhere the limitation of this function. > > To be fair, this is a preexisting restriction and this is far from the > only place which will need fixing. It might be a preexisting condition, but the previous version was taking care of the vcpu_mask internally. Now we are using it as an argument. AFAIK, this is the only place where we use unsigned long to describe a list of vcpu. Every other place in Xen use correctly cpumask. -- Julien Grall