Re: [RFC PATCH 38/49] ARM: new VGIC: handle hardware mapped IRQs

[Andre Przywara <andre.przywara@linaro.org>]

Hi,

On 23/02/18 18:14, Julien Grall wrote:
> 
> 
> On 23/02/18 18:02, Andre Przywara wrote:
>> Hi,
> 
> Hi Andre,
> 
>> On 19/02/18 12:19, Julien Grall wrote:
>>> Hi,
>>>
>>> On 09/02/18 14:39, Andre Przywara wrote:
>>>> The VGIC supports virtual IRQs to be connected to a hardware IRQ, so
>>>> when a guest EOIs the virtual interrupt, it affects the state of that
>>>> corresponding interrupt on the hardware side at the same time.
>>>> Implement the interface that the Xen arch/core code expects to connect
>>>> the virtual and the physical world.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>>> ---
>>>>    xen/arch/arm/vgic/vgic.c | 63
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 63 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
>>>> index dc5e011fa3..8d5260a7db 100644
>>>> --- a/xen/arch/arm/vgic/vgic.c
>>>> +++ b/xen/arch/arm/vgic/vgic.c
>>>> @@ -693,6 +693,69 @@ void vgic_kick_vcpus(struct domain *d)
>>>>        }
>>>>    }
>>>>    +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu
>>>> *v,
>>>> +                                      unsigned int virq)
>>>> +{
>>>> +    struct irq_desc *desc = NULL;
>>>> +    struct vgic_irq *irq = vgic_get_irq(d, v, virq);
>>>> +    unsigned long flags;
>>>> +
>>>> +    if ( !irq )
>>>> +        return NULL;
>>>> +
>>>> +    spin_lock_irqsave(&irq->irq_lock, flags);
>>>> +    if ( irq->hw )
>>>> +        desc = irq_to_desc(irq->hwintid);
>>>
>>> This is not going to work well for PPIs. We should consider to add at
>>> least an ASSERT(...) in the code to prevent bad use of it.
>>
>> Yeah, done. But I wonder if we eventually should extend the
>> irq_to_desc() function to take the vCPU, since we will need it anyway
>> once we use hardware mapped timer IRQs (PPIs) in the future. But this
>> should not be in this series, I guess.
> 
> irq_to_desc only deal with hardware interrupt, so you mean pCPU instead
> of vCPU?

Yes, indeed. But I think this points to the problem of this approach:
the virtual IRQ is tied to a VCPU, and we have to make sure that not
only the affinity is updated on a CPU migration (as we do for SPIs), but
actually the interrupt itself is changed: since CPU0/PPI9 has a
different irq_desc* from, say, CPU1/PPI9.
So there is more than just adding a parameter to irq_to_desc().

>>>> +    spin_unlock_irqrestore(&irq->irq_lock, flags);
>>>> +
>>>> +    vgic_put_irq(d, irq);
>>>> +
>>>> +    return desc;
>>>> +}
>>>> +
>>>> +/*
>>>> + * was:
>>>> + *      int kvm_vgic_map_phys_irq(struct vcpu *vcpu, u32 virt_irq,
>>>> u32 phys_irq)
>>>> + *      int kvm_vgic_unmap_phys_irq(struct vcpu *vcpu, unsigned int
>>>> virt_irq)
>>>> + */
>>>> +int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu,
>>>> +            unsigned int virt_irq, struct irq_desc *desc,
>>>> +            bool connect)
>>>
>>> Indentation.
>>>
>>>> +{
>>>> +    struct vgic_irq *irq = vgic_get_irq(d, vcpu, virt_irq);
>>>> +    unsigned long flags;
>>>> +    int ret = 0;
>>>> +
>>>> +    if ( !irq )
>>>> +        return -EINVAL;
>>>> +
>>>> +    spin_lock_irqsave(&irq->irq_lock, flags);
>>>> +
>>>> +    if ( connect )                      /* assign a mapped IRQ */
>>>> +    {
>>>> +        /* The VIRQ should not be already enabled by the guest */
>>>> +        if ( !irq->hw && !irq->enabled )
>>>> +        {
>>>> +            irq->hw = true;
>>>> +            irq->hwintid = desc->irq;
>>>> +        }
>>>> +        else
>>>> +        {
>>>> +            ret = -EBUSY;
>>>> +        }
>>>
>>> I know that it should not matter for SPIs today. But aren't you meant to
>>> get a reference on that interrupt if you connect it?
>>
>> No, the refcount feature is strictly for the pointer to the structure,
>> not for everything related to this virtual IRQ.
>> We store only the virtual IRQ number in the dev_id/info members, we will
>> get the struct vgic_irq pointer via the vIRQ number on do_IRQ().
>> Does that make sense?
> 
> But technically you "allocate" the virtual SPI at that time, right? So
> this would mean you need to get a reference, otherwise it might disappear.

We will realise that is has disappeared when vgic_get_irq() called with
that virtual number returns NULL. The refcount is really just to know
when you can free dynamically allocated struct vgic_irqs, so it's
strictly about the *pointer* to the *memory*, not about the logical
entity of that particular virtual IRQ.
Actually it should not really happen that you end up with a hardware IRQ
still assigned to an abandoned virtual IRQ, as you would expect to free
that connection *before* disbanding the virtual IRQ.

> So I am not entirely sure why the reference is not necessary here.

Typically to remove a virtual IRQ, you arrange for vgic_get_irq() to
return NULL on that number. Then you "wait" for the refcount to drop to
zero, at which point it's safe to free the memory allocated for that
vgic_irq. As mentioned, only really useful for LPIs, but it's a central
property of the new VGIC architecture, because we need to have those
gets/puts in virtually every function.

Cheers,
Andre.

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