Re: [Xen-devel] ARM Generic Timer interrupt

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 28 May 2014, Ian Campbell wrote:
> On Wed, 2014-05-28 at 12:51 +0100, Stefano Stabellini wrote:
> > On Wed, 28 May 2014, Ian Campbell wrote:
> > > On Wed, 2014-05-28 at 12:32 +0100, Julien Grall wrote:
> > > > On 05/28/2014 11:10 AM, Ian Campbell wrote:
> > > > > On Tue, 2014-05-27 at 13:11 +0100, Stefano Stabellini wrote:
> > > > >>> But, I have question:
> > > > >>> Should the Hypervisor masks virtual timer IRQ on his own?
> > > > >>> It is a guest's resource and the guest itself should decide what to 
> > > > >>> do.
> > > > >>> For example, I see that Linux Kernel (3.8) sets and clears timer 
> > > > >>> interrupt mask by itself.
> > > > >>
> > > > >> In principle I agree with you that the vtimer is a guest resource.
> > > > >> However in practice if we don't mask the irq we can easily get into 
> > > > >> an
> > > > >> interrupt storm situation: if the guest doesn't handle the interrupt
> > > > >> immediately we could keep receiving the vtimer irq in the hypervisor 
> > > > >> and
> > > > >> busy loop around it.
> > > > > 
> > > > > Do we not do a priority drop on the interrupt when we receive it, so 
> > > > > we
> > > > > won't get any more interrupts from the timer until it acks the
> > > > > interrupt?
> > > > 
> > > > The timer interrupt is acked directly by Xen. We can't wait the guest
> > > > VCPU as EOI the interrupt because the guest may have move to another
> > > > pCPU by this time.
> > > 
> > > Surely we can arrange to handle that though. The way we currently handle
> > > the timer stuff always seemed suboptimal to me.
> > 
> > Aside from vcpu migration that we can obviously handle correctly, in
> > order to avoid the current "hack" we would need to introduce 2 vtimer
> > special cases in vgic.c and gic.c. Also even though I don't have any
> > numbers to prove it, I suspect that activating/deactivating the vtimer
> > irq at the GICD level all the time might be slower than just masking it
> > at the vtimer level.
> > 
> > So the tradeoff is: worse, slower hypervisor code but correctness of the
> > interface, or faster, leaner hypervisor code but a slightly worse guest
> > interface?
> > I don't know what the right answer is, but I am leaning toward the
> > second option.
> 
> Adding requirements to the guest's use of the vtimer to make our lives
> earlier certainly seems to make sense, but it would be best if this were
> backed up by some standard from ARM or someone so that guests do it
> consistently.
> 
> I was mostly objecting to the proposed patch which seemed to be choosing
> the worst of both options above...

I see.

The patch uses GICD_ICENABLER to disable the timer because after the
desc->handler->end call, the vtimer can fire again unless properly
masked/deactivated. At the same time we need to call end otherwise we
won't be receiving other interrupts in Xen.

I guess an alternative implementation would introduce a special case in
do_IRQ, avoid calling desc->handler->end for the vtimer and only do
GICC[GICC_EOIR] = vtimer_irq to lower the priority, similarly to
IRQ_GUEST. We would then EOI the irq after receiving the maintenance
interrupt, but not in the maintenance interrupt handler, because we need
to EOI the maintenance interrupt first.   In addition it also needs
another special case in gic_save_state to EOI the vtimer interrupt if
the guest hasn't yet done so on vcpu save. And yet another special case
in gic_restore_state to deactivate the interrupt using GICD_ICENABLER,
because since the guest might not have handled it yet, it could fire
again continuously and we are not protected by the missing EOI anymore.
It doesn't sound better overall.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel