Re: [edk2-devel] [PATCH v1 3/3] ShellPkg: UefiShellDebug1CommandsLib: Conformance Profiles in Dmem.c

["Stuart Yoder" <stuart.yoder@arm.com>]

Hi Sam,

See inline comments...

On 1/24/24 2:56 PM, Sam Kaynor wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4352
> 
> Implemented dumping of the UEFI Conformance Profiles Table using Dmem.c
> Additionally added the base support for the table with new
> header file ConformanceProfiles.h (Cc'd maintainers of MdePkg for this)
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Signed-off-by: Sam Kaynor <Sam.Kaynor@arm.com>
> ---
>   MdePkg/MdePkg.dec                                                          |  7 ++
>   ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf |  3 +
>   MdePkg/Include/Guid/ConformanceProfiles.h                                  | 57 +++++++++++++++
>   ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c                         | 76 ++++++++++++++++++++
>   ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni |  5 ++
>   5 files changed, 148 insertions(+)
> 
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index 2ee112cc087a..d22320d7bdae 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -740,6 +740,13 @@ [Guids]
>     ## Include/Protocol/SerilaIo.h
>     gEfiSerialTerminalDeviceTypeGuid = { 0x6AD9A60F, 0x5815, 0x4C7C, { 0x8A, 0x10, 0x50, 0x53, 0xD2, 0xBF, 0x7A, 0x1B }}
>   
> +  # GUIDs defined in UEFI 2.10
> +  #
> +  ## Include/Guid/ConformanceProfiles.h
> +  gEfiConfProfilesTableGuid        = { 0x36122546, 0xf7e7, 0x4c8f, { 0xbd, 0x9b, 0xeb, 0x85, 0x25, 0xb5, 0x0c, 0x0b }}
> +  gEfiConfProfilesUefiSpecGuid     = { 0x523c91af, 0xa195, 0x4382, { 0x81, 0x8d, 0x29, 0x5f, 0xe4, 0x00, 0x64, 0x65 }}
> +  gEfiConfProfilesEbbrSpecGuid     = { 0xcce33c35, 0x74ac, 0x4087, { 0xbc, 0xe7, 0x8b, 0x29, 0xb0, 0x2e, 0xeb, 0x27 }}
> +
>     #
>     # GUID defined in PI1.0
>     #
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf
> index 3741dac5d94c..172ac2862ba1 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf
> @@ -139,3 +139,6 @@ [Guids]
>     gEfiJsonConfigDataTableGuid     ## SOMETIMES_CONSUMES ## SystemTable
>     gEfiJsonCapsuleDataTableGuid    ## SOMETIMES_CONSUMES ## SystemTable
>     gEfiJsonCapsuleResultTableGuid  ## SOMETIMES_CONSUMES ## SystemTable
> +  gEfiConfProfilesTableGuid       ## SOMETIMES_CONSUMES ## SystemTable
> +  gEfiConfProfilesUefiSpecGuid    ## SOMETIMES_CONSUMES ## GUID
> +  gEfiConfProfilesEbbrSpecGuid    ## SOMETIMES_CONSUMES ## GUID
> diff --git a/MdePkg/Include/Guid/ConformanceProfiles.h b/MdePkg/Include/Guid/ConformanceProfiles.h
> new file mode 100644
> index 000000000000..88517eaaad25
> --- /dev/null
> +++ b/MdePkg/Include/Guid/ConformanceProfiles.h
> @@ -0,0 +1,57 @@
> +/** @file
> +  Legal information
> +
> +**/
> +
> +#ifndef __CONFORMANCE_PROFILES_TABLE_GUID_H__
> +#define __CONFORMANCE_PROFILES_TABLE_GUID_H__
> +
> +
> +//
> +// (This is copied straight from the 2.10 spec, to be modified)

Is the "to be modified" comment supposed to be here?

> +// This table allows the platform to advertise its UEFI specification conformance
> +// in the form of pre-defined profiles. Each profile is identified by a GUID, with
> +// known profiles listed in the section below.
> +// The absence of this table shall indicate that the platform implementation is
> +// conformant with the UEFI specification requirements, as defined in Section 2.6.
> +// This is equivalent to publishing this configuration table with the
> +// EFI_CONFORMANCE_PROFILES_UEFI_SPEC_GUID conformance profile.
> +//
> +#define EFI_CONFORMANCE_PROFILES_TABLE_GUID \
> +  { \
> +    0x36122546, 0xf7e7, 0x4c8f, { 0xbd, 0x9b, 0xeb, 0x85, 0x25, 0xb5, 0x0c, 0x0b } \
> +  }
> +
> +#pragma pack(1)
> +
> +typedef struct {
> +  ///
> +  /// Version of the table must be 0x1
> +  ///
> +  UINT16 Version;
> +  ///
> +  /// The number of profiles GUIDs present in ConformanceProfiles
> +  ///
> +  UINT16 NumberOfProfiles;
> +  ///
> +  /// An array of conformance profile GUIDs that are supported by this system.
> +  /// EFI_GUID        ConformanceProfiles[];
> +  ///
> +} EFI_CONFORMANCE_PROFILES_TABLE;
> +
> +#define EFI_CONFORMANCE_PROFILES_TABLE_VERSION 0x1
> +
> +//
> +// GUID defined in spec.
> +//
> +#define EFI_CONFORMANCE_PROFILES_UEFI_SPEC_GUID \
> +    { 0x523c91af, 0xa195, 0x4382, \
> +    { 0x81, 0x8d, 0x29, 0x5f, 0xe4, 0x00, 0x64, 0x65 }}
> +#define EFI_CONFORMANCE_PROFILE_EBBR_2_1_GUID \
> +    { 0xcce33c35, 0x74ac, 0x4087, \
> +    { 0xbc, 0xe7, 0x8b, 0x29, 0xb0, 0x2e, 0xeb, 0x27 }}
> +
> +extern EFI_GUID  gEfiConfProfilesTableGuid;
> +extern EFI_GUID  gEfiConfProfilesUefiSpecGuid;
> +
> +#endif
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c
> index 5b0730b75268..ad20ef028e99 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c
> @@ -19,6 +19,7 @@
>   #include <Guid/SystemResourceTable.h>
>   #include <Guid/DebugImageInfoTable.h>
>   #include <Guid/ImageAuthentication.h>
> +#include <Guid/ConformanceProfiles.h>
>   
>   /**
>     Make a printable character.
> @@ -272,7 +273,74 @@ DisplayImageExecutionEntries (
>     return (ShellStatus);
>   }
>   
> +/**
> +  Display the ConformanceProfileTable entries
>   
> +  @param[in] Address    The pointer to the ConformanceProfileTable.
> +**/
> +SHELL_STATUS
> +DisplayConformanceProfiles (
> +  IN UINT64 Address
> +  )
> +{
> +  SHELL_STATUS    ShellStatus;
> +  EFI_STATUS      Status;
> +  VOID            *EntryPtr;
> +  EFI_GUID        *EntryGuid;
> +  CHAR16          *GuidName;
> +  EFI_CONFORMANCE_PROFILES_TABLE            *ConfProfTable;
> +
> +  ShellStatus = SHELL_SUCCESS;
> +
> +  if (Address != 0) {
> +    Status = EfiGetSystemConfigurationTable (&gEfiConfProfilesTableGuid, (VOID **)&ConfProfTable);

Why do you get the table address here, when it was already passed in
as a parameter to this function?

> +    if (EFI_ERROR (Status)) {
> +      ShellStatus = SHELL_NOT_FOUND;
> +      ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_DMEM_ERR_GET_FAIL), gShellDebug1HiiHandle, L"ConformanceProfileTable");
> +    } else {
> +      ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_DMEM_CONF_PRO_TABLE), gShellDebug1HiiHandle);
> +
> +      EntryPtr = ConfProfTable;
> +
> +      EntryPtr += sizeof(ConfProfTable->Version);
> +      EntryPtr += sizeof(ConfProfTable->NumberOfProfiles);

Similar to my comment on the exec info table I would suggest something
like:

EntryGuid = (EFI_CONFORMANCE_PROFILES_TABLE *)(ConfProfTable + 1);

> +
> +      for (int Profile = 0; Profile < ConfProfTable->NumberOfProfiles; Profile++) {
> +        EntryGuid = EntryPtr;

You shouldn't need EntryPtr.  I think the loop works with just the
EntryGuid ptr which you increment on each iteration.

> +        GuidName = L"Unknown_Profile";
> +
> +        if (CompareGuid (EntryGuid, &gEfiConfProfilesEbbrSpecGuid)) {
> +          GuidName = L"EBBR_2.1";
> +        }
> +
> +        ShellPrintHiiEx (
> +          -1,
> +          -1,
> +          NULL,
> +          STRING_TOKEN (STR_DMEM_CONF_PRO_ROW),
> +          gShellDebug1HiiHandle,
> +          GuidName,
> +          EntryGuid
> +          );
> +
> +        EntryPtr += sizeof(EFI_GUID);

Move the above to the part of the for() loop that increments on each
iteration.

> +      }
> +    }
> +  } else {
> +        ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_DMEM_CONF_PRO_TABLE), gShellDebug1HiiHandle);
> +    ShellPrintHiiEx (
> +      -1,
> +      -1,
> +      NULL,
> +      STRING_TOKEN (STR_DMEM_CONF_PRO_ROW),
> +      gShellDebug1HiiHandle,
> +      L"EFI_CONFORMANCE_PROFILES_UEFI_SPEC_GUID",
> +      &gEfiConfProfilesUefiSpecGuid
> +      );
> +  }
> +
> +  return (ShellStatus);
> +}
>   
>   STATIC CONST SHELL_PARAM_ITEM  ParamList[] = {
>     { L"-mmio", TypeFlag },
> @@ -464,6 +532,11 @@ ShellCommandRunDmem (
>                 HiiDatabaseExportBufferAddress = (UINT64)(UINTN)gST->ConfigurationTable[TableWalker].VendorTable;
>                 continue;
>               }
> +
> +            if (CompareGuid (&gST->ConfigurationTable[TableWalker].VendorGuid, &gEfiConfProfilesTableGuid)) {
> +              ConformanceProfileTableAddress = (UINT64)(UINTN)gST->ConfigurationTable[TableWalker].VendorTable;
> +              continue;
> +            }
>             }
>   
>             ShellPrintHiiEx (
> @@ -507,6 +580,9 @@ ShellCommandRunDmem (
>             if (ShellStatus == SHELL_SUCCESS) {
>               ShellStatus = DisplayImageExecutionEntries (ImageExecutionTableAddress);
>             }
> +          if (ShellStatus == SHELL_SUCCESS) {
> +            ShellStatus = DisplayConformanceProfiles(ConformanceProfileTableAddress);
> +          }
>           }
>   
>         } else {
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni
> index eee9384e3ffb..c99c881776b6 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni
> @@ -147,6 +147,11 @@
>   #string STR_DMEM_IMG_EXE_TABLE    #language en-US "\r\nImage Execution Table\r\n"
>                                                     "----------------------------------------\r\n"
>   #string STR_DMEM_IMG_EXE_ENTRY    #language en-US "%s: %s\r\n"
> +#string STR_DMEM_CONF_PRO_TABLE   #language en-US "\r\nConformance Profile Table\r\n"
> +                                                  "----------------------------------------\r\n"
> +                                                  "Version     0x1\r\n"
> +                                                  "Profile GUIDs:\r\n"
> +#string STR_DMEM_CONF_PRO_ROW     #language en-US "    %s    %g\r\n"
>   #string STR_DMEM_ERR_NOT_FOUND    #language en-US "\r\n%H%s%N: Table address not found.\r\n"
>   #string STR_DMEM_ERR_GET_FAIL     #language en-US "\r\n%H%s%N: Unable to get table information.\r\n"
>   

Thanks,
Stuart


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116341): https://edk2.groups.io/g/devel/message/116341
Mute This Topic: https://groups.io/mt/103940860/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-