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

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

On Wed, 23 Aug 2023 11:22:00 +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>

Hi Klaus

Minor suggestions inline.  Either way given how trivial they are

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> diff --git a/hw/nvme/nmi-i2c.c b/hw/nvme/nmi-i2c.c
> new file mode 100644
> index 000000000000..9040ba28a87c
> --- /dev/null
> +++ b/hw/nvme/nmi-i2c.c
> @@ -0,0 +1,418 @@
...

> +#define NMI_MAX_MESSAGE_LENGTH 4224

Spec reference or similar would be good for this.

...

> +
> +static void nmi_set_error(NMIDevice *nmi, uint8_t status)
> +{
> +    uint8_t buf[4] = {};
> +
> +    buf[0] = status;

Could just do

    uint8_t buf[4] = { status, };

> +
> +    memcpy(nmi->scratch + nmi->pos, buf, 4);
> +    nmi->pos += 4;
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 = (dw0 >> 24) & 0xf;

Maybe FIELD_EX32() with appropriate defines?

> +    uint8_t *buf;
> +    size_t len;
> +
> +    trace_nmi_handle_mi_read_nmi_ds(dtyp);
> +
> +    static 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 */
> +    };
> +
> +    /* cannot be static since we need to patch in the i2c address */
> +    uint8_t nmi_ds_ports[36] = {
> +        0x00,       /* success */
> +        0x20, 0x00, /* response data length */
> +        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,       /* vpd i2c address */
> +        0x00,       /* vpd i2c frequency */
> +        0x00,       /* management endpoint i2c address */
> +        0x01,       /* management endpoint i2c frequency */
> +        0x00,       /* nvme basic management command NOT supported */
> +    };
> +
> +    /**
> +     * Controller Information is zeroed, since there are no associated
> +     * controllers at this point.
> +     */
> +    static uint8_t nmi_ds_ctrl[36] = {};
> +
> +    /**
> +     * For the Controller List, Optionally Supported Command List and
> +     * Management Endpoint Buffer Supported Command List data structures.
> +     *
> +     * The Controller List data structure is defined in the NVM Express Base
> +     * Specification, revision 2.0b, Figure 134.
> +     */
> +    static uint8_t nmi_ds_empty[6] = {
> +        0x00,       /* success */
> +        0x02,       /* response data length */
> +        0x00, 0x00, /* reserved */
> +        0x00, 0x00, /* number of entries (zero) */
> +    };
> +
> +    switch (dtyp) {
> +    case NMI_CMD_READ_NMI_DS_SUBSYSTEM:
> +        len = sizeof(nmi_ds_subsystem);
> +        buf = nmi_ds_subsystem;
> +
> +        break;
> +
> +    case NMI_CMD_READ_NMI_DS_PORTS:
> +        len = sizeof(nmi_ds_ports);
> +        buf = nmi_ds_ports;
> +
> +        /* patch in the i2c address of the endpoint */
> +        buf[14] = i2c->address;
> +
> +        break;
> +
> +    case NMI_CMD_READ_NMI_DS_CTRL_INFO:
> +        len = sizeof(nmi_ds_ctrl);
> +        buf = nmi_ds_ctrl;
> +
> +        break;
> +
> +    case NMI_CMD_READ_NMI_DS_CTRL_LIST:
> +    case NMI_CMD_READ_NMI_DS_OPT_CMD_SUPPORT:
> +    case NMI_CMD_READ_NMI_DS_MEB_CMD_SUPPORT:
> +        len = sizeof(nmi_ds_empty);
> +        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;
> +}
> +

> +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 */
> +    };

const for these? Same for other similar buffers.

> +
> +    static uint8_t mtu[4] = {
> +        0x00,       /* success */
> +        0x40, 0x00, /* 64 */
> +        0x00,       /* reserved */
> +    };
> +
> +    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;
> +}
> +



> +static void nmi_handle(MCTPI2CEndpoint *mctp)
> +{
> +    NMIDevice *nmi = NMI_I2C_DEVICE(mctp);
> +    NMIMessage *msg = (NMIMessage *)nmi->buffer;
> +    uint32_t crc;
> +    uint8_t nmimt;
> +
> +    uint8_t buf[] = {

const?

> +        msg->mctpd,
> +        FIELD_DP8(msg->nmp, NMI_NMP, ROR, 1),
> +        0x0, 0x0,
> +    };
> +
> +    if (FIELD_EX8(msg->mctpd, NMI_MCTPD, MT) != NMI_MCTPD_MT_NMI) {
> +        goto drop;
> +    }
> +
> +    if (FIELD_EX8(msg->mctpd, NMI_MCTPD, IC) != NMI_MCTPD_IC_ENABLED) {
> +        goto drop;
> +    }
> +
> +    memcpy(nmi->scratch, buf, sizeof(buf));

Jonathan