[PATCH v2] hw/nvme: clean up CC register write logic

[PATCH v2] hw/nvme: clean up CC register write logic

[Klaus Jensen]

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>
---
 hw/nvme/ctrl.c | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

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;
-- 
2.36.1

Re: [PATCH v2] hw/nvme: clean up CC register write logic

[Lukasz Maniak]

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
> 

Re: [PATCH v2] hw/nvme: clean up CC register write logic

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

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)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] hw/nvme: clean up CC register write logic

[Łukasz Gieryk]

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.

Re: [PATCH v2] hw/nvme: clean up CC register write logic

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1387 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]