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

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

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.

/
    Leif