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

Message ID 1477937590-10361-5-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel Oct. 31, 2016, 6:13 p.m.
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(-)

-- 
2.7.4

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

Comments

Leif Lindholm Nov. 1, 2016, 10:32 p.m. | #1
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)?

>    Status = gBS->InstallMultipleProtocolInterfaces (

>                  &mCpuHandle,

>                  &gEfiCpuArchProtocolGuid,           &mCpu,

> -- 

> 2.7.4

> 

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


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

Patch

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 ();
+
   Status = gBS->InstallMultipleProtocolInterfaces (
                 &mCpuHandle,
                 &gEfiCpuArchProtocolGuid,           &mCpu,