[edk2,4/5] ArmPkg/CpuDxe: set DmaBufferAlignment according to CWG

Message ID CAKv+Gu9xpf-geDgtHEiHRLDCL9-Tu+_h9im2SbZ5XAfd=kpHJw@mail.gmail.com
State New
Headers show

Commit Message

Ard Biesheuvel Nov. 2, 2016, 4:17 p.m.
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?

"""
"""
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm Nov. 2, 2016, 4:21 p.m. | #1
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,

> """

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Nov. 2, 2016, 4:23 p.m. | #2
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

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