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....@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