[v2,06/20] nvme: add support for the abort command
diff mbox series

Message ID 20191015103900.313928-7-its@irrelevant.dk
State New
Headers show
Series
  • nvme: support NVMe v1.3d, SGLs and multiple namespaces
Related show

Commit Message

Klaus Birkelund Oct. 15, 2019, 10:38 a.m. UTC
Required for compliance with NVMe revision 1.2.1. See NVM Express 1.2.1,
Section 5.1 ("Abort command").

The Abort command is a best effort command; for now, the device always
fails to abort the given command.

Signed-off-by: Klaus Jensen <klaus.jensen@cnexlabs.com>
---
 hw/block/nvme.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Beata Michalska Nov. 12, 2019, 3:04 p.m. UTC | #1
Hi Klaus

On Tue, 15 Oct 2019 at 11:41, Klaus Jensen <its@irrelevant.dk> wrote:
>
> Required for compliance with NVMe revision 1.2.1. See NVM Express 1.2.1,
> Section 5.1 ("Abort command").
>
> The Abort command is a best effort command; for now, the device always
> fails to abort the given command.
>
> Signed-off-by: Klaus Jensen <klaus.jensen@cnexlabs.com>
> ---
>  hw/block/nvme.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index daa2367b0863..84e4f2ea7a15 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -741,6 +741,18 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
>      }
>  }
>
> +static uint16_t nvme_abort(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> +{
> +    uint16_t sqid = le32_to_cpu(cmd->cdw10) & 0xffff;
> +
> +    req->cqe.result = 1;
> +    if (nvme_check_sqid(n, sqid)) {
> +        return NVME_INVALID_FIELD | NVME_DNR;
> +    }
> +
Shouldn't we validate the CID as well ?

> +    return NVME_SUCCESS;
> +}
> +
>  static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
>  {
>      trace_nvme_setfeat_timestamp(ts);
> @@ -859,6 +871,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>          trace_nvme_err_invalid_setfeat(dw10);
>          return NVME_INVALID_FIELD | NVME_DNR;
>      }
> +
>      return NVME_SUCCESS;
>  }
>
> @@ -875,6 +888,8 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>          return nvme_create_cq(n, cmd);
>      case NVME_ADM_CMD_IDENTIFY:
>          return nvme_identify(n, cmd);
> +    case NVME_ADM_CMD_ABORT:
> +        return nvme_abort(n, cmd, req);
>      case NVME_ADM_CMD_SET_FEATURES:
>          return nvme_set_feature(n, cmd, req);
>      case NVME_ADM_CMD_GET_FEATURES:
> @@ -1388,6 +1403,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>      id->ieee[2] = 0xb3;
>      id->ver = cpu_to_le32(0x00010201);
>      id->oacs = cpu_to_le16(0);
> +    id->acl = 3;
So we are setting the max number of concurrent commands
but there is no logic to enforce that and wrap up with the
status suggested by specification.

BR


Beata
>      id->frmw = 7 << 1;
>      id->lpa = 1 << 0;
>      id->sqes = (0x6 << 4) | 0x6;
> --
> 2.23.0
>
>
Klaus Birkelund Nov. 13, 2019, 6:12 a.m. UTC | #2
On Tue, Nov 12, 2019 at 03:04:38PM +0000, Beata Michalska wrote:
> Hi Klaus
> 

Hi Beata,

Thank you very much for your thorough reviews! I'll start going through
them one by one :) You might have seen that I've posted a v3, but I will
make sure to consolidate between v2 and v3!

> On Tue, 15 Oct 2019 at 11:41, Klaus Jensen <its@irrelevant.dk> wrote:
> >
> > Required for compliance with NVMe revision 1.2.1. See NVM Express 1.2.1,
> > Section 5.1 ("Abort command").
> >
> > The Abort command is a best effort command; for now, the device always
> > fails to abort the given command.
> >
> > Signed-off-by: Klaus Jensen <klaus.jensen@cnexlabs.com>
> > ---
> >  hw/block/nvme.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index daa2367b0863..84e4f2ea7a15 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -741,6 +741,18 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
> >      }
> >  }
> >
> > +static uint16_t nvme_abort(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > +{
> > +    uint16_t sqid = le32_to_cpu(cmd->cdw10) & 0xffff;
> > +
> > +    req->cqe.result = 1;
> > +    if (nvme_check_sqid(n, sqid)) {
> > +        return NVME_INVALID_FIELD | NVME_DNR;
> > +    }
> > +
> Shouldn't we validate the CID as well ?
> 

According to the specification it is "implementation specific if/when a
controller chooses to complete the command when the command to abort is
not found".

I'm interpreting this to mean that, yes, an invalid command identifier
could be given in the command, but this implementation does not care
about that.

I still think the controller should check the validity of the submission
queue identifier though. It is a general invariant that the sqid should
be valid.

> > +    return NVME_SUCCESS;
> > +}
> > +
> >  static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
> >  {
> >      trace_nvme_setfeat_timestamp(ts);
> > @@ -859,6 +871,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> >          trace_nvme_err_invalid_setfeat(dw10);
> >          return NVME_INVALID_FIELD | NVME_DNR;
> >      }
> > +
> >      return NVME_SUCCESS;
> >  }
> >
> > @@ -875,6 +888,8 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> >          return nvme_create_cq(n, cmd);
> >      case NVME_ADM_CMD_IDENTIFY:
> >          return nvme_identify(n, cmd);
> > +    case NVME_ADM_CMD_ABORT:
> > +        return nvme_abort(n, cmd, req);
> >      case NVME_ADM_CMD_SET_FEATURES:
> >          return nvme_set_feature(n, cmd, req);
> >      case NVME_ADM_CMD_GET_FEATURES:
> > @@ -1388,6 +1403,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> >      id->ieee[2] = 0xb3;
> >      id->ver = cpu_to_le32(0x00010201);
> >      id->oacs = cpu_to_le16(0);
> > +    id->acl = 3;
> So we are setting the max number of concurrent commands
> but there is no logic to enforce that and wrap up with the
> status suggested by specification.
> 

That is true, but because the controller always completes the Abort
command immediately this cannot happen. If the controller did try to
abort executing commands, the Abort command would need to linger in the
controller state until a completion queue entry is posted for the
command to be aborted before the completion queue entry can be posted
for the Abort command. This takes up resources in the controller and is
the reason for the Abort Command Limit.

You could argue that we should set ACL to 0 then, but the specification
recommends a value of 3 and I do not see any harm in conveying a
"reasonable", though inconsequential, value.
Beata Michalska Nov. 15, 2019, 11:56 a.m. UTC | #3
Hi Klaus,

On Wed, 13 Nov 2019 at 06:12, Klaus Birkelund <its@irrelevant.dk> wrote:
>
> On Tue, Nov 12, 2019 at 03:04:38PM +0000, Beata Michalska wrote:
> > Hi Klaus
> >
>
> Hi Beata,
>
> Thank you very much for your thorough reviews! I'll start going through
> them one by one :) You might have seen that I've posted a v3, but I will
> make sure to consolidate between v2 and v3!
>
> > On Tue, 15 Oct 2019 at 11:41, Klaus Jensen <its@irrelevant.dk> wrote:
> > >
> > > Required for compliance with NVMe revision 1.2.1. See NVM Express 1.2.1,
> > > Section 5.1 ("Abort command").
> > >
> > > The Abort command is a best effort command; for now, the device always
> > > fails to abort the given command.
> > >
> > > Signed-off-by: Klaus Jensen <klaus.jensen@cnexlabs.com>
> > > ---
> > >  hw/block/nvme.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index daa2367b0863..84e4f2ea7a15 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -741,6 +741,18 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
> > >      }
> > >  }
> > >
> > > +static uint16_t nvme_abort(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > > +{
> > > +    uint16_t sqid = le32_to_cpu(cmd->cdw10) & 0xffff;
> > > +
> > > +    req->cqe.result = 1;
> > > +    if (nvme_check_sqid(n, sqid)) {
> > > +        return NVME_INVALID_FIELD | NVME_DNR;
> > > +    }
> > > +
> > Shouldn't we validate the CID as well ?
> >
>
> According to the specification it is "implementation specific if/when a
> controller chooses to complete the command when the command to abort is
> not found".
>
> I'm interpreting this to mean that, yes, an invalid command identifier
> could be given in the command, but this implementation does not care
> about that.
>
> I still think the controller should check the validity of the submission
> queue identifier though. It is a general invariant that the sqid should
> be valid.
>
> > > +    return NVME_SUCCESS;
> > > +}
> > > +
> > >  static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
> > >  {
> > >      trace_nvme_setfeat_timestamp(ts);
> > > @@ -859,6 +871,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > >          trace_nvme_err_invalid_setfeat(dw10);
> > >          return NVME_INVALID_FIELD | NVME_DNR;
> > >      }
> > > +
> > >      return NVME_SUCCESS;
> > >  }
> > >
> > > @@ -875,6 +888,8 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > >          return nvme_create_cq(n, cmd);
> > >      case NVME_ADM_CMD_IDENTIFY:
> > >          return nvme_identify(n, cmd);
> > > +    case NVME_ADM_CMD_ABORT:
> > > +        return nvme_abort(n, cmd, req);
> > >      case NVME_ADM_CMD_SET_FEATURES:
> > >          return nvme_set_feature(n, cmd, req);
> > >      case NVME_ADM_CMD_GET_FEATURES:
> > > @@ -1388,6 +1403,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> > >      id->ieee[2] = 0xb3;
> > >      id->ver = cpu_to_le32(0x00010201);
> > >      id->oacs = cpu_to_le16(0);
> > > +    id->acl = 3;
> > So we are setting the max number of concurrent commands
> > but there is no logic to enforce that and wrap up with the
> > status suggested by specification.
> >
>
> That is true, but because the controller always completes the Abort
> command immediately this cannot happen. If the controller did try to
> abort executing commands, the Abort command would need to linger in the
> controller state until a completion queue entry is posted for the
> command to be aborted before the completion queue entry can be posted
> for the Abort command. This takes up resources in the controller and is
> the reason for the Abort Command Limit.
>
> You could argue that we should set ACL to 0 then, but the specification
> recommends a value of 3 and I do not see any harm in conveying a
> "reasonable", though inconsequential, value.

Could we  potentially add some comment describing the above ?

BR
Beata
Klaus Birkelund Nov. 18, 2019, 8:49 a.m. UTC | #4
On Fri, Nov 15, 2019 at 11:56:00AM +0000, Beata Michalska wrote:
> Hi Klaus,
> 
> On Wed, 13 Nov 2019 at 06:12, Klaus Birkelund <its@irrelevant.dk> wrote:
> >
> > On Tue, Nov 12, 2019 at 03:04:38PM +0000, Beata Michalska wrote:
> > > Hi Klaus
> > >
> >
> > Hi Beata,
> >
> > Thank you very much for your thorough reviews! I'll start going through
> > them one by one :) You might have seen that I've posted a v3, but I will
> > make sure to consolidate between v2 and v3!
> >
> > > On Tue, 15 Oct 2019 at 11:41, Klaus Jensen <its@irrelevant.dk> wrote:
> > > >
> > > > Required for compliance with NVMe revision 1.2.1. See NVM Express 1.2.1,
> > > > Section 5.1 ("Abort command").
> > > >
> > > > The Abort command is a best effort command; for now, the device always
> > > > fails to abort the given command.
> > > >
> > > > Signed-off-by: Klaus Jensen <klaus.jensen@cnexlabs.com>
> > > > ---
> > > >  hw/block/nvme.c | 16 ++++++++++++++++
> > > >  1 file changed, 16 insertions(+)
> > > >
> > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > > index daa2367b0863..84e4f2ea7a15 100644
> > > > --- a/hw/block/nvme.c
> > > > +++ b/hw/block/nvme.c
> > > > @@ -741,6 +741,18 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
> > > >      }
> > > >  }
> > > >
> > > > +static uint16_t nvme_abort(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > > > +{
> > > > +    uint16_t sqid = le32_to_cpu(cmd->cdw10) & 0xffff;
> > > > +
> > > > +    req->cqe.result = 1;
> > > > +    if (nvme_check_sqid(n, sqid)) {
> > > > +        return NVME_INVALID_FIELD | NVME_DNR;
> > > > +    }
> > > > +
> > > Shouldn't we validate the CID as well ?
> > >
> >
> > According to the specification it is "implementation specific if/when a
> > controller chooses to complete the command when the command to abort is
> > not found".
> >
> > I'm interpreting this to mean that, yes, an invalid command identifier
> > could be given in the command, but this implementation does not care
> > about that.
> >
> > I still think the controller should check the validity of the submission
> > queue identifier though. It is a general invariant that the sqid should
> > be valid.
> >
> > > > +    return NVME_SUCCESS;
> > > > +}
> > > > +
> > > >  static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
> > > >  {
> > > >      trace_nvme_setfeat_timestamp(ts);
> > > > @@ -859,6 +871,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > > >          trace_nvme_err_invalid_setfeat(dw10);
> > > >          return NVME_INVALID_FIELD | NVME_DNR;
> > > >      }
> > > > +
> > > >      return NVME_SUCCESS;
> > > >  }
> > > >
> > > > @@ -875,6 +888,8 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > > >          return nvme_create_cq(n, cmd);
> > > >      case NVME_ADM_CMD_IDENTIFY:
> > > >          return nvme_identify(n, cmd);
> > > > +    case NVME_ADM_CMD_ABORT:
> > > > +        return nvme_abort(n, cmd, req);
> > > >      case NVME_ADM_CMD_SET_FEATURES:
> > > >          return nvme_set_feature(n, cmd, req);
> > > >      case NVME_ADM_CMD_GET_FEATURES:
> > > > @@ -1388,6 +1403,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> > > >      id->ieee[2] = 0xb3;
> > > >      id->ver = cpu_to_le32(0x00010201);
> > > >      id->oacs = cpu_to_le16(0);
> > > > +    id->acl = 3;
> > > So we are setting the max number of concurrent commands
> > > but there is no logic to enforce that and wrap up with the
> > > status suggested by specification.
> > >
> >
> > That is true, but because the controller always completes the Abort
> > command immediately this cannot happen. If the controller did try to
> > abort executing commands, the Abort command would need to linger in the
> > controller state until a completion queue entry is posted for the
> > command to be aborted before the completion queue entry can be posted
> > for the Abort command. This takes up resources in the controller and is
> > the reason for the Abort Command Limit.
> >
> > You could argue that we should set ACL to 0 then, but the specification
> > recommends a value of 3 and I do not see any harm in conveying a
> > "reasonable", though inconsequential, value.
> 
> Could we  potentially add some comment describing the above ?
> 

Yes, absolutely! :)


Klaus

Patch
diff mbox series

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index daa2367b0863..84e4f2ea7a15 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -741,6 +741,18 @@  static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
     }
 }
 
+static uint16_t nvme_abort(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
+{
+    uint16_t sqid = le32_to_cpu(cmd->cdw10) & 0xffff;
+
+    req->cqe.result = 1;
+    if (nvme_check_sqid(n, sqid)) {
+        return NVME_INVALID_FIELD | NVME_DNR;
+    }
+
+    return NVME_SUCCESS;
+}
+
 static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
 {
     trace_nvme_setfeat_timestamp(ts);
@@ -859,6 +871,7 @@  static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
         trace_nvme_err_invalid_setfeat(dw10);
         return NVME_INVALID_FIELD | NVME_DNR;
     }
+
     return NVME_SUCCESS;
 }
 
@@ -875,6 +888,8 @@  static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
         return nvme_create_cq(n, cmd);
     case NVME_ADM_CMD_IDENTIFY:
         return nvme_identify(n, cmd);
+    case NVME_ADM_CMD_ABORT:
+        return nvme_abort(n, cmd, req);
     case NVME_ADM_CMD_SET_FEATURES:
         return nvme_set_feature(n, cmd, req);
     case NVME_ADM_CMD_GET_FEATURES:
@@ -1388,6 +1403,7 @@  static void nvme_realize(PCIDevice *pci_dev, Error **errp)
     id->ieee[2] = 0xb3;
     id->ver = cpu_to_le32(0x00010201);
     id->oacs = cpu_to_le16(0);
+    id->acl = 3;
     id->frmw = 7 << 1;
     id->lpa = 1 << 0;
     id->sqes = (0x6 << 4) | 0x6;