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

[Julien Grall <julien.grall@xxxxxxx>]

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.
> 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