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

[Stefano Stabellini <sstabellini@kernel.org>]

On Fri, 5 Oct 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 10/04/2018 10:52 PM, Stefano Stabellini wrote:
> > On Wed, 1 Aug 2018, Jan Beulich wrote:
> > > > > > On 01.08.18 at 01:28, <sstabellini@kernel.org> 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.
> 
> Which two patches? I agreed to merge #24 and #22. Not #23. Merging the 3 of
> them is going to make a massive patch which is not going to help understand
> patches after they get merged.

No, you are right, I got confused. That's correct #22 and #24 are the
ones to be merged, I'll add a note about this to the patches. Sorry
about that.


> > 
> > > > @@ -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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel