| Message ID | 20220607112320.58532-1-its@irrelevant.dk |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] hw/nvme: clean up CC register write logic | expand |
On Tue, Jun 07, 2022 at 01:23:20PM +0200, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > The SRIOV series exposed an issued with how CC register writes are > handled and how CSTS is set in response to that. Specifically, after > applying the SRIOV series, the controller could end up in a state with > CC.EN set to '1' but with CSTS.RDY cleared to '0', causing drivers to > expect CSTS.RDY to transition to '1' but timing out. > > Clean this up. > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > --- > v3: > * clear intms/intmc/cc regardless of reset type > > hw/nvme/ctrl.c | 38 ++++++++++++++++---------------------- > 1 file changed, 16 insertions(+), 22 deletions(-) > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c > index 658584d417fe..a558f5cb29c1 100644 > --- a/hw/nvme/ctrl.c > +++ b/hw/nvme/ctrl.c > @@ -6190,10 +6190,15 @@ static void nvme_ctrl_reset(NvmeCtrl *n, NvmeResetType rst) > > if (pci_is_vf(pci_dev)) { > sctrl = nvme_sctrl(n); > + > stl_le_p(&n->bar.csts, sctrl->scs ? 0 : NVME_CSTS_FAILED); > } else { > stl_le_p(&n->bar.csts, 0); > } > + > + stl_le_p(&n->bar.intms, 0); > + stl_le_p(&n->bar.intmc, 0); > + stl_le_p(&n->bar.cc, 0); > } > > static void nvme_ctrl_shutdown(NvmeCtrl *n) > @@ -6405,20 +6410,21 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data, > nvme_irq_check(n); > break; > case NVME_REG_CC: > + stl_le_p(&n->bar.cc, data); > + > trace_pci_nvme_mmio_cfg(data & 0xffffffff); > > - /* Windows first sends data, then sends enable bit */ > - if (!NVME_CC_EN(data) && !NVME_CC_EN(cc) && > - !NVME_CC_SHN(data) && !NVME_CC_SHN(cc)) > - { > - cc = data; > + if (NVME_CC_SHN(data) && !(NVME_CC_SHN(cc))) { > + trace_pci_nvme_mmio_shutdown_set(); > + nvme_ctrl_shutdown(n); > + csts &= ~(CSTS_SHST_MASK << CSTS_SHST_SHIFT); > + csts |= NVME_CSTS_SHST_COMPLETE; > + } else if (!NVME_CC_SHN(data) && NVME_CC_SHN(cc)) { > + trace_pci_nvme_mmio_shutdown_cleared(); > + csts &= ~(CSTS_SHST_MASK << CSTS_SHST_SHIFT); > } > > 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(); > csts = NVME_CSTS_FAILED; > @@ -6429,22 +6435,10 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data, > } else if (!NVME_CC_EN(data) && NVME_CC_EN(cc)) { > trace_pci_nvme_mmio_stopped(); > nvme_ctrl_reset(n, NVME_RESET_CONTROLLER); > - cc = 0; > - csts &= ~NVME_CSTS_READY; > - } > > - if (NVME_CC_SHN(data) && !(NVME_CC_SHN(cc))) { > - trace_pci_nvme_mmio_shutdown_set(); > - nvme_ctrl_shutdown(n); > - cc = data; > - csts |= NVME_CSTS_SHST_COMPLETE; > - } else if (!NVME_CC_SHN(data) && NVME_CC_SHN(cc)) { > - trace_pci_nvme_mmio_shutdown_cleared(); > - csts &= ~NVME_CSTS_SHST_COMPLETE; > - cc = data; > + break; > } > > - stl_le_p(&n->bar.cc, cc); > stl_le_p(&n->bar.csts, csts); > > break; > -- > 2.36.1 > Reviewed-by: Lukasz Maniak <lukasz.maniak@linux.intel.com>
On Tue, Jun 07, 2022 at 01:23:20PM +0200, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > The SRIOV series exposed an issued with how CC register writes are > handled and how CSTS is set in response to that. Specifically, after > applying the SRIOV series, the controller could end up in a state with > CC.EN set to '1' but with CSTS.RDY cleared to '0', causing drivers to > expect CSTS.RDY to transition to '1' but timing out. > > Clean this up. > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > Reviewed-by: Lukasz Maniak <lukasz.maniak@linux.intel.com> > --- > v3: > * clear intms/intmc/cc regardless of reset type > > hw/nvme/ctrl.c | 38 ++++++++++++++++---------------------- > 1 file changed, 16 insertions(+), 22 deletions(-) > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c > index 658584d417fe..a558f5cb29c1 100644 > --- a/hw/nvme/ctrl.c > +++ b/hw/nvme/ctrl.c > @@ -6190,10 +6190,15 @@ static void nvme_ctrl_reset(NvmeCtrl *n, NvmeResetType rst) > > if (pci_is_vf(pci_dev)) { > sctrl = nvme_sctrl(n); > + > stl_le_p(&n->bar.csts, sctrl->scs ? 0 : NVME_CSTS_FAILED); > } else { > stl_le_p(&n->bar.csts, 0); > } > + > + stl_le_p(&n->bar.intms, 0); > + stl_le_p(&n->bar.intmc, 0); > + stl_le_p(&n->bar.cc, 0); > } > > static void nvme_ctrl_shutdown(NvmeCtrl *n) > @@ -6405,20 +6410,21 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data, > nvme_irq_check(n); > break; > case NVME_REG_CC: > + stl_le_p(&n->bar.cc, data); > + > trace_pci_nvme_mmio_cfg(data & 0xffffffff); > > - /* Windows first sends data, then sends enable bit */ > - if (!NVME_CC_EN(data) && !NVME_CC_EN(cc) && > - !NVME_CC_SHN(data) && !NVME_CC_SHN(cc)) > - { > - cc = data; > + if (NVME_CC_SHN(data) && !(NVME_CC_SHN(cc))) { > + trace_pci_nvme_mmio_shutdown_set(); > + nvme_ctrl_shutdown(n); > + csts &= ~(CSTS_SHST_MASK << CSTS_SHST_SHIFT); > + csts |= NVME_CSTS_SHST_COMPLETE; > + } else if (!NVME_CC_SHN(data) && NVME_CC_SHN(cc)) { > + trace_pci_nvme_mmio_shutdown_cleared(); > + csts &= ~(CSTS_SHST_MASK << CSTS_SHST_SHIFT); > } > > 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(); > csts = NVME_CSTS_FAILED; > @@ -6429,22 +6435,10 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data, > } else if (!NVME_CC_EN(data) && NVME_CC_EN(cc)) { > trace_pci_nvme_mmio_stopped(); > nvme_ctrl_reset(n, NVME_RESET_CONTROLLER); > - cc = 0; > - csts &= ~NVME_CSTS_READY; > - } > > - if (NVME_CC_SHN(data) && !(NVME_CC_SHN(cc))) { > - trace_pci_nvme_mmio_shutdown_set(); > - nvme_ctrl_shutdown(n); > - cc = data; > - csts |= NVME_CSTS_SHST_COMPLETE; > - } else if (!NVME_CC_SHN(data) && NVME_CC_SHN(cc)) { > - trace_pci_nvme_mmio_shutdown_cleared(); > - csts &= ~NVME_CSTS_SHST_COMPLETE; > - cc = data; > + break; > } > > - stl_le_p(&n->bar.cc, cc); > stl_le_p(&n->bar.csts, csts); > > break; > -- > 2.36.1 Reviewed-by: Łukasz Gieryk <lukasz.gieryk@linux.intel.com>
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index 658584d417fe..a558f5cb29c1 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -6190,10 +6190,15 @@ static void nvme_ctrl_reset(NvmeCtrl *n, NvmeResetType rst) if (pci_is_vf(pci_dev)) { sctrl = nvme_sctrl(n); + stl_le_p(&n->bar.csts, sctrl->scs ? 0 : NVME_CSTS_FAILED); } else { stl_le_p(&n->bar.csts, 0); } + + stl_le_p(&n->bar.intms, 0); + stl_le_p(&n->bar.intmc, 0); + stl_le_p(&n->bar.cc, 0); } static void nvme_ctrl_shutdown(NvmeCtrl *n) @@ -6405,20 +6410,21 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data, nvme_irq_check(n); break; case NVME_REG_CC: + stl_le_p(&n->bar.cc, data); + trace_pci_nvme_mmio_cfg(data & 0xffffffff); - /* Windows first sends data, then sends enable bit */ - if (!NVME_CC_EN(data) && !NVME_CC_EN(cc) && - !NVME_CC_SHN(data) && !NVME_CC_SHN(cc)) - { - cc = data; + if (NVME_CC_SHN(data) && !(NVME_CC_SHN(cc))) { + trace_pci_nvme_mmio_shutdown_set(); + nvme_ctrl_shutdown(n); + csts &= ~(CSTS_SHST_MASK << CSTS_SHST_SHIFT); + csts |= NVME_CSTS_SHST_COMPLETE; + } else if (!NVME_CC_SHN(data) && NVME_CC_SHN(cc)) { + trace_pci_nvme_mmio_shutdown_cleared(); + csts &= ~(CSTS_SHST_MASK << CSTS_SHST_SHIFT); } 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(); csts = NVME_CSTS_FAILED; @@ -6429,22 +6435,10 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data, } else if (!NVME_CC_EN(data) && NVME_CC_EN(cc)) { trace_pci_nvme_mmio_stopped(); nvme_ctrl_reset(n, NVME_RESET_CONTROLLER); - cc = 0; - csts &= ~NVME_CSTS_READY; - } - if (NVME_CC_SHN(data) && !(NVME_CC_SHN(cc))) { - trace_pci_nvme_mmio_shutdown_set(); - nvme_ctrl_shutdown(n); - cc = data; - csts |= NVME_CSTS_SHST_COMPLETE; - } else if (!NVME_CC_SHN(data) && NVME_CC_SHN(cc)) { - trace_pci_nvme_mmio_shutdown_cleared(); - csts &= ~NVME_CSTS_SHST_COMPLETE; - cc = data; + break; } - stl_le_p(&n->bar.cc, cc); stl_le_p(&n->bar.csts, csts); break;