Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751869AbdGZQkO (ORCPT ); Wed, 26 Jul 2017 12:40:14 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:60846 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750898AbdGZQkM (ORCPT ); Wed, 26 Jul 2017 12:40:12 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org C4C296073E Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=nwatters@codeaurora.org Subject: Re: [PATCH 0/4] ACPI: DMA ranges management To: Lorenzo Pieralisi , Robin Murphy Cc: linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Will Deacon , Hanjun Guo , Feng Kan , Jon Masters , Robert Moore , Zhang Rui , "Rafael J. Wysocki" References: <20170720144517.32529-1-lorenzo.pieralisi@arm.com> <7df11172-5a40-685f-5202-9a0bceb6e19b@codeaurora.org> <20170726153519.GA4696@red-moon> From: Nate Watterson Message-ID: <282f767e-565f-4a59-196f-411f26665c10@codeaurora.org> Date: Wed, 26 Jul 2017 12:39:58 -0400 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170726153519.GA4696@red-moon> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5257 Lines: 124 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 > > Thanks, > Lorenzo > >> Robin. >> >>> -Nate >>>> >>>> Cc: Will Deacon >>>> Cc: Hanjun Guo >>>> Cc: Feng Kan >>>> Cc: Jon Masters >>>> Cc: Robert Moore >>>> Cc: Robin Murphy >>>> Cc: Zhang Rui >>>> Cc: "Rafael J. Wysocki" >>>> >>>> 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.