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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.


> > +     * 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.
> > +     */
> > +    if ( gic_events_need_delivery() )
> > +        return 1;
> > +
> > +    if ( vcpu_info(current, evtchn_upcall_pending) &&
> > +        !vcpu_info(current, evtchn_upcall_mask) &&
> > +        list_empty(&p->inflight) )
> > +        return 1;
> > +
> > +    return 0;
> > +}
> > +
> > +static inline int local_events_need_delivery(void)
> > +{
> > +    return _local_events_need_delivery(1);
> >  }
> >  
> >  int local_event_delivery_is_enabled(void);
> >  
> >  static inline void local_event_delivery_disable(void)
> >  {
> > -    /* TODO current->vcpu_info->evtchn_upcall_mask = 1; */
> > +    current->vcpu_info->evtchn_upcall_mask = 1;
> 
> You use vcpu_info in the above, I think you should use it here too for
> consistency.

OK.


> However a bigger concern is that Xen on ARM doesn't really use
> evtchn_upcall_mask on the guest side, so I'm not sure the hypervisor
> should use it internally either.

Good point.


> I don't see any callers of this disable function outside of arch/x86.

I should just remove the function then.


> >  }
> >  
> >  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.

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.
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.
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.

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

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