Hi Klaus, On Wed, 13 Nov 2019 at 06:12, Klaus Birkelund <i...@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 <i...@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.jen...@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