Re: [Xen-devel] [PATCH v3 22/25] xen/arm: Allow vpl011 to be used by DomU

[Julien Grall <julien.grall@xxxxxxx>]

On 03/10/2018 22:21, Stefano Stabellini wrote:
> On Wed, 22 Aug 2018, Julien Grall wrote:
> > On 16/08/18 20:21, Stefano Stabellini wrote:
> > > On Mon, 13 Aug 2018, Julien Grall wrote:
> > > > Hi Stefano,
> > > >
> > > > On 01/08/18 00:28, Stefano Stabellini wrote:
> > > > > Make vpl011 being able to be used without a userspace component in Dom0.
> > > > > In that case, output is printed to the Xen serial and input is received
> > > > > from the Xen serial one character at a time.
> > > > >
> > > > > Call domain_vpl011_init during construct_domU if vpl011 is enabled.
> > > > >
> > > > > Introduce a new ring struct with only the ring array to avoid a waste of
> > > > > memory. Introduce separate read_data and write_data functions for
> > > > > initial domains: vpl011_write_data_xen is very simple and just writes
> > > > > to the console, while vpl011_read_data_xen is a duplicate of
> > > > > vpl011_read_data. Although textually almost identical, we are forced to
> > > > > duplicate the functions because the struct layout is different.
> > > > >
> > > > > Output characters are printed one by one, potentially leading to
> > > > > intermixed output of different domains on the console. A follow-up patch
> > > > > will solve the issue by introducing buffering.
> > > > >
> > > > > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > > > > ---
> > > > > Changes in v3:
> > > > > - add in-code comments
> > > > > - improve existing comments
> > > > > - remove ifdef around domain_vpl011_init in construct_domU
> > > > > - add ASSERT
> > > > > - use SBSA_UART_FIFO_SIZE for in buffer size
> > > > > - rename ring_enable to backend_in_domain
> > > > > - rename struct xencons_in to struct vpl011_xen_backend
> > > > > - rename inring field to xen
> > > > > - rename helper functions accordingly
> > > > > - remove unnecessary stub implementation of vpl011_rx_char
> > > > > - move vpl011_rx_char_xen within the file to avoid the need of a forward
> > > > >      declaration of vpl011_data_avail
> > > > > - fix small bug in vpl011_rx_char_xen: increment in_prod before using it
> > > > >      to check xencons_queued.
> > > > >
> > > > > Changes in v2:
> > > > > - only init if vpl011
> > > > > - rename vpl011_read_char to vpl011_rx_char
> > > > > - remove spurious change
> > > > > - fix coding style
> > > > > - use different ring struct
> > > > > - move the write_data changes to their own function
> > > > >      (vpl011_write_data_noring)
> > > > > - duplicate vpl011_read_data
> > > > > ---
> > > > >     xen/arch/arm/domain_build.c  |   9 +-
> > > > >     xen/arch/arm/vpl011.c        | 198
> > > > > ++++++++++++++++++++++++++++++++++++++-----
> > > > >     xen/include/asm-arm/vpl011.h |   8 ++
> > > > >     3 files changed, 192 insertions(+), 23 deletions(-)
> > > > >
> > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > > > index f9fa484..0888a76 100644
> > > > > --- a/xen/arch/arm/domain_build.c
> > > > > +++ b/xen/arch/arm/domain_build.c
> > > > > @@ -2638,7 +2638,14 @@ static int __init construct_domU(struct domain
> > > > > *d,
> > > > > struct dt_device_node *node)
> > > > >         if ( rc < 0 )
> > > > >             return rc;
> > > > >     -    return __construct_domain(d, &kinfo);
> > > > > +    rc = __construct_domain(d, &kinfo);
> > > > > +    if ( rc < 0 )
> > > > > +        return rc;
> > > > > +
> > > > > +    if ( kinfo.vpl011 )
> > > > > +        rc = domain_vpl011_init(d, NULL);
> > > > > +
> > > > > +    return rc;
> > > > >     }
> > > > >       void __init create_domUs(void)
> > > > > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> > > > > index 725a203..f206c61 100644
> > > > > --- a/xen/arch/arm/vpl011.c
> > > > > +++ b/xen/arch/arm/vpl011.c
> > > > > @@ -77,6 +77,91 @@ static void vpl011_update_interrupt_status(struct
> > > > > domain
> > > > > *d)
> > > > >     #endif
> > > > >     }
> > > > >     +/*
> > > > > + * vpl011_write_data_xen writes chars from the vpl011 out buffer to the
> > > > > + * console. Only to be used when the backend is Xen.
> > > > > + */
> > > > > +static void vpl011_write_data_xen(struct domain *d, uint8_t data)
> > > > > +{
> > > > > +    unsigned long flags;
> > > > > +    struct vpl011 *vpl011 = &d->arch.vpl011;
> > > > > +
> > > > > +    VPL011_LOCK(d, flags);
> > > > > +
> > > > > +    printk("%c", data);
> > > > > +    if (data == '\n')
> > > > > +        printk("DOM%u: ", d->domain_id);
> > > > There are a problem in this code. The first line of a domain will always
> > > > printed without "DOM%u: " in front. This means you don't really know where
> > > > it
> > > > is coming from until you get the second line.
> > > This problem is solved by the follow-up patch that introduces characters
> > > buffering. I'll mention it in the commit message.
> > To be honest, this should be solved in this patch and not the follow-up one.
> I agree. I kept them separate to make them easier to review. Would you
> be OK with me merging the two patches into one once they have both been
> acked? Otherwise, if you prefer that I merge them now, let me know.
I am ok to have them merged when committing.

Cheers,

--
Julien Grall

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