| Message ID | 20191015103900.313928-7-its@irrelevant.dk |
|---|---|
| State | New |
| Headers | show |
| Series | nvme: support NVMe v1.3d, SGLs and multiple namespaces | expand |
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 > >
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.
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
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
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;
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(+)