Message-ID: <20170726153519.GA4696@red-moon>
Date:   Wed, 26 Jul 2017 16:35:19 +0100
From:   Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:     Robin Murphy <robin.murphy@....com>
Cc:     Nate Watterson <nwatters@...eaurora.org>,
        linux-acpi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, Will Deacon <will.deacon@....com>,
        Hanjun Guo <hanjun.guo@...aro.org>, Feng Kan <fkan@....com>,
        Jon Masters <jcm@...hat.com>,
        Robert Moore <robert.moore@...el.com>,
        Zhang Rui <rui.zhang@...el.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH 0/4] ACPI: DMA ranges management

On Wed, Jul 26, 2017 at 04:05:55PM +0100, Robin Murphy wrote:
> Hi Nate,
> 
> On 26/07/17 15:46, Nate Watterson wrote:
> > Hi Lorenzo,
> > 
> > On 7/20/2017 10:45 AM, Lorenzo Pieralisi wrote:
> >> As reported in:
> >>
> >> http://lkml.kernel.org/r/CAL85gmA_SSCwM80TKdkZqEe+S1beWzDEvdki1kpkmUTDRmSP7g@mail.gmail.com
> >>
> >>
> >> the bus connecting devices to an IOMMU bus can be smaller in size than
> >> the IOMMU input address bits which results in devices DMA HW bugs in
> >> particular related to IOVA allocation (ie chopping of higher address
> >> bits owing to system bus HW capabilities mismatch with the IOMMU).
> >>
> >> Fortunately this problem can be solved through an already present but
> >> never
> >> used ACPI 6.2 firmware bindings (ie _DMA object) allowing to define
> >> the DMA
> >> window for a specific bus in ACPI and therefore all upstream devices
> >> connected to it.
> >>
> >> This small patch series enables _DMA parsing in ACPI core code and
> >> use it in ACPI IORT code in order to detect DMA ranges for devices and
> >> update their data structures to make them work with their related DMA
> >> addressing restrictions.
> > 
> > I tested the patches and unfortunately it seems like the DMA addressing
> > restrictions are not really enforced for devices that attempt to set
> > their own dma_mask based on controller capabilities. For instance,
> > consider the following from the ahci_platform driver:
> > 
> >     if (hpriv->cap & HOST_CAP_64) {
> >         rc = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
> >         [...]
> >     }
> > 
> > Prior to the check, I can see that the device dma_mask respects the
> > limits enumerated in the _DMA object, however it is then clobbered by
> > the call to dma_coerce_mask_and_coherent(). Interestingly, if
> > HOST_CAP_64 was not set and the _DMA object for the device (or its
> > parent) indicated support for > 32-bit addrs, the host controller
> > could end up getting programmed with addresses beyond what it actually
> > supports. That is more a bug with the ahci_platform driver assuming a
> > default 32-bit dma_mask, but I would not be surprised to find other
> > drivers that rely on the same assumption.
> 
> Yup, you've hit upon the more general problem, which applies equally to
> DT "dma-ranges" too. I'm working on arm64 DMA stuff at the moment, and
> have the patch to actually enforce the firmware-described limit when
> drivers update their masks, but that depends on everyone passing the
> correct information to arch_setup_dma_ops() in the first place (I think
> DT needs more fixing than ACPI does).
> 
> > To ensure that dma_set_mask() and friends actually respect _DMA, would
> > you consider introducing a dma_supported() callback to check the input
> > dma_mask against the FW defined limits? This would end up aggressively
> > clipping the dma_mask to 32-bits for devices like the above if the _DMA
> > limit was less than 64-bits, but that is probably preferable to the
> > controller accessing unintended addresses.
> > 
> > Also, how would you feel about adding support for the IORT named_node
> > memory_address_limit field?
> 
> We will certainly need that for some platform devices, so if you fancy
> giving it a go before Lorenzo or I get there, feel free!

I can do it for v2 but I would like to understand why using _DMA is
not good enough for named components - having two bindings describing
the same thing is not ideal and I'd rather avoid it - if there is
a reason I am happy to add the necessary code.

Thanks,
Lorenzo

> Robin.
> 
> > -Nate
> >>
> >> Cc: Will Deacon <will.deacon@....com>
> >> Cc: Hanjun Guo <hanjun.guo@...aro.org>
> >> Cc: Feng Kan <fkan@....com>
> >> Cc: Jon Masters <jcm@...hat.com>
> >> Cc: Robert Moore <robert.moore@...el.com>
> >> Cc: Robin Murphy <robin.murphy@....com>
> >> Cc: Zhang Rui <rui.zhang@...el.com>
> >> Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>
> >>
> >> Lorenzo Pieralisi (4):
> >>    ACPI: Allow _DMA method in walk resources
> >>    ACPI: Make acpi_dev_get_resources() method agnostic
> >>    ACPI: Introduce DMA ranges parsing
> >>    ACPI: Make acpi_dma_configure() DMA regions aware
> >>
> >>   drivers/acpi/acpica/rsxface.c |  7 ++--
> >>   drivers/acpi/arm64/iort.c     | 27 +++++++++++-
> >>   drivers/acpi/resource.c       | 83
> >> ++++++++++++++++++++++++++++---------
> >>   drivers/acpi/scan.c           | 95
> >> +++++++++++++++++++++++++++++++++++++++----
> >>   include/acpi/acnames.h        |  1 +
> >>   include/acpi/acpi_bus.h       |  2 +
> >>   include/linux/acpi.h          |  8 ++++
> >>   include/linux/acpi_iort.h     |  5 ++-
> >>   8 files changed, 194 insertions(+), 34 deletions(-)
> >>
> > 
> 

Powered by blists - more mailing lists