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

[Stefano Stabellini <sstabellini@kernel.org>]

On Mon, 13 Aug 2018, Julien Grall wrote:
> 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@xilinx.com>
> > ---
> > 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);
> > +
> > +    vpl011->uartris |= TXI;
> > +    vpl011->uartfr &= ~TXFE;
> > +    vpl011_update_interrupt_status(d);
> > +
> > +    VPL011_UNLOCK(d, flags);
> > +}
> > +
> > +/*
> > + * vpl011_read_data_xen reads data when the backend is xen. Characters
> > + * are added to the vpl011 receive buffer by vpl011_rx_char_xen.
> > + */
> > +static uint8_t vpl011_read_data_xen(struct domain *d)
> > +{
> > +    unsigned long flags;
> > +    uint8_t data = 0;
> > +    struct vpl011 *vpl011 = &d->arch.vpl011;
> > +    struct vpl011_xen_backend *intf = vpl011->xen;
> > +    XENCONS_RING_IDX in_cons, in_prod;
> > +
> > +    VPL011_LOCK(d, flags);
> > +
> > +    in_cons = intf->in_cons;
> > +    in_prod = intf->in_prod;
> > +
> > +    smp_rmb();
> > +
> > +    /*
> > +     * It is expected that there will be data in the ring buffer when this
> > +     * function is called since the guest is expected to read the data
> > register
> > +     * only if the TXFE flag is not set.
> > +     * If the guest still does read when TXFE bit is set then 0 will be
> > returned.
> > +     */
> > +    if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
> > +    {
> > +        unsigned int fifo_level;
> > +
> > +        data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
> > +        in_cons += 1;
> > +        smp_mb();
> > +        intf->in_cons = in_cons;
> > +
> > +        fifo_level = xencons_queued(in_prod, in_cons, sizeof(intf->in));
> > +
> > +        /* If the FIFO is now empty, we clear the receive timeout
> > interrupt. */
> > +        if ( fifo_level == 0 )
> > +        {
> > +            vpl011->uartfr |= RXFE;
> > +            vpl011->uartris &= ~RTI;
> > +        }
> > +
> > +        /* If the FIFO is more than half empty, we clear the RX interrupt.
> > */
> > +        if ( fifo_level < sizeof(intf->in) - SBSA_UART_FIFO_LEVEL )
> > +            vpl011->uartris &= ~RXI;
> > +
> > +        vpl011_update_interrupt_status(d);
> > +    }
> > +    else
> > +        gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
> > +
> > +    /*
> > +     * We have consumed a character or the FIFO was empty, so clear the
> > +     * "FIFO full" bit.
> > +     */
> > +    vpl011->uartfr &= ~RXFF;
> > +
> > +    VPL011_UNLOCK(d, flags);
> > +
> > +    return data;
> > +}
> > +
> >   static uint8_t vpl011_read_data(struct domain *d)
> >   {
> >       unsigned long flags;
> > @@ -240,7 +325,10 @@ static int vpl011_mmio_read(struct vcpu *v,
> >       case DR:
> >           if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> >   -        *r = vreg_reg32_extract(vpl011_read_data(d), info);
> > +        if ( vpl011->backend_in_domain )
> > +            *r = vreg_reg32_extract(vpl011_read_data(d), info);
> > +        else
> > +            *r = vreg_reg32_extract(vpl011_read_data_xen(d), info);
> >           return 1;
> >         case RSR:
> > @@ -325,7 +413,10 @@ static int vpl011_mmio_write(struct vcpu *v,
> >             vreg_reg32_update(&data, r, info);
> >           data &= 0xFF;
> > -        vpl011_write_data(v->domain, data);
> > +        if ( vpl011->backend_in_domain )
> > +            vpl011_write_data(v->domain, data);
> > +        else
> > +            vpl011_write_data_xen(v->domain, data);
> >           return 1;
> >       }
> >   @@ -430,6 +521,39 @@ static void vpl011_data_avail(struct domain *d,
> >           vpl011->uartfr |= TXFE;
> >   }
> >   +/*
> > + * vpl011_rx_char_xen adds a char to a domain's vpl011 receive buffer.
> > + * It is only used when the vpl011 backend is in Xen.
> > + */
> > +void vpl011_rx_char_xen(struct domain *d, char c)
> > +{
> > +    unsigned long flags;
> > +    struct vpl011 *vpl011 = &d->arch.vpl011;
> > +    struct vpl011_xen_backend *intf = vpl011->xen;
> > +    XENCONS_RING_IDX in_cons, in_prod, in_fifo_level;
> > +
> > +    ASSERT(!vpl011->backend_in_domain);
> > +    VPL011_LOCK(d, flags);
> > +
> > +    in_cons = intf->in_cons;
> > +    in_prod = intf->in_prod;
> > +    if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) ==
> > sizeof(intf->in) )
> > +    {
> > +        VPL011_UNLOCK(d, flags);
> > +        return;
> > +    }
> > +
> > +    intf->in[xencons_mask(in_prod, sizeof(intf->in))] = c;
> > +    intf->in_prod = ++in_prod;
> > +
> > +    in_fifo_level = xencons_queued(in_prod,
> > +                                   in_cons,
> > +                                   sizeof(intf->in));
> > +
> > +    vpl011_data_avail(d, in_fifo_level, sizeof(intf->in), 0,
> > SBSA_UART_FIFO_SIZE);
> > +    VPL011_UNLOCK(d, flags);
> > +}
> > +
> >   static void vpl011_notification(struct vcpu *v, unsigned int port)
> >   {
> >       unsigned long flags;
> > @@ -470,27 +594,47 @@ int domain_vpl011_init(struct domain *d, struct
> > vpl011_init_info *info)
> >       if ( vpl011->dom.ring_buf )
> >           return -EINVAL;
> >   -    /* Map the guest PFN to Xen address space. */
> > -    rc =  prepare_ring_for_helper(d,
> > -                                  gfn_x(info->gfn),
> > -                                  &vpl011->dom.ring_page,
> > -                                  &vpl011->dom.ring_buf);
> > -    if ( rc < 0 )
> > -        goto out;
> > +    /*
> > +     * info is NULL when the backend is in Xen.
> > +     * info is != NULL when the backend is in a domain.
> > +     */
> > +    if ( info != NULL )
> > +    {
> > +        vpl011->backend_in_domain = true;
> > +
> > +        /* Map the guest PFN to Xen address space. */
> > +        rc =  prepare_ring_for_helper(d,
> > +                gfn_x(info->gfn),
> > +                &vpl011->dom.ring_page,
> > +                &vpl011->dom.ring_buf);
> 
> The indentation looks wrong here.

I'll fix

> > +        if ( rc < 0 )
> > +            goto out;
> > +
> > +        rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid,
> > +                vpl011_notification);
> 
> The indentation looks wrong here.

Here too

> > +        if ( rc < 0 )
> > +            goto out1;
> > +
> > +        vpl011->evtchn = info->evtchn = rc;
> > +    }
> > +    else
> > +    {
> > +        vpl011->backend_in_domain = false;
> > +
> > +        vpl011->xen = xzalloc(struct vpl011_xen_backend);
> > +        if ( vpl011->xen == NULL )
> > +        {
> > +            rc = -EINVAL;
> > +            goto out1;
> > +        }
> > +    }
> >         rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
> >       if ( !rc )
> >       {
> >           rc = -EINVAL;
> > -        goto out1;
> > -    }
> > -
> > -    rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid,
> > -                                         vpl011_notification);
> > -    if ( rc < 0 )
> >           goto out2;
> > -
> > -    vpl011->evtchn = info->evtchn = rc;
> > +    }
> >         spin_lock_init(&vpl011->lock);
> >   @@ -503,7 +647,10 @@ out2:
> >       vgic_free_virq(d, GUEST_VPL011_SPI);
> >     out1:
> > -    destroy_ring_for_helper(&vpl011->dom.ring_buf, vpl011->dom.ring_page);
> > +    if ( vpl011->backend_in_domain )
> > +        destroy_ring_for_helper(&vpl011->dom.ring_buf,
> > vpl011->dom.ring_page);
> > +    else
> > +        xfree(vpl011->xen);
> >     out:
> >       return rc;
> > @@ -513,11 +660,18 @@ void domain_vpl011_deinit(struct domain *d)
> >   {
> >       struct vpl011 *vpl011 = &d->arch.vpl011;
> >   -    if ( !vpl011->dom.ring_buf )
> > -        return;
> > +    if ( vpl011->backend_in_domain )
> > +    {
> > +        if ( !vpl011->dom.ring_buf )
> > +            return;
> >   -    free_xen_event_channel(d, vpl011->evtchn);
> > -    destroy_ring_for_helper(&vpl011->dom.ring_buf, vpl011->dom.ring_page);
> > +        free_xen_event_channel(d, vpl011->evtchn);
> > +        destroy_ring_for_helper(&vpl011->dom.ring_buf,
> > vpl011->dom.ring_page);
> > +    }
> > +    else
> > +    {
> > +        xfree(vpl011->xen);
> > +    }
> 
> NIT: The { } is not necessary for one line.

OK

> >   }
> >     /*
> > diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> > index b873a29..c9918c1 100644
> > --- a/xen/include/asm-arm/vpl011.h
> > +++ b/xen/include/asm-arm/vpl011.h
> > @@ -21,6 +21,7 @@
> >     #include <public/domctl.h>
> >   #include <public/io/ring.h>
> > +#include <public/io/console.h>
> >   #include <asm/vreg.h>
> >   #include <xen/mm.h>
> >   @@ -29,13 +30,19 @@
> >   #define VPL011_UNLOCK(d,flags)
> > spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
> >     #define SBSA_UART_FIFO_SIZE 32
> > +struct vpl011_xen_backend {
> > +    char in[SBSA_UART_FIFO_SIZE];
> > +    XENCONS_RING_IDX in_cons, in_prod;
> > +};
> >     struct vpl011 {
> > +    bool backend_in_domain;
> >       union {
> >           struct {
> >               void *ring_buf;
> >               struct page_info *ring_page;
> >           } dom;
> > +        struct vpl011_xen_backend *xen;
> >       };
> >       uint32_t    uartfr;         /* Flag register */
> >       uint32_t    uartcr;         /* Control register */
> > @@ -57,6 +64,7 @@ struct vpl011_init_info {
> >   int domain_vpl011_init(struct domain *d,
> >                          struct vpl011_init_info *info);
> >   void domain_vpl011_deinit(struct domain *d);
> > +void vpl011_rx_char_xen(struct domain *d, char c);
> >   #else
> >   static inline int domain_vpl011_init(struct domain *d,
> >                                        struct vpl011_init_info *info)
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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