Re: [patch 1/7] ArmVirtPkg: Update UefiDecompressLib instance

["Dandan Bi" <dandan.bi@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, April 04, 2019 10:58 PM
> To: Bi, Dandan <dandan.bi@intel.com>; devel@edk2.groups.io
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Julien Grall
> <julien.grall@arm.com>
> Subject: Re: [patch 1/7] ArmVirtPkg: Update UefiDecompressLib instance
> 
> On 04/04/19 15:52, Dandan Bi wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1683
> >
> > BaseUefiDecompressLib in MdePkg is the base UEFI decompress Library.
> > BaseUefiTianoCustomDecompressLib in IntelFrameworkModulePkg
> implements
> > the base UEFI decompress functionality and Tiano decompress
> > functionality.
> >
> > 1. TIANOCOMPRESSED rule in ArmVirtRules.fdf.inc is not used, so remove
> > it.
> 
> That's correct, the only FDF files that refer to any TIANOCOMPRESSED rules,
> with RuleOverride, are:
> 
> - Nt32Pkg.fdf (for EnglishDxe.inf)
> - Quark.fdf (for Shell.inf)
> 
> And the GUID in the rule being removed, A31280AD-481E-41B6-95E8-
> 127F4C984779, is indeed gTianoCustomDecompressGuid.
> 
> > 2. Platform doesn't use the TianoCompress, so do not have to use
> > BaseUefiTianoCustomDecompressLib, can use the BaseUefiDecompressLib
> in
> > MdePkg directly.
> 
> It looks like we have the following guid-ed section extractors in edk2:
> 
>   BrotliCustomDecompress          3D532050-5CDA-4FD0-879E-
> 0F7F630D5AFB
>   EfiCertTypeRsa2048Sha256        A7717414-C616-4977-9420-
> 844712A735BF
>   EfiCrc32GuidedSectionExtraction FC1BCDB0-7D31-49AA-936A-
> A4600D9DD083
>   LzmaCustomDecompress            EE4E5898-3914-4259-9D6E-
> DC7BD79403CF
>   LzmaF86CustomDecompress         D42AE6BD-1352-4BFB-909A-
> CA72A6EAE889
>   TianoCustomDecompress           A31280AD-481E-41B6-95E8-
> 127F4C984779
> 
> (with the leading "g" and trailing "Guid" stripped).
> 
> In order to see whether a platform uses an algorithm for section encoding,
> we should grep that platform's FDF files for the algorithm's GUID directly...
> And, indeed, under ArmVirtPkg, the only mention of A31280AD-481E-41B6-
> 95E8-127F4C984779 is in the (unused) rule that's being removed.
> 
> The one algorithm that ArmVirtPkg does use, for section encoding, is
> LzmaCustomDecompress. For that, we have
> "MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecomp
> ressLib.inf" resolutions in place already.
> 
> So what needs UefiDecompressLib at all? ... Aha, both of these:
> - MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> - MdeModulePkg/Core/Dxe/DxeMain.inf
> 
> That's a good argument for preserving the lib class resolution at all.
> 
> I've compared "BaseUefiTianoCustomDecompressLib.c" to
> "BaseUefiDecompressLib.c". The main differences are:
> 
> * the latter has no constructor that registers a guid-ed section extractor --
> and that's fine here, because in the code modified by this patch, the lib
> instance is not hooked into another module via NULL resolution,
> 
> * the following hunk:
> 
>    //
>    // The length of the field 'Position Set Code Length Array Size' in Block
> Header.
>    // For UEFI 2.0 de/compression algorithm(Version 1), mPBit = 4
> -  // For Tiano de/compression algorithm(Version 2), mPBit = 5
>    //
> -  switch (Version) {
> -    case 1 :
> -      Sd->mPBit = 4;
> -      break;
> -    case 2 :
> -      Sd->mPBit = 5;
> -      break;
> -    default:
> -      ASSERT (FALSE);
> -  }
> +  Sd->mPBit     = 4;
> 
> So, let's see the patch:
> 
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Julien Grall <julien.grall@arm.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> > ---
> >  ArmVirtPkg/ArmVirt.dsc.inc      | 4 ++--
> >  ArmVirtPkg/ArmVirtRules.fdf.inc | 9 ---------
> >  2 files changed, 2 insertions(+), 11 deletions(-)
> >
> > diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> > index d172a082c9..556ec0dcb1 100644
> > --- a/ArmVirtPkg/ArmVirt.dsc.inc
> > +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> > @@ -223,17 +223,17 @@
> >
> SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagement
> Lib/DxeSecurityManagementLib.inf
> >
> PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerforma
> nceLib.inf
> >
> >
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMem
> oryA
> > llocationLib.inf
> >
> >  [LibraryClasses.common.UEFI_APPLICATION]
> > -
> >
> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCust
> omD
> > ecompressLib/BaseUefiTianoCustomDecompressLib.inf
> > +
> > +
> UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDeco
> m
> > + pressLib.inf
> >
> PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerforma
> nceLib.inf
> >
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMem
> oryAllocationLib.inf
> >    HiiLib|MdeModulePkg/Library/UefiHiiLib/UefiHiiLib.inf
> >
> >  [LibraryClasses.common.UEFI_DRIVER]
> > -
> >
> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCust
> omD
> > ecompressLib/BaseUefiTianoCustomDecompressLib.inf
> > +
> > +
> UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDeco
> m
> > + pressLib.inf
> >
> ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/Dxe
> ExtractGuidedSectionLib.inf
> >
> PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerforma
> nceLib.inf
> >
> >
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMem
> oryA
> > llocationLib.inf
> >
> >  [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> > diff --git a/ArmVirtPkg/ArmVirtRules.fdf.inc
> > b/ArmVirtPkg/ArmVirtRules.fdf.inc index 5ff3004786..bed06364e1
> 100644
> > --- a/ArmVirtPkg/ArmVirtRules.fdf.inc
> > +++ b/ArmVirtPkg/ArmVirtRules.fdf.inc
> > @@ -56,19 +56,10 @@
> >       PEI_DEPEX PEI_DEPEX Optional
> $(INF_OUTPUT)/$(MODULE_NAME).depex
> >       TE       TE Align = Auto           $(INF_OUTPUT)/$(MODULE_NAME).efi
> >       UI       STRING="$(MODULE_NAME)" Optional
> >    }
> >
> > -[Rule.Common.PEIM.TIANOCOMPRESSED]
> > -  FILE PEIM = $(NAMED_GUID) DEBUG_MYTOOLS_IA32 {
> > -    PEI_DEPEX PEI_DEPEX Optional
> $(INF_OUTPUT)/$(MODULE_NAME).depex
> > -    GUIDED A31280AD-481E-41B6-95E8-127F4C984779
> PROCESSING_REQUIRED = TRUE {
> > -      PE32      PE32                    $(INF_OUTPUT)/$(MODULE_NAME).efi
> > -      UI        STRING="$(MODULE_NAME)" Optional
> > -    }
> > -  }
> > -
> >  [Rule.Common.DXE_CORE]
> >    FILE DXE_CORE = $(NAMED_GUID) {
> >      PE32     PE32                       $(INF_OUTPUT)/$(MODULE_NAME).efi
> >      UI       STRING="$(MODULE_NAME)" Optional
> >    }
> >
> 
> The patch seems good, but we could improve it: in "ArmVirtPkg", we
> already have the following UefiDecompressLib resolutions:
> 
> ArmVirt.dsc.inc:44:
> UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDeco
> mpressLib.inf
> ArmVirt.dsc.inc:192:
> UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDeco
> mpressLib.inf
> ArmVirt.dsc.inc:208:
> UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDeco
> mpressLib.inf
> ArmVirt.dsc.inc:219:
> UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDeco
> mpressLib.inf
> ArmVirt.dsc.inc:228:
> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCust
> omDecompressLib/BaseUefiTianoCustomDecompressLib.inf
> ArmVirt.dsc.inc:234:
> UefiDecompressLib|IntelFrameworkModulePkg/Library/BaseUefiTianoCust
> omDecompressLib/BaseUefiTianoCustomDecompressLib.inf
> 
> On line 44, the resolution is for [LibraryClasses.common]. So, rather than
> multiplying the same resolution for UEFI_APPLICATION and UEFI_DRIVER
> too, I suggest removing *all* the resolutions, except the one on line 44 (i.e.,
> in section [LibraryClasses.common]).

Thanks for your suggestion.
I had planned to leave one UefiDecompressLib resolution in common section before, but then gave up since didn't want to touch too much platform update. 
I will cover this update in V2.

Thanks,
Dandan
> 
> The commit message needs a minimal change only -- please append a
> sentence about the default resolution being right already, and relying on
> that for all module types.
> 
> The "ArmVirtRules.fdf.inc" hunk is good as-is, of course.
> 
> Thanks!
> Laszlo