Re: [PATCH 3/6] xen: add new domctl get_changed_domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.10.2024 15:10, Juergen Gross wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -154,6 +154,57 @@ void domain_reset_states(void)
>      rcu_read_unlock(&domlist_read_lock);
>  }
>  
> +static void set_domain_state_info(struct xen_domctl_get_domain_state *info,
> +                                  const struct domain *d)
> +{
> +    info->state = XEN_DOMCTL_GETDOMSTATE_STATE_EXIST;
> +    if ( d->is_shut_down )
> +        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN;
> +    if ( d->is_dying == DOMDYING_dead )
> +        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DYING;
> +    info->unique_id = d->unique_id;
> +}
> +
> +int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain 
> *d)
> +{
> +    unsigned int dom;
> +
> +    memset(info, 0, sizeof(*info));

Would this better go into set_domain_state_info()? Ah, no, you ...

> +    if ( d )
> +    {
> +        set_domain_state_info(info, d);
> +
> +        return 0;
> +    }
> +
> +    while ( (dom = find_first_bit(dom_state_changed, DOMID_MASK + 1)) <
> +            DOMID_FIRST_RESERVED )
> +    {
> +        d = rcu_lock_domain_by_id(dom);

... acquiring the lock early and then ...

> +        if ( test_and_clear_bit(dom, dom_state_changed) )
> +        {
> +            info->domid = dom;
> +            if ( d )
> +            {
> +                set_domain_state_info(info, d);

... potentially bypassing the call (with just the domid set) requires it
that way.

As to the point in time when the lock is acquired: Why is that, seeing that
it complicates the unlocking a little, by requiring a 2nd unlock a few
lines down?

> +                rcu_unlock_domain(d);
> +            }
> +
> +            return 0;
> +        }
> +
> +        if ( d )
> +        {
> +            rcu_unlock_domain(d);
> +        }

Nit: No need for the braces.

> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -969,11 +969,18 @@ static struct domain *global_virq_handlers[NR_VIRQS] 
> __read_mostly;
>  
>  static DEFINE_SPINLOCK(global_virq_handlers_lock);
>  
> +struct domain *get_global_virq_handler(uint32_t virq)
> +{
> +    ASSERT(virq_is_global(virq));
> +
> +    return global_virq_handlers[virq] ?: hardware_domain;
> +}
> +
>  void send_global_virq(uint32_t virq)
>  {
>      ASSERT(virq_is_global(virq));
>  
> -    send_guest_global_virq(global_virq_handlers[virq] ?: hardware_domain, 
> virq);
> +    send_guest_global_virq(get_global_virq_handler(virq), virq);
>  }

Is this a stale leftover from an earlier version? There's no other caller of
get_global_virq_handler() here, hence the change looks unmotivated here.

> @@ -1236,7 +1237,37 @@ struct xen_domctl_dt_overlay {
>  };
>  #endif
>  
> +/*
> + * XEN_DOMCTL_get_domain_state (stable interface)
> + *
> + * Get state information of a domain.
> + *
> + * In case domain is DOMID_INVALID, return 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).
> + *
> + * Supported interface versions: 0x00000000
> + */
> +#define XEN_DOMCTL_GETDOMSTATE_VERS_MAX    0
> +struct xen_domctl_get_domain_state {
> +    domid_t domid;

Despite the DOMID_INVALID special case the redundant domid here is odd.
You actually add the new sub-op to the special casing of op->domain at the
top of do_domctl(), so the sole difference to most other sub-ops would be
that this then is an IN/OUT (rather than the field here being an output
only when DOMID_INVALID was passed in via the common domid field).

> +    uint16_t state;
> +#define XEN_DOMCTL_GETDOMSTATE_STATE_EXIST     0x0001  /* Domain is 
> existing. */
> +#define XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN  0x0002  /* Shutdown finished. 
> */
> +#define XEN_DOMCTL_GETDOMSTATE_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. */
> +};

What are the intentions with this padding array?

Jan