RE: [PATCH v3 02/15] hw/block/nvme: Report actual LBA data shift in LBAF

[Dmitry Fomichev]

> -----Original Message-----
> From: Klaus Jensen <its@irrelevant.dk>
> Sent: Tuesday, September 15, 2020 3:34 AM
> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> Cc: Keith Busch <kbusch@kernel.org>; Klaus Jensen
> <k.jensen@samsung.com>; Kevin Wolf <kwolf@redhat.com>; Philippe
> Mathieu-Daudé <philmd@redhat.com>; Maxim Levitsky
> <mlevitsk@redhat.com>; Fam Zheng <fam@euphon.net>; Niklas Cassel
> <Niklas.Cassel@wdc.com>; Damien Le Moal <Damien.LeMoal@wdc.com>;
> qemu-block@nongnu.org; qemu-devel@nongnu.org; Alistair Francis
> <Alistair.Francis@wdc.com>; Matias Bjorling <Matias.Bjorling@wdc.com>
> Subject: Re: [PATCH v3 02/15] hw/block/nvme: Report actual LBA data shift in
> LBAF
> 
> On Sep 14 07:14, Dmitry Fomichev wrote:
> > Calculate the data shift value to report based on the set value of
> > logical_block_size device property.
> >
> > In the process, use a local variable to calculate the LBA format
> > index instead of the hardcoded value 0. This makes the code more
> > readable and it will make it easier to add support for multiple LBA
> > formats in the future.
> >
> > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > ---
> >  hw/block/nvme.c |  4 +++-
> >  hw/block/nvme.h | 11 +++++++++++
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 3a90d80694..1cfc136042 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -2203,6 +2203,7 @@ static void nvme_init_namespace(NvmeCtrl *n,
> NvmeNamespace *ns, Error **errp)
> >  {
> >      int64_t bs_size;
> >      NvmeIdNs *id_ns = &ns->id_ns;
> > +    int lba_index;
> >
> >      bs_size = blk_getlength(n->conf.blk);
> >      if (bs_size < 0) {
> > @@ -2212,7 +2213,8 @@ static void nvme_init_namespace(NvmeCtrl *n,
> NvmeNamespace *ns, Error **errp)
> >
> >      n->ns_size = bs_size;
> >
> > -    id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
> > +    lba_index = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
> > +    id_ns->lbaf[lba_index].ds = nvme_ilog2(n->conf.logical_block_size);
> 
> Instead of defining a new function, we can directly use clz32().
> 
>   31 - clz32(n->conf.logical_block_size)

Ok nice! I looked up what QEMU uses for binary log, but couldn't find it quickly
so I decided to define a function for that :) Will switch to clzXX in the patch 
set -
I believe there are three occurrences where ilog2 is used in the added code.