From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.7453.1588931326745174372 for ; Fri, 08 May 2020 02:48:46 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=GgFNglR8; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1588931325; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=20GwCTgogmHl4lrgqxwlp0l6d28f+zewou6lpbK55Mc=; b=GgFNglR8/5VXMQf7GV4UOqWfinzABU3W4Y4yaaoBoV97+li9d7e1x1Uaf+U1CsxV8zJEdQ Eqtq6acVPV24239qmPeUdbVUiPIFRSHln8/Wk6nzxb7VeSaMeivEa4X1UbkDeEbw0YcKlu aRCm/jp5yfHtMSLCMLqx/VQBz6xQQ0A= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-193-PM0QXz1gMKeF7h-9Ue_Llg-1; Fri, 08 May 2020 05:48:38 -0400 X-MC-Unique: PM0QXz1gMKeF7h-9Ue_Llg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 23B04835B41; Fri, 8 May 2020 09:48:36 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-60.ams2.redhat.com [10.36.114.60]) by smtp.corp.redhat.com (Postfix) with ESMTP id 516C569C88; Fri, 8 May 2020 09:48:33 +0000 (UTC) Subject: Re: [edk2-devel] APRIORI in RISC-V or Where did OVMF APRIORIs come from? To: Ard Biesheuvel , Daniel Schaefer , devel@edk2.groups.io Cc: "Chang, Abner (HPS SW/FW Technologist)" , Atish Patra , Heinrich Schuchardt , Atish Patra , Alexander Graf , Anup Patel , "leif@nuviainc.com" , jordan.l.justen@intel.com References: <20200224221949.28826-1-atish.patra@wdc.com> <4de513ea-26fb-c830-2348-c0f57946fd3d@hpe.com> <3cb1d47e-983f-dd93-33d2-5a7896791dde@arm.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 8 May 2020 11:48:32 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 05/07/20 15:53, Ard Biesheuvel wrote: > On 5/7/20 3:43 PM, Daniel Schaefer wrote: >> Should I/we try to remove the APRIORI entries from OVMF in a similar >> way? >> > > Let's get to the bottom of this first. Laszlo may remember why exactly > those entries are there in the first place, and I suspect it is a > different issue. > Some of the APRIORI entries are very old and not well documented (via git-blame / git log). I've done some cursory tests now. (1) "MdeModulePkg/Universal/PCD/Pei/Pcd.inf" in APRIORI PEI goes back to commit 49ba9447c92d ("Add initial version of Open Virtual Machine Firmware (OVMF) platform.", 2009-05-27). I've tried removing it now (eliminating the whole APRIORI PEI file), and there seem to be no ill effects. Note that this module is built like this in the DSC file: MdeModulePkg/Universal/PCD/Pei/Pcd.inf { PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf } That's because the PCD PEI depends on a number lib classes, and some of the lib instances used to resolve those lib classes depend on PcdLib. The "usual" PEI instance of PcdLib would depend on the PCD PPI, hence the PCD PEI would depend on itself. The above lib instance override breaks up the cycle. (2) The same cannot be said about "MdeModulePkg/Universal/PCD/Dxe/Pcd.inf" in APRIORI DXE. While the addition of this entry goes back to the same historical commit 49ba9447c92d ("Add initial version of Open Virtual Machine Firmware (OVMF) platform.", 2009-05-27); it does have a live dependency. (2a) Namely, "OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf" depends on dynamic PCDs, and FvbServicesRuntimeDxe really does have to be on the APRIORI DXE list. Refer to commit 182eb4562731 ("OvmfPkg: Add QemuFlashFvbServicesRuntimeDxe to firmware image", 2013-11-12). In short, FvbServicesRuntimeDxe and EmuVariableFvbRuntimeDxe are alternatives (dependent on whether QEMU runs with or without pflash), and dynamic PCDs are used to arbitrate between them. FvbServicesRuntimeDxe performs the flash detection so it must come first. (2b) Now, if OVMF is built with SMM_REQUIRE, then both FvbServicesRuntimeDxe and EmuVariableFvbRuntimeDxe fall away (pflash is a hard requirement, and an *SMM* flash driver is used, namely "OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf"). Per commit 46df0216b0ed ("OvmfPkg: pull in SMM-based variable driver stack", 2015-11-30), the FvbServicesRuntimeDxe driver is then removed from the APRIORI DXE file as well. This suggests that the PCD DXE driver *too* could be removed (with nothing depending on it in the APRIORI DXE list), when SMM_REQUIRE is TRUE. I've tested conditionalizing the PCD DXE driver like that, and it seems to work. (3) The "MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf" driver in the APRIORI DXE file is justified. It goes back to commit 5c3481b0b611 ("OvmfPkg: Use the new DevicePathLib for all platforms", 2013-08-19), and commit 863986b3c8e6. (3a) This APRIORI DXE entry is needed because of the following dependency chain: MdeModulePkg/Universal/PCD/Dxe/Pcd.inf -> MdePkg/Library/DxeHobLib/DxeHobLib.inf -> MdePkg/Library/UefiLib/UefiLib.inf -> MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.inf Due to the PCD DXE driver being on the APRIORI DXE list (see point (2) above), DevicePathDxe has to be as well. This chain could be probably broken by modifying the DSC file, to link the PCD DXE driver with the "thick" DevicePathLib instance: "MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf". (3b) Building upon (2b), we get tempted to take the PCD DXE driver and the DevicePathDxe driver *together* off the APRIORI DXE list, if if SMM_REQUIRE is defined. Unfortunately, this doesn't work, as AmdSevDxe is also in the APRIORI DXE file (see the next point), and it comes with the following dependency chain: OvmfPkg/AmdSevDxe/AmdSevDxe.inf -> MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf -> MdePkg/Library/UefiLib/UefiLib.inf -> MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.inf (4) AmdSevDxe belongs in the APRIORI DXE file. The explanation is given in commit 24e4ad75546b ("OvmfPkg: Add AmdSevDxe driver", 2017-07-10): The driver is being added to the APRIORI DXE file so that, we clear the C-bit from MMIO regions before any driver accesses it. This is not a perfect solution by far. But a better solution would have required new infrastructure -- see --, and we could never reach an agreement on that! (Just read through the ticket -- multiple approaches were proposed and some even prototyped (with patches posted); nothing was accepted.) Summarizing the status for APRIORI DXE, AmdSevDxe must be on the APRIORI DXE list, and AmdSevDxe requires DevicePathDxe. "Dxe/Pcd.inf" could be conditionalized on SMM_REQUIRE=FALSE. Summarizing the status for APRIORI PEI, I think it could be eliminated at this point. Thanks, Laszlo