From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B3E901A1F32 for ; Wed, 21 Sep 2016 20:32:09 -0700 (PDT) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1D82E9E629; Thu, 22 Sep 2016 03:32:09 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-25.phx2.redhat.com [10.3.116.25]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u8M3W7f1013620; Wed, 21 Sep 2016 23:32:07 -0400 To: Ard Biesheuvel References: <1473787681-27142-1-git-send-email-ard.biesheuvel@linaro.org> Cc: edk2-devel@ml01.01.org From: Laszlo Ersek Message-ID: Date: Thu, 22 Sep 2016 05:32:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1473787681-27142-1-git-send-email-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Thu, 22 Sep 2016 03:32:09 +0000 (UTC) X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [PATCH] ArmVirtPkg: move all platforms to MdePkg/ BaseMemoryLib implementations X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Sep 2016 03:32:09 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 > --- > 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