Re: [Qemu-devel] [PATCH 16/16] nvme: support multiple namespaces

2019-11-04 Thread Ross Lagerwall 
On 11/4/19 9:04 AM, Klaus Birkelund wrote:
> On Mon, Nov 04, 2019 at 08:46:29AM +, Ross Lagerwall wrote:
>> On 8/23/19 9:10 AM, Klaus Birkelund wrote:
>>> On Thu, Aug 22, 2019 at 02:18:05PM +0100, Ross Lagerwall wrote:
 On 7/5/19 8:23 AM, Klaus Birkelund Jensen wrote:

 I tried this patch series by installing Windows with a single NVME
 controller having two namespaces. QEMU crashed in get_feature /
 NVME_VOLATILE_WRITE_CACHE because req->ns was NULL.

>>>
>>> Hi Ross,
>>>
>>> Good catch!
>>>
 nvme_get_feature / nvme_set_feature look wrong to me since I can't see how
 req->ns would have been set. Should they have similar code to nvme_io_cmd 
 to
 set req->ns from cmd->nsid?
>>>
>>> Definitely. I will fix that for v2.
>>>

 After working around this issue everything else seemed to be working well.
 Thanks for your work on this patch series.

>>>
>>> And thank you for trying out my patches!
>>>
>>
>> One more thing... it doesn't handle inactive namespaces properly so if you
>> have two namespaces with e.g. nsid=1 and nsid=3 QEMU ends up crashing in
>> certain situations. The patch below adds support for inactive namespaces.
>>
>> Still hoping to see a v2 some day :-)
>>
>  
> Hi Ross,
> 
> v2[1] is actually out, but only CC'ed Paul. Sorry about that! It fixes
> the support for discontiguous nsid's, but does not handle inactive
> namespaces correctly in identify.
> 
> I'll incorporate that in a v3 along with a couple of other fixes I did.
> 
> Thanks!
> 
> 
>   [1]: https://patchwork.kernel.org/cover/11190045/
> 

Thanks for pointing me at that, I missed it.

Regards,
-- 
Ross Lagerwall

Re: [Qemu-devel] [PATCH 16/16] nvme: support multiple namespaces

2019-11-04 Thread Klaus Birkelund 
On Mon, Nov 04, 2019 at 08:46:29AM +, Ross Lagerwall wrote:
> On 8/23/19 9:10 AM, Klaus Birkelund wrote:
> > On Thu, Aug 22, 2019 at 02:18:05PM +0100, Ross Lagerwall wrote:
> >> On 7/5/19 8:23 AM, Klaus Birkelund Jensen wrote:
> >>
> >> I tried this patch series by installing Windows with a single NVME
> >> controller having two namespaces. QEMU crashed in get_feature /
> >> NVME_VOLATILE_WRITE_CACHE because req->ns was NULL.
> >>
> > 
> > Hi Ross,
> > 
> > Good catch!
> > 
> >> nvme_get_feature / nvme_set_feature look wrong to me since I can't see how
> >> req->ns would have been set. Should they have similar code to nvme_io_cmd 
> >> to
> >> set req->ns from cmd->nsid?
> > 
> > Definitely. I will fix that for v2.
> > 
> >>
> >> After working around this issue everything else seemed to be working well.
> >> Thanks for your work on this patch series.
> >>
> > 
> > And thank you for trying out my patches!
> > 
> 
> One more thing... it doesn't handle inactive namespaces properly so if you
> have two namespaces with e.g. nsid=1 and nsid=3 QEMU ends up crashing in
> certain situations. The patch below adds support for inactive namespaces.
> 
> Still hoping to see a v2 some day :-)
> 
 
Hi Ross,

v2[1] is actually out, but only CC'ed Paul. Sorry about that! It fixes
the support for discontiguous nsid's, but does not handle inactive
namespaces correctly in identify.

I'll incorporate that in a v3 along with a couple of other fixes I did.

Thanks!


  [1]: https://patchwork.kernel.org/cover/11190045/

Re: [Qemu-devel] [PATCH 16/16] nvme: support multiple namespaces

2019-11-04 Thread Ross Lagerwall 
On 8/23/19 9:10 AM, Klaus Birkelund wrote:
> On Thu, Aug 22, 2019 at 02:18:05PM +0100, Ross Lagerwall wrote:
>> On 7/5/19 8:23 AM, Klaus Birkelund Jensen wrote:
>>
>> I tried this patch series by installing Windows with a single NVME
>> controller having two namespaces. QEMU crashed in get_feature /
>> NVME_VOLATILE_WRITE_CACHE because req->ns was NULL.
>>
> 
> Hi Ross,
> 
> Good catch!
> 
>> nvme_get_feature / nvme_set_feature look wrong to me since I can't see how
>> req->ns would have been set. Should they have similar code to nvme_io_cmd to
>> set req->ns from cmd->nsid?
> 
> Definitely. I will fix that for v2.
> 
>>
>> After working around this issue everything else seemed to be working well.
>> Thanks for your work on this patch series.
>>
> 
> And thank you for trying out my patches!
> 

One more thing... it doesn't handle inactive namespaces properly so if you
have two namespaces with e.g. nsid=1 and nsid=3 QEMU ends up crashing in
certain situations. The patch below adds support for inactive namespaces.

Still hoping to see a v2 some day :-)

Thanks,
Ross

-- 8-< --
nvme-ns: Allow inactive namespaces

The controller advertises the maximum namespace number but not all of these
slots may have proper namespaces. These are defined as inactive namespaces by
the spec.  Implement support for inactive namespaces instead of crashing.

Changes are needed in a few places:
* When identify_ns is used with an inactive namespace, the controller should
  return all zeroes.
* Only active namespaces should be returned by identify_ns_list.
* When the controller is unplugged, only cleanup active namespaces.
* Keep track of and advertise the maximum valid namespace number rather than
* the number of active namespaces.
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 1b8a09853d..29ea5c2023 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1302,6 +1302,7 @@ static uint16_t nvme_identify_ctrl(NvmeCtrl *n, NvmeCmd 
*cmd, NvmeRequest *req)
 static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
 {
 NvmeNamespace *ns;
+NvmeIdNs *id_ns, invalid_ns_id = {0};
 uint32_t nsid = le32_to_cpu(cmd->nsid);
 
 trace_nvme_identify_ns(nsid);
@@ -1312,9 +1313,13 @@ static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeCmd 
*cmd, NvmeRequest *req)
 }
 
 ns = n->namespaces[nsid - 1];
+if (ns) {
+id_ns = >id_ns;
+} else {
+id_ns = _ns_id;
+}
 
-return nvme_dma_read(n, (uint8_t *) >id_ns, sizeof(ns->id_ns), cmd,
-req);
+return nvme_dma_read(n, (uint8_t *) id_ns, sizeof(*id_ns), cmd, req);
 }
 
 static uint16_t nvme_identify_ns_list(NvmeCtrl *n, NvmeCmd *cmd,
@@ -1330,7 +1335,7 @@ static uint16_t nvme_identify_ns_list(NvmeCtrl *n, 
NvmeCmd *cmd,
 
 list = g_malloc0(data_len);
 for (i = 0; i < n->num_namespaces; i++) {
-if (i < min_nsid) {
+if (i < min_nsid || !n->namespaces[i]) {
 continue;
 }
 list[j++] = cpu_to_le32(i + 1);
@@ -1861,7 +1866,9 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
 int i;
 
 for (i = 0; i < n->num_namespaces; i++) {
-blk_drain(n->namespaces[i]->conf.blk);
+if (n->namespaces[i]) {
+blk_drain(n->namespaces[i]->conf.blk);
+}
 }
 
 for (i = 0; i < n->params.num_queues; i++) {
@@ -1886,7 +1893,9 @@ static void nvme_clear_ctrl(NvmeCtrl *n)
 }
 
 for (i = 0; i < n->num_namespaces; i++) {
-blk_flush(n->namespaces[i]->conf.blk);
+if (n->namespaces[i]) {
+blk_flush(n->namespaces[i]->conf.blk);
+}
 }
 
 n->bar.cc = 0;
@@ -2464,8 +2473,9 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace 
*ns, Error **errp)
 trace_nvme_register_namespace(nsid);
 
 n->namespaces[nsid - 1] = ns;
-n->num_namespaces++;
-n->id_ctrl.nn++;
+if (nsid > n->num_namespaces)
+n->num_namespaces = nsid;
+n->id_ctrl.nn = n->num_namespaces;
 
 return 0;
 }

Re: [Qemu-devel] [PATCH 16/16] nvme: support multiple namespaces

2019-08-23 Thread Klaus Birkelund 
On Thu, Aug 22, 2019 at 02:18:05PM +0100, Ross Lagerwall wrote:
> On 7/5/19 8:23 AM, Klaus Birkelund Jensen wrote:
> 
> I tried this patch series by installing Windows with a single NVME
> controller having two namespaces. QEMU crashed in get_feature /
> NVME_VOLATILE_WRITE_CACHE because req->ns was NULL.
> 

Hi Ross,

Good catch!

> nvme_get_feature / nvme_set_feature look wrong to me since I can't see how
> req->ns would have been set. Should they have similar code to nvme_io_cmd to
> set req->ns from cmd->nsid?

Definitely. I will fix that for v2.

> 
> After working around this issue everything else seemed to be working well.
> Thanks for your work on this patch series.
> 

And thank you for trying out my patches!


Cheers,
Klaus

Re: [Qemu-devel] [PATCH 16/16] nvme: support multiple namespaces

2019-08-22 Thread Ross Lagerwall 

On 7/5/19 8:23 AM, Klaus Birkelund Jensen wrote:

This adds support for multiple namespaces by introducing a new 'nvme-ns'
device model. The nvme device creates a bus named from the device name
('id'). The nvme-ns devices then connect to this and registers
themselves with the nvme device.

This changes how an nvme device is created. Example with two namespaces:

   -drive file=nvme0n1.img,if=none,id=disk1
   -drive file=nvme0n2.img,if=none,id=disk2
   -device nvme,serial=deadbeef,id=nvme0
   -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
   -device nvme-ns,drive=disk2,bus=nvme0,nsid=2

A maximum of 256 namespaces can be configured.


snip

  static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
  {
+NvmeNamespace *ns = req->ns;
+
  uint32_t dw10 = le32_to_cpu(cmd->cdw10);
  uint32_t dw11 = le32_to_cpu(cmd->cdw11);
  uint32_t result;
@@ -1464,7 +1474,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd 
*cmd, NvmeRequest *req)
  result = cpu_to_le32(n->features.err_rec);
  break;
  case NVME_VOLATILE_WRITE_CACHE:
-result = blk_enable_write_cache(n->conf.blk);
+result = blk_enable_write_cache(ns->conf.blk);
  trace_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
  break;


I tried this patch series by installing Windows with a single NVME 
controller having two namespaces. QEMU crashed in get_feature / 
NVME_VOLATILE_WRITE_CACHE because req->ns was NULL.


nvme_get_feature / nvme_set_feature look wrong to me since I can't see 
how req->ns would have been set. Should they have similar code to 
nvme_io_cmd to set req->ns from cmd->nsid?


After working around this issue everything else seemed to be working 
well. Thanks for your work on this patch series.


Thanks,
--
Ross Lagerwall

[Qemu-devel] [PATCH 16/16] nvme: support multiple namespaces

2019-07-05 Thread Klaus Birkelund Jensen 
This adds support for multiple namespaces by introducing a new 'nvme-ns'
device model. The nvme device creates a bus named from the device name
('id'). The nvme-ns devices then connect to this and registers
themselves with the nvme device.

This changes how an nvme device is created. Example with two namespaces:

  -drive file=nvme0n1.img,if=none,id=disk1
  -drive file=nvme0n2.img,if=none,id=disk2
  -device nvme,serial=deadbeef,id=nvme0
  -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
  -device nvme-ns,drive=disk2,bus=nvme0,nsid=2

A maximum of 256 namespaces can be configured.

Signed-off-by: Klaus Birkelund Jensen 
---
 hw/block/Makefile.objs |   2 +-
 hw/block/nvme-ns.c | 139 +
 hw/block/nvme-ns.h |  35 +
 hw/block/nvme.c| 169 -
 hw/block/nvme.h|  29 ---
 hw/block/trace-events  |   1 +
 6 files changed, 255 insertions(+), 120 deletions(-)
 create mode 100644 hw/block/nvme-ns.c
 create mode 100644 hw/block/nvme-ns.h

diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
index f5f643f0cc06..d44a2f4b780d 100644
--- a/hw/block/Makefile.objs
+++ b/hw/block/Makefile.objs
@@ -7,7 +7,7 @@ common-obj-$(CONFIG_PFLASH_CFI02) += pflash_cfi02.o
 common-obj-$(CONFIG_XEN) += xen-block.o
 common-obj-$(CONFIG_ECC) += ecc.o
 common-obj-$(CONFIG_ONENAND) += onenand.o
-common-obj-$(CONFIG_NVME_PCI) += nvme.o
+common-obj-$(CONFIG_NVME_PCI) += nvme.o nvme-ns.o
 
 obj-$(CONFIG_SH4) += tc58128.o
 
diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
new file mode 100644
index ..11b594467991
--- /dev/null
+++ b/hw/block/nvme-ns.c
@@ -0,0 +1,139 @@
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+#include "qemu/cutils.h"
+#include "qemu/log.h"
+#include "hw/block/block.h"
+#include "hw/pci/msix.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/block-backend.h"
+#include "qapi/error.h"
+
+#include "hw/qdev-core.h"
+
+#include "nvme.h"
+#include "nvme-ns.h"
+
+static uint64_t nvme_ns_calc_blks(NvmeNamespace *ns)
+{
+return ns->size / nvme_ns_lbads_bytes(ns);
+}
+
+static void nvme_ns_init_identify(NvmeIdNs *id_ns)
+{
+id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
+}
+
+static int nvme_ns_init(NvmeNamespace *ns)
+{
+uint64_t ns_blks;
+NvmeIdNs *id_ns = >id_ns;
+
+nvme_ns_init_identify(id_ns);
+
+ns_blks = nvme_ns_calc_blks(ns);
+id_ns->nuse = id_ns->ncap = id_ns->nsze = cpu_to_le64(ns_blks);
+
+return 0;
+}
+
+static int nvme_ns_init_blk(NvmeNamespace *ns, NvmeIdCtrl *id, Error **errp)
+{
+blkconf_blocksizes(>conf);
+
+if (!blkconf_apply_backend_options(>conf,
+blk_is_read_only(ns->conf.blk), false, errp)) {
+return 1;
+}
+
+ns->size = blk_getlength(ns->conf.blk);
+if (ns->size < 0) {
+error_setg_errno(errp, -ns->size, "blk_getlength");
+return 1;
+}
+
+if (!blk_enable_write_cache(ns->conf.blk)) {
+id->vwc = 0;
+}
+
+return 0;
+}
+
+static int nvme_ns_check_constraints(NvmeNamespace *ns, Error **errp)
+{
+if (!ns->conf.blk) {
+error_setg(errp, "nvme-ns: block backend not configured");
+return 1;
+}
+
+return 0;
+}
+
+
+static void nvme_ns_realize(DeviceState *dev, Error **errp)
+{
+NvmeNamespace *ns = NVME_NS(dev);
+BusState *s = qdev_get_parent_bus(dev);
+NvmeCtrl *n = NVME(s->parent);
+Error *local_err = NULL;
+
+if (nvme_ns_check_constraints(ns, _err)) {
+error_propagate_prepend(errp, local_err,
+"nvme_ns_check_constraints: ");
+return;
+}
+
+if (nvme_ns_init_blk(ns, >id_ctrl, _err)) {
+error_propagate_prepend(errp, local_err, "nvme_ns_init_blk: ");
+return;
+}
+
+nvme_ns_init(ns);
+if (nvme_register_namespace(n, ns, _err)) {
+error_propagate_prepend(errp, local_err, "nvme_register_namespace: ");
+return;
+}
+}
+
+static Property nvme_ns_props[] = {
+DEFINE_BLOCK_PROPERTIES(NvmeNamespace, conf),
+DEFINE_NVME_NS_PROPERTIES(NvmeNamespace, params),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void nvme_ns_class_init(ObjectClass *oc, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(oc);
+
+set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+
+dc->bus_type = TYPE_NVME_BUS;
+dc->realize = nvme_ns_realize;
+dc->props = nvme_ns_props;
+dc->desc = "virtual nvme namespace";
+}
+
+static void nvme_ns_instance_init(Object *obj)
+{
+NvmeNamespace *ns = NVME_NS(obj);
+char *bootindex = g_strdup_printf("/namespace@%d,0", ns->params.nsid);
+
+device_add_bootindex_property(obj, >conf.bootindex, "bootindex",
+bootindex, DEVICE(obj), _abort);
+
+g_free(bootindex);
+}
+
+static const TypeInfo nvme_ns_info = {
+.name = TYPE_NVME_NS,
+.parent = TYPE_DEVICE,
+.class_init = nvme_ns_class_init,
+.instance_size = sizeof(NvmeNamespace),
+.instance_init = nvme_ns_instance_init,
+};
+
+static void nvme_ns_register_types(void)
+{
+