[edk2-devel] [PATCH] OvmfPkg: Disable PcdFirstTimeWakeUpAPsBySipi.

2023-08-16 Thread Yuanhao Xie
Deactivate PcdFirstTimeWakeUpAPsBySipi for AMD SEV and SNP to preserve
the original execution of INIT-SIPI-SIPI.

Cc: Eric Dong 
Cc: Ray Ni 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Jordan Justen 
Signed-off-by: Yuanhao Xie 
---
 OvmfPkg/AmdSev/AmdSevX64.dsc | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 2c6ed7c974..af52438fb2 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -467,6 +467,13 @@
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdConInConnectOnDemand|TRUE
   gUefiOvmfPkgTokenSpaceGuid.PcdBootRestrictToFirmware|TRUE
+  #
+  # PcdFirstTimeWakeUpAPsBySipi determines whether to employ
+  # SIPI instead of the INIT-SIPI-SIPI sequence during APs
+  # initialization. Deactivate this parameter for AMD SEV and
+  #  SNP to preserve the original execution of INIT-SIPI-SIPI.
+  #
+  gUefiCpuPkgTokenSpaceGuid.PcdFirstTimeWakeUpAPsBySipi|FALSE
 
 

 #
-- 
2.36.1.windows.1



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




Re: [edk2-devel] About EDK2 supports Self Modifying Code

2023-08-16 Thread Chao Li

Loop Mike.

Hi Mike and Liming,

Please refer to history emails. The Self-Modifying-Code(SMC) method has 
security risks and no one approve it. Ard and Pedro suggest using inline 
ASM in MdePkg.


For this problem, it can only dealt with in the preprocessing stage, 
because the CSR instructions can only recognize immediate values, so 
macro can only be used to expand and obtain the immediate values in the 
preprocessing stage.


So, can I create a .h file and using inline ASM? I think this file might 
be named Csr.h and located in MdePkg/Include/Register/LoongArch64/. I 
have two plans if possible:


*Plan A,
*

All of CSR register numbers are defined in Csr.h and include three CSR 
operations inline ASM, pseudocode:


#if defined(__GNUC__)

#define CsrRead(Sel)

({



)}

...

#endif


*Plan B,*

These three macros are defined in another file, which may be named 
CsrOperationGcc.h, and include it in Csr.h, pseudocode:


#if defined(__GNUC__)

#include "CsrPerationGcc.h"

#endif


Hope you can give your suggestions.



Thanks,
Chao
在 2023/8/16 05:26, Andrew Fish via groups.io 写道:




On Aug 15, 2023, at 11:48 AM, Ard Biesheuvel  wrote:

On Tue, 15 Aug 2023 at 18:31, Andrew Fish viagroups.io 


 wrote:




On Aug 15, 2023, at 8:39 AM, Pedro Falcato 
 wrote:


On Tue, Aug 15, 2023 at 4:05 PM Andrew Fish via groups.io
 wrote:


Chao,

From a quick google it looks like CSR* is used to access banks of 
registers that relate to things like performance counters and 
debug infrastructure and the number of banks of these register 
sets is likely implementation defined. Seems like we could 
introduce some Fixed At Build PCD values that define the maximum 
number of elements in a given bank.


If we are forced to use assembler it might be possible to write 
some macros that used the fixed at build values to only generate 
functions for banks that are needed for a given build. Then I 
think it becomes an exercise in dead code stripping the assembler. 
Most compilers generate assembler that contains functions that can 
be stripped as long as those functions follow certain rules.


As a side note it would be good for us to have an FAQ/Wiki entry 
for the dead code stripping rules for the various flavors of 
assembler. I know the Apple assembler has a unique take on this.


FWIW, I'm almost positive there's no DCE in GNU as (or llvm-as as
well). Unless you use something ffunction-sections
-fdata-sections-like, but then you're relying on the linker +
gc-sections to take care of it, just like GCC/clang would.



I guess I usually think of DCE as a linker job, since the linker 
knows the functions that are NOT called. At least from the Apple 
tools the DCE has the same rules if you are using Link Time 
Optimization or not. It is basically a flag in the object that tells 
the inker it is OK to follow the DCE rules around labels and remove 
stuff.


Worst case it seems like we could have macros that generate assembly 
files based on build time constants so we have one function per 
file. This might take a tweak to the build system, but I’d rather do 
that than have library functions that magically turn on Self 
Modifying Code.


Regardless of the answer I think documenting the rules is a useful 
exercises since needing to save size in firmware images is not an 
uncommon task.




There is already prior art in MdePkg where code targeting both GCC and
VS uses inline asm, so I don't see why we would make our lives
difficult and deviate from that for LoongArch.



If you look at the BaseLib you can see an example of the INF file[1] 
using C inline assembler for the GCC family[2] of compilers and NASM 
for the MSFT [3] tools. Maybe you can plan on using a similar pattern.


[1] 
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseLib/BaseLib.inf#L321
[2] 
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseLib/X64/GccInline.c
[3] 
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseLib/X64/CpuPause.nasm


Thanks,

Andrew Fish









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




Re: [edk2-devel] [PATCH] SecurityPkg: DxeTcg2PhysicalPresenceLib for Clear Graphics Screen To unblock and Display TPM messages

2023-08-16 Thread Yao, Jiewen
Hi Karunakar
Thanks for the patient.

I think my concern is universal, no matter it is after user input the key or 
not.
The clear-screen behavior is a *change*. And it may break the compatibility.

Unless all BIOS consumer in EDKII community agree the behavior, the risk is 
always there.


To move forward, I would like to hear the feedback from other OEM/ODM/IBV, who 
is consuming DxeTcg2PhysicalPresenceLib.

Thank you
Yao, Jiewen



From: Poosapalli, Karunakar 
Sent: Thursday, August 17, 2023 1:28 AM
To: Yao, Jiewen ; Gao, Liming ; 
devel@edk2.groups.io
Subject: RE: [edk2-devel] [PATCH] SecurityPkg: DxeTcg2PhysicalPresenceLib for 
Clear Graphics Screen To unblock and Display TPM messages

Hi @Yao, Jiewen,

Could you please share your thoughts. Please let me know if you have any 
queries or concerns.

Thanks & Regards
Karunakar Poosapalli
Firmware Principal Engineer, Client BIOS
Customer BIOS | Dell Core BIOS
CPG Software Engineering | Dell Technologies
Mobile +91 9951902957
karunakar_poosapa...@dell.com



Internal Use - Confidential
From: Poosapalli, Karunakar
Sent: Wednesday, August 9, 2023 9:59 AM
To: Yao, Jiewen; Gao, Liming; devel@edk2.groups.io
Subject: RE: [edk2-devel] [PATCH] SecurityPkg: DxeTcg2PhysicalPresenceLib for 
Clear Graphics Screen To unblock and Display TPM messages

Hi @Yao, Jiewen,

The proposed solution to clear screen will be called only when user has to 
provide the input key. This call will not execute in other conditions.
When there is a pending request in Tcg2, system will wait until user press 
input key. If there is no proper UI to customer, they feel it's system hang and 
go for customer support.
Even BIOS vendors also won't come to what is happening at customer box without 
any user information.
This is generic issue and not specific to any platform.

Thanks for sharing your thoughts.

Thanks & Regards
Karunakar Poosapalli
Firmware Principal Engineer, Client BIOS
Customer BIOS | Dell Core BIOS
CPG Software Engineering | Dell Technologies
Mobile +91 9951902957
karunakar_poosapa...@dell.com



Internal Use - Confidential
From: Yao, Jiewen mailto:jiewen@intel.com>>
Sent: Wednesday, August 9, 2023 4:44 AM
To: Poosapalli, Karunakar; Gao, Liming; 
devel@edk2.groups.io
Cc: Yao, Jiewen
Subject: RE: [edk2-devel] [PATCH] SecurityPkg: DxeTcg2PhysicalPresenceLib for 
Clear Graphics Screen To unblock and Display TPM messages


[EXTERNAL EMAIL]
My concern is that you unconditionally clear the screen. What if someone did 
want to show something on the screen?

This seems an incompatible change. That is why I think it should be a platform 
policy.

Another way is that you may consider to duplicate the library for your platform 
and clear it for your platform.

Thank you
Yao, Jiewen


From: Poosapalli, Karunakar 
mailto:karunakarpoosapa...@dell.com>>
Sent: Wednesday, August 9, 2023 2:13 AM
To: Yao, Jiewen mailto:jiewen@intel.com>>; Gao, 
Liming mailto:gaolim...@byosoft.com.cn>>; 
devel@edk2.groups.io; Poosapalli, Karunakar 
mailto:karunakarpoosapa...@dell.com>>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg: DxeTcg2PhysicalPresenceLib for 
Clear Graphics Screen To unblock and Display TPM messages

Hi @Yao, Jiewen,

Thanks for your review and feedback.
When there is a pending Tcg request, the control will be in Tcg2 library and it 
will not reach to BDS until user press the input key.
As there is no information the screen because of screen is blocked by some 
other messages/Logo.
So user won't able to press the key without any user information and control 
will not reach to BDS phase.

As TCG user confirmation is the highest priority and it blocks the POST until 
the user presses the input key. Before TCG messages Print on the console, there 
should be logic added to clear the graphics screen

Please let me know your thoughts.

Thanks & Regards
Karunakar Poosapalli
Firmware Principal Engineer, Client BIOS
Customer BIOS | Dell Core BIOS
CPG Software Engineering | Dell Technologies
Mobile +91 9951902957
karunakar_poosapa...@dell.com



Internal Use - Confidential
From: Yao, Jiewen mailto:jiewen@intel.com>>
Sent: Tuesday, July 25, 2023 5:48 PM
To: Poosapalli, Karunakar; Gao, Liming; 
devel@edk2.groups.io
Subject: RE: [edk2-devel] [PATCH] SecurityPkg: DxeTcg2PhysicalPresenceLib for 
Clear Graphics Screen To unblock and Display TPM messages


[EXTERNAL EMAIL]
Hello
I agree with you on the problem statement.

But I don't think this is a desired solution.
We expect Platform BDS to call the PhysicalPresenceLib. As such, why not clear 
the  screen in the platform BDS?

Thank you
Yao, Jiewen

From: Poosapalli, Karunakar 
mailto:karunakarpoosapa...@dell.com>>
Sent: Monday, July 

Re: [edk2-devel] [PATCH v2 5/7] SecurityPkg: Apply uncrustify formatting to relevant files

2023-08-16 Thread Yao, Jiewen
Reviewed-by: Jiewen Yao 

> -Original Message-
> From: VivianNK 
> Sent: Thursday, August 17, 2023 5:15 AM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen ; Wang, Jian J 
> Subject: [PATCH v2 5/7] SecurityPkg: Apply uncrustify formatting to relevant 
> files
> 
> Apply uncrustify formatting to GoogleTest cpp and header files.
> 
> 
> Cc: Jiewen Yao 
> Cc: Jian J Wang 
> Signed-off-by: Vivian Nowka-Keane 
> ---
> 
> SecurityPkg/Library/SecureBootVariableLib/GoogleTest/SecureBootVariableLibGo
> ogleTest.cpp | 205 
> 
> SecurityPkg/Test/Mock/Include/GoogleTest/Library/MockPlatformPKProtectionLi
> b.h   |   4 +-
> 
> SecurityPkg/Test/Mock/Library/GoogleTest/MockPlatformPKProtectionLib/MockP
> latformPKProtectionLib.cpp |   4 +-
>  3 files changed, 124 insertions(+), 89 deletions(-)
> 
> diff --git
> a/SecurityPkg/Library/SecureBootVariableLib/GoogleTest/SecureBootVariableLib
> GoogleTest.cpp
> b/SecurityPkg/Library/SecureBootVariableLib/GoogleTest/SecureBootVariableLib
> GoogleTest.cpp
> index c9190c8ffd61..f66c595a97a8 100644
> ---
> a/SecurityPkg/Library/SecureBootVariableLib/GoogleTest/SecureBootVariableLib
> GoogleTest.cpp
> +++
> b/SecurityPkg/Library/SecureBootVariableLib/GoogleTest/SecureBootVariableLib
> GoogleTest.cpp
> @@ -21,154 +21,189 @@ using namespace testing;
> 
> 
>  
> //
> 
>  class SetSecureBootModeTest : public Test {
> 
> -  protected:
> 
> -MockUefiRuntimeServicesTableLib RtServicesMock;
> 
> -UINT8   SecureBootMode;
> 
> -EFI_STATUS  Status;
> 
> +protected:
> 
> +  MockUefiRuntimeServicesTableLib RtServicesMock;
> 
> +  UINT8 SecureBootMode;
> 
> +  EFI_STATUS Status;
> 
> 
> 
> -void SetUp() override {
> 
> -  // Any random magic number can be used for these tests
> 
> -  SecureBootMode = 0xAB;
> 
> -}
> 
> +  void
> 
> +  SetUp (
> 
> +) override
> 
> +  {
> 
> +// Any random magic number can be used for these tests
> 
> +SecureBootMode = 0xAB;
> 
> +  }
> 
>  };
> 
> 
> 
>  // Test SetSecureBootMode() API from SecureBootVariableLib to verify the
> 
>  // expected error is returned when the call to gRT->SetVariable() fails.
> 
> -TEST_F(SetSecureBootModeTest, SetVarError) {
> 
> -  EXPECT_CALL(RtServicesMock, gRT_SetVariable)
> 
> -.WillOnce(Return(EFI_INVALID_PARAMETER));
> 
> +TEST_F (SetSecureBootModeTest, SetVarError) {
> 
> +  EXPECT_CALL (RtServicesMock, gRT_SetVariable)
> 
> +.WillOnce (Return (EFI_INVALID_PARAMETER));
> 
> 
> 
> -  Status = SetSecureBootMode(SecureBootMode);
> 
> -  EXPECT_EQ(Status, EFI_INVALID_PARAMETER);
> 
> +  Status = SetSecureBootMode (SecureBootMode);
> 
> +  EXPECT_EQ (Status, EFI_INVALID_PARAMETER);
> 
>  }
> 
> 
> 
>  // Test SetSecureBootMode() API from SecureBootVariableLib to verify the
> 
>  // expected secure boot mode is written to the correct variable in the call
> 
>  // to gRT->SetVariable().
> 
> -TEST_F(SetSecureBootModeTest, PropogateModeToSetVar) {
> 
> -  EXPECT_CALL(RtServicesMock,
> 
> -gRT_SetVariable(
> 
> -  Char16StrEq(EFI_CUSTOM_MODE_NAME),
> 
> -  BufferEq(, sizeof(EFI_GUID)),
> 
> +TEST_F (SetSecureBootModeTest, PropogateModeToSetVar) {
> 
> +  EXPECT_CALL (
> 
> +RtServicesMock,
> 
> +gRT_SetVariable (
> 
> +  Char16StrEq (EFI_CUSTOM_MODE_NAME),
> 
> +  BufferEq (, sizeof (EFI_GUID)),
> 
>EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS,
> 
> -  sizeof(SecureBootMode),
> 
> -  BufferEq(, sizeof(SecureBootMode
> 
> -.WillOnce(Return(EFI_SUCCESS));
> 
> +  sizeof (SecureBootMode),
> 
> +  BufferEq (, sizeof (SecureBootMode))
> 
> +  )
> 
> +)
> 
> +.WillOnce (Return (EFI_SUCCESS));
> 
> 
> 
> -  Status = SetSecureBootMode(SecureBootMode);
> 
> -  EXPECT_EQ(Status, EFI_SUCCESS);
> 
> +  Status = SetSecureBootMode (SecureBootMode);
> 
> +  EXPECT_EQ (Status, EFI_SUCCESS);
> 
>  }
> 
> 
> 
>  
> //
> 
>  class GetSetupModeTest : public Test {
> 
> -  protected:
> 
> -MockUefiRuntimeServicesTableLib RtServicesMock;
> 
> -UINT8   SetupMode;
> 
> -EFI_STATUS  Status;
> 
> -UINT8   ExpSetupMode;
> 
> +protected:
> 
> +  MockUefiRuntimeServicesTableLib RtServicesMock;
> 
> +  UINT8 SetupMode;
> 
> +  EFI_STATUS Status;
> 
> +  UINT8 ExpSetupMode;
> 
> 
> 
> -void SetUp() override {
> 
> -  // Any random magic number can be used for these tests
> 
> -  ExpSetupMode = 0xAB;
> 
> -}
> 
> +  void
> 
> +  SetUp (
> 
> +) override
> 
> +  {
> 
> +// Any random magic number can be used for these tests
> 
> +ExpSetupMode = 0xAB;
> 
> +  }
> 
>  };
> 
> 
> 
>  // Test GetSetupMode() API from SecureBootVariableLib to verify the expected
> 
>  // error is returned when the call to gRT->GetVariable() fails.
> 
> -TEST_F(GetSetupModeTest, GetVarError) {
> 
> 

Re: [edk2-devel] About EDK2 supports Self Modifying Code

2023-08-16 Thread Chao Li

Hi Pedro,

Sorry for the late reply, I was a bit busy yesterday.

I think the better way is to use inline asm, because this issue must has 
to be dealt with in preprocessing stage, because in other stages, it has 
no chance to get immediate value except using SMC. But then we should 
ask to the MdePkg maintainer if it is OK.



Thanks,
Chao
在 2023/8/15 23:35, Pedro Falcato 写道:

On Tue, Aug 15, 2023 at 9:20 AM Chao Li  wrote:

Hi Andrew,

Yes, you are right, I also think that SMC is a bit flawed in terms of security, 
but can we use some security mechanism to protect the SMC, like encryption and 
decryption? Sorry, I'm not consider mature enough about SMC security.

There isn't any. Actual use cases in something like a kernel are
heavily vetted and read-protected as soon as possible.


I can tell you real problem, there are some CSR instructions in LoongArch64 
that can only accept immediate value, for example: `csrrd $a0, 0x1`, the 0x1 is 
the selection of CSR register number, it can't use the registers to select. 
This operation should be in the MdePkg base library.

I know that .c or .h files in MdePkg shouldn't depend on a single compiler feature, so I can't 
use the GNU AT style inline ASM function(AT style inline supports input parameters 
being immedite value, use "i" option). In this case, I think using SMC can handle this, 
that is use register transfer the CSR registers selection, and dynamically modify CSR 
instructions during execution phase with reference to transfer register value, this way is depend 
on the .text section or target memory is executable and writable.

FYI, poking instructions willy-nilly is unsafe and unreliable (except
on x86 due to kludges, but then it's slow).


The problem of immediate values can only be handled by preprocessing stage or 
using SMC, otherwise I can only write a lot of similar functions and use 
`switch case` to call them. This method will cause the program size to expand a 
lot.

So, I think I have following choice:

Choice 1:

Use AT style inline function, and create a file named: CsrOperationGcc.c, and 
other future compiler feature-dependent files will be named: CsrOperationClang.c, 
CsrOperationXlang.c and so on.

If you're going to use inline assembly, just expose them directly? I
don't see the problem there, I don't expect loongarch to be picked up
by visual studio any time soon.



Choice 2:

Use SMC.


Choice 3:

Write a lot of similar CSR functions.

You /could/ use a GAS macro.

.macro csr_write csr
.global CsrWrite\csr
CsrWrite\csr:
 csrw a0, \csr
 ret

(this is riscv pseudo-asm but I know your arch is similar enough)




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




[edk2-devel] [PATCH v2 0/7] Uncrustify GoogleTest update

2023-08-16 Thread VivianNK
v1 -> v2:
 - Update commit message to explain the audit only mode change is
   temporary to prevent intermediate CI failures. 
 - Format patch Cc's correctly

v1 archive:https://edk2.groups.io/g/devel/message/107665

VivianNK (7):
  .pytool: Set uncrustify check to audit only (temporary)
  .pytool: Add cpp support to uncrustify plugin
  MdeModulePkg: Apply uncrustify formatting to relevant files.
  MdePkg: Apply uncrustify formatting to relevant files
  SecurityPkg: Apply uncrustify formatting to relevant files
  UnitTestFrameworkPkg: Apply uncrustify formatting to relevant files
  .pytool: Undo uncrustify check change

 .pytool/Plugin/UncrustifyCheck/UncrustifyCheck.py  
 |   2 +-
 .pytool/Plugin/UncrustifyCheck/uncrustify.cfg  
 |   4 +-
 MdeModulePkg/Library/UefiSortLib/GoogleTest/UefiSortLibGoogleTest.cpp  
 |  37 +-
 MdeModulePkg/Test/Mock/Include/GoogleTest/Library/MockPciHostBridgeLib.h   
 |   4 +-
 
MdeModulePkg/Test/Mock/Library/GoogleTest/MockPciHostBridgeLib/MockPciHostBridgeLib.cpp
 |   8 +-
 
MdePkg/Test/GoogleTest/Library/BaseSafeIntLib/SafeIntLibUintnIntnUnitTests32.cpp
| 114 ++--
 
MdePkg/Test/GoogleTest/Library/BaseSafeIntLib/SafeIntLibUintnIntnUnitTests64.cpp
| 114 ++--
 MdePkg/Test/GoogleTest/Library/BaseSafeIntLib/TestBaseSafeIntLib.cpp   
 | 563 ++--
 MdePkg/Test/Mock/Include/GoogleTest/Library/MockHobLib.h   
 |   6 +-
 MdePkg/Test/Mock/Include/GoogleTest/Library/MockPeiServicesLib.h   
 |   6 +-
 MdePkg/Test/Mock/Include/GoogleTest/Library/MockUefiLib.h  
 |   4 +-
 MdePkg/Test/Mock/Include/GoogleTest/Library/MockUefiRuntimeServicesTableLib.h  
 |   4 +-
 MdePkg/Test/Mock/Library/GoogleTest/MockHobLib/MockHobLib.cpp  
 |  40 +-
 MdePkg/Test/Mock/Library/GoogleTest/MockPeiServicesLib/MockPeiServicesLib.cpp  
 |  52 +-
 MdePkg/Test/Mock/Library/GoogleTest/MockUefiLib/MockUefiLib.cpp
 |   6 +-
 
MdePkg/Test/Mock/Library/GoogleTest/MockUefiRuntimeServicesTableLib/MockUefiRuntimeServicesTableLib.cpp
 |  12 +-
 
SecurityPkg/Library/SecureBootVariableLib/GoogleTest/SecureBootVariableLibGoogleTest.cpp
| 205 ---
 SecurityPkg/Test/Mock/Include/GoogleTest/Library/MockPlatformPKProtectionLib.h 
 |   4 +-
 
SecurityPkg/Test/Mock/Library/GoogleTest/MockPlatformPKProtectionLib/MockPlatformPKProtectionLib.cpp
|   4 +-
 UnitTestFrameworkPkg/Include/Library/GoogleTestLib.h   
 |   2 +-
 
UnitTestFrameworkPkg/Test/GoogleTest/Sample/SampleGoogleTest/SampleGoogleTest.cpp
   |  76 +--
 21 files changed, 664 insertions(+), 603 deletions(-)

-- 
2.41.0.windows.3



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




Re: [edk2-devel] [PATCH 2/2] OvmfPkg/AmdSev: Disable PcdFirstTimeWakeUpAPsBySipti

2023-08-16 Thread Ard Biesheuvel
On Wed, 16 Aug 2023 at 22:13, Michael Roth  wrote:
>
> PcdFirstTimeWakeUpAPsBySipi was recently introduced to indicate when
> the full INIT-SIPI-SIPI sequence can be skipped for AP bringup. It is
> true by default, but needs to be disabled for QEMU/OVMF where early INIT
> is not simulated. Commit 1d76560146 ("OvmfPkg: Disable
> PcdFirstTimeWakeUpAPsBySipi.") added changes to disable it
> by default for OvmfPkg, but a similar change was not made for the
> AmdSev package. This breaks booting of SEV and SNP guests.
>
> Fix this defaulting PcdFirstTimeWakeUpAPsBySipi to false for AmdSev
> package, as was previously done for OvmfPkg variants.
>
> Fixes: eaffa1d7ff ("UefiCpuPkg:Wake up APs after power-up or RESET through 
> SIPI.")
> Cc: YuanhaoXie 
> Cc: Tom Lendacky 
> Cc: Eric Dong 
> Cc: Ray Ni 
> Cc: Rahul Kumar 
> Cc: Gerd Hoffmann 
> Cc: Ard Biesheuvel 
> Cc: Jiewen Yao 
> Cc: Jordan Justen 
> Cc: Erdem Aktas 
> Cc: James Bottomley 
> Cc: Min Xu 
> Signed-off-by: Michael Roth 

Reviewed-by: Ard Biesheuvel 

Apologies for the oversight. This should be included in the upcoming stable tag.


> ---
>  OvmfPkg/AmdSev/AmdSevX64.dsc | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index f43300a95e..cf058f6a05 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -469,6 +469,14 @@
>gEfiMdeModulePkgTokenSpaceGuid.PcdConInConnectOnDemand|TRUE
>gUefiOvmfPkgTokenSpaceGuid.PcdBootRestrictToFirmware|TRUE
>
> +  #
> +  # INIT is now triggered before BIOS by ucode/hardware. In the OVMF
> +  # environment, QEMU lacks a simulation for the INIT process.
> +  # To address this, PcdFirstTimeWakeUpAPsBySipi set to FALSE to
> +  # broadcast INIT-SIPI-SIPI for the first time.
> +  #
> +  gUefiCpuPkgTokenSpaceGuid.PcdFirstTimeWakeUpAPsBySipi|FALSE
> +
>  
> 
>  #
>  # Pcd Dynamic Section - list of all EDK II PCD Entries defined by this 
> Platform
> --
> 2.25.1
>


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




[edk2-devel] [PATCH 2/2] OvmfPkg/AmdSev: Disable PcdFirstTimeWakeUpAPsBySipti

2023-08-16 Thread Roth, Michael via groups.io
PcdFirstTimeWakeUpAPsBySipi was recently introduced to indicate when
the full INIT-SIPI-SIPI sequence can be skipped for AP bringup. It is
true by default, but needs to be disabled for QEMU/OVMF where early INIT
is not simulated. Commit 1d76560146 ("OvmfPkg: Disable
PcdFirstTimeWakeUpAPsBySipi.") added changes to disable it
by default for OvmfPkg, but a similar change was not made for the
AmdSev package. This breaks booting of SEV and SNP guests.

Fix this defaulting PcdFirstTimeWakeUpAPsBySipi to false for AmdSev
package, as was previously done for OvmfPkg variants.

Fixes: eaffa1d7ff ("UefiCpuPkg:Wake up APs after power-up or RESET through 
SIPI.")
Cc: YuanhaoXie 
Cc: Tom Lendacky 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Jordan Justen 
Cc: Erdem Aktas 
Cc: James Bottomley 
Cc: Min Xu 
Signed-off-by: Michael Roth 
---
 OvmfPkg/AmdSev/AmdSevX64.dsc | 8 
 1 file changed, 8 insertions(+)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index f43300a95e..cf058f6a05 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -469,6 +469,14 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdConInConnectOnDemand|TRUE

   gUefiOvmfPkgTokenSpaceGuid.PcdBootRestrictToFirmware|TRUE

 

+  #

+  # INIT is now triggered before BIOS by ucode/hardware. In the OVMF

+  # environment, QEMU lacks a simulation for the INIT process.

+  # To address this, PcdFirstTimeWakeUpAPsBySipi set to FALSE to

+  # broadcast INIT-SIPI-SIPI for the first time.

+  #

+  gUefiCpuPkgTokenSpaceGuid.PcdFirstTimeWakeUpAPsBySipi|FALSE

+

 


 #

 # Pcd Dynamic Section - list of all EDK II PCD Entries defined by this Platform

-- 
2.25.1



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




[edk2-devel] [PATCH 1/2] OvmfPkg/AmdSev: fix BdsPlatform.c assertion failure during boot

2023-08-16 Thread Roth, Michael via groups.io
Booting an SEV guest with AmdSev OVMF package currently triggers the
following assertion with QEMU:

  InstallQemuFwCfgTables: installed 7 tables
  PcRtc: Write 0x20 to CMOS location 0x32
  [Variable]END_OF_DXE is signaled
  Initialize variable error flag (FF)

  ASSERT_EFI_ERROR (Status = Not Found)
  ASSERT [BdsDxe] 
/home/VT_BUILD/ovmf/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c(1711): 
!(((INTN)(RETURN_STATUS)(Status)) < 0)

This seems to be due to commit 81dc0d8b4c, which switched to using
PlatformBootManagerLib instead of PlatformBootManagerLibGrub. That
pulls in a dependency on gEfiS3SaveStateProtocolGuid provider being
available (which is asserted for in
BdsPlatform.c:PlatformBootManagerBeforeConsole()/SaveS3BootScript()),
but the libraries that provide it aren't currently included in the
build. Add them similarly to what's done for OvmfPkg.

Fixes: 81dc0d8b4c ("OvmfPkg/AmdSev: stop using PlatformBootManagerLibGrub")
Cc: Gerd Hoffmann 
Cc: Ray Ni 
Cc: Erdem Aktas 
Cc: James Bottomley 
Cc: Jiewen Yao 
Cc: Min Xu 
Cc: Tom Lendacky 
Signed-off-by: Michael Roth 
---
 OvmfPkg/AmdSev/AmdSevX64.dsc | 3 +++
 OvmfPkg/AmdSev/AmdSevX64.fdf | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 2c6ed7c974..f43300a95e 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -200,6 +200,7 @@
 

   SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf

   
OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf

+  
S3BootScriptLib|MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf

 

 !include OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc

 

@@ -709,6 +710,8 @@
   #

   MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf

   OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf

+  MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf

+  MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf

   
MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf

 

   #

diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
index 463bd3e9ef..b2ab0c7773 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -270,6 +270,8 @@ INF  OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
 

 INF  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf

 INF  OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf

+INF  MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf

+INF  
MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf

 INF  
MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf

 

 INF  FatPkg/EnhancedFatDxe/Fat.inf

-- 
2.25.1



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




[edk2-devel] [0/2] Fixes for AmdSev OVMF package regressions

2023-08-16 Thread Roth, Michael via groups.io
A couple changes broke booting of SEV and SNP guests when using the
AmdSevX64 package in conjunction with QEMU:

  UefiCpuPkg:Wake up APs after power-up or RESET through SIPI.
  OvmfPkg/AmdSev: stop using PlatformBootManagerLibGrub

Both fixes are fairly straightforward and mainly seem to stem from
AmdSevX64.dsc and friends getting overlooked when updating various
OvmfPkg variants with new changes. This series updates the AmdSev
build accordingly. I've grouped them into a series since both
patches are needed to fix booting of SEV/SNP guests.


Michael Roth (2):
  OvmfPkg/AmdSev: fix BdsPlatform.c assertion failure during boot
  OvmfPkg/AmdSev: Disable PcdFirstTimeWakeUpAPsBySipti

 OvmfPkg/AmdSev/AmdSevX64.dsc | 11 +++
 OvmfPkg/AmdSev/AmdSevX64.fdf |  2 ++
 2 files changed, 13 insertions(+)




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




Re: [edk2-devel] [PATCH v4 00/14] Add ImagePropertiesRecordLib and Fix MAT Bugs

2023-08-16 Thread Taylor Beebe

Can I please get reviews/feedback on this patch series?

On 8/4/2023 12:46 PM, Taylor Beebe via groups.io wrote:

From: Taylor Beebe 

v4:
- Expose additional functions in the Library API
- Add NULL checks to library functions and return a
   status where applicable.

v3:
- Refactor patch series so the transition of logic from the DXE
   MAT logic to the new library is more clear.
- Update function headers to improve clarity and follow EDK2
   standards.
- Add Create and Delete functions for Image Properties Records
   and redirect some of the SMM and DXE MAT code to use these
   functions.
- Update/Add DumpImageRecords() to print the image name and code
   sections of each runtime image which will be put in the MAT.
   The DXE and SMM MAT logic will now invoke the DumpImageRecords()
   on DEBUG builds at the EndOfDxe event to install the MAT.

v2:
- A one-line change in patch 3 was moved to patch 9 for correctness.

Reference: https://github.com/tianocore/edk2/pull/4590
Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4492

The UEFI and SMM MAT logic contains duplicate logic for manipulating image
properties records which is used to track runtime images.
This patch series adds a new library, ImagePropertiesRecordLib,
which consolidates this logic and fixes the bugs which currently exist in
the MAT logic.

The first patch adds the ImagePropertiesRecordLib implementation which
is a copy of the UEFI MAT logic with minor modifications to remove the
reliance on globabl variables and make the code unit testable.

The second patch adds a unit test for the ImagePropertiesRecordLib. The
logic tests various potential layouts of the EFI memory map and runtime
images. 3/4 of these tests will fail which demonstrates the MAT logic
bugs.

The third patch fixes the logic in the ImagePropertiesRecordLib so
that all of the unit tests pass and the MAT logic can be fixed by
using the library.

The remaining patches add library instances to DSC files and remove
the image properties record logic from the SMM and UEFI MAT logic.

Cc: Andrew Fish 
Cc: Ard Biesheuvel 
Cc: Dandan Bi 
Cc: Eric Dong 
Cc: Gerd Hoffmann 
Cc: Guo Dong 
Cc: Gua Guo 
Cc: James Lu 
Cc: Jian J Wang 
Cc: Jiewen Yao 
Cc: Jordan Justen 
Cc: Leif Lindholm 
Cc: Liming Gao 
Cc: Rahul Kumar 
Cc: Ray Ni 
Cc: Sami Mujawar 
Cc: Sean Rhodes 

Taylor Beebe (14):
   MdeModulePkg: Add ImagePropertiesRecordLib
   ArmVirtPkg: Add ImagePropertiesRecordLib Instance
   EmulatorPkg: Add ImagePropertiesRecordLib Instance
   OvmfPkg: Add ImagePropertiesRecordLib Instance
   UefiPayloadPkg: Add ImagePropertiesRecordLib Instance
   MdeModulePkg: Update MemoryAttributesTable.c to Reduce Global Variable
 Use
   MdeModulePkg: Move Some DXE MAT Logic to ImagePropertiesRecordLib
   MdeModulePkg: Add ImagePropertiesRecordLib Host-Based Unit Test
   MdeModulePkg: Fix Bugs in MAT Logic
   MdeModulePkg: Add NULL checks and Return Status to
 ImagePropertiesRecordLib
   UefiCpuPkg: Use Attribute From SMM MemoryAttributesTable if Nonzero
   MdeModulePkg: Transition SMM MAT Logic to Use ImagePropertiesRecordLib
   MdeModulePkg: Add Logic to Create/Delete Image Properties Records
   MdeModulePkg: Update DumpImageRecord() in ImagePropertiesRecordLib

  MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
  |  967 +
  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c 
  |   24 +-
  MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c   
  |  958 +---
  MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c  
  | 1144 
  
MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.c
   |  938 
  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
  |   19 +-
  ArmVirtPkg/ArmVirt.dsc.inc
  |1 +
  EmulatorPkg/EmulatorPkg.dsc   
  |1 +
  MdeModulePkg/Core/Dxe/DxeMain.h   
  |   20 -
  MdeModulePkg/Core/Dxe/DxeMain.inf 
  |1 +
  MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf 
  |1 +
  MdeModulePkg/Include/Library/ImagePropertiesRecordLib.h   
  |  234 
  MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf
  |   31 +
  
MdeModulePkg/Library/ImagePropertiesRecordLib/UnitTest/ImagePropertiesRecordLibUnitTestHost.inf
 |   35 +
  MdeModulePkg/MdeModulePkg.dec 
  |5 +
  MdeModulePkg/MdeModulePkg.dsc  

Re: [edk2-devel] How to report bugs in the Intel GigUndiDxe driver?

2023-08-16 Thread Ard Biesheuvel
On Wed, 16 Aug 2023 at 05:40, Rebecca Cran  wrote:
>
> Intel has been releasing the EDK2/UEFI source code for their network
> drivers
> (https://www.intel.com/content/www/us/en/download/15755/intel-ethernet-connections-boot-utility-preboot-images-and-efi-drivers.html)
> but I've found a few problems with it. The first issue I ran into is
> that GigUndiDxe.inf includes StdLibC which obviously breaks the build.
>
> This evening I also found a problem with a debug print which causes a crash:
>
> DEBUGPRINT (HII, ("Package list languages - %a\n"));
>
>
> Less important, but slightly irritating, the documentation files such as
> build_instruction.txt are in UTF-16 format.
>
>
> I was wondering if there's any way to send feedback to the team
> responsible for the driver(s) without going through the official Intel
> Support process?
>
>
> Once I have it working, if there's interest I was thinking about having
> the code added to edk2-non-osi under Drivers/Intel/IntelUndiPkg. I don't
> know if that's something that would be useful?
>

Best to refer to

https://github.com/tianocore/edk2-staging/tree/Intel_UNDI

I contributed AArch64 + GCC build support in the past, and all my
patches got squashed together and combined with other changes into

https://github.com/tianocore/edk2-staging/commit/c5c2c87494e686c6418e72f68d05e02e90a7d53c

but at least they took my changes an merged them.


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




Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG?

2023-08-16 Thread Michael D Kinney
Hi Andrew,

There are compiler flags we set to suppress some of these types of specific 
warnings to avoid disable warnings as errors.

For example, this commit to tools_def.template:

https://github.com/tianocore/edk2/commit/8e985ac3fdb2b117968ac1fa1f54666e166af8ac

I would like to see the compiler, version, arch, log info with the specific 
warning being flagged to see if we can apply the techniques we have been able 
to apply in the past.

Mike

From: Andrew (EFI) Fish 
Sent: Wednesday, August 16, 2023 10:41 AM
To: edk2-devel-groups-io ; Kinney, Michael D 

Cc: wang...@iscas.ac.cn
Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG?

Mike,

I seem to remember it was a maybe-uninitialized compiler error. The DEBUG build 
did not hit it due to the NULL check in the ASSERT. The NULL check only exists 
if ASSERT() is enabled. So if ASSERT is disabled this code in the DXE Core will 
dereference a NULL pointer.

  ASSERT (Prot != NULL);
  //
  // EFI_ALREADY_STARTED is not an error for bus driver.
  // Return the corresponding protocol interface.
  //
  *Interface = Prot->Interface;

Thanks,

Andrew Fish


On Aug 16, 2023, at 10:02 AM, Michael D Kinney 
mailto:michael.d.kin...@intel.com>> wrote:

Can you provide the specific build error?

So far, we have not had to relax that flag for any RELEASE builds.

Thanks,

Mike

From: devel@edk2.groups.io 
mailto:devel@edk2.groups.io>> On Behalf Of ??
Sent: Wednesday, August 16, 2023 12:46 AM
To: Andrew (EFI) Fish mailto:af...@apple.com>>
Cc: edk2-devel-groups-io mailto:devel@edk2.groups.io>>
Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG?


Yes, the RELEASE build compiler flags should be relaxed, all error checks that 
the DEBUG target should do, it doesn't belong to the RELEASE's job.



---
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
index 28d9af4..ac3b5ec 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
+++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
@@ -54,7 +54,7 @@
!include MdePkg/MdeLibs.dsc.inc

[BuildOptions]
-  GCC:RELEASE_*_*_CC_FLAGS   = -DMDEPKG_NDEBUG
+  GCC:RELEASE_*_*_CC_FLAGS   = -flto
!ifdef $(SOURCE_DEBUG_ENABLE)
  GCC:*_*_RISCV64_GENFW_FLAGS= --keepexceptiontable
!endif
--
2.39.1



I have added the LTO flag and dropped MDEPKG_NDEBUG in the .dsc file, it 
compiled successfully, the build 
log:https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64

Do you have other way to add LTO flag to compile that don't change the source 
code, i tried to use environment variable, like exported CFLAGS=-flto to 
compile, but the compiler doesn't work with it.







-原始郵件-
發件人:"Andrew (EFI) Fish" mailto:af...@apple.com>>
發送時間:2023-08-10 21:50:55 (星期四)
收件人: edk2-devel-groups-io mailto:devel@edk2.groups.io>>, 
wang...@iscas.ac.cn
抄送:
主題: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG?
I think that you are advocating that since we have code that error checks on 
DEBUG and not RELEASE builds we should relax the RELEASE build compiler flags?


/home/abuild/rpmbuild/BUILD/edk2-edk2-stable202305/MdeModulePkg/Core/Dxe/Hand/Handle.c:1183:24:
 error: ‘Prot’ may be used uninitialized [-Werror=maybe-uninitialized]
  ASSERT (Prot != NULL);
  //
  // EFI_ALREADY_STARTED is not an error for bus driver.
  // Return the corresponding protocol interface.
  //
  *Interface = Prot->Interface;

A given platform can add ASSERT into release builds if it wants. I’ve actually 
done that for power on before. If your compiler supports LTO you are not 
required to set MDEPKG_NDEBUG on RELEASE builds, and you can used a PCD to 
configure your debug level, per build type.

Maybe we should just have the error checks in all paths?

Thanks,

Andrew Fish



On Aug 10, 2023, at 6:44 AM, 汪流 
mailto:wang...@iscas.ac.cn>> wrote:

I want to build a rpm package for edk2-stable202305 on riscv64, however I get 
some uninitialized variable error, I have found that the reason is -Werror flag.
My build target was release. I think the flag should used in the debug ,not in 
release.
My build command:  build -t GCC5 -n $NCPUS -b RELEASE -a RISCV64 -p 
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc -D SECURE_BOOT_ENABLE=TRUE -D 
TPM_ENABLE=TRUE -D TPM_CONFIG_ENABLE=TRUE

This is my packet log:

https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/edk2-test/openEuler_Mainline_standard_riscv64_gcc/riscv64
https://build.tarsier-infra.com/build/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/openEuler_Mainline_standard_riscv64_gcc/riscv64/edk2-test/_log





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG?

2023-08-16 Thread Andrew Fish via groups.io
Mike,

I seem to remember it was a maybe-uninitialized compiler error. The DEBUG build 
did not hit it due to the NULL check in the ASSERT. The NULL check only exists 
if ASSERT() is enabled. So if ASSERT is disabled this code in the DXE Core will 
dereference a NULL pointer. 

>   ASSERT (Prot != NULL);
>   // 
>   // EFI_ALREADY_STARTED is not an error for bus driver.
>   // Return the corresponding protocol interface.
>   // 
>   *Interface = Prot->Interface;


Thanks,

Andrew Fish

> On Aug 16, 2023, at 10:02 AM, Michael D Kinney  
> wrote:
> 
> Can you provide the specific build error?
>  
> So far, we have not had to relax that flag for any RELEASE builds.
>  
> Thanks,
>  
> Mike
>  
> From: devel@edk2.groups.io  
> mailto:devel@edk2.groups.io>> On Behalf Of ??
> Sent: Wednesday, August 16, 2023 12:46 AM
> To: Andrew (EFI) Fish mailto:af...@apple.com>>
> Cc: edk2-devel-groups-io mailto:devel@edk2.groups.io>>
> Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG?
>  
> Yes, the RELEASE build compiler flags should be relaxed, all error checks 
> that the DEBUG target should do, it doesn't belong to the RELEASE's job.
> 
>  
> 
> --- 
> OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 2 +- 
> 1 file changed, 1 insertion(+), 1 deletion(-) 
> 
> diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
> b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
> index 28d9af4..ac3b5ec 100644 
> --- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
> +++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
> @@ -54,7 +54,7 @@ 
> !include MdePkg/MdeLibs.dsc.inc 
> 
> [BuildOptions] 
> -  GCC:RELEASE_*_*_CC_FLAGS   = -DMDEPKG_NDEBUG 
> +  GCC:RELEASE_*_*_CC_FLAGS   = -flto 
> !ifdef $(SOURCE_DEBUG_ENABLE) 
>   GCC:*_*_RISCV64_GENFW_FLAGS= --keepexceptiontable 
> !endif 
> -- 
> 2.39.1
> 
> 
> I have added the LTO flag and dropped MDEPKG_NDEBUG in the .dsc file, it 
> compiled successfully, the build 
> log:https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64
> 
> Do you have other way to add LTO flag to compile that don't change the source 
> code, i tried to use environment variable, like exported CFLAGS=-flto to 
> compile, but the compiler doesn't work with it.
> 
>  
>  
> 
>  
> 
>  
> 
> -原始郵件-
> 發件人:"Andrew (EFI) Fish" mailto:af...@apple.com>>
> 發送時間:2023-08-10 21:50:55 (星期四)
> 收件人: edk2-devel-groups-io  >, wang...@iscas.ac.cn 
> 
> 抄送: 
> 主題: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG?
> 
> I think that you are advocating that since we have code that error checks on 
> DEBUG and not RELEASE builds we should relax the RELEASE build compiler flags?
>  
> /home/abuild/rpmbuild/BUILD/edk2-edk2-stable202305/MdeModulePkg/Core/Dxe/Hand/Handle.c:1183:24:
>  error: ‘Prot’ may be used uninitialized [-Werror=maybe-uninitialized]
>   ASSERT (Prot != NULL);
>   // 
>   // EFI_ALREADY_STARTED is not an error for bus driver.
>   // Return the corresponding protocol interface.
>   // 
>   *Interface = Prot->Interface;
>  
> A given platform can add ASSERT into release builds if it wants. I’ve 
> actually done that for power on before. If your compiler supports LTO you are 
> not required to set MDEPKG_NDEBUG on RELEASE builds, and you can used a PCD 
> to configure your debug level, per build type.  
>  
> Maybe we should just have the error checks in all paths?
>  
> Thanks, 
>  
> Andrew Fish 
> 
> 
> On Aug 10, 2023, at 6:44 AM, 汪流  > wrote:
>  
> I want to build a rpm package for edk2-stable202305 on riscv64, however I get 
> some uninitialized variable error, I have found that the reason is -Werror 
> flag.
> My build target was release. I think the flag should used in the debug ,not 
> in release.
> My build command:  build -t GCC5 -n $NCPUS -b RELEASE -a RISCV64 -p 
> OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc -D SECURE_BOOT_ENABLE=TRUE -D 
> TPM_ENABLE=TRUE -D TPM_CONFIG_ENABLE=TRUE
> This is my packet log:
> 
> https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/edk2-test/openEuler_Mainline_standard_riscv64_gcc/riscv64
> 
> https://build.tarsier-infra.com/build/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/openEuler_Mainline_standard_riscv64_gcc/riscv64/edk2-test/_log
>  
> 



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




Re: [edk2-devel] [PATCH] SecurityPkg: DxeTcg2PhysicalPresenceLib for Clear Graphics Screen To unblock and Display TPM messages

2023-08-16 Thread Poosapalli, Karunakar via groups.io
Hi @Yao, Jiewen,

Could you please share your thoughts. Please let me know if you have any 
queries or concerns.

Thanks & Regards
Karunakar Poosapalli
Firmware Principal Engineer, Client BIOS
Customer BIOS | Dell Core BIOS
CPG Software Engineering | Dell Technologies
Mobile +91 9951902957
karunakar_poosapa...@dell.com



Internal Use - Confidential
From: Poosapalli, Karunakar
Sent: Wednesday, August 9, 2023 9:59 AM
To: Yao, Jiewen; Gao, Liming; devel@edk2.groups.io
Subject: RE: [edk2-devel] [PATCH] SecurityPkg: DxeTcg2PhysicalPresenceLib for 
Clear Graphics Screen To unblock and Display TPM messages

Hi @Yao, Jiewen,

The proposed solution to clear screen will be called only when user has to 
provide the input key. This call will not execute in other conditions.
When there is a pending request in Tcg2, system will wait until user press 
input key. If there is no proper UI to customer, they feel it's system hang and 
go for customer support.
Even BIOS vendors also won't come to what is happening at customer box without 
any user information.
This is generic issue and not specific to any platform.

Thanks for sharing your thoughts.

Thanks & Regards
Karunakar Poosapalli
Firmware Principal Engineer, Client BIOS
Customer BIOS | Dell Core BIOS
CPG Software Engineering | Dell Technologies
Mobile +91 9951902957
karunakar_poosapa...@dell.com



Internal Use - Confidential
From: Yao, Jiewen mailto:jiewen@intel.com>>
Sent: Wednesday, August 9, 2023 4:44 AM
To: Poosapalli, Karunakar; Gao, Liming; 
devel@edk2.groups.io
Cc: Yao, Jiewen
Subject: RE: [edk2-devel] [PATCH] SecurityPkg: DxeTcg2PhysicalPresenceLib for 
Clear Graphics Screen To unblock and Display TPM messages


[EXTERNAL EMAIL]
My concern is that you unconditionally clear the screen. What if someone did 
want to show something on the screen?

This seems an incompatible change. That is why I think it should be a platform 
policy.

Another way is that you may consider to duplicate the library for your platform 
and clear it for your platform.

Thank you
Yao, Jiewen


From: Poosapalli, Karunakar 
mailto:karunakarpoosapa...@dell.com>>
Sent: Wednesday, August 9, 2023 2:13 AM
To: Yao, Jiewen mailto:jiewen@intel.com>>; Gao, 
Liming mailto:gaolim...@byosoft.com.cn>>; 
devel@edk2.groups.io; Poosapalli, Karunakar 
mailto:karunakarpoosapa...@dell.com>>
Subject: RE: [edk2-devel] [PATCH] SecurityPkg: DxeTcg2PhysicalPresenceLib for 
Clear Graphics Screen To unblock and Display TPM messages

Hi @Yao, Jiewen,

Thanks for your review and feedback.
When there is a pending Tcg request, the control will be in Tcg2 library and it 
will not reach to BDS until user press the input key.
As there is no information the screen because of screen is blocked by some 
other messages/Logo.
So user won't able to press the key without any user information and control 
will not reach to BDS phase.

As TCG user confirmation is the highest priority and it blocks the POST until 
the user presses the input key. Before TCG messages Print on the console, there 
should be logic added to clear the graphics screen

Please let me know your thoughts.

Thanks & Regards
Karunakar Poosapalli
Firmware Principal Engineer, Client BIOS
Customer BIOS | Dell Core BIOS
CPG Software Engineering | Dell Technologies
Mobile +91 9951902957
karunakar_poosapa...@dell.com



Internal Use - Confidential
From: Yao, Jiewen mailto:jiewen@intel.com>>
Sent: Tuesday, July 25, 2023 5:48 PM
To: Poosapalli, Karunakar; Gao, Liming; 
devel@edk2.groups.io
Subject: RE: [edk2-devel] [PATCH] SecurityPkg: DxeTcg2PhysicalPresenceLib for 
Clear Graphics Screen To unblock and Display TPM messages


[EXTERNAL EMAIL]
Hello
I agree with you on the problem statement.

But I don't think this is a desired solution.
We expect Platform BDS to call the PhysicalPresenceLib. As such, why not clear 
the  screen in the platform BDS?

Thank you
Yao, Jiewen

From: Poosapalli, Karunakar 
mailto:karunakarpoosapa...@dell.com>>
Sent: Monday, July 24, 2023 11:26 PM
To: Yao, Jiewen mailto:jiewen@intel.com>>; Gao, 
Liming mailto:gaolim...@byosoft.com.cn>>; 
devel@edk2.groups.io
Subject: RE: [edk2-devel] [PATCH] SecurityPkg: DxeTcg2PhysicalPresenceLib for 
Clear Graphics Screen To unblock and Display TPM messages

Can you please review and share your feedback?


Thanks & Regards
Karunakar Poosapalli
Firmware Principal Engineer, Client BIOS
Customer BIOS | Dell Core BIOS
CPG Software Engineering | Dell Technologies
Mobile +91 9951902957
karunakar_poosapa...@dell.com



Internal Use - Confidential
From: Poosapalli, Karunakar
Sent: Saturday, July 22, 2023 1:21 AM
To: 

Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG?

2023-08-16 Thread Michael D Kinney
Can you provide the specific build error?

So far, we have not had to relax that flag for any RELEASE builds.

Thanks,

Mike

From: devel@edk2.groups.io  On Behalf Of ??
Sent: Wednesday, August 16, 2023 12:46 AM
To: Andrew (EFI) Fish 
Cc: edk2-devel-groups-io 
Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG?


Yes, the RELEASE build compiler flags should be relaxed, all error checks that 
the DEBUG target should do, it doesn't belong to the RELEASE's job.



---
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
index 28d9af4..ac3b5ec 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
+++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
@@ -54,7 +54,7 @@
!include MdePkg/MdeLibs.dsc.inc

[BuildOptions]
-  GCC:RELEASE_*_*_CC_FLAGS   = -DMDEPKG_NDEBUG
+  GCC:RELEASE_*_*_CC_FLAGS   = -flto
!ifdef $(SOURCE_DEBUG_ENABLE)
  GCC:*_*_RISCV64_GENFW_FLAGS= --keepexceptiontable
!endif
--
2.39.1


I have added the LTO flag and dropped MDEPKG_NDEBUG in the .dsc file, it 
compiled successfully, the build log: 
https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64

Do you have other way to add LTO flag to compile that don't change the source 
code, i tried to use environment variable, like exported CFLAGS=-flto to 
compile, but the compiler doesn't work with it.







-原始郵件-
發件人:"Andrew (EFI) Fish" mailto:af...@apple.com>>
發送時間:2023-08-10 21:50:55 (星期四)
收件人: edk2-devel-groups-io mailto:devel@edk2.groups.io>>, 
wang...@iscas.ac.cn
抄送:
主題: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG?
I think that you are advocating that since we have code that error checks on 
DEBUG and not RELEASE builds we should relax the RELEASE build compiler flags?


/home/abuild/rpmbuild/BUILD/edk2-edk2-stable202305/MdeModulePkg/Core/Dxe/Hand/Handle.c:1183:24:
 error: ‘Prot’ may be used uninitialized [-Werror=maybe-uninitialized]
  ASSERT (Prot != NULL);
  //
  // EFI_ALREADY_STARTED is not an error for bus driver.
  // Return the corresponding protocol interface.
  //
  *Interface = Prot->Interface;

A given platform can add ASSERT into release builds if it wants. I’ve actually 
done that for power on before. If your compiler supports LTO you are not 
required to set MDEPKG_NDEBUG on RELEASE builds, and you can used a PCD to 
configure your debug level, per build type.

Maybe we should just have the error checks in all paths?

Thanks,

Andrew Fish


On Aug 10, 2023, at 6:44 AM, 汪流 
mailto:wang...@iscas.ac.cn>> wrote:

I want to build a rpm package for edk2-stable202305 on riscv64, however I get 
some uninitialized variable error, I have found that the reason is -Werror flag.
My build target was release. I think the flag should used in the debug ,not in 
release.
My build command:  build -t GCC5 -n $NCPUS -b RELEASE -a RISCV64 -p 
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc -D SECURE_BOOT_ENABLE=TRUE -D 
TPM_ENABLE=TRUE -D TPM_CONFIG_ENABLE=TRUE

This is my packet log:

https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/edk2-test/openEuler_Mainline_standard_riscv64_gcc/riscv64
https://build.tarsier-infra.com/build/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/openEuler_Mainline_standard_riscv64_gcc/riscv64/edk2-test/_log




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




Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild Descriptor Table

2023-08-16 Thread Chesley, Brit via groups.io
[AMD Official Use Only - General]

Hello Hao,

Its no problem. I agree that the endpoint transfer rings should be allocated 
after the UsbSetConfig command, but this is not the case. In 
XhcControlTransfer, after the if statement checking for USB_REQ_SET_CONFIG, the 
for-loop loops through all of DevDesc.NumConfigurations and calls 
XhcSetConfigCmd. The issue here is that after a reset is issued 
XhcInitializeDeviceSlot64 is called which frees Xhc->UsbDevContext[SlotId]. 
This sets Xhc->UsbDevContext[SlotId].DevDesc.NumConfigurations to 0. So 
XhcSetConfigCmd is never called, and the other endpoint transfer rings besides 
the default are never reinitialized. From what I could tell the easiest way to 
reacquire the proper NumConfigurations value was to call UsbBuildDescTable for 
the device.

Best,

Brit Chesley

> -Original Message-
> From: Wu, Hao A 
> Sent: Monday, July 31, 2023 2:37 AM
> To: devel@edk2.groups.io; Chesley, Brit 
> Cc: Wang, Jian J ; Gao, Liming
> ; Ni, Ray ; Chang, Abner
> 
> Subject: RE: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild
> Descriptor Table
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> > -Original Message-
> > From: devel@edk2.groups.io  On Behalf Of
> > brit.chesley via groups.io
> > Sent: Saturday, July 8, 2023 1:07 AM
> > To: devel@edk2.groups.io
> > Cc: Wang, Jian J ; Gao, Liming
> > ; Wu, Hao A ; Ni, Ray
> > ; Abner Chang 
> > Subject: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild
> > Descriptor Table
> >
> > From: Britton Chesley 
> >
> > Fixed a bug which led to an ASSERT due to the USB device context being
> > maintained after a port reset, but the underlying XHCI context was
> > uninitialized. Specifically, Xhc->UsbDevContext is freed after a reset
> > and only re-allocates the default [0] enpoint transfer ring. Added
> > build
>
>
> Really sorry for another question.
>
> My take is that the transfer ring of other endpoints (besides the Default
> Control Endpoint) will be re-initialized by the below flow:
> UsbSetConfig -> UsbCtrlRequest (USB_REQ_SET_CONFIG) ->
> XhcSetConfigCmd(64) -> XhcInitializeEndpointContext(64)
>
> Could you help to elaborate a bit more on what is the issue with the above
> (current) flow and why rebuilding the Descriptor Table before UsbSetConfig
> can resolve it?
>
> Thanks in advance.
>
> Best Regards,
> Hao Wu
>
>
> > descriptor table call in UsbIoPortReset. BZ 4456
> >
> > Cc: Jian J Wang 
> > Cc: Liming Gao 
> > Cc: Hao A Wu 
> > Cc: Ray Ni 
> > Cc: Abner Chang 
> > Signed-off-by: Britton Chesley 
> > ---
> >  MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 26
> > -
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > index c25f3cc2f279..a5b798bd8d6c 100644
> > --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > @@ -821,6 +821,7 @@ UsbIoPortReset (
> >EFI_TPLOldTpl;
> >EFI_STATUS Status;
> >UINT8  DevAddress;
> > +  UINT8  Config;
> >
> >OldTpl = gBS->RaiseTPL (USB_BUS_TPL);
> >
> > @@ -882,8 +883,26 @@ UsbIoPortReset (
> >// is in CONFIGURED state.
> >//
> >if (Dev->ActiveConfig != NULL) {
> > -Status = UsbSetConfig (Dev, Dev->ActiveConfig-
> >Desc.ConfigurationValue);
> > +UsbFreeDevDesc (Dev->DevDesc);
> >
> > +Status = UsbRemoveConfig (Dev);
> > +if (EFI_ERROR (Status)) {
> > +  DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to remove
> > configuration - %r\n", Status));
> > +}
> > +
> > +Status = UsbGetMaxPacketSize0 (Dev);
> > +if (EFI_ERROR (Status)) {
> > +  DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to get max packet
> > + size -
> >  %r\n", Status));
> > +}
> > +
> > +Status = UsbBuildDescTable (Dev);
> > +if (EFI_ERROR (Status)) {
> > +  DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to build
> > + descriptor
> > table - %r\n", Status));
> > +}
> > +
> > +Config = Dev->DevDesc->Configs[0]->Desc.ConfigurationValue;
> > +
> > +Status = UsbSetConfig (Dev, Config);
> >  if (EFI_ERROR (Status)) {
> >DEBUG ((
> >  DEBUG_ERROR,
> > @@ -892,6 +911,11 @@ UsbIoPortReset (
> >  Status
> >  ));
> >  }
> > +
> > +Status = UsbSelectConfig (Dev, Config);
> > +if (EFI_ERROR (Status)) {
> > +  DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to set
> > + configuration -
> >  %r\n", Status));
> > +}
> >}
> >
> >  ON_EXIT:
> > --
> > 2.36.1
> >
> >
> >
> > 
> >



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

[edk2-devel] [PATCH edk2-platforms 0/1] Enlarge firmware offsets

2023-08-16 Thread Marcin Juszkiewicz
I am back from vacations. Started debug build and was greeted with
message that my firmware is too big:

GenFv: ERROR 3000: Invalid
  the required fv image size 0x216510 exceeds the set fv image size 0x20

Which is weird as QemuSbsa uses 256MB flash images.

So as quick hack I decided to bump some offsets and check does it work.
It did but I am not sure is it the proper way.

Marcin Juszkiewicz (1):
  Platform/QemuSbsa: enlarge firmware offsets

 Platform/Qemu/SbsaQemu/SbsaQemu.fdf | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
2.41.0



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




[edk2-devel] [PATCH edk2-platforms 1/1] Platform/QemuSbsa: enlarge firmware offsets

2023-08-16 Thread Marcin Juszkiewicz
GenFv: ERROR 3000: Invalid
  the required fv image size 0x216510 exceeds the set fv image size 0x20

Signed-off-by: Marcin Juszkiewicz 
---
 Platform/Qemu/SbsaQemu/SbsaQemu.fdf | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf 
b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
index 677cea6344dc..bc432cac6fc8 100644
--- a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
+++ b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
@@ -58,20 +58,20 @@ [FD.SBSA_FLASH0]
 
 [FD.SBSA_FLASH1]
 BaseAddress   = 0x1000|gArmTokenSpaceGuid.PcdFdBaseAddress
-Size  = 0x002C|gArmTokenSpaceGuid.PcdFdSize
+Size  = 0x003C|gArmTokenSpaceGuid.PcdFdSize
 ErasePolarity = 1
 BlockSize = 0x1000
-NumBlocks = 0x2C0
+NumBlocks = 0x3C0
 
 ## Place for EFI (BL33)
 # This offset (if any as it is 0x0 currently) + BaseAddress (0x1000) must 
be set in PRELOADED_BL33_BASE at ATF
-0x|0x0020
+0x|0x0030
 gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize
 FV = FVMAIN_COMPACT
 
 ## Place for Variables. They share flash1 with EFI
 # Must be aligned to Flash Block size 0x4
-0x0020|0x0004
+0x0030|0x0004
 
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
 #NV_VARIABLE_STORE
 DATA = {
@@ -109,7 +109,7 @@ [FD.SBSA_FLASH1]
   0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
 }
 
-0x0024|0x0004
+0x0034|0x0004
 
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
 #NV_FTW_WORKING
 DATA = {
@@ -123,7 +123,7 @@ [FD.SBSA_FLASH1]
   0xE0, 0xFF, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00
 }
 
-0x0028|0x0004
+0x0038|0x0004
 
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
 #NV_FTW_SPARE
 
-- 
2.41.0



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




回复: [edk2-devel] [PATCH] OvmfPkg/RiscVVirt: Fix issues in VarStore Blockmap config

2023-08-16 Thread Qingyu Shang
Hi Sunil, 

Happy you like that! I will CC all the related maintainers by that python 
script next time. 

Thanks! 
Qingyu 

-原始邮件- 
发件人: "Sunil V L"  
收件人: "Li, Yong"  
抄送: devel@edk2.groups.io, 2931013...@sjtu.edu.cn, "Warkentin, Andrei" 
 
已发送邮件: 星期三, 8 月 16, 2023 05:03:00 PM 
主题: Re: [edk2-devel] [PATCH] OvmfPkg/RiscVVirt: Fix issues in VarStore Blockmap 
config 

On Wed, Aug 16, 2023 at 07:08:58AM +, Li, Yong wrote: 
> Hi Sunil, 
> 
> Qingyu is from Penglai team, when we were working together in enabling 
> StandaloneMm variable service, 
> we found an issue in OvmfPkg/RiscVVirt/VarStore.fdf.inc, in which the 
> blockmap config is not aligned with the value in 
> OvmfPkg/RiscVVirt/Library/VirtNorFlashPlatformLib/VirtNorFlashStaticLib.c, 
> which is SIZE_256KB 
> 
> That mis-alignment won't cause any issue if the variable size is small and 
> less than 0x1000, but if store more data in the flash device then it will 
> cause data loss issue during reboot. 
> Once reboot if any FV header data is corrupted it will result the whole 
> variable system be re-initialized. 
> 
> Please help review the patch and give the comments, thanks 
> 
Hi Yong Li, 

Thank you very much for providing the context. Sorry, I actually had 
missed this. 

Hi Quingyu Shang, 

The patch LGTM. Thanks a lot for finding the issue and fixing it. In 
future, could you please run BaseTools/Scripts/GetMaintainer.py on the 
patches and CC all the people it lists using "Cc:" tag? 

Reviewed-by: Sunil V L  

Thanks! 
Sunil 


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




Re: [edk2-devel] [PATCH] OvmfPkg/RiscVVirt: Fix issues in VarStore Blockmap config

2023-08-16 Thread Sunil V L
On Wed, Aug 16, 2023 at 07:08:58AM +, Li, Yong wrote:
> Hi Sunil,
> 
> Qingyu is from Penglai team, when we were working together in enabling 
> StandaloneMm variable service, 
> we found an issue in OvmfPkg/RiscVVirt/VarStore.fdf.inc,  in which the 
> blockmap config is not aligned with the value in
> OvmfPkg/RiscVVirt/Library/VirtNorFlashPlatformLib/VirtNorFlashStaticLib.c, 
> which is SIZE_256KB
> 
> That mis-alignment won't cause any issue if the variable size is small and 
> less than 0x1000, but if store more data in the flash device then it will 
> cause data loss issue during reboot.
> Once reboot if any FV header data is corrupted it will result the whole 
> variable system be re-initialized.
> 
> Please help review the patch and give the comments, thanks 
>
Hi Yong Li,

Thank you very much for providing the context. Sorry, I actually had
missed this.

Hi Quingyu Shang,

The patch LGTM. Thanks a lot for finding the issue and fixing it. In
future, could you please run BaseTools/Scripts/GetMaintainer.py on the
patches and CC all the people it lists using "Cc:" tag?

Reviewed-by: Sunil V L 

Thanks!
Sunil


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107794): https://edk2.groups.io/g/devel/message/107794
Mute This Topic: https://groups.io/mt/100684601/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 2/2] StandaloneMmPkg: Fix HOB space and heap space conflicted issue

2023-08-16 Thread Nhi Pham via groups.io

Hi Ard and Ming,

I have been seeing an issue with StandaloneMM HobLib that can be fixed 
by this patch as well.


The function CreateHob() in the HobLib instance 
StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf 
does not work at all. The HobList is early created by the 
StandaloneMmCoreEntryPoint then it is relocated on the heap memory by 
StandaloneMmCore. But the FreeMemoryTop and FreeMemoryBottom are not 
updated accordingly and the HOB free memory top is overlapped with the 
heap space. This causes the CreateHob() function to not work as 
expected. Introducing the PcdMemoryHobSize is reasonable to fix this issue.


I tested this patch in my end.

Tested-by: Nhi Pham 

Thanks,

-Nhi

On 5/12/2022 5:09 PM, Ming Huang via groups.io wrote:


在 5/3/22 5:10 PM, Ard Biesheuvel 写道:

On Wed, 9 Feb 2022 at 13:26, Ming Huang  wrote:

The heap space will be rewrote if a StandloneMmPkg module create HOB
by BuildGuidHob() interface and write data to HOB space.

Can you elaborate? What is supposed to happen and why, and what is
happening instead?

I tried my best to explain the issue:

-<--HandOffHob->EfiFreeMemoryTop
|   |
|   |
|   |
|   |
|   |
|   |
|  mMmMemoryMap |
|---|<--HandOffHob->EfiFreeMemoryBottom
|  HobEnd   |
|---|<--HandOffHob->EfiEndOfHobList
|   |
|  Hob #1   |
-<--MmHobStart
1 The mMmMemoryMap which use for free page is on above the HobEnd.
2 Create a hob by BuildGuidHob(), the HobEnd will move up and cover
   the structure or list using by free page.

After this patch, there is a pre-allocation space for creating hob.

-<--HandOffHob->EfiFreeMemoryTop
|   |
|   |
|   |
|   |
| mMmMemoryMap  |
|---|<--HandOffHob->EfiFreeMemoryBottom
|  Hob free space   | by PcdMemoryHobSize
|---|
|  HobEnd   |
|---|<--HandOffHob->EfiEndOfHobList
|   |
|  Hob #1   |
-<--MmHobStart


Add a PCD PcdMemoryHobSize for pre-allocation a space to create HOB to
fix this issue.

Signed-off-by: Ming Huang 
---
  StandaloneMmPkg/Core/StandaloneMmCore.c   | 17 -
  StandaloneMmPkg/Core/StandaloneMmCore.inf |  3 +++
  StandaloneMmPkg/StandaloneMmPkg.dec   |  2 ++
  3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.c 
b/StandaloneMmPkg/Core/StandaloneMmCore.c
index d221f1d111..1cf259d946 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.c
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.c
@@ -512,6 +512,9 @@ StandaloneMmMain (
EFI_MMRAM_DESCRIPTOR*MmramRanges;
UINTN   MmramRangeCount;
EFI_HOB_FIRMWARE_VOLUME *BfvHob;
+  EFI_HOB_HANDOFF_INFO_TABLE  *HandOffHobNew;
+  EFI_HOB_HANDOFF_INFO_TABLE  *HandOffHobOrg;
+  UINT64  MaxHobSize = PcdGet64 (PcdMemoryHobSize);

ProcessLibraryConstructorList (HobStart, );

@@ -619,10 +622,22 @@ StandaloneMmMain (
//
HobSize = GetHobListSize (HobStart);
DEBUG ((DEBUG_INFO, "HobSize - 0x%x\n", HobSize));
-  MmHobStart = AllocatePool (HobSize);
+  ASSERT (HobSize <= MaxHobSize);
+  MmHobStart = AllocatePool (MaxHobSize);
DEBUG ((DEBUG_INFO, "MmHobStart - 0x%x\n", MmHobStart));
ASSERT (MmHobStart != NULL);
CopyMem (MmHobStart, HobStart, HobSize);
+  //
+  // Initlialize the new HOB table
+  //
+  HandOffHobOrg = (EFI_HOB_HANDOFF_INFO_TABLE *)HobStart;
+  HandOffHobNew = (EFI_HOB_HANDOFF_INFO_TABLE *)MmHobStart;
+  HandOffHobNew->EfiEndOfHobList = (EFI_PHYSICAL_ADDRESS)MmHobStart +
+(HandOffHobOrg->EfiEndOfHobList - (EFI_PHYSICAL_ADDRESS)HobStart);
+  HandOffHobNew->EfiFreeMemoryBottom = HandOffHobNew->EfiEndOfHobList +
+   sizeof (EFI_HOB_GENERIC_HEADER);
+  HandOffHobNew->EfiFreeMemoryTop = (EFI_PHYSICAL_ADDRESS)MmHobStart + 
MaxHobSize;
+
Status = MmInstallConfigurationTable (, , 
MmHobStart, HobSize);
ASSERT_EFI_ERROR (Status);

diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf 
b/StandaloneMmPkg/Core/StandaloneMmCore.inf
index c44b9ff333..37e6135d73 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
@@ -76,6 +76,9 @@
gEfiEventExitBootServicesGuid
gEfiEventReadyToBootGuid

+[FixedPcd]
+  gStandaloneMmPkgTokenSpaceGuid.PcdMemoryHobSize
+
  #
  # This configuration fails for CLANGPDB, which does not support PIE in the GCC
  # sense. Such however is required for ARM family 

Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG?

2023-08-16 Thread 汪流
Yes, the RELEASE build compiler flags should be relaxed, all error checks that 
the DEBUG target should do, it doesn't belong to the RELEASE's job.




---
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
index 28d9af4..ac3b5ec 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
+++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
@@ -54,7 +54,7 @@
!include MdePkg/MdeLibs.dsc.inc

[BuildOptions]
-  GCC:RELEASE_*_*_CC_FLAGS   = -DMDEPKG_NDEBUG
+  GCC:RELEASE_*_*_CC_FLAGS   = -flto
!ifdef $(SOURCE_DEBUG_ENABLE)
  GCC:*_*_RISCV64_GENFW_FLAGS= --keepexceptiontable
!endif
--
2.39.1




I have added the LTO flag and dropped MDEPKG_NDEBUG in the .dsc file, it 
compiled successfully, the build log: 
https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64

Do you have other way to add LTO flag to compile that don't change the source 
code, i tried to use environment variable, like exported CFLAGS=-flto to 
compile, but the compiler doesn't work with it.













-原始郵件-
發件人:"Andrew (EFI) Fish" 
發送時間:2023-08-10 21:50:55 (星期四)
收件人: edk2-devel-groups-io , wang...@iscas.ac.cn
抄送:
主題: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG?


I think that you are advocating that since we have code that error checks on 
DEBUG and not RELEASE builds we should relax the RELEASE build compiler flags?


/home/abuild/rpmbuild/BUILD/edk2-edk2-stable202305/MdeModulePkg/Core/Dxe/Hand/Handle.c:1183:24:
 error: ‘Prot’ may be used uninitialized [-Werror=maybe-uninitialized]
  ASSERT (Prot != NULL);
  //
  // EFI_ALREADY_STARTED is not an error for bus driver.
  // Return the corresponding protocol interface.
  //
  *Interface = Prot->Interface;


A given platform can add ASSERT into release builds if it wants. I’ve actually 
done that for power on before. If your compiler supports LTO you are not 
required to set MDEPKG_NDEBUG on RELEASE builds, and you can used a PCD to 
configure your debug level, per build type. 


Maybe we should just have the error checks in all paths?


Thanks,


Andrew Fish


On Aug 10, 2023, at 6:44 AM, 汪流  wrote:


I want to build a rpm package for edk2-stable202305 on riscv64, however I get 
some uninitialized variable error, I have found that the reason is -Werror flag.
My build target was release. I think the flag should used in the debug ,not in 
release.
My build command:  build -t GCC5 -n $NCPUS -b RELEASE -a RISCV64 -p 
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc -D SECURE_BOOT_ENABLE=TRUE -D 
TPM_ENABLE=TRUE -D TPM_CONFIG_ENABLE=TRUE


This is my packet log:   

https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/edk2-test/openEuler_Mainline_standard_riscv64_gcc/riscv64

https://build.tarsier-infra.com/build/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/openEuler_Mainline_standard_riscv64_gcc/riscv64/edk2-test/_log





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




Re: [edk2-devel] [PATCH] OvmfPkg/RiscVVirt: Fix issues in VarStore Blockmap config

2023-08-16 Thread Li, Yong
Hi Sunil,

Qingyu is from Penglai team, when we were working together in enabling 
StandaloneMm variable service, 
we found an issue in OvmfPkg/RiscVVirt/VarStore.fdf.inc,  in which the blockmap 
config is not aligned with the value in
OvmfPkg/RiscVVirt/Library/VirtNorFlashPlatformLib/VirtNorFlashStaticLib.c, 
which is SIZE_256KB

That mis-alignment won't cause any issue if the variable size is small and less 
than 0x1000, but if store more data in the flash device then it will cause data 
loss issue during reboot.
Once reboot if any FV header data is corrupted it will result the whole 
variable system be re-initialized.

Please help review the patch and give the comments, thanks 

Thanks,
Yong Li

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Qingyu Shang
Sent: Friday, August 11, 2023 7:05 PM
To: devel@edk2.groups.io
Cc: Qingyu Shang <2931013...@sjtu.edu.cn>
Subject: [edk2-devel] [PATCH] OvmfPkg/RiscVVirt: Fix issues in VarStore 
Blockmap config

The block size configuration of Blockmap does not match that in Qemu 
VirtNorFlash, which causes variable data to be written into FtwWorkBlock by 
mistake, resulting in data loss during reboot. Fix it and update new checksum 
value.

Signed-off-by: Qingyu Shang <2931013...@sjtu.edu.cn>
---
 OvmfPkg/RiscVVirt/VarStore.fdf.inc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/RiscVVirt/VarStore.fdf.inc 
b/OvmfPkg/RiscVVirt/VarStore.fdf.inc
index 6bc619e50c..aba32315cc 100644
--- a/OvmfPkg/RiscVVirt/VarStore.fdf.inc
+++ b/OvmfPkg/RiscVVirt/VarStore.fdf.inc
@@ -30,9 +30,9 @@ DATA = {
   # Signature "_FVH"   # Attributes   0x5f, 0x46, 0x56, 0x48, 0xff, 0xfe, 
0x04, 0x00,   # HeaderLength # CheckSum # ExtHeaderOffset #Reserved #Revision-  
0x48, 0x00, 0x2F, 0xF1, 0x00, 0x00, 0x00, 0x02,-  # Blockmap[0]: 0x20 Blocks * 
0x1000 Bytes / Block-  0x00, 0x08, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00,+  0x48, 
0x00, 0x28, 0x09, 0x00, 0x00, 0x00, 0x02,+  # Blockmap[0]: 0x3 Blocks * 0x4 
Bytes / Block+  0x3, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04, 0x00,   # Blockmap[1]: 
End   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,   ## This is the 
VARIABLE_STORE_HEADER-- 
2.25.1



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107708): https://edk2.groups.io/g/devel/message/107708
Mute This Topic: https://groups.io/mt/100684601/6998987
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [yong...@intel.com] 
-=-=-=-=-=-=




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