From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi0-x230.google.com (mail-oi0-x230.google.com [IPv6:2607:f8b0:4003:c06::230]) (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 87C0281D39 for ; Wed, 2 Nov 2016 09:23:25 -0700 (PDT) Received: by mail-oi0-x230.google.com with SMTP id 128so23489403oih.0 for ; Wed, 02 Nov 2016 09:23:27 -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=xIr3pQaQULAo+M1TKxHvHzbhUEatLsPSQiEiin+d+sw=; b=JRvODlJYyCeJX5F7m5aDlxGoyYKVmMDqDZbAjtS7CRFH/GiQGaswCzfvJe30MSRnna URUywhnqDWL4XFYd9CfOogbwS1zMwFnu08YDvChpx832/Zo/KDtMxOArjKQcKB0oveor hIUj8qMGM0v6Athq+vTbDJhzjRIE4UP9+/X+0= 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=xIr3pQaQULAo+M1TKxHvHzbhUEatLsPSQiEiin+d+sw=; b=mBJ1UAKBWQEUvb7v0eZNLFRkiD3mwqO2GgOw+qPUdwTGCNsldOxGKKLaQ5S/bnaAOA BQVOEp6jFx/arThbZTSUO8BgKwKt9wMHW0G2mG7KIaD8XTxVqL7VR9Va/RJoQovTRoMG nsk4o+1y8q5u5RNF2YEehjAp1GRVqzEDI0LaJhbXbkASKw6yqM9Uxp7M6b3AT1HdfzmV 6+Qn1CPwXAWWEDUpQDOuk9aFKSZfuYHCCZIj7EQ2w9Uo02pTpFsv82IXwDnPjkY+22j3 tTHqcMFZu0geadih9ngJKpFE8fDGuo0KKWp+Svztp0YbCNYqtoAzG9SNeNGbTRTGC735 qbEQ== X-Gm-Message-State: ABUngveSv36eo711dDNbQv7uWsKbLV6EKVWT4uY0X924Jt1c24l3IDFPx4eK7eapV+SisXBZb4QvbSGzGyJXFM8r X-Received: by 10.107.25.11 with SMTP id 11mr5039428ioz.138.1478103806359; Wed, 02 Nov 2016 09:23:26 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.59.147 with HTTP; Wed, 2 Nov 2016 09:23:25 -0700 (PDT) In-Reply-To: <20161102162129.GG27644@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> <20161102162129.GG27644@bivouac.eciton.net> From: Ard Biesheuvel Date: Wed, 2 Nov 2016 16:23:25 +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:23:25 -0000 Content-Type: text/plain; charset=UTF-8 On 2 November 2016 at 16:21, Leif Lindholm wrote: > On Wed, Nov 02, 2016 at 04:17:03PM +0000, Ard Biesheuvel wrote: >> 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. > > Agreed. > >> So you are happy with the patch if I fold the following into it? > > Super happy - thanks! > Reviewed-by: Leif Lindholm > OK, pushed.