feat: add `PmTimer` #83

toku-sa-n · 2020-12-06T04:57:02Z

This PR introduces a new struct called PmTimer, which contains the information of the ACPI Power Management Timer. It can be constructed from AcpiTables.

IsaacWoods

Thanks for working on this! I've been wondering about how to approach ACPI's register blocks in a good way for a while - I think providing a good interface is going to be slightly tricky.

The problem with the approach in this PR is how it will scale to the entire register block - we'll end up mapping the FADT potentially many times, just to pull a few bytes out here and there each time.

My alternative suggestion is to expose this through the PlatformInfo type - this already parses the FADT for various other things, and I think collecting all of this information in there creates a nice interface for when we parse the rest of the fixed hardware stuff. What do you think?

I've also highlighted a few problems implementation-wise.

IsaacWoods · 2020-12-10T19:49:16Z

acpi/src/fadt.rs

+/// Information about the ACPI Power Management Timer (ACPI PM Timer).
+pub struct PmTimer {
+    /// An I/O space address to the register block of ACPI PM Timer.
+    pub io_base: u32,


From §4.8.3.3 of the spec, it looks like the PM timer doesn't have to be in I/O space, but can instead be in system memory. PM_TMR_BLK should always be treated as an I/O address, but is superceded by the X_PM_TMR_BLK field (if present and non-zero), which is a GenericAddress. This should therefore probably be a GenericAddress (which should be created from PM_TMR_BLK in I/O space if that field is used.

Edit: currently GenericAddress is pub(crate). We'll have to make it public, and should probably think about providing an enum-based representation for better user-consumption, while we can change it without breaking semver.

Do you mean like this?

enum Addr{ Phys(usize), Io(u32), Pci(u32), }

Sort of, unfortunately we need to handle the rest of the address spaces off the bat as well as we'd break semver by adding the other variants later.

I've had a more careful look at how we should do this, and it turns out an enum doesn't really work as well as I thought it would. I basically ended up writing what I thought would work; hope that's okay. If you rebase your branch you should be able to switch to just using the new GenericAddress.

IsaacWoods · 2020-12-10T19:50:11Z

acpi/src/fadt.rs

+}
+impl PmTimer {
+    /// Creates a new instance of `PmTimer`.
+    pub fn new<H>(tables: &AcpiTables<H>) -> Result<PmTimer, AcpiError>


The PM timer is an optional feature (if both PM_TMR_BLK and X_PM_TMR_BLK are zero), so this should probably return Result<Option<PmTimer>, AcpiError>.

IsaacWoods · 2020-12-10T19:53:33Z

acpi/src/fadt.rs

+                .ok_or(AcpiError::TableMissing(crate::sdt::Signature::FADT))?
+        };
+
+        Ok(PmTimer { io_base: fadt.pm_timer_block, supports_32bit: fadt.flags.get_bit(8) })


This needs to handle two more things:

If X_PM_TMR_BLK is present (check with ExtendedField::access), use that instead of PM_TMR_BLK. Actually, this logic is probably best implemented as a pm_timer_block() method on Fadt, like we currently do with dsdt_address

If both fields are zero, there is no PM timer. This should then return Ok(None).

IsaacWoods · 2020-12-10T19:55:06Z

acpi/src/fadt.rs

@@ -128,3 +131,27 @@ impl Fadt {
        }
    }
 }
+
+/// Information about the ACPI Power Management Timer (ACPI PM Timer).
+pub struct PmTimer {


(Context provided in the top-level review comment)
I think this might be better living in platform.rs, stored as a Option<PmTimer in PlatformInfo, and extracted from the FADT in PlatformInfo::new

toku-sa-n · 2020-12-17T13:24:04Z

Done. By the way, I think the members should be private and instead, getters should be called not to wrongly edit the values. What do you think?

IsaacWoods

Great, this is looking pretty good! Just two small questions, and then this can be merged.

Thanks!

IsaacWoods · 2020-12-17T13:38:23Z

acpi/src/fadt.rs

+            self.x_pm_timer_block.access(self.header().revision).or_else(|| {
+                if self.pm_timer_block != 0 {
+                    Some(RawGenericAddress {
+                        address_space: 0,


If the older pm_timer_block is in I/O space (I think it is, as the spec says "system port address", but I wish that was clearer), shouldn't this be 1?

IsaacWoods · 2020-12-17T13:39:27Z

acpi/src/fadt.rs

+        }
+    }
+
+    pub fn flags(&self) -> u32 {


Since this will be public, could we call it raw_flags, as we may want to provide a nicer view into the flags in the future, and it would be nice for that to be called flags?

(as mentioned below) as this is just a field access, I wouldn't be against just making the field pub, and then we can add a flags method later on for parsing the bitfield if we want?

IsaacWoods · 2020-12-17T13:43:29Z

By the way, I think the members should be private and instead, getters should be called not to wrongly edit the values. What do you think?

Do you mean on PlatformInfo? I don't generally like adding getters for simple field accesses in Rust (we do it in the FADT because most require more complex logic, and people shouldn't have to care about the differences between the raw fields. For fields like flags, I wouldn't be aversed to just making them pub [you could do this instead of changing flags to raw_flags, up to you]), and I don't think people will get stumped into changing them and expecting anything to happen, as we're passing back this struct by value. Do you think people could be confused by allowing that?

toku-sa-n · 2020-12-17T14:25:23Z

OK, I made flags public for consistency.

IsaacWoods · 2020-12-17T18:21:58Z

Thanks very much!

toku-sa-n added 3 commits December 6, 2020 11:02

feat: add PmTimer

cd079c6

refactor: remove an unnecessary method

85756a3

docs: add a missing word

19bee29

IsaacWoods reviewed Dec 10, 2020

View reviewed changes

toku-sa-n added 9 commits December 17, 2020 19:01

Merge branch 'master' into implement_acpi_pm_timer

854c36c

chore: avoid borrowing a packed field

20c1cd4

chore: return an Option value

8c6e5f0

refactor: define fetch_base method

18d5859

refactor: define Fadt::pm_timer_block

b9eec31

refactor: remove an unnecessary pub

23cbb7f

chore: add pm_timer as a member of PlatformInfo

b26ea52

refactor: move the definition of PmTimer to platform module

1d60841

chore: remove an empty file

fc86d4c

IsaacWoods requested changes Dec 17, 2020

View reviewed changes

toku-sa-n added 2 commits December 17, 2020 23:22

fix: wrong address space value was specified

c315620

chore: make flags public member

8bfa51f

IsaacWoods approved these changes Dec 17, 2020

View reviewed changes

IsaacWoods merged commit 9d4bba2 into rust-osdev:master Dec 17, 2020

toku-sa-n deleted the implement_acpi_pm_timer branch December 18, 2020 02:19

feat: add `PmTimer` #83

feat: add `PmTimer` #83

toku-sa-n commented Dec 6, 2020

IsaacWoods left a comment

IsaacWoods Dec 10, 2020 •

edited

toku-sa-n Dec 11, 2020

IsaacWoods Dec 13, 2020

IsaacWoods Dec 10, 2020

IsaacWoods Dec 10, 2020

IsaacWoods Dec 10, 2020

toku-sa-n commented Dec 17, 2020

IsaacWoods left a comment

IsaacWoods Dec 17, 2020

IsaacWoods Dec 17, 2020

IsaacWoods Dec 17, 2020

IsaacWoods commented Dec 17, 2020

toku-sa-n commented Dec 17, 2020

IsaacWoods commented Dec 17, 2020

feat: add PmTimer #83

feat: add PmTimer #83

Conversation

toku-sa-n commented Dec 6, 2020

IsaacWoods left a comment

Choose a reason for hiding this comment

IsaacWoods Dec 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toku-sa-n commented Dec 17, 2020

IsaacWoods left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IsaacWoods commented Dec 17, 2020

toku-sa-n commented Dec 17, 2020

IsaacWoods commented Dec 17, 2020

feat: add `PmTimer` #83

feat: add `PmTimer` #83

IsaacWoods Dec 10, 2020 •

edited