Re: [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs

[Henry Wang <xin.wang2@amd.com>]

Hi Julien,

On 5/19/2024 7:08 PM, Julien Grall wrote:
> Hi,
>
> On 17/05/2024 07:03, Henry Wang wrote:
>>> @@ -444,14 +444,18 @@ int vgic_connect_hw_irq(struct domain *d, 
>>> struct vcpu *v, unsigned int virq,
>>>       {
>>>           /* The VIRQ should not be already enabled by the guest */
>
> This comment needs to be updated.

Yes, sorry. I will update this and the one in the new vGIC in v3.

>>>           if ( !p->desc &&
>>> -             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
>>> +             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
>>> +             !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) &&
>>> +             !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
>>>               p->desc = desc;
>>>           else
>>>               ret = -EBUSY;
>>>       }
>>>       else
>>>       {
>>> -        if ( desc && p->desc != desc )
>>> +        if ( desc && p->desc != desc &&
>>> +             (test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
>>> +              test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status)) )
>>
>> This should be
>>
>> +        if ( (desc && p->desc != desc) ||
>> +             test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
>> +             test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> Looking at gic_set_lr(), we first check p->desc, before setting 
> IRQ_GUEST_VISIBLE.
>
> I can't find a common lock, so what would guarantee that p->desc is 
> not going to be used or IRQ_GUEST_VISIBLE set afterwards?

I think the gic_set_lr() is supposed to be called with v->arch.vgic.lock 
taken, at least the current two callers (gic_raise_guest_irq() and 
gic_restore_pending_irqs()) are doing it this way. Would this address 
your concern? Thanks.

Kind regards,
Henry