Re: [Xen-devel] [PATCH v3 13/39] ARM: new VGIC: Add IRQ sync/flush framework

[André Przywara <andre.przywara@xxxxxxx>]

On 27/03/18 20:20, Stefano Stabellini wrote:
> On Tue, 27 Mar 2018, Stefano Stabellini wrote:
>> On Tue, 27 Mar 2018, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 26/03/18 22:30, Stefano Stabellini wrote:
>>>> On Wed, 21 Mar 2018, Andre Przywara wrote:
>>>>> Implement the framework for syncing IRQs between our emulation and the
>>>>> list registers, which represent the guest's view of IRQs.
>>>>> This is done in vgic_sync_from_lrs() and vgic_sync_to_lrs(), which
>>>>> get called on guest entry and exit, respectively.
>>>>> The code talking to the actual GICv2/v3 hardware is added in the
>>>>> following patches.
>>>>>
>>>>> This is based on Linux commit 0919e84c0fc1, written by Marc Zyngier.
>>>>>
>>>>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
>>>>
>>>> Just one question below, but the code looks nice
>>>>
>>>>
>>>>> ---
>>>>> Changelog v2 ... v3:
>>>>> - replace "true" instead of "1" for the boolean parameter
>>>>>
>>>>> Changelog v1 ... v2:
>>>>> - make functions void
>>>>> - do underflow setting directly (no v2/v3 indirection)
>>>>> - fix multiple SGIs injections (as the late Linux bugfix)
>>>>>
>>>>>  xen/arch/arm/vgic/vgic.c | 232 
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  xen/arch/arm/vgic/vgic.h |   2 +
>>>>>  2 files changed, 234 insertions(+)
>>>>>
>>>>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
>>>>> index ee0de8d2e0..52e1669888 100644
>>>>> --- a/xen/arch/arm/vgic/vgic.c
>>>>> +++ b/xen/arch/arm/vgic/vgic.c
>>>>> @@ -409,6 +409,238 @@ void vgic_inject_irq(struct domain *d, struct vcpu 
>>>>> *vcpu, unsigned int intid,
>>>
>>> ....
>>>
>>>>> +/* Requires the ap_list_lock to be held. */
>>>>> +static int compute_ap_list_depth(struct vcpu *vcpu)
>>>>> +{
>>>>> +    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
>>>>> +    struct vgic_irq *irq;
>>>>> +    int count = 0;
>>>>> +
>>>>> +    ASSERT(spin_is_locked(&vgic_cpu->ap_list_lock));
>>>>> +
>>>>> +    list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list)
>>>>> +    {
>>>>> +        spin_lock(&irq->irq_lock);
>>>>> +        /* GICv2 SGIs can count for more than one... */
>>>>> +        if ( vgic_irq_is_sgi(irq->intid) && irq->source )
>>>>> +            count += hweight8(irq->source);
>>>>
>>>> Why is this done?
>>>
>>> GICv2 SGIs always have a source CPU ID connected to them. So if two CPUs
>>> signal another CPU at the same time, there are *two* distinct SGIs, with
>>> two different source IDs. This is an architectural feature of GICv2, so
>>> we have to properly emulate this.
>>> Despite them being edge triggered IRQs, we cannot coalesce them in this
>>> case.
>>
>> I went through the whole lifecycle of SGIs with the new vgic and it is
>> quite different from before, but it makes sense to me now.
>>
>> Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 
> Actually, I take it back, one more question :-)
> 
> I understand that every bit set in irq->source corresponds to a
> different interrupt that needs to be injected into the guest. They are
> distinct interrupts.
> 
> However, compute_ap_list_depth is called to figure out whether the
> entries in ap_list overflow the LR registers, and it is never the case
> that we write to more than one LR register for a given SGI, even if
> irq->source has multiple bit sets, right?

Yes, that was actually a recent change in KVM:
https://lists.cs.columbia.edu/pipermail/kvmarm/2018-March/030226.html

So I basically took this patch right into the series.

Now there are more subtleties about priorities (see the follow-ups on
this thread), which actually led to a different patch being merged:
https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git/commit/?id=16ca6a607
(what I only learned today).
So this is a bit more sophisticated, and needs some porting because of
the new "empty LR" interrupt. I would rather do this on top.

> In a concrete example, if we have 3 LR registers and 3 interrupts in
> ap_list, one of them is an SGI with multiple irq->source bits, there is
> still no need to sort the ap_list, correct?
> 
> I think we should remove the special if statement for sgis in
> compute_ap_list_depth.

Yes, I believe this is a good intermediate measure, until we get the
proper solution.
Let me test this tomorrow, then I can push a reworked version of this patch.

Cheers,
Andre.


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