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

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

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,

- [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