On Sunday, March 21, 2021 at 8:25:45 AM UTC-4 Nadav Har'El wrote:
> Looks good to me, but I'm not at all an expert on these ARM devices. > I only left minor comments below. > > > On Fri, Mar 19, 2021 at 5:34 PM Waldemar Kozaczuk <[email protected]> > wrote: > >> This patch provides basic implementation of a subset of >> functionality of the PrimeCell Real Time Clock (PL031) device >> that is enough to capture boot time as number of seconds since epoch. >> > Please note that this implementation does not handle any host >> clock drift and provides current time functionality that is only >> accurate up to a second. >> >> This patch effectively fixes 2 unit tests - tst-time.cc and >> tst-timerfd.cc. >> >> Fixes #1091 >> >> Signed-off-by: Waldemar Kozaczuk <[email protected]> >> --- >> Makefile | 1 + >> arch/aarch64/arch-dtb.cc | 19 +++++++++++++ >> arch/aarch64/arch-dtb.hh | 7 +++++ >> arch/aarch64/arm-clock.cc | 18 +++++++++++-- >> drivers/pl031.cc | 56 +++++++++++++++++++++++++++++++++++++++ >> drivers/pl031.hh | 50 ++++++++++++++++++++++++++++++++++ >> scripts/test.py | 4 +-- >> 7 files changed, 150 insertions(+), 5 deletions(-) >> create mode 100644 drivers/pl031.cc >> create mode 100644 drivers/pl031.hh >> >> diff --git a/Makefile b/Makefile >> index 833803cf..f33b1cdb 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -835,6 +835,7 @@ endif # x64 >> ifeq ($(arch),aarch64) >> drivers += drivers/mmio-isa-serial.o >> drivers += drivers/pl011.o >> +drivers += drivers/pl031.o >> drivers += drivers/cadence-uart.o >> drivers += drivers/xenconsole.o >> drivers += drivers/virtio.o >> diff --git a/arch/aarch64/arch-dtb.cc b/arch/aarch64/arch-dtb.cc >> index 24ac9267..63db8b00 100644 >> --- a/arch/aarch64/arch-dtb.cc >> +++ b/arch/aarch64/arch-dtb.cc >> @@ -190,6 +190,25 @@ u64 dtb_get_uart(int *irqid) >> return retval; >> } >> >> +u64 dtb_get_rtc() >> +{ >> + u64 retval; >> + int node; size_t len; >> + >> + if (!dtb) >> + return 0; >> + >> + node = fdt_node_offset_by_compatible(dtb, -1, "arm,pl031"); >> + if (node < 0) >> + return 0; >> + >> + len = dtb_get_reg(node, &retval); >> + if (!len) >> + return 0; >> + >> + return retval; >> +} >> + >> u64 dtb_get_mmio_serial_console(int *irqid) >> { >> int node; >> diff --git a/arch/aarch64/arch-dtb.hh b/arch/aarch64/arch-dtb.hh >> index 0aebb4b3..10e1c859 100644 >> --- a/arch/aarch64/arch-dtb.hh >> +++ b/arch/aarch64/arch-dtb.hh >> @@ -49,6 +49,13 @@ size_t dtb_get_phys_memory(u64 *addr); >> */ >> u64 dtb_get_uart(int *irqid); >> >> +/* u64 dtb_get_rtc() >> + * >> + * return the base address of the RTC (PL031) >> + * device or returns zero on failure. >> + */ >> +u64 dtb_get_rtc(); >> + >> /* u64 dtb_get_mmio_serial_console(int *irqid) >> * >> * return the base address of the serial console and writes the >> diff --git a/arch/aarch64/arm-clock.cc b/arch/aarch64/arm-clock.cc >> index c1f4b277..7d11e454 100644 >> --- a/arch/aarch64/arm-clock.cc >> +++ b/arch/aarch64/arm-clock.cc >> @@ -7,6 +7,7 @@ >> >> #include "drivers/clockevent.hh" >> #include "drivers/clock.hh" >> +#include "drivers/pl031.hh" >> #include "arm-clock.hh" >> #include <osv/interrupt.hh> >> #include "exceptions.hh" >> @@ -36,6 +37,8 @@ protected: >> u32 freq_hz; /* frequency in Hz (updates per second) */ >> >> friend class arm_clock_events; >> +private: >> + u64 _boot_time; >> > > I think it will be good - using the name of this variable or comments - > stress that this variable has nanosecond units (as in boot_time() in > drivers/clock.hh). There are two many other ways to understand this. > > Agreed. > }; > > >> >> arm_clock::arm_clock() { >> @@ -49,6 +52,13 @@ arm_clock::arm_clock() { >> #if CONF_logger_debug >> debug_early_u64("arm_clock(): frequency read as ", freq_hz); >> #endif >> + u64 rtc_address = dtb_get_rtc(); >> + if (rtc_address) { >> + pl031 rtc(rtc_address); >> + _boot_time = rtc.wallclock_ns(); >> + } else { >> + _boot_time = 0; >> + } >> } >> >> static __attribute__((constructor(init_prio::clock))) void >> setup_arm_clock() >> @@ -70,12 +80,16 @@ s64 arm_clock::uptime() >> >> s64 arm_clock::time() >> { >> - return uptime(); >> + //Given that _boot_time came from RTC which gives only 1 second >> + //precision the result time() is only accurate up to a second >> > > Is this 1-second precision a limitation of this pl031, or we're not yet > using it properly? > As I understand the spec it is the limitation of pl031. > > + //On top of this, this implementation does not account for any host >> + //clock drifts >> + return _boot_time + uptime(); >> } >> >> s64 arm_clock::boot_time() >> { >> - return uptime(); >> + return _boot_time; >> } >> >> u64 arm_clock::processor_to_nano(u64 ticks) >> diff --git a/drivers/pl031.cc b/drivers/pl031.cc >> > > I think at some point we should reconsider the drivers/ directory. > E.g., why is arm-clock.hh in arch/aarch64 but the new pl031.hh in drivers? > Perhaps we should move the ARM-specific drivers to arch/aarch64, or another > option is to create drivers/aarch64 where we put ARM-only drivers (and > similarly > have other subdirectories in drivers? Like x86, xen, kvm, etc. > > Another goal is for x86 builds to exclude ARM-specific drivers, and vice > versa. > > But we can reconsider this later. > Good idea. > > >> new file mode 100644 >> index 00000000..b81b38b9 >> --- /dev/null >> +++ b/drivers/pl031.cc >> @@ -0,0 +1,56 @@ >> +/* >> + * Copyright (C) 2021 Waldemar Kozaczuk >> + * >> + * This work is open source software, licensed under the terms of the >> + * BSD license as described in the LICENSE file in the top-level >> directory. >> + */ >> + >> +#include "pl031.hh" >> +#include <osv/mmu.hh> >> + >> +/* spec: see PrimeCell Real Time Clock (PL031) Technical Reference >> Manual. >> > > Maybe you should mention that this is an ARM thing? > The more stuff we have in drivers/, the less obvious it becomes which > systems these drivers are relevant for. > > + * implemented according to Revision: r1p3. >> + * See >> https://static.docs.arm.com/ddi0224/c/real_time_clock_pl031_r1p3_technical_reference_manual_DDI0224C.pdf >> + * Please note that this a minimal implementation of a subset of the >> + * functionality which is enough to read the current time in seconds >> + * at the boot time. >> + */ >> + >> +#define rtc_reg(ADDR,REG_OFFSET) (*(volatile u32 *)(ADDR+REG_OFFSET)) >> + >> +pl031::pl031(u64 address) >> +{ >> + _address = address; >> + mmu::linear_map((void *)_address, _address, mmu::page_size, >> mmu::page_size, >> + mmu::mattr::dev); >> +} >> + >> +pl031::~pl031() >> +{ >> + mmu::munmap((void*)_address, mmu::page_size); >> +} >> + >> +uint64_t pl031::wallclock_ns() >> +{ >> + // Let us read various identificatin registers to >> + // verify that it is indeed a valid PL031 device >> + if( rtc_reg(_address, RTCPeriphID0) == RTCPeriphID0_val && >> + rtc_reg(_address, RTCPeriphID1) == RTCPeriphID1_val && >> + (rtc_reg(_address, RTCPeriphID2) & RTCPeriphID2_mask) == >> RTCPeriphID2_val && >> + rtc_reg(_address, RTCPeriphID3) == RTCPeriphID3_val && >> + rtc_reg(_address, RTCPCellID0) == RTCPCellID0_val && >> + rtc_reg(_address, RTCPCellID1) == RTCPCellID1_val && >> + rtc_reg(_address, RTCPCellID2) == RTCPCellID2_val && >> + rtc_reg(_address, RTCPCellID3) == RTCPCellID3_val) { >> + // Read value of RTC which is number of seconds since epoch >> + // representing current time >> + u64 epoch_time_in_seconds = rtc_reg(_address, RTCDR); >> +#if CONF_logger_debug >> + debug_early_u64("pl031::wallclock_ns(): RTC seconds since epoch >> read as ", epoch_time_in_seconds); >> +#endif >> + return epoch_time_in_seconds * 1000000000; >> + } else { >> + debug_early("pl031: could no detect!"); >> + return 0; >> + } >> +} >> diff --git a/drivers/pl031.hh b/drivers/pl031.hh >> new file mode 100644 >> index 00000000..7b594ff4 >> --- /dev/null >> +++ b/drivers/pl031.hh >> @@ -0,0 +1,50 @@ >> +/* >> + * Copyright (C) 2021 Waldemar Kozaczuk >> + * >> + * This work is open source software, licensed under the terms of the >> + * BSD license as described in the LICENSE file in the top-level >> directory. >> + */ >> + >> +#ifndef OSV_PL031_HH >> +#define OSV_PL031_HH >> + >> +#include <osv/types.h> >> + >> +#define RTCDR 0x000 /* data */ >> +#define RTCMR 0x004 /* match */ >> +#define RTCLR 0x008 /* load */ >> +#define RTCCR 0x00c /* control */ >> +#define RTCIMSC 0x010 /* interrupt mask set or clear */ >> +#define RTCRIS 0x014 /* raw interrupt status */ >> +#define RTCMIS 0x018 /* masked interrupt status */ >> +#define RTCICR 0x01c /* interrupt clear */ >> +#define RTCPeriphID0 0xfe0 /* peripheral ID bits [7:0] */ >> +#define RTCPeriphID1 0xfe4 /* peripheral ID bits [15:8] */ >> +#define RTCPeriphID2 0xfe8 /* peripheral ID bits [23:16] */ >> +#define RTCPeriphID3 0xfec /* peripheral ID bits [31:24] */ >> +#define RTCPCellID0 0xff0 /* PrimeCell ID bits [7:0] */ >> +#define RTCPCellID1 0xff4 /* PrimeCell ID bits [7:0] */ >> +#define RTCPCellID2 0xff8 /* PrimeCell ID bits [7:0] */ >> +#define RTCPCellID3 0xffc /* PrimeCell ID bits [7:0] */ >> + >> +#define RTCPeriphID0_val 0x31 >> +#define RTCPeriphID1_val 0x10 >> +#define RTCPeriphID2_mask 0x0f >> +#define RTCPeriphID2_val 0x04 >> +#define RTCPeriphID3_val 0x00 >> + >> +#define RTCPCellID0_val 0x0d >> +#define RTCPCellID1_val 0xf0 >> +#define RTCPCellID2_val 0x05 >> +#define RTCPCellID3_val 0xb1 >> + >> +class pl031 { >> +public: >> + pl031(u64 address); >> + ~pl031(); >> + uint64_t wallclock_ns(); >> +private: >> + u64 _address; >> +}; >> + >> +#endif //OSV_PL031_HH >> diff --git a/scripts/test.py b/scripts/test.py >> index 8e3882af..f0c12d6c 100755 >> --- a/scripts/test.py >> +++ b/scripts/test.py >> @@ -31,7 +31,7 @@ firecracker_disabled_list= [ >> "tcp_close_without_reading_on_qemu" >> ] >> >> -#At this point there are 120 out of 131 unit tests that pass on aarch64. >> +#At this point there are 122 out of 131 unit tests that pass on aarch64. >> #The remaining ones are disabled below until we fix various >> #issues that prevent those tests from passing. >> aarch64_disabled_list= [ >> @@ -46,8 +46,6 @@ aarch64_disabled_list= [ >> #Please see comments on the right side for more details >> "tst-elf-permissions.so", # Infinite page fault >> "tst-mmap.so", # Infinite page fault >> - "tst-time.so", # One assertion fails - >> 'tst-time.cc(70): fatal error: in "time_time": critical check >> (static_cast<time_t>(0)) != (t1) has failed' >> - "tst-timerfd.so", # Some assertions fail - 'SUMMARY: >> 212 tests, 10 failures' >> #These tests fail due to some other shortcomings in the test scripts >> "tracing_smoke_test", >> "tcp_close_without_reading_on_fc", >> -- >> 2.30.2 >> >> -- >> You received this message because you are subscribed to the Google Groups >> "OSv Development" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to [email protected]. >> To view this discussion on the web visit >> https://groups.google.com/d/msgid/osv-dev/20210319153359.69109-1-jwkozaczuk%40gmail.com >> . >> > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/0dc51af1-efd0-459d-a16f-05501d2c442en%40googlegroups.com.
