[PATCH 1/3] hw/pci: Add all Data Object Types

[PATCH 1/3] hw/pci: Add all Data Object Types

[Alistair Francis]

Add all of the defined protocols/features from the PCIe-SIG
"Table 6-32 PCI-SIG defined Data Object Types (Vendor ID = 0001h)"
table.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 include/hw/pci/pcie_doe.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/hw/pci/pcie_doe.h b/include/hw/pci/pcie_doe.h
index 87dc17dcef..15d94661f9 100644
--- a/include/hw/pci/pcie_doe.h
+++ b/include/hw/pci/pcie_doe.h
@@ -46,6 +46,8 @@ REG32(PCI_DOE_CAP_STATUS, 0)
 
 /* PCI-SIG defined Data Object Types - r6.0 Table 6-32 */
 #define PCI_SIG_DOE_DISCOVERY       0x00
+#define PCI_SIG_DOE_CMA             0x01
+#define PCI_SIG_DOE_SECURED_CMA     0x02
 
 #define PCI_DOE_DW_SIZE_MAX         (1 << 18)
 #define PCI_DOE_PROTOCOL_NUM_MAX    256
-- 
2.41.0

[PATCH 2/3] backends: Initial support for SPDM socket support

[Alistair Francis]

From: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>

SPDM enables authentication, attestation and key exchange to assist in
providing infrastructure security enablement. It's a standard published
by the DMTF [1].

SPDM currently supports PCIe DOE and MCTP transports, but it can be
extended to support others in the future. This patch adds
support to QEMU to connect to an external SPDM instance.

SPDM support can be added to any QEMU device by exposing a
TCP socket to a SPDM server. The server can then implement the SPDM
decoding/encoding support, generally using libspdm [2].

This is similar to how the current TPM implementation works and means
that the heavy lifting of setting up certificate chains, capabilities,
measurements and complex crypto can be done outside QEMU by a well
supported and tested library.

1: https://www.dmtf.org/standards/SPDM
2: https://github.com/DMTF/libspdm

Signed-off-by: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
Signed-off-by: Chris Browy <cbrowy@avery-design.com>
Co-developed-by: Jonathan Cameron <Jonathan.cameron@huawei.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
[ Changes by AF:
 - Convert to be more QEMU-ified
 - Move to backends as it isn't PCIe specific
]
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
---
 include/sysemu/spdm-socket.h |  44 +++++++
 backends/spdm-socket.c       | 215 +++++++++++++++++++++++++++++++++++
 backends/Kconfig             |   4 +
 backends/meson.build         |   2 +
 4 files changed, 265 insertions(+)
 create mode 100644 include/sysemu/spdm-socket.h
 create mode 100644 backends/spdm-socket.c

diff --git a/include/sysemu/spdm-socket.h b/include/sysemu/spdm-socket.h
new file mode 100644
index 0000000000..24e6fccb83
--- /dev/null
+++ b/include/sysemu/spdm-socket.h
@@ -0,0 +1,44 @@
+/*
+ * QEMU SPDM socket support
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef SPDM_REQUESTER_H
+#define SPDM_REQUESTER_H
+
+int spdm_socket_connect(uint16_t port, Error **errp);
+uint32_t spdm_socket_rsp(const int socket, uint32_t transport_type,
+                         void *req, uint32_t req_len,
+                         void *rsp, uint32_t rsp_len);
+void spdm_socket_close(const int socket, uint32_t transport_type);
+
+#define SPDM_SOCKET_COMMAND_NORMAL                0x0001
+#define SPDM_SOCKET_COMMAND_OOB_ENCAP_KEY_UPDATE  0x8001
+#define SPDM_SOCKET_COMMAND_CONTINUE              0xFFFD
+#define SPDM_SOCKET_COMMAND_SHUTDOWN              0xFFFE
+#define SPDM_SOCKET_COMMAND_UNKOWN                0xFFFF
+#define SPDM_SOCKET_COMMAND_TEST                  0xDEAD
+
+#define SPDM_SOCKET_TRANSPORT_TYPE_MCTP           0x01
+#define SPDM_SOCKET_TRANSPORT_TYPE_PCI_DOE        0x02
+
+#define SPDM_SOCKET_MAX_MESSAGE_BUFFER_SIZE       0x1200
+
+#endif
diff --git a/backends/spdm-socket.c b/backends/spdm-socket.c
new file mode 100644
index 0000000000..2f31ba80ba
--- /dev/null
+++ b/backends/spdm-socket.c
@@ -0,0 +1,215 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * QEMU SPDM socket support
+ *
+ * This is based on:
+ * https://github.com/DMTF/spdm-emu/blob/07c0a838bcc1c6207c656ac75885c0603e344b6f/spdm_emu/spdm_emu_common/command.c
+ * but has been re-written to match QEMU style
+ *
+ * Copyright (c) 2021, DMTF. All rights reserved.
+ * Copyright (c) 2023. Western Digital Corporation or its affiliates.
+ */
+
+#include "qemu/osdep.h"
+#include "sysemu/spdm-socket.h"
+#include "qapi/error.h"
+
+static bool read_bytes(const int socket, uint8_t *buffer,
+                       size_t number_of_bytes)
+{
+    ssize_t number_received = 0;
+    ssize_t result;
+
+    while (number_received < number_of_bytes) {
+        result = recv(socket, buffer + number_received,
+                      number_of_bytes - number_received, 0);
+        if (result <= 0) {
+            return false;
+        }
+        number_received += result;
+    }
+    return true;
+}
+
+static bool read_data32(const int socket, uint32_t *data)
+{
+    bool result;
+
+    result = read_bytes(socket, (uint8_t *)data, sizeof(uint32_t));
+    if (!result) {
+        return result;
+    }
+    *data = ntohl(*data);
+    return true;
+}
+
+static bool read_multiple_bytes(const int socket, uint8_t *buffer,
+                                uint32_t *bytes_received,
+                                uint32_t max_buffer_length)
+{
+    uint32_t length;
+    bool result;
+
+    result = read_data32(socket, &length);
+    if (!result) {
+        return result;
+    }
+
+    if (length > max_buffer_length) {
+        return false;
+    }
+
+    if (bytes_received) {
+        *bytes_received = length;
+    }
+
+    if (length == 0) {
+        return true;
+    }
+
+    return read_bytes(socket, buffer, length);
+}
+
+static bool receive_platform_data(const int socket,
+                                  uint32_t transport_type,
+                                  uint32_t *command,
+                                  uint8_t *receive_buffer,
+                                  uint32_t *bytes_to_receive)
+{
+    bool result;
+    uint32_t response;
+    uint32_t bytes_received;
+
+    result = read_data32(socket, &response);
+    if (!result) {
+        return result;
+    }
+    *command = response;
+
+    result = read_data32(socket, &transport_type);
+    if (!result) {
+        return result;
+    }
+
+    bytes_received = 0;
+    result = read_multiple_bytes(socket, receive_buffer, &bytes_received,
+                                 *bytes_to_receive);
+    if (!result) {
+        return result;
+    }
+    *bytes_to_receive = bytes_received;
+
+    return result;
+}
+
+static bool write_bytes(const int socket, const uint8_t *buffer,
+                        uint32_t number_of_bytes)
+{
+    ssize_t number_sent = 0;
+    ssize_t result;
+
+    while (number_sent < number_of_bytes) {
+        result = send(socket, buffer + number_sent,
+                      number_of_bytes - number_sent, 0);
+        if (result == -1) {
+            return false;
+        }
+        number_sent += result;
+    }
+    return true;
+}
+
+static bool write_data32(const int socket, uint32_t data)
+{
+    data = htonl(data);
+    return write_bytes(socket, (uint8_t *)&data, sizeof(uint32_t));
+}
+
+static bool write_multiple_bytes(const int socket, const uint8_t *buffer,
+                                 uint32_t bytes_to_send)
+{
+    bool result;
+
+    result = write_data32(socket, bytes_to_send);
+    if (!result) {
+        return result;
+    }
+
+    return write_bytes(socket, buffer, bytes_to_send);
+}
+
+static bool send_platform_data(const int socket,
+                               uint32_t transport_type, uint32_t command,
+                               const uint8_t *send_buffer, size_t bytes_to_send)
+{
+    bool result;
+
+    result = write_data32(socket, command);
+    if (!result) {
+        return result;
+    }
+
+    result = write_data32(socket, transport_type);
+    if (!result) {
+        return result;
+    }
+
+    return write_multiple_bytes(socket, send_buffer, bytes_to_send);
+}
+
+int spdm_socket_connect(uint16_t port, Error **errp)
+{
+    int client_socket;
+    struct sockaddr_in server_addr;
+
+    client_socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
+    if (client_socket < 0) {
+        error_setg(errp, "cannot create socket: %s", strerror(errno));
+        return -1;
+    }
+
+    memset((char *)&server_addr, 0, sizeof(server_addr));
+    server_addr.sin_family = AF_INET;
+    server_addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
+    server_addr.sin_port = htons(port);
+
+
+    if (connect(client_socket, (struct sockaddr *)&server_addr, sizeof(server_addr)) < 0) {
+        error_setg(errp, "cannot connect: %s", strerror(errno));
+        close(client_socket);
+        return -1;
+    }
+
+    return client_socket;
+}
+
+uint32_t spdm_socket_rsp(const int socket, uint32_t transport_type,
+                         void *req, uint32_t req_len,
+                         void *rsp, uint32_t rsp_len)
+{
+    uint32_t command;
+    bool result;
+
+    result = send_platform_data(socket, transport_type,
+                                SPDM_SOCKET_COMMAND_NORMAL,
+                                req, req_len);
+    if (!result) {
+        return 0;
+    }
+
+    result = receive_platform_data(socket, transport_type, &command,
+                                   (uint8_t *)rsp, &rsp_len);
+    if (!result) {
+        return 0;
+    }
+
+    assert(command != 0);
+
+    return rsp_len;
+}
+
+void spdm_socket_close(const int socket, uint32_t transport_type)
+{
+    send_platform_data(socket, transport_type,
+                       SPDM_SOCKET_COMMAND_SHUTDOWN, NULL, 0);
+}
diff --git a/backends/Kconfig b/backends/Kconfig
index f35abc1609..648e58a9b3 100644
--- a/backends/Kconfig
+++ b/backends/Kconfig
@@ -1 +1,5 @@
 source tpm/Kconfig
+
+config SPDM_SOCKET
+    bool
+    default y
diff --git a/backends/meson.build b/backends/meson.build
index 914c7c4afb..77d48ffdaa 100644
--- a/backends/meson.build
+++ b/backends/meson.build
@@ -26,4 +26,6 @@ endif
 system_ss.add(when: gio, if_true: files('dbus-vmstate.c'))
 system_ss.add(when: 'CONFIG_SGX', if_true: files('hostmem-epc.c'))
 
+system_ss.add(when: 'CONFIG_SPDM_SOCKET', if_true: files('spdm-socket.c'))
+
 subdir('tpm')
-- 
2.41.0

Re: [PATCH 2/3] backends: Initial support for SPDM socket support

[Jonathan Cameron via]

On Fri, 15 Sep 2023 21:27:22 +1000
Alistair Francis <alistair23@gmail.com> wrote:

> From: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>

Great to see you taking this forwards!


> 
> SPDM enables authentication, attestation and key exchange to assist in
> providing infrastructure security enablement. It's a standard published
> by the DMTF [1].
> 
> SPDM currently supports PCIe DOE and MCTP transports, but it can be
> extended to support others in the future. This patch adds
> support to QEMU to connect to an external SPDM instance.

It supports way more that that these days.  I'd just say 'multiple'
transports.

> 
> SPDM support can be added to any QEMU device by exposing a
> TCP socket to a SPDM server. The server can then implement the SPDM
> decoding/encoding support, generally using libspdm [2].
> 
> This is similar to how the current TPM implementation works and means
> that the heavy lifting of setting up certificate chains, capabilities,
> measurements and complex crypto can be done outside QEMU by a well
> supported and tested library.

Is this sufficient for usecases beyond initial attestation flows?
How does measurement work for example?  We need settings from the
emulated device to squirt into the SPDM agent so that it can be
encrypted and signed etc.

Measurement reports often need to include the status of various config
space registers + any device specific additional stuff - not sure
what is defined for NVME but I suspect the list will grow, particularly
when tdisp is included.  There are some things called out in the PCIe
state as must haves, like any debug features must be reported.
Also we need a way to mess with firmware revisions reported
as those are likely to be checked.

I'm not sure that model will work with the spdm-emu approach.

Anyhow, I think we need to have gotten a little further figuring that
out before we merge a solution.  I've been carrying this on the CXL
staging tree for a long time because I couldn't figure out a good solution
to the amount of information that needs to go between them.

For those not familiar with the fun of libSPDM it is a pain to work with
which is why Huai-Cheng instead connected with the demo app.

Any more luck getting a reliable build to work?

> 
> 1: https://www.dmtf.org/standards/SPDM
> 2: https://github.com/DMTF/libspdm
> 
> Signed-off-by: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> Signed-off-by: Chris Browy <cbrowy@avery-design.com>
> Co-developed-by: Jonathan Cameron <Jonathan.cameron@huawei.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> [ Changes by AF:
>  - Convert to be more QEMU-ified
>  - Move to backends as it isn't PCIe specific
> ]
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Alistair, you sent this so I think your sign off should be last
+ some indication of Wilfred's involvement would be good?
Probably another Co-developed-by



> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> ---

I've looked at this code too much in the past to give much
real review.  Still a few comments inline.
I'm very keen to get a solution to this upstream, though I think
we do need to discuss a few general points (no cover letter so I'll
do it here).


...

> diff --git a/backends/spdm-socket.c b/backends/spdm-socket.c
> new file mode 100644
> index 0000000000..2f31ba80ba
> --- /dev/null
> +++ b/backends/spdm-socket.c
> @@ -0,0 +1,215 @@


> +
> +int spdm_socket_connect(uint16_t port, Error **errp)
> +{
> +    int client_socket;
> +    struct sockaddr_in server_addr;
> +
> +    client_socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> +    if (client_socket < 0) {
> +        error_setg(errp, "cannot create socket: %s", strerror(errno));
> +        return -1;
> +    }
> +
> +    memset((char *)&server_addr, 0, sizeof(server_addr));
> +    server_addr.sin_family = AF_INET;
> +    server_addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> +    server_addr.sin_port = htons(port);
> +
> +
> +    if (connect(client_socket, (struct sockaddr *)&server_addr, sizeof(server_addr)) < 0) {
Wrap the line.

> +        error_setg(errp, "cannot connect: %s", strerror(errno));
> +        close(client_socket);
> +        return -1;
> +    }
> +
> +    return client_socket;
> +}

Re: [PATCH 2/3] backends: Initial support for SPDM socket support

[Alistair Francis]

On Sat, Sep 16, 2023 at 1:19 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Fri, 15 Sep 2023 21:27:22 +1000
> Alistair Francis <alistair23@gmail.com> wrote:
>
> > From: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
>
> Great to see you taking this forwards!
>
>
> >
> > SPDM enables authentication, attestation and key exchange to assist in
> > providing infrastructure security enablement. It's a standard published
> > by the DMTF [1].
> >
> > SPDM currently supports PCIe DOE and MCTP transports, but it can be
> > extended to support others in the future. This patch adds
> > support to QEMU to connect to an external SPDM instance.
>
> It supports way more that that these days.  I'd just say 'multiple'
> transports.
>
> >
> > SPDM support can be added to any QEMU device by exposing a
> > TCP socket to a SPDM server. The server can then implement the SPDM
> > decoding/encoding support, generally using libspdm [2].
> >
> > This is similar to how the current TPM implementation works and means
> > that the heavy lifting of setting up certificate chains, capabilities,
> > measurements and complex crypto can be done outside QEMU by a well
> > supported and tested library.
>
> Is this sufficient for usecases beyond initial attestation flows?

I believe so.

The SPDM responder would be in charge of doing all of this. For the
rest of the discussion the responder is the software on the other end
of the QEMU socket.

> How does measurement work for example?  We need settings from the

In a basic case the responder can generate measurement data. For
example the responder can return digests of the firmware. Now there
won't actually be "firmware", but the responder can still return
measurement data.

if you are trying to test an existing product, you could fake it and
return the same values as a real device. Otherwise you could return
example data.

> emulated device to squirt into the SPDM agent so that it can be
> encrypted and signed etc.
>
> Measurement reports often need to include the status of various config
> space registers + any device specific additional stuff - not sure
> what is defined for NVME but I suspect the list will grow, particularly
> when tdisp is included.  There are some things called out in the PCIe
> state as must haves, like any debug features must be reported.

So this is probably the hard part. How can the responder measurements
change based on configuration values set by the host.

Just for completeness, the idea would be the host would set some state
in the NVMe controller for example. Then we would expect the
measurement values to change.

That's trickier, but we could extend the socket to communicate state
like that. So when a measurement state changes in the QEMU model, we
relay that to the responder. Which then changes the measurements

> Also we need a way to mess with firmware revisions reported
> as those are likely to be checked.

That seems like something you can do via command line arguments or
configuration settings to the responder. This is separate to QEMU

>
> I'm not sure that model will work with the spdm-emu approach.

I don't think there is anything specifically in the socket approach
that limits this from working. It comes down to passing information
from the QEMU emulated device to the SPDM responder. That needs to be
done either way. It probably is simpler to do if libspdm is included
as part of QEMU, but that brings along other complexities.

As you pointed out in my original RFC, the complexity of configuration
a responder via the QEMU command line will be very difficult. I think
it's simpler to keep the responder outside and QEMU and just pass any
relevant data to the responder as required.

>
> Anyhow, I think we need to have gotten a little further figuring that
> out before we merge a solution.  I've been carrying this on the CXL
> staging tree for a long time because I couldn't figure out a good solution
> to the amount of information that needs to go between them.
>
> For those not familiar with the fun of libSPDM it is a pain to work with
> which is why Huai-Cheng instead connected with the demo app.
>
> Any more luck getting a reliable build to work?

Yes!

libspdm is now packaged in buildroot:
https://github.com/buildroot/buildroot/blob/master/package/libspdm/libspdm.mk

You can now build libspdm with openSSL provided by your distro as you
don't need to access any private openSSL APIs any more.

The hope is we can continue to march towards including libspdm as a
standard library in distros, which should make the entire process
easier.

>
> >
> > 1: https://www.dmtf.org/standards/SPDM
> > 2: https://github.com/DMTF/libspdm
> >
> > Signed-off-by: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > Signed-off-by: Chris Browy <cbrowy@avery-design.com>
> > Co-developed-by: Jonathan Cameron <Jonathan.cameron@huawei.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > [ Changes by AF:
> >  - Convert to be more QEMU-ified
> >  - Move to backends as it isn't PCIe specific
> > ]
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Alistair, you sent this so I think your sign off should be last
> + some indication of Wilfred's involvement would be good?
> Probably another Co-developed-by
>
>
>
> > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > ---
>
> I've looked at this code too much in the past to give much
> real review.  Still a few comments inline.
> I'm very keen to get a solution to this upstream, though I think
> we do need to discuss a few general points (no cover letter so I'll
> do it here).

Yeah, I should have included a cover letter. V2 will

Alistair

>
>
> ...
>
> > diff --git a/backends/spdm-socket.c b/backends/spdm-socket.c
> > new file mode 100644
> > index 0000000000..2f31ba80ba
> > --- /dev/null
> > +++ b/backends/spdm-socket.c
> > @@ -0,0 +1,215 @@
>
>
> > +
> > +int spdm_socket_connect(uint16_t port, Error **errp)
> > +{
> > +    int client_socket;
> > +    struct sockaddr_in server_addr;
> > +
> > +    client_socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> > +    if (client_socket < 0) {
> > +        error_setg(errp, "cannot create socket: %s", strerror(errno));
> > +        return -1;
> > +    }
> > +
> > +    memset((char *)&server_addr, 0, sizeof(server_addr));
> > +    server_addr.sin_family = AF_INET;
> > +    server_addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> > +    server_addr.sin_port = htons(port);
> > +
> > +
> > +    if (connect(client_socket, (struct sockaddr *)&server_addr, sizeof(server_addr)) < 0) {
> Wrap the line.
>
> > +        error_setg(errp, "cannot connect: %s", strerror(errno));
> > +        close(client_socket);
> > +        return -1;
> > +    }
> > +
> > +    return client_socket;
> > +}
>
>

Re: [PATCH 2/3] backends: Initial support for SPDM socket support

[Jonathan Cameron via]

On Mon, 18 Sep 2023 13:16:01 +1000
Alistair Francis <alistair23@gmail.com> wrote:

> On Sat, Sep 16, 2023 at 1:19 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Fri, 15 Sep 2023 21:27:22 +1000
> > Alistair Francis <alistair23@gmail.com> wrote:
> >  
> > > From: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>  
> >
> > Great to see you taking this forwards!
> >
> >  
> > >
> > > SPDM enables authentication, attestation and key exchange to assist in
> > > providing infrastructure security enablement. It's a standard published
> > > by the DMTF [1].
> > >
> > > SPDM currently supports PCIe DOE and MCTP transports, but it can be
> > > extended to support others in the future. This patch adds
> > > support to QEMU to connect to an external SPDM instance.  
> >
> > It supports way more that that these days.  I'd just say 'multiple'
> > transports.
> >  
> > >
> > > SPDM support can be added to any QEMU device by exposing a
> > > TCP socket to a SPDM server. The server can then implement the SPDM
> > > decoding/encoding support, generally using libspdm [2].
> > >
> > > This is similar to how the current TPM implementation works and means
> > > that the heavy lifting of setting up certificate chains, capabilities,
> > > measurements and complex crypto can be done outside QEMU by a well
> > > supported and tested library.  
> >
> > Is this sufficient for usecases beyond initial attestation flows?  
> 
> I believe so.
> 
> The SPDM responder would be in charge of doing all of this. For the
> rest of the discussion the responder is the software on the other end
> of the QEMU socket.
> 
> > How does measurement work for example?  We need settings from the  
> 
> In a basic case the responder can generate measurement data. For
> example the responder can return digests of the firmware. Now there
> won't actually be "firmware", but the responder can still return
> measurement data.
> 
> if you are trying to test an existing product, you could fake it and
> return the same values as a real device. Otherwise you could return
> example data.
> 
> > emulated device to squirt into the SPDM agent so that it can be
> > encrypted and signed etc.
> >
> > Measurement reports often need to include the status of various config
> > space registers + any device specific additional stuff - not sure
> > what is defined for NVME but I suspect the list will grow, particularly
> > when tdisp is included.  There are some things called out in the PCIe
> > state as must haves, like any debug features must be reported.  
> 
> So this is probably the hard part. How can the responder measurements
> change based on configuration values set by the host.
> 
> Just for completeness, the idea would be the host would set some state
> in the NVMe controller for example. Then we would expect the
> measurement values to change.
> 
> That's trickier, but we could extend the socket to communicate state
> like that. So when a measurement state changes in the QEMU model, we
> relay that to the responder. Which then changes the measurements
> 
> > Also we need a way to mess with firmware revisions reported
> > as those are likely to be checked.  
> 
> That seems like something you can do via command line arguments or
> configuration settings to the responder. This is separate to QEMU
I'm not convinced it is because QEMU is emulating the firmware behavior
and that may well change with version.  Still it's just more metadata
to push out from QEMU to the SPDM instance.

My gut feeling is you'd just add an interface for the whole measurement
record coming from QEMU.  No need to be clever with sending only small
subsets of info and building the measurement.

> 
> >
> > I'm not sure that model will work with the spdm-emu approach.  
> 
> I don't think there is anything specifically in the socket approach
> that limits this from working. It comes down to passing information
> from the QEMU emulated device to the SPDM responder. That needs to be
> done either way. It probably is simpler to do if libspdm is included
> as part of QEMU, but that brings along other complexities.

Agreed that we can do this with an external agent.  So do you think
we can persuade dmtf tools lot to allow interfaces for this purpose
or are we looking at a fork of the spdm-emu examples?

> 
> As you pointed out in my original RFC, the complexity of configuration
> a responder via the QEMU command line will be very difficult. I think
> it's simpler to keep the responder outside and QEMU and just pass any
> relevant data to the responder as required.

I'm not sure how bad the interface would actually be.
My expectation is that we aren't going to need to emulate the
full flexibility of SPDM - for example we can keep to only the
required protocols etc.  As such, what do we need to pass in
beyond the cert chains?  We might provide options to do the more fun
stuff, but mostly defaults shoudl work.

> 
> >
> > Anyhow, I think we need to have gotten a little further figuring that
> > out before we merge a solution.  I've been carrying this on the CXL
> > staging tree for a long time because I couldn't figure out a good solution
> > to the amount of information that needs to go between them.
> >
> > For those not familiar with the fun of libSPDM it is a pain to work with
> > which is why Huai-Cheng instead connected with the demo app.
> >
> > Any more luck getting a reliable build to work?  
> 
> Yes!
> 
> libspdm is now packaged in buildroot:
> https://github.com/buildroot/buildroot/blob/master/package/libspdm/libspdm.mk
> 
> You can now build libspdm with openSSL provided by your distro as you
> don't need to access any private openSSL APIs any more.

Great.

> 
> The hope is we can continue to march towards including libspdm as a
> standard library in distros, which should make the entire process
> easier.

That is indeed good either way.

Jonathan

> 
> >  
> > >
> > > 1: https://www.dmtf.org/standards/SPDM
> > > 2: https://github.com/DMTF/libspdm
> > >
> > > Signed-off-by: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > > Signed-off-by: Chris Browy <cbrowy@avery-design.com>
> > > Co-developed-by: Jonathan Cameron <Jonathan.cameron@huawei.com>
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > [ Changes by AF:
> > >  - Convert to be more QEMU-ified
> > >  - Move to backends as it isn't PCIe specific
> > > ]
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>  
> > Alistair, you sent this so I think your sign off should be last
> > + some indication of Wilfred's involvement would be good?
> > Probably another Co-developed-by
> >
> >
> >  
> > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > > ---  
> >
> > I've looked at this code too much in the past to give much
> > real review.  Still a few comments inline.
> > I'm very keen to get a solution to this upstream, though I think
> > we do need to discuss a few general points (no cover letter so I'll
> > do it here).  
> 
> Yeah, I should have included a cover letter. V2 will
> 
> Alistair
> 
> >
> >
> > ...
> >  
> > > diff --git a/backends/spdm-socket.c b/backends/spdm-socket.c
> > > new file mode 100644
> > > index 0000000000..2f31ba80ba
> > > --- /dev/null
> > > +++ b/backends/spdm-socket.c
> > > @@ -0,0 +1,215 @@  
> >
> >  
> > > +
> > > +int spdm_socket_connect(uint16_t port, Error **errp)
> > > +{
> > > +    int client_socket;
> > > +    struct sockaddr_in server_addr;
> > > +
> > > +    client_socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> > > +    if (client_socket < 0) {
> > > +        error_setg(errp, "cannot create socket: %s", strerror(errno));
> > > +        return -1;
> > > +    }
> > > +
> > > +    memset((char *)&server_addr, 0, sizeof(server_addr));
> > > +    server_addr.sin_family = AF_INET;
> > > +    server_addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> > > +    server_addr.sin_port = htons(port);
> > > +
> > > +
> > > +    if (connect(client_socket, (struct sockaddr *)&server_addr, sizeof(server_addr)) < 0) {  
> > Wrap the line.
> >  
> > > +        error_setg(errp, "cannot connect: %s", strerror(errno));
> > > +        close(client_socket);
> > > +        return -1;
> > > +    }
> > > +
> > > +    return client_socket;
> > > +}  
> >
> >  
> 

Re: [PATCH 2/3] backends: Initial support for SPDM socket support

[Alistair Francis]

On Mon, Sep 18, 2023 at 8:28 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Mon, 18 Sep 2023 13:16:01 +1000
> Alistair Francis <alistair23@gmail.com> wrote:
>
> > On Sat, Sep 16, 2023 at 1:19 AM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:
> > >
> > > On Fri, 15 Sep 2023 21:27:22 +1000
> > > Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > > From: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > >
> > > Great to see you taking this forwards!
> > >
> > >
> > > >
> > > > SPDM enables authentication, attestation and key exchange to assist in
> > > > providing infrastructure security enablement. It's a standard published
> > > > by the DMTF [1].
> > > >
> > > > SPDM currently supports PCIe DOE and MCTP transports, but it can be
> > > > extended to support others in the future. This patch adds
> > > > support to QEMU to connect to an external SPDM instance.
> > >
> > > It supports way more that that these days.  I'd just say 'multiple'
> > > transports.
> > >
> > > >
> > > > SPDM support can be added to any QEMU device by exposing a
> > > > TCP socket to a SPDM server. The server can then implement the SPDM
> > > > decoding/encoding support, generally using libspdm [2].
> > > >
> > > > This is similar to how the current TPM implementation works and means
> > > > that the heavy lifting of setting up certificate chains, capabilities,
> > > > measurements and complex crypto can be done outside QEMU by a well
> > > > supported and tested library.
> > >
> > > Is this sufficient for usecases beyond initial attestation flows?
> >
> > I believe so.
> >
> > The SPDM responder would be in charge of doing all of this. For the
> > rest of the discussion the responder is the software on the other end
> > of the QEMU socket.
> >
> > > How does measurement work for example?  We need settings from the
> >
> > In a basic case the responder can generate measurement data. For
> > example the responder can return digests of the firmware. Now there
> > won't actually be "firmware", but the responder can still return
> > measurement data.
> >
> > if you are trying to test an existing product, you could fake it and
> > return the same values as a real device. Otherwise you could return
> > example data.
> >
> > > emulated device to squirt into the SPDM agent so that it can be
> > > encrypted and signed etc.
> > >
> > > Measurement reports often need to include the status of various config
> > > space registers + any device specific additional stuff - not sure
> > > what is defined for NVME but I suspect the list will grow, particularly
> > > when tdisp is included.  There are some things called out in the PCIe
> > > state as must haves, like any debug features must be reported.
> >
> > So this is probably the hard part. How can the responder measurements
> > change based on configuration values set by the host.
> >
> > Just for completeness, the idea would be the host would set some state
> > in the NVMe controller for example. Then we would expect the
> > measurement values to change.
> >
> > That's trickier, but we could extend the socket to communicate state
> > like that. So when a measurement state changes in the QEMU model, we
> > relay that to the responder. Which then changes the measurements
> >
> > > Also we need a way to mess with firmware revisions reported
> > > as those are likely to be checked.
> >
> > That seems like something you can do via command line arguments or
> > configuration settings to the responder. This is separate to QEMU
> I'm not convinced it is because QEMU is emulating the firmware behavior
> and that may well change with version.  Still it's just more metadata
> to push out from QEMU to the SPDM instance.
>
> My gut feeling is you'd just add an interface for the whole measurement
> record coming from QEMU.  No need to be clever with sending only small
> subsets of info and building the measurement.

That also works. libspdm delegates the measurement work anyway, so
it's easy to handle either in the external socket wrapper or in QEMU.

>
> >
> > >
> > > I'm not sure that model will work with the spdm-emu approach.
> >
> > I don't think there is anything specifically in the socket approach
> > that limits this from working. It comes down to passing information
> > from the QEMU emulated device to the SPDM responder. That needs to be
> > done either way. It probably is simpler to do if libspdm is included
> > as part of QEMU, but that brings along other complexities.
>
> Agreed that we can do this with an external agent.  So do you think
> we can persuade dmtf tools lot to allow interfaces for this purpose
> or are we looking at a fork of the spdm-emu examples?

I think that depends on them. We can either convince them to add
support, which doesn't seem impossible. Or we can just fork it.

It's worth pointing out that other tools besides spdm-emu can be used
here. It's a pretty simple transport protocol.

>
> >
> > As you pointed out in my original RFC, the complexity of configuration
> > a responder via the QEMU command line will be very difficult. I think
> > it's simpler to keep the responder outside and QEMU and just pass any
> > relevant data to the responder as required.
>
> I'm not sure how bad the interface would actually be.
> My expectation is that we aren't going to need to emulate the
> full flexibility of SPDM - for example we can keep to only the
> required protocols etc.  As such, what do we need to pass in
> beyond the cert chains?  We might provide options to do the more fun
> stuff, but mostly defaults shoudl work.

There are a range of configurations:
 - The supported capabilities
 - The asym algorithms
 - The hash algorithms
 - DHE, AED cipher suits
 - Measurement specs
 - Certificate chains

We could go down the route of saying "this is what the device
supports" and not letting users customise it. That is more like
modelling a real world device. That's handy for users attaching a
device, but not as useful for testing guest software (like the LInux
implementation).

If we keep this external (as in this series) we can allow very
customised implementations to be used, which will help test guest
software. That will allow software like EDK2 and Linux to be
thoroughly tested and even allow fuzzing or automated unit tests.

That doesn't preclude us from integrating libspdm directly in the
future though, if we want to just hard code the setup for specific
devices though. Which might end up happening when we start seeing real
world hardware that we want to emulate that includes SPDM support.

Alistair

>
> >
> > >
> > > Anyhow, I think we need to have gotten a little further figuring that
> > > out before we merge a solution.  I've been carrying this on the CXL
> > > staging tree for a long time because I couldn't figure out a good solution
> > > to the amount of information that needs to go between them.
> > >
> > > For those not familiar with the fun of libSPDM it is a pain to work with
> > > which is why Huai-Cheng instead connected with the demo app.
> > >
> > > Any more luck getting a reliable build to work?
> >
> > Yes!
> >
> > libspdm is now packaged in buildroot:
> > https://github.com/buildroot/buildroot/blob/master/package/libspdm/libspdm.mk
> >
> > You can now build libspdm with openSSL provided by your distro as you
> > don't need to access any private openSSL APIs any more.
>
> Great.
>
> >
> > The hope is we can continue to march towards including libspdm as a
> > standard library in distros, which should make the entire process
> > easier.
>
> That is indeed good either way.
>
> Jonathan
>
> >
> > >
> > > >
> > > > 1: https://www.dmtf.org/standards/SPDM
> > > > 2: https://github.com/DMTF/libspdm
> > > >
> > > > Signed-off-by: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > > > Signed-off-by: Chris Browy <cbrowy@avery-design.com>
> > > > Co-developed-by: Jonathan Cameron <Jonathan.cameron@huawei.com>
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > [ Changes by AF:
> > > >  - Convert to be more QEMU-ified
> > > >  - Move to backends as it isn't PCIe specific
> > > > ]
> > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > Alistair, you sent this so I think your sign off should be last
> > > + some indication of Wilfred's involvement would be good?
> > > Probably another Co-developed-by
> > >
> > >
> > >
> > > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > > > ---
> > >
> > > I've looked at this code too much in the past to give much
> > > real review.  Still a few comments inline.
> > > I'm very keen to get a solution to this upstream, though I think
> > > we do need to discuss a few general points (no cover letter so I'll
> > > do it here).
> >
> > Yeah, I should have included a cover letter. V2 will
> >
> > Alistair
> >
> > >
> > >
> > > ...
> > >
> > > > diff --git a/backends/spdm-socket.c b/backends/spdm-socket.c
> > > > new file mode 100644
> > > > index 0000000000..2f31ba80ba
> > > > --- /dev/null
> > > > +++ b/backends/spdm-socket.c
> > > > @@ -0,0 +1,215 @@
> > >
> > >
> > > > +
> > > > +int spdm_socket_connect(uint16_t port, Error **errp)
> > > > +{
> > > > +    int client_socket;
> > > > +    struct sockaddr_in server_addr;
> > > > +
> > > > +    client_socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> > > > +    if (client_socket < 0) {
> > > > +        error_setg(errp, "cannot create socket: %s", strerror(errno));
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    memset((char *)&server_addr, 0, sizeof(server_addr));
> > > > +    server_addr.sin_family = AF_INET;
> > > > +    server_addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> > > > +    server_addr.sin_port = htons(port);
> > > > +
> > > > +
> > > > +    if (connect(client_socket, (struct sockaddr *)&server_addr, sizeof(server_addr)) < 0) {
> > > Wrap the line.
> > >
> > > > +        error_setg(errp, "cannot connect: %s", strerror(errno));
> > > > +        close(client_socket);
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    return client_socket;
> > > > +}
> > >
> > >
> >
>

Re: [PATCH 2/3] backends: Initial support for SPDM socket support

[Jonathan Cameron via]

On Thu, 21 Sep 2023 16:28:00 +1000
Alistair Francis <alistair23@gmail.com> wrote:

> On Mon, Sep 18, 2023 at 8:28 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Mon, 18 Sep 2023 13:16:01 +1000
> > Alistair Francis <alistair23@gmail.com> wrote:
> >  
> > > On Sat, Sep 16, 2023 at 1:19 AM Jonathan Cameron
> > > <Jonathan.Cameron@huawei.com> wrote:  
> > > >
> > > > On Fri, 15 Sep 2023 21:27:22 +1000
> > > > Alistair Francis <alistair23@gmail.com> wrote:
> > > >  
> > > > > From: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>  
> > > >
> > > > Great to see you taking this forwards!
> > > >
> > > >  
> > > > >
> > > > > SPDM enables authentication, attestation and key exchange to assist in
> > > > > providing infrastructure security enablement. It's a standard published
> > > > > by the DMTF [1].
> > > > >
> > > > > SPDM currently supports PCIe DOE and MCTP transports, but it can be
> > > > > extended to support others in the future. This patch adds
> > > > > support to QEMU to connect to an external SPDM instance.  
> > > >
> > > > It supports way more that that these days.  I'd just say 'multiple'
> > > > transports.
> > > >  
> > > > >
> > > > > SPDM support can be added to any QEMU device by exposing a
> > > > > TCP socket to a SPDM server. The server can then implement the SPDM
> > > > > decoding/encoding support, generally using libspdm [2].
> > > > >
> > > > > This is similar to how the current TPM implementation works and means
> > > > > that the heavy lifting of setting up certificate chains, capabilities,
> > > > > measurements and complex crypto can be done outside QEMU by a well
> > > > > supported and tested library.  
> > > >
> > > > Is this sufficient for usecases beyond initial attestation flows?  
> > >
> > > I believe so.
> > >
> > > The SPDM responder would be in charge of doing all of this. For the
> > > rest of the discussion the responder is the software on the other end
> > > of the QEMU socket.
> > >  
> > > > How does measurement work for example?  We need settings from the  
> > >
> > > In a basic case the responder can generate measurement data. For
> > > example the responder can return digests of the firmware. Now there
> > > won't actually be "firmware", but the responder can still return
> > > measurement data.
> > >
> > > if you are trying to test an existing product, you could fake it and
> > > return the same values as a real device. Otherwise you could return
> > > example data.
> > >  
> > > > emulated device to squirt into the SPDM agent so that it can be
> > > > encrypted and signed etc.
> > > >
> > > > Measurement reports often need to include the status of various config
> > > > space registers + any device specific additional stuff - not sure
> > > > what is defined for NVME but I suspect the list will grow, particularly
> > > > when tdisp is included.  There are some things called out in the PCIe
> > > > state as must haves, like any debug features must be reported.  
> > >
> > > So this is probably the hard part. How can the responder measurements
> > > change based on configuration values set by the host.
> > >
> > > Just for completeness, the idea would be the host would set some state
> > > in the NVMe controller for example. Then we would expect the
> > > measurement values to change.
> > >
> > > That's trickier, but we could extend the socket to communicate state
> > > like that. So when a measurement state changes in the QEMU model, we
> > > relay that to the responder. Which then changes the measurements
> > >  
> > > > Also we need a way to mess with firmware revisions reported
> > > > as those are likely to be checked.  
> > >
> > > That seems like something you can do via command line arguments or
> > > configuration settings to the responder. This is separate to QEMU  
> > I'm not convinced it is because QEMU is emulating the firmware behavior
> > and that may well change with version.  Still it's just more metadata
> > to push out from QEMU to the SPDM instance.
> >
> > My gut feeling is you'd just add an interface for the whole measurement
> > record coming from QEMU.  No need to be clever with sending only small
> > subsets of info and building the measurement.  
> 
> That also works. libspdm delegates the measurement work anyway, so
> it's easy to handle either in the external socket wrapper or in QEMU.
> 
> >  
> > >  
> > > >
> > > > I'm not sure that model will work with the spdm-emu approach.  
> > >
> > > I don't think there is anything specifically in the socket approach
> > > that limits this from working. It comes down to passing information
> > > from the QEMU emulated device to the SPDM responder. That needs to be
> > > done either way. It probably is simpler to do if libspdm is included
> > > as part of QEMU, but that brings along other complexities.  
> >
> > Agreed that we can do this with an external agent.  So do you think
> > we can persuade dmtf tools lot to allow interfaces for this purpose
> > or are we looking at a fork of the spdm-emu examples?  
> 
> I think that depends on them. We can either convince them to add
> support, which doesn't seem impossible. Or we can just fork it.
> 
> It's worth pointing out that other tools besides spdm-emu can be used
> here. It's a pretty simple transport protocol.

True enough - though I hope we don't end up replicating the complexity of
libspdm.  Changing the the actual app is less of an issue.

> 
> >  
> > >
> > > As you pointed out in my original RFC, the complexity of configuration
> > > a responder via the QEMU command line will be very difficult. I think
> > > it's simpler to keep the responder outside and QEMU and just pass any
> > > relevant data to the responder as required.  
> >
> > I'm not sure how bad the interface would actually be.
> > My expectation is that we aren't going to need to emulate the
> > full flexibility of SPDM - for example we can keep to only the
> > required protocols etc.  As such, what do we need to pass in
> > beyond the cert chains?  We might provide options to do the more fun
> > stuff, but mostly defaults shoudl work.  
> 
> There are a range of configurations:
>  - The supported capabilities
>  - The asym algorithms
>  - The hash algorithms
>  - DHE, AED cipher suits
>  - Measurement specs
>  - Certificate chains
> 
> We could go down the route of saying "this is what the device
> supports" and not letting users customise it. That is more like
> modelling a real world device. That's handy for users attaching a
> device, but not as useful for testing guest software (like the LInux
> implementation).

There is a different between providing interfaces to play with all the
details, and expecting most users to care about them.  Defaults would
cover anyone not trying to shake out bugs in the transfers etc.

You are allowed to do a lot of things in SPDM, but CMA limits things
you must do support to far less I suspect for a while at least that's
all software stacks will bother with.

> 
> If we keep this external (as in this series) we can allow very
> customised implementations to be used, which will help test guest
> software. That will allow software like EDK2 and Linux to be
> thoroughly tested and even allow fuzzing or automated unit tests.
> 
> That doesn't preclude us from integrating libspdm directly in the
> future though, if we want to just hard code the setup for specific
> devices though. Which might end up happening when we start seeing real
> world hardware that we want to emulate that includes SPDM support.

I'm nervous that we will build a very complex set of interactions but
sure, if we allow for a path to link libSPDM in for the future
I'm fine with an alternative of an external agent.

I'm seeing more and more uses of SPDM surfacing and some of them
are going to involve a lot of complexity in that agent that is tightly
coupled to the hardware in QEMU. However we definitely shouldn't
let the quest for a 'perfect' solution get in the way of a something
that actually works.  So I'm fine with this solution for now.

For giggles I'll hook this up to an MCTP over I2C device as well when
I get time as looks like people also care about that path for key
programming etc.

Jonathan

> 
> Alistair
> 
> >  
> > >  
> > > >
> > > > Anyhow, I think we need to have gotten a little further figuring that
> > > > out before we merge a solution.  I've been carrying this on the CXL
> > > > staging tree for a long time because I couldn't figure out a good solution
> > > > to the amount of information that needs to go between them.
> > > >
> > > > For those not familiar with the fun of libSPDM it is a pain to work with
> > > > which is why Huai-Cheng instead connected with the demo app.
> > > >
> > > > Any more luck getting a reliable build to work?  
> > >
> > > Yes!
> > >
> > > libspdm is now packaged in buildroot:
> > > https://github.com/buildroot/buildroot/blob/master/package/libspdm/libspdm.mk
> > >
> > > You can now build libspdm with openSSL provided by your distro as you
> > > don't need to access any private openSSL APIs any more.  
> >
> > Great.
> >  
> > >
> > > The hope is we can continue to march towards including libspdm as a
> > > standard library in distros, which should make the entire process
> > > easier.  
> >
> > That is indeed good either way.
> >
> > Jonathan
> >  
> > >  
> > > >  
> > > > >
> > > > > 1: https://www.dmtf.org/standards/SPDM
> > > > > 2: https://github.com/DMTF/libspdm
> > > > >
> > > > > Signed-off-by: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > > > > Signed-off-by: Chris Browy <cbrowy@avery-design.com>
> > > > > Co-developed-by: Jonathan Cameron <Jonathan.cameron@huawei.com>
> > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > [ Changes by AF:
> > > > >  - Convert to be more QEMU-ified
> > > > >  - Move to backends as it isn't PCIe specific
> > > > > ]
> > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>  
> > > > Alistair, you sent this so I think your sign off should be last
> > > > + some indication of Wilfred's involvement would be good?
> > > > Probably another Co-developed-by
> > > >
> > > >
> > > >  
> > > > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > > > > ---  
> > > >
> > > > I've looked at this code too much in the past to give much
> > > > real review.  Still a few comments inline.
> > > > I'm very keen to get a solution to this upstream, though I think
> > > > we do need to discuss a few general points (no cover letter so I'll
> > > > do it here).  
> > >
> > > Yeah, I should have included a cover letter. V2 will
> > >
> > > Alistair
> > >  
> > > >
> > > >
> > > > ...
> > > >  
> > > > > diff --git a/backends/spdm-socket.c b/backends/spdm-socket.c
> > > > > new file mode 100644
> > > > > index 0000000000..2f31ba80ba
> > > > > --- /dev/null
> > > > > +++ b/backends/spdm-socket.c
> > > > > @@ -0,0 +1,215 @@  
> > > >
> > > >  
> > > > > +
> > > > > +int spdm_socket_connect(uint16_t port, Error **errp)
> > > > > +{
> > > > > +    int client_socket;
> > > > > +    struct sockaddr_in server_addr;
> > > > > +
> > > > > +    client_socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> > > > > +    if (client_socket < 0) {
> > > > > +        error_setg(errp, "cannot create socket: %s", strerror(errno));
> > > > > +        return -1;
> > > > > +    }
> > > > > +
> > > > > +    memset((char *)&server_addr, 0, sizeof(server_addr));
> > > > > +    server_addr.sin_family = AF_INET;
> > > > > +    server_addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> > > > > +    server_addr.sin_port = htons(port);
> > > > > +
> > > > > +
> > > > > +    if (connect(client_socket, (struct sockaddr *)&server_addr, sizeof(server_addr)) < 0) {  
> > > > Wrap the line.
> > > >  
> > > > > +        error_setg(errp, "cannot connect: %s", strerror(errno));
> > > > > +        close(client_socket);
> > > > > +        return -1;
> > > > > +    }
> > > > > +
> > > > > +    return client_socket;
> > > > > +}  
> > > >
> > > >  
> > >  
> >  
> 
> 

[PATCH 3/3] hw/nvme: Add SPDM over DOE support

[Alistair Francis]

From: Wilfred Mallawa <wilfred.mallawa@wdc.com>

Setup Data Object Exchance (DOE) as an extended capability for the NVME
controller and connect SPDM to it (CMA) to it.

Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 docs/specs/index.rst        |  1 +
 docs/specs/spdm.rst         | 56 +++++++++++++++++++++++++++++++++++++
 include/hw/pci/pci_device.h |  5 ++++
 include/hw/pci/pcie_doe.h   |  3 ++
 hw/nvme/ctrl.c              | 52 ++++++++++++++++++++++++++++++++++
 hw/nvme/trace-events        |  1 +
 6 files changed, 118 insertions(+)
 create mode 100644 docs/specs/spdm.rst

diff --git a/docs/specs/index.rst b/docs/specs/index.rst
index e58be38c41..c398541388 100644
--- a/docs/specs/index.rst
+++ b/docs/specs/index.rst
@@ -24,3 +24,4 @@ guest hardware that is specific to QEMU.
    acpi_erst
    sev-guest-firmware
    fw_cfg
+   spdm
diff --git a/docs/specs/spdm.rst b/docs/specs/spdm.rst
new file mode 100644
index 0000000000..0f96d618ef
--- /dev/null
+++ b/docs/specs/spdm.rst
@@ -0,0 +1,56 @@
+======================================================
+QEMU Security Protocols and Data Models (SPDM) Support
+======================================================
+
+SPDM enables authentication, attestation and key exchange to assist in
+providing infrastructure security enablement. It's a standard published
+by the DMTF https://www.dmtf.org/standards/SPDM.
+
+Setting up a SPDM server
+========================
+
+When using QEMU with SPDM devices QEMU will connect to a server which
+implements the SPDM functionality.
+
+spdm-emu
+--------
+
+You can use spdm-emu https://github.com/dmtf/spdm-emu to model the
+SPDM responder.
+
+.. code-block:: shell
+
+    $ cd spdm-emu
+    $ git submodule init; git submodule update --recursive
+    $ mkdir build; cd build
+    $ cmake -DARCH=x64 -DTOOLCHAIN=GCC -DTARGET=Debug -DCRYPTO=openssl ..
+    $ make -j32
+    $ make copy_sample_key # Build certificates, required for SPDM authentication.
+
+The responder can then be launched with
+
+.. code-block:: shell
+
+    $ cd bin
+    $ ./spdm_responder_emu --trans PCI_DOE
+
+Connecting an SPDM NVMe device
+==============================
+
+Once a SPDM server is running we can start QEMU and connect to the server.
+
+For an NVMe device first let's setup a block we can use
+
+.. code-block:: shell
+
+    $ cd qemu-spdm/linux/image
+    $ dd if=/dev/zero of=blknvme bs=1M count=2096 # 2GB NNMe Drive
+
+Then you can add this to your QEMU command line:
+
+.. code-block:: shell
+
+    -drive file=blknvme,if=none,id=mynvme,format=raw \
+    -device nvme,drive=mynvme,serial=deadbeef,spdm=2323
+
+At which point QEMU will connect to the SPDM server.
diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
index d3dd0f64b2..b8379c78f1 100644
--- a/include/hw/pci/pci_device.h
+++ b/include/hw/pci/pci_device.h
@@ -3,6 +3,7 @@
 
 #include "hw/pci/pci.h"
 #include "hw/pci/pcie.h"
+#include "hw/pci/pcie_doe.h"
 
 #define TYPE_PCI_DEVICE "pci-device"
 typedef struct PCIDeviceClass PCIDeviceClass;
@@ -157,6 +158,10 @@ struct PCIDevice {
     MSIVectorReleaseNotifier msix_vector_release_notifier;
     MSIVectorPollNotifier msix_vector_poll_notifier;
 
+    /* DOE */
+    DOECap doe_spdm;
+    uint16_t spdm_port;
+
     /* ID of standby device in net_failover pair */
     char *failover_pair_id;
     uint32_t acpi_index;
diff --git a/include/hw/pci/pcie_doe.h b/include/hw/pci/pcie_doe.h
index 15d94661f9..eb8f4e393d 100644
--- a/include/hw/pci/pcie_doe.h
+++ b/include/hw/pci/pcie_doe.h
@@ -108,6 +108,9 @@ struct DOECap {
     /* Protocols and its callback response */
     DOEProtocol *protocols;
     uint16_t protocol_num;
+
+    /* Used for spdm-socket */
+    int socket;
 };
 
 void pcie_doe_init(PCIDevice *pdev, DOECap *doe_cap, uint16_t offset,
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 90687b168a..1ff30a9ad4 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -203,6 +203,7 @@
 #include "sysemu/hostmem.h"
 #include "hw/pci/msix.h"
 #include "hw/pci/pcie_sriov.h"
+#include "sysemu/spdm-socket.h"
 #include "migration/vmstate.h"
 
 #include "nvme.h"
@@ -8077,6 +8078,28 @@ static int nvme_add_pm_capability(PCIDevice *pci_dev, uint8_t offset)
     return 0;
 }
 
+static bool pcie_doe_spdm_rsp(DOECap *doe_cap)
+{
+    void *req = pcie_doe_get_write_mbox_ptr(doe_cap);
+    uint32_t req_len = pcie_doe_get_obj_len(req) * 4;
+    void *rsp = doe_cap->read_mbox;
+    uint32_t rsp_len = SPDM_SOCKET_MAX_MESSAGE_BUFFER_SIZE;
+    uint32_t recvd;
+
+    recvd = spdm_socket_rsp(doe_cap->socket,
+                             SPDM_SOCKET_TRANSPORT_TYPE_PCI_DOE,
+                             req, req_len, rsp, rsp_len);
+    doe_cap->read_mbox_len += DIV_ROUND_UP(recvd, 4);
+
+    return (recvd == 0) ? false : true;
+}
+
+static DOEProtocol doe_spdm_prot[] = {
+    { PCI_VENDOR_ID_PCI_SIG, PCI_SIG_DOE_CMA, pcie_doe_spdm_rsp },
+    { PCI_VENDOR_ID_PCI_SIG, PCI_SIG_DOE_SECURED_CMA, pcie_doe_spdm_rsp },
+    { }
+};
+
 static bool nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
 {
     ERRP_GUARD();
@@ -8133,6 +8156,23 @@ static bool nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
 
     nvme_update_msixcap_ts(pci_dev, n->conf_msix_qsize);
 
+    pcie_cap_deverr_init(pci_dev);
+
+    /* DOE Initialisation */
+    if (pci_dev->spdm_port) {
+        uint16_t doe_offset = n->params.sriov_max_vfs ?
+                                  PCI_CONFIG_SPACE_SIZE + PCI_ARI_SIZEOF
+                                  : PCI_CONFIG_SPACE_SIZE;
+
+        pcie_doe_init(pci_dev, &pci_dev->doe_spdm, doe_offset, doe_spdm_prot, true, 0);
+
+        pci_dev->doe_spdm.socket = spdm_socket_connect(pci_dev->spdm_port, errp);
+
+        if (pci_dev->doe_spdm.socket < 0 ) {
+            trace_pci_cma_err_openspdm_conn();
+        }
+    }
+
     if (n->params.cmb_size_mb) {
         nvme_init_cmb(n, pci_dev);
     }
@@ -8419,6 +8459,7 @@ static Property nvme_props[] = {
                       params.sriov_max_vi_per_vf, 0),
     DEFINE_PROP_UINT8("sriov_max_vq_per_vf", NvmeCtrl,
                       params.sriov_max_vq_per_vf, 0),
+    DEFINE_PROP_UINT16("spdm", PCIDevice, spdm_port, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -8501,10 +8542,20 @@ static void nvme_pci_write_config(PCIDevice *dev, uint32_t address,
                                   uint32_t val, int len)
 {
     nvme_sriov_pre_write_ctrl(dev, address, val, len);
+    pcie_doe_write_config(&dev->doe_spdm, address, val, len);
     pci_default_write_config(dev, address, val, len);
     pcie_cap_flr_write_config(dev, address, val, len);
 }
 
+static uint32_t nvme_pci_read_config(PCIDevice *dev, uint32_t address, int len)
+{
+    uint32_t val;
+    if (pcie_doe_read_config(&dev->doe_spdm, address, len, &val)) {
+        return val;
+    }
+    return pci_default_read_config(dev, address, len);
+}
+
 static const VMStateDescription nvme_vmstate = {
     .name = "nvme",
     .unmigratable = 1,
@@ -8517,6 +8568,7 @@ static void nvme_class_init(ObjectClass *oc, void *data)
 
     pc->realize = nvme_realize;
     pc->config_write = nvme_pci_write_config;
+    pc->config_read = nvme_pci_read_config;
     pc->exit = nvme_exit;
     pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
     pc->revision = 2;
diff --git a/hw/nvme/trace-events b/hw/nvme/trace-events
index 3a67680c6a..c0e2ce9fda 100644
--- a/hw/nvme/trace-events
+++ b/hw/nvme/trace-events
@@ -191,6 +191,7 @@ pci_nvme_err_startfail(void) "setting controller enable bit failed"
 pci_nvme_err_startfail_virt_state(uint16_t vq, uint16_t vi) "nvme_start_ctrl failed due to ctrl state: vi=%u vq=%u"
 pci_nvme_err_invalid_mgmt_action(uint8_t action) "action=0x%"PRIx8""
 pci_nvme_err_ignored_mmio_vf_offline(uint64_t addr, unsigned size) "addr 0x%"PRIx64" size %d"
+pci_cma_err_openspdm_conn(void) "Failed to connect to OpenSPDM, SPDM disabled"
 
 # undefined behavior
 pci_nvme_ub_mmiowr_misaligned32(uint64_t offset) "MMIO write not 32-bit aligned, offset=0x%"PRIx64""
-- 
2.41.0

Re: [PATCH 3/3] hw/nvme: Add SPDM over DOE support

[Jonathan Cameron via]

On Fri, 15 Sep 2023 21:27:23 +1000
Alistair Francis <alistair23@gmail.com> wrote:

> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> 
> Setup Data Object Exchance (DOE) as an extended capability for the NVME
> controller and connect SPDM to it (CMA) to it.
> 
> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
A few comments inline.  

> ---
>  docs/specs/index.rst        |  1 +
>  docs/specs/spdm.rst         | 56 +++++++++++++++++++++++++++++++++++++
>  include/hw/pci/pci_device.h |  5 ++++
>  include/hw/pci/pcie_doe.h   |  3 ++
>  hw/nvme/ctrl.c              | 52 ++++++++++++++++++++++++++++++++++
>  hw/nvme/trace-events        |  1 +
>  6 files changed, 118 insertions(+)
>  create mode 100644 docs/specs/spdm.rst
> 
> diff --git a/docs/specs/index.rst b/docs/specs/index.rst
> index e58be38c41..c398541388 100644
> --- a/docs/specs/index.rst
> +++ b/docs/specs/index.rst
> @@ -24,3 +24,4 @@ guest hardware that is specific to QEMU.
>     acpi_erst
>     sev-guest-firmware
>     fw_cfg
> +   spdm
> diff --git a/docs/specs/spdm.rst b/docs/specs/spdm.rst
> new file mode 100644
> index 0000000000..0f96d618ef
> --- /dev/null
> +++ b/docs/specs/spdm.rst
> @@ -0,0 +1,56 @@
> +======================================================
> +QEMU Security Protocols and Data Models (SPDM) Support
> +======================================================
> +
> +SPDM enables authentication, attestation and key exchange to assist in
> +providing infrastructure security enablement. It's a standard published
> +by the DMTF https://www.dmtf.org/standards/SPDM.
> +
> +Setting up a SPDM server
> +========================
> +
> +When using QEMU with SPDM devices QEMU will connect to a server which
> +implements the SPDM functionality.
> +
> +spdm-emu
> +--------
> +
> +You can use spdm-emu https://github.com/dmtf/spdm-emu to model the
> +SPDM responder.
> +
> +.. code-block:: shell
> +
> +    $ cd spdm-emu
> +    $ git submodule init; git submodule update --recursive
> +    $ mkdir build; cd build
> +    $ cmake -DARCH=x64 -DTOOLCHAIN=GCC -DTARGET=Debug -DCRYPTO=openssl ..
> +    $ make -j32
> +    $ make copy_sample_key # Build certificates, required for SPDM authentication.
> +
> +The responder can then be launched with
> +
> +.. code-block:: shell
> +
> +    $ cd bin
> +    $ ./spdm_responder_emu --trans PCI_DOE
> +
> +Connecting an SPDM NVMe device
> +==============================
> +
> +Once a SPDM server is running we can start QEMU and connect to the server.
> +
> +For an NVMe device first let's setup a block we can use
> +
> +.. code-block:: shell
> +
> +    $ cd qemu-spdm/linux/image
> +    $ dd if=/dev/zero of=blknvme bs=1M count=2096 # 2GB NNMe Drive
> +
> +Then you can add this to your QEMU command line:
> +
> +.. code-block:: shell
> +
> +    -drive file=blknvme,if=none,id=mynvme,format=raw \
> +    -device nvme,drive=mynvme,serial=deadbeef,spdm=2323
> +
> +At which point QEMU will connect to the SPDM server.

try to connect.

...

>  
>  void pcie_doe_init(PCIDevice *pdev, DOECap *doe_cap, uint16_t offset,
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 90687b168a..1ff30a9ad4 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -203,6 +203,7 @@
>  #include "sysemu/hostmem.h"
>  #include "hw/pci/msix.h"
>  #include "hw/pci/pcie_sriov.h"
> +#include "sysemu/spdm-socket.h"
>  #include "migration/vmstate.h"
>  
>  #include "nvme.h"
> @@ -8077,6 +8078,28 @@ static int nvme_add_pm_capability(PCIDevice *pci_dev, uint8_t offset)
>      return 0;
>  }
>  
> +static bool pcie_doe_spdm_rsp(DOECap *doe_cap)
> +{
> +    void *req = pcie_doe_get_write_mbox_ptr(doe_cap);
> +    uint32_t req_len = pcie_doe_get_obj_len(req) * 4;
> +    void *rsp = doe_cap->read_mbox;
> +    uint32_t rsp_len = SPDM_SOCKET_MAX_MESSAGE_BUFFER_SIZE;
> +    uint32_t recvd;
> +
> +    recvd = spdm_socket_rsp(doe_cap->socket,
> +                             SPDM_SOCKET_TRANSPORT_TYPE_PCI_DOE,
> +                             req, req_len, rsp, rsp_len);
> +    doe_cap->read_mbox_len += DIV_ROUND_UP(recvd, 4);
> +
> +    return (recvd == 0) ? false : true;

return recd != 0;

> +}
> +
> +static DOEProtocol doe_spdm_prot[] = {
> +    { PCI_VENDOR_ID_PCI_SIG, PCI_SIG_DOE_CMA, pcie_doe_spdm_rsp },
> +    { PCI_VENDOR_ID_PCI_SIG, PCI_SIG_DOE_SECURED_CMA, pcie_doe_spdm_rsp },
> +    { }
> +};
> +
>  static bool nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
>  {
>      ERRP_GUARD();
> @@ -8133,6 +8156,23 @@ static bool nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
>  
>      nvme_update_msixcap_ts(pci_dev, n->conf_msix_qsize);
>  
> +    pcie_cap_deverr_init(pci_dev);

Unrelated. Or I can't tell why it is related anyway.

> +
> +    /* DOE Initialisation */
> +    if (pci_dev->spdm_port) {
> +        uint16_t doe_offset = n->params.sriov_max_vfs ?
> +                                  PCI_CONFIG_SPACE_SIZE + PCI_ARI_SIZEOF
> +                                  : PCI_CONFIG_SPACE_SIZE;
> +
> +        pcie_doe_init(pci_dev, &pci_dev->doe_spdm, doe_offset, doe_spdm_prot, true, 0);
> +
> +        pci_dev->doe_spdm.socket = spdm_socket_connect(pci_dev->spdm_port, errp);
If going to carry on anyway, which is a valid choice, I'd put the result of this
in a local variable and only set pci_dev->doe_spdm.socket on success perhaps?
Also, don't register the DOE if that happens.

Better still - error out as if someone asked for it and the socket connect failed.
 
> +
> +        if (pci_dev->doe_spdm.socket < 0 ) {
> +            trace_pci_cma_err_openspdm_conn();
> +        }
> +    }
> +
>      if (n->params.cmb_size_mb) {
>          nvme_init_cmb(n, pci_dev);
>      }
> @@ -8419,6 +8459,7 @@ static Property nvme_props[] = {
>                        params.sriov_max_vi_per_vf, 0),
>      DEFINE_PROP_UINT8("sriov_max_vq_per_vf", NvmeCtrl,
>                        params.sriov_max_vq_per_vf, 0),
> +    DEFINE_PROP_UINT16("spdm", PCIDevice, spdm_port, 0),
>      DEFINE_PROP_END_OF_LIST(),

Re: [PATCH 3/3] hw/nvme: Add SPDM over DOE support

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1026 bytes --]

On Sep 15 21:27, Alistair Francis wrote:
> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> 
> Setup Data Object Exchance (DOE) as an extended capability for the NVME
> controller and connect SPDM to it (CMA) to it.
> 
> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  docs/specs/index.rst        |  1 +
>  docs/specs/spdm.rst         | 56 +++++++++++++++++++++++++++++++++++++
>  include/hw/pci/pci_device.h |  5 ++++
>  include/hw/pci/pcie_doe.h   |  3 ++
>  hw/nvme/ctrl.c              | 52 ++++++++++++++++++++++++++++++++++
>  hw/nvme/trace-events        |  1 +
>  6 files changed, 118 insertions(+)
>  create mode 100644 docs/specs/spdm.rst
> 

This looks reasonable enough, but could this not be implemented at the
PCI layer? I do not see anything that is tied specifically to the nvme
device, why can the spdm parameter not be a PCIDevice parameter such
that all PCIDevice-derived devices gains this functionality?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] hw/nvme: Add SPDM over DOE support

[Jonathan Cameron via]

On Mon, 2 Oct 2023 09:15:58 +0200
Klaus Jensen <its@irrelevant.dk> wrote:

> On Sep 15 21:27, Alistair Francis wrote:
> > From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > 
> > Setup Data Object Exchance (DOE) as an extended capability for the NVME
> > controller and connect SPDM to it (CMA) to it.
> > 
> > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  docs/specs/index.rst        |  1 +
> >  docs/specs/spdm.rst         | 56 +++++++++++++++++++++++++++++++++++++
> >  include/hw/pci/pci_device.h |  5 ++++
> >  include/hw/pci/pcie_doe.h   |  3 ++
> >  hw/nvme/ctrl.c              | 52 ++++++++++++++++++++++++++++++++++
> >  hw/nvme/trace-events        |  1 +
> >  6 files changed, 118 insertions(+)
> >  create mode 100644 docs/specs/spdm.rst
> >   
> 
> This looks reasonable enough, but could this not be implemented at the
> PCI layer? I do not see anything that is tied specifically to the nvme
> device, why can the spdm parameter not be a PCIDevice parameter such
> that all PCIDevice-derived devices gains this functionality?
> 
Whilst a few parts of this feel like they'd reasonably move to PCI core
library code (though may not be worth it as they are trivial wrappers).
I can look at that when we add CXL support on top of this (which will be
simple given Alastair used the SPDM sockets stuff the CXL stuff is
based on.)

I'm not sure it makes sense to have it as a property for all
devices. If we did go that way possible issues would include.
1) Need find some extended config space to put it in and extended config
   space layouts are per device. We'd need to do something like
   pcie_endpoint_cap_init() that provided a suitable address range.
   So other than stashing the address int he PCI core to allow the
   default config_read / config_write to work we don't gain much.
2) This will get more complex and more device specific as there are a whole
   load of device specific protocols that will get layered on top. Sure
   we could do all that with callbacks, but seems simpler just to stick to
   a bit of repeating boiler plate.
3) There will probably be real devices with multiple instances.  Whether
   we emulate the in QEMU is a whole different question. Virtualizing real
   instances of this is going to be interest (if we don't just hide them
   completely in the host).

Anyhow I'd go with a 'maybe' but not yet.  Easy to move this later I think
if it does make sense to move where it is implemented?

Jonathan

Re: [PATCH 3/3] hw/nvme: Add SPDM over DOE support

[Jonathan Cameron]

On Mon, 2 Oct 2023 09:15:58 +0200
Klaus Jensen <its@irrelevant.dk> wrote:

> On Sep 15 21:27, Alistair Francis wrote:
> > From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > 
> > Setup Data Object Exchance (DOE) as an extended capability for the NVME
> > controller and connect SPDM to it (CMA) to it.
> > 
> > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  docs/specs/index.rst        |  1 +
> >  docs/specs/spdm.rst         | 56 +++++++++++++++++++++++++++++++++++++
> >  include/hw/pci/pci_device.h |  5 ++++
> >  include/hw/pci/pcie_doe.h   |  3 ++
> >  hw/nvme/ctrl.c              | 52 ++++++++++++++++++++++++++++++++++
> >  hw/nvme/trace-events        |  1 +
> >  6 files changed, 118 insertions(+)
> >  create mode 100644 docs/specs/spdm.rst
> >   
> 
> This looks reasonable enough, but could this not be implemented at the
> PCI layer? I do not see anything that is tied specifically to the nvme
> device, why can the spdm parameter not be a PCIDevice parameter such
> that all PCIDevice-derived devices gains this functionality?
> 
Whilst a few parts of this feel like they'd reasonably move to PCI core
library code (though may not be worth it as they are trivial wrappers).
I can look at that when we add CXL support on top of this (which will be
simple given Alastair used the SPDM sockets stuff the CXL stuff is
based on.)

I'm not sure it makes sense to have it as a property for all
devices. If we did go that way possible issues would include.
1) Need find some extended config space to put it in and extended config
   space layouts are per device. We'd need to do something like
   pcie_endpoint_cap_init() that provided a suitable address range.
   So other than stashing the address int he PCI core to allow the
   default config_read / config_write to work we don't gain much.
2) This will get more complex and more device specific as there are a whole
   load of device specific protocols that will get layered on top. Sure
   we could do all that with callbacks, but seems simpler just to stick to
   a bit of repeating boiler plate.
3) There will probably be real devices with multiple instances.  Whether
   we emulate the in QEMU is a whole different question. Virtualizing real
   instances of this is going to be interest (if we don't just hide them
   completely in the host).

Anyhow I'd go with a 'maybe' but not yet.  Easy to move this later I think
if it does make sense to move where it is implemented?

Jonathan

Re: [PATCH 3/3] hw/nvme: Add SPDM over DOE support

[Lukas Wunner]

On Fri, Sep 15, 2023 at 09:27:23PM +1000, Alistair Francis wrote:
> --- /dev/null
> +++ b/docs/specs/spdm.rst
> @@ -0,0 +1,56 @@
> +======================================================
> +QEMU Security Protocols and Data Models (SPDM) Support
> +======================================================
> +
> +SPDM enables authentication, attestation and key exchange to assist in
> +providing infrastructure security enablement. It's a standard published
> +by the DMTF https://www.dmtf.org/standards/SPDM.
> +
> +Setting up a SPDM server
[...]
> +    $ cd spdm-emu
> +    $ git submodule init; git submodule update --recursive
> +    $ mkdir build; cd build
> +    $ cmake -DARCH=x64 -DTOOLCHAIN=GCC -DTARGET=Debug -DCRYPTO=openssl ..
> +    $ make -j32
> +    $ make copy_sample_key # Build certificates, required for SPDM authentication.

Might be worth pointing out that certificates need to have a
Subject Alternative Name in compliance with PCIe r6.1 sec 6.31.3,
what to add to openssl.cnf to get one, e.g. ...

    subjectAltName = otherName:2.23.147;UTF8:Vendor=1b36:Device=0010:CC=010802:REV=02:SSVID=1af4:SSID=1100
    2.23.147 = ASN1:OID:2.23.147

... and how to regenerate certificates after modifying openssl.cnf, e.g. ...

    $ openssl req -nodes -newkey ec:param.pem -keyout end_responder.key -out end_responder.req -sha384 -batch -subj "/CN=DMTF libspdm ECP384 responder cert"
    $ openssl x509 -req -in end_responder.req -out end_responder.cert -CA inter.cert -CAkey inter.key -sha384 -days 3650 -set_serial 3 -extensions v3_end -extfile ../openssl.cnf
    $ openssl asn1parse -in end_responder.cert -out end_responder.cert.der
    $ cat ca.cert.der inter.cert.der end_responder.cert.der > bundle_responder.certchain.der

Or preferably modify upstream libspdm to automate this process,
make it less cumbersome and error-prone.


> +static bool pcie_doe_spdm_rsp(DOECap *doe_cap)
> +{
> +    void *req = pcie_doe_get_write_mbox_ptr(doe_cap);
> +    uint32_t req_len = pcie_doe_get_obj_len(req) * 4;
> +    void *rsp = doe_cap->read_mbox;
> +    uint32_t rsp_len = SPDM_SOCKET_MAX_MESSAGE_BUFFER_SIZE;
> +    uint32_t recvd;

Might be worth mentioning somewhere that this only implements the
responder role.

CPUs are coming to market which contain a Trusted Security Module.
Some of those TSMs are capable of the SPDM requester role.  Should
qemu ever have the need to emulate a CPU containing a TSM, it may
become necessary to add SPDM requester support.

Thanks,

Lukas

RE: [PATCH 3/3] hw/nvme: Add SPDM over DOE support

[Yao, Jiewen]

Comment on subjectAltName.

PCI-SIG realized that it may cause problem for certain device and decided to remove such requirement in future ECN.
I don't think that is absolutely needed.



> -----Original Message-----
> From: Lukas Wunner <lukas@wunner.de>
> Sent: Monday, October 2, 2023 4:48 PM
> To: Alistair Francis <alistair23@gmail.com>
> Cc: wilfred.mallawa@wdc.com; Jonathan.Cameron@Huawei.com; Yao, Jiewen
> <jiewen.yao@intel.com>; qemu-devel@nongnu.org; kbusch@kernel.org;
> its@irrelevant.dk; mst@redhat.com; marcel.apfelbaum@gmail.com;
> hchkuo@avery-design.com.tw; Browy, Chris <cbrowy@avery-design.com>; qemu-
> block@nongnu.org; Alistair Francis <alistair.francis@wdc.com>
> Subject: Re: [PATCH 3/3] hw/nvme: Add SPDM over DOE support
> 
> On Fri, Sep 15, 2023 at 09:27:23PM +1000, Alistair Francis wrote:
> > --- /dev/null
> > +++ b/docs/specs/spdm.rst
> > @@ -0,0 +1,56 @@
> > +======================================================
> > +QEMU Security Protocols and Data Models (SPDM) Support
> > +======================================================
> > +
> > +SPDM enables authentication, attestation and key exchange to assist in
> > +providing infrastructure security enablement. It's a standard published
> > +by the DMTF https://www.dmtf.org/standards/SPDM.
> > +
> > +Setting up a SPDM server
> [...]
> > +    $ cd spdm-emu
> > +    $ git submodule init; git submodule update --recursive
> > +    $ mkdir build; cd build
> > +    $ cmake -DARCH=x64 -DTOOLCHAIN=GCC -DTARGET=Debug -
> DCRYPTO=openssl ..
> > +    $ make -j32
> > +    $ make copy_sample_key # Build certificates, required for SPDM
> authentication.
> 
> Might be worth pointing out that certificates need to have a
> Subject Alternative Name in compliance with PCIe r6.1 sec 6.31.3,
> what to add to openssl.cnf to get one, e.g. ...
> 
>     subjectAltName =
> otherName:2.23.147;UTF8:Vendor=1b36:Device=0010:CC=010802:REV=02:SSVI
> D=1af4:SSID=1100
>     2.23.147 = ASN1:OID:2.23.147
> 
> ... and how to regenerate certificates after modifying openssl.cnf, e.g. ...
> 
>     $ openssl req -nodes -newkey ec:param.pem -keyout end_responder.key -out
> end_responder.req -sha384 -batch -subj "/CN=DMTF libspdm ECP384 responder
> cert"
>     $ openssl x509 -req -in end_responder.req -out end_responder.cert -CA
> inter.cert -CAkey inter.key -sha384 -days 3650 -set_serial 3 -extensions v3_end -
> extfile ../openssl.cnf
>     $ openssl asn1parse -in end_responder.cert -out end_responder.cert.der
>     $ cat ca.cert.der inter.cert.der end_responder.cert.der >
> bundle_responder.certchain.der
> 
> Or preferably modify upstream libspdm to automate this process,
> make it less cumbersome and error-prone.
> 
> 
> > +static bool pcie_doe_spdm_rsp(DOECap *doe_cap)
> > +{
> > +    void *req = pcie_doe_get_write_mbox_ptr(doe_cap);
> > +    uint32_t req_len = pcie_doe_get_obj_len(req) * 4;
> > +    void *rsp = doe_cap->read_mbox;
> > +    uint32_t rsp_len = SPDM_SOCKET_MAX_MESSAGE_BUFFER_SIZE;
> > +    uint32_t recvd;
> 
> Might be worth mentioning somewhere that this only implements the
> responder role.
> 
> CPUs are coming to market which contain a Trusted Security Module.
> Some of those TSMs are capable of the SPDM requester role.  Should
> qemu ever have the need to emulate a CPU containing a TSM, it may
> become necessary to add SPDM requester support.
> 
> Thanks,
> 
> Lukas

Re: [PATCH 3/3] hw/nvme: Add SPDM over DOE support

[Lukas Wunner]

On Mon, Oct 02, 2023 at 11:36:25AM +0000, Yao, Jiewen wrote:
> Comment on subjectAltName.
> 
> PCI-SIG realized that it may cause problem for certain device
> and decided to remove such requirement in future ECN.
> I don't think that is absolutely needed.

We have to follow what's in the spec.  We can't just leave out
certain elements because they might possibly maybe be removed
in the future.

PCIe r6.1 does require the Subject Alternative Name and that's
the latest version, so we follow that.

The ECN that you're referring to only exists as a draft in the
PCISIG's Review Zone Archive.

My understanding is that the Subject Alternative Name's purpose
is to eliminate certain threats in the CMA threat model:
The Subject Alternative Name is basically a signed version of the
device's identity in config space.  Without it, a different device
might misappropriate a device's certificate + private key.

If the Subject Alternative Name requirement is dropped, I would
like to know how that threat is prevented instead?

I don't quite understand what you mean by "may cause problem for
certain device".  I've asked the editor of the PCIe Base Spec why
they're considering removing the requirement and the gist of the
answer was -- I'm paraphrasing here -- that vendors thought the
requirement is generally quite narrow and perceived as a straight-jacket
and that at this point, more flexibility is desired as to the
identification scheme.  There was no mention at all of "problems
for certain devices".

Thanks,

Lukas

Re: [PATCH 1/3] hw/pci: Add all Data Object Types

[Jonathan Cameron via]

On Fri, 15 Sep 2023 21:27:21 +1000
Alistair Francis <alistair23@gmail.com> wrote:

> Add all of the defined protocols/features from the PCIe-SIG
> "Table 6-32 PCI-SIG defined Data Object Types (Vendor ID = 0001h)"

Which version of the specification?  These references can rot.
Obviously it's below, but who knows if anyone will look there ;)
It's already changed in 6.1 and the table has more entries.

I'd just change this to say, Add all Data Object Types defined in PCIe r6.0


> table.
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  include/hw/pci/pcie_doe.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/hw/pci/pcie_doe.h b/include/hw/pci/pcie_doe.h
> index 87dc17dcef..15d94661f9 100644
> --- a/include/hw/pci/pcie_doe.h
> +++ b/include/hw/pci/pcie_doe.h
> @@ -46,6 +46,8 @@ REG32(PCI_DOE_CAP_STATUS, 0)
>  
>  /* PCI-SIG defined Data Object Types - r6.0 Table 6-32 */
>  #define PCI_SIG_DOE_DISCOVERY       0x00
> +#define PCI_SIG_DOE_CMA             0x01
> +#define PCI_SIG_DOE_SECURED_CMA     0x02
>  
>  #define PCI_DOE_DW_SIZE_MAX         (1 << 18)
>  #define PCI_DOE_PROTOCOL_NUM_MAX    256