[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
Hello Wei Chen,
On 07/18/2018 04:44 AM, Wei Chen wrote:
Hi Sharan,
-----Original Message-----
From: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
Sent: 2018年7月16日 19:30
To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx
Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 20/43] plat/kvm: Add console
library for Arm64
Hello Wei Chen,
As a general comment, I would try to offload as much improvements onto
subsequent patch series as it wouldn't keep this patch series open for
long. If there are bugs we fix them as a part of this patch.
I agree with you. We don't want this series blocking other series too long.
We have found some bugs and left comments, but we can use subsequent patch
series to fix them one by one.
It is wise to fix the bugs and postpone the improvements to subsequent
patches. I have indicated in line which of the bugs which we focus on
this patch. Please have look at them.
On 07/16/2018 10:07 AM, Wei Chen wrote:
Hi Sharan,
-----Original Message-----
From: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
Sent: 2018年7月14日 23:56
To: minios-devel@xxxxxxxxxxxxxxxxxxxx; Wei Chen <Wei.Chen@xxxxxxx>
Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 20/43] plat/kvm: Add console
library for Arm64
Hello Wei Chen,
Please find my comment in line:
I agree we could move the driver specific calls
* init_pl011
* _libkvmplat_init_console
* pl011_putc
* pl011_getc
as a part of the console driver. But I would avoid doing this as a
part of this patch series is already extensive.
On 07/06/2018 11:03 AM, Wei Chen wrote:
QEMU/KVM provide a PL011 uart for virtual machine, so we
implement a PL011 device driver library for console.
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 */
+/*
+ * 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_DR0x00
Suggest to rename the register map macros as UART_<REGNAME>_OFFSET or
REG_<REGNAME>_OFFSET?
Ok, I prefer the first. And I also have one concern that, because
we are porting lots of code from other systems like FreeBSD. We also
copied their macros like registers' definition. So we will have lots
of different register macro styles. Should we need a standard to define
register macros for Unikraft?
I agree, it may be wise to discuss about standard way of describing
these register macro. We are still discussing on how far we need to
standardize it as these are internal driver register map. If you have
any suggestions on the way to standardize them, please send it in.
+
+/* Flag register */
+#define UART_FR0x06
+#define FR_TXFF(1 << 5) /* Transmit FIFO/reg full */
+#define FR_RXFE(1 << 4) /* Receive FIFO/reg empty */
+
+/* Line control register */
+#define UART_LCR_H0x0b
+#define LCR_H_WLEN8(0x3 << 5) /* Data width is 8-bits */
+
+/* Control register */
+#define UART_CR0x0c
+#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_IMSC0x0e
+
We are adding the offset directly to the uint64_t integer. Is this the
expected behavior? Since these 32-bit aligned register offset, shouldn't
the offset be multiplied with 4.
For example I tried to get address calculation expanded without reading
the pointer and I got it expanded as follows,
PL011_REG_READ(6) ----> (((const volatile uint16_t*)(pl011_uart_bas + (6))))
Oh, yes, you're right. Thanks for reviewing so carefully! This is a big
mistake I have made. I used the FreeBSD's register definition, but I didn't
use the same access function. So the offset be multiplied with 4.
I don't know I am lucky or not, if the UART_DR is not zero, this library
could not work properly ; (
This has to be fixed in this patch.
+/* 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_BAS0x09000000
The address configuration could be a part of Config.uk, with the early
print option enabled.
Yes, I agree. I plan to add a new config option in next version.
According to the document[1], the peripheral address map is 32-bit
aligned I would probably use it as
* static volatile uint32_t *pl011_uart_base = EARLY_PRINT_UART_BAS;
32-bit alignment doesn't mean this UART can only be placed at address
lower than 4GB. If some SoC designer place the UART to address higher
than 4GB, uint32_t is not enough.
No, I am assigning the pointer to a 32-bit unsigned integer as the base
address.
+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;
+
Since we are clearing the interrupt masking, do we also clear the
interrupts which were there already.
The interrupt clear register is at 0x11 offset.
+/* Mask all interrupts */
+PL011_REG_WRITE(UART_IMSC, PL011_REG_READ(UART_IMSC) & 0xf800);
+
+/* Disable UART for configuration */
+PL011_REG_WRITE(UART_CR, 0);
+
In the below code,
1) Why are we reading from the interrupt masking register and writing it
to Line Control Register?
It's a typo, it should be:
PL011_REG_WRITE(UART_LCR_H, (PL011_REG_READ(UART_LCR_H) & 0xff00) |
LCR_H_WLEN8);
This has to be fixed in this patch. The rest of the discussion below can
be done as a part of the subsequent patch.
2) Do we make a decision to disable FIFO mode, bit '4' on the control
register[1]?
In this patch series, I just want to see hello world as soon as possible, so
I select the simplest way to print strings. Maybe we can have another patch
to enable the FIFO. But I still have some concern, on a virtual machine,
does the FIFO can improve the virtual UART performance? For a real UART I
think the answer is YES. And for a debug UART, should we need to enable the
FIFO? Does it will increase the possibility of losing data while crash?
Agreed, we could enable it as part of another patch series.
3) In the documentation[1] the following is described in section 3.3.7
"
The UARTLCR_H, UARTIBRD, and UARTFBRD registers form the single 30-bit
wide UARTLCR Register that is updated on a single write strobe generated
by a UARTLCR_H write. So, to internally update the contents of UARTIBRD
or UARTFBRD, a UARTLCR_H write must always be performed at the end.
"
We are not initializing the integer baud rate and the fractional baud
rate. Are we expecting something things to be configured by qemu?
Yes, because we're a virtual UART, any baud rate is the same, QEMU will
not check these values. But for a bare metal, we need to configure them,
and we may need to provide a parameter for user to select baud rate.
I want to improve this library later to make it more friendly for a
bare metal.
I agree. Since we were discussing about moving some driver code
separately, I recommend adding these changes as a part of that series.
+/* 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");
+
The code does not seem to be correct. The function description
documentation in lib/fdt/include/libfdt.h explains in detail on the how
to parse with compatible string. Please use it as reference.
Sorry, did you mean I can't use the 0 as startoffset?
Yes we should be using -1.
As well the subsequent check (!offset). The API describes that on
success the function return 0 or offset and on error a negative integer.
This has to be fixed in this patch.
+offset = fdt_node_offset_by_compatible(_libkvmplat_dtb, 0, "arm,pl011");
+if (!offset)
+UK_CRASH("No console uart found!\n");
+
As an improvement, we could try to read the address cell and size cells
of the DTB to determine the len variable. If we should not hard his value.
OK.
+regs = fdt_getprop(_libkvmplat_dtb, offset, "reg", &len);
+if (regs == NULL && len < 16)
+UK_CRASH("Bad 'reg' property: %p %d\n", regs, len);
+
+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)
+{
Do we want to wait infinitely for the buffer to be empty?
If we are using a single byte Transmit FIFO, we could use the busy bit
(Bit nr. 3) to check if the UART is busy transmitting data.
Mmm, BUSY "This bit remains set until the complete byte, including all the
stop bits, has been sent from the shift register"
But we don't need to wait shift register become empty. When transmit holding
Register is empty, we can write data to FIFO. So I think TXFF here is more
Sensible.
I agree with the TXFF check.
+/* 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 */
+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;
+}
[1] PL011:
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf
Thanks & Regards
Sharan
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
Thanks & Regards
Sharan
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
Thanks & Regards
Sharan
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
