Re: [PATCH v7 01/20] xen: introduce CONFIG_HAS_SHARED_INFO for archs without a shared page

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.08.2026 17:47, Oleksii Kurochko wrote:
> On architectures that run guests in dom0less mode without the PV ABI
> (currently RISC-V), no shared_info page is allocated and d->shared_info
> remains NULL throughout the domain lifetime. Several places in common
> code access d->shared_info through the shared_info() macro or directly,
> causing UBSAN null-pointer errors on such architectures.
> 
> Rather than adding runtime NULL guards that are logically unreachable
> on x86 and Arm (where shared_info is always allocated), introduce a new
> Kconfig symbol CONFIG_HAS_SHARED_INFO selected by x86 and Arm.
> 
> On !HAS_SHARED_INFO the shared_info() macro expands to a dereference
> of shared_info_absent, an extern pointer that is declared but
> intentionally never defined.  Any use of shared_info() that is not
> dead-code-eliminated will therefore cause a link-time failure, making
> missed guards impossible to overlook.
> 
> The 2L event-channel ops call shared_info() and must not be compiled on
> architectures without a shared_info page, so event_2l.o is gated on
> CONFIG_HAS_SHARED_INFO. On such architectures evtchn_init() installs the
> FIFO ops as a placeholder instead, so that a later guest opt-in to the
> FIFO ABI via EVTCHNOP_init_control has no special-casing to do; if FIFO
> support itself is also unavailable (!CONFIG_EVTCHN_FIFO), a dedicated
> no-op evtchn_port_ops_none table is installed instead, so that
> d->evtchn_port_ops is never NULL. evtchn_fifo_word_from_port() is
> guarded against uninitialised d->evtchn_fifo so the FIFO ops are safe
> before evtchn_fifo_init_control() is called by the guest.
> 
> With CONFIG_HAS_SHARED_INFO=n all vCPUs fall back to the global
> dummy_vcpu_info, so writes through vcpu_info() could leak data between
> vCPUs. Reviewing the write paths in common code: the write in
> map_guest_area() stores the constant ~0 so nothing serious would happen
> if it were leaked; the event_2l.c paths are not compiled on
> !HAS_SHARED_INFO, as event_2l.o is gated on CONFIG_HAS_SHARED_INFO; the
> write in vcpu_info_populate() targets the new mapping buffer, not
> dummy_vcpu_info.
> 
> Outside common code, the remaining writes are x86 PV-specific, for which
> CONFIG_HAS_SHARED_INFO=y. No code changes are needed.
> 
> Finally, struct domain's shared_info field itself is gated on
> CONFIG_HAS_SHARED_INFO, as it would otherwise be a permanently NULL
> pointer: every user of it is either arch code for an architecture that
> selects HAS_SHARED_INFO, or common code already guarded by the same
> Kconfig symbol.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
albeit still with a number of comments / requests:

> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -40,6 +40,41 @@
>  
>  #define consumer_is_xen(e) (!!(e)->xen_consumer)
>  
> +#if !defined(CONFIG_HAS_SHARED_INFO) && !defined(CONFIG_EVTCHN_FIFO)
> +/*
> + * 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

I'd omit the part in parentheses - it only repeats what the #if already
has.

> + * 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.
> + */
> +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,
> +};
> +
> +static void evtchn_none_init(struct domain *d)
> +{
> +    d->evtchn_port_ops = &evtchn_port_ops_none;
> +}
> +#else
> +/* Declaration only; the calls below are DCE'd unless both configs are off. 
> */
> +void evtchn_none_init(struct domain *d);
> +#endif /* !CONFIG_HAS_SHARED_INFO && !CONFIG_EVTCHN_FIFO */

I think the (inverted) comment would be more valuable on the #else line.

> @@ -1324,9 +1359,15 @@ 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 if ( IS_ENABLED(CONFIG_EVTCHN_FIFO) )
> +            evtchn_fifo_init_ops(d);
> +        else
> +            evtchn_none_init(d);

This being the same as ...

> @@ -1625,7 +1666,13 @@ void evtchn_check_pollers(struct domain *d, unsigned 
> int port)
>  
>  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);

... this: Maybe have a small helper (evtchn_preinit()?), to reduce the
duplication? Would require comment updates then as well.

> --- a/xen/common/event_fifo.c
> +++ b/xen/common/event_fifo.c
> @@ -62,6 +62,9 @@ static inline event_word_t 
> *evtchn_fifo_word_from_port(const struct domain *d,
>       */
>      smp_rmb();
>  
> +    if ( unlikely(!d->evtchn_fifo) )
> +        return NULL;
> +
>      if ( unlikely(port >= d->evtchn_fifo->num_evtchns) )
>          return NULL;
>  
> @@ -419,6 +422,19 @@ static const struct evtchn_port_ops evtchn_port_ops_fifo 
> =
>      .print_state   = evtchn_fifo_print_state,
>  };
>  
> +/*
> + * evtchn_fifo_init_ops()'s only call sites are in the
> + * IS_ENABLED(CONFIG_EVTCHN_FIFO) dead branches of evtchn_init() and

Perhaps better drop "dead" from here; those branches are dead only when ...

> + * evtchn_reset(), which are never reached on HAS_SHARED_INFO=y builds
> + * because of DCE.

... HAS_SHARED_INFO=y, not generally.

> --- a/xen/include/xen/shared.h
> +++ b/xen/include/xen/shared.h
> @@ -43,7 +43,13 @@ typedef struct vcpu_info vcpu_info_t;
>  
>  extern vcpu_info_t dummy_vcpu_info;
>  
> -#define shared_info(d, field)      __shared_info(d, (d)->shared_info, field)
> +#ifdef CONFIG_HAS_SHARED_INFO
> +#define shared_info(d, field) __shared_info(d, (d)->shared_info, field)

Is there a reason this line cannot simply be kept as it was?

> --- a/xen/include/xen/time.h
> +++ b/xen/include/xen/time.h
> @@ -66,7 +66,11 @@ struct tm wallclock_time(uint64_t *ns);
>  #define version_update_begin(v) (((v) + 1) | 1)
>  #define version_update_end(v)   ((v) + 1)
>  extern void update_vcpu_system_time(struct vcpu *v);
> +#ifdef CONFIG_HAS_SHARED_INFO
>  extern void update_domain_wallclock_time(struct domain *d);
> +#else
> +static inline void update_domain_wallclock_time(struct domain *d) {}
> +#endif

Perhaps best to insert a blank line ahead of the #ifdef.

Jan