nvme-pci: 512 byte aligned dma pool segment quirk

From: Robert Beckett <[email protected]>

We initially put in a quick fix of limiting the queue depth to 1 as experimentation showed that it fixed data corruption on 64GB steamdecks.

After further experimentation, it appears that the corruption is fixed by aligning the small dma pool segments to 512 bytes. Testing via desync image verification shows that it now passes thousands of verification loops, where previously
it never managed above 7.

Quoted 6 lines · Bob Beckett

From: Robert Beckett <[email protected]>

We initially put in a quick fix of limiting the queue depth to 1 as experimentation showed that it fixed data corruption on 64GB steamdecks.

Quoted 5 lines

After further experimentation, it appears that the corruption is fixed by aligning the small dma pool segments to 512 bytes. Testing via desync image verification shows that it now passes thousands of verification loops, where previously
it never managed above 7.

As suggested before, instead of changing the pool size please just always use the large pool for this device.

Quoted 13 lines · Bob Beckett

Quoted 11 lines

From: Robert Beckett [email protected]>

We initially put in a quick fix of limiting the queue depth to 1 as experimentation showed that it fixed data corruption on 64GB steamdecks.

After further experimentation, it appears that the corruption is fixed by aligning the small dma pool segments to 512 bytes. Testing via desync image verification shows that it now passes thousands of verification loops, where previously
it never managed above 7.

Quoted 2 lines

As suggested before, instead of changing the pool size please just always use the large pool for this device.

Well, he's doing what I suggested. I thought this was better because it puts the decision making in the initialization path instead of the IO path.

Well, he's doing what I suggested. I thought this was better because it puts the decision making in the initialization path instead of the IO path.

Quoted 3 lines

Well, he's doing what I suggested. I thought this was better because it puts the decision making in the initialization path instead of the IO path.

I guess it's fine in that regard. It just usually expect patch authors to at least reply to reviews..

When doing a write of 264 sectors with PRP list offset of 0xf00, the bridge treats data as a pointer, and writes incorrect data to the drive. This might be why Robert is experiencing fs corruption.

Quoted 3 lines · Pawel Anikiel

When doing a write of 264 sectors with PRP list offset of 0xf00, the bridge treats data as a pointer, and writes incorrect data to the drive. This might be why Robert is experiencing fs corruption.

Not having the hardware and just speculating, but this seems like a pretty likely failure mode.

Hi all,

I've been tracking down an issue that seems to be related (identical?) to this one, and I would like to propose a different fix.

I have a device with the aforementioned NVMe-eMMC bridge, and I was experiencing nvme read timeouts after updating the kernel from 5.15 to 6.6. Doing a kernel bisect, I arrived at the same dma pool commit as Robert in the original thread.

After trying out some changes in the nvme-pci driver, I came up with the same fix as in this thread: change the alignment of the small pool to
512. However, I wanted to get a deeper understanding of what's going on.

After a lot of analysis, I found out why the nvme timeouts were happening:
The bridge incorrectly implements PRP list chaining.

When doing a read of exactly 264 sectors, and allocating a PRP list with offset 0xf00, the last PRP entry in that list lies right before a page boundary. The bridge incorrectly (?) assumes that it's a pointer to a chained PRP list, tries to do a DMA to address 0x0, gets a bus error, and crashes.

When doing a write of 264 sectors with PRP list offset of 0xf00, the bridge treats data as a pointer, and writes incorrect data to the drive. This might be why Robert is experiencing fs corruption.

So if my findings are right, the correct quirk would be "don't make PRP lists ending on a page boundary".

Changing the small dma pool alignment to 512 happens to fix the issue because it never allocates a PRP list with offset 0xf00. Theoretically, the issue could still happen with the page pool, but this bridge has a max transfer size of 64 pages, which is not enough to fill an entire page-sized PRP list.

Robert, could you check that the fs corruption happens only after transfers of size 257-264 and PRP list offset of 0xf00? This would confirm my theory.

This is interesting.
I had the same idea previously. I initially just changed the hard coded 256 / 8 to use 31 instead, which should have ensured the last entry of each segment never gets used. When I tested that, it not longer failed, which was a good sign. So then I modified it to only do that on the last 256 byte segment of a page, but then is started failing again.

I never saw any bus error during my testing, just wrong data read, which then fails image verification. I was expecting iommu error logs if it was trying to access a chain in to nowhere if it always interpreted last entry in page as a link. I never saw any iommu errors.

I'd be glad to if you could share your testing method.

Quoted 3 lines · On Thu, Nov 14, 2024 at 2:24 PM Robert Beckett [email protected]> wrote:

This is interesting.
I had the same idea previously. I initially just changed the hard coded 256 / 8 to use 31 instead, which should have ensured the last entry of each segment never gets used. When I tested that, it not longer failed, which was a good sign. So then I modified it to only do that on the last 256 byte segment of a page, but then is started failing again.

Could you elaborate the "only do that on the last 256 byte segment of a page" part? How did you check which chunk of the page would be allocated before choosing the dma pool?

Quoted 1 line

I never saw any bus error during my testing, just wrong data read, which then fails image verification. I was expecting iommu error logs if it was trying to access a chain in to nowhere if it always interpreted last entry in page as a link. I never saw any iommu errors.

Maybe I misspoke, the "bus error" part was just my speculation, I didn't look at the IOMMU logs or anything like that.

Quoted 1 line

I'd be glad to if you could share your testing method.

I dumped all the nvme transfers before the crash happened (using tracefs), and I saw a read of size 264 = 8 + 256, which led me to the chaining theory. To test this claim, I wrote a simple pci device driver which creates one IO queue and submits a read command where the PRP list is set up in a way that tests if the controller treats it as a chained list or not. I ran it, and it indeed treated the last PRP entry as a chained pointer.

It is possible that this is not the only thing that's wrong. Could you share your patch that checks your "only do that on the last 256 byte segment of a page" idea?

---- On Thu, 14 Nov 2024 14:13:52 +0000 Paweł Anikiel wrote ---

Quoted 21 lines

Quoted 3 lines · On Thu, Nov 14, 2024 at 2:24 PM Robert Beckett [email protected]> wrote:

This is interesting.
I had the same idea previously. I initially just changed the hard coded 256 / 8 to use 31 instead, which should have ensured the last entry of each segment never gets used. When I tested that, it not longer failed, which was a good sign. So then I modified it to only do that on the last 256 byte segment of a page, but then is started failing again.

Could you elaborate the "only do that on the last 256 byte segment of a page" part? How did you check which chunk of the page would be allocated before choosing the dma pool?

Quoted 5 lines

I never saw any bus error during my testing, just wrong data read, which then fails image verification. I was expecting iommu error logs if it was trying to access a chain in to nowhere if it always interpreted last entry in page as a link. I never saw any iommu errors.

Maybe I misspoke, the "bus error" part was just my speculation, I didn't look at the IOMMU logs or anything like that.

Quoted 1 line

I'd be glad to if you could share your testing method.

Quoted 7 lines

I dumped all the nvme transfers before the crash happened (using tracefs), and I saw a read of size 264 = 8 + 256, which led me to the chaining theory. To test this claim, I wrote a simple pci device driver which creates one IO queue and submits a read command where the PRP list is set up in a way that tests if the controller treats it as a chained list or not. I ran it, and it indeed treated the last PRP entry as a chained pointer.

hmm, I guess a simple debugfs trigger file could be used to construct specially formulated requests. Would work as a debug tool.

Though at this point, the simple dmapool alignment param usage fixes both of these scenarios, so it will be kind of academic to continue putting effort in to understand this. I am trying to get answers out of the vendor to confirm any of these theories, which I hope will be more conclusive than our combined inference from testing.

Quoted 32 lines

---- On Thu, 14 Nov 2024 14:13:52 +0000 Paweł Anikiel wrote ---

Quoted 21 lines

Quoted 3 lines · On Thu, Nov 14, 2024 at 2:24 PM Robert Beckett [email protected]> wrote:

This is interesting.
I had the same idea previously. I initially just changed the hard coded 256 / 8 to use 31 instead, which should have ensured the last entry of each segment never gets used. When I tested that, it not longer failed, which was a good sign. So then I modified it to only do that on the last 256 byte segment of a page, but then is started failing again.

Could you elaborate the "only do that on the last 256 byte segment of a page" part? How did you check which chunk of the page would be allocated before choosing the dma pool?

Quoted 5 lines

I never saw any bus error during my testing, just wrong data read, which then fails image verification. I was expecting iommu error logs if it was trying to access a chain in to nowhere if it always interpreted last entry in page as a link. I never saw any iommu errors.

Maybe I misspoke, the "bus error" part was just my speculation, I didn't look at the IOMMU logs or anything like that.

Quoted 1 line

I'd be glad to if you could share your testing method.

Quoted 7 lines

I dumped all the nvme transfers before the crash happened (using tracefs), and I saw a read of size 264 = 8 + 256, which led me to the chaining theory. To test this claim, I wrote a simple pci device driver which creates one IO queue and submits a read command where the PRP list is set up in a way that tests if the controller treats it as a chained list or not. I ran it, and it indeed treated the last PRP entry as a chained pointer.

hmm, I guess a simple debugfs trigger file could be used to construct specially formulated requests. Would work as a debug tool.

Quoted 5 lines

Though at this point, the simple dmapool alignment param usage fixes both of these scenarios, so it will be kind of academic to continue putting effort in to understand this. I am trying to get answers out of the vendor to confirm any of these theories, which I hope will be more conclusive than our combined inference from testing.

Any updates on this? I'm satisfied with the quirk patch, so we can move this forward if you're okay with the current understanding.

[...]
I have no further updates on this. I have received no further info from the vendor.
I think we can go ahead and use the alignment patch as is. The only outstanding question was whether it is an implicit last entry per page chain vs simple alisngment requirement. Either way, using the dmapool alignment fixes both of these potential causes, so we should just take it as is.
If we ever get any better info and can do a more specific patch in future, we can rework it then.

Quoted 6 lines · Robert Beckett

[...]
I have no further updates on this. I have received no further info from the vendor.
I think we can go ahead and use the alignment patch as is. The only outstanding question was whether it is an implicit last entry per page chain vs simple alisngment requirement. Either way, using the dmapool alignment fixes both of these potential causes, so we should just take it as is.
If we ever get any better info and can do a more specific patch in future, we can rework it then.

I think the 512 byte alignment fix is good. I tried coming up with something more specific, but everything I could think of was either too complicated or artificial.

Regarding the question of whether this is an alignment requirement or the last PRP entry issue, I strongly believe it's the latter. I have a piece of code that clearly demonstrates the hardware bug when run on a device with the nvme bridge. I would really appreciate it if this was verified and my explanation was included in the patch.

Quoted 12 lines · On Mon, Dec 9, 2024 at 1:33 PM Robert Beckett [email protected]> wrote:

Quoted 6 lines

[...]
I have no further updates on this. I have received no further info from the vendor.
I think we can go ahead and use the alignment patch as is. The only outstanding question was whether it is an implicit last entry per page chain vs simple alisngment requirement. Either way, using the dmapool alignment fixes both of these potential causes, so we should just take it as is.
If we ever get any better info and can do a more specific patch in future, we can rework it then.

I think the 512 byte alignment fix is good. I tried coming up with something more specific, but everything I could think of was either too complicated or artificial.

Quoted 5 lines

Regarding the question of whether this is an alignment requirement or the last PRP entry issue, I strongly believe it's the latter. I have a piece of code that clearly demonstrates the hardware bug when run on a device with the nvme bridge. I would really appreciate it if this was verified and my explanation was included in the patch.

I've pushed this to nvme-6.13 with an updated commit message here:

https://git.infradead.org/?p=nvme.git;a=commitdiff;h=ccd84b8d6f4a60626dacb933b5d56dadca75c0ca

I can force an update if you have any edit suggestions.

Commit message copied below:

Author: Robert Beckett [email protected]>

nvme-pci: 512 byte aligned dma pool segment quirk

We initially introduced a quick fix limiting the queue depth to 1 as experimentation showed that it fixed data corruption on 64GB steamdecks.

Further experimentation revealed corruption only happens when the last PRP data element aligns to the end of the page boundary. The device appears to treat this as a PRP chain to a new list instead of the data element that it actually is. This is an implementation is in violation of the spec. Encountering this errata with the Linux driver requires the host request a 128k transfer and coincidently get the last small pool dma buffer within a page.

The QD1 quirk effectly works around this because the last data PRP always was at a 248 byte offset from the page start, so it never appeared at the end of the page. Further, the MDTS is also small enough that the "large" prp pool can hold enough PRP elements to never get to the end, so that pool is not a problem either.

Introduce a new quirk to ensure the small pool is always aligned such that the last PRP element can't appear a the end of the page. This comes at the expense of wasting 256 bytes per small pool page allocated.

Link https://lore.kernel.org/linux-nvme/[email protected]/T/#u open here

Fixes 83bdfcbdbe5d ("nvme-pci: qdepth 1 quirk")

Cc Paweł Anikiel [email protected]>

Signed-off-by Robert Beckett [email protected]>

Signed-off-by Keith Busch [email protected]>

---- On Tue, 10 Dec 2024 21:36:55 +0000 Keith Busch wrote ---

Quoted 19 lines · Paweł Anikiel

Quoted 17 lines · On Mon, Dec 9, 2024 at 1:33 PM Robert Beckett [email protected]> wrote:

Quoted 6 lines

[...]
I have no further updates on this. I have received no further info from the vendor.
I think we can go ahead and use the alignment patch as is. The only outstanding question was whether it is an implicit last entry per page chain vs simple alisngment requirement. Either way, using the dmapool alignment fixes both of these potential causes, so we should just take it as is.
If we ever get any better info and can do a more specific patch in future, we can rework it then.

I think the 512 byte alignment fix is good. I tried coming up with something more specific, but everything I could think of was either too complicated or artificial.

Regarding the question of whether this is an alignment requirement or the last PRP entry issue, I strongly believe it's the latter. I have a piece of code that clearly demonstrates the hardware bug when run on a device with the nvme bridge. I would really appreciate it if this was verified and my explanation was included in the patch.

Quoted 3 lines

I've pushed this to nvme-6.13 with an updated commit message here:

https://git.infradead.org/?p=nvme.git;a=commitdiff;h=ccd84b8d6f4a60626dacb933b5d56dadca75c0ca

lgtm. Thanks!

I've been tracking down an issue that seems to be related (identical?) to this one, and I would like to propose a different fix.

I have a device with the aforementioned NVMe-eMMC bridge, and I was experiencing nvme read timeouts after updating the kernel from 5.15 to 6.6. Doing a kernel bisect, I arrived at the same dma pool commit as Robert in the original thread.

After trying out some changes in the nvme-pci driver, I came up with the same fix as in this thread: change the alignment of the small pool to
512. However, I wanted to get a deeper understanding of what's going on.

After a lot of analysis, I found out why the nvme timeouts were happening:
The bridge incorrectly implements PRP list chaining.

When doing a read of exactly 264 sectors, and allocating a PRP list with offset 0xf00, the last PRP entry in that list lies right before a page boundary. The bridge incorrectly (?) assumes that it's a pointer to a chained PRP list, tries to do a DMA to address 0x0, gets a bus error, and crashes.

When doing a write of 264 sectors with PRP list offset of 0xf00, the bridge treats data as a pointer, and writes incorrect data to the drive. This might be why Robert is experiencing fs corruption.

So if my findings are right, the correct quirk would be "don't make PRP lists ending on a page boundary".

Changing the small dma pool alignment to 512 happens to fix the issue because it never allocates a PRP list with offset 0xf00. Theoretically, the issue could still happen with the page pool, but this bridge has a max transfer size of 64 pages, which is not enough to fill an entire page-sized PRP list.

Quoted 21 lines

I've been tracking down an issue that seems to be related (identical?) to this one, and I would like to propose a different fix.

I have a device with the aforementioned NVMe-eMMC bridge, and I was experiencing nvme read timeouts after updating the kernel from 5.15 to 6.6. Doing a kernel bisect, I arrived at the same dma pool commit as Robert in the original thread.

After trying out some changes in the nvme-pci driver, I came up with the same fix as in this thread: change the alignment of the small pool to
512. However, I wanted to get a deeper understanding of what's going on.

After a lot of analysis, I found out why the nvme timeouts were happening:
The bridge incorrectly implements PRP list chaining.

When doing a read of exactly 264 sectors, and allocating a PRP list with offset 0xf00, the last PRP entry in that list lies right before a page boundary. The bridge incorrectly (?) assumes that it's a pointer to a chained PRP list, tries to do a DMA to address 0x0, gets a bus error, and crashes.

Quoted 3 lines

When doing a write of 264 sectors with PRP list offset of 0xf00, the bridge treats data as a pointer, and writes incorrect data to the drive. This might be why Robert is experiencing fs corruption.

This sounds very plausible, great analysis. Curious though, even without the dma pool optimizations, you could still allocate a PRP list at that offset. I wonder why the problem only showed up once we optimized the pool allocator.

Quoted 3 lines

So if my findings are right, the correct quirk would be "don't make PRP lists ending on a page boundary".

Coincidently enough, the quirk in this patch achieves that. But it's great to understand why it was successful.

Quoted 5 lines

Changing the small dma pool alignment to 512 happens to fix the issue because it never allocates a PRP list with offset 0xf00. Theoretically, the issue could still happen with the page pool, but this bridge has a max transfer size of 64 pages, which is not enough to fill an entire page-sized PRP list.

Thanks, this answers my question in the other thread: MDTS is too small to hit the same bug with the large pool.

From: Robert Beckett <[email protected]>

We initially put in a quick fix of limiting the queue depth to 1 as experimentation showed that it fixed data corruption on 64GB steamdecks.

After further experimentation, it appears that the corruption is fixed by aligning the small dma pool segments to 512 bytes. Testing via desync image verification shows that it now passes thousands of verification loops, where previously
it never managed above 7.

Currently it is not known why this fixes the corruption. Perhaps it is doing something nasty like using an mmc page as a cache for the prp lists (mmc min. page size is 512 bytes) and not invalidating properly, so that the dma pool change to treats segment list as a stack ends up giving a previous segment in the same cached page.

This fixes the previous queue depth limitation as it fixes the corruption without incurring a 37% tested performance degredation.

Fixes 83bdfcbdbe5d ("nvme-pci: qdepth 1 quirk")

Quoted 23 lines · Bob Beckett

From: Robert Beckett <[email protected]>

We initially put in a quick fix of limiting the queue depth to 1 as experimentation showed that it fixed data corruption on 64GB steamdecks.

After further experimentation, it appears that the corruption is fixed by aligning the small dma pool segments to 512 bytes. Testing via desync image verification shows that it now passes thousands of verification loops, where previously
it never managed above 7.

Currently it is not known why this fixes the corruption. Perhaps it is doing something nasty like using an mmc page as a cache for the prp lists (mmc min. page size is 512 bytes) and not invalidating properly, so that the dma pool change to treats segment list as a stack ends up giving a previous segment in the same cached page.

This fixes the previous queue depth limitation as it fixes the corruption without incurring a 37% tested performance degredation.

Quoted 1 line

Fixes 83bdfcbdbe5d ("nvme-pci: qdepth 1 quirk")

I had this queued up for the nvme-6.12 pull request, which I'm about to send out, but I guess we should drop it until we conclude this discussion. With 6.12 likely to be released on Sunday, this better mitigation would need to target 6.13, then stable.

From: Robert Beckett <[email protected]>

We initially put in a quick fix of limiting the queue depth to 1 as experimentation showed that it fixed data corruption on 64GB steamdecks.

After further experimentation, it appears that the corruption is fixed by aligning the small dma pool segments to 512 bytes. Testing via desync image verification shows that it now passes thousands of verification loops, where previously
it never managed above 7.

Currently it is not known why this fixes the corruption. Perhaps it is doing something nasty like using an mmc page as a cache for the prp lists (mmc min. page size is 512 bytes) and not invalidating properly, so that the dma pool change to treats segment list as a stack ends up giving a previous segment in the same cached page.

This fixes the previous queue depth limitation as it fixes the corruption without incurring a 37% tested performance degredation.

Fixes 83bdfcbdbe5d ("nvme-pci: qdepth 1 quirk")

Signed-off-by Robert Beckett <[email protected]>

---
 drivers/nvme/host/nvme.h | 5 +++++
 drivers/nvme/host/pci.c  | 6 +++---
 2 files changed, 8 insertions(+), 3 deletions(-)

drivers/nvme/host/nvme.h +5 -0

index 093cb423f536..61bba5513de0 100644
@@ -173,6 +173,11 @@ enum nvme_quirks {
 	 * MSI (but not MSI-X) interrupts are broken and never fire.
 	 */
 	NVME_QUIRK_BROKEN_MSI			= (1 << 21),
+
+	/*
+	 * Align dma pool segment size to 512 bytes
+	 */
+	NVME_QUIRK_DMAPOOL_ALIGN_512		= (1 << 22),
 };
 
 /*

drivers/nvme/host/pci.c +3 -3

index 4b9fda0b1d9a..6fcd3bb413c4 100644
@@ -2700,8 +2700,8 @@ static int nvme_setup_prp_pools(struct nvme_dev *dev)
 		return -ENOMEM;
 
 	/* Optimisation for I/Os between 4k and 128k */
-	dev->prp_small_pool = dma_pool_create("prp list 256", dev->dev,
-						256, 256, 0);
+	dev->prp_small_pool = dma_pool_create("prp list 256", dev->dev,256,
+				       dev->ctrl.quirks & NVME_QUIRK_DMAPOOL_ALIGN_512 ? 512 : 256, 0);
 	if (!dev->prp_small_pool) {
 		dma_pool_destroy(dev->prp_page_pool);
 		return -ENOMEM;

@@ -3449,7 +3449,7 @@ static const struct pci_device_id nvme_id_table[] = {
 	{ PCI_VDEVICE(REDHAT, 0x0010),	/* Qemu emulated controller */
 		.driver_data = NVME_QUIRK_BOGUS_NID, },
 	{ PCI_DEVICE(0x1217, 0x8760), /* O2 Micro 64GB Steam Deck */
-		.driver_data = NVME_QUIRK_QDEPTH_ONE },
+		.driver_data = NVME_QUIRK_DMAPOOL_ALIGN_512, },
 	{ PCI_DEVICE(0x126f, 0x2262),	/* Silicon Motion generic */
 		.driver_data = NVME_QUIRK_NO_DEEPEST_PS |
 				NVME_QUIRK_BOGUS_NID, },

Signature · 1 line

2.45.2