From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x233.google.com (mail-io0-x233.google.com [IPv6:2607:f8b0:4001:c06::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8C4AF1A1F37 for ; Thu, 22 Sep 2016 01:09:10 -0700 (PDT) Received: by mail-io0-x233.google.com with SMTP id m79so78316948ioo.3 for ; Thu, 22 Sep 2016 01:09:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=beolZKT2TgAd2Brqmpk6hChvpit4+0Bd5Aam30eY67Y=; b=IkqVG5K9H/AeR40p/TTqxzUL8elGU+eULDuMDCb0gJCYU4AF1NF4hLT7zfUzlN37tH Mvz59ND6t88uU+e+hDTKh3OguGWZwaZUABNnR80Y/l+6bB8PkQdEXjaVz6djgvjNudYm ymY2/Ahd/sdAOD3O5OwCgwf78YAl5NePYTsE8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=beolZKT2TgAd2Brqmpk6hChvpit4+0Bd5Aam30eY67Y=; b=P2p1aApZj8DQaqP4oFo4Cp1bGG2Nb7EwnMKnzDEMJh7b2A7jlEUd0+X3GIA07eodif 0Nh3ddc0BVAELqYnzlmtby0Zth1MgBlcbmH4LtLzXrzv2wxZNP7/4ozLC59G+EN21TbP bF6o1NBpuETHRv9W0ys4fTNY1UwTaOmBStOsI/EaXNBs82BdmDtrAyqKnVTYnsD3Azwv 4FVZUIcOxZKXv7rutjSwZ0f86S1kUsI9HjJPmLx2cmM0kPh4dYFCQ5KbH36/MNzyoSD8 nfqoiCJ9GVXB/O8/S9007fuN1i6jvziwRm5t449G/RSSYDrgl4cwX4/LC2hN7JrXc/F4 Zd0w== X-Gm-Message-State: AE9vXwPe58F4oTK4qCDKkEihjIGPCgEyk81a+QHMqVDZAYaaecDu2/eOoM+CljWZHVKjj5+7599Jp7ka1Pt1u0BX X-Received: by 10.107.133.17 with SMTP id h17mr1510265iod.148.1474531749829; Thu, 22 Sep 2016 01:09:09 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.204.195 with HTTP; Thu, 22 Sep 2016 01:09:09 -0700 (PDT) In-Reply-To: References: <1473787681-27142-1-git-send-email-ard.biesheuvel@linaro.org> From: Ard Biesheuvel Date: Thu, 22 Sep 2016 09:09:09 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" 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 08:09:10 -0000 Content-Type: text/plain; charset=UTF-8 On 22 September 2016 at 04:32, Laszlo Ersek 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 >> --- >> 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