Re: [edk2-devel] [PATCH v3 0/6] SEV Encrypted Boot for Ovmf

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

On 12/04/20 02:05, James Bottomley wrote:
> On Fri, 2020-12-04 at 01:46 +0100, Laszlo Ersek wrote:
>> On 12/03/20 15:27, James Bottomley wrote:
>>> On Thu, 2020-12-03 at 13:26 +0100, Laszlo Ersek wrote:
>>>> Hi James,
>>>>
>>>> On 11/30/20 21:28, James Bottomley wrote:
>>>>> v3:
>>>>>
>>>>> - More grub and boot stripping (I think I got everything out,
>>>>> but there may be something that strayed in the boot panic
>>>>> resolution).
>>>>> - grub.sh tidy up with tabs->spaces.
>>>>> - Move the reset vector GUIDisation patch to the front so it
>>>>> can be applied independently
>>>>> - Update the .dsc and .fdf files for variable policy
>>>>
>>>> In preparation for submitting the github PR for merging this
>>>> series, I first ran PatchCheck.py locally.
>>>>
>>>> It doesn't like that I converted "OvmfPkg/AmdSev/Grub/grub.cfg"
>>>> to LF, in addition to "OvmfPkg/AmdSev/Grub/grub.sh".
>>>>
>>>> PatchCheck.py recognizes the ".sh" suffix, so it's not
>>>> complaining about "OvmfPkg/AmdSev/Grub/grub.sh". But, the config
>>>> file is a problem.
>>>>
>>>> Can you please confirm that grub works fine if the config file
>>>> has CRLF line terminators? Because then I'll just convert the
>>>> config file back to CRLF, and merge the series.
>>>
>>> I converted the entire file with unix2dos and grub does seem to be
>>> fine with the CRLF line endings (probably because it wants to be a
>>> boot loader beyond linux), so I think converting the file to CRLF
>>> is the right way to go.
>>
>> I submitted PR <https://github.com/tianocore/edk2/pull/1175>;, but it
>> was rejected due to ECC failures.
>>
>> I think we all need to have a serious talk about ECC. I'll send a
>> separate email.
>>
>> I'm really sorry.
>
> Is it just complaining that gGrubFileGuid is the same as the FILE_GUID
> in Grub.inf (as the comment in Grub.inf says) but it shouldn't be, so
> the fix is to remove the comment and generate a new FILE_GUID for
> Grub.inf?
>
> James
>
>

Log files (same content, modulo timestamps etc):

  https://dev.azure.com/tianocore/11ea4a10-ac9f-4e5f-8b13-7def1f19d478/_apis/build/builds/16071/logs/115
  https://dev.azure.com/tianocore/11ea4a10-ac9f-4e5f-8b13-7def1f19d478/_apis/build/builds/16072/logs/113

Two plugins failed (search either log for the string "Test Failed" --
there's going to be two instances).

(1) The GUID check plugin is one of both failures.

It's unjustified of course, because you *need* to know the FILE_GUID of
Grub.inf -- that's how you can generate boot option to it in the
Platform BDS library. Making gGrubFileGuid different from what's in
FILE_GUID would *break the code*.

One hack for this is to change the FILE_GUID in Grub.inf (regenerate it
with uuidgen), but then use a DSC-level FILE_GUID override, to make it
match gGrubFileGuid from the DEC file again:

> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index bb7697eb324b..1593856faca1 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -779,7 +779,10 @@ [Components]
>    }
>  !endif
>    OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> -  OvmfPkg/AmdSev/Grub/Grub.inf
> +  OvmfPkg/AmdSev/Grub/Grub.inf {
> +    <Defines>
> +      FILE_GUID = ...
> +  }
>  !if $(BUILD_SHELL) == TRUE
>    ShellPkg/Application/Shell/Shell.inf {
>      <LibraryClasses>

But it's terrible for code that's actually valid.

So instead, we should add an exception to "OvmfPkg/OvmfPkg.ci.yaml",
near "GuidCheck".

(2) The other failure is from ECC.

I've evaluated the errors it has reported now, and all of them are
unjustifiedly reported.

Again, more exceptions are needed in "OvmfPkg/OvmfPkg.ci.yaml", this
time near "EccCheck".

I will send a short patch series to add the exceptions, and once that's
upstream, we *will* merge this (v3) series.

Thanks
Laszlo