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

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

On 06/24/21 14:59, Laszlo Ersek wrote:
> 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",

To clarify: please place

  DEFINE ACPIVIEW_ENABLE                 = TRUE

in a new [Defines.AARCH64] section in "ArmVirtPkg/ArmVirtKvmTool.dsc",
not in the existent [Defines] section.

This should happen just before !including "ArmVirtPkg/ArmVirt.dsc.inc".

Thanks
Laszlo

> 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
>