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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-04-18 at 18:59 +0100, Stefano Stabellini wrote:
> On Wed, 10 Apr 2013, Ian Campbell wrote:
> > > diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
> > > index 10f58af..99a2b13 100644
> > > --- a/xen/include/asm-arm/event.h
> > > +++ b/xen/include/asm-arm/event.h
> > > @@ -1,27 +1,55 @@
> > >  #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(void)
> > > +static inline int _local_events_need_delivery(int check_masked)
> > >  {
> > > -    /* TODO
> > > -     * return (vcpu_info(v, evtchn_upcall_pending) &&
> > > -                        !vcpu_info(v, evtchn_upcall_mask)); */
> > > +    struct pending_irq *p = irq_to_pending(current, 
> > > VGIC_IRQ_EVTCHN_CALLBACK);
> > > +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> > > +
> > > +    /* guest IRQs are masked */
> > > +    if ( check_masked && (regs->cpsr & PSR_IRQ_MASK) )
> > >          return 0;
> > > +
> > > +    /* XXX: if the first interrupt has already been delivered, we should
> > 
> > XXX usually means "TODO", whereas this is just a normal comment I think?
> 
> It is something that we should implement at some point, that's why it is
> marked as XXX or TODO.

Oh, I read this as describing the reason for the following
gic_events_need_delivery, not describing something which is missing
there.

> > >  }
> > >  
> > >  static inline void local_event_delivery_enable(void)
> > 
> > This one is called from do_block in common code but if the guest never
> > sets this then it seems a bit pointless. Perhaps this should be
> > manipulating CPSR_I instead?
> 
> I think it's fine to keep it as it is because in the future a guest on
> ARM might decide to use it and I think it's an interface that we should
> support.

No it isn't, it is a piece of PV legacy which introduces difficult to
solve races (all that critical section stuff) on the event re-enable
path. There is no reason at all to use it when you have proper
virtualised interrupt mask functionality in the hardware.

> Clearing CPSR_I might be the right thing to do in response to a
> SCHEDOP_block hypercall, but it is not something we can do in the WFI
> trap.

That's fine, we can make both behave in an appropriate manner, assuming
we need to keep both at all.

> Given that the SCHEDOP_block hypercall is never called and there is no
> reason to call it thanks to the existance of WFI, I say we should keep
> local_event_delivery_enable as it is in this patch.

It may well be appropriate for local_event_delivery_enable to do nothing
or to clear the interrupt flag, that is something we can decide, and you
make a good argument that in the WFI case it should do nothing. I expect
the right answer is that your new do_block function should only
optionally reenable interrupts, the SCHEDOP_block and WFI can both have
their correct semantics.

However it is completely clear to me is that this function shouldn't be
touching the s/w event channel mask on ARM because it conceptually
simply does not exist.

> Also PV on HVM x86 guests have the same issue and at the moment they
> don't clear the interrupt flag in local_event_delivery_enable either.

PV on HVM x86 doesn't call SCHEDOP_block because it doesn't use the PV
IRQ ops. I expect it will use the hlt instruction instead.
evtchn_upcall_mask is also meaningless and unused for such guests as
well.

So SCHEDOP_block is probably equivalently buggy on x86 PVHVM, but can be
forgiven somewhat because at least the bit has meaning in some x86
domain modes.

> Maybe we should find a way to return an error if a guest calls
> SCHEDOP_block on ARM?

That would work also.

Ian.


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