[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Minios-devel] [UNIKRAFT PATCHv4 20/43] plat/kvm: Add console library for Arm64





On 07/08/2018 09:35 PM, Julien Grall wrote:
Hi,

On 07/06/2018 10:03 AM, Wei Chen wrote:
QEMU/KVM provide a PL011 uart for virtual machine, so we
implement a PL011 device driver library for console.

Could we have the PL011 driver outside plat/kvm/arm/console.c? This could be useful for other architecture.

I meant platform here.



Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
---
  plat/kvm/arm/console.c | 156 +++++++++++++++++++++++++++++++++++++++++
  1 file changed, 156 insertions(+)
  create mode 100644 plat/kvm/arm/console.c

diff --git a/plat/kvm/arm/console.c b/plat/kvm/arm/console.c
new file mode 100644
index 0000000..5ee59d6
--- /dev/null
+++ b/plat/kvm/arm/console.c
@@ -0,0 +1,156 @@
+/* SPDX-License-Identifier: ISC */

Same remark as before for SPDX. But this is a bit confusing, some of the code is BSD-3, the other are ISC (not sure what it stands for). What is the rationale behind it?

+/*
+ * Authors: Wei Chen <Wei.Chen@xxxxxxx>
+ *
+ * Copyright (c) 2018 Arm Ltd.
+ *
+ * Permission to use, copy, modify, and/or distribute this software
+ * for any purpose with or without fee is hereby granted, provided
+ * that the above copyright notice and this permission notice appear
+ * in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
+ * WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE
+ * AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
+ * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+ * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+#include <string.h>
+#include <libfdt.h>
+#include <kvm/console.h>
+#include <uk/plat/console.h>
+#include <uk/assert.h>
+#include <uk/essentials.h>
+#include <uk/print.h>
+#include <arm/cpu.h>
+
+/* PL011 UART registers and masks*/
+/* Data register */
+#define UART_DR        0x00
+
+/* Flag register */
+#define UART_FR        0x06
+#define FR_TXFF        (1 << 5)    /* Transmit FIFO/reg full */
+#define FR_RXFE        (1 << 4)    /* Receive FIFO/reg empty */
+
+/* Line control register */
+#define UART_LCR_H        0x0b
+#define LCR_H_WLEN8        (0x3 << 5)  /* Data width is 8-bits */
+
+/* Control register */
+#define UART_CR        0x0c
+#define CR_RXE        (1 << 9)    /* Receive enable */
+#define CR_TXE        (1 << 8)    /* Transmit enable */
+#define CR_UARTEN    (1 << 0)    /* UART enable */
+
+/* Interrupt mask set/clear register */
+#define UART_IMSC    0x0e
+
+/* Macros to access PL011 Registers with base address */
+#define PL011_REG_READ(r)        REG_READ16(pl011_uart_bas + (r))
+#define PL011_REG_WRITE(r, v)    REG_WRITE16(pl011_uart_bas + (r), v)
+
+/*
+ * Before pl011 uart has been initialized, we user EARLY PRINT UART
+ * to do early print.
+ */
+#define EARLY_PRINT_UART_BAS    0x09000000

This should really be defined in a Makefile for a given platform.

+static uint64_t pl011_uart_bas = EARLY_PRINT_UART_BAS;
+
+extern void *_libkvmplat_dtb;
+
+static void init_pl011(uint64_t bas)
+{
+    pl011_uart_bas = bas;
+
+    /* Mask all interrupts */
+    PL011_REG_WRITE(UART_IMSC, PL011_REG_READ(UART_IMSC) & 0xf800);
+
+    /* Disable UART for configuration */
+    PL011_REG_WRITE(UART_CR, 0);
+
+    /* Select 8-bits data transmit and receive */
+    PL011_REG_WRITE(UART_LCR_H, \
+        (PL011_REG_READ(UART_IMSC) & 0xff00) | LCR_H_WLEN8);
+
+    /* Just enable UART and data transmit/receive */
+    PL011_REG_WRITE(UART_CR, CR_TXE | CR_UARTEN);
+}
+
+void _libkvmplat_init_console(void)
+{
+    int offset, len;
+    const uint64_t *regs;
+    uint64_t uart_bas;
+
+    uk_printd(DLVL_INFO, "Serial initializing\n");
+
+    offset = fdt_node_offset_by_compatible(_libkvmplat_dtb, 0, "arm,pl011");
+    if (!offset)
+        UK_CRASH("No console uart found!\n");

s/uart/UART/

+
+    regs = fdt_getprop(_libkvmplat_dtb, offset, "reg", &len);
+    if (regs == NULL && len < 16)
+        UK_CRASH("Bad 'reg' property: %p %d\n", regs, len);

That looks totally wrong to me. What does prevent the DT to have only one cells to describe the address and the size?

I would rather implement a bunch of helpers to parse the DT correctly rather than assuming QEMU will always do that. The day, it is slightly changing you are going to be in deep trouble.

+
+    uart_bas = fdt64_to_cpu(regs[0]);
+    uk_printd(DLVL_INFO, "Found PL011 UART on: 0x%lx\n", uart_bas);
+
+    init_pl011(uart_bas);
+    uk_printd(DLVL_INFO, "PL011 UART initialized\n");
+}
+
+int ukplat_coutd(const char *str, uint32_t len)
+{
+    return ukplat_coutk(str, len);
+}
+
+static void pl011_write(char a)
+{
+    /* Wait until TX FIFO becomes empty */
+    while (PL011_REG_READ(UART_FR) & FR_TXFF)
+        ;
+
+    PL011_REG_WRITE(UART_DR, a & 0xff);
+}
+
+static void pl011_putc(char a)
+{
+    if (a == '\n')
+        pl011_write('\r');
+    pl011_write(a);
+}
+
+/* Try to get data from pl011 UART without block */

s/block/blocking/

+static int pl011_getc(void)
+{
+    /* If RX FIFO is empty, return -1 immediately */
+    if (PL011_REG_READ(UART_FR) & FR_RXFE)
+        return -1;
+
+    return (int) (PL011_REG_READ(UART_DR) & 0xff);
+}
+
+int ukplat_coutk(const char *buf, unsigned int len)
+{
+    for (unsigned int i = 0; i < len; i++)
+        pl011_putc(buf[i]);
+    return len;
+}
+
+int ukplat_cink(char *buf, unsigned int maxlen)
+{
+    int ret;
+    unsigned int num = 0;
+
+    while (num < maxlen
+            && (ret = pl011_getc()) >= 0) {
+        *(buf++) = (char) ret;
+        num++;
+    }
+
+    return (int) num;
+}


Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.