| Message ID | 20220525073524.2227333-1-its@irrelevant.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] hw/nvme: clean up CC register write logic | expand |
On Wed, May 25, 2022 at 09:35:24AM +0200, Klaus Jensen wrote: > > + stl_le_p(&n->bar.intms, 0); > + stl_le_p(&n->bar.intmc, 0); > + stl_le_p(&n->bar.cc, 0); Looks fine, though it seems the NVMe spec says the above registers should be cleared during each reset for VF as well. > -- > 2.36.1 >
On Jun 1 15:28, Lukasz Maniak wrote: > On Wed, May 25, 2022 at 09:35:24AM +0200, Klaus Jensen wrote: > > > > + stl_le_p(&n->bar.intms, 0); > > + stl_le_p(&n->bar.intmc, 0); > > + stl_le_p(&n->bar.cc, 0); > > Looks fine, though it seems the NVMe spec says the above registers > should be cleared during each reset for VF as well. > Aren't the values of all other registers than CSTS just undefined? (NVMe v2.0b, Section 8.26.3)
On Fri, Jun 03, 2022 at 10:24:51PM +0200, Klaus Jensen wrote: > On Jun 1 15:28, Lukasz Maniak wrote: > > On Wed, May 25, 2022 at 09:35:24AM +0200, Klaus Jensen wrote: > > > > > > + stl_le_p(&n->bar.intms, 0); > > > + stl_le_p(&n->bar.intmc, 0); > > > + stl_le_p(&n->bar.cc, 0); > > > > Looks fine, though it seems the NVMe spec says the above registers > > should be cleared during each reset for VF as well. > > > > Aren't the values of all other registers than CSTS just undefined? (NVMe > v2.0b, Section 8.26.3) My 2 cents – When VF is online: - Both Controller Reset (CR) and PCIe Function Level Reset (FLR) can be issued to given VF - Both resets shall return all (except specific) Nvme registers of given VF to their reset values (3.7.2) When VF is offline: - CR cannot be issued (only CSTS is defined, writes to CC are dropped), so doesn’t need an explicit IF - FLR is allowed as it’s a part of the procedure to bring VF online (mentioned in 8.26.3) - At least FLR shall reset the registers for VF So I agree with the other Lukasz's suggestion. I would also clear intms/intmc/cc for both: VF and PF reset paths, regardless of the actual reset type.
On Jun 7 13:06, Łukasz Gieryk wrote: > On Fri, Jun 03, 2022 at 10:24:51PM +0200, Klaus Jensen wrote: > > On Jun 1 15:28, Lukasz Maniak wrote: > > > On Wed, May 25, 2022 at 09:35:24AM +0200, Klaus Jensen wrote: > > > > > > > > + stl_le_p(&n->bar.intms, 0); > > > > + stl_le_p(&n->bar.intmc, 0); > > > > + stl_le_p(&n->bar.cc, 0); > > > > > > Looks fine, though it seems the NVMe spec says the above registers > > > should be cleared during each reset for VF as well. > > > > > > > Aren't the values of all other registers than CSTS just undefined? (NVMe > > v2.0b, Section 8.26.3) > > My 2 cents – > > When VF is online: > - Both Controller Reset (CR) and PCIe Function Level Reset (FLR) can be > issued to given VF > - Both resets shall return all (except specific) Nvme registers of given > VF to their reset values (3.7.2) > > When VF is offline: > - CR cannot be issued (only CSTS is defined, writes to CC are dropped), > so doesn’t need an explicit IF > - FLR is allowed as it’s a part of the procedure to bring VF online > (mentioned in 8.26.3) > - At least FLR shall reset the registers for VF > > So I agree with the other Lukasz's suggestion. I would also clear > intms/intmc/cc for both: VF and PF reset paths, regardless of the actual > reset type. > Alright thanks - sounds good. See v3.
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index 658584d417fe..8b16b2b8febb 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -6190,8 +6190,12 @@ 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.intms, 0); + stl_le_p(&n->bar.intmc, 0); + stl_le_p(&n->bar.cc, 0); stl_le_p(&n->bar.csts, 0); } } @@ -6405,20 +6409,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 +6434,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;