* [PATCH v2 0/5] hw/nvme: fix mmio read
@ 2021-07-13 19:24 Klaus Jensen
2021-07-13 19:24 ` [PATCH v2 1/5] hw/nvme: split pmrmsc register into upper and lower Klaus Jensen
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Klaus Jensen @ 2021-07-13 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Thomas Huth, qemu-block, Peter Maydell,
Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, Max Reitz,
Keith Busch, Stefan Hajnoczi, Klaus Jensen, Paolo Bonzini
From: Klaus Jensen <k.jensen@samsung.com>
Fix mmio read issues on big-endian hosts. The core issue is that values
in the BAR is not stored in little endian as required.
Fix that and add a regression test for this. This required a bit of
cleanup, so it blew up into a series.
Klaus Jensen (5):
hw/nvme: split pmrmsc register into upper and lower
hw/nvme: use symbolic names for registers
hw/nvme: fix out-of-bounds reads
hw/nvme: fix mmio read
tests/qtest/nvme-test: add mmio read test
include/block/nvme.h | 58 +++++--
hw/nvme/ctrl.c | 366 +++++++++++++++++++++++-----------------
tests/qtest/nvme-test.c | 26 +++
3 files changed, 276 insertions(+), 174 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/5] hw/nvme: split pmrmsc register into upper and lower
2021-07-13 19:24 [PATCH v2 0/5] hw/nvme: fix mmio read Klaus Jensen
@ 2021-07-13 19:24 ` Klaus Jensen
2021-07-13 19:24 ` [PATCH v2 2/5] hw/nvme: use symbolic names for registers Klaus Jensen
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Klaus Jensen @ 2021-07-13 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Thomas Huth, qemu-block, Peter Maydell,
Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, Max Reitz,
Keith Busch, Stefan Hajnoczi, Klaus Jensen, Paolo Bonzini
From: Klaus Jensen <k.jensen@samsung.com>
The specification uses a set of 32 bit PMRMSCL and PMRMSCU registers to
make up the 64 bit logical PMRMSC register.
Make it so.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
include/block/nvme.h | 31 ++++++++++++++++---------------
hw/nvme/ctrl.c | 9 +++++----
2 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 527105fafc0b..84053b68b987 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -26,7 +26,8 @@ typedef struct QEMU_PACKED NvmeBar {
uint32_t pmrsts;
uint32_t pmrebs;
uint32_t pmrswtp;
- uint64_t pmrmsc;
+ uint32_t pmrmscl;
+ uint32_t pmrmscu;
uint8_t css[484];
} NvmeBar;
@@ -475,25 +476,25 @@ enum NvmePmrswtpMask {
#define NVME_PMRSWTP_SET_PMRSWTV(pmrswtp, val) \
(pmrswtp |= (uint64_t)(val & PMRSWTP_PMRSWTV_MASK) << PMRSWTP_PMRSWTV_SHIFT)
-enum NvmePmrmscShift {
- PMRMSC_CMSE_SHIFT = 1,
- PMRMSC_CBA_SHIFT = 12,
+enum NvmePmrmsclShift {
+ PMRMSCL_CMSE_SHIFT = 1,
+ PMRMSCL_CBA_SHIFT = 12,
};
-enum NvmePmrmscMask {
- PMRMSC_CMSE_MASK = 0x1,
- PMRMSC_CBA_MASK = 0xfffffffffffff,
+enum NvmePmrmsclMask {
+ PMRMSCL_CMSE_MASK = 0x1,
+ PMRMSCL_CBA_MASK = 0xfffff,
};
-#define NVME_PMRMSC_CMSE(pmrmsc) \
- ((pmrmsc >> PMRMSC_CMSE_SHIFT) & PMRMSC_CMSE_MASK)
-#define NVME_PMRMSC_CBA(pmrmsc) \
- ((pmrmsc >> PMRMSC_CBA_SHIFT) & PMRMSC_CBA_MASK)
+#define NVME_PMRMSCL_CMSE(pmrmscl) \
+ ((pmrmscl >> PMRMSCL_CMSE_SHIFT) & PMRMSCL_CMSE_MASK)
+#define NVME_PMRMSCL_CBA(pmrmscl) \
+ ((pmrmscl >> PMRMSCL_CBA_SHIFT) & PMRMSCL_CBA_MASK)
-#define NVME_PMRMSC_SET_CMSE(pmrmsc, val) \
- (pmrmsc |= (uint64_t)(val & PMRMSC_CMSE_MASK) << PMRMSC_CMSE_SHIFT)
-#define NVME_PMRMSC_SET_CBA(pmrmsc, val) \
- (pmrmsc |= (uint64_t)(val & PMRMSC_CBA_MASK) << PMRMSC_CBA_SHIFT)
+#define NVME_PMRMSCL_SET_CMSE(pmrmscl, val) \
+ (pmrmscl |= (uint32_t)(val & PMRMSCL_CMSE_MASK) << PMRMSCL_CMSE_SHIFT)
+#define NVME_PMRMSCL_SET_CBA(pmrmscl, val) \
+ (pmrmscl |= (uint32_t)(val & PMRMSCL_CBA_MASK) << PMRMSCL_CBA_SHIFT)
enum NvmeSglDescriptorType {
NVME_SGL_DESCR_TYPE_DATA_BLOCK = 0x0,
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 2f0524e12a36..28299c6f3764 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5916,11 +5916,12 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
return;
}
- n->bar.pmrmsc = (n->bar.pmrmsc & ~0xffffffff) | (data & 0xffffffff);
+ n->bar.pmrmscl = data & 0xffffffff;
n->pmr.cmse = false;
- if (NVME_PMRMSC_CMSE(n->bar.pmrmsc)) {
- hwaddr cba = NVME_PMRMSC_CBA(n->bar.pmrmsc) << PMRMSC_CBA_SHIFT;
+ if (NVME_PMRMSCL_CMSE(n->bar.pmrmscl)) {
+ hwaddr cba = n->bar.pmrmscu |
+ (NVME_PMRMSCL_CBA(n->bar.pmrmscl) << PMRMSCL_CBA_SHIFT);
if (cba + int128_get64(n->pmr.dev->mr.size) < cba) {
NVME_PMRSTS_SET_CBAI(n->bar.pmrsts, 1);
return;
@@ -5936,7 +5937,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
return;
}
- n->bar.pmrmsc = (n->bar.pmrmsc & 0xffffffff) | (data << 32);
+ n->bar.pmrmscu = data & 0xffffffff;
return;
default:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_invalid,
--
2.32.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/5] hw/nvme: use symbolic names for registers
2021-07-13 19:24 [PATCH v2 0/5] hw/nvme: fix mmio read Klaus Jensen
2021-07-13 19:24 ` [PATCH v2 1/5] hw/nvme: split pmrmsc register into upper and lower Klaus Jensen
@ 2021-07-13 19:24 ` Klaus Jensen
2021-07-13 22:12 ` Philippe Mathieu-Daudé
2021-07-13 19:24 ` [PATCH v2 3/5] hw/nvme: fix out-of-bounds reads Klaus Jensen
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Klaus Jensen @ 2021-07-13 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Thomas Huth, qemu-block, Peter Maydell,
Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, Max Reitz,
Keith Busch, Stefan Hajnoczi, Klaus Jensen, Paolo Bonzini
From: Klaus Jensen <k.jensen@samsung.com>
Add the NvmeBarRegs enum and use these instead of explicit register
offsets.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
include/block/nvme.h | 27 +++++++++++++++++++++++++++
hw/nvme/ctrl.c | 44 ++++++++++++++++++++++----------------------
2 files changed, 49 insertions(+), 22 deletions(-)
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 84053b68b987..082d4bddbf9f 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -31,6 +31,33 @@ typedef struct QEMU_PACKED NvmeBar {
uint8_t css[484];
} NvmeBar;
+enum NvmeBarRegs {
+ NVME_REG_CAP = 0x0,
+ NVME_REG_VS = 0x8,
+ NVME_REG_INTMS = 0xc,
+ NVME_REG_INTMC = 0x10,
+ NVME_REG_CC = 0x14,
+ NVME_REG_CSTS = 0x1c,
+ NVME_REG_NSSR = 0x20,
+ NVME_REG_AQA = 0x24,
+ NVME_REG_ASQ = 0x28,
+ NVME_REG_ACQ = 0x30,
+ NVME_REG_CMBLOC = 0x38,
+ NVME_REG_CMBSZ = 0x3c,
+ NVME_REG_BPINFO = 0x40,
+ NVME_REG_BPRSEL = 0x44,
+ NVME_REG_BPMBL = 0x48,
+ NVME_REG_CMBMSC = 0x50,
+ NVME_REG_CMBSTS = 0x58,
+ NVME_REG_PMRCAP = 0xe00,
+ NVME_REG_PMRCTL = 0xe04,
+ NVME_REG_PMRSTS = 0xe08,
+ NVME_REG_PMREBS = 0xe0c,
+ NVME_REG_PMRSWTP = 0xe10,
+ NVME_REG_PMRMSCL = 0xe14,
+ NVME_REG_PMRMSCU = 0xe18,
+};
+
enum NvmeCapShift {
CAP_MQES_SHIFT = 0,
CAP_CQR_SHIFT = 16,
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 28299c6f3764..8c305315f41c 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5740,7 +5740,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
}
switch (offset) {
- case 0xc: /* INTMS */
+ case NVME_REG_INTMS:
if (unlikely(msix_enabled(&(n->parent_obj)))) {
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_intmask_with_msix,
"undefined access to interrupt mask set"
@@ -5752,7 +5752,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
trace_pci_nvme_mmio_intm_set(data & 0xffffffff, n->bar.intmc);
nvme_irq_check(n);
break;
- case 0x10: /* INTMC */
+ case NVME_REG_INTMC:
if (unlikely(msix_enabled(&(n->parent_obj)))) {
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_intmask_with_msix,
"undefined access to interrupt mask clr"
@@ -5764,7 +5764,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
trace_pci_nvme_mmio_intm_clr(data & 0xffffffff, n->bar.intmc);
nvme_irq_check(n);
break;
- case 0x14: /* CC */
+ case NVME_REG_CC:
trace_pci_nvme_mmio_cfg(data & 0xffffffff);
/* Windows first sends data, then sends enable bit */
if (!NVME_CC_EN(data) && !NVME_CC_EN(n->bar.cc) &&
@@ -5798,7 +5798,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
n->bar.cc = data;
}
break;
- case 0x1c: /* CSTS */
+ case NVME_REG_CSTS:
if (data & (1 << 4)) {
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_ssreset_w1c_unsupported,
"attempted to W1C CSTS.NSSRO"
@@ -5809,7 +5809,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
" of controller status");
}
break;
- case 0x20: /* NSSR */
+ case NVME_REG_NSSR:
if (data == 0x4e564d65) {
trace_pci_nvme_ub_mmiowr_ssreset_unsupported();
} else {
@@ -5817,38 +5817,38 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
return;
}
break;
- case 0x24: /* AQA */
+ case NVME_REG_AQA:
n->bar.aqa = data & 0xffffffff;
trace_pci_nvme_mmio_aqattr(data & 0xffffffff);
break;
- case 0x28: /* ASQ */
+ case NVME_REG_ASQ:
n->bar.asq = size == 8 ? data :
(n->bar.asq & ~0xffffffffULL) | (data & 0xffffffff);
trace_pci_nvme_mmio_asqaddr(data);
break;
- case 0x2c: /* ASQ hi */
+ case NVME_REG_ASQ + 4:
n->bar.asq = (n->bar.asq & 0xffffffff) | (data << 32);
trace_pci_nvme_mmio_asqaddr_hi(data, n->bar.asq);
break;
- case 0x30: /* ACQ */
+ case NVME_REG_ACQ:
trace_pci_nvme_mmio_acqaddr(data);
n->bar.acq = size == 8 ? data :
(n->bar.acq & ~0xffffffffULL) | (data & 0xffffffff);
break;
- case 0x34: /* ACQ hi */
+ case NVME_REG_ACQ + 4:
n->bar.acq = (n->bar.acq & 0xffffffff) | (data << 32);
trace_pci_nvme_mmio_acqaddr_hi(data, n->bar.acq);
break;
- case 0x38: /* CMBLOC */
+ case NVME_REG_CMBLOC:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_cmbloc_reserved,
"invalid write to reserved CMBLOC"
" when CMBSZ is zero, ignored");
return;
- case 0x3C: /* CMBSZ */
+ case NVME_REG_CMBSZ:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_cmbsz_readonly,
"invalid write to read only CMBSZ, ignored");
return;
- case 0x50: /* CMBMSC */
+ case NVME_REG_CMBMSC:
if (!NVME_CAP_CMBS(n->bar.cap)) {
return;
}
@@ -5876,15 +5876,15 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
}
return;
- case 0x54: /* CMBMSC hi */
+ case NVME_REG_CMBMSC + 4:
n->bar.cmbmsc = (n->bar.cmbmsc & 0xffffffff) | (data << 32);
return;
- case 0xe00: /* PMRCAP */
+ case NVME_REG_PMRCAP:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrcap_readonly,
"invalid write to PMRCAP register, ignored");
return;
- case 0xe04: /* PMRCTL */
+ case NVME_REG_PMRCTL:
if (!NVME_CAP_PMRS(n->bar.cap)) {
return;
}
@@ -5899,19 +5899,19 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
n->pmr.cmse = false;
}
return;
- case 0xe08: /* PMRSTS */
+ case NVME_REG_PMRSTS:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrsts_readonly,
"invalid write to PMRSTS register, ignored");
return;
- case 0xe0C: /* PMREBS */
+ case NVME_REG_PMREBS:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrebs_readonly,
"invalid write to PMREBS register, ignored");
return;
- case 0xe10: /* PMRSWTP */
+ case NVME_REG_PMRSWTP:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrswtp_readonly,
"invalid write to PMRSWTP register, ignored");
return;
- case 0xe14: /* PMRMSCL */
+ case NVME_REG_PMRMSCL:
if (!NVME_CAP_PMRS(n->bar.cap)) {
return;
}
@@ -5932,7 +5932,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
}
return;
- case 0xe18: /* PMRMSCU */
+ case NVME_REG_PMRMSCU:
if (!NVME_CAP_PMRS(n->bar.cap)) {
return;
}
@@ -5974,7 +5974,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
* from PMRSTS should ensure prior writes
* made it to persistent media
*/
- if (addr == 0xe08 &&
+ if (addr == NVME_REG_PMRSTS &&
(NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
}
--
2.32.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/5] hw/nvme: fix out-of-bounds reads
2021-07-13 19:24 [PATCH v2 0/5] hw/nvme: fix mmio read Klaus Jensen
2021-07-13 19:24 ` [PATCH v2 1/5] hw/nvme: split pmrmsc register into upper and lower Klaus Jensen
2021-07-13 19:24 ` [PATCH v2 2/5] hw/nvme: use symbolic names for registers Klaus Jensen
@ 2021-07-13 19:24 ` Klaus Jensen
2021-07-13 19:24 ` [PATCH v2 4/5] hw/nvme: fix mmio read Klaus Jensen
2021-07-13 19:24 ` [PATCH v2 5/5] tests/qtest/nvme-test: add mmio read test Klaus Jensen
4 siblings, 0 replies; 11+ messages in thread
From: Klaus Jensen @ 2021-07-13 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Thomas Huth, qemu-block, Peter Maydell,
Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, Max Reitz,
Keith Busch, Stefan Hajnoczi, Klaus Jensen, Paolo Bonzini
From: Klaus Jensen <k.jensen@samsung.com>
Peter noticed that mmio access may read into the NvmeParams member in
the NvmeCtrl struct.
Fix the bounds check.
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/nvme/ctrl.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 8c305315f41c..0449cc4dee9b 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5968,23 +5968,26 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
/* should RAZ, fall through for now */
}
- if (addr < sizeof(n->bar)) {
- /*
- * When PMRWBM bit 1 is set then read from
- * from PMRSTS should ensure prior writes
- * made it to persistent media
- */
- if (addr == NVME_REG_PMRSTS &&
- (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
- memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
- }
- memcpy(&val, ptr + addr, size);
- } else {
+ if (addr > sizeof(n->bar) - size) {
NVME_GUEST_ERR(pci_nvme_ub_mmiord_invalid_ofs,
"MMIO read beyond last register,"
" offset=0x%"PRIx64", returning 0", addr);
+
+ return 0;
}
+ /*
+ * When PMRWBM bit 1 is set then read from
+ * from PMRSTS should ensure prior writes
+ * made it to persistent media
+ */
+ if (addr == NVME_REG_PMRSTS &&
+ (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
+ memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
+ }
+
+ memcpy(&val, ptr + addr, size);
+
return val;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 4/5] hw/nvme: fix mmio read
2021-07-13 19:24 [PATCH v2 0/5] hw/nvme: fix mmio read Klaus Jensen
` (2 preceding siblings ...)
2021-07-13 19:24 ` [PATCH v2 3/5] hw/nvme: fix out-of-bounds reads Klaus Jensen
@ 2021-07-13 19:24 ` Klaus Jensen
2021-07-13 22:18 ` Philippe Mathieu-Daudé
2021-07-13 19:24 ` [PATCH v2 5/5] tests/qtest/nvme-test: add mmio read test Klaus Jensen
4 siblings, 1 reply; 11+ messages in thread
From: Klaus Jensen @ 2021-07-13 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Thomas Huth, qemu-block, Peter Maydell,
Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, Max Reitz,
Keith Busch, Stefan Hajnoczi, Klaus Jensen, Paolo Bonzini
From: Klaus Jensen <k.jensen@samsung.com>
The new PMR test unearthed a long-standing issue with MMIO reads on
big-endian hosts.
Fix this by unconditionally storing all controller registers in little
endian.
Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/nvme/ctrl.c | 304 ++++++++++++++++++++++++++++---------------------
1 file changed, 174 insertions(+), 130 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 0449cc4dee9b..ddac9344a74e 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -439,10 +439,12 @@ static uint8_t nvme_sq_empty(NvmeSQueue *sq)
static void nvme_irq_check(NvmeCtrl *n)
{
+ uint32_t intms = le32_to_cpu(n->bar.intms);
+
if (msix_enabled(&(n->parent_obj))) {
return;
}
- if (~n->bar.intms & n->irq_status) {
+ if (~intms & n->irq_status) {
pci_irq_assert(&n->parent_obj);
} else {
pci_irq_deassert(&n->parent_obj);
@@ -1289,7 +1291,7 @@ static void nvme_post_cqes(void *opaque)
if (ret) {
trace_pci_nvme_err_addr_write(addr);
trace_pci_nvme_err_cfs();
- n->bar.csts = NVME_CSTS_FAILED;
+ n->bar.csts = cpu_to_le64(NVME_CSTS_FAILED);
break;
}
QTAILQ_REMOVE(&cq->req_list, req, entry);
@@ -4022,7 +4024,7 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeRequest *req)
trace_pci_nvme_err_invalid_create_sq_sqid(sqid);
return NVME_INVALID_QID | NVME_DNR;
}
- if (unlikely(!qsize || qsize > NVME_CAP_MQES(n->bar.cap))) {
+ if (unlikely(!qsize || qsize > NVME_CAP_MQES(le64_to_cpu(n->bar.cap)))) {
trace_pci_nvme_err_invalid_create_sq_size(qsize);
return NVME_MAX_QSIZE_EXCEEDED | NVME_DNR;
}
@@ -4208,7 +4210,7 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t csi, uint32_t buf_len,
return NVME_INVALID_FIELD | NVME_DNR;
}
- switch (NVME_CC_CSS(n->bar.cc)) {
+ switch (NVME_CC_CSS(le32_to_cpu(n->bar.cc))) {
case NVME_CC_CSS_NVM:
src_iocs = nvme_cse_iocs_nvm;
/* fall through */
@@ -4370,7 +4372,7 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeRequest *req)
trace_pci_nvme_err_invalid_create_cq_cqid(cqid);
return NVME_INVALID_QID | NVME_DNR;
}
- if (unlikely(!qsize || qsize > NVME_CAP_MQES(n->bar.cap))) {
+ if (unlikely(!qsize || qsize > NVME_CAP_MQES(le64_to_cpu(n->bar.cap)))) {
trace_pci_nvme_err_invalid_create_cq_size(qsize);
return NVME_MAX_QSIZE_EXCEEDED | NVME_DNR;
}
@@ -5163,17 +5165,19 @@ static void nvme_update_dmrsl(NvmeCtrl *n)
static void nvme_select_iocs_ns(NvmeCtrl *n, NvmeNamespace *ns)
{
+ uint32_t cc = le32_to_cpu(n->bar.cc);
+
ns->iocs = nvme_cse_iocs_none;
switch (ns->csi) {
case NVME_CSI_NVM:
- if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY) {
+ if (NVME_CC_CSS(cc) != NVME_CC_CSS_ADMIN_ONLY) {
ns->iocs = nvme_cse_iocs_nvm;
}
break;
case NVME_CSI_ZONED:
- if (NVME_CC_CSS(n->bar.cc) == NVME_CC_CSS_CSI) {
+ if (NVME_CC_CSS(cc) == NVME_CC_CSS_CSI) {
ns->iocs = nvme_cse_iocs_zoned;
- } else if (NVME_CC_CSS(n->bar.cc) == NVME_CC_CSS_NVM) {
+ } else if (NVME_CC_CSS(cc) == NVME_CC_CSS_NVM) {
ns->iocs = nvme_cse_iocs_nvm;
}
break;
@@ -5510,7 +5514,7 @@ static void nvme_process_sq(void *opaque)
if (nvme_addr_read(n, addr, (void *)&cmd, sizeof(cmd))) {
trace_pci_nvme_err_addr_read(addr);
trace_pci_nvme_err_cfs();
- n->bar.csts = NVME_CSTS_FAILED;
+ n->bar.csts = cpu_to_le32(NVME_CSTS_FAILED);
break;
}
nvme_inc_sq_head(sq);
@@ -5565,8 +5569,6 @@ static void nvme_ctrl_reset(NvmeCtrl *n)
n->aer_queued = 0;
n->outstanding_aers = 0;
n->qs_created = false;
-
- n->bar.cc = 0;
}
static void nvme_ctrl_shutdown(NvmeCtrl *n)
@@ -5605,7 +5607,12 @@ static void nvme_select_iocs(NvmeCtrl *n)
static int nvme_start_ctrl(NvmeCtrl *n)
{
- uint32_t page_bits = NVME_CC_MPS(n->bar.cc) + 12;
+ uint64_t cap = le64_to_cpu(n->bar.cap);
+ uint32_t cc = le32_to_cpu(n->bar.cc);
+ uint32_t aqa = le32_to_cpu(n->bar.aqa);
+ uint64_t asq = le64_to_cpu(n->bar.asq);
+ uint64_t acq = le64_to_cpu(n->bar.acq);
+ uint32_t page_bits = NVME_CC_MPS(cc) + 12;
uint32_t page_size = 1 << page_bits;
if (unlikely(n->cq[0])) {
@@ -5616,73 +5623,72 @@ static int nvme_start_ctrl(NvmeCtrl *n)
trace_pci_nvme_err_startfail_sq();
return -1;
}
- if (unlikely(!n->bar.asq)) {
+ if (unlikely(!asq)) {
trace_pci_nvme_err_startfail_nbarasq();
return -1;
}
- if (unlikely(!n->bar.acq)) {
+ if (unlikely(!acq)) {
trace_pci_nvme_err_startfail_nbaracq();
return -1;
}
- if (unlikely(n->bar.asq & (page_size - 1))) {
- trace_pci_nvme_err_startfail_asq_misaligned(n->bar.asq);
+ if (unlikely(asq & (page_size - 1))) {
+ trace_pci_nvme_err_startfail_asq_misaligned(asq);
return -1;
}
- if (unlikely(n->bar.acq & (page_size - 1))) {
- trace_pci_nvme_err_startfail_acq_misaligned(n->bar.acq);
+ if (unlikely(acq & (page_size - 1))) {
+ trace_pci_nvme_err_startfail_acq_misaligned(acq);
return -1;
}
- if (unlikely(!(NVME_CAP_CSS(n->bar.cap) & (1 << NVME_CC_CSS(n->bar.cc))))) {
- trace_pci_nvme_err_startfail_css(NVME_CC_CSS(n->bar.cc));
+ if (unlikely(!(NVME_CAP_CSS(cap) & (1 << NVME_CC_CSS(cc))))) {
+ trace_pci_nvme_err_startfail_css(NVME_CC_CSS(cc));
return -1;
}
- if (unlikely(NVME_CC_MPS(n->bar.cc) <
- NVME_CAP_MPSMIN(n->bar.cap))) {
+ if (unlikely(NVME_CC_MPS(cc) < NVME_CAP_MPSMIN(cap))) {
trace_pci_nvme_err_startfail_page_too_small(
- NVME_CC_MPS(n->bar.cc),
- NVME_CAP_MPSMIN(n->bar.cap));
+ NVME_CC_MPS(cc),
+ NVME_CAP_MPSMIN(cap));
return -1;
}
- if (unlikely(NVME_CC_MPS(n->bar.cc) >
- NVME_CAP_MPSMAX(n->bar.cap))) {
+ if (unlikely(NVME_CC_MPS(cc) >
+ NVME_CAP_MPSMAX(cap))) {
trace_pci_nvme_err_startfail_page_too_large(
- NVME_CC_MPS(n->bar.cc),
- NVME_CAP_MPSMAX(n->bar.cap));
+ NVME_CC_MPS(cc),
+ NVME_CAP_MPSMAX(cap));
return -1;
}
- if (unlikely(NVME_CC_IOCQES(n->bar.cc) <
+ if (unlikely(NVME_CC_IOCQES(cc) <
NVME_CTRL_CQES_MIN(n->id_ctrl.cqes))) {
trace_pci_nvme_err_startfail_cqent_too_small(
- NVME_CC_IOCQES(n->bar.cc),
- NVME_CTRL_CQES_MIN(n->bar.cap));
+ NVME_CC_IOCQES(cc),
+ NVME_CTRL_CQES_MIN(cap));
return -1;
}
- if (unlikely(NVME_CC_IOCQES(n->bar.cc) >
+ if (unlikely(NVME_CC_IOCQES(cc) >
NVME_CTRL_CQES_MAX(n->id_ctrl.cqes))) {
trace_pci_nvme_err_startfail_cqent_too_large(
- NVME_CC_IOCQES(n->bar.cc),
- NVME_CTRL_CQES_MAX(n->bar.cap));
+ NVME_CC_IOCQES(cc),
+ NVME_CTRL_CQES_MAX(cap));
return -1;
}
- if (unlikely(NVME_CC_IOSQES(n->bar.cc) <
+ if (unlikely(NVME_CC_IOSQES(cc) <
NVME_CTRL_SQES_MIN(n->id_ctrl.sqes))) {
trace_pci_nvme_err_startfail_sqent_too_small(
- NVME_CC_IOSQES(n->bar.cc),
- NVME_CTRL_SQES_MIN(n->bar.cap));
+ NVME_CC_IOSQES(cc),
+ NVME_CTRL_SQES_MIN(cap));
return -1;
}
- if (unlikely(NVME_CC_IOSQES(n->bar.cc) >
+ if (unlikely(NVME_CC_IOSQES(cc) >
NVME_CTRL_SQES_MAX(n->id_ctrl.sqes))) {
trace_pci_nvme_err_startfail_sqent_too_large(
- NVME_CC_IOSQES(n->bar.cc),
- NVME_CTRL_SQES_MAX(n->bar.cap));
+ NVME_CC_IOSQES(cc),
+ NVME_CTRL_SQES_MAX(cap));
return -1;
}
- if (unlikely(!NVME_AQA_ASQS(n->bar.aqa))) {
+ if (unlikely(!NVME_AQA_ASQS(aqa))) {
trace_pci_nvme_err_startfail_asqent_sz_zero();
return -1;
}
- if (unlikely(!NVME_AQA_ACQS(n->bar.aqa))) {
+ if (unlikely(!NVME_AQA_ACQS(aqa))) {
trace_pci_nvme_err_startfail_acqent_sz_zero();
return -1;
}
@@ -5690,12 +5696,10 @@ static int nvme_start_ctrl(NvmeCtrl *n)
n->page_bits = page_bits;
n->page_size = page_size;
n->max_prp_ents = n->page_size / sizeof(uint64_t);
- n->cqe_size = 1 << NVME_CC_IOCQES(n->bar.cc);
- n->sqe_size = 1 << NVME_CC_IOSQES(n->bar.cc);
- nvme_init_cq(&n->admin_cq, n, n->bar.acq, 0, 0,
- NVME_AQA_ACQS(n->bar.aqa) + 1, 1);
- nvme_init_sq(&n->admin_sq, n, n->bar.asq, 0, 0,
- NVME_AQA_ASQS(n->bar.aqa) + 1);
+ n->cqe_size = 1 << NVME_CC_IOCQES(cc);
+ n->sqe_size = 1 << NVME_CC_IOSQES(cc);
+ nvme_init_cq(&n->admin_cq, n, acq, 0, 0, NVME_AQA_ACQS(aqa) + 1, 1);
+ nvme_init_sq(&n->admin_sq, n, asq, 0, 0, NVME_AQA_ASQS(aqa) + 1);
nvme_set_timestamp(n, 0ULL);
@@ -5708,22 +5712,38 @@ static int nvme_start_ctrl(NvmeCtrl *n)
static void nvme_cmb_enable_regs(NvmeCtrl *n)
{
- NVME_CMBLOC_SET_CDPCILS(n->bar.cmbloc, 1);
- NVME_CMBLOC_SET_CDPMLS(n->bar.cmbloc, 1);
- NVME_CMBLOC_SET_BIR(n->bar.cmbloc, NVME_CMB_BIR);
+ uint32_t cmbloc = 0, cmbsz = 0;
- NVME_CMBSZ_SET_SQS(n->bar.cmbsz, 1);
- NVME_CMBSZ_SET_CQS(n->bar.cmbsz, 0);
- NVME_CMBSZ_SET_LISTS(n->bar.cmbsz, 1);
- NVME_CMBSZ_SET_RDS(n->bar.cmbsz, 1);
- NVME_CMBSZ_SET_WDS(n->bar.cmbsz, 1);
- NVME_CMBSZ_SET_SZU(n->bar.cmbsz, 2); /* MBs */
- NVME_CMBSZ_SET_SZ(n->bar.cmbsz, n->params.cmb_size_mb);
+ NVME_CMBLOC_SET_CDPCILS(cmbloc, 1);
+ NVME_CMBLOC_SET_CDPMLS(cmbloc, 1);
+ NVME_CMBLOC_SET_BIR(cmbloc, NVME_CMB_BIR);
+ n->bar.cmbloc = cpu_to_le32(cmbloc);
+
+ NVME_CMBSZ_SET_SQS(cmbsz, 1);
+ NVME_CMBSZ_SET_CQS(cmbsz, 0);
+ NVME_CMBSZ_SET_LISTS(cmbsz, 1);
+ NVME_CMBSZ_SET_RDS(cmbsz, 1);
+ NVME_CMBSZ_SET_WDS(cmbsz, 1);
+ NVME_CMBSZ_SET_SZU(cmbsz, 2); /* MBs */
+ NVME_CMBSZ_SET_SZ(cmbsz, n->params.cmb_size_mb);
+ n->bar.cmbsz = cpu_to_le32(cmbsz);
}
static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
unsigned size)
{
+ uint64_t cap = le64_to_cpu(n->bar.cap);
+ uint32_t cc = le32_to_cpu(n->bar.cc);
+ uint32_t intms = le32_to_cpu(n->bar.intms);
+ uint32_t csts = le32_to_cpu(n->bar.csts);
+ uint64_t asq = le64_to_cpu(n->bar.asq);
+ uint64_t acq = le64_to_cpu(n->bar.acq);
+ uint64_t cmbmsc = le64_to_cpu(n->bar.cmbmsc);
+ uint32_t cmbsts = le32_to_cpu(n->bar.cmbsts);
+ uint32_t pmrsts = le32_to_cpu(n->bar.pmrsts);
+ uint32_t pmrmscl = le32_to_cpu(n->bar.pmrmscl);
+ uint32_t pmrmscu = le32_to_cpu(n->bar.pmrmscu);
+
if (unlikely(offset & (sizeof(uint32_t) - 1))) {
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_misaligned32,
"MMIO write not 32-bit aligned,"
@@ -5747,9 +5767,9 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
" when MSI-X is enabled");
/* should be ignored, fall through for now */
}
- n->bar.intms |= data & 0xffffffff;
- n->bar.intmc = n->bar.intms;
- trace_pci_nvme_mmio_intm_set(data & 0xffffffff, n->bar.intmc);
+ intms |= data & 0xffffffff;
+ n->bar.intmc = n->bar.intms = cpu_to_le32(intms);
+ trace_pci_nvme_mmio_intm_set(data & 0xffffffff, intms);
nvme_irq_check(n);
break;
case NVME_REG_INTMC:
@@ -5759,44 +5779,54 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
" when MSI-X is enabled");
/* should be ignored, fall through for now */
}
- n->bar.intms &= ~(data & 0xffffffff);
- n->bar.intmc = n->bar.intms;
- trace_pci_nvme_mmio_intm_clr(data & 0xffffffff, n->bar.intmc);
+ intms &= ~(data & 0xffffffff);
+ n->bar.intmc = n->bar.intms = cpu_to_le32(intms);
+ trace_pci_nvme_mmio_intm_clr(data & 0xffffffff, intms);
nvme_irq_check(n);
break;
case NVME_REG_CC:
trace_pci_nvme_mmio_cfg(data & 0xffffffff);
+
/* Windows first sends data, then sends enable bit */
- if (!NVME_CC_EN(data) && !NVME_CC_EN(n->bar.cc) &&
- !NVME_CC_SHN(data) && !NVME_CC_SHN(n->bar.cc))
+ if (!NVME_CC_EN(data) && !NVME_CC_EN(cc) &&
+ !NVME_CC_SHN(data) && !NVME_CC_SHN(cc))
{
- n->bar.cc = data;
+ cc = data;
}
- if (NVME_CC_EN(data) && !NVME_CC_EN(n->bar.cc)) {
- n->bar.cc = data;
+ if (NVME_CC_EN(data) && !NVME_CC_EN(cc)) {
+ cc = data;
+
+ /* flush CC since nvme_start_ctrl() needs the value */
+ n->bar.cc = cpu_to_le32(cc);
if (unlikely(nvme_start_ctrl(n))) {
trace_pci_nvme_err_startfail();
- n->bar.csts = NVME_CSTS_FAILED;
+ csts = NVME_CSTS_FAILED;
} else {
trace_pci_nvme_mmio_start_success();
- n->bar.csts = NVME_CSTS_READY;
+ csts = NVME_CSTS_READY;
}
- } else if (!NVME_CC_EN(data) && NVME_CC_EN(n->bar.cc)) {
+ } else if (!NVME_CC_EN(data) && NVME_CC_EN(cc)) {
trace_pci_nvme_mmio_stopped();
nvme_ctrl_reset(n);
- n->bar.csts &= ~NVME_CSTS_READY;
+ cc = 0;
+ csts &= ~NVME_CSTS_READY;
}
- if (NVME_CC_SHN(data) && !(NVME_CC_SHN(n->bar.cc))) {
+
+ if (NVME_CC_SHN(data) && !(NVME_CC_SHN(cc))) {
trace_pci_nvme_mmio_shutdown_set();
nvme_ctrl_shutdown(n);
- n->bar.cc = data;
- n->bar.csts |= NVME_CSTS_SHST_COMPLETE;
- } else if (!NVME_CC_SHN(data) && NVME_CC_SHN(n->bar.cc)) {
+ cc = data;
+ csts |= NVME_CSTS_SHST_COMPLETE;
+ } else if (!NVME_CC_SHN(data) && NVME_CC_SHN(cc)) {
trace_pci_nvme_mmio_shutdown_cleared();
- n->bar.csts &= ~NVME_CSTS_SHST_COMPLETE;
- n->bar.cc = data;
+ csts &= ~NVME_CSTS_SHST_COMPLETE;
+ cc = data;
}
+
+ n->bar.cc = cpu_to_le32(cc);
+ n->bar.csts = cpu_to_le32(csts);
+
break;
case NVME_REG_CSTS:
if (data & (1 << 4)) {
@@ -5818,26 +5848,30 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
}
break;
case NVME_REG_AQA:
- n->bar.aqa = data & 0xffffffff;
- trace_pci_nvme_mmio_aqattr(data & 0xffffffff);
+ n->bar.aqa = cpu_to_le32(data & 0xffffffff);
+ trace_pci_nvme_mmio_aqattr(cpu_to_le32(data & 0xffffffff));
break;
case NVME_REG_ASQ:
- n->bar.asq = size == 8 ? data :
- (n->bar.asq & ~0xffffffffULL) | (data & 0xffffffff);
+ asq = size == 8 ? data :
+ (asq & ~0xffffffffULL) | (data & 0xffffffff);
+ n->bar.asq = cpu_to_le64(asq);
trace_pci_nvme_mmio_asqaddr(data);
break;
case NVME_REG_ASQ + 4:
- n->bar.asq = (n->bar.asq & 0xffffffff) | (data << 32);
- trace_pci_nvme_mmio_asqaddr_hi(data, n->bar.asq);
+ asq = (asq & 0xffffffff) | (data << 32);
+ n->bar.asq = cpu_to_le64(asq);
+ trace_pci_nvme_mmio_asqaddr_hi(data, asq);
break;
case NVME_REG_ACQ:
trace_pci_nvme_mmio_acqaddr(data);
- n->bar.acq = size == 8 ? data :
- (n->bar.acq & ~0xffffffffULL) | (data & 0xffffffff);
+ acq = size == 8 ? data :
+ (acq & ~0xffffffffULL) | (data & 0xffffffff);
+ n->bar.acq = cpu_to_le64(acq);
break;
case NVME_REG_ACQ + 4:
- n->bar.acq = (n->bar.acq & 0xffffffff) | (data << 32);
- trace_pci_nvme_mmio_acqaddr_hi(data, n->bar.acq);
+ acq = (acq & 0xffffffff) | (data << 32);
+ n->bar.acq = cpu_to_le64(acq);
+ trace_pci_nvme_mmio_acqaddr_hi(data, acq);
break;
case NVME_REG_CMBLOC:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_cmbloc_reserved,
@@ -5849,12 +5883,13 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
"invalid write to read only CMBSZ, ignored");
return;
case NVME_REG_CMBMSC:
- if (!NVME_CAP_CMBS(n->bar.cap)) {
+ if (!NVME_CAP_CMBS(cap)) {
return;
}
- n->bar.cmbmsc = size == 8 ? data :
- (n->bar.cmbmsc & ~0xffffffff) | (data & 0xffffffff);
+ cmbmsc = size == 8 ? data :
+ (cmbmsc & ~0xffffffff) | (data & 0xffffffff);
+ n->bar.cmbmsc = cpu_to_le64(cmbmsc);
n->cmb.cmse = false;
if (NVME_CMBMSC_CRE(data)) {
@@ -5863,7 +5898,8 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
if (NVME_CMBMSC_CMSE(data)) {
hwaddr cba = NVME_CMBMSC_CBA(data) << CMBMSC_CBA_SHIFT;
if (cba + int128_get64(n->cmb.mem.size) < cba) {
- NVME_CMBSTS_SET_CBAI(n->bar.cmbsts, 1);
+ NVME_CMBSTS_SET_CBAI(cmbsts, 1);
+ n->bar.cmbsts = cpu_to_le32(cmbsts);
return;
}
@@ -5877,7 +5913,8 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
return;
case NVME_REG_CMBMSC + 4:
- n->bar.cmbmsc = (n->bar.cmbmsc & 0xffffffff) | (data << 32);
+ cmbmsc = (cmbmsc & 0xffffffff) | (data << 32);
+ n->bar.cmbmsc = cpu_to_le64(cmbmsc);
return;
case NVME_REG_PMRCAP:
@@ -5885,19 +5922,20 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
"invalid write to PMRCAP register, ignored");
return;
case NVME_REG_PMRCTL:
- if (!NVME_CAP_PMRS(n->bar.cap)) {
+ if (!NVME_CAP_PMRS(cap)) {
return;
}
- n->bar.pmrctl = data;
+ n->bar.pmrctl = cpu_to_le32(data);
if (NVME_PMRCTL_EN(data)) {
memory_region_set_enabled(&n->pmr.dev->mr, true);
- n->bar.pmrsts = 0;
+ pmrsts = 0;
} else {
memory_region_set_enabled(&n->pmr.dev->mr, false);
- NVME_PMRSTS_SET_NRDY(n->bar.pmrsts, 1);
+ NVME_PMRSTS_SET_NRDY(pmrsts, 1);
n->pmr.cmse = false;
}
+ n->bar.pmrsts = cpu_to_le32(pmrsts);
return;
case NVME_REG_PMRSTS:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrsts_readonly,
@@ -5912,18 +5950,20 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
"invalid write to PMRSWTP register, ignored");
return;
case NVME_REG_PMRMSCL:
- if (!NVME_CAP_PMRS(n->bar.cap)) {
+ if (!NVME_CAP_PMRS(cap)) {
return;
}
- n->bar.pmrmscl = data & 0xffffffff;
+ pmrmscl = data & 0xffffffff;
+ n->bar.pmrmscl = cpu_to_le32(pmrmscl);
n->pmr.cmse = false;
- if (NVME_PMRMSCL_CMSE(n->bar.pmrmscl)) {
- hwaddr cba = n->bar.pmrmscu |
- (NVME_PMRMSCL_CBA(n->bar.pmrmscl) << PMRMSCL_CBA_SHIFT);
+ if (NVME_PMRMSCL_CMSE(data)) {
+ hwaddr cba = pmrmscu |
+ (NVME_PMRMSCL_CBA(pmrmscl) << PMRMSCL_CBA_SHIFT);
if (cba + int128_get64(n->pmr.dev->mr.size) < cba) {
- NVME_PMRSTS_SET_CBAI(n->bar.pmrsts, 1);
+ NVME_PMRSTS_SET_CBAI(pmrsts, 1);
+ n->bar.pmrsts = cpu_to_le32(pmrsts);
return;
}
@@ -5933,11 +5973,11 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
return;
case NVME_REG_PMRMSCU:
- if (!NVME_CAP_PMRS(n->bar.cap)) {
+ if (!NVME_CAP_PMRS(cap)) {
return;
}
- n->bar.pmrmscu = data & 0xffffffff;
+ n->bar.pmrmscu = cpu_to_le32(data & 0xffffffff);
return;
default:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_invalid,
@@ -5952,7 +5992,6 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
{
NvmeCtrl *n = (NvmeCtrl *)opaque;
uint8_t *ptr = (uint8_t *)&n->bar;
- uint64_t val = 0;
trace_pci_nvme_mmio_read(addr, size);
@@ -5982,13 +6021,11 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
* made it to persistent media
*/
if (addr == NVME_REG_PMRSTS &&
- (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
+ (NVME_PMRCAP_PMRWBM(le32_to_cpu(n->bar.pmrcap)) & 0x02)) {
memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
}
- memcpy(&val, ptr + addr, size);
-
- return val;
+ return ldn_le_p(ptr + addr, size);
}
static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
@@ -6246,6 +6283,7 @@ static void nvme_init_state(NvmeCtrl *n)
static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
{
uint64_t cmb_size = n->params.cmb_size_mb * MiB;
+ uint64_t cap = le64_to_cpu(n->bar.cap);
n->cmb.buf = g_malloc0(cmb_size);
memory_region_init_io(&n->cmb.mem, OBJECT(n), &nvme_cmb_ops, n,
@@ -6255,7 +6293,8 @@ static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
PCI_BASE_ADDRESS_MEM_TYPE_64 |
PCI_BASE_ADDRESS_MEM_PREFETCH, &n->cmb.mem);
- NVME_CAP_SET_CMBS(n->bar.cap, 1);
+ NVME_CAP_SET_CMBS(cap, 1);
+ n->bar.cap = cpu_to_le64(cap);
if (n->params.legacy_cmb) {
nvme_cmb_enable_regs(n);
@@ -6265,14 +6304,17 @@ static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
static void nvme_init_pmr(NvmeCtrl *n, PCIDevice *pci_dev)
{
- NVME_PMRCAP_SET_RDS(n->bar.pmrcap, 1);
- NVME_PMRCAP_SET_WDS(n->bar.pmrcap, 1);
- NVME_PMRCAP_SET_BIR(n->bar.pmrcap, NVME_PMR_BIR);
- /* Turn on bit 1 support */
- NVME_PMRCAP_SET_PMRWBM(n->bar.pmrcap, 0x02);
- NVME_PMRCAP_SET_CMSS(n->bar.pmrcap, 1);
+ uint32_t pmrcap = 0;
- pci_register_bar(pci_dev, NVME_PMRCAP_BIR(n->bar.pmrcap),
+ NVME_PMRCAP_SET_RDS(pmrcap, 1);
+ NVME_PMRCAP_SET_WDS(pmrcap, 1);
+ NVME_PMRCAP_SET_BIR(pmrcap, NVME_PMR_BIR);
+ /* Turn on bit 1 support */
+ NVME_PMRCAP_SET_PMRWBM(pmrcap, 0x02);
+ NVME_PMRCAP_SET_CMSS(pmrcap, 1);
+ n->bar.pmrcap = cpu_to_le32(pmrcap);
+
+ pci_register_bar(pci_dev, NVME_PMR_BIR,
PCI_BASE_ADDRESS_SPACE_MEMORY |
PCI_BASE_ADDRESS_MEM_TYPE_64 |
PCI_BASE_ADDRESS_MEM_PREFETCH, &n->pmr.dev->mr);
@@ -6362,6 +6404,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
{
NvmeIdCtrl *id = &n->id_ctrl;
uint8_t *pci_conf = pci_dev->config;
+ uint64_t cap = 0;
id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));
@@ -6440,17 +6483,18 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
id->cmic |= NVME_CMIC_MULTI_CTRL;
}
- NVME_CAP_SET_MQES(n->bar.cap, 0x7ff);
- NVME_CAP_SET_CQR(n->bar.cap, 1);
- NVME_CAP_SET_TO(n->bar.cap, 0xf);
- NVME_CAP_SET_CSS(n->bar.cap, NVME_CAP_CSS_NVM);
- NVME_CAP_SET_CSS(n->bar.cap, NVME_CAP_CSS_CSI_SUPP);
- NVME_CAP_SET_CSS(n->bar.cap, NVME_CAP_CSS_ADMIN_ONLY);
- NVME_CAP_SET_MPSMAX(n->bar.cap, 4);
- NVME_CAP_SET_CMBS(n->bar.cap, n->params.cmb_size_mb ? 1 : 0);
- NVME_CAP_SET_PMRS(n->bar.cap, n->pmr.dev ? 1 : 0);
+ NVME_CAP_SET_MQES(cap, 0x7ff);
+ NVME_CAP_SET_CQR(cap, 1);
+ NVME_CAP_SET_TO(cap, 0xf);
+ NVME_CAP_SET_CSS(cap, NVME_CAP_CSS_NVM);
+ NVME_CAP_SET_CSS(cap, NVME_CAP_CSS_CSI_SUPP);
+ NVME_CAP_SET_CSS(cap, NVME_CAP_CSS_ADMIN_ONLY);
+ NVME_CAP_SET_MPSMAX(cap, 4);
+ NVME_CAP_SET_CMBS(cap, n->params.cmb_size_mb ? 1 : 0);
+ NVME_CAP_SET_PMRS(cap, n->pmr.dev ? 1 : 0);
+ n->bar.cap = cpu_to_le64(cap);
- n->bar.vs = NVME_SPEC_VER;
+ n->bar.vs = cpu_to_le32(NVME_SPEC_VER);
n->bar.intmc = n->bar.intms = 0;
}
@@ -6601,7 +6645,7 @@ static void nvme_set_smart_warning(Object *obj, Visitor *v, const char *name,
cap = NVME_SMART_SPARE | NVME_SMART_TEMPERATURE | NVME_SMART_RELIABILITY
| NVME_SMART_MEDIA_READ_ONLY | NVME_SMART_FAILED_VOLATILE_MEDIA;
- if (NVME_CAP_PMRS(n->bar.cap)) {
+ if (NVME_CAP_PMRS(le64_to_cpu(n->bar.cap))) {
cap |= NVME_SMART_PMR_UNRELIABLE;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 5/5] tests/qtest/nvme-test: add mmio read test
2021-07-13 19:24 [PATCH v2 0/5] hw/nvme: fix mmio read Klaus Jensen
` (3 preceding siblings ...)
2021-07-13 19:24 ` [PATCH v2 4/5] hw/nvme: fix mmio read Klaus Jensen
@ 2021-07-13 19:24 ` Klaus Jensen
4 siblings, 0 replies; 11+ messages in thread
From: Klaus Jensen @ 2021-07-13 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Thomas Huth, qemu-block, Peter Maydell,
Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, Max Reitz,
Keith Busch, Stefan Hajnoczi, Klaus Jensen, Paolo Bonzini
From: Klaus Jensen <k.jensen@samsung.com>
Add a regression test for mmio read on big-endian hosts.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
tests/qtest/nvme-test.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/tests/qtest/nvme-test.c b/tests/qtest/nvme-test.c
index 47e757d7e2af..f8bafb5d70fb 100644
--- a/tests/qtest/nvme-test.c
+++ b/tests/qtest/nvme-test.c
@@ -67,6 +67,30 @@ static void nvmetest_oob_cmb_test(void *obj, void *data, QGuestAllocator *alloc)
g_assert_cmpint(qpci_io_readl(pdev, bar, cmb_bar_size - 1), !=, 0x44332211);
}
+static void nvmetest_reg_read_test(void *obj, void *data, QGuestAllocator *alloc)
+{
+ QNvme *nvme = obj;
+ QPCIDevice *pdev = &nvme->dev;
+ QPCIBar bar;
+ uint32_t cap_lo, cap_hi;
+ uint64_t cap;
+
+ qpci_device_enable(pdev);
+ bar = qpci_iomap(pdev, 0, NULL);
+
+ cap_lo = qpci_io_readl(pdev, bar, 0x0);
+ g_assert_cmpint(NVME_CAP_MQES(cap_lo), ==, 0x7ff);
+
+ cap_hi = qpci_io_readl(pdev, bar, 0x4);
+ g_assert_cmpint(NVME_CAP_MPSMAX((uint64_t)cap_hi << 32), ==, 0x4);
+
+ cap = qpci_io_readq(pdev, bar, 0x0);
+ g_assert_cmpint(NVME_CAP_MQES(cap), ==, 0x7ff);
+ g_assert_cmpint(NVME_CAP_MPSMAX(cap), ==, 0x4);
+
+ qpci_iounmap(pdev, bar);
+}
+
static void nvmetest_pmr_reg_test(void *obj, void *data, QGuestAllocator *alloc)
{
QNvme *nvme = obj;
@@ -142,6 +166,8 @@ static void nvme_register_nodes(void)
&(QOSGraphTestOptions) {
.edge.extra_device_opts = "pmrdev=pmr0"
});
+
+ qos_add_test("reg-read", "nvme", nvmetest_reg_read_test, NULL);
}
libqos_init(nvme_register_nodes);
--
2.32.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/5] hw/nvme: use symbolic names for registers
2021-07-13 19:24 ` [PATCH v2 2/5] hw/nvme: use symbolic names for registers Klaus Jensen
@ 2021-07-13 22:12 ` Philippe Mathieu-Daudé
2021-07-13 22:21 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-13 22:12 UTC (permalink / raw)
To: Klaus Jensen, qemu-devel
Cc: Kevin Wolf, Fam Zheng, Thomas Huth, qemu-block, Peter Maydell,
Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, Max Reitz,
Stefan Hajnoczi, Keith Busch, Paolo Bonzini
On 7/13/21 9:24 PM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Add the NvmeBarRegs enum and use these instead of explicit register
> offsets.
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
> include/block/nvme.h | 27 +++++++++++++++++++++++++++
> hw/nvme/ctrl.c | 44 ++++++++++++++++++++++----------------------
> 2 files changed, 49 insertions(+), 22 deletions(-)
>
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 84053b68b987..082d4bddbf9f 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -31,6 +31,33 @@ typedef struct QEMU_PACKED NvmeBar {
> uint8_t css[484];
> } NvmeBar;
>
> +enum NvmeBarRegs {
> + NVME_REG_CAP = 0x0,
> + NVME_REG_VS = 0x8,
> + NVME_REG_INTMS = 0xc,
> + NVME_REG_INTMC = 0x10,
> + NVME_REG_CC = 0x14,
> + NVME_REG_CSTS = 0x1c,
> + NVME_REG_NSSR = 0x20,
> + NVME_REG_AQA = 0x24,
> + NVME_REG_ASQ = 0x28,
> + NVME_REG_ACQ = 0x30,
> + NVME_REG_CMBLOC = 0x38,
> + NVME_REG_CMBSZ = 0x3c,
> + NVME_REG_BPINFO = 0x40,
> + NVME_REG_BPRSEL = 0x44,
> + NVME_REG_BPMBL = 0x48,
> + NVME_REG_CMBMSC = 0x50,
> + NVME_REG_CMBSTS = 0x58,
> + NVME_REG_PMRCAP = 0xe00,
> + NVME_REG_PMRCTL = 0xe04,
> + NVME_REG_PMRSTS = 0xe08,
> + NVME_REG_PMREBS = 0xe0c,
> + NVME_REG_PMRSWTP = 0xe10,
> + NVME_REG_PMRMSCL = 0xe14,
> + NVME_REG_PMRMSCU = 0xe18,
> +};
> +
> enum NvmeCapShift {
> CAP_MQES_SHIFT = 0,
> CAP_CQR_SHIFT = 16,
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 28299c6f3764..8c305315f41c 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -5740,7 +5740,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
> }
>
> switch (offset) {
> - case 0xc: /* INTMS */
> + case NVME_REG_INTMS:
What about using offsetof(NvmeBar, intms) instead?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/5] hw/nvme: fix mmio read
2021-07-13 19:24 ` [PATCH v2 4/5] hw/nvme: fix mmio read Klaus Jensen
@ 2021-07-13 22:18 ` Philippe Mathieu-Daudé
2021-07-14 5:32 ` Klaus Jensen
0 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-13 22:18 UTC (permalink / raw)
To: Klaus Jensen, qemu-devel
Cc: Kevin Wolf, Fam Zheng, Thomas Huth, qemu-block, Peter Maydell,
Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, Max Reitz,
Stefan Hajnoczi, Keith Busch, Paolo Bonzini
On 7/13/21 9:24 PM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> The new PMR test unearthed a long-standing issue with MMIO reads on
> big-endian hosts.
>
> Fix this by unconditionally storing all controller registers in little
> endian.
>
> Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
> hw/nvme/ctrl.c | 304 ++++++++++++++++++++++++++++---------------------
> 1 file changed, 174 insertions(+), 130 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 0449cc4dee9b..ddac9344a74e 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -439,10 +439,12 @@ static uint8_t nvme_sq_empty(NvmeSQueue *sq)
>
> static void nvme_irq_check(NvmeCtrl *n)
> {
> + uint32_t intms = le32_to_cpu(n->bar.intms);
> +
> if (msix_enabled(&(n->parent_obj))) {
> return;
> }
> - if (~n->bar.intms & n->irq_status) {
> + if (~intms & n->irq_status) {
> pci_irq_assert(&n->parent_obj);
> } else {
> pci_irq_deassert(&n->parent_obj);
> @@ -1289,7 +1291,7 @@ static void nvme_post_cqes(void *opaque)
> if (ret) {
> trace_pci_nvme_err_addr_write(addr);
> trace_pci_nvme_err_cfs();
> - n->bar.csts = NVME_CSTS_FAILED;
> + n->bar.csts = cpu_to_le64(NVME_CSTS_FAILED);
The load/store API is safer than the cpu_to_X() one because
it removes alignment problems. Here it becomes:
stq_le_p(&n->bar.csts, NVME_CSTS_FAILED);
> break;
> }
> QTAILQ_REMOVE(&cq->req_list, req, entry);
> @@ -4022,7 +4024,7 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeRequest *req)
> trace_pci_nvme_err_invalid_create_sq_sqid(sqid);
> return NVME_INVALID_QID | NVME_DNR;
> }
> - if (unlikely(!qsize || qsize > NVME_CAP_MQES(n->bar.cap))) {
> + if (unlikely(!qsize || qsize > NVME_CAP_MQES(le64_to_cpu(n->bar.cap)))) {
And here:
if (unlikely(!qsize || qsize > NVME_CAP_MQES(ldq_le_p(&n->bar.cap)))) {
> trace_pci_nvme_err_invalid_create_sq_size(qsize);
> return NVME_MAX_QSIZE_EXCEEDED | NVME_DNR;
> }
However for the BAR is likely aligned, so using the cpu_to() API
shouldn't be a problem until we try to deprecate/remove it.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/5] hw/nvme: use symbolic names for registers
2021-07-13 22:12 ` Philippe Mathieu-Daudé
@ 2021-07-13 22:21 ` Philippe Mathieu-Daudé
2021-07-14 5:32 ` Klaus Jensen
0 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-13 22:21 UTC (permalink / raw)
To: Klaus Jensen, qemu-devel
Cc: Kevin Wolf, Fam Zheng, Thomas Huth, qemu-block, Peter Maydell,
Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, Max Reitz,
Stefan Hajnoczi, Keith Busch, Paolo Bonzini
On 7/14/21 12:12 AM, Philippe Mathieu-Daudé wrote:
> On 7/13/21 9:24 PM, Klaus Jensen wrote:
>> From: Klaus Jensen <k.jensen@samsung.com>
>>
>> Add the NvmeBarRegs enum and use these instead of explicit register
>> offsets.
>>
>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> ---
>> include/block/nvme.h | 27 +++++++++++++++++++++++++++
>> hw/nvme/ctrl.c | 44 ++++++++++++++++++++++----------------------
>> 2 files changed, 49 insertions(+), 22 deletions(-)
>>
>> diff --git a/include/block/nvme.h b/include/block/nvme.h
>> index 84053b68b987..082d4bddbf9f 100644
>> --- a/include/block/nvme.h
>> +++ b/include/block/nvme.h
>> @@ -31,6 +31,33 @@ typedef struct QEMU_PACKED NvmeBar {
>> uint8_t css[484];
>> } NvmeBar;
>>
>> +enum NvmeBarRegs {
>> + NVME_REG_CAP = 0x0,
>> + NVME_REG_VS = 0x8,
>> + NVME_REG_INTMS = 0xc,
>> + NVME_REG_INTMC = 0x10,
>> + NVME_REG_CC = 0x14,
>> + NVME_REG_CSTS = 0x1c,
>> + NVME_REG_NSSR = 0x20,
>> + NVME_REG_AQA = 0x24,
>> + NVME_REG_ASQ = 0x28,
>> + NVME_REG_ACQ = 0x30,
>> + NVME_REG_CMBLOC = 0x38,
>> + NVME_REG_CMBSZ = 0x3c,
>> + NVME_REG_BPINFO = 0x40,
>> + NVME_REG_BPRSEL = 0x44,
>> + NVME_REG_BPMBL = 0x48,
>> + NVME_REG_CMBMSC = 0x50,
>> + NVME_REG_CMBSTS = 0x58,
>> + NVME_REG_PMRCAP = 0xe00,
>> + NVME_REG_PMRCTL = 0xe04,
>> + NVME_REG_PMRSTS = 0xe08,
>> + NVME_REG_PMREBS = 0xe0c,
>> + NVME_REG_PMRSWTP = 0xe10,
>> + NVME_REG_PMRMSCL = 0xe14,
>> + NVME_REG_PMRMSCU = 0xe18,
>> +};
>> +
>> enum NvmeCapShift {
>> CAP_MQES_SHIFT = 0,
>> CAP_CQR_SHIFT = 16,
>> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>> index 28299c6f3764..8c305315f41c 100644
>> --- a/hw/nvme/ctrl.c
>> +++ b/hw/nvme/ctrl.c
>> @@ -5740,7 +5740,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>> }
>>
>> switch (offset) {
>> - case 0xc: /* INTMS */
>> + case NVME_REG_INTMS:
>
> What about using offsetof(NvmeBar, intms) instead?
BTW I'm not suggesting this is better, I just wonder how we can avoid
to duplicate the definitions. Alternative is declaring:
enum NvmeBarRegs {
NVME_REG_CAP = offsetof(NvmeBar, cap),
NVME_REG_VS = offsetof(NvmeBar, vs),
...
Or keeping your patch.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/5] hw/nvme: use symbolic names for registers
2021-07-13 22:21 ` Philippe Mathieu-Daudé
@ 2021-07-14 5:32 ` Klaus Jensen
0 siblings, 0 replies; 11+ messages in thread
From: Klaus Jensen @ 2021-07-14 5:32 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, Fam Zheng, Thomas Huth, qemu-block, Peter Maydell,
Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, qemu-devel,
Max Reitz, Stefan Hajnoczi, Keith Busch, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]
On Jul 14 00:21, Philippe Mathieu-Daudé wrote:
> On 7/14/21 12:12 AM, Philippe Mathieu-Daudé wrote:
> > On 7/13/21 9:24 PM, Klaus Jensen wrote:
> >> From: Klaus Jensen <k.jensen@samsung.com>
> >>
> >> Add the NvmeBarRegs enum and use these instead of explicit register
> >> offsets.
> >>
> >> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> >> ---
> >> include/block/nvme.h | 27 +++++++++++++++++++++++++++
> >> hw/nvme/ctrl.c | 44 ++++++++++++++++++++++----------------------
> >> 2 files changed, 49 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/include/block/nvme.h b/include/block/nvme.h
> >> index 84053b68b987..082d4bddbf9f 100644
> >> --- a/include/block/nvme.h
> >> +++ b/include/block/nvme.h
> >> @@ -31,6 +31,33 @@ typedef struct QEMU_PACKED NvmeBar {
> >> uint8_t css[484];
> >> } NvmeBar;
> >>
> >> +enum NvmeBarRegs {
> >> + NVME_REG_CAP = 0x0,
> >> + NVME_REG_VS = 0x8,
> >> + NVME_REG_INTMS = 0xc,
> >> + NVME_REG_INTMC = 0x10,
> >> + NVME_REG_CC = 0x14,
> >> + NVME_REG_CSTS = 0x1c,
> >> + NVME_REG_NSSR = 0x20,
> >> + NVME_REG_AQA = 0x24,
> >> + NVME_REG_ASQ = 0x28,
> >> + NVME_REG_ACQ = 0x30,
> >> + NVME_REG_CMBLOC = 0x38,
> >> + NVME_REG_CMBSZ = 0x3c,
> >> + NVME_REG_BPINFO = 0x40,
> >> + NVME_REG_BPRSEL = 0x44,
> >> + NVME_REG_BPMBL = 0x48,
> >> + NVME_REG_CMBMSC = 0x50,
> >> + NVME_REG_CMBSTS = 0x58,
> >> + NVME_REG_PMRCAP = 0xe00,
> >> + NVME_REG_PMRCTL = 0xe04,
> >> + NVME_REG_PMRSTS = 0xe08,
> >> + NVME_REG_PMREBS = 0xe0c,
> >> + NVME_REG_PMRSWTP = 0xe10,
> >> + NVME_REG_PMRMSCL = 0xe14,
> >> + NVME_REG_PMRMSCU = 0xe18,
> >> +};
> >> +
> >> enum NvmeCapShift {
> >> CAP_MQES_SHIFT = 0,
> >> CAP_CQR_SHIFT = 16,
> >> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> >> index 28299c6f3764..8c305315f41c 100644
> >> --- a/hw/nvme/ctrl.c
> >> +++ b/hw/nvme/ctrl.c
> >> @@ -5740,7 +5740,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
> >> }
> >>
> >> switch (offset) {
> >> - case 0xc: /* INTMS */
> >> + case NVME_REG_INTMS:
> >
> > What about using offsetof(NvmeBar, intms) instead?
>
> BTW I'm not suggesting this is better, I just wonder how we can avoid
> to duplicate the definitions. Alternative is declaring:
>
> enum NvmeBarRegs {
> NVME_REG_CAP = offsetof(NvmeBar, cap),
> NVME_REG_VS = offsetof(NvmeBar, vs),
> ...
>
I like this suggestion!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/5] hw/nvme: fix mmio read
2021-07-13 22:18 ` Philippe Mathieu-Daudé
@ 2021-07-14 5:32 ` Klaus Jensen
0 siblings, 0 replies; 11+ messages in thread
From: Klaus Jensen @ 2021-07-14 5:32 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, Fam Zheng, Thomas Huth, qemu-block, Peter Maydell,
Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, qemu-devel,
Max Reitz, Stefan Hajnoczi, Keith Busch, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 2693 bytes --]
On Jul 14 00:18, Philippe Mathieu-Daudé wrote:
> On 7/13/21 9:24 PM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > The new PMR test unearthed a long-standing issue with MMIO reads on
> > big-endian hosts.
> >
> > Fix this by unconditionally storing all controller registers in little
> > endian.
> >
> > Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> > hw/nvme/ctrl.c | 304 ++++++++++++++++++++++++++++---------------------
> > 1 file changed, 174 insertions(+), 130 deletions(-)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 0449cc4dee9b..ddac9344a74e 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -439,10 +439,12 @@ static uint8_t nvme_sq_empty(NvmeSQueue *sq)
> >
> > static void nvme_irq_check(NvmeCtrl *n)
> > {
> > + uint32_t intms = le32_to_cpu(n->bar.intms);
> > +
> > if (msix_enabled(&(n->parent_obj))) {
> > return;
> > }
> > - if (~n->bar.intms & n->irq_status) {
> > + if (~intms & n->irq_status) {
> > pci_irq_assert(&n->parent_obj);
> > } else {
> > pci_irq_deassert(&n->parent_obj);
> > @@ -1289,7 +1291,7 @@ static void nvme_post_cqes(void *opaque)
> > if (ret) {
> > trace_pci_nvme_err_addr_write(addr);
> > trace_pci_nvme_err_cfs();
> > - n->bar.csts = NVME_CSTS_FAILED;
> > + n->bar.csts = cpu_to_le64(NVME_CSTS_FAILED);
>
> The load/store API is safer than the cpu_to_X() one because
> it removes alignment problems. Here it becomes:
>
> stq_le_p(&n->bar.csts, NVME_CSTS_FAILED);
>
> > break;
> > }
> > QTAILQ_REMOVE(&cq->req_list, req, entry);
> > @@ -4022,7 +4024,7 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeRequest *req)
> > trace_pci_nvme_err_invalid_create_sq_sqid(sqid);
> > return NVME_INVALID_QID | NVME_DNR;
> > }
> > - if (unlikely(!qsize || qsize > NVME_CAP_MQES(n->bar.cap))) {
> > + if (unlikely(!qsize || qsize > NVME_CAP_MQES(le64_to_cpu(n->bar.cap)))) {
>
> And here:
>
> if (unlikely(!qsize || qsize > NVME_CAP_MQES(ldq_le_p(&n->bar.cap)))) {
>
> > trace_pci_nvme_err_invalid_create_sq_size(qsize);
> > return NVME_MAX_QSIZE_EXCEEDED | NVME_DNR;
> > }
>
> However for the BAR is likely aligned, so using the cpu_to() API
> shouldn't be a problem until we try to deprecate/remove it.
>
>
Right. It makes sense to use that API for new code - I will amend it :)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-07-14 5:42 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-13 19:24 [PATCH v2 0/5] hw/nvme: fix mmio read Klaus Jensen
2021-07-13 19:24 ` [PATCH v2 1/5] hw/nvme: split pmrmsc register into upper and lower Klaus Jensen
2021-07-13 19:24 ` [PATCH v2 2/5] hw/nvme: use symbolic names for registers Klaus Jensen
2021-07-13 22:12 ` Philippe Mathieu-Daudé
2021-07-13 22:21 ` Philippe Mathieu-Daudé
2021-07-14 5:32 ` Klaus Jensen
2021-07-13 19:24 ` [PATCH v2 3/5] hw/nvme: fix out-of-bounds reads Klaus Jensen
2021-07-13 19:24 ` [PATCH v2 4/5] hw/nvme: fix mmio read Klaus Jensen
2021-07-13 22:18 ` Philippe Mathieu-Daudé
2021-07-14 5:32 ` Klaus Jensen
2021-07-13 19:24 ` [PATCH v2 5/5] tests/qtest/nvme-test: add mmio read test Klaus Jensen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.