Re: [PATCH V5 04/10] xen/arm: Use cpumask_t type for vcpu_mask in vgic_to_sgi

[Julien Grall <julien.grall@citrix.com>]

Hi Chen,

On 30/05/2015 12:07, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> 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 <baozich@gmail.com>
> ---
>   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 <asm/gic.h>
>   #include <asm/vgic.h>
>
> +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<<GICD_SGI_TARGET_SHIFT)
> +#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