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 at 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.> > Thanks, > Zhi. > > > The firmware is released once the abstraction is dropped. > > > > Signed-off-by: Danilo Krummrich <dakr at 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)] >
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 at 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 at 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)] >> >
On Tue, May 28, 2024 at 08:40:20AM +0000, Zhi Wang wrote:> 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 at 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.With a generic abstraction, as in 1), at lest some of the abstraction's functions would be unsafe themselves, since they have to rely on the layout (or offset) passed by the driver being correct. What if I pass a wrong layout / offset for a structure that contains a pointer? This might still result in an invalid pointer dereference. Am I missing something?> > 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.This sounds more like a driver specific abstraction to me, which, as you write, we can probably come up with some macros that help implementing it. But again, what if the offsets are within the boundary, but still at a wrong offset? What if the data obtained from a wrong offset leads to other safety implications when further processing them? I think no generic abstraction can ever cover the safety parts of this (entirely). I think there are always semanic parts to this the driver has to cover. Generally, I think we should aim for some generalization, but I think we should not expect it to cover all the safety aspects.> > The cons is still the driver might need to sync between the C-version > definition and rust-version definition.What do you mean with the driver needs to sync between a C and a Rust version of structure definitions?> > 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.I think we should have the structure definitions in Rust directly. - Danilo> > > >> > >> Thanks, > >> Zhi. > >>