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

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@linaro.org>
---

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

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@linaro.org>
> ---
> 
> 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
>

Hi Bhupinder,

On 10/05/17 15:24, 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@linaro.org>
> ---
>
> 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)

Please pass hsr_dabt in parameter rather than the size directly. Which 
BTW should have really be unsigned int.

> +{
> +    return (size == DABT_DOUBLE_WORD)? false : true;

This could be simplified with (size != DABT_DOUBLE_WORD). Also please 
add a comment explaining why we allow all the sizes but 64-bit.

> +}
> +
> +static void vpl011_update_spi(struct domain *d)

Please rename this function to vpl011_update.

> +{
> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
> +
> +    if ( vpl011->uartris & vpl011->uartimsc )

Likely you want a todo to say we need to revisit that when the vGIC is 
handling properly level interrupt.

> +        vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
> +}
> +
> +static void vpl011_read_data(struct domain *d, uint8_t *data)

If a pointer is passed to be filled, then I would expect an error value 
to be returned.

I would usually pointer if the function has to fill a structure or it is 
not possible to use the return.

In this case, the value is 8-bit and you don't have return value. So 
return the value rather than passing a pointer.

> +{
> +    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) )
> +    {
> +        XENCONS_RING_IDX in_cons = intf->in_cons;

newline here please.

> +        *data = intf->in[VPL011_RING_IDX_MASK(in_cons, intf->in)];
> +        smp_mb();
> +        intf->in_cons = in_cons + 1;
> +    }

For debugging purpose, it would be good to have a message if the ring is 
empty.

> +
> +    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);

It would be good to explain why you need to notification here.

> +}
> +
> +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) )
> +    {
> +        XENCONS_RING_IDX out_prod = intf->out_prod;

newline here please.

> +        intf->out[VPL011_RING_IDX_MASK(out_prod, intf->out)] = data;
> +        smp_wmb();
> +        intf->out_prod = out_prod + 1;
> +    }

For debugging purpose, it would be useful to get a message if the ring 
is full.

> +
> +    if ( VPL011_OUT_RING_FULL(intf) )
> +    {
> +        vpl011->uartfr |= TXFF;
> +        vpl011->uartris &= ~TXI;
> +    }
> +
> +    vpl011->uartfr |= BUSY;

I am not sure to understand why you set the bit BUSY here.

> +
> +    vpl011->uartfr &= ~TXFE;
> +
> +    VPL011_UNLOCK(d, flags);
> +
> +    notify_via_xen_event_channel(d, vpl011->evtchn);

Same as the previous notify_*.

> +}
> +
> +static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t *r, void *priv)
> +{
> +    uint8_t ch;

This could be restricted to the case DR.

> +    struct hsr_dabt dabt = info->dabt;
> +    int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);

Please use either unsigned int or uint32_t. Something would be really 
wrong if the offset is negative here.

> +    struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
> +
> +    if ( !vpl011_reg32_check_access(dabt.size) ) goto bad_width;

Please add a comment why the check is the same for all registers.

> +
> +    switch ( vpl011_reg )
> +    {
> +    case DR:

As mentioned above, you could do:

{
      uint8_t ch;
      ....
}

> +        vpl011_read_data(v->domain, &ch);
> +        *r = ch;

Please use vreg_reg32_extract(...).

> +        break;

I admit I would prefer the "return 1;" here rather than "break;". This 
makes easier to follow the emulation for a given register.

I would even be in favor of duplicating the "if ( !vpl011... )" in each 
case for the same reason.

> +
> +    case RSR:
> +        /* It always returns 0 as there are no physical errors. */
> +        *r = 0;

Note that I am ok to keep the 0 like that here.

> +        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);

The coding style in Xen is to split after the &. E.g

vpl011->uartris &
vpl011->uartimsc

Also, none of the pl011 register emulation is using any locking. This is 
quite surprising to me. So can you explain why?

> +        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;

This should be restricted to the DR case below. But I am not sure to 
understand the purpose the structure here. It makes less readable and 
also you only use it for the write case.

> +    struct hsr_dabt dabt = info->dabt;
> +    int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);

Same as above for vpl011_reg.

> +    struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
> +
> +    if ( !vpl011_reg32_check_access(dabt.size) ) goto bad_width;

Same as the previous vpl011_reg32_check_access... and all of my remarks 
in the read emulation stand for the write one.

> +
> +    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);

I would have expected some locking here.

> +        break;
> +
> +    case ICR:
> +        vreg_reg32_clearbits(&vpl011->uartris, r, info);
> +        vpl011_update_spi(v->domain);

Same here.

> +        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)

This function should either have the prototype defined in an header if 
used outside or static.

Also, I have asked to use gfn_t and not xen_pfn_t. The former is a 
typesafe avoiding mix between MFN and 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);

Looking at the usage it is only used in domain_vpl011_init. So I am not 
convinced of the of usefulness of this wrapper.

> +}
> +
> +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;

I am a bit confused with the term "depth". It does not seem to fit for a 
ring.

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

You introduced a macro VPL011_RING_DEPTH for hiding vpl011_queued, but 
don't use it.

> +    out_ring_depth = vpl011_queued(intf->out_prod, intf->out_cons, sizeof(intf->out));

Ditto.

> +
> +    /* 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,

This should be gfn_t and not xen_pfn_t.

> +                       evtchn_port_t *evtchn)
> +{
> +    int rc;
> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
> +

What if domain_vpl011_init is called twice? After nothing seem to 
prevent the new DOMCTL to be called twice.

> +    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);

You register MMIO handler but never remove them. So if this call fail, 
you will end up with the handlers existing but the rest half-initialized 
which likely lead to a segfault, or worst leaking data.

> +
> +    spin_lock_init(&vpl011->lock);
> +
> +    rc = alloc_unbound_xen_event_channel(d, 0, console_domid,
> +                                         vpl011_notification);
> +    if (rc < 0)

Coding style:

if (  ... )

> +        goto out2;
> +
> +    vpl011->evtchn = *evtchn = rc;
> +
> +    vpl011->initialized = true;

I am not sure to understand the purpose of this variable. You only use 
it in domain_vpl011_deinit. But you can easily know if you need to free 
by looking at the state of ring_buf.

> +
> +    return 0;
> +
> +out2:
> +    xfree(d->arch.vmmio.handlers);
> +out1:

I would have expected vgic_free_virq to be called in case of error.

> +    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 )

See my remark about vpl011->initialized above.

In any case, I would prefer the condition to be inverted to avoid an 
extra layer of tab. I.e

if ( !vpl011->initialized )
    return;

> +    {
> +        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);

I am sure someone already said it in a previous version. The vpl011 is 
the console ring. So why are we defining our own internally?

At least this should have been used by xenconsole, but this is not the 
case... So we should really avoid defining our own ring here and re-use 
public/io/console.h.

> +
> +/* 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;
> +};

I don't see any benefits of this structure.

> +
> +struct vpl011_s {

Please remove the _s in the name.

> +    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,

static inline here.

> +                       uint32_t console_domid,
> +                       xen_pfn_t gfn,
> +                       evtchn_port_t *evtchn) { return -ENOSYS; }

Please indent correctly { return -ENOSYS };

> +
> +static inline void domain_vpl011_deinit(struct domain *d) { }

Note that I am ok with the { } on the same line here.

> +#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;

Why this is here?

>  };
>  #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
>

Cheers,

Hi Julien,

Thanks for your comments.

>
>
>> +static bool vpl011_reg32_check_access(int size)
>
>
> Please pass hsr_dabt in parameter rather than the size directly. Which BTW
> should have really be unsigned int.
>
ok.

>> +{
>> +    return (size == DABT_DOUBLE_WORD)? false : true;
>
>
> This could be simplified with (size != DABT_DOUBLE_WORD). Also please add a
> comment explaining why we allow all the sizes but 64-bit.
>
ok.

>> +}
>> +
>> +static void vpl011_update_spi(struct domain *d)
>
>
> Please rename this function to vpl011_update.
>
ok.
>> +{
>> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>> +    if ( vpl011->uartris & vpl011->uartimsc )
>
>
> Likely you want a todo to say we need to revisit that when the vGIC is
> handling properly level interrupt.
>
ok.

>> +        vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
>> +}
>> +
>> +static void vpl011_read_data(struct domain *d, uint8_t *data)
>
>
> If a pointer is passed to be filled, then I would expect an error value to
> be returned.
>
> I would usually pointer if the function has to fill a structure or it is not
> possible to use the return.
>
> In this case, the value is 8-bit and you don't have return value. So return
> the value rather than passing a pointer.
>
I will return the data.


>
>> +        *data = intf->in[VPL011_RING_IDX_MASK(in_cons, intf->in)];
>> +        smp_mb();
>> +        intf->in_cons = in_cons + 1;
>> +    }
>
>
> For debugging purpose, it would be good to have a message if the ring is
> empty.
ok.

>
>> +
>> +    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);
>
>
> It would be good to explain why you need to notification here.
>
On second thoughts, notification is not required here.


>
>> +        intf->out[VPL011_RING_IDX_MASK(out_prod, intf->out)] = data;
>> +        smp_wmb();
>> +        intf->out_prod = out_prod + 1;
>> +    }
>
>
> For debugging purpose, it would be useful to get a message if the ring is
> full.
>
ok.
>> +
>> +    if ( VPL011_OUT_RING_FULL(intf) )
>> +    {
>> +        vpl011->uartfr |= TXFF;
>> +        vpl011->uartris &= ~TXI;
>> +    }
>> +
>> +    vpl011->uartfr |= BUSY;
>
>
> I am not sure to understand why you set the bit BUSY here.

Since the OUT ring buffer is non-empty here, this bit is set to
indicate that there is data in the ring buffer (which is emulating a
UART transmit FIFO). As per pl011 TRM, this bit should be set if there
is data in the transmit fifo.

>
>> +
>> +    vpl011->uartfr &= ~TXFE;
>> +
>> +    VPL011_UNLOCK(d, flags);
>> +
>> +    notify_via_xen_event_channel(d, vpl011->evtchn);
>
>
> Same as the previous notify_*.
>
ok. I will add a comment on why we are sending an event here.

>> +}
>> +
>> +static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t
>> *r, void *priv)
>> +{
>> +    uint8_t ch;
>
>
> This could be restricted to the case DR.
>
ok.

>> +    struct hsr_dabt dabt = info->dabt;
>> +    int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
>
>
> Please use either unsigned int or uint32_t. Something would be really wrong
> if the offset is negative here.
ok.
>
>> +    struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
>> +
>> +    if ( !vpl011_reg32_check_access(dabt.size) ) goto bad_width;
>
>
> Please add a comment why the check is the same for all registers.
>
ok.

>> +
>> +    switch ( vpl011_reg )
>> +    {
>> +    case DR:
>
>
> As mentioned above, you could do:
>
> {
>      uint8_t ch;
>      ....
> }
>
>> +        vpl011_read_data(v->domain, &ch);
>> +        *r = ch;
>
>
> Please use vreg_reg32_extract(...).
>
ok.

>> +        break;
>
>
> I admit I would prefer the "return 1;" here rather than "break;". This makes
> easier to follow the emulation for a given register.
>
> I would even be in favor of duplicating the "if ( !vpl011... )" in each case
> for the same reason.
Do you mean that I should repeat the vpl011_reg32_check_access() and
return for each switch case?

>
>> +
>> +    case RSR:
>> +        /* It always returns 0 as there are no physical errors. */
>> +        *r = 0;
>
>
> Note that I am ok to keep the 0 like that here.
>
>> +        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);
>
>
> The coding style in Xen is to split after the &. E.g
>
> vpl011->uartris &
> vpl011->uartimsc
>
ok.

> Also, none of the pl011 register emulation is using any locking. This is
> quite surprising to me. So can you explain why?

In vpl011_mmio_read(), since pl011 registers are only read, it should
be ok to let
them be read without any locking.
>
>
>> +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;
>
>
> This should be restricted to the DR case below. But I am not sure to
> understand the purpose the structure here. It makes less readable and also
> you only use it for the write case.
There was a review comment earlier to make the typecasting more
readable because earlier I was doing r & 0xff to retrieve the data. So
I added a structure which tells the layout of the DR. I guess I can
use vreg_reg32_extract() to extract uint8_t value from register_t r.

>
>> +    struct hsr_dabt dabt = info->dabt;
>> +    int vpl011_reg = (int)(info->gpa - GUEST_PL011_BASE);
>
>
> Same as above for vpl011_reg.
ok.
>
>> +    struct vpl011_s *vpl011 = &v->domain->arch.vpl011;
>> +
>> +    if ( !vpl011_reg32_check_access(dabt.size) ) goto bad_width;
>
>
> Same as the previous vpl011_reg32_check_access... and all of my remarks in
> the read emulation stand for the write one.
>
>> +
>> +    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);
>
>
> I would have expected some locking here.
Yes here lock should be taken.

>
>> +        break;
>> +
>> +    case ICR:
>> +        vreg_reg32_clearbits(&vpl011->uartris, r, info);
>> +        vpl011_update_spi(v->domain);
>
>
> Same here.
Yes here lock should be taken.
>
>
>> +        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)
>
>
> This function should either have the prototype defined in an header if used
> outside or static.
>
> Also, I have asked to use gfn_t and not xen_pfn_t. The former is a typesafe
> avoiding mix between MFN and 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);
>
>
> Looking at the usage it is only used in domain_vpl011_init. So I am not
> convinced of the of usefulness of this wrapper.
>
Yes. I will remove the wrapper function.

>> +}
>> +
>> +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;
>
>
> I am a bit confused with the term "depth". It does not seem to fit for a
> ring.
I will use in_ring_qsize and out_ring_qsize instead.

>
>> +
>> +    VPL011_LOCK(d, flags);
>> +
>> +    in_ring_depth = vpl011_queued(intf->in_prod, intf->in_cons,
>> sizeof(intf->in));
>
>
> You introduced a macro VPL011_RING_DEPTH for hiding vpl011_queued, but don't
> use it.
I have removed this macro as much of ring buffer manipultation is
handled through vpl011_queued()
and vpl011_mask().

>
>> +    out_ring_depth = vpl011_queued(intf->out_prod, intf->out_cons,
>> sizeof(intf->out));
>
>
> Ditto.
>
>
>> +
>> +    /* 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,
>
>
> This should be gfn_t and not xen_pfn_t.
>
>> +                       evtchn_port_t *evtchn)
>> +{
>> +    int rc;
>> +    struct vpl011_s *vpl011 = &d->arch.vpl011;
>> +
>
>
> What if domain_vpl011_init is called twice? After nothing seem to prevent
> the new DOMCTL to be called twice.
>
I will add a check in the init function to return without doing
anything if vpl011 is already initialized.

>> +    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);
>
>
> You register MMIO handler but never remove them. So if this call fail, you
> will end up with the handlers existing but the rest half-initialized which
> likely lead to a segfault, or worst leaking data.
>
I will free the mmio handlers in the vpl011 deinit function.

>> +
>> +    spin_lock_init(&vpl011->lock);
>> +
>> +    rc = alloc_unbound_xen_event_channel(d, 0, console_domid,
>> +                                         vpl011_notification);
>> +    if (rc < 0)
>
>
> Coding style:
>
> if (  ... )
>
ok.
>> +        goto out2;
>> +
>> +    vpl011->evtchn = *evtchn = rc;
>> +
>> +    vpl011->initialized = true;
>
>
> I am not sure to understand the purpose of this variable. You only use it in
> domain_vpl011_deinit. But you can easily know if you need to free by looking
> at the state of ring_buf.
It is an extraneous variable. I will remove it.

>
>> +
>> +    return 0;
>> +
>> +out2:
>> +    xfree(d->arch.vmmio.handlers);
>> +out1:
>
>
> I would have expected vgic_free_virq to be called in case of error.
>
ok.

>> +    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 )
>
>
> See my remark about vpl011->initialized above.
Yes, I will use the if ( vpl011->ring_buf ) check instead.
>
> In any case, I would prefer the condition to be inverted to avoid an extra
> layer of tab. I.e
>
> if ( !vpl011->initialized )
>    return;
>
ok.
>
>> +#ifndef _VPL011_H_
>> +
>> +#define _VPL011_H_
>> +
>> +#include <public/io/ring.h>
>> +#include <asm-arm/vreg.h>
>> +
>> +DEFINE_XEN_FLEX_RING(vpl011);
>
>
> I am sure someone already said it in a previous version. The vpl011 is the
> console ring. So why are we defining our own internally?
>
> At least this should have been used by xenconsole, but this is not the
> case... So we should really avoid defining our own ring here and re-use
> public/io/console.h.
>
>
>> +
>> +struct uartdr_reg {
>> +    uint8_t data;
>> +    uint8_t error_status:4;
>> +    uint8_t reserved1:4;
>> +    uint16_t reserved2;
>> +    uint32_t reserved3;
>> +};
>
>
> I don't see any benefits of this structure.
I will remove this structure and use the register value directly after
masking it.
>
>> +
>> +struct vpl011_s {
>
>
> Please remove the _s in the name.
ok.
>
>> +    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,
>
>
> static inline here.
ok.
>
>> +                       uint32_t console_domid,
>> +                       xen_pfn_t gfn,
>> +                       evtchn_port_t *evtchn) { return -ENOSYS; }
>
>
> Please indent correctly { return -ENOSYS };
>
ok.
>> +
>> +static inline void domain_vpl011_deinit(struct domain *d) { }
>
>
> Note that I am ok with the { } on the same line here.
>
>> 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;
>
>
> Why this is here?
I will move it out of here.

Regards,
Bhupinder

Hi Julien,

On 26 May 2017 at 19:12, Bhupinder Thakur <bhupinder.thakur@linaro.org> wrote:
>>> +#ifndef _VPL011_H_
>>> +
>>> +#define _VPL011_H_
>>> +
>>> +#include <public/io/ring.h>
>>> +#include <asm-arm/vreg.h>
>>> +
>>> +DEFINE_XEN_FLEX_RING(vpl011);
>>
>>
>> I am sure someone already said it in a previous version. The vpl011 is the
>> console ring. So why are we defining our own internally?
>>
This macro only defines standard functions to operate on the console
ring. Stefano suggested to use the standard functions to operate on
the ring buffer.

>> At least this should have been used by xenconsole, but this is not the
>> case... So we should really avoid defining our own ring here and re-use
>> public/io/console.h.

I am using the console ring definition as defined in
xen/include/public/io/console.h.

Regards,
Bhupinder

On 05/29/2017 08:13 AM, Bhupinder Thakur wrote:
> Hi Julien,

Hi Bhupinder,

> On 26 May 2017 at 19:12, Bhupinder Thakur <bhupinder.thakur@linaro.org> wrote:
>>>> +#ifndef _VPL011_H_
>>>> +
>>>> +#define _VPL011_H_
>>>> +
>>>> +#include <public/io/ring.h>
>>>> +#include <asm-arm/vreg.h>
>>>> +
>>>> +DEFINE_XEN_FLEX_RING(vpl011);
>>>
>>>
>>> I am sure someone already said it in a previous version. The vpl011 is the
>>> console ring. So why are we defining our own internally?
>>>
> This macro only defines standard functions to operate on the console
> ring. Stefano suggested to use the standard functions to operate on
> the ring buffer.

I don't want things to be mixed up like that, this is a call to trouble 
later on if someone decide to update console.h.

If you need to introduce standard functions, they should be defined in 
console.h and not vpl011.h.

>
>>> At least this should have been used by xenconsole, but this is not the
>>> case... So we should really avoid defining our own ring here and re-use
>>> public/io/console.h.
>
> I am using the console ring definition as defined in
> xen/include/public/io/console.h.

See above.

Cheers,

Hi Julien,

On 22 May 2017 at 19:54, Julien Grall <julien.grall@arm.com> wrote:
>> +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)
>
>
> This function should either have the prototype defined in an header if used
> outside or static.
>
> Also, I have asked to use gfn_t and not xen_pfn_t. The former is a typesafe
> avoiding mix between MFN and GFN.

The gfn is passed on from the domctl API to this function. Is the
gfn_t definition exposed to domctl interface? Currently, gfn_t is
defined in xen/include/xen/mm.h. So I would have to include this
header file in xen/include/public/domctl.h where I have defined the
vuart_ops structure containing gfn. Is it ok to include a xen header
file in the public header file?

Regards,
Bhupinder

On 01/06/17 11:33, Bhupinder Thakur wrote:
> Hi Julien,

Hi Bhupinder,

> On 22 May 2017 at 19:54, Julien Grall <julien.grall@arm.com> wrote:
>>> +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)
>>
>>
>> This function should either have the prototype defined in an header if used
>> outside or static.
>>
>> Also, I have asked to use gfn_t and not xen_pfn_t. The former is a typesafe
>> avoiding mix between MFN and GFN.
>
> The gfn is passed on from the domctl API to this function. Is the
> gfn_t definition exposed to domctl interface? Currently, gfn_t is
> defined in xen/include/xen/mm.h. So I would have to include this
> header file in xen/include/public/domctl.h where I have defined the
> vuart_ops structure containing gfn. Is it ok to include a xen header
> file in the public header file?

No. The only thing I asked is to replace xen_pfn_t by gfn_t in the 
parameter and the caller should use _gfn(...) to do the conversion.

Have a look on how we do in populate_physmap (xen/common/memory.c).

Cheers,

Hi Julien,

On 26 May 2017 at 19:12, Bhupinder Thakur <bhupinder.thakur@linaro.org> wrote:
>>> +
>>> +    switch ( vpl011_reg )
>>> +    {
>>> +    case DR:
>>
>>
>> As mentioned above, you could do:
>>
>> {
>>      uint8_t ch;
>>      ....
>> }
>>
>>> +        vpl011_read_data(v->domain, &ch);
>>> +        *r = ch;
>>
>>
>> Please use vreg_reg32_extract(...).
>>
> ok.
>
>>> +        break;
>>
>>
>> I admit I would prefer the "return 1;" here rather than "break;". This makes
>> easier to follow the emulation for a given register.
>>
>> I would even be in favor of duplicating the "if ( !vpl011... )" in each case
>> for the same reason.

Do you mean that I should repeat the vpl011_reg32_check_access() and
return for each switch case?

Regards,
Bhupinder

Hi Bhupinder,

On 01/06/17 14:34, Bhupinder Thakur wrote:
> On 26 May 2017 at 19:12, Bhupinder Thakur <bhupinder.thakur@linaro.org> wrote:
>>>> +
>>>> +    switch ( vpl011_reg )
>>>> +    {
>>>> +    case DR:
>>>
>>>
>>> As mentioned above, you could do:
>>>
>>> {
>>>      uint8_t ch;
>>>      ....
>>> }
>>>
>>>> +        vpl011_read_data(v->domain, &ch);
>>>> +        *r = ch;
>>>
>>>
>>> Please use vreg_reg32_extract(...).
>>>
>> ok.
>>
>>>> +        break;
>>>
>>>
>>> I admit I would prefer the "return 1;" here rather than "break;". This makes
>>> easier to follow the emulation for a given register.
>>>
>>> I would even be in favor of duplicating the "if ( !vpl011... )" in each case
>>> for the same reason.
>
> Do you mean that I should repeat the vpl011_reg32_check_access() and
> return for each switch case?

I think it would make easier to follow the emulation if you repeat the 
if (vpl011_reg32_check_access) and the return 1 in each case.

So each register emulation is standalone.

Cheers,