Message ID | 20240520172554.182094-11-dakr@redhat.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Bjorn Helgaas |
Headers | show |
Series | Device / Driver and PCI Rust abstractions | expand |
On Mon, May 20, 2024 at 7:27 PM Danilo Krummrich <dakr@redhat.com> wrote: > > through its Drop() implementation. Nit: `Drop`, `Deref` and so on are traits -- what do the `()` mean here? I guess you may be referring to their method, but those are lowercase. > +/// IO-mapped memory, starting at the base pointer @ioptr and spanning @malxen bytes. Please use Markdown code spans instead (and intra-doc links where possible) -- we don't use the `@` notation. There is a typo on the variable name too. > +pub struct IoMem { > + pub ioptr: usize, This field is public, which raises some questions... > + pub fn readb(&self, offset: usize) -> Result<u8> { > + let ioptr: usize = self.get_io_addr(offset, 1)?; > + > + Ok(unsafe { bindings::readb(ioptr as _) }) > + } These methods are unsound, since `ioptr` may end up being anything here, given `self.ioptr` it is controlled by the caller. One could also trigger an overflow in `get_io_addr`. Wedson wrote a similar abstraction in the past (`rust/kernel/io_mem.rs` in the old `rust` branch), with a compile-time `SIZE` -- it is probably worth taking a look. Also, there are missing `// SAFETY:` comments here. Documentation and examples would also be nice to have. Thanks! Cheers, Miguel
> > Wedson wrote a similar abstraction in the past > (`rust/kernel/io_mem.rs` in the old `rust` branch), with a > compile-time `SIZE` -- it is probably worth taking a look. > Just on this point, we can't know in advance what size the IO BARs are at compile time. The old method just isn't useful for real devices with runtime IO BAR sizes. Dave.
(quick reply from my phone) > On 21. May 2024, at 00:32, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Mon, May 20, 2024 at 7:27 PM Danilo Krummrich <dakr@redhat.com> wrote: >> >> through its Drop() implementation. > > Nit: `Drop`, `Deref` and so on are traits -- what do the `()` mean > here? I guess you may be referring to their method, but those are > lowercase. > >> +/// IO-mapped memory, starting at the base pointer @ioptr and spanning @malxen bytes. > > Please use Markdown code spans instead (and intra-doc links where > possible) -- we don't use the `@` notation. There is a typo on the > variable name too. > >> +pub struct IoMem { >> + pub ioptr: usize, > > This field is public, which raises some questions... > >> + pub fn readb(&self, offset: usize) -> Result<u8> { >> + let ioptr: usize = self.get_io_addr(offset, 1)?; >> + >> + Ok(unsafe { bindings::readb(ioptr as _) }) >> + } > > These methods are unsound, since `ioptr` may end up being anything > here, given `self.ioptr` it is controlled by the caller. One could > also trigger an overflow in `get_io_addr`. > I think the only thing we really want from IoMem is a generic implementation of the read*() and write*() functions. Everything else should be up the the resource itself, see e.g. pci::Bar. Hence I think we should just make IoMem a trait instead and just let the resource implement a getter for the MMIO pointer, etc. > Wedson wrote a similar abstraction in the past > (`rust/kernel/io_mem.rs` in the old `rust` branch), with a > compile-time `SIZE` -- it is probably worth taking a look. > > Also, there are missing `// SAFETY:` comments here. Documentation and > examples would also be nice to have. > > Thanks! > > Cheers, > Miguel >
On Mon, 20 May 2024 at 23:07, Dave Airlie <airlied@gmail.com> wrote: > > > > > Wedson wrote a similar abstraction in the past > > (`rust/kernel/io_mem.rs` in the old `rust` branch), with a > > compile-time `SIZE` -- it is probably worth taking a look. > > > > Just on this point, we can't know in advance what size the IO BARs are > at compile time. > > The old method just isn't useful for real devices with runtime IO BAR sizes. The compile-time `SIZE` in my implementation is a minimum size. Attempts to read/write with constants within that size (offset + size) were checked at compile time, that is, they would have zero additional runtime cost when compared to C. Reads/writes beyond the minimum would have to be checked at runtime.
On Tue, 2024-05-21 at 00:32 +0200, Miguel Ojeda wrote: > On Mon, May 20, 2024 at 7:27 PM Danilo Krummrich <dakr@redhat.com> > wrote: > > > > through its Drop() implementation. > > Nit: `Drop`, `Deref` and so on are traits -- what do the `()` mean > here? I guess you may be referring to their method, but those are > lowercase. ACK > > > +/// IO-mapped memory, starting at the base pointer @ioptr and > > spanning @malxen bytes. > > Please use Markdown code spans instead (and intra-doc links where > possible) -- we don't use the `@` notation. There is a typo on the > variable name too. > > > +pub struct IoMem { > > + pub ioptr: usize, > > This field is public, which raises some questions... 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. The subsystem (as PCI does here) shall not make an instance of IoMem mutable, so the driver programmer couldn't modify ioptr. I'm very open for ideas for alternatives, though. See also the other mail where Danilo brainstorms about making IoMem a trait. > > > + pub fn readb(&self, offset: usize) -> Result<u8> { > > + let ioptr: usize = self.get_io_addr(offset, 1)?; > > + > > + Ok(unsafe { bindings::readb(ioptr as _) }) > > + } > > These methods are unsound, since `ioptr` may end up being anything > here, given `self.ioptr` it is controlled by the caller. Only if IoMem is mutable, correct? 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. > One could > also trigger an overflow in `get_io_addr`. 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? Only alternative I can think of would be to do a wrapping_add(), but that would be even worse UB. Ideas? > > Wedson wrote a similar abstraction in the past > (`rust/kernel/io_mem.rs` in the old `rust` branch), with a > compile-time `SIZE` -- it is probably worth taking a look. Yes, we're aware of that one. We also did some experiments with it. Will discuss it in the other thread where Dave and Wedson mention it. > > Also, there are missing `// SAFETY:` comments here. Documentation and > examples would also be nice to have. Oh yes, ACK, will do Thx for the review! > > Thanks! > > Cheers, > Miguel >
On Tue, 2024-05-21 at 00:01 -0300, Wedson Almeida Filho wrote: > On Mon, 20 May 2024 at 23:07, Dave Airlie <airlied@gmail.com> wrote: > > > > > > > > Wedson wrote a similar abstraction in the past > > > (`rust/kernel/io_mem.rs` in the old `rust` branch), with a > > > compile-time `SIZE` -- it is probably worth taking a look. > > > > > > > Just on this point, we can't know in advance what size the IO BARs > > are > > at compile time. > > > > The old method just isn't useful for real devices with runtime IO > > BAR sizes. > > The compile-time `SIZE` in my implementation is a minimum size. > > Attempts to read/write with constants within that size (offset + > size) > were checked at compile time, that is, they would have zero > additional > runtime cost when compared to C. Reads/writes beyond the minimum > would > have to be checked at runtime. > We looked at this implementation Its disadvantage is that it moves the responsibility for setting that minimum size to the driver programmer. Andreas Hindborg is using that currently for rnvme [1]. I believe that the driver programmer in Rust should not be responsible for controlling such sensitive parameters (one could far more easily provide invalid values), but the subsystem (e.g. PCI) should do it, because it knows about the exact resource lengths. The only way to set the actual, real value is through subsystem code. But when we (i.e., currently, the driver programmer) have to use that anyways, we can just use it from the very beginning and have the exact valid parameters. So driver programmers would always have correct IoMem and would have a harder time breaking stuff, and wouldn't have to think about minimum lengths and things like that. P. [1] https://github.com/metaspace/linux/blob/rnvme/drivers/block/rnvme.rs#L580
On Tue, May 21, 2024 at 9:36 AM Philipp Stanner <pstanner@redhat.com> 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
On Tue, May 21, 2024 at 11:18:04AM +0200, Miguel Ojeda wrote: > On Tue, May 21, 2024 at 9:36 AM Philipp Stanner <pstanner@redhat.com> 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. As mentioned earlier, given the context how we use IoMem, I think IoMem should just be a trait. And given that, maybe we'd want to name this trait differently then, something like `trait IoOps` maybe? pub trait IoOps { // INVARIANT: The implementation must ensure that the returned value is // either an error code or a non-null and valid address suitable for I/O // operations of the given offset and length. fn io_addr(&self, offset: usize, len: usize) -> Result<usize>; fn readb(&self, offset: usize) -> Result<u8> { let addr = self.io_addr(offset, 1)?; // SAFETY: `addr` is guaranteed to be valid as by the invariant required // by `io_addr`. Ok(unsafe { bindings::readb(addr as _) }) } [...] } We can let the resource type (e.g. `pci::Bar`) track the base address and limit instead and just let pci::Bar implement `IoMem::io_addr`. As for the compile time size, this would be up the the actual resource then. `pci::Bar` can't make use of this optimization, while others might be able to. Does that sound reasonable? - Danilo
On Tue, 21 May 2024 at 05:03, Philipp Stanner <pstanner@redhat.com> wrote: > > On Tue, 2024-05-21 at 00:01 -0300, Wedson Almeida Filho wrote: > > On Mon, 20 May 2024 at 23:07, Dave Airlie <airlied@gmail.com> wrote: > > > > > > > > > > > Wedson wrote a similar abstraction in the past > > > > (`rust/kernel/io_mem.rs` in the old `rust` branch), with a > > > > compile-time `SIZE` -- it is probably worth taking a look. > > > > > > > > > > Just on this point, we can't know in advance what size the IO BARs > > > are > > > at compile time. > > > > > > The old method just isn't useful for real devices with runtime IO > > > BAR sizes. > > > > The compile-time `SIZE` in my implementation is a minimum size. > > > > Attempts to read/write with constants within that size (offset + > > size) > > were checked at compile time, that is, they would have zero > > additional > > runtime cost when compared to C. Reads/writes beyond the minimum > > would > > have to be checked at runtime. > > > > We looked at this implementation > > Its disadvantage is that it moves the responsibility for setting that > minimum size to the driver programmer. Andreas Hindborg is using that > currently for rnvme [1]. > > I believe that the driver programmer in Rust should not be responsible > for controlling such sensitive parameters (one could far more easily > provide invalid values), but the subsystem (e.g. PCI) should do it, > because it knows about the exact resource lengths. There is no responsibility being moved. The bus is still that one that knows about the resources attached to the device. The driver, however, can say for example: I need at least 4 registers of 32 bits starting at offset X, which results in a minimum size of X + 16. If at runtime a device compatible with this driver appears and has an io mem of at least that size, then the driver can drive it without any additional runtime checks. I did this in the gpio driver here: https://lwn.net/Articles/863459/ Note that in addition to not having to check offset at runtime, the reads/writes are also infallible because all failures are caught at compile time. Obviously not all drivers can benefit from this, but it is considerable simplification for the ones that can. > The only way to set the actual, real value is through subsystem code. > But when we (i.e., currently, the driver programmer) have to use that > anyways, we can just use it from the very beginning and have the exact > valid parameters. Yes, only the bus knows. But to reiterate: if the driver declares and checks a minimum size at attach time, it obviates the needs to check again throughout the lifetime of the driver, which is more performant and eliminates error paths.
diff --git a/rust/helpers.c b/rust/helpers.c index c3d80301185c..dc2405772b1a 100644 --- a/rust/helpers.c +++ b/rust/helpers.c @@ -34,6 +34,7 @@ #include <linux/wait.h> #include <linux/workqueue.h> #include <linux/pci.h> +#include <linux/io.h> __noreturn void rust_helper_BUG(void) { @@ -179,6 +180,111 @@ int rust_helper_devm_add_action(struct device *dev, void (*action)(void *), void return devm_add_action(dev, action, data); } +/* io.h */ +u8 rust_helper_readb(const volatile void __iomem *addr) +{ + return readb(addr); +} +EXPORT_SYMBOL_GPL(rust_helper_readb); + +u16 rust_helper_readw(const volatile void __iomem *addr) +{ + return readw(addr); +} +EXPORT_SYMBOL_GPL(rust_helper_readw); + +u32 rust_helper_readl(const volatile void __iomem *addr) +{ + return readl(addr); +} +EXPORT_SYMBOL_GPL(rust_helper_readl); + +#ifdef CONFIG_64BIT +u64 rust_helper_readq(const volatile void __iomem *addr) +{ + return readq(addr); +} +EXPORT_SYMBOL_GPL(rust_helper_readq); +#endif + +void rust_helper_writeb(u8 value, volatile void __iomem *addr) +{ + writeb(value, addr); +} +EXPORT_SYMBOL_GPL(rust_helper_writeb); + +void rust_helper_writew(u16 value, volatile void __iomem *addr) +{ + writew(value, addr); +} +EXPORT_SYMBOL_GPL(rust_helper_writew); + +void rust_helper_writel(u32 value, volatile void __iomem *addr) +{ + writel(value, addr); +} +EXPORT_SYMBOL_GPL(rust_helper_writel); + +#ifdef CONFIG_64BIT +void rust_helper_writeq(u64 value, volatile void __iomem *addr) +{ + writeq(value, addr); +} +EXPORT_SYMBOL_GPL(rust_helper_writeq); +#endif + +u8 rust_helper_readb_relaxed(const volatile void __iomem *addr) +{ + return readb_relaxed(addr); +} +EXPORT_SYMBOL_GPL(rust_helper_readb_relaxed); + +u16 rust_helper_readw_relaxed(const volatile void __iomem *addr) +{ + return readw_relaxed(addr); +} +EXPORT_SYMBOL_GPL(rust_helper_readw_relaxed); + +u32 rust_helper_readl_relaxed(const volatile void __iomem *addr) +{ + return readl_relaxed(addr); +} +EXPORT_SYMBOL_GPL(rust_helper_readl_relaxed); + +#ifdef CONFIG_64BIT +u64 rust_helper_readq_relaxed(const volatile void __iomem *addr) +{ + return readq_relaxed(addr); +} +EXPORT_SYMBOL_GPL(rust_helper_readq_relaxed); +#endif + +void rust_helper_writeb_relaxed(u8 value, volatile void __iomem *addr) +{ + writeb_relaxed(value, addr); +} +EXPORT_SYMBOL_GPL(rust_helper_writeb_relaxed); + +void rust_helper_writew_relaxed(u16 value, volatile void __iomem *addr) +{ + writew_relaxed(value, addr); +} +EXPORT_SYMBOL_GPL(rust_helper_writew_relaxed); + +void rust_helper_writel_relaxed(u32 value, volatile void __iomem *addr) +{ + writel_relaxed(value, addr); +} +EXPORT_SYMBOL_GPL(rust_helper_writel_relaxed); + +#ifdef CONFIG_64BIT +void rust_helper_writeq_relaxed(u64 value, volatile void __iomem *addr) +{ + writeq_relaxed(value, addr); +} +EXPORT_SYMBOL_GPL(rust_helper_writeq_relaxed); +#endif + void rust_helper_pci_set_drvdata(struct pci_dev *pdev, void *data) { pci_set_drvdata(pdev, data); diff --git a/rust/kernel/iomem.rs b/rust/kernel/iomem.rs new file mode 100644 index 000000000000..efb6cd0829b4 --- /dev/null +++ b/rust/kernel/iomem.rs @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: GPL-2.0 + +use crate::bindings; +use crate::error::{code::EINVAL, Result}; + +/// IO-mapped memory, starting at the base pointer @ioptr and spanning @malxen bytes. +/// +/// The creator (usually a subsystem such as PCI) is responsible for creating the +/// mapping, performing an additional region request etc. +pub struct IoMem { + pub ioptr: usize, + maxlen: usize, +} + +impl IoMem { + pub(crate) fn new(ioptr: usize, maxlen: usize) -> Result<Self> { + if ioptr == 0 || maxlen == 0 { + return Err(EINVAL); + } + + Ok(Self { ioptr, maxlen }) + } + + fn get_io_addr(&self, offset: usize, len: usize) -> Result<usize> { + if offset + len > self.maxlen { + return Err(EINVAL); + } + + Ok(self.ioptr + offset) + } + + pub fn readb(&self, offset: usize) -> Result<u8> { + let ioptr: usize = self.get_io_addr(offset, 1)?; + + Ok(unsafe { bindings::readb(ioptr as _) }) + } + + pub fn readw(&self, offset: usize) -> Result<u16> { + let ioptr: usize = self.get_io_addr(offset, 2)?; + + Ok(unsafe { bindings::readw(ioptr as _) }) + } + + pub fn readl(&self, offset: usize) -> Result<u32> { + let ioptr: usize = self.get_io_addr(offset, 4)?; + + Ok(unsafe { bindings::readl(ioptr as _) }) + } + + pub fn readq(&self, offset: usize) -> Result<u64> { + let ioptr: usize = self.get_io_addr(offset, 8)?; + + Ok(unsafe { bindings::readq(ioptr as _) }) + } + + pub fn readb_relaxed(&self, offset: usize) -> Result<u8> { + let ioptr: usize = self.get_io_addr(offset, 1)?; + + Ok(unsafe { bindings::readb_relaxed(ioptr as _) }) + } + + pub fn readw_relaxed(&self, offset: usize) -> Result<u16> { + let ioptr: usize = self.get_io_addr(offset, 2)?; + + Ok(unsafe { bindings::readw_relaxed(ioptr as _) }) + } + + pub fn readl_relaxed(&self, offset: usize) -> Result<u32> { + let ioptr: usize = self.get_io_addr(offset, 4)?; + + Ok(unsafe { bindings::readl_relaxed(ioptr as _) }) + } + + pub fn readq_relaxed(&self, offset: usize) -> Result<u64> { + let ioptr: usize = self.get_io_addr(offset, 8)?; + + Ok(unsafe { bindings::readq_relaxed(ioptr as _) }) + } + + pub fn writeb(&self, byte: u8, offset: usize) -> Result { + let ioptr: usize = self.get_io_addr(offset, 1)?; + + unsafe { bindings::writeb(byte, ioptr as _) } + Ok(()) + } + + pub fn writew(&self, word: u16, offset: usize) -> Result { + let ioptr: usize = self.get_io_addr(offset, 2)?; + + unsafe { bindings::writew(word, ioptr as _) } + Ok(()) + } + + pub fn writel(&self, lword: u32, offset: usize) -> Result { + let ioptr: usize = self.get_io_addr(offset, 4)?; + + unsafe { bindings::writel(lword, ioptr as _) } + Ok(()) + } + + pub fn writeq(&self, qword: u64, offset: usize) -> Result { + let ioptr: usize = self.get_io_addr(offset, 8)?; + + unsafe { bindings::writeq(qword, ioptr as _) } + Ok(()) + } + + pub fn writeb_relaxed(&self, byte: u8, offset: usize) -> Result { + let ioptr: usize = self.get_io_addr(offset, 1)?; + + unsafe { bindings::writeb_relaxed(byte, ioptr as _) } + Ok(()) + } + + pub fn writew_relaxed(&self, word: u16, offset: usize) -> Result { + let ioptr: usize = self.get_io_addr(offset, 2)?; + + unsafe { bindings::writew_relaxed(word, ioptr as _) } + Ok(()) + } + + pub fn writel_relaxed(&self, lword: u32, offset: usize) -> Result { + let ioptr: usize = self.get_io_addr(offset, 4)?; + + unsafe { bindings::writel_relaxed(lword, ioptr as _) } + Ok(()) + } + + pub fn writeq_relaxed(&self, qword: u64, offset: usize) -> Result { + let ioptr: usize = self.get_io_addr(offset, 8)?; + + unsafe { bindings::writeq_relaxed(qword, ioptr as _) } + Ok(()) + } +}