Re: [PATCH v5] xen: introduce CONFIG_HAS_SHARED_INFO for archs without a shared page

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 7/8/26 12:52 PM, Jan Beulich wrote:
> On 06.07.2026 17:57, Oleksii Kurochko wrote:
> > --- a/xen/common/event_channel.c
> > +++ b/xen/common/event_channel.c
> > @@ -40,6 +40,9 @@
> >   
> >   #define consumer_is_xen(e) (!!(e)->xen_consumer)
> >   
> > +/* Defined below when !CONFIG_HAS_SHARED_INFO; call is DCE'd otherwise. */
> > +void evtchn_none_init(struct domain *d);
> The definition wants to be static, so this declaration needs to become
> conditional. Assuming the evtchn_port_ops_none block can move up in the file,
> it could be put in an #else there.
I will move  evtchn_port_ops_none block up and add #else to it.

> > @@ -1323,9 +1326,13 @@ int evtchn_reset(struct domain *d, bool resuming)
> >           rc = -EAGAIN;
> >       else if ( d->evtchn_fifo )
> >       {
> > -        /* Switching back to 2-level ABI. */
> >           evtchn_fifo_destroy(d);
> > -        evtchn_2l_init(d);
> > +
> > +        if ( IS_ENABLED(CONFIG_HAS_SHARED_INFO) )
> > +            /* Switching back to 2-level ABI. */
> > +            evtchn_2l_init(d);
> > +        else
> > +            evtchn_none_init(d);
> >       }
> Do we really need to call evtchn_none_init() here when FIFO is available?
Agree, when FIFO is available there is no need to call 
evtchn_none_init() so it seems like it would be better to have the 
similar to what we have in evtchn_init():
@@ -1331,6 +1361,8 @@ int evtchn_reset(struct domain *d, bool resuming)
         if ( IS_ENABLED(CONFIG_HAS_SHARED_INFO) )
             /* Switching back to 2-level ABI. */
             evtchn_2l_init(d);
+        else if ( IS_ENABLED(CONFIG_EVTCHN_FIFO) )
+            evtchn_fifo_init_ops(d);
         else
             evtchn_none_init(d);
     }

> This is connected to ...
>
> > @@ -1622,9 +1629,45 @@ void evtchn_check_pollers(struct domain *d, unsigned int 
> > port)
> >       }
> >   }
> >   
> > +#ifndef CONFIG_HAS_SHARED_INFO
> > +/*
> > + * Placeholder ops for domains with neither a shared_info page nor (yet)
> > + * a FIFO control block.  None of these are ever reachable in practice;
> > + * they only exist to keep d->evtchn_port_ops non-NULL.
> > + */
> > +static void cf_check evtchn_none_set_pending(
> > +    struct vcpu *v, struct evtchn *evtchn) {}
> > +static void cf_check evtchn_none_noop(
> > +    struct domain *d, struct evtchn *evtchn) {}
> > +static bool cf_check evtchn_none_false(
> > +    const struct domain *d, const struct evtchn *evtchn) { return false; }
> > +static void cf_check evtchn_none_print_state(
> > +    struct domain *d, const struct evtchn *evtchn) {}
> > +
> > +static const struct evtchn_port_ops evtchn_port_ops_none = {
> > +    .set_pending   = evtchn_none_set_pending,
> > +    .clear_pending = evtchn_none_noop,
> > +    .unmask        = evtchn_none_noop,
> > +    .is_pending    = evtchn_none_false,
> > +    .is_masked     = evtchn_none_false,
> > +    .print_state   = evtchn_none_print_state,
> > +};
> > +
> > +void evtchn_none_init(struct domain *d)
> > +{
> > +    d->evtchn_port_ops = &evtchn_port_ops_none;
> > +}
> > +#endif /* !CONFIG_HAS_SHARED_INFO */
> ... we wondering whether any of this is needed when FIFO is available. In
> v4 all that was noticed was that SHARED_INFO=n together with EVTCHN_FIFO=n
> is a problem. And having fewer cf_check functions in the build is always a
> win (I think).
I will update #ifndef to:

#if !defined(CONFIG_HAS_SHARED_INFO) && !defined(CONFIG_EVTCHN_FIFO)


> As to the comment saying "None of these are ever reachable in practice":
> What do you base this on? In the SHARED_INFO=n + EVTCHN_FIFO=n case they
> look reachable to me.
I thought about that it is unlikely that both of the configs will be =n. 
I will reword the comment to:
/*
 * Placeholder ops for domains with neither a shared_info page nor a FIFO
 * control block (CONFIG_HAS_SHARED_INFO=n and CONFIG_EVTCHN_FIFO=n). Such
 * a domain has no ABI to record event state in, so these are reachable
 * whenever an event is delivered to (or queried on) one of its ports; they
 * just discard/no-op it.  They exist to keep d->evtchn_port_ops non-NULL.
 */

> >   int evtchn_init(struct domain *d, unsigned int max_port)
> >   {
> > -    evtchn_2l_init(d);
> > +    if ( IS_ENABLED(CONFIG_HAS_SHARED_INFO) )
> > +        evtchn_2l_init(d);
> > +    else if ( IS_ENABLED(CONFIG_EVTCHN_FIFO) )
> > +        evtchn_fifo_init_ops(d);
> > +    else
> > +        evtchn_none_init(d);
> Note how here you actually call evtchn_none_init() only in the one special
> case. Imo this model should be followed also in evtchn_reset().
>
> > --- a/xen/common/event_channel.h
> > +++ b/xen/common/event_channel.h
> > @@ -55,6 +55,7 @@ struct evtchn_expand_array;
> >   int evtchn_fifo_init_control(struct evtchn_init_control *init_control);
> >   int evtchn_fifo_expand_array(const struct evtchn_expand_array *expand_array);
> >   void evtchn_fifo_destroy(struct domain *d);
> > +void evtchn_fifo_init_ops(struct domain *d);
> >   #else
> >   static inline int evtchn_fifo_init_control(struct evtchn_init_control 
> > *init_control)
> >   {
> > @@ -68,6 +69,7 @@ static inline void evtchn_fifo_destroy(struct domain *d)
> >   {
> >       return;
> >   }
> > +static inline void evtchn_fifo_init_ops(struct domain *d) {}
> Why would this be needed? You (again) only need a declaration, just that it
> needs to live outside of the #ifdef.
Agree. I will drop that.

Thanks.

~ Oleksii