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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 26 Oct 2018, Jan Beulich wrote:
> >>> 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.

OK, I wrote:

 * CTRL-<switch_char> changes input direction, rotating among Xen, Dom0,
 * and the DomUs started from Xen at boot.

but I would be happy take suggestion for a simple single word attribute
to describe this kind of domains. Maybe "dom0less 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 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 ...

OK

 
> > +    else
> > +        printk("*** Serial input to DOM%d", console_rx - 1);
> 
> ... after this printk(), but (obviously) then inside the else block.

Yes, I can do that


> >      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.

OK


> > +        /*
> > +         * 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.

I'll add a comment

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