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

[Jan Beulich <jbeulich@xxxxxxxx>]

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