From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-x233.google.com (mail-wr0-x233.google.com [IPv6:2a00:1450:400c:c0c::233]) (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 7FEBF2095B9D0 for ; Thu, 17 Aug 2017 05:40:59 -0700 (PDT) Received: by mail-wr0-x233.google.com with SMTP id y96so39517589wrc.1 for ; Thu, 17 Aug 2017 05:43:26 -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=QaeJxujFGrsKRw2niA7t9+iTjLc1h5/DAZSO5vYcMfk=; b=OdEIsoD1RMIMVd0TGX2Fu3DpKePKrBqyXM9deQE4KNSMcoHEJ0P2HaEVZbj2uDdmOC yFGtMl7cZAiK5YSQgKwLpXPEk86ZNW97FYQ4FZyEjAywllN05oglRcHJmir1hAVo3D43 f2KA4qT158wh7tec0d/JdpLa4NyXfBoGuT2gE= 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=QaeJxujFGrsKRw2niA7t9+iTjLc1h5/DAZSO5vYcMfk=; b=mL1Ota0HJz7iRsbzmwgJ2zw3E+W5duDC2haV0oLqQfr+1rSDG9W5yvl7M3VFwKo6ni o1DwSEHJ7Fxa0uTlSrojiYbFbNQ2ddSISEACo/ImdMl+th5cxxCmCQoEs7T+yjhfZ3yg 0uexv0IUpJr3su08dAOzWuVhDrkf/NORw+Kb77MwhxzH5FY67/fWQ3eRJHDyqPJhDUqC Q5C32fV9kvWyl5/jxCtAGTXye55hzWiZ7iCpe7nZBNqEpM2I0YIPExVd/MubYeidqO99 GQGhYgxllHi141pOf7t9KWe2Qstzf2ZLbE4N03MuZxsbJEqfpx+CnVvzWfAImOg3cMHa neBg== X-Gm-Message-State: AHYfb5g2RnW4RuWeeRevIicIKheEXEK3RRhRUOB0eWma3HTjf9xNXQUX fzRNy5bFteRJT4qb X-Received: by 10.28.15.75 with SMTP id 72mr1082921wmp.177.1502973805081; Thu, 17 Aug 2017 05:43:25 -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 h33sm2312276wrh.23.2017.08.17.05.43.24 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 17 Aug 2017 05:43:24 -0700 (PDT) Date: Thu, 17 Aug 2017 13:43:22 +0100 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, lersek@redhat.com Message-ID: <20170817124322.mlkus7e7fsuuns4g@bivouac.eciton.net> References: <20170817122546.17683-1-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20170817122546.17683-1-ard.biesheuvel@linaro.org> 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:41:00 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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? Anyway, folding in your own commit message correction, and possibly one whitespace change: Reviewed-by: Leif Lindholm / Leif > } else { > Map->DoubleBuffer = FALSE; > } > @@ -207,6 +212,7 @@ DmaUnmap ( > { > MAP_INFO_INSTANCE *Map; > EFI_STATUS Status; > + VOID *Buffer; > > if (Mapping == NULL) { > ASSERT (FALSE); > @@ -217,17 +223,20 @@ DmaUnmap ( > > Status = EFI_SUCCESS; > if (Map->DoubleBuffer) { > - ASSERT (Map->Operation != MapOperationBusMasterCommonBuffer); > + ASSERT (Map->Operation == MapOperationBusMasterWrite); > > - if (Map->Operation == MapOperationBusMasterCommonBuffer) { > + if (Map->Operation != MapOperationBusMasterWrite) { > Status = EFI_INVALID_PARAMETER; > - } else if (Map->Operation == MapOperationBusMasterWrite) { > - CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress, > - Map->NumberOfBytes); > - } > + } else { > + Buffer = ALIGN_POINTER (Map->BufferAddress, mCpu->DmaBufferAlignment); > + > + mCpu->FlushDataCache (mCpu, (UINTN)Buffer, Map->NumberOfBytes, > + EfiCpuFlushTypeInvalidate); > > - DmaFreeBuffer (EFI_SIZE_TO_PAGES (Map->NumberOfBytes), Map->BufferAddress); > + CopyMem ((VOID *)(UINTN)Map->HostAddress, Buffer, Map->NumberOfBytes); > > + FreePool (Map->BufferAddress); > + } > } else { > if (Map->Operation == MapOperationBusMasterWrite) { > // > -- > 2.11.0 >