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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 1 Aug 2018, Jan Beulich wrote:
> >>> On 01.08.18 at 01:28, <sstabellini@xxxxxxxxxx> 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.
> 
> As always in such cases I don't think such functions should be added
> without any caller.

I'll add console_input_domain within an #if 0 and remove the #if 0 in
the following patch. If you are OK with it, the two patches can be
merged on commit (Julien already agreed to it.) They are separate only
to make them easier to review.


> > @@ -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;
> 
> Originally this was supposed to be bool, but didn't get switched yet.
> With your current scheme it can't go negative and hence should be
> unsigned int. However, I still wonder whether it wouldn't be better
> (especially for readers of the code) is this was an actual domid_t.
> But as clarified before, I'm not meaning to make this a requirement.

I'll use unsigned int


> > +struct domain *console_input_domain(void)
> > +{
> > +    return get_domain_by_id(console_rx - 1);
> > +}
> 
> And this is exactly the reason for the above remark: This is (or at
> least looks) broken for console_rx == 0.

I'll fix


> >  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;
> 
> Same here - the literal 2 at least raises questions. I think it would
> at least be a little less confusing if you had
> 
>     if ( console_rx++ == max_init_domid + 1 )
>         console_rx = 0;

I'll do


> >  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. 
> > */
> 
> Just like you adjust "guest" in this comment, I think you'd better ...
> 
> > +    if ( console_rx == 1 )
> > +    {
> > +        /* Deliver input to guest buffer, unless it is already full. */
> 
> ... adjust it here too.

I'll fix


> > +        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);
> > +
> > +        /*
> > +         * 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
> > +     * getting stuck.
> > +     */
> >      send_global_virq(VIRQ_CONSOLE);
> 
> Why does this not move into the if() body above?

That was a mistake, I fixed it


> > @@ -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;
> 
> According to the comment I think you need to store
> max_init_domid + 1 here, so that the switch_serial_input() a few
> lines down would actually switch to Xen.

I'll fix

Thanks for the comments!

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