Re: [Minios-devel] [UNIKRAFT RFC PATCH] Implement PL031 RTC library for Arm

["Jianyong Wu (Arm Technology China)" <Jianyong.Wu@xxxxxxx>]

Hi,

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: Saturday, November 10, 2018 1:44 AM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; simon.kuenzer@xxxxxxxxx
> Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd
> <nd@xxxxxxx>; Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>
> Subject: Re: [Minios-devel] [UNIKRAFT RFC PATCH] Implement PL031 RTC
> library for Arm
> 
> Hi,
> 
> On 09/11/2018 08:55, Jianyong Wu wrote:
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> > ---
> >   lib/fdt/include/libfdt.h      |   1 +
> >   plat/common/arm/rtc.c         | 140
> ++++++++++++++++++++++++++++++++++++++++++
> >   plat/common/include/arm/rtc.h |  43 +++++++++++++
> >   plat/kvm/Makefile.uk          |   1 +
> >   4 files changed, 185 insertions(+)
> >   create mode 100644 plat/common/arm/rtc.c
> >   create mode 100644 plat/common/include/arm/rtc.h
> >
> > diff --git a/lib/fdt/include/libfdt.h b/lib/fdt/include/libfdt.h index
> > 05dedbd..96d530d 100644
> > --- a/lib/fdt/include/libfdt.h
> > +++ b/lib/fdt/include/libfdt.h
> > @@ -1873,4 +1873,5 @@ const char *fdt_strerror(int errval);
> >   }
> >   #endif
> >
> > +void *_libkvmplat_dtb;
> 
> This likely belong to a separate patch.
> 
Yeah, this should be removed.

> >   #endif /* _LIBFDT_H */
> > diff --git a/plat/common/arm/rtc.c b/plat/common/arm/rtc.c new file
> > mode 100644 index 0000000..56ee015
> > --- /dev/null
> > +++ b/plat/common/arm/rtc.c
> > @@ -0,0 +1,140 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause */
> > +/*
> > + * Authors: Wei Chen <Wei.Chen@xxxxxxx>
> > + *          Jianyong Wu <Jianyong.Wu@xxxxxxx>
> > + *
> > + * Copyright (c) 2018, Arm Ltd. All rights reserved.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + *
> > + * 1. Redistributions of source code must retain the above copyright
> > + *    notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + *    notice, this list of conditions and the following disclaimer in the
> > + *    documentation and/or other materials provided with the distribution.
> > + * 3. Neither the name of the copyright holder nor the names of its
> > + *    contributors may be used to endorse or promote products derived
> from
> > + *    this software without specific prior written permission.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> CONTRIBUTORS "AS IS"
> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> LIMITED
> > +TO, THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> PARTICULAR
> > +PURPOSE
> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
> > +CONTRIBUTORS BE
> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY,
> > +OR
> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> PROCUREMENT
> > +OF
> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> > +BUSINESS
> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
> > +WHETHER IN
> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> > +OTHERWISE)
> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
> > +ADVISED OF THE
> > + * POSSIBILITY OF SUCH DAMAGE.
> > + *
> > + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> > + */
> > +#include <string.h>
> > +#include <libfdt.h>
> > +#include <uk/assert.h>
> > +#include <uk/essentials.h>
> > +#include <uk/print.h>
> > +#include <arm/cpu.h>
> > +
> > +static void *rtc_base_addr;
> > +uint32_t rtc_boot_seconds;
> > +
> > +/* Define offset of RTC registers */
> > +#define RTC_REG_DR 0
> > +#define RTC_REG_MR 0x4
> > +#define RTC_REG_LR 0x8
> > +#define RTC_REG_CR 0xc
> > +#define RTC_REG_IMSC       0x10
> > +#define RTC_REG_RIS        0x14
> > +#define RTC_REG_MIS        0x18
> > +#define RTC_REG_ICR        0x1c
> > +
> > +#define RTC_REG(r) (rtc_base_addr + (r))
> > +
> > +static char *rtc_device_list[] = {
> > +   "arm,pl031",
> > +};
> > +
> > +uint32_t rtc_read(void)
> > +{
> > +   return ioreg_read32(RTC_REG(RTC_REG_DR));
> > +}
> > +
> > +/*
> > + * set rtc match register comparing with counter
> > + * value to generat a interrupt
> > + */
> > +void rtc_set_match(uint32_t alam)
> > +{
> > +   ioreg_write32(RTC_REG(RTC_REG_MR), alam); }
> > +
> > +void rtc_update(uint32_t val)
> > +{
> > +   ioreg_write32(RTC_REG(RTC_REG_LR), val); }
> > +
> > +void rtc_enable(void)
> > +{
> > +   ioreg_write32(RTC_REG(RTC_REG_CR), 1); }
> > +
> > +/* return rtc status, 1 denotes enable and 0 denotes disable */
> > +uint32_t rtc_get_status(void) {
> > +   uint32_t val;
> > +
> > +   val = ioreg_read32(RTC_REG(RTC_REG_CR));
> > +   val &= 0x1;
> > +   return val;
> > +}
> > +
> > +/* mask alam */
> > +void rtc_mask_intr(void)
> > +{
> > +   ioreg_write32(RTC_REG(RTC_REG_IMSC), 1); }
> > +
> > +/* clear alam mask */
> > +void rtc_unmask_intr(void)
> > +{
> > +   ioreg_write32(RTC_REG(RTC_REG_IMSC), 0); }
> > +
> > +/* return the raw state of rtc interrupt before masking*/ uint32_t
> > +rtc_get_intr_raw_state(void) {
> > +   return ioreg_read32(RTC_REG(RTC_REG_RIS));
> > +}
> > +
> > +/* return interrupt state after interrupt masking */ uint32_t
> > +rtc_get_intr_state(void) {
> > +   return ioreg_read32(RTC_REG(RTC_REG_MIS));
> > +}
> > +
> > +void rtc_clear_intr(void)
> > +{
> > +   ioreg_write32(RTC_REG(RTC_REG_ICR), 1); }
> > +
> > +int _dtb_init_rtc(void *dtb)
> > +{
> > +   uint32_t idx;
> > +   int fdt_rtc, naddr, nsize, size, prop_len, prop_min_len;
> > +   const uint64_t *regs;
> > +
> > +   uk_printd(DLVL_INFO, "Probing RTC...\n");
> > +   fdt_rtc = uk_dtb_find_device(dtb, rtc_device_list,
> > +           sizeof(rtc_device_list));
> 
> I would not assume the RTC is always present.

The check is inside uk_dtb_read_region.
> 
> > +   naddr = uk_dtb_read_region(dtb, fdt_rtc, &nsize, &regs);
> > +   rtc_base_addr = uk_dtb_read_term(regs, 0, naddr, nsize, &size);
> 
> Similarly here, I would not assume uk_dtb_read_term() will always succeed.
> It would make the code more safe if the DT passed is wrong.


Check has been done in this function.
> 
> > +
> > +   /* Record the boot seconds */
> > +   rtc_boot_seconds = rtc_read();
> > +
> > +   uk_printd(DLVL_INFO, "Found RTC on: %p\n", rtc_base_addr);
> 
> I am slightly surprised this compile in Unikraft. This function is meant to
> return an integer but I don't see any return here.

Which function? Rtc_read()? Check it again.

Bests
Jianyong wu
> 
> > +}
> > diff --git a/plat/common/include/arm/rtc.h
> > b/plat/common/include/arm/rtc.h new file mode 100644 index
> > 0000000..d8bab7f
> > --- /dev/null
> > +++ b/plat/common/include/arm/rtc.h
> > @@ -0,0 +1,43 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause */
> > +/*
> > + * Authors: Wei Chen <Wei.Chen@xxxxxxx>
> > + *          Jianyong Wu <Jianyong.Wu@xxxxxxx>
> > + *
> > + * Copyright (c) 2018, Arm Ltd. All rights reserved.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + *
> > + * 1. Redistributions of source code must retain the above copyright
> > + *    notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + *    notice, this list of conditions and the following disclaimer in the
> > + *    documentation and/or other materials provided with the distribution.
> > + * 3. Neither the name of the copyright holder nor the names of its
> > + *    contributors may be used to endorse or promote products derived
> from
> > + *    this software without specific prior written permission.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> CONTRIBUTORS "AS IS"
> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> LIMITED
> > +TO, THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> PARTICULAR
> > +PURPOSE
> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
> > +CONTRIBUTORS BE
> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY,
> > +OR
> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> PROCUREMENT
> > +OF
> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> > +BUSINESS
> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
> > +WHETHER IN
> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> > +OTHERWISE)
> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
> > +ADVISED OF THE
> > + * POSSIBILITY OF SUCH DAMAGE.
> > + *
> > + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> > + */
> > +
> > +#ifndef __PLAT_CMN_ARM_RTC_H__
> > +#define __PLAT_CMN_ARM_RTC_H__
> > +
> > +extern uint32_t rtc_boot_seconds;
> > +
> > +int _dtb_init_rtc(void *dtb);
> 
> Where do you expect this function to be called?
> 
> > +
> > +#endif //__PLAT_CMN_ARM_GICV2_H__
> 
> Wrong guard?
> 
> > diff --git a/plat/kvm/Makefile.uk b/plat/kvm/Makefile.uk index
> > 8e481b4..197175a 100644
> > --- a/plat/kvm/Makefile.uk
> > +++ b/plat/kvm/Makefile.uk
> > @@ -60,6 +60,7 @@ LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
> $(UK_PLAT_COMMON_BASE)/arm/psci_arm64.S
> >   LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
> $(UK_PLAT_COMMON_BASE)/arm/time.c|common
> >   LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
> $(UK_PLAT_COMMON_BASE)/arm/traps.c|common
> >   LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
> > $(UK_PLAT_COMMON_BASE)/arm/fdt.c|common
> > +LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
> > +$(UK_PLAT_COMMON_BASE)/arm/rtc.c|common
> >   LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
> $(LIBKVMPLAT_BASE)/arm/entry64.S
> >   LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
> $(LIBKVMPLAT_BASE)/arm/exceptions.S
> >   LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
> > $(LIBKVMPLAT_BASE)/arm/pagetable.S
> >
> 
> --
> Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel