Re: [edk2-devel] [PATCH v1 5/5] ArmVirtPkg: Enable Acpiview for ArmVirtPkg

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

On 06/23/21 16:06, PierreGondois wrote:
> From: Sami Mujawar <sami.mujawar@arm.com>
> 
> Acpiview is a command line tool allowing to display, dump, or
> check installed ACPI tables. Add the tool to ArmVirt platforms.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index d9abadbe708c..269ac4990a6c 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -1,5 +1,5 @@
>  #
> -#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +#  Copyright (c) 2011-2021, Arm Limited. All rights reserved.
>  #  Copyright (c) 2014, Linaro Limited. All rights reserved.
>  #  Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.
>  #  Copyright (c) Microsoft Corporation.
> @@ -398,6 +398,7 @@ [Components.common]
>        NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.inf
>        NULL|ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib.inf
>        NULL|ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf
> +      NULL|ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
>        NULL|ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInstall1CommandsLib.inf
>        NULL|ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.inf
>  !if $(NETWORK_IP6_ENABLE) == TRUE
> 

I disagree with this patch, as it will cause the Shell binary in all
ArmVirtPkg platforms to include the (rather large) ACPIVIEW command.

ACPIVIEW is super useful for when the tables are (dynamically) generated
by the firmware itself, but that does not apply to the Qemu and Xen
platforms.

Note NETWORK_IP6_ENABLE: UefiShellNetwork2CommandsLib is only hooked
into the shell application if NETWORK_IP6_ENABLE is TRUE.

Please add

  DEFINE ACPIVIEW_ENABLE                 = TRUE

to "ArmVirtPkg/ArmVirtKvmTool.dsc", and in "ArmVirtPkg/ArmVirt.dsc.inc",
include the new command lib conditionally on ACPIVIEW_ENABLE being TRUE.
(Can be in the same patch.)

Acked-by: Laszlo Ersek <lersek@redhat.com>


Thanks
Laszlo