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

[Klaus Jensen <its@irrelevant.dk>]

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

On Aug  2 10:54, Klaus Jensen wrote:
> 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.
> > 
> 

I meant `offset >> (2 + dstrd + 1)` ('+', not '*') like above of course.

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