[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH RFC 3/4] xen: add new stable control hypercall



On 25.11.21 10:38, Jan Beulich wrote:
On 25.11.2021 07:55, Juergen Gross wrote:
On 22.11.21 16:39, Jan Beulich wrote:
On 14.09.2021 14:35, Juergen Gross wrote:
@@ -103,6 +104,43 @@ void domain_reset_states(void)
       rcu_read_unlock(&domlist_read_lock);
   }
+int domain_get_dom_state_changed(struct xen_control_changed_domain *info)
+{
+    unsigned int dom;
+    struct domain *d;
+
+    while ( (dom = find_first_bit(dom_state_changed, DOMID_MASK + 1)) <
+            DOMID_FIRST_RESERVED )

As per my comment on the earlier patch - the use of DOMID_MASK + 1 vs
is quite puzzling here.

Okay, will change that.


+    {
+        d = rcu_lock_domain_by_id(dom);
+
+        if ( test_and_clear_bit(dom, dom_state_changed) )
+        {
+            info->domid = dom;
+            if ( d )
+            {
+                info->state = XEN_CONTROL_CHANGEDDOM_STATE_EXIST;
+                if ( d->is_shut_down )
+                    info->state |= XEN_CONTROL_CHANGEDDOM_STATE_SHUTDOWN;
+                if ( d->is_dying == DOMDYING_dead )
+                    info->state |= XEN_CONTROL_CHANGEDDOM_STATE_DYING;
+                info->unique_id = d->unique_id;
+
+                rcu_unlock_domain(d);
+            }
+
+            return 0;

With rapid creation of short lived domains, will the caller ever get to
see information on higher numbered domains (if, say, it gets "suitably"
preempted within its own environment)? IOW shouldn't there be a way for
the caller to specify a domid to start from?

I'd rather have a local variable for the last reported domid and start
from that.

Well, it probably doesn't matter much to have yet one more aspect making
this a single-consumer-only interface.

For making it an interface consumable by multiple users you'd need to
have a per-consumer data set, which would include the bitmap of changed
domains and could include the domid last tested.

As one consumer is Xenstore, and Xenstore can run either in a dedicated
domain or in dom0, I believe a multiple user capable interface would
even need to support multiple users in the same domain, which would be
even more complicated. So I continue to be rather hesitant to add this
additional complexity with only some vague idea of "might come handy in
the future".


+/*
+ * XEN_CONTROL_OP_get_state_changed_domain
+ *
+ * Get 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).
+ *
+ * arg: XEN_GUEST_HANDLE(struct xen_control_changed_domain)
+ *
+ * Possible return values:
+ * 0: success
+ * <0 : negative Xen errno value
+ */
+#define XEN_CONTROL_OP_get_state_changed_domain     1
+struct xen_control_changed_domain {
+    domid_t domid;
+    uint16_t state;
+#define XEN_CONTROL_CHANGEDDOM_STATE_EXIST     0x0001  /* Domain is existing. 
*/
+#define XEN_CONTROL_CHANGEDDOM_STATE_SHUTDOWN  0x0002  /* Shutdown finished. */
+#define XEN_CONTROL_CHANGEDDOM_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. */

I think the padding fields have to be zero on input, not just on return.

I don't see why this would be needed, as this structure is only ever
copied to the caller, so "on input" just doesn't apply here.

Unless you mean to mandate them to be OUT only now and forever. I also

The whole struct is OUT only.

Right now, yes. I wouldn't exclude "pad1" to become a flags field,
controlling some future behavioral aspect of the operation.

Right now I don't see the need for that, see my reasoning above.


wonder how the trailing padding plays up with the version sub-op: Do we
really need such double precaution?

I can remove it.

Also - should we use uint64_aligned_t here?

Yes.

But you realize this isn't straightforward, for the type not being
available in plain C89 (nor C99)? That's why it's presently used in
tools-only interfaces only, and the respective header are excluded
from the "is ANSI compatible" checking (memory.h and hvm/dm_op.h
have special but imo crude "precautions").

No, I didn't realize that. I just looked how it is used today and
agreed I should follow current usage.

But even with using a stable interface I'm not sure we need to make it
strictly ANSI compatible, as usage of this interface will still be
restricted to tools.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.