Re: [BUG v2] common/domctl: xsm update for get_domain_state access

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.02.2026 20:08, Daniel P. Smith wrote:
> @@ -238,28 +242,39 @@ int get_domain_state(struct xen_domctl_get_domain_state 
> *info, struct domain *d,
>  
>      while ( dom_state_changed )
>      {
> -        dom = find_first_bit(dom_state_changed, DOMID_MASK + 1);
> +        dom = find_next_bit(dom_state_changed, DOMID_MASK + 1, dom + 1);
>          if ( dom >= DOMID_FIRST_RESERVED )
>              break;
> +
> +        d = rcu_lock_domain_by_id(dom);
> +        if ( d && xsm_get_domain_state(XSM_XS_PRIV, d) )
> +        {
> +            rcu_unlock_domain(d);
> +            d = NULL;

This looks unnecessary; the next loop iteration will set d unconditionally,
and d isn't (and wasn't) used past the loop. Plus there is also no such
clearing after the other rcu_unlock_domain().

> +            continue;
> +        }

This cleanup here also is redundant with the one done further down. Imo where
possible we should prefer to have only a single such instance, which looks to
be easily possible by using ...

>          if ( test_and_clear_bit(dom, dom_state_changed) )


        if ( (!d || !xsm_get_domain_state(XSM_XS_PRIV, d)) &&
             test_and_clear_bit(dom, dom_state_changed) )

or

        if ( (d && xsm_get_domain_state(XSM_XS_PRIV, d)) ||
             !test_and_clear_bit(dom, dom_state_changed) )
        {
             ...
             continue;
        }

albeit then the reduction of indentation of the subsequent code would cause
quite a bit more code churn.

>          {
>              *domid = dom;
>  
> -            d = rcu_lock_domain_by_id(dom);
> -
>              if ( d )
>              {
>                  set_domain_state_info(info, d);
> -
>                  rcu_unlock_domain(d);
>              }
>              else
>                  memset(info, 0, sizeof(*info));
>  
>              rc = 0;
> -
>              break;

I don't think the blank lines need dropping for the purpose of the patch?
Yes, they may seem excessive, but nevertheless some prefer to have rather
too many of them than too few. (Personally I don't mind their removal,
but that really doesn't look to belong here.)

>          }
> +
> +        if ( d )
> +        {
> +            rcu_unlock_domain(d);
> +            d = NULL;
> +        }
>      }
>  
>   out:
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -860,12 +860,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>          break;
>  
>      case XEN_DOMCTL_get_domain_state:
> -        ret = xsm_get_domain_state(XSM_XS_PRIV, d);
> -        if ( ret )
> -            break;
> -
> -        copyback = 1;
>          ret = get_domain_state(&op->u.get_domain_state, d, &op->domain);
> +        if ( !ret )
> +            copyback = 1;

Nit: As you need to touch this, please switch to using "true".

Jan