Re: [PATCH 4/5] ArmPkg/CpuDxe: set DmaBufferAlignment according to CWG

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 2 November 2016 at 16:10, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Nov 02, 2016 at 01:40:17PM +0000, Ard Biesheuvel wrote:
>> On 1 November 2016 at 22:32, Leif Lindholm <leif.lindholm@linaro.org> 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 <ard.biesheuvel@linaro.org>
>> >> ---
>> >>  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,
"""