Date   

Re: Bug report: serial timeout gets overwritten shortly after setting it

Michael D Kinney
 

Hi Nicholas,

How did you open the Serial I/O Protocol?

If you open BY_DRIVER or EXCLUSIVE, then it should disconnect the Terminal
driver from the Serial I/O Protocol and allow you to change the timeout and
keep that timeout setting. For your UEFI application test case, the open
mode of EXCLUSIVE would be the best choice.

You can also use DisconnectController() to disconnect the Terminal driver
from the Serial I/O Protocol you want to manage and change the timeout and
then open it to use it. EXCLUSIVE does the disconnect for you.

In order to do an OpenProtcol() with the appropriate open type or
DisconnectController(), you will want to use LocateHandle() instead of
LocateProtocol() to get an EFI_HANDLE with he correct Serial I/O Protocol.
You may need to evaluate the Device Path Protocol instance to make sure
you have selected the correct Serial I/O Protocol instance if the
platform has more than one.

The UEFI Driver Writer's Guide also has a description of these issues.
Please see section 5.1.3.4.

https://tianocore-docs.github.io/edk2-UefiDriverWritersGuide/draft/5_uefi_services/51_services_that_uefi_drivers_commonly_use/513_handle_database_and_protocol_services.html#513-handle-database-and-protocol-services

Thanks,

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of nicholasbishop@gmail.com
Sent: Sunday, November 21, 2021 9:52 AM
To: devel@edk2.groups.io
Subject: [edk2-devel] Bug report: serial timeout gets overwritten shortly after setting it

Hi, I have a bug to report with the serial I/O protocol. Using the SetAttributes function to change the timeout initially works, but after a short amount of time the timeout gets reset to 173 microseconds.

I believe this is likely due to the implementation of the simple text input protocol, which alters the serial timeout:
https://github.com/tianocore/edk2/blob/e1e7306b54147e65cb7347b060e94f336d4a82d2/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c#L549

I've created a full example here:
https://github.com/nicholasbishop/edk2/commit/d6087f863a48f3c7812f47ea21d5d673adb8d754

The example sets the timeout to 1s which works correctly, then sleeps for 1s and reads the timeout value again. The timeout is now 173ms.


Bug report: serial timeout gets overwritten shortly after setting it

nicholasbishop@...
 

Hi, I have a bug to report with the serial I/O protocol. Using the SetAttributes function to change the timeout initially works, but after a short amount of time the timeout gets reset to 173 microseconds.

I believe this is likely due to the implementation of the simple text input protocol, which alters the serial timeout:
https://github.com/tianocore/edk2/blob/e1e7306b54147e65cb7347b060e94f336d4a82d2/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c#L549

I've created a full example here:
https://github.com/nicholasbishop/edk2/commit/d6087f863a48f3c7812f47ea21d5d673adb8d754

The example sets the timeout to 1s which works correctly, then sleeps for 1s and reads the timeout value again. The timeout is now 173ms.


Event: TianoCore Bug Triage - APAC / NAMO - 11/23/2021 #cal-reminder

devel@edk2.groups.io Calendar <noreply@...>
 

Reminder: TianoCore Bug Triage - APAC / NAMO

When:
11/23/2021
6:30pm to 7:30pm
(UTC-08:00) America/Los Angeles

Where:
https://teams.microsoft.com/l/meetup-join/19%3ameeting_OTUyZTg2NjgtNDhlNS00ODVlLTllYTUtYzg1OTNjNjdiZjFh%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%22b286b53a-1218-4db3-bfc9-3d4c5aa7669e%22%7d

Organizer: Liming Gao gaoliming@...

View Event

Description:

TianoCore Bug Triage - APAC / NAMO

Hosted by Liming Gao

 

________________________________________________________________________________

Microsoft Teams meeting

Join on your computer or mobile app

Click here to join the meeting

Join with a video conferencing device

teams@...

Video Conference ID: 116 062 094 0

Alternate VTC dialing instructions

Or call in (audio only)

+1 916-245-6934,,77463821#   United States, Sacramento

Phone Conference ID: 774 638 21#

Find a local number | Reset PIN

Learn More | Meeting options


Re: [PATCH] MdePkg: DebugLib: Compilation fix for clang-13.

Michael Brown
 

On 22/11/2021 16:42, Michael D Kinney wrote:
You are also modifying the DebugLib in the paths where ASSERT() macros
are disabled. When they are disabled, we want all code/data associated
with ASSERT() to be removed by the optimizing compiler/linker. The source
code change appears to force a reference to a variable/expression. Does
this have any size impact to any of the toolchains when ASSERT() is
disabled? Can you provide the size comparison before and after this
change?
I would very strongly recommend having the non-debug version of the macro use something like:

#define ASSERT(Expression) do { \
if (FALSE) { \
(VOID) (Expression); \
} \
} while (FALSE)

Without the "if (FALSE)", you will find that an expression that may have side-effects (e.g. by calling an external function) will result in executable code being generated.

Michael


Re: [PATCH v3 0/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser

Sami Mujawar
 

Hi Abdul,

Thank you for this patch series.

This series looks good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 22/11/2021 10:21 AM, Abdul Lateef Attar wrote:
Hi Sami, Zhichao,
Addressed all review comments.
Created two different patches, one for bitfield parser and
another for FADT Flags parser.

Thanks
AbduL

REF : https://github.com/abdattar/edk2/tree/FadtFlagsParser

Cc: Ray Ni <ray.ni@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
Abdul Lateef Attar (2):
ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser
ShellPkg/AcpiView: PrintFormatter for FADT Flags field

ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 45 +++++
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 177 ++++++++++++++++++++
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 59 ++++++-
3 files changed, 280 insertions(+), 1 deletion(-)


Re: [PATCH] MdePkg: DebugLib: Compilation fix for clang-13.

Michael D Kinney
 

Hi Mikhail,

Are you able to provide a few examples of dead code this change uncovers?

If there is additional issues then wouldn't applying this patch break all the
open source package builds that contain these issues? Does this patch
series pass EDK II CI?

Wouldn't it also make sense to remove the disable of these warnings from
all the toolchains if these updates are the only ones required to pass
EDK II CI for all supported toolchains?

You are also modifying the DebugLib in the paths where ASSERT() macros
are disabled. When they are disabled, we want all code/data associated
with ASSERT() to be removed by the optimizing compiler/linker. The source
code change appears to force a reference to a variable/expression. Does
this have any size impact to any of the toolchains when ASSERT() is
disabled? Can you provide the size comparison before and after this
change?

I would like to add that I am in favor of using the compilers to help find
issues in source code and increasing warning levels is great technique.
We just need to make sure the changes do not generate false positives on
issues and that the size and performance impacts of the changes are
measured as part of the evaluation of the change. Especially components
like DebugLib.h that can impact almost every component in the entire
FW stack.

Thanks,

Mike

-----Original Message-----
From: krichanov@ispras.ru <krichanov@ispras.ru>
Sent: Monday, November 22, 2021 12:15 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
Cc: gaoliming@byosoft.com.cn; Liu, Zhiguang <zhiguang.liu@intel.com>; vit9696@protonmail.com
Subject: Re: [PATCH] MdePkg: DebugLib: Compilation fix for clang-13.

Hi Mike,

This warning helps to find dead code. Disabling it leads to code quality
degradation.
---
Mikhail Krichanov

On 2021-11-19 19:51, Kinney, Michael D wrote:
Hi Mikhail,

For RELEASE GCC5 toolchains in tools_def.txt, I see this warning is
disabled. Likely for the same reason.

RELEASE_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os
-Wno-unused-but-set-variable -Wno-unused-const-variable
RELEASE_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto
-DUSING_LTO -Os -Wno-unused-but-set-variable
-Wno-unused-const-variable
RELEASE_GCC5_ARM_CC_FLAGS = DEF(GCC5_ARM_CC_FLAGS) -flto
-Wno-unused-but-set-variable -Wno-unused-const-variable
RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto
-Wno-unused-but-set-variable -Wno-unused-const-variable

I think it would be better to ignore the same warning in RELEASE
CLANG38 tool chains with an update to tools_def.txt instead of
DebugLib.h.

Unless you are thinking that disabling this warning is hiding some bad
code practices that need to be cleaned up.

Best regards,

Mike


-----Original Message-----
From: Mikhail Krichanov <krichanov@ispras.ru>
Sent: Friday, November 19, 2021 1:05 AM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
<gaoliming@byosoft.com.cn>; Liu, Zhiguang
<zhiguang.liu@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>;
Mikhail Krichanov <krichanov@ispras.ru>
Subject: [PATCH] MdePkg: DebugLib: Compilation fix for clang-13.

build -a X64 -t CLANG38 -b RELEASE -p OvmfPkg/OvmfPkgX64.dsc
results in
UDK/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c:1284:31:
error: variable 'Status' set but not used
[-Werror,-Wunused-but-set-variable]
EFI_STATUS Status;
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3704

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Mikhail Krichanov <krichanov@ispras.ru>
---
MdePkg/Include/Library/DebugLib.h | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/MdePkg/Include/Library/DebugLib.h
b/MdePkg/Include/Library/DebugLib.h
index 4cacd4b8..7785e99d 100644
--- a/MdePkg/Include/Library/DebugLib.h
+++ b/MdePkg/Include/Library/DebugLib.h
@@ -370,7 +370,10 @@ UnitTestDebugAssert (
} \
} while (FALSE)
#else
- #define ASSERT(Expression)
+ #define ASSERT(Expression) \
+ do { \
+ (VOID) (Expression); \
+ } while (FALSE)
#endif

/**
@@ -392,6 +395,14 @@ UnitTestDebugAssert (
_DEBUG (Expression); \
} \
} while (FALSE)
+#elif defined(__GNUC__) || defined(__clang__)
+ #define DEBUG(Expression) \
+ do { \
+ _Pragma("GCC diagnostic push") \
+ _Pragma("GCC diagnostic ignored \"-Wunused-value\"") \
+ (VOID) Expression; \
+ _Pragma("GCC diagnostic pop") \
+ } while (FALSE)
#else
#define DEBUG(Expression)
#endif
@@ -419,7 +430,10 @@ UnitTestDebugAssert (
}
\
} while (FALSE)
#else
- #define ASSERT_EFI_ERROR(StatusParameter)
+ #define ASSERT_EFI_ERROR(StatusParameter)
\
+ do {
\
+ (VOID) (StatusParameter);
\
+ } while (FALSE)
#endif

/**
@@ -446,7 +460,10 @@ UnitTestDebugAssert (
}
\
} while (FALSE)
#else
- #define ASSERT_RETURN_ERROR(StatusParameter)
+ #define ASSERT_RETURN_ERROR(StatusParameter)
\
+ do {
\
+ (VOID) (StatusParameter);
\
+ } while (FALSE)
#endif

/**
--
2.20.1


Re: CdePkgBlog 2021-11-14

Maciej Rabeda
 

Hi Kilian,

From my point of view, the main problem with adoption of CdePkg to EDK2 is that it relies on Torito C library.
  1. Torito C library License (https://github.com/KilianKegel/toro-C-Library/blob/master/LICENSE.md) only allows for creating UEFI Shell applications.
    • What about applications that do not rely on ShellPkg (example: SysPrep application that might want to use Redfish, which depends on C standard library)?
    • What about drivers/libraries that rely on C standard library?
    • How is that compatible with EDK2 BSD-2-Clause-Patent?
  2. Torito C is pre-compiled.
    • How can I verify what was actually implemented inside? Industry would have to trust your tests, perform own set of tests or/and disassemble it (doable, but unacceptable effort-wise).

Unless those problems are solved, I simply cannot use it.

Thanks,
Maciej

On 14-Nov-21 20:51, Kilian Kegel wrote:

Hi All,

 

as announced last summer, I’d like to start a comprehensive tutorial on

how to use Standard C / ANSI C in the UEFI environment and how to design and implement

such a library for Shell and POST usage.

 

Since most parts of that comprehensive work are already done,

I will report, demonstrate and discuss implementation principles and details on edk2.groups.io

as a kind of “blog” on a biweekly basis.

 

Please checkout my first CdePkgBlog https://github.com/tianocore/edk2-staging/tree/CdePkg/blogs/2021-11-14#cdepkgblog-2021-11-14

and enjoy the breathtaking build speed if compiler and linker are used exclusively to create MY LEGACY TOOLBOX,

a handy set of one-trick-ponies that I have been using for about 25 years.

 

Have fun,

Kilian

 



Re: [PATCH] OvmfPkg/AmdSev/SecretPei: Mark SEV launch secret area as reserved

Yao, Jiewen
 

Acked-by: Jiewen Yao <Jiewen.Yao@intel.com>

-----Original Message-----
From: Brijesh Singh <brijesh.singh@amd.com>
Sent: Monday, November 22, 2021 11:43 PM
To: Dov Murik <dovmurik@linux.ibm.com>; devel@edk2.groups.io
Cc: brijesh.singh@amd.com; Ard Biesheuvel <ardb+tianocore@kernel.org>;
Justen, Jordan L <jordan.l.justen@intel.com>; Gerd Hoffmann
<kraxel@redhat.com>; Erdem Aktas <erdemaktas@google.com>; James
Bottomley <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu,
Min M <min.m.xu@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>;
Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Subject: Re: [PATCH] OvmfPkg/AmdSev/SecretPei: Mark SEV launch secret area
as reserved



On 11/18/21 5:31 AM, Dov Murik wrote:
Mark the SEV launch secret MEMFD area as reserved, which will allow the
guest OS to use it during the lifetime of the OS, without creating
copies of the sensitive content.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
OvmfPkg/AmdSev/SecretPei/SecretPei.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
index db94c26b54d1..6bf1a55dea64 100644
--- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
+++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
@@ -19,7 +19,7 @@ InitializeSecretPei (
BuildMemoryAllocationHob (
PcdGet32 (PcdSevLaunchSecretBase),
ALIGN_VALUE (PcdGet32 (PcdSevLaunchSecretSize), EFI_PAGE_SIZE),
- EfiBootServicesData
+ EfiReservedMemoryType
);

return EFI_SUCCESS;

Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

thanks


Re: [PATCH] OvmfPkg/AmdSev/SecretPei: Mark SEV launch secret area as reserved

Brijesh Singh
 

On 11/18/21 5:31 AM, Dov Murik wrote:
Mark the SEV launch secret MEMFD area as reserved, which will allow the
guest OS to use it during the lifetime of the OS, without creating
copies of the sensitive content.
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
OvmfPkg/AmdSev/SecretPei/SecretPei.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
index db94c26b54d1..6bf1a55dea64 100644
--- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
+++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
@@ -19,7 +19,7 @@ InitializeSecretPei (
BuildMemoryAllocationHob (
PcdGet32 (PcdSevLaunchSecretBase),
ALIGN_VALUE (PcdGet32 (PcdSevLaunchSecretSize), EFI_PAGE_SIZE),
- EfiBootServicesData
+ EfiReservedMemoryType
);
return EFI_SUCCESS;

Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

thanks


Re: [edk2-platforms][PATCH 00/14] Revise U500 for the latest RISC-V packages.

Daniel Schaefer
 

Great! U500 confirmed working again and cleanup through generic code.

Reviewed-by: Daniel Schaefer <daniel.schaefer@...>


From: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>
Sent: Monday, November 15, 2021 10:56
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>; Schaefer, Daniel (ROM Janitor) <daniel.schaefer@...>; Sunil V L <sunilvl@...>
Subject: [edk2-platforms][PATCH 00/14] Revise U500 for the latest RISC-V packages.
 
Incorporate U500 platform with the latest RISC-V ProcessPkg and PlatformPkg.

Signed-off-by: Abner Chang <abner.chang@...>
Cc: Daniel Schaefer <daniel.schaefer@...>
Cc: Sunil V L <sunilvl@...>

Abner Chang (14):
  RiscVPlatformPkg/U500: Pass DTB from PEI to DXE
  RiscVPlatformPkg/U500: Fix up FDT and install into config table
  RiscVPlatformPkg/U500: Use FirmwareContext library
  RiscVPlatformPkg/U500: Use generic platform library
  RiscVPlatformPkg/U500: Creates opensbi firmware domains
  RiscVPlatformPkg/U500: Uses RISC-V PeiCoreEntryPoint library
  RiscVPlatformPkg/U500: Use PlatormSecPpiLib
  RiscVPlatformPkg/U500: U500 uses mtime CSR library
  RiscVPlatformPkg/U500: Determines hart number from DTB
  RiscVPlatformPkg/U500: Use NULL instance of RiscVSpecialPlatformLib
  RiscVPlatformPkg/U500: Add device tree for U500 platform
  RiscVPlatformPkg/U500: Add device tree to build
  Platform/RISC-V: Add debug message to SecMain.c
  Platform/RISC-V: Initialize variable to zero

 .../FreedomU500VC707Board/U500.dsc            |  18 +-
 .../FreedomU500VC707Board/U500.fdf            |   8 +
 .../FreedomU500VC707Board/DeviceTree.fdf.inc  |  33 +++
 .../FreedomU500VC707Board/U500.fdf.inc        |  84 ++++--
 .../FreedomU500VC707Board/VarStore.fdf.inc    |   6 +-
 .../DeviceTree/U500DeviceTree.inf             |  25 ++
 .../OpensbiPlatformLib/OpensbiPlatformLib.inf |  54 ----
 .../FreedomU500VC707Board/DeviceTree/gpio.h   |  45 +++
 .../DeviceTree/sifive-fu500-prci.h            |  19 ++
 .../RiscVSpecialPlatformLib.c                 |   2 +-
 .../PlatformPkg/Universal/Sec/SecMain.c       |  14 +-
 .../Library/OpensbiPlatformLib/Platform.c     | 206 -------------
 .../DeviceTree/fu500-c000.dtsi                | 276 ++++++++++++++++++
 .../DeviceTree/hifive-unleashed-a00.dts       | 108 +++++++
 14 files changed, 611 insertions(+), 287 deletions(-)
 create mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/DeviceTree.fdf.inc
 create mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/DeviceTree/U500DeviceTree.inf
 delete mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/Library/OpensbiPlatformLib/OpensbiPlatformLib.inf
 create mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/DeviceTree/gpio.h
 create mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/DeviceTree/sifive-fu500-prci.h
 delete mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/Library/OpensbiPlatformLib/Platform.c
 create mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/DeviceTree/fu500-c000.dtsi
 create mode 100644 Platform/SiFive/U5SeriesPkg/FreedomU500VC707Board/DeviceTree/hifive-unleashed-a00.dts

--
2.31.1


[PATCH v3 2/2] ShellPkg/AcpiView: PrintFormatter for FADT Flags field

Abdul Lateef Attar
 

Adds PrintFormatter function to the FADT flags field.
Prints indivisual flag name along with flag value.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
---
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 59 +++++++++++++++++++-
1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
index d86718bab67d..3b59864d2c7e 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
@@ -2,6 +2,7 @@
FADT table parser

Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
+ Copyright (c) 2021, AMD Incorporated. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent

@par Reference(s):
@@ -127,6 +128,62 @@ ValidateFlags (
#endif
}

+STATIC CONST ACPI_PARSER FadtFlagParser [] = {
+ {L"WBINVD", 1, 0, L"%d", NULL, NULL, NULL, NULL},
+ {L"WBINVD_FLUSH", 1, 1, L"%d", NULL, NULL, NULL, NULL},
+ {L"PROC_C1", 1, 2, L"%d", NULL, NULL, NULL, NULL},
+ {L"P_LVL2_UP", 1, 3, L"%d", NULL, NULL, NULL, NULL},
+ {L"PWR_BUTTON", 1, 4, L"%d", NULL, NULL, NULL, NULL},
+ {L"SLP_BUTTON", 1, 5, L"%d", NULL, NULL, NULL, NULL},
+ {L"FIX_RTC", 1, 6, L"%d", NULL, NULL, NULL, NULL},
+ {L"RTC_S4", 1, 7, L"%d", NULL, NULL, NULL, NULL},
+ {L"TMR_VAL_EXT", 1, 8, L"%d", NULL, NULL, NULL, NULL},
+ {L"DCK_CAP", 1, 9, L"%d", NULL, NULL, NULL, NULL},
+ {L"RESET_REG_SUP", 1, 10, L"%d", NULL, NULL, NULL, NULL},
+ {L"SEALED_CASE", 1, 11, L"%d", NULL, NULL, NULL, NULL},
+ {L"HEADLESS", 1, 12, L"%d", NULL, NULL, NULL, NULL},
+ {L"CPU_SW_SLP", 1, 13, L"%d", NULL, NULL, NULL, NULL},
+ {L"PCI_EXP_WAK", 1, 14, L"%d", NULL, NULL, NULL, NULL},
+ {L"USE_PLATFORM_CLOCK", 1, 15, L"%d", NULL, NULL, NULL, NULL},
+ {L"S4_RTC_STS_VALID", 1, 16, L"%d", NULL, NULL, NULL, NULL},
+ {L"REMOTE_POWER_ON_CAPABLE", 1, 17, L"%d", NULL, NULL, NULL, NULL},
+ {L"FORCE_APIC_CLUSTER_MODEL", 1, 18, L"%d", NULL, NULL, NULL, NULL},
+ {L"FORCE_APIC_PHYSICAL_DESTINATION_MODE", 1, 19, L"%d", NULL, NULL, NULL, NULL},
+ {L"HW_REDUCED_ACPI", 1, 20, L"%d", NULL, NULL, NULL, NULL},
+ {L"LOW_POWER_S0_IDLE_CAPABLE", 1, 21, L"%d", NULL, NULL, NULL, NULL},
+ {L"Reserved", 10, 22, L"%d", NULL, NULL, NULL, NULL}
+};
+
+/**
+ This function traces FADT 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
+DumpFadtFlags (
+ 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 (FadtFlagParser)
+ );
+}
+
/**
An ACPI_PARSER array describing the ACPI FADT Table.
**/
@@ -170,7 +227,7 @@ STATIC CONST ACPI_PARSER FadtParser[] = {
{L"CENTURY", 1, 108, L"0x%x", NULL, NULL, NULL, NULL},
{L"IAPC_BOOT_ARCH", 2, 109, L"0x%x", NULL, NULL, NULL, NULL},
{L"Reserved", 1, 111, L"0x%x", NULL, NULL, NULL, NULL},
- {L"Flags", 4, 112, L"0x%x", NULL, (VOID**)&Flags, ValidateFlags, NULL},
+ {L"Flags", 4, 112, NULL, DumpFadtFlags, (VOID**)&Flags, ValidateFlags, NULL},
{L"RESET_REG", 12, 116, NULL, DumpGas, NULL, NULL, NULL},
{L"RESET_VALUE", 1, 128, L"0x%x", NULL, NULL, NULL, NULL},
{L"ARM_BOOT_ARCH", 2, 129, L"0x%x", NULL, NULL, NULL, NULL},
--
2.25.1


[PATCH v3 1/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser

Abdul Lateef Attar
 

Adds ParseAcpiBitFields() which is based on
ParseAcpi() and capable of parsing the bit fields.
Supports parsing of UINT8, UINT16, UINT32 and UINT64 byte data.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
---
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 45 +++++
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 177 ++++++++++++++++++++
2 files changed, 222 insertions(+)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
index 5e34a70c8bae..d64eec533fab 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
@@ -2,6 +2,7 @@
Header file for ACPI parser

Copyright (c) 2016 - 2020, Arm Limited. All rights reserved.
+ Copyright (c) 2021, AMD Incorporated. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
**/

@@ -251,6 +252,11 @@ typedef VOID (EFIAPI *FNPTR_FIELD_VALIDATOR)(UINT8* Ptr, VOID* Context);
the field data. If the field is more complex and requires additional
processing for formatting and representation a print formatter function
can be specified in 'PrintFormatter'.
+
+ ParseAcpiBitFields() uses AcpiParser structure to parse the bit fields.
+ It considers Length as a number of bits that need to be parsed.
+ Also, the Offset field will be considered as starting offset of the bitfield.
+
The PrintFormatter function may choose to use the format string
specified by 'Format' or use its own internal format string.

@@ -265,10 +271,12 @@ typedef struct AcpiParser {

/// The length of the field.
/// (Byte Length column from ACPI table spec)
+ /// Length(in bits) of the bitfield if used with ParseAcpiBitFields().
UINT32 Length;

/// The offset of the field from the start of the table.
/// (Byte Offset column from ACPI table spec)
+ /// The Bit offset of the field if used with ParseAcpiBitFields().
UINT32 Offset;

/// Optional Print() style format string for tracing the data. If not
@@ -365,6 +373,43 @@ ParseAcpi (
IN UINT32 ParserItems
);

+/**
+ This function is used to parse an ACPI table bitfield buffer.
+
+ The ACPI table buffer is parsed using the ACPI table parser information
+ specified by a pointer to an array of ACPI_PARSER elements. This parser
+ function iterates through each item on the ACPI_PARSER array and logs the ACPI table bitfields.
+
+ This function can optionally be used to parse ACPI tables and fetch specific
+ field values. The ItemPtr member of the ACPI_PARSER structure (where used)
+ is updated by this parser function to point to the selected field data
+ (e.g. useful for variable length nested fields).
+
+ @param [in] Trace Trace the ACPI fields TRUE else only parse the
+ table.
+ @param [in] Indent Number of spaces to indent the output.
+ @param [in] AsciiName Optional pointer to an ASCII string that describes
+ the table being parsed.
+ @param [in] Ptr Pointer to the start of the buffer.
+ @param [in] Length Length of the buffer pointed by Ptr.
+ @param [in] Parser Pointer to an array of ACPI_PARSER structure that
+ describes the table being parsed.
+ @param [in] ParserItems Number of items in the ACPI_PARSER array.
+
+ @retval Number of bits parsed.
+**/
+UINT32
+EFIAPI
+ParseAcpiBitFields (
+ IN BOOLEAN Trace,
+ IN UINT32 Indent,
+ IN CONST CHAR8* AsciiName OPTIONAL,
+ IN UINT8* Ptr,
+ IN UINT32 Length,
+ IN CONST ACPI_PARSER* Parser,
+ IN UINT32 ParserItems
+);
+
/**
This is a helper macro to pass parameters to the Parser functions.

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
index 74056e72c359..d89c68ba3905 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
@@ -2,12 +2,14 @@
ACPI parser

Copyright (c) 2016 - 2021, Arm Limited. All rights reserved.
+ Copyright (c) 2021, AMD Incorporated. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
**/

#include <Uefi.h>
#include <Library/UefiLib.h>
#include <Library/UefiBootServicesTableLib.h>
+#include <Library/BaseMemoryLib.h>
#include "AcpiParser.h"
#include "AcpiView.h"
#include "AcpiViewConfig.h"
@@ -738,3 +740,178 @@ ParseAcpiHeader (

return BytesParsed;
}
+
+/**
+ This function is used to parse an ACPI table bitfield buffer.
+
+ The ACPI table buffer is parsed using the ACPI table parser information
+ specified by a pointer to an array of ACPI_PARSER elements. This parser
+ function iterates through each item on the ACPI_PARSER array and logs the ACPI table bitfields.
+
+ This function can optionally be used to parse ACPI tables and fetch specific
+ field values. The ItemPtr member of the ACPI_PARSER structure (where used)
+ is updated by this parser function to point to the selected field data
+ (e.g. useful for variable length nested fields).
+
+ @param [in] Trace Trace the ACPI fields TRUE else only parse the
+ table.
+ @param [in] Indent Number of spaces to indent the output.
+ @param [in] AsciiName Optional pointer to an ASCII string that describes
+ the table being parsed.
+ @param [in] Ptr Pointer to the start of the buffer.
+ @param [in] Length Length in bytes of the buffer pointed by Ptr.
+ @param [in] Parser Pointer to an array of ACPI_PARSER structure that
+ describes the table being parsed.
+ @param [in] ParserItems Number of items in the ACPI_PARSER array.
+
+ @retval Number of bits parsed.
+**/
+UINT32
+EFIAPI
+ParseAcpiBitFields (
+ IN BOOLEAN Trace,
+ IN UINT32 Indent,
+ IN CONST CHAR8* AsciiName OPTIONAL,
+ IN UINT8* Ptr,
+ IN UINT32 Length,
+ IN CONST ACPI_PARSER* Parser,
+ IN UINT32 ParserItems
+)
+{
+ UINT32 Index;
+ UINT32 Offset;
+ BOOLEAN HighLight;
+ UINTN OriginalAttribute;
+
+ UINT64 Data;
+ UINT64 BitsData;
+
+ if (Length == 0 || Length > 8) {
+ IncrementErrorCount ();
+ Print (
+ L"\nERROR: Bitfield Length(%d) is zero or exceeding the 64 bit limit.\n",
+ Length
+ );
+ return 0;
+ }
+ //
+ // set local variables to suppress incorrect compiler/analyzer warnings
+ //
+ OriginalAttribute = 0;
+ Offset = 0;
+
+ // Increment the Indent
+ gIndent += Indent;
+
+ CopyMem ((VOID *)&BitsData, (VOID *)Ptr, Length);
+ if (Trace && (AsciiName != NULL)){
+ HighLight = GetColourHighlighting ();
+
+ if (HighLight) {
+ OriginalAttribute = gST->ConOut->Mode->Attribute;
+ gST->ConOut->SetAttribute (
+ gST->ConOut,
+ EFI_TEXT_ATTR(EFI_YELLOW,
+ ((OriginalAttribute&(BIT4|BIT5|BIT6))>>4))
+ );
+ }
+ Print (
+ L"%*a%-*a :\n",
+ gIndent,
+ "",
+ (OUTPUT_FIELD_COLUMN_WIDTH - gIndent),
+ AsciiName
+ );
+ if (HighLight) {
+ gST->ConOut->SetAttribute (gST->ConOut, OriginalAttribute);
+ }
+ }
+
+ for (Index = 0; Index < ParserItems; Index++) {
+ if ((Offset + Parser[Index].Length) > (Length * 8)) {
+ // For fields outside the buffer length provided, reset any pointers
+ // which were supposed to be updated by this function call
+ if (Parser[Index].ItemPtr != NULL) {
+ *Parser[Index].ItemPtr = NULL;
+ }
+
+ // We don't parse past the end of the max length specified
+ continue;
+ }
+
+ if (Parser[Index].Length == 0) {
+ // don't parse the bitfield whose length is zero
+ continue;
+ }
+
+ if (GetConsistencyChecking () &&
+ (Offset != Parser[Index].Offset)) {
+ IncrementErrorCount ();
+ Print (
+ L"\nERROR: %a: Offset Mismatch for %s\n"
+ L"CurrentOffset = %d FieldOffset = %d\n",
+ AsciiName,
+ Parser[Index].NameStr,
+ Offset,
+ Parser[Index].Offset
+ );
+ }
+
+ // extract Bitfield data for the current item
+ Data = (BitsData >> Parser[Index].Offset) & ~(~0ULL << Parser[Index].Length);
+
+ if (Trace) {
+ // if there is a Formatter function let the function handle
+ // the printing else if a Format is specified in the table use
+ // the Format for printing
+ PrintFieldName (2, Parser[Index].NameStr);
+ if (Parser[Index].PrintFormatter != NULL) {
+ Parser[Index].PrintFormatter (Parser[Index].Format, (UINT8 *)&Data);
+ } else if (Parser[Index].Format != NULL) {
+ // convert bit length to byte length
+ switch ((Parser[Index].Length + 7) >> 3) {
+ // print the data depends on byte size
+ case 1:
+ DumpUint8 (Parser[Index].Format, (UINT8 *)&Data);
+ break;
+ case 2:
+ DumpUint16 (Parser[Index].Format, (UINT8 *)&Data);
+ break;
+ case 3:
+ case 4:
+ DumpUint32 (Parser[Index].Format, (UINT8 *)&Data);
+ break;
+ case 5:
+ case 6:
+ case 7:
+ case 8:
+ DumpUint64 (Parser[Index].Format, (UINT8 *)&Data);
+ break;
+ default:
+ Print (
+ L"\nERROR: %a: CANNOT PARSE THIS FIELD, Field Length = %d\n",
+ AsciiName,
+ Parser[Index].Length
+ );
+ } // switch
+ }
+ // Validating only makes sense if we are tracing
+ // the parsed table entries, to report by table name.
+ if (GetConsistencyChecking () &&
+ (Parser[Index].FieldValidator != NULL)) {
+ Parser[Index].FieldValidator ((UINT8 *)&Data, Parser[Index].Context);
+ }
+ Print (L"\n");
+ } // if (Trace)
+
+ if (Parser[Index].ItemPtr != NULL) {
+ *Parser[Index].ItemPtr = (VOID*)(UINT8 *)&Data;
+ }
+
+ Offset += Parser[Index].Length;
+ } // for
+
+ // Decrement the Indent
+ gIndent -= Indent;
+ return Offset;
+}
--
2.25.1


[PATCH v3 0/2] ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser

Abdul Lateef Attar
 

Hi Sami, Zhichao,
Addressed all review comments.
Created two different patches, one for bitfield parser and
another for FADT Flags parser.

Thanks
AbduL

REF : https://github.com/abdattar/edk2/tree/FadtFlagsParser

Cc: Ray Ni <ray.ni@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
Abdul Lateef Attar (2):
ShellPkg/AcpiView: Adds ACPI_PARSER bitfield parser
ShellPkg/AcpiView: PrintFormatter for FADT Flags field

ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 45 +++++
ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 177 ++++++++++++++++++++
ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 59 ++++++-
3 files changed, 280 insertions(+), 1 deletion(-)

--
2.25.1


Re: [PATCH V3 3/3] OvmfPkg: Move LocalApicTimerDxe to UefiCpuPkg

Yao, Jiewen
 

Thanks Ray.

I prefer to put the LocalApicTimerDxe to UefiCpuPkg, because it is hardware feature.
There is no specific OVMF or QEMU related thing there. It does not make sense to keep it to OvmfPkg.

Your quality concern is valid. I think the quality expectation for LocalApicTimerDxe should be same, no matter it is in OvmfPkg or UefiCpuPkg.
I don't think the expectation is that the quality can be lower if it is in OvmfPkg and it must be higher if it is in UefiCpuPkg.

All quality issues of LocalApicTimerDxe should be resolved, no matter where it is finally located.

Thank you
Yao Jiewen

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Monday, November 22, 2021 11:05 AM
To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann
<kraxel@redhat.com>; Anthony Perard <anthony.perard@citrix.com>; Julien
Grall <julien@xen.org>; Dong, Eric <eric.dong@intel.com>
Subject: RE: [PATCH V3 3/3] OvmfPkg: Move LocalApicTimerDxe to UefiCpuPkg

Min,
What's the reason of moving this driver to UefiCpuPkg?

When the LocalApicTimerDxe is in OvmfPkg, it's clear that this driver is only used
by OVMF/QEMU platform.
Now since the patch moves the driver to UefiCpuPkg, it's possible that other
platforms may choose this driver as the timer driver in DXE phase.

So, we need to make sure the quality of this driver is good enough for a broad
scope of platforms.

1. What's the issue when this driver still stays in OvmfPkg? If it's a must that this
driver stays in UefiCpuPkg, please help to address following questions:

2. Can SourceLevelDebug (rely on Local APIC timer) work if this driver is chosen
as DXE timer driver?
3. Can detailed comments be added for " @bug : This does not handle missed
timer interrupts" in TimerInterruptHandler()?
4. Can detailed comments be added for " DisableInterrupts ();" in
TimerInterruptHandler()?
5. In general what kinds of platforms are capable of using this driver as Timer
driver?

Thanks,
Ray

-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Monday, November 8, 2021 2:08 PM
To: devel@edk2.groups.io
Cc: Xu, Min M <min.m.xu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
Gerd Hoffmann <kraxel@redhat.com>; Anthony Perard
<anthony.perard@citrix.com>; Julien Grall <julien@xen.org>; Dong, Eric
<eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: [PATCH V3 3/3] OvmfPkg: Move LocalApicTimerDxe to UefiCpuPkg

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3711

OvmfPkg.dec is removed from [Packages] because it doesn't depend
on OvmfPkg. LocalApicTimerDxe is moved to UefiCpuPkg.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/AmdSev/AmdSevX64.dsc | 2 +-
OvmfPkg/AmdSev/AmdSevX64.fdf | 2 +-
OvmfPkg/Microvm/MicrovmX64.dsc | 2 +-
OvmfPkg/Microvm/MicrovmX64.fdf | 2 +-
OvmfPkg/OvmfPkgIa32.dsc | 2 +-
OvmfPkg/OvmfPkgIa32.fdf | 2 +-
OvmfPkg/OvmfPkgIa32X64.dsc | 2 +-
OvmfPkg/OvmfPkgIa32X64.fdf | 2 +-
OvmfPkg/OvmfPkgX64.dsc | 2 +-
OvmfPkg/OvmfPkgX64.fdf | 2 +-
OvmfPkg/OvmfXen.dsc | 2 +-
OvmfPkg/OvmfXen.fdf | 2 +-
.../LocalApicTimerDxe/LocalApicTimerDxe.c | 0
.../LocalApicTimerDxe/LocalApicTimerDxe.h | 0
.../LocalApicTimerDxe/LocalApicTimerDxe.inf | 6 ++++--
15 files changed, 16 insertions(+), 14 deletions(-)
rename {OvmfPkg => UefiCpuPkg}/LocalApicTimerDxe/LocalApicTimerDxe.c
(100%)
rename {OvmfPkg => UefiCpuPkg}/LocalApicTimerDxe/LocalApicTimerDxe.h
(100%)
rename {OvmfPkg => UefiCpuPkg}/LocalApicTimerDxe/LocalApicTimerDxe.inf
(79%)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc
b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 88c51dfe8337..888fc24f1b58 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -674,7 +674,7 @@
MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
UefiCpuPkg/CpuDxe/CpuDxe.inf
- OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf

OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf
b/OvmfPkg/AmdSev/AmdSevX64.fdf
index 7489b04198fe..659810f96bec 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -208,7 +208,7 @@ INF
MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
INF UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
INF UefiCpuPkg/CpuDxe/CpuDxe.inf
-INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
INF
OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
INF MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
diff --git a/OvmfPkg/Microvm/MicrovmX64.dsc
b/OvmfPkg/Microvm/MicrovmX64.dsc
index 1a0e848f8356..fcb8b571a041 100644
--- a/OvmfPkg/Microvm/MicrovmX64.dsc
+++ b/OvmfPkg/Microvm/MicrovmX64.dsc
@@ -656,7 +656,7 @@

MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
- OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
UefiCpuPkg/CpuDxe/CpuDxe.inf

OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
diff --git a/OvmfPkg/Microvm/MicrovmX64.fdf
b/OvmfPkg/Microvm/MicrovmX64.fdf
index ac9efba26811..d02e88e2a48e 100644
--- a/OvmfPkg/Microvm/MicrovmX64.fdf
+++ b/OvmfPkg/Microvm/MicrovmX64.fdf
@@ -215,7 +215,7 @@ INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
INF MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
-INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
INF UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
INF UefiCpuPkg/CpuDxe/CpuDxe.inf
INF
OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index d0e9f3ca05f6..f8c8ef1e58be 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -763,7 +763,7 @@
OvmfPkg/8259InterruptControllerDxe/8259.inf
OvmfPkg/8254TimerDxe/8254Timer.inf
!else
- OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
!endif

OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index b7b35a3a490a..321d4a871afa 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -218,7 +218,7 @@ INF UefiCpuPkg/CpuDxe/CpuDxe.inf
INF OvmfPkg/8259InterruptControllerDxe/8259.inf
INF OvmfPkg/8254TimerDxe/8254Timer.inf
!else
- INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
!endif
INF
OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index d3531d388e24..4de4ed21a5ca 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -777,7 +777,7 @@
OvmfPkg/8259InterruptControllerDxe/8259.inf
OvmfPkg/8254TimerDxe/8254Timer.inf
!else
- OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
!endif

OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index 986228a44c78..10e97c35001f 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -222,7 +222,7 @@ INF UefiCpuPkg/CpuDxe/CpuDxe.inf
INF OvmfPkg/8259InterruptControllerDxe/8259.inf
INF OvmfPkg/8254TimerDxe/8254Timer.inf
!else
- INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
!endif
INF
OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index c6ee624fc738..57b0c3c10826 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -775,7 +775,7 @@
OvmfPkg/8259InterruptControllerDxe/8259.inf
OvmfPkg/8254TimerDxe/8254Timer.inf
!else
- OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
!endif

OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 99339e73bb51..b52c43127845 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -238,7 +238,7 @@ INF UefiCpuPkg/CpuDxe/CpuDxe.inf
INF OvmfPkg/8259InterruptControllerDxe/8259.inf
INF OvmfPkg/8254TimerDxe/8254Timer.inf
!else
- INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
!endif
INF
OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 7c4c5412d102..d4a9e8f87def 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -551,7 +551,7 @@
MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf

MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
- OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
UefiCpuPkg/CpuDxe/CpuDxe.inf
OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf
index 196853740753..76934d354fa6 100644
--- a/OvmfPkg/OvmfXen.fdf
+++ b/OvmfPkg/OvmfXen.fdf
@@ -298,7 +298,7 @@ INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
INF MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
-INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
INF UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
INF UefiCpuPkg/CpuDxe/CpuDxe.inf
INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
similarity index 100%
rename from OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
rename to UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
diff --git a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.h
b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.h
similarity index 100%
rename from OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.h
rename to UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.h
diff --git a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
similarity index 79%
rename from OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
rename to UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
index 3ad28a148c5b..4f2b4db9e5dc 100644
--- a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+++ b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
@@ -1,6 +1,9 @@
## @file
# Local APIC timer driver that provides Timer Arch protocol.
-# PcdFSBClock is defined in MdePkg and it should be set by the consumer.
+#
+# This driver is to support fixed frequency. If a real platform happens
+# to have fixed frequency, then it can be used. In this case the consumer
+# should set PcdFSBClock which is defined in MdePkg.
#
# Copyright (c) 2005 - 2019, Intel Corporation. All rights reserved.<BR>
# Copyright (c) 2019, Citrix Systems, Inc.
@@ -21,7 +24,6 @@
[Packages]
MdePkg/MdePkg.dec
UefiCpuPkg/UefiCpuPkg.dec
- OvmfPkg/OvmfPkg.dec

[LibraryClasses]
UefiBootServicesTableLib
--
2.29.2.windows.2


Re: [PATCH] UefiPayloadPkg/UefiPayloadPkg.dsc:Add BootManagerLib for BootManagerMenuApp

Ni, Ray
 

Reviewed-by: Ray Ni <ray.ni@intel.com>

-----Original Message-----
From: Feng, Ning <ning.feng@intel.com>
Sent: Thursday, November 18, 2021 6:10 PM
To: devel@edk2.groups.io
Cc: Feng, Ning <ning.feng@intel.com>; Dong, Guo <guo.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Ma, Maurice <maurice.ma@intel.com>; You, Benjamin <benjamin.you@intel.com>
Subject: [PATCH] UefiPayloadPkg/UefiPayloadPkg.dsc:Add BootManagerLib for BootManagerMenuApp

Add PlatformBootManagerLibconstructor for BootManagerMenuApp,
to get the value PcdBootManagefile overrided by platform side.

Signed-off-by: Ning Feng <ning.feng@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>
---
UefiPayloadPkg/UefiPayloadPkg.dsc | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc b/UefiPayloadPkg/UefiPayloadPkg.dsc
index 0df5f827c9..3e05da9877 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -519,8 +519,10 @@
NULL|MdeModulePkg/Library/BootManagerUiLib/BootManagerUiLib.inf

NULL|MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceManagerUiLib.inf

}

- MdeModulePkg/Application/BootManagerMenuApp/BootManagerMenuApp.inf

-

+ MdeModulePkg/Application/BootManagerMenuApp/BootManagerMenuApp.inf {

+ <LibraryClasses>

+ NULL|UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf

+ }

PcAtChipsetPkg/HpetTimerDxe/HpetTimerDxe.inf

MdeModulePkg/Universal/Metronome/Metronome.inf

MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf

--
2.26.2.windows.1


Re: [PATCH V3 23/29] UefiCpuPkg: Update AddressEncMask in CpuPageTable

Ni, Ray
 

Gerd, thanks. I am about to raise the same comments...

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd Hoffmann
Sent: Wednesday, November 3, 2021 3:00 PM
To: Xu, Min M <min.m.xu@intel.com>
Cc: devel@edk2.groups.io; Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V3 23/29] UefiCpuPkg: Update AddressEncMask in CpuPageTable

Hi,

+ gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask ## CONSUMES
AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) &
PAGING_1G_ADDRESS_MASK_64;
+ if (AddressEncMask == 0) {
+ AddressEncMask = PcdGet64 (PcdTdxSharedBitMask) &
+ PAGING_1G_ADDRESS_MASK_64; }
Looks like two PCDs for basically the same thing.
Should we create a common CC PCD here?

take care,
Gerd


Re: [PATCH V3 3/3] OvmfPkg: Move LocalApicTimerDxe to UefiCpuPkg

Ni, Ray
 

Min,
What's the reason of moving this driver to UefiCpuPkg?

When the LocalApicTimerDxe is in OvmfPkg, it's clear that this driver is only used by OVMF/QEMU platform.
Now since the patch moves the driver to UefiCpuPkg, it's possible that other platforms may choose this driver as the timer driver in DXE phase.

So, we need to make sure the quality of this driver is good enough for a broad scope of platforms.

1. What's the issue when this driver still stays in OvmfPkg? If it's a must that this driver stays in UefiCpuPkg, please help to address following questions:

2. Can SourceLevelDebug (rely on Local APIC timer) work if this driver is chosen as DXE timer driver?
3. Can detailed comments be added for " @bug : This does not handle missed timer interrupts" in TimerInterruptHandler()?
4. Can detailed comments be added for " DisableInterrupts ();" in TimerInterruptHandler()?
5. In general what kinds of platforms are capable of using this driver as Timer driver?

Thanks,
Ray

-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Monday, November 8, 2021 2:08 PM
To: devel@edk2.groups.io
Cc: Xu, Min M <min.m.xu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Anthony Perard <anthony.perard@citrix.com>; Julien Grall <julien@xen.org>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: [PATCH V3 3/3] OvmfPkg: Move LocalApicTimerDxe to UefiCpuPkg

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3711

OvmfPkg.dec is removed from [Packages] because it doesn't depend
on OvmfPkg. LocalApicTimerDxe is moved to UefiCpuPkg.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/AmdSev/AmdSevX64.dsc | 2 +-
OvmfPkg/AmdSev/AmdSevX64.fdf | 2 +-
OvmfPkg/Microvm/MicrovmX64.dsc | 2 +-
OvmfPkg/Microvm/MicrovmX64.fdf | 2 +-
OvmfPkg/OvmfPkgIa32.dsc | 2 +-
OvmfPkg/OvmfPkgIa32.fdf | 2 +-
OvmfPkg/OvmfPkgIa32X64.dsc | 2 +-
OvmfPkg/OvmfPkgIa32X64.fdf | 2 +-
OvmfPkg/OvmfPkgX64.dsc | 2 +-
OvmfPkg/OvmfPkgX64.fdf | 2 +-
OvmfPkg/OvmfXen.dsc | 2 +-
OvmfPkg/OvmfXen.fdf | 2 +-
.../LocalApicTimerDxe/LocalApicTimerDxe.c | 0
.../LocalApicTimerDxe/LocalApicTimerDxe.h | 0
.../LocalApicTimerDxe/LocalApicTimerDxe.inf | 6 ++++--
15 files changed, 16 insertions(+), 14 deletions(-)
rename {OvmfPkg => UefiCpuPkg}/LocalApicTimerDxe/LocalApicTimerDxe.c (100%)
rename {OvmfPkg => UefiCpuPkg}/LocalApicTimerDxe/LocalApicTimerDxe.h (100%)
rename {OvmfPkg => UefiCpuPkg}/LocalApicTimerDxe/LocalApicTimerDxe.inf (79%)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 88c51dfe8337..888fc24f1b58 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -674,7 +674,7 @@
MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
UefiCpuPkg/CpuDxe/CpuDxe.inf
- OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
index 7489b04198fe..659810f96bec 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -208,7 +208,7 @@ INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
INF UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
INF UefiCpuPkg/CpuDxe/CpuDxe.inf
-INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
INF OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
INF MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
diff --git a/OvmfPkg/Microvm/MicrovmX64.dsc b/OvmfPkg/Microvm/MicrovmX64.dsc
index 1a0e848f8356..fcb8b571a041 100644
--- a/OvmfPkg/Microvm/MicrovmX64.dsc
+++ b/OvmfPkg/Microvm/MicrovmX64.dsc
@@ -656,7 +656,7 @@

MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
- OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
UefiCpuPkg/CpuDxe/CpuDxe.inf
OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
diff --git a/OvmfPkg/Microvm/MicrovmX64.fdf b/OvmfPkg/Microvm/MicrovmX64.fdf
index ac9efba26811..d02e88e2a48e 100644
--- a/OvmfPkg/Microvm/MicrovmX64.fdf
+++ b/OvmfPkg/Microvm/MicrovmX64.fdf
@@ -215,7 +215,7 @@ INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
INF MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
-INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
INF UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
INF UefiCpuPkg/CpuDxe/CpuDxe.inf
INF OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index d0e9f3ca05f6..f8c8ef1e58be 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -763,7 +763,7 @@
OvmfPkg/8259InterruptControllerDxe/8259.inf
OvmfPkg/8254TimerDxe/8254Timer.inf
!else
- OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
!endif
OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index b7b35a3a490a..321d4a871afa 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -218,7 +218,7 @@ INF UefiCpuPkg/CpuDxe/CpuDxe.inf
INF OvmfPkg/8259InterruptControllerDxe/8259.inf
INF OvmfPkg/8254TimerDxe/8254Timer.inf
!else
- INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
!endif
INF OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index d3531d388e24..4de4ed21a5ca 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -777,7 +777,7 @@
OvmfPkg/8259InterruptControllerDxe/8259.inf
OvmfPkg/8254TimerDxe/8254Timer.inf
!else
- OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
!endif
OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index 986228a44c78..10e97c35001f 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -222,7 +222,7 @@ INF UefiCpuPkg/CpuDxe/CpuDxe.inf
INF OvmfPkg/8259InterruptControllerDxe/8259.inf
INF OvmfPkg/8254TimerDxe/8254Timer.inf
!else
- INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
!endif
INF OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index c6ee624fc738..57b0c3c10826 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -775,7 +775,7 @@
OvmfPkg/8259InterruptControllerDxe/8259.inf
OvmfPkg/8254TimerDxe/8254Timer.inf
!else
- OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
!endif
OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 99339e73bb51..b52c43127845 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -238,7 +238,7 @@ INF UefiCpuPkg/CpuDxe/CpuDxe.inf
INF OvmfPkg/8259InterruptControllerDxe/8259.inf
INF OvmfPkg/8254TimerDxe/8254Timer.inf
!else
- INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
!endif
INF OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.inf
INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 7c4c5412d102..d4a9e8f87def 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -551,7 +551,7 @@
MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf

MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
- OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+ UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
UefiCpuPkg/CpuDxe/CpuDxe.inf
OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf
index 196853740753..76934d354fa6 100644
--- a/OvmfPkg/OvmfXen.fdf
+++ b/OvmfPkg/OvmfXen.fdf
@@ -298,7 +298,7 @@ INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
INF MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
-INF OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+INF UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
INF UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
INF UefiCpuPkg/CpuDxe/CpuDxe.inf
INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
diff --git a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
similarity index 100%
rename from OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
rename to UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.c
diff --git a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.h b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.h
similarity index 100%
rename from OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.h
rename to UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.h
diff --git a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
similarity index 79%
rename from OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
rename to UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
index 3ad28a148c5b..4f2b4db9e5dc 100644
--- a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+++ b/UefiCpuPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
@@ -1,6 +1,9 @@
## @file
# Local APIC timer driver that provides Timer Arch protocol.
-# PcdFSBClock is defined in MdePkg and it should be set by the consumer.
+#
+# This driver is to support fixed frequency. If a real platform happens
+# to have fixed frequency, then it can be used. In this case the consumer
+# should set PcdFSBClock which is defined in MdePkg.
#
# Copyright (c) 2005 - 2019, Intel Corporation. All rights reserved.<BR>
# Copyright (c) 2019, Citrix Systems, Inc.
@@ -21,7 +24,6 @@
[Packages]
MdePkg/MdePkg.dec
UefiCpuPkg/UefiCpuPkg.dec
- OvmfPkg/OvmfPkg.dec

[LibraryClasses]
UefiBootServicesTableLib
--
2.29.2.windows.2


Event: TianoCore Design Meeting - APAC/NAMO - 11/26/2021 #cal-reminder

devel@edk2.groups.io Calendar <noreply@...>
 

Reminder: TianoCore Design Meeting - APAC/NAMO

When:
11/26/2021
9:30am to 10:30am
(UTC+08:00) Asia/Shanghai

Where:
Microsoft Teams

Organizer: Ray Ni ray.ni@...

View Event

Description:

TOPIC

  1. NA

For more info, see here: https://www.tianocore.org/design-meeting/


Microsoft Teams meeting

Join on your computer or mobile app

Click here to join the meeting

Join with a video conferencing device

teams@...

Video Conference ID: 119 715 416 0

Alternate VTC dialing instructions

Learn More | Meeting options


Re: [PATCH V3 15/29] OvmfPkg: Update SecEntry.nasm to support Tdx

Yao, Jiewen
 

-----Original Message-----
From: Gerd Hoffmann <kraxel@redhat.com>
Sent: Friday, November 19, 2021 11:12 PM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
Lendacky <thomas.lendacky@amd.com>
Subject: Re: [PATCH V3 15/29] OvmfPkg: Update SecEntry.nasm to support Tdx

Hi,

Comment on config-B.
I'm sure I've asked this before: Why skip the PEI phase? So far
I have not seen any convincing argument for it.
Skipping PEI phase is valid architecture design.
Sure.

Second, the confidential computing changes the threat model
completely. One of our goal is to simplify the design for CC-firmware
(TDVF) - remove unnecessary modules, remove unnecessary interface,
make the image smaller and faster. That will reduce the validation
effort, too.

That is the main motivation.
That totally makes sense. I expect TDVF Config-B will look alot like
the existing AmdSev configuration variant which is stripped down too.
[Jiewen] I don't think TDVF config-B will be like the AMD SEV is right statement.
TDVF and SEV are two different platforms.
Intel mainly focuses on TDVF and we will let AMD defines the feature set in SEV.
They MAY be alike if possible.
But difference is also acceptable if there is architecture difference or different decision in different company.


No SMM support, no network stack, ...

There wouldn't be left much in PEI beside PeiCore and
OvmfPkg/PlatformPei.

But I don't see how dropping the PEI phase altogether helps much in
stripping down the firmware image. The initialization currently handled
by OvmfPkg/PlatformPei must happen somewhere else instead. Given SEC is
a very restricted environment I don't expect the code can be shared
easily, so we will probably end up with code duplication. Also two
different boot workflows which I fear can easily introduce subtle bugs
due to differences like a initialization order changes. This is what I
see as maintenance problem.
[Jiewen] I don't think this is right statement.
In Tiano history, there were security bugs exposed in PEI phase, even the PEI Core on FV processing.
I do see the value to skip PEI core.

Again, I am very familiar with non-PEI flow.
Back to 10 years ago, I was maintainer of a non-PEI platform (named DUET) and we jumped from SEC to DXE directly.
I don't see any maintenance problem.



Config-A is to keep current architecture, to maximum compatible with
OVMF. And we don't remove VMM out of TCB.
Config-B is to have a new TDVF design, to maximum satisfy the security
requirement. And we remove VMM out of TCB.
Sure.

config-a is ovmf with tdx support added, all uefi features present, only
basic tdx/sev support (basically support memory encryption).

config-b is simliar to AmdSev (maybe we'll merge them some day),
stripped down uefi feature set (no network etc), full tdx support
including attestation etc.
[Jiewen] I think we are debating two different things.
Your statement that "config-B is similar to AmdSev" does not support the statement "config-B should be adopt what AmdSev chooses".
TDVF config-B is proposed by Intel. AMD SEV is proposed by AMD. I respect SEV people and I *may* propose something, but I will ack their decision.
I would not force them to accept the TDVF config-B. And vice versa.




I don't want question all that. I still don't see the point in dropping
the PEI phase and make config-b work different that all other ovmf
variants though.
[Jiewen] My point is simple - Threat Model is different.
That causes security objective difference and design difference.
Each CC env owner should have freedom to choose what they want to enforce.
IMHO, they are the policy decision maker.



Jiewen argued this is a simplification. Which is not completely wrong,
but it's also only half the truth. Switching all OVMF builds over to
PEI-less boot doesn't work because some features supported by OVMF
depend on PEI Modules. Therefore TDX Config-B skipping the PEI phase
means we would have to maintain two boot work flows (with and without
PEI phase) for OVMF. Which in turn would imply more work for
maintenance, testing and so on.
[Jiewen] I am not asking your to OVMF build to PEI-less.
But if you want to do, I will not object.
s3, smm, tpm and maybe more depends on pei modules, so dropping the PEi
phase is not an option for a full-featured OVMF build. So I'd very much
prefer all ovmf variants (including tdvf) use the PEI phase.

On contrast, if we keep PEI for config B, it adds extra burden from
security assurance perspective. That means, every issue in PEI may be
exposed to TDVF.
Same for all other modules used by tdvf.

The list of affected PEI modules is rather short though. I think it's
only PeiCore and DxeIpl. PlatformPei doesn't count as the code wouldn't
go away but would be moved to SEC (and maybe parts of it to DXE).

Comparing the effort to maintain the work flow and the effort to
handle potential security issue, I would choose to maintain the work
flow. I have experience to wait for 1 year embargo to fix EDKII
security issue, it is very painful and brings huge burden.
The security workflow is a serious problem indeed. Not only for TDVF,
also for OVMF in general, and other platforms too. We should certainly
try to improve it.
[Jiewen] If you look at how we state config-A and config-B again, you will find we made difference statement.
I copy it here again.
1) Config-A is to keep current architecture, to maximum compatible with OVMF. And we don't remove VMM out of TCB.
2) Config-B is to have a new TDVF design, to maximum satisfy the security requirement. And we remove VMM out of TCB.

Because of the threat model difference, in config-A, we can safely make some assumption that the VMM is benign and VMM will not input malicious data. As such, we might not perform data validation. We just trust VMM input.
However, in config-B, VMM is malicious, which means we need be careful to NOT trust VMM at any time.

The code in config-A and config-B may do totally different thing to handle the difference situation.

I don't think it is hidden assumption that if TDVF need do some check, then a generic OVMF need do this check.
If OVMF trusts the VMM, the check might be totally unnecessary.



I'm not going to open that discussion in this thread. But let me drop
two links two links to osfc talk and workshop (Not 30th + Dec 1st),
titled "The firmware supply-chain security is broken: can we fix it?"

https://talks.osfc.io/osfc2021/talk/D9X39Z/
https://talks.osfc.io/osfc2021/talk/E9YYJF/

I want TDVF be consistent with the rest of OVMF. Makes long-term
maintenance easier. Building a single binary for both SEV and TDX with
full confidential computing support (including config-b features) will
be easier too.
[Jiewen] I am not convinced that TDVF be consist with rest of OVMF.
The goal of project is different. The choice can be different.
I don't see a reason that one platform must be in this way, just because other
platform does in this way.

Hmm? Seeing TDVF as "other platform" is a rather strange view given
that we are integrating tdx support into OVMF right now ...
[Jiewen] This is how Intel views the "platform".
In history, we call this one binary mode is "multiple-platform" or "multiple-SKU".
That means we only have one binary, and this single binary can boot different platforms, which has similar CPU or silicon but different platform board design.
And there will be platform specific code flow, such as
Switch (PlatformId) {
Case PlatformA:
{do platformA init}
Case PlatformB:
{do platformB init}
}

If you treat CC_TYPE to be platformID, it perfectly matches.

Also a platform may has extra module (a driver or an FV) to support the platform specific feature. Or a platform may much simpler design and skip some drivers.
The actual multi-platform design is even more complicated, because we also have group concept. So I will stop the discussion here.




I think a PEI-less TDVF is much easier to maintain, comparing with
complicated OVMF flow, at least from security perspective. The less
code we have, the less issue we have.
Well, we will have TDX support in the normal OVMF workflow anyway,
because we need that for config-a. Why use and maintain something
different for config-b? That looks rather pointless to me ...
[Jiewen] I think I have explained a lot above.
The key difference between config-a and config-b is threat mode.
I even propose config-a skip PEI phase. I am persuaded to let config-a adopt the OVMF way, because the threat model of config-A is same as the normal OVMF.
But config-B is NOT.
Different threat model drives different solution.
I completely don't understand why they must be same.

If you force me to add PEI to config-B. Finally, that causes TDVF config-B is compromised due to an issue in PEI.
Who will take the responsibility? Will you or RedHat take that?







take care,
Gerd


Re: [PATCH] MdePkg: DebugLib: Compilation fix for clang-13.

Michael D Kinney
 

Hi Mikhail,

For RELEASE GCC5 toolchains in tools_def.txt, I see this warning is disabled. Likely for the same reason.

RELEASE_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os -Wno-unused-but-set-variable -Wno-unused-const-variable
RELEASE_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os -Wno-unused-but-set-variable -Wno-unused-const-variable
RELEASE_GCC5_ARM_CC_FLAGS = DEF(GCC5_ARM_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable
RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable

I think it would be better to ignore the same warning in RELEASE CLANG38 tool chains with an update to tools_def.txt instead of DebugLib.h.

Unless you are thinking that disabling this warning is hiding some bad code practices that need to be cleaned up.

Best regards,

Mike

-----Original Message-----
From: Mikhail Krichanov <krichanov@ispras.ru>
Sent: Friday, November 19, 2021 1:05 AM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
<zhiguang.liu@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>; Mikhail Krichanov <krichanov@ispras.ru>
Subject: [PATCH] MdePkg: DebugLib: Compilation fix for clang-13.

build -a X64 -t CLANG38 -b RELEASE -p OvmfPkg/OvmfPkgX64.dsc
results in
UDK/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c:1284:31:
error: variable 'Status' set but not used [-Werror,-Wunused-but-set-variable]
EFI_STATUS Status;
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3704

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Mikhail Krichanov <krichanov@ispras.ru>
---
MdePkg/Include/Library/DebugLib.h | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
index 4cacd4b8..7785e99d 100644
--- a/MdePkg/Include/Library/DebugLib.h
+++ b/MdePkg/Include/Library/DebugLib.h
@@ -370,7 +370,10 @@ UnitTestDebugAssert (
} \
} while (FALSE)
#else
- #define ASSERT(Expression)
+ #define ASSERT(Expression) \
+ do { \
+ (VOID) (Expression); \
+ } while (FALSE)
#endif

/**
@@ -392,6 +395,14 @@ UnitTestDebugAssert (
_DEBUG (Expression); \
} \
} while (FALSE)
+#elif defined(__GNUC__) || defined(__clang__)
+ #define DEBUG(Expression) \
+ do { \
+ _Pragma("GCC diagnostic push") \
+ _Pragma("GCC diagnostic ignored \"-Wunused-value\"") \
+ (VOID) Expression; \
+ _Pragma("GCC diagnostic pop") \
+ } while (FALSE)
#else
#define DEBUG(Expression)
#endif
@@ -419,7 +430,10 @@ UnitTestDebugAssert (
} \
} while (FALSE)
#else
- #define ASSERT_EFI_ERROR(StatusParameter)
+ #define ASSERT_EFI_ERROR(StatusParameter) \
+ do { \
+ (VOID) (StatusParameter); \
+ } while (FALSE)
#endif

/**
@@ -446,7 +460,10 @@ UnitTestDebugAssert (
} \
} while (FALSE)
#else
- #define ASSERT_RETURN_ERROR(StatusParameter)
+ #define ASSERT_RETURN_ERROR(StatusParameter) \
+ do { \
+ (VOID) (StatusParameter); \
+ } while (FALSE)
#endif

/**
--
2.20.1

201 - 220 of 84037