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