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

[Alistair Francis]

On Thu, Oct 15, 2026 at 5:30 PM Alexandre Courbot <acourbot@xxxxxxxxxx> wrote:
>
> 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.

There isn't a public Rust API, but `SpdmState` and its functions are
more or less the 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.

That was kind of the goal with `SpdmState`. The issue is all of the
drivers calling this (PCIe, NVMe, SATA and SCSI) are C drivers and
won't be Rust for the foreseeable future. So there aren't any Rust
users.

I have improved things a lot with v5, let me know if you still want a
Rust API. It just feels a bit silly to add one that will probably
never be used.

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

That's just where the C implementation was as well, so I followed
that. It felt right, as it's a helper library. But I'm happy to move
it if that's prefered?


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

Ok, I think I have done something along the lines of this in v5

>
> <...>
> >>
> >> > +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`).

You convinced me :)

Fixed in v5

Alistair