[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 12/17] hw/block/nvme: support the get/set features select and
|
From: |
Klaus Jensen |
|
Subject: |
Re: [PATCH 12/17] hw/block/nvme: support the get/set features select and save fields |
|
Date: |
Fri, 3 Jul 2020 08:03:24 +0200 |
On Jul 3 00:46, Dmitry Fomichev wrote:
> On Mon, 2020-06-29 at 20:26 +0200, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > 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 <k.jensen@samsung.com>
> > ---
> > 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 0xFFFFFFFF. 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);
> > - 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;
> > + }
> > +
> > + return NVME_FEAT_NOT_NS_SPEC | NVME_DNR;
> > + }
> > +
> > + if (!(nvme_feature_cap[fid] & NVME_FEAT_CAP_CHANGE)) {
> > + return NVME_FEAT_NOT_CHANGABLE | NVME_DNR;
> > + }
> > +
> > switch (fid) {
> > case NVME_TEMPERATURE_THRESHOLD:
> > if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
> > @@ -1998,7 +2067,9 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice
> > *pci_dev)
> > id->sqes = (0x6 << 4) | 0x6;
> > id->cqes = (0x4 << 4) | 0x4;
> > id->nn = cpu_to_le32(n->num_namespaces);
> > - id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS | NVME_ONCS_TIMESTAMP);
> > + id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS | NVME_ONCS_TIMESTAMP |
> > + NVME_ONCS_FEATURES);
> > +
> > id->psd[0].mp = cpu_to_le16(0x9c4);
> > id->psd[0].enlat = cpu_to_le32(0x10);
> > id->psd[0].exlat = cpu_to_le32(0x4);
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index d0763eb59e5d..34dddda29d96 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -90,6 +90,14 @@ typedef struct NvmeFeatureVal {
> > uint32_t async_config;
> > } NvmeFeatureVal;
> >
> > +static const uint32_t nvme_feature_cap[0x100] = {
> > + [NVME_TEMPERATURE_THRESHOLD] = NVME_FEAT_CAP_CHANGE,
> > + [NVME_VOLATILE_WRITE_CACHE] = NVME_FEAT_CAP_CHANGE,
> > + [NVME_NUMBER_OF_QUEUES] = NVME_FEAT_CAP_CHANGE,
> > + [NVME_ASYNCHRONOUS_EVENT_CONF] = NVME_FEAT_CAP_CHANGE,
> > + [NVME_TIMESTAMP] = NVME_FEAT_CAP_CHANGE,
> > +};
> > +
> > static const uint32_t nvme_feature_default[0x100] = {
> > [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> > };
> > diff --git a/hw/block/trace-events b/hw/block/trace-events
> > index 42e62f4649f8..4a4ef34071df 100644
> > --- a/hw/block/trace-events
> > +++ b/hw/block/trace-events
> > @@ -46,8 +46,8 @@ pci_nvme_identify_ctrl(void) "identify controller"
> > pci_nvme_identify_ns(uint32_t ns) "nsid %"PRIu32""
> > pci_nvme_identify_nslist(uint32_t ns) "nsid %"PRIu32""
> > pci_nvme_get_log(uint16_t cid, uint8_t lid, uint8_t lsp, uint8_t rae,
> > uint32_t len, uint64_t off) "cid %"PRIu16" lid 0x%"PRIx8" lsp 0x%"PRIx8"
> > rae 0x%"PRIx8" len %"PRIu32" off %"PRIu64""
> > -pci_nvme_getfeat(uint16_t cid, uint8_t fid, uint32_t cdw11) "cid %"PRIu16"
> > fid 0x%"PRIx8" cdw11 0x%"PRIx32""
> > -pci_nvme_setfeat(uint16_t cid, uint8_t fid, uint32_t cdw11) "cid %"PRIu16"
> > fid 0x%"PRIx8" cdw11 0x%"PRIx32""
> > +pci_nvme_getfeat(uint16_t cid, uint8_t fid, uint8_t sel, uint32_t cdw11)
> > "cid %"PRIu16" fid 0x%"PRIx8" sel 0x%"PRIx8" cdw11 0x%"PRIx32""
> > +pci_nvme_setfeat(uint16_t cid, uint8_t fid, uint8_t save, uint32_t cdw11)
> > "cid %"PRIu16" fid 0x%"PRIx8" save 0x%"PRIx8" cdw11 0x%"PRIx32""
> > pci_nvme_getfeat_vwcache(const char* result) "get feature volatile write
> > cache, result=%s"
> > pci_nvme_getfeat_numq(int result) "get feature number of queues, result=%d"
> > pci_nvme_setfeat_numq(int reqcq, int reqsq, int gotcq, int gotsq)
> > "requested cq_count=%d sq_count=%d, responding with cq_count=%d sq_count=%d"
> > diff --git a/include/block/nvme.h b/include/block/nvme.h
> > index 662e521c9e9b..60833039a6c5 100644
> > --- a/include/block/nvme.h
> > +++ b/include/block/nvme.h
> > @@ -663,7 +663,7 @@ enum NvmeStatusCodes {
> > NVME_INVALID_QUEUE_DEL = 0x010c,
> > NVME_FID_NOT_SAVEABLE = 0x010d,
> > NVME_FEAT_NOT_CHANGABLE = 0x010e,
> > - NVME_FID_NOT_NSID_SPEC = 0x010f,
> > + NVME_FEAT_NOT_NS_SPEC = 0x010f,
> > NVME_FW_REQ_SUSYSTEM_RESET = 0x0110,
> > NVME_CONFLICTING_ATTRS = 0x0180,
> > NVME_INVALID_PROT_INFO = 0x0181,
> > @@ -906,9 +906,32 @@ enum NvmeFeatureIds {
> > NVME_SOFTWARE_PROGRESS_MARKER = 0x80
> > };
> >
> > +typedef enum NvmeFeatureCap {
> > + NVME_FEAT_CAP_SAVE = 1 << 0,
> > + NVME_FEAT_CAP_NS = 1 << 1,
> > + NVME_FEAT_CAP_CHANGE = 1 << 2,
> > +} NvmeFeatureCap;
> > +
> > +typedef enum NvmeGetFeatureSelect {
> > + NVME_GETFEAT_SELECT_CURRENT = 0x0,
> > + NVME_GETFEAT_SELECT_DEFAULT = 0x1,
> > + NVME_GETFEAT_SELECT_SAVED = 0x2,
> > + NVME_GETFEAT_SELECT_CAP = 0x3,
> > +} NvmeGetFeatureSelect;
> > +
> > #define NVME_GETSETFEAT_FID_MASK 0xff
> > #define NVME_GETSETFEAT_FID(dw10) (dw10 & NVME_GETSETFEAT_FID_MASK)
> >
> > +#define NVME_GETFEAT_SELECT_SHIFT 8
> > +#define NVME_GETFEAT_SELECT_MASK 0x7
> > +#define NVME_GETFEAT_SELECT(dw10) \
> > + ((dw10 >> NVME_GETFEAT_SELECT_SHIFT) & NVME_GETFEAT_SELECT_MASK)
> > +
> > +#define NVME_SETFEAT_SAVE_SHIFT 31
> > +#define NVME_SETFEAT_SAVE_MASK 0x1
> > +#define NVME_SETFEAT_SAVE(dw10) \
> > + ((dw10 >> NVME_SETFEAT_SAVE_SHIFT) & NVME_SETFEAT_SAVE_MASK)
> > +
> > typedef struct NvmeRangeType {
> > uint8_t type;
> > uint8_t attributes;
> > @@ -925,6 +948,8 @@ typedef struct NvmeLBAF {
> > uint8_t rp;
> > } NvmeLBAF;
> >
> > +#define NVME_NSID_BROADCAST 0xffffffff
> > +
> > typedef struct NvmeIdNs {
> > uint64_t nsze;
> > uint64_t ncap;