Re: [Xen-devel] [PATCH 03/12 v3] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 10 May 2017, Bhupinder Thakur wrote:
> Add emulation code to emulate read/write access to pl011 registers
> and pl011 interrupts:
> 
>     - Emulate DR read/write by reading and writing from/to the IN
>       and OUT ring buffers and raising an event to the backend when
>       there is data in the OUT ring buffer and injecting an interrupt
>       to the guest when there is data in the IN ring buffer
> 
>     - Other registers are related to interrupt management and
>       essentially control when interrupts are delivered to the guest
> 
> The SBSA compliant pl011 uart is covered in Appendix B of
> https://static.docs.arm.com/den0029/a/Server_Base_System_Architecture_v3_1_ARM_DEN_0029A.pdf
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
> ---
> 
> Changes since v2:
> 
> - Use generic vreg_reg* for read/write of registers emulating pl011.
> - Use generic ring buffer functions defined using DEFINE_XEN_FLEX_RING.
> - Renamed the SPI injection function to vpl011_update_spi() to reflect level 
>   triggered nature of pl011 interrupts.
> - The pl011 register access address should always be the base address of the
>   corresponding register as per section B of the SBSA document. For this 
> reason,
>   the register range address access is not allowed.
> 
> Changes since v1:
> 
> - Removed the optimiztion related to sendiing events to xenconsole 
> - Use local variables as ring buffer indices while using the ring buffer
> 
>  xen/arch/arm/Kconfig             |   5 +
>  xen/arch/arm/Makefile            |   1 +
>  xen/arch/arm/vpl011.c            | 350 
> +++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/domain.h     |   6 +
>  xen/include/asm-arm/pl011-uart.h |   2 +
>  xen/include/asm-arm/vpl011.h     |  94 +++++++++++
>  xen/include/public/arch-arm.h    |   8 +
>  7 files changed, 466 insertions(+)
>  create mode 100644 xen/arch/arm/vpl011.c
>  create mode 100644 xen/include/asm-arm/vpl011.h
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index d46b98c..c1a0e7f 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -50,6 +50,11 @@ config HAS_ITS
>          prompt "GICv3 ITS MSI controller support" if EXPERT = "y"
>          depends on HAS_GICV3
>  
> +config VPL011_CONSOLE
> +     bool "Emulated pl011 console support"
> +     default y
> +     ---help---
> +       Allows a guest to use pl011 UART as a console
>  endmenu
>  
>  menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 49e1fb2..15efc13 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -52,6 +52,7 @@ obj-y += vm_event.o
>  obj-y += vtimer.o
>  obj-y += vpsci.o
>  obj-y += vuart.o
> +obj-$(CONFIG_VPL011_CONSOLE) += vpl011.o
>  
>  #obj-bin-y += ....o
>  
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> new file mode 100644
> index 0000000..2f71148
> --- /dev/null
> +++ b/xen/arch/arm/vpl011.c
> @@ -0,0 +1,350 @@
> +/*
> + * arch/arm/vpl011.c
> + *
> + * Virtual PL011 UART
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/errno.h>
> +#include <xen/event.h>
> +#include <xen/guest_access.h>
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/sched.h>
> +#include <public/io/console.h>
> +#include <asm-arm/pl011-uart.h>
> +#include <asm-arm/vgic-emul.h>
> +#include <asm-arm/vpl011.h>
> +
> +static bool vpl011_reg32_check_access(int size)
> +{
> +    return (size == DABT_DOUBLE_WORD)? false : true;
> +}
> +
> +static void vpl011_update_spi(struct domain *d)
> +{
> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
> +
> +    if ( vpl011->uartris & vpl011->uartimsc )
> +        vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
> +}
> +
> +static void vpl011_read_data(struct domain *d, uint8_t *data)
> +{
> +    unsigned long flags;
> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
> +    struct xencons_interface *intf = vpl011->ring_buf;
> +
> +    /*
> +     * Initialize the data so that even if there is no data in ring buffer
> +     * 0 is returned.
> +     */
> +    *data = 0;
> +
> +    VPL011_LOCK(d, flags);
> +
> +    /*
> +     * 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 ( !VPL011_IN_RING_EMPTY(intf) )

Similar to vpl011_data_avail, we need to read the ring indexes only once
to local variables to void dealing with thier runtime changes. Something
like:

    in_prod = intf->in_prod;
    in_cons = intf->in_cons;
    smb_rmb();

    if ( vpl011_queued(in_prod, in_cons, VPL011_RING_MAX_DEPTH(intf, in)) > 0 ) 
{
        *data = intf->in[VPL011_RING_IDX_MASK(in_cons, intf->in)];
        in_cons += 1;
        intf->in_cons = in_cons;
        smp_mb();
    }

    if ( vpl011_queued(in_prod, in_cons, VPL011_RING_MAX_DEPTH(intf, in)) == 0 
) {

    <rest of the code>


> +    {
> +        XENCONS_RING_IDX in_cons = intf->in_cons;
> +        *data = intf->in[VPL011_RING_IDX_MASK(in_cons, intf->in)];
> +        smp_mb();
> +        intf->in_cons = in_cons + 1;
> +    }
> +
> +    if ( VPL011_IN_RING_EMPTY(intf) )
> +    {
> +        vpl011->uartfr |= RXFE;
> +        vpl011->uartris &= ~RXI;
> +    }
> +    vpl011->uartfr &= ~RXFF;
> +    VPL011_UNLOCK(d, flags);
> +
> +    notify_via_xen_event_channel(d, vpl011->evtchn);
> +}
> +
> +static void vpl011_write_data(struct domain *d, uint8_t data)
> +{
> +    unsigned long flags;
> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
> +    struct xencons_interface *intf = vpl011->ring_buf;
> +
> +    VPL011_LOCK(d, flags);
> +
> +    /*
> +     * It is expected that the ring is not full when this function is called
> +     * as the guest is expected to write to the data register only when the
> +     * TXFF flag is not set.
> +     * In case the guest does write even when the TXFF flag is set then the
> +     * data will be silently dropped.
> +     */
> +    if ( !VPL011_OUT_RING_FULL(intf) )

Same here:

    out_prod = intf->out_prod;
    out_cons = intf->out_cons;
    smb_mb();

    if ( vpl011_queued(out_prod, out_cons, VPL011_RING_MAX_DEPTH(intf, out)) !=
        VPL011_RING_MAX_DEPTH(intf,out) ) {
        intf->out[VPL011_RING_IDX_MASK(out_prod, intf->out)] = data;
        smp_wmb();
        out_prod += 1;
        intf->out_prod = out_prod;
    }
    if ( vpl011_queued(out_prod, out_cons, VPL011_RING_MAX_DEPTH(intf,out)) ==
        VPL011_RING_MAX_DEPTH(intf,out) ) {

    <rest of the code>


> +    {
> +        XENCONS_RING_IDX out_prod = intf->out_prod;
> +        intf->out[VPL011_RING_IDX_MASK(out_prod, intf->out)] = data;
> +        smp_wmb();
> +        intf->out_prod = out_prod + 1;
> +    }
> +
> +    if ( VPL011_OUT_RING_FULL(intf) )
> +    {
> +        vpl011->uartfr |= TXFF;
> +        vpl011->uartris &= ~TXI;
> +    }
> +
> +    vpl011->uartfr |= BUSY;
> +
> +    vpl011->uartfr &= ~TXFE;
> +
> +    VPL011_UNLOCK(d, flags);
> +
> +    notify_via_xen_event_channel(d, vpl011->evtchn);
> +}
> +
> +static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t 
> *r, void *priv)
> +{
> +    uint8_t ch;
> +    struct hsr_dabt dabt = info->dabt;
> +    int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
> +    struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
> +
> +    if ( !vpl011_reg32_check_access(dabt.size) ) goto bad_width;
> +
> +    switch ( vpl011_reg )
> +    {
> +    case DR:
> +        vpl011_read_data(v->domain, &ch);
> +        *r = ch;
> +        break;
> +
> +    case RSR:
> +        /* It always returns 0 as there are no physical errors. */
> +        *r = 0;
> +        break;
> +
> +    case FR:
> +        *r = vreg_reg32_extract(vpl011->uartfr, info);
> +        break;
> +
> +    case RIS:
> +        *r = vreg_reg32_extract(vpl011->uartris, info);
> +        break;
> +
> +    case MIS:
> +        *r = vreg_reg32_extract(vpl011->uartris
> +                                & vpl011->uartimsc, info);
> +        break;
> +
> +    case IMSC:
> +        *r = vreg_reg32_extract(vpl011->uartimsc, info);
> +        break;
> +
> +    case ICR:
> +        /* Only write is valid. */
> +        return 0;
> +
> +    default:
> +        gprintk(XENLOG_ERR, "vpl011: unhandled read r%d offset %#08x\n",
> +                dabt.reg, vpl011_reg);
> +        return 0;
> +    }
> +
> +    return 1;
> +
> +bad_width:
> +    gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
> +            dabt.size, dabt.reg, vpl011_reg);
> +    domain_crash_synchronous();
> +    return 0;
> +
> +}
> +
> +static int vpl011_mmio_write(struct vcpu *v, mmio_info_t *info, register_t 
> r, void *priv)
> +{
> +    uint8_t ch = ((struct uartdr_reg *)&r)->data;
> +    struct hsr_dabt dabt = info->dabt;
> +    int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
> +    struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
> +
> +    if ( !vpl011_reg32_check_access(dabt.size) ) goto bad_width;
> +
> +    switch ( vpl011_reg )
> +    {
> +    case DR:
> +        vpl011_write_data(v->domain, ch);
> +        break;
> +
> +    case RSR: /* Nothing to clear. */
> +        break;
> +
> +    case FR:
> +    case RIS:
> +    case MIS:
> +        goto write_ignore;
> +
> +    case IMSC:
> +        vreg_reg32_update(&vpl011->uartimsc, r, info);
> +        vpl011_update_spi(v->domain);
> +        break;
> +
> +    case ICR:
> +        vreg_reg32_clearbits(&vpl011->uartris, r, info);
> +        vpl011_update_spi(v->domain);
> +        break;
> +
> +    default:
> +        gprintk(XENLOG_ERR, "vpl011: unhandled write r%d offset %#08x\n",
> +                dabt.reg, vpl011_reg);
> +        return 0;
> +    }
> +
> +write_ignore:
> +    return 1;
> +
> +bad_width:
> +    gprintk(XENLOG_ERR, "vpl011: bad write width %d r%d offset %#08x\n",
> +            dabt.size, dabt.reg, vpl011_reg);
> +    domain_crash_synchronous();
> +    return 0;
> +
> +}
> +
> +static const struct mmio_handler_ops vpl011_mmio_handler = {
> +    .read = vpl011_mmio_read,
> +    .write = vpl011_mmio_write,
> +};
> +
> +int vpl011_map_guest_page(struct domain *d, xen_pfn_t gfn)
> +{
> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
> +
> +    /* Map the guest PFN to Xen address space. */
> +    return prepare_ring_for_helper(d,
> +                                   gfn,
> +                                   &vpl011->ring_page,
> +                                   &vpl011->ring_buf);
> +}
> +
> +static void vpl011_data_avail(struct domain *d)
> +{
> +    unsigned long flags;
> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
> +    struct xencons_interface *intf = vpl011->ring_buf;
> +    RING_IDX in_ring_depth, out_ring_depth;

Please use XENCONS_RING_IDX consistently.


> +    VPL011_LOCK(d, flags);
> +
> +    in_ring_depth = vpl011_queued(intf->in_prod, intf->in_cons, 
> sizeof(intf->in));
> +    out_ring_depth = vpl011_queued(intf->out_prod, intf->out_cons, 
> sizeof(intf->out));

Watch out for long lines (> 80 chars), but I think the code is OK.



> +    /* Update the uart rx state if the buffer is not empty. */
> +    if ( in_ring_depth != 0 )
> +    {
> +        vpl011->uartfr &= ~RXFE;
> +        if ( in_ring_depth == VPL011_RING_MAX_DEPTH(intf, in) )
> +            vpl011->uartfr |= RXFF;
> +        vpl011->uartris |= RXI;
> +    }
> +
> +    /* Update the uart tx state if the buffer is not full. */
> +    if ( out_ring_depth != VPL011_RING_MAX_DEPTH(intf, out) )
> +    {
> +        vpl011->uartfr &= ~TXFF;
> +        vpl011->uartris |= TXI;
> +        if ( out_ring_depth == 0 )
> +        {
> +            vpl011->uartfr &= ~BUSY;
> +            vpl011->uartfr |= TXFE;
> +        }
> +    }
> +
> +    VPL011_UNLOCK(d, flags);
> +
> +    vpl011_update_spi(d);
> +}
> +
> +
> +static void vpl011_notification(struct vcpu *v, unsigned int port)
> +{
> +    vpl011_data_avail(v->domain);
> +}
> +
> +int domain_vpl011_init(struct domain *d,
> +                       uint32_t console_domid,
> +                       xen_pfn_t gfn,
> +                       evtchn_port_t *evtchn)
> +{
> +    int rc;
> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
> +
> +    rc = vpl011_map_guest_page(d, gfn);
> +    if ( rc < 0 )
> +        goto out;
> +
> +    rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
> +    if ( !rc )
> +    {
> +        rc = -EINVAL;
> +        goto out1;
> +    }
> +
> +    register_mmio_handler(d, &vpl011_mmio_handler,
> +                          GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
> +
> +    spin_lock_init(&vpl011->lock);
> +
> +    rc = alloc_unbound_xen_event_channel(d, 0, console_domid,
> +                                         vpl011_notification);
> +    if (rc < 0)
> +        goto out2;
> +
> +    vpl011->evtchn = *evtchn = rc;
> +
> +    vpl011->initialized = true;
> +
> +    return 0;
> +
> +out2:
> +    xfree(d->arch.vmmio.handlers);
> +out1:
> +    destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
> +out:
> +    return rc;
> +}
> +
> +void domain_vpl011_deinit(struct domain *d)
> +{
> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
> +
> +    if ( vpl011->initialized )
> +    {
> +        free_xen_event_channel(d, vpl011->evtchn);
> +        destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
> +    }
> +    vpl011->initialized = false;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 6de8082..54611e0 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -11,6 +11,7 @@
>  #include <public/hvm/params.h>
>  #include <xen/serial.h>
>  #include <xen/rbtree.h>
> +#include <asm-arm/vpl011.h>
>  
>  struct hvm_domain
>  {
> @@ -133,6 +134,11 @@ struct arch_domain
>      struct {
>          uint8_t privileged_call_enabled : 1;
>      } monitor;
> +
> +#ifdef CONFIG_VPL011_CONSOLE
> +    struct vpl011_s vpl011;
> +#endif
> +
>  }  __cacheline_aligned;
>  
>  struct arch_vcpu
> diff --git a/xen/include/asm-arm/pl011-uart.h 
> b/xen/include/asm-arm/pl011-uart.h
> index 123f477..57e9ec7 100644
> --- a/xen/include/asm-arm/pl011-uart.h
> +++ b/xen/include/asm-arm/pl011-uart.h
> @@ -49,6 +49,8 @@
>  /* FR bits */
>  #define TXFE   (1<<7) /* TX FIFO empty */
>  #define RXFE   (1<<4) /* RX FIFO empty */
> +#define TXFF   (1<<5) /* TX FIFO full */
> +#define RXFF   (1<<6) /* RX FIFO full */
>  #define BUSY   (1<<3) /* Transmit is not complete */
>  
>  /* LCR_H bits */
> diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> new file mode 100644
> index 0000000..df7e6b7
> --- /dev/null
> +++ b/xen/include/asm-arm/vpl011.h
> @@ -0,0 +1,94 @@
> +/*
> + * include/xen/vpl011.h
> + *
> + * Virtual PL011 UART
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef _VPL011_H_
> +
> +#define _VPL011_H_
> +
> +#include <public/io/ring.h>
> +#include <asm-arm/vreg.h>
> +
> +DEFINE_XEN_FLEX_RING(vpl011);
> +
> +/* helper macros */
> +#define VPL011_RING_IDX_MASK(idx, ring) (vpl011_mask(idx, sizeof(ring)))
> +
> +#define VPL011_RING_DEPTH(intf,dir) (vpl011_queued((intf)->dir ## _prod,    \
> +                                                   (intf)->dir ## _cons,    \
> +                                                   sizeof((intf)->dir)))
> +
> +#define VPL011_RING_MAX_DEPTH(intf,dir) (sizeof((intf)->dir))
> +
> +#define VPL011_IN_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, in) == 0)
> +
> +#define VPL011_OUT_RING_EMPTY(intf) (VPL011_RING_DEPTH(intf, out) == 0)
> +
> +#define VPL011_IN_RING_FULL(intf) (VPL011_RING_DEPTH(intf, in) == 
> VPL011_RING_MAX_DEPTH(intf, in))
> +
> +#define VPL011_OUT_RING_FULL(intf) (VPL011_RING_DEPTH(intf, out) == 
> VPL011_RING_MAX_DEPTH(intf,out))
> +
> +#define VPL011_LOCK(d,flags) spin_lock_irqsave(&(d)->arch.vpl011.lock, flags)
> +#define VPL011_UNLOCK(d,flags) 
> spin_unlock_irqrestore(&(d)->arch.vpl011.lock, flags)
> +
> +struct uartdr_reg {
> +    uint8_t data;
> +    uint8_t error_status:4;
> +    uint8_t reserved1:4;
> +    uint16_t reserved2;
> +    uint32_t reserved3;
> +};
> +
> +struct vpl011_s {
> +    void *ring_buf;
> +    struct page_info *ring_page;
> +    uint32_t    uartfr;     /* Flag register */
> +    uint32_t    uartcr;     /* Control register */
> +    uint32_t    uartimsc;   /* Interrupt mask register*/
> +    uint32_t    uarticr;    /* Interrupt clear register */
> +    uint32_t    uartris;    /* Raw interrupt status register */
> +    uint32_t    uartmis;    /* Masked interrupt register */
> +    spinlock_t  lock;
> +    evtchn_port_t evtchn;
> +    bool        initialized; /* Flag which tells whether vpl011 is 
> initialized */
> +};
> +
> +#ifdef CONFIG_VPL011_CONSOLE
> +int domain_vpl011_init(struct domain *d,
> +                       uint32_t console_domid,
> +                       xen_pfn_t gfn,
> +                       evtchn_port_t *evtchn);
> +void domain_vpl011_deinit(struct domain *d);
> +#else
> +int domain_vpl011_init(struct domain *d,
> +                       uint32_t console_domid,
> +                       xen_pfn_t gfn,
> +                       evtchn_port_t *evtchn) { return -ENOSYS; }
> +
> +static inline void domain_vpl011_deinit(struct domain *d) { }
> +#endif
> +
> +#endif
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index bd974fb..5f91207 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -322,6 +322,8 @@ struct xen_arch_domainconfig {
>       *
>       */
>      uint32_t clock_frequency;
> +
> +    uint32_t console_domid;
>  };
>  #endif /* __XEN__ || __XEN_TOOLS__ */
>  
> @@ -410,6 +412,10 @@ typedef uint64_t xen_callback_t;
>  #define GUEST_ACPI_BASE 0x20000000ULL
>  #define GUEST_ACPI_SIZE 0x02000000ULL
>  
> +/* PL011 mappings */
> +#define GUEST_PL011_BASE    0x22000000ULL
> +#define GUEST_PL011_SIZE    0x00001000ULL
> +
>  /*
>   * 16MB == 4096 pages reserved for guest to use as a region to map its
>   * grant table in.
> @@ -444,6 +450,8 @@ typedef uint64_t xen_callback_t;
>  #define GUEST_TIMER_PHYS_NS_PPI 30
>  #define GUEST_EVTCHN_PPI        31
>  
> +#define GUEST_VPL011_SPI        32
> +
>  /* PSCI functions */
>  #define PSCI_cpu_suspend 0
>  #define PSCI_cpu_off     1
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel