* [Qemu-devel] fix incorrect identify implementation in nvme @ 2016-08-04 19:42 Christoph Hellwig 2016-08-04 19:42 ` [Qemu-devel] [PATCH 1/2] nvme: fix identify to be NVMe 1.1 compliant Christoph Hellwig ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Christoph Hellwig @ 2016-08-04 19:42 UTC (permalink / raw) To: qemu-block; +Cc: keith.busch, qemu-devel 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 1/2] nvme: fix identify to be NVMe 1.1 compliant 2016-08-04 19:42 [Qemu-devel] fix incorrect identify implementation in nvme Christoph Hellwig @ 2016-08-04 19:42 ` Christoph Hellwig 2016-08-04 19:42 ` [Qemu-devel] [PATCH 2/2] nvme: bump PCI revision Christoph Hellwig 2016-08-04 20:14 ` [Qemu-devel] fix incorrect identify implementation in nvme Keith Busch 2 siblings, 0 replies; 6+ messages in thread From: Christoph Hellwig @ 2016-08-04 19:42 UTC (permalink / raw) To: qemu-block; +Cc: keith.busch, qemu-devel 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 2/2] nvme: bump PCI revision 2016-08-04 19:42 [Qemu-devel] fix incorrect identify implementation in nvme Christoph Hellwig 2016-08-04 19:42 ` [Qemu-devel] [PATCH 1/2] nvme: fix identify to be NVMe 1.1 compliant Christoph Hellwig @ 2016-08-04 19:42 ` Christoph Hellwig 2016-08-04 20:14 ` [Qemu-devel] fix incorrect identify implementation in nvme Keith Busch 2 siblings, 0 replies; 6+ messages in thread From: Christoph Hellwig @ 2016-08-04 19:42 UTC (permalink / raw) To: qemu-block; +Cc: keith.busch, qemu-devel 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 a0655a3..cef3bb4 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -954,7 +954,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); -- 2.1.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] fix incorrect identify implementation in nvme 2016-08-04 19:42 [Qemu-devel] fix incorrect identify implementation in nvme Christoph Hellwig 2016-08-04 19:42 ` [Qemu-devel] [PATCH 1/2] nvme: fix identify to be NVMe 1.1 compliant Christoph Hellwig 2016-08-04 19:42 ` [Qemu-devel] [PATCH 2/2] nvme: bump PCI revision Christoph Hellwig @ 2016-08-04 20:14 ` Keith Busch 2016-08-05 6:48 ` Markus Armbruster 2 siblings, 1 reply; 6+ messages in thread From: Keith Busch @ 2016-08-04 20:14 UTC (permalink / raw) To: Christoph Hellwig; +Cc: qemu-block, qemu-devel On Thu, Aug 04, 2016 at 09:42:13PM +0200, Christoph Hellwig wrote: > 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. Yes, these are great. Do we need to ping a maintainer to go through their tree, or can this be applied immediately? If need be, I can apply and send a pull request. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] fix incorrect identify implementation in nvme 2016-08-04 20:14 ` [Qemu-devel] fix incorrect identify implementation in nvme Keith Busch @ 2016-08-05 6:48 ` Markus Armbruster 2016-08-05 8:54 ` Kevin Wolf 0 siblings, 1 reply; 6+ messages in thread From: Markus Armbruster @ 2016-08-05 6:48 UTC (permalink / raw) To: Keith Busch Cc: Christoph Hellwig, qemu-devel, qemu-block, Kevin Wolf, Max Reitz Keith Busch <keith.busch@intel.com> writes: > On Thu, Aug 04, 2016 at 09:42:13PM +0200, Christoph Hellwig wrote: >> 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. > > Yes, these are great. Do we need to ping a maintainer to go through > their tree, or can this be applied immediately? If need be, I can apply > and send a pull request. $ scripts/get_maintainer.pl -f hw/block/nvme.c Keith Busch <keith.busch@intel.com> (supporter:nvme) Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core) Max Reitz <mreitz@redhat.com> (supporter:Block layer core) qemu-block@nongnu.org (open list:nvme) qemu-devel@nongnu.org (open list:All patches CC here) Send a pull request (assuming you have a properly signed PGP key). ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] fix incorrect identify implementation in nvme 2016-08-05 6:48 ` Markus Armbruster @ 2016-08-05 8:54 ` Kevin Wolf 0 siblings, 0 replies; 6+ messages in thread From: Kevin Wolf @ 2016-08-05 8:54 UTC (permalink / raw) To: Markus Armbruster Cc: Keith Busch, Christoph Hellwig, qemu-devel, qemu-block, Max Reitz Am 05.08.2016 um 08:48 hat Markus Armbruster geschrieben: > Keith Busch <keith.busch@intel.com> writes: > > > On Thu, Aug 04, 2016 at 09:42:13PM +0200, Christoph Hellwig wrote: > >> 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. > > > > Yes, these are great. Do we need to ping a maintainer to go through > > their tree, or can this be applied immediately? If need be, I can apply > > and send a pull request. > > $ scripts/get_maintainer.pl -f hw/block/nvme.c > Keith Busch <keith.busch@intel.com> (supporter:nvme) > Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core) > Max Reitz <mreitz@redhat.com> (supporter:Block layer core) > qemu-block@nongnu.org (open list:nvme) > qemu-devel@nongnu.org (open list:All patches CC here) > > Send a pull request (assuming you have a properly signed PGP key). Keith, I'll take the patches through my tree with your Acked-by, assuming that this makes the process easier for you. Sorry for forgetting about the previous version, I had intended to give others a chance to comment before I apply them, but then it fell through the cracks. Next time someone just send a quick "ping" reply after a week or so, please. Kevin ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-08-05 8:54 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-08-04 19:42 [Qemu-devel] fix incorrect identify implementation in nvme Christoph Hellwig 2016-08-04 19:42 ` [Qemu-devel] [PATCH 1/2] nvme: fix identify to be NVMe 1.1 compliant Christoph Hellwig 2016-08-04 19:42 ` [Qemu-devel] [PATCH 2/2] nvme: bump PCI revision Christoph Hellwig 2016-08-04 20:14 ` [Qemu-devel] fix incorrect identify implementation in nvme Keith Busch 2016-08-05 6:48 ` Markus Armbruster 2016-08-05 8:54 ` Kevin Wolf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).