[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen
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@xxxxxxxxxx>
---
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@xxxxxxx>)
+
+ VPL011_LOCK(d, flags);
+
+ /*
+ * It is expected that there will be data in the ring buffer when this
+ * function is called since the guest is expected to read the data register
+ * only if the TXFE flag is not set.
+ * If the guest still does read when TXFE bit is set then 0 will be
returned.
+ */
+ if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
+ {
+ data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
+ in_cons += 1;
+ intf->in_cons = in_cons;
+ smp_mb();
I don't understand why you moved the barrier from between reading the
data and intf->in_cons. You have to ensure the character is read before
updating in_cons.
+ }
+ else
+ {
+ gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
+ }
The {} are not necessary.
+
+ if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 )
What if the other end of the ring has put more data whilst reading one
character?
+ {
+ vpl011->uartfr |= RXFE;
+ vpl011->uartris &= ~RXI;
+ }
+ vpl011->uartfr &= ~RXFF;
+ VPL011_UNLOCK(d, flags);
+
+ return data;
+}
+
+static void vpl011_write_data(struct domain *d, uint8_t data)
+{
+ unsigned long flags;
+ struct vpl011 *vpl011 = &d->arch.vpl011;
+ struct xencons_interface *intf = vpl011->ring_buf;
+ XENCONS_RING_IDX out_cons = intf->out_cons;
+ XENCONS_RING_IDX out_prod = intf->out_prod;
See my remark above.
+
+ VPL011_LOCK(d, flags);
+
+ /*
+ * It is expected that the ring is not full when this function is called
+ * as the guest is expected to write to the data register only when the
+ * TXFF flag is not set.
+ * In case the guest does write even when the TXFF flag is set then the
+ * data will be silently dropped.
+ */
+ if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
+ sizeof (intf->out) )
+ {
+ intf->out[xencons_mask(out_prod, sizeof(intf->out))] = data;
+ smp_wmb();
+ out_prod += 1;
+ intf->out_prod = out_prod;
+ }
+ else
+ {
+ gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
+ }
The {} are not necessary.
+
+ if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) ==
+ sizeof (intf->out) )
Ditto here.
+ {
+ vpl011->uartfr |= TXFF;
+ vpl011->uartris &= ~TXI;
+ }
+
+ vpl011->uartfr |= BUSY;
+
+ vpl011->uartfr &= ~TXFE;
+
+ VPL011_UNLOCK(d, flags);
+
+ /*
+ * Send an event to console backend to indicate that there is
+ * data in the OUT ring buffer.
+ */
+ notify_via_xen_event_channel(d, vpl011->evtchn);
+}
+
+static int vpl011_mmio_read(struct vcpu *v,
+ mmio_info_t *info,
+ register_t *r,
+ void *priv)
+{
+ struct hsr_dabt dabt = info->dabt;
+ uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
+ struct vpl011 *vpl011 = &v->domain->arch.vpl011;
+
+ switch ( vpl011_reg )
+ {
+ case DR:
+ /*
+ * Since pl011 registers are 32-bit registers, all registers
+ * are handled similarly allowing 8-bit, 16-bit and 32-bit
+ * accesses.
+ */
This comment should be on top of the declaration of
vpl011_reg32_check_access.
+ if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
+
+ *r = vreg_reg32_extract(vpl011_read_data(v->domain), info);
+ return 1;
+
+ case RSR:
+ if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
+
+ /* It always returns 0 as there are no physical errors. */
+ *r = 0;
+ return 1;
+
+ case FR:
+ if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
+
+ *r = vreg_reg32_extract(vpl011->uartfr, info);
You need to ensure that uartfr is read only once because
vreg_reg32_extract does not currently ensure that.
+ return 1;
+
+ case RIS:
+ if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
+
+ *r = vreg_reg32_extract(vpl011->uartris, info);
Ditto.
+ return 1;
+
+ case MIS:
+ if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
+
+ *r = vreg_reg32_extract(vpl011->uartris &
+ vpl011->uartimsc, info);
Ditto.
+ return 1;
+
+ case IMSC:
+ if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
+
+ *r = vreg_reg32_extract(vpl011->uartimsc, info);
Ditto.
+ return 1;
+
+ case ICR:
+ if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
+
+ /* Only write is valid. */
+ return 0;
+
+ default:
+ gprintk(XENLOG_ERR, "vpl011: unhandled read r%d offset %#08x\n",
+ dabt.reg, vpl011_reg);
+ return 0;
+ }
+
+ return 1;
+
+bad_width:
+ gprintk(XENLOG_ERR, "vpl011: bad read width %d r%d offset %#08x\n",
+ dabt.size, dabt.reg, vpl011_reg);
+ domain_crash_synchronous();
+ return 0;
+
+}
+
+static int vpl011_mmio_write(struct vcpu *v,
+ mmio_info_t *info,
+ register_t r,
+ void *priv)
+{
+ struct hsr_dabt dabt = info->dabt;
+ uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
+ struct vpl011 *vpl011 = &v->domain->arch.vpl011;
+ struct domain *d = v->domain;
+ unsigned long flags;
+
+ switch ( vpl011_reg )
+ {
+ case DR:
+ {
+ uint32_t data = 0;
+
+ /*
+ * Since pl011 registers are 32-bit registers, all registers
+ * are handled similarly allowing 8-bit, 16-bit and 32-bit
+ * accesses.
+ */
See above.
+ if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
+
+ vreg_reg32_update(&data, r, info);
+ data &= 0xFF;
+ vpl011_write_data(v->domain, data);
+ return 1;
+ }
+ case RSR: /* Nothing to clear. */
+ if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
+
+ return 1;
+
+ case FR:
+ case RIS:
+ case MIS:
+ goto write_ignore;
+
+ case IMSC:
+ if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
+
+ VPL011_LOCK(d, flags);
+ vreg_reg32_update(&vpl011->uartimsc, r, info);
+ VPL011_UNLOCK(d, flags);
+ vpl011_update(v->domain);
I think this should be call with under the lock.
+ return 1;
+
+ case ICR:
+ if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
+
+ VPL011_LOCK(d, flags);
+ vreg_reg32_clearbits(&vpl011->uartris, r, info);
+ VPL011_UNLOCK(d, flags);
+ vpl011_update(d);
Ditto.
+ return 1;
+
+ default:
+ gprintk(XENLOG_ERR, "vpl011: unhandled write r%d offset %#08x\n",
+ dabt.reg, vpl011_reg);
+ return 0;
+ }
+
+write_ignore:
+ return 1;
+
+bad_width:
+ gprintk(XENLOG_ERR, "vpl011: bad write width %d r%d offset %#08x\n",
+ dabt.size, dabt.reg, vpl011_reg);
+ domain_crash_synchronous();
+ return 0;
+
+}
+
+static const struct mmio_handler_ops vpl011_mmio_handler = {
+ .read = vpl011_mmio_read,
+ .write = vpl011_mmio_write,
+};
+
+static void vpl011_data_avail(struct domain *d)
+{
+ unsigned long flags;
+ struct vpl011 *vpl011 = &d->arch.vpl011;
+ struct xencons_interface *intf = vpl011->ring_buf;
+ XENCONS_RING_IDX in_cons = intf->in_cons;
+ XENCONS_RING_IDX in_prod = intf->in_prod;
+ XENCONS_RING_IDX out_cons = intf->out_cons;
+ XENCONS_RING_IDX out_prod = intf->out_prod;
Same as above for the barrier.
+ XENCONS_RING_IDX in_ring_qsize, out_ring_qsize;
+
+ VPL011_LOCK(d, flags);
+
+ in_ring_qsize = xencons_queued(in_prod,
+ in_cons,
+ sizeof(intf->in));
+
+ out_ring_qsize = xencons_queued(out_prod,
+ out_cons,
+ sizeof(intf->out));
+
+ /* Update the uart rx state if the buffer is not empty. */
+ if ( in_ring_qsize != 0 )
+ {
+ vpl011->uartfr &= ~RXFE;
+ if ( in_ring_qsize == sizeof(intf->in) )
+ vpl011->uartfr |= RXFF;
+ vpl011->uartris |= RXI;
+ }
+
+ /* Update the uart tx state if the buffer is not full. */
+ if ( out_ring_qsize != sizeof(intf->out) )
+ {
+ vpl011->uartfr &= ~TXFF;
+ vpl011->uartris |= TXI;
+ if ( out_ring_qsize == 0 )
+ {
+ vpl011->uartfr &= ~BUSY;
+ vpl011->uartfr |= TXFE;
+ }
+ }
+
+ VPL011_UNLOCK(d, flags);
+
+ vpl011_update(d);
See my comment above for the calling vpl011_update
+}
+
+
+static void vpl011_notification(struct vcpu *v, unsigned int port)
+{
+ vpl011_data_avail(v->domain);
+}
+
+int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
+{
+ int rc;
+ struct vpl011 *vpl011 = &d->arch.vpl011;
+
+ if ( vpl011->ring_buf )
+ return 0;
IHMO, you should return an error if the PL011 is already initialized.
This should never happen.
+
+ /* Map the guest PFN to Xen address space. */
+ rc = prepare_ring_for_helper(d,
+ gfn_x(info->gfn),
+ &vpl011->ring_page,
+ &vpl011->ring_buf);
+ if ( rc < 0 )
+ goto out;
+
+ rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
+ if ( !rc )
+ {
+ rc = -EINVAL;
+ goto out1;
+ }
+
+ register_mmio_handler(d, &vpl011_mmio_handler,
+ GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
Again, you register MMIO handler but never remove them. So if this call
fail, you will end up with the handlers existing but the rest
half-initialized which likely lead to a segfault, or worst leaking data.
+
+ spin_lock_init(&vpl011->lock);
+
+ rc = alloc_unbound_xen_event_channel(d, 0, info->console_domid,
+ vpl011_notification);
+ if ( rc < 0 )
You
+ goto out2;
+
+ vpl011->evtchn = info->evtchn = rc;
+
+ return 0;
+
+out2:
+ xfree(d->arch.vmmio.handlers);
+ vgic_free_virq(d, GUEST_VPL011_SPI);
+
+out1:
+ destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
+
+out:
+ return rc;
+}
+
+void domain_vpl011_deinit(struct domain *d)
+{
+ struct vpl011 *vpl011 = &d->arch.vpl011;
+
+ if ( !vpl011->ring_buf )
You will bail out if ring_buf is NULL. However, if you called
domain_vpl011_init first and it failed, you may have ring_buf set but
the rest not fully updated. This means that you will free garbagge.
I think this could be solved by reinitialize ring_buf if an error occur
in domain_vpl011_init.
+ return;
+
+ free_xen_event_channel(d, vpl011->evtchn);
+ destroy_ring_for_helper(&vpl011->ring_buf, vpl011->ring_page);
+ xfree(d->arch.vmmio.handlers);
+}
[...]
diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
new file mode 100644
index 0000000..b3e332d
--- /dev/null
+++ b/xen/include/asm-arm/vpl011.h
@@ -0,0 +1,74 @@
+/*
+ * include/xen/vpl011.h
+ *
+ * Virtual PL011 UART
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _VPL011_H_
+
We tend to keep #ifndef and #define together. So please drop the newline
here.
+#ifdef CONFIG_VPL011_CONSOLE
+int domain_vpl011_init(struct domain *d,
+ struct vpl011_init_info *info);
+void domain_vpl011_deinit(struct domain *d);
+#else
+static inline int domain_vpl011_init(struct domain *d,
+ struct vpl011_init_info *info)
+{
+ return -ENOSYS;
+}
+
+static inline void domain_vpl011_deinit(struct domain *d) { }
+#endif
+
+#endif
+
Please drop this newline.
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
