On Aug 2 10:54, Klaus Jensen wrote:
> On Aug 2 16:31, Jinhao Fan wrote:
> > at 2:02 PM, Klaus Jensen <i...@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 <fanjinhao...@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.
signature.asc
Description: PGP signature
