[Qemu-devel] (no subject)

[Qemu-devel] (no subject)

[Christoph Hellwig]

From: Christoph Hellwig <hch@lst.de>
Subject: a nasty nvme fix
In-Reply-To: 

Hi all,

below is a fix for a bug in the qemu NVMe identify implementation that's
causing us some trouble with an updated Linux driver.  We'll have to
blacklist the existing Qemu device ID for it, so I wonder how we can
advertize a fixed controller.  Maybe a new PCI ID?  Or maybe just bump
the PCI revision, altough that would be a bit more complicated in the
driver.

[Qemu-devel] [PATCH] nvme: fix identify to be NVMe 1.1 compliant

[Christoph Hellwig]

NVMe 1.1 requires devices to implement a Namespace List subcommand of
the identify command.  Qemu not only not implements this features, but
also misinterprets it as an Identify Controller request.  Due to this
any OS trying to use the Namespace List will fail the probe.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 hw/block/nvme.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 7 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5da41b2..d5717d3 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -462,19 +462,22 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd *cmd)
     return NVME_SUCCESS;
 }
 
-static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
+static uint16_t nvme_identify_ctrl(NvmeCtrl *n, NvmeIdentify *c)
+{
+    uint64_t prp1 = le64_to_cpu(c->prp1);
+    uint64_t prp2 = le64_to_cpu(c->prp2);
+
+    return nvme_dma_read_prp(n, (uint8_t *)&n->id_ctrl, sizeof(n->id_ctrl),
+        prp1, prp2);
+}
+
+static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeIdentify *c)
 {
     NvmeNamespace *ns;
-    NvmeIdentify *c = (NvmeIdentify *)cmd;
-    uint32_t cns  = le32_to_cpu(c->cns);
     uint32_t nsid = le32_to_cpu(c->nsid);
     uint64_t prp1 = le64_to_cpu(c->prp1);
     uint64_t prp2 = le64_to_cpu(c->prp2);
 
-    if (cns) {
-        return nvme_dma_read_prp(n, (uint8_t *)&n->id_ctrl, sizeof(n->id_ctrl),
-            prp1, prp2);
-    }
     if (nsid == 0 || nsid > n->num_namespaces) {
         return NVME_INVALID_NSID | NVME_DNR;
     }
@@ -484,6 +487,50 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
         prp1, prp2);
 }
 
+static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
+{
+    static const int data_len = 4096;
+    uint32_t min_nsid = le32_to_cpu(c->nsid);
+    uint64_t prp1 = le64_to_cpu(c->prp1);
+    uint64_t prp2 = le64_to_cpu(c->prp2);
+    uint32_t *list;
+    uint16_t ret;
+    int i;
+
+    list = g_malloc(data_len);
+    for (i = 0; i < n->num_namespaces; i++) {
+        if (i <= min_nsid) {
+            continue;
+        }
+        list[i] = i;
+        if (i == data_len / sizeof(uint32_t)) {
+            goto out;
+        }
+    }
+    list[i] = 0;
+out:
+    ret = nvme_dma_read_prp(n, (uint8_t *)list, data_len, prp1, prp2);
+    g_free(list);
+    return ret;
+}
+
+
+static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
+{
+    NvmeIdentify *c = (NvmeIdentify *)cmd;
+
+    switch (le32_to_cpu(c->cns)) {
+    case 0x00:
+        return nvme_identify_ns(n, c);
+    case 0x01:
+        return nvme_identify_ctrl(n, c);
+    case 0x02:
+        return nvme_identify_nslist(n, c);
+    default:
+        return NVME_INVALID_FIELD | NVME_DNR;
+    }
+}
+
 static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
 {
     uint32_t dw10 = le32_to_cpu(cmd->cdw10);
-- 
1.9.1

Re: [Qemu-devel] [PATCH] nvme: fix identify to be NVMe 1.1 compliant

[Keith Busch]

On Tue, Nov 17, 2015 at 02:08:09PM +0100, Christoph Hellwig wrote:
> NVMe 1.1 requires devices to implement a Namespace List subcommand of
> the identify command.  Qemu not only not implements this features, but
> also misinterprets it as an Identify Controller request.  Due to this
> any OS trying to use the Namespace List will fail the probe.

Thanks for the fix! Minor comment below, no need to fix.

Acked-by: Keith Busch <keith.busch@intel.com>

Re: [Qemu-devel] (no subject)

[Paolo Bonzini]

On 17/11/2015 14:08, Christoph Hellwig wrote:
> below is a fix for a bug in the qemu NVMe identify implementation that's
> causing us some trouble with an updated Linux driver.  We'll have to
> blacklist the existing Qemu device ID for it, so I wonder how we can
> advertize a fixed controller.  Maybe a new PCI ID?  Or maybe just bump
> the PCI revision, altough that would be a bit more complicated in the
> driver.

Bumping the PCI revision would be ideal, but I guess the PCI ID would
work too if it's really that bad.

Paolo

Re: [Qemu-devel] [PATCH] nvme: fix identify to be NVMe 1.1 compliant

[Busch, Keith]

> Thanks for the fix! Minor comment below, no need to fix.
> 
> Acked-by: Keith Busch <keith.busch@intel.com>

I accidently deleted my comment. Here's what it said:

+    list = g_malloc(data_len);
+    for (i = 0; i < n->num_namespaces; i++) {
+        if (i <= min_nsid) {
+            continue;
+        }
+        list[i] = i;

This should be:

+        list[i] = cpu_to_le32(i);

Re: [Qemu-devel] [PATCH] nvme: fix identify to be NVMe 1.1 compliant

[Keith Busch]

On Tue, Nov 17, 2015 at 09:33:11AM -0800, Busch, Keith wrote:
> I accidently deleted my comment. Here's what it said:
> 
> +    list = g_malloc(data_len);
> +    for (i = 0; i < n->num_namespaces; i++) {
> +        if (i <= min_nsid) {
> +            continue;
> +        }
> +        list[i] = i;
> 
> This should be:
> 
> +        list[i] = cpu_to_le32(i);

Just saw this: we can't use the raw 'i' for the list index. It could
return a badly formatted list if min_nsid is non-zero, or, even worse,
corrupt memory if num_namsepaces > 1024. Need to do this instead:

+        list[i - min_nsid] = cpu_to_le32(i);

Re: [Qemu-devel] [PATCH] nvme: fix identify to be NVMe 1.1 compliant

[Christoph Hellwig]

On Tue, Nov 17, 2015 at 05:41:04PM +0000, Keith Busch wrote:
> On Tue, Nov 17, 2015 at 09:33:11AM -0800, Busch, Keith wrote:
> > I accidently deleted my comment. Here's what it said:
> > 
> > +    list = g_malloc(data_len);
> > +    for (i = 0; i < n->num_namespaces; i++) {
> > +        if (i <= min_nsid) {
> > +            continue;
> > +        }
> > +        list[i] = i;
> > 
> > This should be:
> > 
> > +        list[i] = cpu_to_le32(i);
> 
> Just saw this: we can't use the raw 'i' for the list index. It could
> return a badly formatted list if min_nsid is non-zero, or, even worse,
> corrupt memory if num_namsepaces > 1024. Need to do this instead:
> 
> +        list[i - min_nsid] = cpu_to_le32(i);

Oh yes, І'll need to fix that up.