Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for a 'Common raw TRB' type #161

Open
paulsohn opened this issue Dec 1, 2023 · 2 comments
Open

Support for a 'Common raw TRB' type #161

paulsohn opened this issue Dec 1, 2023 · 2 comments

Comments

@paulsohn
Copy link
Contributor

paulsohn commented Dec 1, 2023

Currently, individual TRB types are defined as transparent wrappers of [u32;4] with some impls.
However speaking about their union types, there are Allowed types which I feel inconvenient for several reasons:

  • Allowed types are not transparently [u32;4], so we can't directly push that type into a ring.
  • Several methods shared among all TRB types, such as extracting raw [u32;4] block or set/clear cycle bits,
    are implemented in Allowed types by matching discriminator arms.
    Although I didn't benchmark, this seems quite redundant to me. This possibly is a bottleneck given that the actual methods perform simple enough tasks like setting a bit or return the block itself.
  • Since this crate doesn't have a Event Ring Segment Table Entry type, I had to build it in my own. (I'll open another issue for this)
    For that entry type, we need a method which takes a Event Ring buffer reference and convert it into an entry.
    What would be the type of that buffer argument? Definitely not &[Allowed], and &[[u32;4]] seems a little bit unclear.

Is it okay that an allocated buffer has the type &[[u32;4]]?

If not, I suggest to add a transparent wrapper for [u32; 4], say pub struct Block(pub(crate) [u32; 4]);, and define some methods like set/clear cycle bits on it.
Then the ERST Entry type will accept &[Block] type buffers, and concrete TRB types would wrap Block rather than raw [u32; 4]. (e.g. pub struct NoOp(Block); instead of pub struct NoOp([u32;4]);)

@toku-sa-n
Copy link
Member

Thanks for the comment. I understand your point; certainly, the current Allowed lacks transparency, and that's not useful so much. I'm not sure having a large match lowers benchmarks, though.

Is it okay that an allocated buffer has the type &[[u32;4]]?

Is this about Ring types? If so, it may be ok, but what methods will the type have?

If not, I suggest to add a transparent wrapper for [u32; 4], say pub struct Block(pub(crate) [u32; 4]);, and define some methods like set/clear cycle bits on it.
Then the ERST Entry type will accept &[Block] type buffers, and concrete TRB types would wrap Block rather than raw [u32; 4]. (e.g. pub struct NoOp(Block); instead of pub struct NoOp([u32;4]);)

I'd rather avoid this because ERST entry and TRBs don't have shared fields, or just define Block as a type-alias of [u32; 4].


Just a thought, but how about defining traits like TRB, CommandTRB, and so on?

@paulsohn
Copy link
Contributor Author

paulsohn commented Dec 4, 2023

I'd rather avoid this because ERST entry and TRBs don't have shared fields.

My words might be misleading.
Just to be clear, I'm saying that considering future realisation of ERST entry type, we should make converters from and into a TRB buffer, and the type of the buffer should be more informative than &[[u32; 4]]. Not something like "ERST entries have [u32; 4] representation so let them be the same type to TRBs".

or just define Block as a type-alias of [u32; 4].

Certainly this can be a solution, but I wanted a type which has at least set/clearing chain bits.
This can be done within current method sets, but according to something like:

  1. Constructing a specific type TRB without chain bits
  2. Transform the TRB into Allowed type
  3. Set the chain bit(use large match 1)
  4. Extract the raw [u32; 4] from the Allowed type, discarding the tag (use large match 2)
  5. Push it into the buffer of [u32; 4]s

I don't know whether this is your intended usage, but this was not intuitive to me, given that Allowed is not transparent and we have to construct one more discriminant tag despite that we already have the discriminant in the TRB payload.

My expected workflow was that

  1. Constructing a specific type TRB without chain bits
  2. Extract the underlying [u32; 4] - like Block type
  3. Push it into the buffer of Blocks.

and setting chain bits can be done either directly to the type TRB(between 1 and 2) or after extracting Block type(between 2 and 3), provided that the Block type supports set/clear methods.

So to elaborate, I would suggest that:

  • Instead of defining common interfaces by opaque Allowed enums, we can have a transparent Block types such as pub struct Block([u32;4]) and define set/clear cycle bit methods from them. This way, we can avoid matches.
  • For each specific type TRBs, declare them as transparent wrappers of the Block type above. TryFrom<Block> for discriminating TRBs. If appropriate, define set/clear cycle bit for them also.
  • Should the mixed usage between different purpose TRBs be prohibited, just provide 3 different Block types: e.g. struct CommandTRB([u32;4]), struct TransferTRB([u32;4]) and struct EventTRB([u32;4]). With them we can even impose type safety to ring buffers - an event ring buffer should have &[EventTRB] and so on.

I feel "a PR is worth a thousand discussions" here. I'll do it shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants