Re: [PATCH 12/17] hw/block/nvme: support the get/set features select and save fields
On Jul 3 00:46, Dmitry Fomichev wrote:
> On Mon, 2020-06-29 at 20:26 +0200, Klaus Jensen wrote:
> > From: Klaus Jensen
> >
> > Since the device does not have any persistance state storage, no
> > features are "saveable" and setting the Save (SV) field in any Set
> > Features command will result in a Feature Identifier Not Saveable status
> > code.
> >
> > Similarly, if the Select (SEL) field is set to request saved values, the
> > devices will (as it should) return the default values instead.
> >
> > Since this also introduces "Supported Capabilities", the nsid field is
> > now also checked for validity wrt. the feature being get/set'ed.
> >
> > Signed-off-by: Klaus Jensen
> > ---
> > hw/block/nvme.c | 87 +++
> > hw/block/nvme.h | 8
> > hw/block/trace-events | 4 +-
> > include/block/nvme.h | 27 +-
> > 4 files changed, 115 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 647f408854ae..a41665746d33 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1056,16 +1056,43 @@ static uint16_t nvme_get_feature(NvmeCtrl *n,
> > NvmeCmd *cmd, NvmeRequest *req)
> > {
> > uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> > uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> > +uint32_t nsid = le32_to_cpu(cmd->nsid);
> > uint32_t result;
> > uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> > +NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
> > uint16_t iv;
> >
> > -trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
> > +trace_pci_nvme_getfeat(nvme_cid(req), fid, sel, dw11);
> >
> > if (!nvme_feature_support[fid]) {
> > return NVME_INVALID_FIELD | NVME_DNR;
> > }
> >
> > +if (nvme_feature_cap[fid] & NVME_FEAT_CAP_NS) {
> > +if (!nsid || nsid > n->num_namespaces) {
> > +/*
> > + * The Reservation Notification Mask and Reservation
> > Persistence
> > + * features require a status code of Invalid Field in Command
> > when
> > + * NSID is 0x. Since the device does not support those
> > + * features we can always return Invalid Namespace or Format
> > as we
> > + * should do for all other features.
> > + */
> > +return NVME_INVALID_NSID | NVME_DNR;
> > +}
> > +}
> > +
> > +switch (sel) {
> > +case NVME_GETFEAT_SELECT_CURRENT:
> > +break;
> > +case NVME_GETFEAT_SELECT_SAVED:
> > +/* no features are saveable by the controller; fallthrough */
> > +case NVME_GETFEAT_SELECT_DEFAULT:
> > +goto defaults;
> > +case NVME_GETFEAT_SELECT_CAP:
> > +result = cpu_to_le32(nvme_feature_cap[fid]);
> > +goto out;
> > +}
> > +
> > switch (fid) {
> > case NVME_TEMPERATURE_THRESHOLD:
> > result = 0;
> > @@ -1091,6 +1118,29 @@ static uint16_t nvme_get_feature(NvmeCtrl *n,
> > NvmeCmd *cmd, NvmeRequest *req)
> > case NVME_VOLATILE_WRITE_CACHE:
> > result = cpu_to_le32(blk_enable_write_cache(n->conf.blk));
> > trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> > +break;
> > +case NVME_ASYNCHRONOUS_EVENT_CONF:
> > +result = cpu_to_le32(n->features.async_config);
> > +break;
> > +case NVME_TIMESTAMP:
> > +return nvme_get_feature_timestamp(n, cmd);
> > +default:
> > +break;
> > +}
> > +
> > +defaults:
> > +switch (fid) {
> > +case NVME_TEMPERATURE_THRESHOLD:
> > +result = 0;
>
> This will reset the high or low threshold value set earlier in this function.
> You could do the following to avoid this -
>
Good catch! All the cases should really just `goto out` anyway, so fixed
that.
> @ -1163,7 +1163,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> break;
> }
>
> -break;
> +goto out;
This actually needs to be a return NVME_INVALID_FIELD since if we reach
this, THSEL was set to a reserved value.
> case NVME_VOLATILE_WRITE_CACHE:
> result = cpu_to_le32(blk_enable_write_cache(n->conf.blk));
> trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
>
> > +
> > +if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
> > +break;
> > +}
> > +
> > +if (NVME_TEMP_THSEL(dw11) == NVME_TEMP_THSEL_OVER) {
> > +result = cpu_to_le16(NVME_TEMPERATURE_WARNING);
> > +}
> > +
> > break;
> > case NVME_NUMBER_OF_QUEUES:
> > result = cpu_to_le32((n->params.max_ioqpairs - 1) |
> > @@ -1110,16 +1160,12 @@ static uint16_t nvme_get_feature(NvmeCtrl *n,
> > NvmeCmd *cmd, NvmeRequest *req)
> >
> > result = cpu_to_le32(result);
> > break;
> > -case NVME_ASYNCHRONOUS_EVENT_CONF:
> > -result = cpu_to_le32(n->features.async_config);
> > -
Re: [PATCH 12/17] hw/block/nvme: support the get/set features select and save fields
On Mon, 2020-06-29 at 20:26 +0200, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Since the device does not have any persistance state storage, no
> features are "saveable" and setting the Save (SV) field in any Set
> Features command will result in a Feature Identifier Not Saveable status
> code.
>
> Similarly, if the Select (SEL) field is set to request saved values, the
> devices will (as it should) return the default values instead.
>
> Since this also introduces "Supported Capabilities", the nsid field is
> now also checked for validity wrt. the feature being get/set'ed.
>
> Signed-off-by: Klaus Jensen
> ---
> hw/block/nvme.c | 87 +++
> hw/block/nvme.h | 8
> hw/block/trace-events | 4 +-
> include/block/nvme.h | 27 +-
> 4 files changed, 115 insertions(+), 11 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 647f408854ae..a41665746d33 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1056,16 +1056,43 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> {
> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> +uint32_t nsid = le32_to_cpu(cmd->nsid);
> uint32_t result;
> uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> +NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
> uint16_t iv;
>
> -trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
> +trace_pci_nvme_getfeat(nvme_cid(req), fid, sel, dw11);
>
> if (!nvme_feature_support[fid]) {
> return NVME_INVALID_FIELD | NVME_DNR;
> }
>
> +if (nvme_feature_cap[fid] & NVME_FEAT_CAP_NS) {
> +if (!nsid || nsid > n->num_namespaces) {
> +/*
> + * The Reservation Notification Mask and Reservation Persistence
> + * features require a status code of Invalid Field in Command
> when
> + * NSID is 0x. Since the device does not support those
> + * features we can always return Invalid Namespace or Format as
> we
> + * should do for all other features.
> + */
> +return NVME_INVALID_NSID | NVME_DNR;
> +}
> +}
> +
> +switch (sel) {
> +case NVME_GETFEAT_SELECT_CURRENT:
> +break;
> +case NVME_GETFEAT_SELECT_SAVED:
> +/* no features are saveable by the controller; fallthrough */
> +case NVME_GETFEAT_SELECT_DEFAULT:
> +goto defaults;
> +case NVME_GETFEAT_SELECT_CAP:
> +result = cpu_to_le32(nvme_feature_cap[fid]);
> +goto out;
> +}
> +
> switch (fid) {
> case NVME_TEMPERATURE_THRESHOLD:
> result = 0;
> @@ -1091,6 +1118,29 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> case NVME_VOLATILE_WRITE_CACHE:
> result = cpu_to_le32(blk_enable_write_cache(n->conf.blk));
> trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> +break;
> +case NVME_ASYNCHRONOUS_EVENT_CONF:
> +result = cpu_to_le32(n->features.async_config);
> +break;
> +case NVME_TIMESTAMP:
> +return nvme_get_feature_timestamp(n, cmd);
> +default:
> +break;
> +}
> +
> +defaults:
> +switch (fid) {
> +case NVME_TEMPERATURE_THRESHOLD:
> +result = 0;
This will reset the high or low threshold value set earlier in this function.
You could do the following to avoid this -
@ -1163,7 +1163,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
*cmd, NvmeRequest *req)
break;
}
-break;
+goto out;
case NVME_VOLATILE_WRITE_CACHE:
result = cpu_to_le32(blk_enable_write_cache(n->conf.blk));
trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> +
> +if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
> +break;
> +}
> +
> +if (NVME_TEMP_THSEL(dw11) == NVME_TEMP_THSEL_OVER) {
> +result = cpu_to_le16(NVME_TEMPERATURE_WARNING);
> +}
> +
> break;
> case NVME_NUMBER_OF_QUEUES:
> result = cpu_to_le32((n->params.max_ioqpairs - 1) |
> @@ -1110,16 +1160,12 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
>
> result = cpu_to_le32(result);
> break;
> -case NVME_ASYNCHRONOUS_EVENT_CONF:
> -result = cpu_to_le32(n->features.async_config);
> -break;
> -case NVME_TIMESTAMP:
> -return nvme_get_feature_timestamp(n, cmd);
> default:
> result = cpu_to_le32(nvme_feature_default[fid]);
> break;
> }
>
> +out:
> req->cqe.result = result;
> return NVME_SUCCESS;
> }
> @@ -1146,14 +1192,37 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> {
> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> +uint32_t
[PATCH 12/17] hw/block/nvme: support the get/set features select and save fields
From: Klaus Jensen
Since the device does not have any persistance state storage, no
features are "saveable" and setting the Save (SV) field in any Set
Features command will result in a Feature Identifier Not Saveable status
code.
Similarly, if the Select (SEL) field is set to request saved values, the
devices will (as it should) return the default values instead.
Since this also introduces "Supported Capabilities", the nsid field is
now also checked for validity wrt. the feature being get/set'ed.
Signed-off-by: Klaus Jensen
---
hw/block/nvme.c | 87 +++
hw/block/nvme.h | 8
hw/block/trace-events | 4 +-
include/block/nvme.h | 27 +-
4 files changed, 115 insertions(+), 11 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 647f408854ae..a41665746d33 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1056,16 +1056,43 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
*cmd, NvmeRequest *req)
{
uint32_t dw10 = le32_to_cpu(cmd->cdw10);
uint32_t dw11 = le32_to_cpu(cmd->cdw11);
+uint32_t nsid = le32_to_cpu(cmd->nsid);
uint32_t result;
uint8_t fid = NVME_GETSETFEAT_FID(dw10);
+NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
uint16_t iv;
-trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
+trace_pci_nvme_getfeat(nvme_cid(req), fid, sel, dw11);
if (!nvme_feature_support[fid]) {
return NVME_INVALID_FIELD | NVME_DNR;
}
+if (nvme_feature_cap[fid] & NVME_FEAT_CAP_NS) {
+if (!nsid || nsid > n->num_namespaces) {
+/*
+ * The Reservation Notification Mask and Reservation Persistence
+ * features require a status code of Invalid Field in Command when
+ * NSID is 0x. Since the device does not support those
+ * features we can always return Invalid Namespace or Format as we
+ * should do for all other features.
+ */
+return NVME_INVALID_NSID | NVME_DNR;
+}
+}
+
+switch (sel) {
+case NVME_GETFEAT_SELECT_CURRENT:
+break;
+case NVME_GETFEAT_SELECT_SAVED:
+/* no features are saveable by the controller; fallthrough */
+case NVME_GETFEAT_SELECT_DEFAULT:
+goto defaults;
+case NVME_GETFEAT_SELECT_CAP:
+result = cpu_to_le32(nvme_feature_cap[fid]);
+goto out;
+}
+
switch (fid) {
case NVME_TEMPERATURE_THRESHOLD:
result = 0;
@@ -1091,6 +1118,29 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
*cmd, NvmeRequest *req)
case NVME_VOLATILE_WRITE_CACHE:
result = cpu_to_le32(blk_enable_write_cache(n->conf.blk));
trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
+break;
+case NVME_ASYNCHRONOUS_EVENT_CONF:
+result = cpu_to_le32(n->features.async_config);
+break;
+case NVME_TIMESTAMP:
+return nvme_get_feature_timestamp(n, cmd);
+default:
+break;
+}
+
+defaults:
+switch (fid) {
+case NVME_TEMPERATURE_THRESHOLD:
+result = 0;
+
+if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
+break;
+}
+
+if (NVME_TEMP_THSEL(dw11) == NVME_TEMP_THSEL_OVER) {
+result = cpu_to_le16(NVME_TEMPERATURE_WARNING);
+}
+
break;
case NVME_NUMBER_OF_QUEUES:
result = cpu_to_le32((n->params.max_ioqpairs - 1) |
@@ -1110,16 +1160,12 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
*cmd, NvmeRequest *req)
result = cpu_to_le32(result);
break;
-case NVME_ASYNCHRONOUS_EVENT_CONF:
-result = cpu_to_le32(n->features.async_config);
-break;
-case NVME_TIMESTAMP:
-return nvme_get_feature_timestamp(n, cmd);
default:
result = cpu_to_le32(nvme_feature_default[fid]);
break;
}
+out:
req->cqe.result = result;
return NVME_SUCCESS;
}
@@ -1146,14 +1192,37 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd
*cmd, NvmeRequest *req)
{
uint32_t dw10 = le32_to_cpu(cmd->cdw10);
uint32_t dw11 = le32_to_cpu(cmd->cdw11);
+uint32_t nsid = le32_to_cpu(cmd->nsid);
uint8_t fid = NVME_GETSETFEAT_FID(dw10);
+uint8_t save = NVME_SETFEAT_SAVE(dw10);
-trace_pci_nvme_setfeat(nvme_cid(req), fid, dw11);
+trace_pci_nvme_setfeat(nvme_cid(req), fid, save, dw11);
+
+if (save) {
+return NVME_FID_NOT_SAVEABLE | NVME_DNR;
+}
if (!nvme_feature_support[fid]) {
return NVME_INVALID_FIELD | NVME_DNR;
}
+if (nvme_feature_cap[fid] & NVME_FEAT_CAP_NS) {
+if (!nsid || (nsid != NVME_NSID_BROADCAST &&
+ nsid > n->num_namespaces)) {
+return NVME_INVALID_NSID | NVME_DNR;
+}
+} else if (nsid && nsid != NVME_NSID_BROADCAST) {
+if (nsid > n->num_namespaces) {
+return NVME_INVALID_NSID | NVME_DNR;
+
