Re: [PATCH v2 20/21] xen: support console_switching between Dom0 and DomUs on ARM

[Stefano Stabellini <sstabellini@kernel.org>]

On Mon, 16 Jul 2018, Jan Beulich wrote:
> >>> On 07.07.18 at 01:12, <sstabellini@kernel.org> wrote:
> > @@ -389,29 +392,49 @@ 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. */
> > +static int __read_mostly xen_rx = 1; /* 1 => input passed to domain 0. */
> 
> I guess this variable wants renaming now.

Yeah. What about `console_rx'? 


> >  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]);
> > +    xen_rx++;
> > +    if ( xen_rx == max_init_domid + 1 )
> > +        xen_rx = 0;
> > +
> > +    if ( !xen_rx )
> > +        printk("*** Serial input xen_rx=%d -> %s", xen_rx, "Xen");
> > +    else
> > +        printk("*** Serial input xen_rx=%d -> DOM%d", xen_rx, xen_rx - 1);
> 
> What are the xen_rx= doing in the format string? They weren't there before.

Ah yes, we don't want to print "xen_rx" anywhere, it's a leftover from
my debugging. I'll remove it completely:

  if ( !xen_rx )
      printk("*** Serial input to Xen");
  else
      printk("*** Serial input to DOM%d", xen_rx - 1);


> >      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 )
> > +    if ( xen_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;
> > +    if ( xen_rx == 1 )
> > +    {
> > +        /* Deliver input to guest buffer, unless it is already full. */
> > +        if ( (serial_rx_prod-serial_rx_cons) != SERIAL_RX_SIZE )
> 
> Please add blanks around the - .

I'll do


> > +            serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> > +    }
> > +#ifdef CONFIG_ARM
> 
> CONFIG_HAS_PL011 ?

I had already spotted this problem. I turned it into:

  #if defined(CONFIG_ARM) && defined(CONFIG_SBSA_VUART_CONSOLE)

It's CONFIG_SBSA_VUART_CONSOLE rather than CONFIG_HAS_PL011 because this
has to do with the virtual pl011 implementation rather than the physical
driver in Xen.


> > +    else
> > +    {
> > +        struct domain *d = get_domain_by_id(xen_rx - 1);
> > +        if ( !d->arch.vpl011.ring_enable && d->arch.vpl011.inring != NULL )
> 
> Blank line between these two lines please.

OK


> > @@ -933,9 +956,6 @@ void __init console_endboot(void)
> >                              "decrease log level threshold", 0);
> >      register_irq_keyhandler('G', &do_toggle_guest,
> >                              "toggle host/guest log level adjustment", 0);
> > -
> > -    /* Serial input is directed to DOM0 by default. */
> > -    switch_serial_input();
> 
> This removes an imo helpful boot time message. Is that intentional,
> and if so why?
 
Yes, it was intentional. switch_serial_input increases xen_rx, I thought
it didn't make too much sense to do that at boot, and would be clearer
to just initialize xen_rx to the wanted value from the get go (the value
would be 1 for dom0). Also, in previous implementations of this patch it
was actually required, but not anymore.

In fact, if you prefer, I could also keep this switch_serial_input()
call as-is and change the initial value of xen_rx to 0. That would also
work, as the increase of xen_rx here would end up selecting still dom0
for input.

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