From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH V5 04/10] xen/arm: Use cpumask_t type for vcpu_mask in vgic_to_sgi Date: Sun, 31 May 2015 14:05:50 +0100 Message-ID: <556B072E.3010504@citrix.com> References: <1432984051-10838-1-git-send-email-cbz@baozis.org> <1432984051-10838-5-git-send-email-cbz@baozis.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" 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 1Yz2w8-00040J-0X for xen-devel@lists.xenproject.org; Sun, 31 May 2015 13:05:56 +0000 In-Reply-To: <1432984051-10838-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 , xen-devel@lists.xenproject.org Cc: Julien Grall , Chen Baozi , Ian Campbell List-Id: xen-devel@lists.xenproject.org Hi Chen, On 30/05/2015 12:07, Chen Baozi wrote: > From: Chen Baozi > > Use cpumask_t instead of unsigned long which can only express 64 cpus at > the most. Add the {gicv2|gicv3}_sgir_to_cpumask in corresponding vGICs > to translate GICD_SGIR/ICC_SGI1R_EL1 to vcpu_mask for vgic_to_sgi. > > Signed-off-by: Chen Baozi > --- > xen/arch/arm/vgic-v2.c | 16 +++++++++++++--- > xen/arch/arm/vgic-v3.c | 19 +++++++++++++++---- > xen/arch/arm/vgic.c | 24 +++++++++++------------- > xen/include/asm-arm/gic.h | 1 + > xen/include/asm-arm/gic_v3_defs.h | 2 ++ > xen/include/asm-arm/vgic.h | 2 +- > 6 files changed, 43 insertions(+), 21 deletions(-) > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c > index 3be1a51..17a3c9f 100644 > --- a/xen/arch/arm/vgic-v2.c > +++ b/xen/arch/arm/vgic-v2.c > @@ -33,6 +33,15 @@ > #include > #include > > +static inline void gicv2_sgir_to_cpumask(cpumask_t *cpumask, > + const register_t sgir) > +{ > + unsigned long target_list; > + > + target_list = ((sgir & GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT); > + bitmap_copy(cpumask_bits(cpumask), &target_list, GICD_SGI_TARGET_BITS); > +} > + > static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info) > { > struct hsr_dabt dabt = info->dabt; > @@ -201,16 +210,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; > + cpumask_t vcpu_mask; > > + cpumask_clear(&vcpu_mask); > irqmode = (sgir & GICD_SGI_TARGET_LIST_MASK) >> GICD_SGI_TARGET_LIST_SHIFT; > virq = (sgir & GICD_SGI_INTID_MASK); > - vcpu_mask = (sgir & GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT; > > /* Map GIC sgi value to enum value */ > switch ( irqmode ) > { > case GICD_SGI_TARGET_LIST_VAL: > + gicv2_sgir_to_cpumask(&vcpu_mask, sgir); > sgi_mode = SGI_TARGET_LIST; > break; > case GICD_SGI_TARGET_OTHERS_VAL: > @@ -226,7 +236,7 @@ static int vgic_v2_to_sgi(struct vcpu *v, register_t sgir) > return 0; > } > > - return vgic_to_sgi(v, sgir, sgi_mode, virq, vcpu_mask); > + return vgic_to_sgi(v, sgir, sgi_mode, virq, &vcpu_mask); > } > > static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info) > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index ef9a71a..a283c8c 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -972,22 +972,33 @@ write_ignore: > return 1; > } > > +static inline void gicv3_sgir_to_cpumask(cpumask_t *cpumask, > + const register_t sgir) > +{ > + unsigned long target_list; > + > + target_list = sgir & ICH_SGI_TARGETLIST_MASK; > + bitmap_copy(cpumask_bits(cpumask), &target_list, ICH_SGI_TARGET_BITS); > + Spurious line. > +} > + > static int vgic_v3_to_sgi(struct vcpu *v, register_t sgir) > { > int virq; > int irqmode; > enum gic_sgi_mode sgi_mode; > - unsigned long vcpu_mask = 0; > + cpumask_t vcpu_mask; > > + cpumask_clear(&vcpu_mask); > irqmode = (sgir >> ICH_SGI_IRQMODE_SHIFT) & ICH_SGI_IRQMODE_MASK; > virq = (sgir >> ICH_SGI_IRQ_SHIFT ) & ICH_SGI_IRQ_MASK; > - /* SGI's are injected at Rdist level 0. ignoring affinity 1, 2, 3 */ > - vcpu_mask = sgir & ICH_SGI_TARGETLIST_MASK; > > /* Map GIC sgi value to enum value */ > switch ( irqmode ) > { > case ICH_SGI_TARGET_LIST: > + /* SGI's are injected at Rdist level 0. ignoring affinity 1, 2, 3 */ > + gicv3_sgir_to_cpumask(&vcpu_mask, sgir); > sgi_mode = SGI_TARGET_LIST; > break; > case ICH_SGI_TARGET_OTHERS: > @@ -998,7 +1009,7 @@ static int vgic_v3_to_sgi(struct vcpu *v, register_t sgir) > return 0; > } > > - return vgic_to_sgi(v, sgir, sgi_mode, virq, vcpu_mask); > + return vgic_to_sgi(v, sgir, sgi_mode, virq, &vcpu_mask); > } > > static int vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr) > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 7b387b7..f9e43f9 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -318,16 +318,12 @@ 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) > + cpumask_t *vcpu_mask) > { > struct domain *d = v->domain; > - int vcpuid; > int i; > > - ASSERT(d->max_vcpus < 8*sizeof(vcpu_mask)); > - As said in the previous version, cpumask_t is based on NR_CPUS and there is no relation between NR_CPUS and MAX_VIRT_CPUS. Furthermore, NR_CPUS which can be configured at build time by the user (see MAX_PHYS_CPUS). We need to catch at build time possible when the cpumask_t would be smaller in order to avoid insecure hypervisor. > ASSERT( virq < 16 ); > > switch ( irqmode ) > @@ -341,12 +337,12 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int > * SGI_TARGET_SELF mode. So Force vcpu_mask to 0 > */ > perfc_incr(vgic_sgi_others); > - vcpu_mask = 0; > + cpumask_clear(vcpu_mask); Given that you already clear the cpumask on the caller of this function, I think we can consider that it will be nice with us rather than wasting CPU time to clear twice. That would require to update the comment above. > for ( i = 0; i < d->max_vcpus; i++ ) > { > if ( i != current->vcpu_id && d->vcpu[i] != NULL && > is_vcpu_online(d->vcpu[i]) ) > - set_bit(i, &vcpu_mask); > + cpumask_set_cpu(i, vcpu_mask); > } > break; > case SGI_TARGET_SELF: > @@ -355,8 +351,8 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int > * SGI_TARGET_SELF mode. So Force vcpu_mask to 0 > */ > perfc_incr(vgic_sgi_self); > - vcpu_mask = 0; > - set_bit(current->vcpu_id, &vcpu_mask); > + cpumask_clear(vcpu_mask); Ditto. > + cpumask_set_cpu(current->vcpu_id, vcpu_mask); > break; > default: > gprintk(XENLOG_WARNING, > @@ -365,15 +361,17 @@ 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 ) > + for ( i = cpumask_first(vcpu_mask); > + i < d->max_vcpus; > + i = cpumask_next(i, vcpu_mask)) The indentation looks wrong here. And why don't you use vcpuid? It would have avoid the renaming vcpuid/i below which increase the path size for nothing. > { > - if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) ) > + if ( d->vcpu[i] != NULL && !is_vcpu_online(d->vcpu[i]) ) > { > gprintk(XENLOG_WARNING, "VGIC: write r=%"PRIregister" \ > - vcpu_mask=%lx, wrong CPUTargetList\n", sgir, vcpu_mask); > + , wrong CPUTargetList\n", sgir); > continue; > } > - vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq); > + vgic_vcpu_inject_irq(d->vcpu[i], virq); > } > return 1; > } > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index 463fffb..c6ef4bf 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -64,6 +64,7 @@ > #define GICD_SGI_TARGET_SELF_VAL (2) > #define GICD_SGI_TARGET_SHIFT (16) > #define GICD_SGI_TARGET_MASK (0xFFUL< +#define GICD_SGI_TARGET_BITS (8) > #define GICD_SGI_GROUP1 (1UL<<15) > #define GICD_SGI_INTID_MASK (0xFUL) > > diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h > index 556f114..e106e67 100644 > --- a/xen/include/asm-arm/gic_v3_defs.h > +++ b/xen/include/asm-arm/gic_v3_defs.h > @@ -152,6 +152,8 @@ > #define ICH_SGI_IRQ_SHIFT 24 > #define ICH_SGI_IRQ_MASK 0xf > #define ICH_SGI_TARGETLIST_MASK 0xffff > +#define ICH_SGI_TARGET_BITS 16 > + > #endif /* __ASM_ARM_GIC_V3_DEFS_H__ */ > > /* > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h > index 6dcdf9f..2f413e1 100644 > --- a/xen/include/asm-arm/vgic.h > +++ b/xen/include/asm-arm/vgic.h > @@ -201,7 +201,7 @@ DEFINE_VGIC_OPS(3) > extern int vcpu_vgic_free(struct vcpu *v); > extern int vgic_to_sgi(struct vcpu *v, register_t sgir, > enum gic_sgi_mode irqmode, int virq, > - unsigned long vcpu_mask); > + cpumask_t *vcpu_mask); > extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq); > > /* Reserve a specific guest vIRQ */ > Regards, -- Julien Grall