Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests

[Julien Grall <julien@xen.org>]

(+ Andre)

On 23/11/2019 20:35, Julien Grall wrote:
> Hi,
> 
> On 15/11/2019 20:10, Stewart Hildebrand wrote:
>> Allow vgic_get_hw_irq_desc to be called with a vcpu argument.
>>
>> Use vcpu argument in vgic_connect_hw_irq.
>>
>> vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce with
>> ASSERTs.
>>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
>>
>> ---
>> v3: new patch
>>
>> ---
>> Note: I have only modified the old vgic to allow delivery of PPIs.
> 
> The new vGIC should also be modified to support delivery of PPIs.
> 
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 82f524a35c..c3933c2687 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -410,10 +410,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t 
>> r, int n)
>>               irq_set_affinity(p->desc, cpumask_of(v_target->processor));
>>               spin_lock_irqsave(&p->desc->lock, flags);
>>               /*
>> -             * The irq cannot be a PPI, we only support delivery of SPIs
>> -             * to guests.
>> +             * The irq cannot be a SGI, we only support delivery of SPIs
>> +             * and PPIs to guests.
>>                */
>> -            ASSERT(irq >= 32);
>> +            ASSERT(irq >= NR_SGIS);
> 
> We usually put ASSERT() in place we know that code wouldn't be able to 
> work correctly if there ASSERT were hit. In this particular case:
> 
>>               if ( irq_type_set_by_domain(d) )
>>                   gic_set_irq_type(p->desc, vgic_get_virq_type(v, n, i));
> 
> 1) We don't want to allow any domain (including Dom0) to modify the 
> interrupt type (i.e. level/edge) for PPIs as this is shared. You will 
> also most likely need to modify the counterpart in setup_guest_irq().
> 
>>               p->desc->handler->enable(p->desc);
> 
> 2) On GICv3, the re-distributor of vCPU A is accessible by vCPU B. So 
> vCPU B could enable the SGI for vCPU A. But this would be called on the 
> wrong pCPU leading to inconsistency between the hardware state of the 
> internal vGIC state.

I thought a bit more of the issue over the week-end. The current vGIC is 
fairly messy. I can see two solutions on how to solve this:
     1) Send an IPI to the pCPU where the vCPU A is running and 
disable/enable the interrupt. The other side would need to the vCPU was 
actually running to avoid disabling the PPI for the wrong pCPU
     2) Keep the HW interrupt always enabled

We propagated the enable/disable because of some messy part in the vGIC:
     - vgic_inject_irq() will not queue any pending interrupt if the 
vCPU is offline. While interrupt cannot be delivered, we still need to 
keep them pending as they will never occur again otherwise. This is 
because they are active on the host side and the guest has no way to 
deactivate them.
     - Our implementation of PSCI CPU will remove all pending interrupts 
(see vgic_clear_pending_irqs()). I am not entirely sure the implication 
here because of the previous.

There are a probably more. Aside the issues with it, I don't really see 
good advantage to propagate the interrupt state as the interrupts (PPIs, 
SPIs) have active state. So they can only be received once until the 
guest actually handles it.

So my preference would still be 2) because this makes the code simpler, 
avoid IPI and other potential locking trouble.

On a side note, there are more issues with enable/disable on the current 
vGIC as a pending interrupt already in the LR will not get dropped...

All of this is quite nasty. The sooner the new vGIC is finished the 
sooner we can kill the current one.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel