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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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