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

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

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.

Working in the other direction is much simpler, because the compiler
and/or the "build" tool will tell you if something is missing. Alas,
they don't tell us when something is listed superfluously. (The same
problem applies to #include directives in any C project -- I'm unaware
of any tool that flags a superfluous #include directive as such.)


>> (13) the indentation seems strange (Linux kernel style?); please
>> don't use hardware tabs. (Hmmm, applies to other parts of this script
>> too.)
> 
> OK, I got emacs to untabify it.  I did try converting it to DOS format
> like the rest of edk2 but bash really doesn't like that:
> 
> /home/jejb/git/edk2/OvmfPkg/AmdSev/Grub/grub.sh: line 10: $'\r':
> command not found

Yup, "grub.cfg" and "grub.sh" should use LF line terminators, not CRLF.

If you use 8bit or base64 Content-Transfer-Encoding with git-send-email,
it should be possible to send a patch that contains hunks with both LF
and CRLF line terminators.

This CRLF mess is a long-standing problem in edk2, and it has trained me
to look for files such as "grub.cfg" and "grub.sh" in patches. I tend to
verify them for LF manually, and if they appear to have CRLF after I
apply them locally -- which may or may not mean that they had CRLF on
the contributor's side too --, then I push them through dos2unix first
(rebasing the series).


... apologies that I sounded irritated earlier; it had been another
hectic day.

Thanks!
Laszlo