Re: [PATCH v4 2/8] ArmVirtPkg: add SecureBootVariableLib class resolution

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

On 06/30/21 14:34, Grzegorz Bernacki wrote:
> The edk2 patch
>   SecurityPkg: Create library for setting Secure Boot variables.
> 
> removes generic functions from SecureBootConfigDxe and places
> them into SecureBootVariableLib. This patch adds SecureBootVariableLib
> mapping for ArmVirtPkg platform.
> 
> Signed-off-by: Grzegorz Bernacki <gjb@semihalf.com>
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index d9abadbe70..11c1f53537 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -168,6 +168,7 @@
>    #
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>    AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> +  SecureBootVariableLib|SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.inf
>  
>    # re-use the UserPhysicalPresent() dummy implementation from the ovmf tree
>    PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf
> 

I know a new version is coming up, but one comment still:

you should please make this series bisectable. That is, the series
should build at every stage.

That implies the following approach (each step below corresponds to a
patch, or to a sequence of patches):

- introduce the new library (class and instance(s)) first, in isolation;
this will duplicate the internal functions of SecureBootConfigDxe

- add lib class resolution(s) to all platforms in edk2 (and
edk2-platforms, possibly) that include SecureBootConfigDxe

- replace the internal functions of SecureBootConfigDxe with the new
library dependency.

Right now, ArmVirtPkg platforms will definitely not build against your
patch set applied up to and including only patch#1, because at patch#1,
SecureBootConfigDxe depends on SecureBootVariableLib, but ArmVirtPkg
doesn't yet resolve that lib class to any instance.

Also, I don't see any OvmfPkg patch in the series... hm, well, there are
OvmfPkg modifications, but they have been squashed into patch#3, "Intel
Platforms: add SecureBootVariableLib class resolution".

Regardless of whether we call OvmfPkg an "Intel Platform" -- I wouldn't,
BTW --, OvmfPkg DSC updates need to go in their own, isolated patch.
Same for EmulatorPkg -- separate patch please.

Thanks,
Laszlo