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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 18 Apr 2013, 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.
> 
> 
> > > +     * 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.

Actually a better way to put it is that evtchn_upcall_mask is already
part of an interface that we support.

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