Message-ID: <282f767e-565f-4a59-196f-411f26665c10@codeaurora.org>
Date:   Wed, 26 Jul 2017 12:39:58 -0400
From:   Nate Watterson <nwatters@...eaurora.org>
To:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Robin Murphy <robin.murphy@....com>
Cc:     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 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@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.

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@...eaurora.org>
> 
> 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(-)
>>>>
>>>
>>

-- 
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.

Powered by blists - more mailing lists