Add registers::debug #286

mkroening · 2021-07-26T14:38:39Z

Fixes #284.

This adds registers::debug.

Dr0-Dr3 implement DebugAddressRegister.
Dr6Flags can be read from Dr6.

For Dr7 I added Dr7Value which contain Dr7Flags but has functions to modify the bits corresponding to the condition and size fields. Via Dr7Value::flags_mut, you can modify flags in place.

mkroening · 2021-08-26T16:42:20Z

I added docs to everything and properly integrated the assembly.

Could you enable the CI for this PR and review it? :)

josephlr · 2021-08-26T18:10:28Z

@mkroening The basic structure here looks reasonable, I'll see if I have time for a full review this weekend.

mkroening · 2021-08-28T00:54:11Z

src/registers/debug.rs

+    ///
+    /// This can be used to modify the flags of this value in-place.
+    #[inline]
+    pub fn flags_mut(&mut self) -> &mut Dr7Flags {


I'm not completely happy with this function, as it could be misused.

I added it to have a way of modifying the Dr7Flags bits inside the Dr7Value. Doing so via this mutable reference causes unintended side effects to the fields in Dr7Value. Perhaps it would be better to manually implement all operations for adding/removing flags from the value instead of transmuting self to the flags. What do you think?

If additional bits are allowed for Dr7Flags, can't we make use Dr7Flags for the inner value directly (instead of u64)?

Alternatively, we could also merge the Dr7Value type into the Dr7Flags type by defining the additional methods in an impl Dr7Flags block. To get the same behavior for from_bits_truncate, we could define an addional bitflag const for all the additional value, i.e. const CONDITIONS_AND_SIZE = (1 << 32) - (1 << 16);.

Ah, yes. We could use Dr7Flags for the inner value.

It would expose the same (problematic?) behavior as defining methods on Dr7Flags or the current behavior: When you have a valid Dr7Value, there are values which are no bitflags (HwBreakpointCondition and HwBreakpointSize). If those values are set to some non-zero value, the behavior of BitAnd and similar traits are not behaving as one might expect.

So I think to cleanly solve this problem, we would have to remove flags_mut and implement BitAnd<Flags> for Value and similar traits manually. We could additionally make use of Dr7Flags for the inner value, but I am not sure yet if that would be beneficial. What do you think?

All right. I went ahead and removed this method and any unsafe code handling this type in 0eff064.

I forbid any use of extra bits in the representation of Dr7Flags and added the explicit *_flags methods for manipulating Dr7Value. What do you think?

src/registers/debug.rs

mkroening · 2021-09-12T09:26:31Z

Rebased on latest master.

phil-opp

Looks very good, thanks a lot! I left a few comments, otherwise this seems ready to be merged.

Sorry for the delay from our side. Thanks for keeping the PR up-to-date!

src/registers/debug.rs

phil-opp · 2021-11-06T15:36:06Z

src/registers/debug.rs

+    ///
+    /// This can be used to modify the flags of this value in-place.
+    #[inline]
+    pub fn flags_mut(&mut self) -> &mut Dr7Flags {


If additional bits are allowed for Dr7Flags, can't we make use Dr7Flags for the inner value directly (instead of u64)?

Alternatively, we could also merge the Dr7Value type into the Dr7Flags type by defining the additional methods in an impl Dr7Flags block. To get the same behavior for from_bits_truncate, we could define an addional bitflag const for all the additional value, i.e. const CONDITIONS_AND_SIZE = (1 << 32) - (1 << 16);.

src/registers/debug.rs

mkroening · 2021-11-28T18:22:20Z

Thanks a lot for the review! I addressed all small issues and have added my thoughts on the big remaining one.

They don't add value over the inherent constructor functions.

mkroening · 2022-06-21T16:21:18Z

Thanks a lot for your reviews and sorry for the wait! I addressed all comments and would appreciate another round of reviews. :)

Freax13

Looks good to me!

@josephlr Do you also want to do another round of review?

josephlr

LGTM, the only thing we might want to change is the fact that we have really long names starting with Debug, given that the module is also called debug. So it might make sense to shorten the names to remove the Debug prefix.

@Freax13 your thoughts?

src/registers/debug.rs

Freax13 · 2022-06-23T14:11:29Z

LGTM, the only thing we might want to change is the fact that we have really long names starting with Debug, given that the module is also called debug. So it might make sense to shorten the names to remove the Debug prefix.

@Freax13 your thoughts?

I think we should keep the prefix, this only affects two types and the names might sound a bit too generic without the Debug prefix (AddressRegister, AddressRegisterNumber). I'd rather we strip the Hw prefix from HwBreakpointCondition and HwBreakpointSize, it's a bit unidiomatic to use abbreviations in type names and it should be pretty clear that those types are meant for hardware breakpoints anyways.

mkroening · 2022-06-23T14:35:16Z

I renamed them to BreakpointCondition and BreakpointSize respectively. 👍

Freax13 · 2022-06-23T19:45:23Z

Thank you for your contribution! 🥳

mkroening · 2022-06-23T20:23:20Z

🥳 Thanks for your help!

mkroening · 2022-07-02T22:31:49Z

Would it be possible to have a 0.14.10 release containing these additions? It would help us out a lot. :)

phil-opp · 2022-07-10T21:45:01Z

I created a release PR at #386.

mkroening force-pushed the debug-registers branch 7 times, most recently from 1961e1e to 805333e Compare July 29, 2021 09:56

mkroening force-pushed the debug-registers branch 2 times, most recently from af0e41e to d2fe1cb Compare August 26, 2021 16:37

mkroening marked this pull request as ready for review August 26, 2021 16:38

mkroening force-pushed the debug-registers branch from d2fe1cb to 4422d54 Compare August 26, 2021 16:42

josephlr self-assigned this Aug 26, 2021

josephlr added the waiting-for-review Waiting for a review from the maintainers. label Aug 26, 2021

mkroening commented Aug 28, 2021

View reviewed changes

toku-sa-n reviewed Sep 4, 2021

View reviewed changes

src/registers/debug.rs Outdated Show resolved Hide resolved

mkroening force-pushed the debug-registers branch from 4422d54 to 57bd96e Compare September 5, 2021 08:49

toku-sa-n reviewed Sep 6, 2021

View reviewed changes

src/registers/debug.rs Outdated Show resolved Hide resolved

mkroening force-pushed the debug-registers branch from 3e6f627 to 6da9989 Compare September 12, 2021 09:26

phil-opp reviewed Nov 6, 2021

View reviewed changes

phil-opp added waiting-on-author Waiting for the author to act on review feedback. and removed waiting-for-review Waiting for a review from the maintainers. labels Nov 6, 2021

Freax13 added waiting-for-review Waiting for a review from the maintainers. and removed waiting-on-author Waiting for the author to act on review feedback. labels Feb 5, 2022

mkroening force-pushed the debug-registers branch from 0eff064 to 78bd682 Compare May 13, 2022 13:39

Add registers::debug

f38f4e7

mkroening force-pushed the debug-registers branch from 78bd682 to f38f4e7 Compare May 13, 2022 13:44

mkroening added 7 commits June 21, 2022 17:40

Declare debug register acesses as nomem

09ac3f6

Use u64 for DebugAddressRegisters

58e8aa5

Dr7: Remove get_ prefix from getters

8d4d159

debug.rs: Remove redundant denying of unsafe_op_in_unsafe_fn

ecdf74e

debug.rs: Remove TryFrom impls

da5068b

They don't add value over the inherent constructor functions.

Don't set repr on DebugAddressRegisterNumber

ca94a17

Make DebugAddressRegisterNumber an enum

d5f2cff

mkroening requested review from phil-opp, josephlr and Freax13 June 21, 2022 16:21

debug.rs: Remove redundant asm import

33eb389

Freax13 added waiting-for-review Waiting for a review from the maintainers. and removed waiting-on-author Waiting for the author to act on review feedback. labels Jun 22, 2022

Freax13 approved these changes Jun 22, 2022

View reviewed changes

josephlr approved these changes Jun 23, 2022

View reviewed changes

src/registers/debug.rs Show resolved Hide resolved

Add DebugAddressRegister::NUM

58e3f1d

Strip Hw Prefix from HwBreakpointCondition and HwBreakpointSize

250a7de

Freax13 approved these changes Jun 23, 2022

View reviewed changes

josephlr merged commit 3a8f380 into rust-osdev:master Jun 23, 2022

mkroening deleted the debug-registers branch June 23, 2022 20:23

mkroening mentioned this pull request Jul 2, 2022

Next release hermit-os/uhyve#346

Closed

This was referenced Jul 6, 2022

This Month in Rust OSDev: June 2022 rust-osdev/homepage#104

Merged

Release v0.14.10 #386

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add registers::debug #286

Add registers::debug #286

mkroening commented Jul 26, 2021 •

edited

Loading

mkroening commented Aug 26, 2021

josephlr commented Aug 26, 2021

mkroening Aug 28, 2021

phil-opp Nov 6, 2021

mkroening Nov 28, 2021

mkroening Feb 5, 2022

mkroening commented Sep 12, 2021

phil-opp left a comment

phil-opp Nov 6, 2021

mkroening commented Nov 28, 2021

mkroening commented Jun 21, 2022

Freax13 left a comment

josephlr left a comment

Freax13 commented Jun 23, 2022

mkroening commented Jun 23, 2022

Freax13 commented Jun 23, 2022

mkroening commented Jun 23, 2022

mkroening commented Jul 2, 2022

phil-opp commented Jul 10, 2022

Add registers::debug #286

Add registers::debug #286

Conversation

mkroening commented Jul 26, 2021 • edited Loading

mkroening commented Aug 26, 2021

josephlr commented Aug 26, 2021

mkroening Aug 28, 2021

Choose a reason for hiding this comment

phil-opp Nov 6, 2021

Choose a reason for hiding this comment

mkroening Nov 28, 2021

Choose a reason for hiding this comment

mkroening Feb 5, 2022

Choose a reason for hiding this comment

mkroening commented Sep 12, 2021

phil-opp left a comment

Choose a reason for hiding this comment

phil-opp Nov 6, 2021

Choose a reason for hiding this comment

mkroening commented Nov 28, 2021

mkroening commented Jun 21, 2022

Freax13 left a comment

Choose a reason for hiding this comment

josephlr left a comment

Choose a reason for hiding this comment

Freax13 commented Jun 23, 2022

mkroening commented Jun 23, 2022

Freax13 commented Jun 23, 2022

mkroening commented Jun 23, 2022

mkroening commented Jul 2, 2022

phil-opp commented Jul 10, 2022

mkroening commented Jul 26, 2021 •

edited

Loading