[PATCH] ArmVirtPkg: move all platforms to MdePkg/ BaseMemoryLib implementations

[PATCH] ArmVirtPkg: move all platforms to MdePkg/ BaseMemoryLib implementations

[Ard Biesheuvel]

The BaseMemoryLibStm implementation under ArmPkg/ is being deprecated,
in favor of the generic versions under MdePkg, now that ARM and AARCH64
support has been added to both the generic C version (BaseMemoryLib) and
the accelerated version (BaseMemoryLibOptDxe). The latter uses unaligned
accesses and special cache maintenance instructions, and can therefore
not be used when the MMU is off.

So move to BaseMemoryLibOptDxe for the DXE phase and later, and to the
generic BaseMemoryLib before that.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/ArmVirt.dsc.inc | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 6c6a52b5e6fb..c624b3cdbecd 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -67,9 +67,8 @@ [LibraryClasses.common]
   #
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
 
-  # 1/123 faster than Stm or Vstm version
-  #BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
-  BaseMemoryLib|ArmPkg/Library/BaseMemoryLibStm/BaseMemoryLibStm.inf
+  # use the accelerated BaseMemoryLibOptDxe by default, overrides for SEC/PEI below
+  BaseMemoryLib|MdePkg/Library/BaseMemoryLibOptDxe/BaseMemoryLibOptDxe.inf
 
   # Networking Requirements
   NetLib|MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf
@@ -154,6 +153,7 @@ [LibraryClasses.common]
 
 [LibraryClasses.common.SEC]
   PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+  BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
 
   DebugAgentLib|ArmPkg/Library/DebugAgentSymbolsBaseLib/DebugAgentSymbolsBaseLib.inf
   DefaultExceptionHandlerLib|ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf
@@ -165,6 +165,7 @@ [LibraryClasses.common.SEC]
 
 [LibraryClasses.common.PEI_CORE]
   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
+  BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
   PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
   MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
@@ -181,6 +182,7 @@ [LibraryClasses.common.PEI_CORE]
 
 [LibraryClasses.common.PEIM]
   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
+  BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
   PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
   MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
-- 
2.7.4

Re: [PATCH] ArmVirtPkg: move all platforms to MdePkg/ BaseMemoryLib implementations

[Laszlo Ersek]

On 09/13/16 19:28, Ard Biesheuvel wrote:
> The BaseMemoryLibStm implementation under ArmPkg/ is being deprecated,
> in favor of the generic versions under MdePkg, now that ARM and AARCH64
> support has been added to both the generic C version (BaseMemoryLib) and
> the accelerated version (BaseMemoryLibOptDxe). The latter uses unaligned
> accesses and special cache maintenance instructions, and can therefore
> not be used when the MMU is off.
> 
> So move to BaseMemoryLibOptDxe for the DXE phase and later, and to the
> generic BaseMemoryLib before that.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 6c6a52b5e6fb..c624b3cdbecd 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -67,9 +67,8 @@ [LibraryClasses.common]
>    #
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
>  
> -  # 1/123 faster than Stm or Vstm version
> -  #BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
> -  BaseMemoryLib|ArmPkg/Library/BaseMemoryLibStm/BaseMemoryLibStm.inf
> +  # use the accelerated BaseMemoryLibOptDxe by default, overrides for SEC/PEI below
> +  BaseMemoryLib|MdePkg/Library/BaseMemoryLibOptDxe/BaseMemoryLibOptDxe.inf
>  
>    # Networking Requirements
>    NetLib|MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf
> @@ -154,6 +153,7 @@ [LibraryClasses.common]
>  
>  [LibraryClasses.common.SEC]
>    PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> +  BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>  
>    DebugAgentLib|ArmPkg/Library/DebugAgentSymbolsBaseLib/DebugAgentSymbolsBaseLib.inf
>    DefaultExceptionHandlerLib|ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf
> @@ -165,6 +165,7 @@ [LibraryClasses.common.SEC]
>  
>  [LibraryClasses.common.PEI_CORE]
>    PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
> +  BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>    HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
>    PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
> @@ -181,6 +182,7 @@ [LibraryClasses.common.PEI_CORE]
>  
>  [LibraryClasses.common.PEIM]
>    PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
> +  BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>    HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
>    PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH] ArmVirtPkg: move all platforms to MdePkg/ BaseMemoryLib implementations

[Laszlo Ersek]

Hi Ard,

On 09/13/16 19:28, Ard Biesheuvel wrote:
> The BaseMemoryLibStm implementation under ArmPkg/ is being deprecated,
> in favor of the generic versions under MdePkg, now that ARM and AARCH64
> support has been added to both the generic C version (BaseMemoryLib) and
> the accelerated version (BaseMemoryLibOptDxe). The latter uses unaligned
> accesses and special cache maintenance instructions, and can therefore
> not be used when the MMU is off.
> 
> So move to BaseMemoryLibOptDxe for the DXE phase and later, and to the
> generic BaseMemoryLib before that.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

unfortunately the optimized BaseMemoryLib implementation regresses the
graphics display with VirtioGpuDxe.

The Blt() implementation in VirtioGpuDxe [OvmfPkg/VirtioGpuDxe/Gop.c]
uses SetMem32() and CopyMem(). I think they hit on a corner case in the
optimized BaseMemoryLib instance.

I'm attaching two screenshots (they are small, approx. 12KB each). The
first screenshot ("Screenshot_aarch64-vgpu-2_2016-09-22_04:45:19.png")
shows the corrupted display, when ArmVirtQemu is built at 7f1bf51bdbca.

The second screenshot
("Screenshot_aarch64-vgpu-2_2016-09-22_04:50:08.png") shows the correct
display, with 3c3cf1cd731f (i.e., this patch) reverted.

There are two differences. The first difference is that on the corrupt
display, the "Console Started" message is not cleared. I think this
means that the SetMem32() function call, under case label
EfiBltVideoFill in function GopBlt() "OvmfPkg/VirtioGpuDxe/Gop.c", is
not working.

The other difference is (obviously) in the progress bar. The progress
bar is drawn as a series of filled rectangles / EfiBltVideoFill
operations; see BootLogoUpdateProgress() in
"MdeModulePkg/Library/BootLogoLib/BootLogoLib.c". Thus, the second
difference implicates SetMem32() again.

In commit c86cd1e175fb "MdePkg/BaseMemoryLibOptDxe: add accelerated
AARCH64 routines", you added the optimized InternalSetMem32() like this
(file "MdePkg/Library/BaseMemoryLibOptDxe/AArch64/SetMem.S"):

ASM_GLOBAL ASM_PFX(InternalMemSetMem32)
ASM_PFX(InternalMemSetMem32):
    dup     v0.4S, valw
    b       0f

According to the ARM ARM, this is "Duplicate general-purpose register to
vector". I tried to match the above assembly against the insn spec
(including to "Vector formats in AArch64 state"), but I failed. Are you
sure the above is correct?

Thanks
Laszlo

Re: [PATCH] ArmVirtPkg: move all platforms to MdePkg/ BaseMemoryLib implementations

[Ard Biesheuvel]

On 22 September 2016 at 04:32, Laszlo Ersek <lersek@redhat.com> wrote:
> Hi Ard,
>
> On 09/13/16 19:28, Ard Biesheuvel wrote:
>> The BaseMemoryLibStm implementation under ArmPkg/ is being deprecated,
>> in favor of the generic versions under MdePkg, now that ARM and AARCH64
>> support has been added to both the generic C version (BaseMemoryLib) and
>> the accelerated version (BaseMemoryLibOptDxe). The latter uses unaligned
>> accesses and special cache maintenance instructions, and can therefore
>> not be used when the MMU is off.
>>
>> So move to BaseMemoryLibOptDxe for the DXE phase and later, and to the
>> generic BaseMemoryLib before that.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmVirtPkg/ArmVirt.dsc.inc | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> unfortunately the optimized BaseMemoryLib implementation regresses the
> graphics display with VirtioGpuDxe.
>
> The Blt() implementation in VirtioGpuDxe [OvmfPkg/VirtioGpuDxe/Gop.c]
> uses SetMem32() and CopyMem(). I think they hit on a corner case in the
> optimized BaseMemoryLib instance.
>
> I'm attaching two screenshots (they are small, approx. 12KB each). The
> first screenshot ("Screenshot_aarch64-vgpu-2_2016-09-22_04:45:19.png")
> shows the corrupted display, when ArmVirtQemu is built at 7f1bf51bdbca.
>
> The second screenshot
> ("Screenshot_aarch64-vgpu-2_2016-09-22_04:50:08.png") shows the correct
> display, with 3c3cf1cd731f (i.e., this patch) reverted.
>
> There are two differences. The first difference is that on the corrupt
> display, the "Console Started" message is not cleared. I think this
> means that the SetMem32() function call, under case label
> EfiBltVideoFill in function GopBlt() "OvmfPkg/VirtioGpuDxe/Gop.c", is
> not working.
>
> The other difference is (obviously) in the progress bar. The progress
> bar is drawn as a series of filled rectangles / EfiBltVideoFill
> operations; see BootLogoUpdateProgress() in
> "MdeModulePkg/Library/BootLogoLib/BootLogoLib.c". Thus, the second
> difference implicates SetMem32() again.
>
> In commit c86cd1e175fb "MdePkg/BaseMemoryLibOptDxe: add accelerated
> AARCH64 routines", you added the optimized InternalSetMem32() like this
> (file "MdePkg/Library/BaseMemoryLibOptDxe/AArch64/SetMem.S"):
>
> ASM_GLOBAL ASM_PFX(InternalMemSetMem32)
> ASM_PFX(InternalMemSetMem32):
>     dup     v0.4S, valw
>     b       0f
>
> According to the ARM ARM, this is "Duplicate general-purpose register to
> vector". I tried to match the above assembly against the insn spec
> (including to "Vector formats in AArch64 state"), but I failed. Are you
> sure the above is correct?
>

Thanks for spotting this, and tracking it down to SetMem##()

When porting this code, I failed to notice that InternalMemSetMem()
does not take a byte count, but an 'item' count, which means I have to
multiply by the size again in the core implementation. ARM is also
affected btw

I will have a patch out asap