Lifetimed indexing for array accessors

[paulsohn]

This PR is the generic design of xhci crate xhci::registers::runtime::{InterrupterRegisterSet, Interrupter} pair.
Main changes are:

• Suggesting indexing entries before read or write. a.read_volatile_at(i) could be rewritten to a.at(i).access.read_volatile().
• If the underlying type T of the accessor is a field struct, instead of a.at(i).read_volatile().x to use one big accessor, we can have a struct of accessors, each field of which represents the accessor to the field of same name, i.e. a.set_at(i).x.read_volatile().
  To use this, the definition of struct T should be decorated with #[derive(accessor::array::LifetimedSetGenericOf)]. This will generate a struct-of-accessors type LifetimedSetGenericOfT, and implement accessor::array::LifetimedSetGeneric<T> with it the as associated type.

Additionally, this PR includes:

• Add built-in accessor::mapper::Identity mapper. The Identity mapper either repeats already-mapped virtual addresses, or assumes that we are using physical addresses all along.

• accessor::mapper::Mapper should implement [Clone] for a.at(i) and a.set_at(i) methods. (Alternatively we could revert this change and implement a.at(i) and a.set_at(i) method only for M: Mapper + Clone.) ((Edit) not necessarily.)

• accessor::marker::AccessorTypeSpecifier should implement [Copy], which is done by default. ((Edit) pointless change now, but a new commit to just revert this seems too much.)

• Crate memoffset is re-exported as accessor::memoffset so that macro expansion can resolve it corretly.
  I'm new to proc-macros, so I'm not sure if it's a right way to refer 3rd-party crates in proc-macro code generation.

Disclaimer: not tested in runtime.
Any suggestions (and testing) are welcomed.

[paulsohn]

Here is some quick self-review. I'm currently working on these issues.

• Change name Lifetimed* into Bound*.
• Found that creating another accessor on indexing might result to "double unmapping" when dropped.
  A possible solution is to make new markers to distinguish whether this accessor is manually created or autogenerated as bound accessor.
• Instead of a.at(i).access.read_volatile(), BoundGeneric can have some impls which allow a.at(i).read_volatile() directly.
• Whether a mapper should implement Clone or not should be user's choice and will be reverted. .at() and .set_at() method will be implemented only for Clone-implemented mappers.

Random thoughts:

• Indexing a register might be unsafe since we might have duplicate accessors like let x = a.at(1); let y = a.at(1);.
  However, since accessors are bound to the array accessor scope, the user can manage the scope.
  For reference, this is exactly how xhci crate realizes interrupter register sets (xhci::registers::runtime::InterrupterRegisterSet::interrupter method is marked as safe)
• .set_at(i) might cause some overhead if the struct data type has lots of fields. Maybe we can adjust it so that .set_at(i) doesn't instantiate a set of accessors eagerly (i.e. .set_at(i).field_a.read_volatile()) but rather lazily with methods (i.e. .set_at(i).field_a()). This requires proc-macro redesign.
• A little bit off-topic, but why should .read() and .write() be deprecated? It's clearly stated that this crate is mainly for volatile MMIO, so _volatile seems redundant.

[paulsohn]

Hinted from InterrupterRegisterSet implementation, in order to prevent double mapping-unmapping issue, we can make array accessor initialization doesn't perform any address mapping.
In this case, mapping only comes when bound accessors are generated by .at(i) or .set_at(i), and will be unmapped when the bound accessors are dropped.

A pitfall of this solution is that

• mappings and unmappings occur too frequently as much as we call .at(i).
• if we have duplicate accessors at the same time let x = a.at(1); let y = a.at(1);, then memory mappings will be occured twice for the same physical address.

For now, I believe that the closest-to-best solution is to map the entire array region once, unmap it when dropped, and add another set of marker types into single-entry accessor type definition, if it doesn't complicate things too much.

[toku-sa-n]

Thanks for the PR, and sorry for the delay. I'm riviewing the code. Meanwhile, could you apply rustfmt?

[paulsohn]

Converted to draft since this PR is incomplete and has a double-unmapping problem mentioned above.
(Maybe I should make 'cleaner' multiple PRs, one per a change, instead of this one.)

[paulsohn]

Resolved double mapping issue w/o creating any new marker structs. The (wrapped) set of single-elem accessors returned from .set_at(i) previously had mapper type M (which required Clone) but now Identity. .at(i) is modified similarly.
Thus, the mapper trait requirement Mapper + Clone has been relaxed to Mapper. Any accessors of any mappers can use .at(i).read_volatile() syntax.

Removed draft status of the PR. Please notify me if anything is unclear.

(Edit) Just followed up rust-osdev/xhci#142 .
I'm working on that crate now, and hopefully this will reduce some code complexity.
Unfortunately, I can neither test (at least in this year) nor guarantee if this works on actual device controllers.

[toku-sa-n]

Sorry, but I'm still trying to understand the code. It'd be great if you provide some sample code that works with your patch. Meanwhile, I think this crate needs test codes, so I'll add them in another PR.

Also, is it correct that the fundamental idea is, instead of having an accessor covering the whole range of a struct or an array, to let such an accessor generate an accessor for its element with a lifetime bound?

[paulsohn]

instead of having an accessor covering the whole range of a struct or an array, to let such an accessor generate an accessor for its element with a lifetime bound

Roughly correct. To sum up:

• An array accessor generates bound single-entry accessors when indexed with .at(idx).
  By the way, this will not deprecate direct methods such as .read_volatile_at(idx) (at least for now), although this is (should be) semantically identical to .at(idx).read_volatile().
• Also, for an array accessor of struct T, #[derive(BoundSetGenericOf)] on the struct definition T will enable .set_at(idx) method which spawns a struct of accessors, one for a field.

I have documented the use cases of new structs. You can refer to them when you make test codes.

[paulsohn]

This is an example of indexing using .at(i) methods. Unlike .set_at(i), this doesn't require to refer any traits.

Basically, .at(i) method is introduced to replace .***_volatile_at(i) with .at(i).***_volatile() so most use cases are limited to these situations.

[toku-sa-n]

Thanks for applying my suggestions.

[toku-sa-n]

Could you create another PR for this change?

[paulsohn]

This identity mapper is actually coupled with the rest of the changes and can't be separated. A bounded accessor internally uses identity as its mapper in order to preserve already-mapped vaddr space.

Say we reserve 0x1000..0x1010 physical address range for [u32;4] array accessor a, and our mapper maps 0x1000 page into 0x3000 frame. Then a.at(1) should hold virtual address range 0x3004..0x3008.
However if we use the same mapper again when creating this bounded accessor, the mapper recognizes 0x3000 as another physical page and will map it into (e.g.) 0x5000 frame, and the bounded accessor will get the wrong range 0x5004..0x5008. (This is what I referred to as 'double mapping issue'.)
The solution I brought for this is to use identity mapper for element accessor, which prevents 0x3000 to be mapped to anywhere except 0x3000 itself.

[toku-sa-n]

Sorry for the late response.

This identity mapper is actually coupled with the rest of the changes and can't be separated.

Does it mean you'll change this mapper in later commits? If not, sorry for not being clear enough, but I thought this identify mapper itself was useful as many people use identify mapping, and I can merge the PR before #44 if you separate this change.

[paulsohn]

Oops. I'd misunderstood that you have meant "This Identity mapper is irrelevant to the rest of the changes so it should be in a separated PR".
For convenience's sake, identity mapper could be included in advance to this PR. Please wait for a couple of days.

[paulsohn]

Thanks for the response. I renamed some structs.

• .set_at(i) -> .structural_at(i)
• BoundGeneric -> Bounded
• BoundedSetGeneric, BoundedSetGenericOf -> BoundedStructural, BoundedStructuralOf

Clippy now complains that there are no Debug impls for Bounded.
Since write-only accessors doesn't impl Debug, I'm curious about how can I satisfy clippy.

[toku-sa-n]

Since write-only accessors doesn't impl Debug, I'm curious about how can I satisfy clippy.

Is it possible to implement Debug if AccessorTypeSpecifier implements Readable and A implements Debug?

[toku-sa-n]

I added a few tests for the existing accessor types. Could you add additional tests for this PR too?

[paulsohn]

Sorry for delay.
I'm aware for your reviews, but I'm currently working on both this PR and an xHC driver.

For some register arrays, it is more concise to design each register entry be moved entirely into another data structures.
An event ring should hold (technically shared with xHC but anyway) an interrupter, and device should hold a doorbell.

So although unsafe, I added an unbounded indexing method .unbounded_at(i) which extracts an unbounded single entry accessor out from an array accessor. .unbounded_structural_at(i) will be added also, but it requires proc-macro rewriting so I'm currently lazy for that.

(I might got something wrong here. I don't have any test OS or any experience writing a driver, so correct me if anything is misunderstood.)

(Edit) self.unbounded_structural_at(i) was implemented as (unsafe { self.unbounded_at(i) }).structural().