nvme: add Apple T2 ANS2 NVMe quirks

[seuros]

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

[ngie]

sys/dev/nvme/nvme_qpair.c

1078

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?

[ngie]

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

[kevans]

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?

[seuros]

In D57087#1308586, @ngie wrote:

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.

[seuros]

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.

[imp]

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?

336

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

1078

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.

[seuros]

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

336

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

[seuros]

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

[seuros]

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

[adrian]

@imp does this look ok to you now?

[imp]

Some final nits to simplify things a bit. Also, if you can find the size of the sqe from the identify and/or the capabilities stuff, that would be better than a quirk. Have you investigated it.

sys/dev/nvme/nvme_ctrlr.c

183

I'll note for future work that we generally have shifted to if ((flag & FLAG) != 0) elsewhere in the kernel. However, this is correct for this driver because it's older and uses the older if (flag & FLAG) style.

sys/dev/nvme/nvme_pci.c

143

As a design thing, I think that this might be better as it's own quirk, though that might be slicing things too thinly.

sys/dev/nvme/nvme_private.h

339

If you normally set sqe_size to 1, and bumped that to '2' for apple, then this macro could actually be the much simpler &(qpair)->cmd[idx] (maybe with an extra set of parens).

sys/dev/nvme/nvme_qpair.c

548

This is where you'd do '1' or '2'.

569

and this would need an additional * sizeof(struct nvme_command) at the end.

1252

same here.

sys/dev/nvme/nvme_sim.c

107

how does

[seuros]

Address feedbacks.

[adrian]

@imp ok, one more review? :-)

[imp]

One last suggestion on the sqe size. I just realized we read the size from (ctrlr->cdata.sqes >> 4) & 0xf; and it's really a shift vale. we should leverage that and just shoft this number - 6 since our default SQE size is 64.

sys/dev/nvme/nvme_qpair.c

539

so if sqes_max is 6 or 7, then we cna simplify sqe_size to be seq_max - 6 always and rename to sqe_shift. But that would also need to change the * seq_size to a << seq_shift.

[adrian]

In D57087#1317346, @imp wrote:

One last suggestion on the sqe size. I just realized we read the size from (ctrlr->cdata.sqes >> 4) & 0xf; and it's really a shift vale. we should leverage that and just shoft this number - 6 since our default SQE size is 64.

ok seuros, this is the last one, right warner? 📦

[imp]

Last one

[ngie]

I forgot to submit this comment earlier:

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

In general I'm ok with the diff modulo the unnecessary goto, but I would want someone like @imp to weigh in before merging the change to :main since it's not my domain of expertise.

sys/dev/nvme/nvme_ctrlr.c

1129–1134

This is odd to create a goto/label which only jumps a few lines like this. Is there a reason why the previous logic was insufficient? I've proposed an alternative diff which avoids the goto/label.

[imp]

sys/dev/nvme/nvme_ctrlr.c

1129–1134

So it occurs to me that

if (nvme_ctrlr_identify(ctrlr) == 0 &&
    nvme_ctrlr_set_num_qpairs(ctrlr) == 0 &&
    nvme_ctrlr_construct_io_qpairs(ctrlr) == 0)
        nvme_ctrlr_start(ctrlr, false);
else
        nvme_ctrlr_fail(ctrlr, false);

might be even better. If all three things succeed, start the controller, otherwise fail it.

[ngie]

sys/dev/nvme/nvme_ctrlr.c

1129–1134

I like @imp's suggestion: it's a lot cleaner than mine.

[seuros]

address review concerns. Tested in hardware.

[imp]

Finally! This picky, grumpy driver maintainer is happy. Thanks for both your patience and your rolling my feedback to make this better.
I could get extra picky about slicing this up into smaller bits, but I think Adrian or I is good-to-go to commit it. If there's a backout,
we'll slice it more finely.

[seuros]

The code is much cleaner than my initial diff. Thank for the review.

Once this land, the next release of the installer will allow users to use internal disk instead.
The T2 driver will allow them to use the internal keyboard/mouse.

This will finalize the effort to support every Intel Apple Computer from 2007-2020 (all hardware tested).

[ngie]

This change causes my non-T2 MacBook Pro ('16 MBP) to panic at boot with "received completion for unknown cmd": https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=296946 .

[seuros]

This is T1 macbook. which is a model that was released in late 2016 to late2017 , then replaced with the T2 in 2018-2020

I think we are missing something to make it behave.

I'm going to get one to test it.

[ngie]

sys/dev/nvme/nvme_pci.c

96

This is no doubt where things went awry with my '16 MBP: they (Apple) probably used the same NVMe controller in the MBP, but the behavior of the controller differs in some key ways. I'm going to boot Linux and see what details I can get from my '16 (pre-T2) MBP and my '19 (T2) MBP.

[seuros]

Before we touch code, please boot to macos and let it update the firmware.

A Lot of time i found bugs in apple hardware, tougth it was quirk, then could not reproduce it once the firmware was updated. (just install the latest version possible, the update flow , update the EFI, SMC and firmwares ...

I bought a 2016 macbook, i will receive it this week. I suspect we need to handle T1.

PS: this quirk is needed for every 2018-2020 computer.

[ngie]

In D57087#1339149, @guest-seuros wrote:

Before we touch code, please boot to macos and let it update the firmware.

A Lot of time i found bugs in apple hardware, tougth it was quirk, then could not reproduce it once the firmware was updated. (just install the latest version possible, the update flow , update the EFI, SMC and firmwares ...

I bought a 2016 macbook, i will receive it this week. I suspect we need to handle T1.

PS: this quirk is needed for every 2018-2020 computer.

This MBP was running the latest firmware distributed with the latest supported version of macOS. AFAIK, there isn't a newer firmware version for the controller.

If you got an early '16 MacBook Pro but it's not the 13" (model is A1708 aka "MacBookPro13,1"), there's a good chance it's not the same as mine. I picked the model without the atrocious 1st generation Touchbar (the ones that lack the physical escape key).

[ngie]

sys/dev/nvme/nvme_pci.c

96

Here's what Linux has down for these controllers:

{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001),
        /*
         * Fix for the Apple controller found in the MacBook8,1 and
         * some MacBook7,1 to avoid controller resets and data loss.
         */
        .driver_data = NVME_QUIRK_SINGLE_VECTOR |
                        NVME_QUIRK_QDEPTH_ONE },
{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) },
{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2005),
        .driver_data = NVME_QUIRK_SINGLE_VECTOR |
                        NVME_QUIRK_128_BYTES_SQES |
                        NVME_QUIRK_SHARED_TAGS |
                        NVME_QUIRK_SKIP_CID_GEN |
                        NVME_QUIRK_IDENTIFY_CNS },

[ngie]

Ok, this is weird... This is actually a "Apple S3X NVMe Controller". I'll look at the logic carefully and see if something's awry here.

[ngie]

sys/dev/nvme/nvme_qpair.c

534–540

This block is what caused the panic on my MBP: hardcoding the value to 0 fixed the panic.

[imp]

sys/dev/nvme/nvme_qpair.c

534–540

Can you add a printf for ctrlr->cdata.sqes (as hex) please?

[ngie]

sys/dev/nvme/nvme_qpair.c

534–540

Can you add a printf for ctrlr->cdata.sqes (as hex) please?

I got 2 printouts (even though there's only 1 controller in the laptop):

• 0
• 0x76

[ngie]

sys/dev/nvme/nvme_qpair.c

534–540

Can you add a printf for ctrlr->cdata.sqes (as hex) please?

I got 2 printouts (even though there's only 1 controller in the laptop):

• 0
• 0x76

The function is called via nvme_ctrlr_construct_admin_qpair and nvme_ctrlr_construct_io_qpair; it's probably barfing on one of the qpair calls and not the other. Let's figure out which one.

[ngie]

sys/dev/nvme/nvme_qpair.c

534–540

Can you add a printf for ctrlr->cdata.sqes (as hex) please?

I got 2 printouts (even though there's only 1 controller in the laptop):

• 0
• 0x76

The function is called via nvme_ctrlr_construct_admin_qpair and nvme_ctrlr_construct_io_qpair; it's probably barfing on one of the qpair calls and not the other. Let's figure out which one.

The I/O qpair call is the one that's returning 0x76; the qpair ID is 1.

To my naive self, it seems kind of odd that cid_base is being adjusted based on adminq.num_trackers if it's the I/O qpair (just based on the name I would expect the ADMIN qpair call to adjust that value, not the I/O one..).

[ngie]

sys/dev/nvme/nvme_qpair.c

534–540

Can you add a printf for ctrlr->cdata.sqes (as hex) please?

I got 2 printouts (even though there's only 1 controller in the laptop):

• 0
• 0x76

The function is called via nvme_ctrlr_construct_admin_qpair and nvme_ctrlr_construct_io_qpair; it's probably barfing on one of the qpair calls and not the other. Let's figure out which one.

The I/O qpair call is the one that's returning 0x76; the qpair ID is 1.

To my naive self, it seems kind of odd that cid_base is being adjusted based on adminq.num_trackers if it's the I/O qpair (just based on the name I would expect the ADMIN qpair call to adjust that value, not the I/O one..).

I started skimming through the NVMe spec a bit [1]. I don't quite understand why this change is needed for shifting the cid_base yet, but I'm onboarding myself a bit with NVMe terminology/concepts.

If you could help me understand @guest-seuros , maybe I can programmer's duck a solution to the bug out with you.

1. https://nvmexpress.org/wp-content/uploads/NVM-Express-Base-Specification-Revision-2.3-2025.08.01-Ratified.pdf

[seuros]

sys/dev/nvme/nvme_qpair.c

534–540

The stopgap for the issue you reported is in: D58821.

The driver was deriving the I/O submission-queue-entry size at runtime by probing cdata.sqes from IDENTIFY CONTROLLER and that probe was giving the wrong value for S3X, because we still don't have MQs.

It worked before because we had defaults that match the T1 Drive.

Your drive could get double throughput once we implement the Multi Queue Support.