* [PATCH] hw/nvme: clean up CC register write logic
@ 2022-05-17 11:16 Klaus Jensen
2022-05-19 12:11 ` Lukasz Maniak
0 siblings, 1 reply; 3+ messages in thread
From: Klaus Jensen @ 2022-05-17 11:16 UTC (permalink / raw)
To: qemu-devel
Cc: Lukasz Maniak, qemu-block, Klaus Jensen, Keith Busch, 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>
---
Note, this applies on top of nvme-next with v8 of Lukasz's sriov series.
hw/nvme/ctrl.c | 35 +++++++++++------------------------
1 file changed, 11 insertions(+), 24 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 658584d417fe..47d971b2404c 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -6190,9 +6190,8 @@ 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);
}
}
@@ -6405,20 +6404,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 +6429,9 @@ 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;
- }
-
- stl_le_p(&n->bar.cc, cc);
stl_le_p(&n->bar.csts, csts);
break;
--
2.36.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] hw/nvme: clean up CC register write logic
2022-05-17 11:16 [PATCH] hw/nvme: clean up CC register write logic Klaus Jensen
@ 2022-05-19 12:11 ` Lukasz Maniak
2022-05-25 7:35 ` Klaus Jensen
0 siblings, 1 reply; 3+ messages in thread
From: Lukasz Maniak @ 2022-05-19 12:11 UTC (permalink / raw)
To: qemu-devel, Klaus Jensen
Cc: qemu-block, Keith Busch, Klaus Jensen, lukasz.gieryk
On Tue, May 17, 2022 at 01:16:05PM +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>
> ---
>
> Note, this applies on top of nvme-next with v8 of Lukasz's sriov series.
>
> hw/nvme/ctrl.c | 35 +++++++++++------------------------
> 1 file changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 658584d417fe..47d971b2404c 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -6190,9 +6190,8 @@ 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);
Are you sure the registers do not need to be cleared for a reset type that
does not involve a CC register i.e. FLR?
Will these registers be zeroed out elsewhere during FLR?
> }
> }
>
> @@ -6405,20 +6404,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 +6429,9 @@ 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;
> - }
> -
> - stl_le_p(&n->bar.cc, cc);
> stl_le_p(&n->bar.csts, csts);
>
> break;
> --
> 2.36.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] hw/nvme: clean up CC register write logic
2022-05-19 12:11 ` Lukasz Maniak
@ 2022-05-25 7:35 ` Klaus Jensen
0 siblings, 0 replies; 3+ messages in thread
From: Klaus Jensen @ 2022-05-25 7:35 UTC (permalink / raw)
To: Lukasz Maniak
Cc: qemu-devel, qemu-block, Keith Busch, Klaus Jensen, lukasz.gieryk
[-- Attachment #1: Type: text/plain, Size: 1506 bytes --]
On May 19 14:11, Lukasz Maniak wrote:
> On Tue, May 17, 2022 at 01:16:05PM +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>
> > ---
> >
> > Note, this applies on top of nvme-next with v8 of Lukasz's sriov series.
> >
> > hw/nvme/ctrl.c | 35 +++++++++++------------------------
> > 1 file changed, 11 insertions(+), 24 deletions(-)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 658584d417fe..47d971b2404c 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -6190,9 +6190,8 @@ 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);
>
> Are you sure the registers do not need to be cleared for a reset type that
> does not involve a CC register i.e. FLR?
> Will these registers be zeroed out elsewhere during FLR?
>
Indeed you are right. Posting a v2.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-05-25 7:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-17 11:16 [PATCH] hw/nvme: clean up CC register write logic Klaus Jensen
2022-05-19 12:11 ` Lukasz Maniak
2022-05-25 7:35 ` Klaus Jensen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).