| Message ID | 20210119170147.19657-9-minwoo.im.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/block/nvme: support multi-path for ctrl/ns | expand |
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 Kind regards, Niklas
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.
> 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. Agreed. > 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've seen this patch and as Klaus said, only thing patches need go with is to put some of nvme_ns_is_attached() helper among the Identify handlers. > 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() Yes, pretty makes sense to update them. But now it seems like 'attach/detach' scheme should have been separated out of this series which just introduced the multi-path for controllers and namespace sharing. I will drop this 'detach' scheme out of this series and make another series to support all of the Identify you mentioned above cleanly. > 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 > > > Kind regards, > Niklas
On Thu, Jan 21, 2021 at 06:58:19AM +0900, Minwoo Im wrote: > > 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. > > Agreed. > > > 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've seen this patch and as Klaus said, only thing patches need go with > is to put some of nvme_ns_is_attached() helper among the Identify > handlers. > > > 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() > > Yes, pretty makes sense to update them. But now it seems like > 'attach/detach' scheme should have been separated out of this series > which just introduced the multi-path for controllers and namespace > sharing. I will drop this 'detach' scheme out of this series and make > another series to support all of the Identify you mentioned above > cleanly. Hello Minwoo, thank you for putting in work on this! Kind regards, Niklas
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:
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(-)