qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH: v3 1/2] add mi device in qemu


From: Padmakar Kalghatgi
Subject: Re: [RFC PATCH: v3 1/2] add mi device in qemu
Date: Fri, 1 Oct 2021 20:45:28 +0530
User-agent: Mutt/1.5.21 (2010-09-15)

On Wed, Sep 29, 2021 at 06:37:54AM +0200, Klaus Jensen wrote:
On Aug  3 12:54, Padmakar Kalghatgi wrote:
From: padmakar <p.kalghatgi@samsung.com>

This patch contains the implementation of certain commands
of nvme-mi specification.The MI commands are useful to
manage/configure/monitor the device.Eventhough the MI commands
can be sent via the inband NVMe-MI send/recieve commands, the idea
here is to emulate the sideband interface for MI.

The changes here includes the interface for i2c/smbus
for nvme-mi protocol. We have used i2c address of 0x15
using which the guest VM can send and recieve the nvme-mi
commands. Since the nvme-mi device uses the I2C_SLAVE as
parent, we have used the send and recieve callbacks by
which the nvme-mi device will get the required notification.
With the callback approach, we have eliminated the use of
threads.

One needs to specify the following command in the launch to
specify the nvme-mi device, link to nvme and assign the i2c address.
<-device nvme-mi,nvme=nvme0,address=0x15>

This module makes use of the NvmeCtrl structure of the nvme module,
to fetch relevant information of the nvme device which are used in
some of the mi commands. Eventhough certain commands might require
modification to the nvme module, currently we have currently refrained
from making changes to the nvme module.

cmd-name                              cmd-type
*************************************************
read nvme-mi ds                        nvme-mi
configuration set                      nvme-mi
configuration get                      nvme-mi
vpd read                               nvme-mi
vpd write                              nvme-mi
controller Health Staus Poll           nvme-mi
nvme subsystem health status poll      nvme-mi
identify                               nvme-admin
get log page                           nvme-admin
get features                           nvme-admin

Signed-off-by: Padmakar Kalghatgi <p.kalghatgi@samsung.com>
Reviewed-by: Klaus Birkelund Jensen <k.jensen@samsung.com>
Reviewed-by: Jaegyu Choi <jg123.choi@samsung.com>

v3
-- rebased on master

---
 hw/nvme/meson.build |   2 +-
 hw/nvme/nvme-mi.c   | 650 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/nvme/nvme-mi.h   | 297 ++++++++++++++++++++++++
 3 files changed, 948 insertions(+), 1 deletion(-)
 create mode 100644 hw/nvme/nvme-mi.c
 create mode 100644 hw/nvme/nvme-mi.h


Some general comments.

* Please be consistent about MI vs Mi in naming. I have no preference,
 but NvmeMi is probably most aligned with existing style.

* hw/nvme generally does not mix declarations and code. Please move
 variables declarations to the top of their scope. And please get rid
 of the hungarian notation ({b,l,u,...}VarName naming) ;)

* I'd like that the header was cleaned up to not include stuff that
 isn't used.

* I understand that you'd like to not impact the hw/nvme/ctrl.c device
 too much, but the Identify, Get Log Page and Get Feature "passthru"
 commands could really benefit from using the same code as in
 hw/nvme/ctrl.c - this requires a bit of refactoring such that the
 data can be returned explicitly instead of being directly DMA'ed to
 the host.

* This is not compliant with the MCTP I2C/SMBus binding spec. The spec
 states that all transactions are based on the SMBus Block Write bus
 protocol. This means that responses require that the device (which is
 defined as an I2C slave must itself master the bus and do a Block
 Write to the message originator (the address of which is contained in
 the fourth byte of the packet).

diff --git a/hw/nvme/meson.build b/hw/nvme/meson.build
index 3cf4004..8dac50e 100644
--- a/hw/nvme/meson.build
+++ b/hw/nvme/meson.build
@@ -1 +1 @@
-softmmu_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('ctrl.c', 'dif.c', 
'ns.c', 'subsys.c'))
+softmmu_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('ctrl.c', 'dif.c', 
'ns.c', 'subsys.c', 'nvme-mi.c'))
diff --git a/hw/nvme/nvme-mi.c b/hw/nvme/nvme-mi.c
new file mode 100644
index 0000000..a90ce90
--- /dev/null
+++ b/hw/nvme/nvme-mi.c
@@ -0,0 +1,650 @@
+/*
+ * QEMU NVMe-MI Controller
+ *
+ * Copyright (c) 2021, Samsung Electronics co Ltd.
+ *
+ * Written by Padmakar Kalghatgi <p.kalghatgi@samsung.com>
+ *            Arun Kumar Agasar <arun.kka@samsung.com>
+ *            Saurav Kumar <saurav.29@partner.samsung.com>
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ */
+
+/**
+ * Reference Specs: 
https://protect2.fireeye.com/v1/url?k=b5aa98b4-d54805e9-b5ab13fb-000babd9f1ba-0224a06b55b4fe71&q=1&e=f0445d9e-6d0d-4fac-8d43-1785c5d98f8e&u=http%3A%2F%2Fwww.nvmexpress.org%2F,
+ *
+ *
+ * Usage
+ * -----
+ * The nvme-mi device has to be used along with nvme device only
+ *
+ * Add options:
+ *    -device  nvme-mi,nvme=<nvme id>,address=0x15",

Considering that NVMe-MI can run with various MCTP bindings, I think
this particular instance of it should be called 'nvme-mi-i2c'.
Supporting VDM on a PCIe binding is probably not really a goal at all,
but it's better to be explicit about this I think.

+ *
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-core.h"
+#include "hw/block/block.h"
+#include "hw/pci/msix.h"
+#include "nvme.h"
+#include "nvme-mi.h"
+#include "qemu/crc32c.h"
+
+#define NVME_TEMPERATURE 0x143
+#define NVME_TEMPERATURE_WARNING 0x157
+#define NVME_TEMPERATURE_CRITICAL 0x175
+

These are from hw/nvme/ctrl.c - they should be moved to hw/nvme/nvme.h
and shared (separate patch please)

+static void nvme_mi_send_resp(NvmeMiCtrl *ctrl_mi, uint8_t *resp, uint32_t 
size)
+{
+    uint32_t crc_value = crc32c(0xFFFFFFFF, resp, size);
+    size += 4;

Prefer that a sizeof(<some header struct>) instead of the magic number.

+    uint32_t offset = 0;
+    uint32_t ofst = 0;
+    uint32_t som = 1;
+    uint32_t eom = 0;
+    uint32_t pktseq = 0;
+    uint32_t mtus = ctrl_mi->mctp_unit_size;
+    while (size > 0) {
+        uint32_t sizesent = size > mtus ? mtus : size;

size_t and maybe just a MIN().

+        size -= sizesent;
+        eom = size > 0 ? 0 : 1;
+        g_autofree uint8_t *buf = (uint8_t *)g_malloc0(sizesent + 8);
+        buf[2] = sizesent + 5;
+        buf[7] = (som << 7) | (eom << 6) | (pktseq << 5);
+        som = 0;
+        memcpy(buf + 8, resp + offset, sizesent);
+        offset += sizesent;
+        if (size <= 0) {
+            memcpy(buf + 8 + offset , &crc_value, sizeof(crc_value));
+        }
+        memcpy(ctrl_mi->misendrecv.sendrecvbuf + ofst, buf, sizesent + 8);
+        ofst += sizesent + 8;
+    }
+}

I think you are missing the PEC byte here.

My back-of-the-envelope calculation tells me that the 5000 byte
sendrecvbuf *should* be big enough to handle any message with MTUS of
64. But it is still arbitrary I think. See below on the i2c_recv where I
suggest that you assemble the packets on the fly instead of staging all
of them in the sendrecvbuf.

+
+static void nvme_mi_resp_hdr_init(NvmeMIResponse *resp, bool bAdminCommand)

Hungarian Notation is definitely not a part of the QEMU Style Guide :p

+static void nvme_mi_read_nvme_mi_ds(NvmeMiCtrl *ctrl_mi, NvmeMIRequest *req)
+{
+    ReadNvmeMiDs ds;
+    ds.cntrlid = req->dword0 & 0xFFFF;
+    ds.portlid = (req->dword0 & 0xFF0000) >> 16;
+    ds.dtyp = (req->dword0 & ~0xFF) >> 24;
+    int dtyp = ds.dtyp;
+    switch (dtyp) {
+    case NVM_SUBSYSTEM_INFORMATION:
+        nvme_mi_nvm_subsys_ds(ctrl_mi, req);
+        break;
+    case OPT_SUPP_CMD_LIST:
+        nvme_mi_opt_supp_cmd_list(ctrl_mi, req);
+        break;
+    }

The Port Information is pretty important, isnt it? That's what should be used
to negotiate the transmission unit size.

+static void nvme_mi_admin_command(NvmeMiCtrl *ctrl_mi, void* req_arg)
+{
+    uint8_t *msg  = ((uint8_t *)req_arg);
+    NvmeMiMessageHeader msghdr;
+    memcpy(&msghdr, msg, sizeof(NvmeMiMessageHeader));
+    if (msghdr.nmimt == 1) {
+        NvmeMIRequest *req = (NvmeMIRequest *) (msg);
+        switch (req->opc) {
+        case READ_NVME_MI_DS:
+            nvme_mi_read_nvme_mi_ds(ctrl_mi, req);
+            break;
+        case CHSP:
+            nvme_mi_controller_health_ds(ctrl_mi, req);
+            break;
+        case NVM_SHSP:
+            nvme_mi_nvm_subsys_health_status_poll(ctrl_mi, req);
+            break;
+        case CONFIGURATION_SET:
+            nvme_mi_configuration_set(ctrl_mi, req);
+            break;
+        case CONFIGURATION_GET:
+            nvme_mi_configuration_get(ctrl_mi, req);
+            break;
+        case VPD_READ:
+            nvme_mi_vpd_read(ctrl_mi, req);
+            break;
+        case VPD_WRITE:
+            nvme_mi_vpd_write(ctrl_mi, req, msg);
+            break;
+        default:
+        {
+            NvmeMIResponse resp;
+            nvme_mi_resp_hdr_init(&resp, false);
+            resp.status = INVALID_COMMAND_OPCODE;
+            nvme_mi_send_resp(ctrl_mi, (uint8_t *)&resp, sizeof(resp));
+            break;
+        }
+        }
+    } else {
+        NvmeAdminMIRequest *req = (NvmeAdminMIRequest *) (msg);
+        switch  (req->opc) {
+        case NVME_ADM_MI_CMD_IDENTIFY:
+            nvme_mi_admin_identify(ctrl_mi, req);
+            break;
+        case NVME_ADM_MI_CMD_GET_LOG_PAGE:
+            nvme_mi_admin_get_log_page(ctrl_mi, req);
+            break;
+        case NVME_ADM_MI_CMD_GET_FEATURES:
+            nvme_mi_admin_get_features(ctrl_mi, req);
+            break;

It would make sense that the nvme device functions were refactored so
they are reused here.

+        default:
+        {
+            NvmeMIResponse resp;
+            nvme_mi_resp_hdr_init(&resp, true);
+            resp.status = INVALID_COMMAND_OPCODE;
+            nvme_mi_send_resp(ctrl_mi, (uint8_t *)&resp, sizeof(resp));
+            break;
+        }
+        }
+    }
+
+    return;
+}
+
+static uint8_t nvme_mi_i2c_recv(I2CSlave *s)
+{
+    NvmeMiCtrl *mictrl = (NvmeMiCtrl *)s;
+    NvmeMiSendRecvStruct *misendrecv = &mictrl->misendrecv;
+    if (misendrecv->bsyncflag == true) {
+        return -1;
+    }
+    return misendrecv->sendrecvbuf[misendrecv->sendlen++];
+}

As I mention at the beginning, this is not how responses are "sent".
Responses (which is another MCTP transaction) must be done by the device
mastering the I2C bus and doing a "master write" to the message
originator.

+
+static int nvme_mi_i2c_send(I2CSlave *s, uint8_t data)
+{
+    NvmeMiCtrl *mictrl = (NvmeMiCtrl *)s;
+    NvmeMiSendRecvStruct *misendrecv = &mictrl->misendrecv;
+
+    misendrecv->bsyncflag = true;
+    misendrecv->sendlen = 0;
+    switch (misendrecv->len) {
+    case 1:

Initially I thought that this should have been 2, but the address byte
is not part of the data that the device receives.

+        misendrecv->total_len = data;
+        break;
+    case 6:
+        misendrecv->eom = (data & 0x40) >> 6;

I think (data >> 6) & 0x1 is slightly more clear.

+        break;
+    }
+    misendrecv->sendrecvbuf[++misendrecv->len] = data;
+    if (misendrecv->len == misendrecv->total_len + 3) {
+        misendrecv->cmdbuffer = (uint8_t *)g_realloc(misendrecv->cmdbuffer,
+                                                     misendrecv->len - 5);

You need to track the current length of the entire message (not just the
packets), otherwise, in this case you just realloc to the size of the current
packet.

+        memcpy(misendrecv->cmdbuffer + misendrecv->offset,
+               misendrecv->sendrecvbuf + 8, misendrecv->len - 5);
+
+        misendrecv->offset = misendrecv->len - 5;

Think the offset must be +='ed here.

+        misendrecv->total_len = 0;
+        misendrecv->len = 0;
+
+        if (misendrecv->eom == 1) {
+            nvme_mi_admin_command(mictrl, misendrecv->cmdbuffer);
+            misendrecv->offset = 0;
+            misendrecv->bsyncflag = false;
+        }
+    }
+    return 0;
+}

This is a little complicated to read. It could probably benefit from
being "state-machined" up a bit and instead of reading everything into a
buffer and copying out at packet boundaries, just append the actual
payloads. Maybe something along these lines (totally
untested/uncompiled, but you get the idea):

   struct state {
     uint8_t buf[4224];
     int pos, pktpos, mode;
   };

   static int i2c_send(I2CSlave *s, uint8_t data)
   {
     state->pktpos++;

     switch (state->mode) {
     case READ_MCTP_HEADER:
       if (state->pktpos == 2) {
         state->pktlen = data;
         return;
       } else if (state->pktpos == 7) {
         state->eom = (data >> 6) & 0x1;
         state->mode = READ_PACKET;
         return;
       }

       /* ignore data */
       return;

     case READ_PACKET:
       state->buf[state->pos++] = data;
       if (state->pktpos == state->pktlen + 3) {
         state->mode = READ_PEC;
       }

     case READ_PEC:
       /* ignore */

       /* we are done with this packet, so reset state and expect the next one 
*/
       state->mode = READ_MCTP_HEADER;
       state->pktpos = 0;

       if (state->eom) {
         /* we have a full message */
         process(state->buf);

         /* reset state */
         state->pos = 0;
       }

       return;
     }
   }

I'm not sure if there is any benefit to implementing the i2c event callback.
Maybe.

+
+static void nvme_mi_realize(DeviceState *dev, Error **errp)
+{
+    NvmeMiCtrl *s = (NvmeMiCtrl *)(dev);
+
+    s->smbus_freq = 100;
+    s->mctp_unit_size = 64;
+}
+static Property nvme_mi_props[] = {
+     DEFINE_PROP_LINK("nvme", NvmeMiCtrl, n, TYPE_NVME,
+                     NvmeCtrl *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void nvme_mi_class_init(ObjectClass *oc, void *data)
+{
+    I2CSlaveClass *k = I2C_SLAVE_CLASS(oc);
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = nvme_mi_realize;
+    k->recv = nvme_mi_i2c_recv;
+    k->send = nvme_mi_i2c_send;
+
+    device_class_set_props(dc, nvme_mi_props);
+}
+
+static const TypeInfo nvme_mi = {
+    .name = TYPE_NVME_MI,
+    .parent = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(NvmeMiCtrl),
+    .class_init = nvme_mi_class_init,
+};
+
+static void register_types(void)
+{
+    type_register_static(&nvme_mi);
+}
+
+type_init(register_types);
diff --git a/hw/nvme/nvme-mi.h b/hw/nvme/nvme-mi.h
new file mode 100644
index 0000000..92a20e6
--- /dev/null
+++ b/hw/nvme/nvme-mi.h
@@ -0,0 +1,297 @@
+/*
+ * QEMU NVMe-MI
+ *
+ * Copyright (c) 2021 Samsung Electronics Co., Ltd.
+ *
+ * Authors:
+ *   Padmakar Kalghatgi      <p.kalghatgi@samsung.com>
+ *   Arun Kumar Agasar       <arun.kka@samsung.com>
+ *   Saurav Kumar            <saurav.29@partner.samsung.com>
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ */
+
+#ifndef NVME_MI_H
+#define NVME_MI_H
+
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+#include <stdint.h>
+#include <stdbool.h>
+#include "hw/i2c/i2c.h"
+
+#define TYPE_NVME_MI "nvme-mi"
+
+#define NVM_SUBSYSTEM_INFORMATION 0
+#define PORT_INFORMATION 1
+#define CONTROLLER_LIST 2
+#define CONTROLLER_INFORMATION 3
+#define OPT_SUPP_CMD_LIST 4
+#define MGMT_EPT_BUFF_CMD_SUPP_LIST 5
+
+
+typedef struct NvmeMiSendRecvStruct {
+   uint32_t sendlen;
+   uint8_t len;
+   uint8_t total_len;
+   uint32_t offset;
+   uint8_t eom;
+   bool bsyncflag;
+   u_char sendrecvbuf[5000];

5000 seems arbitrary - the max MCTP message size is 4224 bytes, no?

How many bytes we need for MCTP and I2C/SMBus overhead depends on MTUS,
so I 5000 enough? Is it excessive?

+   uint8_t *cmdbuffer;
+} NvmeMiSendRecvStruct;
+typedef struct NvmeMiVpdElements {
+    long common_header;

Use a fixed length type here.

+} NvmeMiVpdElements;
+
+typedef struct NvmeMiCtrl {
+   I2CSlave parent_obj;
+   uint32_t mctp_unit_size;
+   uint32_t smbus_freq;
+   NvmeMiVpdElements vpd_data;
+   NvmeMiSendRecvStruct  misendrecv;
+   NvmeCtrl *n;
+} NvmeMiCtrl;
+
+enum NvmeMiMngmtInterfaceCmdSetsOpcodes {
+   READ_NVME_MI_DS                   = 0x00,
+   NVM_SHSP                          = 0x01,
+   CHSP                              = 0x02,
+   CONFIGURATION_SET                 = 0x03,
+   CONFIGURATION_GET                 = 0x04,
+   VPD_READ                          = 0x05,
+   VPD_WRITE                         = 0x06,
+   MI_RESET                          = 0x07,
+   SES_RECEIVE                       = 0x08,
+   SES_SEND                          = 0x09,
+   MANAGEMENT_ENDPOINT_BUFFER_READ   = 0x0A,
+   MANAGEMENT_ENDPOINT_BUFFER_WRITE  = 0x0B,
+   MI_RESERVED                       = 0x0C,
+   VENDOR_SPECIFIC                   = 0xC0
+};
+
+enum NvmeMiControlPrimitiveOpcodes {
+   PAUSE                             = 0x00,
+   RESUME                            = 0x01,
+   ABORT                             = 0x02,
+   GET_STATE                         = 0x03,
+   REPLAY                            = 0x04,
+   CTRL_PRIMITIVE_RESERVED           = 0x05,
+   CTRL_PRIMITIVE_VENDOR_SPECIFIC    = 0xF0
+};
+
+enum NvmeMiAdminCommands {
+   NVME_ADM_MI_CMD_DELETE_SQ      = 0x00,
+   NVME_ADM_MI_CMD_CREATE_SQ      = 0x01,
+   NVME_ADM_MI_CMD_GET_LOG_PAGE   = 0x02,
+   NVME_ADM_MI_CMD_DELETE_CQ      = 0x04,
+   NVME_ADM_MI_CMD_CREATE_CQ      = 0x05,
+   NVME_ADM_MI_CMD_IDENTIFY       = 0x06,
+   NVME_ADM_MI_CMD_ABORT          = 0x08,
+   NVME_ADM_MI_CMD_SET_FEATURES   = 0x09,
+   NVME_ADM_MI_CMD_GET_FEATURES   = 0x0a,
+   NVME_ADM_MI_CMD_ASYNC_EV_REQ   = 0x0c,
+   NVME_ADM_MI_CMD_NS_MANAGEMENT  = 0x0d,
+   NVME_ADM_MI_CMD_ACTIVATE_FW    = 0x10,
+   NVME_ADM_MI_CMD_DOWNLOAD_FW    = 0x11,
+   NVME_ADM_MI_CMD_DST            = 0x14,
+   NVME_ADM_MI_CMD_NS_ATTACHMENT  = 0x15,
+   NVME_ADM_MI_CMD_KEEP_ALIVE     = 0x18,
+   NVME_ADM_MI_CMD_DIRECTIVE_SEND = 0x19,
+   NVME_ADM_MI_CMD_DIRECTIVE_RECV = 0x1A,
+   NVME_ADM_MI_CMD_VIRTUALIZATION = 0x1C,
+   NVME_ADM_MI_CMD_DB_BUF_CONIF   = 0x7C,
+   NVME_ADM_MI_CMD_FORMAT_NVM     = 0x80,
+   NVME_ADM_MI_CMD_SECURITY_SEND  = 0x81,
+   NVME_ADM_MI_CMD_SECURITY_RECV  = 0x82,
+   NVME_ADM_MI_CMD_SANITIZE       = 0x84,
+   NVME_ADM_MI_CMD_GET_LBA_STATUS = 0x86,
+};

These opcodes are the same as in the Admin Command set, why repeat them
here?

+
+enum NvmeMiConfigGetResponseValue {
+   DEFAULT_MCTP_SIZE   = 64,
+   DEFAULT_SMBUS_FREQ  = 1,
+   SET_SMBUS_FREQ      = 129,
+   SET_7BITS           = 255,
+   SET_4BITS           = 15,
+   SET_16BITS          = 65535
+};
+
+enum NvmeMiConfigurationIdentifier {
+   SMBUS_I2C_FREQ = 1,
+   HEALTH_STATUS_CHG,
+   MCTP_TRANS_UNIT_SIZE,
+};
+
+enum NvmeMiResponseMessageStatus {
+   SUCCESS,
+   MORE_PROCESSING_REQUIRED,
+   INTERNAL_ERROR,
+   INVALID_COMMAND_OPCODE,
+   INVALID_PARAMETER,
+   INVALID_COMMAND_SIZE,
+   INVALID_COMMAND_INPUT_DATA_SIZE,
+   ACCESS_DENIED,
+   VPD_UPDATES_EXCEEDED = 0x20,
+   PCIe_INACCESSIBLE
+};
+
+typedef struct NvmeMiMctpHeader {
+   uint32_t byte_count:8;
+   uint32_t SOM:1;
+   uint32_t EOM:1;
+   uint32_t pckt_seq:2;
+} NvmeMiMctpHeader;

This doesnt look right - looks like its mixing the i2c header and the
mctp header. But I don't think its used anyway.

+
+typedef struct NvmeMiMessageHeader {
+   uint32_t msgtype:7;
+   uint32_t ic:1;
+   uint32_t csi:1;
+   uint32_t reserved:2;
+   uint32_t nmimt:4;
+   uint32_t ror:1;
+   uint32_t reserved1:16;
+} NvmeMiMessageHeader;
+
+typedef struct NvmeMIRequest {
+   NvmeMiMessageHeader msg_header;
+   uint32_t               opc:8;
+   uint32_t               rsvd:24;
+   uint32_t               dword0;
+   uint32_t               dword1;
+   uint8_t                *buf;

That's not right. This is effectively just a 4 or 8 byte wide field. You
might want to use a flexible array member at the end here and have it
include both the request data and the crc.

+   uint32_t               mic;
+} NvmeMIRequest;
+
+typedef struct NvmeAdminMIRequest {
+   NvmeMiMessageHeader msg_header;
+   uint8_t                opc;
+   uint8_t                cmdflags;
+   uint16_t               cntlid;
+   uint32_t               sqentry1;
+   uint32_t               sqentry2;
+   uint32_t               sqentry3;
+   uint32_t               sqentry4;
+   uint32_t               sqentry5;
+   uint32_t               dataofst;
+   uint32_t               datalen;
+   uint32_t               reserved[2];
+   uint32_t               sqentry10;
+   uint32_t               sqentry11;
+   uint32_t               sqentry12;
+   uint32_t               sqentry13;
+   uint32_t               sqentry14;
+   uint32_t               sqentry15;
+   uint8_t                *buf;

Same issue as above.

+   uint32_t               mic;
+} NvmeAdminMIRequest;
+
+typedef struct ReadNvmeMiDs {
+    uint16_t cntrlid;
+    uint8_t  portlid;
+    uint8_t  dtyp;
+}  ReadNvmeMiDs;
+
+typedef struct NvmeMiConfigurationSet {
+    uint8_t conf_identifier_dword_0;
+    uint16_t conf_identifier_specific_dword_0;
+    uint16_t conf_identifier_specific_dword_1;
+}  MiConfigurationSet;
+
+typedef struct NvmeMiNvmSubsysHspds {
+    uint8_t nss;
+    uint8_t sw;
+    uint8_t ctemp;
+    uint8_t pdlu;
+    uint16_t ccs;
+    uint16_t reserved;
+} NvmeMiNvmSubsysHspds;
+
+typedef struct NvmeMiControlPrimitives {
+    uint32_t nmh;
+    uint32_t cpo;
+    uint32_t tag;
+    uint32_t cpsp;
+    uint32_t mic;
+} NvmeMiControlPrimitives;
+
+typedef struct NvmMiSubsysInfoDs {
+    uint8_t nump;
+    uint8_t mjr;
+    uint8_t mnr;
+    uint8_t rsvd[29];
+} NvmMiSubsysInfoDs;
+
+typedef struct NvmeMiCwarnStruct {
+    uint8_t spare_thresh:1;
+    uint8_t temp_above_or_under_thresh:1;
+    uint8_t rel_degraded:1;
+    uint8_t read_only:1;
+    uint8_t vol_mem_bup_fail:1;
+    uint8_t reserved:3;
+} NvmeMiCwarnStruct;
+
+typedef struct NvmeMiCstsStruct {
+    uint16_t rdy:1;
+    uint16_t cfs:1;
+    uint16_t shst:2;
+    uint16_t nssro:1;
+    uint16_t en:1;
+    uint16_t nssac:1;
+    uint16_t fwact:1;
+    uint16_t reserved:8;
+} NvmeMiCstsStruct;
+
+typedef struct NvmeMiCtrlHealthDs {
+   uint16_t ctlid;
+   NvmeMiCstsStruct csts;
+   uint16_t ctemp;
+   uint16_t pdlu;
+   uint8_t spare;
+   NvmeMiCwarnStruct cwarn;
+   uint8_t reserved[7];
+} NvmeMiCtrlHealthDs;
+
+typedef struct NvmeMIResponse {
+   NvmeMiMessageHeader msg_header;
+   uint8_t                status;
+   uint32_t               mgmt_resp:24;

Don't mix bitfields like this. Either do a bitfield on the status as
well or use a uint8_t[3] for the mgmt_resp.

+} NvmeMIResponse;
+
+typedef struct NvmeMIAdminResponse {
+   NvmeMiMessageHeader msg_header;
+   uint32_t status:8;
+   uint32_t mgmt_resp:24;
+   uint32_t cqdword0;
+   uint32_t cqdword1;
+   uint32_t cqdword3;
+} NvmeMIAdminResponse;
+
+uint32_t NvmeMiCmdOptSupList[] = {
+  /*
+   * MANAGEMENT_ENDPOINT_BUFFER_READ,
+   * MANAGEMENT_ENDPOINT_BUFFER_WRITE,
+   */
+};
+
+uint32_t NvmeMiAdminCmdOptSupList[] = {
+   /*
+    *  NVME_ADM_CMD_DST,
+    *  NVME_ADM_CMD_DOWNLOAD_FW,
+    *  NVME_ADM_CMD_ACTIVATE_FW,
+    *  NVME_ADM_CMD_FORMAT_NVM,
+    *  NVME_ADM_CMD_NS_MANAGEMENT,
+    *  NVME_ADM_CMD_NS_ATTACHMENT,
+    *  NVME_ADM_CMD_DIRECTIVE_SEND,
+    *  NVME_ADM_CMD_DIRECTIVE_RECV,
+    *  NVME_ADM_CMD_SET_FEATURES,
+    *  NVME_ADM_CMD_SANITIZE,
+    */
+};
+
+void *vsock_server_start(void *);
+void *ControlPrimitiveThread(void *);
+void *nvme_mi_admin_commandthread(void *);
+

These look like left-overs from v1.

In general, please trim all the stuff that isnt used by the
implementation.

+#endif
--
2.7.0.windows.1



Thanks very much Klaus for the review comments and feedback.
We will work on it and update the patch with these changes.





reply via email to

[Prev in Thread] Current Thread [Next in Thread]