Re: [XenARM] XEN tools for ARM with Virtualization Extensions - Patch #2

[Julien Grall <julien.grall@linaro.org>]

On 07/19/2013 09:45 AM, Ian Campbell wrote:
> On Tue, 2013-07-16 at 12:24 +0100, Stefano Stabellini wrote:
>> On Tue, 16 Jul 2013, Eric Trudeau wrote:
>>>> -----Original Message-----
>>>> From: Julien Grall [mailto:julien.grall@linaro.org]
>>>> Sent: Monday, July 15, 2013 7:14 PM
>>>> To: Eric Trudeau
>>>> Cc: xen-devel; Stefano.Stabellini@citrix.com; Ian Campbell
>>>> Subject: Re: [Xen-devel] [XenARM] XEN tools for ARM with Virtualization
>>>> Extensions - Patch #2
>>>>
>>>> On 12 July 2013 20:27, Eric Trudeau <etrudeau@broadcom.com> wrote:
>>>>> Second patch submitted with changes based on comments on first patch.
>>>>>
>>>>>>> From 551f174c9653def01890fadd2dd769ab8a9acde7 Mon Sep 17 00:00:00
>>>>>> 2001
>>>>>>> From: Eric Trudeau <etrudeau@broadcom.com>
>>>>>>> Date: Thu, 11 Jul 2013 20:03:51 -0400
>>>>>>> Subject: [PATCH] Add support for Guest physdev irqs
>>>>>>>
>>>>>>> ---
>>>>>>>  xen/arch/arm/domain.c  | 16 ++++++++++++++++
>>>>>>>  xen/arch/arm/gic.c     | 15 ++++++++++-----
>>>>>>>  xen/arch/arm/physdev.c | 48
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++--
>>>>>>>  xen/arch/arm/vgic.c    |  5 +----
>>>>>>>  4 files changed, 73 insertions(+), 11 deletions(-)
>>>>>>>
>>>>>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>>>>>> index 4c434a1..52d3429 100644
>>>>>>> --- a/xen/arch/arm/domain.c
>>>>>>> +++ b/xen/arch/arm/domain.c
>>>>>>> @@ -31,6 +31,8 @@
>>>>>>>  #include <asm/gic.h>
>>>>>>>  #include "vtimer.h"
>>>>>>>  #include "vpl011.h"
>>>>>>> +#include <xen/iocap.h>
>>>>>>> +#include <xen/irq.h>
>>>>>>>
>>>>>>>  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
>>>>>>>
>>>>>>> @@ -513,8 +515,22 @@ fail:
>>>>>>>      return rc;
>>>>>>>  }
>>>>>>>
>>>>>>> +static int release_domain_irqs(struct domain *d)
>>>>>>> +{
>>>>>>> +    int i;
>>>>>>> +    for (i = 0; i <= d->nr_pirqs; i++) {
>>>>>>> +        if (irq_access_permitted(d, i)) {
>>>>>>> +            release_irq(i);
>>>>>>> +        }
>>>>>>> +    }
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>
>>>>>> As you may know, release_irq will spin until the flags IRQ_INPROGRESS
>>>>>> is unset. This is done my the maintenance handler.
>>>>>>
>>>>>> Now, let say the domain has crashed and the IRQ is not yet EOIed, then you
>>>>>> will spin forever.
>>>>>>
>>>>>
>>>>> I put a timeout in the IRQ_INPROGRESS check in release_irq() of 100 attempts
>>>>> with a 1 msec delay per loop iteration.
>>>>
>>>> If we plan to only use release_irq when a domain is destroyed, this
>>>> check is useless,
>>>> so it should be removed.
>>>>
>>>> An IRQ stays with the flag IRQ_INPROGRESS until Xen will eoi it.
>>>> If the domain has crashed or received an hard shutdown (ie xl
>>>> destroy), the IRQ will
>>>> remain "inflight" and can never come up again.
>>>>
>>>> You need to check if the IRQ is still inflight and, if yes, eoi it.
>>>>
>>
>> I think that Julien is right: we need to call something similar to
>> xen/arch/arm/gic.c:maintenance_interrupt. I would refactor the code
>> there into a separate function that ends up being called by
>> maintenance_interrupt and release_irq.
>>
>>
>>> I will have to research this implementation as I am not familiar with the flow
>>> and functions in the IRQ handler code.  If you have any suggestions, please
>>> let me know.  Is it simply a matter of checking IRQ_INFLIGHT bit in the
>>> desc->status word and then calling a function to EOI the interrupt?
>>
>> First we need to make sure that the domain is paused and about to be
>> destroyed otherwise we risk breaking the gic/vgic interface.  Then we
>> need to check whether the irq is currently inflight, looking at the
>> inflight queue, see for example
>> xen/arch/arm/vgic.c:vgic_vcpu_inject_irq.  If the irq is inflight we
>> need to check whether it's actually in one of the LR registers or just
>> in the lr_queue. See xen/arch/arm/gic.c:gic_set_guest_irq to see how the
>> lr_queue works. If the irq is queued there, just remove it from the
>> lr_queue, EOI it and remove it from the inflight queue. If the irq is
>> not present in lr_queue, it means it's in one of the LR registers. In
>> that case we can call something similar to maintenance_interrupt to
>> remove it from the LR, EOI the interrupt and remove it from the inflight
>> queue.
> 
> This all sounds plausible to me. I'm not sure we need to worry overly
> much about the impact on the guest of pulling an interrupt out from
> under it -- that's not ever going to be a clever thing to do and it
> seems like it should be up to the host and guest admin to arrange that
> the guest has quiesced the device. If the guest admin won't co-operate,
> well then they get to suffer the consequences. I guess that doesn't mean
> we shouldn't try and at least be a bit graceful about it if it's easy
> enough to do.
> 
> The thing to worry about is that the IRQ remains usable at the host
> level and can be assigned to someone else etc.

Is it enough to check IRQ_INPROGRESS flags and if it's enabled Xen will
need to EOI the interrupt?

So the code of release_irq could be:
	1) disable/mask the IRQ
        2) if IRQ_INPROGRESS => EOI it on the right CPU

> One thing to bare in mind is that when you EOI the interrupt, unless the
> guest has dealt with it you might immediately get another, which you may
> not currently be set up to handle.
> 
> You likely want to make sure it is at least masked at the physical level
> or maybe you want to try and ensure you do things in the right order
> such that it is routed to the host before you do the EOI. Safer to mask
> I think.
> 
> Ian.
>