Re: [asahilinux:nvme/dev 13/17] drivers/nvme/host/pci.c:2249:2-3: Unneeded semicolon

Re: [asahilinux:nvme/dev 13/17] drivers/nvme/host/pci.c:2249:2-3: Unneeded semicolon

[Arnd Bergmann]

 + On Sat, Aug 7, 2021 at 7:43 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> FYI, I think the approach to mess up the nvme-pci driver in this
> branch is completely unacceptable for upstream.
>
> Please create an entirely separate driver plugging into nvme_ctrl_ops.

Hi Christoph,

I wrote the initial version of the series with the intention for testing
out how much of the code can be shared between the pci_driver
and the platform_driver version.

I tried to avoid moving around or duplicating code for this, to make
it easier to rebase until we have decided what the final code should
look like. It would be fairly easy to take the version that sven is
testing, and then remove all the PCI bits from that driver to get
to a separate nvme_ctrl_ops implementation, but that does mean
a great deal of duplication. I initially planned to send my patches
as an RFC to the list, but I never managed to get it working before
I lost access to the machine I had. Sven got it working fine with a
little rework then.

Out of the 3557 lines for the combined pci/platform driver, I ended
up with 475 lines that are specific to the PCI version, and 104
lines that are specific to the platform driver. There are probably
a few more bits that could be removed from a platform-only version,
but I still expect the majority of the code to be the same, which is why
I was hoping we could rearrange the code in a way that puts those
into a library module, either as part of the existing nvme-core.ko,
or a new nvme-mmio.ko that is used by both pci and platform
front-ends.

Simply moving the common functions into a separate file, and
exporting the symbols would work, but probably won't lead to
the best structure without some more rework.

Do you have any further suggestions? Should we just duplicate
the driver and then simplify the platform version as much as possible
as you suggested above, or try to share some of the code according
to my original plan?

      Arnd

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [asahilinux:nvme/dev 13/17] drivers/nvme/host/pci.c:2249:2-3: Unneeded semicolon

[Christoph Hellwig]

On Mon, Aug 09, 2021 at 12:02:55PM +0200, Arnd Bergmann wrote:
> Do you have any further suggestions? Should we just duplicate
> the driver and then simplify the platform version as much as possible
> as you suggested above, or try to share some of the code according
> to my original plan?

Yes, please duplicate it.  The tradeoff between absolute performance
requirements and various bits of broken consumer hardware already
is hard enough as-is that we don't need to support apples frankenchip
in the same codebase.

Note that I suspect you can probably drop a whole lot of the duplicate
code for the apple driver.  I doubt it supports things like CMB, HMB
T10 protection information, shadow doorbells, SGLs, etc.

Also given that it only supports a single I/O queue there is no need
to support read queues or poll queues or any kind of queue mapping.

Also can one of you look how PRPs are actually used by MacOS?  Given
that this device always seems to be behind a IOMMU creating one entry
per page seems rather weird given that the apple_nvmmu_tcb structure
already contains the full length.  Maybe it actually ignores all but
the first PRP?

So yes, duplicating the driver will probably look a little worse
from a pure LOC, but it will be much easier to maintain and to
understand.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [asahilinux:nvme/dev 13/17] drivers/nvme/host/pci.c:2249:2-3: Unneeded semicolon

[Arnd Bergmann]

On Mon, Aug 9, 2021 at 4:29 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Aug 09, 2021 at 12:02:55PM +0200, Arnd Bergmann wrote:
> > Do you have any further suggestions? Should we just duplicate
> > the driver and then simplify the platform version as much as possible
> > as you suggested above, or try to share some of the code according
> > to my original plan?
>
> Yes, please duplicate it.  The tradeoff between absolute performance
> requirements and various bits of broken consumer hardware already
> is hard enough as-is that we don't need to support apples frankenchip
> in the same codebase.
>
> Note that I suspect you can probably drop a whole lot of the duplicate
> code for the apple driver.  I doubt it supports things like CMB, HMB
> T10 protection information, shadow doorbells, SGLs, etc.

Ok, fair enough. That will probably also help adding some of the
special bits that are still not implemented. I was originally thinking
that this driver could be used with other SoCs that want to support
NVMe without having a full PCIe host, but it is quite likely that nobody
else other than Apple will ever do it this way.

> Also given that it only supports a single I/O queue there is no need
> to support read queues or poll queues or any kind of queue mapping.

This one seems possible to get added in the future. Also the HMB
support, since Apple currently has their own non-standard way
of doing the same thing from their firmware, using a boot-time memory
carve-out. But since the code is already there for the PCIe version,
I suppose that could both be added back in the future.

> Also can one of you look how PRPs are actually used by MacOS?  Given
> that this device always seems to be behind a IOMMU creating one entry
> per page seems rather weird given that the apple_nvmmu_tcb structure
> already contains the full length.  Maybe it actually ignores all but
> the first PRP?

I'll leave this up to Sven to answer. He also wrote the iommu driver,
so he probably has a good idea of what is going on here already.

      Arnd

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [asahilinux:nvme/dev 13/17] drivers/nvme/host/pci.c:2249:2-3: Unneeded semicolon

[Sven Peter]

On Mon, Aug 9, 2021, at 17:53, Arnd Bergmann wrote:
> On Mon, Aug 9, 2021 at 4:29 PM Christoph Hellwig <hch@infradead.org> wrote:
> > Also can one of you look how PRPs are actually used by MacOS?  Given
> > that this device always seems to be behind a IOMMU creating one entry
> > per page seems rather weird given that the apple_nvmmu_tcb structure
> > already contains the full length.  Maybe it actually ignores all but
> > the first PRP?
> 
> I'll leave this up to Sven to answer. He also wrote the iommu driver,
> so he probably has a good idea of what is going on here already.
> 
>       Arnd
> 

Not yet, but figuring out how this NVMe-IOMMU works in detail was
already on my TODO list :-)

Some background - the M1 has at least four different IOMMU-like
HW blocks:
DART (for which I wrote a driver and where I'd actually know what's going
on in detail), SART (simple DMA address filter required by the NVMe
co-processor for non-nvme transactions), this weird NVMe IOMMU (that
also seems to be somehow related to disk encryption) and GART for their GPU.



Sven

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [asahilinux:nvme/dev 13/17] drivers/nvme/host/pci.c:2249:2-3: Unneeded semicolon

[Sven Peter]

Alright, I've observed what macOS does by using the simple hypervisor we
built and tracing its MMIO access. There actually is a single entry per page
in both the TCB struct and the command queue and all entries are used.

The reason for that needs a little more background around XNU and it's
security architecture works:

On these machines, XNU is split into two parts: The main kernel with all its
extensions and hardware drivers, and a small section called "Page Protection Layer"
or PPL.
These machines have CPU extensions that allow them to prevent the normal kernel
from writing to pagetables or accessing any MMIO related to IOMMUs. Switching to
the PPL section is done with a custom instruction ("genter") which changes memory
permissions, such that PPL is then able to modify pagetables and configures the 
IOMMUs. It kinda works like a low-overhead hypervisor that controls pagetables.
There are some writeups available about this if you are curious about
the details [1][2][3].

The TCB structs and the NVMMU MMIO registers cannot be accessed by the part of the
kernel that contains the NVME driver. The NVME driver can only prepare the
command structure and fill the PRP list with entries for the DMA buffers.
It then calls out to PPL, which verifies all the pages listed inside the PRP
are allowed for DMA and then constructs the same structure again inside
protected memory that can no longer be touched by the regular kernel.

Then the NVMMU is configured with this secondary PRP list from inside PPL
before it returns back control to the NVME driver. Effectively,
this prevents someone from breaking into the normal kernel to just DMA
over any buffer they want.

For Linux we can ignore all this and just point the NVMMU and the
queue entry to the same PRP list.



Sven


[1] Jonathan Levin's writeup about the Page Protection Layer http://newosxbook.com/articles/CasaDePPL.html
[2] siguza's writeup about how this was done on iOS https://siguza.github.io/APRR/
[3] my writeup about the CPU extensions https://blog.svenpeter.dev/posts/m1_sprr_gxf/


On Mon, Aug 9, 2021, at 22:11, Sven Peter wrote:
> 
> 
> On Mon, Aug 9, 2021, at 17:53, Arnd Bergmann wrote:
> > On Mon, Aug 9, 2021 at 4:29 PM Christoph Hellwig <hch@infradead.org> wrote:
> > > Also can one of you look how PRPs are actually used by MacOS?  Given
> > > that this device always seems to be behind a IOMMU creating one entry
> > > per page seems rather weird given that the apple_nvmmu_tcb structure
> > > already contains the full length.  Maybe it actually ignores all but
> > > the first PRP?
> > 
> > I'll leave this up to Sven to answer. He also wrote the iommu driver,
> > so he probably has a good idea of what is going on here already.
> > 
> >       Arnd
> > 
> 
> Not yet, but figuring out how this NVMe-IOMMU works in detail was
> already on my TODO list :-)
> 
> Some background - the M1 has at least four different IOMMU-like
> HW blocks:
> DART (for which I wrote a driver and where I'd actually know what's going
> on in detail), SART (simple DMA address filter required by the NVMe
> co-processor for non-nvme transactions), this weird NVMe IOMMU (that
> also seems to be somehow related to disk encryption) and GART for their GPU.
> 
> 
> 
> Sven
> 


-- 
Sven Peter

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme