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@xxxxxxxxxx>
> Sent: 2018年9月10日 19:16
> To: Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>; Julien Grall
> <Julien.Grall@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
> 
> 
> 
> On 10/09/18 10:12, Wei Chen (Arm Technology China) wrote:
> > Hi Julien,
> 
> Hi,
> 
> >> -----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.
> 
> If I understand correctly, KVM_EARLY_DEBUG_PL011_UART will exist if
> KVM_DEBUG_SERIAL_CONSOLE is set. So one solution would be to introduce
> an extra variable to check whether the UART has been initialized.
> 
> This would be set to 1 at boot when KVM_DEBUG_SERIAL_CONSOLE is set.
> 

Ok, I understand now.

Just a digression, if an IPA 0 is possible, so I think most of the NULL
Check would be unreliable. For example, lots of fdt_get_property will
return a pointer. You know, most of us will use the if(!pointer) to check
the return value.

> >>> 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?
> 
> No, keep UK_BUG() here. If you want the printk, then it is best to
> introduce when you introduced the UK_BUG().
> 

OK.

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