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

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

On 2 November 2016 at 16:21, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 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>
>

OK, pushed.