From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x22d.google.com (mail-wm0-x22d.google.com [IPv6:2a00:1450:400c:c09::22d]) (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 D539081C8B for ; Wed, 2 Nov 2016 09:21:31 -0700 (PDT) Received: by mail-wm0-x22d.google.com with SMTP id a197so151306089wmd.0 for ; Wed, 02 Nov 2016 09:21:33 -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=X94OxC85hsIUCs0A35ozis92rqRzv8RQHg6oJ8Yrbv4=; b=RwZoJg7J43S/MVS2F51N7fgaRDWKeNs22qEoMbYdTUfoLdrRkrlVxqIiM57OxTeVlZ ZUOjAHGiPzCP5jxjJ5UQyE7tUVFWohuGld3u2Y6auG5Zng8Xral0zTg8RH3wlBIaeK/p WHcBI+JcQGRVdKPqWWBLJkbsBOqaAU48mqUwg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=X94OxC85hsIUCs0A35ozis92rqRzv8RQHg6oJ8Yrbv4=; b=X1yvklDY9oFhUSB5n4/Yr+Rfn2uBwvEGRAXy6/miVPWxs0sjMtBWzGII3A7vHHxvqQ paxKYP6QNCPliXlK7DQS29Zbl2Q/fOGGU0KrS3ggaJa0DJn7zAmN484d0hqkiho2fPAk ItqnwqCM51olsfVcUREF/1BloGjs+KweKMdihe48eDWxzaj7Az1HHrQIDPn0s28EsgW2 6AOf7+jkQK0cI2r+L2AogbqtjNELDY5uQvyvOBEmhhPjCreCRsl9XfripjsV98d9o4CX +jGVQm33kNpzNB/ztzTRL6bHgZQlU6zXlt31jHFRh62qTDKVr1FzNq1QnYjlLgeXJvVp l5tw== X-Gm-Message-State: ABUngvcn4LMOeUtDWBOk/2CzXzcxUR/oOi5C4dttxlhYJ8LHW1KZyjfQs2k7zWSuo9fbOEda X-Received: by 10.28.6.203 with SMTP id 194mr3946846wmg.16.1478103691506; Wed, 02 Nov 2016 09:21:31 -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 v3sm3657512wjm.4.2016.11.02.09.21.30 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 02 Nov 2016 09:21:30 -0700 (PDT) Date: Wed, 2 Nov 2016 16:21:29 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel-01 Message-ID: <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> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) 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:21:32 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > > """ > --- 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, > """