transport: storage: add support for SPDM over the storage binding (DSP0286) #2827

twilfredo · 2024-09-05T02:17:59Z

This series adds libspdm transport support for SPDM over Storage Binding Specification [1]. This allows for example NVMe or SCSI devices that conform to [1] to communicate with a host requester using the storage transport medium as defined in [1].

[1] https://www.dmtf.org/sites/default/files/standards/documents/DSP0286_1.0.0WIP90.pdf

twilfredo · 2024-09-05T02:18:09Z

@alistair23

steven-bellock · 2024-09-05T14:53:54Z

A couple of high level comments.

This pull request will not be able to be merged until DSP0286 has been published, but that looks to be soonish.
The WDC-authored files have a WDC copyright. I believe (not 100 % sure) that per the "DMTF Members’ Software Submission Policy" it should have the DMTF copyright. If you want to keep the WDC copyright we can talk to the DMTF open source folks to clarify things.

twilfredo · 2024-09-06T04:51:39Z

A couple of high level comments.

1. This pull request will not be able to be merged until DSP0286 has been published, but that looks to be soonish.

Yeah that sounds good, just wanted to get comments early on

2. The WDC-authored files have a WDC copyright. I believe (not 100 % sure) that per the "DMTF Members’ Software Submission Policy" it should have the DMTF copyright. If you want to keep the WDC copyright we can talk to the DMTF open source folks to clarify things.

I've updated the license header with an author tag, is that sufficient? Diff: https://github.com/DMTF/libspdm/compare/7a14888587717c8a16e7390388fd39629e7a7076..c63269b162af091a35ef282052980087a378c68f

steven-bellock · 2024-09-06T16:28:10Z

Presumably authorship is recorded in the commit log, but if you want to put it in the file itself:

/**
 *  Copyright Notice:
 *  Copyright 2024 DMTF. All rights reserved.
 *  License: BSD 3-Clause License. For full text see link: https://github.com/DMTF/libspdm/blob/main/LICENSE.md
 **/
 
 /**
  *  Author: Wilfred Mallawa <wilfred.mallawa@wdc.com>
  *          Alistair Francis <alistair.francis@wdc.com>
  **/

steven-bellock · 2024-09-08T21:28:32Z

include/industry_standard/storage.h

+typedef struct {
+    uint16_t data_length;
+    uint16_t storage_binding_version;
+    uint8_t max_connection_id : 2;


The codebase avoids bitfields due to ambiguities in the C specification. The current specification calls this Byte4 which should probably be changed to something more descriptive. See https://github.com/DMTF/SPDM-WG/issues/3621.

okay sounds good, do we want to wait on this until the spec is updated then?

Yes, we'll probably discuss this issue at the VF2F meeting today.

right. Using bitfield is not recommended.

I recommend you just using uint8_t max_connection_id, then use bitmask MACRO, such as SPDM_STORAGE_MAX_CONNECTION_ID_MASK 0x3 to get the value.

include/industry_standard/storage.h

steven-bellock · 2024-09-08T21:29:48Z

include/industry_standard/storage.h

+    uint16_t storage_binding_version;
+    uint8_t max_connection_id : 2;
+    uint8_t _reserved1[3];
+    uint8_t supported_operations[8];


This should be one byte in the specification if https://github.com/DMTF/SPDM-WG/issues/3609 gets approved.

steven-bellock · 2024-09-08T21:36:39Z

include/industry_standard/storage.h

+#define STORAGE_OPERATION_CODE_MESSAGE          0x05
+#define STORAGE_OPERATION_CODE_SECURED_MESSAGE  0x06
+
+#define STORAGE_MAX_SIZE_IN_BYTE 0x00100000


This is defined or inferred from the specification?

This is not from the spec, we used the DoE transport for layer as a reference for this.

libspdm/include/industry_standard/pcidoe.h

Line 40 in b327ead

#define PCI_DOE_MAX_SIZE_IN_BYTE 0x00100000

The PCIe specification explicitly defines those values. If they are not in DSP0286 or an underlying specification then they should be removed.

right. only SPDM storage binding spec defined data should be here.

Any other implementation definition should NOT be added here.

include/industry_standard/storage.h

library/spdm_transport_storage_lib/CMakeLists.txt

steven-bellock · 2024-09-08T21:54:36Z

library/spdm_transport_storage_lib/libspdm_storage.c

+        return LIBSPDM_STATUS_INVALID_MSG_FIELD;
+    }
+
+#if CMAKE_C_BYTE_ORDER == LITTLE_ENDIAN


SPDM message fields (and libspdm) are all little-endian. Is there a big-endian field coming from the underlying storage technology?

It was to be consistent with the byte ordering defined in the SCSI Primary Commands specification (SPC) where BE is used for the command blocks. The SPC is referred to by the NVMe spec for the Security In/Out implementation also.

For context in qemu, we will generate the header as such: https://github.com/twilfredo/qemu/blob/d40afa3ad43417d01cf7a6e758a434e3858d0200/hw/nvme/ctrl.c#L7124

So NVMe / SCSI use BE. What about ATA?

@bhenning10 the specification should probably have a statement on endianness.

The ATA Spec specifies the trusted send/recv commands to be a 28bit command block in LE. Since this is a virtual header (libspdm specific) we are parsing here, would it be best to use LE to stay consistent with SPDM message fields? What do we think?

@twilfredo we can discuss that in https://github.com/DMTF/SPDM-WG/issues/3625.

steven-bellock · 2024-09-09T15:14:37Z

library/spdm_transport_storage_lib/libspdm_storage.c

+#include "hal/library/debuglib.h"
+#include "hal/library/memlib.h"
+
+# define BSWAP32(x)  ((((x) & 0x000000ff) << 24) |      \


libspdm/include/internal/libspdm_common_lib.h

Line 1705 in b327ead

static inline uint64_t libspdm_le_to_be_64(uint64_t value)

already exists in a header. I would rename these to libspdm_byte_swap_* and place them there. libspdm_le_to_be_64 can be renamed to libspdm_byte_swap_64 in a subsequent pull request to make things consistent.

jyao1 · 2024-09-11T02:46:37Z

Usually, the implementation for WIP should NOT be posted to main, but a WIP specific branch
For example, StorageBinding_WIP, in this case.

The WIP branch can be merged into main, after the WIP spec becomes official standard.

twilfredo · 2024-09-11T03:29:40Z

Usually, the implementation for WIP should NOT be posted to main, but a WIP specific branch For example, StorageBinding_WIP, in this case.

The WIP branch can be merged into main, after the WIP spec becomes official standard.

Okay noted, are you happy to keep this up for now? since review is in progress.

jyao1 · 2024-09-11T03:51:34Z

Okay noted, are you happy to keep this up for now? since review is in progress.

That is fine. We can keep reviewing and address all feedback in this PR.

Whenever you think it is done and ready for merge, you can switch to branch merge request.

include/industry_standard/storage.h

jyao1 · 2024-09-11T04:02:32Z

library/spdm_transport_storage_lib/libspdm_storage.c

+ *          Alistair Francis <alistair.francis@wdc.com>
+ **/
+
+#include "library/spdm_transport_storage_lib.h"


Do you have any unit test associated to prove the correctness of this function?

I haven't written any explicit test cases for all the functions in this files. We have tested the logic by testing with QEMU + spdm-utils + linux (Kernel SPDM) though. I'm waiting for the spec to become ratified so we can add some of the changes discussed above.

I can add some tests if they are required? Are there exists tests for other transports?

Usually, we will need two level of test:

Unit test in libspdm project.

System test in spdm-emu project.

In history, both tests can catch some error.

Please consider to add both.

As defined by DSP0826, add support for SPDM storage transport. The transport layer uses a virtual storage header that encapsulates SPDM requests to allow the caller to generate the required parameters for a storage SPDM request. SPDM responses are not transport encoded, with this header, instead just the message is returned. Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

This allows transports such as Storage (DSP0286) to determine if the next response is protected via secured messages. Unlike other transport layers, storage does not encode the message type in a response header. As such, the requester must track the expected type. The issue [1] discusses this implementation requirement in further detail with regards to DSP0286. [1] DMTF/SPDM-WG#3520 Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

jyao1 · 2024-11-01T02:22:44Z

@twilfredo , do you want to keep it in PR? or do you want to push to a branch, and keep updating the branch?

I am OK either way. But I just feel a branch might be more helpful.

twilfredo · 2024-11-01T06:11:21Z

@twilfredo , do you want to keep it in PR? or do you want to push to a branch, and keep updating the branch?

I am OK either way. But I just feel a branch might be more helpful.

I'll stick to the PR is that's okay with everyone. I'm just waiting for the spec to be ratified so I can address all the changes in one go.

twilfredo force-pushed the wilfred/add-trans-storage-05.9 branch 7 times, most recently from 8ab52c6 to 7a14888 Compare September 5, 2024 04:21

twilfredo force-pushed the wilfred/add-trans-storage-05.9 branch from 7a14888 to c63269b Compare September 6, 2024 04:50

steven-bellock requested changes Sep 8, 2024

View reviewed changes

twilfredo force-pushed the wilfred/add-trans-storage-05.9 branch 3 times, most recently from e50ff9f to 8fc10b2 Compare September 9, 2024 01:34

steven-bellock reviewed Sep 9, 2024

View reviewed changes

jyao1 reviewed Sep 11, 2024

View reviewed changes

include/industry_standard/storage.h Outdated Show resolved Hide resolved

jyao1 reviewed Sep 11, 2024

View reviewed changes

include/industry_standard/storage.h Outdated Show resolved Hide resolved

jyao1 reviewed Sep 11, 2024

View reviewed changes

include/industry_standard/storage.h Outdated Show resolved Hide resolved

jyao1 reviewed Sep 11, 2024

View reviewed changes

include/industry_standard/storage.h Outdated Show resolved Hide resolved

jyao1 reviewed Sep 11, 2024

View reviewed changes

twilfredo force-pushed the wilfred/add-trans-storage-05.9 branch 4 times, most recently from 5a5ce6f to 7d5dfea Compare October 2, 2024 02:32

twilfredo force-pushed the wilfred/add-trans-storage-05.9 branch from 7d5dfea to 5569c6c Compare October 2, 2024 02:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transport: storage: add support for SPDM over the storage binding (DSP0286) #2827

transport: storage: add support for SPDM over the storage binding (DSP0286) #2827

twilfredo commented Sep 5, 2024

twilfredo commented Sep 5, 2024

steven-bellock commented Sep 5, 2024

twilfredo commented Sep 6, 2024 •

edited

Loading

steven-bellock commented Sep 6, 2024

steven-bellock Sep 8, 2024

twilfredo Sep 8, 2024

steven-bellock Sep 9, 2024

jyao1 Sep 11, 2024

steven-bellock Sep 8, 2024

steven-bellock Sep 8, 2024

twilfredo Sep 9, 2024

steven-bellock Sep 9, 2024 •

edited

Loading

jyao1 Sep 11, 2024

steven-bellock Sep 8, 2024

twilfredo Sep 9, 2024

twilfredo Sep 9, 2024

steven-bellock Sep 9, 2024

twilfredo Sep 10, 2024 •

edited

Loading

steven-bellock Sep 10, 2024

steven-bellock Sep 9, 2024

jyao1 commented Sep 11, 2024 •

edited

Loading

twilfredo commented Sep 11, 2024 •

edited

Loading

jyao1 commented Sep 11, 2024

jyao1 Sep 11, 2024

twilfredo Oct 2, 2024

jyao1 Oct 12, 2024

jyao1 commented Nov 1, 2024

twilfredo commented Nov 1, 2024

transport: storage: add support for SPDM over the storage binding (DSP0286) #2827

Are you sure you want to change the base?

transport: storage: add support for SPDM over the storage binding (DSP0286) #2827

Conversation

twilfredo commented Sep 5, 2024

twilfredo commented Sep 5, 2024

steven-bellock commented Sep 5, 2024

twilfredo commented Sep 6, 2024 • edited Loading

steven-bellock commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-bellock Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twilfredo Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyao1 commented Sep 11, 2024 • edited Loading

twilfredo commented Sep 11, 2024 • edited Loading

jyao1 commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyao1 commented Nov 1, 2024

twilfredo commented Nov 1, 2024

twilfredo commented Sep 6, 2024 •

edited

Loading

steven-bellock Sep 9, 2024 •

edited

Loading

twilfredo Sep 10, 2024 •

edited

Loading

jyao1 commented Sep 11, 2024 •

edited

Loading

twilfredo commented Sep 11, 2024 •

edited

Loading