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

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

Hi,

> -----Original Message-----
> From: Jianyong Wu (Arm Technology China)
> Sent: Sunday, November 11, 2018 10:00 AM
> To: Julien Grall <julien.grall@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,
> 
> > -----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?

As earlier as possible, maybe after init dtb.

> >
> > > +
> > > +#endif //__PLAT_CMN_ARM_GICV2_H__
> >
> > Wrong guard?
Yeah, I will fix it.

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