* [PATCH v2 01/18] hw/block/nvme: bump spec data structures to v1.3
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 6:34 ` [PATCH v2 02/18] hw/block/nvme: additional tracing Klaus Jensen
` (16 subsequent siblings)
17 siblings, 0 replies; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Fam Zheng, Dmitry Fomichev, Klaus Jensen, qemu-devel,
Max Reitz, Klaus Jensen, Keith Busch, Javier Gonzalez,
Maxim Levitsky, Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
Add missing fields in the Identify Controller and Identify Namespace
data structures to bring them in line with NVMe v1.3.
This also adds data structures and defines for SGL support which
requires a couple of trivial changes to the nvme block driver as well.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Acked-by: Fam Zheng <fam@euphon.net>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
---
block/nvme.c | 18 ++---
hw/block/nvme.c | 12 ++--
include/block/nvme.h | 156 ++++++++++++++++++++++++++++++++++++++-----
3 files changed, 154 insertions(+), 32 deletions(-)
diff --git a/block/nvme.c b/block/nvme.c
index 374e26891573..c1c4c07ac6cc 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -518,7 +518,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
error_setg(errp, "Cannot map buffer for DMA");
goto out;
}
- cmd.prp1 = cpu_to_le64(iova);
+ cmd.dptr.prp1 = cpu_to_le64(iova);
if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {
error_setg(errp, "Failed to identify controller");
@@ -629,7 +629,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
}
cmd = (NvmeCmd) {
.opcode = NVME_ADM_CMD_CREATE_CQ,
- .prp1 = cpu_to_le64(q->cq.iova),
+ .dptr.prp1 = cpu_to_le64(q->cq.iova),
.cdw10 = cpu_to_le32(((queue_size - 1) << 16) | (n & 0xFFFF)),
.cdw11 = cpu_to_le32(0x3),
};
@@ -640,7 +640,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
}
cmd = (NvmeCmd) {
.opcode = NVME_ADM_CMD_CREATE_SQ,
- .prp1 = cpu_to_le64(q->sq.iova),
+ .dptr.prp1 = cpu_to_le64(q->sq.iova),
.cdw10 = cpu_to_le32(((queue_size - 1) << 16) | (n & 0xFFFF)),
.cdw11 = cpu_to_le32(0x1 | (n << 16)),
};
@@ -988,16 +988,16 @@ try_map:
case 0:
abort();
case 1:
- cmd->prp1 = pagelist[0];
- cmd->prp2 = 0;
+ cmd->dptr.prp1 = pagelist[0];
+ cmd->dptr.prp2 = 0;
break;
case 2:
- cmd->prp1 = pagelist[0];
- cmd->prp2 = pagelist[1];
+ cmd->dptr.prp1 = pagelist[0];
+ cmd->dptr.prp2 = pagelist[1];
break;
default:
- cmd->prp1 = pagelist[0];
- cmd->prp2 = cpu_to_le64(req->prp_list_iova + sizeof(uint64_t));
+ cmd->dptr.prp1 = pagelist[0];
+ cmd->dptr.prp2 = cpu_to_le64(req->prp_list_iova + sizeof(uint64_t));
break;
}
trace_nvme_cmd_map_qiov(s, cmd, req, qiov, entries);
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 1aee042d4cb2..71b388aa0e20 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -397,8 +397,8 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
NvmeRwCmd *rw = (NvmeRwCmd *)cmd;
uint32_t nlb = le32_to_cpu(rw->nlb) + 1;
uint64_t slba = le64_to_cpu(rw->slba);
- uint64_t prp1 = le64_to_cpu(rw->prp1);
- uint64_t prp2 = le64_to_cpu(rw->prp2);
+ uint64_t prp1 = le64_to_cpu(rw->dptr.prp1);
+ uint64_t prp2 = le64_to_cpu(rw->dptr.prp2);
uint8_t lba_index = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
uint8_t data_shift = ns->id_ns.lbaf[lba_index].ds;
@@ -795,8 +795,8 @@ static inline uint64_t nvme_get_timestamp(const NvmeCtrl *n)
static uint16_t nvme_get_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd)
{
- uint64_t prp1 = le64_to_cpu(cmd->prp1);
- uint64_t prp2 = le64_to_cpu(cmd->prp2);
+ uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
+ uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
uint64_t timestamp = nvme_get_timestamp(n);
@@ -834,8 +834,8 @@ static uint16_t nvme_set_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd)
{
uint16_t ret;
uint64_t timestamp;
- uint64_t prp1 = le64_to_cpu(cmd->prp1);
- uint64_t prp2 = le64_to_cpu(cmd->prp2);
+ uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
+ uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
ret = nvme_dma_write_prp(n, (uint8_t *)×tamp,
sizeof(timestamp), prp1, prp2);
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 1720ee1d5158..2a80d2a7ed89 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -377,15 +377,53 @@ enum NvmePmrmscMask {
#define NVME_PMRMSC_SET_CBA(pmrmsc, val) \
(pmrmsc |= (uint64_t)(val & PMRMSC_CBA_MASK) << PMRMSC_CBA_SHIFT)
+enum NvmeSglDescriptorType {
+ NVME_SGL_DESCR_TYPE_DATA_BLOCK = 0x0,
+ NVME_SGL_DESCR_TYPE_BIT_BUCKET = 0x1,
+ NVME_SGL_DESCR_TYPE_SEGMENT = 0x2,
+ NVME_SGL_DESCR_TYPE_LAST_SEGMENT = 0x3,
+ NVME_SGL_DESCR_TYPE_KEYED_DATA_BLOCK = 0x4,
+
+ NVME_SGL_DESCR_TYPE_VENDOR_SPECIFIC = 0xf,
+};
+
+enum NvmeSglDescriptorSubtype {
+ NVME_SGL_DESCR_SUBTYPE_ADDRESS = 0x0,
+};
+
+typedef struct NvmeSglDescriptor {
+ uint64_t addr;
+ uint32_t len;
+ uint8_t rsvd[3];
+ uint8_t type;
+} NvmeSglDescriptor;
+
+#define NVME_SGL_TYPE(type) ((type >> 4) & 0xf)
+#define NVME_SGL_SUBTYPE(type) (type & 0xf)
+
+typedef union NvmeCmdDptr {
+ struct {
+ uint64_t prp1;
+ uint64_t prp2;
+ };
+
+ NvmeSglDescriptor sgl;
+} NvmeCmdDptr;
+
+enum NvmePsdt {
+ PSDT_PRP = 0x0,
+ PSDT_SGL_MPTR_CONTIGUOUS = 0x1,
+ PSDT_SGL_MPTR_SGL = 0x2,
+};
+
typedef struct NvmeCmd {
uint8_t opcode;
- uint8_t fuse;
+ uint8_t flags;
uint16_t cid;
uint32_t nsid;
uint64_t res1;
uint64_t mptr;
- uint64_t prp1;
- uint64_t prp2;
+ NvmeCmdDptr dptr;
uint32_t cdw10;
uint32_t cdw11;
uint32_t cdw12;
@@ -394,6 +432,9 @@ typedef struct NvmeCmd {
uint32_t cdw15;
} NvmeCmd;
+#define NVME_CMD_FLAGS_FUSE(flags) (flags & 0x3)
+#define NVME_CMD_FLAGS_PSDT(flags) ((flags >> 6) & 0x3)
+
enum NvmeAdminCommands {
NVME_ADM_CMD_DELETE_SQ = 0x00,
NVME_ADM_CMD_CREATE_SQ = 0x01,
@@ -493,8 +534,7 @@ typedef struct NvmeRwCmd {
uint32_t nsid;
uint64_t rsvd2;
uint64_t mptr;
- uint64_t prp1;
- uint64_t prp2;
+ NvmeCmdDptr dptr;
uint64_t slba;
uint16_t nlb;
uint16_t control;
@@ -534,8 +574,7 @@ typedef struct NvmeDsmCmd {
uint16_t cid;
uint32_t nsid;
uint64_t rsvd2[2];
- uint64_t prp1;
- uint64_t prp2;
+ NvmeCmdDptr dptr;
uint32_t nr;
uint32_t attributes;
uint32_t rsvd12[4];
@@ -599,6 +638,12 @@ enum NvmeStatusCodes {
NVME_CMD_ABORT_MISSING_FUSE = 0x000a,
NVME_INVALID_NSID = 0x000b,
NVME_CMD_SEQ_ERROR = 0x000c,
+ NVME_INVALID_SGL_SEG_DESCR = 0x000d,
+ NVME_INVALID_NUM_SGL_DESCRS = 0x000e,
+ NVME_DATA_SGL_LEN_INVALID = 0x000f,
+ NVME_MD_SGL_LEN_INVALID = 0x0010,
+ NVME_SGL_DESCR_TYPE_INVALID = 0x0011,
+ NVME_INVALID_USE_OF_CMB = 0x0012,
NVME_LBA_RANGE = 0x0080,
NVME_CAP_EXCEEDED = 0x0081,
NVME_NS_NOT_READY = 0x0082,
@@ -687,7 +732,7 @@ enum NvmeSmartWarn {
NVME_SMART_FAILED_VOLATILE_MEDIA = 1 << 4,
};
-enum LogIdentifier {
+enum NvmeLogIdentifier {
NVME_LOG_ERROR_INFO = 0x01,
NVME_LOG_SMART_INFO = 0x02,
NVME_LOG_FW_SLOT_INFO = 0x03,
@@ -711,6 +756,7 @@ enum {
NVME_ID_CNS_NS = 0x0,
NVME_ID_CNS_CTRL = 0x1,
NVME_ID_CNS_NS_ACTIVE_LIST = 0x2,
+ NVME_ID_CNS_NS_DESCR_LIST = 0x3,
};
typedef struct NvmeIdCtrl {
@@ -723,7 +769,15 @@ typedef struct NvmeIdCtrl {
uint8_t ieee[3];
uint8_t cmic;
uint8_t mdts;
- uint8_t rsvd255[178];
+ uint16_t cntlid;
+ uint32_t ver;
+ uint32_t rtd3r;
+ uint32_t rtd3e;
+ uint32_t oaes;
+ uint32_t ctratt;
+ uint8_t rsvd100[12];
+ uint8_t fguid[16];
+ uint8_t rsvd128[128];
uint16_t oacs;
uint8_t acl;
uint8_t aerl;
@@ -731,10 +785,28 @@ typedef struct NvmeIdCtrl {
uint8_t lpa;
uint8_t elpe;
uint8_t npss;
- uint8_t rsvd511[248];
+ uint8_t avscc;
+ uint8_t apsta;
+ uint16_t wctemp;
+ uint16_t cctemp;
+ uint16_t mtfa;
+ uint32_t hmpre;
+ uint32_t hmmin;
+ uint8_t tnvmcap[16];
+ uint8_t unvmcap[16];
+ uint32_t rpmbs;
+ uint16_t edstt;
+ uint8_t dsto;
+ uint8_t fwug;
+ uint16_t kas;
+ uint16_t hctma;
+ uint16_t mntmt;
+ uint16_t mxtmt;
+ uint32_t sanicap;
+ uint8_t rsvd332[180];
uint8_t sqes;
uint8_t cqes;
- uint16_t rsvd515;
+ uint16_t maxcmd;
uint32_t nn;
uint16_t oncs;
uint16_t fuses;
@@ -742,8 +814,14 @@ typedef struct NvmeIdCtrl {
uint8_t vwc;
uint16_t awun;
uint16_t awupf;
- uint8_t rsvd703[174];
- uint8_t rsvd2047[1344];
+ uint8_t nvscc;
+ uint8_t rsvd531;
+ uint16_t acwu;
+ uint8_t rsvd534[2];
+ uint32_t sgls;
+ uint8_t rsvd540[228];
+ uint8_t subnqn[256];
+ uint8_t rsvd1024[1024];
NvmePSD psd[32];
uint8_t vs[1024];
} NvmeIdCtrl;
@@ -769,6 +847,16 @@ enum NvmeIdCtrlOncs {
#define NVME_CTRL_CQES_MIN(cqes) ((cqes) & 0xf)
#define NVME_CTRL_CQES_MAX(cqes) (((cqes) >> 4) & 0xf)
+#define NVME_CTRL_SGLS_SUPPORT_MASK (0x3 << 0)
+#define NVME_CTRL_SGLS_SUPPORT_NO_ALIGN (0x1 << 0)
+#define NVME_CTRL_SGLS_SUPPORT_DWORD_ALIGN (0x1 << 1)
+#define NVME_CTRL_SGLS_KEYED (0x1 << 2)
+#define NVME_CTRL_SGLS_BITBUCKET (0x1 << 16)
+#define NVME_CTRL_SGLS_MPTR_CONTIGUOUS (0x1 << 17)
+#define NVME_CTRL_SGLS_EXCESS_LENGTH (0x1 << 18)
+#define NVME_CTRL_SGLS_MPTR_SGL (0x1 << 19)
+#define NVME_CTRL_SGLS_ADDR_OFFSET (0x1 << 20)
+
typedef struct NvmeFeatureVal {
uint32_t arbitration;
uint32_t power_mgmt;
@@ -791,6 +879,15 @@ typedef struct NvmeFeatureVal {
#define NVME_INTC_THR(intc) (intc & 0xff)
#define NVME_INTC_TIME(intc) ((intc >> 8) & 0xff)
+#define NVME_TEMP_THSEL(temp) ((temp >> 20) & 0x3)
+#define NVME_TEMP_THSEL_OVER 0x0
+#define NVME_TEMP_THSEL_UNDER 0x1
+
+#define NVME_TEMP_TMPSEL(temp) ((temp >> 16) & 0xf)
+#define NVME_TEMP_TMPSEL_COMPOSITE 0x0
+
+#define NVME_TEMP_TMPTH(temp) ((temp >> 0) & 0xffff)
+
enum NvmeFeatureIds {
NVME_ARBITRATION = 0x1,
NVME_POWER_MANAGEMENT = 0x2,
@@ -833,18 +930,43 @@ typedef struct NvmeIdNs {
uint8_t mc;
uint8_t dpc;
uint8_t dps;
-
uint8_t nmic;
uint8_t rescap;
uint8_t fpi;
uint8_t dlfeat;
-
- uint8_t res34[94];
+ uint16_t nawun;
+ uint16_t nawupf;
+ uint16_t nacwu;
+ uint16_t nabsn;
+ uint16_t nabo;
+ uint16_t nabspf;
+ uint16_t noiob;
+ uint8_t nvmcap[16];
+ uint8_t rsvd64[40];
+ uint8_t nguid[16];
+ uint64_t eui64;
NvmeLBAF lbaf[16];
- uint8_t res192[192];
+ uint8_t rsvd192[192];
uint8_t vs[3712];
} NvmeIdNs;
+typedef struct NvmeIdNsDescr {
+ uint8_t nidt;
+ uint8_t nidl;
+ uint8_t rsvd2[2];
+} NvmeIdNsDescr;
+
+enum {
+ NVME_NIDT_EUI64_LEN = 8,
+ NVME_NIDT_NGUID_LEN = 16,
+ NVME_NIDT_UUID_LEN = 16,
+};
+
+enum NvmeNsIdentifierType {
+ NVME_NIDT_EUI64 = 0x1,
+ NVME_NIDT_NGUID = 0x2,
+ NVME_NIDT_UUID = 0x3,
+};
/*Deallocate Logical Block Features*/
#define NVME_ID_NS_DLFEAT_GUARD_CRC(dlfeat) ((dlfeat) & 0x10)
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v2 02/18] hw/block/nvme: additional tracing
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
2020-07-03 6:34 ` [PATCH v2 01/18] hw/block/nvme: bump spec data structures " Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 8:03 ` Philippe Mathieu-Daudé
2020-07-03 6:34 ` [PATCH v2 03/18] hw/block/nvme: add support for the abort command Klaus Jensen
` (15 subsequent siblings)
17 siblings, 1 reply; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
Add various additional tracing and streamline nvme_identify_ns and
nvme_identify_nslist (they do not need to repeat the command, it is
already in the trace name).
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
hw/block/nvme.c | 19 +++++++++++++++++++
hw/block/nvme.h | 14 ++++++++++++++
hw/block/trace-events | 13 +++++++++++--
3 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 71b388aa0e20..f5d9148f0936 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -331,6 +331,8 @@ static void nvme_post_cqes(void *opaque)
static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
{
assert(cq->cqid == req->sq->cqid);
+ trace_pci_nvme_enqueue_req_completion(nvme_cid(req), cq->cqid,
+ req->status);
QTAILQ_REMOVE(&req->sq->out_req_list, req, entry);
QTAILQ_INSERT_TAIL(&cq->req_list, req, entry);
timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
@@ -343,6 +345,8 @@ static void nvme_rw_cb(void *opaque, int ret)
NvmeCtrl *n = sq->ctrl;
NvmeCQueue *cq = n->cq[sq->cqid];
+ trace_pci_nvme_rw_cb(nvme_cid(req));
+
if (!ret) {
block_acct_done(blk_get_stats(n->conf.blk), &req->acct);
req->status = NVME_SUCCESS;
@@ -378,6 +382,8 @@ static uint16_t nvme_write_zeros(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
uint64_t offset = slba << data_shift;
uint32_t count = nlb << data_shift;
+ trace_pci_nvme_write_zeroes(nvme_cid(req), slba, nlb);
+
if (unlikely(slba + nlb > ns->id_ns.nsze)) {
trace_pci_nvme_err_invalid_lba_range(slba, nlb, ns->id_ns.nsze);
return NVME_LBA_RANGE | NVME_DNR;
@@ -445,6 +451,8 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
NvmeNamespace *ns;
uint32_t nsid = le32_to_cpu(cmd->nsid);
+ trace_pci_nvme_io_cmd(nvme_cid(req), nsid, nvme_sqid(req), cmd->opcode);
+
if (unlikely(nsid == 0 || nsid > n->num_namespaces)) {
trace_pci_nvme_err_invalid_ns(nsid, n->num_namespaces);
return NVME_INVALID_NSID | NVME_DNR;
@@ -876,6 +884,8 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
{
+ trace_pci_nvme_admin_cmd(nvme_cid(req), nvme_sqid(req), cmd->opcode);
+
switch (cmd->opcode) {
case NVME_ADM_CMD_DELETE_SQ:
return nvme_del_sq(n, cmd);
@@ -1204,6 +1214,8 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
uint8_t *ptr = (uint8_t *)&n->bar;
uint64_t val = 0;
+ trace_pci_nvme_mmio_read(addr);
+
if (unlikely(addr & (sizeof(uint32_t) - 1))) {
NVME_GUEST_ERR(pci_nvme_ub_mmiord_misaligned32,
"MMIO read not 32-bit aligned,"
@@ -1273,6 +1285,8 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
return;
}
+ trace_pci_nvme_mmio_doorbell_cq(cq->cqid, new_head);
+
start_sqs = nvme_cq_full(cq) ? 1 : 0;
cq->head = new_head;
if (start_sqs) {
@@ -1311,6 +1325,8 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
return;
}
+ trace_pci_nvme_mmio_doorbell_sq(sq->sqid, new_tail);
+
sq->tail = new_tail;
timer_mod(sq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
}
@@ -1320,6 +1336,9 @@ static void nvme_mmio_write(void *opaque, hwaddr addr, uint64_t data,
unsigned size)
{
NvmeCtrl *n = (NvmeCtrl *)opaque;
+
+ trace_pci_nvme_mmio_write(addr, data);
+
if (addr < sizeof(n->bar)) {
nvme_write_bar(n, addr, data, size);
} else if (addr >= 0x1000) {
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 1d30c0bca283..1bf5c80ed843 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -115,4 +115,18 @@ static inline uint64_t nvme_ns_nlbas(NvmeCtrl *n, NvmeNamespace *ns)
return n->ns_size >> nvme_ns_lbads(ns);
}
+static inline uint16_t nvme_cid(NvmeRequest *req)
+{
+ if (req) {
+ return le16_to_cpu(req->cqe.cid);
+ }
+
+ return 0xffff;
+}
+
+static inline uint16_t nvme_sqid(NvmeRequest *req)
+{
+ return le16_to_cpu(req->sq->sqid);
+}
+
#endif /* HW_NVME_H */
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 958fcc5508d1..c40c0d2e4b28 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -33,19 +33,28 @@ pci_nvme_irq_msix(uint32_t vector) "raising MSI-X IRQ vector %u"
pci_nvme_irq_pin(void) "pulsing IRQ pin"
pci_nvme_irq_masked(void) "IRQ is masked"
pci_nvme_dma_read(uint64_t prp1, uint64_t prp2) "DMA read, prp1=0x%"PRIx64" prp2=0x%"PRIx64""
+pci_nvme_io_cmd(uint16_t cid, uint32_t nsid, uint16_t sqid, uint8_t opcode) "cid %"PRIu16" nsid %"PRIu32" sqid %"PRIu16" opc 0x%"PRIx8""
+pci_nvme_admin_cmd(uint16_t cid, uint16_t sqid, uint8_t opcode) "cid %"PRIu16" sqid %"PRIu16" opc 0x%"PRIx8""
pci_nvme_rw(const char *verb, uint32_t blk_count, uint64_t byte_count, uint64_t lba) "%s %"PRIu32" blocks (%"PRIu64" bytes) from LBA %"PRIu64""
+pci_nvme_rw_cb(uint16_t cid) "cid %"PRIu16""
+pci_nvme_write_zeroes(uint16_t cid, uint64_t slba, uint32_t nlb) "cid %"PRIu16" slba %"PRIu64" nlb %"PRIu32""
pci_nvme_create_sq(uint64_t addr, uint16_t sqid, uint16_t cqid, uint16_t qsize, uint16_t qflags) "create submission queue, addr=0x%"PRIx64", sqid=%"PRIu16", cqid=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16""
pci_nvme_create_cq(uint64_t addr, uint16_t cqid, uint16_t vector, uint16_t size, uint16_t qflags, int ien) "create completion queue, addr=0x%"PRIx64", cqid=%"PRIu16", vector=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16", ien=%d"
pci_nvme_del_sq(uint16_t qid) "deleting submission queue sqid=%"PRIu16""
pci_nvme_del_cq(uint16_t cqid) "deleted completion queue, cqid=%"PRIu16""
pci_nvme_identify_ctrl(void) "identify controller"
-pci_nvme_identify_ns(uint16_t ns) "identify namespace, nsid=%"PRIu16""
-pci_nvme_identify_nslist(uint16_t ns) "identify namespace list, nsid=%"PRIu16""
+pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
+pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
pci_nvme_getfeat_vwcache(const char* result) "get feature volatile write cache, result=%s"
pci_nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
pci_nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
pci_nvme_setfeat_timestamp(uint64_t ts) "set feature timestamp = 0x%"PRIx64""
pci_nvme_getfeat_timestamp(uint64_t ts) "get feature timestamp = 0x%"PRIx64""
+pci_nvme_enqueue_req_completion(uint16_t cid, uint16_t cqid, uint16_t status) "cid %"PRIu16" cqid %"PRIu16" status 0x%"PRIx16""
+pci_nvme_mmio_read(uint64_t addr) "addr 0x%"PRIx64""
+pci_nvme_mmio_write(uint64_t addr, uint64_t data) "addr 0x%"PRIx64" data 0x%"PRIx64""
+pci_nvme_mmio_doorbell_cq(uint16_t cqid, uint16_t new_head) "cqid %"PRIu16" new_head %"PRIu16""
+pci_nvme_mmio_doorbell_sq(uint16_t sqid, uint16_t new_tail) "cqid %"PRIu16" new_tail %"PRIu16""
pci_nvme_mmio_intm_set(uint64_t data, uint64_t new_mask) "wrote MMIO, interrupt mask set, data=0x%"PRIx64", new_mask=0x%"PRIx64""
pci_nvme_mmio_intm_clr(uint64_t data, uint64_t new_mask) "wrote MMIO, interrupt mask clr, data=0x%"PRIx64", new_mask=0x%"PRIx64""
pci_nvme_mmio_cfg(uint64_t data) "wrote MMIO, config controller config=0x%"PRIx64""
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v2 02/18] hw/block/nvme: additional tracing
2020-07-03 6:34 ` [PATCH v2 02/18] hw/block/nvme: additional tracing Klaus Jensen
@ 2020-07-03 8:03 ` Philippe Mathieu-Daudé
2020-07-03 8:14 ` Klaus Jensen
0 siblings, 1 reply; 42+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-03 8:03 UTC (permalink / raw)
To: Klaus Jensen, qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Keith Busch, Javier Gonzalez, Maxim Levitsky
On 7/3/20 8:34 AM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Add various additional tracing and streamline nvme_identify_ns and
> nvme_identify_nslist (they do not need to repeat the command, it is
> already in the trace name).
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
> hw/block/nvme.c | 19 +++++++++++++++++++
> hw/block/nvme.h | 14 ++++++++++++++
> hw/block/trace-events | 13 +++++++++++--
> 3 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 71b388aa0e20..f5d9148f0936 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -331,6 +331,8 @@ static void nvme_post_cqes(void *opaque)
> static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
> {
> assert(cq->cqid == req->sq->cqid);
> + trace_pci_nvme_enqueue_req_completion(nvme_cid(req), cq->cqid,
> + req->status);
> QTAILQ_REMOVE(&req->sq->out_req_list, req, entry);
> QTAILQ_INSERT_TAIL(&cq->req_list, req, entry);
> timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
> @@ -343,6 +345,8 @@ static void nvme_rw_cb(void *opaque, int ret)
> NvmeCtrl *n = sq->ctrl;
> NvmeCQueue *cq = n->cq[sq->cqid];
>
> + trace_pci_nvme_rw_cb(nvme_cid(req));
> +
> if (!ret) {
> block_acct_done(blk_get_stats(n->conf.blk), &req->acct);
> req->status = NVME_SUCCESS;
> @@ -378,6 +382,8 @@ static uint16_t nvme_write_zeros(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
> uint64_t offset = slba << data_shift;
> uint32_t count = nlb << data_shift;
>
> + trace_pci_nvme_write_zeroes(nvme_cid(req), slba, nlb);
> +
> if (unlikely(slba + nlb > ns->id_ns.nsze)) {
> trace_pci_nvme_err_invalid_lba_range(slba, nlb, ns->id_ns.nsze);
> return NVME_LBA_RANGE | NVME_DNR;
> @@ -445,6 +451,8 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> NvmeNamespace *ns;
> uint32_t nsid = le32_to_cpu(cmd->nsid);
>
> + trace_pci_nvme_io_cmd(nvme_cid(req), nsid, nvme_sqid(req), cmd->opcode);
> +
> if (unlikely(nsid == 0 || nsid > n->num_namespaces)) {
> trace_pci_nvme_err_invalid_ns(nsid, n->num_namespaces);
> return NVME_INVALID_NSID | NVME_DNR;
> @@ -876,6 +884,8 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>
> static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> {
> + trace_pci_nvme_admin_cmd(nvme_cid(req), nvme_sqid(req), cmd->opcode);
> +
> switch (cmd->opcode) {
> case NVME_ADM_CMD_DELETE_SQ:
> return nvme_del_sq(n, cmd);
> @@ -1204,6 +1214,8 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
> uint8_t *ptr = (uint8_t *)&n->bar;
> uint64_t val = 0;
>
> + trace_pci_nvme_mmio_read(addr);
> +
> if (unlikely(addr & (sizeof(uint32_t) - 1))) {
> NVME_GUEST_ERR(pci_nvme_ub_mmiord_misaligned32,
> "MMIO read not 32-bit aligned,"
> @@ -1273,6 +1285,8 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
> return;
> }
>
> + trace_pci_nvme_mmio_doorbell_cq(cq->cqid, new_head);
> +
> start_sqs = nvme_cq_full(cq) ? 1 : 0;
> cq->head = new_head;
> if (start_sqs) {
> @@ -1311,6 +1325,8 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
> return;
> }
>
> + trace_pci_nvme_mmio_doorbell_sq(sq->sqid, new_tail);
> +
> sq->tail = new_tail;
> timer_mod(sq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
> }
> @@ -1320,6 +1336,9 @@ static void nvme_mmio_write(void *opaque, hwaddr addr, uint64_t data,
> unsigned size)
> {
> NvmeCtrl *n = (NvmeCtrl *)opaque;
> +
> + trace_pci_nvme_mmio_write(addr, data);
> +
> if (addr < sizeof(n->bar)) {
> nvme_write_bar(n, addr, data, size);
> } else if (addr >= 0x1000) {
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 1d30c0bca283..1bf5c80ed843 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -115,4 +115,18 @@ static inline uint64_t nvme_ns_nlbas(NvmeCtrl *n, NvmeNamespace *ns)
> return n->ns_size >> nvme_ns_lbads(ns);
> }
>
> +static inline uint16_t nvme_cid(NvmeRequest *req)
> +{
> + if (req) {
> + return le16_to_cpu(req->cqe.cid);
> + }
> +
> + return 0xffff;
In this case I find the inverted logic easier. Matter
of taste :)
> +}
> +
> +static inline uint16_t nvme_sqid(NvmeRequest *req)
> +{
> + return le16_to_cpu(req->sq->sqid);
> +}
Later I'd prefer we move these out of the header, and remove
the inline attribute.
I.e. nvme_ns_nlbas() is only used once.
OK for now.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> +
> #endif /* HW_NVME_H */
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 958fcc5508d1..c40c0d2e4b28 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -33,19 +33,28 @@ pci_nvme_irq_msix(uint32_t vector) "raising MSI-X IRQ vector %u"
> pci_nvme_irq_pin(void) "pulsing IRQ pin"
> pci_nvme_irq_masked(void) "IRQ is masked"
> pci_nvme_dma_read(uint64_t prp1, uint64_t prp2) "DMA read, prp1=0x%"PRIx64" prp2=0x%"PRIx64""
> +pci_nvme_io_cmd(uint16_t cid, uint32_t nsid, uint16_t sqid, uint8_t opcode) "cid %"PRIu16" nsid %"PRIu32" sqid %"PRIu16" opc 0x%"PRIx8""
> +pci_nvme_admin_cmd(uint16_t cid, uint16_t sqid, uint8_t opcode) "cid %"PRIu16" sqid %"PRIu16" opc 0x%"PRIx8""
> pci_nvme_rw(const char *verb, uint32_t blk_count, uint64_t byte_count, uint64_t lba) "%s %"PRIu32" blocks (%"PRIu64" bytes) from LBA %"PRIu64""
> +pci_nvme_rw_cb(uint16_t cid) "cid %"PRIu16""
> +pci_nvme_write_zeroes(uint16_t cid, uint64_t slba, uint32_t nlb) "cid %"PRIu16" slba %"PRIu64" nlb %"PRIu32""
> pci_nvme_create_sq(uint64_t addr, uint16_t sqid, uint16_t cqid, uint16_t qsize, uint16_t qflags) "create submission queue, addr=0x%"PRIx64", sqid=%"PRIu16", cqid=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16""
> pci_nvme_create_cq(uint64_t addr, uint16_t cqid, uint16_t vector, uint16_t size, uint16_t qflags, int ien) "create completion queue, addr=0x%"PRIx64", cqid=%"PRIu16", vector=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16", ien=%d"
> pci_nvme_del_sq(uint16_t qid) "deleting submission queue sqid=%"PRIu16""
> pci_nvme_del_cq(uint16_t cqid) "deleted completion queue, cqid=%"PRIu16""
> pci_nvme_identify_ctrl(void) "identify controller"
> -pci_nvme_identify_ns(uint16_t ns) "identify namespace, nsid=%"PRIu16""
> -pci_nvme_identify_nslist(uint16_t ns) "identify namespace list, nsid=%"PRIu16""
> +pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
> +pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
> pci_nvme_getfeat_vwcache(const char* result) "get feature volatile write cache, result=%s"
> pci_nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
> pci_nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
> pci_nvme_setfeat_timestamp(uint64_t ts) "set feature timestamp = 0x%"PRIx64""
> pci_nvme_getfeat_timestamp(uint64_t ts) "get feature timestamp = 0x%"PRIx64""
> +pci_nvme_enqueue_req_completion(uint16_t cid, uint16_t cqid, uint16_t status) "cid %"PRIu16" cqid %"PRIu16" status 0x%"PRIx16""
> +pci_nvme_mmio_read(uint64_t addr) "addr 0x%"PRIx64""
> +pci_nvme_mmio_write(uint64_t addr, uint64_t data) "addr 0x%"PRIx64" data 0x%"PRIx64""
> +pci_nvme_mmio_doorbell_cq(uint16_t cqid, uint16_t new_head) "cqid %"PRIu16" new_head %"PRIu16""
> +pci_nvme_mmio_doorbell_sq(uint16_t sqid, uint16_t new_tail) "cqid %"PRIu16" new_tail %"PRIu16""
> pci_nvme_mmio_intm_set(uint64_t data, uint64_t new_mask) "wrote MMIO, interrupt mask set, data=0x%"PRIx64", new_mask=0x%"PRIx64""
> pci_nvme_mmio_intm_clr(uint64_t data, uint64_t new_mask) "wrote MMIO, interrupt mask clr, data=0x%"PRIx64", new_mask=0x%"PRIx64""
> pci_nvme_mmio_cfg(uint64_t data) "wrote MMIO, config controller config=0x%"PRIx64""
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 02/18] hw/block/nvme: additional tracing
2020-07-03 8:03 ` Philippe Mathieu-Daudé
@ 2020-07-03 8:14 ` Klaus Jensen
0 siblings, 0 replies; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 8:14 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, qemu-block, Dmitry Fomichev, Klaus Jensen,
qemu-devel, Max Reitz, Keith Busch, Javier Gonzalez,
Maxim Levitsky
On Jul 3 10:03, Philippe Mathieu-Daudé wrote:
> On 7/3/20 8:34 AM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > Add various additional tracing and streamline nvme_identify_ns and
> > nvme_identify_nslist (they do not need to repeat the command, it is
> > already in the trace name).
> >
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > ---
> > hw/block/nvme.c | 19 +++++++++++++++++++
> > hw/block/nvme.h | 14 ++++++++++++++
> > hw/block/trace-events | 13 +++++++++++--
> > 3 files changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 71b388aa0e20..f5d9148f0936 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -331,6 +331,8 @@ static void nvme_post_cqes(void *opaque)
> > static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
> > {
> > assert(cq->cqid == req->sq->cqid);
> > + trace_pci_nvme_enqueue_req_completion(nvme_cid(req), cq->cqid,
> > + req->status);
> > QTAILQ_REMOVE(&req->sq->out_req_list, req, entry);
> > QTAILQ_INSERT_TAIL(&cq->req_list, req, entry);
> > timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
> > @@ -343,6 +345,8 @@ static void nvme_rw_cb(void *opaque, int ret)
> > NvmeCtrl *n = sq->ctrl;
> > NvmeCQueue *cq = n->cq[sq->cqid];
> >
> > + trace_pci_nvme_rw_cb(nvme_cid(req));
> > +
> > if (!ret) {
> > block_acct_done(blk_get_stats(n->conf.blk), &req->acct);
> > req->status = NVME_SUCCESS;
> > @@ -378,6 +382,8 @@ static uint16_t nvme_write_zeros(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
> > uint64_t offset = slba << data_shift;
> > uint32_t count = nlb << data_shift;
> >
> > + trace_pci_nvme_write_zeroes(nvme_cid(req), slba, nlb);
> > +
> > if (unlikely(slba + nlb > ns->id_ns.nsze)) {
> > trace_pci_nvme_err_invalid_lba_range(slba, nlb, ns->id_ns.nsze);
> > return NVME_LBA_RANGE | NVME_DNR;
> > @@ -445,6 +451,8 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > NvmeNamespace *ns;
> > uint32_t nsid = le32_to_cpu(cmd->nsid);
> >
> > + trace_pci_nvme_io_cmd(nvme_cid(req), nsid, nvme_sqid(req), cmd->opcode);
> > +
> > if (unlikely(nsid == 0 || nsid > n->num_namespaces)) {
> > trace_pci_nvme_err_invalid_ns(nsid, n->num_namespaces);
> > return NVME_INVALID_NSID | NVME_DNR;
> > @@ -876,6 +884,8 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> >
> > static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > {
> > + trace_pci_nvme_admin_cmd(nvme_cid(req), nvme_sqid(req), cmd->opcode);
> > +
> > switch (cmd->opcode) {
> > case NVME_ADM_CMD_DELETE_SQ:
> > return nvme_del_sq(n, cmd);
> > @@ -1204,6 +1214,8 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
> > uint8_t *ptr = (uint8_t *)&n->bar;
> > uint64_t val = 0;
> >
> > + trace_pci_nvme_mmio_read(addr);
> > +
> > if (unlikely(addr & (sizeof(uint32_t) - 1))) {
> > NVME_GUEST_ERR(pci_nvme_ub_mmiord_misaligned32,
> > "MMIO read not 32-bit aligned,"
> > @@ -1273,6 +1285,8 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
> > return;
> > }
> >
> > + trace_pci_nvme_mmio_doorbell_cq(cq->cqid, new_head);
> > +
> > start_sqs = nvme_cq_full(cq) ? 1 : 0;
> > cq->head = new_head;
> > if (start_sqs) {
> > @@ -1311,6 +1325,8 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
> > return;
> > }
> >
> > + trace_pci_nvme_mmio_doorbell_sq(sq->sqid, new_tail);
> > +
> > sq->tail = new_tail;
> > timer_mod(sq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
> > }
> > @@ -1320,6 +1336,9 @@ static void nvme_mmio_write(void *opaque, hwaddr addr, uint64_t data,
> > unsigned size)
> > {
> > NvmeCtrl *n = (NvmeCtrl *)opaque;
> > +
> > + trace_pci_nvme_mmio_write(addr, data);
> > +
> > if (addr < sizeof(n->bar)) {
> > nvme_write_bar(n, addr, data, size);
> > } else if (addr >= 0x1000) {
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index 1d30c0bca283..1bf5c80ed843 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -115,4 +115,18 @@ static inline uint64_t nvme_ns_nlbas(NvmeCtrl *n, NvmeNamespace *ns)
> > return n->ns_size >> nvme_ns_lbads(ns);
> > }
> >
> > +static inline uint16_t nvme_cid(NvmeRequest *req)
> > +{
> > + if (req) {
> > + return le16_to_cpu(req->cqe.cid);
> > + }
> > +
> > + return 0xffff;
>
> In this case I find the inverted logic easier. Matter
> of taste :)
>
Your taste is definitely better than mine. I'll queue it up for a style
fix ;)
> > +}
> > +
> > +static inline uint16_t nvme_sqid(NvmeRequest *req)
> > +{
> > + return le16_to_cpu(req->sq->sqid);
> > +}
>
> Later I'd prefer we move these out of the header, and remove
> the inline attribute.
> I.e. nvme_ns_nlbas() is only used once.
>
> OK for now.
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
I agree, I'll also queue that up and include it if the series calls for
a v3.
> > +
> > #endif /* HW_NVME_H */
> > diff --git a/hw/block/trace-events b/hw/block/trace-events
> > index 958fcc5508d1..c40c0d2e4b28 100644
> > --- a/hw/block/trace-events
> > +++ b/hw/block/trace-events
> > @@ -33,19 +33,28 @@ pci_nvme_irq_msix(uint32_t vector) "raising MSI-X IRQ vector %u"
> > pci_nvme_irq_pin(void) "pulsing IRQ pin"
> > pci_nvme_irq_masked(void) "IRQ is masked"
> > pci_nvme_dma_read(uint64_t prp1, uint64_t prp2) "DMA read, prp1=0x%"PRIx64" prp2=0x%"PRIx64""
> > +pci_nvme_io_cmd(uint16_t cid, uint32_t nsid, uint16_t sqid, uint8_t opcode) "cid %"PRIu16" nsid %"PRIu32" sqid %"PRIu16" opc 0x%"PRIx8""
> > +pci_nvme_admin_cmd(uint16_t cid, uint16_t sqid, uint8_t opcode) "cid %"PRIu16" sqid %"PRIu16" opc 0x%"PRIx8""
> > pci_nvme_rw(const char *verb, uint32_t blk_count, uint64_t byte_count, uint64_t lba) "%s %"PRIu32" blocks (%"PRIu64" bytes) from LBA %"PRIu64""
> > +pci_nvme_rw_cb(uint16_t cid) "cid %"PRIu16""
> > +pci_nvme_write_zeroes(uint16_t cid, uint64_t slba, uint32_t nlb) "cid %"PRIu16" slba %"PRIu64" nlb %"PRIu32""
> > pci_nvme_create_sq(uint64_t addr, uint16_t sqid, uint16_t cqid, uint16_t qsize, uint16_t qflags) "create submission queue, addr=0x%"PRIx64", sqid=%"PRIu16", cqid=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16""
> > pci_nvme_create_cq(uint64_t addr, uint16_t cqid, uint16_t vector, uint16_t size, uint16_t qflags, int ien) "create completion queue, addr=0x%"PRIx64", cqid=%"PRIu16", vector=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16", ien=%d"
> > pci_nvme_del_sq(uint16_t qid) "deleting submission queue sqid=%"PRIu16""
> > pci_nvme_del_cq(uint16_t cqid) "deleted completion queue, cqid=%"PRIu16""
> > pci_nvme_identify_ctrl(void) "identify controller"
> > -pci_nvme_identify_ns(uint16_t ns) "identify namespace, nsid=%"PRIu16""
> > -pci_nvme_identify_nslist(uint16_t ns) "identify namespace list, nsid=%"PRIu16""
> > +pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
> > +pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
> > pci_nvme_getfeat_vwcache(const char* result) "get feature volatile write cache, result=%s"
> > pci_nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
> > pci_nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
> > pci_nvme_setfeat_timestamp(uint64_t ts) "set feature timestamp = 0x%"PRIx64""
> > pci_nvme_getfeat_timestamp(uint64_t ts) "get feature timestamp = 0x%"PRIx64""
> > +pci_nvme_enqueue_req_completion(uint16_t cid, uint16_t cqid, uint16_t status) "cid %"PRIu16" cqid %"PRIu16" status 0x%"PRIx16""
> > +pci_nvme_mmio_read(uint64_t addr) "addr 0x%"PRIx64""
> > +pci_nvme_mmio_write(uint64_t addr, uint64_t data) "addr 0x%"PRIx64" data 0x%"PRIx64""
> > +pci_nvme_mmio_doorbell_cq(uint16_t cqid, uint16_t new_head) "cqid %"PRIu16" new_head %"PRIu16""
> > +pci_nvme_mmio_doorbell_sq(uint16_t sqid, uint16_t new_tail) "cqid %"PRIu16" new_tail %"PRIu16""
> > pci_nvme_mmio_intm_set(uint64_t data, uint64_t new_mask) "wrote MMIO, interrupt mask set, data=0x%"PRIx64", new_mask=0x%"PRIx64""
> > pci_nvme_mmio_intm_clr(uint64_t data, uint64_t new_mask) "wrote MMIO, interrupt mask clr, data=0x%"PRIx64", new_mask=0x%"PRIx64""
> > pci_nvme_mmio_cfg(uint64_t data) "wrote MMIO, config controller config=0x%"PRIx64""
> >
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 03/18] hw/block/nvme: add support for the abort command
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
2020-07-03 6:34 ` [PATCH v2 01/18] hw/block/nvme: bump spec data structures " Klaus Jensen
2020-07-03 6:34 ` [PATCH v2 02/18] hw/block/nvme: additional tracing Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 6:34 ` [PATCH v2 04/18] hw/block/nvme: add temperature threshold feature Klaus Jensen
` (14 subsequent siblings)
17 siblings, 0 replies; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
Required for compliance with NVMe revision 1.3d. See NVM Express 1.3d,
Section 5.1 ("Abort command").
The Abort command is a best effort command; for now, the device always
fails to abort the given command.
Signed-off-by: Klaus Jensen <klaus.jensen@cnexlabs.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Acked-by: Keith Busch <kbusch@kernel.org>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
hw/block/nvme.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index f5d9148f0936..b7037a7d3504 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -761,6 +761,18 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
}
}
+static uint16_t nvme_abort(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
+{
+ uint16_t sqid = le32_to_cpu(cmd->cdw10) & 0xffff;
+
+ req->cqe.result = 1;
+ if (nvme_check_sqid(n, sqid)) {
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+
+ return NVME_SUCCESS;
+}
+
static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
{
trace_pci_nvme_setfeat_timestamp(ts);
@@ -897,6 +909,8 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
return nvme_create_cq(n, cmd);
case NVME_ADM_CMD_IDENTIFY:
return nvme_identify(n, cmd);
+ case NVME_ADM_CMD_ABORT:
+ return nvme_abort(n, cmd, req);
case NVME_ADM_CMD_SET_FEATURES:
return nvme_set_feature(n, cmd, req);
case NVME_ADM_CMD_GET_FEATURES:
@@ -1582,6 +1596,19 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
id->ieee[1] = 0x02;
id->ieee[2] = 0xb3;
id->oacs = cpu_to_le16(0);
+
+ /*
+ * Because the controller always completes the Abort command immediately,
+ * there can never be more than one concurrently executing Abort command,
+ * so this value is never used for anything. Note that there can easily be
+ * many Abort commands in the queues, but they are not considered
+ * "executing" until processed by nvme_abort.
+ *
+ * The specification recommends a value of 3 for Abort Command Limit (four
+ * concurrently outstanding Abort commands), so lets use that though it is
+ * inconsequential.
+ */
+ id->acl = 3;
id->frmw = 7 << 1;
id->lpa = 1 << 0;
id->sqes = (0x6 << 4) | 0x6;
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v2 04/18] hw/block/nvme: add temperature threshold feature
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
` (2 preceding siblings ...)
2020-07-03 6:34 ` [PATCH v2 03/18] hw/block/nvme: add support for the abort command Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 8:08 ` Philippe Mathieu-Daudé
2020-07-03 6:34 ` [PATCH v2 05/18] hw/block/nvme: mark fw slot 1 as read-only Klaus Jensen
` (13 subsequent siblings)
17 siblings, 1 reply; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
It might seem weird to implement this feature for an emulated device,
but it is mandatory to support and the feature is useful for testing
asynchronous event request support, which will be added in a later
patch.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Acked-by: Keith Busch <kbusch@kernel.org>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
---
hw/block/nvme.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
hw/block/nvme.h | 1 +
include/block/nvme.h | 5 ++++-
3 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index b7037a7d3504..5ca50646369e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -59,6 +59,9 @@
#define NVME_DB_SIZE 4
#define NVME_CMB_BIR 2
#define NVME_PMR_BIR 2
+#define NVME_TEMPERATURE 0x143
+#define NVME_TEMPERATURE_WARNING 0x157
+#define NVME_TEMPERATURE_CRITICAL 0x175
#define NVME_GUEST_ERR(trace, fmt, ...) \
do { \
@@ -827,9 +830,31 @@ static uint16_t nvme_get_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd)
static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
{
uint32_t dw10 = le32_to_cpu(cmd->cdw10);
+ uint32_t dw11 = le32_to_cpu(cmd->cdw11);
uint32_t result;
switch (dw10) {
+ case NVME_TEMPERATURE_THRESHOLD:
+ result = 0;
+
+ /*
+ * The controller only implements the Composite Temperature sensor, so
+ * return 0 for all other sensors.
+ */
+ if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
+ break;
+ }
+
+ switch (NVME_TEMP_THSEL(dw11)) {
+ case NVME_TEMP_THSEL_OVER:
+ result = cpu_to_le16(n->features.temp_thresh_hi);
+ break;
+ case NVME_TEMP_THSEL_UNDER:
+ result = cpu_to_le16(n->features.temp_thresh_low);
+ break;
+ }
+
+ break;
case NVME_VOLATILE_WRITE_CACHE:
result = blk_enable_write_cache(n->conf.blk);
trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
@@ -874,6 +899,23 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
uint32_t dw11 = le32_to_cpu(cmd->cdw11);
switch (dw10) {
+ case NVME_TEMPERATURE_THRESHOLD:
+ if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
+ break;
+ }
+
+ switch (NVME_TEMP_THSEL(dw11)) {
+ case NVME_TEMP_THSEL_OVER:
+ n->features.temp_thresh_hi = NVME_TEMP_TMPTH(dw11);
+ break;
+ case NVME_TEMP_THSEL_UNDER:
+ n->features.temp_thresh_low = NVME_TEMP_TMPTH(dw11);
+ break;
+ default:
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+
+ break;
case NVME_VOLATILE_WRITE_CACHE:
blk_set_enable_write_cache(n->conf.blk, dw11 & 1);
break;
@@ -1454,6 +1496,7 @@ static void nvme_init_state(NvmeCtrl *n)
n->namespaces = g_new0(NvmeNamespace, n->num_namespaces);
n->sq = g_new0(NvmeSQueue *, n->params.max_ioqpairs + 1);
n->cq = g_new0(NvmeCQueue *, n->params.max_ioqpairs + 1);
+ n->features.temp_thresh_hi = NVME_TEMPERATURE_WARNING;
}
static void nvme_init_blk(NvmeCtrl *n, Error **errp)
@@ -1611,6 +1654,11 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
id->acl = 3;
id->frmw = 7 << 1;
id->lpa = 1 << 0;
+
+ /* recommended default value (~70 C) */
+ id->wctemp = cpu_to_le16(NVME_TEMPERATURE_WARNING);
+ id->cctemp = cpu_to_le16(NVME_TEMPERATURE_CRITICAL);
+
id->sqes = (0x6 << 4) | 0x6;
id->cqes = (0x4 << 4) | 0x4;
id->nn = cpu_to_le32(n->num_namespaces);
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 1bf5c80ed843..3acde10e1d2a 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -107,6 +107,7 @@ typedef struct NvmeCtrl {
NvmeSQueue admin_sq;
NvmeCQueue admin_cq;
NvmeIdCtrl id_ctrl;
+ NvmeFeatureVal features;
} NvmeCtrl;
/* calculate the number of LBAs that the namespace can accomodate */
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 2a80d2a7ed89..d2c457695b38 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -860,7 +860,10 @@ enum NvmeIdCtrlOncs {
typedef struct NvmeFeatureVal {
uint32_t arbitration;
uint32_t power_mgmt;
- uint32_t temp_thresh;
+ struct {
+ uint16_t temp_thresh_hi;
+ uint16_t temp_thresh_low;
+ };
uint32_t err_rec;
uint32_t volatile_wc;
uint32_t num_queues;
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v2 04/18] hw/block/nvme: add temperature threshold feature
2020-07-03 6:34 ` [PATCH v2 04/18] hw/block/nvme: add temperature threshold feature Klaus Jensen
@ 2020-07-03 8:08 ` Philippe Mathieu-Daudé
2020-07-03 8:18 ` Klaus Jensen
0 siblings, 1 reply; 42+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-03 8:08 UTC (permalink / raw)
To: Klaus Jensen, qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Keith Busch, Javier Gonzalez, Maxim Levitsky
On 7/3/20 8:34 AM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> It might seem weird to implement this feature for an emulated device,
> but it is mandatory to support and the feature is useful for testing
> asynchronous event request support, which will be added in a later
> patch.
It might be interesting to plug that to the "temperature sensor
interface" I suggested here (I'll rework on it during 5.2):
https://www.mail-archive.com/qemu-block@nongnu.org/msg65192.html
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Acked-by: Keith Busch <kbusch@kernel.org>
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> hw/block/nvme.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
> hw/block/nvme.h | 1 +
> include/block/nvme.h | 5 ++++-
> 3 files changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index b7037a7d3504..5ca50646369e 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -59,6 +59,9 @@
> #define NVME_DB_SIZE 4
> #define NVME_CMB_BIR 2
> #define NVME_PMR_BIR 2
> +#define NVME_TEMPERATURE 0x143
> +#define NVME_TEMPERATURE_WARNING 0x157
> +#define NVME_TEMPERATURE_CRITICAL 0x175
>
> #define NVME_GUEST_ERR(trace, fmt, ...) \
> do { \
> @@ -827,9 +830,31 @@ static uint16_t nvme_get_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd)
> static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> {
> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> + uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> uint32_t result;
>
> switch (dw10) {
> + case NVME_TEMPERATURE_THRESHOLD:
> + result = 0;
> +
> + /*
> + * The controller only implements the Composite Temperature sensor, so
> + * return 0 for all other sensors.
> + */
> + if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
> + break;
> + }
> +
> + switch (NVME_TEMP_THSEL(dw11)) {
> + case NVME_TEMP_THSEL_OVER:
> + result = cpu_to_le16(n->features.temp_thresh_hi);
> + break;
> + case NVME_TEMP_THSEL_UNDER:
> + result = cpu_to_le16(n->features.temp_thresh_low);
> + break;
> + }
> +
> + break;
> case NVME_VOLATILE_WRITE_CACHE:
> result = blk_enable_write_cache(n->conf.blk);
> trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> @@ -874,6 +899,23 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
>
> switch (dw10) {
> + case NVME_TEMPERATURE_THRESHOLD:
> + if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
> + break;
> + }
> +
> + switch (NVME_TEMP_THSEL(dw11)) {
> + case NVME_TEMP_THSEL_OVER:
> + n->features.temp_thresh_hi = NVME_TEMP_TMPTH(dw11);
> + break;
> + case NVME_TEMP_THSEL_UNDER:
> + n->features.temp_thresh_low = NVME_TEMP_TMPTH(dw11);
> + break;
> + default:
> + return NVME_INVALID_FIELD | NVME_DNR;
> + }
> +
> + break;
> case NVME_VOLATILE_WRITE_CACHE:
> blk_set_enable_write_cache(n->conf.blk, dw11 & 1);
> break;
> @@ -1454,6 +1496,7 @@ static void nvme_init_state(NvmeCtrl *n)
> n->namespaces = g_new0(NvmeNamespace, n->num_namespaces);
> n->sq = g_new0(NvmeSQueue *, n->params.max_ioqpairs + 1);
> n->cq = g_new0(NvmeCQueue *, n->params.max_ioqpairs + 1);
> + n->features.temp_thresh_hi = NVME_TEMPERATURE_WARNING;
> }
>
> static void nvme_init_blk(NvmeCtrl *n, Error **errp)
> @@ -1611,6 +1654,11 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
> id->acl = 3;
> id->frmw = 7 << 1;
> id->lpa = 1 << 0;
> +
> + /* recommended default value (~70 C) */
> + id->wctemp = cpu_to_le16(NVME_TEMPERATURE_WARNING);
> + id->cctemp = cpu_to_le16(NVME_TEMPERATURE_CRITICAL);
> +
> id->sqes = (0x6 << 4) | 0x6;
> id->cqes = (0x4 << 4) | 0x4;
> id->nn = cpu_to_le32(n->num_namespaces);
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 1bf5c80ed843..3acde10e1d2a 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -107,6 +107,7 @@ typedef struct NvmeCtrl {
> NvmeSQueue admin_sq;
> NvmeCQueue admin_cq;
> NvmeIdCtrl id_ctrl;
> + NvmeFeatureVal features;
> } NvmeCtrl;
>
> /* calculate the number of LBAs that the namespace can accomodate */
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 2a80d2a7ed89..d2c457695b38 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -860,7 +860,10 @@ enum NvmeIdCtrlOncs {
> typedef struct NvmeFeatureVal {
> uint32_t arbitration;
> uint32_t power_mgmt;
> - uint32_t temp_thresh;
> + struct {
> + uint16_t temp_thresh_hi;
> + uint16_t temp_thresh_low;
> + };
> uint32_t err_rec;
> uint32_t volatile_wc;
> uint32_t num_queues;
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 04/18] hw/block/nvme: add temperature threshold feature
2020-07-03 8:08 ` Philippe Mathieu-Daudé
@ 2020-07-03 8:18 ` Klaus Jensen
0 siblings, 0 replies; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 8:18 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, qemu-block, Dmitry Fomichev, Klaus Jensen,
qemu-devel, Max Reitz, Keith Busch, Javier Gonzalez,
Maxim Levitsky
On Jul 3 10:08, Philippe Mathieu-Daudé wrote:
> On 7/3/20 8:34 AM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > It might seem weird to implement this feature for an emulated device,
> > but it is mandatory to support and the feature is useful for testing
> > asynchronous event request support, which will be added in a later
> > patch.
>
> It might be interesting to plug that to the "temperature sensor
> interface" I suggested here (I'll rework on it during 5.2):
> https://www.mail-archive.com/qemu-block@nongnu.org/msg65192.html
>
That would be pretty cool, since currently only the thresholds can be
changed to cause a reaction.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 05/18] hw/block/nvme: mark fw slot 1 as read-only
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
` (3 preceding siblings ...)
2020-07-03 6:34 ` [PATCH v2 04/18] hw/block/nvme: add temperature threshold feature Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 6:34 ` [PATCH v2 06/18] hw/block/nvme: add support for the get log page command Klaus Jensen
` (12 subsequent siblings)
17 siblings, 0 replies; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
Mark firmware slot 1 as read-only and only support that slot.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
hw/block/nvme.c | 3 ++-
include/block/nvme.h | 4 ++++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5ca50646369e..f8e91a6965ed 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -62,6 +62,7 @@
#define NVME_TEMPERATURE 0x143
#define NVME_TEMPERATURE_WARNING 0x157
#define NVME_TEMPERATURE_CRITICAL 0x175
+#define NVME_NUM_FW_SLOTS 1
#define NVME_GUEST_ERR(trace, fmt, ...) \
do { \
@@ -1652,7 +1653,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
* inconsequential.
*/
id->acl = 3;
- id->frmw = 7 << 1;
+ id->frmw = (NVME_NUM_FW_SLOTS << 1) | NVME_FRMW_SLOT1_RO;
id->lpa = 1 << 0;
/* recommended default value (~70 C) */
diff --git a/include/block/nvme.h b/include/block/nvme.h
index d2c457695b38..d639e8bbee92 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -842,6 +842,10 @@ enum NvmeIdCtrlOncs {
NVME_ONCS_TIMESTAMP = 1 << 6,
};
+enum NvmeIdCtrlFrmw {
+ NVME_FRMW_SLOT1_RO = 1 << 0,
+};
+
#define NVME_CTRL_SQES_MIN(sqes) ((sqes) & 0xf)
#define NVME_CTRL_SQES_MAX(sqes) (((sqes) >> 4) & 0xf)
#define NVME_CTRL_CQES_MIN(cqes) ((cqes) & 0xf)
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v2 06/18] hw/block/nvme: add support for the get log page command
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
` (4 preceding siblings ...)
2020-07-03 6:34 ` [PATCH v2 05/18] hw/block/nvme: mark fw slot 1 as read-only Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 6:34 ` [PATCH v2 07/18] hw/block/nvme: add support for the asynchronous event request command Klaus Jensen
` (11 subsequent siblings)
17 siblings, 0 replies; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
Add support for the Get Log Page command and basic implementations of
the mandatory Error Information, SMART / Health Information and Firmware
Slot Information log pages.
In violation of the specification, the SMART / Health Information log
page does not persist information over the lifetime of the controller
because the device has no place to store such persistent state.
Note that the LPA field in the Identify Controller data structure
intentionally has bit 0 cleared because there is no namespace specific
information in the SMART / Health information log page.
Required for compliance with NVMe revision 1.3d. See NVM Express 1.3d,
Section 5.14 ("Get Log Page command").
Signed-off-by: Klaus Jensen <klaus.jensen@cnexlabs.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Acked-by: Keith Busch <kbusch@kernel.org>
---
hw/block/nvme.c | 140 +++++++++++++++++++++++++++++++++++++++++-
hw/block/nvme.h | 2 +
hw/block/trace-events | 2 +
include/block/nvme.h | 8 ++-
4 files changed, 149 insertions(+), 3 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index f8e91a6965ed..0c018fd952ae 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -592,6 +592,140 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeCmd *cmd)
return NVME_SUCCESS;
}
+static uint16_t nvme_smart_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t buf_len,
+ uint64_t off, NvmeRequest *req)
+{
+ uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
+ uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
+ uint32_t nsid = le32_to_cpu(cmd->nsid);
+
+ uint32_t trans_len;
+ time_t current_ms;
+ uint64_t units_read = 0, units_written = 0;
+ uint64_t read_commands = 0, write_commands = 0;
+ NvmeSmartLog smart;
+ BlockAcctStats *s;
+
+ if (nsid && nsid != 0xffffffff) {
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+
+ s = blk_get_stats(n->conf.blk);
+
+ units_read = s->nr_bytes[BLOCK_ACCT_READ] >> BDRV_SECTOR_BITS;
+ units_written = s->nr_bytes[BLOCK_ACCT_WRITE] >> BDRV_SECTOR_BITS;
+ read_commands = s->nr_ops[BLOCK_ACCT_READ];
+ write_commands = s->nr_ops[BLOCK_ACCT_WRITE];
+
+ if (off > sizeof(smart)) {
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+
+ trans_len = MIN(sizeof(smart) - off, buf_len);
+
+ memset(&smart, 0x0, sizeof(smart));
+
+ smart.data_units_read[0] = cpu_to_le64(units_read / 1000);
+ smart.data_units_written[0] = cpu_to_le64(units_written / 1000);
+ smart.host_read_commands[0] = cpu_to_le64(read_commands);
+ smart.host_write_commands[0] = cpu_to_le64(write_commands);
+
+ smart.temperature = cpu_to_le16(n->temperature);
+
+ if ((n->temperature >= n->features.temp_thresh_hi) ||
+ (n->temperature <= n->features.temp_thresh_low)) {
+ smart.critical_warning |= NVME_SMART_TEMPERATURE;
+ }
+
+ current_ms = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
+ smart.power_on_hours[0] =
+ cpu_to_le64((((current_ms - n->starttime_ms) / 1000) / 60) / 60);
+
+ return nvme_dma_read_prp(n, (uint8_t *) &smart + off, trans_len, prp1,
+ prp2);
+}
+
+static uint16_t nvme_fw_log_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t buf_len,
+ uint64_t off, NvmeRequest *req)
+{
+ uint32_t trans_len;
+ uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
+ uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
+ NvmeFwSlotInfoLog fw_log = {
+ .afi = 0x1,
+ };
+
+ strpadcpy((char *)&fw_log.frs1, sizeof(fw_log.frs1), "1.0", ' ');
+
+ if (off > sizeof(fw_log)) {
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+
+ trans_len = MIN(sizeof(fw_log) - off, buf_len);
+
+ return nvme_dma_read_prp(n, (uint8_t *) &fw_log + off, trans_len, prp1,
+ prp2);
+}
+
+static uint16_t nvme_error_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t buf_len,
+ uint64_t off, NvmeRequest *req)
+{
+ uint32_t trans_len;
+ uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
+ uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
+ NvmeErrorLog errlog;
+
+ if (off > sizeof(errlog)) {
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+
+ memset(&errlog, 0x0, sizeof(errlog));
+
+ trans_len = MIN(sizeof(errlog) - off, buf_len);
+
+ return nvme_dma_read_prp(n, (uint8_t *)&errlog, trans_len, prp1, prp2);
+}
+
+static uint16_t nvme_get_log(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
+{
+ uint32_t dw10 = le32_to_cpu(cmd->cdw10);
+ uint32_t dw11 = le32_to_cpu(cmd->cdw11);
+ uint32_t dw12 = le32_to_cpu(cmd->cdw12);
+ uint32_t dw13 = le32_to_cpu(cmd->cdw13);
+ uint8_t lid = dw10 & 0xff;
+ uint8_t lsp = (dw10 >> 8) & 0xf;
+ uint8_t rae = (dw10 >> 15) & 0x1;
+ uint32_t numdl, numdu;
+ uint64_t off, lpol, lpou;
+ size_t len;
+
+ numdl = (dw10 >> 16);
+ numdu = (dw11 & 0xffff);
+ lpol = dw12;
+ lpou = dw13;
+
+ len = (((numdu << 16) | numdl) + 1) << 2;
+ off = (lpou << 32ULL) | lpol;
+
+ if (off & 0x3) {
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+
+ trace_pci_nvme_get_log(nvme_cid(req), lid, lsp, rae, len, off);
+
+ switch (lid) {
+ case NVME_LOG_ERROR_INFO:
+ return nvme_error_info(n, cmd, len, off, req);
+ case NVME_LOG_SMART_INFO:
+ return nvme_smart_info(n, cmd, len, off, req);
+ case NVME_LOG_FW_SLOT_INFO:
+ return nvme_fw_log_info(n, cmd, len, off, req);
+ default:
+ trace_pci_nvme_err_invalid_log_page(nvme_cid(req), lid);
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+}
+
static void nvme_free_cq(NvmeCQueue *cq, NvmeCtrl *n)
{
n->cq[cq->cqid] = NULL;
@@ -946,6 +1080,8 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
return nvme_del_sq(n, cmd);
case NVME_ADM_CMD_CREATE_SQ:
return nvme_create_sq(n, cmd);
+ case NVME_ADM_CMD_GET_LOG_PAGE:
+ return nvme_get_log(n, cmd, req);
case NVME_ADM_CMD_DELETE_CQ:
return nvme_del_cq(n, cmd);
case NVME_ADM_CMD_CREATE_CQ:
@@ -1497,7 +1633,9 @@ static void nvme_init_state(NvmeCtrl *n)
n->namespaces = g_new0(NvmeNamespace, n->num_namespaces);
n->sq = g_new0(NvmeSQueue *, n->params.max_ioqpairs + 1);
n->cq = g_new0(NvmeCQueue *, n->params.max_ioqpairs + 1);
+ n->temperature = NVME_TEMPERATURE;
n->features.temp_thresh_hi = NVME_TEMPERATURE_WARNING;
+ n->starttime_ms = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
}
static void nvme_init_blk(NvmeCtrl *n, Error **errp)
@@ -1654,7 +1792,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
*/
id->acl = 3;
id->frmw = (NVME_NUM_FW_SLOTS << 1) | NVME_FRMW_SLOT1_RO;
- id->lpa = 1 << 0;
+ id->lpa = NVME_LPA_EXTENDED;
/* recommended default value (~70 C) */
id->wctemp = cpu_to_le16(NVME_TEMPERATURE_WARNING);
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 3acde10e1d2a..3ddbc3722d7c 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -98,6 +98,8 @@ typedef struct NvmeCtrl {
uint32_t irq_status;
uint64_t host_timestamp; /* Timestamp sent by the host */
uint64_t timestamp_set_qemu_clock_ms; /* QEMU clock time */
+ uint64_t starttime_ms;
+ uint16_t temperature;
HostMemoryBackend *pmrdev;
diff --git a/hw/block/trace-events b/hw/block/trace-events
index c40c0d2e4b28..3330d74e48db 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -45,6 +45,7 @@ pci_nvme_del_cq(uint16_t cqid) "deleted completion queue, cqid=%"PRIu16""
pci_nvme_identify_ctrl(void) "identify controller"
pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
+pci_nvme_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae, uint32_t len, uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" lsp 0x%"PRIx8" rae 0x%"PRIx8" len %"PRIu32" off %"PRIu64""
pci_nvme_getfeat_vwcache(const char* result) "get feature volatile write cache, result=%s"
pci_nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
pci_nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
@@ -94,6 +95,7 @@ pci_nvme_err_invalid_create_cq_qflags(uint16_t qflags) "failed creating completi
pci_nvme_err_invalid_identify_cns(uint16_t cns) "identify, invalid cns=0x%"PRIx16""
pci_nvme_err_invalid_getfeat(int dw10) "invalid get features, dw10=0x%"PRIx32""
pci_nvme_err_invalid_setfeat(uint32_t dw10) "invalid set features, dw10=0x%"PRIx32""
+pci_nvme_err_invalid_log_page(uint16_t cid, uint16_t lid) "cid %"PRIu16" lid 0x%"PRIx16""
pci_nvme_err_startfail_cq(void) "nvme_start_ctrl failed because there are non-admin completion queues"
pci_nvme_err_startfail_sq(void) "nvme_start_ctrl failed because there are non-admin submission queues"
pci_nvme_err_startfail_nbarasq(void) "nvme_start_ctrl failed because the admin submission queue address is null"
diff --git a/include/block/nvme.h b/include/block/nvme.h
index d639e8bbee92..49ce97ae1ab4 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -704,9 +704,9 @@ typedef struct NvmeErrorLog {
uint8_t resv[35];
} NvmeErrorLog;
-typedef struct NvmeSmartLog {
+typedef struct QEMU_PACKED NvmeSmartLog {
uint8_t critical_warning;
- uint8_t temperature[2];
+ uint16_t temperature;
uint8_t available_spare;
uint8_t available_spare_threshold;
uint8_t percentage_used;
@@ -846,6 +846,10 @@ enum NvmeIdCtrlFrmw {
NVME_FRMW_SLOT1_RO = 1 << 0,
};
+enum NvmeIdCtrlLpa {
+ NVME_LPA_EXTENDED = 1 << 2,
+};
+
#define NVME_CTRL_SQES_MIN(sqes) ((sqes) & 0xf)
#define NVME_CTRL_SQES_MAX(sqes) (((sqes) >> 4) & 0xf)
#define NVME_CTRL_CQES_MIN(cqes) ((cqes) & 0xf)
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v2 07/18] hw/block/nvme: add support for the asynchronous event request command
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
` (5 preceding siblings ...)
2020-07-03 6:34 ` [PATCH v2 06/18] hw/block/nvme: add support for the get log page command Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 6:34 ` [PATCH v2 08/18] hw/block/nvme: move NvmeFeatureVal into hw/block/nvme.h Klaus Jensen
` (10 subsequent siblings)
17 siblings, 0 replies; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
Add support for the Asynchronous Event Request command. Required for
compliance with NVMe revision 1.3d. See NVM Express 1.3d, Section 5.2
("Asynchronous Event Request command").
Mostly imported from Keith's qemu-nvme tree. Modified with a max number
of queued events (controllable with the aer_max_queued device
parameter). The spec states that the controller *should* retain
events, so we do best effort here.
Signed-off-by: Klaus Jensen <klaus.jensen@cnexlabs.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Acked-by: Keith Busch <kbusch@kernel.org>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
hw/block/nvme.c | 180 ++++++++++++++++++++++++++++++++++++++++--
hw/block/nvme.h | 10 ++-
hw/block/trace-events | 9 +++
include/block/nvme.h | 8 +-
4 files changed, 198 insertions(+), 9 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 0c018fd952ae..3eac1e231bf9 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -342,6 +342,85 @@ static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
}
+static void nvme_process_aers(void *opaque)
+{
+ NvmeCtrl *n = opaque;
+ NvmeAsyncEvent *event, *next;
+
+ trace_pci_nvme_process_aers(n->aer_queued);
+
+ QTAILQ_FOREACH_SAFE(event, &n->aer_queue, entry, next) {
+ NvmeRequest *req;
+ NvmeAerResult *result;
+
+ /* can't post cqe if there is nothing to complete */
+ if (!n->outstanding_aers) {
+ trace_pci_nvme_no_outstanding_aers();
+ break;
+ }
+
+ /* ignore if masked (cqe posted, but event not cleared) */
+ if (n->aer_mask & (1 << event->result.event_type)) {
+ trace_pci_nvme_aer_masked(event->result.event_type, n->aer_mask);
+ continue;
+ }
+
+ QTAILQ_REMOVE(&n->aer_queue, event, entry);
+ n->aer_queued--;
+
+ n->aer_mask |= 1 << event->result.event_type;
+ n->outstanding_aers--;
+
+ req = n->aer_reqs[n->outstanding_aers];
+
+ result = (NvmeAerResult *) &req->cqe.result;
+ result->event_type = event->result.event_type;
+ result->event_info = event->result.event_info;
+ result->log_page = event->result.log_page;
+ g_free(event);
+
+ req->status = NVME_SUCCESS;
+
+ trace_pci_nvme_aer_post_cqe(result->event_type, result->event_info,
+ result->log_page);
+
+ nvme_enqueue_req_completion(&n->admin_cq, req);
+ }
+}
+
+static void nvme_enqueue_event(NvmeCtrl *n, uint8_t event_type,
+ uint8_t event_info, uint8_t log_page)
+{
+ NvmeAsyncEvent *event;
+
+ trace_pci_nvme_enqueue_event(event_type, event_info, log_page);
+
+ if (n->aer_queued == n->params.aer_max_queued) {
+ trace_pci_nvme_enqueue_event_noqueue(n->aer_queued);
+ return;
+ }
+
+ event = g_new(NvmeAsyncEvent, 1);
+ event->result = (NvmeAerResult) {
+ .event_type = event_type,
+ .event_info = event_info,
+ .log_page = log_page,
+ };
+
+ QTAILQ_INSERT_TAIL(&n->aer_queue, event, entry);
+ n->aer_queued++;
+
+ nvme_process_aers(n);
+}
+
+static void nvme_clear_events(NvmeCtrl *n, uint8_t event_type)
+{
+ n->aer_mask &= ~(1 << event_type);
+ if (!QTAILQ_EMPTY(&n->aer_queue)) {
+ nvme_process_aers(n);
+ }
+}
+
static void nvme_rw_cb(void *opaque, int ret)
{
NvmeRequest *req = opaque;
@@ -592,8 +671,9 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeCmd *cmd)
return NVME_SUCCESS;
}
-static uint16_t nvme_smart_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t buf_len,
- uint64_t off, NvmeRequest *req)
+static uint16_t nvme_smart_info(NvmeCtrl *n, NvmeCmd *cmd, uint8_t rae,
+ uint32_t buf_len, uint64_t off,
+ NvmeRequest *req)
{
uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
@@ -641,6 +721,10 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t buf_len,
smart.power_on_hours[0] =
cpu_to_le64((((current_ms - n->starttime_ms) / 1000) / 60) / 60);
+ if (!rae) {
+ nvme_clear_events(n, NVME_AER_TYPE_SMART);
+ }
+
return nvme_dma_read_prp(n, (uint8_t *) &smart + off, trans_len, prp1,
prp2);
}
@@ -667,14 +751,19 @@ static uint16_t nvme_fw_log_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t buf_len,
prp2);
}
-static uint16_t nvme_error_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t buf_len,
- uint64_t off, NvmeRequest *req)
+static uint16_t nvme_error_info(NvmeCtrl *n, NvmeCmd *cmd, uint8_t rae,
+ uint32_t buf_len, uint64_t off,
+ NvmeRequest *req)
{
uint32_t trans_len;
uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
NvmeErrorLog errlog;
+ if (!rae) {
+ nvme_clear_events(n, NVME_AER_TYPE_ERROR);
+ }
+
if (off > sizeof(errlog)) {
return NVME_INVALID_FIELD | NVME_DNR;
}
@@ -715,9 +804,9 @@ static uint16_t nvme_get_log(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
switch (lid) {
case NVME_LOG_ERROR_INFO:
- return nvme_error_info(n, cmd, len, off, req);
+ return nvme_error_info(n, cmd, rae, len, off, req);
case NVME_LOG_SMART_INFO:
- return nvme_smart_info(n, cmd, len, off, req);
+ return nvme_smart_info(n, cmd, rae, len, off, req);
case NVME_LOG_FW_SLOT_INFO:
return nvme_fw_log_info(n, cmd, len, off, req);
default:
@@ -999,6 +1088,9 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
((n->params.max_ioqpairs - 1) << 16));
trace_pci_nvme_getfeat_numq(result);
break;
+ case NVME_ASYNCHRONOUS_EVENT_CONF:
+ result = cpu_to_le32(n->features.async_config);
+ break;
case NVME_TIMESTAMP:
return nvme_get_feature_timestamp(n, cmd);
default:
@@ -1050,6 +1142,14 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
return NVME_INVALID_FIELD | NVME_DNR;
}
+ if (((n->temperature >= n->features.temp_thresh_hi) ||
+ (n->temperature <= n->features.temp_thresh_low)) &&
+ NVME_AEC_SMART(n->features.async_config) & NVME_SMART_TEMPERATURE) {
+ nvme_enqueue_event(n, NVME_AER_TYPE_SMART,
+ NVME_AER_INFO_SMART_TEMP_THRESH,
+ NVME_LOG_SMART_INFO);
+ }
+
break;
case NVME_VOLATILE_WRITE_CACHE:
blk_set_enable_write_cache(n->conf.blk, dw11 & 1);
@@ -1062,6 +1162,9 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
req->cqe.result = cpu_to_le32((n->params.max_ioqpairs - 1) |
((n->params.max_ioqpairs - 1) << 16));
break;
+ case NVME_ASYNCHRONOUS_EVENT_CONF:
+ n->features.async_config = dw11;
+ break;
case NVME_TIMESTAMP:
return nvme_set_feature_timestamp(n, cmd);
default:
@@ -1071,6 +1174,25 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
return NVME_SUCCESS;
}
+static uint16_t nvme_aer(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
+{
+ trace_pci_nvme_aer(nvme_cid(req));
+
+ if (n->outstanding_aers > n->params.aerl) {
+ trace_pci_nvme_aer_aerl_exceeded();
+ return NVME_AER_LIMIT_EXCEEDED;
+ }
+
+ n->aer_reqs[n->outstanding_aers] = req;
+ n->outstanding_aers++;
+
+ if (!QTAILQ_EMPTY(&n->aer_queue)) {
+ nvme_process_aers(n);
+ }
+
+ return NVME_NO_COMPLETE;
+}
+
static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
{
trace_pci_nvme_admin_cmd(nvme_cid(req), nvme_sqid(req), cmd->opcode);
@@ -1094,6 +1216,8 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
return nvme_set_feature(n, cmd, req);
case NVME_ADM_CMD_GET_FEATURES:
return nvme_get_feature(n, cmd, req);
+ case NVME_ADM_CMD_ASYNC_EV_REQ:
+ return nvme_aer(n, cmd, req);
default:
trace_pci_nvme_err_invalid_admin_opc(cmd->opcode);
return NVME_INVALID_OPCODE | NVME_DNR;
@@ -1148,6 +1272,15 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
}
}
+ while (!QTAILQ_EMPTY(&n->aer_queue)) {
+ NvmeAsyncEvent *event = QTAILQ_FIRST(&n->aer_queue);
+ QTAILQ_REMOVE(&n->aer_queue, event, entry);
+ g_free(event);
+ }
+
+ n->aer_queued = 0;
+ n->outstanding_aers = 0;
+
blk_flush(n->conf.blk);
n->bar.cc = 0;
}
@@ -1244,6 +1377,8 @@ static int nvme_start_ctrl(NvmeCtrl *n)
nvme_set_timestamp(n, 0ULL);
+ QTAILQ_INIT(&n->aer_queue);
+
return 0;
}
@@ -1465,6 +1600,13 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
"completion queue doorbell write"
" for nonexistent queue,"
" sqid=%"PRIu32", ignoring", qid);
+
+ if (n->outstanding_aers) {
+ nvme_enqueue_event(n, NVME_AER_TYPE_ERROR,
+ NVME_AER_INFO_ERR_INVALID_DB_REGISTER,
+ NVME_LOG_ERROR_INFO);
+ }
+
return;
}
@@ -1475,6 +1617,13 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
" beyond queue size, sqid=%"PRIu32","
" new_head=%"PRIu16", ignoring",
qid, new_head);
+
+ if (n->outstanding_aers) {
+ nvme_enqueue_event(n, NVME_AER_TYPE_ERROR,
+ NVME_AER_INFO_ERR_INVALID_DB_VALUE,
+ NVME_LOG_ERROR_INFO);
+ }
+
return;
}
@@ -1505,6 +1654,13 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
"submission queue doorbell write"
" for nonexistent queue,"
" sqid=%"PRIu32", ignoring", qid);
+
+ if (n->outstanding_aers) {
+ nvme_enqueue_event(n, NVME_AER_TYPE_ERROR,
+ NVME_AER_INFO_ERR_INVALID_DB_REGISTER,
+ NVME_LOG_ERROR_INFO);
+ }
+
return;
}
@@ -1515,6 +1671,13 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
" beyond queue size, sqid=%"PRIu32","
" new_tail=%"PRIu16", ignoring",
qid, new_tail);
+
+ if (n->outstanding_aers) {
+ nvme_enqueue_event(n, NVME_AER_TYPE_ERROR,
+ NVME_AER_INFO_ERR_INVALID_DB_VALUE,
+ NVME_LOG_ERROR_INFO);
+ }
+
return;
}
@@ -1636,6 +1799,7 @@ static void nvme_init_state(NvmeCtrl *n)
n->temperature = NVME_TEMPERATURE;
n->features.temp_thresh_hi = NVME_TEMPERATURE_WARNING;
n->starttime_ms = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
+ n->aer_reqs = g_new0(NvmeRequest *, n->params.aerl + 1);
}
static void nvme_init_blk(NvmeCtrl *n, Error **errp)
@@ -1791,6 +1955,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
* inconsequential.
*/
id->acl = 3;
+ id->aerl = n->params.aerl;
id->frmw = (NVME_NUM_FW_SLOTS << 1) | NVME_FRMW_SLOT1_RO;
id->lpa = NVME_LPA_EXTENDED;
@@ -1865,6 +2030,7 @@ static void nvme_exit(PCIDevice *pci_dev)
g_free(n->namespaces);
g_free(n->cq);
g_free(n->sq);
+ g_free(n->aer_reqs);
if (n->params.cmb_size_mb) {
g_free(n->cmbuf);
@@ -1885,6 +2051,8 @@ static Property nvme_props[] = {
DEFINE_PROP_UINT32("num_queues", NvmeCtrl, params.num_queues, 0),
DEFINE_PROP_UINT32("max_ioqpairs", NvmeCtrl, params.max_ioqpairs, 64),
DEFINE_PROP_UINT16("msix_qsize", NvmeCtrl, params.msix_qsize, 65),
+ DEFINE_PROP_UINT8("aerl", NvmeCtrl, params.aerl, 3),
+ DEFINE_PROP_UINT32("aer_max_queued", NvmeCtrl, params.aer_max_queued, 64),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 3ddbc3722d7c..1f64a0e94035 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -9,10 +9,12 @@ typedef struct NvmeParams {
uint32_t max_ioqpairs;
uint16_t msix_qsize;
uint32_t cmb_size_mb;
+ uint8_t aerl;
+ uint32_t aer_max_queued;
} NvmeParams;
typedef struct NvmeAsyncEvent {
- QSIMPLEQ_ENTRY(NvmeAsyncEvent) entry;
+ QTAILQ_ENTRY(NvmeAsyncEvent) entry;
NvmeAerResult result;
} NvmeAsyncEvent;
@@ -94,6 +96,7 @@ typedef struct NvmeCtrl {
uint32_t num_namespaces;
uint32_t max_q_ents;
uint64_t ns_size;
+ uint8_t outstanding_aers;
uint8_t *cmbuf;
uint32_t irq_status;
uint64_t host_timestamp; /* Timestamp sent by the host */
@@ -103,6 +106,11 @@ typedef struct NvmeCtrl {
HostMemoryBackend *pmrdev;
+ uint8_t aer_mask;
+ NvmeRequest **aer_reqs;
+ QTAILQ_HEAD(, NvmeAsyncEvent) aer_queue;
+ int aer_queued;
+
NvmeNamespace *namespaces;
NvmeSQueue **sq;
NvmeCQueue **cq;
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 3330d74e48db..091af16ca7d7 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -51,6 +51,15 @@ pci_nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
pci_nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
pci_nvme_setfeat_timestamp(uint64_t ts) "set feature timestamp = 0x%"PRIx64""
pci_nvme_getfeat_timestamp(uint64_t ts) "get feature timestamp = 0x%"PRIx64""
+pci_nvme_process_aers(int queued) "queued %d"
+pci_nvme_aer(uint16_t cid) "cid %"PRIu16""
+pci_nvme_aer_aerl_exceeded(void) "aerl exceeded"
+pci_nvme_aer_masked(uint8_t type, uint8_t mask) "type 0x%"PRIx8" mask 0x%"PRIx8""
+pci_nvme_aer_post_cqe(uint8_t typ, uint8_t info, uint8_t log_page) "type 0x%"PRIx8" info 0x%"PRIx8" lid 0x%"PRIx8""
+pci_nvme_enqueue_event(uint8_t typ, uint8_t info, uint8_t log_page) "type 0x%"PRIx8" info 0x%"PRIx8" lid 0x%"PRIx8""
+pci_nvme_enqueue_event_noqueue(int queued) "queued %d"
+pci_nvme_enqueue_event_masked(uint8_t typ) "type 0x%"PRIx8""
+pci_nvme_no_outstanding_aers(void) "ignoring event; no outstanding AERs"
pci_nvme_enqueue_req_completion(uint16_t cid, uint16_t cqid, uint16_t status) "cid %"PRIu16" cqid %"PRIu16" status 0x%"PRIx16""
pci_nvme_mmio_read(uint64_t addr) "addr 0x%"PRIx64""
pci_nvme_mmio_write(uint64_t addr, uint64_t data) "addr 0x%"PRIx64" data 0x%"PRIx64""
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 49ce97ae1ab4..2101292ed5e8 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -597,8 +597,8 @@ enum NvmeAsyncEventRequest {
NVME_AER_TYPE_SMART = 1,
NVME_AER_TYPE_IO_SPECIFIC = 6,
NVME_AER_TYPE_VENDOR_SPECIFIC = 7,
- NVME_AER_INFO_ERR_INVALID_SQ = 0,
- NVME_AER_INFO_ERR_INVALID_DB = 1,
+ NVME_AER_INFO_ERR_INVALID_DB_REGISTER = 0,
+ NVME_AER_INFO_ERR_INVALID_DB_VALUE = 1,
NVME_AER_INFO_ERR_DIAG_FAIL = 2,
NVME_AER_INFO_ERR_PERS_INTERNAL_ERR = 3,
NVME_AER_INFO_ERR_TRANS_INTERNAL_ERR = 4,
@@ -899,6 +899,10 @@ typedef struct NvmeFeatureVal {
#define NVME_TEMP_TMPTH(temp) ((temp >> 0) & 0xffff)
+#define NVME_AEC_SMART(aec) (aec & 0xff)
+#define NVME_AEC_NS_ATTR(aec) ((aec >> 8) & 0x1)
+#define NVME_AEC_FW_ACTIVATION(aec) ((aec >> 9) & 0x1)
+
enum NvmeFeatureIds {
NVME_ARBITRATION = 0x1,
NVME_POWER_MANAGEMENT = 0x2,
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v2 08/18] hw/block/nvme: move NvmeFeatureVal into hw/block/nvme.h
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
` (6 preceding siblings ...)
2020-07-03 6:34 ` [PATCH v2 07/18] hw/block/nvme: add support for the asynchronous event request command Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 6:34 ` [PATCH v2 09/18] hw/block/nvme: flush write cache when disabled Klaus Jensen
` (9 subsequent siblings)
17 siblings, 0 replies; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
The NvmeFeatureVal does not belong with the spec-related data structures
in include/block/nvme.h that is shared between the block-level nvme
driver and the emulated nvme device.
Move it into the nvme device specific header file as it is the only
user of the structure. Also, remove the unused members.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
hw/block/nvme.h | 8 ++++++++
include/block/nvme.h | 17 -----------------
2 files changed, 8 insertions(+), 17 deletions(-)
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 1f64a0e94035..f8940435f9ef 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -79,6 +79,14 @@ static inline uint8_t nvme_ns_lbads(NvmeNamespace *ns)
#define NVME(obj) \
OBJECT_CHECK(NvmeCtrl, (obj), TYPE_NVME)
+typedef struct NvmeFeatureVal {
+ struct {
+ uint16_t temp_thresh_hi;
+ uint16_t temp_thresh_low;
+ };
+ uint32_t async_config;
+} NvmeFeatureVal;
+
typedef struct NvmeCtrl {
PCIDevice parent_obj;
MemoryRegion iomem;
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 2101292ed5e8..0dce15af6bcf 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -865,23 +865,6 @@ enum NvmeIdCtrlLpa {
#define NVME_CTRL_SGLS_MPTR_SGL (0x1 << 19)
#define NVME_CTRL_SGLS_ADDR_OFFSET (0x1 << 20)
-typedef struct NvmeFeatureVal {
- uint32_t arbitration;
- uint32_t power_mgmt;
- struct {
- uint16_t temp_thresh_hi;
- uint16_t temp_thresh_low;
- };
- uint32_t err_rec;
- uint32_t volatile_wc;
- uint32_t num_queues;
- uint32_t int_coalescing;
- uint32_t *int_vector_config;
- uint32_t write_atomicity;
- uint32_t async_config;
- uint32_t sw_prog_marker;
-} NvmeFeatureVal;
-
#define NVME_ARB_AB(arb) (arb & 0x7)
#define NVME_ARB_LPW(arb) ((arb >> 8) & 0xff)
#define NVME_ARB_MPW(arb) ((arb >> 16) & 0xff)
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v2 09/18] hw/block/nvme: flush write cache when disabled
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
` (7 preceding siblings ...)
2020-07-03 6:34 ` [PATCH v2 08/18] hw/block/nvme: move NvmeFeatureVal into hw/block/nvme.h Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 6:34 ` [PATCH v2 10/18] hw/block/nvme: fix missing endian conversion Klaus Jensen
` (8 subsequent siblings)
17 siblings, 0 replies; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
If the write cache is disabled with a Set Features command, flush it if
currently enabled.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
hw/block/nvme.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 3eac1e231bf9..f3a5b857bc92 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1152,6 +1152,10 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
break;
case NVME_VOLATILE_WRITE_CACHE:
+ if (!(dw11 & 0x1) && blk_enable_write_cache(n->conf.blk)) {
+ blk_flush(n->conf.blk);
+ }
+
blk_set_enable_write_cache(n->conf.blk, dw11 & 1);
break;
case NVME_NUMBER_OF_QUEUES:
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v2 10/18] hw/block/nvme: fix missing endian conversion
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
` (8 preceding siblings ...)
2020-07-03 6:34 ` [PATCH v2 09/18] hw/block/nvme: flush write cache when disabled Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 8:34 ` Philippe Mathieu-Daudé
2020-07-03 6:34 ` [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters Klaus Jensen
` (7 subsequent siblings)
17 siblings, 1 reply; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
Fix a missing cpu_to conversion.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
hw/block/nvme.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index f3a5b857bc92..ba523f6768bf 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1080,7 +1080,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
break;
case NVME_VOLATILE_WRITE_CACHE:
- result = blk_enable_write_cache(n->conf.blk);
+ result = cpu_to_le32(blk_enable_write_cache(n->conf.blk));
trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
break;
case NVME_NUMBER_OF_QUEUES:
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v2 10/18] hw/block/nvme: fix missing endian conversion
2020-07-03 6:34 ` [PATCH v2 10/18] hw/block/nvme: fix missing endian conversion Klaus Jensen
@ 2020-07-03 8:34 ` Philippe Mathieu-Daudé
2020-07-03 10:11 ` Klaus Jensen
0 siblings, 1 reply; 42+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-03 8:34 UTC (permalink / raw)
To: Klaus Jensen, qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Keith Busch, Javier Gonzalez, Maxim Levitsky
On 7/3/20 8:34 AM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Fix a missing cpu_to conversion.
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
> hw/block/nvme.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index f3a5b857bc92..ba523f6768bf 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1080,7 +1080,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>
> break;
> case NVME_VOLATILE_WRITE_CACHE:
> - result = blk_enable_write_cache(n->conf.blk);
> + result = cpu_to_le32(blk_enable_write_cache(n->conf.blk));
> trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> break;
> case NVME_NUMBER_OF_QUEUES:
>
This doesn't look correct. What you probably want:
-- >8 --
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -815,8 +815,8 @@ static uint16_t nvme_get_feature(NvmeCtrl *n,
NvmeCmd *cmd, NvmeRequest *req)
trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
break;
case NVME_NUMBER_OF_QUEUES:
- result = cpu_to_le32((n->params.max_ioqpairs - 1) |
- ((n->params.max_ioqpairs - 1) << 16));
+ result = (n->params.max_ioqpairs - 1)
+ | ((n->params.max_ioqpairs - 1) << 16);
trace_pci_nvme_getfeat_numq(result);
break;
case NVME_TIMESTAMP:
@@ -825,8 +825,8 @@ static uint16_t nvme_get_feature(NvmeCtrl *n,
NvmeCmd *cmd, NvmeRequest *req)
trace_pci_nvme_err_invalid_getfeat(dw10);
return NVME_INVALID_FIELD | NVME_DNR;
}
+ req->cqe.result = cpu_to_le32(result);
- req->cqe.result = result;
return NVME_SUCCESS;
}
---
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 10/18] hw/block/nvme: fix missing endian conversion
2020-07-03 8:34 ` Philippe Mathieu-Daudé
@ 2020-07-03 10:11 ` Klaus Jensen
0 siblings, 0 replies; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 10:11 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, qemu-block, Klaus Jensen, qemu-devel, Max Reitz,
Keith Busch, Javier Gonzalez
On Jul 3 10:34, Philippe Mathieu-Daudé wrote:
> On 7/3/20 8:34 AM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > Fix a missing cpu_to conversion.
> >
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > ---
> > hw/block/nvme.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index f3a5b857bc92..ba523f6768bf 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1080,7 +1080,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> >
> > break;
> > case NVME_VOLATILE_WRITE_CACHE:
> > - result = blk_enable_write_cache(n->conf.blk);
> > + result = cpu_to_le32(blk_enable_write_cache(n->conf.blk));
> > trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> > break;
> > case NVME_NUMBER_OF_QUEUES:
> >
>
> This doesn't look correct. What you probably want:
>
> -- >8 --
>
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -815,8 +815,8 @@ static uint16_t nvme_get_feature(NvmeCtrl *n,
> NvmeCmd *cmd, NvmeRequest *req)
> trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> break;
> case NVME_NUMBER_OF_QUEUES:
> - result = cpu_to_le32((n->params.max_ioqpairs - 1) |
> - ((n->params.max_ioqpairs - 1) << 16));
> + result = (n->params.max_ioqpairs - 1)
> + | ((n->params.max_ioqpairs - 1) << 16);
> trace_pci_nvme_getfeat_numq(result);
> break;
> case NVME_TIMESTAMP:
> @@ -825,8 +825,8 @@ static uint16_t nvme_get_feature(NvmeCtrl *n,
> NvmeCmd *cmd, NvmeRequest *req)
> trace_pci_nvme_err_invalid_getfeat(dw10);
> return NVME_INVALID_FIELD | NVME_DNR;
> }
> + req->cqe.result = cpu_to_le32(result);
>
> - req->cqe.result = result;
> return NVME_SUCCESS;
> }
> ---
>
I replaced this patch with a new one earlier in the series that does
what you suggest and then let the change trickle down to other patches
that add feature cases.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
` (9 preceding siblings ...)
2020-07-03 6:34 ` [PATCH v2 10/18] hw/block/nvme: fix missing endian conversion Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 8:31 ` Philippe Mathieu-Daudé
2020-07-03 6:34 ` [PATCH v2 12/18] hw/block/nvme: support the get/set features select and save fields Klaus Jensen
` (6 subsequent siblings)
17 siblings, 1 reply; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
Add support for any remaining mandatory controller operating parameters
(features).
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
hw/block/nvme.c | 39 +++++++++++++++++++++++++++++++++------
hw/block/nvme.h | 18 ++++++++++++++++++
hw/block/trace-events | 2 ++
include/block/nvme.h | 7 +++++++
4 files changed, 60 insertions(+), 6 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index ba523f6768bf..affb9a967534 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1056,8 +1056,16 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
uint32_t dw10 = le32_to_cpu(cmd->cdw10);
uint32_t dw11 = le32_to_cpu(cmd->cdw11);
uint32_t result;
+ uint8_t fid = NVME_GETSETFEAT_FID(dw10);
+ uint16_t iv;
- switch (dw10) {
+ trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
+
+ if (!nvme_feature_support[fid]) {
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+
+ switch (fid) {
case NVME_TEMPERATURE_THRESHOLD:
result = 0;
@@ -1088,14 +1096,27 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
((n->params.max_ioqpairs - 1) << 16));
trace_pci_nvme_getfeat_numq(result);
break;
+ case NVME_INTERRUPT_VECTOR_CONF:
+ iv = dw11 & 0xffff;
+ if (iv >= n->params.max_ioqpairs + 1) {
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+
+ result = iv;
+ if (iv == n->admin_cq.vector) {
+ result |= NVME_INTVC_NOCOALESCING;
+ }
+
+ result = cpu_to_le32(result);
+ break;
case NVME_ASYNCHRONOUS_EVENT_CONF:
result = cpu_to_le32(n->features.async_config);
break;
case NVME_TIMESTAMP:
return nvme_get_feature_timestamp(n, cmd);
default:
- trace_pci_nvme_err_invalid_getfeat(dw10);
- return NVME_INVALID_FIELD | NVME_DNR;
+ result = cpu_to_le32(nvme_feature_default[fid]);
+ break;
}
req->cqe.result = result;
@@ -1124,8 +1145,15 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
{
uint32_t dw10 = le32_to_cpu(cmd->cdw10);
uint32_t dw11 = le32_to_cpu(cmd->cdw11);
+ uint8_t fid = NVME_GETSETFEAT_FID(dw10);
- switch (dw10) {
+ trace_pci_nvme_setfeat(nvme_cid(req), fid, dw11);
+
+ if (!nvme_feature_support[fid]) {
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+
+ switch (fid) {
case NVME_TEMPERATURE_THRESHOLD:
if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
break;
@@ -1172,8 +1200,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
case NVME_TIMESTAMP:
return nvme_set_feature_timestamp(n, cmd);
default:
- trace_pci_nvme_err_invalid_setfeat(dw10);
- return NVME_INVALID_FIELD | NVME_DNR;
+ return NVME_FEAT_NOT_CHANGEABLE | NVME_DNR;
}
return NVME_SUCCESS;
}
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index f8940435f9ef..8ad1e3c89cee 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -87,6 +87,24 @@ typedef struct NvmeFeatureVal {
uint32_t async_config;
} NvmeFeatureVal;
+static const uint32_t nvme_feature_default[0x100] = {
+ [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
+};
+
+static const bool nvme_feature_support[0x100] = {
+ [NVME_ARBITRATION] = true,
+ [NVME_POWER_MANAGEMENT] = true,
+ [NVME_TEMPERATURE_THRESHOLD] = true,
+ [NVME_ERROR_RECOVERY] = true,
+ [NVME_VOLATILE_WRITE_CACHE] = true,
+ [NVME_NUMBER_OF_QUEUES] = true,
+ [NVME_INTERRUPT_COALESCING] = true,
+ [NVME_INTERRUPT_VECTOR_CONF] = true,
+ [NVME_WRITE_ATOMICITY] = true,
+ [NVME_ASYNCHRONOUS_EVENT_CONF] = true,
+ [NVME_TIMESTAMP] = true,
+};
+
typedef struct NvmeCtrl {
PCIDevice parent_obj;
MemoryRegion iomem;
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 091af16ca7d7..42e62f4649f8 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -46,6 +46,8 @@ pci_nvme_identify_ctrl(void) "identify controller"
pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
pci_nvme_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae, uint32_t len, uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" lsp 0x%"PRIx8" rae 0x%"PRIx8" len %"PRIu32" off %"PRIu64""
+pci_nvme_getfeat(uint16_t cid, uint8_t fid, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" cdw11 0x%"PRIx32""
+pci_nvme_setfeat(uint16_t cid, uint8_t fid, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" cdw11 0x%"PRIx32""
pci_nvme_getfeat_vwcache(const char* result) "get feature volatile write cache, result=%s"
pci_nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
pci_nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 0dce15af6bcf..406648d4cf94 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -662,6 +662,7 @@ enum NvmeStatusCodes {
NVME_FW_REQ_RESET = 0x010b,
NVME_INVALID_QUEUE_DEL = 0x010c,
NVME_FID_NOT_SAVEABLE = 0x010d,
+ NVME_FEAT_NOT_CHANGEABLE = 0x010e,
NVME_FID_NOT_NSID_SPEC = 0x010f,
NVME_FW_REQ_SUSYSTEM_RESET = 0x0110,
NVME_CONFLICTING_ATTRS = 0x0180,
@@ -866,6 +867,7 @@ enum NvmeIdCtrlLpa {
#define NVME_CTRL_SGLS_ADDR_OFFSET (0x1 << 20)
#define NVME_ARB_AB(arb) (arb & 0x7)
+#define NVME_ARB_AB_NOLIMIT 0x7
#define NVME_ARB_LPW(arb) ((arb >> 8) & 0xff)
#define NVME_ARB_MPW(arb) ((arb >> 16) & 0xff)
#define NVME_ARB_HPW(arb) ((arb >> 24) & 0xff)
@@ -873,6 +875,8 @@ enum NvmeIdCtrlLpa {
#define NVME_INTC_THR(intc) (intc & 0xff)
#define NVME_INTC_TIME(intc) ((intc >> 8) & 0xff)
+#define NVME_INTVC_NOCOALESCING (0x1 << 16)
+
#define NVME_TEMP_THSEL(temp) ((temp >> 20) & 0x3)
#define NVME_TEMP_THSEL_OVER 0x0
#define NVME_TEMP_THSEL_UNDER 0x1
@@ -902,6 +906,9 @@ enum NvmeFeatureIds {
NVME_SOFTWARE_PROGRESS_MARKER = 0x80
};
+#define NVME_GETSETFEAT_FID_MASK 0xff
+#define NVME_GETSETFEAT_FID(dw10) (dw10 & NVME_GETSETFEAT_FID_MASK)
+
typedef struct NvmeRangeType {
uint8_t type;
uint8_t attributes;
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters
2020-07-03 6:34 ` [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters Klaus Jensen
@ 2020-07-03 8:31 ` Philippe Mathieu-Daudé
2020-07-03 8:46 ` Klaus Jensen
0 siblings, 1 reply; 42+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-03 8:31 UTC (permalink / raw)
To: Klaus Jensen, qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Keith Busch, Javier Gonzalez, Maxim Levitsky
On 7/3/20 8:34 AM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Add support for any remaining mandatory controller operating parameters
> (features).
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
> hw/block/nvme.c | 39 +++++++++++++++++++++++++++++++++------
> hw/block/nvme.h | 18 ++++++++++++++++++
> hw/block/trace-events | 2 ++
> include/block/nvme.h | 7 +++++++
> 4 files changed, 60 insertions(+), 6 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index ba523f6768bf..affb9a967534 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1056,8 +1056,16 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> uint32_t result;
> + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> + uint16_t iv;
>
> - switch (dw10) {
> + trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
> +
> + if (!nvme_feature_support[fid]) {
> + return NVME_INVALID_FIELD | NVME_DNR;
> + }
> +
> + switch (fid) {
> case NVME_TEMPERATURE_THRESHOLD:
> result = 0;
>
> @@ -1088,14 +1096,27 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> ((n->params.max_ioqpairs - 1) << 16));
> trace_pci_nvme_getfeat_numq(result);
> break;
> + case NVME_INTERRUPT_VECTOR_CONF:
> + iv = dw11 & 0xffff;
> + if (iv >= n->params.max_ioqpairs + 1) {
> + return NVME_INVALID_FIELD | NVME_DNR;
> + }
> +
> + result = iv;
> + if (iv == n->admin_cq.vector) {
> + result |= NVME_INTVC_NOCOALESCING;
> + }
> +
> + result = cpu_to_le32(result);
> + break;
> case NVME_ASYNCHRONOUS_EVENT_CONF:
> result = cpu_to_le32(n->features.async_config);
> break;
> case NVME_TIMESTAMP:
> return nvme_get_feature_timestamp(n, cmd);
> default:
> - trace_pci_nvme_err_invalid_getfeat(dw10);
> - return NVME_INVALID_FIELD | NVME_DNR;
> + result = cpu_to_le32(nvme_feature_default[fid]);
So here we expect uninitialized fid entries to return 0, right?
> + break;
> }
>
> req->cqe.result = result;
> @@ -1124,8 +1145,15 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> {
> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
>
> - switch (dw10) {
> + trace_pci_nvme_setfeat(nvme_cid(req), fid, dw11);
> +
> + if (!nvme_feature_support[fid]) {
> + return NVME_INVALID_FIELD | NVME_DNR;
> + }
> +
> + switch (fid) {
> case NVME_TEMPERATURE_THRESHOLD:
> if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
> break;
> @@ -1172,8 +1200,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> case NVME_TIMESTAMP:
> return nvme_set_feature_timestamp(n, cmd);
> default:
> - trace_pci_nvme_err_invalid_setfeat(dw10);
> - return NVME_INVALID_FIELD | NVME_DNR;
> + return NVME_FEAT_NOT_CHANGEABLE | NVME_DNR;
> }
> return NVME_SUCCESS;
> }
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index f8940435f9ef..8ad1e3c89cee 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -87,6 +87,24 @@ typedef struct NvmeFeatureVal {
> uint32_t async_config;
> } NvmeFeatureVal;
>
> +static const uint32_t nvme_feature_default[0x100] = {
> + [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> +};
> +
> +static const bool nvme_feature_support[0x100] = {
> + [NVME_ARBITRATION] = true,
> + [NVME_POWER_MANAGEMENT] = true,
> + [NVME_TEMPERATURE_THRESHOLD] = true,
> + [NVME_ERROR_RECOVERY] = true,
> + [NVME_VOLATILE_WRITE_CACHE] = true,
> + [NVME_NUMBER_OF_QUEUES] = true,
> + [NVME_INTERRUPT_COALESCING] = true,
> + [NVME_INTERRUPT_VECTOR_CONF] = true,
> + [NVME_WRITE_ATOMICITY] = true,
> + [NVME_ASYNCHRONOUS_EVENT_CONF] = true,
> + [NVME_TIMESTAMP] = true,
> +};
Nack. No variable assignation in header please.
Move that to the source file.
> +
> typedef struct NvmeCtrl {
> PCIDevice parent_obj;
> MemoryRegion iomem;
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 091af16ca7d7..42e62f4649f8 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -46,6 +46,8 @@ pci_nvme_identify_ctrl(void) "identify controller"
> pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
> pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
> pci_nvme_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae, uint32_t len, uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" lsp 0x%"PRIx8" rae 0x%"PRIx8" len %"PRIu32" off %"PRIu64""
> +pci_nvme_getfeat(uint16_t cid, uint8_t fid, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" cdw11 0x%"PRIx32""
> +pci_nvme_setfeat(uint16_t cid, uint8_t fid, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" cdw11 0x%"PRIx32""
> pci_nvme_getfeat_vwcache(const char* result) "get feature volatile write cache, result=%s"
> pci_nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
> pci_nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 0dce15af6bcf..406648d4cf94 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -662,6 +662,7 @@ enum NvmeStatusCodes {
> NVME_FW_REQ_RESET = 0x010b,
> NVME_INVALID_QUEUE_DEL = 0x010c,
> NVME_FID_NOT_SAVEABLE = 0x010d,
> + NVME_FEAT_NOT_CHANGEABLE = 0x010e,
> NVME_FID_NOT_NSID_SPEC = 0x010f,
> NVME_FW_REQ_SUSYSTEM_RESET = 0x0110,
> NVME_CONFLICTING_ATTRS = 0x0180,
> @@ -866,6 +867,7 @@ enum NvmeIdCtrlLpa {
> #define NVME_CTRL_SGLS_ADDR_OFFSET (0x1 << 20)
>
> #define NVME_ARB_AB(arb) (arb & 0x7)
> +#define NVME_ARB_AB_NOLIMIT 0x7
> #define NVME_ARB_LPW(arb) ((arb >> 8) & 0xff)
> #define NVME_ARB_MPW(arb) ((arb >> 16) & 0xff)
> #define NVME_ARB_HPW(arb) ((arb >> 24) & 0xff)
> @@ -873,6 +875,8 @@ enum NvmeIdCtrlLpa {
> #define NVME_INTC_THR(intc) (intc & 0xff)
> #define NVME_INTC_TIME(intc) ((intc >> 8) & 0xff)
>
> +#define NVME_INTVC_NOCOALESCING (0x1 << 16)
> +
> #define NVME_TEMP_THSEL(temp) ((temp >> 20) & 0x3)
> #define NVME_TEMP_THSEL_OVER 0x0
> #define NVME_TEMP_THSEL_UNDER 0x1
> @@ -902,6 +906,9 @@ enum NvmeFeatureIds {
> NVME_SOFTWARE_PROGRESS_MARKER = 0x80
> };
>
> +#define NVME_GETSETFEAT_FID_MASK 0xff
> +#define NVME_GETSETFEAT_FID(dw10) (dw10 & NVME_GETSETFEAT_FID_MASK)
> +
> typedef struct NvmeRangeType {
> uint8_t type;
> uint8_t attributes;
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters
2020-07-03 8:31 ` Philippe Mathieu-Daudé
@ 2020-07-03 8:46 ` Klaus Jensen
2020-07-03 9:21 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 8:46 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, qemu-block, Dmitry Fomichev, Klaus Jensen,
qemu-devel, Max Reitz, Keith Busch, Javier Gonzalez,
Maxim Levitsky
On Jul 3 10:31, Philippe Mathieu-Daudé wrote:
> On 7/3/20 8:34 AM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > Add support for any remaining mandatory controller operating parameters
> > (features).
> >
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > ---
> > hw/block/nvme.c | 39 +++++++++++++++++++++++++++++++++------
> > hw/block/nvme.h | 18 ++++++++++++++++++
> > hw/block/trace-events | 2 ++
> > include/block/nvme.h | 7 +++++++
> > 4 files changed, 60 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index ba523f6768bf..affb9a967534 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1056,8 +1056,16 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> > uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> > uint32_t result;
> > + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> > + uint16_t iv;
> >
> > - switch (dw10) {
> > + trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
> > +
> > + if (!nvme_feature_support[fid]) {
> > + return NVME_INVALID_FIELD | NVME_DNR;
> > + }
> > +
> > + switch (fid) {
> > case NVME_TEMPERATURE_THRESHOLD:
> > result = 0;
> >
> > @@ -1088,14 +1096,27 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > ((n->params.max_ioqpairs - 1) << 16));
> > trace_pci_nvme_getfeat_numq(result);
> > break;
> > + case NVME_INTERRUPT_VECTOR_CONF:
> > + iv = dw11 & 0xffff;
> > + if (iv >= n->params.max_ioqpairs + 1) {
> > + return NVME_INVALID_FIELD | NVME_DNR;
> > + }
> > +
> > + result = iv;
> > + if (iv == n->admin_cq.vector) {
> > + result |= NVME_INTVC_NOCOALESCING;
> > + }
> > +
> > + result = cpu_to_le32(result);
> > + break;
> > case NVME_ASYNCHRONOUS_EVENT_CONF:
> > result = cpu_to_le32(n->features.async_config);
> > break;
> > case NVME_TIMESTAMP:
> > return nvme_get_feature_timestamp(n, cmd);
> > default:
> > - trace_pci_nvme_err_invalid_getfeat(dw10);
> > - return NVME_INVALID_FIELD | NVME_DNR;
> > + result = cpu_to_le32(nvme_feature_default[fid]);
>
> So here we expect uninitialized fid entries to return 0, right?
>
Yes, if defaults are not 0 (like NVME_ARBITRATION), it is explicitly set.
> > + break;
> > }
> >
> > req->cqe.result = result;
> > @@ -1124,8 +1145,15 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > {
> > uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> > uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> > + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> >
> > - switch (dw10) {
> > + trace_pci_nvme_setfeat(nvme_cid(req), fid, dw11);
> > +
> > + if (!nvme_feature_support[fid]) {
> > + return NVME_INVALID_FIELD | NVME_DNR;
> > + }
> > +
> > + switch (fid) {
> > case NVME_TEMPERATURE_THRESHOLD:
> > if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
> > break;
> > @@ -1172,8 +1200,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > case NVME_TIMESTAMP:
> > return nvme_set_feature_timestamp(n, cmd);
> > default:
> > - trace_pci_nvme_err_invalid_setfeat(dw10);
> > - return NVME_INVALID_FIELD | NVME_DNR;
> > + return NVME_FEAT_NOT_CHANGEABLE | NVME_DNR;
> > }
> > return NVME_SUCCESS;
> > }
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index f8940435f9ef..8ad1e3c89cee 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -87,6 +87,24 @@ typedef struct NvmeFeatureVal {
> > uint32_t async_config;
> > } NvmeFeatureVal;
> >
> > +static const uint32_t nvme_feature_default[0x100] = {
> > + [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> > +};
> > +
> > +static const bool nvme_feature_support[0x100] = {
> > + [NVME_ARBITRATION] = true,
> > + [NVME_POWER_MANAGEMENT] = true,
> > + [NVME_TEMPERATURE_THRESHOLD] = true,
> > + [NVME_ERROR_RECOVERY] = true,
> > + [NVME_VOLATILE_WRITE_CACHE] = true,
> > + [NVME_NUMBER_OF_QUEUES] = true,
> > + [NVME_INTERRUPT_COALESCING] = true,
> > + [NVME_INTERRUPT_VECTOR_CONF] = true,
> > + [NVME_WRITE_ATOMICITY] = true,
> > + [NVME_ASYNCHRONOUS_EVENT_CONF] = true,
> > + [NVME_TIMESTAMP] = true,
> > +};
>
> Nack. No variable assignation in header please.
> Move that to the source file.
>
Understood :)
> > +
> > typedef struct NvmeCtrl {
> > PCIDevice parent_obj;
> > MemoryRegion iomem;
> > diff --git a/hw/block/trace-events b/hw/block/trace-events
> > index 091af16ca7d7..42e62f4649f8 100644
> > --- a/hw/block/trace-events
> > +++ b/hw/block/trace-events
> > @@ -46,6 +46,8 @@ pci_nvme_identify_ctrl(void) "identify controller"
> > pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
> > pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
> > pci_nvme_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae, uint32_t len, uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" lsp 0x%"PRIx8" rae 0x%"PRIx8" len %"PRIu32" off %"PRIu64""
> > +pci_nvme_getfeat(uint16_t cid, uint8_t fid, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" cdw11 0x%"PRIx32""
> > +pci_nvme_setfeat(uint16_t cid, uint8_t fid, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" cdw11 0x%"PRIx32""
> > pci_nvme_getfeat_vwcache(const char* result) "get feature volatile write cache, result=%s"
> > pci_nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
> > pci_nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
> > diff --git a/include/block/nvme.h b/include/block/nvme.h
> > index 0dce15af6bcf..406648d4cf94 100644
> > --- a/include/block/nvme.h
> > +++ b/include/block/nvme.h
> > @@ -662,6 +662,7 @@ enum NvmeStatusCodes {
> > NVME_FW_REQ_RESET = 0x010b,
> > NVME_INVALID_QUEUE_DEL = 0x010c,
> > NVME_FID_NOT_SAVEABLE = 0x010d,
> > + NVME_FEAT_NOT_CHANGEABLE = 0x010e,
> > NVME_FID_NOT_NSID_SPEC = 0x010f,
> > NVME_FW_REQ_SUSYSTEM_RESET = 0x0110,
> > NVME_CONFLICTING_ATTRS = 0x0180,
> > @@ -866,6 +867,7 @@ enum NvmeIdCtrlLpa {
> > #define NVME_CTRL_SGLS_ADDR_OFFSET (0x1 << 20)
> >
> > #define NVME_ARB_AB(arb) (arb & 0x7)
> > +#define NVME_ARB_AB_NOLIMIT 0x7
> > #define NVME_ARB_LPW(arb) ((arb >> 8) & 0xff)
> > #define NVME_ARB_MPW(arb) ((arb >> 16) & 0xff)
> > #define NVME_ARB_HPW(arb) ((arb >> 24) & 0xff)
> > @@ -873,6 +875,8 @@ enum NvmeIdCtrlLpa {
> > #define NVME_INTC_THR(intc) (intc & 0xff)
> > #define NVME_INTC_TIME(intc) ((intc >> 8) & 0xff)
> >
> > +#define NVME_INTVC_NOCOALESCING (0x1 << 16)
> > +
> > #define NVME_TEMP_THSEL(temp) ((temp >> 20) & 0x3)
> > #define NVME_TEMP_THSEL_OVER 0x0
> > #define NVME_TEMP_THSEL_UNDER 0x1
> > @@ -902,6 +906,9 @@ enum NvmeFeatureIds {
> > NVME_SOFTWARE_PROGRESS_MARKER = 0x80
> > };
> >
> > +#define NVME_GETSETFEAT_FID_MASK 0xff
> > +#define NVME_GETSETFEAT_FID(dw10) (dw10 & NVME_GETSETFEAT_FID_MASK)
> > +
> > typedef struct NvmeRangeType {
> > uint8_t type;
> > uint8_t attributes;
> >
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters
2020-07-03 8:46 ` Klaus Jensen
@ 2020-07-03 9:21 ` Philippe Mathieu-Daudé
2020-07-03 10:10 ` Klaus Jensen
0 siblings, 1 reply; 42+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-03 9:21 UTC (permalink / raw)
To: Klaus Jensen
Cc: Kevin Wolf, qemu-block, Dmitry Fomichev, Klaus Jensen,
qemu-devel, Max Reitz, Keith Busch, Javier Gonzalez,
Maxim Levitsky
On 7/3/20 10:46 AM, Klaus Jensen wrote:
> On Jul 3 10:31, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 8:34 AM, Klaus Jensen wrote:
>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>
>>> Add support for any remaining mandatory controller operating parameters
>>> (features).
>>>
>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
>>> ---
>>> hw/block/nvme.c | 39 +++++++++++++++++++++++++++++++++------
>>> hw/block/nvme.h | 18 ++++++++++++++++++
>>> hw/block/trace-events | 2 ++
>>> include/block/nvme.h | 7 +++++++
>>> 4 files changed, 60 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>> index ba523f6768bf..affb9a967534 100644
>>> --- a/hw/block/nvme.c
>>> +++ b/hw/block/nvme.c
>>> @@ -1056,8 +1056,16 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>>> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
>>> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
>>> uint32_t result;
>>> + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
>>> + uint16_t iv;
>>>
>>> - switch (dw10) {
>>> + trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
>>> +
>>> + if (!nvme_feature_support[fid]) {
>>> + return NVME_INVALID_FIELD | NVME_DNR;
>>> + }
>>> +
>>> + switch (fid) {
>>> case NVME_TEMPERATURE_THRESHOLD:
>>> result = 0;
>>>
>>> @@ -1088,14 +1096,27 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>>> ((n->params.max_ioqpairs - 1) << 16));
>>> trace_pci_nvme_getfeat_numq(result);
>>> break;
>>> + case NVME_INTERRUPT_VECTOR_CONF:
>>> + iv = dw11 & 0xffff;
>>> + if (iv >= n->params.max_ioqpairs + 1) {
>>> + return NVME_INVALID_FIELD | NVME_DNR;
>>> + }
>>> +
>>> + result = iv;
>>> + if (iv == n->admin_cq.vector) {
>>> + result |= NVME_INTVC_NOCOALESCING;
>>> + }
>>> +
>>> + result = cpu_to_le32(result);
>>> + break;
>>> case NVME_ASYNCHRONOUS_EVENT_CONF:
>>> result = cpu_to_le32(n->features.async_config);
>>> break;
>>> case NVME_TIMESTAMP:
>>> return nvme_get_feature_timestamp(n, cmd);
>>> default:
>>> - trace_pci_nvme_err_invalid_getfeat(dw10);
>>> - return NVME_INVALID_FIELD | NVME_DNR;
>>> + result = cpu_to_le32(nvme_feature_default[fid]);
>>
>> So here we expect uninitialized fid entries to return 0, right?
>>
>
> Yes, if defaults are not 0 (like NVME_ARBITRATION), it is explicitly set.
>
>>> + break;
>>> }
>>>
>>> req->cqe.result = result;
>>> @@ -1124,8 +1145,15 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>>> {
>>> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
>>> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
>>> + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
>>>
>>> - switch (dw10) {
>>> + trace_pci_nvme_setfeat(nvme_cid(req), fid, dw11);
>>> +
>>> + if (!nvme_feature_support[fid]) {
>>> + return NVME_INVALID_FIELD | NVME_DNR;
>>> + }
>>> +
>>> + switch (fid) {
>>> case NVME_TEMPERATURE_THRESHOLD:
>>> if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
>>> break;
>>> @@ -1172,8 +1200,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>>> case NVME_TIMESTAMP:
>>> return nvme_set_feature_timestamp(n, cmd);
>>> default:
>>> - trace_pci_nvme_err_invalid_setfeat(dw10);
>>> - return NVME_INVALID_FIELD | NVME_DNR;
>>> + return NVME_FEAT_NOT_CHANGEABLE | NVME_DNR;
>>> }
>>> return NVME_SUCCESS;
>>> }
>>> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
>>> index f8940435f9ef..8ad1e3c89cee 100644
>>> --- a/hw/block/nvme.h
>>> +++ b/hw/block/nvme.h
>>> @@ -87,6 +87,24 @@ typedef struct NvmeFeatureVal {
>>> uint32_t async_config;
>>> } NvmeFeatureVal;
What do you think about adding:
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -804,7 +804,8 @@ enum NvmeFeatureIds {
NVME_WRITE_ATOMICITY = 0xa,
NVME_ASYNCHRONOUS_EVENT_CONF = 0xb,
NVME_TIMESTAMP = 0xe,
- NVME_SOFTWARE_PROGRESS_MARKER = 0x80
+ NVME_SOFTWARE_PROGRESS_MARKER = 0x80,
+ NVME_FEATURE_ID_COUNT = 0x100
};
>>>
>>> +static const uint32_t nvme_feature_default[0x100] = {
Why uint32_t and not uint16_t?
With the previously suggested enum you can now replace 0x100
by NVME_FEATURE_ID_COUNT.
>>> + [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
>>> +};
>>> +
>>> +static const bool nvme_feature_support[0x100] = {
Ditto NVME_FEATURE_ID_COUNT.
>>> + [NVME_ARBITRATION] = true,
>>> + [NVME_POWER_MANAGEMENT] = true,
>>> + [NVME_TEMPERATURE_THRESHOLD] = true,
>>> + [NVME_ERROR_RECOVERY] = true,
>>> + [NVME_VOLATILE_WRITE_CACHE] = true,
>>> + [NVME_NUMBER_OF_QUEUES] = true,
>>> + [NVME_INTERRUPT_COALESCING] = true,
>>> + [NVME_INTERRUPT_VECTOR_CONF] = true,
>>> + [NVME_WRITE_ATOMICITY] = true,
>>> + [NVME_ASYNCHRONOUS_EVENT_CONF] = true,
>>> + [NVME_TIMESTAMP] = true,
>>> +};
>>
>> Nack. No variable assignation in header please.
Rationale is if another C source file include this header,
the array will be redeclared (statically) and eventually
unused.
>> Move that to the source file.
>>
>
> Understood :)
>
>>> +
>>> typedef struct NvmeCtrl {
>>> PCIDevice parent_obj;
>>> MemoryRegion iomem;
>>> diff --git a/hw/block/trace-events b/hw/block/trace-events
>>> index 091af16ca7d7..42e62f4649f8 100644
>>> --- a/hw/block/trace-events
>>> +++ b/hw/block/trace-events
>>> @@ -46,6 +46,8 @@ pci_nvme_identify_ctrl(void) "identify controller"
>>> pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
>>> pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
>>> pci_nvme_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae, uint32_t len, uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" lsp 0x%"PRIx8" rae 0x%"PRIx8" len %"PRIu32" off %"PRIu64""
>>> +pci_nvme_getfeat(uint16_t cid, uint8_t fid, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" cdw11 0x%"PRIx32""
>>> +pci_nvme_setfeat(uint16_t cid, uint8_t fid, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" cdw11 0x%"PRIx32""
>>> pci_nvme_getfeat_vwcache(const char* result) "get feature volatile write cache, result=%s"
>>> pci_nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
>>> pci_nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
>>> diff --git a/include/block/nvme.h b/include/block/nvme.h
>>> index 0dce15af6bcf..406648d4cf94 100644
>>> --- a/include/block/nvme.h
>>> +++ b/include/block/nvme.h
>>> @@ -662,6 +662,7 @@ enum NvmeStatusCodes {
>>> NVME_FW_REQ_RESET = 0x010b,
>>> NVME_INVALID_QUEUE_DEL = 0x010c,
>>> NVME_FID_NOT_SAVEABLE = 0x010d,
>>> + NVME_FEAT_NOT_CHANGEABLE = 0x010e,
>>> NVME_FID_NOT_NSID_SPEC = 0x010f,
>>> NVME_FW_REQ_SUSYSTEM_RESET = 0x0110,
>>> NVME_CONFLICTING_ATTRS = 0x0180,
>>> @@ -866,6 +867,7 @@ enum NvmeIdCtrlLpa {
>>> #define NVME_CTRL_SGLS_ADDR_OFFSET (0x1 << 20)
>>>
>>> #define NVME_ARB_AB(arb) (arb & 0x7)
>>> +#define NVME_ARB_AB_NOLIMIT 0x7
>>> #define NVME_ARB_LPW(arb) ((arb >> 8) & 0xff)
>>> #define NVME_ARB_MPW(arb) ((arb >> 16) & 0xff)
>>> #define NVME_ARB_HPW(arb) ((arb >> 24) & 0xff)
>>> @@ -873,6 +875,8 @@ enum NvmeIdCtrlLpa {
>>> #define NVME_INTC_THR(intc) (intc & 0xff)
>>> #define NVME_INTC_TIME(intc) ((intc >> 8) & 0xff)
>>>
>>> +#define NVME_INTVC_NOCOALESCING (0x1 << 16)
>>> +
>>> #define NVME_TEMP_THSEL(temp) ((temp >> 20) & 0x3)
>>> #define NVME_TEMP_THSEL_OVER 0x0
>>> #define NVME_TEMP_THSEL_UNDER 0x1
>>> @@ -902,6 +906,9 @@ enum NvmeFeatureIds {
>>> NVME_SOFTWARE_PROGRESS_MARKER = 0x80
>>> };
>>>
>>> +#define NVME_GETSETFEAT_FID_MASK 0xff
>>> +#define NVME_GETSETFEAT_FID(dw10) (dw10 & NVME_GETSETFEAT_FID_MASK)
>>> +
>>> typedef struct NvmeRangeType {
>>> uint8_t type;
>>> uint8_t attributes;
>>>
>>
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters
2020-07-03 9:21 ` Philippe Mathieu-Daudé
@ 2020-07-03 10:10 ` Klaus Jensen
2020-07-03 11:02 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 10:10 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, qemu-block, Klaus Jensen, qemu-devel, Max Reitz,
Keith Busch, Javier Gonzalez
On Jul 3 11:21, Philippe Mathieu-Daudé wrote:
> On 7/3/20 10:46 AM, Klaus Jensen wrote:
> > On Jul 3 10:31, Philippe Mathieu-Daudé wrote:
> >> On 7/3/20 8:34 AM, Klaus Jensen wrote:
> >>> From: Klaus Jensen <k.jensen@samsung.com>
> >>>
> >>> Add support for any remaining mandatory controller operating parameters
> >>> (features).
> >>>
> >>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> >>> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> >>> ---
> >>> hw/block/nvme.c | 39 +++++++++++++++++++++++++++++++++------
> >>> hw/block/nvme.h | 18 ++++++++++++++++++
> >>> hw/block/trace-events | 2 ++
> >>> include/block/nvme.h | 7 +++++++
> >>> 4 files changed, 60 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> >>> index ba523f6768bf..affb9a967534 100644
> >>> --- a/hw/block/nvme.c
> >>> +++ b/hw/block/nvme.c
> >>> @@ -1056,8 +1056,16 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> >>> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> >>> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> >>> uint32_t result;
> >>> + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> >>> + uint16_t iv;
> >>>
> >>> - switch (dw10) {
> >>> + trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
> >>> +
> >>> + if (!nvme_feature_support[fid]) {
> >>> + return NVME_INVALID_FIELD | NVME_DNR;
> >>> + }
> >>> +
> >>> + switch (fid) {
> >>> case NVME_TEMPERATURE_THRESHOLD:
> >>> result = 0;
> >>>
> >>> @@ -1088,14 +1096,27 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> >>> ((n->params.max_ioqpairs - 1) << 16));
> >>> trace_pci_nvme_getfeat_numq(result);
> >>> break;
> >>> + case NVME_INTERRUPT_VECTOR_CONF:
> >>> + iv = dw11 & 0xffff;
> >>> + if (iv >= n->params.max_ioqpairs + 1) {
> >>> + return NVME_INVALID_FIELD | NVME_DNR;
> >>> + }
> >>> +
> >>> + result = iv;
> >>> + if (iv == n->admin_cq.vector) {
> >>> + result |= NVME_INTVC_NOCOALESCING;
> >>> + }
> >>> +
> >>> + result = cpu_to_le32(result);
> >>> + break;
> >>> case NVME_ASYNCHRONOUS_EVENT_CONF:
> >>> result = cpu_to_le32(n->features.async_config);
> >>> break;
> >>> case NVME_TIMESTAMP:
> >>> return nvme_get_feature_timestamp(n, cmd);
> >>> default:
> >>> - trace_pci_nvme_err_invalid_getfeat(dw10);
> >>> - return NVME_INVALID_FIELD | NVME_DNR;
> >>> + result = cpu_to_le32(nvme_feature_default[fid]);
> >>
> >> So here we expect uninitialized fid entries to return 0, right?
> >>
> >
> > Yes, if defaults are not 0 (like NVME_ARBITRATION), it is explicitly set.
> >
> >>> + break;
> >>> }
> >>>
> >>> req->cqe.result = result;
> >>> @@ -1124,8 +1145,15 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> >>> {
> >>> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> >>> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> >>> + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> >>>
> >>> - switch (dw10) {
> >>> + trace_pci_nvme_setfeat(nvme_cid(req), fid, dw11);
> >>> +
> >>> + if (!nvme_feature_support[fid]) {
> >>> + return NVME_INVALID_FIELD | NVME_DNR;
> >>> + }
> >>> +
> >>> + switch (fid) {
> >>> case NVME_TEMPERATURE_THRESHOLD:
> >>> if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
> >>> break;
> >>> @@ -1172,8 +1200,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> >>> case NVME_TIMESTAMP:
> >>> return nvme_set_feature_timestamp(n, cmd);
> >>> default:
> >>> - trace_pci_nvme_err_invalid_setfeat(dw10);
> >>> - return NVME_INVALID_FIELD | NVME_DNR;
> >>> + return NVME_FEAT_NOT_CHANGEABLE | NVME_DNR;
> >>> }
> >>> return NVME_SUCCESS;
> >>> }
> >>> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> >>> index f8940435f9ef..8ad1e3c89cee 100644
> >>> --- a/hw/block/nvme.h
> >>> +++ b/hw/block/nvme.h
> >>> @@ -87,6 +87,24 @@ typedef struct NvmeFeatureVal {
> >>> uint32_t async_config;
> >>> } NvmeFeatureVal;
>
> What do you think about adding:
>
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -804,7 +804,8 @@ enum NvmeFeatureIds {
> NVME_WRITE_ATOMICITY = 0xa,
> NVME_ASYNCHRONOUS_EVENT_CONF = 0xb,
> NVME_TIMESTAMP = 0xe,
> - NVME_SOFTWARE_PROGRESS_MARKER = 0x80
> + NVME_SOFTWARE_PROGRESS_MARKER = 0x80,
> + NVME_FEATURE_ID_COUNT = 0x100
> };
>
I can definitely go with that. I added it as NVME_FID_MAX.
> >>>
> >>> +static const uint32_t nvme_feature_default[0x100] = {
>
> Why uint32_t and not uint16_t?
>
The feature values are by definition 32 bits wide, so even though the
defaults here do not require it, I think uin32_t should be used.
> With the previously suggested enum you can now replace 0x100
> by NVME_FEATURE_ID_COUNT.
>
> >>> + [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> >>> +};
> >>> +
> >>> +static const bool nvme_feature_support[0x100] = {
>
> Ditto NVME_FEATURE_ID_COUNT.
>
Fixed!
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters
2020-07-03 10:10 ` Klaus Jensen
@ 2020-07-03 11:02 ` Philippe Mathieu-Daudé
2020-07-03 14:37 ` Klaus Jensen
0 siblings, 1 reply; 42+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-03 11:02 UTC (permalink / raw)
To: Klaus Jensen
Cc: Kevin Wolf, qemu-block, Klaus Jensen, qemu-devel, Max Reitz,
Keith Busch, Javier Gonzalez
On 7/3/20 12:10 PM, Klaus Jensen wrote:
> On Jul 3 11:21, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 10:46 AM, Klaus Jensen wrote:
>>> On Jul 3 10:31, Philippe Mathieu-Daudé wrote:
>>>> On 7/3/20 8:34 AM, Klaus Jensen wrote:
>>>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>>>
>>>>> Add support for any remaining mandatory controller operating parameters
>>>>> (features).
>>>>>
>>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>>>> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
>>>>> ---
>>>>> hw/block/nvme.c | 39 +++++++++++++++++++++++++++++++++------
>>>>> hw/block/nvme.h | 18 ++++++++++++++++++
>>>>> hw/block/trace-events | 2 ++
>>>>> include/block/nvme.h | 7 +++++++
>>>>> 4 files changed, 60 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>>>> index ba523f6768bf..affb9a967534 100644
>>>>> --- a/hw/block/nvme.c
>>>>> +++ b/hw/block/nvme.c
>>>>> @@ -1056,8 +1056,16 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>>>>> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
>>>>> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
>>>>> uint32_t result;
>>>>> + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
>>>>> + uint16_t iv;
>>>>>
>>>>> - switch (dw10) {
>>>>> + trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
>>>>> +
>>>>> + if (!nvme_feature_support[fid]) {
>>>>> + return NVME_INVALID_FIELD | NVME_DNR;
>>>>> + }
>>>>> +
>>>>> + switch (fid) {
>>>>> case NVME_TEMPERATURE_THRESHOLD:
>>>>> result = 0;
>>>>>
>>>>> @@ -1088,14 +1096,27 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>>>>> ((n->params.max_ioqpairs - 1) << 16));
>>>>> trace_pci_nvme_getfeat_numq(result);
>>>>> break;
>>>>> + case NVME_INTERRUPT_VECTOR_CONF:
>>>>> + iv = dw11 & 0xffff;
>>>>> + if (iv >= n->params.max_ioqpairs + 1) {
>>>>> + return NVME_INVALID_FIELD | NVME_DNR;
>>>>> + }
>>>>> +
>>>>> + result = iv;
>>>>> + if (iv == n->admin_cq.vector) {
>>>>> + result |= NVME_INTVC_NOCOALESCING;
>>>>> + }
>>>>> +
>>>>> + result = cpu_to_le32(result);
>>>>> + break;
>>>>> case NVME_ASYNCHRONOUS_EVENT_CONF:
>>>>> result = cpu_to_le32(n->features.async_config);
>>>>> break;
>>>>> case NVME_TIMESTAMP:
>>>>> return nvme_get_feature_timestamp(n, cmd);
>>>>> default:
>>>>> - trace_pci_nvme_err_invalid_getfeat(dw10);
>>>>> - return NVME_INVALID_FIELD | NVME_DNR;
>>>>> + result = cpu_to_le32(nvme_feature_default[fid]);
>>>>
>>>> So here we expect uninitialized fid entries to return 0, right?
>>>>
>>>
>>> Yes, if defaults are not 0 (like NVME_ARBITRATION), it is explicitly set.
>>>
>>>>> + break;
>>>>> }
>>>>>
>>>>> req->cqe.result = result;
>>>>> @@ -1124,8 +1145,15 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>>>>> {
>>>>> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
>>>>> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
>>>>> + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
>>>>>
>>>>> - switch (dw10) {
>>>>> + trace_pci_nvme_setfeat(nvme_cid(req), fid, dw11);
>>>>> +
>>>>> + if (!nvme_feature_support[fid]) {
>>>>> + return NVME_INVALID_FIELD | NVME_DNR;
>>>>> + }
>>>>> +
>>>>> + switch (fid) {
>>>>> case NVME_TEMPERATURE_THRESHOLD:
>>>>> if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
>>>>> break;
>>>>> @@ -1172,8 +1200,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>>>>> case NVME_TIMESTAMP:
>>>>> return nvme_set_feature_timestamp(n, cmd);
>>>>> default:
>>>>> - trace_pci_nvme_err_invalid_setfeat(dw10);
>>>>> - return NVME_INVALID_FIELD | NVME_DNR;
>>>>> + return NVME_FEAT_NOT_CHANGEABLE | NVME_DNR;
>>>>> }
>>>>> return NVME_SUCCESS;
>>>>> }
>>>>> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
>>>>> index f8940435f9ef..8ad1e3c89cee 100644
>>>>> --- a/hw/block/nvme.h
>>>>> +++ b/hw/block/nvme.h
>>>>> @@ -87,6 +87,24 @@ typedef struct NvmeFeatureVal {
>>>>> uint32_t async_config;
>>>>> } NvmeFeatureVal;
>>
>> What do you think about adding:
>>
>> --- a/include/block/nvme.h
>> +++ b/include/block/nvme.h
>> @@ -804,7 +804,8 @@ enum NvmeFeatureIds {
>> NVME_WRITE_ATOMICITY = 0xa,
>> NVME_ASYNCHRONOUS_EVENT_CONF = 0xb,
>> NVME_TIMESTAMP = 0xe,
>> - NVME_SOFTWARE_PROGRESS_MARKER = 0x80
>> + NVME_SOFTWARE_PROGRESS_MARKER = 0x80,
>> + NVME_FEATURE_ID_COUNT = 0x100
>> };
>>
>
> I can definitely go with that. I added it as NVME_FID_MAX.
Good name.
>
>>>>>
>>>>> +static const uint32_t nvme_feature_default[0x100] = {
>>
>> Why uint32_t and not uint16_t?
>>
>
> The feature values are by definition 32 bits wide, so even though the
> defaults here do not require it, I think uin32_t should be used.
Can you prepend a new patch making nvme_get_feature() return uin32_t
instead of uint16_t? That would be clearer.
>
>> With the previously suggested enum you can now replace 0x100
>> by NVME_FEATURE_ID_COUNT.
>>
>>>>> + [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
>>>>> +};
>>>>> +
>>>>> +static const bool nvme_feature_support[0x100] = {
>>
>> Ditto NVME_FEATURE_ID_COUNT.
>>
>
> Fixed!
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters
2020-07-03 11:02 ` Philippe Mathieu-Daudé
@ 2020-07-03 14:37 ` Klaus Jensen
2020-07-04 21:33 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 14:37 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, qemu-block, Klaus Jensen, qemu-devel, Max Reitz,
Keith Busch, Javier Gonzalez
On Jul 3 13:02, Philippe Mathieu-Daudé wrote:
> On 7/3/20 12:10 PM, Klaus Jensen wrote:
> > On Jul 3 11:21, Philippe Mathieu-Daudé wrote:
> >> On 7/3/20 10:46 AM, Klaus Jensen wrote:
> >>> On Jul 3 10:31, Philippe Mathieu-Daudé wrote:
> >>>> On 7/3/20 8:34 AM, Klaus Jensen wrote:
> >>>>> From: Klaus Jensen <k.jensen@samsung.com>
> >>>>>
> >>>>> Add support for any remaining mandatory controller operating parameters
> >>>>> (features).
> >>>>>
> >>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> >>>>> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> >>>>> ---
> >>>>> hw/block/nvme.c | 39 +++++++++++++++++++++++++++++++++------
> >>>>> hw/block/nvme.h | 18 ++++++++++++++++++
> >>>>> hw/block/trace-events | 2 ++
> >>>>> include/block/nvme.h | 7 +++++++
> >>>>> 4 files changed, 60 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> >>>>> index ba523f6768bf..affb9a967534 100644
> >>>>> --- a/hw/block/nvme.c
> >>>>> +++ b/hw/block/nvme.c
> >>>>> @@ -1056,8 +1056,16 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> >>>>> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> >>>>> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> >>>>> uint32_t result;
> >>>>> + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> >>>>> + uint16_t iv;
> >>>>>
> >>>>> - switch (dw10) {
> >>>>> + trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
> >>>>> +
> >>>>> + if (!nvme_feature_support[fid]) {
> >>>>> + return NVME_INVALID_FIELD | NVME_DNR;
> >>>>> + }
> >>>>> +
> >>>>> + switch (fid) {
> >>>>> case NVME_TEMPERATURE_THRESHOLD:
> >>>>> result = 0;
> >>>>>
> >>>>> @@ -1088,14 +1096,27 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> >>>>> ((n->params.max_ioqpairs - 1) << 16));
> >>>>> trace_pci_nvme_getfeat_numq(result);
> >>>>> break;
> >>>>> + case NVME_INTERRUPT_VECTOR_CONF:
> >>>>> + iv = dw11 & 0xffff;
> >>>>> + if (iv >= n->params.max_ioqpairs + 1) {
> >>>>> + return NVME_INVALID_FIELD | NVME_DNR;
> >>>>> + }
> >>>>> +
> >>>>> + result = iv;
> >>>>> + if (iv == n->admin_cq.vector) {
> >>>>> + result |= NVME_INTVC_NOCOALESCING;
> >>>>> + }
> >>>>> +
> >>>>> + result = cpu_to_le32(result);
> >>>>> + break;
> >>>>> case NVME_ASYNCHRONOUS_EVENT_CONF:
> >>>>> result = cpu_to_le32(n->features.async_config);
> >>>>> break;
> >>>>> case NVME_TIMESTAMP:
> >>>>> return nvme_get_feature_timestamp(n, cmd);
> >>>>> default:
> >>>>> - trace_pci_nvme_err_invalid_getfeat(dw10);
> >>>>> - return NVME_INVALID_FIELD | NVME_DNR;
> >>>>> + result = cpu_to_le32(nvme_feature_default[fid]);
> >>>>
> >>>> So here we expect uninitialized fid entries to return 0, right?
> >>>>
> >>>
> >>> Yes, if defaults are not 0 (like NVME_ARBITRATION), it is explicitly set.
> >>>
> >>>>> + break;
> >>>>> }
> >>>>>
> >>>>> req->cqe.result = result;
> >>>>> @@ -1124,8 +1145,15 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> >>>>> {
> >>>>> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> >>>>> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> >>>>> + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> >>>>>
> >>>>> - switch (dw10) {
> >>>>> + trace_pci_nvme_setfeat(nvme_cid(req), fid, dw11);
> >>>>> +
> >>>>> + if (!nvme_feature_support[fid]) {
> >>>>> + return NVME_INVALID_FIELD | NVME_DNR;
> >>>>> + }
> >>>>> +
> >>>>> + switch (fid) {
> >>>>> case NVME_TEMPERATURE_THRESHOLD:
> >>>>> if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
> >>>>> break;
> >>>>> @@ -1172,8 +1200,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> >>>>> case NVME_TIMESTAMP:
> >>>>> return nvme_set_feature_timestamp(n, cmd);
> >>>>> default:
> >>>>> - trace_pci_nvme_err_invalid_setfeat(dw10);
> >>>>> - return NVME_INVALID_FIELD | NVME_DNR;
> >>>>> + return NVME_FEAT_NOT_CHANGEABLE | NVME_DNR;
> >>>>> }
> >>>>> return NVME_SUCCESS;
> >>>>> }
> >>>>> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> >>>>> index f8940435f9ef..8ad1e3c89cee 100644
> >>>>> --- a/hw/block/nvme.h
> >>>>> +++ b/hw/block/nvme.h
> >>>>> @@ -87,6 +87,24 @@ typedef struct NvmeFeatureVal {
> >>>>> uint32_t async_config;
> >>>>> } NvmeFeatureVal;
> >>
> >> What do you think about adding:
> >>
> >> --- a/include/block/nvme.h
> >> +++ b/include/block/nvme.h
> >> @@ -804,7 +804,8 @@ enum NvmeFeatureIds {
> >> NVME_WRITE_ATOMICITY = 0xa,
> >> NVME_ASYNCHRONOUS_EVENT_CONF = 0xb,
> >> NVME_TIMESTAMP = 0xe,
> >> - NVME_SOFTWARE_PROGRESS_MARKER = 0x80
> >> + NVME_SOFTWARE_PROGRESS_MARKER = 0x80,
> >> + NVME_FEATURE_ID_COUNT = 0x100
> >> };
> >>
> >
> > I can definitely go with that. I added it as NVME_FID_MAX.
>
> Good name.
>
> >
> >>>>>
> >>>>> +static const uint32_t nvme_feature_default[0x100] = {
> >>
> >> Why uint32_t and not uint16_t?
> >>
> >
> > The feature values are by definition 32 bits wide, so even though the
> > defaults here do not require it, I think uin32_t should be used.
>
> Can you prepend a new patch making nvme_get_feature() return uin32_t
> instead of uint16_t? That would be clearer.
>
Ah. Now I see where the confusion comes from ;)
nvme_get_feature() does not return the value of the feature. It returns
a uint16_t with a value from the NvmeStatusCodes enum (which really
should be a typedef used all over the place.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters
2020-07-03 14:37 ` Klaus Jensen
@ 2020-07-04 21:33 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 42+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-04 21:33 UTC (permalink / raw)
To: Klaus Jensen
Cc: Kevin Wolf, qemu-block, Klaus Jensen, qemu-devel, Max Reitz,
Keith Busch, Javier Gonzalez
On 7/3/20 4:37 PM, Klaus Jensen wrote:
> On Jul 3 13:02, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 12:10 PM, Klaus Jensen wrote:
>>> On Jul 3 11:21, Philippe Mathieu-Daudé wrote:
>>>> On 7/3/20 10:46 AM, Klaus Jensen wrote:
>>>>> On Jul 3 10:31, Philippe Mathieu-Daudé wrote:
>>>>>> On 7/3/20 8:34 AM, Klaus Jensen wrote:
>>>>>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>>>>>
>>>>>>> Add support for any remaining mandatory controller operating parameters
>>>>>>> (features).
>>>>>>>
>>>>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>>>>>> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
>>>>>>> ---
>>>>>>> hw/block/nvme.c | 39 +++++++++++++++++++++++++++++++++------
>>>>>>> hw/block/nvme.h | 18 ++++++++++++++++++
>>>>>>> hw/block/trace-events | 2 ++
>>>>>>> include/block/nvme.h | 7 +++++++
>>>>>>> 4 files changed, 60 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>>>>>> index ba523f6768bf..affb9a967534 100644
>>>>>>> --- a/hw/block/nvme.c
>>>>>>> +++ b/hw/block/nvme.c
>>>>>>> @@ -1056,8 +1056,16 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>>>>>>> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
>>>>>>> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
>>>>>>> uint32_t result;
>>>>>>> + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
>>>>>>> + uint16_t iv;
>>>>>>>
>>>>>>> - switch (dw10) {
>>>>>>> + trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
>>>>>>> +
>>>>>>> + if (!nvme_feature_support[fid]) {
>>>>>>> + return NVME_INVALID_FIELD | NVME_DNR;
>>>>>>> + }
>>>>>>> +
>>>>>>> + switch (fid) {
>>>>>>> case NVME_TEMPERATURE_THRESHOLD:
>>>>>>> result = 0;
>>>>>>>
>>>>>>> @@ -1088,14 +1096,27 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>>>>>>> ((n->params.max_ioqpairs - 1) << 16));
>>>>>>> trace_pci_nvme_getfeat_numq(result);
>>>>>>> break;
>>>>>>> + case NVME_INTERRUPT_VECTOR_CONF:
>>>>>>> + iv = dw11 & 0xffff;
>>>>>>> + if (iv >= n->params.max_ioqpairs + 1) {
>>>>>>> + return NVME_INVALID_FIELD | NVME_DNR;
>>>>>>> + }
>>>>>>> +
>>>>>>> + result = iv;
>>>>>>> + if (iv == n->admin_cq.vector) {
>>>>>>> + result |= NVME_INTVC_NOCOALESCING;
>>>>>>> + }
>>>>>>> +
>>>>>>> + result = cpu_to_le32(result);
>>>>>>> + break;
>>>>>>> case NVME_ASYNCHRONOUS_EVENT_CONF:
>>>>>>> result = cpu_to_le32(n->features.async_config);
>>>>>>> break;
>>>>>>> case NVME_TIMESTAMP:
>>>>>>> return nvme_get_feature_timestamp(n, cmd);
>>>>>>> default:
>>>>>>> - trace_pci_nvme_err_invalid_getfeat(dw10);
>>>>>>> - return NVME_INVALID_FIELD | NVME_DNR;
>>>>>>> + result = cpu_to_le32(nvme_feature_default[fid]);
>>>>>>
>>>>>> So here we expect uninitialized fid entries to return 0, right?
>>>>>>
>>>>>
>>>>> Yes, if defaults are not 0 (like NVME_ARBITRATION), it is explicitly set.
>>>>>
>>>>>>> + break;
>>>>>>> }
>>>>>>>
>>>>>>> req->cqe.result = result;
>>>>>>> @@ -1124,8 +1145,15 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>>>>>>> {
>>>>>>> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
>>>>>>> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
>>>>>>> + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
>>>>>>>
>>>>>>> - switch (dw10) {
>>>>>>> + trace_pci_nvme_setfeat(nvme_cid(req), fid, dw11);
>>>>>>> +
>>>>>>> + if (!nvme_feature_support[fid]) {
>>>>>>> + return NVME_INVALID_FIELD | NVME_DNR;
>>>>>>> + }
>>>>>>> +
>>>>>>> + switch (fid) {
>>>>>>> case NVME_TEMPERATURE_THRESHOLD:
>>>>>>> if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
>>>>>>> break;
>>>>>>> @@ -1172,8 +1200,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>>>>>>> case NVME_TIMESTAMP:
>>>>>>> return nvme_set_feature_timestamp(n, cmd);
>>>>>>> default:
>>>>>>> - trace_pci_nvme_err_invalid_setfeat(dw10);
>>>>>>> - return NVME_INVALID_FIELD | NVME_DNR;
>>>>>>> + return NVME_FEAT_NOT_CHANGEABLE | NVME_DNR;
>>>>>>> }
>>>>>>> return NVME_SUCCESS;
>>>>>>> }
>>>>>>> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
>>>>>>> index f8940435f9ef..8ad1e3c89cee 100644
>>>>>>> --- a/hw/block/nvme.h
>>>>>>> +++ b/hw/block/nvme.h
>>>>>>> @@ -87,6 +87,24 @@ typedef struct NvmeFeatureVal {
>>>>>>> uint32_t async_config;
>>>>>>> } NvmeFeatureVal;
>>>>
>>>> What do you think about adding:
>>>>
>>>> --- a/include/block/nvme.h
>>>> +++ b/include/block/nvme.h
>>>> @@ -804,7 +804,8 @@ enum NvmeFeatureIds {
>>>> NVME_WRITE_ATOMICITY = 0xa,
>>>> NVME_ASYNCHRONOUS_EVENT_CONF = 0xb,
>>>> NVME_TIMESTAMP = 0xe,
>>>> - NVME_SOFTWARE_PROGRESS_MARKER = 0x80
>>>> + NVME_SOFTWARE_PROGRESS_MARKER = 0x80,
>>>> + NVME_FEATURE_ID_COUNT = 0x100
>>>> };
>>>>
>>>
>>> I can definitely go with that. I added it as NVME_FID_MAX.
>>
>> Good name.
>>
>>>
>>>>>>>
>>>>>>> +static const uint32_t nvme_feature_default[0x100] = {
>>>>
>>>> Why uint32_t and not uint16_t?
>>>>
>>>
>>> The feature values are by definition 32 bits wide, so even though the
>>> defaults here do not require it, I think uin32_t should be used.
>>
>> Can you prepend a new patch making nvme_get_feature() return uin32_t
>> instead of uint16_t? That would be clearer.
>>
>
> Ah. Now I see where the confusion comes from ;)
>
> nvme_get_feature() does not return the value of the feature. It returns
> a uint16_t with a value from the NvmeStatusCodes enum (which really
> should be a typedef used all over the place.
Ah! You are right, I misread it indeed :)
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 12/18] hw/block/nvme: support the get/set features select and save fields
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
` (10 preceding siblings ...)
2020-07-03 6:34 ` [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 6:34 ` [PATCH v2 13/18] hw/block/nvme: make sure ncqr and nsqr is valid Klaus Jensen
` (5 subsequent siblings)
17 siblings, 0 replies; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
Since the device does not have any persistent state storage, no
features are "saveable" and setting the Save (SV) field in any Set
Features command will result in a Feature Identifier Not Saveable status
code.
Similarly, if the Select (SEL) field is set to request saved values, the
devices will (as it should) return the default values instead.
Since this also introduces "Supported Capabilities", the nsid field is
now also checked for validity wrt. the feature being get/set'ed.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/block/nvme.c | 95 +++++++++++++++++++++++++++++++++++++------
hw/block/nvme.h | 8 ++++
hw/block/trace-events | 4 +-
include/block/nvme.h | 27 +++++++++++-
4 files changed, 119 insertions(+), 15 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index affb9a967534..9cff0f45dd7e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1055,16 +1055,43 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
{
uint32_t dw10 = le32_to_cpu(cmd->cdw10);
uint32_t dw11 = le32_to_cpu(cmd->cdw11);
+ uint32_t nsid = le32_to_cpu(cmd->nsid);
uint32_t result;
uint8_t fid = NVME_GETSETFEAT_FID(dw10);
+ NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
uint16_t iv;
- trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
+ trace_pci_nvme_getfeat(nvme_cid(req), fid, sel, dw11);
if (!nvme_feature_support[fid]) {
return NVME_INVALID_FIELD | NVME_DNR;
}
+ if (nvme_feature_cap[fid] & NVME_FEAT_CAP_NS) {
+ if (!nsid || nsid > n->num_namespaces) {
+ /*
+ * The Reservation Notification Mask and Reservation Persistence
+ * features require a status code of Invalid Field in Command when
+ * NSID is 0xFFFFFFFF. Since the device does not support those
+ * features we can always return Invalid Namespace or Format as we
+ * should do for all other features.
+ */
+ return NVME_INVALID_NSID | NVME_DNR;
+ }
+ }
+
+ switch (sel) {
+ case NVME_GETFEAT_SELECT_CURRENT:
+ break;
+ case NVME_GETFEAT_SELECT_SAVED:
+ /* no features are saveable by the controller; fallthrough */
+ case NVME_GETFEAT_SELECT_DEFAULT:
+ goto defaults;
+ case NVME_GETFEAT_SELECT_CAP:
+ result = cpu_to_le32(nvme_feature_cap[fid]);
+ goto out;
+ }
+
switch (fid) {
case NVME_TEMPERATURE_THRESHOLD:
result = 0;
@@ -1074,22 +1101,45 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
* return 0 for all other sensors.
*/
if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
- break;
+ goto out;
}
switch (NVME_TEMP_THSEL(dw11)) {
case NVME_TEMP_THSEL_OVER:
result = cpu_to_le16(n->features.temp_thresh_hi);
- break;
+ goto out;
case NVME_TEMP_THSEL_UNDER:
result = cpu_to_le16(n->features.temp_thresh_low);
- break;
+ goto out;
}
- break;
+ return NVME_INVALID_FIELD | NVME_DNR;
case NVME_VOLATILE_WRITE_CACHE:
result = cpu_to_le32(blk_enable_write_cache(n->conf.blk));
trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
+ goto out;
+ case NVME_ASYNCHRONOUS_EVENT_CONF:
+ result = cpu_to_le32(n->features.async_config);
+ goto out;
+ case NVME_TIMESTAMP:
+ return nvme_get_feature_timestamp(n, cmd);
+ default:
+ break;
+ }
+
+defaults:
+ switch (fid) {
+ case NVME_TEMPERATURE_THRESHOLD:
+ result = 0;
+
+ if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
+ break;
+ }
+
+ if (NVME_TEMP_THSEL(dw11) == NVME_TEMP_THSEL_OVER) {
+ result = cpu_to_le16(NVME_TEMPERATURE_WARNING);
+ }
+
break;
case NVME_NUMBER_OF_QUEUES:
result = cpu_to_le32((n->params.max_ioqpairs - 1) |
@@ -1109,16 +1159,12 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
result = cpu_to_le32(result);
break;
- case NVME_ASYNCHRONOUS_EVENT_CONF:
- result = cpu_to_le32(n->features.async_config);
- break;
- case NVME_TIMESTAMP:
- return nvme_get_feature_timestamp(n, cmd);
default:
result = cpu_to_le32(nvme_feature_default[fid]);
break;
}
+out:
req->cqe.result = result;
return NVME_SUCCESS;
}
@@ -1145,14 +1191,37 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
{
uint32_t dw10 = le32_to_cpu(cmd->cdw10);
uint32_t dw11 = le32_to_cpu(cmd->cdw11);
+ uint32_t nsid = le32_to_cpu(cmd->nsid);
uint8_t fid = NVME_GETSETFEAT_FID(dw10);
+ uint8_t save = NVME_SETFEAT_SAVE(dw10);
- trace_pci_nvme_setfeat(nvme_cid(req), fid, dw11);
+ trace_pci_nvme_setfeat(nvme_cid(req), fid, save, dw11);
+
+ if (save) {
+ return NVME_FID_NOT_SAVEABLE | NVME_DNR;
+ }
if (!nvme_feature_support[fid]) {
return NVME_INVALID_FIELD | NVME_DNR;
}
+ if (nvme_feature_cap[fid] & NVME_FEAT_CAP_NS) {
+ if (!nsid || (nsid != NVME_NSID_BROADCAST &&
+ nsid > n->num_namespaces)) {
+ return NVME_INVALID_NSID | NVME_DNR;
+ }
+ } else if (nsid && nsid != NVME_NSID_BROADCAST) {
+ if (nsid > n->num_namespaces) {
+ return NVME_INVALID_NSID | NVME_DNR;
+ }
+
+ return NVME_FEAT_NOT_NS_SPEC | NVME_DNR;
+ }
+
+ if (!(nvme_feature_cap[fid] & NVME_FEAT_CAP_CHANGE)) {
+ return NVME_FEAT_NOT_CHANGEABLE | NVME_DNR;
+ }
+
switch (fid) {
case NVME_TEMPERATURE_THRESHOLD:
if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
@@ -1997,7 +2066,9 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
id->sqes = (0x6 << 4) | 0x6;
id->cqes = (0x4 << 4) | 0x4;
id->nn = cpu_to_le32(n->num_namespaces);
- id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS | NVME_ONCS_TIMESTAMP);
+ id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS | NVME_ONCS_TIMESTAMP |
+ NVME_ONCS_FEATURES);
+
id->psd[0].mp = cpu_to_le16(0x9c4);
id->psd[0].enlat = cpu_to_le32(0x10);
id->psd[0].exlat = cpu_to_le32(0x4);
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 8ad1e3c89cee..450e4294193a 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -87,6 +87,14 @@ typedef struct NvmeFeatureVal {
uint32_t async_config;
} NvmeFeatureVal;
+static const uint32_t nvme_feature_cap[0x100] = {
+ [NVME_TEMPERATURE_THRESHOLD] = NVME_FEAT_CAP_CHANGE,
+ [NVME_VOLATILE_WRITE_CACHE] = NVME_FEAT_CAP_CHANGE,
+ [NVME_NUMBER_OF_QUEUES] = NVME_FEAT_CAP_CHANGE,
+ [NVME_ASYNCHRONOUS_EVENT_CONF] = NVME_FEAT_CAP_CHANGE,
+ [NVME_TIMESTAMP] = NVME_FEAT_CAP_CHANGE,
+};
+
static const uint32_t nvme_feature_default[0x100] = {
[NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
};
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 42e62f4649f8..4a4ef34071df 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -46,8 +46,8 @@ pci_nvme_identify_ctrl(void) "identify controller"
pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
pci_nvme_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae, uint32_t len, uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" lsp 0x%"PRIx8" rae 0x%"PRIx8" len %"PRIu32" off %"PRIu64""
-pci_nvme_getfeat(uint16_t cid, uint8_t fid, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" cdw11 0x%"PRIx32""
-pci_nvme_setfeat(uint16_t cid, uint8_t fid, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" cdw11 0x%"PRIx32""
+pci_nvme_getfeat(uint16_t cid, uint8_t fid, uint8_t sel, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" sel 0x%"PRIx8" cdw11 0x%"PRIx32""
+pci_nvme_setfeat(uint16_t cid, uint8_t fid, uint8_t save, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" save 0x%"PRIx8" cdw11 0x%"PRIx32""
pci_nvme_getfeat_vwcache(const char* result) "get feature volatile write cache, result=%s"
pci_nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
pci_nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq) "requested cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 406648d4cf94..36f7d76b3926 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -663,7 +663,7 @@ enum NvmeStatusCodes {
NVME_INVALID_QUEUE_DEL = 0x010c,
NVME_FID_NOT_SAVEABLE = 0x010d,
NVME_FEAT_NOT_CHANGEABLE = 0x010e,
- NVME_FID_NOT_NSID_SPEC = 0x010f,
+ NVME_FEAT_NOT_NS_SPEC = 0x010f,
NVME_FW_REQ_SUSYSTEM_RESET = 0x0110,
NVME_CONFLICTING_ATTRS = 0x0180,
NVME_INVALID_PROT_INFO = 0x0181,
@@ -906,9 +906,32 @@ enum NvmeFeatureIds {
NVME_SOFTWARE_PROGRESS_MARKER = 0x80
};
+typedef enum NvmeFeatureCap {
+ NVME_FEAT_CAP_SAVE = 1 << 0,
+ NVME_FEAT_CAP_NS = 1 << 1,
+ NVME_FEAT_CAP_CHANGE = 1 << 2,
+} NvmeFeatureCap;
+
+typedef enum NvmeGetFeatureSelect {
+ NVME_GETFEAT_SELECT_CURRENT = 0x0,
+ NVME_GETFEAT_SELECT_DEFAULT = 0x1,
+ NVME_GETFEAT_SELECT_SAVED = 0x2,
+ NVME_GETFEAT_SELECT_CAP = 0x3,
+} NvmeGetFeatureSelect;
+
#define NVME_GETSETFEAT_FID_MASK 0xff
#define NVME_GETSETFEAT_FID(dw10) (dw10 & NVME_GETSETFEAT_FID_MASK)
+#define NVME_GETFEAT_SELECT_SHIFT 8
+#define NVME_GETFEAT_SELECT_MASK 0x7
+#define NVME_GETFEAT_SELECT(dw10) \
+ ((dw10 >> NVME_GETFEAT_SELECT_SHIFT) & NVME_GETFEAT_SELECT_MASK)
+
+#define NVME_SETFEAT_SAVE_SHIFT 31
+#define NVME_SETFEAT_SAVE_MASK 0x1
+#define NVME_SETFEAT_SAVE(dw10) \
+ ((dw10 >> NVME_SETFEAT_SAVE_SHIFT) & NVME_SETFEAT_SAVE_MASK)
+
typedef struct NvmeRangeType {
uint8_t type;
uint8_t attributes;
@@ -925,6 +948,8 @@ typedef struct NvmeLBAF {
uint8_t rp;
} NvmeLBAF;
+#define NVME_NSID_BROADCAST 0xffffffff
+
typedef struct NvmeIdNs {
uint64_t nsze;
uint64_t ncap;
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v2 13/18] hw/block/nvme: make sure ncqr and nsqr is valid
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
` (11 preceding siblings ...)
2020-07-03 6:34 ` [PATCH v2 12/18] hw/block/nvme: support the get/set features select and save fields Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 6:34 ` [PATCH v2 14/18] hw/block/nvme: support identify namespace descriptor list Klaus Jensen
` (4 subsequent siblings)
17 siblings, 0 replies; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
0xffff is not an allowed value for NCQR and NSQR in Set Features on
Number of Queues.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Acked-by: Keith Busch <kbusch@kernel.org>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
hw/block/nvme.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9cff0f45dd7e..8230e0e3826b 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1256,6 +1256,14 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
blk_set_enable_write_cache(n->conf.blk, dw11 & 1);
break;
case NVME_NUMBER_OF_QUEUES:
+ /*
+ * NVMe v1.3, Section 5.21.1.7: 0xffff is not an allowed value for NCQR
+ * and NSQR.
+ */
+ if ((dw11 & 0xffff) == 0xffff || ((dw11 >> 16) & 0xffff) == 0xffff) {
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+
trace_pci_nvme_setfeat_numq((dw11 & 0xFFFF) + 1,
((dw11 >> 16) & 0xFFFF) + 1,
n->params.max_ioqpairs,
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v2 14/18] hw/block/nvme: support identify namespace descriptor list
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
` (12 preceding siblings ...)
2020-07-03 6:34 ` [PATCH v2 13/18] hw/block/nvme: make sure ncqr and nsqr is valid Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 8:27 ` Philippe Mathieu-Daudé
2020-07-03 6:34 ` [PATCH v2 15/18] hw/block/nvme: reject invalid nsid values in active namespace id list Klaus Jensen
` (3 subsequent siblings)
17 siblings, 1 reply; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
Since we are not providing the NGUID or EUI64 fields, we must support
the Namespace UUID. We do not have any way of storing a persistent
unique identifier, so conjure up a UUID that is just the namespace id.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
hw/block/nvme.c | 41 +++++++++++++++++++++++++++++++++++++++++
hw/block/trace-events | 1 +
2 files changed, 42 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 8230e0e3826b..65c2fa3ac1f4 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -971,6 +971,45 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
return ret;
}
+static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeIdentify *c)
+{
+ uint32_t nsid = le32_to_cpu(c->nsid);
+ uint64_t prp1 = le64_to_cpu(c->prp1);
+ uint64_t prp2 = le64_to_cpu(c->prp2);
+
+ uint8_t list[NVME_IDENTIFY_DATA_SIZE];
+
+ struct data {
+ struct {
+ NvmeIdNsDescr hdr;
+ uint8_t v[16];
+ } uuid;
+ };
+
+ struct data *ns_descrs = (struct data *)list;
+
+ trace_pci_nvme_identify_ns_descr_list(nsid);
+
+ if (unlikely(nsid == 0 || nsid > n->num_namespaces)) {
+ trace_pci_nvme_err_invalid_ns(nsid, n->num_namespaces);
+ return NVME_INVALID_NSID | NVME_DNR;
+ }
+
+ memset(list, 0x0, sizeof(list));
+
+ /*
+ * Because the NGUID and EUI64 fields are 0 in the Identify Namespace data
+ * structure, a Namespace UUID (nidt = 0x3) must be reported in the
+ * Namespace Identification Descriptor. Add a very basic Namespace UUID
+ * here.
+ */
+ ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID;
+ ns_descrs->uuid.hdr.nidl = NVME_NIDT_UUID_LEN;
+ stl_be_p(&ns_descrs->uuid.v, nsid);
+
+ return nvme_dma_read_prp(n, list, NVME_IDENTIFY_DATA_SIZE, prp1, prp2);
+}
+
static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
{
NvmeIdentify *c = (NvmeIdentify *)cmd;
@@ -982,6 +1021,8 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
return nvme_identify_ctrl(n, c);
case NVME_ID_CNS_NS_ACTIVE_LIST:
return nvme_identify_nslist(n, c);
+ case NVME_ID_CNS_NS_DESCR_LIST:
+ return nvme_identify_ns_descr_list(n, c);
default:
trace_pci_nvme_err_invalid_identify_cns(le32_to_cpu(c->cns));
return NVME_INVALID_FIELD | NVME_DNR;
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 4a4ef34071df..7b7303cab1dd 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -45,6 +45,7 @@ pci_nvme_del_cq(uint16_t cqid) "deleted completion queue, cqid=%"PRIu16""
pci_nvme_identify_ctrl(void) "identify controller"
pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
+pci_nvme_identify_ns_descr_list(uint32_t ns) "nsid %"PRIu32""
pci_nvme_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae, uint32_t len, uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" lsp 0x%"PRIx8" rae 0x%"PRIx8" len %"PRIu32" off %"PRIu64""
pci_nvme_getfeat(uint16_t cid, uint8_t fid, uint8_t sel, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" sel 0x%"PRIx8" cdw11 0x%"PRIx32""
pci_nvme_setfeat(uint16_t cid, uint8_t fid, uint8_t save, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" save 0x%"PRIx8" cdw11 0x%"PRIx32""
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v2 14/18] hw/block/nvme: support identify namespace descriptor list
2020-07-03 6:34 ` [PATCH v2 14/18] hw/block/nvme: support identify namespace descriptor list Klaus Jensen
@ 2020-07-03 8:27 ` Philippe Mathieu-Daudé
2020-07-03 10:00 ` Klaus Jensen
0 siblings, 1 reply; 42+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-03 8:27 UTC (permalink / raw)
To: Klaus Jensen, qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Keith Busch, Javier Gonzalez, Maxim Levitsky
On 7/3/20 8:34 AM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Since we are not providing the NGUID or EUI64 fields, we must support
> the Namespace UUID. We do not have any way of storing a persistent
> unique identifier, so conjure up a UUID that is just the namespace id.
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
> hw/block/nvme.c | 41 +++++++++++++++++++++++++++++++++++++++++
> hw/block/trace-events | 1 +
> 2 files changed, 42 insertions(+)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 8230e0e3826b..65c2fa3ac1f4 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -971,6 +971,45 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
> return ret;
> }
>
> +static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeIdentify *c)
> +{
> + uint32_t nsid = le32_to_cpu(c->nsid);
> + uint64_t prp1 = le64_to_cpu(c->prp1);
> + uint64_t prp2 = le64_to_cpu(c->prp2);
> +
> + uint8_t list[NVME_IDENTIFY_DATA_SIZE];
> +
> + struct data {
> + struct {
> + NvmeIdNsDescr hdr;
> + uint8_t v[16];
You might consider to use QemuUUID from "qemu/uuid.h". The benefits
are you can use qemu_uuid_parse() qemu_uuid_unparse*() for tracing,
and DEFINE_PROP_UUID() in case you want to set a particular UUID
from command line, it case it is important to the guest.
> + } uuid;
> + };
> +
> + struct data *ns_descrs = (struct data *)list;
> +
> + trace_pci_nvme_identify_ns_descr_list(nsid);
> +
> + if (unlikely(nsid == 0 || nsid > n->num_namespaces)) {
> + trace_pci_nvme_err_invalid_ns(nsid, n->num_namespaces);
> + return NVME_INVALID_NSID | NVME_DNR;
> + }
> +
> + memset(list, 0x0, sizeof(list));
> +
> + /*
> + * Because the NGUID and EUI64 fields are 0 in the Identify Namespace data
> + * structure, a Namespace UUID (nidt = 0x3) must be reported in the
> + * Namespace Identification Descriptor. Add a very basic Namespace UUID
> + * here.
> + */
> + ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID;
> + ns_descrs->uuid.hdr.nidl = NVME_NIDT_UUID_LEN;
> + stl_be_p(&ns_descrs->uuid.v, nsid);
> +
> + return nvme_dma_read_prp(n, list, NVME_IDENTIFY_DATA_SIZE, prp1, prp2);
> +}
> +
> static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
> {
> NvmeIdentify *c = (NvmeIdentify *)cmd;
> @@ -982,6 +1021,8 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
> return nvme_identify_ctrl(n, c);
> case NVME_ID_CNS_NS_ACTIVE_LIST:
> return nvme_identify_nslist(n, c);
> + case NVME_ID_CNS_NS_DESCR_LIST:
> + return nvme_identify_ns_descr_list(n, c);
> default:
> trace_pci_nvme_err_invalid_identify_cns(le32_to_cpu(c->cns));
> return NVME_INVALID_FIELD | NVME_DNR;
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 4a4ef34071df..7b7303cab1dd 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -45,6 +45,7 @@ pci_nvme_del_cq(uint16_t cqid) "deleted completion queue, cqid=%"PRIu16""
> pci_nvme_identify_ctrl(void) "identify controller"
> pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
> pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
> +pci_nvme_identify_ns_descr_list(uint32_t ns) "nsid %"PRIu32""
> pci_nvme_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae, uint32_t len, uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" lsp 0x%"PRIx8" rae 0x%"PRIx8" len %"PRIu32" off %"PRIu64""
> pci_nvme_getfeat(uint16_t cid, uint8_t fid, uint8_t sel, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" sel 0x%"PRIx8" cdw11 0x%"PRIx32""
> pci_nvme_setfeat(uint16_t cid, uint8_t fid, uint8_t save, uint32_t cdw11) "cid %"PRIu16" fid 0x%"PRIx8" save 0x%"PRIx8" cdw11 0x%"PRIx32""
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 14/18] hw/block/nvme: support identify namespace descriptor list
2020-07-03 8:27 ` Philippe Mathieu-Daudé
@ 2020-07-03 10:00 ` Klaus Jensen
2020-07-03 11:00 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 10:00 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, qemu-block, Dmitry Fomichev, Klaus Jensen,
qemu-devel, Max Reitz, Keith Busch, Javier Gonzalez,
Maxim Levitsky
On Jul 3 10:27, Philippe Mathieu-Daudé wrote:
> On 7/3/20 8:34 AM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > Since we are not providing the NGUID or EUI64 fields, we must support
> > the Namespace UUID. We do not have any way of storing a persistent
> > unique identifier, so conjure up a UUID that is just the namespace id.
> >
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > ---
> > hw/block/nvme.c | 41 +++++++++++++++++++++++++++++++++++++++++
> > hw/block/trace-events | 1 +
> > 2 files changed, 42 insertions(+)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 8230e0e3826b..65c2fa3ac1f4 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -971,6 +971,45 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
> > return ret;
> > }
> >
> > +static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeIdentify *c)
> > +{
> > + uint32_t nsid = le32_to_cpu(c->nsid);
> > + uint64_t prp1 = le64_to_cpu(c->prp1);
> > + uint64_t prp2 = le64_to_cpu(c->prp2);
> > +
> > + uint8_t list[NVME_IDENTIFY_DATA_SIZE];
> > +
> > + struct data {
> > + struct {
> > + NvmeIdNsDescr hdr;
> > + uint8_t v[16];
>
> You might consider to use QemuUUID from "qemu/uuid.h". The benefits
> are you can use qemu_uuid_parse() qemu_uuid_unparse*() for tracing,
> and DEFINE_PROP_UUID() in case you want to set a particular UUID
> from command line, it case it is important to the guest.
>
Yes, definitely. Niklas also does this in his patch for namespace types
support. And I think that it's very important that it can be made
persistent, which would require a device property.
Thus, if it is OK with the rest of you, I would like to defer this to
when we merge the multiple namespaces patch and add "uuid" as a nvme-ns
device property there. Then, we do not have to add the uuid property on
the nvme device now and then have to keep it around when the namespace
related properties moves to the nvme-ns device.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 14/18] hw/block/nvme: support identify namespace descriptor list
2020-07-03 10:00 ` Klaus Jensen
@ 2020-07-03 11:00 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 42+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-03 11:00 UTC (permalink / raw)
To: Klaus Jensen
Cc: Kevin Wolf, qemu-block, Dmitry Fomichev, Klaus Jensen,
qemu-devel, Max Reitz, Keith Busch, Javier Gonzalez,
Maxim Levitsky
On 7/3/20 12:00 PM, Klaus Jensen wrote:
> On Jul 3 10:27, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 8:34 AM, Klaus Jensen wrote:
>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>
>>> Since we are not providing the NGUID or EUI64 fields, we must support
>>> the Namespace UUID. We do not have any way of storing a persistent
>>> unique identifier, so conjure up a UUID that is just the namespace id.
>>>
>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
>>> ---
>>> hw/block/nvme.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>> hw/block/trace-events | 1 +
>>> 2 files changed, 42 insertions(+)
>>>
>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>> index 8230e0e3826b..65c2fa3ac1f4 100644
>>> --- a/hw/block/nvme.c
>>> +++ b/hw/block/nvme.c
>>> @@ -971,6 +971,45 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
>>> return ret;
>>> }
>>>
>>> +static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeIdentify *c)
>>> +{
>>> + uint32_t nsid = le32_to_cpu(c->nsid);
>>> + uint64_t prp1 = le64_to_cpu(c->prp1);
>>> + uint64_t prp2 = le64_to_cpu(c->prp2);
>>> +
>>> + uint8_t list[NVME_IDENTIFY_DATA_SIZE];
>>> +
>>> + struct data {
>>> + struct {
>>> + NvmeIdNsDescr hdr;
>>> + uint8_t v[16];
>>
>> You might consider to use QemuUUID from "qemu/uuid.h". The benefits
>> are you can use qemu_uuid_parse() qemu_uuid_unparse*() for tracing,
>> and DEFINE_PROP_UUID() in case you want to set a particular UUID
>> from command line, it case it is important to the guest.
>>
>
> Yes, definitely. Niklas also does this in his patch for namespace types
> support. And I think that it's very important that it can be made
> persistent, which would require a device property.
>
> Thus, if it is OK with the rest of you, I would like to defer this to
> when we merge the multiple namespaces patch and add "uuid" as a nvme-ns
> device property there. Then, we do not have to add the uuid property on
> the nvme device now and then have to keep it around when the namespace
> related properties moves to the nvme-ns device.
No objection, it was a simple suggestion to consider for later ;)
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 15/18] hw/block/nvme: reject invalid nsid values in active namespace id list
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
` (13 preceding siblings ...)
2020-07-03 6:34 ` [PATCH v2 14/18] hw/block/nvme: support identify namespace descriptor list Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 8:20 ` Philippe Mathieu-Daudé
2020-07-03 6:34 ` [PATCH v2 16/18] hw/block/nvme: enforce valid queue creation sequence Klaus Jensen
` (2 subsequent siblings)
17 siblings, 1 reply; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the
Active Namespace ID list.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/block/nvme.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 65c2fa3ac1f4..0dac7a41ddae 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
trace_pci_nvme_identify_nslist(min_nsid);
+ if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) {
+ return NVME_INVALID_NSID | NVME_DNR;
+ }
+
list = g_malloc0(data_len);
for (i = 0; i < n->num_namespaces; i++) {
if (i < min_nsid) {
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v2 15/18] hw/block/nvme: reject invalid nsid values in active namespace id list
2020-07-03 6:34 ` [PATCH v2 15/18] hw/block/nvme: reject invalid nsid values in active namespace id list Klaus Jensen
@ 2020-07-03 8:20 ` Philippe Mathieu-Daudé
2020-07-03 8:37 ` Klaus Jensen
0 siblings, 1 reply; 42+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-03 8:20 UTC (permalink / raw)
To: Klaus Jensen, qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Keith Busch, Javier Gonzalez, Maxim Levitsky
On 7/3/20 8:34 AM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the
> Active Namespace ID list.
Can we have a definition instead of this 0xfffffffe magic value please?
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
> hw/block/nvme.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 65c2fa3ac1f4..0dac7a41ddae 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
>
> trace_pci_nvme_identify_nslist(min_nsid);
>
> + if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) {
> + return NVME_INVALID_NSID | NVME_DNR;
> + }
> +
> list = g_malloc0(data_len);
> for (i = 0; i < n->num_namespaces; i++) {
> if (i < min_nsid) {
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 15/18] hw/block/nvme: reject invalid nsid values in active namespace id list
2020-07-03 8:20 ` Philippe Mathieu-Daudé
@ 2020-07-03 8:37 ` Klaus Jensen
2020-07-03 9:14 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 8:37 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, qemu-block, Dmitry Fomichev, Klaus Jensen,
qemu-devel, Max Reitz, Keith Busch, Javier Gonzalez,
Maxim Levitsky
On Jul 3 10:20, Philippe Mathieu-Daudé wrote:
> On 7/3/20 8:34 AM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the
> > Active Namespace ID list.
>
> Can we have a definition instead of this 0xfffffffe magic value please?
>
Hmm, not really actually. It's not a magic value, its just because the
logic in Active Namespace ID list would require that it should report
any namespaces with ids *higher* than the one specified, so since
0xffffffff (NVME_NSID_BROADCAST) is invalid, NVME_NSID_BROADCAST - 1
needs to be as well.
What do you say I change it to `min_nsid >= NVME_NSID_BROADCAST - 1`?
The original condition just reads well if you are sitting with the spec
on the side.
> >
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> > hw/block/nvme.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 65c2fa3ac1f4..0dac7a41ddae 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
> >
> > trace_pci_nvme_identify_nslist(min_nsid);
> >
> > + if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) {
> > + return NVME_INVALID_NSID | NVME_DNR;
> > + }
> > +
> > list = g_malloc0(data_len);
> > for (i = 0; i < n->num_namespaces; i++) {
> > if (i < min_nsid) {
> >
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 15/18] hw/block/nvme: reject invalid nsid values in active namespace id list
2020-07-03 8:37 ` Klaus Jensen
@ 2020-07-03 9:14 ` Philippe Mathieu-Daudé
2020-07-03 9:22 ` Klaus Jensen
0 siblings, 1 reply; 42+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-03 9:14 UTC (permalink / raw)
To: Klaus Jensen
Cc: Kevin Wolf, qemu-block, Dmitry Fomichev, Klaus Jensen,
qemu-devel, Max Reitz, Keith Busch, Javier Gonzalez,
Maxim Levitsky
On 7/3/20 10:37 AM, Klaus Jensen wrote:
> On Jul 3 10:20, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 8:34 AM, Klaus Jensen wrote:
>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>
>>> Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the
>>> Active Namespace ID list.
>>
>> Can we have a definition instead of this 0xfffffffe magic value please?
>>
>
> Hmm, not really actually. It's not a magic value, its just because the
> logic in Active Namespace ID list would require that it should report
> any namespaces with ids *higher* than the one specified, so since
> 0xffffffff (NVME_NSID_BROADCAST) is invalid, NVME_NSID_BROADCAST - 1
> needs to be as well.
OK.
>
> What do you say I change it to `min_nsid >= NVME_NSID_BROADCAST - 1`?
> The original condition just reads well if you are sitting with the spec
> on the side.
IMO this is clearer:
if (min_nsid + 1 >= NVME_NSID_BROADCAST) {
return NVME_INVALID_NSID | NVME_DNR;
}
Whichever form you prefer you can amend to the respin patch:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
>>>
>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>> ---
>>> hw/block/nvme.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>> index 65c2fa3ac1f4..0dac7a41ddae 100644
>>> --- a/hw/block/nvme.c
>>> +++ b/hw/block/nvme.c
>>> @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
>>>
>>> trace_pci_nvme_identify_nslist(min_nsid);
>>>
>>> + if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) {
>>> + return NVME_INVALID_NSID | NVME_DNR;
>>> + }
>>> +
>>> list = g_malloc0(data_len);
>>> for (i = 0; i < n->num_namespaces; i++) {
>>> if (i < min_nsid) {
>>>
>>
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 15/18] hw/block/nvme: reject invalid nsid values in active namespace id list
2020-07-03 9:14 ` Philippe Mathieu-Daudé
@ 2020-07-03 9:22 ` Klaus Jensen
2020-07-03 10:59 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 9:22 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, qemu-block, Dmitry Fomichev, Klaus Jensen,
qemu-devel, Max Reitz, Keith Busch, Javier Gonzalez,
Maxim Levitsky
On Jul 3 11:14, Philippe Mathieu-Daudé wrote:
> On 7/3/20 10:37 AM, Klaus Jensen wrote:
> > On Jul 3 10:20, Philippe Mathieu-Daudé wrote:
> >> On 7/3/20 8:34 AM, Klaus Jensen wrote:
> >>> From: Klaus Jensen <k.jensen@samsung.com>
> >>>
> >>> Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the
> >>> Active Namespace ID list.
> >>
> >> Can we have a definition instead of this 0xfffffffe magic value please?
> >>
> >
> > Hmm, not really actually. It's not a magic value, its just because the
> > logic in Active Namespace ID list would require that it should report
> > any namespaces with ids *higher* than the one specified, so since
> > 0xffffffff (NVME_NSID_BROADCAST) is invalid, NVME_NSID_BROADCAST - 1
> > needs to be as well.
>
> OK.
>
> >
> > What do you say I change it to `min_nsid >= NVME_NSID_BROADCAST - 1`?
> > The original condition just reads well if you are sitting with the spec
> > on the side.
>
> IMO this is clearer:
>
> if (min_nsid + 1 >= NVME_NSID_BROADCAST) {
> return NVME_INVALID_NSID | NVME_DNR;
> }
>
But since min_nsid is uint32_t that would not be wise ;)
I'll go with the - 1 and add a comment!
> Whichever form you prefer you can amend to the respin patch:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> >
> >>>
> >>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> >>> ---
> >>> hw/block/nvme.c | 4 ++++
> >>> 1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> >>> index 65c2fa3ac1f4..0dac7a41ddae 100644
> >>> --- a/hw/block/nvme.c
> >>> +++ b/hw/block/nvme.c
> >>> @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
> >>>
> >>> trace_pci_nvme_identify_nslist(min_nsid);
> >>>
> >>> + if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) {
> >>> + return NVME_INVALID_NSID | NVME_DNR;
> >>> + }
> >>> +
> >>> list = g_malloc0(data_len);
> >>> for (i = 0; i < n->num_namespaces; i++) {
> >>> if (i < min_nsid) {
> >>>
> >>
> >
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 15/18] hw/block/nvme: reject invalid nsid values in active namespace id list
2020-07-03 9:22 ` Klaus Jensen
@ 2020-07-03 10:59 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 42+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-03 10:59 UTC (permalink / raw)
To: Klaus Jensen
Cc: Kevin Wolf, qemu-block, Dmitry Fomichev, Klaus Jensen,
qemu-devel, Max Reitz, Keith Busch, Javier Gonzalez,
Maxim Levitsky
On 7/3/20 11:22 AM, Klaus Jensen wrote:
> On Jul 3 11:14, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 10:37 AM, Klaus Jensen wrote:
>>> On Jul 3 10:20, Philippe Mathieu-Daudé wrote:
>>>> On 7/3/20 8:34 AM, Klaus Jensen wrote:
>>>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>>>
>>>>> Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the
>>>>> Active Namespace ID list.
>>>>
>>>> Can we have a definition instead of this 0xfffffffe magic value please?
>>>>
>>>
>>> Hmm, not really actually. It's not a magic value, its just because the
>>> logic in Active Namespace ID list would require that it should report
>>> any namespaces with ids *higher* than the one specified, so since
>>> 0xffffffff (NVME_NSID_BROADCAST) is invalid, NVME_NSID_BROADCAST - 1
>>> needs to be as well.
>>
>> OK.
>>
>>>
>>> What do you say I change it to `min_nsid >= NVME_NSID_BROADCAST - 1`?
>>> The original condition just reads well if you are sitting with the spec
>>> on the side.
>>
>> IMO this is clearer:
>>
>> if (min_nsid + 1 >= NVME_NSID_BROADCAST) {
>> return NVME_INVALID_NSID | NVME_DNR;
>> }
>>
>
> But since min_nsid is uint32_t that would not be wise ;)
Hmm indeed.
>
> I'll go with the - 1 and add a comment!
Good, thanks.
>
>> Whichever form you prefer you can amend to the respin patch:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>>>
>>>>>
>>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>>>> ---
>>>>> hw/block/nvme.c | 4 ++++
>>>>> 1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>>>> index 65c2fa3ac1f4..0dac7a41ddae 100644
>>>>> --- a/hw/block/nvme.c
>>>>> +++ b/hw/block/nvme.c
>>>>> @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
>>>>>
>>>>> trace_pci_nvme_identify_nslist(min_nsid);
>>>>>
>>>>> + if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) {
>>>>> + return NVME_INVALID_NSID | NVME_DNR;
>>>>> + }
>>>>> +
>>>>> list = g_malloc0(data_len);
>>>>> for (i = 0; i < n->num_namespaces; i++) {
>>>>> if (i < min_nsid) {
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 16/18] hw/block/nvme: enforce valid queue creation sequence
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
` (14 preceding siblings ...)
2020-07-03 6:34 ` [PATCH v2 15/18] hw/block/nvme: reject invalid nsid values in active namespace id list Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 6:34 ` [PATCH v2 17/18] hw/block/nvme: provide the mandatory subnqn field Klaus Jensen
2020-07-03 6:34 ` [PATCH v2 18/18] hw/block/nvme: bump supported version to v1.3 Klaus Jensen
17 siblings, 0 replies; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
Support returning Command Sequence Error if Set Features on Number of
Queues is called after queues have been created.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
hw/block/nvme.c | 12 ++++++++++++
hw/block/nvme.h | 1 +
2 files changed, 13 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 0dac7a41ddae..8138baa6fbd8 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -910,6 +910,13 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd *cmd)
cq = g_malloc0(sizeof(*cq));
nvme_init_cq(cq, n, prp1, cqid, vector, qsize + 1,
NVME_CQ_FLAGS_IEN(qflags));
+
+ /*
+ * It is only required to set qs_created when creating a completion queue;
+ * creating a submission queue without a matching completion queue will
+ * fail.
+ */
+ n->qs_created = true;
return NVME_SUCCESS;
}
@@ -1301,6 +1308,10 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
blk_set_enable_write_cache(n->conf.blk, dw11 & 1);
break;
case NVME_NUMBER_OF_QUEUES:
+ if (n->qs_created) {
+ return NVME_CMD_SEQ_ERROR | NVME_DNR;
+ }
+
/*
* NVMe v1.3, Section 5.21.1.7: 0xffff is not an allowed value for NCQR
* and NSQR.
@@ -1433,6 +1444,7 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
n->aer_queued = 0;
n->outstanding_aers = 0;
+ n->qs_created = false;
blk_flush(n->conf.blk);
n->bar.cc = 0;
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 450e4294193a..668d2cd3df16 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -121,6 +121,7 @@ typedef struct NvmeCtrl {
BlockConf conf;
NvmeParams params;
+ bool qs_created;
uint32_t page_size;
uint16_t page_bits;
uint16_t max_prp_ents;
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v2 17/18] hw/block/nvme: provide the mandatory subnqn field
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
` (15 preceding siblings ...)
2020-07-03 6:34 ` [PATCH v2 16/18] hw/block/nvme: enforce valid queue creation sequence Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
2020-07-03 8:18 ` Philippe Mathieu-Daudé
2020-07-03 6:34 ` [PATCH v2 18/18] hw/block/nvme: bump supported version to v1.3 Klaus Jensen
17 siblings, 1 reply; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
The SUBNQN field is mandatory in NVM Express 1.3.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
hw/block/nvme.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 8138baa6fbd8..5bbb6aa0efc3 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2134,6 +2134,9 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS | NVME_ONCS_TIMESTAMP |
NVME_ONCS_FEATURES);
+ pstrcpy((char *) id->subnqn, sizeof(id->subnqn), "nqn.2019-08.org.qemu:");
+ pstrcat((char *) id->subnqn, sizeof(id->subnqn), n->params.serial);
+
id->psd[0].mp = cpu_to_le16(0x9c4);
id->psd[0].enlat = cpu_to_le32(0x10);
id->psd[0].exlat = cpu_to_le32(0x4);
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v2 17/18] hw/block/nvme: provide the mandatory subnqn field
2020-07-03 6:34 ` [PATCH v2 17/18] hw/block/nvme: provide the mandatory subnqn field Klaus Jensen
@ 2020-07-03 8:18 ` Philippe Mathieu-Daudé
2020-07-03 8:25 ` Klaus Jensen
0 siblings, 1 reply; 42+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-03 8:18 UTC (permalink / raw)
To: Klaus Jensen, qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Keith Busch, Javier Gonzalez, Maxim Levitsky
On 7/3/20 8:34 AM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> The SUBNQN field is mandatory in NVM Express 1.3.
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
> hw/block/nvme.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 8138baa6fbd8..5bbb6aa0efc3 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -2134,6 +2134,9 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
> id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS | NVME_ONCS_TIMESTAMP |
> NVME_ONCS_FEATURES);
>
> + pstrcpy((char *) id->subnqn, sizeof(id->subnqn), "nqn.2019-08.org.qemu:");
> + pstrcat((char *) id->subnqn, sizeof(id->subnqn), n->params.serial);
What about using strpadcpy()?
char *subnqn = g_strdup_printf("nqn.2019-08.org.qemu:%s",
n->params.serial);
strpadcpy((char *)id->subnqn, sizeof(id->subnqn), subnqn, '\0');
g_free(subnqn);
> +
> id->psd[0].mp = cpu_to_le16(0x9c4);
> id->psd[0].enlat = cpu_to_le32(0x10);
> id->psd[0].exlat = cpu_to_le32(0x4);
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 17/18] hw/block/nvme: provide the mandatory subnqn field
2020-07-03 8:18 ` Philippe Mathieu-Daudé
@ 2020-07-03 8:25 ` Klaus Jensen
0 siblings, 0 replies; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 8:25 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, qemu-block, Dmitry Fomichev, Klaus Jensen,
qemu-devel, Max Reitz, Keith Busch, Javier Gonzalez,
Maxim Levitsky
On Jul 3 10:18, Philippe Mathieu-Daudé wrote:
> On 7/3/20 8:34 AM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > The SUBNQN field is mandatory in NVM Express 1.3.
> >
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> > Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > ---
> > hw/block/nvme.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 8138baa6fbd8..5bbb6aa0efc3 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -2134,6 +2134,9 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
> > id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS | NVME_ONCS_TIMESTAMP |
> > NVME_ONCS_FEATURES);
> >
> > + pstrcpy((char *) id->subnqn, sizeof(id->subnqn), "nqn.2019-08.org.qemu:");
> > + pstrcat((char *) id->subnqn, sizeof(id->subnqn), n->params.serial);
>
> What about using strpadcpy()?
>
> char *subnqn = g_strdup_printf("nqn.2019-08.org.qemu:%s",
> n->params.serial);
> strpadcpy((char *)id->subnqn, sizeof(id->subnqn), subnqn, '\0');
> g_free(subnqn);
>
Thanks, that's better. Fixed!
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 18/18] hw/block/nvme: bump supported version to v1.3
2020-07-03 6:34 [PATCH v2 00/18] hw/block/nvme: bump to v1.3 Klaus Jensen
` (16 preceding siblings ...)
2020-07-03 6:34 ` [PATCH v2 17/18] hw/block/nvme: provide the mandatory subnqn field Klaus Jensen
@ 2020-07-03 6:34 ` Klaus Jensen
17 siblings, 0 replies; 42+ messages in thread
From: Klaus Jensen @ 2020-07-03 6:34 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Dmitry Fomichev, Klaus Jensen, qemu-devel, Max Reitz,
Klaus Jensen, Keith Busch, Javier Gonzalez, Maxim Levitsky,
Philippe Mathieu-Daudé
From: Klaus Jensen <k.jensen@samsung.com>
Bump the supported NVM Express version to v1.3.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
hw/block/nvme.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5bbb6aa0efc3..204032aac2c8 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -57,6 +57,7 @@
#define NVME_MAX_IOQPAIRS 0xffff
#define NVME_REG_SIZE 0x1000
#define NVME_DB_SIZE 4
+#define NVME_SPEC_VER 0x00010300
#define NVME_CMB_BIR 2
#define NVME_PMR_BIR 2
#define NVME_TEMPERATURE 0x143
@@ -2106,6 +2107,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
id->ieee[0] = 0x00;
id->ieee[1] = 0x02;
id->ieee[2] = 0xb3;
+ id->ver = cpu_to_le32(NVME_SPEC_VER);
id->oacs = cpu_to_le16(0);
/*
@@ -2151,7 +2153,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
NVME_CAP_SET_CSS(n->bar.cap, 1);
NVME_CAP_SET_MPSMAX(n->bar.cap, 4);
- n->bar.vs = 0x00010200;
+ n->bar.vs = NVME_SPEC_VER;
n->bar.intmc = n->bar.intms = 0;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 42+ messages in thread