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

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

On 17/06/15 14:00, Ian Campbell wrote:
> On Fri, 2015-06-12 at 16:32 +0800, Chen Baozi wrote:
>> From: Chen Baozi <baozich@gmail.com>
>>
>> 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@gmail.com>
>> ---
>>  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.

Well, only SGI_TARGET_LIST is caring about struct sgi_target (see
vgic_to_sgi). I would only initialize it when it's required.

> Or maybe you prefer a struct initialiser on the declaration with the
> defaults.

It would be nice to have something initializing all the fields (even a
new one is added). It would avoid problem later.

>> -    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?)

I don't see any problem, if d->vcpu[vcpuid] is NULL there is no need to
send an SGI as the VCPU is not present.

Although d->vcpu[vcpuid] will unlikely be NULL.

> Note that at least the use of d->max_vcpus from the old code has now
> gone.

The array d->vcpu is based on d->max_vcpus, if by any chance vcpuid is
greater, you will access to a wrong address which will potentially crash
the host.

Regards,

-- 
Julien Grall