From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yb0-x22a.google.com (mail-yb0-x22a.google.com [IPv6:2607:f8b0:4002:c09::22a]) (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 2F6B181D36 for ; Wed, 2 Nov 2016 09:17:03 -0700 (PDT) Received: by mail-yb0-x22a.google.com with SMTP id o7so6607940ybb.0 for ; Wed, 02 Nov 2016 09:17:04 -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=k/Ju/hekCVKUELJ6E6WzQ6OruGwZL1TNkaK3SNTcj8w=; b=P2CNBcpI8ZeDzFxZVA02m21qaJn2OWJ4oAzaPIOw4b49KO44wopagGj/qCk60R/rxw V2sKB/X/hjQOii96p06akpU4f/QpLdeRQu+poRdq1sRoNAfjrpMKYAbsYn+7YpF7tWw7 NwHWDrAKPl7AoEW5Sbs+KzCeL9WBlIUUyMb0M= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=k/Ju/hekCVKUELJ6E6WzQ6OruGwZL1TNkaK3SNTcj8w=; b=mTjqprpIierdkSHf7LrtCENtoF0VSKJmnMMr0i3fV9d1tr/ze8hpDKAvOQrOtS9L7z uXf0QjsArrtbwUF6mn7etnpIYhBmbBHFiAkBnfC7+pi3etGTk6v10ZJlTqJiXuxJb+JQ SMg+7vxcIpHnCqzMQoRjaJ6wzw4ztYu929LbHsUD4J7MbA2MR2XZ7wensegedJKdnqOs 067l5HHBqA/to6nNfdEqzGkC7No208AHAQukv9Rf5sAnrv74mNDGXEQdQJ1YPojXItrY vlHX/3iceiLZ1Byg5m3ZNM8mtSuhcQKsV4Ar19KWExCvH+g0RX07YsPWiedPeSJjltZT Cilg== X-Gm-Message-State: ABUngvcRyphOQOXhkVUzhOTdqexxbGJ4SDsxR0IbXQt4rEvWQv+pPn39fY6FFbWhcHDYI7MaOvw64JHd7+G15lAR X-Received: by 10.36.80.67 with SMTP id m64mr3221162itb.59.1478103423829; Wed, 02 Nov 2016 09:17:03 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.59.147 with HTTP; Wed, 2 Nov 2016 09:17:03 -0700 (PDT) In-Reply-To: <20161102161040.GD27644@bivouac.eciton.net> References: <1477937590-10361-1-git-send-email-ard.biesheuvel@linaro.org> <1477937590-10361-5-git-send-email-ard.biesheuvel@linaro.org> <20161101223227.GP1161@bivouac.eciton.net> <20161102161040.GD27644@bivouac.eciton.net> From: Ard Biesheuvel Date: Wed, 2 Nov 2016 16:17:03 +0000 Message-ID: To: Leif Lindholm Cc: edk2-devel-01 Subject: Re: [PATCH 4/5] ArmPkg/CpuDxe: set DmaBufferAlignment according to CWG 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: Wed, 02 Nov 2016 16:17:03 -0000 Content-Type: text/plain; charset=UTF-8 On 2 November 2016 at 16:10, Leif Lindholm wrote: > On Wed, Nov 02, 2016 at 01:40:17PM +0000, Ard Biesheuvel wrote: >> On 1 November 2016 at 22:32, Leif Lindholm wrote: >> > On Mon, Oct 31, 2016 at 06:13:09PM +0000, Ard Biesheuvel wrote: >> >> The DmaBufferAlignment currently defaults to 4, which is dangerously >> >> small and may result in lost data on platform that perform non-coherent >> >> DMA. So instead, take the CWG value from the cache info registers. >> >> >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> >> Signed-off-by: Ard Biesheuvel >> >> --- >> >> ArmPkg/Drivers/CpuDxe/CpuDxe.c | 4 +++- >> >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c >> >> index d089cb2d119f..ddc64fd255a0 100644 >> >> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c >> >> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c >> >> @@ -225,7 +225,7 @@ EFI_CPU_ARCH_PROTOCOL mCpu = { >> >> CpuGetTimerValue, >> >> CpuSetMemoryAttributes, >> >> 0, // NumberOfTimers >> >> - 4, // DmaBufferAlignment >> >> + 2048, // DmaBufferAlignment >> >> }; >> >> >> >> EFI_STATUS >> >> @@ -239,6 +239,8 @@ CpuDxeInitialize ( >> >> >> >> InitializeExceptions (&mCpu); >> >> >> >> + mCpu.DmaBufferAlignment = ArmCacheWritebackGranule (); >> >> + >> > >> > Could we hide the internal structure of mCpu here by moving this to a >> > helper function and calling >> > InitializeDma (&mCpu); >> > (or something)? >> > >> >> We could, but why? The actual struct is defined 10 lines up > > It's just that it's the only place in this function we're prodding the > internals of the object directly. Messes slightly with my zen. > Not a strong opinion, just a preference. > Sure, if you want. In fact, I will break this out as a separate patch, considering that it fixes a serious bug. So you are happy with the patch if I fold the following into it? """ --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c @@ -228,6 +228,15 @@ EFI_CPU_ARCH_PROTOCOL mCpu = { 2048, // DmaBufferAlignment }; +STATIC +VOID +InitializeDma ( + IN OUT EFI_CPU_ARCH_PROTOCOL *CpuArchProtocol + ) +{ + CpuArchProtocol->DmaBufferAlignment = ArmCacheWritebackGranule (); +} + EFI_STATUS CpuDxeInitialize ( IN EFI_HANDLE ImageHandle, @@ -239,7 +248,7 @@ CpuDxeInitialize ( InitializeExceptions (&mCpu); - mCpu.DmaBufferAlignment = ArmCacheWritebackGranule (); + InitializeDma (&mCpu); Status = gBS->InstallMultipleProtocolInterfaces ( &mCpuHandle, """