Port hardware clocks to new clocksource API

[tsoutsman]

We should port the hardware clock sources to the new API added in #615.

• [ ] APIC
• [x] HPET
• [ ] PIT
• [x] TSC

[kevinaboos]

yep, agreed. Been on my to-do list, just haven't gotten around to it. This one is solely about clock sources; next up would be a hardware timer abstraction.

[amab8901]

I'll make an attempt on this

[amab8901]

what does the porting entail? Can you elaborate?

[tsoutsman]

I'll make an attempt on this

Great!

what does the porting entail? Can you elaborate?

We need to implement the ClockSource trait for the clocks.

The TSC should be the easiest to start with. We'll want to implement the trait for a zero-sized type like so:

pub struct Tsc;

impl ClockSource for Tsc {
    // ...
}

The TSC is a monotonic clock source, so now should return an Instant with counter set to the value of the TSC register. One thing we missed in the initial PR was a new function for Instant, so we'll want to add that as well. The unit_to_duration and duration_to_unit functions should use the TSC's frequency to convert between the value of counter and a concrete Duration.

[amab8901]

I noticed that time::ClockType is sealed. Do you want me to unseal it and write the implementation in each clock's crate? Or do you want me to write all the implementations of all the clocks in the time crate?

[tsoutsman]

The naming may be confusing. time::ClockType differentiates categories of clock sources. It is implemented for Monotonic and WallTime. The TSC is not a clock type (i.e. it shouldn't implement ClockType) but rather a clock source (i.e. it should implement ClockSource). You shouldn't need to unseal ClockType to write the implementations in the clock crates.

pub struct Tsc;

impl time::ClockSource for Tsc {
    type ClockType = time::Monotonic;

    // ...
}

BTW it's probably best to create a new PR for each clock source port, e.g. a PR modifying only tsc.

[amab8901]

I'm working on TSC now. The trait time::ClockSource says in the function signature that duration_to_unit should return <Self::ClockType as time::ClockType>::Unit . But the documentation right above says that Monotonic clocks should just return the [Duration]. This seems like a contradiction in the case of monotonic clocks. So I thought "maybe I should change the return type to Duration when implementing for TSC. But when I do that, the compiler delivers this complainment:

error[E0053]: method `duration_to_unit` has an incompatible type for trait
   --> kernel/tsc/src/lib.rs:107:48
    |
107 |     fn duration_to_unit(duration: Duration) -> Duration {
    |                                                ^^^^^^^^
    |                                                |
    |                                                expected struct `Instant`, found struct `Duration`
    |                                                help: change the output type to match the trait: `Instant`
    |
    = note: expected fn pointer `fn(Duration) -> Instant`
               found fn pointer `fn(Duration) -> Duration`

This is probably due to the fact that the trait definition time::ClockSource requires duration_to_unit method to return <Self::ClockType as ClockType>::Unit, which means that these documented instructions cannot be applied (as far as I know):

    /// Converts a [`Duration`] into a [`ClockType::Unit`].
    ///
    /// Monotonic clocks should just return the [`Duration`].

Because the trait definition requires you to be consistent with the return type regardless of whether you use monotonic clock or wall clock.

Should we reconsider the policy of using different return types for monotonic and wall clocks? Or is there a way to achieve different return types for different implementations of the same trait?

[amab8901]

Here is what I have so far in tsc:

impl time::ClockSource for Tsc {
          /// The type of clock (either [`Monotonic`] or [`WallTime`]).
    type ClockType = time::Monotonic;

    /// The current time according to the clock.
    ///
    /// Monotonic clocks return an [`Instant`] whereas wall time clocks return a
    /// [`Duration`] signifying the time since 12:00am January 1st 1970 (i.e.
    /// Unix time).
    fn now() -> <Self::ClockType as time::ClockType>::Unit {
        time::Instant { counter: 0 }
    }

    /// Converts a [`ClockType::Unit`] into a [`Duration`].
    ///
    /// Monotonic clocks should just return the [`ClockType::Unit`].
    fn unit_to_duration(unit: <Self::ClockType as time::ClockType>::Unit) -> <Self::ClockType as time::ClockType>::Unit {
        10
    }

    /// Converts a [`Duration`] into a [`ClockType::Unit`].
    ///
    /// Monotonic clocks should just return the [`Duration`].
    fn duration_to_unit(duration: Duration) -> <Self::ClockType as time::ClockType>::Unit {
        duration
    }
}

[tsoutsman]

:grimacing: oops sorry, the doc comments are wrong. They should say "Wall time", not "Monotonic", i.e. "Wall time clocks should just return the [Duration]".

This is because for wall time clocks, ClockType::Unit is Duration.

But for monotonic clocks, ClockType::Unit is Instant (defined here).

[amab8901]

grimacing oops sorry, the doc comments are wrong. They should say "Wall time", not "Monotonic", i.e. "Wall time clocks should just return the [Duration]".

This is because for wall time clocks, ClockType::Unit is Duration.

But for monotonic clocks, ClockType::Unit is Instant (defined here).

Does your correction apply also to unit_to_duration in addtion to duration_to_unit? Or is it correctly written as it is in unit_to_duration?

[tsoutsman]

Does your correction apply also to unit_to_duration in addtion to duration_to_unit? Or is it correctly written as it is in unit_to_duration?

Yes. #739 removes the unit_to_duration and duration_to_unit functions, so it may be best to wait for #739 to be merged.

[amab8901]

Does your correction apply also to unit_to_duration in addtion to duration_to_unit? Or is it correctly written as it is in unit_to_duration?

Yes. #739 removes the unit_to_duration and duration_to_unit functions, so it may be best to wait for #739 to be merged.

ok so what should I do while waiting for #739 to be merged? Anything I can work on meanwhile?

[kevinaboos]

#739 has been merged in :tada: