Re: [PATCH v4 10/22] lib: rspdm: Initial commit of Rust SPDM

["Alexandre Courbot" <acourbot@nvidia.com>]

On Thu Oct 15, 2026 at 12:06 PM JST, Alistair Francis wrote:
<...>
>> > +/// The current SPDM session state for a device.
>> > +///
>> > +/// Concurrent access is serialised by wrapping the whole struct in a
>> > +/// `Mutex<SpdmState>` at the FFI boundary, so `spdm_authenticate()` callers
>> > +/// run one at a time and the locked `&mut SpdmState` is the only way to
>> > +/// reach the inner fields.
>> > +///
>> > +/// `dev`: Responder device.  Used for error reporting and passed to @transport.
>> > +/// `transport`: Transport function to perform one message exchange.
>> > +/// `transport_priv`: Transport private data.
>> > +/// `transport_sz`: Maximum message size the transport is capable of (in bytes).
>> > +///  Used as DataTransferSize in GET_CAPABILITIES exchange.
>> > +/// `validate`: Function to validate additional leaf certificate requirements.
>> > +///
>> > +/// `version`: Maximum common supported version of requester and responder.
>> > +///  Negotiated during GET_VERSION exchange.
>>
>> Field documentation should be their own doccomments before each
>> individual field (see [1] for an example and general good practices when
>> writing Rust doccomments).
>>
>> [1] https://doc.rust-lang.org/rust-by-example/meta/doc.html
>>
>> > +#[expect(dead_code)]
>> > +pub(crate) struct SpdmState {
>> > +    pub(crate) dev: *mut bindings::device,
>>
>> This makes me very nervous. I see this is passed to C callbacks. We need
>> to maintain the guarantee that the device still exists, so an
>> `ARef<Device>`, from which you obtain a pointer to pass to the C
>> callbacks, should be the minimum here. Does the device also need to be
>> bound? Looking at `drivers/pci/cma.c` it seems to be. Then this needs to
>> be a `&Device<Bound>`, with a lifetime to `SpdmState` so callers only
>> use it while the device is bound.
>
> Good point, I have converted this to use a `&Device<Bound>`

Another related point of feedback is that this code seems to be
designed to interact with C drivers exclusively - i.e. I don't see a
publicly exported Rust API.

That's probably what has guided the current design of storing pointers,
as that's what the C functions expect; and the C side is also
responsible for taking care of lifetime and bound device status, so in
that context using a `&Device<Bound>` will be of little help as it will
need to be forged by trusting that the calling C code knows what it is
doing.

I think it would be helpful on the contrary to design things around a
Rust API, on top of which the C FFI is built. That way you would have to
work with properly bound references, and have a proof that the model is
viable before exposing it to C. Also I guess Rust drivers will want to
use this at some point, so better design with that in mind from the
get-go.

Also related, what is the reason for having this code in `lib`? Why not
make this a regular kernel module?

>
>>
>> Just storing a C pointer and passing it whenever a function needs it
>> cancels the purpose of using Rust and make the safety requirements of
>> the callback you pass it to impossible to guarantee, as nothing enforces
>> that the device still exists or is in a valid state when they are
>> invoked.
>>
>> > +    pub(crate) transport: bindings::spdm_transport,
>> > +    pub(crate) transport_priv: *mut c_void,
>>
>> Here I also suspect we want to define some kind of ownership model for
>> the private data instead of hoping that the pointer is still valid when
>> we use it.
>
> Any ideas here? It's just a pointer that we pass back to C when
> calling the `transport_function()`, I'm not clear on what we could do
> to ensure it's still valid.

I suspect something close to what `fwctl` does? Basically `SpdmState`
would store a reference to a generic type, which outlives the SPDM
driver bound time. That's also why having the Rust API first is helpful,
as it will provide the lifetime that needs to be outlived.

Looking at the example in `cma.c`, the private data appears to be
`host.doe_mb`, which exists before `spdm_create` is called and is still
there after `spdm_destroy`, so that seems to be the correct model, i.e.
there should be no need to transfer full ownership of the private data
to the SPDM driver - a reference should be sufficient.

Actually thinking some more about it seems like the `fwctl` Rust
bindings could be a good model to follow. They are simple and adopt a
similar pattern of a sub-component of a larger driver that never
outlives its parent.

<...>
>>
>> > +impl<'a, 'c> Validate<'c, Untrusted<&'a [u8]>> for SpdmHeader {
>> > +    type Err = Error;
>> > +
>> > +    type Context = &'c SpdmState;
>> > +
>> > +    fn validate(unvalidated: &[u8], _context: &'c SpdmState) -> Result<Self, Self::Err> {
>> > +        Ok(SpdmHeader {
>> > +            version: *unvalidated.get(0).ok_or(EIO)?,
>> > +            code: *unvalidated.get(1).ok_or(EIO)?,
>> > +            param1: *unvalidated.get(2).ok_or(EIO)?,
>> > +            param2: *unvalidated.get(3).ok_or(EIO)?,
>> > +        })
>>
>> Here you will definitely want to leverage zerocopy's `FromBytes` trait.
>
> I was hoping not too actually.
>
> The SPDM responses are a bit complex. For example NegotiateAlgsRsp has
> multiple dynamically sized arrays, and the fields change based on the
> version.
>
> Using zerocopy means we need packed structs and even then I'm not
> clear on how we can handle the KVec<u32> or even the
> KVec<RespAlgStruct> nicley.
>
> The current approach has the obvious downside of copying the data, but
> as the data is pretty small in the scheme of things and not part of a
> hot path, I think the tradeoff of copying the data with much cleaner
> and easier to understand code is worth it.
>
> Maybe in the future zerocopy could be a performance improvement, but I
> think right now it's a large complexity overhead in this case.
>
> Note that obviously SpdmHeader is not complex, but I think they should
> all follow the same pattern.

For more complex structures sure, but in the case of `SpdmHeader`
zerocopy looks like it would work nicely? It also supports performing a
copy (see `read_from_bytes` and `read_from_prefix`).