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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-04-30 at 18:48 +0100, Stefano Stabellini wrote:
> On Tue, 30 Apr 2013, Ian Campbell wrote:
> > 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.
> 
> I wouldn't worry too much about it: the guest we support today, Linux,
> always runs the interrupt handlers with interrupts disabled.
> If it suddenly changed behaviour it wouldn't stop working but our vgic
> "emulation" wouldn't be as accurate as it should be. In practice we have
> a vgic that doesn't support priorities, but neither does Linux today.

I worry quite a bit that these sorts of shortcomings will become our
supported ABI if we are not careful. This needs to go on the blocker
list for declaring the ABI stable IMHO.

I've added an entry to http://wiki.xen.org/wiki/Xen_ARM_TODO

> > 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?
> 
> The latter (pending/active bit in every one), that's why it is difficult.

Right.

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

Good, can you mention that?



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