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

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 2101 bytes --]

On Aug  2 16:31, Jinhao Fan wrote:
> 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`.
> 

I am unsure if the compiler will transform that division into the shift
if it can infer that the divisor is a power of two (it most likely
will be able to).

But I see no reason to have a potential division here when we can do
without and to me it is just as readable when you know the definition of
DSTRD is `2 ^ (2 + DSTRD)`.

> >> +
> >> +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.
> 

-- 
One of us - No more doubt, silence or taboo about mental illness.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]