[PATCH v8 0/9] remove libxenctrl usage from xenstored

[PATCH v8 0/9] remove libxenctrl usage from xenstored

[Juergen Gross]

Xenstored is using libxenctrl for only one purpose: to get information
about state of domains.

This patch series is removing that dependency by introducing a new
stable interface which can be used by xenstored instead.

There was a RFC series sent out 3 years ago, which I have taken as a
base and by addressing all comments from back then.

The main differences since that RFC series are:

- Instead of introducing an new main hypercall for a stable management
  interface I have just added a new domctl sub-op, as requested in 2021.

- I have added a new library libxenmanage for easy use of the new
  stable hypervisor interface. Main motivation for adding the library
  was the recent attempt to decouple oxenstored from the Xen git tree.
  By using the new library, oxenstored could benefit in the same way as
  xenstored from the new interface: it would be possible to rely on
  stable libraries only.

- Mini-OS has gained some more config options recently, so it was rather
  easy to make xenstore[pvh]-stubdom independent from libxenctrl, too.

Please note that the last 4 patches can be committed only after the
related Mini-OS patch "config: add support for libxenmanage" has gone in
AND the Mini-OS commit-id has been updated in Config.mk accordingly! The
Mini-OS patch has been Acked already, so it can go in as soon as patch
5 of this series (the one introducing libxenmanage) has been committed.

As patches 1 and 2 change current behavior, Jan didn't want to give his
Ack (he didn't reject the patch either). So I'm asking other "Rest"
maintainers to look at those patches specifically.

Changes in V2:
- new patch 1
- former patch 5 mover earlier, now patch 2 (can go in without the rest
  of the series)
- addressed comments

Changes in V3:
- addressed comments

Changes in V4:
- patches 1 and 3 of V3 dropped, as already committed
- addressed comments

Changes in V5:
- addressed comments

Changes in V6:
- patch 1 of V5 has been committed
- new patches 1-3 for fixing a race and avoiding new races with the
  added functionality (result of a comment by Jan Beulich)
- rework of locking in patch 4 (Jan Beulich)

Changes in V7:
- addressed comments
- rebase

Changes in V8:
- patch 1 of v7 has gone in
- addressed comments
- new patches 7-9 using the new unique_id and xenmanage_poll_changed_domain()

Juergen Gross (9):
  xen/events: don't allow binding a global virq from any domain
  xen/events: allow setting of global virq handler only for unbound
    virqs
  xen: add bitmap to indicate per-domain state changes
  xen: add new domctl get_changed_domain
  tools/libs: add a new libxenmanage library
  tools/xenstored: use new stable interface instead of libxenctrl
  docs: update xenstore migration stream definition
  tools/xenstored: use unique_id to identify new domain with same domid
  tools/xenstored: use xenmanage_poll_changed_domain()

 docs/designs/xenstore-migration.md     |  14 ++-
 stubdom/Makefile                       |   8 +-
 stubdom/mini-os.mk                     |   1 +
 tools/flask/policy/modules/dom0.te     |   1 +
 tools/flask/policy/modules/xen.if      |   5 +-
 tools/flask/policy/modules/xenstore.te |   1 +
 tools/include/xenmanage.h              |  92 ++++++++++++++
 tools/libs/Makefile                    |   1 +
 tools/libs/manage/Makefile             |  10 ++
 tools/libs/manage/Makefile.common      |   3 +
 tools/libs/manage/core.c               | 168 +++++++++++++++++++++++++
 tools/libs/manage/libxenmanage.map     |   8 ++
 tools/libs/uselibs.mk                  |   2 +
 tools/xenstored/Makefile               |   2 +-
 tools/xenstored/Makefile.common        |   2 +-
 tools/xenstored/core.h                 |   1 -
 tools/xenstored/domain.c               | 145 ++++++++++++++-------
 tools/xenstored/lu.c                   |   1 +
 tools/xenstored/lu_daemon.c            |   1 +
 tools/xenstored/xenstore_state.h       |   2 +-
 xen/common/domain.c                    | 125 ++++++++++++++++++
 xen/common/domctl.c                    |  18 ++-
 xen/common/event_channel.c             |  72 ++++++++++-
 xen/include/public/domctl.h            |  26 ++++
 xen/include/xen/event.h                |   4 +
 xen/include/xen/sched.h                |   5 +
 xen/include/xsm/dummy.h                |   8 ++
 xen/include/xsm/xsm.h                  |   6 +
 xen/xsm/dummy.c                        |   1 +
 xen/xsm/flask/hooks.c                  |   7 ++
 xen/xsm/flask/policy/access_vectors    |   2 +
 31 files changed, 677 insertions(+), 65 deletions(-)
 create mode 100644 tools/include/xenmanage.h
 create mode 100644 tools/libs/manage/Makefile
 create mode 100644 tools/libs/manage/Makefile.common
 create mode 100644 tools/libs/manage/core.c
 create mode 100644 tools/libs/manage/libxenmanage.map

-- 
2.43.0

[PATCH v8 1/9] xen/events: don't allow binding a global virq from any domain

[Juergen Gross]

Today Xen will happily allow binding a global virq by a domain which
isn't configured to receive it. This won't result in any bad actions,
but the bind will appear to have succeeded with no event ever being
received by that event channel.

Instead of allowing the bind, error out if the domain isn't set to
handle that virq. Note that this check is inside the write_lock() on
purpose, as a future patch will put a related check into
set_global_virq_handler() with the addition of using the same lock.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V6:
- new patch
V7:
- move handling domain check inside locked region (Jan Beulich)
- style fix (Jan Beulich)
---
 xen/common/event_channel.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 46281b16ce..cd6f5a1211 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -120,6 +120,13 @@ static uint8_t get_xen_consumer(xen_event_channel_notification_t fn)
 /* Get the notification function for a given Xen-bound event channel. */
 #define xen_notification_fn(e) (xen_consumers[(e)->xen_consumer-1])
 
+static struct domain *__read_mostly global_virq_handlers[NR_VIRQS];
+
+static struct domain *get_global_virq_handler(unsigned int virq)
+{
+    return global_virq_handlers[virq] ?: hardware_domain;
+}
+
 static bool virq_is_global(unsigned int virq)
 {
     switch ( virq )
@@ -469,6 +476,7 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port)
     struct domain *d = current->domain;
     int            virq = bind->virq, vcpu = bind->vcpu;
     int            rc = 0;
+    bool           is_global;
 
     if ( (virq < 0) || (virq >= ARRAY_SIZE(v->virq_to_evtchn)) )
         return -EINVAL;
@@ -478,8 +486,9 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port)
     * speculative execution.
     */
     virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn));
+    is_global = virq_is_global(virq);
 
-    if ( virq_is_global(virq) && (vcpu != 0) )
+    if ( is_global && vcpu != 0 )
         return -EINVAL;
 
     if ( (v = domain_vcpu(d, vcpu)) == NULL )
@@ -487,6 +496,12 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port)
 
     write_lock(&d->event_lock);
 
+    if ( is_global && get_global_virq_handler(virq) != d )
+    {
+        rc = -EBUSY;
+        goto out;
+    }
+
     if ( read_atomic(&v->virq_to_evtchn[virq]) )
     {
         rc = -EEXIST;
@@ -965,15 +980,13 @@ void send_guest_pirq(struct domain *d, const struct pirq *pirq)
     }
 }
 
-static struct domain *global_virq_handlers[NR_VIRQS] __read_mostly;
-
 static DEFINE_SPINLOCK(global_virq_handlers_lock);
 
 void send_global_virq(uint32_t virq)
 {
     ASSERT(virq_is_global(virq));
 
-    send_guest_global_virq(global_virq_handlers[virq] ?: hardware_domain, virq);
+    send_guest_global_virq(get_global_virq_handler(virq), virq);
 }
 
 int set_global_virq_handler(struct domain *d, uint32_t virq)
-- 
2.43.0

[PATCH v8 2/9] xen/events: allow setting of global virq handler only for unbound virqs

[Juergen Gross]

XEN_DOMCTL_set_virq_handler will happily steal a global virq from the
current domain having bound it and assign it to another domain. The
former domain will just never receive any further events for that
virq without knowing what happened.

Change the behavior to allow XEN_DOMCTL_set_virq_handler only if the
virq in question is not bound by the current domain allowed to use it.

Currently the only user of XEN_DOMCTL_set_virq_handler in the Xen code
base is init-xenstore-domain, so changing the behavior like above will
not cause any problems.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V6:
- new patch
---
 xen/common/event_channel.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index cd6f5a1211..4dba59efa2 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -991,7 +991,8 @@ void send_global_virq(uint32_t virq)
 
 int set_global_virq_handler(struct domain *d, uint32_t virq)
 {
-    struct domain *old;
+    struct domain *old, *hdl;
+    const struct vcpu *v;
     int rc = 0;
 
     if (virq >= NR_VIRQS)
@@ -1023,7 +1024,22 @@ int set_global_virq_handler(struct domain *d, uint32_t virq)
     else
     {
         old = global_virq_handlers[virq];
-        global_virq_handlers[virq] = d;
+        hdl = get_global_virq_handler(virq);
+        if ( hdl != d )
+        {
+            read_lock(&hdl->event_lock);
+
+            v = hdl->vcpu[0];
+            if ( v && read_atomic(&v->virq_to_evtchn[virq]) )
+            {
+                rc = -EBUSY;
+                old = d;
+            }
+            else
+                global_virq_handlers[virq] = d;
+
+            read_unlock(&hdl->event_lock);
+        }
     }
 
     spin_unlock(&global_virq_handlers_lock);
-- 
2.43.0

[PATCH v8 3/9] xen: add bitmap to indicate per-domain state changes

[Juergen Gross]

Add a bitmap with one bit per possible domid indicating the respective
domain has changed its state (created, deleted, dying, crashed,
shutdown).

Registering the VIRQ_DOM_EXC event will result in setting the bits for
all existing domains and resetting all other bits.

As the usage of this bitmap is tightly coupled with the VIRQ_DOM_EXC
event, it is meant to be used only by a single consumer in the system,
just like the VIRQ_DOM_EXC event.

Resetting a bit will be done in a future patch.

This information is needed for Xenstore to keep track of all domains.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
V2:
- use DOMID_FIRST_RESERVED instead of DOMID_MASK + 1 (Jan Beulich)
- use const (Jan Beulich)
- move call of domain_reset_states() into evtchn_bind_virq() (Jan Beulich)
- dynamically allocate dom_state_changed bitmap (Jan Beulich)
V3:
- use xvzalloc_array() (Jan Beulich)
- don't rename existing label (Jan Beulich)
V4:
- add __read_mostly (Jan Beulich)
- use __set_bit() (Jan Beulich)
- fix error handling in evtchn_bind_virq() (Jan Beulich)
V5:
- domain_init_states() may be called only if evtchn_bind_virq() has been
  called validly (Jan Beulich)
V6:
- guard dom_state_changed bitmap with d->event_lock (Jan Beulich)
V7:
- still use __set_bit() at one place (Jan Beulich)
- use rw_is_write_locked_by_me() (Jan Beulich)
---
 xen/common/domain.c        | 51 ++++++++++++++++++++++++++++++++++++++
 xen/common/event_channel.c | 31 +++++++++++++++++++++++
 xen/include/xen/event.h    |  4 +++
 xen/include/xen/sched.h    |  3 +++
 4 files changed, 89 insertions(+)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 0c4cc77111..1c1d6da885 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -35,6 +35,7 @@
 #include <xen/irq.h>
 #include <xen/argo.h>
 #include <xen/llc-coloring.h>
+#include <xen/xvmalloc.h>
 #include <asm/p2m.h>
 #include <asm/processor.h>
 #include <public/sched.h>
@@ -139,6 +140,51 @@ bool __read_mostly vmtrace_available;
 
 bool __read_mostly vpmu_is_available;
 
+static unsigned long *__read_mostly dom_state_changed;
+
+int domain_init_states(void)
+{
+    const struct domain *d;
+
+    ASSERT(!dom_state_changed);
+    ASSERT(rw_is_write_locked_by_me(&current->domain->event_lock));
+
+    dom_state_changed = xvzalloc_array(unsigned long,
+                                       BITS_TO_LONGS(DOMID_FIRST_RESERVED));
+    if ( !dom_state_changed )
+        return -ENOMEM;
+
+    rcu_read_lock(&domlist_read_lock);
+
+    for_each_domain ( d )
+        __set_bit(d->domain_id, dom_state_changed);
+
+    rcu_read_unlock(&domlist_read_lock);
+
+    return 0;
+}
+
+void domain_deinit_states(const struct domain *d)
+{
+    ASSERT(rw_is_write_locked_by_me(&d->event_lock));
+
+    XVFREE(dom_state_changed);
+}
+
+static void domain_changed_state(const struct domain *d)
+{
+    struct domain *hdl;
+
+    hdl = lock_dom_exc_handler();
+    if ( unlikely(!hdl) )
+        return;
+
+    if ( dom_state_changed )
+        set_bit(d->domain_id, dom_state_changed);
+
+    unlock_dom_exc_handler(hdl);
+}
+
 static void __domain_finalise_shutdown(struct domain *d)
 {
     struct vcpu *v;
@@ -153,6 +199,7 @@ static void __domain_finalise_shutdown(struct domain *d)
             return;
 
     d->is_shut_down = 1;
+    domain_changed_state(d);
     if ( (d->shutdown_code == SHUTDOWN_suspend) && d->suspend_evtchn )
         evtchn_send(d, d->suspend_evtchn);
     else
@@ -840,6 +887,7 @@ struct domain *domain_create(domid_t domid,
      */
     domlist_insert(d);
 
+    domain_changed_state(d);
     memcpy(d->handle, config->handle, sizeof(d->handle));
 
     return d;
@@ -1105,6 +1153,7 @@ int domain_kill(struct domain *d)
         /* Mem event cleanup has to go here because the rings 
          * have to be put before we call put_domain. */
         vm_event_cleanup(d);
+        domain_changed_state(d);
         put_domain(d);
         send_global_virq(VIRQ_DOM_EXC);
         /* fallthrough */
@@ -1294,6 +1343,8 @@ static void cf_check complete_domain_destroy(struct rcu_head *head)
 
     xfree(d->vcpu);
 
+    domain_changed_state(d);
+
     _domain_destroy(d);
 
     send_global_virq(VIRQ_DOM_EXC);
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 4dba59efa2..4ee6b6b4ce 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -509,10 +509,18 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port)
         goto out;
     }
 
+    if ( virq == VIRQ_DOM_EXC )
+    {
+        rc = domain_init_states();
+        if ( rc )
+            goto out;
+    }
+
     port = rc = evtchn_get_port(d, port);
     if ( rc < 0 )
     {
         gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc);
+        domain_deinit_states(d);
         goto out;
     }
 
@@ -745,6 +753,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest)
         struct vcpu *v;
         unsigned long flags;
 
+        if ( chn1->u.virq == VIRQ_DOM_EXC )
+            domain_deinit_states(d1);
+
         v = d1->vcpu[virq_is_global(chn1->u.virq) ? 0 : chn1->notify_vcpu_id];
 
         write_lock_irqsave(&v->virq_lock, flags);
@@ -1075,6 +1086,26 @@ static void clear_global_virq_handlers(struct domain *d)
     }
 }
 
+struct domain *lock_dom_exc_handler(void)
+{
+    struct domain *d;
+
+    d = get_global_virq_handler(VIRQ_DOM_EXC);
+    if ( unlikely(!get_domain(d)) )
+        return NULL;
+
+    read_lock(&d->event_lock);
+
+    return d;
+}
+
+void unlock_dom_exc_handler(struct domain *d)
+{
+    read_unlock(&d->event_lock);
+
+    put_domain(d);
+}
+
 int evtchn_status(evtchn_status_t *status)
 {
     struct domain   *d;
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 48b79f3d62..5c0ba90c9f 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -100,6 +100,10 @@ bool evtchn_virq_enabled(const struct vcpu *v, unsigned int virq);
 /* Notify remote end of a Xen-attached event channel.*/
 void notify_via_xen_event_channel(struct domain *ld, int lport);
 
+/* Lock/unlock of VIRQ_DOM_EXC associated data (read_lock(d->event_lock)). */
+struct domain *lock_dom_exc_handler(void);
+void unlock_dom_exc_handler(struct domain *d);
+
 /*
  * Internal event channel object storage.
  *
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 037c83fda2..9d9b89ec27 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -805,6 +805,9 @@ void domain_resume(struct domain *d);
 
 int domain_soft_reset(struct domain *d, bool resuming);
 
+int domain_init_states(void);
+void domain_deinit_states(const struct domain *d);
+
 int vcpu_start_shutdown_deferral(struct vcpu *v);
 void vcpu_end_shutdown_deferral(struct vcpu *v);
 
-- 
2.43.0

[PATCH v8 4/9] xen: add new domctl get_changed_domain

[Juergen Gross]

Add a new domctl sub-function to get data of a domain having changed
state (this is needed by Xenstore).

The returned state just contains the domid, the domain unique id,
and some flags (existing, shutdown, dying).

In order to enable Xenstore stubdom being built for multiple Xen
versions, make this domctl stable.  For stable domctls the
interface_version is always 0.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
V1:
- use a domctl subop for the new interface (Jan Beulich)
V2:
- fix XSM hooks (Daniel P. Smith)
- remove versioning of stable sub-ops (Jan Beulich)
- use domctl.domain for retuning domid of a changed domain (Jan Beulich)
- simplify locking in get_domain_state() (Jan Beulich)
- undo stray change in event_channel.c (Jan Beulich)
V3:
- have disjunct states "dying" and "dead" (Jan Beulich)
- check padding fields to be 0 (Jan Beulich)
- drop memset() (Jan Beulich)
V4:
- add locking in get_domain_state() (Jan Beulich)
- only allow querying domain having changed state by domain receiving
  VIRQ_DOM_EXC events (Jan Beulich)
V5:
- use memset() (Jan Beulich)
V7:
- modify test for domain handling VIRQ_DOM_EXC, allowing to drop
  domain_handles_global_virq() (Jan Beulich)
---
 tools/flask/policy/modules/dom0.te     |  1 +
 tools/flask/policy/modules/xen.if      |  5 +-
 tools/flask/policy/modules/xenstore.te |  1 +
 xen/common/domain.c                    | 74 ++++++++++++++++++++++++++
 xen/common/domctl.c                    | 18 ++++++-
 xen/include/public/domctl.h            | 26 +++++++++
 xen/include/xen/sched.h                |  2 +
 xen/include/xsm/dummy.h                |  8 +++
 xen/include/xsm/xsm.h                  |  6 +++
 xen/xsm/dummy.c                        |  1 +
 xen/xsm/flask/hooks.c                  |  7 +++
 xen/xsm/flask/policy/access_vectors    |  2 +
 12 files changed, 148 insertions(+), 3 deletions(-)

diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
index f148bfbf27..ccadbd6469 100644
--- a/tools/flask/policy/modules/dom0.te
+++ b/tools/flask/policy/modules/dom0.te
@@ -41,6 +41,7 @@ allow dom0_t dom0_t:domain {
 allow dom0_t dom0_t:domain2 {
 	set_cpu_policy gettsc settsc setscheduler set_vnumainfo
 	get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy dt_overlay
+	get_domain_state
 };
 allow dom0_t dom0_t:resource { add remove };
 
diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if
index f7cf7c43c8..cff51febbf 100644
--- a/tools/flask/policy/modules/xen.if
+++ b/tools/flask/policy/modules/xen.if
@@ -54,7 +54,8 @@ define(`create_domain_common', `
 	allow $1 $2:domain2 { set_cpu_policy settsc setscheduler setclaim
 			set_vnumainfo get_vnumainfo cacheflush
 			psr_cmt_op psr_alloc soft_reset
-			resource_map get_cpu_policy vuart_op set_llc_colors };
+			resource_map get_cpu_policy vuart_op set_llc_colors
+			get_domain_state };
 	allow $1 $2:security check_context;
 	allow $1 $2:shadow enable;
 	allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage mmuext_op updatemp };
@@ -94,7 +95,7 @@ define(`manage_domain', `
 			getaddrsize pause unpause trigger shutdown destroy
 			setaffinity setdomainmaxmem getscheduler resume
 			setpodtarget getpodtarget getpagingmempool setpagingmempool };
-    allow $1 $2:domain2 { set_vnumainfo dt_overlay };
+    allow $1 $2:domain2 { set_vnumainfo dt_overlay get_domain_state };
 ')
 
 # migrate_domain_out(priv, target)
diff --git a/tools/flask/policy/modules/xenstore.te b/tools/flask/policy/modules/xenstore.te
index 519566ab81..49de53ebe2 100644
--- a/tools/flask/policy/modules/xenstore.te
+++ b/tools/flask/policy/modules/xenstore.te
@@ -13,6 +13,7 @@ allow dom0_t xenstore_t:domain set_virq_handler;
 allow xenstore_t xen_t:xen writeconsole;
 # Xenstore queries domaininfo on all domains
 allow xenstore_t domain_type:domain getdomaininfo;
+allow xenstore_t domain_type:domain2 get_domain_state;
 
 # As a shortcut, the following 3 rules are used instead of adding a domain_comms
 # rule between xenstore_t and every domain type that talks to xenstore
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 1c1d6da885..b887c60ecc 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -185,6 +185,80 @@ static void domain_changed_state(const struct domain *d)
     unlock_dom_exc_handler(hdl);
 }
 
+static void set_domain_state_info(struct xen_domctl_get_domain_state *info,
+                                  const struct domain *d)
+{
+    info->state = XEN_DOMCTL_GETDOMSTATE_STATE_EXIST;
+    if ( d->is_shut_down )
+        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN;
+    if ( d->is_dying == DOMDYING_dying )
+        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DYING;
+    if ( d->is_dying == DOMDYING_dead )
+        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DEAD;
+    info->unique_id = d->unique_id;
+}
+
+int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d,
+                     domid_t *domid)
+{
+    unsigned int dom;
+    int rc = -ENOENT;
+    struct domain *hdl;
+
+    if ( info->pad0 || info->pad1 )
+        return -EINVAL;
+
+    if ( d )
+    {
+        set_domain_state_info(info, d);
+
+        return 0;
+    }
+
+    hdl = lock_dom_exc_handler();
+
+    /*
+     * Only domain registered for VIRQ_DOM_EXC event is allowed to query
+     * domains having changed state.
+     */
+    if ( current->domain != hdl )
+    {
+        rc = -EACCES;
+        goto out;
+    }
+
+    while ( dom_state_changed )
+    {
+        dom = find_first_bit(dom_state_changed, DOMID_MASK + 1);
+        if ( dom >= DOMID_FIRST_RESERVED )
+            break;
+        if ( test_and_clear_bit(dom, dom_state_changed) )
+        {
+            *domid = dom;
+
+            d = rcu_lock_domain_by_id(dom);
+
+            if ( d )
+            {
+                set_domain_state_info(info, d);
+
+                rcu_unlock_domain(d);
+            }
+            else
+                memset(info, 0, sizeof(*info));
+
+            rc = 0;
+
+            break;
+        }
+    }
+
+ out:
+    unlock_dom_exc_handler(hdl);
+
+    return rc;
+}
+
 static void __domain_finalise_shutdown(struct domain *d)
 {
     struct vcpu *v;
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 05abb581a0..b897ca8723 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -279,6 +279,11 @@ static struct vnuma_info *vnuma_init(const struct xen_domctl_vnuma *uinfo,
     return ERR_PTR(ret);
 }
 
+static bool is_stable_domctl(uint32_t cmd)
+{
+    return cmd == XEN_DOMCTL_get_domain_state;
+}
+
 long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 {
     long ret = 0;
@@ -289,7 +294,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
     if ( copy_from_guest(op, u_domctl, 1) )
         return -EFAULT;
 
-    if ( op->interface_version != XEN_DOMCTL_INTERFACE_VERSION )
+    if ( op->interface_version !=
+         (is_stable_domctl(op->cmd) ? 0 : XEN_DOMCTL_INTERFACE_VERSION) )
         return -EACCES;
 
     switch ( op->cmd )
@@ -310,6 +316,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         fallthrough;
     case XEN_DOMCTL_test_assign_device:
     case XEN_DOMCTL_vm_event_op:
+    case XEN_DOMCTL_get_domain_state:
         if ( op->domain == DOMID_INVALID )
         {
             d = NULL;
@@ -876,6 +883,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             ret = -EOPNOTSUPP;
         break;
 
+    case XEN_DOMCTL_get_domain_state:
+        ret = xsm_get_domain_state(XSM_XS_PRIV, d);
+        if ( ret )
+            break;
+
+        copyback = 1;
+        ret = get_domain_state(&op->u.get_domain_state, d, &op->domain);
+        break;
+
     default:
         ret = arch_do_domctl(op, d, u_domctl);
         break;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index e2d392d1e5..5b2063eed9 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -28,6 +28,7 @@
  * Pure additions (e.g. new sub-commands) or compatible interface changes
  * (e.g. adding semantics to 0-checked input fields or data to zeroed output
  * fields) don't require a change of the version.
+ * Stable ops are NOT covered by XEN_DOMCTL_INTERFACE_VERSION!
  *
  * Last version bump: Xen 4.19
  */
@@ -1243,7 +1244,30 @@ struct xen_domctl_set_llc_colors {
     XEN_GUEST_HANDLE_64(uint32) llc_colors;
 };
 
+/*
+ * XEN_DOMCTL_get_domain_state (stable interface)
+ *
+ * Get state information of a domain.
+ *
+ * In case domain is DOMID_INVALID, return information about a domain having
+ * changed state and reset the state change indicator for that domain. This
+ * function is usable only by a domain having registered the VIRQ_DOM_EXC
+ * event (normally Xenstore).
+ * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
+ */
+struct xen_domctl_get_domain_state {
+    uint16_t state;
+#define XEN_DOMCTL_GETDOMSTATE_STATE_EXIST     0x0001  /* Domain is existing. */
+#define XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN  0x0002  /* Shutdown finished. */
+#define XEN_DOMCTL_GETDOMSTATE_STATE_DYING     0x0004  /* Domain dying. */
+#define XEN_DOMCTL_GETDOMSTATE_STATE_DEAD      0x0008  /* Domain dead. */
+    uint16_t pad0;           /* Must be 0 on input, returned as 0. */
+    uint32_t pad1;           /* Must be 0 on input, returned as 0. */
+    uint64_t unique_id;      /* Unique domain identifier. */
+};
+
 struct xen_domctl {
+/* Stable domctl ops: interface_version is required to be 0.  */
     uint32_t cmd;
 #define XEN_DOMCTL_createdomain                   1
 #define XEN_DOMCTL_destroydomain                  2
@@ -1333,6 +1357,7 @@ struct xen_domctl {
 #define XEN_DOMCTL_dt_overlay                    87
 #define XEN_DOMCTL_gsi_permission                88
 #define XEN_DOMCTL_set_llc_colors                89
+#define XEN_DOMCTL_get_domain_state              90 /* stable interface */
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1400,6 +1425,7 @@ struct xen_domctl {
         struct xen_domctl_dt_overlay        dt_overlay;
 #endif
         struct xen_domctl_set_llc_colors    set_llc_colors;
+        struct xen_domctl_get_domain_state  get_domain_state;
         uint8_t                             pad[128];
     } u;
 };
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 9d9b89ec27..ea63ca1c79 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -807,6 +807,8 @@ int domain_soft_reset(struct domain *d, bool resuming);
 
 int domain_init_states(void);
 void domain_deinit_states(const struct domain *d);
+int get_domain_state(struct xen_domctl_get_domain_state *info,
+                     struct domain *d, domid_t *domid);
 
 int vcpu_start_shutdown_deferral(struct vcpu *v);
 void vcpu_end_shutdown_deferral(struct vcpu *v);
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 6a2fc33c3b..a8d06de6b0 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -173,6 +173,7 @@ static XSM_INLINE int cf_check xsm_domctl(
     case XEN_DOMCTL_unbind_pt_irq:
         return xsm_default_action(XSM_DM_PRIV, current->domain, d);
     case XEN_DOMCTL_getdomaininfo:
+    case XEN_DOMCTL_get_domain_state:
         return xsm_default_action(XSM_XS_PRIV, current->domain, d);
     default:
         return xsm_default_action(XSM_PRIV, current->domain, d);
@@ -815,6 +816,13 @@ static XSM_INLINE int cf_check xsm_argo_send(
 
 #endif /* CONFIG_ARGO */
 
+static XSM_INLINE int cf_check xsm_get_domain_state(
+    XSM_DEFAULT_ARG struct domain *d)
+{
+    XSM_ASSERT_ACTION(XSM_XS_PRIV);
+    return xsm_default_action(action, current->domain, d);
+}
+
 #include <public/version.h>
 static XSM_INLINE int cf_check xsm_xen_version(XSM_DEFAULT_ARG uint32_t op)
 {
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 4dbff9d866..0689bf5c9f 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -200,6 +200,7 @@ struct xsm_ops {
     int (*argo_register_any_source)(const struct domain *d);
     int (*argo_send)(const struct domain *d, const struct domain *t);
 #endif
+    int (*get_domain_state)(struct domain *d);
 };
 
 #ifdef CONFIG_XSM
@@ -774,6 +775,11 @@ static inline int xsm_argo_send(const struct domain *d, const struct domain *t)
 
 #endif /* CONFIG_ARGO */
 
+static inline int xsm_get_domain_state(struct domain *d)
+{
+    return alternative_call(xsm_ops.get_domain_state, d);
+}
+
 #endif /* XSM_NO_WRAPPERS */
 
 #ifdef CONFIG_MULTIBOOT
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index e6ffa948f7..ce6fbdc6c5 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -148,6 +148,7 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = {
     .argo_register_any_source      = xsm_argo_register_any_source,
     .argo_send                     = xsm_argo_send,
 #endif
+    .get_domain_state              = xsm_get_domain_state,
 };
 
 void __init xsm_fixup_ops(struct xsm_ops *ops)
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 14d84df9ca..389707a164 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -688,6 +688,7 @@ static int cf_check flask_domctl(struct domain *d, unsigned int cmd,
     case XEN_DOMCTL_memory_mapping:
     case XEN_DOMCTL_set_target:
     case XEN_DOMCTL_vm_event_op:
+    case XEN_DOMCTL_get_domain_state:
 
     /* These have individual XSM hooks (arch/../domctl.c) */
     case XEN_DOMCTL_bind_pt_irq:
@@ -1869,6 +1870,11 @@ static int cf_check flask_argo_send(
 
 #endif
 
+static int cf_check flask_get_domain_state(struct domain *d)
+{
+    return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__GET_DOMAIN_STATE);
+}
+
 static const struct xsm_ops __initconst_cf_clobber flask_ops = {
     .set_system_active = flask_set_system_active,
     .security_domaininfo = flask_security_domaininfo,
@@ -2005,6 +2011,7 @@ static const struct xsm_ops __initconst_cf_clobber flask_ops = {
     .argo_register_any_source = flask_argo_register_any_source,
     .argo_send = flask_argo_send,
 #endif
+    .get_domain_state = flask_get_domain_state,
 };
 
 const struct xsm_ops *__init flask_init(
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 320d77706d..51a1577a66 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -257,6 +257,8 @@ class domain2
     dt_overlay
 # XEN_DOMCTL_set_llc_colors
     set_llc_colors
+# XEN_DOMCTL_get_domain_state
+    get_domain_state
 }
 
 # Similar to class domain, but primarily contains domctls related to HVM domains
-- 
2.43.0

[PATCH v8 5/9] tools/libs: add a new libxenmanage library

[Juergen Gross]

In order to have a stable interface in user land for using stable
domctl and possibly later sysctl interfaces, add a new library
libxenmanage.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
---
V1:
- new patch
V2:
- define __XEN_TOOLS__ via Makefile (Anthony PERARD)
- use SPDX in header file (Anthony PERARD)
- change function name to xenmanage_poll_changed_domain() (Anthony PERARD)
- add short library description (Anthony PERARD)
- narrow scope of xen_domctl_get_domain_state pointer (Anthony PERARD)
V4:
- use LGPL-2.1-only SPDX identifier (Anthony PERARD)
---
 tools/include/xenmanage.h          |  92 ++++++++++++++++
 tools/libs/Makefile                |   1 +
 tools/libs/manage/Makefile         |  10 ++
 tools/libs/manage/Makefile.common  |   3 +
 tools/libs/manage/core.c           | 168 +++++++++++++++++++++++++++++
 tools/libs/manage/libxenmanage.map |   8 ++
 tools/libs/uselibs.mk              |   2 +
 7 files changed, 284 insertions(+)
 create mode 100644 tools/include/xenmanage.h
 create mode 100644 tools/libs/manage/Makefile
 create mode 100644 tools/libs/manage/Makefile.common
 create mode 100644 tools/libs/manage/core.c
 create mode 100644 tools/libs/manage/libxenmanage.map

diff --git a/tools/include/xenmanage.h b/tools/include/xenmanage.h
new file mode 100644
index 0000000000..956b7a0a44
--- /dev/null
+++ b/tools/include/xenmanage.h
@@ -0,0 +1,92 @@
+/* SPDX-License-Identifier: LGPL-2.1-only */
+/*
+ * Copyright (c) 2024 SUSE Software Solutions Germany GmbH
+ *
+ * Interfaces of libxenmanage.
+ *
+ * libxenmanage provides management functions for the host using stable
+ * hypercall interfaces.
+ */
+#ifndef XENMANAGE_H
+#define XENMANAGE_H
+
+#include <stdint.h>
+
+/* Avoid the need to #include <xentoollog.h> */
+struct xentoollog_logger;
+
+typedef struct xenmanage_handle xenmanage_handle;
+
+/*
+ * Open libxenmanage.
+ *
+ * Get a handle of the xenmanage library. The handle is required for all
+ * further operations of the library.
+ * Parameters:
+ *   logger:     Logging function to use. If NULL logging is done to stderr.
+ *   open_flags: Only 0 supported.
+ * Return value: Handle or NULL if error.
+ */
+xenmanage_handle *xenmanage_open(struct xentoollog_logger *logger,
+                                 unsigned int open_flags);
+
+/*
+ * Close libxenmanage.
+ *
+ * Return a handle of the xenmanage library.
+ * Parameters:
+ *    hdl: Handle obtained by xenmanage_open().
+ * Return value: always 0.
+ */
+int xenmanage_close(xenmanage_handle *hdl);
+
+#define XENMANAGE_GETDOMSTATE_STATE_EXIST     0x0001  /* Domain is existing. */
+#define XENMANAGE_GETDOMSTATE_STATE_SHUTDOWN  0x0002  /* Shutdown finished. */
+#define XENMANAGE_GETDOMSTATE_STATE_DYING     0x0004  /* Domain dying. */
+#define XENMANAGE_GETDOMSTATE_STATE_DEAD      0x0008  /* Domain dead. */
+
+/*
+ * Return state information of an existing domain.
+ *
+ * Returns the domain state and unique id of the given domain.
+ * Parameters:
+ *   hdl:       handle returned by xenmanage_open()
+ *   domid:     domain id of the domain to get the information for
+ *   state:     where to store the state (XENMANAGE_GETDOMSTATE_STATE_ flags,
+ *              nothing stored if NULL)
+ *   unique_id: where to store the unique id of the domain (nothing stored if
+ *              NULL)
+ * Return value: 0 if information was stored, -1 else (errno is set)
+ */
+int xenmanage_get_domain_info(xenmanage_handle *hdl, unsigned int domid,
+                              unsigned int *state, uint64_t *unique_id);
+
+/*
+ * Return information of a domain having changed state recently.
+ *
+ * Returns the domain id, state and unique id of a domain having changed
+ * state (any of the state bits was modified) since the last time information
+ * for that domain was returned by this function. Only usable by callers who
+ * have registered the VIRQ_DOM_EXC event (normally Xenstore).
+ * Parameters:
+ *   hdl:       handle returned by xenmanage_open()
+ *   domid:     where to store the domid of the domain (not NULL)
+ *   state:     where to store the state (XENMANAGE_GETDOMSTATE_STATE_ flags,
+ *              nothing stored if NULL)
+ *   unique_id: where to store the unique id of the domain (nothing stored if
+ *              NULL)
+ * Return value: 0 if information was stored, -1 else (errno is set)
+ */
+int xenmanage_poll_changed_domain(xenmanage_handle *hdl, unsigned int *domid,
+                                  unsigned int *state, uint64_t *unique_id);
+#endif /* XENMANAGE_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libs/Makefile b/tools/libs/Makefile
index 1afcd12e2b..d39516c1b3 100644
--- a/tools/libs/Makefile
+++ b/tools/libs/Makefile
@@ -12,6 +12,7 @@ SUBDIRS-y += devicemodel
 SUBDIRS-y += ctrl
 SUBDIRS-y += guest
 SUBDIRS-y += hypfs
+SUBDIRS-y += manage
 SUBDIRS-y += store
 SUBDIRS-y += stat
 SUBDIRS-$(CONFIG_Linux) += vchan
diff --git a/tools/libs/manage/Makefile b/tools/libs/manage/Makefile
new file mode 100644
index 0000000000..dbfe70d259
--- /dev/null
+++ b/tools/libs/manage/Makefile
@@ -0,0 +1,10 @@
+XEN_ROOT = $(CURDIR)/../../..
+include $(XEN_ROOT)/tools/Rules.mk
+
+MAJOR    = 1
+MINOR    = 0
+version-script := libxenmanage.map
+
+include Makefile.common
+
+include $(XEN_ROOT)/tools/libs/libs.mk
diff --git a/tools/libs/manage/Makefile.common b/tools/libs/manage/Makefile.common
new file mode 100644
index 0000000000..533ba30fba
--- /dev/null
+++ b/tools/libs/manage/Makefile.common
@@ -0,0 +1,3 @@
+CFLAGS += -D__XEN_TOOLS__
+
+OBJS-y  += core.o
diff --git a/tools/libs/manage/core.c b/tools/libs/manage/core.c
new file mode 100644
index 0000000000..8fb421df41
--- /dev/null
+++ b/tools/libs/manage/core.c
@@ -0,0 +1,168 @@
+/*
+ * Copyright (c) 2024 SUSE Software Solutions Germany GmbH
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#define _GNU_SOURCE
+
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <xentoollog.h>
+#include <xenmanage.h>
+#include <xencall.h>
+#include <xentoolcore_internal.h>
+
+#include <xen/xen.h>
+#include <xen/domctl.h>
+
+struct xenmanage_handle {
+    xentoollog_logger *logger, *logger_tofree;
+    unsigned int flags;
+    xencall_handle *xcall;
+};
+
+xenmanage_handle *xenmanage_open(xentoollog_logger *logger,
+                                 unsigned int open_flags)
+{
+    xenmanage_handle *hdl = calloc(1, sizeof(*hdl));
+    int saved_errno;
+
+    if ( !hdl )
+        return NULL;
+
+    if ( open_flags )
+    {
+        errno = EINVAL;
+        goto err;
+    }
+
+    hdl->flags = open_flags;
+    hdl->logger = logger;
+    hdl->logger_tofree = NULL;
+
+    if ( !hdl->logger )
+    {
+        hdl->logger = hdl->logger_tofree =
+            (xentoollog_logger *)
+            xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0);
+        if ( !hdl->logger )
+            goto err;
+    }
+
+    hdl->xcall = xencall_open(hdl->logger, 0);
+    if ( !hdl->xcall )
+        goto err;
+
+    return hdl;
+
+err:
+    saved_errno = errno;
+    xenmanage_close(hdl);
+    errno = saved_errno;
+
+    return NULL;
+}
+
+int xenmanage_close(xenmanage_handle *hdl)
+{
+    if ( !hdl )
+        return 0;
+
+    xencall_close(hdl->xcall);
+    xtl_logger_destroy(hdl->logger_tofree);
+    free(hdl);
+    return 0;
+}
+
+static int xenmanage_do_domctl_get_domain_state(xenmanage_handle *hdl,
+                                                unsigned int domid_in,
+                                                unsigned int *domid_out,
+                                                unsigned int *state,
+                                                uint64_t *unique_id)
+{
+    struct xen_domctl *buf;
+    int saved_errno;
+    int ret;
+
+    buf = xencall_alloc_buffer(hdl->xcall, sizeof(*buf));
+    if ( !buf )
+    {
+        errno = ENOMEM;
+        return -1;
+    }
+
+    memset(buf, 0, sizeof(*buf));
+
+    buf->cmd = XEN_DOMCTL_get_domain_state;
+    buf->domain = domid_in;
+
+    ret = xencall1(hdl->xcall, __HYPERVISOR_domctl, (unsigned long)buf);
+    saved_errno = errno;
+    if ( !ret )
+    {
+        struct xen_domctl_get_domain_state *st = &buf->u.get_domain_state;
+
+        if ( domid_out )
+            *domid_out = buf->domain;
+        if ( state )
+        {
+            *state = 0;
+            if ( st->state & XEN_DOMCTL_GETDOMSTATE_STATE_EXIST )
+                *state |= XENMANAGE_GETDOMSTATE_STATE_EXIST;
+            if ( st->state & XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN )
+                *state |= XENMANAGE_GETDOMSTATE_STATE_SHUTDOWN;
+            if ( st->state & XEN_DOMCTL_GETDOMSTATE_STATE_DYING )
+                *state |= XENMANAGE_GETDOMSTATE_STATE_DYING;
+            if ( st->state & XEN_DOMCTL_GETDOMSTATE_STATE_DEAD )
+                *state |= XENMANAGE_GETDOMSTATE_STATE_DEAD;
+        }
+        if ( unique_id )
+            *unique_id = st->unique_id;
+    }
+
+    xencall_free_buffer(hdl->xcall, buf);
+
+    errno = saved_errno;
+
+    return ret;
+}
+
+int xenmanage_get_domain_info(xenmanage_handle *hdl, unsigned int domid,
+                              unsigned int *state, uint64_t *unique_id)
+{
+    if ( !hdl || domid >= DOMID_FIRST_RESERVED )
+    {
+        errno = EINVAL;
+        return -1;
+    }
+
+    return xenmanage_do_domctl_get_domain_state(hdl, domid, NULL, state,
+                                                unique_id);
+}
+
+int xenmanage_poll_changed_domain(xenmanage_handle *hdl, unsigned int *domid,
+                                  unsigned int *state, uint64_t *unique_id)
+{
+    if ( !hdl || !domid )
+    {
+        errno = EINVAL;
+        return -1;
+    }
+
+    return xenmanage_do_domctl_get_domain_state(hdl, DOMID_INVALID, domid,
+                                                state, unique_id);
+}
diff --git a/tools/libs/manage/libxenmanage.map b/tools/libs/manage/libxenmanage.map
new file mode 100644
index 0000000000..64c793e603
--- /dev/null
+++ b/tools/libs/manage/libxenmanage.map
@@ -0,0 +1,8 @@
+VERS_1.0 {
+	global:
+		xenmanage_open;
+		xenmanage_close;
+		xenmanage_get_domain_info;
+		xenmanage_poll_changed_domain;
+	local: *; /* Do not expose anything by default */
+};
diff --git a/tools/libs/uselibs.mk b/tools/libs/uselibs.mk
index 7aa8d83e06..c0a234cfec 100644
--- a/tools/libs/uselibs.mk
+++ b/tools/libs/uselibs.mk
@@ -16,6 +16,8 @@ LIBS_LIBS += devicemodel
 USELIBS_devicemodel := toollog toolcore call
 LIBS_LIBS += hypfs
 USELIBS_hypfs := toollog toolcore call
+LIBS_LIBS += manage
+USELIBS_manage := toollog toolcore call
 LIBS_LIBS += ctrl
 USELIBS_ctrl := toollog call evtchn gnttab foreignmemory devicemodel
 LIBS_LIBS += guest
-- 
2.43.0

[PATCH v8 6/9] tools/xenstored: use new stable interface instead of libxenctrl

[Juergen Gross]

Replace the current use of the unstable xc_domain_getinfo_single()
interface with the stable domctl XEN_DOMCTL_get_domain_state call
via the new libxenmanage library.

This will remove the last usage of libxenctrl by Xenstore, so update
the library dependencies accordingly.

For now only do a direct replacement without using the functionality
of obtaining information about domains having changed the state.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
---
V1:
- use library instead of direct hypercall, only replace current
  libxenctrl use case

Please note that this patch can be committed only after the related
Mini-OS patch "config: add support for libxenmanage" has gone in AND
the Mini-OS commit-id has been updated in Config.mk accordingly!

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 stubdom/Makefile                |  8 ++---
 stubdom/mini-os.mk              |  1 +
 tools/xenstored/Makefile        |  2 +-
 tools/xenstored/Makefile.common |  2 +-
 tools/xenstored/core.h          |  1 -
 tools/xenstored/domain.c        | 52 ++++++++++++---------------------
 tools/xenstored/lu.c            |  1 +
 tools/xenstored/lu_daemon.c     |  1 +
 8 files changed, 28 insertions(+), 40 deletions(-)

diff --git a/stubdom/Makefile b/stubdom/Makefile
index 2a81af28a1..ca800b243c 100644
--- a/stubdom/Makefile
+++ b/stubdom/Makefile
@@ -307,7 +307,7 @@ endif
 # libraries under tools/libs
 #######
 
-STUB_LIBS := toolcore toollog evtchn gnttab call foreignmemory devicemodel ctrl guest
+STUB_LIBS := toolcore toollog evtchn gnttab call foreignmemory devicemodel ctrl guest manage
 
 LIBDEP_guest := cross-zlib
 
@@ -465,7 +465,7 @@ grub: cross-polarssl grub-upstream $(CROSS_ROOT) grub-$(XEN_TARGET_ARCH)-minios-
 # xenstore
 ##########
 
-xenstore-minios.gen.cfg: APP_LIBS = gnttab evtchn toollog ctrl
+xenstore-minios.gen.cfg: APP_LIBS = gnttab evtchn toollog manage
 xenstore-minios.gen.cfg: xenstore-minios.cfg Makefile
 	$(GEN_config) >$@
 
@@ -480,7 +480,7 @@ xenstore: $(CROSS_ROOT) xenstore-minios-config.mk
 # xenstorepvh
 #############
 
-xenstorepvh-minios.gen.cfg: APP_LIBS = gnttab evtchn toollog ctrl
+xenstorepvh-minios.gen.cfg: APP_LIBS = gnttab evtchn toollog manage
 xenstorepvh-minios.gen.cfg: xenstorepvh-minios.cfg Makefile
 	$(GEN_config) >$@
 
@@ -523,7 +523,7 @@ else
 pv-grub-if-enabled:
 endif
 
-XENSTORE_DEPS := libxenevtchn libxengnttab libxenctrl
+XENSTORE_DEPS := libxenevtchn libxengnttab libxenmanage
 
 .PHONY: xenstore-stubdom
 xenstore-stubdom: mini-os-$(XEN_TARGET_ARCH)-xenstore $(XENSTORE_DEPS) xenstore
diff --git a/stubdom/mini-os.mk b/stubdom/mini-os.mk
index 7e4968e026..be32302f9e 100644
--- a/stubdom/mini-os.mk
+++ b/stubdom/mini-os.mk
@@ -13,5 +13,6 @@ GNTTAB_PATH = $(XEN_ROOT)/stubdom/libs-$(MINIOS_TARGET_ARCH)/gnttab
 CALL_PATH = $(XEN_ROOT)/stubdom/libs-$(MINIOS_TARGET_ARCH)/call
 FOREIGNMEMORY_PATH = $(XEN_ROOT)/stubdom/libs-$(MINIOS_TARGET_ARCH)/foreignmemory
 DEVICEMODEL_PATH = $(XEN_ROOT)/stubdom/libs-$(MINIOS_TARGET_ARCH)/devicemodel
+MANAGE_PATH = $(XEN_ROOT)/stubdom/libs-$(MINIOS_TARGET_ARCH)/manage
 CTRL_PATH = $(XEN_ROOT)/stubdom/libs-$(MINIOS_TARGET_ARCH)/ctrl
 GUEST_PATH = $(XEN_ROOT)/stubdom/libs-$(MINIOS_TARGET_ARCH)/guest
diff --git a/tools/xenstored/Makefile b/tools/xenstored/Makefile
index 09adfe1d50..81c42838e0 100644
--- a/tools/xenstored/Makefile
+++ b/tools/xenstored/Makefile
@@ -5,7 +5,7 @@ include Makefile.common
 
 xenstored: LDLIBS += $(LDLIBS_libxenevtchn)
 xenstored: LDLIBS += $(LDLIBS_libxengnttab)
-xenstored: LDLIBS += $(LDLIBS_libxenctrl)
+xenstored: LDLIBS += $(LDLIBS_libxenmanage)
 xenstored: LDLIBS += -lrt
 xenstored: LDLIBS += $(SOCKET_LIBS)
 
diff --git a/tools/xenstored/Makefile.common b/tools/xenstored/Makefile.common
index 27fdb3b49e..271134fcc1 100644
--- a/tools/xenstored/Makefile.common
+++ b/tools/xenstored/Makefile.common
@@ -12,7 +12,7 @@ XENSTORED_OBJS-$(CONFIG_MiniOS) += minios.o lu_minios.o
 # Include configure output (config.h)
 CFLAGS += -include $(XEN_ROOT)/tools/config.h
 CFLAGS += $(CFLAGS_libxenevtchn)
-CFLAGS += $(CFLAGS_libxenctrl)
+CFLAGS += $(CFLAGS_libxenmanage)
 CFLAGS += $(CFLAGS_libxentoolcore)
 
 $(XENSTORED_OBJS-y): CFLAGS += $(CFLAGS_libxengnttab)
diff --git a/tools/xenstored/core.h b/tools/xenstored/core.h
index e58779e88c..632886cecf 100644
--- a/tools/xenstored/core.h
+++ b/tools/xenstored/core.h
@@ -19,7 +19,6 @@
 #ifndef _XENSTORED_CORE_H
 #define _XENSTORED_CORE_H
 
-#include <xenctrl.h>
 #include <xengnttab.h>
 
 #include <sys/types.h>
diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
index 64c8fd0cc3..a6506a5bb2 100644
--- a/tools/xenstored/domain.c
+++ b/tools/xenstored/domain.c
@@ -34,14 +34,15 @@
 #include "control.h"
 
 #include <xenevtchn.h>
-#include <xenctrl.h>
+#include <xenmanage.h>
+#include <xen-barrier.h>
 #include <xen/grant_table.h>
 
 #ifdef __MINIOS__
 #include <mini-os/xenbus.h>
 #endif
 
-static xc_interface **xc_handle;
+static xenmanage_handle *xm_handle;
 xengnttab_handle **xgt_handle;
 static evtchn_port_t virq_port;
 
@@ -619,32 +620,28 @@ static int destroy_domain(void *_domain)
 	return 0;
 }
 
-static bool get_domain_info(unsigned int domid, xc_domaininfo_t *dominfo)
-{
-	return xc_domain_getinfo_single(*xc_handle, domid, dominfo) == 0;
-}
-
 static int check_domain(const void *k, void *v, void *arg)
 {
-	xc_domaininfo_t dominfo;
+	unsigned int state;
 	struct connection *conn;
-	bool dom_valid;
+	int dom_invalid;
 	struct domain *domain = v;
 	bool *notify = arg;
 
-	dom_valid = get_domain_info(domain->domid, &dominfo);
+	dom_invalid = xenmanage_get_domain_info(xm_handle, domain->domid,
+						&state, NULL);
 	if (!domain->introduced) {
-		if (!dom_valid)
+		if (dom_invalid)
 			talloc_free(domain);
 		return 0;
 	}
-	if (dom_valid) {
-		if ((dominfo.flags & XEN_DOMINF_shutdown)
+	if (!dom_invalid) {
+		if ((state & XENMANAGE_GETDOMSTATE_STATE_SHUTDOWN)
 		    && !domain->shutdown) {
 			domain->shutdown = true;
 			*notify = true;
 		}
-		if (!(dominfo.flags & XEN_DOMINF_dying))
+		if (!(state & XENMANAGE_GETDOMSTATE_STATE_DEAD))
 			return 0;
 	}
 	if (domain->conn) {
@@ -786,10 +783,9 @@ static struct domain *find_or_alloc_domain(const void *ctx, unsigned int domid)
 static struct domain *find_or_alloc_existing_domain(unsigned int domid)
 {
 	struct domain *domain;
-	xc_domaininfo_t dominfo;
 
 	domain = find_domain_struct(domid);
-	if (!domain && get_domain_info(domid, &dominfo))
+	if (!domain && !xenmanage_get_domain_info(xm_handle, domid, NULL, NULL))
 		domain = alloc_domain(NULL, domid);
 
 	return domain;
@@ -1187,12 +1183,6 @@ int do_reset_watches(const void *ctx, struct connection *conn,
 	return 0;
 }
 
-static int close_xc_handle(void *_handle)
-{
-	xc_interface_close(*(xc_interface**)_handle);
-	return 0;
-}
-
 static int close_xgt_handle(void *_handle)
 {
 	xengnttab_close(*(xengnttab_handle **)_handle);
@@ -1258,15 +1248,9 @@ void domain_early_init(void)
 	if (!domhash)
 		barf_perror("Failed to allocate domain hashtable");
 
-	xc_handle = talloc(talloc_autofree_context(), xc_interface*);
-	if (!xc_handle)
-		barf_perror("Failed to allocate domain handle");
-
-	*xc_handle = xc_interface_open(0,0,0);
-	if (!*xc_handle)
-		barf_perror("Failed to open connection to hypervisor");
-
-	talloc_set_destructor(xc_handle, close_xc_handle);
+	xm_handle = xenmanage_open(NULL, 0);
+	if (!xm_handle)
+		barf_perror("Failed to open connection to libxenmanage");
 
 	xgt_handle = talloc(talloc_autofree_context(), xengnttab_handle*);
 	if (!xgt_handle)
@@ -1306,6 +1290,8 @@ void domain_deinit(void)
 {
 	if (virq_port)
 		xenevtchn_unbind(xce_handle, virq_port);
+
+	xenmanage_close(xm_handle);
 }
 
 /*
@@ -1335,13 +1321,13 @@ int domain_alloc_permrefs(struct node_perms *perms)
 {
 	unsigned int i, domid;
 	struct domain *d;
-	xc_domaininfo_t dominfo;
 
 	for (i = 0; i < perms->num; i++) {
 		domid = perms->p[i].id;
 		d = find_domain_struct(domid);
 		if (!d) {
-			if (!get_domain_info(domid, &dominfo))
+			if (xenmanage_get_domain_info(xm_handle, domid,
+						      NULL, NULL))
 				perms->p[i].perms |= XS_PERM_IGNORE;
 			else if (!alloc_domain(NULL, domid))
 				return ENOMEM;
diff --git a/tools/xenstored/lu.c b/tools/xenstored/lu.c
index bec2a84e10..4fccbbc195 100644
--- a/tools/xenstored/lu.c
+++ b/tools/xenstored/lu.c
@@ -11,6 +11,7 @@
 #include <stdlib.h>
 #include <syslog.h>
 #include <time.h>
+#include <unistd.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
 
diff --git a/tools/xenstored/lu_daemon.c b/tools/xenstored/lu_daemon.c
index 6df6c80a2a..88d8d9e1b3 100644
--- a/tools/xenstored/lu_daemon.c
+++ b/tools/xenstored/lu_daemon.c
@@ -6,6 +6,7 @@
  */
 
 #include <syslog.h>
+#include <unistd.h>
 #include <sys/stat.h>
 
 #include "talloc.h"
-- 
2.43.0

[PATCH v8 7/9] docs: update xenstore migration stream definition

[Juergen Gross]

In order to close a race window for Xenstore live update when using
the new unique_id of domains, the migration stream needs to contain
this unique_id for each domain known by Xenstore.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V8:
- new patch
---
 docs/designs/xenstore-migration.md | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/docs/designs/xenstore-migration.md b/docs/designs/xenstore-migration.md
index 082314bf72..fba691ee0d 100644
--- a/docs/designs/xenstore-migration.md
+++ b/docs/designs/xenstore-migration.md
@@ -156,7 +156,7 @@ the domain being migrated.
 ```
     0       1       2       3       4       5       6       7    octet
 +-------+-------+-------+-------+-------+-------+-------+-------+
-| conn-id                       | conn-type     |               |
+| conn-id                       | conn-type     | uniq-id-off   |
 +-------------------------------+---------------+---------------+
 | conn-spec
 ...
@@ -165,6 +165,9 @@ the domain being migrated.
 +---------------+---------------+-------------------------------+
 | data
 ...
++---------------------------------------------------------------+
+| unique-id                                                     |
++---------------------------------------------------------------+
 ```
 
 
@@ -178,6 +181,12 @@ the domain being migrated.
 |                | 0x0001: socket                               |
 |                | 0x0002 - 0xFFFF: reserved for future use     |
 |                |                                              |
+| `uniq-id-off`  | The offset (in octets) of the `unique-id`    |
+|                | field from the start of the record body.     |
+|                | If 0, no `unique-id` field is present.       |
+|                | Only needed for `shared ring` connection in  |
+|                | live update streams.                         |
+|                |                                              |
 | `conn-spec`    | See below                                    |
 |                |                                              |
 | `in-data-len`  | The length (in octets) of any data read      |
@@ -193,6 +202,9 @@ the domain being migrated.
 | `data`         | Pending data: first in-data-len octets of    |
 |                | read data, then out-data-len octets of       |
 |                | written data (any of both may be empty)      |
+|                |                                              |
+| `unique-id`    | Unique identifier of a domain                |
+|                |                                              |
 
 In case of live update the connection record for the connection via which
 the live update command was issued will contain the response for the live
-- 
2.43.0

[PATCH v8 8/9] tools/xenstored: use unique_id to identify new domain with same domid

[Juergen Gross]

Use the new unique_id of a domain in order to detect that a domain
has been replaced with another one reusing the doamin-id of the old
domain.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V8:
- new patch
---
 tools/xenstored/domain.c         | 53 +++++++++++++++++++++++++++-----
 tools/xenstored/xenstore_state.h |  2 +-
 2 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
index a6506a5bb2..63df24030e 100644
--- a/tools/xenstored/domain.c
+++ b/tools/xenstored/domain.c
@@ -110,6 +110,7 @@ struct domain
 {
 	/* The id of this domain */
 	unsigned int domid;
+	uint64_t unique_id;
 
 	/* Event channel port */
 	evtchn_port_t port;
@@ -627,9 +628,17 @@ static int check_domain(const void *k, void *v, void *arg)
 	int dom_invalid;
 	struct domain *domain = v;
 	bool *notify = arg;
+	uint64_t unique_id;
 
 	dom_invalid = xenmanage_get_domain_info(xm_handle, domain->domid,
-						&state, NULL);
+						&state, &unique_id);
+	if (!dom_invalid) {
+		if (!domain->unique_id)
+			domain->unique_id = unique_id;
+		else if (domain->unique_id != unique_id)
+			dom_invalid = 1;
+	}
+
 	if (!domain->introduced) {
 		if (dom_invalid)
 			talloc_free(domain);
@@ -747,7 +756,8 @@ int domain_max_global_acc(const void *ctx, struct connection *conn)
 	return 0;
 }
 
-static struct domain *alloc_domain(const void *context, unsigned int domid)
+static struct domain *alloc_domain(const void *context, unsigned int domid,
+				   uint64_t unique_id)
 {
 	struct domain *domain;
 
@@ -758,6 +768,7 @@ static struct domain *alloc_domain(const void *context, unsigned int domid)
 	}
 
 	domain->domid = domid;
+	domain->unique_id = unique_id;
 	domain->generation = generation;
 	domain->introduced = false;
 
@@ -777,16 +788,27 @@ static struct domain *find_or_alloc_domain(const void *ctx, unsigned int domid)
 	struct domain *domain;
 
 	domain = find_domain_struct(domid);
-	return domain ? : alloc_domain(ctx, domid);
+	/* If domain not already known, use unique_id = 0 meaning "unknown". */
+	return domain ? : alloc_domain(ctx, domid, 0);
 }
 
 static struct domain *find_or_alloc_existing_domain(unsigned int domid)
 {
 	struct domain *domain;
+	uint64_t unique_id = 0;
+	int dom_invalid = 0;
 
 	domain = find_domain_struct(domid);
-	if (!domain && !xenmanage_get_domain_info(xm_handle, domid, NULL, NULL))
-		domain = alloc_domain(NULL, domid);
+	if (!domain || !domain->unique_id)
+		dom_invalid = xenmanage_get_domain_info(xm_handle, domid,
+							NULL, &unique_id);
+
+	if (!dom_invalid) {
+		if (!domain)
+			domain = alloc_domain(NULL, domid, unique_id);
+		else if (unique_id)
+			domain->unique_id = unique_id;
+	}
 
 	return domain;
 }
@@ -1321,15 +1343,16 @@ int domain_alloc_permrefs(struct node_perms *perms)
 {
 	unsigned int i, domid;
 	struct domain *d;
+	uint64_t unique_id;
 
 	for (i = 0; i < perms->num; i++) {
 		domid = perms->p[i].id;
 		d = find_domain_struct(domid);
 		if (!d) {
 			if (xenmanage_get_domain_info(xm_handle, domid,
-						      NULL, NULL))
+						      NULL, &unique_id))
 				perms->p[i].perms |= XS_PERM_IGNORE;
-			else if (!alloc_domain(NULL, domid))
+			else if (!alloc_domain(NULL, domid, unique_id))
 				return ENOMEM;
 		}
 	}
@@ -1697,12 +1720,14 @@ const char *dump_state_connections(FILE *fp)
 	struct xs_state_record_header head;
 	struct connection *c;
 
+	BUILD_BUG_ON(sizeof(c->domain->unique_id) != sizeof(uint64_t));
+
 	list_for_each_entry(c, &connections, list) {
 		head.type = XS_STATE_TYPE_CONN;
 		head.length = sizeof(sc);
 
 		sc.conn_id = conn_id++;
-		sc.pad = 0;
+		sc.uniq_id_off = 0;
 		memset(&sc.spec, 0, sizeof(sc.spec));
 		if (c->domain) {
 			sc.conn_type = XS_STATE_CONN_TYPE_RING;
@@ -1720,6 +1745,10 @@ const char *dump_state_connections(FILE *fp)
 			return ret;
 		head.length += sc.data_in_len + sc.data_out_len;
 		head.length = ROUNDUP(head.length, 3);
+		if (c->domain) {
+			sc.uniq_id_off = head.length;
+			head.length += sizeof(uint64_t);
+		}
 		if (fwrite(&head, sizeof(head), 1, fp) != 1)
 			return "Dump connection state error";
 		if (fwrite(&sc, offsetof(struct xs_state_connection, data),
@@ -1731,6 +1760,9 @@ const char *dump_state_connections(FILE *fp)
 		ret = dump_state_align(fp);
 		if (ret)
 			return ret;
+		if (c->domain &&
+		    fwrite(&c->domain->unique_id, sizeof(uint64_t), 1, fp) != 1)
+			return "Dump connection state error";
 
 		ret = dump_state_watches(fp, c, sc.conn_id);
 		if (ret)
@@ -1748,6 +1780,7 @@ void read_state_connection(const void *ctx, const void *state)
 
 	if (sc->conn_type == XS_STATE_CONN_TYPE_SOCKET) {
 		conn = add_socket_connection(sc->spec.socket_fd);
+		domain = NULL;
 	} else {
 		domain = introduce_domain(ctx, sc->spec.ring.domid,
 					  sc->spec.ring.evtchn, true);
@@ -1778,6 +1811,10 @@ void read_state_connection(const void *ctx, const void *state)
 	conn->conn_id = sc->conn_id;
 
 	read_state_buffered_data(ctx, conn, sc);
+
+	/* Validity of unique_id will be tested by check_domains() later. */
+	if (sc->uniq_id_off && domain)
+		domain->unique_id = *(uint64_t *)(state + sc->uniq_id_off);
 }
 
 struct domain_acc {
diff --git a/tools/xenstored/xenstore_state.h b/tools/xenstored/xenstore_state.h
index ae0d053c8f..4c785e3774 100644
--- a/tools/xenstored/xenstore_state.h
+++ b/tools/xenstored/xenstore_state.h
@@ -74,7 +74,7 @@ struct xs_state_connection {
     uint16_t conn_type;
 #define XS_STATE_CONN_TYPE_RING   0
 #define XS_STATE_CONN_TYPE_SOCKET 1
-    uint16_t pad;
+    uint16_t uniq_id_off;
     union {
         struct {
             uint16_t domid;  /* Domain-Id. */
-- 
2.43.0

[PATCH v8 9/9] tools/xenstored: use xenmanage_poll_changed_domain()

[Juergen Gross]

Instead of checking each known domain after having received a
VIRQ_DOM_EXC event, use the new xenmanage_poll_changed_domain()
function for directly getting the domid of a domain having changed
its state.

A test doing "xl shutdown" of 1000 guests has shown to reduce the
consumed cpu time of xenstored by 6% with this change applied.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V8:
- new patch
---
 tools/xenstored/domain.c | 64 +++++++++++++++++++++++++++++-----------
 1 file changed, 46 insertions(+), 18 deletions(-)

diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
index 63df24030e..ad16a00ce3 100644
--- a/tools/xenstored/domain.c
+++ b/tools/xenstored/domain.c
@@ -621,30 +621,24 @@ static int destroy_domain(void *_domain)
 	return 0;
 }
 
-static int check_domain(const void *k, void *v, void *arg)
+static int do_check_domain(struct domain *domain, bool *notify,
+			   unsigned int state, uint64_t unique_id)
 {
-	unsigned int state;
 	struct connection *conn;
-	int dom_invalid;
-	struct domain *domain = v;
-	bool *notify = arg;
-	uint64_t unique_id;
 
-	dom_invalid = xenmanage_get_domain_info(xm_handle, domain->domid,
-						&state, &unique_id);
-	if (!dom_invalid) {
+	if (unique_id) {
 		if (!domain->unique_id)
 			domain->unique_id = unique_id;
 		else if (domain->unique_id != unique_id)
-			dom_invalid = 1;
+			unique_id = 0;
 	}
 
 	if (!domain->introduced) {
-		if (dom_invalid)
+		if (!unique_id)
 			talloc_free(domain);
 		return 0;
 	}
-	if (!dom_invalid) {
+	if (unique_id) {
 		if ((state & XENMANAGE_GETDOMSTATE_STATE_SHUTDOWN)
 		    && !domain->shutdown) {
 			domain->shutdown = true;
@@ -667,6 +661,21 @@ static int check_domain(const void *k, void *v, void *arg)
 	return 0;
 }
 
+static int check_domain(const void *k, void *v, void *arg)
+{
+	struct domain *domain = v;
+	unsigned int state;
+	uint64_t unique_id;
+
+	if (xenmanage_get_domain_info(xm_handle, domain->domid, &state,
+				      &unique_id)) {
+		unique_id = 0;
+		state = 0;
+	}
+
+	return do_check_domain(domain, arg, state, unique_id);
+}
+
 void check_domains(void)
 {
 	bool notify = false;
@@ -678,6 +687,30 @@ void check_domains(void)
 		fire_special_watches("@releaseDomain");
 }
 
+static struct domain *find_domain_struct(unsigned int domid)
+{
+	return hashtable_search(domhash, &domid);
+}
+
+static void do_check_domains(void)
+{
+	unsigned int domid;
+	unsigned int state;
+	uint64_t unique_id;
+	struct domain *domain;
+	bool notify = false;
+
+	while (!xenmanage_poll_changed_domain(xm_handle, &domid, &state,
+					      &unique_id)) {
+		domain = find_domain_struct(domid);
+		if (domain)
+			do_check_domain(domain, &notify, state, unique_id);
+	}
+
+	if (notify)
+		fire_special_watches("@releaseDomain");
+}
+
 /* We scan all domains rather than use the information given here. */
 void handle_event(void)
 {
@@ -687,7 +720,7 @@ void handle_event(void)
 		barf_perror("Failed to read from event fd");
 
 	if (port == virq_port)
-		check_domains();
+		do_check_domains();
 
 	if (xenevtchn_unmask(xce_handle, port) == -1)
 		barf_perror("Failed to write to event fd");
@@ -698,11 +731,6 @@ static char *talloc_domain_path(const void *context, unsigned int domid)
 	return talloc_asprintf(context, "/local/domain/%u", domid);
 }
 
-static struct domain *find_domain_struct(unsigned int domid)
-{
-	return hashtable_search(domhash, &domid);
-}
-
 int domain_get_quota(const void *ctx, struct connection *conn,
 		     unsigned int domid)
 {
-- 
2.43.0

Re: [PATCH v8 0/9] remove libxenctrl usage from xenstored

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1845 bytes --]

Ping? Especially ...

On 04.02.25 12:33, Juergen Gross wrote:
> Xenstored is using libxenctrl for only one purpose: to get information
> about state of domains.
> 
> This patch series is removing that dependency by introducing a new
> stable interface which can be used by xenstored instead.
> 
> There was a RFC series sent out 3 years ago, which I have taken as a
> base and by addressing all comments from back then.
> 
> The main differences since that RFC series are:
> 
> - Instead of introducing an new main hypercall for a stable management
>    interface I have just added a new domctl sub-op, as requested in 2021.
> 
> - I have added a new library libxenmanage for easy use of the new
>    stable hypervisor interface. Main motivation for adding the library
>    was the recent attempt to decouple oxenstored from the Xen git tree.
>    By using the new library, oxenstored could benefit in the same way as
>    xenstored from the new interface: it would be possible to rely on
>    stable libraries only.
> 
> - Mini-OS has gained some more config options recently, so it was rather
>    easy to make xenstore[pvh]-stubdom independent from libxenctrl, too.
> 
> Please note that the last 4 patches can be committed only after the
> related Mini-OS patch "config: add support for libxenmanage" has gone in
> AND the Mini-OS commit-id has been updated in Config.mk accordingly! The
> Mini-OS patch has been Acked already, so it can go in as soon as patch
> 5 of this series (the one introducing libxenmanage) has been committed.
> 
> As patches 1 and 2 change current behavior, Jan didn't want to give his
> Ack (he didn't reject the patch either). So I'm asking other "Rest"
> maintainers to look at those patches specifically.

... patches 1 and 2 could need an additional opinion.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v8 0/9] remove libxenctrl usage from xenstored

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2192 bytes --]

On 25.02.25 12:10, Juergen Gross wrote:
> Ping? Especially ...
> 
> On 04.02.25 12:33, Juergen Gross wrote:
>> Xenstored is using libxenctrl for only one purpose: to get information
>> about state of domains.
>>
>> This patch series is removing that dependency by introducing a new
>> stable interface which can be used by xenstored instead.
>>
>> There was a RFC series sent out 3 years ago, which I have taken as a
>> base and by addressing all comments from back then.
>>
>> The main differences since that RFC series are:
>>
>> - Instead of introducing an new main hypercall for a stable management
>>    interface I have just added a new domctl sub-op, as requested in 2021.
>>
>> - I have added a new library libxenmanage for easy use of the new
>>    stable hypervisor interface. Main motivation for adding the library
>>    was the recent attempt to decouple oxenstored from the Xen git tree.
>>    By using the new library, oxenstored could benefit in the same way as
>>    xenstored from the new interface: it would be possible to rely on
>>    stable libraries only.
>>
>> - Mini-OS has gained some more config options recently, so it was rather
>>    easy to make xenstore[pvh]-stubdom independent from libxenctrl, too.
>>
>> Please note that the last 4 patches can be committed only after the
>> related Mini-OS patch "config: add support for libxenmanage" has gone in
>> AND the Mini-OS commit-id has been updated in Config.mk accordingly! The
>> Mini-OS patch has been Acked already, so it can go in as soon as patch
>> 5 of this series (the one introducing libxenmanage) has been committed.
>>
>> As patches 1 and 2 change current behavior, Jan didn't want to give his
>> Ack (he didn't reject the patch either). So I'm asking other "Rest"
>> maintainers to look at those patches specifically.
> 
> ... patches 1 and 2 could need an additional opinion.

And another ping.

One of Andrew, Stefano, Julien, Roger, Anthony, Mical: Please have a look.

The related discussion between Jan and me can be found here (patches 2 and 3):

https://lore.kernel.org/xen-devel/20250107101711.5980-1-jgross@suse.com/


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v8 0/9] remove libxenctrl usage from xenstored

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2543 bytes --]

On Wed, 5 Mar 2025, Juergen Gross wrote:
> On 25.02.25 12:10, Juergen Gross wrote:
> > Ping? Especially ...
> > 
> > On 04.02.25 12:33, Juergen Gross wrote:
> > > Xenstored is using libxenctrl for only one purpose: to get information
> > > about state of domains.
> > > 
> > > This patch series is removing that dependency by introducing a new
> > > stable interface which can be used by xenstored instead.
> > > 
> > > There was a RFC series sent out 3 years ago, which I have taken as a
> > > base and by addressing all comments from back then.
> > > 
> > > The main differences since that RFC series are:
> > > 
> > > - Instead of introducing an new main hypercall for a stable management
> > >    interface I have just added a new domctl sub-op, as requested in 2021.
> > > 
> > > - I have added a new library libxenmanage for easy use of the new
> > >    stable hypervisor interface. Main motivation for adding the library
> > >    was the recent attempt to decouple oxenstored from the Xen git tree.
> > >    By using the new library, oxenstored could benefit in the same way as
> > >    xenstored from the new interface: it would be possible to rely on
> > >    stable libraries only.
> > > 
> > > - Mini-OS has gained some more config options recently, so it was rather
> > >    easy to make xenstore[pvh]-stubdom independent from libxenctrl, too.
> > > 
> > > Please note that the last 4 patches can be committed only after the
> > > related Mini-OS patch "config: add support for libxenmanage" has gone in
> > > AND the Mini-OS commit-id has been updated in Config.mk accordingly! The
> > > Mini-OS patch has been Acked already, so it can go in as soon as patch
> > > 5 of this series (the one introducing libxenmanage) has been committed.
> > > 
> > > As patches 1 and 2 change current behavior, Jan didn't want to give his
> > > Ack (he didn't reject the patch either). So I'm asking other "Rest"
> > > maintainers to look at those patches specifically.
> > 
> > ... patches 1 and 2 could need an additional opinion.
> 
> And another ping.
> 
> One of Andrew, Stefano, Julien, Roger, Anthony, Mical: Please have a look.
> 
> The related discussion between Jan and me can be found here (patches 2 and 3):
> 
> https://lore.kernel.org/xen-devel/20250107101711.5980-1-jgross@suse.com/

I didn't do an in-details review but based on Jan's comments and your
replies, I think they are an improvement. If someone else wants to do a
proper review, they would be welcome.

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [PATCH v8 0/9] remove libxenctrl usage from xenstored

[Jan Beulich]

On 06.03.2025 00:32, Stefano Stabellini wrote:
> On Wed, 5 Mar 2025, Juergen Gross wrote:
>> On 25.02.25 12:10, Juergen Gross wrote:
>>> Ping? Especially ...
>>>
>>> On 04.02.25 12:33, Juergen Gross wrote:
>>>> Xenstored is using libxenctrl for only one purpose: to get information
>>>> about state of domains.
>>>>
>>>> This patch series is removing that dependency by introducing a new
>>>> stable interface which can be used by xenstored instead.
>>>>
>>>> There was a RFC series sent out 3 years ago, which I have taken as a
>>>> base and by addressing all comments from back then.
>>>>
>>>> The main differences since that RFC series are:
>>>>
>>>> - Instead of introducing an new main hypercall for a stable management
>>>>    interface I have just added a new domctl sub-op, as requested in 2021.
>>>>
>>>> - I have added a new library libxenmanage for easy use of the new
>>>>    stable hypervisor interface. Main motivation for adding the library
>>>>    was the recent attempt to decouple oxenstored from the Xen git tree.
>>>>    By using the new library, oxenstored could benefit in the same way as
>>>>    xenstored from the new interface: it would be possible to rely on
>>>>    stable libraries only.
>>>>
>>>> - Mini-OS has gained some more config options recently, so it was rather
>>>>    easy to make xenstore[pvh]-stubdom independent from libxenctrl, too.
>>>>
>>>> Please note that the last 4 patches can be committed only after the
>>>> related Mini-OS patch "config: add support for libxenmanage" has gone in
>>>> AND the Mini-OS commit-id has been updated in Config.mk accordingly! The
>>>> Mini-OS patch has been Acked already, so it can go in as soon as patch
>>>> 5 of this series (the one introducing libxenmanage) has been committed.
>>>>
>>>> As patches 1 and 2 change current behavior, Jan didn't want to give his
>>>> Ack (he didn't reject the patch either). So I'm asking other "Rest"
>>>> maintainers to look at those patches specifically.
>>>
>>> ... patches 1 and 2 could need an additional opinion.
>>
>> And another ping.
>>
>> One of Andrew, Stefano, Julien, Roger, Anthony, Mical: Please have a look.
>>
>> The related discussion between Jan and me can be found here (patches 2 and 3):
>>
>> https://lore.kernel.org/xen-devel/20250107101711.5980-1-jgross@suse.com/
> 
> I didn't do an in-details review but based on Jan's comments and your
> replies, I think they are an improvement. If someone else wants to do a
> proper review, they would be welcome.
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>

I've taken the conservative approach and interpreted this as an ack for the
two patches in question only, rather than the entire series. Please indicate
if it was meant the other way around, as then the final 3 patches could also
go in.

Jan

Re: [PATCH v8 0/9] remove libxenctrl usage from xenstored

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 3283 bytes --]

On 06.03.25 14:13, Jan Beulich wrote:
> On 06.03.2025 00:32, Stefano Stabellini wrote:
>> On Wed, 5 Mar 2025, Juergen Gross wrote:
>>> On 25.02.25 12:10, Juergen Gross wrote:
>>>> Ping? Especially ...
>>>>
>>>> On 04.02.25 12:33, Juergen Gross wrote:
>>>>> Xenstored is using libxenctrl for only one purpose: to get information
>>>>> about state of domains.
>>>>>
>>>>> This patch series is removing that dependency by introducing a new
>>>>> stable interface which can be used by xenstored instead.
>>>>>
>>>>> There was a RFC series sent out 3 years ago, which I have taken as a
>>>>> base and by addressing all comments from back then.
>>>>>
>>>>> The main differences since that RFC series are:
>>>>>
>>>>> - Instead of introducing an new main hypercall for a stable management
>>>>>     interface I have just added a new domctl sub-op, as requested in 2021.
>>>>>
>>>>> - I have added a new library libxenmanage for easy use of the new
>>>>>     stable hypervisor interface. Main motivation for adding the library
>>>>>     was the recent attempt to decouple oxenstored from the Xen git tree.
>>>>>     By using the new library, oxenstored could benefit in the same way as
>>>>>     xenstored from the new interface: it would be possible to rely on
>>>>>     stable libraries only.
>>>>>
>>>>> - Mini-OS has gained some more config options recently, so it was rather
>>>>>     easy to make xenstore[pvh]-stubdom independent from libxenctrl, too.
>>>>>
>>>>> Please note that the last 4 patches can be committed only after the
>>>>> related Mini-OS patch "config: add support for libxenmanage" has gone in
>>>>> AND the Mini-OS commit-id has been updated in Config.mk accordingly! The
>>>>> Mini-OS patch has been Acked already, so it can go in as soon as patch
>>>>> 5 of this series (the one introducing libxenmanage) has been committed.
>>>>>
>>>>> As patches 1 and 2 change current behavior, Jan didn't want to give his
>>>>> Ack (he didn't reject the patch either). So I'm asking other "Rest"
>>>>> maintainers to look at those patches specifically.
>>>>
>>>> ... patches 1 and 2 could need an additional opinion.
>>>
>>> And another ping.
>>>
>>> One of Andrew, Stefano, Julien, Roger, Anthony, Mical: Please have a look.
>>>
>>> The related discussion between Jan and me can be found here (patches 2 and 3):
>>>
>>> https://lore.kernel.org/xen-devel/20250107101711.5980-1-jgross@suse.com/
>>
>> I didn't do an in-details review but based on Jan's comments and your
>> replies, I think they are an improvement. If someone else wants to do a
>> proper review, they would be welcome.
>>
>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> I've taken the conservative approach and interpreted this as an ack for the
> two patches in question only, rather than the entire series. Please indicate
> if it was meant the other way around, as then the final 3 patches could also
> go in.

Sorry, but please revert the last patch of this series you've committed
already. As stated in the cover letter AND that patch, a Mini-OS patch and
the bump of the Mini-OS commit in Config.mk are required in order to avoid
build failures when trying to build the Xenstore-stubdom binaries.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v8 0/9] remove libxenctrl usage from xenstored

[Jan Beulich]

On 06.03.2025 14:27, Jürgen Groß wrote:
> On 06.03.25 14:13, Jan Beulich wrote:
>> On 06.03.2025 00:32, Stefano Stabellini wrote:
>>> On Wed, 5 Mar 2025, Juergen Gross wrote:
>>>> On 25.02.25 12:10, Juergen Gross wrote:
>>>>> Ping? Especially ...
>>>>>
>>>>> On 04.02.25 12:33, Juergen Gross wrote:
>>>>>> Xenstored is using libxenctrl for only one purpose: to get information
>>>>>> about state of domains.
>>>>>>
>>>>>> This patch series is removing that dependency by introducing a new
>>>>>> stable interface which can be used by xenstored instead.
>>>>>>
>>>>>> There was a RFC series sent out 3 years ago, which I have taken as a
>>>>>> base and by addressing all comments from back then.
>>>>>>
>>>>>> The main differences since that RFC series are:
>>>>>>
>>>>>> - Instead of introducing an new main hypercall for a stable management
>>>>>>     interface I have just added a new domctl sub-op, as requested in 2021.
>>>>>>
>>>>>> - I have added a new library libxenmanage for easy use of the new
>>>>>>     stable hypervisor interface. Main motivation for adding the library
>>>>>>     was the recent attempt to decouple oxenstored from the Xen git tree.
>>>>>>     By using the new library, oxenstored could benefit in the same way as
>>>>>>     xenstored from the new interface: it would be possible to rely on
>>>>>>     stable libraries only.
>>>>>>
>>>>>> - Mini-OS has gained some more config options recently, so it was rather
>>>>>>     easy to make xenstore[pvh]-stubdom independent from libxenctrl, too.
>>>>>>
>>>>>> Please note that the last 4 patches can be committed only after the
>>>>>> related Mini-OS patch "config: add support for libxenmanage" has gone in
>>>>>> AND the Mini-OS commit-id has been updated in Config.mk accordingly! The
>>>>>> Mini-OS patch has been Acked already, so it can go in as soon as patch
>>>>>> 5 of this series (the one introducing libxenmanage) has been committed.
>>>>>>
>>>>>> As patches 1 and 2 change current behavior, Jan didn't want to give his
>>>>>> Ack (he didn't reject the patch either). So I'm asking other "Rest"
>>>>>> maintainers to look at those patches specifically.
>>>>>
>>>>> ... patches 1 and 2 could need an additional opinion.
>>>>
>>>> And another ping.
>>>>
>>>> One of Andrew, Stefano, Julien, Roger, Anthony, Mical: Please have a look.
>>>>
>>>> The related discussion between Jan and me can be found here (patches 2 and 3):
>>>>
>>>> https://lore.kernel.org/xen-devel/20250107101711.5980-1-jgross@suse.com/
>>>
>>> I didn't do an in-details review but based on Jan's comments and your
>>> replies, I think they are an improvement. If someone else wants to do a
>>> proper review, they would be welcome.
>>>
>>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>> I've taken the conservative approach and interpreted this as an ack for the
>> two patches in question only, rather than the entire series. Please indicate
>> if it was meant the other way around, as then the final 3 patches could also
>> go in.
> 
> Sorry, but please revert the last patch of this series you've committed
> already. As stated in the cover letter AND that patch, a Mini-OS patch and
> the bump of the Mini-OS commit in Config.mk are required in order to avoid
> build failures when trying to build the Xenstore-stubdom binaries.

Indeed, I overlooked this while preparing what to commit (while I remember
noticing it earlier on). Still it's probably sub-optimal to have a series
split in the middle like this.

Instead of reverting, let's bump the MiniOS ref in staging instead, as you
did suggest on Matrix.

Jan

Re: [PATCH v8 0/9] remove libxenctrl usage from xenstored

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 3898 bytes --]

On 06.03.25 14:53, Jan Beulich wrote:
> On 06.03.2025 14:27, Jürgen Groß wrote:
>> On 06.03.25 14:13, Jan Beulich wrote:
>>> On 06.03.2025 00:32, Stefano Stabellini wrote:
>>>> On Wed, 5 Mar 2025, Juergen Gross wrote:
>>>>> On 25.02.25 12:10, Juergen Gross wrote:
>>>>>> Ping? Especially ...
>>>>>>
>>>>>> On 04.02.25 12:33, Juergen Gross wrote:
>>>>>>> Xenstored is using libxenctrl for only one purpose: to get information
>>>>>>> about state of domains.
>>>>>>>
>>>>>>> This patch series is removing that dependency by introducing a new
>>>>>>> stable interface which can be used by xenstored instead.
>>>>>>>
>>>>>>> There was a RFC series sent out 3 years ago, which I have taken as a
>>>>>>> base and by addressing all comments from back then.
>>>>>>>
>>>>>>> The main differences since that RFC series are:
>>>>>>>
>>>>>>> - Instead of introducing an new main hypercall for a stable management
>>>>>>>      interface I have just added a new domctl sub-op, as requested in 2021.
>>>>>>>
>>>>>>> - I have added a new library libxenmanage for easy use of the new
>>>>>>>      stable hypervisor interface. Main motivation for adding the library
>>>>>>>      was the recent attempt to decouple oxenstored from the Xen git tree.
>>>>>>>      By using the new library, oxenstored could benefit in the same way as
>>>>>>>      xenstored from the new interface: it would be possible to rely on
>>>>>>>      stable libraries only.
>>>>>>>
>>>>>>> - Mini-OS has gained some more config options recently, so it was rather
>>>>>>>      easy to make xenstore[pvh]-stubdom independent from libxenctrl, too.
>>>>>>>
>>>>>>> Please note that the last 4 patches can be committed only after the
>>>>>>> related Mini-OS patch "config: add support for libxenmanage" has gone in
>>>>>>> AND the Mini-OS commit-id has been updated in Config.mk accordingly! The
>>>>>>> Mini-OS patch has been Acked already, so it can go in as soon as patch
>>>>>>> 5 of this series (the one introducing libxenmanage) has been committed.
>>>>>>>
>>>>>>> As patches 1 and 2 change current behavior, Jan didn't want to give his
>>>>>>> Ack (he didn't reject the patch either). So I'm asking other "Rest"
>>>>>>> maintainers to look at those patches specifically.
>>>>>>
>>>>>> ... patches 1 and 2 could need an additional opinion.
>>>>>
>>>>> And another ping.
>>>>>
>>>>> One of Andrew, Stefano, Julien, Roger, Anthony, Mical: Please have a look.
>>>>>
>>>>> The related discussion between Jan and me can be found here (patches 2 and 3):
>>>>>
>>>>> https://lore.kernel.org/xen-devel/20250107101711.5980-1-jgross@suse.com/
>>>>
>>>> I didn't do an in-details review but based on Jan's comments and your
>>>> replies, I think they are an improvement. If someone else wants to do a
>>>> proper review, they would be welcome.
>>>>
>>>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>>>
>>> I've taken the conservative approach and interpreted this as an ack for the
>>> two patches in question only, rather than the entire series. Please indicate
>>> if it was meant the other way around, as then the final 3 patches could also
>>> go in.
>>
>> Sorry, but please revert the last patch of this series you've committed
>> already. As stated in the cover letter AND that patch, a Mini-OS patch and
>> the bump of the Mini-OS commit in Config.mk are required in order to avoid
>> build failures when trying to build the Xenstore-stubdom binaries.
> 
> Indeed, I overlooked this while preparing what to commit (while I remember
> noticing it earlier on). Still it's probably sub-optimal to have a series
> split in the middle like this.

In future I'll use 2 series in such a case.

> 
> Instead of reverting, let's bump the MiniOS ref in staging instead, as you
> did suggest on Matrix.

Yeah, patch has been sent already.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v8 3/9] xen: add bitmap to indicate per-domain state changes

[Jan Beulich]

On 04.02.2025 12:34, Juergen Gross wrote:
> @@ -1075,6 +1086,26 @@ static void clear_global_virq_handlers(struct domain *d)
>      }
>  }
>  
> +struct domain *lock_dom_exc_handler(void)
> +{
> +    struct domain *d;
> +
> +    d = get_global_virq_handler(VIRQ_DOM_EXC);

The Arm dom0less crash(es?) found by CI are apparently a result of this possibly
returning NULL (hardware_domain == NULL). Returning NULL from here also in this
case (and not just ...

> +    if ( unlikely(!get_domain(d)) )
> +        return NULL;

... in this one) would at least deal with the use from domain_changed_state(). I
didn't closely check other uses. Thoughts?

Jan

Re: [PATCH v8 3/9] xen: add bitmap to indicate per-domain state changes

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 848 bytes --]

On 06.03.25 16:41, Jan Beulich wrote:
> On 04.02.2025 12:34, Juergen Gross wrote:
>> @@ -1075,6 +1086,26 @@ static void clear_global_virq_handlers(struct domain *d)
>>       }
>>   }
>>   
>> +struct domain *lock_dom_exc_handler(void)
>> +{
>> +    struct domain *d;
>> +
>> +    d = get_global_virq_handler(VIRQ_DOM_EXC);
> 
> The Arm dom0less crash(es?) found by CI are apparently a result of this possibly
> returning NULL (hardware_domain == NULL). Returning NULL from here also in this
> case (and not just ...
> 
>> +    if ( unlikely(!get_domain(d)) )
>> +        return NULL;
> 
> ... in this one) would at least deal with the use from domain_changed_state(). I
> didn't closely check other uses. Thoughts?
> 
> Jan

Just about to post a patch.

There is one other case where a NULL check is wanted.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v8 1/9] xen/events: don't allow binding a global virq from any domain

[Julien Grall]

Hi Juergen,

On 04/02/2025 11:33, Juergen Gross wrote:
> Today Xen will happily allow binding a global virq by a domain which
> isn't configured to receive it. This won't result in any bad actions,
> but the bind will appear to have succeeded with no event ever being
> received by that event channel.
> 
> Instead of allowing the bind, error out if the domain isn't set to
> handle that virq. Note that this check is inside the write_lock() on
> purpose, as a future patch will put a related check into
> set_global_virq_handler() with the addition of using the same lock.
 > > Signed-off-by: Juergen Gross <jgross@suse.com>

I see this patch was already committed. But I have a question about the 
logic.

> ---
> V6:
> - new patch
> V7:
> - move handling domain check inside locked region (Jan Beulich)
> - style fix (Jan Beulich)
> ---
>   xen/common/event_channel.c | 21 +++++++++++++++++----
>   1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index 46281b16ce..cd6f5a1211 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -120,6 +120,13 @@ static uint8_t get_xen_consumer(xen_event_channel_notification_t fn)
>   /* Get the notification function for a given Xen-bound event channel. */
>   #define xen_notification_fn(e) (xen_consumers[(e)->xen_consumer-1])
>   
> +static struct domain *__read_mostly global_virq_handlers[NR_VIRQS];
> +
> +static struct domain *get_global_virq_handler(unsigned int virq)
> +{
> +    return global_virq_handlers[virq] ?: hardware_domain;
> +}
> +
>   static bool virq_is_global(unsigned int virq)
>   {
>       switch ( virq )
> @@ -469,6 +476,7 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port)
>       struct domain *d = current->domain;
>       int            virq = bind->virq, vcpu = bind->vcpu;
>       int            rc = 0;
> +    bool           is_global;
>   
>       if ( (virq < 0) || (virq >= ARRAY_SIZE(v->virq_to_evtchn)) )
>           return -EINVAL;
> @@ -478,8 +486,9 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port)
>       * speculative execution.
>       */
>       virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn));
> +    is_global = virq_is_global(virq);
>   
> -    if ( virq_is_global(virq) && (vcpu != 0) )
> +    if ( is_global && vcpu != 0 )
>           return -EINVAL;
>   
>       if ( (v = domain_vcpu(d, vcpu)) == NULL )
> @@ -487,6 +496,12 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port)
>   
>       write_lock(&d->event_lock);
>   
> +    if ( is_global && get_global_virq_handler(virq) != d )

What prevent a race between get_global_virq_handler() and 
set_global_virq_handler()? Also, it is not clear in the implementation 
of get_global_virq_handler() that it will ever only read 
global_virq_handlers[virq] once.

Cheers,

-- 
Julien Grall

Re: [PATCH v8 7/9] docs: update xenstore migration stream definition

[Julien Grall]

Hi Juergen,

On 04/02/2025 11:34, Juergen Gross wrote:
> In order to close a race window for Xenstore live update when using
> the new unique_id of domains, the migration stream needs to contain
> this unique_id for each domain known by Xenstore.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V8:
> - new patch
> ---
>   docs/designs/xenstore-migration.md | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/designs/xenstore-migration.md b/docs/designs/xenstore-migration.md
> index 082314bf72..fba691ee0d 100644
> --- a/docs/designs/xenstore-migration.md
> +++ b/docs/designs/xenstore-migration.md
> @@ -156,7 +156,7 @@ the domain being migrated.
>   ```
>       0       1       2       3       4       5       6       7    octet
>   +-------+-------+-------+-------+-------+-------+-------+-------+
> -| conn-id                       | conn-type     |               |
> +| conn-id                       | conn-type     | uniq-id-off   |
>   +-------------------------------+---------------+---------------+
 >   | conn-spec>   ...
> @@ -165,6 +165,9 @@ the domain being migrated.
>   +---------------+---------------+-------------------------------+
>   | data
>   ...
> ++---------------------------------------------------------------+
> +| unique-id                                                     |
> ++---------------------------------------------------------------+
>   ```
>   
>   
> @@ -178,6 +181,12 @@ the domain being migrated.
>   |                | 0x0001: socket                               |
>   |                | 0x0002 - 0xFFFF: reserved for future use     |
>   |                |                                              |
> +| `uniq-id-off`  | The offset (in octets) of the `unique-id`    |
> +|                | field from the start of the record body.     |
> +|                | If 0, no `unique-id` field is present.       |
> +|                | Only needed for `shared ring` connection in  |
> +|                | live update streams.                         |
> +|                |                                              |

Looking at the rest of the record, the unique ID would be past the 
in-data (length is 2-byte) and the out-data (length is 4-byte). So 
technically the offset would need 5-bytes. But here you are using 
2-bytes. Can you explain why?

Cheers,

-- 
Julien Grall

Re: [PATCH v8 1/9] xen/events: don't allow binding a global virq from any domain

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 3518 bytes --]

On 11.03.25 10:35, Julien Grall wrote:
> Hi Juergen,
> 
> On 04/02/2025 11:33, Juergen Gross wrote:
>> Today Xen will happily allow binding a global virq by a domain which
>> isn't configured to receive it. This won't result in any bad actions,
>> but the bind will appear to have succeeded with no event ever being
>> received by that event channel.
>>
>> Instead of allowing the bind, error out if the domain isn't set to
>> handle that virq. Note that this check is inside the write_lock() on
>> purpose, as a future patch will put a related check into
>> set_global_virq_handler() with the addition of using the same lock.
>  > > Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> I see this patch was already committed. But I have a question about the logic.
> 
>> ---
>> V6:
>> - new patch
>> V7:
>> - move handling domain check inside locked region (Jan Beulich)
>> - style fix (Jan Beulich)
>> ---
>>   xen/common/event_channel.c | 21 +++++++++++++++++----
>>   1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>> index 46281b16ce..cd6f5a1211 100644
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -120,6 +120,13 @@ static uint8_t 
>> get_xen_consumer(xen_event_channel_notification_t fn)
>>   /* Get the notification function for a given Xen-bound event channel. */
>>   #define xen_notification_fn(e) (xen_consumers[(e)->xen_consumer-1])
>> +static struct domain *__read_mostly global_virq_handlers[NR_VIRQS];
>> +
>> +static struct domain *get_global_virq_handler(unsigned int virq)
>> +{
>> +    return global_virq_handlers[virq] ?: hardware_domain;
>> +}
>> +
>>   static bool virq_is_global(unsigned int virq)
>>   {
>>       switch ( virq )
>> @@ -469,6 +476,7 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
>> evtchn_port_t port)
>>       struct domain *d = current->domain;
>>       int            virq = bind->virq, vcpu = bind->vcpu;
>>       int            rc = 0;
>> +    bool           is_global;
>>       if ( (virq < 0) || (virq >= ARRAY_SIZE(v->virq_to_evtchn)) )
>>           return -EINVAL;
>> @@ -478,8 +486,9 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
>> evtchn_port_t port)
>>       * speculative execution.
>>       */
>>       virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn));
>> +    is_global = virq_is_global(virq);
>> -    if ( virq_is_global(virq) && (vcpu != 0) )
>> +    if ( is_global && vcpu != 0 )
>>           return -EINVAL;
>>       if ( (v = domain_vcpu(d, vcpu)) == NULL )
>> @@ -487,6 +496,12 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
>> evtchn_port_t port)
>>       write_lock(&d->event_lock);
>> +    if ( is_global && get_global_virq_handler(virq) != d )
> 
> What prevent a race between get_global_virq_handler() and 
> set_global_virq_handler()? Also, it is not clear in the implementation of 
> get_global_virq_handler() that it will ever only read global_virq_handlers[virq] 
> once.

set_global_virq_handler() is taking the event_lock of the domain
registered as handler.

So if a domain is registered for handling a virq, d->event_lock is
protecting against the handling domain to be changed. Concurrent
calls of set_global_virq_handler() are handled via taking the
global_virq_handlers_lock spin_lock.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v8 7/9] docs: update xenstore migration stream definition

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 3139 bytes --]

On 11.03.25 10:43, Julien Grall wrote:
> Hi Juergen,
> 
> On 04/02/2025 11:34, Juergen Gross wrote:
>> In order to close a race window for Xenstore live update when using
>> the new unique_id of domains, the migration stream needs to contain
>> this unique_id for each domain known by Xenstore.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V8:
>> - new patch
>> ---
>>   docs/designs/xenstore-migration.md | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/designs/xenstore-migration.md b/docs/designs/xenstore- 
>> migration.md
>> index 082314bf72..fba691ee0d 100644
>> --- a/docs/designs/xenstore-migration.md
>> +++ b/docs/designs/xenstore-migration.md
>> @@ -156,7 +156,7 @@ the domain being migrated.
>>   ```
>>       0       1       2       3       4       5       6       7    octet
>>   +-------+-------+-------+-------+-------+-------+-------+-------+
>> -| conn-id                       | conn-type     |               |
>> +| conn-id                       | conn-type     | uniq-id-off   |
>>   +-------------------------------+---------------+---------------+
>  >   | conn-spec>   ...
>> @@ -165,6 +165,9 @@ the domain being migrated.
>>   +---------------+---------------+-------------------------------+
>>   | data
>>   ...
>> ++---------------------------------------------------------------+
>> +| unique-id                                                     |
>> ++---------------------------------------------------------------+
>>   ```
>> @@ -178,6 +181,12 @@ the domain being migrated.
>>   |                | 0x0001: socket                               |
>>   |                | 0x0002 - 0xFFFF: reserved for future use     |
>>   |                |                                              |
>> +| `uniq-id-off`  | The offset (in octets) of the `unique-id`    |
>> +|                | field from the start of the record body.     |
>> +|                | If 0, no `unique-id` field is present.       |
>> +|                | Only needed for `shared ring` connection in  |
>> +|                | live update streams.                         |
>> +|                |                                              |
> 
> Looking at the rest of the record, the unique ID would be past the in-data 
> (length is 2-byte) and the out-data (length is 4-byte). So technically the 
> offset would need 5-bytes. But here you are using 2-bytes. Can you explain why?

Good catch.

I think I need to change this to be a flag indicating that the unique-id
is present and located at the next 8-byte boundary after "data".


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v8 7/9] docs: update xenstore migration stream definition

[Julien Grall]

Hi,

On 11/03/2025 09:58, Jürgen Groß wrote:
> On 11.03.25 10:43, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 04/02/2025 11:34, Juergen Gross wrote:
>>> In order to close a race window for Xenstore live update when using
>>> the new unique_id of domains, the migration stream needs to contain
>>> this unique_id for each domain known by Xenstore.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> V8:
>>> - new patch
>>> ---
>>>   docs/designs/xenstore-migration.md | 14 +++++++++++++-
>>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/docs/designs/xenstore-migration.md b/docs/designs/ 
>>> xenstore- migration.md
>>> index 082314bf72..fba691ee0d 100644
>>> --- a/docs/designs/xenstore-migration.md
>>> +++ b/docs/designs/xenstore-migration.md
>>> @@ -156,7 +156,7 @@ the domain being migrated.
>>>   ```
>>>       0       1       2       3       4       5       6       7    octet
>>>   +-------+-------+-------+-------+-------+-------+-------+-------+
>>> -| conn-id                       | conn-type     |               |
>>> +| conn-id                       | conn-type     | uniq-id-off   |
>>>   +-------------------------------+---------------+---------------+
>>  >   | conn-spec>   ...
>>> @@ -165,6 +165,9 @@ the domain being migrated.
>>>   +---------------+---------------+-------------------------------+
>>>   | data
>>>   ...
>>> ++---------------------------------------------------------------+
>>> +| unique-id                                                     |
>>> ++---------------------------------------------------------------+
>>>   ```
>>> @@ -178,6 +181,12 @@ the domain being migrated.
>>>   |                | 0x0001: socket                               |
>>>   |                | 0x0002 - 0xFFFF: reserved for future use     |
>>>   |                |                                              |
>>> +| `uniq-id-off`  | The offset (in octets) of the `unique-id`    |
>>> +|                | field from the start of the record body.     |
>>> +|                | If 0, no `unique-id` field is present.       |
>>> +|                | Only needed for `shared ring` connection in  |
>>> +|                | live update streams.                         |
>>> +|                |                                              |
>>
>> Looking at the rest of the record, the unique ID would be past the in- 
>> data (length is 2-byte) and the out-data (length is 4-byte). So 
>> technically the offset would need 5-bytes. But here you are using 2- 
>> bytes. Can you explain why?
> 
> Good catch.
> 
> I think I need to change this to be a flag indicating that the unique-id
> is present and located at the next 8-byte boundary after "data".

This would work for me.

Cheers,

-- 
Julien Grall

Re: [PATCH v8 1/9] xen/events: don't allow binding a global virq from any domain

[Julien Grall]

Hi Juergen,

On 11/03/2025 09:51, Jürgen Groß wrote:
> On 11.03.25 10:35, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 04/02/2025 11:33, Juergen Gross wrote:
>>> Today Xen will happily allow binding a global virq by a domain which
>>> isn't configured to receive it. This won't result in any bad actions,
>>> but the bind will appear to have succeeded with no event ever being
>>> received by that event channel.
>>>
>>> Instead of allowing the bind, error out if the domain isn't set to
>>> handle that virq. Note that this check is inside the write_lock() on
>>> purpose, as a future patch will put a related check into
>>> set_global_virq_handler() with the addition of using the same lock.
>>  > > Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>> I see this patch was already committed. But I have a question about 
>> the logic.
>>
>>> ---
>>> V6:
>>> - new patch
>>> V7:
>>> - move handling domain check inside locked region (Jan Beulich)
>>> - style fix (Jan Beulich)
>>> ---
>>>   xen/common/event_channel.c | 21 +++++++++++++++++----
>>>   1 file changed, 17 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>>> index 46281b16ce..cd6f5a1211 100644
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -120,6 +120,13 @@ static uint8_t 
>>> get_xen_consumer(xen_event_channel_notification_t fn)
>>>   /* Get the notification function for a given Xen-bound event 
>>> channel. */
>>>   #define xen_notification_fn(e) (xen_consumers[(e)->xen_consumer-1])
>>> +static struct domain *__read_mostly global_virq_handlers[NR_VIRQS];
>>> +
>>> +static struct domain *get_global_virq_handler(unsigned int virq)
>>> +{
>>> +    return global_virq_handlers[virq] ?: hardware_domain;
>>> +}
>>> +
>>>   static bool virq_is_global(unsigned int virq)
>>>   {
>>>       switch ( virq )
>>> @@ -469,6 +476,7 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
>>> evtchn_port_t port)
>>>       struct domain *d = current->domain;
>>>       int            virq = bind->virq, vcpu = bind->vcpu;
>>>       int            rc = 0;
>>> +    bool           is_global;
>>>       if ( (virq < 0) || (virq >= ARRAY_SIZE(v->virq_to_evtchn)) )
>>>           return -EINVAL;
>>> @@ -478,8 +486,9 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
>>> evtchn_port_t port)
>>>       * speculative execution.
>>>       */
>>>       virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn));
>>> +    is_global = virq_is_global(virq);
>>> -    if ( virq_is_global(virq) && (vcpu != 0) )
>>> +    if ( is_global && vcpu != 0 )
>>>           return -EINVAL;
>>>       if ( (v = domain_vcpu(d, vcpu)) == NULL )
>>> @@ -487,6 +496,12 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
>>> evtchn_port_t port)
>>>       write_lock(&d->event_lock);
>>> +    if ( is_global && get_global_virq_handler(virq) != d )
>>
>> What prevent a race between get_global_virq_handler() and 
>> set_global_virq_handler()? Also, it is not clear in the implementation 
>> of get_global_virq_handler() that it will ever only read 
>> global_virq_handlers[virq] once.
> 
> set_global_virq_handler() is taking the event_lock of the domain
> registered as handler.
> 
> So if a domain is registered for handling a virq, d->event_lock is
> protecting against the handling domain to be changed. Concurrent
> calls of set_global_virq_handler() are handled via taking the
> global_virq_handlers_lock spin_lock.

I agree this would work for evtchn_bind_virq() because we only ever 
compare. But I still wonder whether get_global_virq_handler() should 
gain an ACCESS_ONCE()? Could the compiler decide to read 
global_virq_handlers[...] twice and therefore return NULL?

Cheers,

-- 
Julien Grall

Re: [PATCH v8 1/9] xen/events: don't allow binding a global virq from any domain

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 4286 bytes --]

On 11.03.25 20:04, Julien Grall wrote:
> Hi Juergen,
> 
> On 11/03/2025 09:51, Jürgen Groß wrote:
>> On 11.03.25 10:35, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 04/02/2025 11:33, Juergen Gross wrote:
>>>> Today Xen will happily allow binding a global virq by a domain which
>>>> isn't configured to receive it. This won't result in any bad actions,
>>>> but the bind will appear to have succeeded with no event ever being
>>>> received by that event channel.
>>>>
>>>> Instead of allowing the bind, error out if the domain isn't set to
>>>> handle that virq. Note that this check is inside the write_lock() on
>>>> purpose, as a future patch will put a related check into
>>>> set_global_virq_handler() with the addition of using the same lock.
>>>  > > Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> I see this patch was already committed. But I have a question about the logic.
>>>
>>>> ---
>>>> V6:
>>>> - new patch
>>>> V7:
>>>> - move handling domain check inside locked region (Jan Beulich)
>>>> - style fix (Jan Beulich)
>>>> ---
>>>>   xen/common/event_channel.c | 21 +++++++++++++++++----
>>>>   1 file changed, 17 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>>>> index 46281b16ce..cd6f5a1211 100644
>>>> --- a/xen/common/event_channel.c
>>>> +++ b/xen/common/event_channel.c
>>>> @@ -120,6 +120,13 @@ static uint8_t 
>>>> get_xen_consumer(xen_event_channel_notification_t fn)
>>>>   /* Get the notification function for a given Xen-bound event channel. */
>>>>   #define xen_notification_fn(e) (xen_consumers[(e)->xen_consumer-1])
>>>> +static struct domain *__read_mostly global_virq_handlers[NR_VIRQS];
>>>> +
>>>> +static struct domain *get_global_virq_handler(unsigned int virq)
>>>> +{
>>>> +    return global_virq_handlers[virq] ?: hardware_domain;
>>>> +}
>>>> +
>>>>   static bool virq_is_global(unsigned int virq)
>>>>   {
>>>>       switch ( virq )
>>>> @@ -469,6 +476,7 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
>>>> evtchn_port_t port)
>>>>       struct domain *d = current->domain;
>>>>       int            virq = bind->virq, vcpu = bind->vcpu;
>>>>       int            rc = 0;
>>>> +    bool           is_global;
>>>>       if ( (virq < 0) || (virq >= ARRAY_SIZE(v->virq_to_evtchn)) )
>>>>           return -EINVAL;
>>>> @@ -478,8 +486,9 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
>>>> evtchn_port_t port)
>>>>       * speculative execution.
>>>>       */
>>>>       virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn));
>>>> +    is_global = virq_is_global(virq);
>>>> -    if ( virq_is_global(virq) && (vcpu != 0) )
>>>> +    if ( is_global && vcpu != 0 )
>>>>           return -EINVAL;
>>>>       if ( (v = domain_vcpu(d, vcpu)) == NULL )
>>>> @@ -487,6 +496,12 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
>>>> evtchn_port_t port)
>>>>       write_lock(&d->event_lock);
>>>> +    if ( is_global && get_global_virq_handler(virq) != d )
>>>
>>> What prevent a race between get_global_virq_handler() and 
>>> set_global_virq_handler()? Also, it is not clear in the implementation of 
>>> get_global_virq_handler() that it will ever only read 
>>> global_virq_handlers[virq] once.
>>
>> set_global_virq_handler() is taking the event_lock of the domain
>> registered as handler.
>>
>> So if a domain is registered for handling a virq, d->event_lock is
>> protecting against the handling domain to be changed. Concurrent
>> calls of set_global_virq_handler() are handled via taking the
>> global_virq_handlers_lock spin_lock.
> 
> I agree this would work for evtchn_bind_virq() because we only ever compare. But 
> I still wonder whether get_global_virq_handler() should gain an ACCESS_ONCE()? 
> Could the compiler decide to read global_virq_handlers[...] twice and therefore 
> return NULL?

I don't think there is currently any use case of get_global_virq_handler()
where this would be a problem. get_global_virq_handler() is allowed to
return NULL in case of e.g. a dom0less system where hardware_domain is NULL.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v8 8/9] tools/xenstored: use unique_id to identify new domain with same domid

[Jason Andryuk]

On 2025-02-04 06:34, Juergen Gross wrote:
> Use the new unique_id of a domain in order to detect that a domain
> has been replaced with another one reusing the doamin-id of the old

s/doamin/domain/

> domain.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---

> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
> index a6506a5bb2..63df24030e 100644
> --- a/tools/xenstored/domain.c
> +++ b/tools/xenstored/domain.c

> @@ -627,9 +628,17 @@ static int check_domain(const void *k, void *v, void *arg)
>   	int dom_invalid;
>   	struct domain *domain = v;
>   	bool *notify = arg;
> +	uint64_t unique_id;
>   
>   	dom_invalid = xenmanage_get_domain_info(xm_handle, domain->domid,
> -						&state, NULL);
> +						&state, &unique_id);
> +	if (!dom_invalid) {

What do you think about  `if (dom_valid)` to avoid the double negative?

If you don't want to change it, the code here and elsewhere looks fine 
to me as-is.

You'll re-spin with updated dump code, correct?

Regards,
Jason

Re: [PATCH v8 9/9] tools/xenstored: use xenmanage_poll_changed_domain()

[Jason Andryuk]

On 2025-02-04 06:34, Juergen Gross wrote:
> Instead of checking each known domain after having received a
> VIRQ_DOM_EXC event, use the new xenmanage_poll_changed_domain()
> function for directly getting the domid of a domain having changed
> its state.
> 
> A test doing "xl shutdown" of 1000 guests has shown to reduce the
> consumed cpu time of xenstored by 6% with this change applied.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

I see at least some dom_invalid uses goes away, so maybe don't bother 
changing it.

Regards,
Jason

Re: [PATCH v8 8/9] tools/xenstored: use unique_id to identify new domain with same domid

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1267 bytes --]

On 12.03.25 18:40, Jason Andryuk wrote:
> On 2025-02-04 06:34, Juergen Gross wrote:
>> Use the new unique_id of a domain in order to detect that a domain
>> has been replaced with another one reusing the doamin-id of the old
> 
> s/doamin/domain/
> 
>> domain.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
> 
>> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
>> index a6506a5bb2..63df24030e 100644
>> --- a/tools/xenstored/domain.c
>> +++ b/tools/xenstored/domain.c
> 
>> @@ -627,9 +628,17 @@ static int check_domain(const void *k, void *v, void *arg)
>>       int dom_invalid;
>>       struct domain *domain = v;
>>       bool *notify = arg;
>> +    uint64_t unique_id;
>>       dom_invalid = xenmanage_get_domain_info(xm_handle, domain->domid,
>> -                        &state, NULL);
>> +                        &state, &unique_id);
>> +    if (!dom_invalid) {
> 
> What do you think about  `if (dom_valid)` to avoid the double negative?

Yes, probably better.

> If you don't want to change it, the code here and elsewhere looks fine to me as-is.
> 
> You'll re-spin with updated dump code, correct?

Yes.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v8 9/9] tools/xenstored: use xenmanage_poll_changed_domain()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 686 bytes --]

On 12.03.25 18:45, Jason Andryuk wrote:
> On 2025-02-04 06:34, Juergen Gross wrote:
>> Instead of checking each known domain after having received a
>> VIRQ_DOM_EXC event, use the new xenmanage_poll_changed_domain()
>> function for directly getting the domid of a domain having changed
>> its state.
>>
>> A test doing "xl shutdown" of 1000 guests has shown to reduce the
>> consumed cpu time of xenstored by 6% with this change applied.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> 
> I see at least some dom_invalid uses goes away, so maybe don't bother changing it.

I'll do the change.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]