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

[Zhi Wang <zhiw@nvidia.com>]

On 28/05/2024 17.18, Danilo Krummrich wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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@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.
> 

I was thinking of a proc_macro that takes a vender-firmware struct 
definition. As it can get the type and the name of each member, then it 
can generate methods like xxx::member_a() that returns the value from 
the "unsafe {*(type *)(pointer + offset)}". Thus, the unsafe stuff stays 
in the generated methods.

For offset, I was hoping the macro can automatically calculate it based 
on the member offset (the vendor-firmware definition) when generating 
xxx::member_x(). (Maybe it can also take alignment into account)

As the return value has a rust type, rust should catch it if the caller 
wants to do something crazy there.

> Generally, I think we should aim for some generalization, but I think we should
> not expect it to cover all the safety aspects.
> 

I agree. I was mostly thinking how to ease the pain of driver and see 
how the best we can do for it.

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

Let's say a C driver maintains quite some headers and support some not 
very new HWs. A new rust driver maintains some headers that written in 
rust, it needs those headers as well. Now the firmware updates, both the 
C headers and rust headers needs to be updated accordingly due to ABI 
changes.

I was thinking if that process can be optimized, at least trying to 
avoid the sync process, which might be painful if the amount of the 
headers are huge.

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