On Thu, Jul 04, 2024 at 08:20:31PM +0200, Stefan Hajnoczi wrote:
> On Thu, Jun 13, 2024 at 03:13:25PM +0800, Changqi Lu wrote:
> > This commit enables ONCS to support the reservation
> > function at the controller level. Also enables rescap
> > function in the namespace by detecting the supported reservation
> > function in the backend driver.
> >
> > Signed-off-by: Changqi Lu <luchangqi.123@bytedance.com>
> > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> > ---
> > hw/nvme/ctrl.c | 3 ++-
> > hw/nvme/ns.c | 5 +++++
> > 2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 127c3d2383..182307a48b 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -8248,7 +8248,8 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
> > id->nn = cpu_to_le32(NVME_MAX_NAMESPACES);
> > id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROES | NVME_ONCS_TIMESTAMP |
> > NVME_ONCS_FEATURES | NVME_ONCS_DSM |
> > - NVME_ONCS_COMPARE | NVME_ONCS_COPY);
> > + NVME_ONCS_COMPARE | NVME_ONCS_COPY |
> > + NVME_ONCS_RESRVATIONS);
>
> RESRVATIONS -> RESERVATIONS typo?
>
> >
> > /*
> > * NOTE: If this device ever supports a command set that does NOT use 0x0
> > diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
> > index ea8db175db..320c9bf658 100644
> > --- a/hw/nvme/ns.c
> > +++ b/hw/nvme/ns.c
> > @@ -20,6 +20,7 @@
> > #include "qemu/bitops.h"
> > #include "sysemu/sysemu.h"
> > #include "sysemu/block-backend.h"
> > +#include "block/block_int.h"
> >
> > #include "nvme.h"
> > #include "trace.h"
> > @@ -33,6 +34,7 @@ void nvme_ns_init_format(NvmeNamespace *ns)
> > BlockDriverInfo bdi;
> > int npdg, ret;
> > int64_t nlbas;
> > + uint8_t blk_pr_cap;
> >
> > ns->lbaf = id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(id_ns->flbas)];
> > ns->lbasz = 1 << ns->lbaf.ds;
> > @@ -55,6 +57,9 @@ void nvme_ns_init_format(NvmeNamespace *ns)
> > }
> >
> > id_ns->npda = id_ns->npdg = npdg - 1;
> > +
> > + blk_pr_cap = blk_bs(ns->blkconf.blk)->file->bs->bl.pr_cap;
>
> Kevin: This unprotected block graph access and the assumption that
> ->file->bs exists could be problematic. What is the best practice for
> making this code safe and defensive?
I posted the following reply in another sub-thread and it seems worth
mentioning here:
"->file could be NULL if the SCSI disk points directly to
--blockdev file without a --blockdev raw on top. I think the block layer
should propagate pr_cap from the leaves of the block graph to the root
node via bdrv_merge_limits() so that traversing the graph (->file) is
not necessary. Instead this line should just be bs->bl.pr_cap."
I think ->file shouldn't be accessed at all. That also sidesteps the
block graph locking question.
>
> > + id_ns->rescap = block_pr_cap_to_nvme(blk_pr_cap);
> > }
> >
> > static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
> > --
> > 2.20.1
> >