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
