Re: [edk2-devel] [PATCH v1 2/2] ShellPkg/Acpiview: Adds ACPI WSMT Table parser

["PierreGondois" <pierre.gondois@arm.com>]

Hello Abdul,

I just have one comment, with that:
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>

On 3/5/24 12:12, Abdul Lateef Attar wrote:
> From: Abdul Lateef Attar <AbdulLateef.Attar@amd.com>
> 
> Adds WSMT parse to the UefiShellAcpiViewCommandLib library.
> 
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Pierre Gondois  <pierre.gondois@arm.com>
> Signed-off-by: Abdul Lateef Attar <AbdulLateef.Attar@amd.com>
> ---
>   .../UefiShellAcpiViewCommandLib/AcpiParser.h  | 17 ++++
>   .../Parsers/Wsmt/WsmtParser.c                 | 89 +++++++++++++++++++
>   .../UefiShellAcpiViewCommandLib.c             |  1 +
>   .../UefiShellAcpiViewCommandLib.inf           |  1 +
>   4 files changed, 108 insertions(+)
>   create mode 100644 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Wsmt/WsmtParser.c
> 
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> index ba3364f2c2..6468fe5d8c 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
> @@ -985,6 +985,23 @@ ParseAcpiSsdt (
>     IN UINT8    AcpiTableRevision
>     );
>   
> +/**
> +  This function parses the ACPI WSMT table.
> +
> +  @param [in] Trace              If TRUE, trace the ACPI fields.
> +  @param [in] Ptr                Pointer to the start of the buffer.
> +  @param [in] AcpiTableLength    Length of the ACPI table.
> +  @param [in] AcpiTableRevision  Revision of the ACPI table.
> +**/
> +VOID
> +EFIAPI
> +ParseAcpiWsmt (
> +  IN BOOLEAN  Trace,
> +  IN UINT8    *Ptr,
> +  IN UINT32   AcpiTableLength,
> +  IN UINT8    AcpiTableRevision
> +  );
> +
>   /**
>     This function parses the ACPI XSDT table
>     and optionally traces the ACPI table fields.
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Wsmt/WsmtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Wsmt/WsmtParser.c
> new file mode 100644
> index 0000000000..eb2668c059
> --- /dev/null
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Wsmt/WsmtParser.c
> @@ -0,0 +1,89 @@
> +/** @file
> +  WSMT table parser
> +
> +  Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved.
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  @par Reference(s):
> +    - Windows SMM Security Mitigation Table spec, version 1.0
> +**/
> +
> +#include <Library/UefiLib.h>
> +#include "AcpiParser.h"
> +
> +STATIC ACPI_DESCRIPTION_HEADER_INFO  AcpiHdrInfo;
> +
> +/**
> +  An ACPI_PARSER array describing the WSMT Protection flag .
> +**/
> +STATIC CONST ACPI_PARSER  WsmtProtectionFlagParser[] = {
> +  { L"FIXED_COMM_BUFFERS ",                1,  0, L"0x%x", NULL, NULL, NULL, NULL },
> +  { L"COMM_BUFFER_NESTED_PTR_PROTECTION ", 1,  1, L"0x%x", NULL, NULL, NULL, NULL },
> +  { L"SYSTEM_RESOURCE_PROTECTION ",        1,  2, L"0x%x", NULL, NULL, NULL, NULL },
> +  { L"Reserved ",                          29, 3, L"0x%x", NULL, NULL, NULL, NULL },
> +};
> +
> +/**
> +  This function prints WSMT Protection flag.
> +  If no format string is specified the Format must be NULL.
> +
> +  @param [in] Format  Optional format string for tracing the data.
> +  @param [in] Ptr     Pointer to the start of the buffer.
> +**/
> +VOID
> +EFIAPI
> +DumpWsmtProtectionFlag (
> +  IN CONST CHAR16  *Format OPTIONAL,
> +  IN UINT8         *Ptr
> +  )
> +{
> +  if (Format != NULL) {
> +    Print (Format, *(UINT32 *)Ptr);
> +    return;
> +  }
> +
> +  Print (L"0x%X\n", *(UINT32 *)Ptr);
> +  ParseAcpiBitFields (
> +    TRUE,
> +    2,
> +    NULL,
> +    Ptr,
> +    4,
> +    PARSER_PARAMS (WsmtProtectionFlagParser)
> +    );
> +}
> +
> +/**
> +  An ACPI_PARSER array describing the ACPI WSMT Table.
> +**/
> +STATIC CONST ACPI_PARSER  WsmtParser[] = {
> +  PARSE_ACPI_HEADER (&AcpiHdrInfo),
> +  { L"Protection Flag",            4,36, NULL, DumpWsmtProtectionFlag, NULL, NULL, NULL }

Maybe a validate function could be added, similar to:
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c::ValidateFlags()

just to check that FIXED_COMM_BUFFERS  is set if COMM_BUFFER_NESTED_PTR_PROTECTION
is set, and use IncrementErrorCount() + Print() if


> +};
> +
> +/**
> +  This function parses the ACPI WSMT table.
> +
> +  @param [in] Trace              If TRUE, trace the ACPI fields.
> +  @param [in] Ptr                Pointer to the start of the buffer.
> +  @param [in] AcpiTableLength    Length of the ACPI table.
> +  @param [in] AcpiTableRevision  Revision of the ACPI table.
> +**/
> +VOID
> +EFIAPI
> +ParseAcpiWsmt (
> +  IN BOOLEAN  Trace,
> +  IN UINT8    *Ptr,
> +  IN UINT32   AcpiTableLength,
> +  IN UINT8    AcpiTableRevision
> +  )
> +{
> +  ParseAcpi (
> +    Trace,
> +    0,
> +    "WSMT",
> +    Ptr,
> +    AcpiTableLength,
> +    PARSER_PARAMS (WsmtParser)
> +    );
> +}
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
> index 9e15979ea2..0bdf068fe0 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
> @@ -73,6 +73,7 @@ ACPI_TABLE_PARSER  ParserList[] = {
>     { EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE,                                        ParseAcpiSpcr },
>     { EFI_ACPI_6_2_SYSTEM_RESOURCE_AFFINITY_TABLE_SIGNATURE,                                               ParseAcpiSrat },
>     { EFI_ACPI_6_2_SECONDARY_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,                                           ParseAcpiSsdt },
> +  { EFI_ACPI_6_5_WINDOWS_SMM_SECURITY_MITIGATION_TABLE_SIGNATURE,                                        ParseAcpiWsmt },
>     { EFI_ACPI_6_2_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,                                            ParseAcpiXsdt }
>   };
>   
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
> index 3338e04379..e62366116c 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
> @@ -52,6 +52,7 @@
>     Parsers/Spcr/SpcrParser.c
>     Parsers/Srat/SratParser.c
>     Parsers/Ssdt/SsdtParser.c
> +  Parsers/Wsmt/WsmtParser.c
>     Parsers/Xsdt/XsdtParser.c
>     UefiShellAcpiViewCommandLib.c
>     UefiShellAcpiViewCommandLib.uni


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