diff mbox series

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

Message ID 20220607112320.58532-1-its@irrelevant.dk
State New
Headers show
Series [v3] hw/nvme: clean up CC register write logic | expand

Commit Message

Klaus Jensen June 7, 2022, 11:23 a.m. UTC
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(-)

Comments

Lukasz Maniak June 7, 2022, 12:23 p.m. UTC | #1
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>
Łukasz Gieryk June 8, 2022, 9:04 a.m. UTC | #2
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 mbox series

Patch

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;