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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

> We only need to provide support for it in Xen, and we have plenty of
> examples from Xen x86.

Ian.


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