|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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 <oleksii.kurochko@xxxxxxxxx>Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> 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). SuchI'd omit the part in parentheses - it only repeats what the #if already has. Sure, I will drop then.
I will do that.
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?
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.
Sure, I will add. Thanks. ~ Oleksii
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |