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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Wed, Nov 02, 2016 at 04:17:03PM +0000, Ard Biesheuvel wrote:
> 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.

Agreed.

> So you are happy with the patch if I fold the following into it?

Super happy - thanks!
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> 
> """
> --- 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,
> """