Re: [edk2-devel] [PATCH v2 6/6] ShellPkg/AcpiView: Add application wrapper

["Tomas Pilar (tpilar)" <Tomas.Pilar@arm.com>]

Thank you Zhichao, this is really useful, I was wondering how to hook into the shell help framework. 

Regarding the lower case const and static, I am entirely happy to change them, but I have a couple of patch series outstanding and don't want to add unnecessary merge conflicts, so I'll do a single formatting patch at the end that changes all instances of const and static in the UefiShellAcpiViewCommandLib and one for AcpiViewApp. I hope that is acceptable.

I am respinning the patches now.

Cheers,
Tom

-----Original Message-----
From: Gao, Zhichao <zhichao.gao@intel.com> 
Sent: 12 June 2020 03:42
To: devel@edk2.groups.io; Tomas Pilar <Tomas.Pilar@arm.com>
Cc: nd <nd@arm.com>; Ni, Ray <ray.ni@intel.com>
Subject: RE: [edk2-devel] [PATCH v2 6/6] ShellPkg/AcpiView: Add application wrapper

(1)
There is one more easy with to add the help info.
Put the help string in the uni file that has the same name with the module file name.
See below example from TftpApp.inf:

//
// String token ID of help message text.
// Shell supports to find help message in the resource section of an application image if // .MAN file is not found. This global variable is added to make build tool recognizes // that the help string is consumed by user and then build tool will add the string into // the resource section. Thus the application can use '-?' option to show help message in // Shell.
//
GLOBAL_REMOVE_IF_UNREFERENCED EFI_STRING_ID mStringHelpTokenId = STRING_TOKEN (STR_GET_HELP_TFTP);

(2)
I suggest to add the UefiShellAcpiViewCommandLib header file to make it visible for other module. The application support to build in other module. It is better to put the lib visible in the inf config file. Consumers can be clearly aware of which libs are required. When introduce new lib:
  a. add header file for the published interface
  b. add lib & header file in the package dec file
  c. add lib in the package dsc file [Components] for build only
  d. add the lib in the [LibraryClasses] for the package that would consume the lib

(3)
One white space in the end of line. It would break the Patchchecker.py. See below.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Tomas 
> Pilar
> (tpilar)
> Sent: Friday, May 22, 2020 2:44 AM
> To: devel@edk2.groups.io
> Cc: nd@arm.com; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao 
> <zhichao.gao@intel.com>
> Subject: [edk2-devel] [PATCH v2 6/6] ShellPkg/AcpiView: Add 
> application wrapper
> 
> Simple application wrapper that invokes the shell command wrapper for acpiview.
> 
> This allows the AcpiView functionality to be used on platforms with 
> older specifications of the UEFI shell or where the 'acpiview' command 
> is not built in due to platform build configuration.
> 
> Furthermore, this app can be integrated into more comprehensive 
> testing frameworks as a single component of a more thorough 
> specification compliance validation strategy.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Signed-off-by: Tomas Pilar <tomas.pilar@arm.com>
> ---
>  ShellPkg/Application/AcpiViewApp/AcpiViewApp.c   | 189
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++
>  ShellPkg/Application/AcpiViewApp/AcpiViewApp.inf |  31 ++++++++++++
>  ShellPkg/ShellPkg.dsc                            |   6 ++-
>  3 files changed, 225 insertions(+), 1 deletion(-)
> 
> diff --git a/ShellPkg/Application/AcpiViewApp/AcpiViewApp.c
> b/ShellPkg/Application/AcpiViewApp/AcpiViewApp.c
> new file mode 100644
> index 000000000000..f83111629c21
> --- /dev/null
> +++ b/ShellPkg/Application/AcpiViewApp/AcpiViewApp.c
> @@ -0,0 +1,189 @@
> +/** @file
> +  Main file for AcpiViewApp application
> +
> +  Copyright (c) 2020, ARM Limited. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> +
> +#include <Library/BaseLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
> +
> +#include <Protocol/ShellParameters.h>
> +
> +/**
> +  Execute the AcpiView command from UefiShellAcpiViewCommandLib.
> +  This function is pulled in directly from the library source.
> +**/
> +SHELL_STATUS
> +EFIAPI
> +ShellCommandRunAcpiView (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE* SystemTable
> +  );
> +
> +CHAR16 mAcpiViewAppHelp[] =
> +  u"\r\n"
> +  "Display ACPI Table information.\r\n"
> +  "\r\n"
> +  "AcpiViewApp.efi [[-?] | [[-l] | [-s AcpiTable [-d]]] [-q] [-h]]\r\n"
> +  " \r\n"
> +  "  -l - Display list of installed ACPI Tables.\r\n"
> +  "  -s - Display only the specified AcpiTable type and only support single\r\n"
> +  "       invocation option.\r\n"
> +  "         AcpiTable    : The required ACPI Table type.\r\n"
> +  "  -d - Generate a binary file dump of the specified AcpiTable.\r\n"
> +  "  -q - Quiet. Suppress errors and warnings. Disables consistency checks.\r\n"
> +  "  -h - Enable colour highlighting.\r\n"
> +  "  -? - Show help.\r\n"
> +  " \r\n"
> +  "  This program is provided to allow examination of ACPI table values from "
> +  "the\r\n"
> +  "  UEFI Shell. This can help with investigations, especially at that "
> +  "stage\r\n"
> +  "  where the tables are not enabling an OS to boot.\r\n"
> +  "  The program is not exhaustive, and only encapsulates detailed knowledge "
> +  "of a\r\n"
> +  "  limited number of table types.\r\n"
> +  " \r\n"
> +  "  Default behaviour is to display the content of all tables installed.\r\n"
> +  "  'Known' table types (listed in NOTES below) will be parsed and "
> +  "displayed\r\n"
> +  "  with descriptions and field values. Where appropriate a degree of\r\n"
> +  "  consistency checking is done and errors may be reported in the output.\r\n"
> +  "  Other table types will be displayed as an array of Hexadecimal bytes.\r\n"
> +  " \r\n"
> +  "  To facilitate debugging, the -s and -d options can be used to generate "
> +  "a\r\n"
> +  "  binary file image of a table that can be copied elsewhere for "
> +  "investigation\r\n"
> +  "  using tools such as those provided by acpica.org. This is especially\r\n"
> +  "  relevant for AML type tables like DSDT and SSDT.\r\n"
> +  " \r\n"
> +  "NOTES:\r\n"
> +  "  1. The AcpiTable parameter can match any installed table type.\r\n"
> +  "     Tables without specific handling will be displayed as a raw hex dump "
> +  "(or\r\n"
> +  "     dumped to a file if -d is used).\r\n"
> +  "  2. -s option supports to display the specified AcpiTable type that is "
> +  "present\r\n"
> +  "     in the system. For normal type AcpiTable, it would display the data of "
> +  "the\r\n"
> +  "     AcpiTable and AcpiTable header. The following type may contain header "
> +  "type\r\n"
> +  "     other than AcpiTable header. The actual header can refer to the ACPI "
> +  "spec\r\n"
> +  "     6.3\r\n"
> +  "     Extra A. Particular types:\r\n"
> +  "       APIC  - Multiple APIC Description Table (MADT)\r\n"
> +  "       BGRT  - Boot Graphics Resource Table\r\n"
> +  "       DBG2  - Debug Port Table 2\r\n"
> +  "       DSDT  - Differentiated System Description Table\r\n"
> +  "       FACP  - Fixed ACPI Description Table (FADT)\r\n"
> +  "       GTDT  - Generic Timer Description Table\r\n"
> +  "       IORT  - IO Remapping Table\r\n"
> +  "       MCFG  - Memory Mapped Config Space Base Address Description
> Table\r\n"
> +  "       PPTT  - Processor Properties Topology Table\r\n"
> +  "       RSDP  - Root System Description Pointer\r\n"
> +  "       SLIT  - System Locality Information Table\r\n"
> +  "       SPCR  - Serial Port Console Redirection Table\r\n"
> +  "       SRAT  - System Resource Affinity Table\r\n"
> +  "       SSDT  - Secondary SystemDescription Table\r\n"
> +  "       XSDT  - Extended System Description Table\r\n"
> +  " \r\n"
> +  "  Table details correspond to those in 'Advanced Configuration and Power\r\n"
> +  "  Interface Specification' Version 6.3 [January 2019]\r\n"
> +  "  (https://uefi.org/specifications)\r\n"
> +  "  "
> +  " \r\n"
> +  "  NOTE: The nature of the ACPI standard means that almost all tables in "
> +  "6.1\r\n"
> +  "        will be 'backwards compatible' with prior version of the "
> +  "specification\r\n"
> +  "        in terms of structure, so formatted output should be correct. The "
> +  "main\r\n"
> +  "        exception will be that previously 'reserved' fields will be "
> +  "reported\r\n"
> +  "        with new names, where they have been added in later versions of "
> +  "the\r\n"
> +  "        specification.\r\n"
> +  " \r\n"
> +  " \r\n"
> +  "EXAMPLES:\r\n"
> +  "  * To display a list of the installed table types:\r\n"
> +  "    fs0:\\> AcpiViewApp.efi -l\r\n"
> +  " \r\n"
> +  "  * To parse and display a specific table type:\r\n"
> +  "    fs0:\\> AcpiViewApp.efi -s GTDT\r\n"
> +  " \r\n"
> +  "  * To save a binary dump of the contents of a table to a file\r\n"
> +  "    in the current working directory:\r\n"
> +  "    fs0:\\> AcpiViewApp.efi -s DSDT -d\r\n"
> +  " \r\n"
> +  "  * To display contents of all ACPI tables:\r\n"
> +  "    fs0:\\> AcpiViewApp.efi\r\n"
> +  " \r\n";
> +
> +/**
> +  Determine if the user wants to display by checking for presence
> +  of '/?' or '--help' on command line. We cannot override '-?' shell
> +  command line handling.
> +
> +  @retval EFI_SUCCESS             No help was printed
> +  @retval EFI_INVALID_PARAMETER   Help was printed
> +**/
> +STATIC
> +EFI_STATUS
> +CheckForHelpRequest (
> +  EFI_HANDLE ImageHandle
> +  )
> +{
> +  EFI_STATUS                    Status;
> +  EFI_SHELL_PARAMETERS_PROTOCOL *ShellParameters;
> +  UINTN                         Index;
> +
> +  Status = gBS->HandleProtocol (
> +    ImageHandle,

Please remove the white space in the end of above line.

Thanks,
Zhichao

> +    &gEfiShellParametersProtocolGuid,
> +    (VOID **)&ShellParameters
> +    );
> +  if (EFI_ERROR(Status)) {
> +    return Status;
> +  }
> +
> +  for (Index = 1; Index < ShellParameters->Argc; Index++) {
> +    if ((StrCmp (ShellParameters->Argv[Index], L"/?") == 0) ||
> +        (StrCmp (ShellParameters->Argv[Index], L"--help") == 0))  {
> +      Print (mAcpiViewAppHelp);
> +      return EFI_INVALID_PARAMETER;
> +    }
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +
> +/**
> +  Application Entry Point wrapper around the shell command
> +
> +  @param[in] ImageHandle  Handle to the Image (NULL if internal).
> +  @param[in] SystemTable  Pointer to the System Table (NULL if internal).
> +**/
> +EFI_STATUS
> +EFIAPI
> +AcpiViewAppMain (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  Status = CheckForHelpRequest (ImageHandle);
> +
> +  // Do not run code if help was printed  if (Status !=
> + EFI_INVALID_PARAMETER) {
> +    Status = ShellCommandRunAcpiView (gImageHandle, SystemTable);  }
> +
> +  return Status;
> +}
> diff --git a/ShellPkg/Application/AcpiViewApp/AcpiViewApp.inf
> b/ShellPkg/Application/AcpiViewApp/AcpiViewApp.inf
> new file mode 100644
> index 000000000000..28e724fa1909
> --- /dev/null
> +++ b/ShellPkg/Application/AcpiViewApp/AcpiViewApp.inf
> @@ -0,0 +1,31 @@
> +##  @file
> +#  EFI application that displays and verifies ACPI tables # # 
> +Copyright (c) 2020, ARM Limited. All rights reserved.<BR> # #
> +SPDX-License-Identifier: BSD-2-Clause-Patent # # ##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001B
> +  BASE_NAME                      = AcpiViewApp
> +  FILE_GUID                      = 46361B5B-AF17-41FF-95F9-E1BCE08435B9
> +  MODULE_TYPE                    = UEFI_APPLICATION
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = AcpiViewAppMain
> +
> +[Sources.common]
> +  AcpiViewApp.c
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  ShellPkg/ShellPkg.dec
> +
> +[LibraryClasses]
> +  UefiBootServicesTableLib
> +  UefiLib
> +  BaseLib
> +  UefiApplicationEntryPoint
> diff --git a/ShellPkg/ShellPkg.dsc b/ShellPkg/ShellPkg.dsc index
> b7ee856b3a16..1ed82ee7aca3 100644
> --- a/ShellPkg/ShellPkg.dsc
> +++ b/ShellPkg/ShellPkg.dsc
> @@ -2,7 +2,7 @@
>  # Shell Package
>  #
>  # Copyright (c) 2007 - 2019, Intel Corporation. All rights 
> reserved.<BR> -# Copyright (c) 2018, Arm Limited. All rights 
> reserved.<BR>
> +# Copyright (c) 2018 - 2020, Arm Limited. All rights reserved.<BR>
>  # Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All 
> rights reserved.<BR>  #
>  #    SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -143,6 +143,10 @@ [Components]
>        gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
>    }
>    ShellPkg/DynamicCommand/DpDynamicCommand/DpApp.inf
> +  ShellPkg/Application/AcpiViewApp/AcpiViewApp.inf {
> +    <LibraryClasses>
> +
> + NULL|ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewC
> + NULL|om
> + mandLib.inf
> +  }
> 
>  [BuildOptions]
>    *_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
> --
> 2.24.1.windows.2
> 
> 
> 
>