[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH V3 8/8] hw/block/nvme: Add Identify Active Namespace ID L
From: |
Klaus Jensen |
Subject: |
Re: [RFC PATCH V3 8/8] hw/block/nvme: Add Identify Active Namespace ID List |
Date: |
Wed, 20 Jan 2021 15:17:34 +0100 |
On Jan 20 14:07, Niklas Cassel wrote:
> On Wed, Jan 20, 2021 at 02:01:47AM +0900, Minwoo Im wrote:
> > Spec v1.4b 6.1.4 "Active and Inactive NSID Types" says:
> >
> > "Active NSIDs for a controller refer to namespaces that are attached to
> > that controller. Allocated NSIDs that are inactive for a controller refer
> > to namespaces that are not attached to that controller."
> >
> > This patch introduced for Identify Active Namespace ID List (CNS 02h).
> >
> > Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> > ---
> > hw/block/nvme.c | 39 ++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 34 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 2b2c07b36c2b..7247167b0ee6 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -2883,6 +2883,39 @@ static uint16_t nvme_identify_ns_csi(NvmeCtrl *n,
> > NvmeRequest *req)
> > return NVME_INVALID_FIELD | NVME_DNR;
> > }
> >
> > +static uint16_t nvme_identify_nslist_active(NvmeCtrl *n, NvmeRequest *req)
> > +{
> > + NvmeNamespace *ns;
> > + NvmeIdentify *c = (NvmeIdentify *)&req->cmd;
> > + uint32_t min_nsid = le32_to_cpu(c->nsid);
> > + uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {};
> > + static const int data_len = sizeof(list);
> > + uint32_t *list_ptr = (uint32_t *)list;
> > + int i, j = 0;
> > +
> > + if (min_nsid >= NVME_NSID_BROADCAST - 1) {
> > + return NVME_INVALID_NSID | NVME_DNR;
> > + }
> > +
> > + for (i = 1; i <= n->num_namespaces; i++) {
> > + ns = nvme_ns(n, i);
> > + if (!ns || ns->params.nsid <= min_nsid) {
> > + continue;
> > + }
> > +
> > + if (!nvme_ns_is_attached(n, ns)) {
> > + continue;
> > + }
> > +
> > + list_ptr[j++] = cpu_to_le32(ns->params.nsid);
> > + if (j == data_len / sizeof(uint32_t)) {
> > + break;
> > + }
> > + }
> > +
> > + return nvme_dma(n, list, data_len, DMA_DIRECTION_FROM_DEVICE, req);
> > +}
> > +
> > static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeRequest *req)
> > {
> > NvmeNamespace *ns;
> > @@ -2914,10 +2947,6 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n,
> > NvmeRequest *req)
> > continue;
> > }
> >
> > - if (!nvme_ns_is_attached(n, ns)) {
> > - continue;
> > - }
> > -
> > list_ptr[j++] = cpu_to_le32(ns->params.nsid);
> > if (j == data_len / sizeof(uint32_t)) {
> > break;
> > @@ -3045,7 +3074,7 @@ static uint16_t nvme_identify(NvmeCtrl *n,
> > NvmeRequest *req)
> > case NVME_ID_CNS_CS_CTRL:
> > return nvme_identify_ctrl_csi(n, req);
> > case NVME_ID_CNS_NS_ACTIVE_LIST:
> > - /* fall through */
> > + return nvme_identify_nslist_active(n, req);
> > case NVME_ID_CNS_NS_PRESENT_LIST:
> > return nvme_identify_nslist(n, req);
> > case NVME_ID_CNS_CS_NS_ACTIVE_LIST:
> > --
> > 2.17.1
> >
> >
>
> Hello Minwoo,
>
> By introducing a detached parameter,
> you are also implicitly making the following
> NVMe commands no longer be spec compliant:
>
> NVME_ID_CNS_NS, NVME_ID_CNS_CS_NS,
> NVME_ID_CNS_NS_ACTIVE_LIST, NVME_ID_CNS_CS_NS_ACTIVE_LIST
>
> When these commands are called on a detached namespace,
> they should usually return a zero filled data struct.
>
> Dmitry and I had a patch on V8 on the ZNS series
> that tried to fix some the existing NVMe commands
> to be spec compliant, by handling detached namespaces
> properly. In the end, in order to make it easier to
> get the ZNS series accepted, we decided to drop the
> detached related stuff from the series.
>
> Feel free to look at that patch for inspiration:
> https://github.com/dmitry-fomichev/qemu/commit/251c0ffee5149c739b1347811fa7e32a1c36bf7c
>
> I'm not sure if you want to modify all the functions that
> our patch modifies, but I think that you should at least
> modify the following nvme functions:
>
> nvme_identify_ns()
> nvme_identify_ns_csi()
> nvme_identify_nslist()
> nvme_identify_nslist_csi()
>
> So they handle detached namespaces correctly for both:
> NVME_ID_CNS_NS, NVME_ID_CNS_CS_NS,
> NVME_ID_CNS_NS_ACTIVE_LIST, NVME_ID_CNS_CS_NS_ACTIVE_LIST
> as well as for:
> NVME_ID_CNS_NS_PRESENT, NVME_ID_CNS_CS_NS_PRESENT,
> NVME_ID_CNS_NS_PRESENT_LIST, NVME_ID_CNS_CS_NS_PRESENT_LIST
>
Definitely - it makes sense to reintroduce your patch here, with a
replacement of `ns->attached` with `nvme_ns_is_attach()`. Looks like
that should be sufficient.
signature.asc
Description: PGP signature
- [RFC PATCH V3 2/8] hw/block/nvme: support to map controller to a subsystem, (continued)
- [RFC PATCH V3 2/8] hw/block/nvme: support to map controller to a subsystem, Minwoo Im, 2021/01/19
- [RFC PATCH V3 3/8] hw/block/nvme: add CMIC enum value for Identify Controller, Minwoo Im, 2021/01/19
- [RFC PATCH V3 4/8] hw/block/nvme: support for multi-controller in subsystem, Minwoo Im, 2021/01/19
- [RFC PATCH V3 5/8] hw/block/nvme: add NMIC enum value for Identify Namespace, Minwoo Im, 2021/01/19
- [RFC PATCH V3 7/8] hw/block/nvme: add 'detached' param not to attach namespace, Minwoo Im, 2021/01/19
- [RFC PATCH V3 6/8] hw/block/nvme: support for shared namespace in subsystem, Minwoo Im, 2021/01/19
- [RFC PATCH V3 8/8] hw/block/nvme: Add Identify Active Namespace ID List, Minwoo Im, 2021/01/19
- Re: [RFC PATCH V3 0/8] hw/block/nvme: support multi-path for ctrl/ns, Klaus Jensen, 2021/01/19