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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: 2018年7月9日 4:35
> To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx;
> simon.kuenzer@xxxxxxxxx
> Cc: Kaly Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 20/43] plat/kvm: Add console
> library for Arm64
> 
> 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.
> 

Yes, I think this code can be used by Xen or other platforms later.

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

Maybe I can provide an additional CONFIG option for user to enable/modify
the DEBUG UART 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;
> > +
> > +   /* 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.

Yes, I can't guarantee that.

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