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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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.

> > > 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().
Cheers,

--
Julien Grall

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