Re: [PATCH v6 3/3] hw/nvme: add nvme management interface model
On Thu, 2023-09-14 at 11:53 +0200, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Add the 'nmi-i2c' device that emulates an NVMe Management Interface
> controller.
>
> Initial support is very basic (Read NMI DS, Configuration Get).
>
> This is based on previously posted code by Padmakar Kalghatgi, Arun
> Kumar Agasar and Saurav Kumar.
>
> Reviewed-by: Jonathan Cameron
> Signed-off-by: Klaus Jensen
> ---
> hw/nvme/Kconfig | 4 +
> hw/nvme/meson.build | 1 +
> hw/nvme/nmi-i2c.c| 407
> +++
> hw/nvme/trace-events | 6 +
> 4 files changed, 418 insertions(+)
>
> diff --git a/hw/nvme/Kconfig b/hw/nvme/Kconfig
> index cfa2ab0f9d5a..e1f6360c0f4b 100644
> --- a/hw/nvme/Kconfig
> +++ b/hw/nvme/Kconfig
> @@ -2,3 +2,7 @@ config NVME_PCI
> bool
> default y if PCI_DEVICES || PCIE_DEVICES
> depends on PCI
> +
> +config NVME_NMI_I2C
> +bool
> +default y if I2C_MCTP
> diff --git a/hw/nvme/meson.build b/hw/nvme/meson.build
> index 1a6a2ca2f307..7bc85f31c149 100644
> --- a/hw/nvme/meson.build
> +++ b/hw/nvme/meson.build
> @@ -1 +1,2 @@
> system_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('ctrl.c', 'dif.c',
> 'ns.c', 'subsys.c'))
> +system_ss.add(when: 'CONFIG_NVME_NMI_I2C', if_true: files('nmi-i2c.c'))
> diff --git a/hw/nvme/nmi-i2c.c b/hw/nvme/nmi-i2c.c
> new file mode 100644
> index ..bf4648db0457
> --- /dev/null
> +++ b/hw/nvme/nmi-i2c.c
> @@ -0,0 +1,407 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * SPDX-FileCopyrightText: Copyright (c) 2023 Samsung Electronics Co., Ltd.
> + *
> + * SPDX-FileContributor: Padmakar Kalghatgi
> + * SPDX-FileContributor: Arun Kumar Agasar
> + * SPDX-FileContributor: Saurav Kumar
> + * SPDX-FileContributor: Klaus Jensen
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/crc32c.h"
> +#include "hw/registerfields.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/i2c/mctp.h"
> +#include "net/mctp.h"
> +#include "trace.h"
> +
> +/* NVM Express Management Interface 1.2c, Section 3.1 */
> +#define NMI_MAX_MESSAGE_LENGTH 4224
> +
> +#define TYPE_NMI_I2C_DEVICE "nmi-i2c"
> +OBJECT_DECLARE_SIMPLE_TYPE(NMIDevice, NMI_I2C_DEVICE)
> +
> +typedef struct NMIDevice {
> +MCTPI2CEndpoint mctp;
> +
> +uint8_t buffer[NMI_MAX_MESSAGE_LENGTH];
> +uint8_t scratch[NMI_MAX_MESSAGE_LENGTH];
> +
> +size_t len;
> +int64_t pos;
> +} NMIDevice;
> +
> +FIELD(NMI_MCTPD, MT, 0, 7)
> +FIELD(NMI_MCTPD, IC, 7, 1)
> +
> +#define NMI_MCTPD_MT_NMI 0x4
> +#define NMI_MCTPD_IC_ENABLED 0x1
> +
> +FIELD(NMI_NMP, ROR, 7, 1)
> +FIELD(NMI_NMP, NMIMT, 3, 4)
> +
> +#define NMI_NMP_NMIMT_NVME_MI 0x1
> +#define NMI_NMP_NMIMT_NVME_ADMIN 0x2
> +
> +typedef struct NMIMessage {
> +uint8_t mctpd;
> +uint8_t nmp;
> +uint8_t rsvd2[2];
> +uint8_t payload[]; /* includes the Message Integrity Check */
> +} NMIMessage;
> +
> +typedef struct NMIRequest {
> + uint8_t opc;
> + uint8_t rsvd1[3];
> + uint32_t dw0;
> + uint32_t dw1;
> + uint32_t mic;
> +} NMIRequest;
> +
> +FIELD(NMI_CMD_READ_NMI_DS_DW0, DTYP, 24, 8)
> +
> +typedef enum NMIReadDSType {
> +NMI_CMD_READ_NMI_DS_SUBSYSTEM = 0x0,
> +NMI_CMD_READ_NMI_DS_PORTS = 0x1,
> +NMI_CMD_READ_NMI_DS_CTRL_LIST = 0x2,
> +NMI_CMD_READ_NMI_DS_CTRL_INFO = 0x3,
> +NMI_CMD_READ_NMI_DS_OPT_CMD_SUPPORT = 0x4,
> +NMI_CMD_READ_NMI_DS_MEB_CMD_SUPPORT = 0x5,
> +} NMIReadDSType;
> +
> +#define NMI_STATUS_INVALID_PARAMETER 0x4
> +
> +static void nmi_scratch_append(NMIDevice *nmi, const void *buf, size_t count)
> +{
> +assert(nmi->pos + count <= NMI_MAX_MESSAGE_LENGTH);
> +
> +memcpy(nmi->scratch + nmi->pos, buf, count);
> +nmi->pos += count;
> +}
> +
> +static void nmi_set_parameter_error(NMIDevice *nmi, uint8_t bit, uint16_t
> byte)
> +{
> +/* NVM Express Management Interface 1.2c, Figure 30 */
> +struct resp {
> +uint8_t status;
> +uint8_t bit;
> +uint16_t byte;
> +};
> +
> +struct resp buf = {
> +.status = NMI_STATUS_INVALID_PARAMETER,
> +.bit = bit & 0x3,
> +.byte = byte,
> +};
> +
> +nmi_scratch_append(nmi, , sizeof(buf));
> +}
> +
> +static void nmi_set_error(NMIDevice *nmi, uint8_t status)
> +{
> +const uint8_t buf[4] = {status,};
> +
> +nmi_scratch_append(nmi, buf, sizeof(buf));
> +}
> +
> +static void nmi_handle_mi_read_nmi_ds(NMIDevice *nmi, NMIRequest *request)
> +{
> +I2CSlave *i2c = I2C_SLAVE(nmi);
> +
> +uint32_t dw0 = le32_to_cpu(request->dw0);
> +uint8_t dtyp = FIELD_EX32(dw0, NMI_CMD_READ_NMI_DS_DW0, DTYP);
> +
> +trace_nmi_handle_mi_read_nmi_ds(dtyp);
> +
> +static const uint8_t nmi_ds_subsystem[36] = {
> +0x00, /* success */
> +0x20, 0x00, /* response data length */
> +0x00, /* reserved */
> +0x00, /* number of ports */
> +0x01, /* major version */
> +0x01, /* minor version */
> +
Re: [PATCH v6 3/3] hw/nvme: add nvme management interface model
On Thu, Sep 14, 2023 at 11:53:43AM +0200, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Add the 'nmi-i2c' device that emulates an NVMe Management Interface
> controller.
>
> Initial support is very basic (Read NMI DS, Configuration Get).
>
> This is based on previously posted code by Padmakar Kalghatgi, Arun
> Kumar Agasar and Saurav Kumar.
This seems fine.
Acked-by: Corey Minyard
One question, though. You don't have any tests. Did you test invalid
packets and such? I think the logic is correct, but those are things
that are good to test. Having tests in qemu would be even better.
>
> Reviewed-by: Jonathan Cameron
> Signed-off-by: Klaus Jensen
> ---
> hw/nvme/Kconfig | 4 +
> hw/nvme/meson.build | 1 +
> hw/nvme/nmi-i2c.c| 407
> +++
> hw/nvme/trace-events | 6 +
> 4 files changed, 418 insertions(+)
>
> diff --git a/hw/nvme/Kconfig b/hw/nvme/Kconfig
> index cfa2ab0f9d5a..e1f6360c0f4b 100644
> --- a/hw/nvme/Kconfig
> +++ b/hw/nvme/Kconfig
> @@ -2,3 +2,7 @@ config NVME_PCI
> bool
> default y if PCI_DEVICES || PCIE_DEVICES
> depends on PCI
> +
> +config NVME_NMI_I2C
> +bool
> +default y if I2C_MCTP
> diff --git a/hw/nvme/meson.build b/hw/nvme/meson.build
> index 1a6a2ca2f307..7bc85f31c149 100644
> --- a/hw/nvme/meson.build
> +++ b/hw/nvme/meson.build
> @@ -1 +1,2 @@
> system_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('ctrl.c', 'dif.c',
> 'ns.c', 'subsys.c'))
> +system_ss.add(when: 'CONFIG_NVME_NMI_I2C', if_true: files('nmi-i2c.c'))
> diff --git a/hw/nvme/nmi-i2c.c b/hw/nvme/nmi-i2c.c
> new file mode 100644
> index ..bf4648db0457
> --- /dev/null
> +++ b/hw/nvme/nmi-i2c.c
> @@ -0,0 +1,407 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * SPDX-FileCopyrightText: Copyright (c) 2023 Samsung Electronics Co., Ltd.
> + *
> + * SPDX-FileContributor: Padmakar Kalghatgi
> + * SPDX-FileContributor: Arun Kumar Agasar
> + * SPDX-FileContributor: Saurav Kumar
> + * SPDX-FileContributor: Klaus Jensen
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/crc32c.h"
> +#include "hw/registerfields.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/i2c/mctp.h"
> +#include "net/mctp.h"
> +#include "trace.h"
> +
> +/* NVM Express Management Interface 1.2c, Section 3.1 */
> +#define NMI_MAX_MESSAGE_LENGTH 4224
> +
> +#define TYPE_NMI_I2C_DEVICE "nmi-i2c"
> +OBJECT_DECLARE_SIMPLE_TYPE(NMIDevice, NMI_I2C_DEVICE)
> +
> +typedef struct NMIDevice {
> +MCTPI2CEndpoint mctp;
> +
> +uint8_t buffer[NMI_MAX_MESSAGE_LENGTH];
> +uint8_t scratch[NMI_MAX_MESSAGE_LENGTH];
> +
> +size_t len;
> +int64_t pos;
> +} NMIDevice;
> +
> +FIELD(NMI_MCTPD, MT, 0, 7)
> +FIELD(NMI_MCTPD, IC, 7, 1)
> +
> +#define NMI_MCTPD_MT_NMI 0x4
> +#define NMI_MCTPD_IC_ENABLED 0x1
> +
> +FIELD(NMI_NMP, ROR, 7, 1)
> +FIELD(NMI_NMP, NMIMT, 3, 4)
> +
> +#define NMI_NMP_NMIMT_NVME_MI 0x1
> +#define NMI_NMP_NMIMT_NVME_ADMIN 0x2
> +
> +typedef struct NMIMessage {
> +uint8_t mctpd;
> +uint8_t nmp;
> +uint8_t rsvd2[2];
> +uint8_t payload[]; /* includes the Message Integrity Check */
> +} NMIMessage;
> +
> +typedef struct NMIRequest {
> + uint8_t opc;
> + uint8_t rsvd1[3];
> + uint32_t dw0;
> + uint32_t dw1;
> + uint32_t mic;
> +} NMIRequest;
> +
> +FIELD(NMI_CMD_READ_NMI_DS_DW0, DTYP, 24, 8)
> +
> +typedef enum NMIReadDSType {
> +NMI_CMD_READ_NMI_DS_SUBSYSTEM = 0x0,
> +NMI_CMD_READ_NMI_DS_PORTS = 0x1,
> +NMI_CMD_READ_NMI_DS_CTRL_LIST = 0x2,
> +NMI_CMD_READ_NMI_DS_CTRL_INFO = 0x3,
> +NMI_CMD_READ_NMI_DS_OPT_CMD_SUPPORT = 0x4,
> +NMI_CMD_READ_NMI_DS_MEB_CMD_SUPPORT = 0x5,
> +} NMIReadDSType;
> +
> +#define NMI_STATUS_INVALID_PARAMETER 0x4
> +
> +static void nmi_scratch_append(NMIDevice *nmi, const void *buf, size_t count)
> +{
> +assert(nmi->pos + count <= NMI_MAX_MESSAGE_LENGTH);
> +
> +memcpy(nmi->scratch + nmi->pos, buf, count);
> +nmi->pos += count;
> +}
> +
> +static void nmi_set_parameter_error(NMIDevice *nmi, uint8_t bit, uint16_t
> byte)
> +{
> +/* NVM Express Management Interface 1.2c, Figure 30 */
> +struct resp {
> +uint8_t status;
> +uint8_t bit;
> +uint16_t byte;
> +};
> +
> +struct resp buf = {
> +.status = NMI_STATUS_INVALID_PARAMETER,
> +.bit = bit & 0x3,
> +.byte = byte,
> +};
> +
> +nmi_scratch_append(nmi, , sizeof(buf));
> +}
> +
> +static void nmi_set_error(NMIDevice *nmi, uint8_t status)
> +{
> +const uint8_t buf[4] = {status,};
> +
> +nmi_scratch_append(nmi, buf, sizeof(buf));
> +}
> +
> +static void nmi_handle_mi_read_nmi_ds(NMIDevice *nmi, NMIRequest *request)
> +{
> +I2CSlave *i2c = I2C_SLAVE(nmi);
> +
> +uint32_t dw0 = le32_to_cpu(request->dw0);
> +uint8_t dtyp = FIELD_EX32(dw0, NMI_CMD_READ_NMI_DS_DW0, DTYP);
> +
> +trace_nmi_handle_mi_read_nmi_ds(dtyp);
> +
> +static const uint8_t nmi_ds_subsystem[36] =
