Re: [Minios-devel] [UNIKRAFT PATCHv5 20/46] plat/common: Add early debug console library for Arm64

["Wei Chen (Arm Technology China)" <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: 2018年9月7日 23:10
> To: Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; simon.kuenzer@xxxxxxxxx
> Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv5 20/46] plat/common: Add early
> debug console library for Arm64
> 
> Hi,
> 
> On 08/10/2018 08:08 AM, Wei Chen wrote:
> > From: Wei Chen <Wei.Chen@xxxxxxx>
> >
> > PL011 UART is used frequently for virtual machine or bare metal,
> > so we implement a simple PL011 device driver library for early
> > debug console. Unikraft Kconfig provides a KVM_EARLY_DEBUG_PL011_UART
> > for early debug console UART address. If users want to enable PL011
> > for early debug, they can configure the base address in this variable.
> >
> > Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> > ---
> >   plat/common/arm/console.c | 139 ++++++++++++++++++++++++++++++++++++++
> 
> While PL011 is a popular UART, this is not the only one existing on Arm.
> So I would rename this to pl011.c
> 

Ok.

> >   plat/kvm/Makefile.uk      |   3 +
> >   plat/kvm/arm/setup.c      |   2 +-
> >   3 files changed, 143 insertions(+), 1 deletion(-)
> >   create mode 100644 plat/common/arm/console.c
> >
> > diff --git a/plat/common/arm/console.c b/plat/common/arm/console.c
> > new file mode 100644
> > index 0000000..5d1b5d4
> > --- /dev/null
> > +++ b/plat/common/arm/console.c
> > @@ -0,0 +1,139 @@
> > +/* 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 <uk/plat/console.h>
> > +#include <uk/assert.h>
> > +#include <arm/cpu.h>
> > +
> > +/* PL011 UART registers and masks*/
> > +/* Data register */
> > +#define REG_UARTDR_OFFSET  0x00
> > +
> > +/* Receive status register/error clear register */
> > +#define REG_UARTRSR_OFFSET 0x04
> > +#define REG_UARTECR_OFFSET 0x04
> > +
> > +/* Flag register */
> > +#define REG_UARTFR_OFFSET  0x18
> > +#define FR_TXFF                    (1 << 5)    /* Transmit FIFO/reg full */
> > +#define FR_RXFE                    (1 << 4)    /* Receive FIFO/reg empty */
> > +
> > +/* Integer baud rate register */
> > +#define REG_UARTIBRD_OFFSET        0x24
> > +/* Fractional baud rate register */
> > +#define REG_UARTFBRD_OFFSET        0x28
> > +
> > +/* Line control register */
> > +#define REG_UARTLCR_H_OFFSET       0x2C
> > +#define LCR_H_WLEN8                (0x3 << 5)  /* Data width is 8-bits */
> > +
> > +/* Control register */
> > +#define REG_UARTCR_OFFSET  0x30
> > +#define CR_RXE                     (1 << 9)    /* Receive enable */
> > +#define CR_TXE                     (1 << 8)    /* Transmit enable */
> > +#define CR_UARTEN          (1 << 0)    /* UART enable */
> > +
> > +/* Interrupt FIFO level select register */
> > +#define REG_UARTIFLS_OFFSET        0x34
> > +/* Interrupt mask set/clear register */
> > +#define REG_UARTIMSC_OFFSET        0x38
> > +/* Raw interrupt status register */
> > +#define REG_UARTRIS_OFFSET 0x3C
> > +/* Masked interrupt status register */
> > +#define REG_UARTMIS_OFFSET 0x40
> > +/* Interrupt clear register */
> > +#define REG_UARTICR_OFFSET 0x44
> > +
> > + /* PL011 UART base address */
> > +#if defined(CONFIG_KVM_EARLY_DEBUG_PL011_UART)
> 
> I don't think this should be KVM specific. Other platform might want to
> use it.
> 

OK,

> > +static uint64_t pl011_uart_bas = CONFIG_KVM_EARLY_DEBUG_PL011_UART;
> > +#else
> > +static uint64_t pl011_uart_bas;
> > +#endif
> 
> A better way to write this code would be:
> 
> #ifndef CONFIG_KVM_EARLY_DEBUG_PL011_UART
> #define CONFIG_KVM_EARLY_DEBUG_PL011_UART 0
> #endif
> 
> static uint64_t pl011_uart_bas = CONFIG_KVM_EARLY_DEBUG_PL011_UART;
> 

Thanks, that seems better : )

> > +
> > +/* Macros to access PL011 Registers with base address */
> > +#define PL011_REG(r)               ((uint16_t *)(pl011_uart_bas + (r)))
> > +#define PL011_REG_READ(r)  ioreg_read16(PL011_REG(r))
> > +#define PL011_REG_WRITE(r, v)      ioreg_write16(PL011_REG(r), v)
> > +
> > +int ukplat_coutd(const char *str, uint32_t len)
> > +{
> > +   return ukplat_coutk(str, len);
> > +}
> > +
> > +static void pl011_write(char a)
> > +{
> > +   /*
> > +    * Avoid using the UART before base address initialized,
> > +    * or CONFIG_KVM_EARLY_DEBUG_PL011_UART doesn't be enabled.
> > +    */
> > +   if (!pl011_uart_bas)
> 
> Nothing actually prevents to the PL011 to start at IPA 0. But this would
> not be supported it.
> 
> I am getting really tempt to place reshuffle Xen gues layout and put
> some RAM/PL011 at address 0 to catch anyone rely on IPA 0 been invalid.
> 

Oh, You beat me. Yes, PL011 start at IPA 0 is possible. But I don't know
how to distinguish PL011 at IPA 0 or #ifndef CONFIG_KVM_EARLY_DEBUG_PL011_UART.
I had tried not to check (!pl011_uart_bas), it will generate an exception,
and the exception entry will call PL011 to print message. It's an infinite loop.

As you said this would not be supported, I decide not to support PL011
at IPA 0 ; (

> > +           return;
> > +
> > +   /* Wait until TX FIFO becomes empty */
> > +   while (PL011_REG_READ(REG_UARTFR_OFFSET) & FR_TXFF)
> > +           ;
> > +
> > +   PL011_REG_WRITE(REG_UARTDR_OFFSET, 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 blocking */
> > +static int pl011_getc(void)
> > +{
> > +   /*
> > +    * Avoid using the UART before base address initialized,
> > +    * or CONFIG_KVM_EARLY_DEBUG_PL011_UART doesn't be enabled.
> > +    */
> > +   if (!pl011_uart_bas)
> > +           return -1;
> > +
> > +   /* If RX FIFO is empty, return -1 immediately */
> > +   if (PL011_REG_READ(REG_UARTFR_OFFSET) & FR_RXFE)
> > +           return -1;
> > +
> > +   return (int) (PL011_REG_READ(REG_UARTDR_OFFSET) & 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;
> > +}
> > diff --git a/plat/kvm/Makefile.uk b/plat/kvm/Makefile.uk
> > index 9f2a01f..4ec90f3 100644
> > --- a/plat/kvm/Makefile.uk
> > +++ b/plat/kvm/Makefile.uk
> > @@ -49,6 +49,9 @@ endif
> >   ## Architecture library definitions for arm64
> >   ##
> >   ifeq ($(CONFIG_ARCH_ARM_64),y)
> > +ifeq ($(findstring y,$(CONFIG_KVM_KERNEL_SERIAL_CONSOLE)
> $(CONFIG_KVM_DEBUG_SERIAL_CONSOLE)),y)
> > +LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
> $(UK_PLAT_COMMON_BASE)/arm/console.c|common
> > +endif
> >   LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) += $(LIBKVMPLAT_BASE)/arm/entry64.S
> >   LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) += $(LIBKVMPLAT_BASE)/arm/setup.c
> >   endif
> > diff --git a/plat/kvm/arm/setup.c b/plat/kvm/arm/setup.c
> > index 2fc4538..e2ece8e 100644
> > --- a/plat/kvm/arm/setup.c
> > +++ b/plat/kvm/arm/setup.c
> > @@ -22,5 +22,5 @@
> >
> >   void _libkvmplat_start(void *dtb_pointer)
> >   {
> > -   UK_BUG();
> > +   uk_printd(DLVL_INFO, "Entering from KVM (arm64)...\n");
> 
> I don't think this belongs to this patch. You don't really implement
> _libkvmplat_start. Just modify the printk.

So, just keep an empty function here?

> 
> >   }
> >
> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel