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

[Juergen Gross <jgross@xxxxxxxx>]

On 25.11.21 11:51, Jan Beulich wrote:
> On 25.11.2021 11:33, Juergen Gross wrote:
> > On 25.11.21 11:19, Jan Beulich wrote:
> > > On 25.11.2021 11:12, Juergen Gross wrote:
> > > > 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.
> > > If your reference is to the single consumer aspect, then I don't see
> > > why that would matter here. Future xenstore may want/need to make
> > > use of such a future flag, yet older xenstore still wouldn't know
> > > about it.
> > I'm not sure it is a good idea to mix IN and OUT fields in such a struct
> > which is meant to return information only.
> >
> > I'd rather add a new sub-op in this case taking another parameter for
> > specifying additional options or a struct prepending the needed IN
> > fields to above struct.
> Well, okay. May ask for a /* OUT */ comment then ahead of the first of
> the struct fields?
Yes, will add it.

> > > > > > > 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.
> > > True. Problem is that our present __XEN_TOOLS__ guards have effectively
> > > dual meaning - "tools only" and "unstable". We merely need to be sure
> > > everyone understands that this is changing. Perhaps when you add such a
> > > guard here, it may want accompanying by a respective comment.
> > I'd be fine with that.
> >
> > Maybe we want a new guard "__XEN_INTERNAL__" for that (new) purpose?
> Not sure - this may result in undesirable code churn elsewhere.
I won't insist on it. :-)


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature