[Xen-devel,PATCH-4.5,4/4] xen/arm: set GICH_HCR_NPIE if all the LRs are in use

Message ID 1391799378-31664-4-git-send-email-stefano.stabellini@eu.citrix.com
State New
Headers show

Commit Message

Stefano Stabellini Feb. 7, 2014, 6:56 p.m.
On return to guest, if there are no free LRs and we still have more
interrupt to inject, set GICH_HCR_NPIE so that we are going to receive a
maintenance interrupt when no pending interrupts are present in the LR
registers.
The maintenance interrupt handler won't do anything anymore, but
receiving the interrupt is going to cause gic_inject to be called on
return to guest that is going to clear the old LRs and inject new
interrupts.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/gic.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Julien Grall Feb. 7, 2014, 11:39 p.m. | #1
On 07/02/14 18:56, Stefano Stabellini wrote:
> On return to guest, if there are no free LRs and we still have more
> interrupt to inject, set GICH_HCR_NPIE so that we are going to receive a
> maintenance interrupt when no pending interrupts are present in the LR
> registers.
> The maintenance interrupt handler won't do anything anymore, but
> receiving the interrupt is going to cause gic_inject to be called on
> return to guest that is going to clear the old LRs and inject new
> interrupts.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>   xen/arch/arm/gic.c |    8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 87bd5d3..bee2618 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -810,8 +810,14 @@ void gic_inject(void)
>       gic_restore_pending_irqs(current);
>       if (!gic_events_need_delivery())
>           gic_inject_irq_stop();
> -    else
> +    else {
>           gic_inject_irq_start();
> +    }
> +
> +    if ( !list_empty(&current->arch.vgic.lr_pending) )
> +        GICH[GICH_HCR] = GICH_HCR_EN | GICH_HCR_NPIE;
> +    else
> +        GICH[GICH_HCR] = GICH_HCR_EN;

Any reason to not move this in the else?

BTW, I think we can safely avoid to reinject the IRQ for the event 
channels if there is pending events. It will also avoid spurious IRQ on 
some specific case :).
Stefano Stabellini Feb. 10, 2014, 4:59 p.m. | #2
On Fri, 7 Feb 2014, Julien Grall wrote:
> On 07/02/14 18:56, Stefano Stabellini wrote:
> > On return to guest, if there are no free LRs and we still have more
> > interrupt to inject, set GICH_HCR_NPIE so that we are going to receive a
> > maintenance interrupt when no pending interrupts are present in the LR
> > registers.
> > The maintenance interrupt handler won't do anything anymore, but
> > receiving the interrupt is going to cause gic_inject to be called on
> > return to guest that is going to clear the old LRs and inject new
> > interrupts.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >   xen/arch/arm/gic.c |    8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 87bd5d3..bee2618 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -810,8 +810,14 @@ void gic_inject(void)
> >       gic_restore_pending_irqs(current);
> >       if (!gic_events_need_delivery())
> >           gic_inject_irq_stop();
> > -    else
> > +    else {
> >           gic_inject_irq_start();
> > +    }
> > +
> > +    if ( !list_empty(&current->arch.vgic.lr_pending) )
> > +        GICH[GICH_HCR] = GICH_HCR_EN | GICH_HCR_NPIE;
> > +    else
> > +        GICH[GICH_HCR] = GICH_HCR_EN;
> 
> Any reason to not move this in the else?

Yes: we need to be able to disable GICH_HCR_NPIE even if there are no
irqs to inject.


> BTW, I think we can safely avoid to reinject the IRQ for the event channels if
> there is pending events. It will also avoid spurious IRQ on some specific case
> :).

Currently we don't set GIC_IRQ_GUEST_PENDING for evtchn_irq if the irq
is already inflight. I'll see if I can come up with a patch to
streamline that behaviour.
Julien Grall Feb. 10, 2014, 5:14 p.m. | #3
On 02/10/2014 04:59 PM, Stefano Stabellini wrote:
> On Fri, 7 Feb 2014, Julien Grall wrote:
>> On 07/02/14 18:56, Stefano Stabellini wrote:
>>> On return to guest, if there are no free LRs and we still have more
>>> interrupt to inject, set GICH_HCR_NPIE so that we are going to receive a
>>> maintenance interrupt when no pending interrupts are present in the LR
>>> registers.
>>> The maintenance interrupt handler won't do anything anymore, but
>>> receiving the interrupt is going to cause gic_inject to be called on
>>> return to guest that is going to clear the old LRs and inject new
>>> interrupts.
>>>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> ---
>>>   xen/arch/arm/gic.c |    8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index 87bd5d3..bee2618 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -810,8 +810,14 @@ void gic_inject(void)
>>>       gic_restore_pending_irqs(current);
>>>       if (!gic_events_need_delivery())
>>>           gic_inject_irq_stop();
>>> -    else
>>> +    else {
>>>           gic_inject_irq_start();
>>> +    }
>>> +
>>> +    if ( !list_empty(&current->arch.vgic.lr_pending) )
>>> +        GICH[GICH_HCR] = GICH_HCR_EN | GICH_HCR_NPIE;
>>> +    else
>>> +        GICH[GICH_HCR] = GICH_HCR_EN;
>>
>> Any reason to not move this in the else?
> 
> Yes: we need to be able to disable GICH_HCR_NPIE even if there are no
> irqs to inject.

In this case, can we simply enable/disable GICH_HCR_NPIE instead of
resetting the register every time? GICH_HCR contains other bits that
could be set in the future.

>> BTW, I think we can safely avoid to reinject the IRQ for the event channels if
>> there is pending events. It will also avoid spurious IRQ on some specific case
>> :).
> 
> Currently we don't set GIC_IRQ_GUEST_PENDING for evtchn_irq if the irq
> is already inflight. I'll see if I can come up with a patch to
> streamline that behaviour.

Ok.
Stefano Stabellini Feb. 10, 2014, 5:16 p.m. | #4
On Mon, 10 Feb 2014, Julien Grall wrote:
> On 02/10/2014 04:59 PM, Stefano Stabellini wrote:
> > On Fri, 7 Feb 2014, Julien Grall wrote:
> >> On 07/02/14 18:56, Stefano Stabellini wrote:
> >>> On return to guest, if there are no free LRs and we still have more
> >>> interrupt to inject, set GICH_HCR_NPIE so that we are going to receive a
> >>> maintenance interrupt when no pending interrupts are present in the LR
> >>> registers.
> >>> The maintenance interrupt handler won't do anything anymore, but
> >>> receiving the interrupt is going to cause gic_inject to be called on
> >>> return to guest that is going to clear the old LRs and inject new
> >>> interrupts.
> >>>
> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>> ---
> >>>   xen/arch/arm/gic.c |    8 +++++++-
> >>>   1 file changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >>> index 87bd5d3..bee2618 100644
> >>> --- a/xen/arch/arm/gic.c
> >>> +++ b/xen/arch/arm/gic.c
> >>> @@ -810,8 +810,14 @@ void gic_inject(void)
> >>>       gic_restore_pending_irqs(current);
> >>>       if (!gic_events_need_delivery())
> >>>           gic_inject_irq_stop();
> >>> -    else
> >>> +    else {
> >>>           gic_inject_irq_start();
> >>> +    }
> >>> +
> >>> +    if ( !list_empty(&current->arch.vgic.lr_pending) )
> >>> +        GICH[GICH_HCR] = GICH_HCR_EN | GICH_HCR_NPIE;
> >>> +    else
> >>> +        GICH[GICH_HCR] = GICH_HCR_EN;
> >>
> >> Any reason to not move this in the else?
> > 
> > Yes: we need to be able to disable GICH_HCR_NPIE even if there are no
> > irqs to inject.
> 
> In this case, can we simply enable/disable GICH_HCR_NPIE instead of
> resetting the register every time? GICH_HCR contains other bits that
> could be set in the future.

Sure

Patch

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 87bd5d3..bee2618 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -810,8 +810,14 @@  void gic_inject(void)
     gic_restore_pending_irqs(current);
     if (!gic_events_need_delivery())
         gic_inject_irq_stop();
-    else
+    else {
         gic_inject_irq_start();
+    }
+
+    if ( !list_empty(&current->arch.vgic.lr_pending) )
+        GICH[GICH_HCR] = GICH_HCR_EN | GICH_HCR_NPIE;
+    else
+        GICH[GICH_HCR] = GICH_HCR_EN;
 }
 
 int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,