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

On 8/13/26 4:54 PM, Jan Beulich wrote:
> 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 <[email protected]>
> 
> Reviewed-by: Jan Beulich <[email protected]>

Thanks.

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

Sure, I will drop then.

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

I will do that.

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

I think then it will be needed to fix a lot of comments. My suggestion 
is the following:

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 0911808fe861..809638ce4bfd 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -71,10 +71,23 @@ 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. */
+/*
+ * Declaration only; the call in evtchn_preinit() is DCE'd unless both
+ * configs are off.
+ */
  void evtchn_none_init(struct domain *d);
  #endif /* !CONFIG_HAS_SHARED_INFO && !CONFIG_EVTCHN_FIFO */

+static void evtchn_preinit(struct domain *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);
+}
+
  /*
   * Lock an event channel exclusively. This is allowed only when the 
channel is
   * free or unbound either when taking or when releasing the lock, as any
@@ -1359,15 +1372,9 @@ int evtchn_reset(struct domain *d, bool resuming)
          rc = -EAGAIN;
      else if ( d->evtchn_fifo )
      {
+        /* Switching back to the default ABI. */
          evtchn_fifo_destroy(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);
+        evtchn_preinit(d);
      }

      write_unlock(&d->event_lock);
@@ -1666,12 +1673,7 @@ void evtchn_check_pollers(struct domain *d, 
unsigned int port)

  int evtchn_init(struct domain *d, unsigned int max_port)
  {
-    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);
+    evtchn_preinit(d);

      d->max_evtchn_port = min_t(unsigned int, max_port, INT_MAX);

diff --git a/xen/common/event_channel.h b/xen/common/event_channel.h
index c8ee09807008..156514fefff6 100644
--- a/xen/common/event_channel.h
+++ b/xen/common/event_channel.h
@@ -71,8 +71,8 @@ static inline void evtchn_fifo_destroy(struct domain *d)
  #endif /* CONFIG_EVTCHN_FIFO */

  /*
- * Declaration only when !CONFIG_EVTCHN_FIFO; the (dead) calls in
- * evtchn_init() and evtchn_reset() are DCE'd in that case.
+ * Declaration only when !CONFIG_EVTCHN_FIFO; the (dead) call in
+ * evtchn_preinit() is DCE'd in that case.
   */
  void evtchn_fifo_init_ops(struct domain *d);

diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index 3b6e619c5278..f11c4c16efa3 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -423,10 +423,9 @@ static const struct evtchn_port_ops 
evtchn_port_ops_fifo =
  };

  /*
- * evtchn_fifo_init_ops()'s only call sites are in the
- * IS_ENABLED(CONFIG_EVTCHN_FIFO) dead branches of evtchn_init() and
- * evtchn_reset(), which are never reached on HAS_SHARED_INFO=y builds
- * because of DCE.
+ * evtchn_fifo_init_ops()'s only call site is the
+ * IS_ENABLED(CONFIG_EVTCHN_FIFO) dead branch of evtchn_preinit(), which is
+ * never reached on HAS_SHARED_INFO=y builds because of DCE.
   */
  #ifndef CONFIG_HAS_SHARED_INFO
  void evtchn_fifo_init_ops(struct domain *d)
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 930190054cf0..595dedf0792c 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -211,7 +211,7 @@ static bool evtchn_usable(const struct evtchn *evtchn)

  void evtchn_check_pollers(struct domain *d, unsigned int port);

-/* Close all event channels and reset to 2-level ABI. */
+/* Close all event channels and reset to the default ABI. */
  int evtchn_reset(struct domain *d, bool resuming);

Does it look good for you?

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

Agree, we could drop it.

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

No, I will fix that.

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

Sure, I will add.

Thanks.

~ Oleksii