nvme: add Apple T2 ANS2 NVMe quirks

[guest-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?

[guest-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.

[guest-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.

[guest-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.

[guest-seuros]

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

[guest-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

[guest-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.

[guest-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.

[guest-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).