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

[br]

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)

[imp]

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

[imp]

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]

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

[br]

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]

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

[jhb]

sys/dev/nvme/nvme_private.h

227 ↗

(On Diff #166506)

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]

rename macro

[imp]

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]

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]

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.