[Qemu-devel] [PATCH 0/2] import nvme fix

[Qemu-devel] [PATCH 0/2] import nvme fix

[Christoph Hellwig]

First one fixes Identify to behave as mandated by the spec, and the
second bumps the PCI revision so that guest drivers can detect
the fixed version of the device so that only the old version has
to be blacklisted.

[Qemu-devel] [PATCH 1/2] 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 | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 52 insertions(+), 7 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5da41b2..4a6443f 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,48 @@ 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, j = 0;
+
+    list = g_malloc0(data_len);
+    for (i = 0; i < n->num_namespaces; i++) {
+        if (i <= min_nsid) {
+            continue;
+        }
+        list[j++] = cpu_to_le32(i);
+        if (j == data_len / sizeof(uint32_t)) {
+            break;
+        }
+    }
+    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 1/2] nvme: fix identify to be NVMe 1.1 compliant

[Christoph Hellwig]

Meh, this was still missing the uncommited changes for the nsid
off by one vs the array index:

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 360be71..4f768d5 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -499,10 +499,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)
 
     list = g_malloc0(data_len);
     for (i = 0; i < n->num_namespaces; i++) {
-        if (i <= min_nsid) {
+        if (i < min_nsid) {
             continue;
         }
-        list[j++] = cpu_to_le32(i);
+        list[j++] = cpu_to_le32(i + 1);
         if (j == data_len / sizeof(uint32_t)) {
             break;
         }

[Qemu-devel] [PATCH 2/2] nvme: bump PCI revision

[Christoph Hellwig]

The broken Identify implementation in earlier Qemu versions means we
need to blacklist it from issueing the NVMe 1.1 Identify Namespace List
command.  As we want to be able to use it in newer Qemu versions we need
a way to identify those.  Bump the PCI revision as a guest visible
indicator of this bug fix.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 4a6443f..360be71 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -946,7 +946,7 @@ static void nvme_class_init(ObjectClass *oc, void *data)
     pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
     pc->vendor_id = PCI_VENDOR_ID_INTEL;
     pc->device_id = 0x5845;
-    pc->revision = 1;
+    pc->revision = 2;
     pc->is_express = 1;
 
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-- 
1.9.1

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] import nvme fix

[Kevin Wolf]

Am 18.11.2015 um 19:53 hat Christoph Hellwig geschrieben:
> First one fixes Identify to behave as mandated by the spec, and the
> second bumps the PCI revision so that guest drivers can detect
> the fixed version of the device so that only the old version has
> to be blacklisted.

Keith, this looks to me like a fix that should still be merged for 2.5,
would you agree? Can you please have a look at the series and either
give your Acked-by or comment?

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] import nvme fix

[Keith Busch]

On Wed, Nov 25, 2015 at 10:28:42AM +0100, Kevin Wolf wrote:
> Am 18.11.2015 um 19:53 hat Christoph Hellwig geschrieben:
> > First one fixes Identify to behave as mandated by the spec, and the
> > second bumps the PCI revision so that guest drivers can detect
> > the fixed version of the device so that only the old version has
> > to be blacklisted.
> 
> Keith, this looks to me like a fix that should still be merged for 2.5,
> would you agree? Can you please have a look at the series and either
> give your Acked-by or comment?

The series looks good to me. I had some difficulty finding the right
patches in the midst of Christoph's Linux patch bombs. :)

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

[Qemu-devel] fix incorrect identify implementation in nvme

[Christoph Hellwig]

Third resent of this series after this didn't get picked up the
previous times.  The Qemu NVMe implementation mistakes the cns
field in the Identify command as a boolean.  This was never
true, and is actively harmful since NVMe1.1 (which the Qemu
device claims to support) supports more than two Identify variants.

We had to add a quirk in Linux to work around this behavior.

[Qemu-devel] [PATCH 1/2] 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 | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 52 insertions(+), 7 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 2ded247..a0655a3 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -469,19 +469,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;
     }
@@ -491,6 +494,48 @@ 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, j = 0;
+
+    list = g_malloc0(data_len);
+    for (i = 0; i < n->num_namespaces; i++) {
+        if (i < min_nsid) {
+            continue;
+        }
+        list[j++] = cpu_to_le32(i + 1);
+        if (j == data_len / sizeof(uint32_t)) {
+            break;
+        }
+    }
+    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);
-- 
2.1.4