Page MenuHomeFreeBSD
Differential D53296

nvme host buffer (HMB): alignment quirck for Crucial NVME
ClosedPublic
Actions

Authored by br on Oct 23 2025, 10:46 AM.
Tags
None
Referenced Files
F175236115: D53296.id164870.diff
Fri, Oct 9, 8:03 AM
F175214521: D53296.id167071.diff
Fri, Oct 9, 4:07 AM
F175214474: D53296.id166506.diff
Fri, Oct 9, 4:07 AM
F175214339: D53296.id.diff
Fri, Oct 9, 4:05 AM
F175214212: D53296.diff
Fri, Oct 9, 4:04 AM
Unknown Object (File)
Wed, Oct 7, 7:15 AM
Unknown Object (File)
Tue, Oct 6, 9:20 AM
Unknown Object (File)
Mon, Oct 5, 10:54 PM
View All Files
Subscribers
dab
jhb

Details

Reviewers
imp
Commits
rGb87f6330013f: nvme(4): Increase HMB alignment.
Summary

This patch fixes the following issue discovered with Crucial P310 NVME on Codasip Prime platform

nvme0: Allocated 64MB host memory buffer
pcib0: Master decode errornvme0: SET_FEATURES (09) sqid:0 cid:15 nsid:0 cdw10:0000000d cdw11:00000001
nvme0: CONTROLLER_PATHING_ERROR (03/60) crd:0 m:0 dnr:0 p:1 sqid:0 cid:15 cdw0:0
nvme0: nvme_ctrlr_hmb_enable failed!

There is no errors shown when the patch is applied.
The controller is operational with or without the patch (no speed difference).

Test Plan

Tested on Codasip Prime

nda0 at nvme0 bus 0 scbus0 target 0 lun 1
nda0: <CT1000P310SSD8 VACR001 25184FFB4BF8>
nda0: Serial Number 25184FFB4BF8
nda0: nvme version 2.0
nda0: 953869MB (1953525168 512 byte sectors)

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

br created this revision.Oct 23 2025, 10:46 AM
Herald added a subscriber: dab. · View Herald TranscriptOct 23 2025, 10:46 AM
br requested review of this revision.Oct 23 2025, 10:46 AM
br edited the test plan for this revision. (Show Details)Oct 23 2025, 10:51 AM
brooks added a reviewer: imp.Oct 23 2025, 10:54 AM
br edited the test plan for this revision. (Show Details)Oct 23 2025, 10:57 AM
br edited the test plan for this revision. (Show Details)
br edited the summary of this revision. (Show Details)Oct 23 2025, 11:02 AM
br edited the test plan for this revision. (Show Details)
br edited the summary of this revision. (Show Details)
imp added a comment.Oct 23 2025, 1:55 PM

why is 32 and better than 16?
Why not 64 or 128? Where does the 32 come from?

imp added a comment.EditedOct 23 2025, 2:23 PM

NVM Express ® Base Specification, revision 2.0
5.27.1.10 Host Memory Buffer (Feature Identifier 0Dh), (Optional)
"Host Memory Descriptor List Lower Address (HMDLLA): This field specifies the least significant 32 bits of the physical location of the Host Memory Descriptor List (refer to Figure 335) for the Host Memory Buffer. This address shall be 16 byte aligned, indicated by bits 3:0 being cleared to 0h."

So why is 32-bytes needed?

I've also been fielding bugs where not-quite-aligned-enough buffers are passed into the load routine that loads bogus PTPs as a result of it.

Also, seems like we should be logging cdw12,13,14,15 on these errors as well...

br added a comment.Oct 23 2025, 4:49 PM

Good points. I will check with Codasip if this is something to do with CHERI (I don't see any connection however)

br added a comment.Oct 24 2025, 7:08 AM

What I see when 32 byte requested it actually gives us PAGE_SIZE aligned address, which works.

When 16 byte (by spec) requested it gives a 16 byte aligned address which does not work.

I guess that Codasip did not see an issue because Linux allocates HMB by pages:
https://github.com/torvalds/linux/blob/master/drivers/nvme/host/pci.c#L2309

br updated this revision to Diff 166506.Nov 15 2025, 12:17 PM
br retitled this revision from nvme host buffer: increase alignment to nvme host buffer (HMB): alignment quirck for Crucial NVME.
br edited the summary of this revision. (Show Details)

The issue arise on Crucial P310 only, so convert to a quirk

jhb added a subscriber: jhb.Nov 15 2025, 1:12 PM
jhb added inline comments.
sys/dev/nvme/nvme_private.h
227

It might be better to name the quirk "QUIRK_HMB_PAGE_ALIGNMENT" to reflect what it does? In particular if we ever end up using it for other vendors. The other alignment quirk is also a bit poorly named IMO.

br updated this revision to Diff 166519.Nov 15 2025, 8:17 PM

rename macro

imp added a comment.Nov 15 2025, 9:00 PM

I've thought about this. And maybe we'd be better off always aligning to a page. It's going to be a large allocation and it will be multiple pages and likely the size is a multiple of pages. It would make the most sense to use full pages, no?

br updated this revision to Diff 166521.Nov 15 2025, 9:47 PM

Agree on using PAGE_SIZE for everyone. Most likely more controllers have this issue (even within Crucial model range), so it is a way easier to align to page_size all of them rather than deal with bug reports from users. We are a general purpose OS.
And again, Linux align to PAGE_SIZE all of them as well.

jhb added a comment.Nov 17 2025, 4:17 PM
In D53296#1227854, @imp wrote:

I've thought about this. And maybe we'd be better off always aligning to a page. It's going to be a large allocation and it will be multiple pages and likely the size is a multiple of pages. It would make the most sense to use full pages, no?

I'm actually surprised that any multi-page request isn't already page aligned? I'm pretty sure this ends up calling contigmalloc() under the hood once the allocation is at least a page in size and that will always return something page-aligned. I wonder what the actual size is in this case? Anyways, I think this patch is probably fine.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 25 2025, 8:46 AM
Closed by commit rGb87f6330013f: nvme(4): Increase HMB alignment. (authored by br). · Explain Why
This revision was automatically updated to reflect the committed changes.
br added a commit: rGb87f6330013f: nvme(4): Increase HMB alignment..

Revision Contents
Changeset List

PathSize
sys/
dev/
nvme/
3 lines
1 line
1 line

Diff 166519

View Options

sys/dev/nvme/nvme_ctrlr.c

Loading...
View Options

sys/dev/nvme/nvme_pci.c

Loading...
View Options

sys/dev/nvme/nvme_private.h

Loading...