[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi, Stefano

On Tue, Oct 23, 2018 at 5:04 AM Stefano Stabellini
<sstabellini@xxxxxxxxxx> 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>
> Acked-by: Julien Grall <julien.grall@xxxxxxx>
> ---
> Changes in v5:
> - renable call to vpl011_rx_char_xen from console.c
>
> Changes in v4:
> - code style
>
> 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        | 200 
> +++++++++++++++++++++++++++++++++++++------
>  xen/drivers/char/console.c   |   2 +-
>  xen/include/asm-arm/vpl011.h |   8 ++
>  4 files changed, 193 insertions(+), 26 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 6026b77..9ffd919 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2629,7 +2629,14 @@ static int __init construct_domU(struct domain *d,
>      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 68452a8..131507e 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->backend.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->backend.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;
Do we need barriers here and after writing the ring?

> +    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->backend.dom.ring_buf )
>          return -EINVAL;
>
> -    /* Map the guest PFN to Xen address space. */
> -    rc =  prepare_ring_for_helper(d,
> -                                  gfn_x(info->gfn),
> -                                  &vpl011->backend.dom.ring_page,
> -                                  &vpl011->backend.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->backend.dom.ring_page,
> +                                      &vpl011->backend.dom.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->backend_in_domain = false;
> +
> +        vpl011->backend.xen = xzalloc(struct vpl011_xen_backend);
> +        if ( vpl011->backend.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,8 +647,11 @@ out2:
>      vgic_free_virq(d, GUEST_VPL011_SPI);
>
>  out1:
> -    destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
> -                                       vpl011->backend.dom.ring_page);
> +    if ( vpl011->backend_in_domain )
> +        destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
> +                                vpl011->backend.dom.ring_page);
> +    else
> +        xfree(vpl011->backend.xen);
>
>  out:
>      return rc;
> @@ -514,12 +661,17 @@ void domain_vpl011_deinit(struct domain *d)
>  {
>      struct vpl011 *vpl011 = &d->arch.vpl011;
>
> -    if ( !vpl011->backend.dom.ring_buf )
> -        return;
> +    if ( vpl011->backend_in_domain )
> +    {
> +        if ( !vpl011->backend.dom.ring_buf )
> +            return;
>
> -    free_xen_event_channel(d, vpl011->evtchn);
> -    destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
> -                                       vpl011->backend.dom.ring_page);
> +        free_xen_event_channel(d, vpl011->evtchn);
> +        destroy_ring_for_helper(&vpl011->backend.dom.ring_buf,
> +                                vpl011->backend.dom.ring_page);
> +    }
> +    else
> +        xfree(vpl011->backend.xen);
>  }
>
>  /*
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 75172e7..990c51c 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -444,7 +444,7 @@ static void __serial_rx(char c, struct cpu_user_regs 
> *regs)
>          send_global_virq(VIRQ_CONSOLE);
>          break;
>      }
> -#if 0
> +#ifdef CONFIG_SBSA_VUART_CONSOLE
>      default:
>      {
>          struct domain *d = rcu_lock_domain_by_any_id(console_rx - 1);
> diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> index 42d7a24..5eb6d25 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;
>      } backend;
>      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)
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel



-- 
Regards,

Oleksandr Tyshchenko

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.