Re: [PATCH] console: generalize the ability for domU access

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.08.2023 18:06, Daniel P. Smith wrote:
> This patch reworks the console rotation logic to provide a general mechanism 
> to
> incorporate domU in to the rotation. It does so by walking the domain list
> using the XSM console privlege check to determine if the domain is given 
> access.
> 
> In reworking the rotation logic, the assumption that the hardware domain is 
> the
> first domain created is removed and is changed to explicitly locate the
> hardware domain at boot.

I guess I'm unable to identify any "at boot only" code. I'm also
puzzled by this indicating there is a need to do so, when the global
variable "hardware_domain" is available, and you actually use it.

> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -473,45 +473,102 @@ static void cf_check dump_console_ring_key(unsigned 
> char key)
>   */
>  static unsigned int __read_mostly console_rx = 0;
>  
> -#define max_console_rx (max_init_domid + 1)
> +#define CON_RX_DOMID (console_rx - 1)
>  
>  /* Make sure to rcu_unlock_domain after use */
>  struct domain *console_input_domain(void)
>  {
>      if ( console_rx == 0 )
>              return NULL;
> -    return rcu_lock_domain_by_id(console_rx - 1);
> +    return rcu_lock_domain_by_id(CON_RX_DOMID);
>  }
>  
>  static void switch_serial_input(void)
>  {
> -    unsigned int next_rx = console_rx;
> +    struct domain *next, *d = console_input_domain();

Looks like "next" cannot be pointer-to-const just because the XSM hooks
still aren't properly const-ified. Oh well.

> -    /*
> -     * Rotate among Xen, dom0 and boot-time created domUs while skipping
> -     * switching serial input to non existing domains.
> -     */

While it would need adjustment, I don't think the comment wants deleting
altogether.

> -    for ( ; ; )
> +    if ( d == NULL )

This covers both Xen receiving input and the domain receiving input having
gone away. Originally in the latter case the next sequential (in domid
numbering) domain would be switched to. In the new logic you start over
from the beginning of the domain list. Such a change in behavior (if
deemed acceptable at all, which I'm not convinced of) needs calling out in
the description.

>      {
> -        struct domain *d;
> +        if ( hardware_domain )
> +        {
> +            console_rx = hardware_domain->domain_id + 1;
> +            printk("*** Serial input to DOM%d", CON_RX_DOMID);

Here and elsewhere - why %d when original code properly used %u? I also
think there are now quite a few too many of these all identical
printk()s.

> +            goto out; //print switch_code statement & newline

Leftover development comment? (There's at least one more.)

> +        }
> +        else

Please avoid "else" after an if() that ends in "return", "goto", or
alike.

> +        {
> +            for_each_domain(next)

What guarantees that the list won't change behind your back? You don't
hold domlist_read_lock here afaict. It might be that you're safe because
that lock is an RCU one and this function is only invoked at init time
or from some form of interrupt handler. But that's far from obvious and
will hence need both properly confirming and stating in a comment. (It
is actually this concern, iirc, which so far had us avoid iterating the
domain list here.)

> +            {
> +                if ( xsm_console_io(XSM_OTHER, next, CONSOLEIO_read) == 0 )
> +                {
> +                    console_rx = next->domain_id + 1;
> +                    printk("*** Serial input to DOM%d", CON_RX_DOMID);
> +                    goto out; //print switch_code statement & newline
> +                }
> +            }
>  
> -        if ( next_rx++ >= max_console_rx )
> +            console_rx = 0;
> +            printk("*** Serial input to Xen");
> +            goto out;
> +        }
> +    }
> +
> +    for ( next = rcu_dereference(d->next_in_list); next != NULL;
> +          next = rcu_dereference(next->next_in_list) )

This looks like an open-coded continuation of for_each_domain() - I'm
afraid I'm wary of introducing anything like this.

> +    {
> +        if ( hardware_domain && next == hardware_domain )
>          {
>              console_rx = 0;
>              printk("*** Serial input to Xen");
> -            break;
> +            goto out;

Since you use "goto" anyway, this wants introducing a 2nd label (maybe
"xen"?) ahead of the identical code you add further down (immediately
ahead of the "out" label), to avoid code duplication.

>          }
>  
> -        d = rcu_lock_domain_by_id(next_rx - 1);
> -        if ( d )
> +        if ( xsm_console_io(XSM_OTHER, next, CONSOLEIO_read) == 0 )
>          {
> -            rcu_unlock_domain(d);
> -            console_rx = next_rx;
> -            printk("*** Serial input to DOM%u", next_rx - 1);
> -            break;
> +            console_rx = next->domain_id + 1;
> +            printk("*** Serial input to DOM%d", CON_RX_DOMID);
> +            goto out;
> +        }
> +    }
> +
> +    /*
> +     * Hit the end of the domain list and instead of assuming that the
> +     * hardware domain is the first in the list, get the first domain
> +     * in the domain list and then if it is not the hardware domain or
> +     * does not have console privilege, iterate the list until we find
> +     * the hardware domain or a domain with console privilege.
> +     */
> +    if ( next == NULL )
> +    {
> +        for_each_domain(next)
> +        {
> +            if ( hardware_domain && next == hardware_domain )
> +            {
> +                console_rx = 0;
> +                printk("*** Serial input to Xen");
> +                goto out;
> +            }
> +
> +            if ( xsm_console_io(XSM_OTHER, next, CONSOLEIO_read) == 0 )
> +            {
> +                console_rx = next->domain_id + 1;
> +                printk("*** Serial input to DOM%d", CON_RX_DOMID);
> +                goto out;
> +            }
>          }
>      }
>  
> +    /*
> +     * If we got here, could not find a domain with console io privilege.
> +     * Default to Xen.
> +     */

"Default to" is a little odd when there are no other options.

> +    console_rx = 0;
> +    printk("*** Serial input to Xen");
> +
> +out:

Labels indented by at least one blank please.

> @@ -520,12 +577,11 @@ static void switch_serial_input(void)
>  
>  static void __serial_rx(char c, struct cpu_user_regs *regs)
>  {
> -    switch ( console_rx )
> -    {
> -    case 0:
> +    if ( console_rx == 0 )

By using CON_RX_DOMID everywhere else you try to carefully avoid and
open-coded assumptions on the precise biasing used there. With this
it would seem to me that here "CON_RX_DOMID > DOMID_MASK" would be
more in line with that model then.

>          return handle_keypress(c, regs);
>  
> -    case 1:
> +    if ( hardware_domain->domain_id == CON_RX_DOMID )

No check of hardware_domain against NULL?

> +    {
>          /*
>           * Deliver input to the hardware domain buffer, unless it is
>           * already full.
> @@ -538,31 +594,37 @@ static void __serial_rx(char c, struct cpu_user_regs 
> *regs)
>           * getting stuck.
>           */
>          send_global_virq(VIRQ_CONSOLE);
> -        break;
> -
> -#ifdef CONFIG_SBSA_VUART_CONSOLE
> -    default:
> +    }
> +    else
>      {
> -        struct domain *d = rcu_lock_domain_by_id(console_rx - 1);
> +        struct domain *d = rcu_lock_domain_by_any_id(CON_RX_DOMID);
>  
> +        if ( d == NULL )
> +            goto unlock_out;
> +
> +#ifdef CONFIG_SBSA_VUART_CONSOLE
>          /*
>           * If we have a properly initialized vpl011 console for the
>           * domain, without a full PV ring to Dom0 (in that case input
>           * comes from the PV ring), then send the character to it.
>           */
> -        if ( d != NULL &&
> -             !d->arch.vpl011.backend_in_domain &&
> +        if ( !d->arch.vpl011.backend_in_domain &&
>               d->arch.vpl011.backend.xen != NULL )
> +        {
>              vpl011_rx_char_xen(d, c);
> -        else
> -            printk("Cannot send chars to Dom%d: no UART available\n",
> -                   console_rx - 1);
> +            goto unlock_out;
> +        }
> +#endif
> +
> +        if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
> +            serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;

This is Dom0's buffer; I don't think sharing with DomU-s is correct.
You also cannot ...

> @@ -717,6 +779,8 @@ long do_console_io(
>          rc = -E2BIG;
>          if ( count > INT_MAX )
>              break;
> +        if ( CON_RX_DOMID != current->domain->domain_id )
> +            return 0;
>  
>          rc = 0;
>          while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )

... assume that by the time this hypercall is invoked input focus
hasn't switched. I think there's no way around a per-domain input
buffer, which of course would need setting up only for console-io-
capable domains.

> @@ -1107,7 +1171,7 @@ void __init console_endboot(void)
>       * a useful 'how to switch' message.
>       */
>      if ( opt_conswitch[1] == 'x' )
> -        console_rx = max_console_rx;
> +        console_rx = 0;

I can't bring this change in line with the comment ahead of the if():
Won't this result in switch_serial_input() switching to Dom0?

Jan