Re: [PATCH] hw/nvme: Add helper functions for qid-db conversion

[Jinhao Fan]

at 2:02 PM, Klaus Jensen <its@irrelevant.dk> wrote:

> On Jul 28 16:07, Jinhao Fan wrote:
>> With the introduction of shadow doorbell and ioeventfd, we need to do
>> frequent conversion between qid and its doorbell offset. The original
>> hard-coded calculation is confusing and error-prone. Add several helper
>> functions to do this task.
>> 
>> Signed-off-by: Jinhao Fan <fanjinhao21s@ict.ac.cn>
>> ---
>> hw/nvme/ctrl.c | 61 ++++++++++++++++++++++++++++++++------------------
>> 1 file changed, 39 insertions(+), 22 deletions(-)
>> 
>> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>> index 533ad14e7a..6116c0e660 100644
>> --- a/hw/nvme/ctrl.c
>> +++ b/hw/nvme/ctrl.c
>> @@ -487,6 +487,29 @@ static int nvme_check_cqid(NvmeCtrl *n, uint16_t cqid)
>> {
>>     return cqid < n->conf_ioqpairs + 1 && n->cq[cqid] != NULL ? 0 : -1;
>> }
>> +static inline bool nvme_db_offset_is_cq(NvmeCtrl *n, hwaddr offset)
>> +{
>> +    hwaddr stride = 4 << NVME_CAP_DSTRD(ldq_le_p(&n->bar.cap));
>> +    return (offset / stride) & 1;
>> +}
> 
> This can be changed morphed into `(offset >> (2 + dstrd)) & 1` if I am not
> mistaken.
> 

Yes. But my current code looks more readable to me. Is it necessary to
change to `(offset >> (2 + dstrd)) & 1`.

>> +
>> +static inline uint16_t nvme_db_offset_to_qid(NvmeCtrl *n, hwaddr offset)
>> +{
>> +    hwaddr stride = 4 << NVME_CAP_DSTRD(ldq_le_p(&n->bar.cap));
>> +    return offset / (2 * stride);
>> +}
> 
> Same, should be able to do `offset >> (2 * dstrd + 1)`, no?

Same as above.