Re: [PATCH 2/3] nvme-pci: Add controller memory buffer supported macro
> On Tue, Jun 23, 2020 at 06:27:51PM +0200, Christoph Hellwig wrote: > > On Tue, Jun 23, 2020 at 09:24:33PM +0800, Baolin Wang wrote: > > > Introduce a new capability macro to indicate if the controller > > > supports the memory buffer or not, instead of reading the > > > NVME_REG_CMBSZ register. > > > > This is a complex issue. The CMBS bit was only added in NVMe 1.4 as > > a backwards incompatible change, as the CMB addressing scheme can lead > > Ah, right, I think I should add another version condition: > if ((ctrl->vs >= NVME_VS(1, 4, 0) && !NVME_CAP_CMBS(dev->ctrl.cap)) || > dev->cmb_size) >return; After more thinking, now we should read NVME_REG_VS register to get the controller version when using the CMBS bit in Capabilities register, there is no benefit, so I'll drop this patch. Thanks. > > to data corruption. The CMBS was added as part of the horribe hack > > that also involves the CBA field, which we'll need to see before > > using it to work around the addressing issue. At the same time we > > should also continue supporting the legacy pre-1.4 CMB with a warning > > (and may reject it if we know we run in a VM).
Re: [PATCH 2/3] nvme-pci: Add controller memory buffer supported macro
On Tue, Jun 23, 2020 at 07:58:17PM -0700, Keith Busch wrote: > On Tue, Jun 23, 2020 at 06:27:51PM +0200, Christoph Hellwig wrote: > > On Tue, Jun 23, 2020 at 09:24:33PM +0800, Baolin Wang wrote: > > > Introduce a new capability macro to indicate if the controller > > > supports the memory buffer or not, instead of reading the > > > NVME_REG_CMBSZ register. > > > > This is a complex issue. The CMBS bit was only added in NVMe 1.4 as > > a backwards incompatible change, as the CMB addressing scheme can lead > > to data corruption. The CMBS was added as part of the horribe hack > > that also involves the CBA field, which we'll need to see before > > using it to work around the addressing issue. At the same time we > > should also continue supporting the legacy pre-1.4 CMB with a warning > > (and may reject it if we know we run in a VM). > > Well, a CMB from an emulated controller (like qemu's) can be used within > a VM. It's only if you direct assign a PCI function that CMB usage > breaks. But we have no idea if a controller is assigned or emulated.
Re: [PATCH 2/3] nvme-pci: Add controller memory buffer supported macro
On Tue, Jun 23, 2020 at 06:27:51PM +0200, Christoph Hellwig wrote: > On Tue, Jun 23, 2020 at 09:24:33PM +0800, Baolin Wang wrote: > > Introduce a new capability macro to indicate if the controller > > supports the memory buffer or not, instead of reading the > > NVME_REG_CMBSZ register. > > This is a complex issue. The CMBS bit was only added in NVMe 1.4 as > a backwards incompatible change, as the CMB addressing scheme can lead > to data corruption. The CMBS was added as part of the horribe hack > that also involves the CBA field, which we'll need to see before > using it to work around the addressing issue. At the same time we > should also continue supporting the legacy pre-1.4 CMB with a warning > (and may reject it if we know we run in a VM). Well, a CMB from an emulated controller (like qemu's) can be used within a VM. It's only if you direct assign a PCI function that CMB usage breaks.
Re: [PATCH 2/3] nvme-pci: Add controller memory buffer supported macro
On Tue, Jun 23, 2020 at 06:27:51PM +0200, Christoph Hellwig wrote: > On Tue, Jun 23, 2020 at 09:24:33PM +0800, Baolin Wang wrote: > > Introduce a new capability macro to indicate if the controller > > supports the memory buffer or not, instead of reading the > > NVME_REG_CMBSZ register. > > This is a complex issue. The CMBS bit was only added in NVMe 1.4 as > a backwards incompatible change, as the CMB addressing scheme can lead Ah, right, I think I should add another version condition: if ((ctrl->vs >= NVME_VS(1, 4, 0) && !NVME_CAP_CMBS(dev->ctrl.cap)) || dev->cmb_size) return; > to data corruption. The CMBS was added as part of the horribe hack > that also involves the CBA field, which we'll need to see before > using it to work around the addressing issue. At the same time we > should also continue supporting the legacy pre-1.4 CMB with a warning > (and may reject it if we know we run in a VM). Thanks for your explanation. I saw we've added an CMB sysfs attribute to get the CMB location and size if we enabled CMB, so should we still add some information in nvme_map_cmb() let user know explicitly? dev_info(dev->ctrl.device, "Registered the CMB, bar=0x%x, size=%lld\n", bar, size);
Re: [PATCH 2/3] nvme-pci: Add controller memory buffer supported macro
On Tue, Jun 23, 2020 at 09:24:33PM +0800, Baolin Wang wrote: > Introduce a new capability macro to indicate if the controller > supports the memory buffer or not, instead of reading the > NVME_REG_CMBSZ register. This is a complex issue. The CMBS bit was only added in NVMe 1.4 as a backwards incompatible change, as the CMB addressing scheme can lead to data corruption. The CMBS was added as part of the horribe hack that also involves the CBA field, which we'll need to see before using it to work around the addressing issue. At the same time we should also continue supporting the legacy pre-1.4 CMB with a warning (and may reject it if we know we run in a VM).
[PATCH 2/3] nvme-pci: Add controller memory buffer supported macro
Introduce a new capability macro to indicate if the controller
supports the memory buffer or not, instead of reading the
NVME_REG_CMBSZ register.
Signed-off-by: Baolin Wang
---
drivers/nvme/host/pci.c | 2 +-
include/linux/nvme.h| 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 4b4b7b7..00c2701 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1760,7 +1760,7 @@ static void nvme_map_cmb(struct nvme_dev *dev)
struct pci_dev *pdev = to_pci_dev(dev->dev);
int bar;
- if (dev->cmb_size)
+ if (!NVME_CAP_CMBS(dev->ctrl.cap) || dev->cmb_size)
return;
dev->cmbsz = readl(dev->bar + NVME_REG_CMBSZ);
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 5ce51ab..e3726be 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -134,6 +134,7 @@ enum {
#define NVME_CAP_NSSRC(cap)(((cap) >> 36) & 0x1)
#define NVME_CAP_MPSMIN(cap) (((cap) >> 48) & 0xf)
#define NVME_CAP_MPSMAX(cap) (((cap) >> 52) & 0xf)
+#define NVME_CAP_CMBS(cap) (((cap) >> 57) & 0x1)
#define NVME_CMB_BIR(cmbloc) ((cmbloc) & 0x7)
#define NVME_CMB_OFST(cmbloc) (((cmbloc) >> 12) & 0xf)
--
1.8.3.1
