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

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

On Tue, 25 Apr 2023 08:35:40 +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>

I don't have time to do too much spec diving so this is very superficial...

Mostly commenting because it gave me a build error.

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

> +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;
> +
> +    trace_nmi_handle_mi_config_get(identifier);
> +
> +    switch (identifier) {
> +    case NMI_CMD_CONFIGURATION_GET_SMBUS_FREQ:
> +        buf = (uint8_t[]) {
> +            0x0, 0x1, 0x0, 0x0,
> +        };
> +
> +        break;
> +
> +    case NMI_CMD_CONFIGURATION_GET_HEALTH_STATUS_CHANGE:
> +        buf = (uint8_t[]) {
> +            0x0, 0x0, 0x0, 0x0,
> +        };
> +
> +        break;
> +
> +    case NMI_CMD_CONFIGURATION_GET_MCTP_TRANSMISSION_UNIT:
> +        buf = (uint8_t[]) {
> +            0x0, 0x40, 0x0, 0x0,
> +        };
> +
> +        break;

No default, which gave me a build error as buf is uninitialized.

> +    }
> +
> +    memcpy(nmi->scratch + nmi->pos, buf, 4);
> +    nmi->pos += 4;
> +}
> +
> +enum {
> +    NMI_CMD_READ_NMI_DS         = 0x0,
> +    NMI_CMD_CONFIGURATION_GET   = 0x4,
> +};
> +


> +static size_t nmi_get_message_types(MCTPI2CEndpoint *mctp, uint8_t *data,
> +                                    size_t maxlen)
> +{
> +    uint8_t buf[] = {
> +        0x0, 0x1, 0x4,

PLDM?  Are you using that so far?  Maybe keep it for when you
add PLDM support?

> +    };
> +
> +    memcpy(data, buf, sizeof(buf));
> +
> +    return sizeof(buf);
> +}