Re: [PATCH v3 2/5] xen: make evtchn_alloc_unbound public

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 28 Mar 2022, Daniel P. Smith wrote:
> On 3/25/22 17:05, Stefano Stabellini wrote:
> > On Fri, 25 Mar 2022, Julien Grall wrote:
> >> So to me, the idea of switching to a "fake" domain or bypassing the check 
> >> is
> >> more appealing. I have a preference for the "fake" domain here.
> > 
> > As a maintainer, I am not opposed to the "fake"/"contructor" domain
> > concept.  It all depends on how many instances of this issue we are
> > going to have.  This is the only one on xen-devel so far. I don't think
> > it is worth adding a constructor domain for one instance only.  But I
> > agree with you and Daniel that if we end up with several instances, then
> > the constructor domain approach is probably the best option overall.
> > 
> > 
> > As a contributor, sadly I won't be able to spend a lot of time on this
> > in the following months. If a significant rework is required, I don't
> > think I'll be able to do it, at least not for this Xen release (and it
> > would be nice to have dom0less PV drivers in the coming release.) If
> > Daniel is willing, I could add his "idle_domain is_priv" patch to this
> > series.  Not as clean as a proper constructor domain but it would work
> > and it would be simple. It could be evolved into a nicer constructor
> > domain later.
> > 
> > This is not my preference but I could do that if Julien and Jan prefer
> > this approach and if Daniel is happy to share his patch.
> > 
> > 
> >> AFAIU, your proposal is to duplicate code. This brings other risks such as
> >> duplicated bug and more code to maintain.
> > 
> > Got it. I'll make one last attempt at a proposal that doesn't involve
> > the fake constructor domain. The goal is to avoid code duplication while
> > providing a safe way forward to make progress with only a small amount
> > of changes. What if we:
> > 
> > - rename evtchn_alloc_unbound to _evtchn_alloc_unbound (still static)
> > - add a skip_xsm parameter to _evtchn_alloc_unbound
> > - introduce a wrapper evtchn_alloc_unbound that always set skip_xsm to
> >   false (same interface as today's evtchn_alloc_unbound)
> > - introduce an __init early_evtchn_alloc_unbound public function that
> >   sets skip_xsm to true
> > 
> > This way:
> > - we have a single implementation in _evtchn_alloc_unbound (no code
> >   duplication)
> > - the only function exposed that skips the XSM check is __init
> > - evtchn_alloc_unbound continue to have the XSM check same as today
> > 
> > 
> > E.g.:
> > static int _evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool 
> > skip_xsm)
> > {
> >     ...
> > }
> > 
> > static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> > {
> >     return _evtchn_alloc_unbound(alloc, false);    
> > }
> > 
> > int __init early_evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> > {
> >     return _evtchn_alloc_unbound(alloc, true);
> > }
> > 
> > 
> > Would this be good enough for now?
> 
> Please see the RFC patch I just posted[1], IMHO I think this is a safer
> approach for this specific instance.
> 
> [1]
> https://lore.kernel.org/xen-devel/20220328203622.30961-1-dpsmith@xxxxxxxxxxxxxxxxxxxx/T/#t

I read it, the patch looks fine. I also tested it together with my
series and it solves the problem. With [1], it is just a matter of
making evtchn_alloc_unbound as is non-static.

If the other maintainers also agree with [1], then I'll just rebase on
it and limit my changes to exporting evtchn_alloc_unbound.