Re: [Xen-devel] [PATCH v9 2/2] xen/arm: trap guest WFI

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-04-29 at 19:09 +0100, Stefano Stabellini wrote:
> On Mon, 29 Apr 2013, Ian Campbell wrote:
> > On Tue, 2013-04-23 at 12:19 +0100, Stefano Stabellini wrote:
> > > diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
> > > index 10f58af..276fef5 100644
> > > --- a/xen/include/asm-arm/event.h
> > > +++ b/xen/include/asm-arm/event.h
> > > @@ -1,27 +1,50 @@
> > >  #ifndef __ASM_EVENT_H__
> > >  #define __ASM_EVENT_H__
> > >  
> > > +#include <asm/gic.h>
> > > +#include <asm/domain.h>
> > > +
> > >  void vcpu_kick(struct vcpu *v);
> > >  void vcpu_mark_events_pending(struct vcpu *v);
> > >  
> > > +static inline int local_events_need_delivery_nomask(void)
> > > +{
> > > +    struct pending_irq *p = irq_to_pending(current, 
> > > VGIC_IRQ_EVTCHN_CALLBACK);
> > > +
> > > +    /* XXX: if the first interrupt has already been delivered, we should
> > > +     * check whether any higher priority interrupts are in the
> > > +     * lr_pending queue or in the LR registers and return 1 only in that
> > > +     * case.
> > > +     * In practice the guest interrupt handler should run with
> > > +     * interrupts disabled so this shouldn't be a problem in the general
> > > +     * case.
> > 
> > I think in practice this would be the problem of the code injecting the
> > actual upcall, rather than the code here which determines if such an
> > upcall is required? Perhaps this comment needs to be in whichever
> > function handles the actual injection of interrupts?
> >
> > Also rather than requiring the guest interrupt handler to run with
> > interrupts disabled I think the code as it stands simply requires that
> > the guest not mind that the upcall doesn't get preempted by a higher
> > priority interrupt. IOW the upcall will complete and then the higher
> > priority interrupt will be injected. To the guest this just looks like
> > the interrupt arrived a bit later, so while it is something to fix
> > eventually its not really a big problem until people start wanting to do
> > more real-timeish stuff?
> 
> Keep in mind that local_events_need_delivery is called by common code to
> make all sort of decisions (including hypercall_preempt_check): it is
> important that local_events_need_delivery* returns an accurate answer.
> If an interrupt has already been injected and it is currently inflight
> (no EOI yet), local_events_need_delivery_nomask shouldn't return
> positive unless an higher interrupt is waiting to be delivered. Doing
> something different would be a mistake and can cause hypercalls to be
> unnecessarily preempted, the vcpu to be woken up more often than
> necessary, SCHEDOP_poll to return a wrong result.
> That's why I think that the comment should to stay in this function.

This description makes me more concerned about the "should run with
interrupts disabled" requirement than I was previously. How hard would
it be to actually implement the behaviour the comment describes?
Checking the lr_pending queue for higher priorities seems trivial since
you can just look at the head of the list, not sure about checking the
LRs -- is that the lr_inflight queue? Or do you need to check the
pending/active bits for every one?

BTW, "any higher priority" -- higher than what? Higher than the
EVTCHN_CALLBACK IRQ priority or the current IAR/PMR value for the GICC?

Ian.


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