From patchwork Thu Sep 4 08:39:54 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laszlo Ersek X-Patchwork-Id: 36683 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-pa0-f70.google.com (mail-pa0-f70.google.com [209.85.220.70]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id AE418202E4 for ; Thu, 4 Sep 2014 08:40:37 +0000 (UTC) Received: by mail-pa0-f70.google.com with SMTP id lf10sf91118846pab.9 for ; Thu, 04 Sep 2014 01:40:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:message-id:date:from:user-agent :mime-version:to:references:in-reply-to:cc:subject:precedence :reply-to:list-id:list-unsubscribe:list-archive:list-post:list-help :list-subscribe:errors-to:x-original-sender :x-original-authentication-results:mailing-list:content-type; bh=AF8PJ/Q7zrogKC5HoDPoLHwZCV/aMH79ExPAYa8qGV8=; b=dpjIHUhW5oKhXyh6OteoX6qQIHbWikf/luJa7Yv9vLejdr4wPDaWHVH6kNziz7kekb Iu5W/vTZIgj/P4GLQdFhZg7Z77CVPCLCkNuLyGSpA+5GcR5KOrWqiaTCZlWZwQ2wQEi8 IM+ofkndP/dmG1JlAi9ffUDlZA6xOEHd3JM57tuKC6uYmLdo0qeQL7tTJ6EAtMiIBF6Y 8fvSvU+BsblB/5JJY2aO76nOGezrDZ6zoNJcud0CiEUgA19JDtmvd5dyC2Ze2lIBTiy7 s10JK4cjHme5g8Zs20iqR2Ec/eRx9liFLoVHu0hXsDML67FKf+PZUNSdBhBcYzgBsVvP R9uQ== X-Gm-Message-State: ALoCoQk1tIHdRzTYgW3N6YYFfZ46IxBMaRTkZN0xD4udrxRosNnefHzgtQd5NUORVTvh6Lg9TM9t X-Received: by 10.70.96.197 with SMTP id du5mr1707238pdb.3.1409820032907; Thu, 04 Sep 2014 01:40:32 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.19.108 with SMTP id 99ls205523qgg.29.gmail; Thu, 04 Sep 2014 01:40:32 -0700 (PDT) X-Received: by 10.220.184.70 with SMTP id cj6mr2202036vcb.5.1409820032290; Thu, 04 Sep 2014 01:40:32 -0700 (PDT) Received: from mail-vc0-f177.google.com (mail-vc0-f177.google.com [209.85.220.177]) by mx.google.com with ESMTPS id jq10si2525265vdb.75.2014.09.04.01.40.32 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 04 Sep 2014 01:40:32 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.177 as permitted sender) client-ip=209.85.220.177; Received: by mail-vc0-f177.google.com with SMTP id hq11so10074307vcb.22 for ; Thu, 04 Sep 2014 01:40:32 -0700 (PDT) X-Received: by 10.52.129.165 with SMTP id nx5mr1037697vdb.25.1409820032139; Thu, 04 Sep 2014 01:40:32 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.221.45.67 with SMTP id uj3csp800541vcb; Thu, 4 Sep 2014 01:40:31 -0700 (PDT) X-Received: by 10.50.142.68 with SMTP id ru4mr4439211igb.18.1409820030829; Thu, 04 Sep 2014 01:40:30 -0700 (PDT) Received: from lists.sourceforge.net (lists.sourceforge.net. [216.34.181.88]) by mx.google.com with ESMTPS id we8si1423579icb.75.2014.09.04.01.40.30 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Thu, 04 Sep 2014 01:40:30 -0700 (PDT) Received-SPF: pass (google.com: domain of edk2-devel-bounces@lists.sourceforge.net designates 216.34.181.88 as permitted sender) client-ip=216.34.181.88; Received: from localhost ([127.0.0.1] helo=sfs-ml-2.v29.ch3.sourceforge.com) by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1XPSaZ-0004Bx-KG; Thu, 04 Sep 2014 08:40:19 +0000 Received: from sog-mx-4.v43.ch3.sourceforge.com ([172.29.43.194] helo=mx.sourceforge.net) by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1XPSaX-0004Br-Eh for edk2-devel@lists.sourceforge.net; Thu, 04 Sep 2014 08:40:17 +0000 Received-SPF: pass (sog-mx-4.v43.ch3.sourceforge.com: domain of redhat.com designates 209.132.183.28 as permitted sender) client-ip=209.132.183.28; envelope-from=lersek@redhat.com; helo=mx1.redhat.com; Received: from mx1.redhat.com ([209.132.183.28]) by sog-mx-4.v43.ch3.sourceforge.com with esmtps (TLSv1:AES256-SHA:256) (Exim 4.76) id 1XPSaS-0002Xw-KO for edk2-devel@lists.sourceforge.net; Thu, 04 Sep 2014 08:40:17 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s848dv6a006266 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 4 Sep 2014 04:39:58 -0400 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-78.ams2.redhat.com [10.36.116.78]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s848dsmM027831; Thu, 4 Sep 2014 04:39:55 -0400 Message-ID: <5408255A.8060601@redhat.com> Date: Thu, 04 Sep 2014 10:39:54 +0200 From: Laszlo Ersek User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.0 MIME-Version: 1.0 To: Ard Biesheuvel References: <1409743096-14919-24-git-send-email-ard.biesheuvel@linaro.org> <1409796596-12615-1-git-send-email-lersek@redhat.com> In-Reply-To: X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Spam-Score: -3.2 (---) X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. -1.5 SPF_CHECK_PASS SPF reports sender host as permitted sender for sender-domain -0.0 SPF_HELO_PASS SPF: HELO matches SPF record -0.0 SPF_PASS SPF: sender matches SPF record -1.6 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain X-Headers-End: 1XPSaS-0002Xw-KO Cc: Peter Maydell , "edk2-devel@lists.sourceforge.net" , Christoffer Dall , Ilias Biris Subject: Re: [edk2] [PATCH 0/9] first round of proposed fixups for "add support for AArch64 QEMU/KVM v6" X-BeenThere: edk2-devel@lists.sourceforge.net X-Mailman-Version: 2.1.9 Precedence: list Reply-To: edk2-devel@lists.sourceforge.net List-Id: List-Unsubscribe: , List-Archive: List-Post: , List-Help: , List-Subscribe: , Errors-To: edk2-devel-bounces@lists.sourceforge.net X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: lersek@redhat.com X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.177 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 On 09/04/14 08:32, Ard Biesheuvel wrote: > On 4 September 2014 04:09, Laszlo Ersek wrote: >> Hi Ard, >> >> I started to review your v6 patchset in reverse order -- I first created >> a map between your v5 and v6 patches (as much as it was possible), then >> started to look at the DSC file(s) first. The requirement to dynamically >> determine the UART base address from the DTB is a very intrusive one, >> unfortunately. So, the first thing that caught my eye was the various >> new instances of the SerialPortLib class. >> >> As we discussed on #linaro-enterprise, "EarlyFdtPL011SerialPortLib" is >> not appropriate for DXE_CORE, because it accesses the initial copy of >> the DTB (at the bottom of the system DRAM), and at that point, when the >> DXE_CORE is already running, that memory area is no longer protected >> (because we decided to relocate it instead of protecting it in-place). >> >> So, I didn't get very far in the v6 review, but I do think I can propose >> an improvement for the DXE_CORE's serial port library. Please see the >> following patches. >> > > Looks great, thanks! I will look in more detail later today, but one > thing comes to mind already: > since the PcdGet() call in the constructor of the ordinary FdtPL011 > library is causing dependency hell and the need for a cloned > UefiBootServicesTableLib, is there any reason we can't use your > DXE_CORE version for *all* stages after DXE_CORE as well? I'm very pleased that you too realized this possibility. Unfortunately, it doesn't work. I tested it, and only upon seeing that it didn't work did I elect to post this version of the fixup series. (This is the reason I posted the series after 4AM, and not at 01:30AM.) Namely, the HobLib class has three instances: HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf The PEI flavor allows linking against PEIM PEI_CORE SEC, but it doesn't work in SEC (at least with the other lib resolutions that are used in ArmPlatformPkg -- FWIW I didn't test it out elsewhere). It is not relevant right now, I'm just mentioning it because originally I even tried to set the HOB in SEC, so that *all* SerialPortLib instances would the HOB based approach, but it didn't fly. PeiHobLib doesn't work in SEC, because needed PEI services are not available. And, in PEI itself, it would be a mess to order some PEIMs (providing those services) against other PEIMs. Hence I preserved your EarlyFdt implementation for PEIM PEI_CORE SEC; it works fine. The DxeCoreHobLib instance is only usable with DXE_CORE, and it has no CONSTRUCTOR. (You do see where this is going!) This allows the DxeCoreSerialPortLib instance I posted to work "automagically", even though it depends on HobLib. (Because, DxeCoreHobLib depends on DebugLib depends on DxeCoreSerialPortLib...) It all works because DxeCoreHobLib uses "gHobList", without a constructor, which just comes from "MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.c". IOW we do exploit that we're in the DXE_CORE -- the DXE_CORE has a special HobLib instance. No such luck with DxeHobLib. That one has a CONSTRUCTOR, and BuildTools immediately detect a circular dependency involving DxeHobLib (depending on DebugLib depending on this new SerialPortLib depending on HobLib). If I update the new, HOB-dependent SerialPortLib instance, removing its constructor, and initializing it in its SerialPortInitialize() function instead, then BuildTools don't detect a cycle -- and upon realizing this, I *mistakenly* posted "I've seen the light" in the other thread. Except it doesn't work, because the *exact same* constructor call order problem hits. Namely, in the VirtFdtDxe module, DebugLib's constructor calls SerialPortInitialize() manually, which calls GetFirstGuidHob(), and that blows up because DxeHobLib's CONSTRUCTOR has not yet run. This is a pity because this approach otherwise allowed me to completely remove both PcdPL011BaseAddress and the forked UefiBootServicesTableLib. To my dismay however, replacing our dependency on dynamic PCDs in SerialPortLib with a dependency on HOBs reintroduces the exact same kind of circular mess. This clearly shows that SerialPortLib had always been designed to be 100% self-contained, and we're violating that by trying to use dynamic PCDs *or* HOBs in it, generally. We only get lucky in DXE_CORE with HOBs (because there a HOB dependency happens to work), and in the other (later) DXE module types with dynamic PCDs (because you hacked around the construction order with your forked UefiBootServicesTableLib). My proposal is to go with this mixed approach (and pray that the above luck not run out), or to own up to the design constraints of SerialPortLib, and stick with a FixedAtBuild UART base address. (Honestly, I don't perceive that to be a grave limitation.) > Getting the > base address from a HOB and caching it doesn't seem more heavy weight > than getting it from a Dynamic PCD, and it would allow us to drop some > of the nasty stuff. Yes, it does, but it also reintroduces the same kind of circular dependencies, just with different names. I guess it could be worked around by cloning DxeHobLib, and removing its DebugLib dependency, but then again you did the exact same thing with UefiBootServicesTableLib, and that one is actually a much simpler library. I can send you this version of the "fixup patchset" if you'd like to experiment with it. As far as I see, *everything* in edk2 seems to depend on DebugLib, whose serial port instance depends on SerialPortLib, which is expected to be fully standalone. It is not just an implementation shortcoming, it's a theoretical problem -- if you need some HOB to be able to log a message, then you can't log messages in the HOB library itself, especially not in its CONSTRUCTOR. Note that such problems don't occur in OVMF due to a very simple reason: we have our DebugLib instance that plainly hardwires the qemu debug ioport (it's not even a serial port). Which would correspond, on ARM, to using a fixed UART base address. In summary, there are three options: - revert the dynamic UART base address feature, stick with fixed - use your approach (UefiBootServicesTableLib dependency hack), just fix it up with this followup series for the DXE_CORE - opt for a clean HOB-based approach, and sever the circular dependencies by cloning every library instance that inconveniently depends on DebugLib (like DxeHobLib), and kill those DebugLib deps. At least determining the set of these libraries is fairly simple, because SerialPortLib's CONSTRUCTOR (if it has one) triggers the cycle check. Basically, by enabling SerialPortLib to depend on other facilities, we must shift the "100% self-reliance" property to some of those other facilities. You know what, I'm attaching the HOB-based approach; perhaps you'd like to experiment with option #3. The first four patches are shared between the two versions (up to and including "ArmVirtualizationPlatformLib: lock down client module types"), they diverge at patch #5 ("ArmVirtualizationPlatformLib: stash dynamic UART base in a HOB in the PEI code"). Thanks, Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ >From 5c6e09d71a8a88c0cca19cda551758eadc435a1d Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Thu, 4 Sep 2014 03:19:09 +0200 Subject: [PATCH 12/12] drop references to ArmVirtualizationUefiBootServicesTableLib Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek --- ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualization.dsc.inc | 1 - 1 file changed, 1 deletion(-) diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualization.dsc.inc b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualization.dsc.inc index 43ec205..1817891 100644 --- a/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualization.dsc.inc +++ b/ArmPlatformPkg/ArmVirtualizationPkg/ArmVirtualization.dsc.inc @@ -83,7 +83,6 @@ PL011UartLib|ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf SerialPortLib|ArmPlatformPkg/ArmVirtualizationPkg/Library/FdtPL011SerialPortLib/DxePL011SerialPortLib.inf SerialPortExtLib|ArmPlatformPkg/ArmVirtualizationPkg/Library/FdtPL011SerialPortLib/NullSerialPortExtLib.inf - UefiBootServicesTableLib|ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationUefiBootServicesTableLib/ArmVirtualizationUefiBootServicesTableLib.inf # EBL Related Libraries EblCmdLib|ArmPlatformPkg/Library/EblCmdLib/EblCmdLib.inf -- 1.8.3.1