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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 19 Apr 2013, Ian Campbell wrote:
> On Fri, 2013-04-19 at 14:03 +0100, Stefano Stabellini wrote:
> > On Fri, 19 Apr 2013, Ian Campbell wrote:
> > > On Fri, 2013-04-19 at 11:01 +0100, Stefano Stabellini wrote:
> > > > On Fri, 19 Apr 2013, Ian Campbell wrote:
> > > > > > > >  }
> > > > > > > >  
> > > > > > > >  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.
> > > > 
> > > > evtchn_upcall_mask is part of struct vcpu_info that is defined in
> > > > xen/include/public/xen.h, that is an arch-independent public header.
> > > > 
> > > > So yes, it is part of the interface that we support.
> > > 
> > > There is all sorts of stuff under xen/include/public which is not part
> > > of the interface on ARM, PVMMU, start_info, etc. You wouldn't say we
> > > support those, would you?
> > 
> > PVMMU hypercalls fail if they are called so they are not a problem;
> > start_info is just an initial set of information and there is no way to
> > map it in the guest so it is also not an issue from my POV.
> > 
> > A single field of a supported struct is another matter though.
> > 
> > There are books out there that describe that interface, I would rather
> > not change it.
> > 
> > 
> > > > I fail to see an easy way to remove it from it, we can't just add a
> > > > comment say "please don't use it".
> > > 
> > > We should take the same approach as with the startinfo, see
> > > <1363178714-5085-1-git-send-email-ian.campbell@xxxxxxxxxx> which is
> > > acked but not yet applied. which is to add a sementic (not arch
> > > specific) define which the archs can opt in or out of.
> > > 
> > > > I wouldn't be happy with ifdef'ing it out because I would rather keep
> > > > this interface arch-independent.
> > > 
> > > Right, this shouldn't be an ifdef __arm__ or anything like that. It
> > > should be ifdef XEN_HAVE_PV_INTERRUPT_MASK or some name along those
> > > lines.
> > 
> > It is still an ifdef.
> 
> That's fine, and not just fine, it is the correct thing to do.
> 
> > > > However I do see an easy way to support it on ARM.
> > > 
> > > I don't think it is actually usable at all even with this patch. But
> > > more importantly the interface is redundant, worse than the other
> > > alternative and actually implementing support for it in the guest is
> > > complex and error prone.
> > 
> > Yes, it is redundant but we are not asking the guest to implement it.
> 
> So you want to implement support for something which you don't believe
> the guest will ever need? Why?
> 
> Especially when the guest side software implementation of interrupt
> masking is known to have short comings compared with the excellent
> hardware support which the ARM platform gives us.
> 
> We shouldn't do things just because they are in theory possible. You
> don't have any actual use case for this functionality.
> 
> It is a waste of time and effort to implement it *and* it is needlessly
> introducing yet another interface to support. For absolutely no gain
> whatsoever.

Given that getting rid of evtchn_upcall_mask should not be part of the
WFI patch anyway, I am just going to ignore it for now.

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