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.
>>> +/*
>>> + * 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.
>> 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").
Jan
