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

[Stefano Stabellini <stefano.stabellini@xilinx.com>]

On Mon, 13 Aug 2018, Julien Grall wrote:
> Hi Stefano,
> 
> OOI, on the previous version you said you will explore the CTRL-x N solution
> (where N is the domID console to switch too). What was the result here?

I meant I'll explore it as a follow-up to this series. I haven't looked
into it yet, but it is in my todo.


> On 01/08/18 00:28, Stefano Stabellini 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.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > CC: andrew.cooper3@citrix.com
> > CC: George.Dunlap@eu.citrix.com
> > CC: ian.jackson@eu.citrix.com
> > CC: jbeulich@suse.com
> > CC: konrad.wilk@oracle.com
> > CC: tim@xen.org
> > CC: wei.liu2@citrix.com
> > ---
> > Changes in v3:
> > - only call vpl011 functions ifdef CONFIG_SBSA_VUART_CONSOLE
> > - add blank line and spaces
> > - remove xen_rx from print messages
> > - rename xen_rx to console_rx
> > - keep switch_serial_input() at boot
> > - add better comments
> > - fix existing comment
> > - add warning if no guests console/uart is available
> > - add console_input_domain function
> > 
> > Changes in v2:
> > - only call vpl011_rx_char if the vpl011 has been initialized
> > ---
> >   xen/drivers/char/console.c | 71
> > +++++++++++++++++++++++++++++++++++++---------
> >   xen/include/xen/console.h  |  2 ++
> >   2 files changed, 60 insertions(+), 13 deletions(-)
> > 
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index 0f05369..cd4dfb1 100644
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -31,10 +31,13 @@
> >   #include <xen/early_printk.h>
> >   #include <xen/warning.h>
> >   #include <xen/pv_console.h>
> > +#include <asm/setup.h>
> >     #ifdef CONFIG_X86
> >   #include <xen/consoled.h>
> >   #include <asm/guest.h>
> > +#else
> > +#include <asm/vpl011.h>
> >   #endif
> >     /* console: comma-separated list of console outputs. */
> > @@ -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;
> > +
> > +struct domain *console_input_domain(void)
> > +{
> > +    return get_domain_by_id(console_rx - 1);
> 
> Please take care of the case where console_rx == 0.

I'll do


> > +}
> >     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;
> > +
> > +    if ( !console_rx )
> > +        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 )
> >           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.
> > */
> > +    if ( console_rx == 1 )
> > +    {
> > +        /* 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;
> > +    }
> > +#ifdef CONFIG_SBSA_VUART_CONSOLE
> > +    else
> > +    {
> > +        struct domain *d = get_domain_by_id(console_rx - 1);
> 
> I don't think this is correct to assume the domain will always be present.
> With this series, it would be possible to retire a domain and therefore
> get_domain_by_id() would return NULL here. This would result to a data abort
> below.

Well, spotted! I'll fix.


> Also, I think you want to use rcu_lock_by_domain here (I am not 100% sure
> though).

Uhm... I think you are right


> > +
> > +        /*
> > +         * 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
> 
> That's not true. If you look at send_global_virq, it will send to the domain
> that register the VIRQ. That may be the hardware domain but could be someone
> else.

The comment and the code are from the existing code, they have only been
moved by this patch. I changed it from "Always notify the guest" to
"Always notify the hardware domain" for clarity. Would you like me to
improve the comment since I am at it? I am happy to include a specific
line, if you prefer.


> However, I still don't understand how this work in presence of multiple
> backend here. Why would you notify Domain B everytime a character is
> redirected to domain A/C/D the console?

You are right, I'll fix that



> > +     * getting stuck.
> > +     */
> >       send_global_virq(VIRQ_CONSOLE);
> >     #ifdef CONFIG_X86
> > @@ -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;
> >         register_keyhandler('w', dump_console_ring_key,
> >                           "synchronously dump console ring buffer (dmesg)",
> > 0);
> > diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
> > index ea06fd8..2fe3912 100644
> > --- a/xen/include/xen/console.h
> > +++ b/xen/include/xen/console.h
> > @@ -31,6 +31,8 @@ void console_end_sync(void);
> >   void console_start_log_everything(void);
> >   void console_end_log_everything(void);
> >   +struct domain *console_input_domain(void);
> > +
> >   /*
> >    * Steal output from the console. Returns +ve identifier, else -ve error.
> >    * Takes the handle of the serial line to steal, and steal callback
> > function.

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