Re: [PATCH V4 4/8] xen/arm: Use cpumask_t type for vcpu_mask in vgic_to_sgi

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

Hi Chen,

On 28/05/15 11:15, 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.

This patch does more than using cpumask_t. It's also add AFF1 support
for GICv3. This should at least be mentioned in the patch (and commit
message). But I would prefer a separate patch to add this feature.

Furthermore, cpumask_t is based on NR_CPUS. There is not relation
between NR_CPUS and MAX_VIRT_CPUS so you need to add a BUILD_BUG_ON in
order to catch wrong configuration.

> 
> 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        | 15 +++++++--------
>  xen/include/asm-arm/vgic.h |  2 +-
>  4 files changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 3be1a51..2dbe371 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -33,6 +33,17 @@
>  #include <asm/gic.h>
>  #include <asm/vgic.h>
>  
> +static void gicv2_sgir_to_cpumask(cpumask_t *cpumask, const register_t sgir)

static inline

> +{
> +    unsigned long target_list;
> +    int cpuid;
> +
> +    target_list = ((sgir & GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT);
> +    for_each_set_bit( cpuid, &target_list, 8 )
> +        cpumask_set_cpu(cpuid, cpumask);

The SGI code is called very often by the guest so we need an optimized code.
As there is already a loop later I'd like to avoid another here.

I think the loop can be replaced with:

bitmap_copy(cpumask_bits(cpumask), &target_list, 8);

The operation will be in O(1).

Also please use a define rather than the hardcoded value 8.

> +
> +}
> +
>  static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
>  {
>      struct hsr_dabt dabt = info->dabt;
> @@ -201,11 +212,12 @@ 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;
> +    gicv2_sgir_to_cpumask(&vcpu_mask, sgir);

This is only necessary to call when the SGI target a list of VCPUs
(GICD_SGI_TARGET_LIST_VAL).

>  
>      /* Map GIC sgi value to enum value */
>      switch ( irqmode )
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index ef9a71a..0da031c 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -972,17 +972,30 @@ write_ignore:
>      return 1;
>  }
>  
> +static void gicv3_sgir_to_cpumask(cpumask_t *cpumask, const register_t sgir)
> +{
> +    int target, cpuid;
> +    unsigned long target_mask = sgir & ICH_SGI_TARGETLIST_MASK;
> +
> +    for_each_set_bit( target, &target_mask, 16 )
> +    {
> +        /* XXX: We assume that only AFF1 is used in ICC_SGI1R_EL1. */
> +        cpuid = target + ((sgir >> 16) & 0xff) * 16;

Please use define rather than constant.

> +        cpumask_set_cpu(cpuid, cpumask);
> +    }

Same remark as above, the loop can be avoided.

Furthermore you need to validate the AFF1. A malicious guest can decide
to pass AFF1 >= 16 which will give a CPU higher than 128 and potentially
allow the guest to do a stack overflow in Xen.

> +}
> +
>  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;
> +    gicv3_sgir_to_cpumask(&vcpu_mask, sgir);

It's only necessary when the SGI targets a list of VCPU
(ICH_SGI_TARGET_LIST).

>  
>      /* Map GIC sgi value to enum value */
>      switch ( irqmode )
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 7b387b7..4bf8486 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -318,9 +318,8 @@ 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)

You need to pass a pointer here, otherwise you will copy all the
cpumask_t everytime which is very expensive.

>  {
>      struct domain *d = v->domain;
>      int vcpuid;

The BUG_ON below can be removed. Keeping here is a call to a potential
unwanted crash of Xen by the guest.

> @@ -341,12 +340,12 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int

>          break;
>      case SGI_TARGET_SELF:
> @@ -355,8 +354,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);
> +        cpumask_set_cpu(current->vcpu_id, &vcpu_mask);
>          break;
>      default:
>          gprintk(XENLOG_WARNING,
> @@ -365,12 +364,12 @@ 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_each_cpu( vcpuid, &vcpu_mask )

Is there any potential performance impact? You are moving from a loop of
d->max_vcpus to 128 (and potentially more).

>      {
>          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);
> +                    , wrong CPUTargetList\n", sgir);
>              continue;
>          }

Regards,

-- 
Julien Grall