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

[Laszlo Ersek <lersek@redhat.com>]

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