| Message ID | 1391799378-31664-4-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
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(¤t->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 :).
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(¤t->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.
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(¤t->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.
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(¤t->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
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(¤t->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,
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(-)