[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.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? >>>>>> 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. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |