diff mbox series

[v6,08/10] hw/nvme: enable ONCS and rescap function

Message ID 20240613071327.2498953-9-luchangqi.123@bytedance.com (mailing list archive)
State New, archived
Headers show
Series Support persistent reservation operations | expand

Commit Message

Changqi Lu June 13, 2024, 7:13 a.m. UTC 
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(-)

Comments

Stefan Hajnoczi July 4, 2024, 6:20 p.m. UTC | #1 
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?

> +    id_ns->rescap = block_pr_cap_to_nvme(blk_pr_cap);
>  }
>  
>  static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
> -- 
> 2.20.1
>
Stefan Hajnoczi July 5, 2024, 10:22 a.m. UTC | #2 
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
> >
diff mbox series

Patch

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);
 
     /*
      * 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;
+    id_ns->rescap = block_pr_cap_to_nvme(blk_pr_cap);
 }
 
 static int nvme_ns_init(NvmeNamespace *ns, Error **errp)