* [PATCH v5 0/5] hw/nvme: fix mmio read
@ 2021-07-19 22:46 Klaus Jensen
2021-07-19 22:46 ` [PATCH v5 1/5] hw/nvme: split pmrmsc register into upper and lower Klaus Jensen
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Klaus Jensen @ 2021-07-19 22:46 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Thomas Huth, qemu-block,
Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, Max Reitz,
Klaus Jensen, Stefan Hajnoczi, Keith Busch, Paolo Bonzini,
Kevin Wolf, 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.
v5:
* "hw/nvme: fix mmio read"
- Hurried the changes a bit. Fixed.
v4:
* "hw/nvme: split pmrmsc register into upper and lower"
- Fix missing left-shift (Peter)
* "hw/nvme: fix mmio read"
- Remove unnecessary masking (Peter)
- Keep existing behaviour and do not zero the register fields doing
initialization (Peter)
v3:
* "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 | 352 ++++++++++++++++++++++------------------
tests/qtest/nvme-test.c | 26 +++
3 files changed, 265 insertions(+), 173 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v5 1/5] hw/nvme: split pmrmsc register into upper and lower 2021-07-19 22:46 [PATCH v5 0/5] hw/nvme: fix mmio read Klaus Jensen @ 2021-07-19 22:46 ` Klaus Jensen 2021-07-20 13:40 ` Peter Maydell 2021-07-19 22:46 ` [PATCH v5 2/5] hw/nvme: use symbolic names for registers Klaus Jensen ` (3 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: Klaus Jensen @ 2021-07-19 22:46 UTC (permalink / raw) To: qemu-devel Cc: Fam Zheng, Peter Maydell, Thomas Huth, qemu-block, Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, Max Reitz, Klaus Jensen, Stefan Hajnoczi, Keith Busch, Paolo Bonzini, Kevin Wolf, 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] 13+ messages in thread
* Re: [PATCH v5 1/5] hw/nvme: split pmrmsc register into upper and lower 2021-07-19 22:46 ` [PATCH v5 1/5] hw/nvme: split pmrmsc register into upper and lower Klaus Jensen @ 2021-07-20 13:40 ` Peter Maydell 2021-07-24 8:34 ` Klaus Jensen 0 siblings, 1 reply; 13+ messages in thread From: Peter Maydell @ 2021-07-20 13:40 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 Mon, 19 Jul 2021 at 23:46, 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> > 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; This mask is unnecessary because pmrmscl is uint32_t. > 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 | pmrmscu still needs to be shifted left by 32 here. > + (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; Mask not required. > return; > default: > NVME_GUEST_ERR(pci_nvme_ub_mmiowr_invalid, > -- thanks -- PMM ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 1/5] hw/nvme: split pmrmsc register into upper and lower 2021-07-20 13:40 ` Peter Maydell @ 2021-07-24 8:34 ` Klaus Jensen 0 siblings, 0 replies; 13+ messages in thread From: Klaus Jensen @ 2021-07-24 8:34 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: 1401 bytes --] On Jul 20 14:40, Peter Maydell wrote: > On Mon, 19 Jul 2021 at 23:46, 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> > > > 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; > > This mask is unnecessary because pmrmscl is uint32_t. > > > 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 | > > pmrmscu still needs to be shifted left by 32 here. > Hi Peter, Patch 4 fixed this, but I have added the fix as well in this patch in v6. I believe the series is ready now, it just needs an R-b. Thanks for your reviews on this! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v5 2/5] hw/nvme: use symbolic names for registers 2021-07-19 22:46 [PATCH v5 0/5] hw/nvme: fix mmio read Klaus Jensen 2021-07-19 22:46 ` [PATCH v5 1/5] hw/nvme: split pmrmsc register into upper and lower Klaus Jensen @ 2021-07-19 22:46 ` Klaus Jensen 2021-07-19 23:25 ` Keith Busch 2021-07-19 22:46 ` [PATCH v5 3/5] hw/nvme: fix out-of-bounds reads Klaus Jensen ` (2 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: Klaus Jensen @ 2021-07-19 22:46 UTC (permalink / raw) To: qemu-devel Cc: Fam Zheng, Peter Maydell, Thomas Huth, qemu-block, Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, Max Reitz, Klaus Jensen, Stefan Hajnoczi, Keith Busch, Paolo Bonzini, Kevin Wolf, 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> Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.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] 13+ messages in thread
* Re: [PATCH v5 2/5] hw/nvme: use symbolic names for registers 2021-07-19 22:46 ` [PATCH v5 2/5] hw/nvme: use symbolic names for registers Klaus Jensen @ 2021-07-19 23:25 ` Keith Busch 0 siblings, 0 replies; 13+ messages in thread From: Keith Busch @ 2021-07-19 23:25 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, Stefan Hajnoczi, Paolo Bonzini, Kevin Wolf, Philippe Mathieu-Daudé On Tue, Jul 20, 2021 at 12:46:44AM +0200, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > Add the NvmeBarRegs enum and use these instead of explicit register > offsets. Thanks, this is a very nice cleanup. For a suggested follow-up companion patch, we should add "ASSERT_OFFSET()" checks for each register to enforce correct positioning of the BAR offsets at build time. Reviewed-by: Keith Busch <kbusch@kernel.org> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.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 [flat|nested] 13+ messages in thread
* [PATCH v5 3/5] hw/nvme: fix out-of-bounds reads 2021-07-19 22:46 [PATCH v5 0/5] hw/nvme: fix mmio read Klaus Jensen 2021-07-19 22:46 ` [PATCH v5 1/5] hw/nvme: split pmrmsc register into upper and lower Klaus Jensen 2021-07-19 22:46 ` [PATCH v5 2/5] hw/nvme: use symbolic names for registers Klaus Jensen @ 2021-07-19 22:46 ` Klaus Jensen 2021-07-19 22:46 ` [PATCH v5 4/5] hw/nvme: fix mmio read Klaus Jensen 2021-07-19 22:46 ` [PATCH v5 5/5] tests/qtest/nvme-test: add mmio read test Klaus Jensen 4 siblings, 0 replies; 13+ messages in thread From: Klaus Jensen @ 2021-07-19 22:46 UTC (permalink / raw) To: qemu-devel Cc: Fam Zheng, Peter Maydell, Thomas Huth, qemu-block, Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, Max Reitz, Klaus Jensen, Stefan Hajnoczi, Keith Busch, Paolo Bonzini, Kevin Wolf, 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> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> --- 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] 13+ messages in thread
* [PATCH v5 4/5] hw/nvme: fix mmio read 2021-07-19 22:46 [PATCH v5 0/5] hw/nvme: fix mmio read Klaus Jensen ` (2 preceding siblings ...) 2021-07-19 22:46 ` [PATCH v5 3/5] hw/nvme: fix out-of-bounds reads Klaus Jensen @ 2021-07-19 22:46 ` Klaus Jensen 2021-07-20 12:58 ` Philippe Mathieu-Daudé 2021-07-20 13:37 ` Peter Maydell 2021-07-19 22:46 ` [PATCH v5 5/5] tests/qtest/nvme-test: add mmio read test Klaus Jensen 4 siblings, 2 replies; 13+ messages in thread From: Klaus Jensen @ 2021-07-19 22:46 UTC (permalink / raw) To: qemu-devel Cc: Fam Zheng, Peter Maydell, Thomas Huth, qemu-block, Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, Max Reitz, Klaus Jensen, Stefan Hajnoczi, Keith Busch, Paolo Bonzini, Kevin Wolf, 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 | 290 +++++++++++++++++++++++++++---------------------- 1 file changed, 162 insertions(+), 128 deletions(-) diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index 0449cc4dee9b..43dfaeac9f54 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,33 @@ 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 = ldl_le_p(&n->bar.cmbloc); + uint32_t cmbsz = ldl_le_p(&n->bar.cmbsz); - 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); + uint32_t pmrsts = ldl_le_p(&n->bar.pmrsts); + if (unlikely(offset & (sizeof(uint32_t) - 1))) { NVME_GUEST_ERR(pci_nvme_ub_mmiowr_misaligned32, "MMIO write not 32-bit aligned," @@ -5747,9 +5762,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; + 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 +5775,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; + 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 +5845,24 @@ 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); trace_pci_nvme_mmio_aqattr(data & 0xffffffff); break; case NVME_REG_ASQ: - n->bar.asq = size == 8 ? data : - (n->bar.asq & ~0xffffffffULL) | (data & 0xffffffff); + 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); + stl_le_p((uint8_t *)&n->bar.asq + 4, data); + trace_pci_nvme_mmio_asqaddr_hi(data, ldq_le_p(&n->bar.asq)); break; case NVME_REG_ACQ: trace_pci_nvme_mmio_acqaddr(data); - n->bar.acq = size == 8 ? data : - (n->bar.acq & ~0xffffffffULL) | (data & 0xffffffff); + stn_le_p(&n->bar.acq, size, data); 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); + stl_le_p((uint8_t *)&n->bar.acq + 4, data); + trace_pci_nvme_mmio_acqaddr_hi(data, ldq_le_p(&n->bar.acq)); break; case NVME_REG_CMBLOC: NVME_GUEST_ERR(pci_nvme_ub_mmiowr_cmbloc_reserved, @@ -5849,21 +5874,23 @@ 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); + stn_le_p(&n->bar.cmbmsc, size, data); n->cmb.cmse = false; if (NVME_CMBMSC_CRE(data)) { nvme_cmb_enable_regs(n); if (NVME_CMBMSC_CMSE(data)) { - hwaddr cba = NVME_CMBMSC_CBA(data) << CMBMSC_CBA_SHIFT; + uint64_t cmbmsc = ldq_le_p(&n->bar.cmbmsc); + hwaddr cba = NVME_CMBMSC_CBA(cmbmsc) << CMBMSC_CBA_SHIFT; if (cba + int128_get64(n->cmb.mem.size) < cba) { - NVME_CMBSTS_SET_CBAI(n->bar.cmbsts, 1); + uint32_t cmbsts = ldl_le_p(&n->bar.cmbsts); + NVME_CMBSTS_SET_CBAI(cmbsts, 1); + stl_le_p(&n->bar.cmbsts, cmbsts); return; } @@ -5877,7 +5904,7 @@ 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); + stl_le_p((uint8_t *)&n->bar.cmbmsc + 4, data); return; case NVME_REG_PMRCAP: @@ -5885,19 +5912,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); 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 +5940,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; + stl_le_p(&n->bar.pmrmscl, data); 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)) { + uint64_t pmrmscu = ldl_le_p(&n->bar.pmrmscu); + hwaddr cba = pmrmscu << 32 | + (NVME_PMRMSCL_CBA(data) << 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 +5963,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); return; default: NVME_GUEST_ERR(pci_nvme_ub_mmiowr_invalid, @@ -5952,7 +5982,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 +6011,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 +6273,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 +6283,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 +6294,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 = ldl_le_p(&n->bar.pmrcap); - 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 +6394,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 = ldq_le_p(&n->bar.cap); 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 +6473,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 +6635,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] 13+ messages in thread
* Re: [PATCH v5 4/5] hw/nvme: fix mmio read 2021-07-19 22:46 ` [PATCH v5 4/5] hw/nvme: fix mmio read Klaus Jensen @ 2021-07-20 12:58 ` Philippe Mathieu-Daudé 2021-07-20 13:33 ` Peter Maydell 2021-07-20 13:37 ` Peter Maydell 1 sibling, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-20 12:58 UTC (permalink / raw) To: Klaus Jensen, Peter Maydell Cc: Fam Zheng, Kevin Wolf, Thomas Huth, qemu-block, Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, qemu-devel, Max Reitz, Stefan Hajnoczi, Keith Busch, Paolo Bonzini On 7/20/21 12:46 AM, 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 | 290 +++++++++++++++++++++++++++---------------------- > 1 file changed, 162 insertions(+), 128 deletions(-) > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c > index 0449cc4dee9b..43dfaeac9f54 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) { Why not use an inline call like the rest of this file? if (~ldl_le_p(&n->bar.intms) & n->irq_status) { Anyway, not an issue. > + 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); This one is understandable. > 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; This change is not documented, is it related to the fix? > } > > 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; My brain overflowed at this point, too many changes to track :/ Would it make sense to split it? (per big function body maybe?) Note, using so many manual endian accesses seems fragile. Maybe we could find a way to write a pair of macros taking 'n' + bar 'fieldname' as input, using the correct size swapping using sizeof_field(fieldname)? Something like (untested): #define ld_bar(fieldname) \ ldn_le_p(&n->bar.fieldname, \ sizeof_field(NvmeBar, fieldname)); #define st_bar(fieldname, val) \ stn_le_p(&n->bar.fieldname, \ sizeof_field(NvmeBar, fieldname), val); Using as: uint64_t cap = ld_bar(cap); Or if you prefer: #define BAR_LD(bar, fieldname) \ ldn_le_p(pbar.fieldname, \ sizeof_field(NvmeBar, fieldname)); as: uint64_t cap = BAR_LD(&n->bar, cap); Regards, Phil. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 4/5] hw/nvme: fix mmio read 2021-07-20 12:58 ` Philippe Mathieu-Daudé @ 2021-07-20 13:33 ` Peter Maydell 2021-07-20 13:39 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 13+ messages in thread From: Peter Maydell @ 2021-07-20 13:33 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Fam Zheng, Kevin Wolf, Thomas Huth, Qemu-block, Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, QEMU Developers, Max Reitz, Keith Busch, Stefan Hajnoczi, Klaus Jensen, Paolo Bonzini On Tue, 20 Jul 2021 at 13:58, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 7/20/21 12:46 AM, 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. > My brain overflowed at this point, too many changes to track :/ > > Would it make sense to split it? (per big function body maybe?) I did review of (a previous version of) the patch by: (1) check that after the patch is applied there aren't any direct accesses to n->bar.anything which don't go via a ld*/st* (ie it didn't miss anything) (2) check that the accesses which use ldq/stq are consistently doing so for those fields and that they are uint64_t (3) read through the patch and check all the changes are ok (either using inline calls to the accessors, or fishing the value out at the start of the function) It is a big patch, but I'm not sure splitting it apart helps much. thanks -- PMM ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 4/5] hw/nvme: fix mmio read 2021-07-20 13:33 ` Peter Maydell @ 2021-07-20 13:39 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-20 13:39 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, Keith Busch, Stefan Hajnoczi, Klaus Jensen, Paolo Bonzini On 7/20/21 3:33 PM, Peter Maydell wrote: > On Tue, 20 Jul 2021 at 13:58, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> On 7/20/21 12:46 AM, 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. > >> My brain overflowed at this point, too many changes to track :/ >> >> Would it make sense to split it? (per big function body maybe?) > > I did review of (a previous version of) the patch by: > (1) check that after the patch is applied there aren't any > direct accesses to n->bar.anything which don't go via a > ld*/st* (ie it didn't miss anything) OK > (2) check that the accesses which use ldq/stq are consistently > doing so for those fields and that they are uint64_t OK > (3) read through the patch and check all the changes are ok > (either using inline calls to the accessors, or fishing > the value out at the start of the function) OK, perfect then :) > It is a big patch, but I'm not sure splitting it apart helps much. Since it is reviewed, not needed. Thanks a lot! Phil. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 4/5] hw/nvme: fix mmio read 2021-07-19 22:46 ` [PATCH v5 4/5] hw/nvme: fix mmio read Klaus Jensen 2021-07-20 12:58 ` Philippe Mathieu-Daudé @ 2021-07-20 13:37 ` Peter Maydell 1 sibling, 0 replies; 13+ messages in thread From: Peter Maydell @ 2021-07-20 13:37 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 Mon, 19 Jul 2021 at 23:47, 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 | 290 +++++++++++++++++++++++++++---------------------- > 1 file changed, 162 insertions(+), 128 deletions(-) Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v5 5/5] tests/qtest/nvme-test: add mmio read test 2021-07-19 22:46 [PATCH v5 0/5] hw/nvme: fix mmio read Klaus Jensen ` (3 preceding siblings ...) 2021-07-19 22:46 ` [PATCH v5 4/5] hw/nvme: fix mmio read Klaus Jensen @ 2021-07-19 22:46 ` Klaus Jensen 4 siblings, 0 replies; 13+ messages in thread From: Klaus Jensen @ 2021-07-19 22:46 UTC (permalink / raw) To: qemu-devel Cc: Fam Zheng, Peter Maydell, Thomas Huth, qemu-block, Laurent Vivier, Klaus Jensen, Gollu Appalanaidu, Max Reitz, Klaus Jensen, Stefan Hajnoczi, Keith Busch, Paolo Bonzini, Kevin Wolf, 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> Reviewed-by: Gollu Appalanaidu <anaidu.gollu@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] 13+ messages in thread
end of thread, other threads:[~2021-07-24 8:38 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-19 22:46 [PATCH v5 0/5] hw/nvme: fix mmio read Klaus Jensen 2021-07-19 22:46 ` [PATCH v5 1/5] hw/nvme: split pmrmsc register into upper and lower Klaus Jensen 2021-07-20 13:40 ` Peter Maydell 2021-07-24 8:34 ` Klaus Jensen 2021-07-19 22:46 ` [PATCH v5 2/5] hw/nvme: use symbolic names for registers Klaus Jensen 2021-07-19 23:25 ` Keith Busch 2021-07-19 22:46 ` [PATCH v5 3/5] hw/nvme: fix out-of-bounds reads Klaus Jensen 2021-07-19 22:46 ` [PATCH v5 4/5] hw/nvme: fix mmio read Klaus Jensen 2021-07-20 12:58 ` Philippe Mathieu-Daudé 2021-07-20 13:33 ` Peter Maydell 2021-07-20 13:39 ` Philippe Mathieu-Daudé 2021-07-20 13:37 ` Peter Maydell 2021-07-19 22:46 ` [PATCH v5 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.