Re: [PATCH v3 3/3] hw/nvme: add nvme management interface model

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Wed, 31 May 2023 13:47:44 +0200
Klaus Jensen <its@irrelevant.dk> wrote:

> From: Klaus Jensen <k.jensen@samsung.com>
> 
> 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.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>

A few comments inline - I dug into the specs this time so some
new questions.

Jonathan

> diff --git a/hw/nvme/nmi-i2c.c b/hw/nvme/nmi-i2c.c
> new file mode 100644
> index 000000000000..38e43e48fa51
> --- /dev/null
> +++ b/hw/nvme/nmi-i2c.c
> @@ -0,0 +1,367 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * SPDX-FileCopyrightText: Copyright (c) 2022 Samsung Electronics Co., Ltd.

Update to 2023?

> +typedef struct NMIRequest {
> +   uint8_t opc;
> +   uint8_t rsvd1[3];
> +   uint32_t dw0;
> +   uint32_t dw1;
> +   uint32_t mic;
> +} NMIRequest;
> +
> +typedef struct NMIResponse {
> +    uint8_t status;
> +    uint8_t response[3];
> +    uint8_t payload[]; /* includes the Message Integrity Check */
> +} NMIResponse;

Not used.

> +
> +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_CMD_SUPPORT     = 0x4,
> +    NMI_CMD_READ_NMI_DS_MEB_CMD_SUPPORT = 0x5,
> +} NMIReadDSType;
> +
> +static void nmi_set_parameter_error(NMIDevice *nmi, uint8_t bit, uint16_t byte)
> +{
> +    nmi->scratch[nmi->pos++] = 0x4;
> +    nmi->scratch[nmi->pos++] = bit;

Mask bit to only 3 bits?

> +    nmi->scratch[nmi->pos++] = (byte >> 4) & 0xf;
> +    nmi->scratch[nmi->pos++] = byte & 0xf;

Not following how byte is split up.  Figure 29 in 1.1d suggests it should be
in bits 23:08 and this doesn't match that.  Perhaps a reference given I guess
I'm looking at wrong thing.

> +}

> +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 = (dw0 >> 24) & 0xf;
> +    uint8_t *buf;
> +    size_t len;
> +
> +    trace_nmi_handle_mi_read_nmi_ds(dtyp);
> +
> +    static uint8_t nmi_ds_subsystem[36] = {
> +        0x00,       /* success */
> +        0x20,       /* response data length */

Not the easiest datasheet to read, but I think length is 2 bytes.
Figure 93 in the 1.2b NMI spec has it as bits 15:00 in
a 24 bit field. Ah. I see this is 1.1 definition, in that it's
the same but in figure 89.


> +        0x00, 0x00, /* reserved */
> +        0x00,       /* number of ports */
> +        0x01,       /* major version */
> +        0x01,       /* minor version */
> +    };
> +
> +    static uint8_t nmi_ds_ports[36] = {
> +        0x00,       /* success */
> +        0x20,       /* response data length */

As above - this is 2 bytes.

> +        0x00, 0x00, /* reserved */
> +        0x02,       /* port type (smbus) */
> +        0x00,       /* reserved */
> +        0x40, 0x00, /* maximum mctp transission unit size (64 bytes) */
> +        0x00, 0x00, 0x00, 0x00, /* management endpoint buffer size */
> +        0x00, 0x00, /* vpd i2c address/freq */

Give separate bytes for the two things, why not just have two lines and
a comment for each one?

> +        0x00, 0x01, /* management endpoint i2c address/freq */

Same here though second byte isn't anything to do with frequency. Documnted
as NVMe Basic Management and indicates if that command is supported.

> +    };
> +
> +    static uint8_t nmi_ds_empty[8] = {
> +        0x00,       /* success */
> +        0x02,       /* response data length */
> +        0x00, 0x00, /* reserved */
> +        0x00, 0x00, /* number of controllers */
A reference for this would be good as I'm not sure it's defined in the NMI spec.
Is it the controller list from 4.4.1 in the main NVME 2.0 spec?


> +        0x00, 0x00, /* padding */
> +    };
> +
> +    switch (dtyp) {
> +    case NMI_CMD_READ_NMI_DS_SUBSYSTEM:
> +        len = 36;
sizeof(nmi_ds_subsystem)  Might as well keep that 36 in just one place.
> +        buf = nmi_ds_subsystem;
> +
> +        break;
> +
> +    case NMI_CMD_READ_NMI_DS_PORTS:
> +        len = 36;
> +        buf = nmi_ds_ports;
same here?
> +
> +        /* patch in the i2c address of the endpoint */
> +        buf[14] = i2c->address;

If you have multiple instances of this as they will
race over the static buffer.  Just make it non static and add
a comment on why.

> +
> +        break;
> +
> +    case NMI_CMD_READ_NMI_DS_CTRL_LIST:
> +    case NMI_CMD_READ_NMI_DS_CMD_SUPPORT:
> +    case NMI_CMD_READ_NMI_DS_MEB_CMD_SUPPORT:
> +        len = 8;
> +        buf = nmi_ds_empty;
> +
> +        break;
> +
> +    default:
> +        nmi_set_parameter_error(nmi, offsetof(NMIRequest, dw0) + 4, 0);
> +
> +        return;
> +    }
> +
> +    memcpy(nmi->scratch + nmi->pos, buf, len);
> +    nmi->pos += len;
> +}
> +
> +enum {
> +    NMI_CMD_CONFIGURATION_GET_SMBUS_FREQ                = 0x1,
> +    NMI_CMD_CONFIGURATION_GET_HEALTH_STATUS_CHANGE      = 0x2,
> +    NMI_CMD_CONFIGURATION_GET_MCTP_TRANSMISSION_UNIT    = 0x3,
> +};
> +
> +static void nmi_handle_mi_config_get(NMIDevice *nmi, NMIRequest *request)
> +{
> +    uint32_t dw0 = le32_to_cpu(request->dw0);
> +    uint8_t identifier = dw0 & 0xff;
> +    uint8_t *buf;
> +
> +    static uint8_t smbus_freq[4] = {
> +        0x00,               /* success */
> +        0x01, 0x00, 0x00,   /* 100 kHz */
> +    };
> +
> +    static uint8_t mtu[4] = {
> +        0x00,               /* success */
> +        0x40, 0x00, 0x00,   /* 64 */

2 bytes only I think with another reserved.
Figure 66 in 1.1d

> +    };
> +
> +    trace_nmi_handle_mi_config_get(identifier);
> +
> +    switch (identifier) {
> +    case NMI_CMD_CONFIGURATION_GET_SMBUS_FREQ:
> +        buf = smbus_freq;
> +        break;
> +
> +    case NMI_CMD_CONFIGURATION_GET_MCTP_TRANSMISSION_UNIT:
> +        buf = mtu;
> +        break;
> +
> +    default:
> +        nmi_set_parameter_error(nmi, 0x0, offsetof(NMIRequest, dw0));
> +        return;
> +    }
> +
> +    memcpy(nmi->scratch + nmi->pos, buf, 4);
> +    nmi->pos += 4;
> +}
> +
> +enum {
> +    NMI_CMD_READ_NMI_DS         = 0x0,
> +    NMI_CMD_CONFIGURATION_GET   = 0x4,
> +};
> +
> +static void nmi_handle_mi(NMIDevice *nmi, NMIMessage *msg)
> +{
> +    NMIRequest *request = (NMIRequest *)msg->payload;
> +
> +    trace_nmi_handle_mi(request->opc);
> +
> +    switch (request->opc) {
> +    case NMI_CMD_READ_NMI_DS:
> +        nmi_handle_mi_read_nmi_ds(nmi, request);
> +        break;
> +
> +    case NMI_CMD_CONFIGURATION_GET:
> +        nmi_handle_mi_config_get(nmi, request);
> +        break;
> +
> +    default:
> +        nmi_set_parameter_error(nmi, 0x0, 0x0);
> +        fprintf(stderr, "nmi command 0x%x not handled\n", request->opc);
> +
> +        break;
> +    }
> +}
> +
> +enum {
> +    NMI_MESSAGE_TYPE_NMI = 0x1,
> +};
> +



> +static size_t nmi_get_types(MCTPI2CEndpoint *mctp, const uint8_t **data)
> +{
> +    static const uint8_t buf[] = {
> +        0x0, 0x1, MCTP_MESSAGE_TYPE_NMI,
> +    };

Perhaps a comment that the 0x1 is the length.  Or you could define
a structure for this with a variable length final field?

Also, should the control type be reported?  I couldn't find a statement
that it shouldn't and it has a message ID.

> +
> +    *data = buf;
> +
> +    return sizeof(buf);
> +}
> +