Re: [PATCH 0/4] ACPI: DMA ranges management

[Nate Watterson]

On 7/26/2017 11:35 AM, Lorenzo Pieralisi wrote:
> 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@xxxxxxxxxxxxxx
> > > >
> > > >
> > > > 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.

My primary reason for requesting this is that I had already configured
the memory_address_limit field for the address challenged platform
devices in our (QDF2400) IORT under the assumption it would eventually
be supported.

Tested-by: Nate Watterson <nwatters@xxxxxxxxxxxxxx>
> Thanks,
> Lorenzo
>
> > Robin.
> >
> > > -Nate
> > > > Cc: Will Deacon <will.deacon@xxxxxxx>
> > > > Cc: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
> > > > Cc: Feng Kan <fkan@xxxxxxx>
> > > > Cc: Jon Masters <jcm@xxxxxxxxxx>
> > > > Cc: Robert Moore <robert.moore@xxxxxxxxx>
> > > > Cc: Robin Murphy <robin.murphy@xxxxxxx>
> > > > Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
> > > > Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
> > > >
> > > > 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(-)

--
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.