| Message ID | 1496769929-23355-4-git-send-email-bhupinder.thakur@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 6 Jun 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> > --- > 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 + > 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 > @@ -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> > 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..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); > +} > + > +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 ) > + 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; After reading the indexes, we always need barriers. In this case: smp_rmb(); > + 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(); > + } > + else > + { > + gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n"); > + } > + > + if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 ) > + { > + vpl011->uartfr |= RXFE; > + vpl011->uartris &= ~RXI; > + } > + vpl011->uartfr &= ~RXFF; > + VPL011_UNLOCK(d, flags); I am pretty sure that the PV console protocol requires us to notify the other end even on reads. We need to add a notify_via_xen_event_channel here, I think. > + 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; smp_mb() > + 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"); > + } > + > + if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) == > + sizeof (intf->out) ) > + { > + 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. > + */ > + 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); > + return 1; > + > + case RIS: > + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; > + > + *r = vreg_reg32_extract(vpl011->uartris, info); > + return 1; > + > + case MIS: > + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; > + > + *r = vreg_reg32_extract(vpl011->uartris & > + vpl011->uartimsc, info); > + return 1; > + > + case IMSC: > + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; > + > + *r = vreg_reg32_extract(vpl011->uartimsc, info); > + 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. > + */ > + 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); > + 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); > + 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; > + XENCONS_RING_IDX in_ring_qsize, out_ring_qsize; smb_mb() > + 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); > +} > + > + > +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; > + > + /* 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); > + > + spin_lock_init(&vpl011->lock); > + > + rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid, > + vpl011_notification); > + if ( rc < 0 ) > + 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 ) > + return; > + > + free_xen_event_channel(d, vpl011->evtchn); > + destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page); > + xfree(d->arch.vmmio.handlers); > +} > + > +/* > + * 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..91d1061 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 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..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_ > + > +#define _VPL011_H_ > + > +#include <public/domctl.h> > +#include <public/io/ring.h> > +#include <asm-arm/vreg.h> > +#include <xen/mm.h> > + > +/* helper macros */ > +#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 vpl011 { > + 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; > +}; > + > +struct vpl011_init_info { > + uint32_t console_domid; > + gfn_t gfn; > + evtchn_port_t evtchn; > +}; > + > +#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 > + > +/* > + * 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..85ab665 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -410,6 +410,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 +448,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 > diff --git a/xen/include/public/io/console.h b/xen/include/public/io/console.h > index e2cd97f..5e45e1c 100644 > --- a/xen/include/public/io/console.h > +++ b/xen/include/public/io/console.h > @@ -27,6 +27,8 @@ > #ifndef __XEN_PUBLIC_IO_CONSOLE_H__ > #define __XEN_PUBLIC_IO_CONSOLE_H__ > > +#include "ring.h" > + > typedef uint32_t XENCONS_RING_IDX; > > #define MASK_XENCONS_IDX(idx, ring) ((idx) & (sizeof(ring)-1)) > @@ -38,6 +40,8 @@ struct xencons_interface { > XENCONS_RING_IDX out_cons, out_prod; > }; > > +DEFINE_XEN_FLEX_RING(xencons); > + > #endif /* __XEN_PUBLIC_IO_CONSOLE_H__ */ > > /* > -- > 2.7.4 >
Hi, On 07/06/17 00:02, Stefano Stabellini wrote: > On Tue, 6 Jun 2017, Bhupinder Thakur wrote: >> +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; > > After reading the indexes, we always need barriers. In this case: > > smp_rmb(); Well, there are already barrier with the spinlock. However, I am bit concern about reading those index without the lock taken. You can have concurrent call to vpl011_read_data happening and therefore the indexes may have changed when the lock will be taken. > > >> + 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(); >> + } >> + else >> + { >> + gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n"); >> + } >> + >> + if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 ) >> + { >> + vpl011->uartfr |= RXFE; >> + vpl011->uartris &= ~RXI; >> + } >> + vpl011->uartfr &= ~RXFF; >> + VPL011_UNLOCK(d, flags); > > I am pretty sure that the PV console protocol requires us to notify the > other end even on reads. We need to add a notify_via_xen_event_channel > here, I think. I would agree here. On the previous version, I asked Bhupinder to explain why it is necessary and he said: "On second thoughs, notification is not required". > > >> + 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; > > smp_mb() Same remark as above. [...] >> +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; >> + XENCONS_RING_IDX in_ring_qsize, out_ring_qsize; > > smb_mb() Ditto. Cheers,
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,
On Fri, 9 Jun 2017, Julien Grall wrote: > Hi, > > On 07/06/17 00:02, Stefano Stabellini wrote: > > On Tue, 6 Jun 2017, Bhupinder Thakur wrote: > > > +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; > > > > After reading the indexes, we always need barriers. In this case: > > > > smp_rmb(); > > Well, there are already barrier with the spinlock. However, I am bit concern > about reading those index without the lock taken. You can have concurrent call > to vpl011_read_data happening and therefore the indexes may have changed when > the lock will be taken. I don't like to rely on the barriers within spin_lock, which might or might not change in future implementations, for PV protocols operations, which need explicit barrier. But it is true the code would work today from that perspective. You are also right that with the current implementation vpl011_read_data (and vpl011_write_data) could be called twice simultaneously. For that to work correctly, we have to move the indexes reads after VPL011_LOCK. In addition, we also need the explicit memory barrier smp_rmb() after the indexes reads to be in sync with the other end. In other words: VPL011_LOCK(d, flags); in_cons = intf->in_cons; in_prod = intf->in_prod; smp_rmb(); /* rest of the code */ The same for the other function.
Hi Julien, >>> +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; >> >> >> After reading the indexes, we always need barriers. In this case: >> >> smp_rmb(); > > > Well, there are already barrier with the spinlock. However, I am bit concern > about reading those index without the lock taken. You can have concurrent > call to vpl011_read_data happening and therefore the indexes may have > changed when the lock will be taken. Is there a possibility of concurrent access since this function is executed as part of trap handling which will serialize access to this function? > > >> >> >>> + 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(); >>> + } >>> + else >>> + { >>> + gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer >>> empty\n"); >>> + } >>> + >>> + if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 ) >>> + { >>> + vpl011->uartfr |= RXFE; >>> + vpl011->uartris &= ~RXI; >>> + } >>> + vpl011->uartfr &= ~RXFF; >>> + VPL011_UNLOCK(d, flags); >> >> >> I am pretty sure that the PV console protocol requires us to notify the >> other end even on reads. We need to add a notify_via_xen_event_channel >> here, I think. > > > I would agree here. On the previous version, I asked Bhupinder to explain > why it is necessary and he said: "On second thoughs, notification is not > required". > I understand that xenconsole is currently using the event notification as an indication to read data from the ring buffer. For writing data, it keeps checking periodically if there is space in the ring buffer. If there is space then it writes more data. I agree that as a protocol, it may be a requirement to send the notifications on read also. I will add back the notification for this case. >> >> >>> + 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; >> >> >> smp_mb() > > > Same remark as above. > > [...] > >>> +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; >>> + XENCONS_RING_IDX in_ring_qsize, out_ring_qsize; >> >> >> smb_mb() > > > Ditto. > Regards, Bhupinder
On 13/06/2017 09:58, Bhupinder Thakur wrote: > Hi Julien, Hi Bhupinder, >>>> +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; >>> >>> >>> After reading the indexes, we always need barriers. In this case: >>> >>> smp_rmb(); >> >> >> Well, there are already barrier with the spinlock. However, I am bit concern >> about reading those index without the lock taken. You can have concurrent >> call to vpl011_read_data happening and therefore the indexes may have >> changed when the lock will be taken. > > Is there a possibility of concurrent access since this function is > executed as part of > trap handling which will serialize access to this function? There are no locking in the common trap handling. The emulation should take care of the locking because multiple vCPU can concurrently access the MMIO region. [....] >>> I am pretty sure that the PV console protocol requires us to notify the >>> other end even on reads. We need to add a notify_via_xen_event_channel >>> here, I think. >> >> >> I would agree here. On the previous version, I asked Bhupinder to explain >> why it is necessary and he said: "On second thoughs, notification is not >> required". >> > I understand that xenconsole is currently using the event notification > as an indication to read > data from the ring buffer. For writing data, it keeps checking > periodically if there is space in the > ring buffer. If there is space then it writes more data. You should not assume that xenconsoled is the only backend console. One could decide to implement its own. Cheers,
Hi Julien, >> >> 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. I will send 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? Since I am including ring.h in console.h, it needs string.h to be included first. > >> @@ -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. > ok. > >> +#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. Since we are just reading the values here, I think it is fine to not take a lock. The condition will either be true or false. > >> + 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. I thought that since these 3 statements are dependent on in_cons, they would be executed in order due to data dependency. The memory barrier after the 3 statements ensures that intf->in_cons is updated before proceeding ahead. > >> + } >> + else >> + { >> + gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n"); >> + } > > > The {} are not necessary. ok. > >> + >> + 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? It will raise an event when the other end puts more data and in the event handling function data_available(), it will clear the RXFE bit. > >> + { >> + 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. I will move index reading under lock. > >> + >> + 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. ok. > >> + >> + 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. ok. > >> + 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. I will read these registers into a local variable and use it. > > >> + 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. ok. > >> + >> + /* 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. This function does not return a status. So there is no way to find out if the mmio handlers were registered successfully. I am removing the mmio handlers in the error legs in domain_vpl011_init(). > >> + >> + spin_lock_init(&vpl011->lock); >> + >> + rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid, >> + vpl011_notification); >> + if ( rc < 0 ) > > You I think this is a type. >> >> + 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. destroy_ring_for_helper() sets the first parameter to NULL incase it fails. > >> + 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. > ok. >> +#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. You mean the newline between the #endifs or after the last #endif? > Regards, Bhupinder
On 13/06/17 11:57, Bhupinder Thakur wrote: > Hi Julien, Hi Bhupinder, > >>> >>> 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. > I will send 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? > Since I am including ring.h in console.h, it needs string.h to be > included first. This should be justified in the commit message. [...] >>> +} >>> + >>> +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. > > Since we are just reading the values here, I think it is fine to not > take a lock. The > condition will either be true or false. uartris and uartimsc may not be updated atomically because vreg_reg32_update does not guarantee it. So you may read a wrong value here and potentially inject spurious interrupt. This will get much worse when level will fully be supported as you may switch the level of the interrupt by mistake. > >> >>> + 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. > I thought that since these 3 statements are dependent on in_cons, they > would be executed in order due to data dependency. How do you know the compiler will generate assembly which contain the data dependency? Likely it will not be the case because in_cons will be used indirectly as we mask it first. > The memory barrier > after the 3 statements ensures that intf->in_cons is updated before > proceeding ahead. Can you explain why? IHMO, what matter here is in_cons to be written after intf->in[...] is read. Otherwise the backend may see in_cons before the character has effectively been read. >> What if the other end of the ring has put more data whilst reading one >> character? > It will raise an event when the other end puts more data and in the > event handling function data_available(), it will clear the RXFE bit. And this is fine because the lock is here to protect uartfr/uartis I guess? [...] >>> + >>> + /* 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. > This function does not return a status. So there is no way to find out > if the mmio handlers were > registered successfully. That's not my point. register_mmio_handler should never fail. However, the code below (alloc_unbount_...) may fail. And you bail > I am removing the mmio handlers in the error > legs in domain_vpl011_init(). Xen does not have any helper to revert the behavior of register_mmio_handler and I don't seem to introduce why. So how do you do it? Anyway, you will not need to worry about removing MMIO handler if you move the call after all the call that may fail. >> >>> + >>> + spin_lock_init(&vpl011->lock); >>> + >>> + rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid, >>> + vpl011_notification); >>> + if ( rc < 0 ) >> >> You > I think this is a type. hmmm likely. > >>> >>> + 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. > destroy_ring_for_helper() sets the first parameter to NULL incase it fails. Fine. I think it is a bit fragile, but I don't see why someone would decide to remove it without checking all the callers. [...] >>> +#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. > You mean the newline between the #endifs or after the last #endif? Yes. Cheers,
On Tue, 13 Jun 2017, Julien Grall wrote: > > > > 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. > > I will send 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? > > Since I am including ring.h in console.h, it needs string.h to be > > included first. > > This should be justified in the commit message. > > [...] > > > > > +} > > > > + > > > > +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. > > > > Since we are just reading the values here, I think it is fine to not > > take a lock. The > > condition will either be true or false. > > uartris and uartimsc may not be updated atomically because vreg_reg32_update > does not guarantee it. > > So you may read a wrong value here and potentially inject spurious interrupt. > This will get much worse when level will fully be supported as you may switch > the level of the interrupt by mistake. > > > > > > > > > > + 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. > > I thought that since these 3 statements are dependent on in_cons, they > > would be executed in order due to data dependency. > > How do you know the compiler will generate assembly which contain the data > dependency? > > Likely it will not be the case because in_cons will be used indirectly as we > mask it first. > > > The memory barrier > > after the 3 statements ensures that intf->in_cons is updated before > > proceeding ahead. > > Can you explain why? > > IHMO, what matter here is in_cons to be written after intf->in[...] is read. > Otherwise the backend may see in_cons before the character has effectively > been read. Julien is right, I missed it in my review. The smp_mb() should be before increasing intf->in_cons: smp_mb(); intf->in_cons = in_cons; Like you have done correctly in vpl011_write_data. The reason is that you need to make sure that the read is complete before increasing the index, because as soon as you do that the other end could overwrite your data.
Hi Julien, >>>> +} >>>> + >>>> +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. >> >> >> Since we are just reading the values here, I think it is fine to not >> take a lock. The >> condition will either be true or false. > > > uartris and uartimsc may not be updated atomically because vreg_reg32_update > does not guarantee it. > > So you may read a wrong value here and potentially inject spurious > interrupt. This will get much worse when level will fully be supported as > you may switch the level of the interrupt by mistake. > Ok. I will check this condition under lock. But should I call vgic_vcpu_inject_spi() also under the same lock? I think we can do like this: vpl011_lock(); mask = vpl011->uartris & vpl011->uartimsc; vpl011_unlock(); if ( mask ) vgic_vcpu_inject_spi(); > >>> 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. >> >> I thought that since these 3 statements are dependent on in_cons, they >> would be executed in order due to data dependency. > > > How do you know the compiler will generate assembly which contain the data > dependency? > > Likely it will not be the case because in_cons will be used indirectly as we > mask it first. > >> The memory barrier >> after the 3 statements ensures that intf->in_cons is updated before >> proceeding ahead. > > > Can you explain why? > > IHMO, what matter here is in_cons to be written after intf->in[...] is read. > Otherwise the backend may see in_cons before the character has effectively > been read. > ok. The issue is that the other end (xenconsole running on maybe some other CPU) may see the increment operation first before the read operation could complete (due to some quirk of memory/cache architecture) even though the CPU running the mmio_read() will see the effect in the correct order only. I will move the smp_mb() before index is incremented. >>> What if the other end of the ring has put more data whilst reading one >>> character? >> >> It will raise an event when the other end puts more data and in the >> event handling function data_available(), it will clear the RXFE bit. > > > And this is fine because the lock is here to protect uartfr/uartis I guess? Yes. > > [...] > >>>> + >>>> + /* 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. >> >> This function does not return a status. So there is no way to find out >> if the mmio handlers were >> registered successfully. > > > That's not my point. register_mmio_handler should never fail. However, the > code below (alloc_unbount_...) may fail. And you bail > >> I am removing the mmio handlers in the error >> legs in domain_vpl011_init(). > > > Xen does not have any helper to revert the behavior of register_mmio_handler > and I don't seem to introduce why. So how do you do it? > > Anyway, you will not need to worry about removing MMIO handler if you move > the call after all the call that may fail. > I will move this call to the last. >>> >>>> + >>>> + spin_lock_init(&vpl011->lock); >>>> + >>>> + rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid, >>>> + vpl011_notification); >>>> + if ( rc < 0 ) >>> >>> >>> You >> >> I think this is a type. > > > hmmm likely. > > >> >>>> >>>> + 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. >> >> destroy_ring_for_helper() sets the first parameter to NULL incase it >> fails. > > > Fine. I think it is a bit fragile, but I don't see why someone would decide > to remove it without checking all the callers. > > [...] > >>>> +#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. >> >> You mean the newline between the #endifs or after the last #endif? > > > Yes. ok. I have removed the newline between the endifs. Regards, Bhupinder
On 06/14/2017 06:47 AM, Bhupinder Thakur wrote: > Hi Julien, Hi Bhupinder, > >>>>> +} >>>>> + >>>>> +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. >>> >>> >>> Since we are just reading the values here, I think it is fine to not >>> take a lock. The >>> condition will either be true or false. >> >> >> uartris and uartimsc may not be updated atomically because vreg_reg32_update >> does not guarantee it. >> >> So you may read a wrong value here and potentially inject spurious >> interrupt. This will get much worse when level will fully be supported as >> you may switch the level of the interrupt by mistake. >> > Ok. I will check this condition under lock. But should I call > vgic_vcpu_inject_spi() also under the same lock? I think we can do > like this: > > vpl011_lock(); > mask = vpl011->uartris & vpl011->uartimsc; > vpl011_unlock(); > > if ( mask ) > vgic_vcpu_inject_spi(); This is not going to work the day we rework the vGIC to fully support level interrupt. If you don't protect vgic_vcpu_inject_spi(), you may lower the level by mistake. As mentioned on a previous mail, I would prefer if vpl011_update is called with the lock taken. [...] >> How do you know the compiler will generate assembly which contain the data >> dependency? >> >> Likely it will not be the case because in_cons will be used indirectly as we >> mask it first. >> >>> The memory barrier >>> after the 3 statements ensures that intf->in_cons is updated before >>> proceeding ahead. >> >> >> Can you explain why? >> >> IHMO, what matter here is in_cons to be written after intf->in[...] is read. >> Otherwise the backend may see in_cons before the character has effectively >> been read. >> > ok. The issue is that the other end (xenconsole running on maybe some > other CPU) may see the increment operation first before the read > operation could complete (due to some quirk of memory/cache > architecture) even though the CPU running the mmio_read() will see the > effect in the correct order only. It is not about quirk of memory/cache but data access ordering at the processor level. A processor is free to re-order the access if it is more efficient. That's why you have the mb/smp_mb barrier to tell the processor to no do that. Cheers,
Hi, 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 + > 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 > @@ -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> > 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 I admit that I am rather late with this comment, but I am not sure we should advertise PL011 emulation here. Technically what you emulate is a "SBSA Generic UART", which is indeed a subset of the PL011, but really only a subset. You confirm this already in patch 13/14, where you use the respective compatible name instead of the generic arm,pl011 one. So while I don't dare to ask for renaming every identifier, I wonder if we should at least keep the publicly visible part confined to "SBSA UART". You could mention the subset nature in the help message here, for instance. The same reasoning applies to other parts of this series which introduce user-visible strings (like in libxl). > 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..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> I think Julien mentioned this already, but I think you should just use "#include <asm/.. .h>" here. > + > +static bool vpl011_reg32_check_access(struct hsr_dabt dabt) > +{ > + return (dabt.size != DABT_DOUBLE_WORD); > +} > + > +static void vpl011_update(struct domain *d) Can you please rename this function to indicate that it updates the *interrupt status*? That name as it is rather generic at the moment. > +{ > + 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 ) > + vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI); So my understanding is that this is using an edge triggered semantic at the moment. While this is not what an PL011 actually implements and not what the driver really expects, this is fine on itself for now. BUT I think we may want to avoid injecting spurious interrupts now: If for instance the receive interrupt condition is set and we clear the transmit interrupt bit, then call this function, it would inject a new interrupt, although in a edge-triggered world we really wouldn't need to do anything. So I believe we would need to have some kind of shadowed interrupt state, which stores the interrupt condition the guest already knows about. As soon as we *add* a bit to this state, we inject the SPI. This would act as a kind of temporary bridge between the level triggered SBSA/PL011 UART and the edge-only VGIC implementation for now. Later when the VGIC properly handles level triggered interrupts, this implementation can be adjusted. But this change should then be confined to this very function. > +} > + > +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; > + > + 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(); > + } > + else > + { > + gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n"); > + } > + > + if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 ) > + { > + vpl011->uartfr |= RXFE; > + vpl011->uartris &= ~RXI; In a level triggered world you would need to possibly change the status of the interrupt line here, so a call to (a renamed and fixed) vpl011_update() function would be due here. To make the transition later as easy as possible, I'd recommend to implement this shadow interrupt state as described above and then call the interrupt update function here already. > + } > + 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; > + > + 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"); > + } > + > + if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) == > + sizeof (intf->out) ) > + { > + vpl011->uartfr |= TXFF; > + vpl011->uartris &= ~TXI; Same as above. Basically whenever you change one of the bits that may affect the status of the interrupt line, I'd call the update function. > + } > + > + 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. > + */ > + 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); > + return 1; > + > + case RIS: > + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; > + > + *r = vreg_reg32_extract(vpl011->uartris, info); > + return 1; > + > + case MIS: > + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; > + > + *r = vreg_reg32_extract(vpl011->uartris & > + vpl011->uartimsc, info); That smells like you need a lock here, since another access could change either RIS and/or IMSC concurrently. But as Julien already mentioned, these accesses are prone to races anyway, since vreg_reg32_extract() is not atomic. So is it worth to take the lock here just before the whole switch statement? > + return 1; > + > + case IMSC: > + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; > + > + *r = vreg_reg32_extract(vpl011->uartimsc, info); > + 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. > + */ > + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; > + > + vreg_reg32_update(&data, r, info); > + data &= 0xFF; This is not needed as vpl011_write_data()'s second argument is uint8_t, so the compiler does this masking anyway. And even if it wouldn't, you could move this statement into the call. > + 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); > + return 1; > + > + case ICR: > + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; > + > + VPL011_LOCK(d, flags); > + vreg_reg32_clearbits(&vpl011->uartris, r, info); Please call the interrupt status update function here. > + VPL011_UNLOCK(d, flags); > + vpl011_update(d); > + 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; > + 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; ... and here ... > + } > + > + /* Update the uart tx state if the buffer is not full. */ > + if ( out_ring_qsize != sizeof(intf->out) ) > + { > + vpl011->uartfr &= ~TXFF; > + vpl011->uartris |= TXI; ... and here. I didn't look at the locking issue on the Xen console for now, but might join this discussion later. Cheers, Andre. > + if ( out_ring_qsize == 0 ) > + { > + vpl011->uartfr &= ~BUSY; > + vpl011->uartfr |= TXFE; > + } > + } > + > + VPL011_UNLOCK(d, flags); > + > + vpl011_update(d); > +} > + > + > +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; > + > + /* 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); > + > + spin_lock_init(&vpl011->lock); > + > + rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid, > + vpl011_notification); > + if ( rc < 0 ) > + 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 ) > + return; > + > + free_xen_event_channel(d, vpl011->evtchn); > + destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page); > + xfree(d->arch.vmmio.handlers); > +} > + > +/* > + * 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..91d1061 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 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..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_ > + > +#define _VPL011_H_ > + > +#include <public/domctl.h> > +#include <public/io/ring.h> > +#include <asm-arm/vreg.h> > +#include <xen/mm.h> > + > +/* helper macros */ > +#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 vpl011 { > + 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; > +}; > + > +struct vpl011_init_info { > + uint32_t console_domid; > + gfn_t gfn; > + evtchn_port_t evtchn; > +}; > + > +#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 > + > +/* > + * 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..85ab665 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -410,6 +410,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 +448,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 > diff --git a/xen/include/public/io/console.h b/xen/include/public/io/console.h > index e2cd97f..5e45e1c 100644 > --- a/xen/include/public/io/console.h > +++ b/xen/include/public/io/console.h > @@ -27,6 +27,8 @@ > #ifndef __XEN_PUBLIC_IO_CONSOLE_H__ > #define __XEN_PUBLIC_IO_CONSOLE_H__ > > +#include "ring.h" > + > typedef uint32_t XENCONS_RING_IDX; > > #define MASK_XENCONS_IDX(idx, ring) ((idx) & (sizeof(ring)-1)) > @@ -38,6 +40,8 @@ struct xencons_interface { > XENCONS_RING_IDX out_cons, out_prod; > }; > > +DEFINE_XEN_FLEX_RING(xencons); > + > #endif /* __XEN_PUBLIC_IO_CONSOLE_H__ */ > > /* >
Hi Andre, Thanks for your comments. >> 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 > > I admit that I am rather late with this comment, but I am not sure we > should advertise PL011 emulation here. > Technically what you emulate is a "SBSA Generic UART", which is indeed a > subset of the PL011, but really only a subset. You confirm this already > in patch 13/14, where you use the respective compatible name instead of > the generic arm,pl011 one. > > So while I don't dare to ask for renaming every identifier, I wonder if > we should at least keep the publicly visible part confined to "SBSA > UART". You could mention the subset nature in the help message here, for > instance. > The same reasoning applies to other parts of this series which introduce > user-visible strings (like in libxl). > >> endmenu >> I will rename the user visible part to "sbsa_uart". >> +#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> > > I think Julien mentioned this already, but I think you should just use > "#include <asm/.. .h>" here. > ok. >> + >> +static bool vpl011_reg32_check_access(struct hsr_dabt dabt) >> +{ >> + return (dabt.size != DABT_DOUBLE_WORD); >> +} >> + >> +static void vpl011_update(struct domain *d) > > Can you please rename this function to indicate that it updates the > *interrupt status*? That name as it is rather generic at the moment. > I will rename it to vpl011_update_interrupt_status. >> +{ >> + 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 ) >> + vgic_vcpu_inject_spi(d, GUEST_VPL011_SPI); > > So my understanding is that this is using an edge triggered semantic at > the moment. While this is not what an PL011 actually implements and not > what the driver really expects, this is fine on itself for now. > BUT I think we may want to avoid injecting spurious interrupts now: > If for instance the receive interrupt condition is set and we clear the > transmit interrupt bit, then call this function, it would inject a new > interrupt, although in a edge-triggered world we really wouldn't need to > do anything. > So I believe we would need to have some kind of shadowed interrupt > state, which stores the interrupt condition the guest already knows > about. As soon as we *add* a bit to this state, we inject the SPI. > This would act as a kind of temporary bridge between the level triggered > SBSA/PL011 UART and the edge-only VGIC implementation for now. > Later when the VGIC properly handles level triggered interrupts, this > implementation can be adjusted. But this change should then be confined > to this very function. > ok. I will update the logic accordingly. >> +} >> + >> +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; >> + >> + 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(); >> + } >> + else >> + { >> + gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n"); >> + } >> + >> + if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 ) >> + { >> + vpl011->uartfr |= RXFE; >> + vpl011->uartris &= ~RXI; > > In a level triggered world you would need to possibly change the status > of the interrupt line here, so a call to (a renamed and fixed) > vpl011_update() function would be due here. To make the transition later > as easy as possible, I'd recommend to implement this shadow interrupt > state as described above and then call the interrupt update function > here already. ok. > >> + } >> + 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; >> + >> + 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"); >> + } >> + >> + if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) == >> + sizeof (intf->out) ) >> + { >> + vpl011->uartfr |= TXFF; >> + vpl011->uartris &= ~TXI; > > Same as above. Basically whenever you change one of the bits that may > affect the status of the interrupt line, I'd call the update function. ok. > >> + } >> + >> + 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. >> + */ >> + 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); >> + return 1; >> + >> + case RIS: >> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; >> + >> + *r = vreg_reg32_extract(vpl011->uartris, info); >> + return 1; >> + >> + case MIS: >> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; >> + >> + *r = vreg_reg32_extract(vpl011->uartris & >> + vpl011->uartimsc, info); > > That smells like you need a lock here, since another access could change > either RIS and/or IMSC concurrently. > But as Julien already mentioned, these accesses are prone to races > anyway, since vreg_reg32_extract() is not atomic. > So is it worth to take the lock here just before the whole switch statement? > Yes, I have taken the lock while reading. >> + return 1; >> + >> + case IMSC: >> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; >> + >> + *r = vreg_reg32_extract(vpl011->uartimsc, info); >> + 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. >> + */ >> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; >> + >> + vreg_reg32_update(&data, r, info); >> + data &= 0xFF; > > This is not needed as vpl011_write_data()'s second argument is uint8_t, > so the compiler does this masking anyway. > And even if it wouldn't, you could move this statement into the call. > Yes. I will remove the masking. >> + 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); >> + return 1; >> + >> + case ICR: >> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; >> + >> + VPL011_LOCK(d, flags); >> + vreg_reg32_clearbits(&vpl011->uartris, r, info); > > Please call the interrupt status update function here. > ok. >> + VPL011_UNLOCK(d, flags); >> + vpl011_update(d); >> + 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; >> + 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; > > ... and here ... > >> + } >> + >> + /* Update the uart tx state if the buffer is not full. */ >> + if ( out_ring_qsize != sizeof(intf->out) ) >> + { >> + vpl011->uartfr &= ~TXFF; >> + vpl011->uartris |= TXI; > > ... and here. > ok Regards, Bhupinder
Hi, On 19/06/17 11:14, Andre Przywara wrote: >> + >> +static bool vpl011_reg32_check_access(struct hsr_dabt dabt) >> +{ >> + return (dabt.size != DABT_DOUBLE_WORD); >> +} >> + >> +static void vpl011_update(struct domain *d) > > Can you please rename this function to indicate that it updates the > *interrupt status*? That name as it is rather generic at the moment. Well, it updates the pl011 state even though today it is only handling interrupt... >> +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. >> + */ >> + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; >> + >> + vreg_reg32_update(&data, r, info); >> + data &= 0xFF; > > This is not needed as vpl011_write_data()'s second argument is uint8_t, > so the compiler does this masking anyway. > And even if it wouldn't, you could move this statement into the call. Sorry, I have only spotted this comment now. We should really avoid implicit cast when calling function. This is a call to make error if we ever decide to change the type of the parameter. More than the local variable data is 32-bit. Better to play safe than handling security issue after afterwards. Cheers,
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 @@ -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> 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..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); +} + +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 ) + 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; + + 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(); + } + else + { + gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n"); + } + + if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 ) + { + 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; + + 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"); + } + + if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) == + sizeof (intf->out) ) + { + 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. + */ + 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); + return 1; + + case RIS: + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; + + *r = vreg_reg32_extract(vpl011->uartris, info); + return 1; + + case MIS: + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; + + *r = vreg_reg32_extract(vpl011->uartris & + vpl011->uartimsc, info); + return 1; + + case IMSC: + if ( !vpl011_reg32_check_access(dabt) ) goto bad_width; + + *r = vreg_reg32_extract(vpl011->uartimsc, info); + 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. + */ + 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); + 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); + 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; + 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); +} + + +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; + + /* 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); + + spin_lock_init(&vpl011->lock); + + rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid, + vpl011_notification); + if ( rc < 0 ) + 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 ) + return; + + free_xen_event_channel(d, vpl011->evtchn); + destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page); + xfree(d->arch.vmmio.handlers); +} + +/* + * 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..91d1061 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 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..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_ + +#define _VPL011_H_ + +#include <public/domctl.h> +#include <public/io/ring.h> +#include <asm-arm/vreg.h> +#include <xen/mm.h> + +/* helper macros */ +#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 vpl011 { + 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; +}; + +struct vpl011_init_info { + uint32_t console_domid; + gfn_t gfn; + evtchn_port_t evtchn; +}; + +#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 + +/* + * 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..85ab665 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -410,6 +410,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 +448,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 diff --git a/xen/include/public/io/console.h b/xen/include/public/io/console.h index e2cd97f..5e45e1c 100644 --- a/xen/include/public/io/console.h +++ b/xen/include/public/io/console.h @@ -27,6 +27,8 @@ #ifndef __XEN_PUBLIC_IO_CONSOLE_H__ #define __XEN_PUBLIC_IO_CONSOLE_H__ +#include "ring.h" + typedef uint32_t XENCONS_RING_IDX; #define MASK_XENCONS_IDX(idx, ring) ((idx) & (sizeof(ring)-1)) @@ -38,6 +40,8 @@ struct xencons_interface { XENCONS_RING_IDX out_cons, out_prod; }; +DEFINE_XEN_FLEX_RING(xencons); + #endif /* __XEN_PUBLIC_IO_CONSOLE_H__ */ /*
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 + 9 files changed, 517 insertions(+), 1 deletion(-) create mode 100644 xen/arch/arm/vpl011.c create mode 100644 xen/include/asm-arm/vpl011.h