Re: [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

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

Hi Bhupinder,

On 06/06/17 18:25, 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>
> ---
> CC: ij
> CC: wl
> CC: ss
> CC: jg
> CC: kw
>
> Changes since v3:
> - Moved the call to DEFINE_XEN_FLEX_RING from vpl011.h to public/console.h. This macro defines
>   standard functions to operate on the ring buffer.
> - Lock taken while updating the interrupt mask and clear registers in mmio_write.
> - Use gfn_t instead of xen_pfn_t.
> - vgic_free_virq called if there is any error in vpl011 initialization.
> - mmio handlers freed if there is any error in vpl011 initialization.
> - Removed vpl011->initialized flag usage as the same check could be done
>   using vpl011->ring-ref.
> - Used return instead of break in the switch handling of emulation of different pl011 registers.
> - Renamed vpl011_update_spi() to vpl011_update().
>
> 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
>
>  tools/console/daemon/io.c        |   2 +-
>  xen/arch/arm/Kconfig             |   5 +
>  xen/arch/arm/Makefile            |   1 +
>  xen/arch/arm/vpl011.c            | 418 +++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/domain.h     |   6 +
>  xen/include/asm-arm/pl011-uart.h |   2 +
>  xen/include/asm-arm/vpl011.h     |  74 +++++++
>  xen/include/public/arch-arm.h    |   6 +
>  xen/include/public/io/console.h  |   4 +

This would require an ACK from Konrad. The addition would also need to 
be justified in the commit message. Although, you probably want to split 
this change in a separate patch.

>  9 files changed, 517 insertions(+), 1 deletion(-)
>  create mode 100644 xen/arch/arm/vpl011.c
>  create mode 100644 xen/include/asm-arm/vpl011.h
>
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 7e6a886..947f13a 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c

Can you explain why you change the position of the include in io.c?

> @@ -21,6 +21,7 @@
>
>  #include "utils.h"
>  #include "io.h"
> +#include <string.h>
>  #include <xenevtchn.h>
>  #include <xengnttab.h>
>  #include <xenstore.h>
> @@ -29,7 +30,6 @@
>
>  #include <stdlib.h>
>  #include <errno.h>
> -#include <string.h>
>  #include <poll.h>
>  #include <fcntl.h>
>  #include <unistd.h>


[...]

>  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

Please the alphabetical order. Just noticed vtimer is not correctly 
positioned. I will send a patch for that.

>
>  #obj-bin-y += ....o
>
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> new file mode 100644
> index 0000000..9b1f27e
> --- /dev/null
> +++ b/xen/arch/arm/vpl011.c
> @@ -0,0 +1,418 @@
> +/*
> + * 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/mm.h>
> +#include <xen/sched.h>
> +#include <public/domctl.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(struct hsr_dabt dabt)
> +{
> +    return (dabt.size != DABT_DOUBLE_WORD);

Again, please add a comment explaining why we allow all the sizes but 
64-bit.

> +}
> +
> +static void vpl011_update(struct domain *d)
> +{
> +    struct vpl011 *vpl011 = &d->arch.vpl011;
> +
> +    /*
> +     * TODO: PL011 interrupts are level triggered which means
> +     * that interrupt needs to be set/clear instead of being
> +     * injected. However, currently vGIC does not handle level
> +     * triggered interrupts properly. This function needs to be
> +     * revisited once vGIC starts handling level triggered
> +     * interrupts.
> +     */
> +    if ( vpl011->uartris & vpl011->uartimsc )

The write in uartirs and uartimsc are protected by a lock. Shouldn't it 
be the case here too? More that they are not updated atomically.

You probably want to call vpl011_update with vpl011 lock taken to make 
sure you don't have any synchronization issue.

> +        vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI);
> +}
> +
> +static uint8_t vpl011_read_data(struct domain *d)
> +{
> +    unsigned long flags;
> +    uint8_t data = 0;
> +    struct vpl011 *vpl011 = &d->arch.vpl011;
> +    struct xencons_interface *intf = vpl011->ring_buf;
> +    XENCONS_RING_IDX in_cons = intf->in_cons;
> +    XENCONS_RING_IDX in_prod = intf->in_prod;

See my answer on Stefano's e-mail regarding the barrier here. 
(<fa3e5003-5c7f-0886-d437-6b643347b4c5@arm.com>)

> +
> +    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 ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
> +    {
> +        data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
> +        in_cons += 1;
> +        intf->in_cons = in_cons;
> +        smp_mb();

I don't understand why you moved the barrier from between reading the 
data and intf->in_cons. You have to ensure the character is read before 
updating in_cons.

> +    }
> +    else
> +    {
> +        gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
> +    }

The {} are not necessary.

> +
> +    if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 )

What if the other end of the ring has put more data whilst reading one 
character?

> +    {
> +        vpl011->uartfr |= RXFE;
> +        vpl011->uartris &= ~RXI;
> +    }
> +    vpl011->uartfr &= ~RXFF;
> +    VPL011_UNLOCK(d, flags);
> +
> +    return data;
> +}
> +
> +static void vpl011_write_data(struct domain *d, uint8_t data)
> +{
> +    unsigned long flags;
> +    struct vpl011 *vpl011 = &d->arch.vpl011;
> +    struct xencons_interface *intf = vpl011->ring_buf;
> +    XENCONS_RING_IDX out_cons = intf->out_cons;
> +    XENCONS_RING_IDX out_prod = intf->out_prod;

See my remark above.

> +
> +    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 ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
> +         sizeof (intf->out) )
> +    {
> +        intf->out[xencons_mask(out_prod, sizeof(intf->out))] = data;
> +        smp_wmb();
> +        out_prod += 1;
> +        intf->out_prod = out_prod;
> +    }
> +    else
> +    {
> +        gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
> +    }

The {} are not necessary.

> +
> +    if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) ==
> +         sizeof (intf->out) )

Ditto here.

> +    {
> +        vpl011->uartfr |= TXFF;
> +        vpl011->uartris &= ~TXI;
> +    }
> +
> +    vpl011->uartfr |= BUSY;
> +
> +    vpl011->uartfr &= ~TXFE;
> +
> +    VPL011_UNLOCK(d, flags);
> +
> +    /*
> +     * Send an event to console backend to indicate that there is
> +     * data in the OUT ring buffer.
> +     */
> +    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)
> +{
> +    struct hsr_dabt dabt = info->dabt;
> +    uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
> +    struct vpl011 *vpl011 = &v->domain->arch.vpl011;
> +
> +    switch ( vpl011_reg )
> +    {
> +    case DR:
> +        /*
> +         * Since pl011 registers are 32-bit registers, all registers
> +         * are handled similarly allowing 8-bit, 16-bit and 32-bit
> +         * accesses.
> +         */

This comment should be on top of the declaration of 
vpl011_reg32_check_access.

> +        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> +
> +        *r = vreg_reg32_extract(vpl011_read_data(v->domain), info);
> +        return 1;
> +
> +    case RSR:
> +        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> +
> +        /* It always returns 0 as there are no physical errors. */
> +        *r = 0;
> +        return 1;
> +
> +    case FR:
> +        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> +
> +        *r = vreg_reg32_extract(vpl011->uartfr, info);

You need to ensure that uartfr is read only once because 
vreg_reg32_extract does not currently ensure that.

> +        return 1;
> +
> +    case RIS:
> +        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> +
> +        *r = vreg_reg32_extract(vpl011->uartris, info);

Ditto.

> +        return 1;
> +
> +    case MIS:
> +        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> +
> +        *r = vreg_reg32_extract(vpl011->uartris &
> +                                vpl011->uartimsc, info);

Ditto.

> +        return 1;
> +
> +    case IMSC:
> +        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> +
> +        *r = vreg_reg32_extract(vpl011->uartimsc, info);

Ditto.

> +        return 1;
> +
> +    case ICR:
> +        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> +
> +        /* 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)
> +{
> +    struct hsr_dabt dabt = info->dabt;
> +    uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
> +    struct vpl011 *vpl011 = &v->domain->arch.vpl011;
> +    struct domain *d = v->domain;
> +    unsigned long flags;
> +
> +    switch ( vpl011_reg )
> +    {
> +    case DR:
> +    {
> +        uint32_t data = 0;
> +
> +        /*
> +         * Since pl011 registers are 32-bit registers, all registers
> +         * are handled similarly allowing 8-bit, 16-bit and 32-bit
> +         * accesses.
> +         */

See above.

> +        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> +
> +        vreg_reg32_update(&data, r, info);
> +        data &= 0xFF;
> +        vpl011_write_data(v->domain, data);
> +        return 1;
> +    }
> +    case RSR: /* Nothing to clear. */
> +        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> +
> +        return 1;
> +
> +    case FR:
> +    case RIS:
> +    case MIS:
> +        goto write_ignore;
> +
> +    case IMSC:
> +        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> +
> +        VPL011_LOCK(d, flags);
> +        vreg_reg32_update(&vpl011->uartimsc, r, info);
> +        VPL011_UNLOCK(d, flags);
> +        vpl011_update(v->domain);

I think this should be call with under the lock.

> +        return 1;
> +
> +    case ICR:
> +        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
> +
> +        VPL011_LOCK(d, flags);
> +        vreg_reg32_clearbits(&vpl011->uartris, r, info);
> +        VPL011_UNLOCK(d, flags);
> +        vpl011_update(d);

Ditto.

> +        return 1;
> +
> +    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,
> +};
> +
> +static void vpl011_data_avail(struct domain *d)
> +{
> +    unsigned long flags;
> +    struct vpl011 *vpl011 = &d->arch.vpl011;
> +    struct xencons_interface *intf = vpl011->ring_buf;
> +    XENCONS_RING_IDX in_cons = intf->in_cons;
> +    XENCONS_RING_IDX in_prod = intf->in_prod;
> +    XENCONS_RING_IDX out_cons = intf->out_cons;
> +    XENCONS_RING_IDX out_prod = intf->out_prod;

Same as above for the barrier.

> +    XENCONS_RING_IDX in_ring_qsize, out_ring_qsize;
> +
> +    VPL011_LOCK(d, flags);
> +
> +    in_ring_qsize = xencons_queued(in_prod,
> +                                   in_cons,
> +                                   sizeof(intf->in));
> +
> +    out_ring_qsize = xencons_queued(out_prod,
> +                                    out_cons,
> +                                    sizeof(intf->out));
> +
> +    /* Update the uart rx state if the buffer is not empty. */
> +    if ( in_ring_qsize != 0 )
> +    {
> +        vpl011->uartfr &= ~RXFE;
> +        if ( in_ring_qsize == sizeof(intf->in) )
> +            vpl011->uartfr |= RXFF;
> +        vpl011->uartris |= RXI;
> +    }
> +
> +    /* Update the uart tx state if the buffer is not full. */
> +    if ( out_ring_qsize != sizeof(intf->out) )
> +    {
> +        vpl011->uartfr &= ~TXFF;
> +        vpl011->uartris |= TXI;
> +        if ( out_ring_qsize == 0 )
> +        {
> +            vpl011->uartfr &= ~BUSY;
> +            vpl011->uartfr |= TXFE;
> +        }
> +    }
> +
> +    VPL011_UNLOCK(d, flags);
> +
> +    vpl011_update(d);

See my comment above for the calling vpl011_update

> +}
> +
> +
> +static void vpl011_notification(struct vcpu *v, unsigned int port)
> +{
> +    vpl011_data_avail(v->domain);
> +}
> +
> +int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
> +{
> +    int rc;
> +    struct vpl011 *vpl011 = &d->arch.vpl011;
> +
> +    if ( vpl011->ring_buf )
> +        return 0;

IHMO, you should return an error if the PL011 is already initialized. 
This should never happen.

> +
> +    /* Map the guest PFN to Xen address space. */
> +    rc =  prepare_ring_for_helper(d,
> +                                  gfn_x(info->gfn),
> +                                  &vpl011->ring_page,
> +                                  &vpl011->ring_buf);
> +    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);

Again, 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, info->console_domid,
> +                                         vpl011_notification);
> +    if ( rc < 0 )
You
> +        goto out2;
> +
> +    vpl011->evtchn = info->evtchn = rc;
> +
> +    return 0;
> +
> +out2:
> +    xfree(d->arch.vmmio.handlers);
> +    vgic_free_virq(d, GUEST_VPL011_SPI);
> +
> +out1:
> +    destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
> +
> +out:
> +    return rc;
> +}
> +
> +void domain_vpl011_deinit(struct domain *d)
> +{
> +    struct vpl011 *vpl011 = &d->arch.vpl011;
> +
> +    if ( !vpl011->ring_buf )

You will bail out if ring_buf is NULL. However, if you called 
domain_vpl011_init first and it failed, you may have ring_buf set but 
the rest not fully updated. This means that you will free garbagge.

I think this could be solved by reinitialize ring_buf if an error occur 
in domain_vpl011_init.

> +        return;
> +
> +    free_xen_event_channel(d, vpl011->evtchn);
> +    destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
> +    xfree(d->arch.vmmio.handlers);
> +}

[...]

> diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> new file mode 100644
> index 0000000..b3e332d
> --- /dev/null
> +++ b/xen/include/asm-arm/vpl011.h
> @@ -0,0 +1,74 @@
> +/*
> + * 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_
> +

We tend to keep #ifndef and #define together. So please drop the newline 
here.

> +#ifdef CONFIG_VPL011_CONSOLE
> +int domain_vpl011_init(struct domain *d,
> +                       struct vpl011_init_info *info);
> +void domain_vpl011_deinit(struct domain *d);
> +#else
> +static inline int domain_vpl011_init(struct domain *d,
> +                                     struct vpl011_init_info *info)
> +{
> +    return -ENOSYS;
> +}
> +
> +static inline void domain_vpl011_deinit(struct domain *d) { }
> +#endif
> +
> +#endif
> +

Please drop this newline.

> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Cheers,

-- 
Julien Grall

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