Re: [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension

2024-03-07 Thread Laszlo Ersek 
On 3/7/24 23:00, Tuan Phan wrote:
> 
> 
> On Mon, Mar 4, 2024 at 10:01 AM Laszlo Ersek  > wrote:
> 
> On 3/2/24 00:20, Tuan Phan wrote:
> > Thanks for the detailed review. Please see my comments below.
> >
> > On Fri, Mar 1, 2024 at 4:14 AM Laszlo Ersek  
> > >> wrote:
> >
> >     On 3/1/24 02:29, Tuan Phan wrote:
> >     > The GCD EFI_MEMORY_UC and EFI_MEMORY_WC memory attributes
> will be
> >     > supported when Svpbmt extension available.
> >     >
> >     > Cc: Gerd Hoffmann    >>
> >     > Cc: Laszlo Ersek    >>
> >     > Cc: Rahul Kumar  
> >     >>
> >     > Cc: Ray Ni mailto:ray...@intel.com>
> >>
> >     > Signed-off-by: Tuan Phan  
> >     >>
> >     > ---
> >     >  .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 101
> >     +++---
> >     >  .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf       |   1 +
> >     >  2 files changed, 88 insertions(+), 14 deletions(-)
> >     >
> >     > diff --git
> a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> >     b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> >     > index 826a1d32a1d4..f4419bb8f380 100644
> >     > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> >     > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> >     > @@ -36,6 +36,11 @@
> >     >  #define PTE_PPN_SHIFT         10
> >     >  #define RISCV_MMU_PAGE_SHIFT  12
> >     > 
> >     > +#define RISCV_CPU_FEATURE_PBMT_BITMASK  BIT2
> >     > +#define PTE_PBMT_NC                     BIT61
> >     > +#define PTE_PBMT_IO                     BIT62
> >     > +#define PTE_PBMT_MASK                   (PTE_PBMT_NC |
> PTE_PBMT_IO)
> >     > +
> >     >  STATIC UINTN  mModeSupport[] = { SATP_MODE_SV57,
> SATP_MODE_SV48,
> >     SATP_MODE_SV39, SATP_MODE_OFF };
> >     >  STATIC UINTN  mMaxRootTableLevel;
> >     >  STATIC UINTN  mBitPerLevel;
> >     > @@ -489,32 +494,89 @@ UpdateRegionMapping (
> >     >  /**
> >     >    Convert GCD attribute to RISC-V page attribute.
> >     > 
> >     > -  @param  GcdAttributes The GCD attribute.
> >     > +  @param  GcdAttributes   The GCD attribute.
> >     > +  @param  RiscVAttribtues The pointer of RISC-V page attribute.
> >     > 
> >     > -  @return               The RISC-V page attribute.
> >     > +  @retval EFI_INVALID_PARAMETER   The RiscVAttribtues is
> NULL or
> >     cache type mask not valid.
> >     > +  @retval EFI_SUCCESS             The operation succesfully.
> >     > 
> >     >  **/
> >     >  STATIC
> >     > -UINTN
> >     > +EFI_STATUS
> >     >  GcdAttributeToPageAttribute (
> >     > -  IN UINTN  GcdAttributes
> >     > +  IN UINTN   GcdAttributes,
> >
> >     Just noticing: why is GcdAttributes *not* UINT64 in the first
> place?
> >
> >     All the bit macros we test against it, such as EFI_MEMORY_RO
> >     (0x0002ULL) are of type unsigned long long (UINT64).
> >
> > Good catch. Will fix it. 
> >
> >
> >     > +  OUT UINTN  *RiscVAttributes
> >     >    )
> >     >  {
> >     > -  UINTN  RiscVAttributes;
> >     > +  UINT64   CacheTypeMask;
> >     > +  BOOLEAN  PmbtExtEnabled = (PcdGet64
> (PcdRiscVFeatureOverride) &
> >     RISCV_CPU_FEATURE_PBMT_BITMASK) ? TRUE : FALSE;
> >
> >     - Per the edk2 coding style, locals should not be initialized
> (separate
> >     assignment is needed).
> >
> >     - Bitmask checks always need an explicit comparison, such as
> >
> >       ((a & b) != 0)
> >
> >     or similar. Implicitly interpreting (a & b) as a truth value
> is not
> >     appropriate.
> >
> >     - "(whatever) ? TRUE : FALSE" is both bad style and unnecessary.
> >
> >       BOOLEAN  PmbtExtEnabled;
> >
> >       PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) &
> >                         RISCV_CPU_FEATURE_PBMT_BITMASK) != 0;
> >
> > Will fix it. 
> >
> >     > 
> >     > -  RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> >     > +  if (!RiscVAttributes) {
> >
> >     - The coding style requires an explicit nullity check:
> >
> >       if (RiscVAttributes == NULL) {
> >
> > Will fix it.  

[edk2-devel] [PATCH v2 2/2] ShellPkg/Acpiview: Adds ACPI WSMT Table parse

2024-03-07 Thread Abdul Lateef Attar via groups.io 
From: Abdul Lateef Attar 

Adds WSMT parse to the UefiShellAcpiViewCommandLib library.

Cc: Zhichao Gao 
Cc: Pierre Gondois  
Signed-off-by: Abdul Lateef Attar 
Reviewed-by: Pierre Gondois  
---
 .../UefiShellAcpiViewCommandLib/AcpiParser.h  |  17 ++
 .../Parsers/Wsmt/WsmtParser.c | 147 ++
 .../UefiShellAcpiViewCommandLib.c |   1 +
 .../UefiShellAcpiViewCommandLib.inf   |   1 +
 4 files changed, 166 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 UINT8AcpiTableRevision
   );
 
+/**
+  This function parses the ACPI WSMT table.
+
+  @param [in] Trace  If TRUE, trace the ACPI fields.
+  @param [in] PtrPointer to the start of the buffer.
+  @param [in] AcpiTableLengthLength 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 UINT8AcpiTableRevision
+  );
+
 /**
   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 00..3c7252b0bf
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Wsmt/WsmtParser.c
@@ -0,0 +1,147 @@
+/** @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 
+#include 
+#include "AcpiParser.h"
+
+STATIC ACPI_DESCRIPTION_HEADER_INFO  AcpiHdrInfo;
+
+/**
+  This function validates the WSMT Protection flag.
+
+  @param [in] Ptr  Pointer to the start of the buffer.
+  @param [in] Context Pointer to context specific information e.g. this
+  could be a pointer to the ACPI table header.
+
+**/
+STATIC
+VOID
+EFIAPI
+ValidateWsmtProtectionFlag (
+  IN UINT8  *Ptr,
+  IN VOID   *Context
+  )
+{
+  UINT32  ProtectionFlag;
+
+  ProtectionFlag = *(UINT32 *)Ptr;
+
+  if ((ProtectionFlag & 
EFI_WSMT_PROTECTION_FLAGS_COMM_BUFFER_NESTED_PTR_PROTECTION) \
+  == EFI_WSMT_PROTECTION_FLAGS_COMM_BUFFER_NESTED_PTR_PROTECTION)
+  {
+if ((ProtectionFlag & EFI_WSMT_PROTECTION_FLAGS_FIXED_COMM_BUFFERS) \
+!= EFI_WSMT_PROTECTION_FLAGS_FIXED_COMM_BUFFERS)
+{
+  IncrementErrorCount ();
+  Print (L"ERROR: COMM_BUFFER_NESTED_PTR_PROTECTION is set but 
FIXED_COMM_BUFFERS is not set.\n");
+}
+  }
+}
+
+/**
+  This function validates the reserved bits in the WSMT Protection flag.
+
+  @param [in] Ptr  Pointer to the start of the buffer.
+  @param [in] Context Pointer to context specific information e.g. this
+  could be a pointer to the ACPI table header.
+**/
+STATIC
+VOID
+EFIAPI
+ValidateReserved (
+  IN UINT8  *Ptr,
+  IN VOID   *Context
+  )
+{
+  UINT32  ProtectionFlag;
+
+  ProtectionFlag = *(UINT32 *)Ptr;
+
+  if ((ProtectionFlag & 0xFFF8) != 0) {
+IncrementErrorCount ();
+Print (L"ERROR: Reserved bits are not zero.\n");
+  }
+}
+
+/**
+  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, 
ValidateReserved, 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 (),
+  { L"Protection Flag",4,36, NULL, DumpWsmtProtectionFlag, NULL,

[edk2-devel] [PATCH v2 1/2] ShellPkg/Acpiview: Adds HPET parser

2024-03-07 Thread Abdul Lateef Attar via groups.io 
From: Abdul Lateef Attar 

Adds HPET parse to the UefiShellAcpiViewCommandLib library.

Cc: Zhichao Gao 
Cc: Pierre Gondois  
Signed-off-by: Abdul Lateef Attar 
Reviewed-by: Pierre Gondois  
---
 .../UefiShellAcpiViewCommandLib/AcpiParser.h  |  17 ++
 .../Parsers/Hpet/HpetParser.c | 222 ++
 .../UefiShellAcpiViewCommandLib.c |   1 +
 .../UefiShellAcpiViewCommandLib.inf   |   1 +
 4 files changed, 241 insertions(+)
 create mode 100644 
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
index 4b4397961b..ba3364f2c2 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
@@ -730,6 +730,23 @@ ParseAcpiHmat (
   IN UINT8AcpiTableRevision
   );
 
+/**
+  This function parses the ACPI HPET table.
+
+  @param [in] Trace  If TRUE, trace the ACPI fields.
+  @param [in] PtrPointer to the start of the buffer.
+  @param [in] AcpiTableLengthLength of the ACPI table.
+  @param [in] AcpiTableRevision  Revision of the ACPI table.
+**/
+VOID
+EFIAPI
+ParseAcpiHpet (
+  IN BOOLEAN  Trace,
+  IN UINT8*Ptr,
+  IN UINT32   AcpiTableLength,
+  IN UINT8AcpiTableRevision
+  );
+
 /**
   This function parses the ACPI IORT table.
   When trace is enabled this function parses the IORT table and
diff --git 
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c
new file mode 100644
index 00..1b4c38f2af
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c
@@ -0,0 +1,222 @@
+/** @file
+  HPET table parser
+
+  Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+  @par Reference(s):
+- HPET spec, version 1.0a
+**/
+
+#include 
+#include "AcpiParser.h"
+
+STATIC ACPI_DESCRIPTION_HEADER_INFO  AcpiHdrInfo;
+
+/**
+  This function prints HPET page protection flags.
+  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
+DumpHpetPageProtectionFlag (
+  IN CONST CHAR16  *Format OPTIONAL,
+  IN UINT8 *Ptr
+  )
+{
+  if (Format != NULL) {
+Print (Format, *(UINT8 *)Ptr);
+return;
+  }
+
+  Print (L"0x%X ", *(UINT8 *)Ptr);
+  switch (*Ptr) {
+case 0:
+  Print (L"(no guarantee for page protection)");
+  break;
+case 1:
+  Print (L"(4K page protection)");
+  break;
+case 2:
+  Print (L"(64K page protection)");
+  break;
+default:
+  IncrementErrorCount ();
+  Print (L"(OEM Reserved)");
+  break;
+  }
+
+  return;
+}
+
+/**
+  An ACPI_PARSER array describing the ACPI HPET flags.
+**/
+STATIC CONST ACPI_PARSER  DumpHpetFlagParser[] = {
+  { L"Page Protection Flag", 4, 0, NULL,DumpHpetPageProtectionFlag, NULL, 
NULL, NULL },
+  { L"OEM Attributes",   4, 4, L"0x%x", NULL,   NULL, 
NULL, NULL }
+};
+
+/**
+  This function prints HPET Flags fields.
+  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
+DumpHpetFlag (
+  IN CONST CHAR16  *Format OPTIONAL,
+  IN UINT8 *Ptr
+  )
+{
+  if (Format != NULL) {
+Print (Format, *(UINT8 *)Ptr);
+return;
+  }
+
+  Print (L"0x%X\n", *(UINT8 *)Ptr);
+  ParseAcpiBitFields (
+TRUE,
+2,
+NULL,
+Ptr,
+4,
+PARSER_PARAMS (DumpHpetFlagParser)
+);
+}
+
+/**
+  This function prints HPET Counter size fields.
+  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
+DumpCounterSize (
+  IN CONST CHAR16  *Format OPTIONAL,
+  IN UINT8 *Ptr
+  )
+{
+  if (Format != NULL) {
+Print (Format, *(UINT32 *)Ptr);
+return;
+  }
+
+  Print (L"0x%X ", *(UINT32 *)Ptr);
+  if (*Ptr == 0) {
+Print (L"(Max 32-bit counter size)");
+  } else {
+Print (L"(Max 64-bit counter size)");
+  }
+}
+
+/**
+  This function validates the flags.
+
+  @param [in] Ptr Pointer to the start of the field data.
+  @param [in] Context Pointer to context specific information e.g. this
+  could be a pointer to the ACPI table header.
+**/
+STATIC
+VOID
+EFIAPI
+ValidateHpetRevId (
+  IN UINT8  *Ptr,
+  IN VOID   *Context
+  )
+{
+  if ((*(UINT8 *)Ptr) == 0) {
+IncrementErrorCount ();
+Print (
+  L"\nERROR: HPET Hardware Rev ID must be set."
+  );
+  }
+}
+
+/**
+  An ACPI_PARSER array

[edk2-devel] [PATCH v2 0/2] ShellPkg/AcpiView: Adds HPET and WSMT parser

2024-03-07 Thread Abdul Lateef Attar via groups.io 
From: Abdul Lateef Attar 

PR : https://github.com/tianocore/edk2/pull/5449

Adds HPET and WSMT parser for acpiview.
V2 delta changes:
  Addressed review comments.
  Added validation functions as per the comments.

Cc: Zhichao Gao 
Cc: Pierre Gondois  
Abdul Lateef Attar (2):
  ShellPkg/Acpiview: Adds HPET parser
  ShellPkg/Acpiview: Adds ACPI WSMT Table parse

 .../UefiShellAcpiViewCommandLib/AcpiParser.h  |  34 +++
 .../Parsers/Hpet/HpetParser.c | 222 ++
 .../Parsers/Wsmt/WsmtParser.c | 147 
 .../UefiShellAcpiViewCommandLib.c |   2 +
 .../UefiShellAcpiViewCommandLib.inf   |   2 +
 5 files changed, 407 insertions(+)
 create mode 100644 
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c
 create mode 100644 
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Wsmt/WsmtParser.c

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116488): https://edk2.groups.io/g/devel/message/116488
Mute This Topic: https://groups.io/mt/104804846/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/2] ShellPkg/Acpiview: Adds HPET parser

2024-03-07 Thread Abdul Lateef Attar via groups.io 
Thanks Pierre for review, I'll address the review comment and submit the 
V2 patch.



On 07-03-2024 16:07, Pierre Gondois wrote:
Caution: This message originated from an External Source. Use proper 
caution when opening attachments, clicking links, or responding.



Hello Abdul,

With the comments resolved:
Reviewed-by: Pierre Gondois 

On 3/5/24 12:12, Abdul Lateef Attar wrote:

From: Abdul Lateef Attar 

Adds HPET parse to the UefiShellAcpiViewCommandLib library.

Cc: Zhichao Gao 
Cc: Pierre Gondois  
Signed-off-by: Abdul Lateef Attar 
---
  .../UefiShellAcpiViewCommandLib/AcpiParser.h  |  17 ++
  .../Parsers/Hpet/HpetParser.c | 221 ++
  .../UefiShellAcpiViewCommandLib.c |   1 +
  .../UefiShellAcpiViewCommandLib.inf   |   1 +
  4 files changed, 240 insertions(+)
  create mode 100644 
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c


diff --git 
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h

index 4b4397961b..ba3364f2c2 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
@@ -730,6 +730,23 @@ ParseAcpiHmat (
    IN UINT8    AcpiTableRevision
    );

+/**
+  This function parses the ACPI HPET 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
+ParseAcpiHpet (
+  IN BOOLEAN  Trace,
+  IN UINT8    *Ptr,
+  IN UINT32   AcpiTableLength,
+  IN UINT8    AcpiTableRevision
+  );
+
  /**
    This function parses the ACPI IORT table.
    When trace is enabled this function parses the IORT table and
diff --git 
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c

new file mode 100644
index 00..d1866f91c1
--- /dev/null
+++ 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c

@@ -0,0 +1,221 @@
+/** @file
+  HPET table parser
+
+  Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+  @par Reference(s):
+    - HPET spec, version 1.0a
+**/
+
+#include 
+#include "AcpiParser.h"
+
+STATIC ACPI_DESCRIPTION_HEADER_INFO  AcpiHdrInfo;
+
+/**
+  This function prints HPET page protection flags.
+  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
+DumpHpetPageProtectionFlag (
+  IN CONST CHAR16  *Format OPTIONAL,
+  IN UINT8 *Ptr
+  )
+{
+  if (Format != NULL) {
+    Print (Format, *(UINT8 *)Ptr);
+    return;
+  }
+
+  Print (L"0x%X ", *(UINT8 *)Ptr);
+  switch (*Ptr) {
+    case 0:
+  Print (L"(no guarantee for page protection)");
+  break;
+    case 1:
+  Print (L"(4K page protection)");
+  break;
+    case 2:
+  Print (L"(64K page protection)");
+  break;
+    default:
+  Print (L"(OEM Reserved)");


Maybe IncrementErrorCount() + Print() here.


+  break;
+  }
+
+  return;
+}
+
+/**
+  An ACPI_PARSER array describing the ACPI HPET flags.
+**/
+STATIC CONST ACPI_PARSER  DumpHpetFlagParser[] = {
+  { L"Page Protection Flag", 4, 0, NULL, DumpHpetPageProtectionFlag, 
NULL, NULL, NULL },
+  { L"OEM Attributes",   4, 4, L"0x%x", 
NULL,   NULL, NULL, NULL }

+};
+
+/**
+  This function prints HPET Flags fields.
+  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
+DumpHpetFlag (
+  IN CONST CHAR16  *Format OPTIONAL,
+  IN UINT8 *Ptr
+  )
+{
+  if (Format != NULL) {
+    Print (Format, *(UINT8 *)Ptr);
+    return;
+  }
+
+  Print (L"0x%X\n", *(UINT8 *)Ptr);
+  ParseAcpiBitFields (
+    TRUE,
+    2,
+    NULL,
+    Ptr,
+    4,
+    PARSER_PARAMS (DumpHpetFlagParser)
+    );
+}
+
+/**
+  This function prints HPET Counter size fields.
+  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
+DumpCounterSize (
+  IN CONST CHAR16  *Format OPTIONAL,
+  IN UINT8 *Ptr
+  )
+{
+  if (Format != NULL) {
+    Print (Format, *(UINT32 *)Ptr);
+    return;
+  }
+
+  Print (L"0x%X ", *(UINT32 *)Ptr);
+  if (*Ptr == 0) {
+    Print (L"(Max 32-bit counter size)");
+  } else {
+    Print (L"(Max 64-bit counter size)");
+  }
+}
+
+/**
+  This function validates the flags.
+
+  @param [in] Ptr Pointer to the start of the field data.
+  @param [in] Context Pointer to

Re: [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension

2024-03-07 Thread Tuan Phan 
On Mon, Mar 4, 2024 at 10:01 AM Laszlo Ersek  wrote:

> On 3/2/24 00:20, Tuan Phan wrote:
> > Thanks for the detailed review. Please see my comments below.
> >
> > On Fri, Mar 1, 2024 at 4:14 AM Laszlo Ersek  > > wrote:
> >
> > On 3/1/24 02:29, Tuan Phan wrote:
> > > The GCD EFI_MEMORY_UC and EFI_MEMORY_WC memory attributes will be
> > > supported when Svpbmt extension available.
> > >
> > > Cc: Gerd Hoffmann mailto:kra...@redhat.com>>
> > > Cc: Laszlo Ersek mailto:ler...@redhat.com>>
> > > Cc: Rahul Kumar  > >
> > > Cc: Ray Ni mailto:ray...@intel.com>>
> > > Signed-off-by: Tuan Phan  > >
> > > ---
> > >  .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 101
> > +++---
> > >  .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf   |   1 +
> > >  2 files changed, 88 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > > index 826a1d32a1d4..f4419bb8f380 100644
> > > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > > @@ -36,6 +36,11 @@
> > >  #define PTE_PPN_SHIFT 10
> > >  #define RISCV_MMU_PAGE_SHIFT  12
> > >
> > > +#define RISCV_CPU_FEATURE_PBMT_BITMASK  BIT2
> > > +#define PTE_PBMT_NC BIT61
> > > +#define PTE_PBMT_IO BIT62
> > > +#define PTE_PBMT_MASK   (PTE_PBMT_NC |
> PTE_PBMT_IO)
> > > +
> > >  STATIC UINTN  mModeSupport[] = { SATP_MODE_SV57, SATP_MODE_SV48,
> > SATP_MODE_SV39, SATP_MODE_OFF };
> > >  STATIC UINTN  mMaxRootTableLevel;
> > >  STATIC UINTN  mBitPerLevel;
> > > @@ -489,32 +494,89 @@ UpdateRegionMapping (
> > >  /**
> > >Convert GCD attribute to RISC-V page attribute.
> > >
> > > -  @param  GcdAttributes The GCD attribute.
> > > +  @param  GcdAttributes   The GCD attribute.
> > > +  @param  RiscVAttribtues The pointer of RISC-V page attribute.
> > >
> > > -  @return   The RISC-V page attribute.
> > > +  @retval EFI_INVALID_PARAMETER   The RiscVAttribtues is NULL or
> > cache type mask not valid.
> > > +  @retval EFI_SUCCESS The operation succesfully.
> > >
> > >  **/
> > >  STATIC
> > > -UINTN
> > > +EFI_STATUS
> > >  GcdAttributeToPageAttribute (
> > > -  IN UINTN  GcdAttributes
> > > +  IN UINTN   GcdAttributes,
> >
> > Just noticing: why is GcdAttributes *not* UINT64 in the first place?
> >
> > All the bit macros we test against it, such as EFI_MEMORY_RO
> > (0x0002ULL) are of type unsigned long long (UINT64).
> >
> > Good catch. Will fix it.
> >
> >
> > > +  OUT UINTN  *RiscVAttributes
> > >)
> > >  {
> > > -  UINTN  RiscVAttributes;
> > > +  UINT64   CacheTypeMask;
> > > +  BOOLEAN  PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) &
> > RISCV_CPU_FEATURE_PBMT_BITMASK) ? TRUE : FALSE;
> >
> > - Per the edk2 coding style, locals should not be initialized
> (separate
> > assignment is needed).
> >
> > - Bitmask checks always need an explicit comparison, such as
> >
> >   ((a & b) != 0)
> >
> > or similar. Implicitly interpreting (a & b) as a truth value is not
> > appropriate.
> >
> > - "(whatever) ? TRUE : FALSE" is both bad style and unnecessary.
> >
> >   BOOLEAN  PmbtExtEnabled;
> >
> >   PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) &
> > RISCV_CPU_FEATURE_PBMT_BITMASK) != 0;
> >
> > Will fix it.
> >
> > >
> > > -  RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> > > +  if (!RiscVAttributes) {
> >
> > - The coding style requires an explicit nullity check:
> >
> >   if (RiscVAttributes == NULL) {
> >
> > Will fix it.
> >
> >
> > > +return EFI_INVALID_PARAMETER;
> > > +  }
> > > +
> > > +  *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> > >
> > >// Determine protection attributes
> > >if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> > > -RiscVAttributes &= ~(RISCV_PG_W);
> > > +*RiscVAttributes &= ~(RISCV_PG_W);
> > >}
> > >
> > >// Process eXecute Never attribute
> > >if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
> > > -RiscVAttributes &= ~RISCV_PG_X;
> > > +*RiscVAttributes &= ~RISCV_PG_X;
> > > +  }
> > > +
> >
> > My next comment is unrelated to the patch, it's just something that
> > catches my eye, and I think is worth fixing:
> >
> > RISCV_PG_W is BIT2 (0x0004), and RISCV_PG_X is BIT3 (0x0008).
> > Meaning, they are of type *signed int* (INT32). Applying the bit-neg
> > operator on them produces a

[edk2-devel] Now: TianoCore edk2-test Bug Triage Meeting - Thursday, March 7, 2024 #cal-notice

2024-03-07 Thread Group Notification 
*TianoCore edk2-test Bug Triage Meeting*

*When:*
Thursday, March 7, 2024
10:00pm to 11:00pm
(UTC+08:00) Asia/Shanghai

*Where:*
https://armltd.zoom.us/j/94348061758?pwd=Q3RDeFA5K2JFaU5jdWUxc1FnaGdyUT09=addon

*Organizer:* Edhaya Chandran edhaya.chand...@arm.com ( 
edhaya.chand...@arm.com?subject=Re:%20Event:%20TianoCore%20edk2-test%20Bug%20Triage%20Meeting
 )

View Event ( https://edk2.groups.io/g/devel/viewevent?eventid=2159767 )

*Description:*


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116485): https://edk2.groups.io/g/devel/message/116485
Mute This Topic: https://groups.io/mt/104787490/21656
Mute #cal-notice:https://edk2.groups.io/g/devel/mutehashtag/cal-notice
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] Event: TianoCore edk2-test Bug Triage Meeting - Thursday, March 7, 2024 #cal-reminder

2024-03-07 Thread Group Notification 
*Reminder: TianoCore edk2-test Bug Triage Meeting*

*When:*
Thursday, March 7, 2024
10:00pm to 11:00pm
(UTC+08:00) Asia/Shanghai

*Where:*
https://armltd.zoom.us/j/94348061758?pwd=Q3RDeFA5K2JFaU5jdWUxc1FnaGdyUT09=addon

*Organizer:* Edhaya Chandran edhaya.chand...@arm.com ( 
edhaya.chand...@arm.com?subject=Re:%20Event:%20TianoCore%20edk2-test%20Bug%20Triage%20Meeting
 )

View Event ( https://edk2.groups.io/g/devel/viewevent?eventid=2159767 )

*Description:*


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116484): https://edk2.groups.io/g/devel/message/116484
Mute This Topic: https://groups.io/mt/104765797/21656
Mute #cal-reminder:https://edk2.groups.io/g/devel/mutehashtag/cal-reminder
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg/SmbiosPlatformDxe: tweak fallback release date again

2024-03-07 Thread Laszlo Ersek 
On 3/7/24 10:04, joeyli via groups.io wrote:
> Hi Laszlo,
> 
> On Tue, Mar 05, 2024 at 09:53:33AM +0100, Laszlo Ersek wrote:
>> On 3/4/24 12:37, joeyli via groups.io wrote:
>>> Hi,
>>>
>>> On Wed, Feb 07, 2024 at 04:02:52PM +0800, joeyli via groups.io wrote:
 On Wed, Feb 07, 2024 at 03:55:49PM +0800, joeyli wrote:
> Hi Laszlo,
>
> First, thanks for your review!
>
> On Mon, Feb 05, 2024 at 05:41:25PM +0100, Laszlo Ersek wrote:
>> On 2/4/24 10:29, Lee, Chun-Yi wrote:
>>> In case PcdFirmwareReleaseDateString is not set use a valid date
>>> as fallback. But the default valid date can _NOT_ pass the Microsoft
>>> SVVP test "Check SMBIOS Table Specific Requirements". The test emitted
>>> the error message:
>>>
>>> BIOS Release Date string is unexpected length: 8. This string must be in
>>> MM/DD/ format. No other format is allowed and no additional 
>>> information
>>> may be included. See field description in the SMBIOS specification.
>>>
>>> Base on SMBIOS spec v3.7.0:
>>>
>>> 08h 2.0+BIOS Release Date   BYTESTRING
>>> String number of the BIOS release date. The date
>>> string, if supplied, is in either mm/dd/yy or
>>> mm/dd/ format. If the year portion of the string
>>> is two digits, the year is assumed to be 19yy.
>>> NOTE: The mm/dd/ format is required for SMBIOS
>>> version 2.3 and later.
>>>
>>> So, let's tweek the fallback release date again.
>>>
>>> Fixes: a0f9628705e3 ("OvmfPkg/SmbiosPlatformDxe: tweak fallback release 
>>> date") [edk2-stable202305~327]
>>> Signed-off-by: "Lee, Chun-Yi" 
>>> ---
>>>  OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c 
>>> b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
>>> index 0ca3776045..e929da6b81 100644
>>> --- a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
>>> +++ b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
>>> @@ -160,7 +160,7 @@ InstallAllStructures (
>>>  DateStr = (CHAR16 *)FixedPcdGetPtr (PcdFirmwareReleaseDateString);
>>>  DateLen = StrLen (DateStr);
>>>  if (DateLen < 3) {
>>> -  DateStr = L"2/2/2022";
>>> +  DateStr = L"02/02/2022";
>>>DateLen = StrLen (DateStr);
>>>  }
>>>  
>>
>> Are you proposing this as an important (but low risk) bugfix that might
>> qualify for the freeze(s)? If so, please loop in Liming and Mike.
>>
>
> hm... What does freeze mean? 
>

 ah... You mean soft feature freeze for edk2-stable202402. 

 Hi Liming, Michael,

 This change is important but low risk. Could you please consider to add it
 to edk2-stable202402 release?

 Thanks a lot!
 Joey Lee
>>>
>>> This patch is not in edk2-stable202402. Will it to be merged to next 
>>> release?
>>
>> Thanks for the reminder, and sorry about the delay!
>>
>> Merged as commit 2a0d4a2641a7, via
>> .
> 
> Thanks for your review and merge!
> 
>>
>> For future contributions: please run PatchCheck.py on the patch series
>> before formatting and posting it (better yet, submit a personal CI build
>> PR).
>>
> 
> For 'submit personal CI build PR', I was created a pull requests on github:
> https://github.com/tianocore/edk2/pull/5349 
> 
> Is it the right approach for triggering a CI build PR?

Yes, it is.

... I think the enforcement of Cc: tags in commit messages may have been
introduced later than your PR. New PRs like yours will catch missing Cc:
tags.

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116483): https://edk2.groups.io/g/devel/message/116483
Mute This Topic: https://groups.io/mt/104153758/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/2] ShellPkg/Acpiview: Adds HPET parser

2024-03-07 Thread PierreGondois 

Hello Abdul,

With the comments resolved:
Reviewed-by: Pierre Gondois 

On 3/5/24 12:12, Abdul Lateef Attar wrote:

From: Abdul Lateef Attar 

Adds HPET parse to the UefiShellAcpiViewCommandLib library.

Cc: Zhichao Gao 
Cc: Pierre Gondois  
Signed-off-by: Abdul Lateef Attar 
---
  .../UefiShellAcpiViewCommandLib/AcpiParser.h  |  17 ++
  .../Parsers/Hpet/HpetParser.c | 221 ++
  .../UefiShellAcpiViewCommandLib.c |   1 +
  .../UefiShellAcpiViewCommandLib.inf   |   1 +
  4 files changed, 240 insertions(+)
  create mode 100644 
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
index 4b4397961b..ba3364f2c2 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
@@ -730,6 +730,23 @@ ParseAcpiHmat (
IN UINT8AcpiTableRevision
);
  
+/**

+  This function parses the ACPI HPET table.
+
+  @param [in] Trace  If TRUE, trace the ACPI fields.
+  @param [in] PtrPointer to the start of the buffer.
+  @param [in] AcpiTableLengthLength of the ACPI table.
+  @param [in] AcpiTableRevision  Revision of the ACPI table.
+**/
+VOID
+EFIAPI
+ParseAcpiHpet (
+  IN BOOLEAN  Trace,
+  IN UINT8*Ptr,
+  IN UINT32   AcpiTableLength,
+  IN UINT8AcpiTableRevision
+  );
+
  /**
This function parses the ACPI IORT table.
When trace is enabled this function parses the IORT table and
diff --git 
a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c 
b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c
new file mode 100644
index 00..d1866f91c1
--- /dev/null
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c
@@ -0,0 +1,221 @@
+/** @file
+  HPET table parser
+
+  Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+  @par Reference(s):
+- HPET spec, version 1.0a
+**/
+
+#include 
+#include "AcpiParser.h"
+
+STATIC ACPI_DESCRIPTION_HEADER_INFO  AcpiHdrInfo;
+
+/**
+  This function prints HPET page protection flags.
+  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
+DumpHpetPageProtectionFlag (
+  IN CONST CHAR16  *Format OPTIONAL,
+  IN UINT8 *Ptr
+  )
+{
+  if (Format != NULL) {
+Print (Format, *(UINT8 *)Ptr);
+return;
+  }
+
+  Print (L"0x%X ", *(UINT8 *)Ptr);
+  switch (*Ptr) {
+case 0:
+  Print (L"(no guarantee for page protection)");
+  break;
+case 1:
+  Print (L"(4K page protection)");
+  break;
+case 2:
+  Print (L"(64K page protection)");
+  break;
+default:
+  Print (L"(OEM Reserved)");


Maybe IncrementErrorCount() + Print() here.


+  break;
+  }
+
+  return;
+}
+
+/**
+  An ACPI_PARSER array describing the ACPI HPET flags.
+**/
+STATIC CONST ACPI_PARSER  DumpHpetFlagParser[] = {
+  { L"Page Protection Flag", 4, 0, NULL,DumpHpetPageProtectionFlag, NULL, 
NULL, NULL },
+  { L"OEM Attributes",   4, 4, L"0x%x", NULL,   NULL, 
NULL, NULL }
+};
+
+/**
+  This function prints HPET Flags fields.
+  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
+DumpHpetFlag (
+  IN CONST CHAR16  *Format OPTIONAL,
+  IN UINT8 *Ptr
+  )
+{
+  if (Format != NULL) {
+Print (Format, *(UINT8 *)Ptr);
+return;
+  }
+
+  Print (L"0x%X\n", *(UINT8 *)Ptr);
+  ParseAcpiBitFields (
+TRUE,
+2,
+NULL,
+Ptr,
+4,
+PARSER_PARAMS (DumpHpetFlagParser)
+);
+}
+
+/**
+  This function prints HPET Counter size fields.
+  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
+DumpCounterSize (
+  IN CONST CHAR16  *Format OPTIONAL,
+  IN UINT8 *Ptr
+  )
+{
+  if (Format != NULL) {
+Print (Format, *(UINT32 *)Ptr);
+return;
+  }
+
+  Print (L"0x%X ", *(UINT32 *)Ptr);
+  if (*Ptr == 0) {
+Print (L"(Max 32-bit counter size)");
+  } else {
+Print (L"(Max 64-bit counter size)");
+  }
+}
+
+/**
+  This function validates the flags.
+
+  @param [in] Ptr Pointer to the start of the field data.
+  @param [in] Context Pointer to context specific information e.g. this
+  could be a pointer to the ACPI table header.
+**/
+STATIC
+VOID
+EFIAPI
+ValidateHpetRevId (
+  IN UINT8  *Ptr,
+  IN VOID   *Context
+  )
+{
+  if ((*(UINT8 *)Ptr) == 0) {
+IncrementErrorCount ();
+

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

2024-03-07 Thread PierreGondois 

Hello Abdul,

I just have one comment, with that:
Reviewed-by: Pierre Gondois 

On 3/5/24 12:12, Abdul Lateef Attar wrote:

From: Abdul Lateef Attar 

Adds WSMT parse to the UefiShellAcpiViewCommandLib library.

Cc: Zhichao Gao 
Cc: Pierre Gondois  
Signed-off-by: Abdul Lateef Attar 
---
  .../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 UINT8AcpiTableRevision
);
  
+/**

+  This function parses the ACPI WSMT table.
+
+  @param [in] Trace  If TRUE, trace the ACPI fields.
+  @param [in] PtrPointer to the start of the buffer.
+  @param [in] AcpiTableLengthLength 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 UINT8AcpiTableRevision
+  );
+
  /**
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 00..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 
+#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 (),
+  { 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] PtrPointer to the start of the buffer.
+  @param [in] AcpiTableLengthLength 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 UINT8AcpiTableRevision
+  )
+{
+  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 },

Re: [edk2-devel] [PATCH v2 08/10] IntelFsp2Pkg: auto-generate SEC ProcessLibraryConstructorList() decl

2024-03-07 Thread Zeng, Star 
Reviewed-by: Star Zeng 

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Laszlo Ersek
Sent: Thursday, March 7, 2024 5:10 AM
To: edk2-devel-groups-io 
Cc: S, Ashraf Ali ; Chiu, Chasel 
; Duggapu, Chinni B ; 
Desimone, Nathaniel L ; Zeng, Star 
; Mohapatra, Susovan ; Kuo, 
Ted 
Subject: Re: [edk2-devel] [PATCH v2 08/10] IntelFsp2Pkg: auto-generate SEC 
ProcessLibraryConstructorList() decl

Can I please get a quick R-b for this patch -- it's urgent because of 
.

Thank you,
Laszlo

On 3/5/24 12:38, Laszlo Ersek wrote:
> Rely on AutoGen for declaring ProcessLibraryConstructorList().
> 
> Build-tested with:
> 
>   build -a X64 -b DEBUG -m IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf \
> -p IntelFsp2Pkg/IntelFsp2Pkg.dsc -t GCC5
> 
>   build -a X64 -b DEBUG -m IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf \
> -p IntelFsp2Pkg/IntelFsp2Pkg.dsc -t GCC5
> 
> Cc: Ashraf Ali S 
> Cc: Chasel Chiu 
> Cc: Duggapu Chinni B 
> Cc: Nate DeSimone 
> Cc: Star Zeng 
> Cc: Susovan Mohapatra 
> Cc: Ted Kuo 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=990
> Signed-off-by: Laszlo Ersek 
> ---
>  IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf |  2 +-
>  IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf   |  2 +-
>  IntelFsp2Pkg/FspSecCore/SecMain.h | 12 
>  3 files changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf 
> b/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf
> index cb011f99f964..7d60e2283e26 100644
> --- a/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf
> +++ b/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf
> @@ -8,7 +8,7 @@
>  ##
>  
>  [Defines]
> -  INF_VERSION= 0x00010005
> +  INF_VERSION= 1.30
>BASE_NAME  = Fsp24SecCoreM
>FILE_GUID  = C5BC0719-4A23-4F6E-94DA-05FB6A0DFA9C
>MODULE_TYPE= SEC
> diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf 
> b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
> index 8029832235ec..d496f3957d1b 100644
> --- a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
> +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
> @@ -8,7 +8,7 @@
>  ##
>  
>  [Defines]
> -  INF_VERSION= 0x00010005
> +  INF_VERSION= 1.30
>BASE_NAME  = FspSecCoreM
>FILE_GUID  = C2F9AE46-3437-4FEF-9CB1-9A568B282FEE
>MODULE_TYPE= SEC
> diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.h 
> b/IntelFsp2Pkg/FspSecCore/SecMain.h
> index 023deb7e2bda..eb1458d19773 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecMain.h
> +++ b/IntelFsp2Pkg/FspSecCore/SecMain.h
> @@ -110,18 +110,6 @@ SecStartup (
>IN UINT32  ApiIdx
>);
>  
> -/**
> -  Autogenerated function that calls the library constructors for all 
> of the module's
> -  dependent libraries.  This function must be called by the SEC Core 
> once a stack has
> -  been established.
> -
> -**/
> -VOID
> -EFIAPI
> -ProcessLibraryConstructorList (
> -  VOID
> -  );
> -
>  /**
>  
>Return value of esp.
> 
> 
> 
> 
> 
> 








-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116480): https://edk2.groups.io/g/devel/message/116480
Mute This Topic: https://groups.io/mt/104742530/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler

2024-03-07 Thread Laszlo Ersek 
On 3/7/24 07:11, Liu, Zhiguang wrote:
> Hi Laszlo,
> 
> We talked about how to unregister SMI handler inside other SMI handler.
> And the conclusion was there is no such usage so we don't need support it.
> However, I find some platform does need to unregister SMI handler inside 
> other SMI handler.

:)

> The design you introduced below is great to meet the needs.

I'm happy to hear that!

> If you don't have time to implement and have no concern, I can implement your 
> design and send the patch out.

Yes, please go ahead and implement it, if you think it fits your needs.
I'll probably not be around to review it, though.

Cheers!
Laszlo

> 
> Thanks
> Zhiguang
> 
>> -Original Message-
>> From: Laszlo Ersek 
>> Sent: Thursday, January 25, 2024 6:48 PM
>> To: devel@edk2.groups.io; Liu, Zhiguang 
>> Cc: Liming Gao ; Wu, Jiaxin
>> ; Ni, Ray ; Ard Biesheuvel
>> ; Sami Mujawar 
>> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to
>> unregister SMI handler inside SMI handler
>>
>> On 1/24/24 05:03, Zhiguang Liu wrote:
>>> To support unregister SMI handler inside SMI handler itself, get next
>>> node before SMI handler is executed, since LIST_ENTRY that Link points
>>> to may be freed if unregister SMI handler in SMI handler itself.
>>>
>>> Cc: Liming Gao 
>>> Cc: Jiaxin Wu 
>>> Cc: Ray Ni 
>>> Signed-off-by: Zhiguang Liu 
>>> ---
>>>  MdeModulePkg/Core/PiSmmCore/Smi.c | 8 +++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c
>>> b/MdeModulePkg/Core/PiSmmCore/Smi.c
>>> index 2985f989c3..a75e52b1ae 100644
>>> --- a/MdeModulePkg/Core/PiSmmCore/Smi.c
>>> +++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
>>> @@ -134,8 +134,14 @@ SmiManage (
>>>
>>>Head = >SmiHandlers;
>>>
>>> -  for (Link = Head->ForwardLink; Link != Head; Link =
>>> Link->ForwardLink) {
>>> +  for (Link = Head->ForwardLink; Link != Head;) {
>>>  SmiHandler = CR (Link, SMI_HANDLER, Link, SMI_HANDLER_SIGNATURE);
>>> +//
>>> +// To support unregiser SMI handler inside SMI handler itself,
>>> +// get next node before handler is executed, since LIST_ENTRY that
>>> +// Link points to may be freed if unregister SMI handler.
>>> +//
>>> +Link = Link->ForwardLink;
>>>
>>>  Status = SmiHandler->Handler (
>>> (EFI_HANDLE)SmiHandler,
>>
>> I've had a further thought here.
>>
>> (1) This patch may enable an SMI handler to unregister *itself*, that is,
>> through the following call chain
>>
>>   SmiManage()
>> SmiHandler->Handler()
>>   SmiHandlerUnRegister()
>>
>> but it still does not ensure *general iterator safety*.
>>
>> Assume that an SMM driver registers two handlers, handler A and handler B.
>> Assume the DispatchHandles for these are stored in global variables in the
>> driver. Then, assume that "handler A" (for whatever reason, under some
>> circumstances) unregisters "handler B".
>>
>> That could still lead to a use-after-free in the SmiManage() loop; is that 
>> right?
>>
>> If that driver scenario is plausible, then something like the following 
>> would be
>> needed:
>>
>> - a global variable of enum type in "MdeModulePkg/Core/PiSmmCore/Smi.c",
>> with three possible values (NotManaging=0, Managing=1, CleanupNeeded=2).
>>
>> - a new "BOOLEAN Deleted" field in SMI_HANDLER
>>
>> - all loops in "Smi.c" iterating over SMI_HANDLERs would have to skip 
>> handlers
>> that have been marked as Deleted
>>
>> - in SmiManage(), set the state to Managing (=1) before the loop. After the
>> loop, check if the state is CleanupNeeded (=2); if so, add an extra pass for
>> actually deleting SMI_HANDLERs from the list that have been marked Deleted.
>> Finally (regardless of the state being Managing (=1) or CleanupNeeded (=2)),
>> reset the state to NotManaging (=0).
>>
>> - in SmiHandlerUnRegister(), if the state is NotManaging (=0), delete the
>> handler immediately. Otherwise (i.e., when the state is Managing
>> (=1) or CleanupNeeded (=2)), set the state to CleanupNeeded (=2), and only
>> mark the handler as Deleted.
>>
>> The idea is to inform SmiHandlerUnRegister() whether it's running or not
>> running on the stack of SmiManage(), and to postpone SMI_HANDLER
>> deletion until after the loop finishes in the former case.
>>
>> I'm not sure if real life SmiHandlerUnRegister() usage is worth this
>> complication, however.
>>
>> (2) Independently: does the same issue exist for the StMM core? I seem to be
>> discovering the same problem in MmiManage()
>> [StandaloneMmPkg/Core/Mmi.c].
>>
>> So, whatever patch we add to the SMM_CORE, should likely be reflected to
>> MM_CORE_STANDALONE too. Adding Ard and Sami to the CC list.
>>
>> Thanks
>> Laszlo
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116479): https://edk2.groups.io/g/devel/message/116479
Mute This Topic: https://groups.io/mt/103925794/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:

Re: [edk2-devel] [PATCH] OvmfPkg/SmbiosPlatformDxe: tweak fallback release date again

2024-03-07 Thread joeyli via groups.io 
Hi Laszlo,

On Tue, Mar 05, 2024 at 09:53:33AM +0100, Laszlo Ersek wrote:
> On 3/4/24 12:37, joeyli via groups.io wrote:
> > Hi,
> > 
> > On Wed, Feb 07, 2024 at 04:02:52PM +0800, joeyli via groups.io wrote:
> >> On Wed, Feb 07, 2024 at 03:55:49PM +0800, joeyli wrote:
> >>> Hi Laszlo,
> >>>
> >>> First, thanks for your review!
> >>>
> >>> On Mon, Feb 05, 2024 at 05:41:25PM +0100, Laszlo Ersek wrote:
>  On 2/4/24 10:29, Lee, Chun-Yi wrote:
> > In case PcdFirmwareReleaseDateString is not set use a valid date
> > as fallback. But the default valid date can _NOT_ pass the Microsoft
> > SVVP test "Check SMBIOS Table Specific Requirements". The test emitted
> > the error message:
> >
> > BIOS Release Date string is unexpected length: 8. This string must be in
> > MM/DD/ format. No other format is allowed and no additional 
> > information
> > may be included. See field description in the SMBIOS specification.
> >
> > Base on SMBIOS spec v3.7.0:
> >
> > 08h 2.0+BIOS Release Date   BYTESTRING
> > String number of the BIOS release date. The date
> > string, if supplied, is in either mm/dd/yy or
> > mm/dd/ format. If the year portion of the string
> > is two digits, the year is assumed to be 19yy.
> > NOTE: The mm/dd/ format is required for SMBIOS
> > version 2.3 and later.
> >
> > So, let's tweek the fallback release date again.
> >
> > Fixes: a0f9628705e3 ("OvmfPkg/SmbiosPlatformDxe: tweak fallback release 
> > date") [edk2-stable202305~327]
> > Signed-off-by: "Lee, Chun-Yi" 
> > ---
> >  OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c 
> > b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> > index 0ca3776045..e929da6b81 100644
> > --- a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> > +++ b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> > @@ -160,7 +160,7 @@ InstallAllStructures (
> >  DateStr = (CHAR16 *)FixedPcdGetPtr (PcdFirmwareReleaseDateString);
> >  DateLen = StrLen (DateStr);
> >  if (DateLen < 3) {
> > -  DateStr = L"2/2/2022";
> > +  DateStr = L"02/02/2022";
> >DateLen = StrLen (DateStr);
> >  }
> >  
> 
>  Are you proposing this as an important (but low risk) bugfix that might
>  qualify for the freeze(s)? If so, please loop in Liming and Mike.
> 
> >>>
> >>> hm... What does freeze mean? 
> >>>
> >>
> >> ah... You mean soft feature freeze for edk2-stable202402. 
> >>
> >> Hi Liming, Michael,
> >>
> >> This change is important but low risk. Could you please consider to add it
> >> to edk2-stable202402 release?
> >>
> >> Thanks a lot!
> >> Joey Lee
> > 
> > This patch is not in edk2-stable202402. Will it to be merged to next 
> > release?
> 
> Thanks for the reminder, and sorry about the delay!
> 
> Merged as commit 2a0d4a2641a7, via
> .

Thanks for your review and merge!

> 
> For future contributions: please run PatchCheck.py on the patch series
> before formatting and posting it (better yet, submit a personal CI build
> PR).
>

For 'submit personal CI build PR', I was created a pull requests on github:
https://github.com/tianocore/edk2/pull/5349 

Is it the right approach for triggering a CI build PR?

Thanks a lot!
Joey Lee


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116478): https://edk2.groups.io/g/devel/message/116478
Mute This Topic: https://groups.io/mt/104153758/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 00/10] clean up ProcessLibraryConstructorList() declarations in SEC modules

2024-03-07 Thread Gerd Hoffmann 
On Tue, Mar 05, 2024 at 12:38:33PM +0100, Laszlo Ersek wrote:
> Bugzillas:
> - https://bugzilla.tianocore.org/show_bug.cgi?id=990
> - https://bugzilla.tianocore.org/show_bug.cgi?id=991
> - https://bugzilla.tianocore.org/show_bug.cgi?id=4643
> 
> CI:
> - https://github.com/tianocore/edk2/pull/5442
> 
> Branch:
> - 
> https://github.com/lersek/edk2/tree/ProcessLibraryConstructorList-SEC-990-991-v2
> 
> This patch series puts the recent BaseTools feature to use in which
> AutoGen generates the ProcessLibraryConstructorList() declaration in
> "AutoGen.h" for such non-library SEC modules whose INF_VERSION is at
> least 1.30. The BaseTools feature is present in both edk2 [1] and
> edk2-basetools [2], and has been documented in the Build spec [3] and
> the Inf spec [4]. Kudos to Rebecca for tagging a new edk2-basetools
> release [5] [6] with the new feature.

Series:
Acked-by: Gerd Hoffmann 

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116477): https://edk2.groups.io/g/devel/message/116477
Mute This Topic: https://groups.io/mt/104742520/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-