Re: [RFC PATCH 7/8] rust: add firmware abstractions

[Zhi Wang <zhiw@nvidia.com>]

On 27/05/2024 22.18, Danilo Krummrich wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, May 21, 2024 at 08:32:31AM +0300, Zhi Wang wrote:
>> On Mon, 20 May 2024 19:24:19 +0200
>> Danilo Krummrich <dakr@redhat.com> wrote:
>>
>>> Add an abstraction around the kernels firmware API to request firmware
>>> images. The abstraction provides functions to access the firmware
>>> buffer and / or copy it to a new buffer allocated with a given
>>> allocator backend.
>>>
>>
>> Was playing with firmware extractions based on this patch.
>> Unfortunately I ended up with a lot of pointer operations, unsafe
>> statements.
>>
>> As we know many vendors have a C headers for the definitions of the
>> firwmare content, the driver extract the data by applying a struct
>> pointer on it.
>>
>> But in rust, I feel it would nice that we can also have a common
>> firmware extractor for drivers, that can wrap the pointer operations,
>> take a list of the firmware struct members that converted from C headers
>> as the input, offer the driver some common ABI methods to query them.
>> Maybe that would ease the pain a lot.
> 
> So, you mean some abstraction that takes a list of types, offsets in the
> firmware and a reference to the firmware itself and provides references to the
> corresponding objects?
> 
> I agree it might be helpful to have some common infrastructure for this, but the
> operations on it would still be unsafe, since ultimately it involves
> dereferencing pointers.
> 

I think the goal is to 1) concentrate the "unsafe" operations in a 
abstraction so the driver doesn't have to write explanation of unsafe 
operations here and there, improve the readability of the driver that 
interacts with vendor-firmware buffer. 2) ease the driver maintenance 
burden.

Some solutions I saw in different projects written in rust for 
de-referencing a per-defined struct:

1) Take the vendor firmware buffer as a whole, invent methods like 
read/write with offset for operating the buffer.

In this scheme, the driver is responsible for taking care of each data 
member in a vendor firmware struct and also its valid offset. The 
abstraction only covers the boundary of the whole firmware buffer.

The cons is the readability of the operation of the vendor firmware 
buffer in the driver is not good comparing with C code.

Hate to think a lot of xxx = vendor_firmware_struct.read(offset); // 
reading item A in the code.

2) Define the firmware struct in rust (might need to find a better way 
to handle "union" in the definition of the vendor firmware struct). Use 
macros to generate methods of accessing each data member for the vendor 
firmware struct.

Then the code in the driver will be like xxx = 
vendor_firmware_struct.item_a(); in the driver.

In this scheme, the abstraction and the generated methods covers the 
boundary check. The "unsafe" statement can stay in the generated 
struct-access methods.

This might even be implemented as a more generic rust feature in the kernel.

The cons is still the driver might need to sync between the C-version 
definition and rust-version definition.

3) Also re-using C bindings generation in the kernel came to my mind 
when thinking of this problem, since it allows the rust code to access 
the C struct, but they don't have the boundary check. Still need another 
layer/macros to wrap it.


>>
>> Thanks,
>> Zhi.
>>
>>> The firmware is released once the abstraction is dropped.
>>>
>>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
>>> ---
>>>   rust/bindings/bindings_helper.h |  1 +
>>>   rust/kernel/firmware.rs         | 74
>>> +++++++++++++++++++++++++++++++++ rust/kernel/lib.rs              |
>>> 1 + 3 files changed, 76 insertions(+)
>>>   create mode 100644 rust/kernel/firmware.rs
>>>
>>> diff --git a/rust/bindings/bindings_helper.h
>>> b/rust/bindings/bindings_helper.h index b245db8d5a87..e4ffc47da5ec
>>> 100644 --- a/rust/bindings/bindings_helper.h
>>> +++ b/rust/bindings/bindings_helper.h
>>> @@ -14,6 +14,7 @@
>>>   #include <kunit/test.h>
>>>   #include <linux/errname.h>
>>>   #include <linux/ethtool.h>
>>> +#include <linux/firmware.h>
>>>   #include <linux/jiffies.h>
>>>   #include <linux/mdio.h>
>>>   #include <linux/pci.h>
>>> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
>>> new file mode 100644
>>> index 000000000000..700504fb3c9c
>>> --- /dev/null
>>> +++ b/rust/kernel/firmware.rs
>>> @@ -0,0 +1,74 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! Firmware abstraction
>>> +//!
>>> +//! C header:
>>> [`include/linux/firmware.h`](../../../../include/linux/firmware.h") +
>>> +use crate::{bindings, device::Device, error::Error, error::Result,
>>> str::CStr, types::Opaque}; +
>>> +/// Abstraction around a C firmware struct.
>>> +///
>>> +/// This is a simple abstraction around the C firmware API. Just
>>> like with the C API, firmware can +/// be requested. Once requested
>>> the abstraction provides direct access to the firmware buffer as +///
>>> `&[u8]`. Alternatively, the firmware can be copied to a new buffer
>>> using `Firmware::copy`. The +/// firmware is released once
>>> [`Firmware`] is dropped. +/// +/// # Examples
>>> +///
>>> +/// ```
>>> +/// let fw = Firmware::request("path/to/firmware.bin",
>>> dev.as_ref())?; +/// driver_load_firmware(fw.data());
>>> +/// ```
>>> +pub struct Firmware(Opaque<*const bindings::firmware>);
>>> +
>>> +impl Firmware {
>>> +    /// Send a firmware request and wait for it. See also
>>> `bindings::request_firmware`.
>>> +    pub fn request(name: &CStr, dev: &Device) -> Result<Self> {
>>> +        let fw = Opaque::uninit();
>>> +
>>> +        let ret = unsafe { bindings::request_firmware(fw.get(),
>>> name.as_char_ptr(), dev.as_raw()) };
>>> +        if ret != 0 {
>>> +            return Err(Error::from_errno(ret));
>>> +        }
>>> +
>>> +        Ok(Firmware(fw))
>>> +    }
>>> +
>>> +    /// Send a request for an optional fw module. See also
>>> `bindings::request_firmware_nowarn`.
>>> +    pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self>
>>> {
>>> +        let fw = Opaque::uninit();
>>> +
>>> +        let ret = unsafe {
>>> +            bindings::firmware_request_nowarn(fw.get(),
>>> name.as_char_ptr(), dev.as_raw())
>>> +        };
>>> +        if ret != 0 {
>>> +            return Err(Error::from_errno(ret));
>>> +        }
>>> +
>>> +        Ok(Firmware(fw))
>>> +    }
>>> +
>>> +    /// Returns the size of the requested firmware in bytes.
>>> +    pub fn size(&self) -> usize {
>>> +        unsafe { (*(*self.0.get())).size }
>>> +    }
>>> +
>>> +    /// Returns the requested firmware as `&[u8]`.
>>> +    pub fn data(&self) -> &[u8] {
>>> +        unsafe {
>>> core::slice::from_raw_parts((*(*self.0.get())).data, self.size()) }
>>> +    }
>>> +}
>>> +
>>> +impl Drop for Firmware {
>>> +    fn drop(&mut self) {
>>> +        unsafe { bindings::release_firmware(*self.0.get()) };
>>> +    }
>>> +}
>>> +
>>> +// SAFETY: `Firmware` only holds a pointer to a C firmware struct,
>>> which is safe to be used from any +// thread.
>>> +unsafe impl Send for Firmware {}
>>> +
>>> +// SAFETY: `Firmware` only holds a pointer to a C firmware struct,
>>> references to which are safe to +// be used from any thread.
>>> +unsafe impl Sync for Firmware {}
>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>>> index 6415968ee3b8..ed97d131661a 100644
>>> --- a/rust/kernel/lib.rs
>>> +++ b/rust/kernel/lib.rs
>>> @@ -35,6 +35,7 @@
>>>   #[cfg(CONFIG_DRM = "y")]
>>>   pub mod drm;
>>>   pub mod error;
>>> +pub mod firmware;
>>>   pub mod init;
>>>   pub mod ioctl;
>>>   #[cfg(CONFIG_KUNIT)]
>>
>