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

[Lukasz Maniak]

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>