Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters

[Philippe Mathieu-Daudé]

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.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
>>  };
>>
> 
> 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.

> 
>> 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.
>>
> 
> Fixed!
>