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

[Joel Fernandes <joelagnelf@nvidia.com>]

On 4/1/2025 12:44 PM, Danilo Krummrich wrote:
> 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.

Thanks a lot for the quick reply. I tried the try_readb() variant and it works
now. I think instead of me doing a runtime check to satisfy the compiler, it may
be better for me rely on the try accessor's runtime checking so I will stick to
that.

Thanks again!

 - Joel



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