| Message ID | CAKv+Gu9xpf-geDgtHEiHRLDCL9-Tu+_h9im2SbZ5XAfd=kpHJw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
--- 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,