Re: [PATCH v5 3/4] xen/console: remove max_init_domid dependency

[dmkhn@xxxxxxxxx]

On Thu, Jun 05, 2025 at 11:20:32PM +0100, Julien Grall wrote:
> Hi Jan,
> 
> On 04/06/2025 13:55, Jan Beulich wrote:
> > On 31.05.2025 01:19, dmkhn@xxxxxxxxx wrote:
> >> --- a/xen/common/domain.c
> >> +++ b/xen/common/domain.c
> >> @@ -2461,6 +2461,39 @@ void domid_free(domid_t domid)
> >>       spin_unlock(&domid_lock);
> >>   }
> >>
> >> +/*
> >> + * Find the ID of the next possible console owner domain.
> >> + *
> >> + * @return Domain ID: DOMID_XEN or non-system domain IDs within
> >> + * the range of [0..DOMID_FIRST_RESERVED-1].
> >> + */
> >> +domid_t domid_find_with_input_allowed(domid_t hint)
> >> +{
> >> +    domid_t domid = DOMID_XEN;
> >> +
> >> +    if ( hint < DOMID_FIRST_RESERVED )
> >> +    {
> >> +        struct domain *d;
> >> +
> >> +        rcu_read_lock(&domlist_read_lock);
> >> +
> >> +        for ( d = domid_to_domain(hint);
> >
> > If the domain with ID "hint" went away, what is being switched to changes
> > compared to behavior prior to this patch, if I'm not mistaken. While this
> > _may_ be acceptable, not saying so in the description is imo a no-go.
> >
> >> +              d && get_domain(d) && d->domain_id < DOMID_FIRST_RESERVED;
> >
> > What's the DOMID_FIRST_RESERVED check for? And where's the put_domain()
> > for the get_domain() here?
> >
> >> +              d = rcu_dereference(d->next_in_list) )
> >> +        {
> >> +            if ( d->console.input_allowed )
> >> +            {
> >> +                domid = d->domain_id;
> >> +                break;
> >> +            }
> >> +        }
> >> +
> >> +        rcu_read_unlock(&domlist_read_lock);
> >> +    }
> >> +
> >> +    return domid;
> >> +}
> >
> > My concern remains: With many domains, the loop here may take quite a few
> > iterations. That's even more concerning because it regresses right away in
> > environments where along with boot-time created domains (eligible for
> > console focus) later further domains are created (non of which are eligible
> > for console focus). That is, the step from last boot-time created back to
> > DOMID_XEN may now take excessively long.
> 
> +1. I vaguely recall making the same concern earlier in the review. One
> possibility would be to have a list of domains where the console is
> supported. I don't think it would be necessary to have the list sorted
> as I view the console switching a debug facility.

Jan, Julien,

Thanks for the feedback!
Will rework.

> 
> Cheers,
> 
> --
> Julien Grall
> 
>