From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-x235.google.com (mail-wr0-x235.google.com [IPv6:2a00:1450:400c:c0c::235]) (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 C589621CFA5F7 for ; Thu, 17 Aug 2017 05:56:43 -0700 (PDT) Received: by mail-wr0-x235.google.com with SMTP id z91so33909859wrc.4 for ; Thu, 17 Aug 2017 05:59:10 -0700 (PDT) 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=HU5hWCwgxjXAmd++3rVnARpaa51Terv7HuWqDzeBHgM=; b=UKEtaT2ISPBrHzNELd9gxQhX8/qbnAIXNr/YyOsnvMjGqiJdvDQJCPyDIpAN5qOeJj VfmX+eoanmHc2zlqVaQ3RHhNkUHC+r5zLO8G13j/tmnSRY0vCXUu7posVcJ3qR6QdbJo iJz/+1hfTc9P13gge8SxhAgpk4KZftJYv6tIs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=HU5hWCwgxjXAmd++3rVnARpaa51Terv7HuWqDzeBHgM=; b=DVM5o1ClnAB2Qg2fP6fkWySrZjHTxY32zub7WtjFmUQfzPLJ0OBHIM6JPvBu2CvkSX N9kQeugKRtAl41Gi0SCiyvCsh2eyu/nzT3smWOa+ZuAnZyczfw2d+Io2fHOcqrSroYsT ugieXknhnKG/dmAC/JHVq9DaOFR1cDLxGL6g2Rxddnq4Y0DaYrkXk9RXlg/QJOyirs38 UInzFsyc2WUkJCwp85mPffTUkgUBUmX/YVFeefqeAxseiI/MqWa3qjZVBtDLrBEahB7f DS0ToQdMCbNNxLhYT72+ptpqeIE/0Yk3ducnK48jtq0tPAKaQa+uKMcAQjwiv+16WCZz 3otA== X-Gm-Message-State: AHYfb5h7MK6amXlfJwuansh+PuenQA0EJrsLFmPCJMS7rOIc8TDJIxbC +aOYcnT9ITR6I1rq X-Received: by 10.28.111.89 with SMTP id k86mr1147212wmc.139.1502974749431; Thu, 17 Aug 2017 05:59:09 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id 9sm2508033wrl.57.2017.08.17.05.59.08 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 17 Aug 2017 05:59:08 -0700 (PDT) Date: Thu, 17 Aug 2017 13:59:06 +0100 From: Leif Lindholm To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , Laszlo Ersek Message-ID: <20170817125906.faoqwuhr4576ytuc@bivouac.eciton.net> References: <20170817122546.17683-1-ard.biesheuvel@linaro.org> <20170817124322.mlkus7e7fsuuns4g@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH] ArmPkg/ArmDmaLib: use double buffering only for bus master write X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Aug 2017 12:56:44 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Aug 17, 2017 at 01:45:58PM +0100, Ard Biesheuvel wrote: > On 17 August 2017 at 13:43, Leif Lindholm wrote: > > On Thu, Aug 17, 2017 at 01:25:46PM +0100, Ard Biesheuvel wrote: > >> The ArmPkg implementation of DmaLib uses double buffering to ensure > >> that any attempt to perform non-coherent DMA on unaligned buffers cannot > >> corrupt adjacent unrelated data which happens to share cachelines with > >> the data we are exchanging with the device. > >> > >> Such corruption can only occur on bus master read, in which case we have > >> to invalidate the caches to ensure the CPU will see the data written to > >> memory by the device. In the bus master write case, we can simply clean > >> and invalidate at the same time, which may purge unrelated adjacent data > >> from the caches, but will not corrupt its contents. > >> > >> Also, this double buffer does not necessarily have to be allocated from > >> uncached memory: by the same reasoning, we can perform cache invalidation > >> on an ordinary pool allocation as long as we take the same alignment > >> constraints into account. > >> > >> So update our code accordingly: remove double buffering from the bus > >> master read path, and switch to a pool allocation for the double buffer. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> Signed-off-by: Ard Biesheuvel > >> --- > >> ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 47 ++++++++++++-------- > >> 1 file changed, 28 insertions(+), 19 deletions(-) > >> > >> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > >> index f4ee9e4c5ea2..61d70614bff0 100644 > >> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > >> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > >> @@ -80,6 +80,7 @@ DmaMap ( > >> MAP_INFO_INSTANCE *Map; > >> VOID *Buffer; > >> EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor; > >> + UINTN AllocSize; > >> > >> if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL || Mapping == NULL ) { > >> return EFI_INVALID_PARAMETER; > >> @@ -104,8 +105,9 @@ DmaMap ( > >> return EFI_OUT_OF_RESOURCES; > >> } > >> > >> - if ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) || > >> - ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0)) { > >> + if (Operation != MapOperationBusMasterRead && > >> + ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) || > >> + ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) { > >> > >> // Get the cacheability of the region > >> Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor); > >> @@ -129,21 +131,24 @@ DmaMap ( > >> } > >> > >> // > >> - // If the buffer does not fill entire cache lines we must double buffer into > >> - // uncached memory. Device (PCI) address becomes uncached page. > >> + // If the buffer does not fill entire cache lines we must double buffer > >> + // into a suitably aligned allocation that allows us to invalidate the > >> + // cache without running the risk of corrupting adjacent unrelated data. > >> + // Note that pool allocations are guaranteed to be 8 byte aligned, so > >> + // we only have to add (alignment - 8) worth of padding. > >> // > >> - Map->DoubleBuffer = TRUE; > >> - Status = DmaAllocateBuffer (EfiBootServicesData, EFI_SIZE_TO_PAGES (*NumberOfBytes), &Buffer); > >> - if (EFI_ERROR (Status)) { > >> + Map->DoubleBuffer = TRUE; > >> + AllocSize = ALIGN_VALUE (*NumberOfBytes, mCpu->DmaBufferAlignment) + > >> + (mCpu->DmaBufferAlignment - 8); > >> + Map->BufferAddress = AllocatePool (AllocSize); > >> + if (Map->BufferAddress == NULL) { > >> + Status = EFI_OUT_OF_RESOURCES; > >> goto FreeMapInfo; > >> } > >> > >> - if (Operation == MapOperationBusMasterRead) { > >> - CopyMem (Buffer, HostAddress, *NumberOfBytes); > >> - } > >> - > >> + Buffer = ALIGN_POINTER (Map->BufferAddress, mCpu->DmaBufferAlignment); > >> *DeviceAddress = HostToDeviceAddress (ConvertToPhysicalAddress (Buffer)); > >> - Map->BufferAddress = Buffer; > >> + > > > > Nothing wrong with a blank line here, but I don't seem to recall you > > doing that in general. Accidental? > > > > Ehm, no ... > > Actually, what I intended to put there was > > // > // Get rid of any dirty cachelines covering the double buffer. This > // prevents them from being written back unexpectedly, potentially > // overwriting the data we receive from the device. > // > mCpu->FlushDataCache (mCpu, (UINTN)Buffer, *NumberOfBytes, > EfiCpuFlushTypeWriteBack); > > but I put a blank line instead :-) > > > Anyway, folding in your own commit message correction, and possibly > > one whitespace change: > > Reviewed-by: Leif Lindholm > > > > Thanks. Please confirm that this covers the addition above. Haha, yes, it does. / Leif