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

[Julien Grall <julien.grall@xxxxxxx>]

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.

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

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