Re: [Xen-devel] [PATCH v8 4/8] xen/arm: Use AFF1 when translating ICC_SGI1R_EL1 to cpumask

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Fri, 2015-06-12 at 16:32 +0800, Chen Baozi wrote:
> From: Chen Baozi <baozich@xxxxxxxxx>
> 
> 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 <baozich@xxxxxxxxx>
> ---
>  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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel