From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web08.53745.1606233542854084222 for ; Tue, 24 Nov 2020 07:59:03 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=AhmaErPr; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1606233542; 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=kmLmSjR0ARpAiFDPHyhxINGR33R6aIoJpbwc+5Rv3aA=; b=AhmaErPrGNgobO9te7NfIeh4eIccPsf1fF2ZFqSsudI1WgE2cUVpFvFPekWI4OE+HrudLB nhNTeNwB71ipepO2OyfPTvN+XAoZenR/RryF1JogXSF94tV27ISdoOoKfnsA0K8YurvnPC HdAR4NjtADefjaN4bVx+dv1R122Cel8= 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-152-ovQy1YjUMTObTFPIWSEtVg-1; Tue, 24 Nov 2020 10:58:55 -0500 X-MC-Unique: ovQy1YjUMTObTFPIWSEtVg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CCB5118C43D3; Tue, 24 Nov 2020 15:58:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-25.ams2.redhat.com [10.36.112.25]) by smtp.corp.redhat.com (Postfix) with ESMTP id E88535D6AB; Tue, 24 Nov 2020 15:58:50 +0000 (UTC) Subject: Re: [PATCH v2 2/6] OvmfPkg/AmdSev: add Grub Firmware Volume Package From: "Laszlo Ersek" To: jejb@linux.ibm.com, devel@edk2.groups.io Cc: dovmurik@linux.vnet.ibm.com, Dov.Murik1@il.ibm.com, ashish.kalra@amd.com, brijesh.singh@amd.com, tobin@ibm.com, david.kaplan@amd.com, jon.grimm@amd.com, thomas.lendacky@amd.com, frankeh@us.ibm.com, "Dr . David Alan Gilbert" References: <20201120184521.19437-1-jejb@linux.ibm.com> <20201120184521.19437-3-jejb@linux.ibm.com> <28e99174-79b3-e805-b977-5fed0071a702@redhat.com> <06b9425507ab8c1b35d377cf9bba155b0cc44147.camel@linux.ibm.com> <3b7899fa-fa52-7652-2d2a-d4ec67ece34d@redhat.com> Message-ID: Date: Tue, 24 Nov 2020 16:58:49 +0100 MIME-Version: 1.0 In-Reply-To: <3b7899fa-fa52-7652-2d2a-d4ec67ece34d@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/24/20 15:54, Laszlo Ersek wrote: > On 11/24/20 09:23, Laszlo Ersek wrote: >> On 11/24/20 07:38, James Bottomley wrote: >>> On Mon, 2020-11-23 at 22:08 +0100, Laszlo Ersek wrote: >> >>>> In [a](5) I requested the removal of everything in this INF file that >>>> was not required for the desired semantics (i.e., for unconditionally >>>> booting the builtin GRUB binary). >>>> >>>> So again I'm unsure if you missed my feedback, or thought it was not >>>> important. (I didn't get a request for clarification either.) >>>> >>>> Keeping INF files minimal is relevant for future contributions. We >>>> frequently need to determine the set of modules that depend on a >>>> particular PCD. If some INF files list PCDs unjustifiedly, then the >>>> affected module set may appear larger than it really is. >>>> >>>> This applies to all sections of the INF file, not just [Pcd]. >>> >>> I did try stripping quite a lot, but then it wouldn't boot. It seems >>> that the PCI devices are needed for grub to find the encrypted volume, >>> so I put most of it back again. Is there some way of identifying >>> superfluous pieces so I can detect if I put too much back? >> >> I suggest proceeding element by element in the INF file (and matching >> that, in the header / C files) -- remove one element per patch. Then you >> can either build+test it as you go, or create a series of micro-patches >> up-front, and if it doesn't boot, bisect it. Finally, squash the >> removals that are justifiable into this patch (for posting), and drop >> the rest. >> >> It's unfortunately trial and error to some extent; however, given that >> each step removes 1 element (PCD, Protocol, GUID, lib class, and finally >> Package), and the number of elements summed over the INF file sections >> is finite, this process is guaranteed to terminate. > > Looking briefly over the file "OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf", I suggest removing: > > - [Packages]: whatever proves unnecessary in the end, > > - [LibraryClasses]: ditto, Perhaps I can make that recommendation / request more detailed too: * drop: - UefiRuntimeServicesTableLib: no use of "gRT" - ReportStatusCodeLib: commit 0a0566d5edad is not relevant, because we justifiedly removed TryRunningQemuKernel() - XenPlatformLib: as discussed before; substitute FALSE for each XenDetected() call, and compress the resultant code * keep: - BaseLib: for CpuDeadLoop() - MemoryAllocationLib: for FreePool() - UefiBootServicesTableLib: for gBS->xxx() - BaseMemoryLib: for CompareMem() - DebugLib: for DEBUG() and ASSERT() - PcdLib: for PcdGet16 (PcdOvmfHostBridgePciDevId) - UefiBootManagerLib: for APIs central to the functionality of PlatformBootManagerLibGrub - BootLogoLib: for BootLogoEnableLogo() - DevicePathLib: for a bunch of device path manipulation - PciLib: mainly for the functions called in PciAcpiInitialization() - UefiLib: for EfiEventGroupSignal() etc - PlatformBmPrintScLib: for PlatformBmPrintScRegisterHandler() -- this is responsible for printing the boot option processing steps to the UEFI console - Tcg2PhysicalPresenceLib: for Tcg2PhysicalPresenceLibProcessRequest() -- we preserve TPM support Then leaving the trimming of [Packages] to the end makes sense -- after trimming everything else, try to remove each package DEC in isolation, and see if the lib instance continues to build. Thanks! Laszlo > > - [Pcd]: PcdOvmfFlashVariablesEnable, PcdPlatformBootTimeOut, > > - [Pcd]: PcdUartDefaultBaudRate, PcdUartDefaultDataBits, PcdUartDefaultParity, PcdUartDefaultStopBits; together with "gXenConsoleDevicePath", "gXenPlatformConsole", XenDetected() / XenPlatformLib, > > - [Pcd.IA32, Pcd.X64]: PcdFSBClock, > > - [Protocols]: gEfiDecompressProtocolGuid, gEfiS3SaveStateProtocolGuid, > > - [Guids]: gEfiGlobalVariableGuid, > > Keep these: > > - [Protocols]: gEfiPciRootBridgeIoProtocolGuid, gEfiDxeSmmReadyToLockProtocolGuid, gEfiLoadedImageProtocolGuid, gEfiFirmwareVolume2ProtocolGuid, > > - [Guids]: gEfiEndOfDxeEventGroupGuid, gRootBridgesConnectedEventGroupGuid, gUefiShellFileGuid, gGrubFileGuid. > > Thanks! > Laszlo >