Hi Bhupinder,
On 02/21/2017 11:25 AM, Bhupinder Thakur wrote:
Add emulation code to emulate read/write access to pl011 registers
and pl011 interrupts:
- It emulates DR read/write by reading and writing from/to the IN
and OUT ring buffers and raising an event to dom0 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
A link to the documentation would have been helpful.
Signed-off-by: Bhupinder Thakur <bhupinder.tha...@linaro.org>
---
xen/arch/arm/Makefile | 1 +
xen/arch/arm/vpl011.c | 366 ++++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/vpl011.h | 208 ++++++++++++++++++++++++
xen/common/Kconfig | 6 +
xen/include/asm-arm/domain.h | 15 ++
xen/include/public/arch-arm.h | 5 +
6 files changed, 601 insertions(+)
create mode 100644 xen/arch/arm/vpl011.c
create mode 100644 xen/arch/arm/vpl011.h
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 7afb8a3..a94bdab 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -49,6 +49,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..88ba968
--- /dev/null
+++ b/xen/arch/arm/vpl011.c
@@ -0,0 +1,366 @@
+/*
+ * 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/config.h>
xen/config.h is included by default. Please drop it.
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/errno.h>
+#include <xen/guest_access.h>
+#include <xen/sched.h>
+#include <xen/monitor.h>
Why do you need to include monitor.h?
+#include <xen/event.h>
+#include <xen/vmap.h>
+
+#include <xsm/xsm.h>
Ditto.
+
+#include <public/xen.h>
+#include <public/hvm/params.h>
+#include <public/hvm/hvm_op.h>
+
+#include <asm/hypercall.h>
Ditto.
+#include "vpl011.h"
+
+static int vpl011_mmio_read(struct vcpu *v, mmio_info_t *info, register_t *r,
void *priv)
+{
+ unsigned char ch;
+
+ switch (info->gpa - GUEST_PL011_BASE)
Coding style:
switch ( ... )
+ {
+ case VPL011_UARTCR_OFFSET:
Coding style: the case should be aligned to {. E.g
{
case ...
Also, I would prefer if you don't include _OFFSET in all the name.
Furthermore, can you please order the case by offset in the MMIO region.
This would help to find if a register has been emulated or not.
Lastly, the user may have requested to read 8-bit, 16-bit, 32-bit. But
you always return a 32-bit. Is a guest allowed to access using any size
and in the middle of a register? Give a look on what is done for
vgic-v{2,3}.c to check the size and read register. You may want to pull
some code out to re-use here.
+ *r = v->domain->arch.vpl011.control;
Similarly, I would prefer if the name of the field match the register name.
+ break;
+ case VPL011_UARTDR_OFFSET:
+ vpl011_read_data(v->domain, &ch);
Should not you check the return value of vpl011_read_data? Also, what if
there is no data?
+ *r = ch;
+ break;
+ case VPL011_UARTFR_OFFSET:
+ *r = v->domain->arch.vpl011.flag;
I am fairly surprised that none of this code is actually protected by
lock. For instance the update of flag is not atomic. So is it safe?
+ break;
+ case VPL011_UARTIMSC_OFFSET:
+ *r = v->domain->arch.vpl011.intr_mask;
+ break;
+ case VPL011_UARTICR_OFFSET:
+ *r = 0;
Looking at the spec, this register is write-only. So why do you
implement RAZ?
+ break;
+ case VPL011_UARTRIS_OFFSET:
+ *r = v->domain->arch.vpl011.raw_intr_status;
+ break;
+ case VPL011_UARTMIS_OFFSET:
+ *r = v->domain->arch.vpl011.raw_intr_status &
+ v->domain->arch.vpl011.intr_mask;
+ break;
+ case VPL011_UARTDMACR_OFFSET:
+ *r = 0; /* uart DMA is not supported. Here it always returns 0 */
My understanding of the spec is DMA is not optional. So what would
happen if the guest tries to enable it?
+ break;
+ case VPL011_UARTRSR_OFFSET:
+ *r = 0; /* it always returns 0 as there are no physical errors */
This register contains contains the bit OE that tells whether the FIFO
is full or not. The FIFO here is the PV ring, so maybe we should set
this bit if the ring is full.
+ break;
+ default:
+ printk ("vpl011_mmio_read: invalid switch case %d\n",
(int)(info->gpa - GUEST_PL011_BASE));
Coding style: printk(...).
Also, printk is not ratelimited by default. Please use gprintk(...)
which will be ratelimited and print the domain information. This is
useful when you have multiple guest.
+ break;
+ }
+
+ return VPL011_EMUL_OK;
Please use plain value as the return is not pl011 specific. Also, I am a
bit surprised that you return "ok" even when a register as not been
emulated. IHMO, a data abort should be sent to the guest.
Furthermore, looking at the overall function, I think it should be
better if you re-use the template used by the other emulation (see
vgic-v2.c) for instance. They have labels that helps to know what's
going on.
I will stop here in the review for today and will comment the rest tomorrow.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
