Message-ID: <CAAFQd5Cm+ZGx9ia2sAdvHQC6zC1U=+9AWs7iW7o-qE4g7wZgsw@mail.gmail.com>
Date:   Mon, 3 Aug 2020 17:15:20 +0200
From:   Tomasz Figa <tfiga@...omium.org>
To:     Claire Chang <tientzu@...omium.org>, Rob Herring <robh@...nel.org>
Cc:     Frank Rowand <frowand.list@...il.com>,
        Christoph Hellwig <hch@....de>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Robin Murphy <robin.murphy@....com>,
        Thierry Reding <treding@...dia.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Saravana Kannan <saravanak@...gle.com>, suzuki.poulose@....com,
        dan.j.williams@...el.com, heikki.krogerus@...ux.intel.com,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        linux-devicetree <devicetree@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        "list@....net:IOMMU DRIVERS <iommu@...ts.linux-foundation.org>, Joerg
        Roedel <joro@...tes.org>," <iommu@...ts.linux-foundation.org>,
        Nicolas Boichat <drinkcat@...omium.org>
Subject: Re: [RFC v2 4/5] dt-bindings: of: Add plumbing for restricted DMA pool

Hi Claire and Rob,

On Mon, Aug 3, 2020 at 4:26 PM Claire Chang <tientzu@...omium.org> wrote:
>
> On Sat, Aug 1, 2020 at 4:58 AM Rob Herring <robh@...nel.org> wrote:
> >
> > On Tue, Jul 28, 2020 at 01:01:39PM +0800, Claire Chang wrote:
> > > Introduce the new compatible string, device-swiotlb-pool, for restricted
> > > DMA. One can specify the address and length of the device swiotlb memory
> > > region by device-swiotlb-pool in the device tree.
> > >
> > > Signed-off-by: Claire Chang <tientzu@...omium.org>
> > > ---
> > >  .../reserved-memory/reserved-memory.txt       | 35 +++++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > index 4dd20de6977f..78850896e1d0 100644
> > > --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > @@ -51,6 +51,24 @@ compatible (optional) - standard definition
> > >            used as a shared pool of DMA buffers for a set of devices. It can
> > >            be used by an operating system to instantiate the necessary pool
> > >            management subsystem if necessary.
> > > +        - device-swiotlb-pool: This indicates a region of memory meant to be
> >
> > swiotlb is a Linux thing. The binding should be independent.
> Got it. Thanks for pointing this out.
>
> >
> > > +          used as a pool of device swiotlb buffers for a given device. When
> > > +          using this, the no-map and reusable properties must not be set, so the
> > > +          operating system can create a virtual mapping that will be used for
> > > +          synchronization. Also, there must be a restricted-dma property in the
> > > +          device node to specify the indexes of reserved-memory nodes. One can
> > > +          specify two reserved-memory nodes in the device tree. One with
> > > +          shared-dma-pool to handle the coherent DMA buffer allocation, and
> > > +          another one with device-swiotlb-pool for regular DMA to/from system
> > > +          memory, which would be subject to bouncing. The main purpose for
> > > +          restricted DMA is to mitigate the lack of DMA access control on
> > > +          systems without an IOMMU, which could result in the DMA accessing the
> > > +          system memory at unexpected times and/or unexpected addresses,
> > > +          possibly leading to data leakage or corruption. The feature on its own
> > > +          provides a basic level of protection against the DMA overwriting buffer
> > > +          contents at unexpected times. However, to protect against general data
> > > +          leakage and system memory corruption, the system needs to provide a
> > > +          way to restrict the DMA to a predefined memory region.
> >
> > I'm pretty sure we already support per device carveouts and I don't
> > understand how this is different.
> We use this to bounce streaming DMA in and out of a specially allocated region.
> I'll try to merge this with the existing one (i.e., shared-dma-pool)
> to see if that
> makes things clearer.
>

Indeed, from the firmware point of view, this is just a carveout, for
which we have the "shared-dma-pool" compatible string defined already.

However, depending on the device and firmware setup, the way the
carevout is used may change. I can see the following scenarios:

1) coherent DMA (dma_alloc_*) within a reserved pool and no
non-coherent DMA (dma_map_*).

This is how the "memory-region" property is handled today in Linux for
devices which can only DMA from/to the given memory region. However,
I'm not sure if no non-coherent DMA is actually enforced in any way by
the DMA subsystem.

2) coherent DMA from a reserved pool and non-coherent DMA from system memory

This is the case for the systems which have some dedicated part of
memory which is guaranteed to be coherent with the DMA, but still can
do non-coherent DMA to any part of the system memory. Linux handles it
the same way as 1), which is what made me believe that 1) might not
actually be handled correctly.

3) coherent DMA and bounced non-coherent DMA within a reserved pool
4) coherent DMA within one pool and bounced non-coherent within another pool

These are the two cases we're interested in. Basically they make it
possible for non-coherent DMA from arbitrary system memory to be
bounced through a reserved pool, which the device has access to. The
current series implements 4), but I'd argue that it:

- is problematic from the firmware point of view, because on most of
the systems, both pools would be just some carveouts and the fact that
Linux would use one for coherent and the other for non-coherent DMA
would be an OS implementation detail,
- suffers from the static memory split between coherent and
non-coherent DMA, which could either result in some wasted memory or
the DMA stopped working after a kernel update if the driver changes
its allocation pattern,

and so we should rather go with 3).

Now, from the firmware point of view, it doesn't matter how the OS
uses the carveout, but I think it's still necessary to tell the OS
about the device DMA capability. Right now we use "memory-region" for
any kind of reserved memory, but looking at the above scenarios, there
are 2 cases:

a) the memory region is preferred for the device, e.g. it enables
coherency, but the device can still DMA across the rest of the system
memory. This is the case in scenario 2) and is kind of assumed in the
Linux DMA subsystem, although it's certainly not the case for a lot of
hardware, even if they use the "memory-region" binding.

b) the memory region is the only region that the device can access.
This is the case in scenarios 1), 3) and 4).

For this, I'd like to propose a "restricted-dma-region" (feel free to
suggest a better name) binding, which is explicitly specified to be
the only DMA-able memory for this device and make Linux use the given
pool for coherent DMA allocations and bouncing non-coherent DMA.

What do you think?

Best regards,
Tomasz

> >
> > What is the last sentence supposed to imply? You need an IOMMU?
> The main purpose is to mitigate the lack of DMA access control on
> systems without an IOMMU.
> For example, we plan to use this plus a MPU for our PCIe WiFi which is
> not behind an IOMMU.
>
> >
> > >          - vendor specific string in the form <vendor>,[<device>-]<usage>
> > >  no-map (optional) - empty property
> > >      - Indicates the operating system must not create a virtual mapping
> > > @@ -117,6 +135,16 @@ one for multimedia processing (named multimedia-memory@...00000, 64MiB).
> > >                       compatible = "acme,multimedia-memory";
> > >                       reg = <0x77000000 0x4000000>;
> > >               };
> > > +
> > > +             wifi_coherent_mem_region: wifi_coherent_mem_region {
> > > +                     compatible = "shared-dma-pool";
> > > +                     reg = <0x50000000 0x400000>;
> > > +             };
> > > +
> > > +             wifi_device_swiotlb_region: wifi_device_swiotlb_region {
> > > +                     compatible = "device-swiotlb-pool";
> > > +                     reg = <0x50400000 0x4000000>;
> > > +             };
> > >       };
> > >
> > >       /* ... */
> > > @@ -135,4 +163,11 @@ one for multimedia processing (named multimedia-memory@...00000, 64MiB).
> > >               memory-region = <&multimedia_reserved>;
> > >               /* ... */
> > >       };
> > > +
> > > +     pcie_wifi: pcie_wifi@0,0 {
> > > +             memory-region = <&wifi_coherent_mem_region>,
> > > +                      <&wifi_device_swiotlb_region>;
> > > +             restricted-dma = <0>, <1>;
> > > +             /* ... */
> > > +     };
> > >  };
> > > --
> > > 2.28.0.rc0.142.g3c755180ce-goog
> > >

Powered by blists - more mailing lists