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

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 26/02/18 16:57, Julien Grall wrote:
> 
> 
> On 02/26/2018 04:48 PM, Andre Przywara wrote:
>> 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@xxxxxxxxxx>
>>>>>> ---
>>>>>>     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().
> 
> Change in the irq_to_desc() interface needs to be justify. The use case
> I have in mind for PPI is the virtual timer. In that case, you will
> always receive the PPI on the right pCPU.

Yes, but the connection between virtual and physical IRQ is realised as
a connection between struct pending_irq/vgic_irq and struct irq_desc.
For an SPI this is always the same irq_desc, regardless of the affinity
or running CPU. But for PPIs you would need to change the actual
irq_desc pointer when changing the affinity. Not really rocket science
(though it may become nasty with the locking), but needs to be implemented.

> Do you really see a use case where a vCPU is running on pCPU A but the
> PPI is routed to pCPU B?

Not at the moment, I guess the very nature of PPIs would avoid this. The
other PPIs I know about are PMUs and the VGIC. The latter is not of a
concern for us yet (fortunately). PMUs should have the same local
property as the arch timer.

Cheers,
Andre.

>>>>>> +    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.
> 
> Thank you for the explanation.
> 
> Cheers,
> 

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