Message-ID: <bc2e02d7-d4a7-4f0f-852c-e26ad6a8688f@proton.me>
Date: Thu, 22 Aug 2024 05:55:32 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Matthew Maurer <mmaurer@...gle.com>, Sami Tolvanen <samitolvanen@...gle.com>, Masahiro Yamada <masahiroy@...nel.org>, Luis Chamberlain <mcgrof@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Gary Guo <gary@...yguo.net>, Petr Pavlu <petr.pavlu@...e.com>, Neal Gompa <neal@...pa.dev>, Hector Martin <marcan@...can.st>, Janne Grunau <j@...nau.net>, Asahi Linux <asahi@...ts.linux.dev>, linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org, linux-modules@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 16/19] gendwarfksyms: Add support for reserved structure fields

On 22.08.24 01:29, Greg Kroah-Hartman wrote:
> On Wed, Aug 21, 2024 at 11:31:25AM +0000, Benno Lossin wrote:
>> On 20.08.24 22:03, Matthew Maurer wrote:
>>>>> The way `KAbiReserved` is implemented is via a `union` (maybe a bit
>>>>> ironic, considering what I said in my other replies, but in this case,
>>>>> we would provide a safe abstraction over this `union`, thus avoiding
>>>>> exposing users of this type to `unsafe`):
>>>>>
>>>>>     #[repr(C)]
>>>>>     pub union KAbiReserved<T, R> {
>>>>>         value: T,
>>>>>         _reserved: R,
>>>>>     }
>>>>
>>>> I like this approach even better, assuming any remaining issues with
>>>> ownership etc. can be sorted out. This would also look identical to
>>>> the C version in DWARF if you rename _reserved in the union to
>>>> __kabi_reserved. Of course, we can always change gendwarfksyms to
>>>> support a different scheme for Rust code if a better solution comes
>>>> along later.
>>
>> Yeah sure, that should also then work directly with this patch, right?
>>
>>>> Sami
>>>
>>> Agreement here - this seems like a good approach to representing
>>> reserved in Rust code. A few minor adjustments we discussed off-list
>>> which aren't required for gendwarfksyms to know about:
>>> 1. Types being added to reserved fields have to be `Copy`, e.g. they
>>> must be `!Drop`.
>>> 2. Types being added to reserved fields must be legal to be
>>> represented by all zeroes.
>>> 3. Reserved fields need to be initialized to zero before having their
>>> union set to the provided value when constructing them.
>>> 4. It may be helpful to have delegating trait implementations to avoid
>>> the couple places where autoderef won't handle the conversion.
>>>
>>> While I think this is the right solution, esp. since it can share a
>>> representation with C, I wanted to call out one minor shortfall - a
>>> reserved field can only be replaced by one type. We could still
>>> indicate a replacement by two fields the same as in C, by using a
>>> tuple which will look like an anonymous struct. The limitation will be
>>> that if two or more new fields were introduced, we'd need to edit the
>>> patches accessing them to do foo.x.y and foo.x.z for their accesses
>>> instead of simply foo.y and foo.z - the autoref trick only works for a
>>> single type.
>>
>> We will have to see how often multiple fields are added to a struct. If
>> they are infrequent and it's fine for those patches to then touch the
>> field accesses, then I think we can just stick with this approach.
>> If there are problems with that, we can also try the following:
>> all fields of kABI structs must be private and must only be accessed
>> through setters/getters. We can then modify the body the setters/getters
>> to handle the additional indirection.
> 
> That's just not going to work, sorry.  Remember, the goal here is to
> keep the code that comes from kernel.org identical to what you have in
> your "enterprise" kernel tree, with the exception of the few extra
> "padding" fields you have added to allow for changes in the future in
> the kernel.org versions.

Yeah, that's what I thought.

> Requiring all kernel.org changes that add a new field to a structure to
> only do so with a settter/getter is going to just not fly at all as they
> will not care one bit.
> 
> Or, we can just forget about "abi stability" for rust code entirely,
> which I am totally fine with.  It's something that managers seem to like
> for a "check box" but in reality, no one really needs it (hint, vendors
> rebuild their code anyway.)

The approach already works for a adding a single field and I got from
the discussions with Matthew and Sami that that is the most common case.
We will reach out to the Rust folks and see what we can do about the
multiple field case.

---
Cheers,
Benno

Powered by blists - more mailing lists