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

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

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_DR                0x00
Suggest to rename the register map macros as UART_<REGNAME>_OFFSET or 
REG_<REGNAME>_OFFSET?
> +
> +/* 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
> +
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))))

> +/* 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
The address configuration could be a part of Config.uk, with the early 
print option enabled.

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;

> +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?
2) Do we make a decision to disable FIFO mode, bit '4' on the control 
register[1]?
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?
> +       /* 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.
> +       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.
> +       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.
> +       /* 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

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