[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 4/5] hw/nvme: fix mmio read
|
From: |
Peter Maydell |
|
Subject: |
Re: [PATCH v3 4/5] hw/nvme: fix mmio read |
|
Date: |
Mon, 19 Jul 2021 10:52:47 +0100 |
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
- Re: [PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower, (continued)
Re: [PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower, Peter Maydell, 2021/07/19
[PATCH v3 2/5] hw/nvme: use symbolic names for registers, Klaus Jensen, 2021/07/14
[PATCH v3 3/5] hw/nvme: fix out-of-bounds reads, Klaus Jensen, 2021/07/14
[PATCH v3 4/5] hw/nvme: fix mmio read, Klaus Jensen, 2021/07/14
- Re: [PATCH v3 4/5] hw/nvme: fix mmio read,
Peter Maydell <=
[PATCH v3 5/5] tests/qtest/nvme-test: add mmio read test, Klaus Jensen, 2021/07/14
Re: [PATCH v3 0/5] hw/nvme: fix mmio read, Klaus Jensen, 2021/07/19