[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 15/18] hw/block/nvme: reject invalid nsid values in active
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v2 15/18] hw/block/nvme: reject invalid nsid values in active namespace id list |
Date: |
Fri, 3 Jul 2020 12:59:52 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
On 7/3/20 11:22 AM, Klaus Jensen wrote:
> On Jul 3 11:14, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 10:37 AM, Klaus Jensen wrote:
>>> On Jul 3 10:20, Philippe Mathieu-Daudé wrote:
>>>> On 7/3/20 8:34 AM, Klaus Jensen wrote:
>>>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>>>
>>>>> Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the
>>>>> Active Namespace ID list.
>>>>
>>>> Can we have a definition instead of this 0xfffffffe magic value please?
>>>>
>>>
>>> Hmm, not really actually. It's not a magic value, its just because the
>>> logic in Active Namespace ID list would require that it should report
>>> any namespaces with ids *higher* than the one specified, so since
>>> 0xffffffff (NVME_NSID_BROADCAST) is invalid, NVME_NSID_BROADCAST - 1
>>> needs to be as well.
>>
>> OK.
>>
>>>
>>> What do you say I change it to `min_nsid >= NVME_NSID_BROADCAST - 1`?
>>> The original condition just reads well if you are sitting with the spec
>>> on the side.
>>
>> IMO this is clearer:
>>
>> if (min_nsid + 1 >= NVME_NSID_BROADCAST) {
>> return NVME_INVALID_NSID | NVME_DNR;
>> }
>>
>
> But since min_nsid is uint32_t that would not be wise ;)
Hmm indeed.
>
> I'll go with the - 1 and add a comment!
Good, thanks.
>
>> Whichever form you prefer you can amend to the respin patch:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>>>
>>>>>
>>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>>>> ---
>>>>> hw/block/nvme.c | 4 ++++
>>>>> 1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>>>> index 65c2fa3ac1f4..0dac7a41ddae 100644
>>>>> --- a/hw/block/nvme.c
>>>>> +++ b/hw/block/nvme.c
>>>>> @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n,
>>>>> NvmeIdentify *c)
>>>>>
>>>>> trace_pci_nvme_identify_nslist(min_nsid);
>>>>>
>>>>> + if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) {
>>>>> + return NVME_INVALID_NSID | NVME_DNR;
>>>>> + }
>>>>> +
>>>>> list = g_malloc0(data_len);
>>>>> for (i = 0; i < n->num_namespaces; i++) {
>>>>> if (i < min_nsid) {
>>>>>
>>>>
>>>
>>
>
- Re: [PATCH v2 11/18] hw/block/nvme: add remaining mandatory controller parameters, (continued)
[PATCH v2 18/18] hw/block/nvme: bump supported version to v1.3, Klaus Jensen, 2020/07/03