Re: [PATCH v2 18/21] xen/arm: Allow vpl011 to be used by DomU

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 07/07/18 00:12, 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_date and write_data functions for
> initial domains: vpl011_write_data_noring is very simple and just writes
> to the console, while vpl011_read_data_inring is a duplicate of
> vpl011_read_data. Although textually almost identical, we are forced to
> duplicate the functions because the struct layout is different.

Looking at the patches applied, I think there need some more comments in 
the code to help a reader differentiate which method is used.

> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> 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  |  10 ++-
>   xen/arch/arm/vpl011.c        | 185 ++++++++++++++++++++++++++++++++++++++-----
>   xen/include/asm-arm/vpl011.h |  10 +++
>   3 files changed, 182 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 718be48..d7e9040 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2531,7 +2531,15 @@ 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;
> +
> +#ifdef CONFIG_SBSA_VUART_CONSOLE
> +    if ( vpl011 )
> +        rc = domain_vpl011_init(d, NULL);
> +#endif
I don't think the #ifdef is necessary here. We want to return an error 
when vpl011 is set but not the emulation compiled in.

> +    return rc;
>   }
>   
>   int __init construct_dom0(struct domain *d)
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index e75957f..d4aab64 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -83,6 +83,111 @@ static void vpl011_update_interrupt_status(struct domain *d)
>   #endif
>   }
>   
> +void vpl011_rx_char(struct domain *d, char c)

Please add documentation explain what the use of this function. I would 
also rename it to clarify who can call it (i.e only in the case when the 
backend is in Xen).

> +{
> +    unsigned long flags;
> +    struct vpl011 *vpl011 = &d->arch.vpl011;
> +    struct xencons_in *intf = vpl011->inring;
> +    XENCONS_RING_IDX in_cons, in_prod, in_fifo_level;
> +

ASSERT(!vpl011->ring_enable);

> +    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 + 1;
> +
> +    in_fifo_level = xencons_queued(in_prod,
> +                                   in_cons,
> +                                   sizeof(intf->in));
> +
> +    vpl011_data_avail(d, in_fifo_level, sizeof(intf->in), 0, 1024);

Where does the 1024 come from? Most likely, you want a define for it.

> +    VPL011_UNLOCK(d, flags);
> +}
> +
> +static void vpl011_write_data_noring(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);

You want to buffer the characters here and only print on newline or when 
the buffer is full. Otherwise characters will get mangled with the Xen 
console output or other domains output.

> +
> +    vpl011->uartris |= TXI;
> +    vpl011->uartfr &= ~TXFE;
> +    vpl011_update_interrupt_status(d);
> +
> +    VPL011_UNLOCK(d, flags);
> +}
> +
> +static uint8_t vpl011_read_data_inring(struct domain *d)
> +{

I think you can avoid the duplication here by using a macro.

> +    unsigned long flags;
> +    uint8_t data = 0;
> +    struct vpl011 *vpl011 = &d->arch.vpl011;
> +    struct xencons_in *intf = vpl011->inring;
> +    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;
> @@ -246,7 +351,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->ring_enable )
> +            *r = vreg_reg32_extract(vpl011_read_data(d), info);
> +        else
> +            *r = vreg_reg32_extract(vpl011_read_data_inring(d), info);

I think some renaming will improve the reading. This is quite 
unintuitive to see a function with "ring" in it, called when 
!vpl011->ring_enabled.

>           return 1;
>   
>       case RSR:
> @@ -331,7 +439,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->ring_enable )
> +            vpl011_write_data(v->domain, data);
> +        else
> +            vpl011_write_data_noring(v->domain, data);
>           return 1;
>       }
>   
> @@ -476,27 +587,47 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>       if ( vpl011->ring.ring_buf )
>           return -EINVAL;
>   
> -    /* Map the guest PFN to Xen address space. */
> -    rc =  prepare_ring_for_helper(d,
> -                                  gfn_x(info->gfn),
> -                                  &vpl011->ring.ring_page,
> -                                  &vpl011->ring.ring_buf);
> -    if ( rc < 0 )
> -        goto out;
> +    /*
> +     * info is NULL for domUs started by Xen at boot time, with no
> +     * corresponding userspace component in dom0 > +     */

I don't think you want to mention domUs at all here. The emulation 
should not care whether it is a guest or the hardware domain. It would 
theoretically possible to have the hardware domain re-using this code.

Furthermore, nothing in the emulation mandates to have the backend in
the hardware domain. This could be anywhere.

It looks like to me you want to offer 2 solutions:
	1) Console backend in a domain
	2) Console backend in the hypervisor

So probably a better naming for ring_enable would be "backend_in_domain" 
(or something similar).

> +    if ( info != NULL )
> +    {
> +        vpl011->ring_enable = true;
> +
> +        /* Map the guest PFN to Xen address space. */
> +        rc =  prepare_ring_for_helper(d,
> +                gfn_x(info->gfn),
> +                &vpl011->ring.ring_page,
> +                &vpl011->ring.ring_buf);
> +        if ( rc < 0 )
> +            goto out;
> +
> +        rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid,
> +                vpl011_notification);
> +        if ( rc < 0 )
> +            goto out1;
> +
> +        vpl011->evtchn = info->evtchn = rc;
> +    }
> +    else
> +    {
> +        vpl011->ring_enable = false;
> +
> +        vpl011->inring = xzalloc(struct xencons_in);
> +        if ( vpl011->inring == 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);
>   
> @@ -509,7 +640,10 @@ out2:
>       vgic_free_virq(d, GUEST_VPL011_SPI);
>   
>   out1:
> -    destroy_ring_for_helper(&vpl011->ring.ring_buf, vpl011->ring.ring_page);
> +    if ( vpl011->ring_enable )
> +        destroy_ring_for_helper(&vpl011->ring.ring_buf, vpl011->ring.ring_page);
> +    else
> +        xfree(vpl011->inring);
>   
>   out:
>       return rc;
> @@ -519,11 +653,18 @@ void domain_vpl011_deinit(struct domain *d)
>   {
>       struct vpl011 *vpl011 = &d->arch.vpl011;
>   
> -    if ( !vpl011->ring.ring_buf )
> -        return;
> +    if ( vpl011->ring_enable )
> +    {
> +        if ( !vpl011->ring.ring_buf )
> +            return;
>   
> -    free_xen_event_channel(d, vpl011->evtchn);
> -    destroy_ring_for_helper(&vpl011->ring.ring_buf, vpl011->ring.ring_page);
> +        free_xen_event_channel(d, vpl011->evtchn);
> +        destroy_ring_for_helper(&vpl011->ring.ring_buf, vpl011->ring.ring_page);
> +    }
> +    else
> +    {
> +        xfree(vpl011->inring);
> +    }
>   }
>   
>   /*
> diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> index c3d375b..be43abf 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>
>   
> @@ -30,12 +31,19 @@
>   
>   #define SBSA_UART_FIFO_SIZE 32
>   
> +struct xencons_in {

The name is too close to xencons_interface and I don't think the name is 
correct for the header it lives.

It probably should be vpl011_xen_backend or something similar.

> +    char in[1024];
> +    XENCONS_RING_IDX in_cons, in_prod;
> +};
> +
>   struct vpl011 {
> +    bool ring_enable;
>       union {
>           struct {
>               void *ring_buf;
>               struct page_info *ring_page;
>           } ring;
> +        struct xencons_in *inring;

The code is quite confusing. You have a field "ring_enabled" to know 
which bits of the union is used. But both name have "ring" in it.

You most likely want to rework the naming. If you follow my suggestion 
above it would be

union
{
     struct {
     } dom;
     struct vpl011_xen_backend xen;
} backend;

The different helpers would then need to be renamed accordingly.


>       };
>       uint32_t    uartfr;         /* Flag register */
>       uint32_t    uartcr;         /* Control register */
> @@ -57,6 +65,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(struct domain *d, char c);
>   #else
>   static inline int domain_vpl011_init(struct domain *d,
>                                        struct vpl011_init_info *info)
> @@ -65,6 +74,7 @@ static inline int domain_vpl011_init(struct domain *d,
>   }
>   
>   static inline void domain_vpl011_deinit(struct domain *d) { }
> +static inline void vpl011_rx_char(struct domain *d, char c) { }

I don't think this is necessary. IIRC, you already ifdef the caller.

>   #endif
>   #endif  /* _VPL011_H_ */
>   
> 

Cheers,

-- 
Julien Grall

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