[PATCH 0/3] NVMe PCI endpoint target fixes

[PATCH 0/3] NVMe PCI endpoint target fixes

[Damien Le Moal]

Two bug fix patch and one cleanup patch in this series.

The first patch fixes an issue with completion queue entries
initialization in the case of failed commands.

The second patch fixes
the initialization of the CC register to ensure that we can always
detect that the host is attempting to enable the controller (with
CC.EN).

The last patch cleans up/simplifies the management of the PCI link
state.

Damien Le Moal (3):
  nvmet: pci-epf: Always fully initialize completion entries
  nvmet: pci-epf: Clear CC and CSTS when disabling the controller
  nvmet: pci-epf: Cleanup link state management

 drivers/nvme/target/pci-epf.c | 65 ++++++++++++++++++++++-------------
 1 file changed, 42 insertions(+), 23 deletions(-)

-- 
2.49.0

[PATCH 1/3] nvmet: pci-epf: Always fully initialize completion entries

[Damien Le Moal]

For a command that is normally processed through the command request
execute() function, the completion entry for the command is initialized
by __nvmet_req_complete() and nvmet_pci_epf_cq_work() only needs to set
the status field and the phase of the completion entry before posting
the entry to the completion queue.

However, for commands that are failed due to an internal error (e.g. the
command data buffer allocation fails), the command request execute()
function is not called and __nvmet_req_complete() is never executed for
the command, leaving the command completion entry uninitialized. For
such command failed before calling req->execute(), the host ends up
seeing completion entries with an invalid submission queue ID and
command ID.

Avoid such issue by always fully initilizing a command completion entry
in nvmet_pci_epf_cq_work(), setting the entry submission queue ID and
command ID.

Fixes: 0faa0fe6f90e ("nvmet: New NVMe PCI endpoint function target driver")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/nvme/target/pci-epf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
index 51c27b32248d..f6b22ef4c267 100644
--- a/drivers/nvme/target/pci-epf.c
+++ b/drivers/nvme/target/pci-epf.c
@@ -1763,6 +1763,8 @@ static void nvmet_pci_epf_cq_work(struct work_struct *work)
 
 		/* Post the IOD completion entry. */
 		cqe = &iod->cqe;
+		cqe->sq_id = cpu_to_le16(iod->sq->qid);
+		cqe->command_id = iod->cmd.common.command_id;
 		cqe->status = cpu_to_le16((iod->status << 1) | cq->phase);
 
 		dev_dbg(ctrl->dev,
-- 
2.49.0

[PATCH 2/3] nvmet: pci-epf: Clear CC and CSTS when disabling the controller

[Damien Le Moal]

When a host shuts down the controller when shutting down but does so
without first disabling the controller, the enable bit remains set in
the controller configuration register. When the host restarts and
attempts to enable the controller again, the
nvmet_pci_epf_poll_cc_work() function is unable to detect the change
from 0 to 1 of the enable bit, and thus the controller is not enabled
again, which result in a device scan timeout on the host. This problem
also occurs if the host shuts down uncleanly or if the PCIe link goes
down: as the CC.EN value is not reset, the controller is not enabled
again when the host restarts.

Fix this by introducing the function nvmet_pci_epf_clear_ctrl_config()
to clear the CC and CSTS registers of the controller when the PCIe link
is lost (nvmet_pci_epf_stop_ctrl() function), and when starting the
controller fails (nvmet_pci_epf_enable_ctrl() fails). Also use this
function in nvmet_pci_epf_init_bar() to simplify the initialization of
the CC and CSTS registers.

Furthermore, modify the function nvmet_pci_epf_disable_ctrl() to clear
the CC.EN bit and write this updated value to the BAR register when the
controller is shutdown by the host, to ensure that upon restart, we can
detect the host setting CC.EN.

Fixes: 0faa0fe6f90e ("nvmet: New NVMe PCI endpoint function target driver")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/nvme/target/pci-epf.c | 49 +++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
index f6b22ef4c267..f18faf407eab 100644
--- a/drivers/nvme/target/pci-epf.c
+++ b/drivers/nvme/target/pci-epf.c
@@ -1802,6 +1802,21 @@ static void nvmet_pci_epf_cq_work(struct work_struct *work)
 				   NVMET_PCI_EPF_CQ_RETRY_INTERVAL);
 }
 
+static void nvmet_pci_epf_clear_ctrl_config(struct nvmet_pci_epf_ctrl *ctrl)
+{
+	struct nvmet_ctrl *tctrl = ctrl->tctrl;
+
+	/* Initialize controller status. */
+	tctrl->csts = 0;
+	ctrl->csts = 0;
+	nvmet_pci_epf_bar_write32(ctrl, NVME_REG_CSTS, ctrl->csts);
+
+	/* Initialize controller configuration and start polling. */
+	tctrl->cc = 0;
+	ctrl->cc = 0;
+	nvmet_pci_epf_bar_write32(ctrl, NVME_REG_CC, ctrl->cc);
+}
+
 static int nvmet_pci_epf_enable_ctrl(struct nvmet_pci_epf_ctrl *ctrl)
 {
 	u64 pci_addr, asq, acq;
@@ -1867,18 +1882,20 @@ static int nvmet_pci_epf_enable_ctrl(struct nvmet_pci_epf_ctrl *ctrl)
 	return 0;
 
 err:
-	ctrl->csts = 0;
+	nvmet_pci_epf_clear_ctrl_config(ctrl);
 	return -EINVAL;
 }
 
-static void nvmet_pci_epf_disable_ctrl(struct nvmet_pci_epf_ctrl *ctrl)
+static void nvmet_pci_epf_disable_ctrl(struct nvmet_pci_epf_ctrl *ctrl,
+				       bool shutdown)
 {
 	int qid;
 
 	if (!ctrl->enabled)
 		return;
 
-	dev_info(ctrl->dev, "Disabling controller\n");
+	dev_info(ctrl->dev, "%s controller\n",
+		 shutdown ? "Shutting down" : "Disabling");
 
 	ctrl->enabled = false;
 	cancel_delayed_work_sync(&ctrl->poll_sqs);
@@ -1895,6 +1912,11 @@ static void nvmet_pci_epf_disable_ctrl(struct nvmet_pci_epf_ctrl *ctrl)
 	nvmet_pci_epf_delete_cq(ctrl->tctrl, 0);
 
 	ctrl->csts &= ~NVME_CSTS_RDY;
+	if (shutdown) {
+		ctrl->csts |= NVME_CSTS_SHST_CMPLT;
+		ctrl->cc &= ~NVME_CC_ENABLE;
+		nvmet_pci_epf_bar_write32(ctrl, NVME_REG_CC, ctrl->cc);
+	}
 }
 
 static void nvmet_pci_epf_poll_cc_work(struct work_struct *work)
@@ -1921,12 +1943,10 @@ static void nvmet_pci_epf_poll_cc_work(struct work_struct *work)
 	}
 
 	if (!nvmet_cc_en(new_cc) && nvmet_cc_en(old_cc))
-		nvmet_pci_epf_disable_ctrl(ctrl);
+		nvmet_pci_epf_disable_ctrl(ctrl, false);
 
-	if (nvmet_cc_shn(new_cc) && !nvmet_cc_shn(old_cc)) {
-		nvmet_pci_epf_disable_ctrl(ctrl);
-		ctrl->csts |= NVME_CSTS_SHST_CMPLT;
-	}
+	if (nvmet_cc_shn(new_cc) && !nvmet_cc_shn(old_cc))
+		nvmet_pci_epf_disable_ctrl(ctrl, true);
 
 	if (!nvmet_cc_shn(new_cc) && nvmet_cc_shn(old_cc))
 		ctrl->csts &= ~NVME_CSTS_SHST_CMPLT;
@@ -1965,16 +1985,10 @@ static void nvmet_pci_epf_init_bar(struct nvmet_pci_epf_ctrl *ctrl)
 	/* Clear Controller Memory Buffer Supported (CMBS). */
 	ctrl->cap &= ~(0x1ULL << 57);
 
-	/* Controller configuration. */
-	ctrl->cc = tctrl->cc & (~NVME_CC_ENABLE);
-
-	/* Controller status. */
-	ctrl->csts = ctrl->tctrl->csts;
-
 	nvmet_pci_epf_bar_write64(ctrl, NVME_REG_CAP, ctrl->cap);
 	nvmet_pci_epf_bar_write32(ctrl, NVME_REG_VS, tctrl->subsys->ver);
-	nvmet_pci_epf_bar_write32(ctrl, NVME_REG_CSTS, ctrl->csts);
-	nvmet_pci_epf_bar_write32(ctrl, NVME_REG_CC, ctrl->cc);
+
+	nvmet_pci_epf_clear_ctrl_config(ctrl);
 }
 
 static int nvmet_pci_epf_create_ctrl(struct nvmet_pci_epf *nvme_epf,
@@ -2079,7 +2093,8 @@ static void nvmet_pci_epf_stop_ctrl(struct nvmet_pci_epf_ctrl *ctrl)
 {
 	cancel_delayed_work_sync(&ctrl->poll_cc);
 
-	nvmet_pci_epf_disable_ctrl(ctrl);
+	nvmet_pci_epf_disable_ctrl(ctrl, false);
+	nvmet_pci_epf_clear_ctrl_config(ctrl);
 }
 
 static void nvmet_pci_epf_destroy_ctrl(struct nvmet_pci_epf_ctrl *ctrl)
-- 
2.49.0

[PATCH 3/3] nvmet: pci-epf: Cleanup link state management

[Damien Le Moal]

Since the link_up boolean field of struct nvmet_pci_epf_ctrl is always
set to true when nvmet_pci_epf_start_ctrl() is called, assign true to
this field in nvmet_pci_epf_start_ctrl(). Conversely, since this field
is set to false when nvmet_pci_epf_stop_ctrl() is called, set this field
to false directly inside that function.

While at it, also add information messages to notify the user of the PCI
link state changes to help troubleshoot any link stability issues
without needing to enable debug messages.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/nvme/target/pci-epf.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
index f18faf407eab..db2da0595423 100644
--- a/drivers/nvme/target/pci-epf.c
+++ b/drivers/nvme/target/pci-epf.c
@@ -2086,11 +2086,18 @@ static int nvmet_pci_epf_create_ctrl(struct nvmet_pci_epf *nvme_epf,
 
 static void nvmet_pci_epf_start_ctrl(struct nvmet_pci_epf_ctrl *ctrl)
 {
+
+	dev_info(ctrl->dev, "PCI link up\n");
+	ctrl->link_up = true;
+
 	schedule_delayed_work(&ctrl->poll_cc, NVMET_PCI_EPF_CC_POLL_INTERVAL);
 }
 
 static void nvmet_pci_epf_stop_ctrl(struct nvmet_pci_epf_ctrl *ctrl)
 {
+	dev_info(ctrl->dev, "PCI link down\n");
+	ctrl->link_up = false;
+
 	cancel_delayed_work_sync(&ctrl->poll_cc);
 
 	nvmet_pci_epf_disable_ctrl(ctrl, false);
@@ -2317,10 +2324,8 @@ static int nvmet_pci_epf_epc_init(struct pci_epf *epf)
 	if (ret)
 		goto out_clear_bar;
 
-	if (!epc_features->linkup_notifier) {
-		ctrl->link_up = true;
+	if (!epc_features->linkup_notifier)
 		nvmet_pci_epf_start_ctrl(&nvme_epf->ctrl);
-	}
 
 	return 0;
 
@@ -2336,7 +2341,6 @@ static void nvmet_pci_epf_epc_deinit(struct pci_epf *epf)
 	struct nvmet_pci_epf *nvme_epf = epf_get_drvdata(epf);
 	struct nvmet_pci_epf_ctrl *ctrl = &nvme_epf->ctrl;
 
-	ctrl->link_up = false;
 	nvmet_pci_epf_destroy_ctrl(ctrl);
 
 	nvmet_pci_epf_deinit_dma(nvme_epf);
@@ -2348,7 +2352,6 @@ static int nvmet_pci_epf_link_up(struct pci_epf *epf)
 	struct nvmet_pci_epf *nvme_epf = epf_get_drvdata(epf);
 	struct nvmet_pci_epf_ctrl *ctrl = &nvme_epf->ctrl;
 
-	ctrl->link_up = true;
 	nvmet_pci_epf_start_ctrl(ctrl);
 
 	return 0;
@@ -2359,7 +2362,6 @@ static int nvmet_pci_epf_link_down(struct pci_epf *epf)
 	struct nvmet_pci_epf *nvme_epf = epf_get_drvdata(epf);
 	struct nvmet_pci_epf_ctrl *ctrl = &nvme_epf->ctrl;
 
-	ctrl->link_up = false;
 	nvmet_pci_epf_stop_ctrl(ctrl);
 
 	return 0;
-- 
2.49.0

Re: [PATCH 0/3] NVMe PCI endpoint target fixes

[Damien Le Moal]

On 4/8/25 11:47 AM, Damien Le Moal wrote:
> Two bug fix patch and one cleanup patch in this series.
> 
> The first patch fixes an issue with completion queue entries
> initialization in the case of failed commands.
> 
> The second patch fixes
> the initialization of the CC register to ensure that we can always
> detect that the host is attempting to enable the controller (with
> CC.EN).
> 
> The last patch cleans up/simplifies the management of the PCI link
> state.

Ping ?

> 
> Damien Le Moal (3):
>   nvmet: pci-epf: Always fully initialize completion entries
>   nvmet: pci-epf: Clear CC and CSTS when disabling the controller
>   nvmet: pci-epf: Cleanup link state management
> 
>  drivers/nvme/target/pci-epf.c | 65 ++++++++++++++++++++++-------------
>  1 file changed, 42 insertions(+), 23 deletions(-)
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/3] NVMe PCI endpoint target fixes

[Christoph Hellwig]

> > The last patch cleans up/simplifies the management of the PCI link
> > state.
> 
> Ping ?

Can I get someone else to look over them for an extra review?
Niklas?

Re: [PATCH 1/3] nvmet: pci-epf: Always fully initialize completion entries

[Keith Busch]

On Tue, Apr 08, 2025 at 11:47:31AM +0900, Damien Le Moal wrote:
> For a command that is normally processed through the command request
> execute() function, the completion entry for the command is initialized
> by __nvmet_req_complete() and nvmet_pci_epf_cq_work() only needs to set
> the status field and the phase of the completion entry before posting
> the entry to the completion queue.
> 
> However, for commands that are failed due to an internal error (e.g. the
> command data buffer allocation fails), the command request execute()
> function is not called and __nvmet_req_complete() is never executed for
> the command, leaving the command completion entry uninitialized. For
> such command failed before calling req->execute(), the host ends up
> seeing completion entries with an invalid submission queue ID and
> command ID.
> 
> Avoid such issue by always fully initilizing a command completion entry
> in nvmet_pci_epf_cq_work(), setting the entry submission queue ID and
> command ID.

Looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH 2/3] nvmet: pci-epf: Clear CC and CSTS when disabling the controller

[Keith Busch]

On Tue, Apr 08, 2025 at 11:47:32AM +0900, Damien Le Moal wrote:
> @@ -1895,6 +1912,11 @@ static void nvmet_pci_epf_disable_ctrl(struct nvmet_pci_epf_ctrl *ctrl)
>  	nvmet_pci_epf_delete_cq(ctrl->tctrl, 0);
>  
>  	ctrl->csts &= ~NVME_CSTS_RDY;
> +	if (shutdown) {
> +		ctrl->csts |= NVME_CSTS_SHST_CMPLT;
> +		ctrl->cc &= ~NVME_CC_ENABLE;
> +		nvmet_pci_epf_bar_write32(ctrl, NVME_REG_CC, ctrl->cc);
> +	}
>  }

I think this is probably okay, but I don't know if it's necessary to be
messing with CC.EN that the host didn't request. The qemu emulated nvme
doesn't do this, at least. But it looks like that would all work out in
the end anyway, so again, I think it's fine.

Re: [PATCH 3/3] nvmet: pci-epf: Cleanup link state management

[Keith Busch]

On Tue, Apr 08, 2025 at 11:47:33AM +0900, Damien Le Moal wrote:
> Since the link_up boolean field of struct nvmet_pci_epf_ctrl is always
> set to true when nvmet_pci_epf_start_ctrl() is called, assign true to
> this field in nvmet_pci_epf_start_ctrl(). Conversely, since this field
> is set to false when nvmet_pci_epf_stop_ctrl() is called, set this field
> to false directly inside that function.
> 
> While at it, also add information messages to notify the user of the PCI
> link state changes to help troubleshoot any link stability issues
> without needing to enable debug messages.

Looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH 1/3] nvmet: pci-epf: Always fully initialize completion entries

[Christoph Hellwig]

On Tue, Apr 08, 2025 at 11:47:31AM +0900, Damien Le Moal wrote:
> @@ -1763,6 +1763,8 @@ static void nvmet_pci_epf_cq_work(struct work_struct *work)
>  
>  		/* Post the IOD completion entry. */
>  		cqe = &iod->cqe;
> +		cqe->sq_id = cpu_to_le16(iod->sq->qid);
> +		cqe->command_id = iod->cmd.common.command_id;

This could use a comment explaining why we are doing this seemingly
duplicate work here.

Otherwise this looks good to me.

Re: [PATCH 1/3] nvmet: pci-epf: Always fully initialize completion entries

[Niklas Cassel]

On Tue, Apr 08, 2025 at 11:47:31AM +0900, Damien Le Moal wrote:
> For a command that is normally processed through the command request
> execute() function, the completion entry for the command is initialized
> by __nvmet_req_complete() and nvmet_pci_epf_cq_work() only needs to set
> the status field and the phase of the completion entry before posting
> the entry to the completion queue.
> 
> However, for commands that are failed due to an internal error (e.g. the
> command data buffer allocation fails), the command request execute()
> function is not called and __nvmet_req_complete() is never executed for
> the command, leaving the command completion entry uninitialized. For
> such command failed before calling req->execute(), the host ends up
> seeing completion entries with an invalid submission queue ID and
> command ID.
> 
> Avoid such issue by always fully initilizing a command completion entry
> in nvmet_pci_epf_cq_work(), setting the entry submission queue ID and
> command ID.
> 
> Fixes: 0faa0fe6f90e ("nvmet: New NVMe PCI endpoint function target driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/nvme/target/pci-epf.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
> index 51c27b32248d..f6b22ef4c267 100644
> --- a/drivers/nvme/target/pci-epf.c
> +++ b/drivers/nvme/target/pci-epf.c
> @@ -1763,6 +1763,8 @@ static void nvmet_pci_epf_cq_work(struct work_struct *work)
>  
>  		/* Post the IOD completion entry. */
>  		cqe = &iod->cqe;
> +		cqe->sq_id = cpu_to_le16(iod->sq->qid);
> +		cqe->command_id = iod->cmd.common.command_id;
>  		cqe->status = cpu_to_le16((iod->status << 1) | cq->phase);

Looking at e.g. __nvmet_req_complete() and __nvmet_fc_fcp_nvme_cmd_done()
in addition to setting sq_id, command_id, and status, they also set:
cqe->sq_head.

(__nvmet_req_complete() -> nvmet_update_sq_head() -> req->cqe->sq_head = ...)

Should we perhaps set sq_head too?


Kind regards,
Niklas

Re: [PATCH 2/3] nvmet: pci-epf: Clear CC and CSTS when disabling the controller

[Niklas Cassel]

On Tue, Apr 08, 2025 at 11:47:32AM +0900, Damien Le Moal wrote:
> When a host shuts down the controller when shutting down but does so
> without first disabling the controller, the enable bit remains set in
> the controller configuration register. When the host restarts and
> attempts to enable the controller again, the
> nvmet_pci_epf_poll_cc_work() function is unable to detect the change
> from 0 to 1 of the enable bit, and thus the controller is not enabled
> again, which result in a device scan timeout on the host. This problem
> also occurs if the host shuts down uncleanly or if the PCIe link goes
> down: as the CC.EN value is not reset, the controller is not enabled
> again when the host restarts.
> 
> Fix this by introducing the function nvmet_pci_epf_clear_ctrl_config()
> to clear the CC and CSTS registers of the controller when the PCIe link
> is lost (nvmet_pci_epf_stop_ctrl() function), and when starting the

s/, and when/, or when/


> controller fails (nvmet_pci_epf_enable_ctrl() fails). Also use this
> function in nvmet_pci_epf_init_bar() to simplify the initialization of
> the CC and CSTS registers.
> 
> Furthermore, modify the function nvmet_pci_epf_disable_ctrl() to clear
> the CC.EN bit and write this updated value to the BAR register when the
> controller is shutdown by the host, to ensure that upon restart, we can
> detect the host setting CC.EN.
> 
> Fixes: 0faa0fe6f90e ("nvmet: New NVMe PCI endpoint function target driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/nvme/target/pci-epf.c | 49 +++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
> index f6b22ef4c267..f18faf407eab 100644
> --- a/drivers/nvme/target/pci-epf.c
> +++ b/drivers/nvme/target/pci-epf.c
> @@ -1802,6 +1802,21 @@ static void nvmet_pci_epf_cq_work(struct work_struct *work)
>  				   NVMET_PCI_EPF_CQ_RETRY_INTERVAL);
>  }
>  
> +static void nvmet_pci_epf_clear_ctrl_config(struct nvmet_pci_epf_ctrl *ctrl)
> +{
> +	struct nvmet_ctrl *tctrl = ctrl->tctrl;
> +
> +	/* Initialize controller status. */
> +	tctrl->csts = 0;
> +	ctrl->csts = 0;

This can be written on one line:
tctrl->csts = ctrl->csts = 0;


> +	nvmet_pci_epf_bar_write32(ctrl, NVME_REG_CSTS, ctrl->csts);
> +
> +	/* Initialize controller configuration and start polling. */
> +	tctrl->cc = 0;
> +	ctrl->cc = 0;

Same here.


Otherwise, this looks good to me:
Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH 3/3] nvmet: pci-epf: Cleanup link state management

[Niklas Cassel]

On Tue, Apr 08, 2025 at 11:47:33AM +0900, Damien Le Moal wrote:
> Since the link_up boolean field of struct nvmet_pci_epf_ctrl is always
> set to true when nvmet_pci_epf_start_ctrl() is called, assign true to
> this field in nvmet_pci_epf_start_ctrl(). Conversely, since this field
> is set to false when nvmet_pci_epf_stop_ctrl() is called, set this field
> to false directly inside that function.
> 
> While at it, also add information messages to notify the user of the PCI
> link state changes to help troubleshoot any link stability issues
> without needing to enable debug messages.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH 2/3] nvmet: pci-epf: Clear CC and CSTS when disabling the controller

[Damien Le Moal]

On 4/10/25 17:34, Keith Busch wrote:
> On Tue, Apr 08, 2025 at 11:47:32AM +0900, Damien Le Moal wrote:
>> @@ -1895,6 +1912,11 @@ static void nvmet_pci_epf_disable_ctrl(struct nvmet_pci_epf_ctrl *ctrl)
>>  	nvmet_pci_epf_delete_cq(ctrl->tctrl, 0);
>>  
>>  	ctrl->csts &= ~NVME_CSTS_RDY;
>> +	if (shutdown) {
>> +		ctrl->csts |= NVME_CSTS_SHST_CMPLT;
>> +		ctrl->cc &= ~NVME_CC_ENABLE;
>> +		nvmet_pci_epf_bar_write32(ctrl, NVME_REG_CC, ctrl->cc);
>> +	}
>>  }
> 
> I think this is probably okay, but I don't know if it's necessary to be
> messing with CC.EN that the host didn't request. The qemu emulated nvme
> doesn't do this, at least. But it looks like that would all work out in
> the end anyway, so again, I think it's fine.

Maybe qemu clears/resets all the registers of the BAR when it does a shutdown ?
I did not check the code. But here, if we do not clear EN, *and* the host does
not first disable the controller before trying to re-enable it (like the linux
host pci driver does), after the shutdown, we fail to detect that the host set
EN again since we can only detect that by polling and comparing to the previous
value.

I also thought about using ctrl->enabled for determining what needs to be done,
but then again, if we do not clear EN from the register, the next CC poll loop
will see EN set and think that it is the host trying to re-enable the controller
and we will re-enable right away.

I think that clearing EN is not necessary with qemu because there is no polling,
since (I think) that qemu nvme driver will be looking at CC.EN only and only
when the guest touches it (as that will trigger a guest exit into the
hypervisor). So the qemu nvme driver always sees the newest value of CC, not the
old one like we do with the endpoint.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/3] nvmet: pci-epf: Clear CC and CSTS when disabling the controller

[Damien Le Moal]

On 4/10/25 20:54, Niklas Cassel wrote:
>> +static void nvmet_pci_epf_clear_ctrl_config(struct nvmet_pci_epf_ctrl *ctrl)
>> +{
>> +	struct nvmet_ctrl *tctrl = ctrl->tctrl;
>> +
>> +	/* Initialize controller status. */
>> +	tctrl->csts = 0;
>> +	ctrl->csts = 0;
> 
> This can be written on one line:
> tctrl->csts = ctrl->csts = 0;

I am aware of this, but really dislike this style as I think it makes the code
less readable. So unless the nvme maintainers insist on this change, I prefer
leaving this as I wrote it.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/3] nvmet: pci-epf: Clear CC and CSTS when disabling the controller

[Keith Busch]

On Fri, Apr 11, 2025 at 09:32:53AM +0900, Damien Le Moal wrote:
> On 4/10/25 17:34, Keith Busch wrote:
> > On Tue, Apr 08, 2025 at 11:47:32AM +0900, Damien Le Moal wrote:
> >> @@ -1895,6 +1912,11 @@ static void nvmet_pci_epf_disable_ctrl(struct nvmet_pci_epf_ctrl *ctrl)
> >>  	nvmet_pci_epf_delete_cq(ctrl->tctrl, 0);
> >>  
> >>  	ctrl->csts &= ~NVME_CSTS_RDY;
> >> +	if (shutdown) {
> >> +		ctrl->csts |= NVME_CSTS_SHST_CMPLT;
> >> +		ctrl->cc &= ~NVME_CC_ENABLE;
> >> +		nvmet_pci_epf_bar_write32(ctrl, NVME_REG_CC, ctrl->cc);
> >> +	}
> >>  }
> > 
> > I think this is probably okay, but I don't know if it's necessary to be
> > messing with CC.EN that the host didn't request. The qemu emulated nvme
> > doesn't do this, at least. But it looks like that would all work out in
> > the end anyway, so again, I think it's fine.
> 
> Maybe qemu clears/resets all the registers of the BAR when it does a shutdown ?
> I did not check the code. But here, if we do not clear EN, *and* the host does
> not first disable the controller before trying to re-enable it (like the linux
> host pci driver does), after the shutdown, we fail to detect that the host set
> EN again since we can only detect that by polling and comparing to the previous
> value.

The spec has some text explaining the host needs to handle a shutdown
controller with CC.EN set in section 3.6.1:

  To start executing commands on the controller after that controller
  reports controller shutdown processing complete (...) utilizing the
  CC.EN bit:

    o if the CC.EN bit is set to `1´, then a Controller Level Reset is
      required to clear the CC.EN bit to `0´ on that controller and the
      CC.EN bit is subsequently required to be set to `1´ as part of the
      initialization sequence 


So I guess some hosts just aren't handling this? No biggie, linux will
react accordingly either way.

I didn't find anything that requires a shutdown controller retain CC.EN,
so clearing it on the condition is probably fine. I never really fully
understood why a disabled vs shutdown-ready controller needed such
distinctions anyway.

Re: [PATCH 1/3] nvmet: pci-epf: Always fully initialize completion entries

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 3/3] nvmet: pci-epf: Cleanup link state management

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 2/3] nvmet: pci-epf: Clear CC and CSTS when disabling the controller

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>