aarch64: Implement RTC Pl031 #1103

dianpopa · 2019-05-24T14:53:50Z

Since on arm there is no kvmclock, we need to
implement an RTC device. Implements RTC PL031.
Follow specification:
https://static.docs.arm.com/ddi0224/c/real_time_clock_pl031_r1p3_technical_reference_manual_DDI0224C.pdf

Signed-off-by: Diana Popa [email protected]

Issue #, if available: Fixes #1066

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

acatangiu

Still need to review some parts of the RTC dev implementation, but so far LGTM.

acatangiu · 2019-05-24T16:58:56Z

arch/src/aarch64/fdt.rs

+            (
+                "rtc".to_string(),
+                MMIODeviceInfo {
+                    addr: 0x00 + LEN,


Isn't this overlapping the previous virtio device address space? Am I missing something? 😕

oh, Yes indeed I used the same base address for both devices but this is an unit test whose purpose is to check that the fdt turns exactly how we intend it to be. We are not using it to spawn a vm. However, I will change it in order to avoid any confusion.

petreeftime

Please check data for size when working with LittleEndian. Otherwise minor nits.

petreeftime · 2019-05-25T10:42:24Z

devices/src/legacy/rtc_pl031.rs

+use time::{SteadyTime, Timespec};
+//use bus::Error;
+
+// As you can see in https://static.docs.arm.com/ddi0224/c/real_time_clock_pl031_r1p3_technical_reference_manual_DDI0224C.pdf


It would be nice to have a link to the documentation in the commit message as well.

Indeed, take a look.

petreeftime · 2019-05-25T10:45:24Z

devices/src/legacy/rtc_pl031.rs

+pub struct RTC {
+    previous_now: SteadyTime,
+    tick_offset: Timespec,
+    // THis is used for implementing the RTC alarm. However, in Firecracker we do not need it.


typo: This

petreeftime · 2019-05-25T10:45:56Z

devices/src/legacy/rtc_pl031.rs

+// As you can see in https://static.docs.arm.com/ddi0224/c/real_time_clock_pl031_r1p3_technical_reference_manual_DDI0224C.pdf
+// at section 3.2 Summary of RTC registers, the total size occupied by this device is 0x000 -> 0xFFC + 4 = 0x1000.
+// From 0x0 to 0x1C we have following registers:
+const DR: u64 = 0x0; // Data Register.


nit: The documentation lists these as RTCDR, RTCMR, etc. Having them written exactly as such would make it easier to find and match them between the code and the spec.

petreeftime · 2019-05-25T10:52:38Z

devices/src/legacy/rtc_pl031.rs

+        ts.sec as u32
+    }
+
+    fn handle_write(&mut self, offset: u64, v: u32) -> Result<()> {


nit: v should be val or value.

Renamed it to val.

petreeftime · 2019-05-25T10:58:25Z

devices/src/legacy/rtc_pl031.rs

+        if read_ok {
+            LittleEndian::write_u32(data, v);
+        } else {
+            error!(


This case seems to be an error with the driver of the device and not the device itself. While this could be printed out, I think it should fall under debug or info, maybe warn at most.

Indeed, error might be too much. I do think though that it is more than an info since not having the time updated could lead to other serious problems. So, I left it as a warning.

petreeftime · 2019-05-25T16:37:31Z

devices/src/legacy/rtc_pl031.rs

+    fn write(&mut self, offset: u64, data: &[u8]) {
+        let v = LittleEndian::read_u32(data);
+        if let Err(e) = self.handle_write(offset, v) {
+            error!("Failed to write to RTC PL031 device: {}", e);


Same as above: some errors here come from a bad driver implementation and not a faulty device.

Modified it.

petreeftime · 2019-05-25T16:40:29Z

devices/src/legacy/rtc_pl031.rs

+    match_value: u32,
+    // Writes to this register load an update value into the RTC.
+    load: u32,
+    imsc: u32,


According to the spec this could be a boolean, which is more suggestive.

According to the spec this is a 1bit register. Moreover, look at what the documentation specifies for the RTCICR register: "The interrupt is cleared by writing any data value to the Interrupt Clear Register, RTCICR". Indeed, these register could very well be represented as bool but since my read/write functions receive data of [u8] type, I d need to add additional post processing logic to transform it to a bool. I d like to keep the model as simple as possible.

petreeftime · 2019-05-25T16:45:22Z

devices/src/legacy/rtc_pl031.rs

+    // Writes to this register load an update value into the RTC.
+    load: u32,
+    imsc: u32,
+    ris: u32,


Also a boolean.

See previous comment.

petreeftime · 2019-05-25T17:02:20Z

devices/src/legacy/rtc_pl031.rs

+    }
+
+    fn write(&mut self, offset: u64, data: &[u8]) {
+        let v = LittleEndian::read_u32(data);


The byteorder crate seems crazy and according to documentation it might panic instead of returning a result, if data is not the right size. Checking this beforehand is probably recommended, since a badly written driver could try to write an u64 instead. https://docs.rs/byteorder/1.3.1/byteorder/trait.ByteOrder.html#tymethod.read_u32

Added a check for that. Take a look.

petreeftime · 2019-05-25T17:04:41Z

devices/src/legacy/rtc_pl031.rs

+            };
+        }
+        if read_ok {
+            LittleEndian::write_u32(data, v);


data might not have enough space for a u32 value.

Ok, i added a check to ensure data.len <=4. take a look.

dianpopa

@petreeftime @acatangiu addressed comments. Please review.

dianpopa · 2019-05-27T09:17:09Z

arch/src/aarch64/fdt.rs

+            (
+                "rtc".to_string(),
+                MMIODeviceInfo {
+                    addr: 0x00 + LEN,


oh, Yes indeed I used the same base address for both devices but this is an unit test whose purpose is to check that the fdt turns exactly how we intend it to be. We are not using it to spawn a vm. However, I will change it in order to avoid any confusion.

dianpopa · 2019-05-27T09:21:25Z

devices/src/legacy/rtc_pl031.rs

+use time::{SteadyTime, Timespec};
+//use bus::Error;
+
+// As you can see in https://static.docs.arm.com/ddi0224/c/real_time_clock_pl031_r1p3_technical_reference_manual_DDI0224C.pdf


Indeed, take a look.

dianpopa · 2019-05-27T09:38:23Z

devices/src/legacy/rtc_pl031.rs

+// As you can see in https://static.docs.arm.com/ddi0224/c/real_time_clock_pl031_r1p3_technical_reference_manual_DDI0224C.pdf
+// at section 3.2 Summary of RTC registers, the total size occupied by this device is 0x000 -> 0xFFC + 4 = 0x1000.
+// From 0x0 to 0x1C we have following registers:
+const DR: u64 = 0x0; // Data Register.


dianpopa · 2019-05-27T09:38:47Z

devices/src/legacy/rtc_pl031.rs

+pub struct RTC {
+    previous_now: SteadyTime,
+    tick_offset: Timespec,
+    // THis is used for implementing the RTC alarm. However, in Firecracker we do not need it.


dianpopa · 2019-05-27T09:42:05Z

devices/src/legacy/rtc_pl031.rs

+    // Writes to this register load an update value into the RTC.
+    load: u32,
+    imsc: u32,
+    ris: u32,


See previous comment.

dianpopa · 2019-05-27T09:58:20Z

devices/src/legacy/rtc_pl031.rs

+        ts.sec as u32
+    }
+
+    fn handle_write(&mut self, offset: u64, v: u32) -> Result<()> {


Renamed it to val.

dianpopa · 2019-05-27T10:01:00Z

devices/src/legacy/rtc_pl031.rs

+            };
+        }
+        if read_ok {
+            LittleEndian::write_u32(data, v);


Ok, i added a check to ensure data.len <=4. take a look.

dianpopa · 2019-05-27T10:04:40Z

devices/src/legacy/rtc_pl031.rs

+        if read_ok {
+            LittleEndian::write_u32(data, v);
+        } else {
+            error!(


Indeed, error might be too much. I do think though that it is more than an info since not having the time updated could lead to other serious problems. So, I left it as a warning.

dianpopa · 2019-05-27T10:07:14Z

devices/src/legacy/rtc_pl031.rs

+    }
+
+    fn write(&mut self, offset: u64, data: &[u8]) {
+        let v = LittleEndian::read_u32(data);


Added a check for that. Take a look.

dianpopa · 2019-05-27T10:07:34Z

devices/src/legacy/rtc_pl031.rs

+    fn write(&mut self, offset: u64, data: &[u8]) {
+        let v = LittleEndian::read_u32(data);
+        if let Err(e) = self.handle_write(offset, v) {
+            error!("Failed to write to RTC PL031 device: {}", e);


Modified it.

Since on arm there is no kvmclock, we need to implement an RTC device. This commit implements the AMBA PL031 specification. See https://static.docs.arm.com/ddi0224/c/ real_time_clock_pl031_r1p3_technical_reference_manual_DDI0224C.pdf Signed-off-by: Diana Popa <[email protected]>

dianpopa requested review from alxiord, alexandruag and acatangiu May 24, 2019 14:53

dianpopa self-assigned this May 24, 2019

dianpopa requested a review from serban300 May 24, 2019 14:53

dianpopa force-pushed the i757m branch 2 times, most recently from dc1ac18 to a9cf91d Compare May 24, 2019 16:57

acatangiu reviewed May 24, 2019

View reviewed changes

petreeftime suggested changes May 25, 2019

View reviewed changes

dianpopa force-pushed the i757m branch from a9cf91d to 9c3a33a Compare May 27, 2019 10:13

dianpopa commented May 27, 2019

View reviewed changes

dianpopa force-pushed the i757m branch from 9c3a33a to 92ace26 Compare May 27, 2019 10:14

petreeftime approved these changes May 27, 2019

View reviewed changes

dianpopa added 2 commits May 27, 2019 13:41

Merge branch 'master' into i757m

b0ed76b

Merge branch 'master' into i757m

5ced66a

acatangiu approved these changes May 27, 2019

View reviewed changes

acatangiu merged commit 18ca215 into firecracker-microvm:master May 27, 2019

dianpopa deleted the i757m branch May 28, 2019 11:16

dianpopa added the Feature: CPU Support: ARM label Oct 29, 2020

aarch64: Implement RTC Pl031 #1103

aarch64: Implement RTC Pl031 #1103

Uh oh!

Conversation

dianpopa commented May 24, 2019

Uh oh!

acatangiu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petreeftime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dianpopa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!