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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> @@ -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?
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).

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.

>  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.

Jan