Clarification regarding "nvme discover" and setting IOSQES/IOCQES

Clarification regarding "nvme discover" and setting IOSQES/IOCQES

[Belanger, Martin]

I'm using "nvme discover" to retrieve discovery log pages. And I'm using nvmet-tcp as the target.

From what I understand, "nvme discover" is supposed to set up a connection to a "Discovery Controller (DC)". During the exchange between the Host and the Target, there is a time when the Host must set CC.EN=1 to "enable" the DC. I'm using Wireshark to analyse the packets sent by the Host to the DC. I can see that when it's time to set CC.EN=1, the Host also sets two additional parameters: IOCQES=4 and IOSQES=6. 

In the NVME Base Specification, Figure 78 - Controller Configuration, these fields are defined as parameters to configure an "I/O Controller (IOC)" and not to configure a "Discovery Controller (DC)". In fact, it says that "If the controller does not support I/O queues, then this field shall be read-only with a value of 0h". 

The way I understand it, DCs do not support I/O queues. So why set IOCQES and IOSQES to non-zero values?

Thanks,
Martin
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: Clarification regarding "nvme discover" and setting IOSQES/IOCQES

[Sagi Grimberg]

> I'm using "nvme discover" to retrieve discovery log pages. And I'm using nvmet-tcp as the target.
> 
>  From what I understand, "nvme discover" is supposed to set up a connection to a "Discovery Controller (DC)". During the exchange between the Host and the Target, there is a time when the Host must set CC.EN=1 to "enable" the DC. I'm using Wireshark to analyse the packets sent by the Host to the DC. I can see that when it's time to set CC.EN=1, the Host also sets two additional parameters: IOCQES=4 and IOSQES=6.
> 
> In the NVME Base Specification, Figure 78 - Controller Configuration, these fields are defined as parameters to configure an "I/O Controller (IOC)" and not to configure a "Discovery Controller (DC)". In fact, it says that "If the controller does not support I/O queues, then this field shall be read-only with a value of 0h".
> 
> The way I understand it, DCs do not support I/O queues. So why set IOCQES and IOSQES to non-zero values?

Its probably an oversight, and the fact that the existing discovery
controllers simply allowed it.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: Clarification regarding "nvme discover" and setting IOSQES/IOCQES

[Sagi Grimberg]

> Hi Sagi,
> 
> Yes. The Discovery Controller currently allows it. The problem, however, 
> is that the DC seems to be expecting these values to be non-zero.
> 
> I tried setting IOCQES=0 and IOSQES=0, which the DC allows (i.e. Prop 
> Set return status=0). However, when I follow this by a "Property Get - 
> Controller Status", the Status now shows "Controller Fatal Status (CFS)" 
> (see Base specs - Figure 79).

Yes, the oversight extends to the target that expects it (shared code
with I/O controllers).

Does this fix your issue?
--
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 546a10407385..a7578c601727 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2443,6 +2443,11 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl)
  }
  EXPORT_SYMBOL_GPL(nvme_disable_ctrl);

+static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
+{
+       return ctrl->opts && ctrl->opts->discovery_nqn;
+}
+
  int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
  {
         unsigned dev_page_min;
@@ -2468,7 +2473,8 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
                 ctrl->ctrl_config = NVME_CC_CSS_NVM;
         ctrl->ctrl_config |= (NVME_CTRL_PAGE_SHIFT - 12) << 
NVME_CC_MPS_SHIFT;
         ctrl->ctrl_config |= NVME_CC_AMS_RR | NVME_CC_SHN_NONE;
-       ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
+       if (!nvme_discovery_ctrl(ctrl))
+               ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
         ctrl->ctrl_config |= NVME_CC_ENABLE;

         ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
@@ -2888,11 +2894,6 @@ static const struct attribute_group 
*nvme_subsys_attrs_groups[] = {
         NULL,
  };

-static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
-{
-       return ctrl->opts && ctrl->opts->discovery_nqn;
-}
-
  static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
                 struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
  {
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 8ce4d59cc9e7..baa26a65d83d 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1107,9 +1107,14 @@ static void nvmet_start_ctrl(struct nvmet_ctrl *ctrl)
  {
         lockdep_assert_held(&ctrl->lock);

-       if (nvmet_cc_iosqes(ctrl->cc) != NVME_NVM_IOSQES ||
-           nvmet_cc_iocqes(ctrl->cc) != NVME_NVM_IOCQES ||
-           nvmet_cc_mps(ctrl->cc) != 0 ||
+       if (!ctrl->subsys->type == NVME_NQN_DISC &&
+           (nvmet_cc_iosqes(ctrl->cc) != NVME_NVM_IOSQES ||
+            nvmet_cc_iocqes(ctrl->cc) != NVME_NVM_IOCQES)) {
+               ctrl->csts = NVME_CSTS_CFS;
+               return;
+       }
+
+       if (nvmet_cc_mps(ctrl->cc) != 0 ||
             nvmet_cc_ams(ctrl->cc) != 0 ||
             nvmet_cc_css(ctrl->cc) != 0) {
                 ctrl->csts = NVME_CSTS_CFS;
--

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: Clarification regarding "nvme discover" and setting IOSQES/IOCQES

[Keith Busch]

On Mon, Feb 08, 2021 at 10:59:43AM -0800, Sagi Grimberg wrote:
> 
> > Hi Sagi,
> > 
> > Yes. The Discovery Controller currently allows it. The problem, however,
> > is that the DC seems to be expecting these values to be non-zero.
> > 
> > I tried setting IOCQES=0 and IOSQES=0, which the DC allows (i.e. Prop
> > Set return status=0). However, when I follow this by a "Property Get -
> > Controller Status", the Status now shows "Controller Fatal Status (CFS)"
> > (see Base specs - Figure 79).
> 
> Yes, the oversight extends to the target that expects it (shared code
> with I/O controllers).
> 
> Does this fix your issue?

Seems like the Identify Controller SQES and CQES fields ought to be
updated too, but the spec isn't clear on this. Feels like ECN
material...

> --
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 546a10407385..a7578c601727 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2443,6 +2443,11 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl)
>  }
>  EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
> 
> +static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
> +{
> +       return ctrl->opts && ctrl->opts->discovery_nqn;
> +}
> +
>  int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
>  {
>         unsigned dev_page_min;
> @@ -2468,7 +2473,8 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
>                 ctrl->ctrl_config = NVME_CC_CSS_NVM;
>         ctrl->ctrl_config |= (NVME_CTRL_PAGE_SHIFT - 12) <<
> NVME_CC_MPS_SHIFT;
>         ctrl->ctrl_config |= NVME_CC_AMS_RR | NVME_CC_SHN_NONE;
> -       ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
> +       if (!nvme_discovery_ctrl(ctrl))
> +               ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
>         ctrl->ctrl_config |= NVME_CC_ENABLE;
> 
>         ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
> @@ -2888,11 +2894,6 @@ static const struct attribute_group
> *nvme_subsys_attrs_groups[] = {
>         NULL,
>  };
> 
> -static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
> -{
> -       return ctrl->opts && ctrl->opts->discovery_nqn;
> -}
> -
>  static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
>                 struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>  {
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 8ce4d59cc9e7..baa26a65d83d 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -1107,9 +1107,14 @@ static void nvmet_start_ctrl(struct nvmet_ctrl *ctrl)
>  {
>         lockdep_assert_held(&ctrl->lock);
> 
> -       if (nvmet_cc_iosqes(ctrl->cc) != NVME_NVM_IOSQES ||
> -           nvmet_cc_iocqes(ctrl->cc) != NVME_NVM_IOCQES ||
> -           nvmet_cc_mps(ctrl->cc) != 0 ||
> +       if (!ctrl->subsys->type == NVME_NQN_DISC &&
> +           (nvmet_cc_iosqes(ctrl->cc) != NVME_NVM_IOSQES ||
> +            nvmet_cc_iocqes(ctrl->cc) != NVME_NVM_IOCQES)) {
> +               ctrl->csts = NVME_CSTS_CFS;
> +               return;
> +       }
> +
> +       if (nvmet_cc_mps(ctrl->cc) != 0 ||
>             nvmet_cc_ams(ctrl->cc) != 0 ||
>             nvmet_cc_css(ctrl->cc) != 0) {
>                 ctrl->csts = NVME_CSTS_CFS;
> --
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: Clarification regarding "nvme discover" and setting IOSQES/IOCQES

[Sagi Grimberg]

>>> Hi Sagi,
>>>
>>> Yes. The Discovery Controller currently allows it. The problem, however,
>>> is that the DC seems to be expecting these values to be non-zero.
>>>
>>> I tried setting IOCQES=0 and IOSQES=0, which the DC allows (i.e. Prop
>>> Set return status=0). However, when I follow this by a "Property Get -
>>> Controller Status", the Status now shows "Controller Fatal Status (CFS)"
>>> (see Base specs - Figure 79).
>>
>> Yes, the oversight extends to the target that expects it (shared code
>> with I/O controllers).
>>
>> Does this fix your issue?
> 
> Seems like the Identify Controller SQES and CQES fields ought to be
> updated too, but the spec isn't clear on this. Feels like ECN
> material...

Yes, we should start with this patch though right?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: Clarification regarding "nvme discover" and setting IOSQES/IOCQES

[Keith Busch]

On Mon, Feb 08, 2021 at 03:01:28PM -0800, Sagi Grimberg wrote:
> 
> > > > Hi Sagi,
> > > > 
> > > > Yes. The Discovery Controller currently allows it. The problem, however,
> > > > is that the DC seems to be expecting these values to be non-zero.
> > > > 
> > > > I tried setting IOCQES=0 and IOSQES=0, which the DC allows (i.e. Prop
> > > > Set return status=0). However, when I follow this by a "Property Get -
> > > > Controller Status", the Status now shows "Controller Fatal Status (CFS)"
> > > > (see Base specs - Figure 79).
> > > 
> > > Yes, the oversight extends to the target that expects it (shared code
> > > with I/O controllers).
> > > 
> > > Does this fix your issue?
> > 
> > Seems like the Identify Controller SQES and CQES fields ought to be
> > updated too, but the spec isn't clear on this. Feels like ECN
> > material...
> 
> Yes, we should start with this patch though right?

Yep, your patch looks good.

I was just thinking a generic implementation could align the CC settings with
the ID_CTRL values rather than checking for a discovery controller, but I
noticed the spec didn't provide us that opportunity. That was actually a
misguided thought anyway since we don't even have ID_CTRL data on the initial
CC setting! :)

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme