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

[Philippe Mathieu-Daudé]

Hi Alistair, Wilfred,

On 7/3/24 01:58, 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>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Acked-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   docs/specs/index.rst        |   1 +
>   docs/specs/spdm.rst         | 122 ++++++++++++++++++++++++++++++++++++
>   include/hw/pci/pci_device.h |   5 ++
>   include/hw/pci/pcie_doe.h   |   3 +
>   hw/nvme/ctrl.c              |  53 ++++++++++++++++
>   5 files changed, 184 insertions(+)
>   create mode 100644 docs/specs/spdm.rst


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

Why not name it 'spdm_socket' like 'spdm_port' earlier?

>   };


>   static bool nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
>   {
>       ERRP_GUARD();
> @@ -8126,6 +8149,24 @@ 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 ) {
> +            error_setg(errp, "Failed to connect to SPDM socket");

spdm_socket_connect() already sets errp in case of failure.

> +            return -ENOTSUP;

This function returns a boolean, so this return value is casted
to 'true'. IIUC true means success, so this is incorrect.

> +        }
> +    }
> +
>       if (n->params.cmb_size_mb) {
>           nvme_init_cmb(n, pci_dev);
>       }
> @@ -8412,6 +8453,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(),
>   };

Regards,

Phil.