[PATCH 0/6] remove libxenctrl usage from xenstored

[PATCH 0/6] 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.

- By moving the CPU barrier definitions out of xenctrl.h into a new
  dedicated header xenstored code no longer needs to #include xenctrl.h,
  thus removing any xenctrl reference from xenstored code.

Please note that the last 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!

Juergen Gross (6):
  xen: add a domain unique id to each domain
  xen: add bitmap to indicate per-domain state changes
  xen: add new domctl get_changed_domain
  tools/libs: add a new libxenmanage library
  tools: add a dedicated header file for barrier definitions
  tools/xenstored: use new stable interface instead of libxenctrl

 stubdom/Makefile                    |   8 +-
 stubdom/mini-os.mk                  |   1 +
 tools/9pfsd/io.c                    |   5 +-
 tools/flask/policy/modules/dom0.te  |   2 +-
 tools/include/xen-barrier.h         |  51 +++++++++
 tools/include/xenctrl.h             |  28 +----
 tools/include/xenmanage.h           |  98 ++++++++++++++++
 tools/libs/Makefile                 |   1 +
 tools/libs/ctrl/Makefile            |   2 +-
 tools/libs/manage/Makefile          |  10 ++
 tools/libs/manage/Makefile.common   |   1 +
 tools/libs/manage/core.c            | 170 ++++++++++++++++++++++++++++
 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            |  52 ++++-----
 tools/xenstored/lu.c                |   1 +
 tools/xenstored/lu_daemon.c         |   1 +
 xen/common/domain.c                 |  92 +++++++++++++++
 xen/common/domctl.c                 |  19 +++-
 xen/common/event_channel.c          |  11 +-
 xen/include/public/domctl.h         |  33 ++++++
 xen/include/xen/event.h             |   6 +
 xen/include/xen/sched.h             |   7 ++
 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, 566 insertions(+), 72 deletions(-)
 create mode 100644 tools/include/xen-barrier.h
 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 1/6] xen: add a domain unique id to each domain

[Juergen Gross]

Xenstore is referencing domains by their domid, but reuse of a domid
can lead to the situation that Xenstore can't tell whether a domain
with that domid has been deleted and created again without Xenstore
noticing the domain is a new one now.

Add a global domain creation unique id which is updated when creating
a new domain, and store that value in struct domain of the new domain.
The global unique id is initialized with the system time and updates
are done via the xorshift algorithm which is used for pseudo random
number generation, too (see https://en.wikipedia.org/wiki/Xorshift).

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
V1:
- make unique_id local to function (Jan Beulich)
- add lock (Julien Grall)
- add comment (Julien Grall)
---
 xen/common/domain.c     | 20 ++++++++++++++++++++
 xen/include/xen/sched.h |  3 +++
 2 files changed, 23 insertions(+)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 92263a4fbd..3948640fb0 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -562,6 +562,25 @@ static void _domain_destroy(struct domain *d)
     free_domain_struct(d);
 }
 
+static uint64_t get_unique_id(void)
+{
+    static uint64_t unique_id;
+    static DEFINE_SPINLOCK(lock);
+    uint64_t x = unique_id ? : NOW();
+
+    spin_lock(&lock);
+
+    /* Pseudo-randomize id in order to avoid consumers relying on sequence. */
+    x ^= x << 13;
+    x ^= x >> 7;
+    x ^= x << 17;
+    unique_id = x;
+
+    spin_unlock(&lock);
+
+    return x;
+}
+
 static int sanitise_domain_config(struct xen_domctl_createdomain *config)
 {
     bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
@@ -654,6 +673,7 @@ struct domain *domain_create(domid_t domid,
 
     /* Sort out our idea of is_system_domain(). */
     d->domain_id = domid;
+    d->unique_id = get_unique_id();
 
     /* Holding CDF_* internal flags. */
     d->cdf = flags;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 90666576c2..1dd8a425f9 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -370,6 +370,9 @@ struct domain
     domid_t          domain_id;
 
     unsigned int     max_vcpus;
+
+    uint64_t         unique_id;       /* Unique domain identifier */
+
     struct vcpu    **vcpu;
 
     shared_info_t   *shared_info;     /* shared data area */
-- 
2.43.0

[PATCH 2/6] 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.

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>
---
 xen/common/domain.c        | 21 +++++++++++++++++++++
 xen/common/event_channel.c |  2 ++
 xen/include/xen/sched.h    |  2 ++
 3 files changed, 25 insertions(+)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 3948640fb0..61b7899cb8 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -138,6 +138,22 @@ bool __read_mostly vmtrace_available;
 
 bool __read_mostly vpmu_is_available;
 
+static DECLARE_BITMAP(dom_state_changed, DOMID_MASK + 1);
+
+void domain_reset_states(void)
+{
+    struct domain *d;
+
+    bitmap_zero(dom_state_changed, DOMID_MASK + 1);
+
+    rcu_read_lock(&domlist_read_lock);
+
+    for_each_domain ( d )
+        set_bit(d->domain_id, dom_state_changed);
+
+    rcu_read_unlock(&domlist_read_lock);
+}
+
 static void __domain_finalise_shutdown(struct domain *d)
 {
     struct vcpu *v;
@@ -152,6 +168,7 @@ static void __domain_finalise_shutdown(struct domain *d)
             return;
 
     d->is_shut_down = 1;
+    set_bit(d->domain_id, dom_state_changed);
     if ( (d->shutdown_code == SHUTDOWN_suspend) && d->suspend_evtchn )
         evtchn_send(d, d->suspend_evtchn);
     else
@@ -832,6 +849,7 @@ struct domain *domain_create(domid_t domid,
      */
     domlist_insert(d);
 
+    set_bit(d->domain_id, dom_state_changed);
     memcpy(d->handle, config->handle, sizeof(d->handle));
 
     return d;
@@ -1097,6 +1115,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);
+        set_bit(d->domain_id, dom_state_changed);
         put_domain(d);
         send_global_virq(VIRQ_DOM_EXC);
         /* fallthrough */
@@ -1286,6 +1305,8 @@ static void cf_check complete_domain_destroy(struct rcu_head *head)
 
     xfree(d->vcpu);
 
+    set_bit(d->domain_id, dom_state_changed);
+
     _domain_destroy(d);
 
     send_global_virq(VIRQ_DOM_EXC);
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 8db2ca4ba2..9b87d29968 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -1296,6 +1296,8 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         rc = evtchn_bind_virq(&bind_virq, 0);
         if ( !rc && __copy_to_guest(arg, &bind_virq, 1) )
             rc = -EFAULT; /* Cleaning up here would be a mess! */
+        if ( !rc && bind_virq.virq == VIRQ_DOM_EXC )
+            domain_reset_states();
         break;
     }
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 1dd8a425f9..667863263d 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -800,6 +800,8 @@ void domain_resume(struct domain *d);
 
 int domain_soft_reset(struct domain *d, bool resuming);
 
+void domain_reset_states(void);
+
 int vcpu_start_shutdown_deferral(struct vcpu *v);
 void vcpu_end_shutdown_deferral(struct vcpu *v);
 
-- 
2.43.0

[PATCH 3/6] 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 specific to the respective domctl op and it is an
in/out parameter: On input the caller is specifying the desired version
of the op, while on output the hypervisor will return the used version
(this will be at max the caller supplied version, but might be lower in
case the hypervisor doesn't support this version).

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V1:
- use a domctl subop for the new interface (Jan Beulich)
---
 tools/flask/policy/modules/dom0.te  |  2 +-
 xen/common/domain.c                 | 51 +++++++++++++++++++++++++++++
 xen/common/domctl.c                 | 19 ++++++++++-
 xen/common/event_channel.c          |  9 ++++-
 xen/include/public/domctl.h         | 33 +++++++++++++++++++
 xen/include/xen/event.h             |  6 ++++
 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, 143 insertions(+), 3 deletions(-)

diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
index 16b8c9646d..6043c01b12 100644
--- a/tools/flask/policy/modules/dom0.te
+++ b/tools/flask/policy/modules/dom0.te
@@ -40,7 +40,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
+	get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy get_domain_state
 };
 allow dom0_t dom0_t:resource { add remove };
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 61b7899cb8..e55c6f6c5e 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -154,6 +154,57 @@ void domain_reset_states(void)
     rcu_read_unlock(&domlist_read_lock);
 }
 
+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_dead )
+        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DYING;
+    info->unique_id = d->unique_id;
+}
+
+int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d)
+{
+    unsigned int dom;
+
+    memset(info, 0, sizeof(*info));
+
+    if ( d )
+    {
+        set_domain_state_info(info, d);
+
+        return 0;
+    }
+
+    while ( (dom = find_first_bit(dom_state_changed, DOMID_MASK + 1)) <
+            DOMID_FIRST_RESERVED )
+    {
+        d = rcu_lock_domain_by_id(dom);
+
+        if ( test_and_clear_bit(dom, dom_state_changed) )
+        {
+            info->domid = dom;
+            if ( d )
+            {
+                set_domain_state_info(info, d);
+
+                rcu_unlock_domain(d);
+            }
+
+            return 0;
+        }
+
+        if ( d )
+        {
+            rcu_unlock_domain(d);
+        }
+    }
+
+    return -ENOENT;
+}
+
 static void __domain_finalise_shutdown(struct domain *d)
 {
     struct vcpu *v;
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index ea16b75910..e030cce2ec 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -278,6 +278,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;
@@ -288,7 +293,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 != XEN_DOMCTL_INTERFACE_VERSION &&
+         !is_stable_domctl(op->cmd) )
         return -EACCES;
 
     switch ( op->cmd )
@@ -309,6 +315,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;
@@ -866,6 +873,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
                 __HYPERVISOR_domctl, "h", u_domctl);
         break;
 
+    case XEN_DOMCTL_get_domain_state:
+        ret = xsm_get_domain_state(XSM_HOOK, d);
+        if ( ret )
+            break;
+
+        copyback = 1;
+        op->interface_version = XEN_DOMCTL_GETDOMSTATE_VERS_MAX;
+        ret = get_domain_state(&op->u.get_domain_state, d);
+        break;
+
     default:
         ret = arch_do_domctl(op, d, u_domctl);
         break;
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 9b87d29968..13484b3e0d 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -969,11 +969,18 @@ static struct domain *global_virq_handlers[NR_VIRQS] __read_mostly;
 
 static DEFINE_SPINLOCK(global_virq_handlers_lock);
 
+struct domain *get_global_virq_handler(uint32_t virq)
+{
+    ASSERT(virq_is_global(virq));
+
+    return global_virq_handlers[virq] ?: hardware_domain;
+}
+
 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)
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 353f831e40..63607f7754 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
  */
@@ -1236,7 +1237,37 @@ struct xen_domctl_dt_overlay {
 };
 #endif
 
+/*
+ * 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).
+ *
+ * Supported interface versions: 0x00000000
+ */
+#define XEN_DOMCTL_GETDOMSTATE_VERS_MAX    0
+struct xen_domctl_get_domain_state {
+    domid_t domid;
+    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. */
+    uint32_t pad1;           /* Returned as 0. */
+    uint64_t unique_id;      /* Unique domain identifier. */
+    uint64_t pad2[6];        /* Returned as 0. */
+};
+
 struct xen_domctl {
+/*
+ * Stable domctl ops:
+ * interface_version is per cmd, hypervisor can support multiple versions
+ * and will return used version (at max caller supplied value) in
+ * interface_version on return.
+ */
     uint32_t cmd;
 #define XEN_DOMCTL_createdomain                   1
 #define XEN_DOMCTL_destroydomain                  2
@@ -1325,6 +1356,7 @@ struct xen_domctl {
 #define XEN_DOMCTL_set_paging_mempool_size       86
 #define XEN_DOMCTL_dt_overlay                    87
 #define XEN_DOMCTL_gsi_permission                88
+#define XEN_DOMCTL_get_domain_state              89 /* stable interface */
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1391,6 +1423,7 @@ struct xen_domctl {
 #if defined(__arm__) || defined(__aarch64__)
         struct xen_domctl_dt_overlay        dt_overlay;
 #endif
+        struct xen_domctl_get_domain_state  get_domain_state;
         uint8_t                             pad[128];
     } u;
 };
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 48b79f3d62..f380668609 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -43,6 +43,12 @@ void send_guest_global_virq(struct domain *d, uint32_t virq);
  */
 int set_global_virq_handler(struct domain *d, uint32_t virq);
 
+/*
+ * get_global_virq_handler: Get domain handling a global virq.
+ *  @virq:     Virtual IRQ number (VIRQ_*), must be global
+ */
+struct domain *get_global_virq_handler(uint32_t virq);
+
 /*
  * send_guest_pirq:
  *  @d:        Domain to which physical IRQ should be sent
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 667863263d..eb256de9d5 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -801,6 +801,8 @@ void domain_resume(struct domain *d);
 int domain_soft_reset(struct domain *d, bool resuming);
 
 void domain_reset_states(void);
+int get_domain_state(struct xen_domctl_get_domain_state *info,
+                     struct domain *d);
 
 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 7956f27a29..eaf5b05093 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_HOOK);
+    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 627c0d2731..933b3889c9 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -201,6 +201,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
@@ -775,6 +776,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 dfa23738cd..b1bad7e420 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -686,6 +686,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:
@@ -1853,6 +1854,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_DOMAIN, DOMAIN__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,
@@ -1989,6 +1995,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 a35e3d4c51..c9a8eeda4e 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -251,6 +251,8 @@ class domain2
     resource_map
 # XEN_DOMCTL_get_cpu_policy
     get_cpu_policy
+# XEN_DOMCTL_get_domain_state
+    get_domain_state
 }
 
 # Similar to class domain, but primarily contains domctls related to HVM domains
-- 
2.43.0

[PATCH 4/6] 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>
---
V1:
- new patch
---
 tools/include/xenmanage.h          |  98 +++++++++++++++++
 tools/libs/Makefile                |   1 +
 tools/libs/manage/Makefile         |  10 ++
 tools/libs/manage/Makefile.common  |   1 +
 tools/libs/manage/core.c           | 170 +++++++++++++++++++++++++++++
 tools/libs/manage/libxenmanage.map |   8 ++
 tools/libs/uselibs.mk              |   2 +
 7 files changed, 290 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..2e6c3dedaa
--- /dev/null
+++ b/tools/include/xenmanage.h
@@ -0,0 +1,98 @@
+/*
+ * 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/>.
+ */
+#ifndef XENMANAGE_H
+#define XENMANAGE_H
+
+#include <stdint.h>
+
+/* Callers who don't care don't 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. */
+
+/*
+ * 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_get_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..d9efe3462e
--- /dev/null
+++ b/tools/libs/manage/Makefile.common
@@ -0,0 +1 @@
+OBJS-y  += core.o
diff --git a/tools/libs/manage/core.c b/tools/libs/manage/core.c
new file mode 100644
index 0000000000..0c9199f829
--- /dev/null
+++ b/tools/libs/manage/core.c
@@ -0,0 +1,170 @@
+/*
+ * 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 __XEN_TOOLS__ 1
+
+#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 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;
+    struct xen_domctl_get_domain_state *st;
+    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->interface_version = XEN_DOMCTL_GETDOMSTATE_VERS_MAX;
+    buf->domain = domid_in;
+
+    ret = xencall1(hdl->xcall, __HYPERVISOR_domctl, (unsigned long)buf);
+    saved_errno = errno;
+    if ( !ret )
+    {
+        st = &buf->u.get_domain_state;
+
+        if ( domid_out )
+            *domid_out = st->domid;
+        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 ( 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_get_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..65ec76f882
--- /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_get_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 5/6] tools: add a dedicated header file for barrier definitions

[Juergen Gross]

Instead of having to include xenctrl.h for getting definitions of cpu
barriers, add a dedicated header for that purpose.

Switch the xen-9pfsd daemon to use the new header instead of xenctrl.h.

This is in preparation of making Xenstore independent from libxenctrl.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V1:
- new patch
---
 tools/9pfsd/io.c            |  5 +++-
 tools/include/xen-barrier.h | 51 +++++++++++++++++++++++++++++++++++++
 tools/include/xenctrl.h     | 28 +-------------------
 tools/libs/ctrl/Makefile    |  2 +-
 4 files changed, 57 insertions(+), 29 deletions(-)
 create mode 100644 tools/include/xen-barrier.h

diff --git a/tools/9pfsd/io.c b/tools/9pfsd/io.c
index 468e0241f5..14cfcaf568 100644
--- a/tools/9pfsd/io.c
+++ b/tools/9pfsd/io.c
@@ -13,15 +13,18 @@
 
 #include <assert.h>
 #include <errno.h>
+#include <stdarg.h>
 #include <stdbool.h>
+#include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <syslog.h>
+#include <unistd.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <dirent.h>
 #include <fcntl.h>
-#include <xenctrl.h>           /* For cpu barriers. */
+#include <xen-barrier.h>
 #include <xen-tools/common-macros.h>
 
 #include "xen-9pfsd.h"
diff --git a/tools/include/xen-barrier.h b/tools/include/xen-barrier.h
new file mode 100644
index 0000000000..62036f528b
--- /dev/null
+++ b/tools/include/xen-barrier.h
@@ -0,0 +1,51 @@
+/******************************************************************************
+ * xen-barrier.h
+ *
+ * Definition of CPU barriers, part of libxenctrl.
+ *
+ * Copyright (c) 2003-2004, K A Fraser.
+ *
+ * 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/>.
+ */
+
+#ifndef XENBARRIER_H
+#define XENBARRIER_H
+
+/*
+ *  DEFINITIONS FOR CPU BARRIERS
+ */
+
+#define xen_barrier() asm volatile ( "" : : : "memory")
+
+#if defined(__i386__)
+#define xen_mb()  asm volatile ( "lock addl $0, -4(%%esp)" ::: "memory" )
+#define xen_rmb() xen_barrier()
+#define xen_wmb() xen_barrier()
+#elif defined(__x86_64__)
+#define xen_mb()  asm volatile ( "lock addl $0, -32(%%rsp)" ::: "memory" )
+#define xen_rmb() xen_barrier()
+#define xen_wmb() xen_barrier()
+#elif defined(__arm__)
+#define xen_mb()   asm volatile ("dmb" : : : "memory")
+#define xen_rmb()  asm volatile ("dmb" : : : "memory")
+#define xen_wmb()  asm volatile ("dmb" : : : "memory")
+#elif defined(__aarch64__)
+#define xen_mb()   asm volatile ("dmb sy" : : : "memory")
+#define xen_rmb()  asm volatile ("dmb sy" : : : "memory")
+#define xen_wmb()  asm volatile ("dmb sy" : : : "memory")
+#else
+#error "Define barriers"
+#endif
+
+#endif /* XENBARRIER_H */
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 29617585c5..ea57e9dbb9 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -48,6 +48,7 @@
 #include <xen/platform.h>
 
 #include "xentoollog.h"
+#include "xen-barrier.h"
 
 #if defined(__i386__) || defined(__x86_64__)
 #include <xen/foreign/x86_32.h>
@@ -61,33 +62,6 @@
 
 #define INVALID_MFN  (~0UL)
 
-/*
- *  DEFINITIONS FOR CPU BARRIERS
- */
-
-#define xen_barrier() asm volatile ( "" : : : "memory")
-
-#if defined(__i386__)
-#define xen_mb()  asm volatile ( "lock addl $0, -4(%%esp)" ::: "memory" )
-#define xen_rmb() xen_barrier()
-#define xen_wmb() xen_barrier()
-#elif defined(__x86_64__)
-#define xen_mb()  asm volatile ( "lock addl $0, -32(%%rsp)" ::: "memory" )
-#define xen_rmb() xen_barrier()
-#define xen_wmb() xen_barrier()
-#elif defined(__arm__)
-#define xen_mb()   asm volatile ("dmb" : : : "memory")
-#define xen_rmb()  asm volatile ("dmb" : : : "memory")
-#define xen_wmb()  asm volatile ("dmb" : : : "memory")
-#elif defined(__aarch64__)
-#define xen_mb()   asm volatile ("dmb sy" : : : "memory")
-#define xen_rmb()  asm volatile ("dmb sy" : : : "memory")
-#define xen_wmb()  asm volatile ("dmb sy" : : : "memory")
-#else
-#error "Define barriers"
-#endif
-
-
 #define XENCTRL_HAS_XC_INTERFACE 1
 /* In Xen 4.0 and earlier, xc_interface_open and xc_evtchn_open would
  * both return ints being the file descriptor.  In 4.1 and later, they
diff --git a/tools/libs/ctrl/Makefile b/tools/libs/ctrl/Makefile
index 5fe0bfad0c..acce8639d3 100644
--- a/tools/libs/ctrl/Makefile
+++ b/tools/libs/ctrl/Makefile
@@ -3,7 +3,7 @@ include $(XEN_ROOT)/tools/Rules.mk
 
 include Makefile.common
 
-LIBHEADER := xenctrl.h xenctrl_compat.h
+LIBHEADER := xenctrl.h xenctrl_compat.h xen-barrier.h
 PKG_CONFIG_FILE := xencontrol.pc
 PKG_CONFIG_NAME := Xencontrol
 
-- 
2.43.0

[PATCH 6/6] 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>
---
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!
---
 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..c0264d9477 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_DYING))
 			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

Re: [PATCH 1/6] xen: add a domain unique id to each domain

[Alejandro Vallejo]

On Wed Oct 23, 2024 at 2:10 PM BST, Juergen Gross wrote:
> Xenstore is referencing domains by their domid, but reuse of a domid
> can lead to the situation that Xenstore can't tell whether a domain
> with that domid has been deleted and created again without Xenstore
> noticing the domain is a new one now.
>
> Add a global domain creation unique id which is updated when creating
> a new domain, and store that value in struct domain of the new domain.
> The global unique id is initialized with the system time and updates
> are done via the xorshift algorithm which is used for pseudo random
> number generation, too (see https://en.wikipedia.org/wiki/Xorshift).
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
> V1:
> - make unique_id local to function (Jan Beulich)
> - add lock (Julien Grall)
> - add comment (Julien Grall)
> ---
>  xen/common/domain.c     | 20 ++++++++++++++++++++
>  xen/include/xen/sched.h |  3 +++
>  2 files changed, 23 insertions(+)
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 92263a4fbd..3948640fb0 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -562,6 +562,25 @@ static void _domain_destroy(struct domain *d)
>      free_domain_struct(d);
>  }
>  
> +static uint64_t get_unique_id(void)
> +{
> +    static uint64_t unique_id;
> +    static DEFINE_SPINLOCK(lock);
> +    uint64_t x = unique_id ? : NOW();
> +
> +    spin_lock(&lock);
> +
> +    /* Pseudo-randomize id in order to avoid consumers relying on sequence. */
> +    x ^= x << 13;
> +    x ^= x >> 7;
> +    x ^= x << 17;
> +    unique_id = x;
> +
> +    spin_unlock(&lock);
> +
> +    return x;
> +}
> +
>  static int sanitise_domain_config(struct xen_domctl_createdomain *config)
>  {
>      bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
> @@ -654,6 +673,7 @@ struct domain *domain_create(domid_t domid,
>  
>      /* Sort out our idea of is_system_domain(). */
>      d->domain_id = domid;
> +    d->unique_id = get_unique_id();
>  
>      /* Holding CDF_* internal flags. */
>      d->cdf = flags;
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 90666576c2..1dd8a425f9 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -370,6 +370,9 @@ struct domain
>      domid_t          domain_id;
>  
>      unsigned int     max_vcpus;
> +
> +    uint64_t         unique_id;       /* Unique domain identifier */
> +

Why not xen_domain_handle_t handle, defined later on? That's meant to be a
UUID, so this feels like a duplicate field.

>      struct vcpu    **vcpu;
>  
>      shared_info_t   *shared_info;     /* shared data area */

Cheers,
Alejandro

Re: [PATCH 1/6] xen: add a domain unique id to each domain

[Juergen Gross]

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

On 23.10.24 16:08, Alejandro Vallejo wrote:
> On Wed Oct 23, 2024 at 2:10 PM BST, Juergen Gross wrote:
>> Xenstore is referencing domains by their domid, but reuse of a domid
>> can lead to the situation that Xenstore can't tell whether a domain
>> with that domid has been deleted and created again without Xenstore
>> noticing the domain is a new one now.
>>
>> Add a global domain creation unique id which is updated when creating
>> a new domain, and store that value in struct domain of the new domain.
>> The global unique id is initialized with the system time and updates
>> are done via the xorshift algorithm which is used for pseudo random
>> number generation, too (see https://en.wikipedia.org/wiki/Xorshift).
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> V1:
>> - make unique_id local to function (Jan Beulich)
>> - add lock (Julien Grall)
>> - add comment (Julien Grall)
>> ---
>>   xen/common/domain.c     | 20 ++++++++++++++++++++
>>   xen/include/xen/sched.h |  3 +++
>>   2 files changed, 23 insertions(+)
>>
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index 92263a4fbd..3948640fb0 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -562,6 +562,25 @@ static void _domain_destroy(struct domain *d)
>>       free_domain_struct(d);
>>   }
>>   
>> +static uint64_t get_unique_id(void)
>> +{
>> +    static uint64_t unique_id;
>> +    static DEFINE_SPINLOCK(lock);
>> +    uint64_t x = unique_id ? : NOW();
>> +
>> +    spin_lock(&lock);
>> +
>> +    /* Pseudo-randomize id in order to avoid consumers relying on sequence. */
>> +    x ^= x << 13;
>> +    x ^= x >> 7;
>> +    x ^= x << 17;
>> +    unique_id = x;
>> +
>> +    spin_unlock(&lock);
>> +
>> +    return x;
>> +}
>> +
>>   static int sanitise_domain_config(struct xen_domctl_createdomain *config)
>>   {
>>       bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
>> @@ -654,6 +673,7 @@ struct domain *domain_create(domid_t domid,
>>   
>>       /* Sort out our idea of is_system_domain(). */
>>       d->domain_id = domid;
>> +    d->unique_id = get_unique_id();
>>   
>>       /* Holding CDF_* internal flags. */
>>       d->cdf = flags;
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index 90666576c2..1dd8a425f9 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -370,6 +370,9 @@ struct domain
>>       domid_t          domain_id;
>>   
>>       unsigned int     max_vcpus;
>> +
>> +    uint64_t         unique_id;       /* Unique domain identifier */
>> +
> 
> Why not xen_domain_handle_t handle, defined later on? That's meant to be a
> UUID, so this feels like a duplicate field.

It is an input value for create domain. So there is absolutely no
guarantee that it is unique.

It can especially be specified in the xl config file, so you could have
a host running multiple guests all with the same UUID (even if this might
be rejected by libxl, destroying a guest and then recreating it with the
same UUID is possible, but Xenstore would need to see different unique Ids
for those 2 guests).


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 3/6] xen: add new domctl get_changed_domain

[Daniel P. Smith]

On 10/23/24 09:10, Juergen Gross wrote:
> 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 specific to the respective domctl op and it is an
> in/out parameter: On input the caller is specifying the desired version
> of the op, while on output the hypervisor will return the used version
> (this will be at max the caller supplied version, but might be lower in
> case the hypervisor doesn't support this version).
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V1:
> - use a domctl subop for the new interface (Jan Beulich)
> ---
>   tools/flask/policy/modules/dom0.te  |  2 +-
>   xen/common/domain.c                 | 51 +++++++++++++++++++++++++++++
>   xen/common/domctl.c                 | 19 ++++++++++-
>   xen/common/event_channel.c          |  9 ++++-
>   xen/include/public/domctl.h         | 33 +++++++++++++++++++
>   xen/include/xen/event.h             |  6 ++++
>   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, 143 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
> index 16b8c9646d..6043c01b12 100644
> --- a/tools/flask/policy/modules/dom0.te
> +++ b/tools/flask/policy/modules/dom0.te
> @@ -40,7 +40,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
> +	get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy get_domain_state

I don't think that is where you want it, as that restricts dom0 to only 
being able to make that call against dom0. The question I have is, are 
you looking for this permission to be explicitly assigned to dom0 or to 
the domain type that was allowed to create the domain. IMHO, I think you 
would want the latter, so not only should the permission go here, but 
also added to xen.if:create_domain_common.

Additionally, I would also recommend adding the following to xenstore.te:

allow xenstore_t domain_type:domain get_domain_state

>   allow dom0_t dom0_t:resource { add remove };
>   

...

> @@ -866,6 +873,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>                   __HYPERVISOR_domctl, "h", u_domctl);
>           break;
>   
> +    case XEN_DOMCTL_get_domain_state:
> +        ret = xsm_get_domain_state(XSM_HOOK, d);

XSM_HOOK will allow any domain to make this call on any domain. What I 
think you want here is XSM_XS_PRIV. That will allow either a domain 
flagged as the xenstore domain or dom0 to make the call.

> +        if ( ret )
> +            break;
> +
> +        copyback = 1;
> +        op->interface_version = XEN_DOMCTL_GETDOMSTATE_VERS_MAX;
> +        ret = get_domain_state(&op->u.get_domain_state, d);
> +        break;
> +
>       default:
>           ret = arch_do_domctl(op, d, u_domctl);
>           break;

...

> @@ -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_HOOK);

Per the above, this would need changed to 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)
>   {

...

> @@ -1853,6 +1854,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_DOMAIN, DOMAIN__GET_DOMAIN_STATE);

I believe you want SECCLASS_DOMAIN2 here.

> +}
> +
>   static const struct xsm_ops __initconst_cf_clobber flask_ops = {
>       .set_system_active = flask_set_system_active,
>       .security_domaininfo = flask_security_domaininfo,

v/r,
dps

Re: [PATCH 3/6] xen: add new domctl get_changed_domain

[Jürgen Groß]

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

On 23.10.24 17:55, Daniel P. Smith wrote:
> On 10/23/24 09:10, Juergen Gross wrote:
>> 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 specific to the respective domctl op and it is an
>> in/out parameter: On input the caller is specifying the desired version
>> of the op, while on output the hypervisor will return the used version
>> (this will be at max the caller supplied version, but might be lower in
>> case the hypervisor doesn't support this version).
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V1:
>> - use a domctl subop for the new interface (Jan Beulich)
>> ---
>>   tools/flask/policy/modules/dom0.te  |  2 +-
>>   xen/common/domain.c                 | 51 +++++++++++++++++++++++++++++
>>   xen/common/domctl.c                 | 19 ++++++++++-
>>   xen/common/event_channel.c          |  9 ++++-
>>   xen/include/public/domctl.h         | 33 +++++++++++++++++++
>>   xen/include/xen/event.h             |  6 ++++
>>   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, 143 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/flask/policy/modules/dom0.te 
>> b/tools/flask/policy/modules/dom0.te
>> index 16b8c9646d..6043c01b12 100644
>> --- a/tools/flask/policy/modules/dom0.te
>> +++ b/tools/flask/policy/modules/dom0.te
>> @@ -40,7 +40,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
>> +    get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy get_domain_state
> 
> I don't think that is where you want it, as that restricts dom0 to only being 
> able to make that call against dom0. The question I have is, are you looking for 
> this permission to be explicitly assigned to dom0 or to the domain type that was 
> allowed to create the domain. IMHO, I think you would want the latter, so not 
> only should the permission go here, but also added to xen.if:create_domain_common.
> 
> Additionally, I would also recommend adding the following to xenstore.te:
> 
> allow xenstore_t domain_type:domain get_domain_state

Okay, but shouldn't this be:

allow xenstore_t domain_type:domain2 get_domain_state;

> 
>>   allow dom0_t dom0_t:resource { add remove };
> 
> ...
> 
>> @@ -866,6 +873,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
>> u_domctl)
>>                   __HYPERVISOR_domctl, "h", u_domctl);
>>           break;
>> +    case XEN_DOMCTL_get_domain_state:
>> +        ret = xsm_get_domain_state(XSM_HOOK, d);
> 
> XSM_HOOK will allow any domain to make this call on any domain. What I think you 
> want here is XSM_XS_PRIV. That will allow either a domain flagged as the 
> xenstore domain or dom0 to make the call.

I thought so, too, but looking at the "getdomaininfo" example it seems
to be okay this way, too. Especially with the addition to xsm_domctl()
checking for XSM_XS_PRIV.

> 
>> +        if ( ret )
>> +            break;
>> +
>> +        copyback = 1;
>> +        op->interface_version = XEN_DOMCTL_GETDOMSTATE_VERS_MAX;
>> +        ret = get_domain_state(&op->u.get_domain_state, d);
>> +        break;
>> +
>>       default:
>>           ret = arch_do_domctl(op, d, u_domctl);
>>           break;
> 
> ...
> 
>> @@ -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_HOOK);
> 
> Per the above, this would need changed to 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)
>>   {
> 
> ...
> 
>> @@ -1853,6 +1854,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_DOMAIN, DOMAIN__GET_DOMAIN_STATE);
> 
> I believe you want SECCLASS_DOMAIN2 here.

Oh, indeed. And probably DOMAIN2__GET_DOMAIN_STATE


Thanks,

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 3/6] xen: add new domctl get_changed_domain

[Daniel P. Smith]

On 10/24/24 05:13, Jürgen Groß wrote:
> On 23.10.24 17:55, Daniel P. Smith wrote:
>> On 10/23/24 09:10, Juergen Gross wrote:
>>> 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 specific to the respective domctl op and it is an
>>> in/out parameter: On input the caller is specifying the desired version
>>> of the op, while on output the hypervisor will return the used version
>>> (this will be at max the caller supplied version, but might be lower in
>>> case the hypervisor doesn't support this version).
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> V1:
>>> - use a domctl subop for the new interface (Jan Beulich)
>>> ---
>>>   tools/flask/policy/modules/dom0.te  |  2 +-
>>>   xen/common/domain.c                 | 51 +++++++++++++++++++++++++++++
>>>   xen/common/domctl.c                 | 19 ++++++++++-
>>>   xen/common/event_channel.c          |  9 ++++-
>>>   xen/include/public/domctl.h         | 33 +++++++++++++++++++
>>>   xen/include/xen/event.h             |  6 ++++
>>>   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, 143 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/flask/policy/modules/dom0.te 
>>> b/tools/flask/policy/modules/dom0.te
>>> index 16b8c9646d..6043c01b12 100644
>>> --- a/tools/flask/policy/modules/dom0.te
>>> +++ b/tools/flask/policy/modules/dom0.te
>>> @@ -40,7 +40,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
>>> +    get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy get_domain_state
>>
>> I don't think that is where you want it, as that restricts dom0 to 
>> only being able to make that call against dom0. The question I have 
>> is, are you looking for this permission to be explicitly assigned to 
>> dom0 or to the domain type that was allowed to create the domain. 
>> IMHO, I think you would want the latter, so not only should the 
>> permission go here, but also added to xen.if:create_domain_common.
>>
>> Additionally, I would also recommend adding the following to xenstore.te:
>>
>> allow xenstore_t domain_type:domain get_domain_state
> 
> Okay, but shouldn't this be:
> 
> allow xenstore_t domain_type:domain2 get_domain_state;

Apologies, yes that was a typo on my part.

>>
>>>   allow dom0_t dom0_t:resource { add remove };
>>
>> ...
>>
>>> @@ -866,6 +873,16 @@ long 
>>> do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>                   __HYPERVISOR_domctl, "h", u_domctl);
>>>           break;
>>> +    case XEN_DOMCTL_get_domain_state:
>>> +        ret = xsm_get_domain_state(XSM_HOOK, d);
>>
>> XSM_HOOK will allow any domain to make this call on any domain. What I 
>> think you want here is XSM_XS_PRIV. That will allow either a domain 
>> flagged as the xenstore domain or dom0 to make the call.
> 
> I thought so, too, but looking at the "getdomaininfo" example it seems
> to be okay this way, too. Especially with the addition to xsm_domctl()
> checking for XSM_XS_PRIV.

I know this has been done in the past, but imho it is not a very good 
practice. Access checks really should always restrict equal or down. 
There should be a strong reason, which probably would deserves a code 
comment, to allow a check to relax up. Restricting equal should not 
reduce access, and if it does, then it means there is an unintended 
access path which is now exposed.

>>
>>> +        if ( ret )
>>> +            break;
>>> +
>>> +        copyback = 1;
>>> +        op->interface_version = XEN_DOMCTL_GETDOMSTATE_VERS_MAX;
>>> +        ret = get_domain_state(&op->u.get_domain_state, d);
>>> +        break;
>>> +
>>>       default:
>>>           ret = arch_do_domctl(op, d, u_domctl);
>>>           break;
>>
>> ...
>>
>>> @@ -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_HOOK);
>>
>> Per the above, this would need changed to 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)
>>>   {
>>
>> ...
>>
>>> @@ -1853,6 +1854,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_DOMAIN, 
>>> DOMAIN__GET_DOMAIN_STATE);
>>
>> I believe you want SECCLASS_DOMAIN2 here.
> 
> Oh, indeed. And probably DOMAIN2__GET_DOMAIN_STATE
> 
> 
> Thanks,

No problem.

v/r,
dps

Re: [PATCH 3/6] xen: add new domctl get_changed_domain

[Jürgen Groß]

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

On 24.10.24 15:35, Daniel P. Smith wrote:
> On 10/24/24 05:13, Jürgen Groß wrote:
>> On 23.10.24 17:55, Daniel P. Smith wrote:
>>> On 10/23/24 09:10, Juergen Gross wrote:
>>>> 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 specific to the respective domctl op and it is an
>>>> in/out parameter: On input the caller is specifying the desired version
>>>> of the op, while on output the hypervisor will return the used version
>>>> (this will be at max the caller supplied version, but might be lower in
>>>> case the hypervisor doesn't support this version).
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>> V1:
>>>> - use a domctl subop for the new interface (Jan Beulich)
>>>> ---
>>>>   tools/flask/policy/modules/dom0.te  |  2 +-
>>>>   xen/common/domain.c                 | 51 +++++++++++++++++++++++++++++
>>>>   xen/common/domctl.c                 | 19 ++++++++++-
>>>>   xen/common/event_channel.c          |  9 ++++-
>>>>   xen/include/public/domctl.h         | 33 +++++++++++++++++++
>>>>   xen/include/xen/event.h             |  6 ++++
>>>>   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, 143 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tools/flask/policy/modules/dom0.te 
>>>> b/tools/flask/policy/modules/dom0.te
>>>> index 16b8c9646d..6043c01b12 100644
>>>> --- a/tools/flask/policy/modules/dom0.te
>>>> +++ b/tools/flask/policy/modules/dom0.te
>>>> @@ -40,7 +40,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
>>>> +    get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy get_domain_state
>>>
>>> I don't think that is where you want it, as that restricts dom0 to only being 
>>> able to make that call against dom0. The question I have is, are you looking 
>>> for this permission to be explicitly assigned to dom0 or to the domain type 
>>> that was allowed to create the domain. IMHO, I think you would want the 
>>> latter, so not only should the permission go here, but also added to 
>>> xen.if:create_domain_common.
>>>
>>> Additionally, I would also recommend adding the following to xenstore.te:
>>>
>>> allow xenstore_t domain_type:domain get_domain_state
>>
>> Okay, but shouldn't this be:
>>
>> allow xenstore_t domain_type:domain2 get_domain_state;
> 
> Apologies, yes that was a typo on my part.
> 
>>>
>>>>   allow dom0_t dom0_t:resource { add remove };
>>>
>>> ...
>>>
>>>> @@ -866,6 +873,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
>>>> u_domctl)
>>>>                   __HYPERVISOR_domctl, "h", u_domctl);
>>>>           break;
>>>> +    case XEN_DOMCTL_get_domain_state:
>>>> +        ret = xsm_get_domain_state(XSM_HOOK, d);
>>>
>>> XSM_HOOK will allow any domain to make this call on any domain. What I think 
>>> you want here is XSM_XS_PRIV. That will allow either a domain flagged as the 
>>> xenstore domain or dom0 to make the call.
>>
>> I thought so, too, but looking at the "getdomaininfo" example it seems
>> to be okay this way, too. Especially with the addition to xsm_domctl()
>> checking for XSM_XS_PRIV.
> 
> I know this has been done in the past, but imho it is not a very good practice. 
> Access checks really should always restrict equal or down. There should be a 
> strong reason, which probably would deserves a code comment, to allow a check to 
> relax up. Restricting equal should not reduce access, and if it does, then it 
> means there is an unintended access path which is now exposed.

Sounds logical.

I'll check if it works the way you are suggesting. If so, I'll send a patch
fixing the getdomaininfo case.


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 3/6] xen: add new domctl get_changed_domain

[Jan Beulich]

On 23.10.2024 15:10, Juergen Gross wrote:
> 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 specific to the respective domctl op and it is an
> in/out parameter: On input the caller is specifying the desired version
> of the op, while on output the hypervisor will return the used version
> (this will be at max the caller supplied version, but might be lower in
> case the hypervisor doesn't support this version).
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V1:
> - use a domctl subop for the new interface (Jan Beulich)

Just as a preliminary reply: With Andrew's stabilization plans I'm not
sure this, in particular including ...

> @@ -1236,7 +1237,37 @@ struct xen_domctl_dt_overlay {
>  };
>  #endif
>  
> +/*
> + * 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).
> + *
> + * Supported interface versions: 0x00000000
> + */
> +#define XEN_DOMCTL_GETDOMSTATE_VERS_MAX    0
> +struct xen_domctl_get_domain_state {
> +    domid_t domid;
> +    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. */
> +    uint32_t pad1;           /* Returned as 0. */
> +    uint64_t unique_id;      /* Unique domain identifier. */
> +    uint64_t pad2[6];        /* Returned as 0. */
> +};
> +
>  struct xen_domctl {
> +/*
> + * Stable domctl ops:
> + * interface_version is per cmd, hypervisor can support multiple versions
> + * and will return used version (at max caller supplied value) in
> + * interface_version on return.
> + */

... this (ab)use of the interface version, is the way to go. I think
we want a brand new hypercall, with stability just like most other
hypercalls have it. Backwards-incompatible interface changes then
simply aren't allowed (as in: require a new sub-op instead).

Jan

Re: [PATCH 3/6] xen: add new domctl get_changed_domain

[Jürgen Groß]

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

On 28.10.24 11:50, Jan Beulich wrote:
> On 23.10.2024 15:10, Juergen Gross wrote:
>> 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 specific to the respective domctl op and it is an
>> in/out parameter: On input the caller is specifying the desired version
>> of the op, while on output the hypervisor will return the used version
>> (this will be at max the caller supplied version, but might be lower in
>> case the hypervisor doesn't support this version).
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V1:
>> - use a domctl subop for the new interface (Jan Beulich)
> 
> Just as a preliminary reply: With Andrew's stabilization plans I'm not
> sure this, in particular including ...
> 
>> @@ -1236,7 +1237,37 @@ struct xen_domctl_dt_overlay {
>>   };
>>   #endif
>>   
>> +/*
>> + * 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).
>> + *
>> + * Supported interface versions: 0x00000000
>> + */
>> +#define XEN_DOMCTL_GETDOMSTATE_VERS_MAX    0
>> +struct xen_domctl_get_domain_state {
>> +    domid_t domid;
>> +    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. */
>> +    uint32_t pad1;           /* Returned as 0. */
>> +    uint64_t unique_id;      /* Unique domain identifier. */
>> +    uint64_t pad2[6];        /* Returned as 0. */
>> +};
>> +
>>   struct xen_domctl {
>> +/*
>> + * Stable domctl ops:
>> + * interface_version is per cmd, hypervisor can support multiple versions
>> + * and will return used version (at max caller supplied value) in
>> + * interface_version on return.
>> + */
> 
> ... this (ab)use of the interface version, is the way to go. I think
> we want a brand new hypercall, with stability just like most other
> hypercalls have it. Backwards-incompatible interface changes then
> simply aren't allowed (as in: require a new sub-op instead).

Okay, so back to my initial RFC variant then. :-)

Andrew, can you confirm you'd prefer that route?


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 2/6] xen: add bitmap to indicate per-domain state changes

[Jan Beulich]

On 23.10.2024 15:10, Juergen Gross wrote:
> 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.

That's furthering the "there can be only one consumer" model that also
is used for VIRQ_DOM_EXC itself. I consider the existing model flawed
(nothing keeps a 2nd party with sufficient privilege from invoking
XEN_DOMCTL_set_virq_handler a 2nd time, taking away the notification
from whoever had first requested it), and hence I dislike this being
extended. Conceivably multiple parties may indeed be interested in
this kind of information. At which point resetting state when the vIRQ
is bound is questionable (or the data would need to become per-domain
rather than global, or even yet more fine-grained, albeit
->virq_to_evtchn[] is also per-domain, when considering global vIRQ-s).

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -138,6 +138,22 @@ bool __read_mostly vmtrace_available;
>  
>  bool __read_mostly vpmu_is_available;
>  
> +static DECLARE_BITMAP(dom_state_changed, DOMID_MASK + 1);

While it won't alter the size of the array, I think DOMID_FIRST_RESERVED
would be more logical to use here and ...

> +void domain_reset_states(void)
> +{
> +    struct domain *d;
> +
> +    bitmap_zero(dom_state_changed, DOMID_MASK + 1);

... here.

> +    rcu_read_lock(&domlist_read_lock);
> +
> +    for_each_domain ( d )
> +        set_bit(d->domain_id, dom_state_changed);

d is used only here, so could be pointer-to-const?

> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1296,6 +1296,8 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          rc = evtchn_bind_virq(&bind_virq, 0);
>          if ( !rc && __copy_to_guest(arg, &bind_virq, 1) )
>              rc = -EFAULT; /* Cleaning up here would be a mess! */
> +        if ( !rc && bind_virq.virq == VIRQ_DOM_EXC )
> +            domain_reset_states();

evtchn_bind_virq() isn't static, so callers beyond the present ones could
appear without noticing the need for this special casing. Is there a reason
the check can't move into the function? Doing the check in spite of the
copy-out failing is imo still reasonable behavior.

Jan

Re: [PATCH 3/6] xen: add new domctl get_changed_domain

[Jan Beulich]

On 23.10.2024 15:10, Juergen Gross wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -154,6 +154,57 @@ void domain_reset_states(void)
>      rcu_read_unlock(&domlist_read_lock);
>  }
>  
> +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_dead )
> +        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DYING;
> +    info->unique_id = d->unique_id;
> +}
> +
> +int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d)
> +{
> +    unsigned int dom;
> +
> +    memset(info, 0, sizeof(*info));

Would this better go into set_domain_state_info()? Ah, no, you ...

> +    if ( d )
> +    {
> +        set_domain_state_info(info, d);
> +
> +        return 0;
> +    }
> +
> +    while ( (dom = find_first_bit(dom_state_changed, DOMID_MASK + 1)) <
> +            DOMID_FIRST_RESERVED )
> +    {
> +        d = rcu_lock_domain_by_id(dom);

... acquiring the lock early and then ...

> +        if ( test_and_clear_bit(dom, dom_state_changed) )
> +        {
> +            info->domid = dom;
> +            if ( d )
> +            {
> +                set_domain_state_info(info, d);

... potentially bypassing the call (with just the domid set) requires it
that way.

As to the point in time when the lock is acquired: Why is that, seeing that
it complicates the unlocking a little, by requiring a 2nd unlock a few
lines down?

> +                rcu_unlock_domain(d);
> +            }
> +
> +            return 0;
> +        }
> +
> +        if ( d )
> +        {
> +            rcu_unlock_domain(d);
> +        }

Nit: No need for the braces.

> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -969,11 +969,18 @@ static struct domain *global_virq_handlers[NR_VIRQS] __read_mostly;
>  
>  static DEFINE_SPINLOCK(global_virq_handlers_lock);
>  
> +struct domain *get_global_virq_handler(uint32_t virq)
> +{
> +    ASSERT(virq_is_global(virq));
> +
> +    return global_virq_handlers[virq] ?: hardware_domain;
> +}
> +
>  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);
>  }

Is this a stale leftover from an earlier version? There's no other caller of
get_global_virq_handler() here, hence the change looks unmotivated here.

> @@ -1236,7 +1237,37 @@ struct xen_domctl_dt_overlay {
>  };
>  #endif
>  
> +/*
> + * 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).
> + *
> + * Supported interface versions: 0x00000000
> + */
> +#define XEN_DOMCTL_GETDOMSTATE_VERS_MAX    0
> +struct xen_domctl_get_domain_state {
> +    domid_t domid;

Despite the DOMID_INVALID special case the redundant domid here is odd.
You actually add the new sub-op to the special casing of op->domain at the
top of do_domctl(), so the sole difference to most other sub-ops would be
that this then is an IN/OUT (rather than the field here being an output
only when DOMID_INVALID was passed in via the common domid field).

> +    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. */
> +    uint32_t pad1;           /* Returned as 0. */
> +    uint64_t unique_id;      /* Unique domain identifier. */
> +    uint64_t pad2[6];        /* Returned as 0. */
> +};

What are the intentions with this padding array?

Jan

Re: [PATCH 1/6] xen: add a domain unique id to each domain

[Alejandro Vallejo]

On Wed Oct 23, 2024 at 3:27 PM BST, Juergen Gross wrote:
> On 23.10.24 16:08, Alejandro Vallejo wrote:
> > On Wed Oct 23, 2024 at 2:10 PM BST, Juergen Gross wrote:
> >> Xenstore is referencing domains by their domid, but reuse of a domid
> >> can lead to the situation that Xenstore can't tell whether a domain
> >> with that domid has been deleted and created again without Xenstore
> >> noticing the domain is a new one now.
> >>
> >> Add a global domain creation unique id which is updated when creating
> >> a new domain, and store that value in struct domain of the new domain.
> >> The global unique id is initialized with the system time and updates
> >> are done via the xorshift algorithm which is used for pseudo random
> >> number generation, too (see https://en.wikipedia.org/wiki/Xorshift).
> >>
> >> Signed-off-by: Juergen Gross <jgross@suse.com>
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> V1:
> >> - make unique_id local to function (Jan Beulich)
> >> - add lock (Julien Grall)
> >> - add comment (Julien Grall)
> >> ---
> >>   xen/common/domain.c     | 20 ++++++++++++++++++++
> >>   xen/include/xen/sched.h |  3 +++
> >>   2 files changed, 23 insertions(+)
> >>
> >> diff --git a/xen/common/domain.c b/xen/common/domain.c
> >> index 92263a4fbd..3948640fb0 100644
> >> --- a/xen/common/domain.c
> >> +++ b/xen/common/domain.c
> >> @@ -562,6 +562,25 @@ static void _domain_destroy(struct domain *d)
> >>       free_domain_struct(d);
> >>   }
> >>   
> >> +static uint64_t get_unique_id(void)
> >> +{
> >> +    static uint64_t unique_id;
> >> +    static DEFINE_SPINLOCK(lock);
> >> +    uint64_t x = unique_id ? : NOW();
> >> +
> >> +    spin_lock(&lock);
> >> +
> >> +    /* Pseudo-randomize id in order to avoid consumers relying on sequence. */
> >> +    x ^= x << 13;
> >> +    x ^= x >> 7;
> >> +    x ^= x << 17;
> >> +    unique_id = x;

How "unique" are they? With those shifts it's far less obvious to know how many
times we can call get_unique_id() and get an ID that hasn't been seen since
reset. With sequential numbers it's pretty obvious that it'd be a
non-overflowable monotonic counter. Here's it's far less clear, particularly
when it's randomly seeded.

I don't quite see why sequential IDs are problematic. What is this
(pseudo)randomization specifically trying to prevent? If it's just breaking the
assumption that numbers go in strict sequence you could just flip the high and
low nibbles (or any other deterministic swapping of counter nibbles)

Plus, with the counter going in sequence we could get rid of the lock because
an atomic fetch_add() would do.

> >> +
> >> +    spin_unlock(&lock);
> >> +
> >> +    return x;
> >> +}
> >> +
> >>   static int sanitise_domain_config(struct xen_domctl_createdomain *config)
> >>   {
> >>       bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
> >> @@ -654,6 +673,7 @@ struct domain *domain_create(domid_t domid,
> >>   
> >>       /* Sort out our idea of is_system_domain(). */
> >>       d->domain_id = domid;
> >> +    d->unique_id = get_unique_id();
> >>   
> >>       /* Holding CDF_* internal flags. */
> >>       d->cdf = flags;
> >> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> >> index 90666576c2..1dd8a425f9 100644
> >> --- a/xen/include/xen/sched.h
> >> +++ b/xen/include/xen/sched.h
> >> @@ -370,6 +370,9 @@ struct domain
> >>       domid_t          domain_id;
> >>   
> >>       unsigned int     max_vcpus;
> >> +
> >> +    uint64_t         unique_id;       /* Unique domain identifier */
> >> +
> > 
> > Why not xen_domain_handle_t handle, defined later on? That's meant to be a
> > UUID, so this feels like a duplicate field.
>
> It is an input value for create domain. So there is absolutely no
> guarantee that it is unique.
>
> It can especially be specified in the xl config file, so you could have
> a host running multiple guests all with the same UUID (even if this might
> be rejected by libxl, destroying a guest and then recreating it with the
> same UUID is possible, but Xenstore would need to see different unique Ids
> for those 2 guests).

Fair points. With that into account, I wouldn't mind seeing a wider comment on
top of unique_id explaining how these IDs are meant to be non-repeatable
between resets and meant to have the same lifetime as their corresponding
domain_id.

>
>
> Juergen

Cheers,
Alejandro

Re: [PATCH 2/6] xen: add bitmap to indicate per-domain state changes

[Jürgen Groß]

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

On 31.10.24 11:59, Jan Beulich wrote:
> On 23.10.2024 15:10, Juergen Gross wrote:
>> 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.
> 
> That's furthering the "there can be only one consumer" model that also
> is used for VIRQ_DOM_EXC itself. I consider the existing model flawed
> (nothing keeps a 2nd party with sufficient privilege from invoking
> XEN_DOMCTL_set_virq_handler a 2nd time, taking away the notification
> from whoever had first requested it), and hence I dislike this being
> extended. Conceivably multiple parties may indeed be interested in
> this kind of information. At which point resetting state when the vIRQ
> is bound is questionable (or the data would need to become per-domain
> rather than global, or even yet more fine-grained, albeit
> ->virq_to_evtchn[] is also per-domain, when considering global vIRQ-s).

The bitmap is directly tied to the VIRQ_DOM_EXC anyway, as it is that
event which makes the consumer look into the bitmap via the new hypercall.

If we decide to allow multiple consumers of VIRQ_DOM_EXC, we'll need to
have one bitmap per consumer of the event. This is not very hard to
modify.

If you'd like that better, I can dynamically allocate the bitmap on
binding VIRQ_DOM_EXC and freeing it again when unbinding is done.

> 
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -138,6 +138,22 @@ bool __read_mostly vmtrace_available;
>>   
>>   bool __read_mostly vpmu_is_available;
>>   
>> +static DECLARE_BITMAP(dom_state_changed, DOMID_MASK + 1);
> 
> While it won't alter the size of the array, I think DOMID_FIRST_RESERVED
> would be more logical to use here and ...
> 
>> +void domain_reset_states(void)
>> +{
>> +    struct domain *d;
>> +
>> +    bitmap_zero(dom_state_changed, DOMID_MASK + 1);
> 
> ... here.

Fine with me.

> 
>> +    rcu_read_lock(&domlist_read_lock);
>> +
>> +    for_each_domain ( d )
>> +        set_bit(d->domain_id, dom_state_changed);
> 
> d is used only here, so could be pointer-to-const?

Agreed.

> 
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -1296,6 +1296,8 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>           rc = evtchn_bind_virq(&bind_virq, 0);
>>           if ( !rc && __copy_to_guest(arg, &bind_virq, 1) )
>>               rc = -EFAULT; /* Cleaning up here would be a mess! */
>> +        if ( !rc && bind_virq.virq == VIRQ_DOM_EXC )
>> +            domain_reset_states();
> 
> evtchn_bind_virq() isn't static, so callers beyond the present ones could
> appear without noticing the need for this special casing. Is there a reason
> the check can't move into the function? Doing the check in spite of the
> copy-out failing is imo still reasonable behavior.

Moving the test into evtchn_bind_virq() should work. I'll change that.


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 3/6] xen: add new domctl get_changed_domain

[Jürgen Groß]

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

On 31.10.24 12:16, Jan Beulich wrote:
> On 23.10.2024 15:10, Juergen Gross wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -154,6 +154,57 @@ void domain_reset_states(void)
>>       rcu_read_unlock(&domlist_read_lock);
>>   }
>>   
>> +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_dead )
>> +        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DYING;
>> +    info->unique_id = d->unique_id;
>> +}
>> +
>> +int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d)
>> +{
>> +    unsigned int dom;
>> +
>> +    memset(info, 0, sizeof(*info));
> 
> Would this better go into set_domain_state_info()? Ah, no, you ...
> 
>> +    if ( d )
>> +    {
>> +        set_domain_state_info(info, d);
>> +
>> +        return 0;
>> +    }
>> +
>> +    while ( (dom = find_first_bit(dom_state_changed, DOMID_MASK + 1)) <
>> +            DOMID_FIRST_RESERVED )
>> +    {
>> +        d = rcu_lock_domain_by_id(dom);
> 
> ... acquiring the lock early and then ...
> 
>> +        if ( test_and_clear_bit(dom, dom_state_changed) )
>> +        {
>> +            info->domid = dom;
>> +            if ( d )
>> +            {
>> +                set_domain_state_info(info, d);
> 
> ... potentially bypassing the call (with just the domid set) requires it
> that way.
> 
> As to the point in time when the lock is acquired: Why is that, seeing that
> it complicates the unlocking a little, by requiring a 2nd unlock a few
> lines down?

I agree this can be simplified.

> 
>> +                rcu_unlock_domain(d);
>> +            }
>> +
>> +            return 0;
>> +        }
>> +
>> +        if ( d )
>> +        {
>> +            rcu_unlock_domain(d);
>> +        }
> 
> Nit: No need for the braces.
> 
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -969,11 +969,18 @@ static struct domain *global_virq_handlers[NR_VIRQS] __read_mostly;
>>   
>>   static DEFINE_SPINLOCK(global_virq_handlers_lock);
>>   
>> +struct domain *get_global_virq_handler(uint32_t virq)
>> +{
>> +    ASSERT(virq_is_global(virq));
>> +
>> +    return global_virq_handlers[virq] ?: hardware_domain;
>> +}
>> +
>>   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);
>>   }
> 
> Is this a stale leftover from an earlier version? There's no other caller of
> get_global_virq_handler() here, hence the change looks unmotivated here.

I think it is indeed stale now.

> 
>> @@ -1236,7 +1237,37 @@ struct xen_domctl_dt_overlay {
>>   };
>>   #endif
>>   
>> +/*
>> + * 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).
>> + *
>> + * Supported interface versions: 0x00000000
>> + */
>> +#define XEN_DOMCTL_GETDOMSTATE_VERS_MAX    0
>> +struct xen_domctl_get_domain_state {
>> +    domid_t domid;
> 
> Despite the DOMID_INVALID special case the redundant domid here is odd.
> You actually add the new sub-op to the special casing of op->domain at the
> top of do_domctl(), so the sole difference to most other sub-ops would be
> that this then is an IN/OUT (rather than the field here being an output
> only when DOMID_INVALID was passed in via the common domid field).

The main idea was to have all data returned by get_domain_state() in struct
xen_domctl_get_domain_state. I'm fine either way.

But I think this discussion is moot now, as it seems we are switching to a
new hypercall anyway, where we probably will have all data in per sub-op
structs.

> 
>> +    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. */
>> +    uint32_t pad1;           /* Returned as 0. */
>> +    uint64_t unique_id;      /* Unique domain identifier. */
>> +    uint64_t pad2[6];        /* Returned as 0. */
>> +};
> 
> What are the intentions with this padding array?

The idea was to allow to return additional domain data in future without having
to extend the struct.


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 1/6] xen: add a domain unique id to each domain

[Jürgen Groß]

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

On 31.10.24 12:58, Alejandro Vallejo wrote:
> On Wed Oct 23, 2024 at 3:27 PM BST, Juergen Gross wrote:
>> On 23.10.24 16:08, Alejandro Vallejo wrote:
>>> On Wed Oct 23, 2024 at 2:10 PM BST, Juergen Gross wrote:
>>>> Xenstore is referencing domains by their domid, but reuse of a domid
>>>> can lead to the situation that Xenstore can't tell whether a domain
>>>> with that domid has been deleted and created again without Xenstore
>>>> noticing the domain is a new one now.
>>>>
>>>> Add a global domain creation unique id which is updated when creating
>>>> a new domain, and store that value in struct domain of the new domain.
>>>> The global unique id is initialized with the system time and updates
>>>> are done via the xorshift algorithm which is used for pseudo random
>>>> number generation, too (see https://en.wikipedia.org/wiki/Xorshift).
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> V1:
>>>> - make unique_id local to function (Jan Beulich)
>>>> - add lock (Julien Grall)
>>>> - add comment (Julien Grall)
>>>> ---
>>>>    xen/common/domain.c     | 20 ++++++++++++++++++++
>>>>    xen/include/xen/sched.h |  3 +++
>>>>    2 files changed, 23 insertions(+)
>>>>
>>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>>> index 92263a4fbd..3948640fb0 100644
>>>> --- a/xen/common/domain.c
>>>> +++ b/xen/common/domain.c
>>>> @@ -562,6 +562,25 @@ static void _domain_destroy(struct domain *d)
>>>>        free_domain_struct(d);
>>>>    }
>>>>    
>>>> +static uint64_t get_unique_id(void)
>>>> +{
>>>> +    static uint64_t unique_id;
>>>> +    static DEFINE_SPINLOCK(lock);
>>>> +    uint64_t x = unique_id ? : NOW();
>>>> +
>>>> +    spin_lock(&lock);
>>>> +
>>>> +    /* Pseudo-randomize id in order to avoid consumers relying on sequence. */
>>>> +    x ^= x << 13;
>>>> +    x ^= x >> 7;
>>>> +    x ^= x << 17;
>>>> +    unique_id = x;
> 
> How "unique" are they? With those shifts it's far less obvious to know how many
> times we can call get_unique_id() and get an ID that hasn't been seen since
> reset. With sequential numbers it's pretty obvious that it'd be a
> non-overflowable monotonic counter. Here's it's far less clear, particularly
> when it's randomly seeded.

If you'd look into the Wikipedia article mentioned in the commit message
you'd know that the period is 2^64 - 1.

> I don't quite see why sequential IDs are problematic. What is this
> (pseudo)randomization specifically trying to prevent? If it's just breaking the
> assumption that numbers go in strict sequence you could just flip the high and
> low nibbles (or any other deterministic swapping of counter nibbles)

That was a request from the RFC series of this patch.

> Plus, with the counter going in sequence we could get rid of the lock because
> an atomic fetch_add() would do.

Its not as if this would be a hot path. So the lock is no real issue IMO.

> 
>>>> +
>>>> +    spin_unlock(&lock);
>>>> +
>>>> +    return x;
>>>> +}
>>>> +
>>>>    static int sanitise_domain_config(struct xen_domctl_createdomain *config)
>>>>    {
>>>>        bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
>>>> @@ -654,6 +673,7 @@ struct domain *domain_create(domid_t domid,
>>>>    
>>>>        /* Sort out our idea of is_system_domain(). */
>>>>        d->domain_id = domid;
>>>> +    d->unique_id = get_unique_id();
>>>>    
>>>>        /* Holding CDF_* internal flags. */
>>>>        d->cdf = flags;
>>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>>> index 90666576c2..1dd8a425f9 100644
>>>> --- a/xen/include/xen/sched.h
>>>> +++ b/xen/include/xen/sched.h
>>>> @@ -370,6 +370,9 @@ struct domain
>>>>        domid_t          domain_id;
>>>>    
>>>>        unsigned int     max_vcpus;
>>>> +
>>>> +    uint64_t         unique_id;       /* Unique domain identifier */
>>>> +
>>>
>>> Why not xen_domain_handle_t handle, defined later on? That's meant to be a
>>> UUID, so this feels like a duplicate field.
>>
>> It is an input value for create domain. So there is absolutely no
>> guarantee that it is unique.
>>
>> It can especially be specified in the xl config file, so you could have
>> a host running multiple guests all with the same UUID (even if this might
>> be rejected by libxl, destroying a guest and then recreating it with the
>> same UUID is possible, but Xenstore would need to see different unique Ids
>> for those 2 guests).
> 
> Fair points. With that into account, I wouldn't mind seeing a wider comment on
> top of unique_id explaining how these IDs are meant to be non-repeatable
> between resets and meant to have the same lifetime as their corresponding
> domain_id.

Okay, I can add such a comment.


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 1/6] xen: add a domain unique id to each domain

[Alejandro Vallejo]

On Fri Nov 1, 2024 at 7:06 AM GMT, Jürgen Groß wrote:
> On 31.10.24 12:58, Alejandro Vallejo wrote:
> > On Wed Oct 23, 2024 at 3:27 PM BST, Juergen Gross wrote:
> >> On 23.10.24 16:08, Alejandro Vallejo wrote:
> >>> On Wed Oct 23, 2024 at 2:10 PM BST, Juergen Gross wrote:
> >>>> Xenstore is referencing domains by their domid, but reuse of a domid
> >>>> can lead to the situation that Xenstore can't tell whether a domain
> >>>> with that domid has been deleted and created again without Xenstore
> >>>> noticing the domain is a new one now.
> >>>>
> >>>> Add a global domain creation unique id which is updated when creating
> >>>> a new domain, and store that value in struct domain of the new domain.
> >>>> The global unique id is initialized with the system time and updates
> >>>> are done via the xorshift algorithm which is used for pseudo random
> >>>> number generation, too (see https://en.wikipedia.org/wiki/Xorshift).
> >>>>
> >>>> Signed-off-by: Juergen Gross <jgross@suse.com>
> >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >>>> ---
> >>>> V1:
> >>>> - make unique_id local to function (Jan Beulich)
> >>>> - add lock (Julien Grall)
> >>>> - add comment (Julien Grall)
> >>>> ---
> >>>>    xen/common/domain.c     | 20 ++++++++++++++++++++
> >>>>    xen/include/xen/sched.h |  3 +++
> >>>>    2 files changed, 23 insertions(+)
> >>>>
> >>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
> >>>> index 92263a4fbd..3948640fb0 100644
> >>>> --- a/xen/common/domain.c
> >>>> +++ b/xen/common/domain.c
> >>>> @@ -562,6 +562,25 @@ static void _domain_destroy(struct domain *d)
> >>>>        free_domain_struct(d);
> >>>>    }
> >>>>    
> >>>> +static uint64_t get_unique_id(void)
> >>>> +{
> >>>> +    static uint64_t unique_id;
> >>>> +    static DEFINE_SPINLOCK(lock);
> >>>> +    uint64_t x = unique_id ? : NOW();
> >>>> +
> >>>> +    spin_lock(&lock);
> >>>> +
> >>>> +    /* Pseudo-randomize id in order to avoid consumers relying on sequence. */
> >>>> +    x ^= x << 13;
> >>>> +    x ^= x >> 7;
> >>>> +    x ^= x << 17;
> >>>> +    unique_id = x;
> > 
> > How "unique" are they? With those shifts it's far less obvious to know how many
> > times we can call get_unique_id() and get an ID that hasn't been seen since
> > reset. With sequential numbers it's pretty obvious that it'd be a
> > non-overflowable monotonic counter. Here's it's far less clear, particularly
> > when it's randomly seeded.
>
> If you'd look into the Wikipedia article mentioned in the commit message
> you'd know that the period is 2^64 - 1.
>

Bah. I did, but skimmed too fast looking for keywords. Thanks for bearing with
me :). Ok, with that I'm perfectly happy.

  Reviewed-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

> > I don't quite see why sequential IDs are problematic. What is this
> > (pseudo)randomization specifically trying to prevent? If it's just breaking the
> > assumption that numbers go in strict sequence you could just flip the high and
> > low nibbles (or any other deterministic swapping of counter nibbles)
>
> That was a request from the RFC series of this patch.
>
> > Plus, with the counter going in sequence we could get rid of the lock because
> > an atomic fetch_add() would do.
>
> Its not as if this would be a hot path. So the lock is no real issue IMO.
>
> > 
> >>>> +
> >>>> +    spin_unlock(&lock);
> >>>> +
> >>>> +    return x;
> >>>> +}
> >>>> +
> >>>>    static int sanitise_domain_config(struct xen_domctl_createdomain *config)
> >>>>    {
> >>>>        bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
> >>>> @@ -654,6 +673,7 @@ struct domain *domain_create(domid_t domid,
> >>>>    
> >>>>        /* Sort out our idea of is_system_domain(). */
> >>>>        d->domain_id = domid;
> >>>> +    d->unique_id = get_unique_id();
> >>>>    
> >>>>        /* Holding CDF_* internal flags. */
> >>>>        d->cdf = flags;
> >>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> >>>> index 90666576c2..1dd8a425f9 100644
> >>>> --- a/xen/include/xen/sched.h
> >>>> +++ b/xen/include/xen/sched.h
> >>>> @@ -370,6 +370,9 @@ struct domain
> >>>>        domid_t          domain_id;
> >>>>    
> >>>>        unsigned int     max_vcpus;
> >>>> +
> >>>> +    uint64_t         unique_id;       /* Unique domain identifier */
> >>>> +
> >>>
> >>> Why not xen_domain_handle_t handle, defined later on? That's meant to be a
> >>> UUID, so this feels like a duplicate field.
> >>
> >> It is an input value for create domain. So there is absolutely no
> >> guarantee that it is unique.
> >>
> >> It can especially be specified in the xl config file, so you could have
> >> a host running multiple guests all with the same UUID (even if this might
> >> be rejected by libxl, destroying a guest and then recreating it with the
> >> same UUID is possible, but Xenstore would need to see different unique Ids
> >> for those 2 guests).
> > 
> > Fair points. With that into account, I wouldn't mind seeing a wider comment on
> > top of unique_id explaining how these IDs are meant to be non-repeatable
> > between resets and meant to have the same lifetime as their corresponding
> > domain_id.
>
> Okay, I can add such a comment.
>
>
> Juergen

Cheers,
Alejandro

Re: [PATCH 2/6] xen: add bitmap to indicate per-domain state changes

[Jan Beulich]

On 01.11.2024 07:48, Jürgen Groß wrote:
> On 31.10.24 11:59, Jan Beulich wrote:
>> On 23.10.2024 15:10, Juergen Gross wrote:
>>> 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.
>>
>> That's furthering the "there can be only one consumer" model that also
>> is used for VIRQ_DOM_EXC itself. I consider the existing model flawed
>> (nothing keeps a 2nd party with sufficient privilege from invoking
>> XEN_DOMCTL_set_virq_handler a 2nd time, taking away the notification
>> from whoever had first requested it), and hence I dislike this being
>> extended. Conceivably multiple parties may indeed be interested in
>> this kind of information. At which point resetting state when the vIRQ
>> is bound is questionable (or the data would need to become per-domain
>> rather than global, or even yet more fine-grained, albeit
>> ->virq_to_evtchn[] is also per-domain, when considering global vIRQ-s).
> 
> The bitmap is directly tied to the VIRQ_DOM_EXC anyway, as it is that
> event which makes the consumer look into the bitmap via the new hypercall.
> 
> If we decide to allow multiple consumers of VIRQ_DOM_EXC, we'll need to
> have one bitmap per consumer of the event. This is not very hard to
> modify.
> 
> If you'd like that better, I can dynamically allocate the bitmap on
> binding VIRQ_DOM_EXC and freeing it again when unbinding is done.

I'd prefer that indeed, yet I'm also curious what other maintainers think.

Jan

Re: [PATCH 1/6] xen: add a domain unique id to each domain

[Julien Grall]

Hi,

On 23/10/2024 14:10, Juergen Gross wrote:
> Xenstore is referencing domains by their domid, but reuse of a domid
> can lead to the situation that Xenstore can't tell whether a domain
> with that domid has been deleted and created again without Xenstore
> noticing the domain is a new one now.
> 
> Add a global domain creation unique id which is updated when creating
> a new domain, and store that value in struct domain of the new domain.
> The global unique id is initialized with the system time and updates
> are done via the xorshift algorithm which is used for pseudo random
> number generation, too (see https://en.wikipedia.org/wiki/Xorshift).
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
> V1:
> - make unique_id local to function (Jan Beulich)
> - add lock (Julien Grall)
> - add comment (Julien Grall)
> ---
>   xen/common/domain.c     | 20 ++++++++++++++++++++
>   xen/include/xen/sched.h |  3 +++
>   2 files changed, 23 insertions(+)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 92263a4fbd..3948640fb0 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -562,6 +562,25 @@ static void _domain_destroy(struct domain *d)
>       free_domain_struct(d);
>   }
>   
> +static uint64_t get_unique_id(void)
> +{
> +    static uint64_t unique_id;
> +    static DEFINE_SPINLOCK(lock);
> +    uint64_t x = unique_id ? : NOW();

I think unique_id needs to be read within the critical section. 
Otherwise, get_unique_id() could return the same ID twice.

> +
> +    spin_lock(&lock);
> +
> +    /* Pseudo-randomize id in order to avoid consumers relying on sequence. */
> +    x ^= x << 13;
> +    x ^= x >> 7;
> +    x ^= x << 17;
> +    unique_id = x;
> +
> +    spin_unlock(&lock);
> +
> +    return x;
> +}
> +
>   static int sanitise_domain_config(struct xen_domctl_createdomain *config)
>   {
>       bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
> @@ -654,6 +673,7 @@ struct domain *domain_create(domid_t domid,
>   
>       /* Sort out our idea of is_system_domain(). */
>       d->domain_id = domid;
> +    d->unique_id = get_unique_id();
>   
>       /* Holding CDF_* internal flags. */
>       d->cdf = flags;
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 90666576c2..1dd8a425f9 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -370,6 +370,9 @@ struct domain
>       domid_t          domain_id;
>   
>       unsigned int     max_vcpus;
> +
> +    uint64_t         unique_id;       /* Unique domain identifier */
> +
>       struct vcpu    **vcpu;
>   
>       shared_info_t   *shared_info;     /* shared data area */

Cheers,

-- 
Julien Grall

Re: [PATCH 2/6] xen: add bitmap to indicate per-domain state changes

[Julien Grall]

Hi Jan & Juergen,

On 04/11/2024 09:35, Jan Beulich wrote:
> On 01.11.2024 07:48, Jürgen Groß wrote:
>> On 31.10.24 11:59, Jan Beulich wrote:
>>> On 23.10.2024 15:10, Juergen Gross wrote:
>>>> 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.
>>>
>>> That's furthering the "there can be only one consumer" model that also
>>> is used for VIRQ_DOM_EXC itself. I consider the existing model flawed
>>> (nothing keeps a 2nd party with sufficient privilege from invoking
>>> XEN_DOMCTL_set_virq_handler a 2nd time, taking away the notification
>>> from whoever had first requested it), and hence I dislike this being
>>> extended. Conceivably multiple parties may indeed be interested in
>>> this kind of information. At which point resetting state when the vIRQ
>>> is bound is questionable (or the data would need to become per-domain
>>> rather than global, or even yet more fine-grained, albeit
>>> ->virq_to_evtchn[] is also per-domain, when considering global vIRQ-s).
>>
>> The bitmap is directly tied to the VIRQ_DOM_EXC anyway, as it is that
>> event which makes the consumer look into the bitmap via the new hypercall.
>>
>> If we decide to allow multiple consumers of VIRQ_DOM_EXC, we'll need to
>> have one bitmap per consumer of the event. This is not very hard to
>> modify.

While in principle I agree that having multiple consumers of 
VIRQ_DOM_EXC would be great. I have some scalability concern because now 
we would end up to have to update N bitmap every time. So we would need 
to put a limit to N. I don't think there is a good limit...

So overall, I am not entirely convinced it is worth the trouble.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/6] xen: add a domain unique id to each domain

[Jürgen Groß]

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

On 16.11.24 11:46, Julien Grall wrote:
> Hi,
> 
> On 23/10/2024 14:10, Juergen Gross wrote:
>> Xenstore is referencing domains by their domid, but reuse of a domid
>> can lead to the situation that Xenstore can't tell whether a domain
>> with that domid has been deleted and created again without Xenstore
>> noticing the domain is a new one now.
>>
>> Add a global domain creation unique id which is updated when creating
>> a new domain, and store that value in struct domain of the new domain.
>> The global unique id is initialized with the system time and updates
>> are done via the xorshift algorithm which is used for pseudo random
>> number generation, too (see https://en.wikipedia.org/wiki/Xorshift).
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> V1:
>> - make unique_id local to function (Jan Beulich)
>> - add lock (Julien Grall)
>> - add comment (Julien Grall)
>> ---
>>   xen/common/domain.c     | 20 ++++++++++++++++++++
>>   xen/include/xen/sched.h |  3 +++
>>   2 files changed, 23 insertions(+)
>>
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index 92263a4fbd..3948640fb0 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -562,6 +562,25 @@ static void _domain_destroy(struct domain *d)
>>       free_domain_struct(d);
>>   }
>> +static uint64_t get_unique_id(void)
>> +{
>> +    static uint64_t unique_id;
>> +    static DEFINE_SPINLOCK(lock);
>> +    uint64_t x = unique_id ? : NOW();
> 
> I think unique_id needs to be read within the critical section. Otherwise, 
> get_unique_id() could return the same ID twice.

Yes, you are right.


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 2/6] xen: add bitmap to indicate per-domain state changes

[Jürgen Groß]

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

On 16.11.24 12:01, Julien Grall wrote:
> Hi Jan & Juergen,
> 
> On 04/11/2024 09:35, Jan Beulich wrote:
>> On 01.11.2024 07:48, Jürgen Groß wrote:
>>> On 31.10.24 11:59, Jan Beulich wrote:
>>>> On 23.10.2024 15:10, Juergen Gross wrote:
>>>>> 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.
>>>>
>>>> That's furthering the "there can be only one consumer" model that also
>>>> is used for VIRQ_DOM_EXC itself. I consider the existing model flawed
>>>> (nothing keeps a 2nd party with sufficient privilege from invoking
>>>> XEN_DOMCTL_set_virq_handler a 2nd time, taking away the notification
>>>> from whoever had first requested it), and hence I dislike this being
>>>> extended. Conceivably multiple parties may indeed be interested in
>>>> this kind of information. At which point resetting state when the vIRQ
>>>> is bound is questionable (or the data would need to become per-domain
>>>> rather than global, or even yet more fine-grained, albeit
>>>> ->virq_to_evtchn[] is also per-domain, when considering global vIRQ-s).
>>>
>>> The bitmap is directly tied to the VIRQ_DOM_EXC anyway, as it is that
>>> event which makes the consumer look into the bitmap via the new hypercall.
>>>
>>> If we decide to allow multiple consumers of VIRQ_DOM_EXC, we'll need to
>>> have one bitmap per consumer of the event. This is not very hard to
>>> modify.
> 
> While in principle I agree that having multiple consumers of VIRQ_DOM_EXC would 
> be great. I have some scalability concern because now we would end up to have to 
> update N bitmap every time. So we would need to put a limit to N. I don't think 
> there is a good limit...

The same applies regarding sending an event. I don't think the additional
setting of a bit is adding a relevant amount of processing time.

I agree that a limit is hard to find, but it could be rather high.

> So overall, I am not entirely convinced it is worth the trouble.

The only real reason I could see would be a setup without Xenstore. Reason
is that with Xenstore all interested parties could register a watch event
instead of directly consuming the VIRQ_DOM_EXC event.

We haven't needed multiple VIRQ_DOM_EXC consumers up to now, so I don't
think we should over-engineer the interface. At least there is a theoretical
solution for multiple consumers, so my patch series wouldn't introduce a
no-go for multiple consumers.


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 4/6] tools/libs: add a new libxenmanage library

[Anthony PERARD]

Hi Juergen,

On Wed, Oct 23, 2024 at 03:10:03PM +0200, Juergen Gross wrote:
> In order to have a stable interface in user land for using stable
> domctl and possibly later sysctl interfaces, add a new library
> libxenmanage.

What this new library could do? What sort of operation could be added in
the future? Domain creation? I'm trying to get convince that "manage" is
the right name for it.

To me, "manage" could be something higher level to take care of a domain
from it's creation to its demise.

So for this lib have get_domain_info() to query about a single domain,
and get_changed_domain() which seems to be a state synchronisation
operation. (For that second function, it resemble an operation of the
Matrix API calling "https://.../sync" which return all the new event
since the last time it was called. But back to the new function name, a
get* function which returns a different value every time you call it
might not actually be the right name for it, maybe other functions that
do something similar, or at least tell when there's a new event, would
be poll() and select(), so maybe poll_changed_domain() would be slightly
better at describing the kind of function that it is?)

So, those two functions only query about the states of domains, without
making any modification is seems, so is "manage" still the right name?
At least, it both function doesn't seems to fit in existing stable
libraries so having a new one seems the right call. So the name
depends of what other operation could be added to the library, as such,
a description of the library would be nice, but at least thanks for
documenting every functions!

> diff --git a/tools/include/xenmanage.h b/tools/include/xenmanage.h
> new file mode 100644
> index 0000000000..2e6c3dedaa
> --- /dev/null
> +++ b/tools/include/xenmanage.h
> @@ -0,0 +1,98 @@
> +/*
> + * 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/>.

Shall we use SPDX tags instead of this boilerplate?

> + */
> +#ifndef XENMANAGE_H
> +#define XENMANAGE_H
> +
> +#include <stdint.h>
> +
> +/* Callers who don't care don't need to #include <xentoollog.h> */
> +struct xentoollog_logger;

More like, callers who care will need to include xentoollog.h. Here, we
just avoid the need to include xentoollog.h in xenmanage.h by declaring
the minimum.

If every time I wanted to include an header, I needed to figure which
header needed to be include before, that would be very annoying. Often,
headers include the needed headers.

If you want to have a comment, how about "Avoid the need to include
<xentoollog.h>", that way the comment tell why "struct
xentoollog_logger" is here, where it came from, and is more explicit.


> diff --git a/tools/libs/manage/core.c b/tools/libs/manage/core.c
> new file mode 100644
> index 0000000000..0c9199f829
> --- /dev/null
> +++ b/tools/libs/manage/core.c
> @@ -0,0 +1,170 @@
...
> +
> +#define __XEN_TOOLS__ 1

This define might be better in the Makefile(.common), or even in libs.mk? So far,
only libxenhypfs does define this in source code, all the other defines
are in CFLAGS or there because xenctrl.h is included.


> +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;
> +    struct xen_domctl_get_domain_state *st;
> +    int saved_errno;
> +    int ret;
> +
...
> +
> +    ret = xencall1(hdl->xcall, __HYPERVISOR_domctl, (unsigned long)buf);
> +    saved_errno = errno;
> +    if ( !ret )
> +    {
> +        st = &buf->u.get_domain_state;

You could define *st here.
struct xen_domctl_get_domain_state *st = &...;

Or even with ".. *const st" but maybe that's not common enough in C
code.

Cheers,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH 4/6] tools/libs: add a new libxenmanage library

[Jürgen Groß]

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

On 22.11.24 14:55, Anthony PERARD wrote:
> Hi Juergen,
> 
> On Wed, Oct 23, 2024 at 03:10:03PM +0200, Juergen Gross wrote:
>> In order to have a stable interface in user land for using stable
>> domctl and possibly later sysctl interfaces, add a new library
>> libxenmanage.
> 
> What this new library could do? What sort of operation could be added in
> the future? Domain creation? I'm trying to get convince that "manage" is
> the right name for it.

It is thought to encapsulate all (future) stable hypercalls replacing
current sysctl and domctl operations.

> To me, "manage" could be something higher level to take care of a domain
> from it's creation to its demise.

Yes, and to manage the host.

> So for this lib have get_domain_info() to query about a single domain,
> and get_changed_domain() which seems to be a state synchronisation
> operation. (For that second function, it resemble an operation of the
> Matrix API calling "https://.../sync" which return all the new event
> since the last time it was called. But back to the new function name, a
> get* function which returns a different value every time you call it
> might not actually be the right name for it, maybe other functions that
> do something similar, or at least tell when there's a new event, would
> be poll() and select(), so maybe poll_changed_domain() would be slightly
> better at describing the kind of function that it is?)

Fine with me. I'm always in favor of descriptive names.

> So, those two functions only query about the states of domains, without
> making any modification is seems, so is "manage" still the right name?
> At least, it both function doesn't seems to fit in existing stable
> libraries so having a new one seems the right call. So the name
> depends of what other operation could be added to the library, as such,
> a description of the library would be nice, but at least thanks for
> documenting every functions!

I can add more comments, of course. This was just a first iteration to
get some feedback whether the general approach is okay.

> 
>> diff --git a/tools/include/xenmanage.h b/tools/include/xenmanage.h
>> new file mode 100644
>> index 0000000000..2e6c3dedaa
>> --- /dev/null
>> +++ b/tools/include/xenmanage.h
>> @@ -0,0 +1,98 @@
>> +/*
>> + * 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/>.
> 
> Shall we use SPDX tags instead of this boilerplate?

My thinking was to avoid that for "official" header files, as those might
be copied verbatim to other projects, which might not use SPDX. So having
the license text verbatim avoids any problems in this regard.

> 
>> + */
>> +#ifndef XENMANAGE_H
>> +#define XENMANAGE_H
>> +
>> +#include <stdint.h>
>> +
>> +/* Callers who don't care don't need to #include <xentoollog.h> */
>> +struct xentoollog_logger;
> 
> More like, callers who care will need to include xentoollog.h. Here, we
> just avoid the need to include xentoollog.h in xenmanage.h by declaring
> the minimum.
> 
> If every time I wanted to include an header, I needed to figure which
> header needed to be include before, that would be very annoying. Often,
> headers include the needed headers.
> 
> If you want to have a comment, how about "Avoid the need to include
> <xentoollog.h>", that way the comment tell why "struct
> xentoollog_logger" is here, where it came from, and is more explicit.

Okay.

>> diff --git a/tools/libs/manage/core.c b/tools/libs/manage/core.c
>> new file mode 100644
>> index 0000000000..0c9199f829
>> --- /dev/null
>> +++ b/tools/libs/manage/core.c
>> @@ -0,0 +1,170 @@
> ...
>> +
>> +#define __XEN_TOOLS__ 1
> 
> This define might be better in the Makefile(.common), or even in libs.mk? So far,
> only libxenhypfs does define this in source code, all the other defines
> are in CFLAGS or there because xenctrl.h is included.

Yes, that's better.

>> +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;
>> +    struct xen_domctl_get_domain_state *st;
>> +    int saved_errno;
>> +    int ret;
>> +
> ...
>> +
>> +    ret = xencall1(hdl->xcall, __HYPERVISOR_domctl, (unsigned long)buf);
>> +    saved_errno = errno;
>> +    if ( !ret )
>> +    {
>> +        st = &buf->u.get_domain_state;
> 
> You could define *st here.
> struct xen_domctl_get_domain_state *st = &...;
> 
> Or even with ".. *const st" but maybe that's not common enough in C
> code.

Okay, I'll move the definition down.


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 4/6] tools/libs: add a new libxenmanage library

[Anthony PERARD]

On Fri, Nov 22, 2024 at 04:12:25PM +0100, Jürgen Groß wrote:
> On 22.11.24 14:55, Anthony PERARD wrote:
> > On Wed, Oct 23, 2024 at 03:10:03PM +0200, Juergen Gross wrote:
> > > diff --git a/tools/include/xenmanage.h b/tools/include/xenmanage.h
> > > new file mode 100644
> > > index 0000000000..2e6c3dedaa
> > > --- /dev/null
> > > +++ b/tools/include/xenmanage.h
> > > @@ -0,0 +1,98 @@
> > > +/*
> > > + * 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/>.
> >
> > Shall we use SPDX tags instead of this boilerplate?
>
> My thinking was to avoid that for "official" header files, as those might
> be copied verbatim to other projects, which might not use SPDX. So having
> the license text verbatim avoids any problems in this regard.

Well, this header in particular would be fairly useless, I believe, if it
was copied into an other project, it described a library so need to be
distributed along side the library. Second, this isn't the text of the
license but something describing which license is used and where to
find the text for it. An SPDX tag does nearly the same thing, but can
actually be parse by a computer.

Official headers that would be useful to be copied into other project
already expose the SPDX tags, many/all the headers in
xen/include/public, as well as headers created by `mkheader.py` in this
directory (tools/include).

I've taken a look into my directory "/usr/include", and they are plenty
of headers having the SPDX tag.

So overall, I think we are fine to use SPDX tags here as well. ;-)

Cheers,

--

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH 5/6] tools: add a dedicated header file for barrier definitions

[Anthony PERARD]

On Wed, Oct 23, 2024 at 03:10:04PM +0200, Juergen Gross wrote:
> diff --git a/tools/include/xen-barrier.h b/tools/include/xen-barrier.h
> new file mode 100644
> index 0000000000..62036f528b
> --- /dev/null
> +++ b/tools/include/xen-barrier.h
> @@ -0,0 +1,51 @@
> +/******************************************************************************
> + * xen-barrier.h
> + *
> + * Definition of CPU barriers, part of libxenctrl.

Does it needs to be part of "libxenctrl" ? :-) Since the goal is to be
able to use the header without xenctrl.

> + *
> + * Copyright (c) 2003-2004, K A Fraser.

I'm not sure this copyright line is enough, looking at `git blame`.

Keir introduce xen_barrier macro in 2012, in
8d3f757328e1 ("libxc: Update rmb/wmb for x86.")
Stefano introduced the Arm macro in 2012, in daa314fe1938 ("arm: compile
libxc"), and Ian in 2013 in ae4b6f29a983 ("tools: libxc: arm64
support").
There's been a modification by Andrew in 2020, so Citrix copyright,
in de16a8fa0db7 ("x86: Use LOCK ADD instead of MFENCE for smp_mb()").

So overall, we probably want:
Copyright (C) 2003-2012, K A Fraser.
Copyright (C) 2012-2020 Citrix Systems, Inc.


> + *
> + * 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/>.
> + */
> +
> +#ifndef XENBARRIER_H
> +#define XENBARRIER_H

With an extra '_' for the '-' in the header filename?
    XEN_BARRIER_H


Otherwise, the rest of the patch looks fine to me, even without the rest
of the series.

Cheers,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH 4/6] tools/libs: add a new libxenmanage library

[Jürgen Groß]

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

On 26.11.24 17:44, Anthony PERARD wrote:
> On Fri, Nov 22, 2024 at 04:12:25PM +0100, Jürgen Groß wrote:
>> On 22.11.24 14:55, Anthony PERARD wrote:
>>> On Wed, Oct 23, 2024 at 03:10:03PM +0200, Juergen Gross wrote:
>>>> diff --git a/tools/include/xenmanage.h b/tools/include/xenmanage.h
>>>> new file mode 100644
>>>> index 0000000000..2e6c3dedaa
>>>> --- /dev/null
>>>> +++ b/tools/include/xenmanage.h
>>>> @@ -0,0 +1,98 @@
>>>> +/*
>>>> + * 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/>.
>>>
>>> Shall we use SPDX tags instead of this boilerplate?
>>
>> My thinking was to avoid that for "official" header files, as those might
>> be copied verbatim to other projects, which might not use SPDX. So having
>> the license text verbatim avoids any problems in this regard.
> 
> Well, this header in particular would be fairly useless, I believe, if it
> was copied into an other project, it described a library so need to be
> distributed along side the library. Second, this isn't the text of the
> license but something describing which license is used and where to
> find the text for it. An SPDX tag does nearly the same thing, but can
> actually be parse by a computer.
> 
> Official headers that would be useful to be copied into other project
> already expose the SPDX tags, many/all the headers in
> xen/include/public, as well as headers created by `mkheader.py` in this
> directory (tools/include).
> 
> I've taken a look into my directory "/usr/include", and they are plenty
> of headers having the SPDX tag.
> 
> So overall, I think we are fine to use SPDX tags here as well. ;-)

Okay, I'll switch to SPDX.


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 5/6] tools: add a dedicated header file for barrier definitions

[Jürgen Groß]

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

On 26.11.24 18:28, Anthony PERARD wrote:
> On Wed, Oct 23, 2024 at 03:10:04PM +0200, Juergen Gross wrote:
>> diff --git a/tools/include/xen-barrier.h b/tools/include/xen-barrier.h
>> new file mode 100644
>> index 0000000000..62036f528b
>> --- /dev/null
>> +++ b/tools/include/xen-barrier.h
>> @@ -0,0 +1,51 @@
>> +/******************************************************************************
>> + * xen-barrier.h
>> + *
>> + * Definition of CPU barriers, part of libxenctrl.
> 
> Does it needs to be part of "libxenctrl" ? :-) Since the goal is to be
> able to use the header without xenctrl.

Yeah, but it was part of libxenctrl until now.

So it needs to be distributed together with xenctrl.h, otherwise we'd
need another distribution unit libxenctrl has to depend on.

>> + *
>> + * Copyright (c) 2003-2004, K A Fraser.
> 
> I'm not sure this copyright line is enough, looking at `git blame`.
> 
> Keir introduce xen_barrier macro in 2012, in
> 8d3f757328e1 ("libxc: Update rmb/wmb for x86.")
> Stefano introduced the Arm macro in 2012, in daa314fe1938 ("arm: compile
> libxc"), and Ian in 2013 in ae4b6f29a983 ("tools: libxc: arm64
> support").
> There's been a modification by Andrew in 2020, so Citrix copyright,
> in de16a8fa0db7 ("x86: Use LOCK ADD instead of MFENCE for smp_mb()").
> 
> So overall, we probably want:
> Copyright (C) 2003-2012, K A Fraser.
> Copyright (C) 2012-2020 Citrix Systems, Inc.

Hmm, I just copied the Copyright from xenctrl.h, where I took the contents
of the new header from.

I can see a reason for adding the Copyright for Citrix, but IMHO this
_should_ have happened when the changes were applied in 2020. And I'm feeling
a little bit uneasy to add a Copyright for a company I'm not working for. How
do I really know Citrix is wanting that Copyright to be added?

>> + *
>> + * 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/>.
>> + */
>> +
>> +#ifndef XENBARRIER_H
>> +#define XENBARRIER_H
> 
> With an extra '_' for the '-' in the header filename?
>      XEN_BARRIER_H

Okay.

> Otherwise, the rest of the patch looks fine to me, even without the rest
> of the series.

Thanks,


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 3/6] xen: add new domctl get_changed_domain

[Juergen Gross]

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

On 23.10.24 15:10, Juergen Gross wrote:
> 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 specific to the respective domctl op and it is an
> in/out parameter: On input the caller is specifying the desired version
> of the op, while on output the hypervisor will return the used version
> (this will be at max the caller supplied version, but might be lower in
> case the hypervisor doesn't support this version).
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Could I please get some feedback regarding the new stable (sub-)hypercall?

I'd _really_ like to get this series into 4.20.

In the RFC series I suggested a new stable main hypercall, but this wasn't
liked by Jan. So I used the current approach to make it a sub-op of the
domctl hypercall, which Jan didn't like either in the current form.

Andrew told me multiple times, he wanted to give some feedback, but hasn't
done so yet.

I think there are multiple options how to proceed, the following coming to
my mind:

1. Use a new stable admin hypercall, similar to the RFC series some time ago.
    In case this is the preferred option, I'd suggest to make it similar to
    the current sysctl hypercall, but without the version field. Any not
    compatible change of a sub-op would need a new sub-op instead.

2. Use stable domctl (and possibly sysctl in the future) sub-ops like in this
    patch, but possibly drop the use of the interface version field. Any not
    compatible change of a stable sub-op would need a new sub-op instead.

I'll set this topic on the agenda of the community call tomorrow in case I
don't get any feedback allowing to proceed.


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 3/6] xen: add new domctl get_changed_domain

[Juergen Gross]

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

On 04.12.24 11:01, Juergen Gross wrote:
> On 23.10.24 15:10, Juergen Gross wrote:
>> 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 specific to the respective domctl op and it is an
>> in/out parameter: On input the caller is specifying the desired version
>> of the op, while on output the hypervisor will return the used version
>> (this will be at max the caller supplied version, but might be lower in
>> case the hypervisor doesn't support this version).
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Could I please get some feedback regarding the new stable (sub-)hypercall?
> 
> I'd _really_ like to get this series into 4.20.
> 
> In the RFC series I suggested a new stable main hypercall, but this wasn't
> liked by Jan. So I used the current approach to make it a sub-op of the
> domctl hypercall, which Jan didn't like either in the current form.
> 
> Andrew told me multiple times, he wanted to give some feedback, but hasn't
> done so yet.
> 
> I think there are multiple options how to proceed, the following coming to
> my mind:
> 
> 1. Use a new stable admin hypercall, similar to the RFC series some time ago.
>     In case this is the preferred option, I'd suggest to make it similar to
>     the current sysctl hypercall, but without the version field. Any not
>     compatible change of a sub-op would need a new sub-op instead.
> 
> 2. Use stable domctl (and possibly sysctl in the future) sub-ops like in this
>     patch, but possibly drop the use of the interface version field. Any not
>     compatible change of a stable sub-op would need a new sub-op instead.
> 
> I'll set this topic on the agenda of the community call tomorrow in case I
> don't get any feedback allowing to proceed.

So just to have the whole discussion on xen-devel:

In the community call Andrew gave his feedback: he is preferring above
variant 2, with the interface version field required to be 0 for stable
sub-ops.

I'll go this route, while addressing Jan's other comments.


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