From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v8 4/8] xen/arm: Use AFF1 when translating ICC_SGI1R_EL1 to cpumask Date: Wed, 17 Jun 2015 14:00:30 +0100 Message-ID: <1434546030.13744.369.camel@citrix.com> References: <1434097969-16793-1-git-send-email-cbz@baozis.org> <1434097969-16793-5-git-send-email-cbz@baozis.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Z5CxK-0003am-84 for xen-devel@lists.xenproject.org; Wed, 17 Jun 2015 13:00:38 +0000 In-Reply-To: <1434097969-16793-5-git-send-email-cbz@baozis.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: Chen Baozi Cc: Julien Grall , xen-devel@lists.xenproject.org, Chen Baozi List-Id: xen-devel@lists.xenproject.org On Fri, 2015-06-12 at 16:32 +0800, Chen Baozi wrote: > From: Chen Baozi > > The old unsigned long type of vcpu_mask can only express 64 cpus at the > most, which might not be enough for the guest which used vGICv3. We > introduce a new struct sgi_target for the target cpu list of SGI, which > holds the affinity path information (only level 1 at the moment). For > GICv2 that has no affinity level, we can just set the corresponding > fields to be 0. > > Signed-off-by: Chen Baozi > --- > xen/arch/arm/vgic-v2.c | 7 +++--- > xen/arch/arm/vgic-v3.c | 10 +++++---- > xen/arch/arm/vgic.c | 45 +++++++++++++++++---------------------- > xen/include/asm-arm/gic_v3_defs.h | 3 +++ > xen/include/asm-arm/vgic.h | 7 +++++- > 5 files changed, 38 insertions(+), 34 deletions(-) > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c > index 3be1a51..5949cf1 100644 > --- a/xen/arch/arm/vgic-v2.c > +++ b/xen/arch/arm/vgic-v2.c > @@ -201,16 +201,17 @@ static int vgic_v2_to_sgi(struct vcpu *v, register_t sgir) > int virq; > int irqmode; > enum gic_sgi_mode sgi_mode; > - unsigned long vcpu_mask = 0; > + struct sgi_target target; > > + memset(&target, 0, sizeof(struct sgi_target)); I'd prefer explicit initialisation of the relevant fields please. Which may mean setting aff1 to 0 somewhere at the top, with a suitable comment as to why, and might involve setting target.list to zero in some other cases below or via an explicit initialiser here. Or maybe you prefer a struct initialiser on the declaration with the defaults. > - unsigned long vcpu_mask = 0; > + struct sgi_target target; > > + memset(&target, 0, sizeof(struct sgi_target)); Likewise. > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 7b387b7..59bd98a 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -318,15 +318,14 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) > } > } > > -/* TODO: unsigned long is used to fit vcpu_mask.*/ > int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int virq, > - unsigned long vcpu_mask) > + const struct sgi_target *target) For a 3 byte struct perhaps we can pass by value instead of reference? I suppose it might eventually be 5 bytes, but even so... > @@ -334,29 +333,33 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int > { > case SGI_TARGET_LIST: > perfc_incr(vgic_sgi_list); > + base = target->aff1 << 4; > + bitmap = target->list; > + for_each_set_bit( i, &bitmap, sizeof(target->list) * 8 ) > + { > + vcpuid = base + i; > + if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) ) What if d->vcpu[vcpuid] is NULL? (Was this a latent bug before, or am I missing something?) Note that at least the use of d->max_vcpus from the old code has now gone. > @@ -365,16 +368,6 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int > return 0; > } > > - for_each_set_bit( vcpuid, &vcpu_mask, d->max_vcpus ) > - { > - if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) ) > - { > - gprintk(XENLOG_WARNING, "VGIC: write r=%"PRIregister" \ > - vcpu_mask=%lx, wrong CPUTargetList\n", sgir, vcpu_mask); > - continue; > - } > - vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq); > - } > return 1; > } > Ian.