[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller p
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters |
Date: |
Fri, 3 Jul 2020 11:21:31 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
On 7/3/20 10:46 AM, Klaus Jensen wrote:
> On Jul 3 10:31, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 8:34 AM, 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 | 39 +++++++++++++++++++++++++++++++++------
>>> hw/block/nvme.h | 18 ++++++++++++++++++
>>> hw/block/trace-events | 2 ++
>>> include/block/nvme.h | 7 +++++++
>>> 4 files changed, 60 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>> index ba523f6768bf..affb9a967534 100644
>>> --- a/hw/block/nvme.c
>>> +++ b/hw/block/nvme.c
>>> @@ -1056,8 +1056,16 @@ 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) {
>>> + 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;
>>>
>>> @@ -1088,14 +1096,27 @@ static uint16_t nvme_get_feature(NvmeCtrl *n,
>>> NvmeCmd *cmd, NvmeRequest *req)
>>> ((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;
>>> + }
>>> +
>>> + 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:
>>> - trace_pci_nvme_err_invalid_getfeat(dw10);
>>> - return NVME_INVALID_FIELD | NVME_DNR;
>>> + result = cpu_to_le32(nvme_feature_default[fid]);
>>
>> So here we expect uninitialized fid entries to return 0, right?
>>
>
> Yes, if defaults are not 0 (like NVME_ARBITRATION), it is explicitly set.
>
>>> + break;
>>> }
>>>
>>> req->cqe.result = result;
>>> @@ -1124,8 +1145,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;
>>> @@ -1172,8 +1200,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/nvme.h b/hw/block/nvme.h
>>> index f8940435f9ef..8ad1e3c89cee 100644
>>> --- a/hw/block/nvme.h
>>> +++ b/hw/block/nvme.h
>>> @@ -87,6 +87,24 @@ typedef struct NvmeFeatureVal {
>>> uint32_t async_config;
>>> } NvmeFeatureVal;
What do you think about adding:
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -804,7 +804,8 @@ 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_FEATURE_ID_COUNT = 0x100
};
>>>
>>> +static const uint32_t nvme_feature_default[0x100] = {
Why uint32_t and not uint16_t?
With the previously suggested enum you can now replace 0x100
by NVME_FEATURE_ID_COUNT.
>>> + [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
>>> +};
>>> +
>>> +static const bool nvme_feature_support[0x100] = {
Ditto NVME_FEATURE_ID_COUNT.
>>> + [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,
>>> +};
>>
>> Nack. No variable assignation in header please.
Rationale is if another C source file include this header,
the array will be redeclared (statically) and eventually
unused.
>> Move that to the source file.
>>
>
> Understood :)
>
>>> +
>>> typedef struct NvmeCtrl {
>>> PCIDevice parent_obj;
>>> MemoryRegion iomem;
>>> 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..406648d4cf94 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
>>> @@ -902,6 +906,9 @@ enum NvmeFeatureIds {
>>> NVME_SOFTWARE_PROGRESS_MARKER = 0x80
>>> };
>>>
>>> +#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;
>>>
>>
>
- [PATCH v2 12/18] hw/block/nvme: support the get/set features select and save fields, (continued)
- [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters, Klaus Jensen, 2020/07/03
- Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters, Philippe Mathieu-Daudé, 2020/07/03
- Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters, Klaus Jensen, 2020/07/03
- Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters,
Philippe Mathieu-Daudé <=
- Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters, Klaus Jensen, 2020/07/03
- Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters, Philippe Mathieu-Daudé, 2020/07/03
- Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters, Klaus Jensen, 2020/07/03
- Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters, Philippe Mathieu-Daudé, 2020/07/04
[PATCH v2 17/18] hw/block/nvme: provide the mandatory subnqn field, Klaus Jensen, 2020/07/03
[PATCH v2 16/18] hw/block/nvme: enforce valid queue creation sequence, Klaus Jensen, 2020/07/03
[PATCH v2 15/18] hw/block/nvme: reject invalid nsid values in active namespace id list, Klaus Jensen, 2020/07/03