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

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Wei Chen,

As a general comment, I would try to offload as much improvements onto 
subsequent patch series as it wouldn't keep this patch series open for 
long. If there are bugs we fix them as a part of this patch.
On 07/16/2018 10:07 AM, Wei Chen wrote:
> Hi Sharan,
>
> > -----Original Message-----
> > From: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> > Sent: 2018年7月14日 23:56
> > To: minios-devel@xxxxxxxxxxxxxxxxxxxx; Wei Chen <Wei.Chen@xxxxxxx>
> > Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 20/43] plat/kvm: Add console
> > library for Arm64
> >
> > Hello Wei Chen,
> >
> > Please find my comment in line:
> >
> >
> > I agree we could move the driver specific calls
> >
> > * init_pl011
> > * _libkvmplat_init_console
> > * pl011_putc
> > * pl011_getc
> >
> >    as a part of the console driver. But I would avoid doing this as a
> > part of this patch series is already extensive.
> >
> >
> > On 07/06/2018 11:03 AM, Wei Chen wrote:
> > > QEMU/KVM provide a PL011 uart for virtual machine, so we
> > > implement a PL011 device driver library for console.
> > >
> > > 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 */
> > > +/*
> > > + * 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_DR0x00
> > Suggest to rename the register map macros as UART_<REGNAME>_OFFSET or
> > REG_<REGNAME>_OFFSET?
> Ok, I prefer the first. And I also have one concern that, because
> we are porting lots of code from other systems like FreeBSD. We also
> copied their macros like registers' definition. So we will have lots
> of different register macro styles. Should we need a standard to define
> register macros for Unikraft?
I agree, it may be wise to discuss about standard way of describing 
these register macro. We are still discussing on how far we need to 
standardize it as these are internal driver register map. If you have 
any suggestions on the way to standardize them, please send it in.
> > > +
> > > +/* Flag register */
> > > +#define UART_FR0x06
> > > +#define FR_TXFF(1 << 5)    /* Transmit FIFO/reg full */
> > > +#define FR_RXFE(1 << 4)    /* Receive FIFO/reg empty */
> > > +
> > > +/* Line control register */
> > > +#define UART_LCR_H0x0b
> > > +#define LCR_H_WLEN8(0x3 << 5)  /* Data width is 8-bits */
> > > +
> > > +/* Control register */
> > > +#define UART_CR0x0c
> > > +#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_IMSC0x0e
> > > +
> > We are adding the offset directly to the uint64_t integer. Is this the
> > expected behavior? Since these 32-bit aligned register offset, shouldn't
> > the offset be multiplied with 4.
> >
> > For example I tried to get address calculation expanded without reading
> > the pointer and I got it expanded as follows,
> > PL011_REG_READ(6) ----> (((const volatile uint16_t*)(pl011_uart_bas + (6))))
> Oh, yes, you're right. Thanks for reviewing so carefully! This is a big
> mistake I have made. I used the FreeBSD's register definition, but I didn't
> use the same access function. So the offset be multiplied with 4.
> I don't know I am lucky or not, if the UART_DR is not zero, this library
> could not work properly ; (
>
> > > +/* 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_BAS0x09000000
> > The address configuration could be a part of Config.uk, with the early
> > print option enabled.
> Yes, I agree. I plan to add a new config option in next version.
>
> > According to the document[1], the peripheral address map is 32-bit
> > aligned I would probably use it as
> > * static volatile uint32_t *pl011_uart_base = EARLY_PRINT_UART_BAS;
> 32-bit alignment doesn't mean this UART can only be placed at address
> lower than 4GB. If some SoC designer place the UART to address higher
> than 4GB, uint32_t is not enough.
No, I am assigning the pointer to a 32-bit unsigned integer as the 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;
> > > +
> > Since we are clearing the interrupt masking, do we also clear the
> > interrupts which were there already.
> >
> > The interrupt clear register is at 0x11 offset.
> > > +/* Mask all interrupts */
> > > +PL011_REG_WRITE(UART_IMSC, PL011_REG_READ(UART_IMSC) & 0xf800);
> > > +
> > > +/* Disable UART for configuration */
> > > +PL011_REG_WRITE(UART_CR, 0);
> > > +
> > In the below code,
> > 1) Why are we reading from the interrupt masking register and writing it
> > to Line Control Register?
> It's a typo, it should be:
> PL011_REG_WRITE(UART_LCR_H, (PL011_REG_READ(UART_LCR_H) & 0xff00) | 
> LCR_H_WLEN8);
>
> > 2) Do we make a decision to disable FIFO mode, bit '4' on the control
> > register[1]?
> In this patch series, I just want to see hello world as soon as possible, so
> I select the simplest way to print strings. Maybe we can have another patch
> to enable the FIFO. But I still have some concern, on a virtual machine,
> does the FIFO can improve the virtual UART performance? For a real UART I
> think the answer is YES. And for a debug UART, should we need to enable the
> FIFO? Does it will increase the possibility of losing data while crash?
Agreed, we could enable it as part of another patch series.

> > 3) In the documentation[1] the following is described in section 3.3.7
> >
> > "
> > The UARTLCR_H, UARTIBRD, and UARTFBRD registers form the single 30-bit
> > wide UARTLCR Register that is updated on a single write strobe generated
> > by a UARTLCR_H write. So, to internally update the contents of UARTIBRD
> > or UARTFBRD, a UARTLCR_H write must always be performed at the end.
> > "
> > We are not initializing the integer baud rate and the fractional baud
> > rate. Are we expecting something things to be configured by qemu?
> Yes, because we're a virtual UART, any baud rate is the same, QEMU will
> not check these values. But for a bare metal, we need to configure them,
> and we may need to provide a parameter for user to select baud rate.
>
> I want to improve this library later to make it more friendly for a
> bare metal.
I agree. Since we were discussing about moving some driver code 
separately, I recommend adding these changes as a part of that series.
> > > +/* 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");
> > > +
> > The code does not seem to be correct. The function description
> > documentation in lib/fdt/include/libfdt.h explains in detail on the how
> > to parse with compatible string. Please use it as reference.
> Sorry, did you mean I can't use the 0 as startoffset?
Yes we should be using -1.

As well the subsequent check (!offset). The API describes that on 
success the function return 0 or offset and on error a negative integer.
> > > +offset = fdt_node_offset_by_compatible(_libkvmplat_dtb, 0, "arm,pl011");
> > > +if (!offset)
> > > +UK_CRASH("No console uart found!\n");
> > > +
> > As an improvement, we could try to read the address cell and size cells
> > of the DTB to determine the len variable. If we should not hard his value.
> OK.
>
> > > +regs = fdt_getprop(_libkvmplat_dtb, offset, "reg", &len);
> > > +if (regs == NULL && len < 16)
> > > +UK_CRASH("Bad 'reg' property: %p %d\n", regs, len);
> > > +
> > > +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)
> > > +{
> > Do we want to wait infinitely for the buffer to be empty?
> > If we are using a single byte Transmit FIFO, we could use the busy bit
> > (Bit nr. 3) to check if the UART is busy transmitting data.
> Mmm, BUSY "This bit remains set until the complete byte, including all the
> stop bits, has been sent from the shift register"
> But we don't need to wait shift register become empty. When transmit holding
> Register is empty, we can write data to FIFO. So I think TXFF here is more
> Sensible.
I agree with the TXFF check.
> > > +/* 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 */
> > > +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;
> > > +}
> > [1]  PL011:
> > http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf
> >
> > Thanks & Regards
> > Sharan
> IMPORTANT NOTICE: The contents of this email and any attachments are 
> confidential and may also be privileged. If you are not the intended recipient, 
> please notify the sender immediately and do not disclose the contents to any 
> other person, use it for any purpose, or store or copy the information in any 
> medium. Thank you.
Thanks & Regards
Sharan

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