Not only domctl-op could do foreign updates to guest state, some hypercall,
like HVMOP_set_param, could also do, and they all need domctl_lock for
syncronization.
Later, we will introduce CONFIG_DOMCTL to wrap domctl.c. In order to
continue using domctl_lock when CONFIG_DOMCTL not defined, we'd like to move
domctl_lock_acquire/release() out of domctl.c, and into more common space,
domain.c
The movement could also fix CI error of a randconfig picking both
PV_SHIM_EXCLUSIVE=y and HVM=y results in hvm.c being built, but
domctl.c not being built, which leaves domctl_lock_acquire/release()
undefined, causing linking to fail.

Fixes: 568f806cba4c ("xen/x86: remove "depends on !PV_SHIM_EXCLUSIVE"")
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
 xen/common/domain.c | 29 +++++++++++++++++++++++++++++
 xen/common/domctl.c | 29 -----------------------------
 2 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index b74d4c7549..6145071e55 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -66,6 +66,35 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock);
 static struct domain *domain_hash[DOMAIN_HASH_SIZE];
 struct domain *domain_list;
 
+static DEFINE_SPINLOCK(domctl_lock);
+
+bool domctl_lock_acquire(void)
+{
+    /*
+     * Caller may try to pause its own VCPUs. We must prevent deadlock
+     * against other non-domctl routines which try to do the same.
+     */
+    if ( !spin_trylock(&current->domain->hypercall_deadlock_mutex) )
+        return 0;
+
+    /*
+     * Trylock here is paranoia if we have multiple privileged domains. Then
+     * we could have one domain trying to pause another which is spinning
+     * on domctl_lock -- results in deadlock.
+     */
+    if ( spin_trylock(&domctl_lock) )
+        return 1;
+
+    spin_unlock(&current->domain->hypercall_deadlock_mutex);
+    return 0;
+}
+
+void domctl_lock_release(void)
+{
+    spin_unlock(&domctl_lock);
+    spin_unlock(&current->domain->hypercall_deadlock_mutex);
+}
+
 /*
  * Insert a domain into the domlist/hash.  This allows the domain to be looked
  * up by domid, and therefore to be the subject of hypercalls/etc.
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 99de77380f..455fbc5160 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -35,8 +35,6 @@
 #include <public/domctl.h>
 #include <xsm/xsm.h>
 
-static DEFINE_SPINLOCK(domctl_lock);
-
 static int nodemask_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_nodemap,
                                      const nodemask_t *nodemask)
 {
@@ -65,33 +63,6 @@ static inline int is_free_domid(domid_t dom)
     return 0;
 }
 
-bool domctl_lock_acquire(void)
-{
-    /*
-     * Caller may try to pause its own VCPUs. We must prevent deadlock
-     * against other non-domctl routines which try to do the same.
-     */
-    if ( !spin_trylock(&current->domain->hypercall_deadlock_mutex) )
-        return 0;
-
-    /*
-     * Trylock here is paranoia if we have multiple privileged domains. Then
-     * we could have one domain trying to pause another which is spinning
-     * on domctl_lock -- results in deadlock.
-     */
-    if ( spin_trylock(&domctl_lock) )
-        return 1;
-
-    spin_unlock(&current->domain->hypercall_deadlock_mutex);
-    return 0;
-}
-
-void domctl_lock_release(void)
-{
-    spin_unlock(&domctl_lock);
-    spin_unlock(&current->domain->hypercall_deadlock_mutex);
-}
-
 void vnuma_destroy(struct vnuma_info *vnuma)
 {
     if ( vnuma )
-- 
2.34.1

On 23.07.2025 08:53, Penny Zheng wrote:
> Not only domctl-op could do foreign updates to guest state, some hypercall,
> like HVMOP_set_param, could also do, and they all need domctl_lock for
> syncronization.

What's "all" here? HVMOP_set_param is the sole such case, and hence - aiui -
it uses the domctl lock only to lock out actual domctl-s. Of which there are
not going to be any with DOMCTL=n, ...

> Later, we will introduce CONFIG_DOMCTL to wrap domctl.c. In order to
> continue using domctl_lock when CONFIG_DOMCTL not defined, we'd like to move
> domctl_lock_acquire/release() out of domctl.c, and into more common space,
> domain.c

... as you indicate is where we're going to move to as a possibility. Hence
the domctl lock is meaningless without DOMCTL=y. At which point moving it out
of domctl.c is the wrong course of action. With DOMCTL=n, we'll rather need
stub domctl_lock_{acquire,release}(), which then likely would do nothing (or
alternatively "#ifdef CONFIG_DOMCTL" could be put around the two uses in
hvm_set_param()).

> The movement could also fix CI error of a randconfig picking both
> PV_SHIM_EXCLUSIVE=y and HVM=y results in hvm.c being built, but
> domctl.c not being built, which leaves domctl_lock_acquire/release()
> undefined, causing linking to fail.

If the work towards DOMCTL as a Kconfig control can't be progressed pretty
quickly (and I don't expect that to be much faster than the SYSCTL work),
then I expect our prime option (apart from reverting the earlier change) is
to accept a bunch of dead code in the shim binary again, by moving domctl.o
out of the PV_SHIM_EXCLUSIVE common/Makefile section as well (plus whatever
other adjustments are necessary to undo that earlier dead code elimination).

Jan