| Message ID | 20191219130921.309264-23-k.jensen@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nvme: support NVMe v1.3d, SGLs and multiple namespaces | expand |
On Thu, Dec 19, 2019 at 02:09:19PM +0100, Klaus Jensen wrote: > @@ -2480,7 +2480,7 @@ static void nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev) > pci_conf[PCI_INTERRUPT_PIN] = 1; > pci_config_set_prog_interface(pci_conf, 0x2); > pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL); > - pci_config_set_device_id(pci_conf, 0x5845); > + pci_config_set_device_id(pci_conf, 0x5846); > pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_EXPRESS); > pcie_endpoint_cap_init(pci_dev, 0x80); We can't just pick a number here, these are supposed to be assigned by the vendor. A day will come when I will be in trouble for using the existing identifier: I found out to late it was supposed to be for internal use only as it was never officially reserved, so lets not make the same mistake for some future device.
On Dec 20 01:16, Keith Busch wrote: > On Thu, Dec 19, 2019 at 02:09:19PM +0100, Klaus Jensen wrote: > > @@ -2480,7 +2480,7 @@ static void nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev) > > pci_conf[PCI_INTERRUPT_PIN] = 1; > > pci_config_set_prog_interface(pci_conf, 0x2); > > pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL); > > - pci_config_set_device_id(pci_conf, 0x5845); > > + pci_config_set_device_id(pci_conf, 0x5846); > > pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_EXPRESS); > > pcie_endpoint_cap_init(pci_dev, 0x80); > > We can't just pick a number here, these are supposed to be assigned by the > vendor. A day will come when I will be in trouble for using the existing > identifier: I found out to late it was supposed to be for internal use > only as it was never officially reserved, so lets not make the same > mistake for some future device. > Makes sense. And there is no "QEMU" vendor, is there? But it would be really nice to get rid of the quirks.
On Thu, Dec 19, 2019 at 06:24:57PM +0100, Klaus Birkelund Jensen wrote: > On Dec 20 01:16, Keith Busch wrote: > > On Thu, Dec 19, 2019 at 02:09:19PM +0100, Klaus Jensen wrote: > > > @@ -2480,7 +2480,7 @@ static void nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev) > > > pci_conf[PCI_INTERRUPT_PIN] = 1; > > > pci_config_set_prog_interface(pci_conf, 0x2); > > > pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL); > > > - pci_config_set_device_id(pci_conf, 0x5845); > > > + pci_config_set_device_id(pci_conf, 0x5846); > > > pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_EXPRESS); > > > pcie_endpoint_cap_init(pci_dev, 0x80); > > > > We can't just pick a number here, these are supposed to be assigned by the > > vendor. A day will come when I will be in trouble for using the existing > > identifier: I found out to late it was supposed to be for internal use > > only as it was never officially reserved, so lets not make the same > > mistake for some future device. > > > > Makes sense. And there is no "QEMU" vendor, is there? I'm not sure if we can use this, but there is a PCI_VENDOR_ID_QEMU, 0x1234, defined in include/hw/pci/pci.h. > But it would be really nice to get rid of the quirks. Since you're bumping the nvme version, Maybe we can change the quirk to include the nvme version number
On Dec 20 02:46, Keith Busch wrote: > On Thu, Dec 19, 2019 at 06:24:57PM +0100, Klaus Birkelund Jensen wrote: > > On Dec 20 01:16, Keith Busch wrote: > > > On Thu, Dec 19, 2019 at 02:09:19PM +0100, Klaus Jensen wrote: > > > > @@ -2480,7 +2480,7 @@ static void nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev) > > > > pci_conf[PCI_INTERRUPT_PIN] = 1; > > > > pci_config_set_prog_interface(pci_conf, 0x2); > > > > pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL); > > > > - pci_config_set_device_id(pci_conf, 0x5845); > > > > + pci_config_set_device_id(pci_conf, 0x5846); > > > > pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_EXPRESS); > > > > pcie_endpoint_cap_init(pci_dev, 0x80); > > > > > > We can't just pick a number here, these are supposed to be assigned by the > > > vendor. A day will come when I will be in trouble for using the existing > > > identifier: I found out to late it was supposed to be for internal use > > > only as it was never officially reserved, so lets not make the same > > > mistake for some future device. > > > > > > > Makes sense. And there is no "QEMU" vendor, is there? > > I'm not sure if we can use this, but there is a PCI_VENDOR_ID_QEMU, > 0x1234, defined in include/hw/pci/pci.h. > Maybe it's possible to use PCI_VENDOR_ID_REDHAT?
diff --git a/hw/block/nvme.c b/hw/block/nvme.c index e1bf9bf3798b..68c433415169 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -2480,7 +2480,7 @@ static void nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev) pci_conf[PCI_INTERRUPT_PIN] = 1; pci_config_set_prog_interface(pci_conf, 0x2); pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL); - pci_config_set_device_id(pci_conf, 0x5845); + pci_config_set_device_id(pci_conf, 0x5846); pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_EXPRESS); pcie_endpoint_cap_init(pci_dev, 0x80); @@ -2651,7 +2651,7 @@ static void nvme_class_init(ObjectClass *oc, void *data) pc->exit = nvme_exit; pc->class_id = PCI_CLASS_STORAGE_EXPRESS; pc->vendor_id = PCI_VENDOR_ID_INTEL; - pc->device_id = 0x5845; + pc->device_id = 0x5846; pc->revision = 2; set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
Since commits 9d6459d21a6e ("nvme: fix write zeroes offset and count") and c7fe50bcf1f1 ("nvme: support multiple namespaces") the controller device no longer has the quirks that the Linux kernel think it has. As the quirks are applied based on pci vendor and device id, bump the device id to get rid of them. Signed-off-by: Klaus Jensen <k.jensen@samsung.com> --- hw/block/nvme.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)