On Tue, 26 Aug 2025 15:46:29 +1000
Wilfred Mallawa <wilfred.opensou...@gmail.com> wrote:
> From: Wilfred Mallawa <wilfred.mall...@wdc.com>
>
> Adds the NVMe Admin Security Send/Receive command support with support
> for DMTFs SPDM. The transport binding for SPDM is defined in the
> DMTF DSP0286.
>
> Signed-off-by: Wilfred Mallawa <wilfred.mall...@wdc.com>
A few suggestions inline that might slightly improve this.
Looks to be in a good state to me.
Jonathan
> ---
> hw/nvme/ctrl.c | 202 ++++++++++++++++++++++++++++++++++++++++++-
> hw/nvme/nvme.h | 5 ++
> include/block/nvme.h | 15 ++++
> 3 files changed, 221 insertions(+), 1 deletion(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index f5ee6bf260..442144642d 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -282,6 +282,8 @@ static const uint32_t nvme_cse_acs_default[256] = {
> [NVME_ADM_CMD_FORMAT_NVM] = NVME_CMD_EFF_CSUPP | NVME_CMD_EFF_LBCC,
> [NVME_ADM_CMD_DIRECTIVE_RECV] = NVME_CMD_EFF_CSUPP,
> [NVME_ADM_CMD_DIRECTIVE_SEND] = NVME_CMD_EFF_CSUPP,
> + [NVME_ADM_CMD_SECURITY_SEND] = NVME_CMD_EFF_CSUPP,
> + [NVME_ADM_CMD_SECURITY_RECV] = NVME_CMD_EFF_CSUPP,
> };
>
> static const uint32_t nvme_cse_iocs_nvm_default[256] = {
> @@ -7282,6 +7284,199 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const
> NvmeRequest *req)
> return NVME_SUCCESS;
> }
>
> +static uint16_t nvme_sec_prot_spdm_send(NvmeCtrl *n, NvmeRequest *req)
> +{
> + StorageSpdmTransportHeader hdr = {0};
> + uint8_t *sec_buf;
> + uint32_t transfer_len = le32_to_cpu(req->cmd.cdw11);
> + uint32_t transport_transfer_len = transfer_len;
> + uint32_t dw10 = le32_to_cpu(req->cmd.cdw10);
> + uint32_t recvd;
> + uint16_t nvme_cmd_status;
> + uint16_t ret;
> + uint8_t secp = (dw10 >> 24) & 0xFF;
> + uint8_t spsp1 = (dw10 >> 16) & 0xFF;
> + uint8_t spsp0 = (dw10 >> 8) & 0xFF;
> + bool spdm_res;
> +
> + transport_transfer_len += sizeof(hdr);
> + if (transport_transfer_len > SPDM_SOCKET_MAX_MESSAGE_BUFFER_SIZE) {
> + return NVME_NO_COMPLETE | NVME_DNR;
> + }
> +
> + /* Generate the NVMe transport header */
> + hdr.security_protocol = secp;
> + hdr.security_protocol_specific = cpu_to_le16((spsp1 << 8) | spsp0);
> + hdr.length = cpu_to_le32(transfer_len);
Similar comments to below apply to this function.
> +
> + sec_buf = g_malloc0(transport_transfer_len);
> + if (!sec_buf) {
> + return NVME_NO_COMPLETE | NVME_DNR;
> + }
> +
> + /* Attach the transport header */
> + memcpy(sec_buf, &hdr, sizeof(hdr));
> + ret = nvme_h2c(n, sec_buf + sizeof(hdr), transfer_len, req);
> + if (ret) {
> + return NVME_NO_COMPLETE | NVME_DNR;
> + }
> +
> + spdm_res = spdm_socket_send(n->spdm_socket,
> SPDM_SOCKET_STORAGE_CMD_IF_SEND,
> + SPDM_SOCKET_TRANSPORT_TYPE_NVME, sec_buf,
> + transport_transfer_len);
> + if (!spdm_res) {
> + g_free(sec_buf);
> + return NVME_NO_COMPLETE | NVME_DNR;
> + }
> +
> + /* The responder shall ack with message status */
> + recvd = spdm_socket_receive(n->spdm_socket,
> SPDM_SOCKET_TRANSPORT_TYPE_NVME,
> + (uint8_t *)&nvme_cmd_status,
> + SPDM_SOCKET_MAX_MSG_STATUS_LEN);
> +
> + nvme_cmd_status = cpu_to_be16(nvme_cmd_status);
> +
> + if (recvd < SPDM_SOCKET_MAX_MSG_STATUS_LEN) {
> + g_free(sec_buf);
> + return NVME_NO_COMPLETE | NVME_DNR;
> + }
> +
> + g_free(sec_buf);
> + return nvme_cmd_status;
> +}
> +
> +/* From host to controller */
> +static uint16_t nvme_security_send(NvmeCtrl *n, NvmeRequest *req)
> +{
> + uint32_t dw10 = le32_to_cpu(req->cmd.cdw10);
> + uint8_t secp = (dw10 >> 24) & 0xff;
> +
> + switch (secp) {
> + case NVME_SEC_PROT_DMTF_SPDM:
> + return nvme_sec_prot_spdm_send(n, req);
> + default:
> + /* Unsupported Security Protocol Type */
> + return NVME_INVALID_FIELD | NVME_DNR;
> + }
> +
> + return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
> +static uint16_t nvme_sec_prot_spdm_receive(NvmeCtrl *n, NvmeRequest *req)
> +{
> + StorageSpdmTransportHeader hdr = {0};
> + uint8_t *rsp_spdm_buf;
> + uint32_t dw10 = le32_to_cpu(req->cmd.cdw10);
> + uint32_t alloc_len = le32_to_cpu(req->cmd.cdw11);
> + uint32_t recvd, spdm_res;
> + uint16_t nvme_cmd_status;
> + uint16_t ret;
Maybe combine with previous line.
> + uint8_t secp = (dw10 >> 24) & 0xFF;
> + uint8_t spsp1 = (dw10 >> 16) & 0xFF;
> + uint8_t spsp0 = (dw10 >> 8) & 0xFF;
Perhaps extract32()
> +
> + if (!alloc_len) {
> + return NVME_INVALID_FIELD | NVME_DNR;
> + }
> +
> + /* Generate the NVMe transport header */
Could do
hdr = (StorageSpdmTransportHeader) {
.security_protocol = secp,
.security_protocol_specific = cpu_to_le...
.length = cpu_to_le32(alloc_len),
};
and skip the {0} above. Up to you but I find this style a little more readable
as can see here that other fields are all zero'd rather than having to look
above.
> + hdr.security_protocol = secp;
> + hdr.security_protocol_specific = cpu_to_le16((spsp1 << 8) | spsp0);
> + hdr.length = cpu_to_le32(alloc_len);
> +
> + /* Forward if_recv to the SPDM Server with SPSP0 */
> + spdm_res = spdm_socket_send(n->spdm_socket,
> SPDM_SOCKET_STORAGE_CMD_IF_RECV,
> + SPDM_SOCKET_TRANSPORT_TYPE_NVME,
> + (uint8_t *)&hdr, sizeof(hdr));
> + if (!spdm_res) {
> + return NVME_NO_COMPLETE | NVME_DNR;
> + }
> +
> + /* The responder shall ack with message status */
> + recvd = spdm_socket_receive(n->spdm_socket,
> SPDM_SOCKET_TRANSPORT_TYPE_NVME,
> + (uint8_t *)&nvme_cmd_status,
> + SPDM_SOCKET_MAX_MSG_STATUS_LEN);
> +
> + nvme_cmd_status = cpu_to_be16(nvme_cmd_status);
> +
Single blank line enough.
> +
> + if (recvd < SPDM_SOCKET_MAX_MSG_STATUS_LEN) {
> + return NVME_NO_COMPLETE | NVME_DNR;
> + }
> +
> + /* An error here implies the prior if_recv from requester was spurious */
> + if (nvme_cmd_status != NVME_SUCCESS) {
> + return nvme_cmd_status;
> + }
> +
> + /* Clear to start receiving data from the server */
> + rsp_spdm_buf = g_malloc0(alloc_len);
maybe
g_autofree uint8_t *rsp_spdm_buf = g_malloc0(alloc_len);
and drop explicit frees later.
> + if (!rsp_spdm_buf) {
> + return NVME_NO_COMPLETE | NVME_DNR;
> + }
> +
> + recvd = spdm_socket_receive(n->spdm_socket,
> + SPDM_SOCKET_TRANSPORT_TYPE_NVME,
> + rsp_spdm_buf, alloc_len);
> + if (!recvd) {
> + g_free(rsp_spdm_buf);
> + return NVME_NO_COMPLETE | NVME_DNR;
> + }
> +
> + ret = nvme_c2h(n, rsp_spdm_buf, MIN(recvd, alloc_len), req);
> + g_free(rsp_spdm_buf);
> +
> + if (ret) {
> + return NVME_NO_COMPLETE | NVME_DNR;
> + }
> +
> + return NVME_SUCCESS;
> +}
> +
> +static uint16_t nvme_get_sec_prot_info(NvmeCtrl *n, NvmeRequest *req)
> +{
> + uint32_t alloc_len = le32_to_cpu(req->cmd.cdw11);
> + uint8_t resp[10] = {0};
Could do
uint8_t resp[10] = {
/* Support Security Protol List Length */
[6] = 0,
[7] = 2,
etc but not particularly important.
> +
> + if (alloc_len < 10) {
> + return NVME_INVALID_FIELD | NVME_DNR;
> + }
> +
> + /* Support Security Protol List Length */
> + resp[6] = 0; /* MSB */
> + resp[7] = 2; /* LSB */
> + /* Support Security Protocol List */
> + resp[8] = SFSC_SECURITY_PROT_INFO;
> + resp[9] = NVME_SEC_PROT_DMTF_SPDM;
> +
> + return nvme_c2h(n, resp, sizeof(resp), req);
> +}
> +
> +/* From controller to host */
> +static uint16_t nvme_security_receive(NvmeCtrl *n, NvmeRequest *req)
> +{
> + uint32_t dw10 = le32_to_cpu(req->cmd.cdw10);
> + uint16_t spsp = (dw10 >> 8) & 0xFFFF;
> + uint8_t secp = (dw10 >> 24) & 0xFF;
Perhaps use extract32().
> +
> + switch (secp) {
> + case SFSC_SECURITY_PROT_INFO:
> + switch (spsp) {
> + case 0:
> + /* Supported security protocol list */
> + return nvme_get_sec_prot_info(n, req);
> + case 1:
> + /* Certificate data */
Even though I guess this is temporary this probably should have
an explicit fall through marking.
> + default:
> + return NVME_INVALID_FIELD | NVME_DNR;
> + }
> + case NVME_SEC_PROT_DMTF_SPDM:
> + return nvme_sec_prot_spdm_receive(n, req);
> + default:
> + return NVME_INVALID_FIELD | NVME_DNR;
> + }
> +}
