Re: [edk2] [PATCH 4/5] ArmPkg/CpuDxe: set DmaBufferAlignment according to CWG
On 2 November 2016 at 16:21, Leif Lindholm wrote:
> On Wed, Nov 02, 2016 at 04:17:03PM +, Ard Biesheuvel wrote:
>> On 2 November 2016 at 16:10, Leif Lindholm wrote:
>> > On Wed, Nov 02, 2016 at 01:40:17PM +, Ard Biesheuvel wrote:
>> >> On 1 November 2016 at 22:32, Leif Lindholm
>> >> wrote:
>> >> > On Mon, Oct 31, 2016 at 06:13:09PM +, 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
>> >> >> ---
>> >> >> 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
>
OK, pushed.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 4/5] ArmPkg/CpuDxe: set DmaBufferAlignment according to CWG
On Wed, Nov 02, 2016 at 04:17:03PM +, Ard Biesheuvel wrote:
> On 2 November 2016 at 16:10, Leif Lindholm wrote:
> > On Wed, Nov 02, 2016 at 01:40:17PM +, Ard Biesheuvel wrote:
> >> On 1 November 2016 at 22:32, Leif Lindholm
> >> wrote:
> >> > On Mon, Oct 31, 2016 at 06:13:09PM +, 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
> >> >> ---
> >> >> 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
>
> """
> --- 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
Re: [edk2] [PATCH 4/5] ArmPkg/CpuDxe: set DmaBufferAlignment according to CWG
On 2 November 2016 at 16:10, Leif Lindholm wrote:
> On Wed, Nov 02, 2016 at 01:40:17PM +, Ard Biesheuvel wrote:
>> On 1 November 2016 at 22:32, Leif Lindholm wrote:
>> > On Mon, Oct 31, 2016 at 06:13:09PM +, 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
>> >> ---
>> >> 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?
"""
--- 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
Re: [edk2] [PATCH 4/5] ArmPkg/CpuDxe: set DmaBufferAlignment according to CWG
On Wed, Nov 02, 2016 at 01:40:17PM +, Ard Biesheuvel wrote:
> On 1 November 2016 at 22:32, Leif Lindholm wrote:
> > On Mon, Oct 31, 2016 at 06:13:09PM +, 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
> >> ---
> >> 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
Re: [edk2] [PATCH 4/5] ArmPkg/CpuDxe: set DmaBufferAlignment according to CWG
On 1 November 2016 at 22:32, Leif Lindholm wrote:
> On Mon, Oct 31, 2016 at 06:13:09PM +, 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
>> ---
>> 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
Re: [edk2] [PATCH 4/5] ArmPkg/CpuDxe: set DmaBufferAlignment according to CWG
On Mon, Oct 31, 2016 at 06:13:09PM +, 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
> ---
> 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
[edk2] [PATCH 4/5] ArmPkg/CpuDxe: set DmaBufferAlignment according to CWG
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
---
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 ();
+
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
