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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Sharan,

Sorry for missing parts of your comments in last email. I had opened
too many windows to reply, and click the button a little quickly :(

> -----Original Message-----
> From: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> Sent: 2018年7月18日 17:05
> To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 20/43] plat/kvm: Add console
> library for Arm64
> 
> Hello Wei Chen,
> 
> On 07/18/2018 04:44 AM, Wei Chen wrote:
> > Hi Sharan,
> >
> >> -----Original Message-----
> >> From: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> >> Sent: 2018年7月16日 19:30
> >> To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx
> >> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 20/43] plat/kvm: Add console
> >> library for Arm64
> >>
> >> 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.
> >>
> >
> > I agree with you. We don't want this series blocking other series too long.
> > We have found some bugs and left comments, but we can use subsequent patch
> > series to fix them one by one.
> >
> 
> It is wise to fix the bugs and postpone the improvements to subsequent
> patches. I have indicated in line which of the bugs which we focus on
> this patch. Please have look at them.
> 
> >> 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 ; (
> >>>
> 
> This has to be fixed in this patch.
> 

OK.

> >>>>> +/* 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.
> >>

EARLY_PRINT_UART_BAS = 0x4000 0000 0000 ULL the pl011_uart_base will overflow.
Although I know most SoC designer will use low 1GB for IO device.

> >>>>> +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);
> 
> This has to be fixed in this patch. The rest of the discussion below can
> be done as a part of the subsequent patch.

OK.

> >>>
> >>>> 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.
> >>
> 
> This has to be fixed in this patch.
> 

OK.

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