Re: [PATCH] OvmfPkg: Skip initrd command on Xcode toolchain

[Roman Bolshakov <r.bolshakov@yadro.com>]

On Wed, May 13, 2020 at 12:23:33PM +0200, Laszlo Ersek wrote:
> On 05/12/20 22:58, Roman Bolshakov wrote:
> > OVMF booting stops with the assert if built with Xcode on macOS:
> > 
> >   Loading driver at 0x0001FAB8000 EntryPoint=0x0001FABF249 LinuxInitrdDynamicShellCommand.efi
> >   InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 1F218398
> >   ProtectUefiImageCommon - 0x1F218140
> >     - 0x000000001FAB8000 - 0x0000000000008A60
> > 
> >   ASSERT_EFI_ERROR (Status = Unsupported)
> >   ASSERT LinuxInitrdDynamicShellCommand.c(378): !EFI_ERROR (Status)
> > 
> > The assert comes from InitializeHiiPackage() after an attempt to
> > retrieve HII package list from ImageHandle.
> > 
> > Xcode still doesn't support HII resource section and
> > LinuxInitrdDynamicShellCommand depends on it. Likewise 277a3958d93a
> > ("OvmfPkg: Don't include TftpDynamicCommand in XCODE5 tool chain"),
> > disable initrd command if built with Xcode toolchain
> > 
> > Fixes: 2632178bc683 ("OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path")
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Andrew Fish <afish@apple.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > ---
> >  OvmfPkg/OvmfPkgIa32.fdf | 2 +-
> >  OvmfPkg/OvmfPkgX64.fdf  | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> Thanks for the patch!
> 
> The reference to commit 277a3958d93a is very welcome.
> 
> (1) However, this patch doesn't follow commit 277a3958d93a closely
> enough. The present patch only conditionalizes some FDF files. We should
> conditionalize the DSC files too, otherwise we're still going to build
> LinuxInitrdDynamicShellCommand, just not include it in the firmware.
> 
> (2) The "Fixes:" reference is incorrect. Technically, the issue (for
> XCODE5) was introduced in the commit that modified the FDF file(s).
> Therefore, based on "git-blame", we should have
> 
> Fixes: ec41733cfd10
> 
> Now, if you look at that commit -- i.e., "OvmfPkg: add the 'initrd'
> dynamic shell command" --, then not only will you find the parts in the
> DSC files that are necessary to gate (per my point (1) above), you'll
> also notice my next point:
> 
> (3) The present patch only covers 2 out of the 4 OvmfPkg platforms;
> namely, "OvmfPkgIa32X64" and "OvmfXen" are missed.
> 
> Please extend the patch to those platforms as well.
> 
> In total, the patch should modify 8 files:
> 
> OvmfPkg/OvmfPkgIa32.dsc
> OvmfPkg/OvmfPkgIa32.fdf
> OvmfPkg/OvmfPkgIa32X64.dsc
> OvmfPkg/OvmfPkgIa32X64.fdf
> OvmfPkg/OvmfPkgX64.dsc
> OvmfPkg/OvmfPkgX64.fdf
> OvmfPkg/OvmfXen.dsc
> OvmfPkg/OvmfXen.fdf
> 
> because they all reference
> "OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf".
> 

Hi Laszlo,

Thanks for the feedback and pointing me out to other places I missed.
I will address the issues shortly.

Regards,
Roman