Re: [PATCH v9 09/10] hw/nvme: add reservation protocal command

[卢长奇]

Hi,

```
2685 nvme_status->regctl_ds[i].cntlid = nvme_ctrl(req)->cntlid;
2686 nvme_status->regctl_ds[i].rkey = keys_info->keys[i];
2687 nvme_status->regctl_ds[i].rcsts = keys_info->keys[i] ==
2688 reservation->key ? 1 : 0;
2689 /* hostid is not supported currently */
2670 memset(&nvme_status->regctl_ds[i].hostid, 0, 8);
```

Klaus, I think hostid(2685) is stored locally like cntlid, i
can get cntlid by nvme_ctrl(req)->cntlid, but I can't
find a good way to get the host ID(2670). So I add a comment
"/* hostid is not supported currently */". Could you give me
some advices?

And using spdk as target will not fail, but it will show 0 at hostid
at present. The relevant tests in qemu are as follows，

```
root@node1:~# nvme resv-report /dev/nvme0n1
NVME Reservation Report success

NVME Reservation status:

gen : 1
regctl : 1
rtype : 0
ptpls : 0
regctl[0] :
cntlid : 0
rcsts : 0
hostid : 0
rkey : 6
```

On 2024/7/17 17:03, 卢长奇 wrote:
> Hi,
>
>
> Thank you for your support.
> 1. I will add a guide on how to get a simple test at next patch.
> 2. I think hostid is stored locally like cntlid, but I can't
> find a way to get the host ID. Is it correct to get it through
> qmp_query_uuid() method?
>
> Using spdk as target will not fail, but it will show 0 at hostid
> at present.
>
> On 2024/7/15 18:09, Klaus Jensen wrote:
>> On Jul 12 10:36, Changqi Lu wrote:
>>> Add reservation acquire, reservation register,
>>> reservation release and reservation report commands
>>> in the nvme device layer.
>>>
>>> By introducing these commands, this enables the nvme
>>> device to perform reservation-related tasks, including
>>> querying keys, querying reservation status, registering
>>> reservation keys, initiating and releasing reservations,
>>> as well as clearing and preempting reservations held by
>>> other keys.
>>>
>>> These commands are crucial for management and control of
>>> shared storage resources in a persistent manner.
>>> Signed-off-by: Changqi Lu
>>> Signed-off-by: zhenwei pi
>>> Acked-by: Klaus Jensen
>>> ---
>>> hw/nvme/ctrl.c | 318 +++++++++++++++++++++++++++++++++++++++++++
>>> hw/nvme/nvme.h | 4 +
>>> include/block/nvme.h | 37 +++++
>>> 3 files changed, 359 insertions(+)
>>>
>>
>> This looks good to me, but two comments.
>>
>> 1. Do you have a small guide on how to get a simple test environment
>> up for this?
>>
>> 2. Can you touch on the justification for not supporting the remaining
>> mandatory features required when indicating Reservation support?
>>
>> hw/nvme *does* compromise on mandatory features from time to time
>> when it makes sense, so I'm not saying that not having the log
>> pages, notifications and so on is a deal breaker, I'm just
>> interested in the justification and/or motivation.
>>
>> For instance, I think the SPDK reserve test will fail on this due
>> to lack of Host Identifier Feature support.