Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Danilo Krummrich]

On Tue, Apr 01, 2025 at 11:57:35AM -0400, Joel Fernandes wrote:
> On Thu, Feb 06, 2025 at 12:57:30PM -0300, Daniel Almeida wrote:
> > Btw, Miguel & others,
> > 
> > IMHO, I think we should write a comment about this somewhere in the docs.
> > 
> > When I first came across this issue myself, it took me a while to understand that
> > the build_error was actually triggering.
> > 
> > That’s because the result is:
> > 
> > ```
> > ERROR: modpost: "rust_build_error" [rust_platform_uio_driver.ko] undefined!
> > ```
> > 
> > When a symbol is undefined, someone would be within their rights to assume that
> > something is broken in some KConfig somewhere, like this person did. It specifically
> > doesn’t tell them that the problem is their own code triggering a build_error because
> > they are misusing an API.
> > 
> > I know that we can’t really provide a message through build_error itself, hence my
> > suggestion about the docs.
> > 
> > I can send a patch if you agree, it will prevent this confusion from coming up in the
> > future.
> 
> Interesting, I just ran into this. I am writing function as follows that
> reads bar0 in the nova driver, however not having the "if current_len + i"
> causes the same issue:
> 
> ERROR: modpost: "rust_build_error" [nova_core.ko] undefined!
> 
> which did not help much about what the issue really is. I had to figure it
> out through tedious trial and error. Also what is the reason for this, the
> compiler is doing some checks in with_bar? Looking at with_bar
> implementation, I could not see any. Also enabling
> CONFIG_RUST_BUILD_ASSERT_ALLOW did not show more menaingful messages. Thanks
> for taking a look:
> 
>     pub(crate) fn read_more(&mut self, bytes: u32) -> Result {
>         with_bar!(self.bar0, |bar0| {
>             // Get current length
>             let current_len = self.data.len();
> 
>             // Read ROM data bytes push directly to vector
>             for i in 0..bytes as usize {
> 
> 		// This check fixes:
> 		// ERROR: modpost: "rust_build_error" [nova_core.ko] undefined!
>                 if current_len + i >= 10000000 {
>                     return Err(EINVAL);
>                 }
> 
>                 // Read a byte from the VBIOS ROM and push it to the data vector
>                 let rom_addr = ROM_OFFSET + current_len + i;
>                 let byte = bar0.readb(rom_addr);

The problem here is that the compiler can't figure out the offset (rom_addr) on
compile time, thus it fails to compile.

The non-try variants of I/O accessors are only supposed to work if the compiler
can figure out the offset on compile time, such that it can be checked against
the expected bar size (not the actual bar size). The expected bar size is what
the driver puts in as a const generic when calling
pci::Device::iomap_region_sized(). This is what makes the non-try variants
infallible.

If the offset is not known at compile time, bar0.try_readb() can be used
instead, which performs a runtime check against the actual bar size instead.

Your above check seems to be enough to let the compiler figure out that
ROM_OFFSET + current_len + i can never be out of bounds for bar0.

>                 self.data.push(byte, GFP_KERNEL)?;
>             }
> 
>             Ok(())
>         })?
>     }
> 
> thanks,
> 
>  - Joel
>