Re: [PATCH v2] xen/console: Skip switching serial input to non existing domains

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 16 Mar 2023, Jan Beulich wrote:
> On 16.03.2023 11:26, Michal Orzel wrote:
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -490,7 +490,24 @@ static void switch_serial_input(void)
> >      }
> >      else
> >      {
> > -        console_rx++;
> > +        unsigned int next_rx = console_rx + 1;
> > +
> > +        /* Skip switching serial input to non existing domains */
> > +        while ( next_rx < max_init_domid + 1 )
> > +        {
> > +            struct domain *d = rcu_lock_domain_by_id(next_rx - 1);
> > +
> > +            if ( d )
> > +            {
> > +                rcu_unlock_domain(d);
> > +                break;
> > +            }
> > +
> > +            next_rx++;
> > +        }
> > +
> > +        console_rx = next_rx;
> > +
> >          printk("*** Serial input to DOM%d", console_rx - 1);
> >      }
> 
> While at the first glance (when you sent it in reply to v1) it looked okay,
> I'm afraid it really isn't: Please consider what happens when the last of
> the DomU-s doesn't exist anymore. (You don't really check whether it still
> exists, because the range check comes ahead of the existence one.) In that
> case you want to move from second-to-last to Xen. I expect the entire
> if/else construct wants to be inside the loop.

I don't think we need another loop, just a check if we found a domain or
not. E.g.:


    unsigned int next_rx = console_rx + 1;

    /* Skip switching serial input to non existing domains */
    while ( next_rx < max_init_domid + 1 )
    {
        struct domain *d = rcu_lock_domain_by_id(next_rx - 1);

        if ( d )
        {
            rcu_unlock_domain(d);
            console_rx = next_rx;
            printk("*** Serial input to DOM%d", console_rx - 1);
            break;
        }

        next_rx++;
    }

    /* no domain found */
    console_rx = 0;
    printk("*** Serial input to Xen");