From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-x22a.google.com (mail-wr0-x22a.google.com [IPv6:2a00:1450:400c:c0c::22a]) (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 D9950802BF for ; Thu, 9 Mar 2017 09:00:26 -0800 (PST) Received: by mail-wr0-x22a.google.com with SMTP id l37so49234649wrc.1 for ; Thu, 09 Mar 2017 09:00:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=LMjFB4hgIaFJCgYNmhykqyqLOatKP9wsoj+mqFVxJfE=; b=QTzRwQmF73cetZT0unrg7sQJ9YQ9bzdhlqSKYFTDuc6uUZeOOhTvrm0hNpHCGisyjr b3MHmXpvsxfToJpEmW5l1mUK/RuLSp+rEkhHJ7REM4gOpDzdCMFWwfGNGItZ15GALKj+ PxqswPVSntIoiND3O0nnmebNfjbbUUJllToxA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=LMjFB4hgIaFJCgYNmhykqyqLOatKP9wsoj+mqFVxJfE=; b=bc5bKKA5kIhQM5i/eT3odeV4/6WrK5hRWaRFFY/O5U6UVuXb6JTJtr6UfOLYzyXvQ+ r+4Yttm8T0K6metMFVvSJ+BGN6cII9wqVfWXYEWJHDoi5M2vW8fA/7d9OB3XBEk3UKVc kRH6KdcBms4/qktBshGrnaPCH49g2CCIfUxrQjC0tNysqvx54EyVRN3JNhyH+x4MUohA 8OgTguXQImYI929pD0uNG5hCF72PGtHUZKHFmXiBWrVHk+5yAlC8QCCNJEsoFVVeFsDQ +Ox7i0+4cGZ/BMXg60IMOeKyc+uUKQLMQXA7PPkKJsW76bdRZOTLEzl5dG59lxYSuRJM yZsw== X-Gm-Message-State: AMke39mz7Y0nJPuuyPNmGR3KJBRht+TxX4L6aU0ZNDNQ8WC3Ep/gmd+3TLXjcxyRXy2VOQq9 X-Received: by 10.223.153.225 with SMTP id y88mr11271828wrb.81.1489078825166; Thu, 09 Mar 2017 09:00:25 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id c2sm8876865wre.55.2017.03.09.09.00.24 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 09 Mar 2017 09:00:24 -0800 (PST) Date: Thu, 9 Mar 2017 17:00:22 +0000 From: Leif Lindholm To: Laszlo Ersek Cc: Ard Biesheuvel , edk2-devel-01 Message-ID: <20170309170022.GI16034@bivouac.eciton.net> References: <20170308190511.31195-1-lersek@redhat.com> <2936c55e-ac00-6595-95a8-d8cfdbd83cb8@redhat.com> <886a23a6-2090-9a74-7672-e86b0d1224be@redhat.com> MIME-Version: 1.0 In-Reply-To: <886a23a6-2090-9a74-7672-e86b0d1224be@redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH 0/6] ArmVirtPkg: don't forward the DT to the OS if QEMU provides ACPI 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, 09 Mar 2017 17:00:27 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Laszlo, Apologies, I didn't ignore this set, I just missed it (and felt Ard's set was a clean solution to this behaviour change). A few comments below. On Thu, Mar 09, 2017 at 04:30:19PM +0100, Laszlo Ersek wrote: > On 03/09/17 13:26, Ard Biesheuvel wrote: > >>> Hi Laszlo, > >>> > >>> This looks complicated to me. Given that it is arguably a policy to > >>> only expose on h/w description or the other, couldn't we simply remove > >>> the FDT config table in BDS if an ACPI/ACPI2.0 config table is > >>> present? > >> > >> Technically we could do that, but I dislike it for two reasons: > >> > >> - BDS is often the first victim found when looking for a driver to add > >> new code to that doesn't seem to fit very well elsewhere. That doesn't > >> make BDS any better a recipient, however. "For lack of a better driver" > >> is not a strong enough argument to dump code into BDS. If there's really > >> no better "topical" driver, then the code usually goes to PlatformDxe. > >> > >> - Installing a sysconfig table (or any other system-wide resource) in > >> one driver, then undoing it in another driver, should be avoided as much > >> as possible, because it leads to non-trivial lifecycles and boggles our > >> minds over the longer term. If we can come to a decision that the table > >> shouldn't be installed in the first place, we should pursue that. > >> > >> Another approach we could look into is: move the installation of the > >> sysconfig table from FdtClientDxe to AcpiPlatformDxe. Look for the ACPI > >> payload first, and fall back to installing DT (from within > >> AcpiPlatformDxe). However, DT should be installed even in builds (like > >> ARM32) that don't contain AcpiPlatformDxe at all. > >> > > > > Or we could hook to the ReadyToBoot event in FdtClientDxe, and install > > the DT config table if there is no ACPI/ACPI2.0 table registered. > > Yes, that's doable in our case, because we control the full platform. > > Installing tables (any kinds of tables) in ReadyToBoot and similar event > handlers is generally a bad idea, because everyone thinks, "okay I'll > wait until the rest of the system is done setting up, and I'll just add > my stuff afterwards". Obviously, this results in much of the logic being > simply moved to such event callbacks, and the invocation order of > callbacks remains unspecified. > > In more precise terms, if the ACPI tables too were installed in a > ReadyToBoot callback, your suggestion above would not work. And our ACPI > tables are not installed in a ReadyToBoot callback partly because I > ultimately introduced a separate event group for "PCI bridges have been > connected", and we signal it now explicitly from BDS (device connections > are BDS jurisdiction, and QEMU's ACPI generation depends on PCI state). All you say above is clearly correct. But I am still not clear on why this is a problem. This is a _very_ specific case, that applies only to virtual machines (which we are in complete control of). For hardware platforms wanting the ability to switch between different hardware description types, as we discussed, we need a configuration setting based on a dynamic Pcd or environment variable - so they won't need to wait until the end. > I just want to point out that we have a kind of "capital" here. By > carefully coding stuff we build capital, and by hooking stuff into > ReadyToBoot callbacks we spend (hopefully not "squander") that capital. I fully agree with this as a strong default position. But I am suggesting that in this case, the callback sort order genuinely does not matter for this feature. At which point I prefer the simpler solution of Ard's set. That said, there are two maintainers of ArmVirtPkg, and I'm neither of them :) > Originally, the APM Mustang firmware (open source, so I can talk about > it) would first install ACPI tables with constant, platform-tailored > contents (built from *.asl / *.aslc files), but reusing the stock > AcpiPlatformDxe C code without customization. Then it would install a > ReadyToBoot callback which looked up the right DSDT or SSDT, by walking > the table tree manually, and then it would poke data into the installed > table (DSDT or SSDT) in-place, using the ACPI SDT protocol. > > Of course it was completely bogus and unreliable, and I changed the > constant table to contain external references, and I provided those > external references in a minimal, hand- and runtime- built SSDT, right > in AcpiPlatformDxe. > > I'm not trying to carefully compose a strawman argument here, just > presenting why I'm nervous about ReadyToBoot callbacks that try to rely > on ordering between system-wide resources. > > > Also, please don't forget about the other (current) consumer of the > feature PCD, ArmVirtPL031FdtClientLib, which is plugged into > RealTimeClockRuntimeDxe. How do you suggest to rewrite the PCD test in > that driver under the ReadyToBoot callback scenario? > > RealTimeClockRuntimeDxe's dispatch order is unspecified relative to the > installation of ACPI tables, so you couldn't look at the latter's > presence to see if the DTB needs an update. (In fact, because > AcpiPlatformDxe's main actions are cued in from BDS in practice, the > tables would be guaranteed not to exist when RealTimeClockRuntimeDxe looks.) > > So ArmVirtPL031FdtClientLib would have to install another ReadyToBoot > callback for modifying the DTB. And this callback could be invoked > before or after the callback to FdtClientDxe. (I guess it would be okay, > but not very intuitive.) (I think Ard's resolution of this, in 1/3, could be applied regardless of method picked for the overall changeset.) Regards, Leif > >> This series indeed turned out a bit more complex than I had expected, > >> but it was the one I could post with a good conscience. Can you perhaps > >> identify the part(s) in more detail that seem overly complex to you? > >> > > > > Building the same library in two different ways, having to call a > > library constructor explicitly in some cases and muck about with TPL > > levels to prevent a protocol notify from triggering are all things I > > would really like to avoid tbh > > Alright; can you please post the alternative patch set? (With the > ReadyToBoot callback(s), that is, not the BDS hack.) > > Thanks! > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel