From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x231.google.com (mail-io0-x231.google.com [IPv6:2607:f8b0:4001:c06::231]) (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 61E842095B9D0 for ; Thu, 17 Aug 2017 05:43:32 -0700 (PDT) Received: by mail-io0-x231.google.com with SMTP id o9so22703364iod.1 for ; Thu, 17 Aug 2017 05:45:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=2iMDQKGVmdtIM5Il2n12QpxPgkbk/NEVOg9bqdLkuy4=; b=K0op6eokE4avHWFFRk2KRhxwD5LUNcrP43YpH1QSFOU1Q3qXboyaC1l5urbyUQIy+0 yAokxZRw6yLd+XTsF14/Pyxq4IG4px+S+elMmFno/arW61wO0CN0tMhpba/+sO7ekVlP wccczw1zF7gtAQWfhhBW8hLTWFUkCKUykEXIw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=2iMDQKGVmdtIM5Il2n12QpxPgkbk/NEVOg9bqdLkuy4=; b=dijwskYd8BLbR2CCOjI23XSF1EpeG/ck8kThayD/Qq1P3SVqu7iHlxuhxE+oHyo4cN Mh7D+R2OSyjHhfVwYVye6dyYDus5sNCM5gJzImJPm8UapV3DoKlRcZY1sMudTK0bH7Ef A7/K7UYtzq5efac2TPExM7Sh6mAoK9QAjHNUPcKWhOeMgMXHkdwZMQ996an04ljWf0wm UnRqTwVcD0x0pCIe0ulh48xsQLixElSTjqA0+uXKdZPYLFKJLzozM0FeddCTDvOCXEBE eo7DtFq5uPRSBvV1KmNOTh5UkuyVozb5xOkwcl63V2e9+B292bGET30u32334GOT+ar6 ds6g== X-Gm-Message-State: AHYfb5gNyOORgZuKvUjHoBp4x7w7JZnJvX2tq3Qk7XTcPeRuJfzuCPkn LSLvrx6pjsED292/nGRkJPiniuFkzG2a X-Received: by 10.107.175.167 with SMTP id p39mr4297219ioo.83.1502973958765; Thu, 17 Aug 2017 05:45:58 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.162.1 with HTTP; Thu, 17 Aug 2017 05:45:58 -0700 (PDT) In-Reply-To: <20170817124322.mlkus7e7fsuuns4g@bivouac.eciton.net> References: <20170817122546.17683-1-ard.biesheuvel@linaro.org> <20170817124322.mlkus7e7fsuuns4g@bivouac.eciton.net> From: Ard Biesheuvel Date: Thu, 17 Aug 2017 13:45:58 +0100 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" , Laszlo Ersek 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:43:32 -0000 Content-Type: text/plain; charset="UTF-8" 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.