Re: [Xen-devel] [PATCH v3 23/25] xen: support console_switching between Dom0 and DomUs on ARM

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.08.18 at 01:28, <sstabellini@xxxxxxxxxx> wrote:
> Today Ctrl-AAA is used to switch between Xen and Dom0. Extend the
> mechanism to allow for switching between Xen, Dom0, and any of the
> initial DomU created from Xen alongside Dom0 out of information provided
> via device tree.
> 
> Rename xen_rx to console_rx to match the new behavior.
> 
> Clarify existing comment about "notify the guest", making it clear that
> it is only about the hardware domain.
> 
> Export a function named console_input_domain() to allow others to know
> which domains has input at a given time.

As always in such cases I don't think such functions should be added
without any caller.

> @@ -389,30 +392,72 @@ static void dump_console_ring_key(unsigned char key)
>      free_xenheap_pages(buf, order);
>  }
>  
> -/* CTRL-<switch_char> switches input direction between Xen and DOM0. */
> +/*
> + * CTRL-<switch_char> switches input direction between Xen, Dom0 and
> + * DomUs.
> + */
>  #define switch_code (opt_conswitch[0]-'a'+1)
> -static int __read_mostly xen_rx = 1; /* FALSE => input passed to domain 0. */
> +/*
> + * console_rx=0 => input to xen
> + * console_rx=1 => input to dom0
> + * console_rx=N => input dom(N-1)
> + */
> +static int __read_mostly console_rx = 0;

Originally this was supposed to be bool, but didn't get switched yet.
With your current scheme it can't go negative and hence should be
unsigned int. However, I still wonder whether it wouldn't be better
(especially for readers of the code) is this was an actual domid_t.
But as clarified before, I'm not meaning to make this a requirement.

> +struct domain *console_input_domain(void)
> +{
> +    return get_domain_by_id(console_rx - 1);
> +}

And this is exactly the reason for the above remark: This is (or at
least looks) broken for console_rx == 0.

>  static void switch_serial_input(void)
>  {
> -    static char *input_str[2] = { "DOM0", "Xen" };
> -    xen_rx = !xen_rx;
> -    printk("*** Serial input -> %s", input_str[xen_rx]);
> +    console_rx++;
> +    if ( console_rx == max_init_domid + 2 )
> +        console_rx = 0;

Same here - the literal 2 at least raises questions. I think it would
at least be a little less confusing if you had

    if ( console_rx++ == max_init_domid + 1 )
        console_rx = 0;

>  static void __serial_rx(char c, struct cpu_user_regs *regs)
>  {
> -    if ( xen_rx )
> +    if ( console_rx == 0 )
>          return handle_keypress(c, regs);
>  
> -    /* Deliver input to guest buffer, unless it is already full. */
> -    if ( (serial_rx_prod-serial_rx_cons) != SERIAL_RX_SIZE )
> -        serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> -    /* Always notify the guest: prevents receive path from getting stuck. */

Just like you adjust "guest" in this comment, I think you'd better ...

> +    if ( console_rx == 1 )
> +    {
> +        /* Deliver input to guest buffer, unless it is already full. */

... adjust it here too.

> +        if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
> +            serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> +    }
> +#ifdef CONFIG_SBSA_VUART_CONSOLE
> +    else
> +    {
> +        struct domain *d = get_domain_by_id(console_rx - 1);
> +
> +        /*
> +         * 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->arch.vpl011.backend_in_domain && d->arch.vpl011.xen != NULL 
> )
> +            vpl011_rx_char_xen(d, c);
> +        else
> +            printk("Cannot send chars to Dom%d: no UART available\n",
> +                   d->domain_id);
> +    }
> +#endif
> +    /*
> +     * Always notify the hardware domain: prevents receive path from
> +     * getting stuck.
> +     */
>      send_global_virq(VIRQ_CONSOLE);

Why does this not move into the if() body above?

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

According to the comment I think you need to store
max_init_domid + 1 here, so that the switch_serial_input() a few
lines down would actually switch to Xen.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel