[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 11/18] hw/block/nvme: add remaining mandatory controller p
From: |
Maxim Levitsky |
Subject: |
Re: [PATCH v3 11/18] hw/block/nvme: add remaining mandatory controller parameters |
Date: |
Wed, 29 Jul 2020 14:31:43 +0300 |
User-agent: |
Evolution 3.36.3 (3.36.3-1.fc32) |
On Mon, 2020-07-06 at 08:12 +0200, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Add support for any remaining mandatory controller operating parameters
> (features).
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
> hw/block/nvme.c | 56 ++++++++++++++++++++++++++++++++++++++-----
> hw/block/trace-events | 2 ++
> include/block/nvme.h | 10 +++++++-
> 3 files changed, 61 insertions(+), 7 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 8fce2ebf69e7..2d85e853403f 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -71,6 +71,20 @@
> " in %s: " fmt "\n", __func__, ## __VA_ARGS__); \
> } while (0)
>
> +static const bool nvme_feature_support[NVME_FID_MAX] = {
> + [NVME_ARBITRATION] = true,
> + [NVME_POWER_MANAGEMENT] = true,
> + [NVME_TEMPERATURE_THRESHOLD] = true,
> + [NVME_ERROR_RECOVERY] = true,
> + [NVME_VOLATILE_WRITE_CACHE] = true,
> + [NVME_NUMBER_OF_QUEUES] = true,
> + [NVME_INTERRUPT_COALESCING] = true,
> + [NVME_INTERRUPT_VECTOR_CONF] = true,
> + [NVME_WRITE_ATOMICITY] = true,
> + [NVME_ASYNCHRONOUS_EVENT_CONF] = true,
> + [NVME_TIMESTAMP] = true,
I checked the spec and mandatory features are all here.
> +};
> +
> static void nvme_process_sq(void *opaque);
>
> static uint16_t nvme_cid(NvmeRequest *req)
> @@ -1070,8 +1084,20 @@ 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 result;
> + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> + uint16_t iv;
>
> - switch (dw10) {
> + static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
> + [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> + };
Nice idea!
> +
> + trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
> +
> + if (!nvme_feature_support[fid]) {
> + return NVME_INVALID_FIELD | NVME_DNR;
> + }
> +
> + switch (fid) {
> case NVME_TEMPERATURE_THRESHOLD:
> result = 0;
>
> @@ -1101,6 +1127,18 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> result = (n->params.max_ioqpairs - 1) |
> ((n->params.max_ioqpairs - 1) << 16);
> trace_pci_nvme_getfeat_numq(result);
> + break;
> + case NVME_INTERRUPT_VECTOR_CONF:
> + iv = dw11 & 0xffff;
> + if (iv >= n->params.max_ioqpairs + 1) {
> + return NVME_INVALID_FIELD | NVME_DNR;
> + }
> +
> + result = iv;
> + if (iv == n->admin_cq.vector) {
> + result |= NVME_INTVC_NOCOALESCING;
> + }
I wonder if this is needed, but it doesn't hurt to have this.
Spec is not clear about this.
> +
> break;
> case NVME_ASYNCHRONOUS_EVENT_CONF:
> result = n->features.async_config;
> @@ -1108,8 +1146,8 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> case NVME_TIMESTAMP:
> return nvme_get_feature_timestamp(n, cmd);
> default:
> - trace_pci_nvme_err_invalid_getfeat(dw10);
> - return NVME_INVALID_FIELD | NVME_DNR;
> + result = nvme_feature_default[fid];
> + break;
> }
>
> req->cqe.result = cpu_to_le32(result);
> @@ -1138,8 +1176,15 @@ 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);
> + uint8_t fid = NVME_GETSETFEAT_FID(dw10);
>
> - switch (dw10) {
> + trace_pci_nvme_setfeat(nvme_cid(req), fid, dw11);
> +
> + if (!nvme_feature_support[fid]) {
> + return NVME_INVALID_FIELD | NVME_DNR;
> + }
> +
> + switch (fid) {
> case NVME_TEMPERATURE_THRESHOLD:
> if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
> break;
> @@ -1186,8 +1231,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> case NVME_TIMESTAMP:
> return nvme_set_feature_timestamp(n, cmd);
> default:
> - trace_pci_nvme_err_invalid_setfeat(dw10);
> - return NVME_INVALID_FIELD | NVME_DNR;
> + return NVME_FEAT_NOT_CHANGEABLE | NVME_DNR;
> }
> return NVME_SUCCESS;
> }
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 091af16ca7d7..42e62f4649f8 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -46,6 +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_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 0dce15af6bcf..cd396111b2f5 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -662,6 +662,7 @@ enum NvmeStatusCodes {
> NVME_FW_REQ_RESET = 0x010b,
> NVME_INVALID_QUEUE_DEL = 0x010c,
> NVME_FID_NOT_SAVEABLE = 0x010d,
> + NVME_FEAT_NOT_CHANGEABLE = 0x010e,
> NVME_FID_NOT_NSID_SPEC = 0x010f,
> NVME_FW_REQ_SUSYSTEM_RESET = 0x0110,
> NVME_CONFLICTING_ATTRS = 0x0180,
> @@ -866,6 +867,7 @@ enum NvmeIdCtrlLpa {
> #define NVME_CTRL_SGLS_ADDR_OFFSET (0x1 << 20)
>
> #define NVME_ARB_AB(arb) (arb & 0x7)
> +#define NVME_ARB_AB_NOLIMIT 0x7
> #define NVME_ARB_LPW(arb) ((arb >> 8) & 0xff)
> #define NVME_ARB_MPW(arb) ((arb >> 16) & 0xff)
> #define NVME_ARB_HPW(arb) ((arb >> 24) & 0xff)
> @@ -873,6 +875,8 @@ enum NvmeIdCtrlLpa {
> #define NVME_INTC_THR(intc) (intc & 0xff)
> #define NVME_INTC_TIME(intc) ((intc >> 8) & 0xff)
>
> +#define NVME_INTVC_NOCOALESCING (0x1 << 16)
> +
> #define NVME_TEMP_THSEL(temp) ((temp >> 20) & 0x3)
> #define NVME_TEMP_THSEL_OVER 0x0
> #define NVME_TEMP_THSEL_UNDER 0x1
> @@ -899,9 +903,13 @@ enum NvmeFeatureIds {
> NVME_WRITE_ATOMICITY = 0xa,
> NVME_ASYNCHRONOUS_EVENT_CONF = 0xb,
> NVME_TIMESTAMP = 0xe,
> - NVME_SOFTWARE_PROGRESS_MARKER = 0x80
> + NVME_SOFTWARE_PROGRESS_MARKER = 0x80,
> + NVME_FID_MAX = 0x100,
> };
>
> +#define NVME_GETSETFEAT_FID_MASK 0xff
> +#define NVME_GETSETFEAT_FID(dw10) (dw10 & NVME_GETSETFEAT_FID_MASK)
> +
> typedef struct NvmeRangeType {
> uint8_t type;
> uint8_t attributes;
Looks good,
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
Maxim Levitsky
- [PATCH v3 07/18] hw/block/nvme: add support for the get log page command, (continued)
- [PATCH v3 09/18] hw/block/nvme: move NvmeFeatureVal into hw/block/nvme.h, Klaus Jensen, 2020/07/06
- [PATCH v3 10/18] hw/block/nvme: flush write cache when disabled, Klaus Jensen, 2020/07/06
- [PATCH v3 11/18] hw/block/nvme: add remaining mandatory controller parameters, Klaus Jensen, 2020/07/06
- Re: [PATCH v3 11/18] hw/block/nvme: add remaining mandatory controller parameters,
Maxim Levitsky <=
- [PATCH v3 13/18] hw/block/nvme: make sure ncqr and nsqr is valid, Klaus Jensen, 2020/07/06
- [PATCH v3 14/18] hw/block/nvme: support identify namespace descriptor list, Klaus Jensen, 2020/07/06
- [PATCH v3 12/18] hw/block/nvme: support the get/set features select and save fields, Klaus Jensen, 2020/07/06
[PATCH v3 17/18] hw/block/nvme: provide the mandatory subnqn field, Klaus Jensen, 2020/07/06