* quirk broken namespace identifiers
@ 2022-04-12 6:11 Christoph Hellwig
2022-04-12 6:11 ` [PATCH 1/3] nvme: add a quirk to disable " Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Christoph Hellwig @ 2022-04-12 6:11 UTC (permalink / raw)
To: Keith Busch, Sagi Grimberg; +Cc: Luis Chamberlain, Klaus Jensen, linux-nvme
Hi all,
this series adds a quirk to ignore namespace identifiers on controllers
where they are completely broken and then quirks two controllers for it.
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH 1/3] nvme: add a quirk to disable namespace identifiers 2022-04-12 6:11 quirk broken namespace identifiers Christoph Hellwig @ 2022-04-12 6:11 ` Christoph Hellwig 2022-04-12 6:56 ` Chaitanya Kulkarni ` (2 more replies) 2022-04-12 6:11 ` [PATCH 2/3] nvme-pci: disable namespace identifiers for the MAXIO MAP1202 Christoph Hellwig 2022-04-12 6:11 ` [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers Christoph Hellwig 2 siblings, 3 replies; 23+ messages in thread From: Christoph Hellwig @ 2022-04-12 6:11 UTC (permalink / raw) To: Keith Busch, Sagi Grimberg; +Cc: Luis Chamberlain, Klaus Jensen, linux-nvme Add a quirk to disable using and exporting namespace identifiers for controllers where they are broken beyond repair. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/nvme/host/core.c | 24 ++++++++++++++++++------ drivers/nvme/host/nvme.h | 5 +++++ 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index efb85c6d8e2d5..41314dccb2209 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1282,6 +1282,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids, switch (cur->nidt) { case NVME_NIDT_EUI64: + if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) + return NVME_NIDT_EUI64_LEN; if (cur->nidl != NVME_NIDT_EUI64_LEN) { dev_warn(ctrl->device, "%s %d for NVME_NIDT_EUI64\n", warn_str, cur->nidl); @@ -1290,6 +1292,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids, memcpy(ids->eui64, data + sizeof(*cur), NVME_NIDT_EUI64_LEN); return NVME_NIDT_EUI64_LEN; case NVME_NIDT_NGUID: + if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) + return NVME_NIDT_NGUID_LEN; if (cur->nidl != NVME_NIDT_NGUID_LEN) { dev_warn(ctrl->device, "%s %d for NVME_NIDT_NGUID\n", warn_str, cur->nidl); @@ -1298,6 +1302,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids, memcpy(ids->nguid, data + sizeof(*cur), NVME_NIDT_NGUID_LEN); return NVME_NIDT_NGUID_LEN; case NVME_NIDT_UUID: + if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) + return NVME_NIDT_UUID_LEN; if (cur->nidl != NVME_NIDT_UUID_LEN) { dev_warn(ctrl->device, "%s %d for NVME_NIDT_UUID\n", warn_str, cur->nidl); @@ -1399,12 +1405,18 @@ static int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid, if ((*id)->ncap == 0) /* namespace not allocated or attached */ goto out_free_id; - if (ctrl->vs >= NVME_VS(1, 1, 0) && - !memchr_inv(ids->eui64, 0, sizeof(ids->eui64))) - memcpy(ids->eui64, (*id)->eui64, sizeof(ids->eui64)); - if (ctrl->vs >= NVME_VS(1, 2, 0) && - !memchr_inv(ids->nguid, 0, sizeof(ids->nguid))) - memcpy(ids->nguid, (*id)->nguid, sizeof(ids->nguid)); + + if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) { + dev_info(ctrl->device, + "Ignoring bogus Namespace Identifiers\n"); + } else { + if (ctrl->vs >= NVME_VS(1, 1, 0) && + !memchr_inv(ids->eui64, 0, sizeof(ids->eui64))) + memcpy(ids->eui64, (*id)->eui64, sizeof(ids->eui64)); + if (ctrl->vs >= NVME_VS(1, 2, 0) && + !memchr_inv(ids->nguid, 0, sizeof(ids->nguid))) + memcpy(ids->nguid, (*id)->nguid, sizeof(ids->nguid)); + } return 0; diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 1393bbf82d71e..a2b53ca633359 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -144,6 +144,11 @@ enum nvme_quirks { * encoding the generation sequence number. */ NVME_QUIRK_SKIP_CID_GEN = (1 << 17), + + /* + * Reports garbage in the namespace identifiers (eui64, nguid, uuid). + */ + NVME_QUIRK_BOGUS_NID = (1 << 18), }; /* -- 2.30.2 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/3] nvme: add a quirk to disable namespace identifiers 2022-04-12 6:11 ` [PATCH 1/3] nvme: add a quirk to disable " Christoph Hellwig @ 2022-04-12 6:56 ` Chaitanya Kulkarni 2022-04-12 7:01 ` Christoph Hellwig 2022-04-12 10:25 ` Sagi Grimberg 2022-04-12 14:16 ` Keith Busch 2 siblings, 1 reply; 23+ messages in thread From: Chaitanya Kulkarni @ 2022-04-12 6:56 UTC (permalink / raw) To: Christoph Hellwig Cc: Luis Chamberlain, Sagi Grimberg, Keith Busch, Klaus Jensen, linux-nvme On 4/11/22 23:11, Christoph Hellwig wrote: > Add a quirk to disable using and exporting namespace identifiers for > controllers where they are broken beyond repair. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- if possible take that quirk check out see [1] it is but more code but look much clean, either way looks good. Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com> -ck [1] totally untested :- +static int nvme_id_check_quirk(u64 nidt) +{ + switch (cur->nidt) { + case NVME_NIDT_EUI64: + if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) + return NVME_NIDT_EUI64_LEN; + return 0; + case NVME_NIDT_NGUID: + if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) + return NVME_NIDT_NGUID_LEN; + return 0; + case NVME_NIDT_UUID: + if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) + return NVME_NIDT_UUID_LEN; + return 0; + default: + return 0; + } +} + static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids, struct nvme_ns_id_desc *cur, bool *csi_seen) { const char *warn_str = "ctrl returned bogus length:"; void *data = cur; + int ret; + + ret = nvme_id_check_quirk(cur->nidt); + if (ret) + return ret; switch (cur->nidt) { case NVME_NIDT_EUI64: ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/3] nvme: add a quirk to disable namespace identifiers 2022-04-12 6:56 ` Chaitanya Kulkarni @ 2022-04-12 7:01 ` Christoph Hellwig 0 siblings, 0 replies; 23+ messages in thread From: Christoph Hellwig @ 2022-04-12 7:01 UTC (permalink / raw) To: Chaitanya Kulkarni Cc: Christoph Hellwig, Luis Chamberlain, Sagi Grimberg, Keith Busch, Klaus Jensen, linux-nvme On Tue, Apr 12, 2022 at 06:56:07AM +0000, Chaitanya Kulkarni wrote: > + ret = nvme_id_check_quirk(cur->nidt); > + if (ret) > + return ret; We still need to process the list to find the CSI. This is actually relevant for modern qemu, which supports multiple command sets. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/3] nvme: add a quirk to disable namespace identifiers 2022-04-12 6:11 ` [PATCH 1/3] nvme: add a quirk to disable " Christoph Hellwig 2022-04-12 6:56 ` Chaitanya Kulkarni @ 2022-04-12 10:25 ` Sagi Grimberg 2022-04-12 14:16 ` Keith Busch 2 siblings, 0 replies; 23+ messages in thread From: Sagi Grimberg @ 2022-04-12 10:25 UTC (permalink / raw) To: Christoph Hellwig, Keith Busch; +Cc: Luis Chamberlain, Klaus Jensen, linux-nvme Reviewed-by: Sagi Grimberg <sagi@grimberg.me> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/3] nvme: add a quirk to disable namespace identifiers 2022-04-12 6:11 ` [PATCH 1/3] nvme: add a quirk to disable " Christoph Hellwig 2022-04-12 6:56 ` Chaitanya Kulkarni 2022-04-12 10:25 ` Sagi Grimberg @ 2022-04-12 14:16 ` Keith Busch 2 siblings, 0 replies; 23+ messages in thread From: Keith Busch @ 2022-04-12 14:16 UTC (permalink / raw) To: Christoph Hellwig Cc: Sagi Grimberg, Luis Chamberlain, Klaus Jensen, linux-nvme On Tue, Apr 12, 2022 at 08:11:24AM +0200, Christoph Hellwig wrote: > @@ -1282,6 +1282,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids, > > switch (cur->nidt) { > case NVME_NIDT_EUI64: > + if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) > + return NVME_NIDT_EUI64_LEN; > if (cur->nidl != NVME_NIDT_EUI64_LEN) { > dev_warn(ctrl->device, "%s %d for NVME_NIDT_EUI64\n", > warn_str, cur->nidl); Shouldn't we still verify that cur->nidl is accurate rather than assume the length? If it is the wrong length, then we have a corrupt descriptor list and should abort the process. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] nvme-pci: disable namespace identifiers for the MAXIO MAP1202 2022-04-12 6:11 quirk broken namespace identifiers Christoph Hellwig 2022-04-12 6:11 ` [PATCH 1/3] nvme: add a quirk to disable " Christoph Hellwig @ 2022-04-12 6:11 ` Christoph Hellwig 2022-04-12 6:57 ` Chaitanya Kulkarni 2022-04-12 10:25 ` Sagi Grimberg 2022-04-12 6:11 ` [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers Christoph Hellwig 2 siblings, 2 replies; 23+ messages in thread From: Christoph Hellwig @ 2022-04-12 6:11 UTC (permalink / raw) To: Keith Busch, Sagi Grimberg Cc: Luis Chamberlain, Klaus Jensen, linux-nvme, 金韬 The MAXIO MAP1202 controllers reports completely bogus Namespace identifiers that even change after suspend cycles. Disable using the Identifiers entirely. Reported-by: 金韬 <me@kingtous.cn> Signed-off-by: Christoph Hellwig <hch@lst.de> Tested-by: 金韬 <me@kingtous.cn> --- drivers/nvme/host/pci.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index d817ca17463ed..c386c91483505 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -3447,6 +3447,8 @@ static const struct pci_device_id nvme_id_table[] = { .driver_data = NVME_QUIRK_NO_DEEPEST_PS, }, { PCI_DEVICE(0x2646, 0x2263), /* KINGSTON A2000 NVMe SSD */ .driver_data = NVME_QUIRK_NO_DEEPEST_PS, }, + { PCI_DEVICE(0x1e4B, 0x1202), /* MAXIO MAP1202 */ + .driver_data = NVME_QUIRK_BOGUS_NID, }, { PCI_DEVICE(PCI_VENDOR_ID_AMAZON, 0x0061), .driver_data = NVME_QUIRK_DMA_ADDRESS_BITS_48, }, { PCI_DEVICE(PCI_VENDOR_ID_AMAZON, 0x0065), -- 2.30.2 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] nvme-pci: disable namespace identifiers for the MAXIO MAP1202 2022-04-12 6:11 ` [PATCH 2/3] nvme-pci: disable namespace identifiers for the MAXIO MAP1202 Christoph Hellwig @ 2022-04-12 6:57 ` Chaitanya Kulkarni 2022-04-12 10:25 ` Sagi Grimberg 1 sibling, 0 replies; 23+ messages in thread From: Chaitanya Kulkarni @ 2022-04-12 6:57 UTC (permalink / raw) To: Christoph Hellwig, Keith Busch, Sagi Grimberg Cc: Luis Chamberlain, Klaus Jensen, linux-nvme, 金韬 On 4/11/22 23:11, Christoph Hellwig wrote: > The MAXIO MAP1202 controllers reports completely bogus Namespace > identifiers that even change after suspend cycles. Disable using > the Identifiers entirely. > > Reported-by: 金韬 <me@kingtous.cn> > Signed-off-by: Christoph Hellwig <hch@lst.de> > Tested-by: 金韬 <me@kingtous.cn> Looks good. Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com> -ck ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] nvme-pci: disable namespace identifiers for the MAXIO MAP1202 2022-04-12 6:11 ` [PATCH 2/3] nvme-pci: disable namespace identifiers for the MAXIO MAP1202 Christoph Hellwig 2022-04-12 6:57 ` Chaitanya Kulkarni @ 2022-04-12 10:25 ` Sagi Grimberg 1 sibling, 0 replies; 23+ messages in thread From: Sagi Grimberg @ 2022-04-12 10:25 UTC (permalink / raw) To: Christoph Hellwig, Keith Busch Cc: Luis Chamberlain, Klaus Jensen, linux-nvme, 金韬 Reviewed-by: Sagi Grimberg <sagi@grimberg.me> ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers 2022-04-12 6:11 quirk broken namespace identifiers Christoph Hellwig 2022-04-12 6:11 ` [PATCH 1/3] nvme: add a quirk to disable " Christoph Hellwig 2022-04-12 6:11 ` [PATCH 2/3] nvme-pci: disable namespace identifiers for the MAXIO MAP1202 Christoph Hellwig @ 2022-04-12 6:11 ` Christoph Hellwig 2022-04-12 6:33 ` Klaus Jensen 2022-04-12 10:25 ` Sagi Grimberg 2 siblings, 2 replies; 23+ messages in thread From: Christoph Hellwig @ 2022-04-12 6:11 UTC (permalink / raw) To: Keith Busch, Sagi Grimberg; +Cc: Luis Chamberlain, Klaus Jensen, linux-nvme Qemu unconditionally reports a UUID, which depending on the qemu version is either all-null (which is incorrect but harmless) or contains a single bit set for all controllers. In addition it can also optionally report a eui64 which needs to be manually set. Disable namespace identifiers for Qemu controlles entirely even if in some cases they could be set corretly through manual intervention. Reported-by: Luis Chamberlain <mcgrof@kernel.org> Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/nvme/host/pci.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index c386c91483505..b191e7dcf15ca 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -3410,6 +3410,8 @@ static const struct pci_device_id nvme_id_table[] = { { PCI_VDEVICE(INTEL, 0x5845), /* Qemu emulated controller */ .driver_data = NVME_QUIRK_IDENTIFY_CNS | NVME_QUIRK_DISABLE_WRITE_ZEROES, }, + { PCI_VDEVICE(REDHAT, 0x0010), /* Qemu emulated controller */ + .driver_data = NVME_QUIRK_BOGUS_NID, }, { PCI_DEVICE(0x126f, 0x2263), /* Silicon Motion unidentified */ .driver_data = NVME_QUIRK_NO_NS_DESC_LIST, }, { PCI_DEVICE(0x1bb1, 0x0100), /* Seagate Nytro Flash Storage */ -- 2.30.2 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers 2022-04-12 6:11 ` [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers Christoph Hellwig @ 2022-04-12 6:33 ` Klaus Jensen 2022-04-12 11:45 ` Christoph Hellwig 2022-04-12 10:25 ` Sagi Grimberg 1 sibling, 1 reply; 23+ messages in thread From: Klaus Jensen @ 2022-04-12 6:33 UTC (permalink / raw) To: Christoph Hellwig Cc: Keith Busch, Sagi Grimberg, Luis Chamberlain, linux-nvme [-- Attachment #1: Type: text/plain, Size: 1542 bytes --] On Apr 12 08:11, Christoph Hellwig wrote: > Qemu unconditionally reports a UUID, which depending on the qemu version > is either all-null (which is incorrect but harmless) or contains a single > bit set for all controllers. In addition it can also optionally report > a eui64 which needs to be manually set. Disable namespace identifiers > for Qemu controlles entirely even if in some cases they could be set > corretly through manual intervention. > > Reported-by: Luis Chamberlain <mcgrof@kernel.org> > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/nvme/host/pci.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index c386c91483505..b191e7dcf15ca 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -3410,6 +3410,8 @@ static const struct pci_device_id nvme_id_table[] = { > { PCI_VDEVICE(INTEL, 0x5845), /* Qemu emulated controller */ > .driver_data = NVME_QUIRK_IDENTIFY_CNS | > NVME_QUIRK_DISABLE_WRITE_ZEROES, }, > + { PCI_VDEVICE(REDHAT, 0x0010), /* Qemu emulated controller */ > + .driver_data = NVME_QUIRK_BOGUS_NID, }, > { PCI_DEVICE(0x126f, 0x2263), /* Silicon Motion unidentified */ > .driver_data = NVME_QUIRK_NO_NS_DESC_LIST, }, > { PCI_DEVICE(0x1bb1, 0x0100), /* Seagate Nytro Flash Storage */ > -- > 2.30.2 > When I fix this in QEMU properly, can we move this quirk to the core_quirks and match on firmware revision? That way I don't have to request a new DID. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers 2022-04-12 6:33 ` Klaus Jensen @ 2022-04-12 11:45 ` Christoph Hellwig 2022-04-12 20:43 ` Klaus Jensen 0 siblings, 1 reply; 23+ messages in thread From: Christoph Hellwig @ 2022-04-12 11:45 UTC (permalink / raw) To: Klaus Jensen Cc: Christoph Hellwig, Keith Busch, Sagi Grimberg, Luis Chamberlain, linux-nvme On Tue, Apr 12, 2022 at 08:33:38AM +0200, Klaus Jensen wrote: > > + { PCI_VDEVICE(REDHAT, 0x0010), /* Qemu emulated controller */ > > + .driver_data = NVME_QUIRK_BOGUS_NID, }, > > { PCI_DEVICE(0x126f, 0x2263), /* Silicon Motion unidentified */ > > .driver_data = NVME_QUIRK_NO_NS_DESC_LIST, }, > > { PCI_DEVICE(0x1bb1, 0x0100), /* Seagate Nytro Flash Storage */ > > -- > > 2.30.2 > > > > When I fix this in QEMU properly, can we move this quirk to the > core_quirks and match on firmware revision? That way I don't have to > request a new DID. Do we known that only one firmware revision reported by Qemu is actually broken? For now I'd like to get the regression fixed ASAP, and I don't think Qemu ever fully got identifiers right so far. We can always change it later if needed. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers 2022-04-12 11:45 ` Christoph Hellwig @ 2022-04-12 20:43 ` Klaus Jensen 0 siblings, 0 replies; 23+ messages in thread From: Klaus Jensen @ 2022-04-12 20:43 UTC (permalink / raw) To: Christoph Hellwig Cc: Keith Busch, Sagi Grimberg, Luis Chamberlain, linux-nvme [-- Attachment #1: Type: text/plain, Size: 1095 bytes --] On Apr 12 13:45, Christoph Hellwig wrote: > On Tue, Apr 12, 2022 at 08:33:38AM +0200, Klaus Jensen wrote: > > > + { PCI_VDEVICE(REDHAT, 0x0010), /* Qemu emulated controller */ > > > + .driver_data = NVME_QUIRK_BOGUS_NID, }, > > > { PCI_DEVICE(0x126f, 0x2263), /* Silicon Motion unidentified */ > > > .driver_data = NVME_QUIRK_NO_NS_DESC_LIST, }, > > > { PCI_DEVICE(0x1bb1, 0x0100), /* Seagate Nytro Flash Storage */ > > > -- > > > 2.30.2 > > > > > > > When I fix this in QEMU properly, can we move this quirk to the > > core_quirks and match on firmware revision? That way I don't have to > > request a new DID. > > Do we known that only one firmware revision reported by Qemu is actually > broken? For now I'd like to get the regression fixed ASAP, and I don't > think Qemu ever fully got identifiers right so far. We can always > change it later if needed. > To my knowledge, QEMU has only ever used 1.0 in FR, but I will check. In any case, I did not want you to change this *now*, so I am totally fine with this getting changed later if required. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers 2022-04-12 6:11 ` [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers Christoph Hellwig 2022-04-12 6:33 ` Klaus Jensen @ 2022-04-12 10:25 ` Sagi Grimberg 1 sibling, 0 replies; 23+ messages in thread From: Sagi Grimberg @ 2022-04-12 10:25 UTC (permalink / raw) To: Christoph Hellwig, Keith Busch; +Cc: Luis Chamberlain, Klaus Jensen, linux-nvme Reviewed-by: Sagi Grimberg <sagi@grimberg.me> ^ permalink raw reply [flat|nested] 23+ messages in thread
* quirk broken namespace identifiers v2 @ 2022-04-13 4:49 Christoph Hellwig 2022-04-13 4:49 ` [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers Christoph Hellwig 0 siblings, 1 reply; 23+ messages in thread From: Christoph Hellwig @ 2022-04-13 4:49 UTC (permalink / raw) To: Keith Busch, Sagi Grimberg; +Cc: Luis Chamberlain, Klaus Jensen, linux-nvme Hi all, this series adds a quirk to ignore namespace identifiers on controllers where they are completely broken and then quirks two controllers for it. Changes since v1: - validate namespace descriptor sizes even if ignoring them - also handle another MAXIO controller with the same problem - fix up a few commit messages ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers 2022-04-13 4:49 quirk broken namespace identifiers v2 Christoph Hellwig @ 2022-04-13 4:49 ` Christoph Hellwig 2022-04-13 7:34 ` Niklas Cassel 0 siblings, 1 reply; 23+ messages in thread From: Christoph Hellwig @ 2022-04-13 4:49 UTC (permalink / raw) To: Keith Busch, Sagi Grimberg; +Cc: Luis Chamberlain, Klaus Jensen, linux-nvme Qemu unconditionally reports a UUID, which depending on the qemu version is either all-null (which is incorrect but harmless) or contains a single bit set for all controllers. In addition it can also optionally report a eui64 which needs to be manually set. Disable namespace identifiers for Qemu controlles entirely even if in some cases they could be set correctly through manual intervention. Reported-by: Luis Chamberlain <mcgrof@kernel.org> Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Sagi Grimberg <sagi@grimberg.me> --- drivers/nvme/host/pci.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index c45dbe8a7dcd7..9b75abdd4478a 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -3410,6 +3410,8 @@ static const struct pci_device_id nvme_id_table[] = { { PCI_VDEVICE(INTEL, 0x5845), /* Qemu emulated controller */ .driver_data = NVME_QUIRK_IDENTIFY_CNS | NVME_QUIRK_DISABLE_WRITE_ZEROES, }, + { PCI_VDEVICE(REDHAT, 0x0010), /* Qemu emulated controller */ + .driver_data = NVME_QUIRK_BOGUS_NID, }, { PCI_DEVICE(0x126f, 0x2263), /* Silicon Motion unidentified */ .driver_data = NVME_QUIRK_NO_NS_DESC_LIST, }, { PCI_DEVICE(0x1bb1, 0x0100), /* Seagate Nytro Flash Storage */ -- 2.30.2 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers 2022-04-13 4:49 ` [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers Christoph Hellwig @ 2022-04-13 7:34 ` Niklas Cassel 2022-04-13 8:40 ` Klaus Jensen 0 siblings, 1 reply; 23+ messages in thread From: Niklas Cassel @ 2022-04-13 7:34 UTC (permalink / raw) To: Christoph Hellwig Cc: Keith Busch, Sagi Grimberg, Luis Chamberlain, Klaus Jensen, linux-nvme On Wed, Apr 13, 2022 at 06:49:05AM +0200, Christoph Hellwig wrote: > Qemu unconditionally reports a UUID, which depending on the qemu version Since it appears that both older and current QEMU versions are not implementing this properly, perhaps you should also consider adding the pci vendor and device id used by older QEMU versions? QEMU nvme pci vendor and device id was changed in commit: https://github.com/qemu/qemu/commit/6eb7a071292a2f11065127ac152fa24248806021 Which was first included in QEMU v5.2.0. Kind regards, Niklas > is either all-null (which is incorrect but harmless) or contains a single > bit set for all controllers. In addition it can also optionally report > a eui64 which needs to be manually set. Disable namespace identifiers > for Qemu controlles entirely even if in some cases they could be set > correctly through manual intervention. > > Reported-by: Luis Chamberlain <mcgrof@kernel.org> > Signed-off-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Sagi Grimberg <sagi@grimberg.me> > --- > drivers/nvme/host/pci.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index c45dbe8a7dcd7..9b75abdd4478a 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -3410,6 +3410,8 @@ static const struct pci_device_id nvme_id_table[] = { > { PCI_VDEVICE(INTEL, 0x5845), /* Qemu emulated controller */ > .driver_data = NVME_QUIRK_IDENTIFY_CNS | > NVME_QUIRK_DISABLE_WRITE_ZEROES, }, > + { PCI_VDEVICE(REDHAT, 0x0010), /* Qemu emulated controller */ > + .driver_data = NVME_QUIRK_BOGUS_NID, }, > { PCI_DEVICE(0x126f, 0x2263), /* Silicon Motion unidentified */ > .driver_data = NVME_QUIRK_NO_NS_DESC_LIST, }, > { PCI_DEVICE(0x1bb1, 0x0100), /* Seagate Nytro Flash Storage */ > -- > 2.30.2 > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers 2022-04-13 7:34 ` Niklas Cassel @ 2022-04-13 8:40 ` Klaus Jensen 2022-04-13 15:34 ` Christoph Hellwig 0 siblings, 1 reply; 23+ messages in thread From: Klaus Jensen @ 2022-04-13 8:40 UTC (permalink / raw) To: Niklas Cassel Cc: Christoph Hellwig, Keith Busch, Sagi Grimberg, Luis Chamberlain, linux-nvme [-- Attachment #1: Type: text/plain, Size: 676 bytes --] On Apr 13 07:34, Niklas Cassel wrote: > On Wed, Apr 13, 2022 at 06:49:05AM +0200, Christoph Hellwig wrote: > > Qemu unconditionally reports a UUID, which depending on the qemu version > > Since it appears that both older and current QEMU versions are not > implementing this properly, perhaps you should also consider adding > the pci vendor and device id used by older QEMU versions? > > QEMU nvme pci vendor and device id was changed in commit: > https://github.com/qemu/qemu/commit/6eb7a071292a2f11065127ac152fa24248806021 > > Which was first included in QEMU v5.2.0. > That is a good point Niklas. +1 for that. The driver already knows about that ID. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers 2022-04-13 8:40 ` Klaus Jensen @ 2022-04-13 15:34 ` Christoph Hellwig 2022-04-13 15:49 ` Klaus Jensen 0 siblings, 1 reply; 23+ messages in thread From: Christoph Hellwig @ 2022-04-13 15:34 UTC (permalink / raw) To: Klaus Jensen Cc: Niklas Cassel, Christoph Hellwig, Keith Busch, Sagi Grimberg, Luis Chamberlain, linux-nvme On Wed, Apr 13, 2022 at 10:40:47AM +0200, Klaus Jensen wrote: > On Apr 13 07:34, Niklas Cassel wrote: > > On Wed, Apr 13, 2022 at 06:49:05AM +0200, Christoph Hellwig wrote: > > > Qemu unconditionally reports a UUID, which depending on the qemu version > > > > Since it appears that both older and current QEMU versions are not > > implementing this properly, perhaps you should also consider adding > > the pci vendor and device id used by older QEMU versions? > > > > QEMU nvme pci vendor and device id was changed in commit: > > https://github.com/qemu/qemu/commit/6eb7a071292a2f11065127ac152fa24248806021 > > > > Which was first included in QEMU v5.2.0. > > > > That is a good point Niklas. +1 for that. The driver already knows about > that ID. 5.2.0 is also the first qemu that supports the namespace descriptor list, that's why I didn't include it. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers 2022-04-13 15:34 ` Christoph Hellwig @ 2022-04-13 15:49 ` Klaus Jensen 2022-04-13 15:50 ` Christoph Hellwig 0 siblings, 1 reply; 23+ messages in thread From: Klaus Jensen @ 2022-04-13 15:49 UTC (permalink / raw) To: Christoph Hellwig Cc: Niklas Cassel, Keith Busch, Sagi Grimberg, Luis Chamberlain, linux-nvme [-- Attachment #1: Type: text/plain, Size: 1497 bytes --] On Apr 13 17:34, Christoph Hellwig wrote: > On Wed, Apr 13, 2022 at 10:40:47AM +0200, Klaus Jensen wrote: > > On Apr 13 07:34, Niklas Cassel wrote: > > > On Wed, Apr 13, 2022 at 06:49:05AM +0200, Christoph Hellwig wrote: > > > > Qemu unconditionally reports a UUID, which depending on the qemu version > > > > > > Since it appears that both older and current QEMU versions are not > > > implementing this properly, perhaps you should also consider adding > > > the pci vendor and device id used by older QEMU versions? > > > > > > QEMU nvme pci vendor and device id was changed in commit: > > > https://github.com/qemu/qemu/commit/6eb7a071292a2f11065127ac152fa24248806021 > > > > > > Which was first included in QEMU v5.2.0. > > > > > > > That is a good point Niklas. +1 for that. The driver already knows about > > that ID. > > 5.2.0 is also the first qemu that supports the namespace descriptor > list, that's why I didn't include it. However, backwards compatibility in QEMU mandates that a compatibility parameter is added when a device changes identity like this (i.e. PCI vendor id). This means that the 'use-intel-id' compatibility parameter may cause a 5.2 nvme device to present itself with the Intel PCI vendor/device id, either because the user explicitly set it, or because the machine is launched using a pre-5.2 machine type (i.e. `-machine pc-q35-5.1`), which you would normally do if you want launch an existing VM on a new version of QEMU. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers 2022-04-13 15:49 ` Klaus Jensen @ 2022-04-13 15:50 ` Christoph Hellwig 2022-04-13 20:08 ` Klaus Jensen 0 siblings, 1 reply; 23+ messages in thread From: Christoph Hellwig @ 2022-04-13 15:50 UTC (permalink / raw) To: Klaus Jensen Cc: Christoph Hellwig, Niklas Cassel, Keith Busch, Sagi Grimberg, Luis Chamberlain, linux-nvme On Wed, Apr 13, 2022 at 05:49:29PM +0200, Klaus Jensen wrote: > This means that the 'use-intel-id' compatibility parameter may cause a > 5.2 nvme device to present itself with the Intel PCI vendor/device id, > either because the user explicitly set it, or because the machine is > launched using a pre-5.2 machine type (i.e. `-machine pc-q35-5.1`), > which you would normally do if you want launch an existing VM on a new > version of QEMU. But if it presents an old machine it also shouldn't support a new identify call, right? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers 2022-04-13 15:50 ` Christoph Hellwig @ 2022-04-13 20:08 ` Klaus Jensen 2022-04-13 20:38 ` Keith Busch 2022-04-14 4:19 ` Christoph Hellwig 0 siblings, 2 replies; 23+ messages in thread From: Klaus Jensen @ 2022-04-13 20:08 UTC (permalink / raw) To: Christoph Hellwig Cc: Niklas Cassel, Keith Busch, Sagi Grimberg, Luis Chamberlain, linux-nvme [-- Attachment #1: Type: text/plain, Size: 1135 bytes --] On Apr 13 17:50, Christoph Hellwig wrote: > On Wed, Apr 13, 2022 at 05:49:29PM +0200, Klaus Jensen wrote: > > This means that the 'use-intel-id' compatibility parameter may cause a > > 5.2 nvme device to present itself with the Intel PCI vendor/device id, > > either because the user explicitly set it, or because the machine is > > launched using a pre-5.2 machine type (i.e. `-machine pc-q35-5.1`), > > which you would normally do if you want launch an existing VM on a new > > version of QEMU. > > But if it presents an old machine it also shouldn't support a new > identify call, right? > You are absolutely right in the case of live-migration, but hw/nvme is marked unmigratable, so we do not worry about that. We probably screwed up with the compatibility parameter here, since it really only guarantees the id and some headaches for you as driver maintainer. I'm sorry about that - I'll keep this in mind for the future. Regardless, the fact remains that it is possible to have a device with a buggy uuid namespace descriptor using the Intel identifier, so we should add the quirk for that as well. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers 2022-04-13 20:08 ` Klaus Jensen @ 2022-04-13 20:38 ` Keith Busch 2022-04-14 4:19 ` Christoph Hellwig 1 sibling, 0 replies; 23+ messages in thread From: Keith Busch @ 2022-04-13 20:38 UTC (permalink / raw) To: Klaus Jensen Cc: Christoph Hellwig, Niklas Cassel, Sagi Grimberg, Luis Chamberlain, linux-nvme On Wed, Apr 13, 2022 at 10:08:53PM +0200, Klaus Jensen wrote: > On Apr 13 17:50, Christoph Hellwig wrote: > > On Wed, Apr 13, 2022 at 05:49:29PM +0200, Klaus Jensen wrote: > > > This means that the 'use-intel-id' compatibility parameter may cause a > > > 5.2 nvme device to present itself with the Intel PCI vendor/device id, > > > either because the user explicitly set it, or because the machine is > > > launched using a pre-5.2 machine type (i.e. `-machine pc-q35-5.1`), > > > which you would normally do if you want launch an existing VM on a new > > > version of QEMU. > > > > But if it presents an old machine it also shouldn't support a new > > identify call, right? > > > > You are absolutely right in the case of live-migration, but hw/nvme is > marked unmigratable, so we do not worry about that. > > We probably screwed up with the compatibility parameter here, since it > really only guarantees the id and some headaches for you as driver > maintainer. I'm sorry about that - I'll keep this in mind for the > future. > > Regardless, the fact remains that it is possible to have a device with a > buggy uuid namespace descriptor using the Intel identifier, so we should > add the quirk for that as well. Reporting that identifier really should be deprecated and removed if at all possible. There was a miscommunication regarding if that PCI device ID was actually reserved for this purpose (it wasn't). Intel at some point will use that DID for some device that isn't NVMe, then we're really going to be in difficult spot. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers 2022-04-13 20:08 ` Klaus Jensen 2022-04-13 20:38 ` Keith Busch @ 2022-04-14 4:19 ` Christoph Hellwig 1 sibling, 0 replies; 23+ messages in thread From: Christoph Hellwig @ 2022-04-14 4:19 UTC (permalink / raw) To: Klaus Jensen Cc: Christoph Hellwig, Niklas Cassel, Keith Busch, Sagi Grimberg, Luis Chamberlain, linux-nvme On Wed, Apr 13, 2022 at 10:08:53PM +0200, Klaus Jensen wrote: > You are absolutely right in the case of live-migration, but hw/nvme is > marked unmigratable, so we do not worry about that. > > We probably screwed up with the compatibility parameter here, since it > really only guarantees the id and some headaches for you as driver > maintainer. I'm sorry about that - I'll keep this in mind for the > future. > > Regardless, the fact remains that it is possible to have a device with a > buggy uuid namespace descriptor using the Intel identifier, so we should > add the quirk for that as well. I've added the quirk to the old ID for now. ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2022-04-14 4:19 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-12 6:11 quirk broken namespace identifiers Christoph Hellwig 2022-04-12 6:11 ` [PATCH 1/3] nvme: add a quirk to disable " Christoph Hellwig 2022-04-12 6:56 ` Chaitanya Kulkarni 2022-04-12 7:01 ` Christoph Hellwig 2022-04-12 10:25 ` Sagi Grimberg 2022-04-12 14:16 ` Keith Busch 2022-04-12 6:11 ` [PATCH 2/3] nvme-pci: disable namespace identifiers for the MAXIO MAP1202 Christoph Hellwig 2022-04-12 6:57 ` Chaitanya Kulkarni 2022-04-12 10:25 ` Sagi Grimberg 2022-04-12 6:11 ` [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers Christoph Hellwig 2022-04-12 6:33 ` Klaus Jensen 2022-04-12 11:45 ` Christoph Hellwig 2022-04-12 20:43 ` Klaus Jensen 2022-04-12 10:25 ` Sagi Grimberg 2022-04-13 4:49 quirk broken namespace identifiers v2 Christoph Hellwig 2022-04-13 4:49 ` [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers Christoph Hellwig 2022-04-13 7:34 ` Niklas Cassel 2022-04-13 8:40 ` Klaus Jensen 2022-04-13 15:34 ` Christoph Hellwig 2022-04-13 15:49 ` Klaus Jensen 2022-04-13 15:50 ` Christoph Hellwig 2022-04-13 20:08 ` Klaus Jensen 2022-04-13 20:38 ` Keith Busch 2022-04-14 4:19 ` Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.