Re: [RFC PATCH 10/11] rust: add basic abstractions for iomem operations

[Miguel Ojeda]

On Tue, May 21, 2024 at 9:36 AM Philipp Stanner <pstanner@xxxxxxxxxx> wrote:
>
> Justified questions – it is public because the Drop implementation for
> pci::Bar requires the ioptr to pass it to pci_iounmap().
>
> The alternative would be to give pci::Bar a copy of ioptr (it's just an
> integer after all), but that would also not be exactly beautiful.

If by copy you mean keeping an actual copy elsewhere, then you could
provide an access method instead.

If you meant the access method, it may not be "beautiful" (Why? What
do you mean?), but it is way more important to be sound.

> The commit message states (btw this file would get more extensive
> comments soonish) that with this design its the subsystem that is
> responsible for creating IoMem validly, because the subsystem is the
> one who knows about the memory regions and lengths and stuff.
>
> The driver should only ever take an IoMem through a subsystem, so that
> would be safe.

The Rust safety boundary is normally the module -- you want that
subsystems cannot make mistakes either when using the `iomem` module,
not just drivers when using the subsystem APIs.

So you can't rely on a user, even if it is a subsystem, to "validly
create" objects and also hope that they do not modify the fields later
etc. If you need to ask subsystems for that, then you need to require
`unsafe` somewhere, e.g. the constructor (and make the field private,
and add an invariant to the type, and add `INVARIANT:` comments).

Think about it this way: if we were to write all the code like that
(e.g. with all structs using public fields), then essentially we would
be back at C, since we would be trusting everybody not to touch what
they shouldn't, and not to put values that could later lead something
else into UB, and we would not even have the documentation/invariants
to verify those either, etc.

> Yes, if the addition violates the capacity of a usize. But that would
> then be a bug we really want to notice, wouldn't we?

Definitely, but what I wanted is that you consider whether it is
reasonable to have the panic possibility there, since it depends on a
value that others control. For instance, would it make sense to make
it a fallible operation instead? Should the panic be documented
otherwise? Could it be prevented somehow? etc.

Please check Wedson's `io_mem` in the old `rust` branch for some ideas too.

Cheers,
Miguel