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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.10.18 at 04:03, <sstabellini@xxxxxxxxxx> wrote:
> @@ -391,31 +394,79 @@ 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> changes input direction, rotating among Xen, Dom0,
> + * and DomUs.

Proper attribute to be inserted here, as per my (late) v4 comment.

> + */
>  #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 to dom(N-1)
> + */
> +static unsigned int __read_mostly 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]);
> +    if ( console_rx++ == max_init_domid + 1 )
> +        console_rx = 0;
> +
> +    if ( console_rx == 0 )
> +        printk("*** Serial input to Xen");

I think these two if()-s can and should be combined. Furthermore
I think it would be worthwhile to defer the increment until ...

> +    else
> +        printk("*** Serial input to DOM%d", console_rx - 1);

... after this printk(), but (obviously) then inside the else block.

>      if ( switch_code )
> -        printk(" (type 'CTRL-%c' three times to switch input to %s)",
> -               opt_conswitch[0], input_str[!xen_rx]);
> +        printk(" (type 'CTRL-%c' three times to switch input)",
> +               opt_conswitch[0]);
>      printk("\n");
>  }
>  
>  static void __serial_rx(char c, struct cpu_user_regs *regs)
>  {
> -    if ( xen_rx )
> +    switch ( console_rx )
> +    {
> +    case 0:
>          return handle_keypress(c, regs);
> +    case 1:
> +    {

Stray figure braces. Blank lines between case blocks please.

> +        /*
> +         * Deliver input to the hardware domain 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 hardware domain: prevents receive path from
> +         * getting stuck.
> +         */
> +        send_global_virq(VIRQ_CONSOLE);
> +        break;
> +    }
> +#if 0

I suppose a later patch is meant to remove this again. But
perhaps still a good idea to attach a brief comment, in case
the series doesn't go in all in one go? This is assuming you
did already consider (and discard for whatever reason) the
option of adding this default case when the code can
actually be used.

Jan



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