Page MenuHomeFreeBSD

nvme: add Apple T2 ANS2 NVMe quirks
Needs ReviewPublic

Authored by guest-seuros on Tue, May 19, 10:26 AM.
Referenced Files
F157554920: D57087.id178309.diff
Fri, May 22, 6:50 PM
F157554897: D57087.id178274.diff
Fri, May 22, 6:50 PM
F157554882: D57087.diff
Fri, May 22, 6:50 PM
F157547257: D57087.id178274.diff
Fri, May 22, 4:52 PM
F157547248: D57087.id178274.diff
Fri, May 22, 4:52 PM
F157547215: D57087.id178274.diff
Fri, May 22, 4:51 PM
F157545573: D57087.id178080.diff
Fri, May 22, 4:23 PM
Unknown Object (File)
Thu, May 21, 1:22 PM
Subscribers

Details

Reviewers
adrian
ngie
imp
Summary

The Apple T2 (ANS2, PCI 106b:2005) requires several quirks:

  • 128-byte submission queue entries (CC.IOSQES = 7)
  • Single MSI vector, one IO queue
  • Admin and IO queues share a CID table; IO CIDs offset by adminq.num_trackers to avoid overlap
  • No async event support
  • IDENTIFY CNS >= 2 rejected to avoid firmware confusion

Tested-on:

  • MacBookPro16,2 (A2251)
  • Mac mini 8,1 (A1993)
  • Multiple Non-Apple computers

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 73308
Build 70191: arc lint + arc unit

Event Timeline

sys/dev/nvme/nvme_qpair.c
1086

Does this work instead? I'm not a huge fan of "wild pointer" arithmetic; the array notation makes alignment, sizing, and indexing clearer to me, if that strategy could be used instead.

sys/dev/nvme/nvme_sim.c
107

Is there a well-defined constant that can be used in place of 2?

sys/dev/nvme/nvme_sysctl.c
72–73

Does this work instead?

Also: what kinds of tests did you run? Attach/detach? Read/write tests (with dd/fio)? Etc.

sys/dev/nvme/nvme_private.h
230

Does it make sense to think about how one might do this as a subclass instead of a series of particularly specific quirks?

Also: what kinds of tests did you run? Attach/detach? Read/write tests (with dd/fio)? Etc.

Well , i didn't test detach since the nvme is soldered and it the disk i booted from.

However i did use it a daily and compiled the kernel 40+ times and build the world 5 time without any hiccups.

guest-seuros added inline comments.
sys/dev/nvme/nvme_private.h
230

Yes, but i'm thinking we could land it without too much mutation and early optimisations, then refactor it.

PS: A bunch of the code/logic is reused in the Apple Sillicon implementation. (I have both running). And the Diff will make no sense without the full picture.

sys/dev/nvme/nvme_sim.c
107

We can add it.

sys/dev/nvme/nvme_pci.c
96

So this is a big ugly hammer for early, buggy NVMe drives. Maybe you can help me understand the symptom this cures? And if the really long time we delay before we check for ready is needed?

341

How does trying something we know won't work future proof the code? Is there other NVMe drives that apple makes with this quirk coming soon?

sys/dev/nvme/nvme_private.h
230

I know that Linux went the subclass route, and I think that these operations may be such that we might want to do that too.
Plus I have access to a crypto card that uses the NVMe interface that has other special needs which a properly designed subclass system will fit into.

Having said that, I'm cool with the hacks for the moment until we have something working and solid. Then we can refactor... I know this is a bit of a risk because once it's working, people move on...

sys/dev/nvme/nvme_qpair.c
1086

Maybe make it a macro instead of inlining it, since we'll need to do this in multiple places.

sys/dev/nvme/nvme_sim.c
107

Yea. I'd be tempted to just make this a field (maybe ctrlr->max_id_cns) and compare against it always for OPC_IDENTIFY commands?

And is this the right place to put it? I'd think we'd want to put it lower in the stack since not all identify commands come through here.

sys/dev/nvme/nvme_sysctl.c
72–73

See my comment about a macro instead.

I will update this diff shortly. I need to retest in the hardware.

Once this diff land, users can install the OS on the internal NVME.

They will need the other linked diffs to get Keyboard,mouse, audio.

sys/dev/nvme/nvme_pci.c
341

There are few other NVME with other models that i could not test on. This code was defensive, i will remove it.

guest-seuros marked an inline comment as done.

Address review: NVME_SQE macro, max_id_cns field, removed defensive code

remove DELAY_B4_CHK_RDY, the timeout is much smaller, will measure later. not a blocker