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 72645821ED for ; Thu, 2 Mar 2017 09:09:56 -0800 (PST) Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (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 C15D761BAC; Thu, 2 Mar 2017 17:09:56 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-103.phx2.redhat.com [10.3.117.103]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v22H9sHH008950; Thu, 2 Mar 2017 12:09:55 -0500 To: Ard Biesheuvel , edk2-devel@ml01.01.org, leif.lindholm@linaro.org References: <1488471305-23752-1-git-send-email-ard.biesheuvel@linaro.org> Cc: jiewen.yao@intel.com From: Laszlo Ersek Message-ID: Date: Thu, 2 Mar 2017 18:09:53 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <1488471305-23752-1-git-send-email-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Thu, 02 Mar 2017 17:09:56 +0000 (UTC) Subject: Re: [PATCH 0/4] ArmVirtPkg: implement basic capsule support 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, 02 Mar 2017 17:09:56 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/02/17 17:15, Ard Biesheuvel wrote: > This wires up the existing basic support for capsules left in memory by > the OS across a warm reset. This involves wiring up the PEI phase modules > to preserve the capsule images before releasing the memory for normal > consumption, and some tweaks to the boot mode and BDS platform routines. > > As proposed, this allows capsules to be used as a pstore backend, which > keeps the pstore payload in memory rather than in EFI variables. For > example, something like this is supported when the prerequisite Linux > patches have been merged (which are currently under review) > > - modprobe capsule-pstore > - echo 1 > /sys/module/kernel/parameters/panic > - echo c > /proc/sysrq-trigger > - system reboot... > - ls -l /sys/fs/pstore/ > -r--r--r-- 1 root root 4386 Feb 12 00:31 console-efi-capsule-0 > -r--r--r-- 1 root root 9065 Feb 12 00:29 dmesg-efi-capsule-6250071391647825921 > -r--r--r-- 1 root root 9073 Feb 12 00:29 dmesg-efi-capsule-6250071391647825922 > -r--r--r-- 1 root root 9096 Feb 12 00:29 dmesg-efi-capsule-6250071391647825923 > -r--r--r-- 1 root root 9073 Feb 12 00:29 dmesg-efi-capsule-6250071391647825924 > -r--r--r-- 1 root root 9048 Feb 12 00:29 dmesg-efi-capsule-6250071391647825925 > > Updating the firmware image in NOR flash in this way should be feasible as > well, but this is something that builds on top of this basic capsule support, > and involves ESRT and FMP, which have far too few vowels for me to explain > what they entail. > > Ard Biesheuvel (4): > ArmVirtPkg/ArmVirtPlatformLib: base boot mode on capsule presence > ArmVirtPkg/ArmVirtMemoryInitPeiLib: check for capsules before memory > init > ArmVirtPkg/PlatformBootManagerLib: process pending capsules > ArmVirtPkg/ArmVirtQemu: enable basic capsule support > > ArmVirtPkg/ArmVirtQemu.dsc | 7 ++- > ArmVirtPkg/ArmVirtQemu.fdf | 2 + > ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c | 60 +++++++++++++++++++- > ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf | 9 ++- > ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf | 5 +- > ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c | 4 ++ > ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 17 ++++++ > ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 2 + > 8 files changed, 101 insertions(+), 5 deletions(-) > (1) I think I disagree with this feature. What is the use of capsule-pstore over efivar-pstore? I dislike the addition of a new boot mode (BOOT_ON_FLASH_UPDATE) -- and all the complications it means for PlatformBootManagerLib -- unless there is a *very* good argument for capsule-pstore. Even if there is such a very good argument, a detailed design document would be necessary about - memory regions (address, size, life cycle etc) used for capsule passing from OS to firmware across warm reboot, - interplay between existent and newly pulled in modules (for example, what produces the EFI_HOB_TYPE_UEFI_CAPSULE HOB before patch #1 and "ArmPlatformPkg/PlatformPei/PlatformPeim.inf" consume it, what ensures the HOB would be prodced in time, how does the new boot mode affect other modules, why do we need setting / returning the new boot mode in both patches #1 and #2, ...) Even if said "very good reason" exists, I think I'd insist on a big red switch over all of this, defaulting to "off" -- a build time flag that by default excludes modules from the DSC / FDF, and also a parallel Feature PCD, which short-circuits all the new code in the modules that we build in unconditionally. (Same as we do with SMM_REQUIRE in OVMF.) (2) I most definitely disagree with the idea of firmware executable updates within the guest. That turns firmware image updates into a multi-master problem (on a virtualization host, you update firmware for guests by running a package update, and then shutting down and restarting eligible guests -- the host file that backs the pflash chip that contains the firmware executable is not even writeable to QEMU). Such a feature might perhaps be useful for testing the capsule thing itself, but in production, it's a recipe for disaster. This kind of capsule-based firmware update was invented to solve a chicken-egg problem on physical hardware, where there's nothing "underneath", but on virtual platforms, the problem doesn't exist in the first place. The implication that this feature -- which I already disagree with, without knowing a compelling reason for it -- lays the foundation for FMP, makes me *very* nervous. Laszlo