On Wed, 31 May 2023 13:47:44 +0200
Klaus Jensen <i...@irrelevant.dk> wrote:
> From: Klaus Jensen <k.jen...@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.jen...@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);
> +}
> +
