public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Leif Lindholm <leif.lindholm@linaro.org>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>
Subject: Re: [PATCH 4/5] ArmPkg/CpuDxe: set DmaBufferAlignment according to CWG
Date: Wed, 2 Nov 2016 16:17:03 +0000	[thread overview]
Message-ID: <CAKv+Gu9xpf-geDgtHEiHRLDCL9-Tu+_h9im2SbZ5XAfd=kpHJw@mail.gmail.com> (raw)
In-Reply-To: <20161102161040.GD27644@bivouac.eciton.net>

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?

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


  reply	other threads:[~2016-11-02 16:17 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-31 18:13 [PATCH 0/5] EmbeddedPkg: generic support for reusing PCI drivers for platform devices Ard Biesheuvel
2016-10-31 18:13 ` [PATCH 1/5] EmbeddedPkg: introduce platform PCI I/O protocol Ard Biesheuvel
2016-11-01 21:54   ` Leif Lindholm
2016-11-02 13:29     ` Ard Biesheuvel
2016-11-02 15:42       ` Leif Lindholm
2016-10-31 18:13 ` [PATCH 2/5] EmbeddedPkg: introduce platform PCI I/O registration library Ard Biesheuvel
2016-11-01 21:57   ` Leif Lindholm
2016-11-02 13:30     ` Ard Biesheuvel
2016-11-02 15:39       ` Leif Lindholm
2016-11-07 14:54       ` Evan Lloyd
2016-10-31 18:13 ` [PATCH 3/5] EmbeddedPkg: implement generic platform PCI I/O driver Ard Biesheuvel
2016-11-01 22:22   ` Leif Lindholm
2016-11-02 13:39     ` Ard Biesheuvel
2016-11-02 16:05       ` Leif Lindholm
2016-10-31 18:13 ` [PATCH 4/5] ArmPkg/CpuDxe: set DmaBufferAlignment according to CWG Ard Biesheuvel
2016-11-01 22:32   ` Leif Lindholm
2016-11-02 13:40     ` Ard Biesheuvel
2016-11-02 16:10       ` Leif Lindholm
2016-11-02 16:17         ` Ard Biesheuvel [this message]
2016-11-02 16:21           ` Leif Lindholm
2016-11-02 16:23             ` Ard Biesheuvel
2016-10-31 18:13 ` [PATCH 5/5] EmbeddedPkg/PlatformPciIoDxe: add support for non-coherent DMA Ard Biesheuvel
2016-11-01 22:43   ` Leif Lindholm
2016-11-02 13:43     ` Ard Biesheuvel
2016-11-02 16:17       ` Leif Lindholm
2016-11-01 21:45 ` [PATCH 0/5] EmbeddedPkg: generic support for reusing PCI drivers for platform devices Leif Lindholm

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKv+Gu9xpf-geDgtHEiHRLDCL9-Tu+_h9im2SbZ5XAfd=kpHJw@mail.gmail.com' \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox