Transparent 'Raw TRB type' #169

paulsohn · 2023-12-06T17:29:03Z

To implement this I rewrote virtually all macros to make them more general, accepting visibility and indexing base expression arguments.

Effectively, these changes are supposed to reflect what I expressed in the issue #161 . Changes includes:

Each of trb::{transfer|command|event} now has its own AllowedType enum and a transparent raw TRB type.
- This means, instead of one big TRB type, we have three TRB types for each purposes.
  Event Ring Segment would be &[event::TRB] and Command Ring &mut [command::TRB], and so on.
- Any concrete TRB type can be converted into raw TRB type; e.g. transfer::TRB implements From<transfer::Normal>. Conversely transfer::Normal implements TryFrom<transfer::TRB>, which performs type and RSVDZ checks.
- A concrete type can be used to get/set appropriate fields except for the cycle bit.
  Cycle bits should be handled by ring buffer implementation or by xhc.
In a favor of AllowedTypes and the concrete types, trb::Type and trb::{transfer|command|event}::Allowed are removed.
- A drawback of this is that, if one wants to match a TRB into a concrete type, then if let Some-ing TryFrom::<Concrete>::try_from(trb) is the only choice now. Hopefully we can also write a macro for this. Current state isn't that bad though, especially for the users (just like me) who don't match every possible TRB types, but check if the type matches with a few supported types and only process them.

Since this is a HUGE breaking change, many minor API changes are involved on refactoring. These includes (but not limited to):

Previously transfer::DataStage and transfer::StatusStage both had direction bit but their types were different. (One was Direction while the other was boolean.) This was fixed so that both types are boolean.
Some TRBs have fields which semantics depends on flags and other fields. For transfer::Isoch, the field representing Transfer Burst Count can be either 'TD Size/TBC' or 'TBC/TRBSts'. The latter is even RSVDZ in some situations. So I renamed transfer_burst_count into tbc_or_sts. (That's one reason why I'm against the RSVDZ checkings in .try_from().)

See here to compare previous implementation(ring::trb) and mine(ring::block) in parallel.
On last commits, I removed previous ring::trb and the name is taken by previous ring::block.

(Disclaimer: currently I haven't tested anything. Only rustc says it's fine and built docs looked nice.)

Co-authored-by: Hiroki Tokunaga <[email protected]>

toku-sa-n · 2023-12-08T00:06:51Z

Thanks for the PR. Can I postpone the review until I finish implementing the test os? It's not easy to review a large PR without any tests.

toku-sa-n · 2023-12-20T04:05:59Z

Sorry for the long delay. I stopped writing a test program for this crate because I'm now in doubt if I really need it (See #159 (comment)). I'll review this PR it in a few days.

toku-sa-n

Thank you for the PR, and again, sorry for the long delay. Sorry, I haven't completely reviewed your code yet, and I understand this is still a draft PR, but I leave a few comments.

toku-sa-n · 2023-12-20T05:21:01Z

src/extended_capabilities/debug.rs

-    }
+    rw_zero_trailing!(
+        pub, self,
+        self.0; 4~; "64-byte aligned",


Suggested change

self.0; 4~; "64-byte aligned",

self.0; 4~; "16-byte aligned",

toku-sa-n · 2023-12-20T05:23:06Z

src/extended_capabilities/xhci_message_interrupt.rs

 use accessor::single;
 use accessor::Mapper;
-use bit_field::BitField;
+// use bit_field::BitField;


Suggested change

// use bit_field::BitField;

toku-sa-n · 2023-12-20T05:24:14Z

src/registers/capability.rs

 use accessor::single;
 use accessor::Mapper;
-use bit_field::BitField;
+// use bit_field::BitField;


Suggested change

// use bit_field::BitField;

toku-sa-n · 2023-12-20T05:34:34Z

src/registers/runtime.rs

 use accessor::Mapper;
 use core::convert::TryFrom;
-use core::convert::TryInto;
+// use core::convert::TryInto;


Suggested change

// use core::convert::TryInto;

toku-sa-n · 2023-12-20T05:47:06Z

src/ring/erst.rs

+    /// # Panics
+    ///
+    /// This method will panic if `len >= 4096`.
+    pub unsafe fn new(base: *const event::TRB, len: usize) -> Self {


I'd change the type of base to u64 because we need the physical address of a ring, not a virtual one.

toku-sa-n · 2023-12-20T05:52:13Z

src/ring/erst.rs

+    /// Returns the mutable slice that this entry is representing.
+    pub fn as_mut_slice(&mut self) -> &mut [event::TRB] {
+        unsafe {
+            let base = self.ring_segment_base_address() as usize as *mut _;
+            let len = self.len();
+
+            core::slice::from_raw_parts_mut(base, len)
+        }
+    }
+}


toku-sa-n · 2023-12-20T05:55:26Z

src/ring/erst.rs

+        }
+    }
+}
+impl EventRingSegmentTableEntry {


Is there a reason to separate impls?

toku-sa-n · 2023-12-20T05:59:35Z

src/ring/erst.rs

It may be better to define only the EventRingSegmentTableEntry type with a correct alignment and necessary field getters/setters because an ERST entry contains physical, not virtual, addresses of rings and the size of the table is variable.

Also, could you split this change to another PR? It'd be easier to review.

toku-sa-n · 2023-12-20T06:01:07Z

src/ring/erst.rs

+    );
+
+    /// Returns the value of the ring segment end address.
+    pub fn ring_segment_bound_address(&self) -> u64 {


ring_segment_end_address might be better, but I'm not sure.

toku-sa-n · 2023-12-20T06:02:52Z

src/ring/mod.rs

A trailing newline is preferred.

paulsohn and others added 14 commits December 1, 2023 14:27

Clarify the exact behavior of RW1C setters

4fd60b9

rename doorbell::Register to Doorbell

f2e067e

mark cap registers as read-only.

b89a5d4

Merge branch 'rust-osdev:main' into merger

6fd2522

convention fix

3e05eca

Co-authored-by: Hiroki Tokunaga <[email protected]>

add visibility qualifier to macros

7303e5d

more macro use

3869266

improve macros

da80701

error fix

b49e2e8

add ring::trb replacement

6a8e585

replace ring::trb with previous ring::block

876f299

cargo fmt

dfbeec5

minor change

22dcf11

Merge branch 'main' into raw-trb

77ace61

paulsohn added 5 commits December 8, 2023 15:34

Add ERST entry

18ac2ae

oops

6b08e37

quick fix

7594282

fix arount erstsz

66ede49

add erst submodule

79abd10

toku-sa-n reviewed Dec 21, 2023

View reviewed changes

	self.0; 4~; "64-byte aligned",
	self.0; 4~; "16-byte aligned",

Uh oh!

Transparent 'Raw TRB type' #169

Are you sure you want to change the base?

Transparent 'Raw TRB type' #169

Uh oh!

Conversation

paulsohn commented Dec 6, 2023

Uh oh!

toku-sa-n commented Dec 8, 2023

Uh oh!

toku-sa-n commented Dec 20, 2023

Uh oh!

toku-sa-n 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants