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

[Julien Grall <julien.grall@xxxxxxx>]

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@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.
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.
Cheers,

> > > @@ -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!
--
Julien Grall

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