* [PATCH v3 0/5] hw/nvme: fix mmio read
@ 2021-07-14 6:01 Klaus Jensen
2021-07-14 6:01 ` [PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower Klaus Jensen
` (5 more replies)
0 siblings, 6 replies; 17+ messages in thread
From: Klaus Jensen @ 2021-07-14 6:01 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Thomas Huth, Stefan Hajnoczi,
qemu-block, Laurent Vivier, Klaus Jensen, Gollu Appalanaidu,
Max Reitz, Keith Busch, Kevin Wolf, Klaus Jensen, Paolo Bonzini,
Philippe Mathieu-Daudé
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.
v2:
* "hw/nvme: use symbolic names for registers"
Use offsetof(NvmeBar, reg) instead of explicit offsets (Philippe)
* "hw/nvme: fix mmio read"
Use the st/ld API instead of cpu_to_X (Philippe)
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 | 60 +++++--
hw/nvme/ctrl.c | 362 +++++++++++++++++++++++-----------------
tests/qtest/nvme-test.c | 26 +++
3 files changed, 276 insertions(+), 172 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower
2021-07-14 6:01 [PATCH v3 0/5] hw/nvme: fix mmio read Klaus Jensen
@ 2021-07-14 6:01 ` Klaus Jensen
[not found] ` <CGME20210714113905epcas5p3d582216af16ab401f806757cad6bcc8d@epcas5p3.samsung.com>
2021-07-19 9:13 ` Peter Maydell
2021-07-14 6:01 ` [PATCH v3 2/5] hw/nvme: use symbolic names for registers Klaus Jensen
` (4 subsequent siblings)
5 siblings, 2 replies; 17+ messages in thread
From: Klaus Jensen @ 2021-07-14 6:01 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Thomas Huth, Stefan Hajnoczi,
qemu-block, Laurent Vivier, Klaus Jensen, Gollu Appalanaidu,
Max Reitz, Keith Busch, Kevin Wolf, Klaus Jensen, Paolo Bonzini,
Philippe Mathieu-Daudé
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] 17+ messages in thread
* [PATCH v3 2/5] hw/nvme: use symbolic names for registers
2021-07-14 6:01 [PATCH v3 0/5] hw/nvme: fix mmio read Klaus Jensen
2021-07-14 6:01 ` [PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower Klaus Jensen
@ 2021-07-14 6:01 ` Klaus Jensen
2021-07-14 9:08 ` Philippe Mathieu-Daudé
[not found] ` <CGME20210714114548epcas5p41a562395f6b695aabcfa4a531f2285d3@epcas5p4.samsung.com>
2021-07-14 6:01 ` [PATCH v3 3/5] hw/nvme: fix out-of-bounds reads Klaus Jensen
` (3 subsequent siblings)
5 siblings, 2 replies; 17+ messages in thread
From: Klaus Jensen @ 2021-07-14 6:01 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Thomas Huth, Stefan Hajnoczi,
qemu-block, Laurent Vivier, Klaus Jensen, Gollu Appalanaidu,
Max Reitz, Keith Busch, Kevin Wolf, Klaus Jensen, Paolo Bonzini,
Philippe Mathieu-Daudé
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 | 29 ++++++++++++++++++++++++++++-
hw/nvme/ctrl.c | 44 ++++++++++++++++++++++----------------------
2 files changed, 50 insertions(+), 23 deletions(-)
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 84053b68b987..77aae0117494 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -9,7 +9,7 @@ typedef struct QEMU_PACKED NvmeBar {
uint32_t cc;
uint8_t rsvd24[4];
uint32_t csts;
- uint32_t nssrc;
+ uint32_t nssr;
uint32_t aqa;
uint64_t asq;
uint64_t acq;
@@ -31,6 +31,33 @@ typedef struct QEMU_PACKED NvmeBar {
uint8_t css[484];
} NvmeBar;
+enum NvmeBarRegs {
+ NVME_REG_CAP = offsetof(NvmeBar, cap),
+ NVME_REG_VS = offsetof(NvmeBar, vs),
+ NVME_REG_INTMS = offsetof(NvmeBar, intms),
+ NVME_REG_INTMC = offsetof(NvmeBar, intmc),
+ NVME_REG_CC = offsetof(NvmeBar, cc),
+ NVME_REG_CSTS = offsetof(NvmeBar, csts),
+ NVME_REG_NSSR = offsetof(NvmeBar, nssr),
+ NVME_REG_AQA = offsetof(NvmeBar, aqa),
+ NVME_REG_ASQ = offsetof(NvmeBar, asq),
+ NVME_REG_ACQ = offsetof(NvmeBar, acq),
+ NVME_REG_CMBLOC = offsetof(NvmeBar, cmbloc),
+ NVME_REG_CMBSZ = offsetof(NvmeBar, cmbsz),
+ NVME_REG_BPINFO = offsetof(NvmeBar, bpinfo),
+ NVME_REG_BPRSEL = offsetof(NvmeBar, bprsel),
+ NVME_REG_BPMBL = offsetof(NvmeBar, bpmbl),
+ NVME_REG_CMBMSC = offsetof(NvmeBar, cmbmsc),
+ NVME_REG_CMBSTS = offsetof(NvmeBar, cmbsts),
+ NVME_REG_PMRCAP = offsetof(NvmeBar, pmrcap),
+ NVME_REG_PMRCTL = offsetof(NvmeBar, pmrctl),
+ NVME_REG_PMRSTS = offsetof(NvmeBar, pmrsts),
+ NVME_REG_PMREBS = offsetof(NvmeBar, pmrebs),
+ NVME_REG_PMRSWTP = offsetof(NvmeBar, pmrswtp),
+ NVME_REG_PMRMSCL = offsetof(NvmeBar, pmrmscl),
+ NVME_REG_PMRMSCU = offsetof(NvmeBar, pmrmscu),
+};
+
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] 17+ messages in thread
* [PATCH v3 3/5] hw/nvme: fix out-of-bounds reads
2021-07-14 6:01 [PATCH v3 0/5] hw/nvme: fix mmio read Klaus Jensen
2021-07-14 6:01 ` [PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower Klaus Jensen
2021-07-14 6:01 ` [PATCH v3 2/5] hw/nvme: use symbolic names for registers Klaus Jensen
@ 2021-07-14 6:01 ` Klaus Jensen
2021-07-19 8:50 ` Stefan Hajnoczi
2021-07-19 9:15 ` Peter Maydell
2021-07-14 6:01 ` [PATCH v3 4/5] hw/nvme: fix mmio read Klaus Jensen
` (2 subsequent siblings)
5 siblings, 2 replies; 17+ messages in thread
From: Klaus Jensen @ 2021-07-14 6:01 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Thomas Huth, Stefan Hajnoczi,
qemu-block, Laurent Vivier, Klaus Jensen, Gollu Appalanaidu,
Max Reitz, Keith Busch, Kevin Wolf, Klaus Jensen, Paolo Bonzini,
Philippe Mathieu-Daudé
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] 17+ messages in thread
* [PATCH v3 4/5] hw/nvme: fix mmio read
2021-07-14 6:01 [PATCH v3 0/5] hw/nvme: fix mmio read Klaus Jensen
` (2 preceding siblings ...)
2021-07-14 6:01 ` [PATCH v3 3/5] hw/nvme: fix out-of-bounds reads Klaus Jensen
@ 2021-07-14 6:01 ` Klaus Jensen
2021-07-19 9:52 ` Peter Maydell
2021-07-14 6:01 ` [PATCH v3 5/5] tests/qtest/nvme-test: add mmio read test Klaus Jensen
2021-07-19 6:43 ` [PATCH v3 0/5] hw/nvme: fix mmio read Klaus Jensen
5 siblings, 1 reply; 17+ messages in thread
From: Klaus Jensen @ 2021-07-14 6:01 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Thomas Huth, Stefan Hajnoczi,
qemu-block, Laurent Vivier, Klaus Jensen, Gollu Appalanaidu,
Max Reitz, Keith Busch, Kevin Wolf, Klaus Jensen, Paolo Bonzini,
Philippe Mathieu-Daudé
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 | 300 ++++++++++++++++++++++++++++---------------------
1 file changed, 173 insertions(+), 127 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 0449cc4dee9b..6d06ed990f2d 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 = ldl_le_p(&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;
+ stl_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(ldq_le_p(&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(ldl_le_p(&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(ldq_le_p(&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 = ldl_le_p(&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;
+ stl_le_p(&n->bar.csts, 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 = ldq_le_p(&n->bar.cap);
+ uint32_t cc = ldl_le_p(&n->bar.cc);
+ uint32_t aqa = ldl_le_p(&n->bar.aqa);
+ uint64_t asq = ldq_le_p(&n->bar.asq);
+ uint64_t acq = ldq_le_p(&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);
+ stl_le_p(&n->bar.cmbloc, 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);
+ stl_le_p(&n->bar.cmbsz, cmbsz);
}
static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
unsigned size)
{
+ uint64_t cap = ldq_le_p(&n->bar.cap);
+ uint32_t cc = ldl_le_p(&n->bar.cc);
+ uint32_t intms = ldl_le_p(&n->bar.intms);
+ uint32_t csts = ldl_le_p(&n->bar.csts);
+ uint64_t asq = ldq_le_p(&n->bar.asq);
+ uint64_t acq = ldq_le_p(&n->bar.acq);
+ uint64_t cmbmsc = ldq_le_p(&n->bar.cmbmsc);
+ uint32_t cmbsts = ldl_le_p(&n->bar.cmbsts);
+ uint32_t pmrsts = ldl_le_p(&n->bar.pmrsts);
+ uint32_t pmrmscl = ldl_le_p(&n->bar.pmrmscl);
+ uint32_t pmrmscu = ldl_le_p(&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,10 @@ 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;
+ intms |= data & 0xffffffff;
+ stl_le_p(&n->bar.intms, intms);
n->bar.intmc = n->bar.intms;
- trace_pci_nvme_mmio_intm_set(data & 0xffffffff, n->bar.intmc);
+ trace_pci_nvme_mmio_intm_set(data & 0xffffffff, intms);
nvme_irq_check(n);
break;
case NVME_REG_INTMC:
@@ -5759,44 +5780,55 @@ 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);
+ intms &= ~(data & 0xffffffff);
+ stl_le_p(&n->bar.intms, intms);
n->bar.intmc = n->bar.intms;
- trace_pci_nvme_mmio_intm_clr(data & 0xffffffff, n->bar.intmc);
+ 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 */
+ stl_le_p(&n->bar.cc, 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;
}
+
+ stl_le_p(&n->bar.cc, cc);
+ stl_le_p(&n->bar.csts, csts);
+
break;
case NVME_REG_CSTS:
if (data & (1 << 4)) {
@@ -5818,26 +5850,30 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
}
break;
case NVME_REG_AQA:
- n->bar.aqa = data & 0xffffffff;
+ stl_le_p(&n->bar.aqa, data & 0xffffffff);
trace_pci_nvme_mmio_aqattr(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);
+ stq_le_p(&n->bar.asq, 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);
+ stq_le_p(&n->bar.asq, 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);
+ stq_le_p(&n->bar.acq, 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);
+ stq_le_p(&n->bar.acq, 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 +5885,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);
+ stq_le_p(&n->bar.cmbmsc, cmbmsc);
n->cmb.cmse = false;
if (NVME_CMBMSC_CRE(data)) {
@@ -5863,7 +5900,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);
+ stl_le_p(&n->bar.cmbsts, cmbsts);
return;
}
@@ -5877,7 +5915,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);
+ stq_le_p(&n->bar.cmbmsc, cmbmsc);
return;
case NVME_REG_PMRCAP:
@@ -5885,19 +5924,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;
+ stl_le_p(&n->bar.pmrctl, data & 0xffffffff);
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;
}
+ stl_le_p(&n->bar.pmrsts, pmrsts);
return;
case NVME_REG_PMRSTS:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrsts_readonly,
@@ -5912,18 +5952,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;
+ stl_le_p(&n->bar.pmrmscl, 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);
+ stl_le_p(&n->bar.pmrsts, pmrsts);
return;
}
@@ -5933,11 +5975,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;
+ stl_le_p(&n->bar.pmrmscu, data & 0xffffffff);
return;
default:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_invalid,
@@ -5952,7 +5994,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 +6023,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(ldl_le_p(&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 +6285,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 = ldq_le_p(&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 +6295,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);
+ stq_le_p(&n->bar.cap, cap);
if (n->params.legacy_cmb) {
nvme_cmb_enable_regs(n);
@@ -6265,14 +6306,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);
+ stl_le_p(&n->bar.pmrcap, 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 +6406,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 +6485,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);
+ stq_le_p(&n->bar.cap, cap);
- n->bar.vs = NVME_SPEC_VER;
+ stl_le_p(&n->bar.vs, NVME_SPEC_VER);
n->bar.intmc = n->bar.intms = 0;
}
@@ -6601,7 +6647,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(ldq_le_p(&n->bar.cap))) {
cap |= NVME_SMART_PMR_UNRELIABLE;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 5/5] tests/qtest/nvme-test: add mmio read test
2021-07-14 6:01 [PATCH v3 0/5] hw/nvme: fix mmio read Klaus Jensen
` (3 preceding siblings ...)
2021-07-14 6:01 ` [PATCH v3 4/5] hw/nvme: fix mmio read Klaus Jensen
@ 2021-07-14 6:01 ` Klaus Jensen
[not found] ` <CGME20210714120156epcas5p212ae986c7e2ed4d30191ce8915304d2c@epcas5p2.samsung.com>
2021-07-19 6:43 ` [PATCH v3 0/5] hw/nvme: fix mmio read Klaus Jensen
5 siblings, 1 reply; 17+ messages in thread
From: Klaus Jensen @ 2021-07-14 6:01 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Thomas Huth, Stefan Hajnoczi,
qemu-block, Laurent Vivier, Klaus Jensen, Gollu Appalanaidu,
Max Reitz, Keith Busch, Kevin Wolf, Klaus Jensen, Paolo Bonzini,
Philippe Mathieu-Daudé
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] 17+ messages in thread
* Re: [PATCH v3 2/5] hw/nvme: use symbolic names for registers
2021-07-14 6:01 ` [PATCH v3 2/5] hw/nvme: use symbolic names for registers Klaus Jensen
@ 2021-07-14 9:08 ` Philippe Mathieu-Daudé
[not found] ` <CGME20210714114548epcas5p41a562395f6b695aabcfa4a531f2285d3@epcas5p4.samsung.com>
1 sibling, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-14 9:08 UTC (permalink / raw)
To: Klaus Jensen, qemu-devel
Cc: Fam Zheng, Peter Maydell, Thomas Huth, Stefan Hajnoczi,
qemu-block, Laurent Vivier, Klaus Jensen, Gollu Appalanaidu,
Max Reitz, Kevin Wolf, Keith Busch, Paolo Bonzini
On 7/14/21 8:01 AM, 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 | 29 ++++++++++++++++++++++++++++-
> hw/nvme/ctrl.c | 44 ++++++++++++++++++++++----------------------
> 2 files changed, 50 insertions(+), 23 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower
[not found] ` <CGME20210714113905epcas5p3d582216af16ab401f806757cad6bcc8d@epcas5p3.samsung.com>
@ 2021-07-14 11:35 ` Gollu Appalanaidu
0 siblings, 0 replies; 17+ messages in thread
From: Gollu Appalanaidu @ 2021-07-14 11:35 UTC (permalink / raw)
To: Klaus Jensen
Cc: Fam Zheng, Peter Maydell, Thomas Huth, qemu-block,
Laurent Vivier, Klaus Jensen, qemu-devel, Max Reitz,
Stefan Hajnoczi, Paolo Bonzini, Keith Busch, Kevin Wolf,
Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 3711 bytes --]
On Wed, Jul 14, 2021 at 08:01:21AM +0200, Klaus Jensen wrote:
>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
>
>
Changes LGTM.
Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/5] hw/nvme: use symbolic names for registers
[not found] ` <CGME20210714114548epcas5p41a562395f6b695aabcfa4a531f2285d3@epcas5p4.samsung.com>
@ 2021-07-14 11:42 ` Gollu Appalanaidu
0 siblings, 0 replies; 17+ messages in thread
From: Gollu Appalanaidu @ 2021-07-14 11:42 UTC (permalink / raw)
To: Klaus Jensen
Cc: Fam Zheng, Peter Maydell, Thomas Huth, qemu-block,
Laurent Vivier, Klaus Jensen, qemu-devel, Max Reitz,
Stefan Hajnoczi, Paolo Bonzini, Keith Busch, Kevin Wolf,
Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 8502 bytes --]
On Wed, Jul 14, 2021 at 08:01:22AM +0200, 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 | 29 ++++++++++++++++++++++++++++-
> hw/nvme/ctrl.c | 44 ++++++++++++++++++++++----------------------
> 2 files changed, 50 insertions(+), 23 deletions(-)
>
>diff --git a/include/block/nvme.h b/include/block/nvme.h
>index 84053b68b987..77aae0117494 100644
>--- a/include/block/nvme.h
>+++ b/include/block/nvme.h
>@@ -9,7 +9,7 @@ typedef struct QEMU_PACKED NvmeBar {
> uint32_t cc;
> uint8_t rsvd24[4];
> uint32_t csts;
>- uint32_t nssrc;
>+ uint32_t nssr;
> uint32_t aqa;
> uint64_t asq;
> uint64_t acq;
>@@ -31,6 +31,33 @@ typedef struct QEMU_PACKED NvmeBar {
> uint8_t css[484];
> } NvmeBar;
>
>+enum NvmeBarRegs {
>+ NVME_REG_CAP = offsetof(NvmeBar, cap),
>+ NVME_REG_VS = offsetof(NvmeBar, vs),
>+ NVME_REG_INTMS = offsetof(NvmeBar, intms),
>+ NVME_REG_INTMC = offsetof(NvmeBar, intmc),
>+ NVME_REG_CC = offsetof(NvmeBar, cc),
>+ NVME_REG_CSTS = offsetof(NvmeBar, csts),
>+ NVME_REG_NSSR = offsetof(NvmeBar, nssr),
>+ NVME_REG_AQA = offsetof(NvmeBar, aqa),
>+ NVME_REG_ASQ = offsetof(NvmeBar, asq),
>+ NVME_REG_ACQ = offsetof(NvmeBar, acq),
>+ NVME_REG_CMBLOC = offsetof(NvmeBar, cmbloc),
>+ NVME_REG_CMBSZ = offsetof(NvmeBar, cmbsz),
>+ NVME_REG_BPINFO = offsetof(NvmeBar, bpinfo),
>+ NVME_REG_BPRSEL = offsetof(NvmeBar, bprsel),
>+ NVME_REG_BPMBL = offsetof(NvmeBar, bpmbl),
>+ NVME_REG_CMBMSC = offsetof(NvmeBar, cmbmsc),
>+ NVME_REG_CMBSTS = offsetof(NvmeBar, cmbsts),
>+ NVME_REG_PMRCAP = offsetof(NvmeBar, pmrcap),
>+ NVME_REG_PMRCTL = offsetof(NvmeBar, pmrctl),
>+ NVME_REG_PMRSTS = offsetof(NvmeBar, pmrsts),
>+ NVME_REG_PMREBS = offsetof(NvmeBar, pmrebs),
>+ NVME_REG_PMRSWTP = offsetof(NvmeBar, pmrswtp),
>+ NVME_REG_PMRMSCL = offsetof(NvmeBar, pmrmscl),
>+ NVME_REG_PMRMSCU = offsetof(NvmeBar, pmrmscu),
>+};
>+
> 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
>
>
LGTM.
Reviewed-by : Gollu Appalanaidu <anaidu.gollu@samsung.com>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 5/5] tests/qtest/nvme-test: add mmio read test
[not found] ` <CGME20210714120156epcas5p212ae986c7e2ed4d30191ce8915304d2c@epcas5p2.samsung.com>
@ 2021-07-14 11:58 ` Gollu Appalanaidu
0 siblings, 0 replies; 17+ messages in thread
From: Gollu Appalanaidu @ 2021-07-14 11:58 UTC (permalink / raw)
To: Klaus Jensen
Cc: Fam Zheng, Peter Maydell, Thomas Huth, qemu-block,
Laurent Vivier, Klaus Jensen, qemu-devel, Max Reitz,
Stefan Hajnoczi, Paolo Bonzini, Keith Busch, Kevin Wolf,
Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]
On Wed, Jul 14, 2021 at 08:01:25AM +0200, Klaus Jensen wrote:
>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
>
>
Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 0/5] hw/nvme: fix mmio read
2021-07-14 6:01 [PATCH v3 0/5] hw/nvme: fix mmio read Klaus Jensen
` (4 preceding siblings ...)
2021-07-14 6:01 ` [PATCH v3 5/5] tests/qtest/nvme-test: add mmio read test Klaus Jensen
@ 2021-07-19 6:43 ` Klaus Jensen
2021-07-19 8:52 ` Stefan Hajnoczi
5 siblings, 1 reply; 17+ messages in thread
From: Klaus Jensen @ 2021-07-19 6:43 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Thomas Huth, Stefan Hajnoczi,
qemu-block, Laurent Vivier, Klaus Jensen, Gollu Appalanaidu,
Max Reitz, Kevin Wolf, Keith Busch, Paolo Bonzini,
Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 1138 bytes --]
On Jul 14 08:01, Klaus Jensen wrote:
> 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.
>
> v2:
>
> * "hw/nvme: use symbolic names for registers"
> Use offsetof(NvmeBar, reg) instead of explicit offsets (Philippe)
>
> * "hw/nvme: fix mmio read"
> Use the st/ld API instead of cpu_to_X (Philippe)
>
> 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 | 60 +++++--
> hw/nvme/ctrl.c | 362 +++++++++++++++++++++++-----------------
> tests/qtest/nvme-test.c | 26 +++
> 3 files changed, 276 insertions(+), 172 deletions(-)
>
Oi,
A review on patch 3 and 4 would be appreciated so this has a chance of
reaching Peter for -rc0 :)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/5] hw/nvme: fix out-of-bounds reads
2021-07-14 6:01 ` [PATCH v3 3/5] hw/nvme: fix out-of-bounds reads Klaus Jensen
@ 2021-07-19 8:50 ` Stefan Hajnoczi
2021-07-19 9:15 ` Peter Maydell
1 sibling, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2021-07-19 8:50 UTC (permalink / raw)
To: Klaus Jensen
Cc: Fam Zheng, Peter Maydell, Thomas Huth, qemu-block,
Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, qemu-devel,
Max Reitz, Kevin Wolf, Keith Busch, Paolo Bonzini,
Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 520 bytes --]
On Wed, Jul 14, 2021 at 08:01:23AM +0200, Klaus Jensen wrote:
> 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(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 0/5] hw/nvme: fix mmio read
2021-07-19 6:43 ` [PATCH v3 0/5] hw/nvme: fix mmio read Klaus Jensen
@ 2021-07-19 8:52 ` Stefan Hajnoczi
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2021-07-19 8:52 UTC (permalink / raw)
To: Klaus Jensen
Cc: Fam Zheng, Peter Maydell, Thomas Huth, qemu-block,
Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, qemu-devel,
Max Reitz, Kevin Wolf, Keith Busch, Paolo Bonzini,
Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]
On Mon, Jul 19, 2021 at 08:43:33AM +0200, Klaus Jensen wrote:
> On Jul 14 08:01, Klaus Jensen wrote:
> > 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.
> >
> > v2:
> >
> > * "hw/nvme: use symbolic names for registers"
> > Use offsetof(NvmeBar, reg) instead of explicit offsets (Philippe)
> >
> > * "hw/nvme: fix mmio read"
> > Use the st/ld API instead of cpu_to_X (Philippe)
> >
> > 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 | 60 +++++--
> > hw/nvme/ctrl.c | 362 +++++++++++++++++++++++-----------------
> > tests/qtest/nvme-test.c | 26 +++
> > 3 files changed, 276 insertions(+), 172 deletions(-)
> >
>
> Oi,
>
> A review on patch 3 and 4 would be appreciated so this has a chance of
> reaching Peter for -rc0 :)
I have reviewed Patch 3. Unfortunately I don't have time to review the
rest right now but maybe you can ask a specific person on the CC list to
review other patches. A Reply-All to multiple people might not receive
any attention :).
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower
2021-07-14 6:01 ` [PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower Klaus Jensen
[not found] ` <CGME20210714113905epcas5p3d582216af16ab401f806757cad6bcc8d@epcas5p3.samsung.com>
@ 2021-07-19 9:13 ` Peter Maydell
2021-07-19 9:32 ` Klaus Jensen
1 sibling, 1 reply; 17+ messages in thread
From: Peter Maydell @ 2021-07-19 9:13 UTC (permalink / raw)
To: Klaus Jensen
Cc: Fam Zheng, Kevin Wolf, Thomas Huth, Qemu-block, Laurent Vivier,
Klaus Jensen, Gollu Appalanaidu, QEMU Developers, Max Reitz,
Stefan Hajnoczi, Keith Busch, Paolo Bonzini,
Philippe Mathieu-Daudé
On Wed, 14 Jul 2021 at 07:01, Klaus Jensen <its@irrelevant.dk> wrote:
>
> 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/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);
Don't we need to shift n->bar.pmrmscu left by 32 before we OR it in
with the pmrmscl part?
> 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;
Not an issue with this patch, but why don't we need to do the
"update cba" stuff for a write to the U register that we do for
a write to the L register? Does the spec mandate that guests
do the writes in the order H then L and only the L change makes
it take effect?
thanks
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/5] hw/nvme: fix out-of-bounds reads
2021-07-14 6:01 ` [PATCH v3 3/5] hw/nvme: fix out-of-bounds reads Klaus Jensen
2021-07-19 8:50 ` Stefan Hajnoczi
@ 2021-07-19 9:15 ` Peter Maydell
1 sibling, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2021-07-19 9:15 UTC (permalink / raw)
To: Klaus Jensen
Cc: Fam Zheng, Kevin Wolf, Thomas Huth, Qemu-block, Laurent Vivier,
Klaus Jensen, Gollu Appalanaidu, QEMU Developers, Max Reitz,
Stefan Hajnoczi, Keith Busch, Paolo Bonzini,
Philippe Mathieu-Daudé
On Wed, 14 Jul 2021 at 07:01, Klaus Jensen <its@irrelevant.dk> wrote:
>
> 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(-)
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower
2021-07-19 9:13 ` Peter Maydell
@ 2021-07-19 9:32 ` Klaus Jensen
0 siblings, 0 replies; 17+ messages in thread
From: Klaus Jensen @ 2021-07-19 9:32 UTC (permalink / raw)
To: Peter Maydell
Cc: Fam Zheng, Kevin Wolf, Thomas Huth, Qemu-block, Laurent Vivier,
Klaus Jensen, Gollu Appalanaidu, QEMU Developers, Max Reitz,
Stefan Hajnoczi, Keith Busch, Paolo Bonzini,
Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 2503 bytes --]
On Jul 19 10:13, Peter Maydell wrote:
> On Wed, 14 Jul 2021 at 07:01, Klaus Jensen <its@irrelevant.dk> wrote:
> >
> > 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/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);
>
> Don't we need to shift n->bar.pmrmscu left by 32 before we OR it in
> with the pmrmscl part?
>
We do indeed - thanks for catching this!
> > 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;
>
> Not an issue with this patch, but why don't we need to do the
> "update cba" stuff for a write to the U register that we do for
> a write to the L register? Does the spec mandate that guests
> do the writes in the order H then L and only the L change makes
> it take effect?
>
Yeah, bit 1 in PMRMSCL is the "Controller Memory Space Enable" bit
(CMSE) and we only enable the address space when that bit is set. So the
driver will typically write the high register first (if needed) and then
write the lower register with or without CMSE set.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 4/5] hw/nvme: fix mmio read
2021-07-14 6:01 ` [PATCH v3 4/5] hw/nvme: fix mmio read Klaus Jensen
@ 2021-07-19 9:52 ` Peter Maydell
0 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2021-07-19 9:52 UTC (permalink / raw)
To: Klaus Jensen
Cc: Fam Zheng, Kevin Wolf, Thomas Huth, Qemu-block, Laurent Vivier,
Klaus Jensen, Gollu Appalanaidu, QEMU Developers, Max Reitz,
Stefan Hajnoczi, Keith Busch, Paolo Bonzini,
Philippe Mathieu-Daudé
On Wed, 14 Jul 2021 at 07:01, Klaus Jensen <its@irrelevant.dk> 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 | 300 ++++++++++++++++++++++++++++---------------------
> 1 file changed, 173 insertions(+), 127 deletions(-)
> @@ -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);
> + stl_le_p(&n->bar.cmbloc, 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);
> + stl_le_p(&n->bar.cmbsz, cmbsz);
This used to only set the listed fields and left the
rest of the registers untouched. Now it zeroes the other
fields in the registers. If this is an intentional change it
should be separate from this patch, I think.
> @@ -5747,9 +5767,10 @@ 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;
> + intms |= data & 0xffffffff;
intms is a uint32_t so the & here is unnecessary; you can just
say "intms |= data;".
> + stl_le_p(&n->bar.intms, intms);
> n->bar.intmc = n->bar.intms;
> - trace_pci_nvme_mmio_intm_set(data & 0xffffffff, n->bar.intmc);
> + trace_pci_nvme_mmio_intm_set(data & 0xffffffff, intms);
> nvme_irq_check(n);
> break;
> case NVME_REG_INTMC:
> @@ -5759,44 +5780,55 @@ 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);
> + intms &= ~(data & 0xffffffff);
Similarly here just "intms &= ~data;" is enough.
> + stl_le_p(&n->bar.intms, intms);
> n->bar.intmc = n->bar.intms;
> - trace_pci_nvme_mmio_intm_clr(data & 0xffffffff, n->bar.intmc);
> + trace_pci_nvme_mmio_intm_clr(data & 0xffffffff, intms);
> nvme_irq_check(n);
> break;
> @@ -5818,26 +5850,30 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
> }
> break;
> case NVME_REG_AQA:
> - n->bar.aqa = data & 0xffffffff;
> + stl_le_p(&n->bar.aqa, data & 0xffffffff);
You don't need to mask here, stl_le_p() takes a uint32_t argument and
will only write 4 bytes.
> trace_pci_nvme_mmio_aqattr(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);
> + stq_le_p(&n->bar.asq, asq);
You could save doing the manual assembly of the 64-byte value and just write
the data you have:
stn_le_p(&n->bar.asq, size, data);
> 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);
> + stq_le_p(&n->bar.asq, asq);
> + trace_pci_nvme_mmio_asqaddr_hi(data, asq);
Similarly here
stn_le_p(&n->bar.asq + 4, size, data);
trace_pci_nvme_mmio_asqaddr_hi(data, ldq_le_p(&n->bar.asq));
(and then you don't need 'asq' as a local variable in this function.)
> 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);
> + stq_le_p(&n->bar.acq, 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);
> + stq_le_p(&n->bar.acq, acq);
> + trace_pci_nvme_mmio_acqaddr_hi(data, acq);
> break;
Same remarks as for ASQ apply here for ACQ.
> case NVME_REG_CMBLOC:
> NVME_GUEST_ERR(pci_nvme_ub_mmiowr_cmbloc_reserved,
> @@ -5849,12 +5885,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);
> + stq_le_p(&n->bar.cmbmsc, cmbmsc);
> n->cmb.cmse = false;
and for CMBMSC
>
> if (NVME_CMBMSC_CRE(data)) {
> @@ -5863,7 +5900,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);
> + stl_le_p(&n->bar.cmbsts, cmbsts);
> return;
> }
>
> @@ -5877,7 +5915,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);
> + stq_le_p(&n->bar.cmbmsc, cmbmsc);
> return;
>
> case NVME_REG_PMRCAP:
> @@ -5885,19 +5924,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;
> + stl_le_p(&n->bar.pmrctl, data & 0xffffffff);
More unnecessary masking
> 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;
> }
> + stl_le_p(&n->bar.pmrsts, pmrsts);
> return;
> case NVME_REG_PMRSTS:
> NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrsts_readonly,
> @@ -5912,18 +5952,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;
pmrmscl is a uint32_t so the mask is unneeded
> + stl_le_p(&n->bar.pmrmscl, 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);
> + stl_le_p(&n->bar.pmrsts, pmrsts);
> return;
> }
>
> @@ -5933,11 +5975,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;
> + stl_le_p(&n->bar.pmrmscu, data & 0xffffffff);
Unneeded mask
> return;
> default:
> NVME_GUEST_ERR(pci_nvme_ub_mmiowr_invalid,
> @@ -6265,14 +6306,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);
> + stl_le_p(&n->bar.pmrcap, 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);
Again, this function is changing from "set these fields" to
"set these fields and zero the rest".
> @@ -6362,6 +6406,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 +6485,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);
> + stq_le_p(&n->bar.cap, cap);
Same here.
> - n->bar.vs = NVME_SPEC_VER;
> + stl_le_p(&n->bar.vs, NVME_SPEC_VER);
> n->bar.intmc = n->bar.intms = 0;
> }
thanks
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-07-19 9:54 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-14 6:01 [PATCH v3 0/5] hw/nvme: fix mmio read Klaus Jensen
2021-07-14 6:01 ` [PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower Klaus Jensen
[not found] ` <CGME20210714113905epcas5p3d582216af16ab401f806757cad6bcc8d@epcas5p3.samsung.com>
2021-07-14 11:35 ` Gollu Appalanaidu
2021-07-19 9:13 ` Peter Maydell
2021-07-19 9:32 ` Klaus Jensen
2021-07-14 6:01 ` [PATCH v3 2/5] hw/nvme: use symbolic names for registers Klaus Jensen
2021-07-14 9:08 ` Philippe Mathieu-Daudé
[not found] ` <CGME20210714114548epcas5p41a562395f6b695aabcfa4a531f2285d3@epcas5p4.samsung.com>
2021-07-14 11:42 ` Gollu Appalanaidu
2021-07-14 6:01 ` [PATCH v3 3/5] hw/nvme: fix out-of-bounds reads Klaus Jensen
2021-07-19 8:50 ` Stefan Hajnoczi
2021-07-19 9:15 ` Peter Maydell
2021-07-14 6:01 ` [PATCH v3 4/5] hw/nvme: fix mmio read Klaus Jensen
2021-07-19 9:52 ` Peter Maydell
2021-07-14 6:01 ` [PATCH v3 5/5] tests/qtest/nvme-test: add mmio read test Klaus Jensen
[not found] ` <CGME20210714120156epcas5p212ae986c7e2ed4d30191ce8915304d2c@epcas5p2.samsung.com>
2021-07-14 11:58 ` Gollu Appalanaidu
2021-07-19 6:43 ` [PATCH v3 0/5] hw/nvme: fix mmio read Klaus Jensen
2021-07-19 8:52 ` Stefan Hajnoczi
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.