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

Oleksii Kurochko posted 1 patch 2 weeks, 3 days ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/6948fb2823ffa41cf2eabbd87952b236e4f379bf.1783085655.git.oleksii.kurochko@gmail.com
xen/arch/arm/Kconfig       |  1 +
xen/arch/x86/Kconfig       |  1 +
xen/common/Kconfig         |  3 +++
xen/common/Makefile        |  2 +-
xen/common/domain.c        |  6 ++---
xen/common/domctl.c        | 11 ++++++---
xen/common/event_channel.c | 49 +++++++++++++++++++++++++++++++++++---
xen/common/event_channel.h |  2 ++
xen/common/event_fifo.c    | 18 +++++++++++++-
xen/common/time.c          |  2 ++
xen/include/xen/shared.h   |  8 ++++++-
xen/include/xen/time.h     |  4 ++++
12 files changed, 95 insertions(+), 12 deletions(-)
[PATCH v5] xen: introduce CONFIG_HAS_SHARED_INFO for archs without a shared page
Posted by Oleksii Kurochko 2 weeks, 3 days ago
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.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v5:
 - drop the static inline evtchn_2l_init() stub for !HAS_SHARED_INFO;
   a plain declaration is enough since the only call sites are guarded by
   IS_ENABLED(CONFIG_HAS_SHARED_INFO) and the dead call is eliminated
   before linking.
 - fix a NULL d->evtchn_port_ops dereference when CONFIG_HAS_SHARED_INFO=n
   and CONFIG_EVTCHN_FIFO=n: evtchn_init() was unconditionally calling
   evtchn_fifo_init_ops(), whose !EVTCHN_FIFO stub leaves d->evtchn_port_ops
   unset.  Gate the FIFO branch on IS_ENABLED(CONFIG_EVTCHN_FIFO) and add
   a dedicated evtchn_port_ops_none table for the remaining case. Stubs
   are shared where signatures permit: evtchn_none_noop covers both
   clear_pending and unmask; evtchn_none_false covers both is_pending and
   is_masked. evtchn_none_init() is called only from event_channel.c, so its
   declaration is kept there rather than in event_channel.h.
 - gate evtchn_fifo_init_ops() on !CONFIG_HAS_SHARED_INFO;
   its only call site is in the IS_ENABLED(CONFIG_EVTCHN_FIFO) dead branch
   of evtchn_init(), which is never reached on HAS_SHARED_INFO=y builds.
---
Changes in v4:
 - event_channel.c: drop the redundant evtchn_fifo_init_ops() in the
   else branch of evtchn_reset(); evtchn_fifo_destroy() does not undo the
   ops installed by evtchn_init(), so only the switch back to 2-level ABI
   needs an explicit call.
 - shared.h: simplify the !HAS_SHARED_INFO shared_info() definition to use
   an undefined "extern struct shared_info *shared_info_absent" instead of
   shared_info_absent() with a typeof cast.
 - Extend the commit description to note that vcpu_info()/__vcpu_info()
   uses were also audited: on !HAS_SHARED_INFO vcpu_info_area.map points at
   dummy_vcpu_info, reads are harmless, and writes in common code do not
   open a cross-domain info-leak side channel, so no code changes are
   needed on that path.
---
Changes in v3:
 - Introduce CONFIG_HAS_SHARED_INFO Kconfig symbol selected by x86
   and Arm; RISC-V does not select it.
 - Gate shared_info() macro on CONFIG_HAS_SHARED_INFO; on
   !HAS_SHARED_INFO it calls shared_info_absent() (declared, never
   defined) so any unguarded use produces a link-time error.
 - Replace runtime if (!d->shared_info) guards with IS_ENABLED() at
   call sites so both branches type-check and dead code is eliminated.
 - Guard shared_info_frame assignment in domctl.c.
 - Gate event_2l.o on CONFIG_HAS_SHARED_INFO; use FIFO ops as
   placeholder on !HAS_SHARED_INFO archs instead of dedicated stub
   ops; guard evtchn_fifo_word_from_port() against uninitialised
   d->evtchn_fifo.
 - Add static inline stubs for evtchn_2l_init() (!HAS_SHARED_INFO)
   and evtchn_fifo_init_ops() (!EVTCHN_FIFO) so call sites can use
   IS_ENABLED() without #ifdef.
 - Drop inaccurate changelog entry about "only FIFO ABI" migration.
 - Update the commit message.
 - Drop R-by: Baptiste ... as some extra checks are added.
---
Changes in v2:
 - Update commit message + subject.
 - Drop Fixes tag.
---
 xen/arch/arm/Kconfig       |  1 +
 xen/arch/x86/Kconfig       |  1 +
 xen/common/Kconfig         |  3 +++
 xen/common/Makefile        |  2 +-
 xen/common/domain.c        |  6 ++---
 xen/common/domctl.c        | 11 ++++++---
 xen/common/event_channel.c | 49 +++++++++++++++++++++++++++++++++++---
 xen/common/event_channel.h |  2 ++
 xen/common/event_fifo.c    | 18 +++++++++++++-
 xen/common/time.c          |  2 ++
 xen/include/xen/shared.h   |  8 ++++++-
 xen/include/xen/time.h     |  4 ++++
 12 files changed, 95 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 5fa89fcb2428..683ab7d25a1e 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -20,6 +20,7 @@ config ARM
 	select HAS_DEVICE_TREE_DISCOVERY
 	select HAS_DOM0LESS
 	select HAS_GRANT_CACHE_FLUSH if GRANT_TABLE
+	select HAS_SHARED_INFO
 	select HAS_STACK_PROTECTOR
 	select HAS_UBSAN
 
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 2ce4747f6ea7..49697b795259 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -29,6 +29,7 @@ config X86
 	select HAS_PCI_MSI
 	select HAS_PIRQ
 	select HAS_SCHED_GRANULARITY
+	select HAS_SHARED_INFO
 	imply HAS_SOFT_RESET
 	select HAS_UBSAN
 	select HAS_VMAP
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 5ff71480eebe..8b48d84c79e8 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -158,6 +158,9 @@ config HAS_PMAP
 config HAS_SCHED_GRANULARITY
 	bool
 
+config HAS_SHARED_INFO
+	bool
+
 config HAS_SOFT_RESET
 	bool
 
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 6018e256147f..f69d47d18934 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -12,7 +12,7 @@ obj-$(CONFIG_DEVICE_TREE_PARSE) += device-tree/
 obj-$(CONFIG_IOREQ_SERVER) += dm.o
 obj-y += domain.o
 obj-y += domid.o
-obj-y += event_2l.o
+obj-$(CONFIG_HAS_SHARED_INFO) += event_2l.o
 obj-y += event_channel.o
 obj-$(CONFIG_EVTCHN_FIFO) += event_fifo.o
 obj-$(CONFIG_GRANT_TABLE) += grant_table.o
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 8f2bfcae2890..fba8e9161937 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -316,9 +316,9 @@ void vcpu_info_reset(struct vcpu *v)
     struct domain *d = v->domain;
 
     v->vcpu_info_area.map =
-        ((v->vcpu_id < XEN_LEGACY_MAX_VCPUS)
-         ? (vcpu_info_t *)&shared_info(d, vcpu_info[v->vcpu_id])
-         : &dummy_vcpu_info);
+        IS_ENABLED(CONFIG_HAS_SHARED_INFO) && v->vcpu_id < XEN_LEGACY_MAX_VCPUS
+        ? (vcpu_info_t *)&shared_info(d, vcpu_info[v->vcpu_id])
+        : &dummy_vcpu_info;
 }
 
 static struct domain *alloc_domain_struct(void)
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index e30b38a337ac..5e5632f912d3 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -102,9 +102,14 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
 #ifdef CONFIG_MEM_PAGING
     info->paged_pages       = atomic_read(&d->paged_pages);
 #endif
-    info->shared_info_frame =
-        gfn_x(mfn_to_gfn(d, _mfn(virt_to_mfn(d->shared_info))));
-    BUG_ON(SHARED_M2P(info->shared_info_frame));
+    if ( IS_ENABLED(CONFIG_HAS_SHARED_INFO) )
+    {
+        info->shared_info_frame =
+            gfn_x(mfn_to_gfn(d, _mfn(virt_to_mfn(d->shared_info))));
+        BUG_ON(SHARED_M2P(info->shared_info_frame));
+    }
+    else
+        info->shared_info_frame = INVALID_GFN_RAW;
 
     info->cpupool = cpupool_get_id(d);
 
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index a3d18bc464e8..ff744e30559d 100644
--- 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);
+
 /*
  * 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
@@ -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);
     }
 
     write_unlock(&d->event_lock);
@@ -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 */
+
 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);
+
     d->max_evtchn_port = min_t(unsigned int, max_port, INT_MAX);
 
     d->evtchn = alloc_evtchn_bucket(d, 0);
diff --git a/xen/common/event_channel.h b/xen/common/event_channel.h
index dc94a43cc2dd..d3e5424f6ac8 100644
--- 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) {}
 #endif /* CONFIG_EVTCHN_FIFO */
 
 #endif /* EVENT_CHANNEL_H */
diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index 37cba9bc4564..56f1bcb967f2 100644
--- 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;
 
@@ -420,6 +423,18 @@ static const struct evtchn_port_ops evtchn_port_ops_fifo =
     .print_state   = evtchn_fifo_print_state,
 };
 
+/*
+ * evtchn_fifo_init_ops() only call site is in the
+ * IS_ENABLED(CONFIG_EVTCHN_FIFO) dead branch of evtchn_init(), 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)
+{
+    d->evtchn_port_ops = &evtchn_port_ops_fifo;
+}
+#endif
+
 static int map_guest_page(struct domain *d, uint64_t gfn, void **virt)
 {
     struct page_info *p;
@@ -562,7 +577,8 @@ static void setup_ports(struct domain *d, unsigned int prev_evtchns)
 
         evtchn = evtchn_from_port(d, port);
 
-        if ( guest_test_bit(d, port, &shared_info(d, evtchn_pending)) )
+        if ( IS_ENABLED(CONFIG_HAS_SHARED_INFO) &&
+             guest_test_bit(d, port, &shared_info(d, evtchn_pending)) )
             evtchn->pending = true;
 
         evtchn_fifo_set_priority(d, evtchn, EVTCHN_FIFO_PRIORITY_DEFAULT);
diff --git a/xen/common/time.c b/xen/common/time.c
index 04a65f00b35c..cdfdc53b6a17 100644
--- a/xen/common/time.c
+++ b/xen/common/time.c
@@ -89,6 +89,7 @@ struct tm gmtime(unsigned long t)
     return tbuf;
 }
 
+#ifdef CONFIG_HAS_SHARED_INFO
 void update_domain_wallclock_time(struct domain *d)
 {
     uint32_t *wc_version;
@@ -117,6 +118,7 @@ void update_domain_wallclock_time(struct domain *d)
 
     spin_unlock(&wc_lock);
 }
+#endif /* CONFIG_HAS_SHARED_INFO */
 
 /* Set clock to <secs,usecs> after 00:00:00 UTC, 1 January, 1970. */
 void do_settime(u64 secs, unsigned int nsecs, u64 system_time_base)
diff --git a/xen/include/xen/shared.h b/xen/include/xen/shared.h
index 5b71342cab32..f20a46801181 100644
--- 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)
+#else
+extern struct shared_info *shared_info_absent;
+#define shared_info(d, field) (((void)(d), shared_info_absent)->field)
+#endif /* CONFIG_HAS_SHARED_INFO */
+
 #define vcpu_info(v, field)        \
         __vcpu_info(v, (vcpu_info_t *)(v)->vcpu_info_area.map, field)
 
diff --git a/xen/include/xen/time.h b/xen/include/xen/time.h
index e9c0822e6f31..2f872f580ffc 100644
--- 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
 
 extern void do_settime(
     u64 secs, unsigned int nsecs, u64 system_time_base);
-- 
2.54.0
Re: [PATCH v5] xen: introduce CONFIG_HAS_SHARED_INFO for archs without a shared page
Posted by Jan Beulich 2 weeks, 1 day ago
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
Re: [PATCH v5] xen: introduce CONFIG_HAS_SHARED_INFO for archs without a shared page
Posted by Oleksii Kurochko 1 week, 6 days ago

On 7/8/26 12:52 PM, Jan Beulich wrote:
>> +#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 thought an opposite that it is good to have cf_check when pointer to 
function is used. So why we have to make an exception in this case?

~ Oleksii
Re: [PATCH v5] xen: introduce CONFIG_HAS_SHARED_INFO for archs without a shared page
Posted by Jan Beulich 1 week, 6 days ago
On 10.07.2026 15:31, Oleksii Kurochko wrote:
> On 7/8/26 12:52 PM, Jan Beulich wrote:
>>> +#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 thought an opposite that it is good to have cf_check when pointer to 
> function is used. So why we have to make an exception in this case?

I didn't ask to drop the cf_check (and you can't really as long as these
functions may be used by x86). I asked to limit the number of them we have
(in a particular configuration) as much as possible. I.e. I was merely
trying to explain why it is relevant to have the #if around this as tight
as possible.

Jan
Re: [PATCH v5] xen: introduce CONFIG_HAS_SHARED_INFO for archs without a shared page
Posted by Oleksii Kurochko 1 week, 6 days ago

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