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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.

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