[v1] Add SPDM over Storage transport support for NVMe

[RFC 2/4] spdm: add spdm storage transport virtual header

Posted by Wilfred Mallawa via 1 year, 1 month ago

This header contains the transport encoding for an SPDM message that
uses the SPDM over Storage transport as defined by the DMTF DSP0286.

Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
---
 include/system/spdm-socket.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/system/spdm-socket.h b/include/system/spdm-socket.h
index 2b7d03f82d..fc007e5b48 100644
--- a/include/system/spdm-socket.h
+++ b/include/system/spdm-socket.h
@@ -88,6 +88,18 @@ bool spdm_socket_send(const int socket, uint32_t socket_cmd,
  */
 void spdm_socket_close(const int socket, uint32_t transport_type);
 
+/*
+ * Defines the transport encoding for SPDM, this information shall be passed
+ * down to the SPDM server, when conforming to the SPDM over Storage standard
+ * as defined by DSP0286.
+ */
+typedef struct QEMU_PACKED {
+    uint8_t security_protocol;
+    uint16_t security_protocol_specific;
+    bool inc_512;
+    uint32_t length;
+} StorageSpdmTransportHeader;
+
 #define SPDM_SOCKET_COMMAND_NORMAL                0x0001
 #define SPDM_SOCKET_STORAGE_CMD_IF_SEND           0x0002
 #define SPDM_SOCKET_STORAGE_CMD_IF_RECV           0x0003
-- 
2.47.1

Re: [RFC 2/4] spdm: add spdm storage transport virtual header

Posted by Klaus Jensen 1 year, 1 month ago

On Jan  7 15:29, Wilfred Mallawa via wrote:
> This header contains the transport encoding for an SPDM message that
> uses the SPDM over Storage transport as defined by the DMTF DSP0286.
> 
> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> ---
>  include/system/spdm-socket.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/system/spdm-socket.h b/include/system/spdm-socket.h
> index 2b7d03f82d..fc007e5b48 100644
> --- a/include/system/spdm-socket.h
> +++ b/include/system/spdm-socket.h
> @@ -88,6 +88,18 @@ bool spdm_socket_send(const int socket, uint32_t socket_cmd,
>   */
>  void spdm_socket_close(const int socket, uint32_t transport_type);
>  
> +/*
> + * Defines the transport encoding for SPDM, this information shall be passed
> + * down to the SPDM server, when conforming to the SPDM over Storage standard
> + * as defined by DSP0286.
> + */
> +typedef struct QEMU_PACKED {
> +    uint8_t security_protocol;
> +    uint16_t security_protocol_specific;
> +    bool inc_512;
> +    uint32_t length;
> +} StorageSpdmTransportHeader;

Does it make sense to pack a bool? Is this defined by the SPDM server in
use? I can't find the definition of this header anywhere.

> +
>  #define SPDM_SOCKET_COMMAND_NORMAL                0x0001
>  #define SPDM_SOCKET_STORAGE_CMD_IF_SEND           0x0002
>  #define SPDM_SOCKET_STORAGE_CMD_IF_RECV           0x0003
> -- 
> 2.47.1
> 
>

Re: [RFC 2/4] spdm: add spdm storage transport virtual header

Posted by Wilfred Mallawa 1 year ago

On Fri, 2025-01-10 at 10:04 +0100, Klaus Jensen wrote:
> On Jan  7 15:29, Wilfred Mallawa via wrote:
> > This header contains the transport encoding for an SPDM message
> > that
> > uses the SPDM over Storage transport as defined by the DMTF
> > DSP0286.
> > 
> > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > ---
> >  include/system/spdm-socket.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/include/system/spdm-socket.h b/include/system/spdm-
> > socket.h
> > index 2b7d03f82d..fc007e5b48 100644
> > --- a/include/system/spdm-socket.h
> > +++ b/include/system/spdm-socket.h
> > @@ -88,6 +88,18 @@ bool spdm_socket_send(const int socket, uint32_t
> > socket_cmd,
> >   */
> >  void spdm_socket_close(const int socket, uint32_t transport_type);
> >  
> > +/*
> > + * Defines the transport encoding for SPDM, this information shall
> > be passed
> > + * down to the SPDM server, when conforming to the SPDM over
> > Storage standard
> > + * as defined by DSP0286.
> > + */
> > +typedef struct QEMU_PACKED {
> > +    uint8_t security_protocol;
> > +    uint16_t security_protocol_specific;
> > +    bool inc_512;
> > +    uint32_t length;
> > +} StorageSpdmTransportHeader;
> 
> Does it make sense to pack a bool? Is this defined by the SPDM server
> in
> use? I can't find the definition of this header anywhere.
> 
This is essentially a virtual header containing essential storage
transport data as per DSP0286. For example, this is defined in the
upstream effort for 
`libspmd` to add storage binding support [1] and in DSP0286 [2], this
is defined in section 5.1.1.

Current implementation of the SPDM server (i.e in `spdm-utils` only one
to have support for storage), will just pass this header to `libspdm`
to be decoded. Once decoded by `libspdm`, `spdm-utils`/server will
contextually check for validity of the message.

As for inc_512, it just need to be yes or no, is there a better way to
represent that here?

[1]
https://github.com/DMTF/libspdm/pull/2827/files#diff-30c523edca23983e0f16e067772ec18e711a40f53ac49c8dda24301450b724d0R44
[2]
https://www.dmtf.org/sites/default/files/standards/documents/DSP0286_1.0.0WIP90.pdf
> > +
> >  #define SPDM_SOCKET_COMMAND_NORMAL                0x0001
> >  #define SPDM_SOCKET_STORAGE_CMD_IF_SEND           0x0002
> >  #define SPDM_SOCKET_STORAGE_CMD_IF_RECV           0x0003
> > -- 
> > 2.47.1
> > 
> >

Re: [RFC 2/4] spdm: add spdm storage transport virtual header

Posted by Klaus Jensen 1 year ago

On Jan 15 02:16, Wilfred Mallawa wrote:
> On Fri, 2025-01-10 at 10:04 +0100, Klaus Jensen wrote:
> > On Jan  7 15:29, Wilfred Mallawa via wrote:
> > > This header contains the transport encoding for an SPDM message
> > > that
> > > uses the SPDM over Storage transport as defined by the DMTF
> > > DSP0286.
> > > 
> > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > > ---
> > >  include/system/spdm-socket.h | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/include/system/spdm-socket.h b/include/system/spdm-
> > > socket.h
> > > index 2b7d03f82d..fc007e5b48 100644
> > > --- a/include/system/spdm-socket.h
> > > +++ b/include/system/spdm-socket.h
> > > @@ -88,6 +88,18 @@ bool spdm_socket_send(const int socket, uint32_t
> > > socket_cmd,
> > >   */
> > >  void spdm_socket_close(const int socket, uint32_t transport_type);
> > >  
> > > +/*
> > > + * Defines the transport encoding for SPDM, this information shall
> > > be passed
> > > + * down to the SPDM server, when conforming to the SPDM over
> > > Storage standard
> > > + * as defined by DSP0286.
> > > + */
> > > +typedef struct QEMU_PACKED {
> > > +    uint8_t security_protocol;
> > > +    uint16_t security_protocol_specific;
> > > +    bool inc_512;
> > > +    uint32_t length;
> > > +} StorageSpdmTransportHeader;
> > 
> > Does it make sense to pack a bool? Is this defined by the SPDM server
> > in
> > use? I can't find the definition of this header anywhere.
> > 
> This is essentially a virtual header containing essential storage
> transport data as per DSP0286. For example, this is defined in the
> upstream effort for 
> `libspmd` to add storage binding support [1] and in DSP0286 [2], this
> is defined in section 5.1.1.
> 
> Current implementation of the SPDM server (i.e in `spdm-utils` only one
> to have support for storage), will just pass this header to `libspdm`
> to be decoded. Once decoded by `libspdm`, `spdm-utils`/server will
> contextually check for validity of the message.
> 

OK, understood.

> As for inc_512, it just need to be yes or no, is there a better way to
> represent that here?

It's a byte, right? Then I think using uint8_t is more clear here.

Re: [RFC 2/4] spdm: add spdm storage transport virtual header

Posted by Wilfred Mallawa 1 year ago

On Tue, 2025-01-28 at 09:03 +0100, Klaus Jensen wrote:
> On Jan 15 02:16, Wilfred Mallawa wrote:
> > On Fri, 2025-01-10 at 10:04 +0100, Klaus Jensen wrote:
> > > On Jan  7 15:29, Wilfred Mallawa via wrote:
> > > > This header contains the transport encoding for an SPDM message
> > > > that
> > > > uses the SPDM over Storage transport as defined by the DMTF
> > > > DSP0286.
> > > > 
> > > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > > > ---
> > > >  include/system/spdm-socket.h | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > > 
> > > > diff --git a/include/system/spdm-socket.h
> > > > b/include/system/spdm-
> > > > socket.h
> > > > index 2b7d03f82d..fc007e5b48 100644
> > > > --- a/include/system/spdm-socket.h
> > > > +++ b/include/system/spdm-socket.h
> > > > @@ -88,6 +88,18 @@ bool spdm_socket_send(const int socket,
> > > > uint32_t
> > > > socket_cmd,
> > > >   */
> > > >  void spdm_socket_close(const int socket, uint32_t
> > > > transport_type);
> > > >  
> > > > +/*
> > > > + * Defines the transport encoding for SPDM, this information
> > > > shall
> > > > be passed
> > > > + * down to the SPDM server, when conforming to the SPDM over
> > > > Storage standard
> > > > + * as defined by DSP0286.
> > > > + */
> > > > +typedef struct QEMU_PACKED {
> > > > +    uint8_t security_protocol;
> > > > +    uint16_t security_protocol_specific;
> > > > +    bool inc_512;
> > > > +    uint32_t length;
> > > > +} StorageSpdmTransportHeader;
> > > 
> > > Does it make sense to pack a bool? Is this defined by the SPDM
> > > server
> > > in
> > > use? I can't find the definition of this header anywhere.
> > > 
> > This is essentially a virtual header containing essential storage
> > transport data as per DSP0286. For example, this is defined in the
> > upstream effort for 
> > `libspmd` to add storage binding support [1] and in DSP0286 [2],
> > this
> > is defined in section 5.1.1.
> > 
> > Current implementation of the SPDM server (i.e in `spdm-utils` only
> > one
> > to have support for storage), will just pass this header to
> > `libspdm`
> > to be decoded. Once decoded by `libspdm`, `spdm-utils`/server will
> > contextually check for validity of the message.
> > 
> 
> OK, understood.
> 
> > As for inc_512, it just need to be yes or no, is there a better way
> > to
> > represent that here?
> 
> It's a byte, right? Then I think using uint8_t is more clear here.
As per,  SCSI Primary Commands - 6 (SPC-6) it's a single bit to
indicate weather it's set or not, hence why I thought a boolean might
make more sense here.