[PATCH 0/2] Misc fixes on registering PCI NVMe CMB

[PATCH 0/2] Misc fixes on registering PCI NVMe CMB

[Icenowy Zheng]

Here is a small patchset that is developed during my investigation of
a WARNING in my boot kernel log (AMD EPYC 7K62 CPU + Intel DC D4502
SSD), which is because of the SSD's too-small CMB block (512KB only).

The first patch is a fix of the PCI DMA registration error handling
codepath, which is just a observation-based patch (because my disk is
only NVMe 1.2 compliant, and the register cleaned up here is only added
in NVMe 1.4).

The second patch really fixes the warning by testing the CMB block
against the memory hotplugging alignment requirement (which the CMB
block of my SSD surely cannot satisfy -- the alignment requirement is
usually 2M with SPAREMEM_VMEMMAP enabled and even larger in other cases).

Refer to commit 6acd7d5ef264 ("libnvdimm/namespace: Enforce
memremap_compat_align()") for a similar approach for NVDIMM subsystem.

Icenowy Zheng (2):
  nvme-pci: clean up CMBMSC when registering CMB fails
  nvme-pci: skip CMB blocks incompatible with PCI P2P DMA

 drivers/nvme/host/pci.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

-- 
2.48.1

[PATCH 1/2] nvme-pci: clean up CMBMSC when registering CMB fails

[Icenowy Zheng]

CMB decoding should get disabled when the CMB block isn't successfully
registered to P2P DMA subsystem.

Clean up the CMBMSC register in this error handling codepath to disable
CMB decoding (and CMBLOC/CMBSZ registers).

Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
 drivers/nvme/host/pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 9197a5b173fdf..659ba85795a91 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2003,6 +2003,7 @@ static void nvme_map_cmb(struct nvme_dev *dev)
 	if (pci_p2pdma_add_resource(pdev, bar, size, offset)) {
 		dev_warn(dev->ctrl.device,
 			 "failed to register the CMB\n");
+		hi_lo_writeq(0, dev->bar + NVME_REG_CMBMSC);
 		return;
 	}
 
-- 
2.48.1

[PATCH 2/2] nvme-pci: skip CMB blocks incompatible with PCI P2P DMA

[Icenowy Zheng]

The PCI P2PDMA code will register the CMB block to the memory
hot-plugging subsystem, which have an alignment requirement. Memory
blocks that do not satisfy this alignment requirement (usually 2MB) will
lead to a WARNING from memory hotplugging.

Verify the CMB block's address and size against the alignment and only
try to send CMB blocks compatible with it to prevent this warning.

Tested on Intel DC D4502 SSD, which has a 512K CMB block that is too
small for memory hotplugging (thus PCI P2PDMA).

Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
 drivers/nvme/host/pci.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 659ba85795a91..1a27688417b3f 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1982,6 +1982,19 @@ static void nvme_map_cmb(struct nvme_dev *dev)
 	if (offset > bar_size)
 		return;
 
+	/*
+	 * Controllers may support a CMB size larger than their BAR,
+	 * for example, due to being behind a bridge. Reduce the CMB to
+	 * the reported size of the BAR
+	 */
+	if (size > bar_size - offset)
+		size = bar_size - offset;
+
+	if (!IS_ALIGNED(size, memremap_compat_align()) ||
+	    !IS_ALIGNED(pci_resource_start(pdev, bar),
+			memremap_compat_align()))
+		return;
+
 	/*
 	 * Tell the controller about the host side address mapping the CMB,
 	 * and enable CMB decoding for the NVMe 1.4+ scheme:
@@ -1992,14 +2005,6 @@ static void nvme_map_cmb(struct nvme_dev *dev)
 			     dev->bar + NVME_REG_CMBMSC);
 	}
 
-	/*
-	 * Controllers may support a CMB size larger than their BAR,
-	 * for example, due to being behind a bridge. Reduce the CMB to
-	 * the reported size of the BAR
-	 */
-	if (size > bar_size - offset)
-		size = bar_size - offset;
-
 	if (pci_p2pdma_add_resource(pdev, bar, size, offset)) {
 		dev_warn(dev->ctrl.device,
 			 "failed to register the CMB\n");
-- 
2.48.1

Re: [PATCH 0/2] Misc fixes on registering PCI NVMe CMB

[Christoph Hellwig]

On Thu, Feb 13, 2025 at 01:04:42AM +0800, Icenowy Zheng wrote:
> Here is a small patchset that is developed during my investigation of
> a WARNING in my boot kernel log (AMD EPYC 7K62 CPU + Intel DC D4502
> SSD), which is because of the SSD's too-small CMB block (512KB only).

Hah, that's certainly and odd CMB configuration.

Re: [PATCH 1/2] nvme-pci: clean up CMBMSC when registering CMB fails

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] nvme-pci: skip CMB blocks incompatible with PCI P2P DMA

[Christoph Hellwig]

On Thu, Feb 13, 2025 at 01:04:44AM +0800, Icenowy Zheng wrote:
> +	/*
> +	 * Controllers may support a CMB size larger than their BAR,
> +	 * for example, due to being behind a bridge. Reduce the CMB to
> +	 * the reported size of the BAR
> +	 */
> +	if (size > bar_size - offset)
> +		size = bar_size - offset;

Nit: use up the full 80 characters for the comment, and maybe switch
the above expression to use the min() helper.

> +	if (!IS_ALIGNED(size, memremap_compat_align()) ||
> +	    !IS_ALIGNED(pci_resource_start(pdev, bar),
> +			memremap_compat_align()))

These compat names looked odd to me, but it looks like they are indeed
the proper interface here.  So modulo the little style things above
this looks good, thanks a lot!

Re: [PATCH 0/2] Misc fixes on registering PCI NVMe CMB

[Icenowy Zheng]

在 2025-02-13星期四的 06:54 +0100，Christoph Hellwig写道：
> On Thu, Feb 13, 2025 at 01:04:42AM +0800, Icenowy Zheng wrote:
> > Here is a small patchset that is developed during my investigation
> > of
> > a WARNING in my boot kernel log (AMD EPYC 7K62 CPU + Intel DC D4502
> > SSD), which is because of the SSD's too-small CMB block (512KB
> > only).
> 
> Hah, that's certainly and odd CMB configuration.
> 

Sure, maybe it's just intended for a little queue. Register 0x38 value
is 0x00000004, Register 0x3c is 0x00080001, and BAR 4 is 64-bit
prefetchable memory with size=512K. I tested writing arbitary data to
the BAR 4's first few words and it correctly retains (which means they
seem to be really memory instead of registers).

I saw some mention of support CMB for submission queue in the brief of
Intel D3700/3600, maybe this applies to D450x as a successor?

BTW I am not sure about the relationship between Intel D series and P
series SSDs (only knows that D series is for dual-port redundancy), and
I have a P4511 as my boot disk (D4502 is data storage), which comes
with no CMB at all. (P4511 and D4502 shares PCI ID, but not PCI
subsystem ID).

Re: [PATCH 2/2] nvme-pci: skip CMB blocks incompatible with PCI P2P DMA

[Icenowy Zheng]

在 2025-02-13星期四的 06:58 +0100，Christoph Hellwig写道：
> On Thu, Feb 13, 2025 at 01:04:44AM +0800, Icenowy Zheng wrote:
> > +       /*
> > +        * Controllers may support a CMB size larger than their
> > BAR,
> > +        * for example, due to being behind a bridge. Reduce the
> > CMB to
> > +        * the reported size of the BAR
> > +        */
> > +       if (size > bar_size - offset)
> > +               size = bar_size - offset;
> 
> Nit: use up the full 80 characters for the comment, and maybe switch
> the above expression to use the min() helper.

I am only moving around existing code here.

Maybe I should refactor the code here and then move it?

> 
> > +       if (!IS_ALIGNED(size, memremap_compat_align()) ||
> > +           !IS_ALIGNED(pci_resource_start(pdev, bar),
> > +                       memremap_compat_align()))
> 
> These compat names looked odd to me, but it looks like they are
> indeed
> the proper interface here.  So modulo the little style things above
> this looks good, thanks a lot!
> 

Re: [PATCH 0/2] Misc fixes on registering PCI NVMe CMB

[Keith Busch]

On Thu, Feb 13, 2025 at 06:54:49AM +0100, Christoph Hellwig wrote:
> On Thu, Feb 13, 2025 at 01:04:42AM +0800, Icenowy Zheng wrote:
> > Here is a small patchset that is developed during my investigation of
> > a WARNING in my boot kernel log (AMD EPYC 7K62 CPU + Intel DC D4502
> > SSD), which is because of the SSD's too-small CMB block (512KB only).
> 
> Hah, that's certainly and odd CMB configuration.

Should be okay if it's just for submission queues. The driver has an
arbitrary requirement that the queues have at least 64 entries for CMB,
and 512k allows us to create 128 submission queues like that. That's
enough for most systems.

Re: [PATCH 0/2] Misc fixes on registering PCI NVMe CMB

[Keith Busch]

On Thu, Feb 13, 2025 at 01:04:42AM +0800, Icenowy Zheng wrote:
> Here is a small patchset that is developed during my investigation of
> a WARNING in my boot kernel log (AMD EPYC 7K62 CPU + Intel DC D4502
> SSD), which is because of the SSD's too-small CMB block (512KB only).
> 
> The first patch is a fix of the PCI DMA registration error handling
> codepath, which is just a observation-based patch (because my disk is
> only NVMe 1.2 compliant, and the register cleaned up here is only added
> in NVMe 1.4).
> 
> The second patch really fixes the warning by testing the CMB block
> against the memory hotplugging alignment requirement (which the CMB
> block of my SSD surely cannot satisfy -- the alignment requirement is
> usually 2M with SPAREMEM_VMEMMAP enabled and even larger in other cases).

Applied to nvme-6.14 with the suggested changes in patch 2 folded in.

Re: [PATCH 0/2] Misc fixes on registering PCI NVMe CMB

[Icenowy Zheng]

在 2025-02-24星期一的 17:17 -0700，Keith Busch写道：
> On Thu, Feb 13, 2025 at 06:54:49AM +0100, Christoph Hellwig wrote:
> > On Thu, Feb 13, 2025 at 01:04:42AM +0800, Icenowy Zheng wrote:
> > > Here is a small patchset that is developed during my
> > > investigation of
> > > a WARNING in my boot kernel log (AMD EPYC 7K62 CPU + Intel DC
> > > D4502
> > > SSD), which is because of the SSD's too-small CMB block (512KB
> > > only).
> > 
> > Hah, that's certainly and odd CMB configuration.
> 
> Should be okay if it's just for submission queues. The driver has an
> arbitrary requirement that the queues have at least 64 entries for
> CMB,
> and 512k allows us to create 128 submission queues like that. That's
> enough for most systems.

Yes, but this configuration seems to not fit the current driver code
that utilizes PCIe P2P setup code. (Is there any driver that could
utilize this configuration now?)