Re: [PATCH v2 2/6] OvmfPkg/AmdSev: add Grub Firmware Volume Package

["Laszlo Ersek" <lersek@redhat.com>]

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
>