[PATCH] nvme: prepare support for Apple NVMe controller

[PATCH] nvme: prepare support for Apple NVMe controller

[Stephan Günther]

The Apple NVMe controller found in the Macbook8,1 (12 inch retina model)
requires ordered split transfers even on 64bit systems to work.
Otherwise, readq() for instance returns all ones, which explains the
ridiculous page size of 128MiB reported before.

With this patch applied, the Apple NVMe controller can manually be bound
to he NVMe driver. Manual binding is still required since the controller
reports a PCI device class of 018002 (mass storage) instead of 010802
(nvme).


Signed-off-by: Stephan G?nther <guenther at tum.de>
Signed-off-by: Maurice Leclaire <leclaire at in.tum.de>
---

modprobe nvme
echo "106b 2001" > /sys/bus/pci/drivers/nvme/new_id

>From lspci after binding:

| 03:00.0 Mass storage controller: Apple Inc. Device 2001 (rev 01)
|   Subsystem: Apple Inc. Device 2001
|   (...)
|   Kernel driver in use: nvme

Partitions are correctly recognized and i/o operations seem to work so
far.

 drivers/block/nvme-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index ccc0c1f93daa..d6c0d2f1df03 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -43,6 +43,9 @@
 #include <scsi/sg.h>
 #include <asm-generic/io-64-nonatomic-lo-hi.h>
 
+#define readq lo_hi_readq
+#define writeq lo_hi_writeq
+
 #define NVME_MINORS		(1U << MINORBITS)
 #define NVME_Q_DEPTH		1024
 #define NVME_AQ_DEPTH		256

[PATCH] nvme: prepare support for Apple NVMe controller

[Jon Derrick]

> +#define readq lo_hi_readq
> +#define writeq lo_hi_writeq
> +

Good job figuring that one out. But this should be a quirk:
a) It will sacrifice some io cycles on other devices
b) It may get lost at some point in the name of performance

Christoph recently added a quirks mechanism where I think it would fit

[PATCH] nvme: prepare support for Apple NVMe controller

[Vedant Lath]

Hi Stephan,

Thank you! I am glad to see that Linux support for this SSD is workable.

I have a MacBookAir7,1 (11" Macbook Air (Early 2015)) which uses the
same (or similar) SSD (PCI ID 106b:2001). I am extremely interested to
test the patch on this laptop. Which kernel tree should I apply this
patch on? Can I apply it on stable (4.2.5)?

I had thought it might have been something related to initialising the
controller because lspci showed correctable errors (DevSta: CorrErr+)
on Linux but not on OS X. I also got discouraged because of the
vendor-specific PCI class (018002) instead of the nvme PCI class
(010802) which indicated a non-standard protocol. It's a nice feeling
to know it only requires a quirk to work.

On Thu, Oct 29, 2015@8:40 PM, Jon Derrick <jonathan.derrick@intel.com> wrote:
>> +#define readq lo_hi_readq
>> +#define writeq lo_hi_writeq
>> +
>
> Good job figuring that one out. But this should be a quirk:
> a) It will sacrifice some io cycles on other devices
> b) It may get lost at some point in the name of performance
>
> Christoph recently added a quirks mechanism where I think it would fit
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH] nvme: prepare support for Apple NVMe controller

[Stephan Günther]

On 2015/October/29 10:06, Vedant Lath wrote:
> Hi Stephan,
> 
> Thank you! I am glad to see that Linux support for this SSD is workable.
> 
> I have a MacBookAir7,1 (11" Macbook Air (Early 2015)) which uses the
> same (or similar) SSD (PCI ID 106b:2001). I am extremely interested to

You are right, the latest 11" MacBook Air presumably uses the same
controller.

> test the patch on this laptop. Which kernel tree should I apply this
> patch on? Can I apply it on stable (4.2.5)?

I tested against 4.3-rc7. However, since the patch consits of 2 lines it 
is very likely that it it also applies to older kernels.

> 
> I had thought it might have been something related to initialising the
> controller because lspci showed correctable errors (DevSta: CorrErr+)
> on Linux but not on OS X. I also got discouraged because of the
> vendor-specific PCI class (018002) instead of the nvme PCI class
> (010802) which indicated a non-standard protocol. It's a nice feeling
> to know it only requires a quirk to work.

Do not forget to bind it manually. And there is still a long way, at 
least for the MacBook8,1 as there is still no clue why the internal 
keyboard is not working...

> 
> On Thu, Oct 29, 2015@8:40 PM, Jon Derrick <jonathan.derrick@intel.com> wrote:
> >> +#define readq lo_hi_readq
> >> +#define writeq lo_hi_writeq
> >> +
> >
> > Good job figuring that one out. But this should be a quirk:
> > a) It will sacrifice some io cycles on other devices
> > b) It may get lost at some point in the name of performance
> >
> > Christoph recently added a quirks mechanism where I think it would fit
> >
> > _______________________________________________
> > Linux-nvme mailing list
> > Linux-nvme at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-nvme
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH] nvme: prepare support for Apple NVMe controller

[Vedant Lath]

On Thu, Oct 29, 2015@11:29 PM, Stephan G?nther <guenther@tum.de> wrote:
>
> On 2015/October/29 10:06, Vedant Lath wrote:
> > Hi Stephan,
> >
> > Thank you! I am glad to see that Linux support for this SSD is workable.
> >
> > I have a MacBookAir7,1 (11" Macbook Air (Early 2015)) which uses the
> > same (or similar) SSD (PCI ID 106b:2001). I am extremely interested to
>
> You are right, the latest 11" MacBook Air presumably uses the same
> controller.

To be more precise, the 11" MacBook Air with the 256 GB SSD uses the
same controller. The 128 GB models use AHCI. The iFixit teardown is of
the 128 GB model which has a Marvell AHCI controller in its SSD.

> > test the patch on this laptop. Which kernel tree should I apply this
> > patch on? Can I apply it on stable (4.2.5)?
>
> I tested against 4.3-rc7. However, since the patch consits of 2 lines it
> is very likely that it it also applies to older kernels.

I have tested it against 4.3-rc7 and it works. Thank you. :)

Most nvme-cli commands also work fine on it. I have posted some output
at http://pastebin.ubuntu.com/13049434/ .

However, nvme-cli show-regs is also affected by the same quirk and
reports some incorrect values from BAR0:
$ sudo ./nvme show-regs /dev/nvme0
cap     : ffffffffffffffff
version : 10001
intms   : 0
intmc   : 0
cc      : 460001
csts    : 1
nssr    : 0
aqa     : ff00ff
asq     : ffffffffffffffff
acq     : ffffffffffffffff
cmbloc  : 0
cmbsz   : 0

> >
> > I had thought it might have been something related to initialising the
> > controller because lspci showed correctable errors (DevSta: CorrErr+)
> > on Linux but not on OS X. I also got discouraged because of the
> > vendor-specific PCI class (018002) instead of the nvme PCI class
> > (010802) which indicated a non-standard protocol. It's a nice feeling
> > to know it only requires a quirk to work.
>
> Do not forget to bind it manually. And there is still a long way, at
> least for the MacBook8,1 as there is still no clue why the internal
> keyboard is not working...

[PATCH] nvme: prepare support for Apple NVMe controller

[Stephan Günther]

On 2015/October/29 09:10, Jon Derrick wrote:
> > +#define readq lo_hi_readq
> > +#define writeq lo_hi_writeq
> > +
> 
> Good job figuring that one out. But this should be a quirk:
> a) It will sacrifice some io cycles on other devices
> b) It may get lost at some point in the name of performance

Yep.

> 
> Christoph recently added a quirks mechanism where I think it would fit

To be honest we don't know what mechanism you are referring to. Please 
give us pointer.


I assume that we concur that this workaround--as well as making the nvme 
driver claim the Apple controller--should be a runtime-decisions based 
on the device id. That avoids both penalties in terms of cpu cycles and 
the tedious manual binding.

How to do that sanely is a bit unclear to me. Detection at runtime would 
require that

1) the nvme driver attempts to bind to non-nvme devices (pci class 
018002),

2) checks whether or not that is Apples special controller (106b 2001), 
and in that case

3) binds to the device while using ordered split writes on 64bit 
platforms, which probably requires a set of function pointers to 
low-level ops.


Another approach might be to write an intermediate driver that relies on 
the nvme driver but encapsulates the remaining stuff.


If Apple's controller is expected to be the only exception, the latter 
approach would make sense. But I don't believe that Apple builds the 
only custom nvme controller that is "almost" compatible. (assuming that 
018002 instead of 010802 isn't a typo anyways).


Best,
Stephan

[PATCH] nvme: prepare support for Apple NVMe controller

[Keith Busch]

On Thu, Oct 29, 2015@08:46:38PM +0100, Stephan G?nther wrote:
> On 2015/October/29 09:10, Jon Derrick wrote:
> > Christoph recently added a quirks mechanism where I think it would fit
> 
> To be honest we don't know what mechanism you are referring to. Please 
> give us pointer.

It's new, not merged in an official tree. A copy is on the mailing list
somewhere. I committed it to my personal tree here:

http://git.infradead.org/users/kbusch/linux-nvme.git/commitdiff/e43f1dc5d9f31380090cc6b67fd3fa43a15089e6
 
> I assume that we concur that this workaround--as well as making the nvme 
> driver claim the Apple controller--should be a runtime-decisions based 
> on the device id. That avoids both penalties in terms of cpu cycles and 
> the tedious manual binding.

Just append the Vendor + Device to struct pci_device_id nvme_id_table
if the 64 bit register access really is the only problem preventing it
from working in an NVMe compliant way. You can check at run time for
the lo_hi_[read/write]q quirk.

If it really works with only this one change to the nvme driver, I also
wonder Apple typo'ed their class code.

[PATCH] nvme: prepare support for Apple NVMe controller

[Stephan Günther]

On 2015/October/29 08:31, Keith Busch wrote:
> On Thu, Oct 29, 2015@08:46:38PM +0100, Stephan G?nther wrote:
> > On 2015/October/29 09:10, Jon Derrick wrote:
> > > Christoph recently added a quirks mechanism where I think it would fit
> > 
> > To be honest we don't know what mechanism you are referring to. Please 
> > give us pointer.
> 
> It's new, not merged in an official tree. A copy is on the mailing list
> somewhere. I committed it to my personal tree here:
> 
> http://git.infradead.org/users/kbusch/linux-nvme.git/commitdiff/e43f1dc5d9f31380090cc6b67fd3fa43a15089e6
>  
> > I assume that we concur that this workaround--as well as making the nvme 
> > driver claim the Apple controller--should be a runtime-decisions based 
> > on the device id. That avoids both penalties in terms of cpu cycles and 
> > the tedious manual binding.
> 
> Just append the Vendor + Device to struct pci_device_id nvme_id_table
> if the 64 bit register access really is the only problem preventing it
> from working in an NVMe compliant way. You can check at run time for
> the lo_hi_[read/write]q quirk.

Great, I will give this a try.

> 
> If it really works with only this one change to the nvme driver, I also
> wonder Apple typo'ed their class code.

The only thing that prevents me from wiping the ssd and debootstrapping 
is the problem with the internal keyboard. That's another story but I'm
looking into that.

Now that there is a way to get the controller working (read/write was ok
when I tested it on the Macbook's EFI partition), we will hopefully have
some answers soon.


Of course everyone should make some backups before trying that... I did 
not test anything else than writing a couple of text files to the
device, syncing and deleting the files afterwards.

Best,
Stephan

[PATCH] nvme: prepare support for Apple NVMe controller

[Christoph Hellwig]

On Thu, Oct 29, 2015@09:10:56AM -0600, Jon Derrick wrote:
> > +#define readq lo_hi_readq
> > +#define writeq lo_hi_writeq
> > +
> 
> Good job figuring that one out. But this should be a quirk:
> a) It will sacrifice some io cycles on other devices
> b) It may get lost at some point in the name of performance
> 
> Christoph recently added a quirks mechanism where I think it would fit

We never access 64 bit registers in the fast path.  I'd be fine to
apply the patch from Stephan as-is as long as we document in detail why
we are doing this.  Stephan, can you respin the patch with a comment
and also with the PCI ID for the Apple controller added to the ID table?

[PATCH] nvme: prepare support for Apple NVMe controller

[Stephan Günther]

On 2015/November/03 05:02, Christoph Hellwig wrote:
> On Thu, Oct 29, 2015@09:10:56AM -0600, Jon Derrick wrote:
> > > +#define readq lo_hi_readq
> > > +#define writeq lo_hi_writeq
> > > +
> > 
> > Good job figuring that one out. But this should be a quirk:
> > a) It will sacrifice some io cycles on other devices
> > b) It may get lost at some point in the name of performance
> > 
> > Christoph recently added a quirks mechanism where I think it would fit
> 
> We never access 64 bit registers in the fast path.  I'd be fine to
> apply the patch from Stephan as-is as long as we document in detail why
> we are doing this.  Stephan, can you respin the patch with a comment
> and also with the PCI ID for the Apple controller added to the ID table?

We are testing the new version of the patch and will submit it
afterwards.