Re: [PATCH v2 4/7] xen: add bitmap to indicate per-domain state changes

[Jürgen Groß]

On 09.12.24 17:52, Jan Beulich wrote:

On 06.12.2024 14:02, 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.

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 <jgr...@suse.com>

What I'm still missing is at least mention of the global-ness of all of
this, and why that's deemed okay for now.

I'll add:

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

--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -138,6 +138,60 @@ bool __read_mostly vmtrace_available;

+static unsigned long *dom_state_changed;
+
+int domain_init_states(void)
+{
+    const struct domain *d;
+    int rc = -ENOMEM;
+
+    spin_lock(&dom_state_changed_lock);
+
+    if ( dom_state_changed )
+        bitmap_zero(dom_state_changed, DOMID_FIRST_RESERVED);
+    else
+    {
+        dom_state_changed = xzalloc_array(unsigned long,
+                                          BITS_TO_LONGS(DOMID_FIRST_RESERVED));

bool __read_mostly vpmu_is_available; +static DEFINE_SPINLOCK(dom_state_changed_lock);

New code wants to use xvmalloc() et al.

Okay.

--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -485,20 +485,27 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
evtchn_port_t port)
      if ( (v = domain_vcpu(d, vcpu)) == NULL )
          return -ENOENT;

+    {
+        rc = domain_init_states();
+        if ( rc )
+            goto out;
+    }
+
      write_lock(&d->event_lock);

      {
          rc = -EEXIST;
          gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc);
-        goto out;
+        goto unlock;
      }

      if ( rc < 0 )
      {
          gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc);
-        goto out;
+        goto unlock;
      }

@@ -524,9 +531,13 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
evtchn_port_t port)
       */
      write_atomic(&v->virq_to_evtchn[virq], port);

+ unlock:
      write_unlock(&d->event_lock);

+    if ( rc )
+        domain_deinit_states();
+
      return rc;
  }

+ if ( virq == VIRQ_DOM_EXC ) if ( read_atomic(&v->virq_to_evtchn[virq]) ) port = rc = evtchn_get_port(d, port); rc = 0; - out: + out:

Renaming the prior label (and hence needing to fiddle with existing goto-s)
feels a little fragile. How about keeping "out" as is and introducing "deinit"
or some such?

Fine with me.


Juergen

OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

OpenPGP_signature.asc
Description: OpenPGP digital signature