On 7/3/20 4:37 PM, Klaus Jensen wrote:
> On Jul 3 13:02, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 12:10 PM, Klaus Jensen wrote:
>>> On Jul 3 11:21, Philippe Mathieu-Daudé wrote:
>>>> 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.jen...@samsung.com>
>>>>>>>
>>>>>>> Add support for any remaining mandatory controller operating parameters
>>>>>>> (features).
>>>>>>>
>>>>>>> Signed-off-by: Klaus Jensen <k.jen...@samsung.com>
>>>>>>> Reviewed-by: Dmitry Fomichev <dmitry.fomic...@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
>>>> };
>>>>
>>>
>>> I can definitely go with that. I added it as NVME_FID_MAX.
>>
>> Good name.
>>
>>>
>>>>>>>
>>>>>>> +static const uint32_t nvme_feature_default[0x100] = {
>>>>
>>>> Why uint32_t and not uint16_t?
>>>>
>>>
>>> The feature values are by definition 32 bits wide, so even though the
>>> defaults here do not require it, I think uin32_t should be used.
>>
>> Can you prepend a new patch making nvme_get_feature() return uin32_t
>> instead of uint16_t? That would be clearer.
>>
>
> Ah. Now I see where the confusion comes from ;)
>
> nvme_get_feature() does not return the value of the feature. It returns
> a uint16_t with a value from the NvmeStatusCodes enum (which really
> should be a typedef used all over the place.
Ah! You are right, I misread it indeed :)
