From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x22b.google.com (mail-wm0-x22b.google.com [IPv6:2a00:1450:400c:c09::22b]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A29DF81E0A for ; Mon, 14 Nov 2016 07:16:13 -0800 (PST) Received: by mail-wm0-x22b.google.com with SMTP id t79so103980137wmt.0 for ; Mon, 14 Nov 2016 07:16:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=T2XQH9DxapbeR8gGF6SxRN4LMCZhLpdK6RCWKYnFZfg=; b=kjvlMysEbHn+90n5GIn9pTJzNksmW9ExX89ahK2dUZBINUtOVQVWa5gXwFroNtT84x zSnBizJYGjo+wbRSFxPbcuxFjCbeekA9QR99gU2lwzL10hVCgnj+UB9OACm0WonrOB99 qSIIIDZ95k71YGPM0uof+vs6GqkYeRfHeg728= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=T2XQH9DxapbeR8gGF6SxRN4LMCZhLpdK6RCWKYnFZfg=; b=H1JJpKFup7W/ao3slI/93vWDo/FnvYoSp25G0D1IMkDMivVObS67hi2eF0v6xMsL+p J8/GIg0n7PYWJ1kn5Y/IjL7k9lmIIOMAiSXvZ6qKKwgTAnb8HJLAm5ihzV2ahUrqQIue O7Y+mB6XJFNsEcHqf0uBB8f6mSw0+4NCHnCtYf/z3W3f7/VxVViskKbK18GxmNA4imxC 1FC9xGmovktyL1tZwOdYpBiiaInhHUBVRScTbWYstqTM5PvWLuqaEdWWbQuKvfmMUGor JEciRZ+z/DRu5totqXSO7yxIVK1uVP+FnSLziauDygzxhyr6MXKnT43ETvxz3nB2Qm0V aKuw== X-Gm-Message-State: ABUngvfSNyQ5UZLlCpa04XPspMtm3FS8d5zMd4QbKaQ5EMLBPtdK4ohF+qGcgFhefQke5vXL X-Received: by 10.28.17.205 with SMTP id 196mr10634442wmr.78.1479136576861; Mon, 14 Nov 2016 07:16:16 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id v2sm29244350wja.41.2016.11.14.07.16.16 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 14 Nov 2016 07:16:16 -0800 (PST) Date: Mon, 14 Nov 2016 15:16:14 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org Message-ID: <20161114151614.GR27644@bivouac.eciton.net> References: <1478955748-14819-1-git-send-email-ard.biesheuvel@linaro.org> <1478955748-14819-4-git-send-email-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <1478955748-14819-4-git-send-email-ard.biesheuvel@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v2 3/4] ArmPkg/ArmDmaLib: clean up abuse of device address X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Nov 2016 15:16:14 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sat, Nov 12, 2016 at 02:02:27PM +0100, Ard Biesheuvel wrote: > In preparation of adding support to ArmDmalib for DMA bus masters whose > view of memory is offset by a constant compared to the CPU's view, clean > up some abuse of the device address. > > The device address is not defined in terms of the CPU's address space, > and so it should not be used in CopyMem () or cache maintenance operations > that require a valid mapping. This not only applies to the above use case, > but also to the DebugUncachedMemoryAllocationLib that unmaps the > primary, cached mapping of an allocation, and returns a host address > which is an uncached alias offset by a constant. > > Since we should never access the device address from the CPU, there is > no need to record it in the MAPINFO struct. Instead, record the buffer > address in case of double buffering, since we do need to copy the contents > (in case of a bus master write) and free the buffer (in all cases) when > DmaUnmap() is called. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel For the fix itself: Reviewed-by: Leif Lindholm However, can we wait for a few Tested-by:s to ensure this fix does not reveal any companion bugs? > --- > ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > index c2a44398d25a..7321388de63e 100644 > --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > @@ -27,7 +27,7 @@ > > typedef struct { > EFI_PHYSICAL_ADDRESS HostAddress; > - EFI_PHYSICAL_ADDRESS DeviceAddress; > + VOID *BufferAddress; > UINTN NumberOfBytes; > DMA_MAP_OPERATION Operation; > BOOLEAN DoubleBuffer; > @@ -94,7 +94,7 @@ DmaMap ( > ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0)) { > > // Get the cacheability of the region > - Status = gDS->GetMemorySpaceDescriptor (*DeviceAddress, &GcdDescriptor); > + Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor); > if (EFI_ERROR(Status)) { > return Status; > } > @@ -128,6 +128,7 @@ DmaMap ( > } > > *DeviceAddress = ConvertToPhysicalAddress ((UINTN)Buffer); > + Map->BufferAddress = Buffer; > } else { > Map->DoubleBuffer = FALSE; > } > @@ -143,7 +144,7 @@ DmaMap ( > // So duplicate the check here when running in DEBUG mode, just to assert > // that we are not trying to create a consistent mapping for cached memory. > // > - Status = gDS->GetMemorySpaceDescriptor (*DeviceAddress, &GcdDescriptor); > + Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor); > ASSERT_EFI_ERROR(Status); > > ASSERT (Operation != MapOperationBusMasterCommonBuffer || > @@ -152,12 +153,11 @@ DmaMap ( > DEBUG_CODE_END (); > > // Flush the Data Cache (should not have any effect if the memory region is uncached) > - mCpu->FlushDataCache (mCpu, *DeviceAddress, *NumberOfBytes, > + mCpu->FlushDataCache (mCpu, (UINTN)HostAddress, *NumberOfBytes, > EfiCpuFlushTypeWriteBackInvalidate); > } > > Map->HostAddress = (UINTN)HostAddress; > - Map->DeviceAddress = *DeviceAddress; > Map->NumberOfBytes = *NumberOfBytes; > Map->Operation = Operation; > > @@ -200,10 +200,11 @@ DmaUnmap ( > if (Map->Operation == MapOperationBusMasterCommonBuffer) { > Status = EFI_INVALID_PARAMETER; > } else if (Map->Operation == MapOperationBusMasterWrite) { > - CopyMem ((VOID *)(UINTN)Map->HostAddress, (VOID *)(UINTN)Map->DeviceAddress, Map->NumberOfBytes); > + CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress, > + Map->NumberOfBytes); > } > > - DmaFreeBuffer (EFI_SIZE_TO_PAGES (Map->NumberOfBytes), (VOID *)(UINTN)Map->DeviceAddress); > + DmaFreeBuffer (EFI_SIZE_TO_PAGES (Map->NumberOfBytes), Map->BufferAddress); > > } else { > if (Map->Operation == MapOperationBusMasterWrite) { > -- > 2.7.4 >