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:19, Ian Campbell wrote:
> On Wed, 2015-06-17 at 14:13 +0100, Julien Grall wrote:
>> 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>
>>>> 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.
> 
> Good point, and by keeping it a pointer you could even pass NULL in the
> other cases, making this more obvious still.

Good idea.

>>>
>>>> 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.
> 
> But the code will, I think. I should have quoted a bit more, briefly it
> is :
> 
> +            if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) )
>                     continue
> +            vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq);
> 
> So if d->vcpu[vcpuid] == NULL it will try and send an SGI to it, won't
> it?

Hmmm, correct. I didn't read carefully the if, sorry. It should be it
"d->vcpu[vcpuid] == NULL || !is_vcpu_online(d->vcpu[vcpuid])".

And yes, this is a latent bug. Although, XEN_DOMCTL_max_vcpus will
return -ENOMEM if it fail to allocate a VCPU and libxl will continue to
create the domain. So no possibility for the guest to crash Xen.

Regards,

-- 
Julien Grall