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

[Danilo Krummrich]

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.
> >>