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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 8 Oct 2018, Jan Beulich wrote:
> >>> On 05.10.18 at 20:47, <sstabellini@xxxxxxxxxx> wrote:
> > @@ -391,31 +394,73 @@ 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.
> > + */
> 
> Just like the title, this comment makes it sound as if any DomU could
> participate in this switching.

Is the following any better?

  CTRL-<switch_char> changes input direction, rotating among Xen, Dom0, and 
DomUs.

Otherwise please suggest your favorite wording and I'll use it.


> >  #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 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 )
> 
> Please be consistent ...

I'll change it to console_rx == 0


> > +        printk("*** Serial input to Xen");
> > +    else
> > +        printk("*** Serial input to DOM%d", console_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 ( console_rx == 0 )
> 
> ... in style. But perhaps ...
> 
> >          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. 
> > */
> > -    send_global_virq(VIRQ_CONSOLE);
> > +    if ( console_rx == 1 )
> 
> ... switch() would be better to use here anyway.

I'll replace these with a switch


> > +    {
> > +        /* Deliver input to hardware domain, unless it is already full. */
> 
> Looks like you've mis-edited the original comment.

I take that you mean that the word "buffer" is missing, right? It should
be:

  Deliver input to the hardware domain buffer, unless it is already full.

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