Date   

[PATCH v2 0/2] Add MM Configuration PPI definition to MdePkg

Kun Qin
 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3440

This patch series is a follow up of previous submission:
https://edk2.groups.io/g/devel/message/76748

v2 patches mainly focus on feedback for commits submitted in v1 patches:
a. Moved EFI_MM_RESERVED_MMRAM_REGION from PiMmCis to PiMultiPhase;
b. Updated inclusion in new file accordingly;

Patch v2 branch: https://github.com/kuqin12/edk2/tree/mm_config_ppi_v2

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Cc: Michael Kubacki <michael.kubacki@...>

Kun Qin (2):
MdePkg: MmConfiguration: Move definition of
EFI_MM_RESERVED_MMRAM_REGION
MdePkg: MmConfiguration: Added definition of MM Configuration PPI

MdePkg/Include/Pi/PiMultiPhase.h | 16 +++++
MdePkg/Include/Ppi/MmConfiguration.h | 62 ++++++++++++++++++++
MdePkg/Include/Protocol/MmConfiguration.h | 16 -----
MdePkg/MdePkg.dec | 3 +
4 files changed, 81 insertions(+), 16 deletions(-)
create mode 100644 MdePkg/Include/Ppi/MmConfiguration.h

--
2.31.1.windows.1


Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update

Bret Barkelew <bret.barkelew@...>
 

Yeah, the only other thought I had for moving forwards quickly (i.e. hackily) is to define a new GUID that just means “I use an updated header configuration” since the EFI_GUID field comes first and is a well-understood size.

 

I would prefer the “deprecate, break builds, fix code, move forward” approach.

 

- Bret

 

From: Kun Qin via groups.io
Sent: Wednesday, June 23, 2021 5:53 PM
To: Wu, Hao A; devel@edk2.groups.io
Cc: Kinney, Michael D; Liming Gao; Liu, Zhiguang; Andrew Fish; Laszlo Ersek; Lindholm, Leif; Bret Barkelew; Michael Kubacki
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update

 

Hi Hao,

We have been circulating different ideas internally about how to enforce
a warning around this change.

There is an idea of deprecating the old structure and replacing it with
a newly defined EFI_MM_COMMUNICATE_HEADER_V2. That way all consumers
will be enforced to update their implementation and (hopefully) inspect
the compatibility accordingly. In addition, we could add other fields
such as signature and/or header version number for further extensibility.

If there are code instances that uses "sizeof(EFI_GUID) + sizeof(UINTN)"
in lieu of OFFSET_OF, this implementation is essentially decoupled from
the structure definition. So compiler might not be able to help. In that
case, runtime protections such as pool guard or stack guard might be useful.

Please let me know if you think header structure V2 is an idea worth to try.

Thanks,
Kun

On 06/15/2021 18:15, Wu, Hao A wrote:
> Thanks Kun,
>
> I am a little concerned on whether there will be other missing cases that are not covered by this patch series.
>
> I am also wondering is there any detection can be made to warn the cases that code modification should be made after this structure update.
> Otherwise, it will be the burden for the platform owners to review the platform codes following your guide mentioned in this patch.
>
> Hoping others can provide some inputs on this.
>
> Best Regards,
> Hao Wu
>

>> -----Original Message-----
>> From: Kun Qin <kuqin12@...>
>> Sent: Wednesday, June 16, 2021 4:51 AM
>> To: Wu, Hao A <hao.a.wu@...>; devel@edk2.groups.io
>> Cc: Kinney, Michael D <michael.d.kinney@...>; Liming Gao
>> <gaoliming@...>; Liu, Zhiguang <zhiguang.liu@...>;
>> Andrew Fish <afish@...>; Laszlo Ersek <lersek@...>; Leif
>> Lindholm <leif@...>
>> Subject: Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification:
>> EFI_MM_COMMUNICATE_HEADER Update
>>
>> Hi Hao,
>>
>> Sorry that I missed comments for this patch earlier. You are correct. I only
>> inspected SmmLockBoxPeiLib. The PEI instance handled mode switch with
>> ```OFFSET_OF ``` function. But DXE instance still have a few use cases that will
>> be impacted.
>>
>> I will update this read me file and add a code change patch for this library in
>> v2. Thanks for pointing this out.
>>
>> Regards,
>> Kun
>>
>> On 06/11/2021 00:46, Wu, Hao A wrote:
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun
>>>> Qin
>>>> Sent: Thursday, June 10, 2021 9:43 AM
>>>> To: devel@edk2.groups.io
>>>> Cc: Kinney, Michael D <michael.d.kinney@...>; Liming Gao
>>>> <gaoliming@...>; Liu, Zhiguang <zhiguang.liu@...>;
>>>> Andrew Fish <afish@...>; Laszlo Ersek <lersek@...>; Leif
>>>> Lindholm <leif@...>
>>>> Subject: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification:
>>>> EFI_MM_COMMUNICATE_HEADER Update
>>>>
>>>> REF: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3430&amp;data=04%7C01%7CBret.Barkelew%40microsoft.com%7Ca480819ff5e84e12239f08d936aa7bc5%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637600928127681913%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=483mG24s%2Bp0xSF90Wf%2Fmh2TN1atrIQa5mOrLyEPCTuE%3D&amp;reserved=0
>>>>
>>>> This change includes specification update markdown file that
>>>> describes the proposed PI Specification v1.7 Errata A in detail and
>>>> potential impact to the existing codebase.
>>>>
>>>> Cc: Michael D Kinney <michael.d.kinney@...>
>>>> Cc: Liming Gao <gaoliming@...>
>>>> Cc: Zhiguang Liu <zhiguang.liu@...>
>>>> Cc: Andrew Fish <afish@...>
>>>> Cc: Laszlo Ersek <lersek@...>
>>>> Cc: Leif Lindholm <leif@...>
>>>>
>>>> Signed-off-by: Kun Qin <kuqin12@...>
>>>> ---
>>>>    BZ3430-SpecChange.md | 88 ++++++++++++++++++++
>>>>    1 file changed, 88 insertions(+)
>>>>
>>>> diff --git a/BZ3430-SpecChange.md b/BZ3430-SpecChange.md new file
>>>> mode
>>>> 100644 index 000000000000..33a1ffda447b
>>>> --- /dev/null
>>>> +++ b/BZ3430-SpecChange.md
>>>> @@ -0,0 +1,88 @@
>>>> +# Title: Change MessageLength Field of
>> EFI_MM_COMMUNICATE_HEADER
>>>> to
>>>> +UINT64
>>>> +
>>>> +## Status: Draft
>>>> +
>>>> +## Document: UEFI Platform Initialization Specification Version 1.7
>>>> +Errata A
>>>> +
>>>> +## License
>>>> +
>>>> +SPDX-License-Identifier: CC-BY-4.0
>>>> +
>>>> +## Submitter: [TianoCore Community](https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.tianocore.org%2F&amp;data=04%7C01%7CBret.Barkelew%40microsoft.com%7Ca480819ff5e84e12239f08d936aa7bc5%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637600928127681913%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=eRu4We7iSzuwrsS9MqIO2iqLxXHZ6CpUkInVpty554c%3D&amp;reserved=0)
>>>> +
>>>> +## Summary of the change
>>>> +
>>>> +Change the `MessageLength` Field of
>> `EFI_MM_COMMUNICATE_HEADER`
>>>> from UINTN to UINT64 to remove architecture dependency:
>>>> +
>>>> +```c
>>>> +typedef struct {
>>>> +  EFI_GUID  HeaderGuid;
>>>> +  UINT64    MessageLength;
>>>> +  UINT8     Data[ANYSIZE_ARRAY];
>>>> +} EFI_MM_COMMUNICATE_HEADER;
>>>> +```
>>>> +
>>>> +## Benefits of the change
>>>> +
>>>> +In PI Spec v1.7 Errata A, Vol.4, Sec 5.7 MM Communication Protocol,
>>>> +the
>>>> MessageLength field of `EFI_MM_COMMUNICATE_HEADER` (also
>> defined as
>>>> `EFI_SMM_COMMUNICATE_HEADER`) is defined as type UINTN.
>>>> +
>>>> +But this structure, as a generic definition, could be used for both
>>>> +PEI and
>>>> DXE MM communication. Thus for a system that supports PEI MM launch,
>>>> but operates PEI in 32bit mode and MM foundation in 64bit, the
>>>> current `EFI_MM_COMMUNICATE_HEADER` definition will cause
>> structure
>>>> parse error due to UINTN used.
>>>> +
>>>> +## Impact of the change
>>>> +
>>>> +This change will impact the known structure consumers including:
>>>> +
>>>> +```bash
>>>> +MdeModulePkg/Core/PiSmmCore/PiSmmIpl
>>>> +MdeModulePkg/Application/SmiHandlerProfileInfo
>>>> +MdeModulePkg/Application/MemoryProfileInfo
>>>> +```
>>>> +
>>>> +For consumers that are not using
>>>> `OFFSET_OF(EFI_MM_COMMUNICATE_HEADER, Data)`, but performing
>> explicit
>>>> addition such as the existing
>>>>
>> MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.
>>>> c, one will need to change code implementation to match new structure
>>>> definition. Otherwise, the code compiled on IA32 architecture will
>>>> experience structure field dereference error.
>>>> +
>>>> +User who currently uses UINTN local variables as place holder of
>>>> MessageLength will need to use caution to make cast from UINTN to
>>>> UINT64 and vice versa. It is recommended to use `SafeUint64ToUintn`
>>>> for such operations when the value is indeterministic.
>>>> +
>>>> +Note: MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib is
>> also
>>>> consuming this structure, but it handled this size discrepancy
>>>> internally. If this
>>>
>>>
>>> Hello Kun,
>>>
>>> Sorry for a question.
>>> I am not sure why the current codes in file SmmLockBoxDxeLib.c will work
>> properly after the UINTN -> UINT64 change.
>>>
>>> For example:
>>> LockBoxGetSmmCommBuffer():
>>>     MinimalSizeNeeded = sizeof (EFI_GUID) +
>>>                         sizeof (UINTN) +
>>>                         MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_SAVE),
>>>                              MAX (sizeof
>> (EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES),
>>>                                   MAX (sizeof
>> (EFI_SMM_LOCK_BOX_PARAMETER_UPDATE),
>>>                                        MAX (sizeof
>> (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE),
>>>                                             sizeof
>>> (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)))));
>>>
>>> SaveLockBox():
>>>     UINT8                           TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN)
>> + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)];
>>>
>>> Is the series missed changes for SmmLockBoxDxeLib or I got something
>> wrong?
>>>
>>> Best Regards,
>>> Hao Wu
>>>
>>>
>>>> potential spec change is not applied, all applicable PEI MM
>>>> communicate callers will need to use the same routine as that of
>>>> SmmLockBoxPeiLib to invoke a properly populated
>>>> EFI_MM_COMMUNICATE_HEADER to be used in X64 MM foundation.
>>>> +
>>>> +## Detailed description of the change [normative updates]
>>>> +
>>>> +### Specification Changes
>>>> +
>>>> +1. In PI Specification v1.7 Errata A: Vol. 4 Page-91, the definition
>>>> +of
>>>> `EFI_MM_COMMUNICATE_HEADER` should be changed from current:
>>>> +
>>>> +```c
>>>> +typedef struct {
>>>> +  EFI_GUID  HeaderGuid;
>>>> +  UINTN     MessageLength;
>>>> +  UINT8     Data[ANYSIZE_ARRAY];
>>>> +} EFI_MM_COMMUNICATE_HEADER;
>>>> +```
>>>> +
>>>> +to:
>>>> +
>>>> +```c
>>>> +typedef struct {
>>>> +  EFI_GUID  HeaderGuid;
>>>> +  UINT64    MessageLength;
>>>> +  UINT8     Data[ANYSIZE_ARRAY];
>>>> +} EFI_MM_COMMUNICATE_HEADER;
>>>> +```
>>>> +
>>>> +### Code Changes
>>>> +
>>>> +1. Remove the explicit calculation of the offset of `Data` in
>>>> `EFI_MM_COMMUNICATE_HEADER`. Thus applicable calculations of
>>>> `sizeof(EFI_GUID) + sizeof(UINTN)` should be replaced with
>>>> `OFFSET_OF(EFI_MM_COMMUNICATE_HEADER, Data)` or similar
>> alternatives.
>>>> These calculations are identified in:
>>>> +
>>>> +```bash
>>>>
>> +MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.
>>>> c
>>>> +MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
>>>> +```
>>>> +
>>>> +1. Resolve potentially mismatched type between `UINTN` and `UINT64`.
>>>> This would occur when `MessageLength` or its derivitive are used for
>>>> local calculation with existing `UINTN` typed variables. Code change
>>>> regarding this perspective is per case evaluation: if the variables
>>>> involved are all deterministic values, and there is no overflow or
>>>> underflow risk, a cast operation (from `UINTN` to `UINT64`) can be
>>>> safely used. Otherwise, the calculation will be performed in `UINT64`
>>>> bitwidth and then convert to `UINTN` using `SafeUint64*` and
>>>> `SafeUint64ToUintn`, respectively. These operations are identified in:
>>>> +
>>>> +```bash
>>>> +MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
>>>>
>> +MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.
>>>> c
>>>> +MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
>>>> +```
>>>> +
>>>> +1. After all above changes applied and specification updated,
>>>> `MdePkg/Include/Protocol/MmCommunication.h` will need to be
>> updated
>>>> to match new definition that includes the field type update.
>>>> --
>>>> 2.31.1.windows.1
>>>>
>>>>
>>>>
>>>>
>>>>
>>>




 


Re: Need help with CI/CD failure for the PR

Michael D Kinney
 

Liming,

Looks like it failed to build BaseTools:

https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24448&view=logs&jobId=4a97f94c-8e1f-5e76-68ba-50872604dd63&j=2640d2b2-7c53-5a2b-80c7-040377c664fd&t=9d40d6fd-f2ec-5369-fb67-2d935d8d6b47

This PR modified GenFw, so it is likely related.

You have to download artifact to see the detailed log. Select "BASETOOLS_BUILD.txt from the following link:

https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=24448&view=artifacts&pathAsName=false&type=publishedArtifacts


INFO - Microsoft (R) Program Maintenance Utility Version 14.29.30038.1
INFO - Copyright (C) Microsoft Corporation. All rights reserved.
INFO -
INFO - cl.exe -c /nologo /Zi /c /O2 /MT /W4 /WX /D _CRT_SECURE_NO_DEPRECATE /D _CRT_NONSTDC_NO_DEPRECATE -I . -I D:\a\1\s\BaseTools\Source\C\Include -I D:\a\1\s\BaseTools\Source\C\Include\Ia32 -I D:\a\1\s\BaseTools\Source\C\Common GenFw.c -FoGenFw.obj
INFO - GenFw.c
INFO - cl.exe -c /nologo /Zi /c /O2 /MT /W4 /WX /D _CRT_SECURE_NO_DEPRECATE /D _CRT_NONSTDC_NO_DEPRECATE -I . -I D:\a\1\s\BaseTools\Source\C\Include -I D:\a\1\s\BaseTools\Source\C\Include\Ia32 -I D:\a\1\s\BaseTools\Source\C\Common ElfConvert.c -FoElfConvert.obj
INFO - ElfConvert.c
INFO - cl.exe -c /nologo /Zi /c /O2 /MT /W4 /WX /D _CRT_SECURE_NO_DEPRECATE /D _CRT_NONSTDC_NO_DEPRECATE -I . -I D:\a\1\s\BaseTools\Source\C\Include -I D:\a\1\s\BaseTools\Source\C\Include\Ia32 -I D:\a\1\s\BaseTools\Source\C\Common Elf32Convert.c -FoElf32Convert.obj
INFO - Elf32Convert.c
INFO - cl.exe -c /nologo /Zi /c /O2 /MT /W4 /WX /D _CRT_SECURE_NO_DEPRECATE /D _CRT_NONSTDC_NO_DEPRECATE -I . -I D:\a\1\s\BaseTools\Source\C\Include -I D:\a\1\s\BaseTools\Source\C\Include\Ia32 -I D:\a\1\s\BaseTools\Source\C\Common Elf64Convert.c -FoElf64Convert.obj
INFO - Elf64Convert.c
INFO - Elf64Convert.c(540): error C2220: the following warning is treated as an error
INFO - Elf64Convert.c(540): warning C4244: '=': conversion from 'Elf64_Addr' to 'UINT32', possible loss of data
INFO - NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30037\bin\HostX86\x86\cl.exe"' : return code '0x2'
INFO - Stop.
INFO -
INFO - NMAKE : fatal error U1077: 'if' : return code '0x1'
INFO - Stop.
INFO - NMAKE : fatal error U1077: 'if' : return code '0x1'
INFO - Stop.
INFO - ------------------------------------------------
INFO - --------------Cmd Output Finished---------------
INFO - --------- Running Time (mm:ss): 00:29 ----------
INFO - ----------- Return Code: 0x00000002 ------------
INFO - ------------------------------------------------

Best regardm,

Mike

-----Original Message-----
From: gaoliming <gaoliming@...>
Sent: Wednesday, June 23, 2021 6:45 PM
To: 'Sunil V L' <sunilvl@...>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@...>; 'Sean
Brogan' <sean.brogan@...>
Cc: 'Chang, Abner (HPS SW/FW Technologist)' <abner.chang@...>; 'Schaefer, Daniel' <daniel.schaefer@...>
Subject: 回复: Need help with CI/CD failure for the PR

Mike and Sean:
Have you any idea on how to find the error message in PR failure report?

For this PR https://github.com/tianocore/edk2/pull/1738, I don't find the real error message.

Thanks
Liming
-----邮件原件-----
发件人: Sunil V L <sunilvl@...>
发送时间: 2021年6月22日 18:53
收件人: devel@edk2.groups.io
抄送: gaoliming@...; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@...>; Schaefer, Daniel <daniel.schaefer@...>
主题: Need help with CI/CD failure for the PR

Hi,

The PR https://github.com/tianocore/edk2/pull/1738 has hit error with below
message.

"PR can not be merged due to a Windows VS2019 failure. Please resolve and
resubmit"

Unfortunately, I am unable to gather any details why it is failing. Also, I don't
have VS2019 build environment to try the build myself. Could some one with
VS2019 or knowledge of this build environment help me what exactly is the
issue with my patch?

Thanks in advance!

Sunil


Re: [PATCHV2] CryptoPkg/BaseCryptLib: Enabled CryptSha512 for Smm/Runtime drivers

Wang, Jian J
 

Pushed at eba32695ee6979137c86c3d20d0711d49d5c3ba8

Regards,
Jian

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@...>
Sent: Thursday, June 24, 2021 10:06 AM
To: devel@edk2.groups.io; gaoliming@...; Xue, Shengfeng
<xueshengfeng@...>; Wang, Jian J <jian.j.wang@...>
Cc: Xue, ShengfengX <shengfengx.xue@...>
Subject: RE: [edk2-devel] [PATCHV2] CryptoPkg/BaseCryptLib: Enabled
CryptSha512 for Smm/Runtime drivers

Ah. Yes. I think so.

Hi Jian
Can you help on that?

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming
Sent: Thursday, June 24, 2021 9:30 AM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@...>; Xue,
Shengfeng <xueshengfeng@...>; Wang, Jian J
<jian.j.wang@...>
Cc: Xue, ShengfengX <shengfengx.xue@...>
Subject: 回复: [edk2-devel] [PATCHV2] CryptoPkg/BaseCryptLib: Enabled
CryptSha512 for Smm/Runtime drivers

So far, there is no objection for this patch. How about merge it?

Thanks
Liming
-----邮件原件-----
发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Yao, Jiewen
发送时间: 2021年6月9日 11:08
收件人: Xue, Shengfeng <xueshengfeng@...>;
devel@edk2.groups.io; Wang, Jian J <jian.j.wang@...>
抄送: Xue, ShengfengX <shengfengx.xue@...>
主题: Re: [edk2-devel] [PATCHV2] CryptoPkg/BaseCryptLib: Enabled
CryptSha512 for Smm/Runtime drivers

Thank you! Shengfeng

Reviewed-by: Jiewen Yao <Jiewen.yao@...>

I recommend to wait for *1 week*, to see if anyone has concern on size
change.

Thank you
Yao Jiewen


-----Original Message-----
From: xueshengfeng <xueshengfeng@...>
Sent: Tuesday, June 8, 2021 12:31 PM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@...>; Wang,
Jian J
<jian.j.wang@...>
Cc: Xue, ShengfengX <shengfengx.xue@...>
Subject: [PATCHV2] CryptoPkg/BaseCryptLib: Enabled CryptSha512 for
Smm/Runtime drivers

Intel Platform utility Syscfg/sysfwupdt will trigger SMI
to enter BIOS interface. then BIOS invoke EncodePassword
in SMM mode to check password.
it's need sha384(in CryptSha512.c) in SMM mode.

the origin SmmCryptLib.lib size is 1389KB,
after changed, the size is 1391KB.

the origin RuntimeCryptLib.lib size is 911KB,
after changed,the size is 913KB.

in SmmCryptLib.inf and RuntimeCryptLib.inf,
change CryptSha512NULL.c to CryptSha512.c.

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

Signed-off-by: xueshengfeng <xueshengfeng@...>
---
CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 6 +++---
CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
index 3d3a6fb94a..fdbb6edfd2 100644
--- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
@@ -11,8 +11,8 @@
# functions, PKCS#7 SignedData sign functions, Diffie-Hellman
functions,
and
# authenticode signature verification functions are not supported in
this
instance.
#
-# Copyright (c) 2009 - 2020, Intel Corporation. All rights
reserved.<BR>
-# Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All
rights
reserved.<BR>
+# Copyright (c) 2009 - 2021, Intel Corporation. All rights
reserved.<BR>
+# Copyright (c) 2021, Hewlett Packard Enterprise Development LP. All
rights
reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
@@ -39,7 +39,7 @@
Hash/CryptSha1.c
Hash/CryptSha256.c
Hash/CryptSm3.c
- Hash/CryptSha512Null.c
+ Hash/CryptSha512.c
Hmac/CryptHmacSha256.c
Kdf/CryptHkdf.c
Cipher/CryptAes.c
diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
index 07c376ce04..e6470d7a21 100644
--- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
@@ -10,7 +10,7 @@
# RSA external functions, PKCS#7 SignedData sign functions,
Diffie-Hellman
functions, and
# authenticode signature verification functions are not supported in
this
instance.
#
-# Copyright (c) 2010 - 2020, Intel Corporation. All rights
reserved.<BR>
+# Copyright (c) 2010 - 2021, Intel Corporation. All rights
reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
@@ -37,7 +37,7 @@
Hash/CryptSha1.c
Hash/CryptSha256.c
Hash/CryptSm3.c
- Hash/CryptSha512Null.c
+ Hash/CryptSha512.c
Hmac/CryptHmacSha256.c
Kdf/CryptHkdfNull.c
Cipher/CryptAes.c
--
2.31.1.windows.1









Re: [PATCHV2] CryptoPkg/BaseCryptLib: Enabled CryptSha512 for Smm/Runtime drivers

Yao, Jiewen
 

Ah. Yes. I think so.

Hi Jian
Can you help on that?

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming
Sent: Thursday, June 24, 2021 9:30 AM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@...>; Xue,
Shengfeng <xueshengfeng@...>; Wang, Jian J
<jian.j.wang@...>
Cc: Xue, ShengfengX <shengfengx.xue@...>
Subject: 回复: [edk2-devel] [PATCHV2] CryptoPkg/BaseCryptLib: Enabled
CryptSha512 for Smm/Runtime drivers

So far, there is no objection for this patch. How about merge it?

Thanks
Liming
-----邮件原件-----
发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Yao, Jiewen
发送时间: 2021年6月9日 11:08
收件人: Xue, Shengfeng <xueshengfeng@...>;
devel@edk2.groups.io; Wang, Jian J <jian.j.wang@...>
抄送: Xue, ShengfengX <shengfengx.xue@...>
主题: Re: [edk2-devel] [PATCHV2] CryptoPkg/BaseCryptLib: Enabled
CryptSha512 for Smm/Runtime drivers

Thank you! Shengfeng

Reviewed-by: Jiewen Yao <Jiewen.yao@...>

I recommend to wait for *1 week*, to see if anyone has concern on size
change.

Thank you
Yao Jiewen


-----Original Message-----
From: xueshengfeng <xueshengfeng@...>
Sent: Tuesday, June 8, 2021 12:31 PM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@...>; Wang,
Jian J
<jian.j.wang@...>
Cc: Xue, ShengfengX <shengfengx.xue@...>
Subject: [PATCHV2] CryptoPkg/BaseCryptLib: Enabled CryptSha512 for
Smm/Runtime drivers

Intel Platform utility Syscfg/sysfwupdt will trigger SMI
to enter BIOS interface. then BIOS invoke EncodePassword
in SMM mode to check password.
it's need sha384(in CryptSha512.c) in SMM mode.

the origin SmmCryptLib.lib size is 1389KB,
after changed, the size is 1391KB.

the origin RuntimeCryptLib.lib size is 911KB,
after changed,the size is 913KB.

in SmmCryptLib.inf and RuntimeCryptLib.inf,
change CryptSha512NULL.c to CryptSha512.c.

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

Signed-off-by: xueshengfeng <xueshengfeng@...>
---
CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 6 +++---
CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
index 3d3a6fb94a..fdbb6edfd2 100644
--- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
@@ -11,8 +11,8 @@
# functions, PKCS#7 SignedData sign functions, Diffie-Hellman
functions,
and
# authenticode signature verification functions are not supported in
this
instance.
#
-# Copyright (c) 2009 - 2020, Intel Corporation. All rights
reserved.<BR>
-# Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All
rights
reserved.<BR>
+# Copyright (c) 2009 - 2021, Intel Corporation. All rights
reserved.<BR>
+# Copyright (c) 2021, Hewlett Packard Enterprise Development LP. All
rights
reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
@@ -39,7 +39,7 @@
Hash/CryptSha1.c
Hash/CryptSha256.c
Hash/CryptSm3.c
- Hash/CryptSha512Null.c
+ Hash/CryptSha512.c
Hmac/CryptHmacSha256.c
Kdf/CryptHkdf.c
Cipher/CryptAes.c
diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
index 07c376ce04..e6470d7a21 100644
--- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
@@ -10,7 +10,7 @@
# RSA external functions, PKCS#7 SignedData sign functions,
Diffie-Hellman
functions, and
# authenticode signature verification functions are not supported in
this
instance.
#
-# Copyright (c) 2010 - 2020, Intel Corporation. All rights
reserved.<BR>
+# Copyright (c) 2010 - 2021, Intel Corporation. All rights
reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
@@ -37,7 +37,7 @@
Hash/CryptSha1.c
Hash/CryptSha256.c
Hash/CryptSm3.c
- Hash/CryptSha512Null.c
+ Hash/CryptSha512.c
Hmac/CryptHmacSha256.c
Kdf/CryptHkdfNull.c
Cipher/CryptAes.c
--
2.31.1.windows.1









回复: [PATCH v1 4/4] .pytool/EccCheck: Set PACKAGES_PATH env var in Ecc

gaoliming
 

Pierre:

-----邮件原件-----
发件人: Pierre.Gondois@... <Pierre.Gondois@...>
发送时间: 2021年6月23日 15:23
收件人: devel@edk2.groups.io; Sean Brogan <sean.brogan@...>;
Bret Barkelew <Bret.Barkelew@...>; Michael D Kinney
<michael.d.kinney@...>; Liming Gao <gaoliming@...>;
Sami Mujawar <sami.mujawar@...>
主题: [PATCH v1 4/4] .pytool/EccCheck: Set PACKAGES_PATH env var in Ecc

From: Pierre Gondois <Pierre.Gondois@...>

When running Ecc on other repositories (e.g.: edk2-platforms with
edk2 as a submodule), edk2 modules are referenced.
E.g.: MdePkg/..
The PACKAGES_PATH env var can be used to reference other directories
containing packages. Set it so that Ecc can find these packages.

Cc: Sean Brogan <sean.brogan@...>
Cc: Bret Barkelew <Bret.Barkelew@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Sami Mujawar <sami.mujawar@...>
Signed-off-by: Pierre Gondois <Pierre.Gondois@...>
---
.pytool/Plugin/EccCheck/EccCheck.py | 1 +
1 file changed, 1 insertion(+)

diff --git a/.pytool/Plugin/EccCheck/EccCheck.py
b/.pytool/Plugin/EccCheck/EccCheck.py
index 87f0e65a140f..49d143b32f91 100644
--- a/.pytool/Plugin/EccCheck/EccCheck.py
+++ b/.pytool/Plugin/EccCheck/EccCheck.py
@@ -67,6 +67,7 @@ class EccCheck(ICiBuildPlugin):
env = shell_environment.GetEnvironment()
env.set_shell_var('PYTHONPATH', python_path)
env.set_shell_var('WORKSPACE', workspace_path)
+ env.set_shell_var('PACKAGES_PATH',
":".join(Edk2pathObj.PackagePathList))
":" is Linux OS path separator. Windows OS path separator is ";".

Thanks
Liming
self.ECC_PASS = True
self.ApplyConfig(pkgconfig, workspace_path, basetools_path,
packagename)
modify_dir_list = self.GetModifyDir(packagename)
--
2.17.1


回复: Need help with CI/CD failure for the PR

gaoliming
 

Mike and Sean:
Have you any idea on how to find the error message in PR failure report?

For this PR https://github.com/tianocore/edk2/pull/1738, I don't find the real error message.

Thanks
Liming

-----邮件原件-----
发件人: Sunil V L <sunilvl@...>
发送时间: 2021年6月22日 18:53
收件人: devel@edk2.groups.io
抄送: gaoliming@...; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@...>; Schaefer, Daniel <daniel.schaefer@...>
主题: Need help with CI/CD failure for the PR

Hi,

The PR https://github.com/tianocore/edk2/pull/1738 has hit error with below
message.

"PR can not be merged due to a Windows VS2019 failure. Please resolve and
resubmit"

Unfortunately, I am unable to gather any details why it is failing. Also, I don't
have VS2019 build environment to try the build myself. Could some one with
VS2019 or knowledge of this build environment help me what exactly is the
issue with my patch?

Thanks in advance!

Sunil


回复: [edk2-devel] [PATCHV2] CryptoPkg/BaseCryptLib: Enabled CryptSha512 for Smm/Runtime drivers

gaoliming
 

So far, there is no objection for this patch. How about merge it?

Thanks
Liming
-----邮件原件-----
发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Yao, Jiewen
发送时间: 2021年6月9日 11:08
收件人: Xue, Shengfeng <xueshengfeng@...>;
devel@edk2.groups.io; Wang, Jian J <jian.j.wang@...>
抄送: Xue, ShengfengX <shengfengx.xue@...>
主题: Re: [edk2-devel] [PATCHV2] CryptoPkg/BaseCryptLib: Enabled
CryptSha512 for Smm/Runtime drivers

Thank you! Shengfeng

Reviewed-by: Jiewen Yao <Jiewen.yao@...>

I recommend to wait for *1 week*, to see if anyone has concern on size
change.

Thank you
Yao Jiewen


-----Original Message-----
From: xueshengfeng <xueshengfeng@...>
Sent: Tuesday, June 8, 2021 12:31 PM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@...>; Wang,
Jian J
<jian.j.wang@...>
Cc: Xue, ShengfengX <shengfengx.xue@...>
Subject: [PATCHV2] CryptoPkg/BaseCryptLib: Enabled CryptSha512 for
Smm/Runtime drivers

Intel Platform utility Syscfg/sysfwupdt will trigger SMI
to enter BIOS interface. then BIOS invoke EncodePassword
in SMM mode to check password.
it's need sha384(in CryptSha512.c) in SMM mode.

the origin SmmCryptLib.lib size is 1389KB,
after changed, the size is 1391KB.

the origin RuntimeCryptLib.lib size is 911KB,
after changed,the size is 913KB.

in SmmCryptLib.inf and RuntimeCryptLib.inf,
change CryptSha512NULL.c to CryptSha512.c.

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

Signed-off-by: xueshengfeng <xueshengfeng@...>
---
CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf | 6 +++---
CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
index 3d3a6fb94a..fdbb6edfd2 100644
--- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
@@ -11,8 +11,8 @@
# functions, PKCS#7 SignedData sign functions, Diffie-Hellman
functions,
and
# authenticode signature verification functions are not supported in
this
instance.
#
-# Copyright (c) 2009 - 2020, Intel Corporation. All rights
reserved.<BR>
-# Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All
rights
reserved.<BR>
+# Copyright (c) 2009 - 2021, Intel Corporation. All rights
reserved.<BR>
+# Copyright (c) 2021, Hewlett Packard Enterprise Development LP. All
rights
reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
@@ -39,7 +39,7 @@
Hash/CryptSha1.c
Hash/CryptSha256.c
Hash/CryptSm3.c
- Hash/CryptSha512Null.c
+ Hash/CryptSha512.c
Hmac/CryptHmacSha256.c
Kdf/CryptHkdf.c
Cipher/CryptAes.c
diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
index 07c376ce04..e6470d7a21 100644
--- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
@@ -10,7 +10,7 @@
# RSA external functions, PKCS#7 SignedData sign functions,
Diffie-Hellman
functions, and
# authenticode signature verification functions are not supported in
this
instance.
#
-# Copyright (c) 2010 - 2020, Intel Corporation. All rights
reserved.<BR>
+# Copyright (c) 2010 - 2021, Intel Corporation. All rights
reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
@@ -37,7 +37,7 @@
Hash/CryptSha1.c
Hash/CryptSha256.c
Hash/CryptSm3.c
- Hash/CryptSha512Null.c
+ Hash/CryptSha512.c
Hmac/CryptHmacSha256.c
Kdf/CryptHkdfNull.c
Cipher/CryptAes.c
--
2.31.1.windows.1




Re: [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Michael D Kinney
 

Liming,

Yes. I am working on a configuration with will keep the current 'push' behavior
and add the 'auto-rebase' option.

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming
Sent: Wednesday, June 23, 2021 6:10 PM
To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@...>; lersek@...; spbrogan@...;
ardb@...
Cc: 'Peter Grehan' <grehan@...>; 'Ard Biesheuvel' <ardb+tianocore@...>; Justen, Jordan L
<jordan.l.justen@...>; 'Sean Brogan' <sean.brogan@...>; 'Rebecca Cran' <rebecca@...>
Subject: 回复: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Mike:
If 'auto-rebase' label is not set, the behavior is still same to current one. Right?
If yes, I agree this proposal. The maintainer can optionally set 'auto-rebase'.

Thanks
Liming
-----邮件原件-----
发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Michael D
Kinney
发送时间: 2021年6月24日 6:08
收件人: devel@edk2.groups.io; lersek@...; spbrogan@...;
ardb@...; Kinney, Michael D <michael.d.kinney@...>
抄送: Peter Grehan <grehan@...>; Ard Biesheuvel
<ardb+tianocore@...>; Justen, Jordan L <jordan.l.justen@...>;
Sean Brogan <sean.brogan@...>; Rebecca Cran
<rebecca@...>
主题: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
remnants

Hi Laszlo,

I understand your point.

I am trying to balance the ease of use for developers, reducing overhead for
maintainers, and
prevent bad commits.

I think you are saying that you want to make sure a maintainer carefully
reviews changes
across multiple PRs that are in the same area of code. The CI checks will of
course make
sure the code builds and passes the basic boot tests, but those tests do not
have full
coverage so an interaction issue between two PRs that pass build and boot
but have
unintended behavior side effects are what require detailed manual review.

I am going to remove the auto-rebase by default and add a optional label that
can
be set by a maintainer to enable auto-rebase. If a maintainer is confident
that
a set of PRs being submitted at the same time with the 'push' label are
independent,
then the maintainer can also set 'auto-rebase'. If they are not confident,
then
they can send PRs one at a time with only 'push' label and manually rebase
each
additional PR and review the manual rebase to make sure there are no
unintended
side effects.

Any objections to this direction?

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Wednesday, June 23, 2021 12:45 PM
To: Kinney, Michael D <michael.d.kinney@...>;
devel@edk2.groups.io; spbrogan@...; ardb@...
Cc: Peter Grehan <grehan@...>; Ard Biesheuvel
<ardb+tianocore@...>; Justen, Jordan L
<jordan.l.justen@...>; Sean Brogan <sean.brogan@...>;
Rebecca Cran <rebecca@...>
Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
remnants

On 06/23/21 20:44, Kinney, Michael D wrote:

Hi Laszlo,

Thank you for the test case.

I created 2 PRs against edk2-codereview using your patches.
I made minor update to commit messages to pass patch check.

https://github.com/tianocore/edk2-codereview/pull/18
https://github.com/tianocore/edk2-codereview/pull/19

Found another issue with PatchCheck for the Mergify merge commit and
fixed that.

Mergify did process #18 and merged it in after passing all CI. Mergify
rebased #19 successfully and merged it after passing all CI. I do not
think this was your expected result.
Indeed, my "desired" result at least would have been for mergify to
reject the rebase.

I looked more closely at the patches you provided. They were not
overlapping in the lines of Readme.rst. This is why no merge conflict
was detected.
More precisely, a contextual conflict *was* determined between the
patches, but that conflict was auto-resolved.

This is risky when done in an automated fashion. It is an extremely
convenient feature of git, when used interactively; that is, when the
auto-merge (automatic conflict resolution) is semantically verified by a
human. Git takes away the chore of conflict resolution, presents a
"likely good" end result, and a human only needs to *look* at the end
result, not *implement* it.

But that "human look" is exactly what's missing from mergify.

Basically what I'd like for mergify is to turn off automatic conflict
resolution.

More or less, speaking in terms of the stand-alone "patch" utility
<https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
to set the "fuzz factor" to zero.


One way a human reviews such context differences is with git-range-diff.
Continuing my previous example commands:

$ git range-diff --color master..b2 b1..b2-rebase

1: 02dc81e58bd6 ! 1: 2cf39d4b1790 world
@@ -6,8 +6,8 @@
--- a/ReadMe.rst
+++ b/ReadMe.rst
@@
-
A modern, feature-rich, cross-platform firmware development
+ HELLO
environment for the UEFI and PI specifications from www.uefi.org.
+ WORLD

This output shows that the "world" addition is the same (it is identical
between pre-rebase and post-rebase in the commit), but the context has
changed. During the rebase, the leading empty line of the context
disappeared, and a HELLO line in the middle of the leading context
appeared.

This result may or may not be semantically correct; it needs a human
decision. What if the original purpose of the "world" patch author was
to say WORLD but only without HELLO? When they looked at the code,
there
was no HELLO yet.

git-range-diff is very powerful, but reading its output takes some
getting used to. (Colorization with the "--color" option is basically
required for understanding; I can't reproduce it in this email, alas.)

I don't want to obsess about this forever, I just want us all to be
aware that this risk exists.

Thanks,
Laszlo


I then created 2 new PRs that added text to the same line # in
Readme.rst.

https://github.com/tianocore/edk2-codereview/pull/21
https://github.com/tianocore/edk2-codereview/pull/22

PR #21 passed all CI tests and was merged. Mergify then attempted to
rebase #22 and got a merge conflict and is still in the open state waiting
for the developer to manually handle the merge conflict.
This case is not worrisome; when there is a clear conflict that cannot be
auto-resolved, I'm not concerned.

My concern is the sneaky contextual conflict that *appears* auto-resolvable,
but is semantically broken. Those things
exist.

Thanks
Laszlo













回复: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

gaoliming
 

Mike:
If 'auto-rebase' label is not set, the behavior is still same to current one. Right?
If yes, I agree this proposal. The maintainer can optionally set 'auto-rebase'.

Thanks
Liming

-----邮件原件-----
发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Michael D
Kinney
发送时间: 2021年6月24日 6:08
收件人: devel@edk2.groups.io; lersek@...; spbrogan@...;
ardb@...; Kinney, Michael D <michael.d.kinney@...>
抄送: Peter Grehan <grehan@...>; Ard Biesheuvel
<ardb+tianocore@...>; Justen, Jordan L <jordan.l.justen@...>;
Sean Brogan <sean.brogan@...>; Rebecca Cran
<rebecca@...>
主题: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
remnants

Hi Laszlo,

I understand your point.

I am trying to balance the ease of use for developers, reducing overhead for
maintainers, and
prevent bad commits.

I think you are saying that you want to make sure a maintainer carefully
reviews changes
across multiple PRs that are in the same area of code. The CI checks will of
course make
sure the code builds and passes the basic boot tests, but those tests do not
have full
coverage so an interaction issue between two PRs that pass build and boot
but have
unintended behavior side effects are what require detailed manual review.

I am going to remove the auto-rebase by default and add a optional label that
can
be set by a maintainer to enable auto-rebase. If a maintainer is confident
that
a set of PRs being submitted at the same time with the 'push' label are
independent,
then the maintainer can also set 'auto-rebase'. If they are not confident,
then
they can send PRs one at a time with only 'push' label and manually rebase
each
additional PR and review the manual rebase to make sure there are no
unintended
side effects.

Any objections to this direction?

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Wednesday, June 23, 2021 12:45 PM
To: Kinney, Michael D <michael.d.kinney@...>;
devel@edk2.groups.io; spbrogan@...; ardb@...
Cc: Peter Grehan <grehan@...>; Ard Biesheuvel
<ardb+tianocore@...>; Justen, Jordan L
<jordan.l.justen@...>; Sean Brogan <sean.brogan@...>;
Rebecca Cran <rebecca@...>
Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
remnants

On 06/23/21 20:44, Kinney, Michael D wrote:

Hi Laszlo,

Thank you for the test case.

I created 2 PRs against edk2-codereview using your patches.
I made minor update to commit messages to pass patch check.

https://github.com/tianocore/edk2-codereview/pull/18
https://github.com/tianocore/edk2-codereview/pull/19

Found another issue with PatchCheck for the Mergify merge commit and
fixed that.

Mergify did process #18 and merged it in after passing all CI. Mergify
rebased #19 successfully and merged it after passing all CI. I do not
think this was your expected result.
Indeed, my "desired" result at least would have been for mergify to
reject the rebase.

I looked more closely at the patches you provided. They were not
overlapping in the lines of Readme.rst. This is why no merge conflict
was detected.
More precisely, a contextual conflict *was* determined between the
patches, but that conflict was auto-resolved.

This is risky when done in an automated fashion. It is an extremely
convenient feature of git, when used interactively; that is, when the
auto-merge (automatic conflict resolution) is semantically verified by a
human. Git takes away the chore of conflict resolution, presents a
"likely good" end result, and a human only needs to *look* at the end
result, not *implement* it.

But that "human look" is exactly what's missing from mergify.

Basically what I'd like for mergify is to turn off automatic conflict
resolution.

More or less, speaking in terms of the stand-alone "patch" utility
<https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
to set the "fuzz factor" to zero.


One way a human reviews such context differences is with git-range-diff.
Continuing my previous example commands:

$ git range-diff --color master..b2 b1..b2-rebase

1: 02dc81e58bd6 ! 1: 2cf39d4b1790 world
@@ -6,8 +6,8 @@
--- a/ReadMe.rst
+++ b/ReadMe.rst
@@
-
A modern, feature-rich, cross-platform firmware development
+ HELLO
environment for the UEFI and PI specifications from www.uefi.org.
+ WORLD

This output shows that the "world" addition is the same (it is identical
between pre-rebase and post-rebase in the commit), but the context has
changed. During the rebase, the leading empty line of the context
disappeared, and a HELLO line in the middle of the leading context
appeared.

This result may or may not be semantically correct; it needs a human
decision. What if the original purpose of the "world" patch author was
to say WORLD but only without HELLO? When they looked at the code,
there
was no HELLO yet.

git-range-diff is very powerful, but reading its output takes some
getting used to. (Colorization with the "--color" option is basically
required for understanding; I can't reproduce it in this email, alas.)

I don't want to obsess about this forever, I just want us all to be
aware that this risk exists.

Thanks,
Laszlo


I then created 2 new PRs that added text to the same line # in
Readme.rst.

https://github.com/tianocore/edk2-codereview/pull/21
https://github.com/tianocore/edk2-codereview/pull/22

PR #21 passed all CI tests and was merged. Mergify then attempted to
rebase #22 and got a merge conflict and is still in the open state waiting
for the developer to manually handle the merge conflict.
This case is not worrisome; when there is a clear conflict that cannot be
auto-resolved, I'm not concerned.

My concern is the sneaky contextual conflict that *appears* auto-resolvable,
but is semantically broken. Those things
exist.

Thanks
Laszlo








回复: [edk2-devel] [PATCH v3 8/8] MdeModulePkg: Use SecureBootVariableLib in PlatformVarCleanupLib.

gaoliming
 

I agree that AuthVariableLib can support UEFI_DRIVER, DXE_DRIVER and UEFI_APPLICATION. There is no limitation for this library.

Thanks
Liming

-----邮件原件-----
发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Grzegorz
Bernacki
发送时间: 2021年6月23日 18:39
收件人: devel@edk2.groups.io; Sunny Wang <Sunny.Wang@...>
抄送: gaoliming@...; leif@...;
ardb+tianocore@...; Samer El-Haj-Mahmoud
<Samer.El-Haj-Mahmoud@...>; Marcin Wojtas <mw@...>;
upstream@...; Yao, Jiewen <jiewen.yao@...>; Wang, Jian J
<jian.j.wang@...>; Xu, Min M <min.m.xu@...>; Laszlo Ersek
<lersek@...>; Sami Mujawar <Sami.Mujawar@...>;
afish@...; ray.ni@...; jordan.l.justen@...;
rebecca@...; grehan@...; Thomas Abraham
<thomas.abraham@...>; chasel.chiu@...;
nathaniel.l.desimone@...; eric.dong@...;
michael.d.kinney@...; zailiang.sun@...; yi.qian@...;
graeme@...; Radosław Biernacki <rad@...>; Pete
Batard <pete@...>
主题: Re: [edk2-devel] [PATCH v3 8/8] MdeModulePkg: Use
SecureBootVariableLib in PlatformVarCleanupLib.

Hi,

AuthVariableLib must support DXE_DRIVER and UEFI_APPLICATION. Adding
it leads to errors for libraries which are used by AuthVariableLib and
which does not support DXE_DRIVER or UEFI_APPLICATION. This changes
causes a lot of minor changes in another libraries.
The whole problem exists because I wanted to remove a duplicate of
CreateTimeBasedPayload() from PlatformVarCleanupLib. I just leave it
there. This module is not used anyway. I think it can be safely
removed, but I don't want this change to be a part of this patchset.
thanks,
greg

śr., 23 cze 2021 o 05:33 Sunny Wang <Sunny.Wang@...> napisał(a):

Hi Greg,
Why can't we make AuthVariableLib support DXE_DRIVER?

Best Regards,
Sunny Wang

-----Original Message-----
From: Grzegorz Bernacki <gjb@...>
Sent: Monday, June 21, 2021 5:59 PM
To: devel@edk2.groups.io; Grzegorz Bernacki <gjb@...>
Cc: gaoliming@...; leif@...;
ardb+tianocore@...; Samer El-Haj-Mahmoud
<Samer.El-Haj-Mahmoud@...>; Sunny Wang <Sunny.Wang@...>;
Marcin Wojtas <mw@...>; upstream@...; Yao, Jiewen
<jiewen.yao@...>; Wang, Jian J <jian.j.wang@...>; Xu, Min M
<min.m.xu@...>; Laszlo Ersek <lersek@...>; Sami Mujawar
<Sami.Mujawar@...>; afish@...; ray.ni@...;
jordan.l.justen@...; rebecca@...; grehan@...;
Thomas Abraham <thomas.abraham@...>; chasel.chiu@...;
nathaniel.l.desimone@...; eric.dong@...;
michael.d.kinney@...; zailiang.sun@...; yi.qian@...;
graeme@...; Radosław Biernacki <rad@...>; Pete
Batard <pete@...>
Subject: Re: [edk2-devel] [PATCH v3 8/8] MdeModulePkg: Use
SecureBootVariableLib in PlatformVarCleanupLib.

Hi,

I moved CreateTimeBasedPayload() to AuthVariableLib, but then I cannot
use it in SecureBootConfigDxe, cause AuthVariableLib does not support
DXE_DRIVER.
So:
- having that function only in AuthVariableLib does not work
- having that function only in SecureBootVariableLib, causes a lot of
changes in platform DSCs files and also causes MdeModulePkg to be
depended on SecurityPkg

Right now I tend to roll back the changes related to
CreateTimeBasedPayload() and just let the modules to have its own copy
of that function. What do you think?
thanks,
greg

pt., 18 cze 2021 o 10:03 Grzegorz Bernacki via groups.io
<gjb@...> napisał(a):

Hi,

Thanks for the comment, I will move that function to AuthVariableLib.
greg

czw., 17 cze 2021 o 04:35 gaoliming <gaoliming@...>
napisał(a):

Grzegorz:
MdeModulePkg is generic base package. It should not depend on
SecurityPkg.

I agree CreateTimeBasedPayload() is the generic API. It can be
shared in
the different modules.
I propose to add it into MdeModulePkg AuthVariableLib.

Thanks
Liming
-----邮件原件-----
发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表
Grzegorz
Bernacki
发送时间: 2021年6月14日 17:43
收件人: devel@edk2.groups.io
抄送: leif@...; ardb+tianocore@...;
Samer.El-Haj-Mahmoud@...; sunny.Wang@...;
mw@...; upstream@...; jiewen.yao@...;
jian.j.wang@...; min.m.xu@...; lersek@...;
sami.mujawar@...; afish@...; ray.ni@...;
jordan.l.justen@...; rebecca@...; grehan@...;
thomas.abraham@...; chasel.chiu@...;
nathaniel.l.desimone@...; gaoliming@...;
eric.dong@...; michael.d.kinney@...;
zailiang.sun@...;
yi.qian@...; graeme@...; rad@...;
pete@...;
Grzegorz Bernacki <gjb@...>
主题: [edk2-devel] [PATCH v3 8/8] MdeModulePkg: Use
SecureBootVariableLib in PlatformVarCleanupLib.

This commits removes CreateTimeBasedPayload() function from
PlatformVarCleanupLib and uses exactly the same function from
SecureBootVariableLib.

Signed-off-by: Grzegorz Bernacki <gjb@...>
---
MdeModulePkg/Library/PlatformVarCleanupLib/PlatformVarCleanupLib.inf |
2 +
MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanup.h
| 1 +
MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
| 84 --------------------
3 files changed, 3 insertions(+), 84 deletions(-)

diff --git
a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatformVarCleanupLib.inf
b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatformVarCleanupLib.inf
index 8d5db826a0..493d03e1d8 100644
---
a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatformVarCleanupLib.inf
+++
b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatformVarCleanupLib.inf
@@ -34,6 +34,7 @@
[Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
+ SecurityPkg/SecurityPkg.dec

[LibraryClasses]
UefiBootServicesTableLib
@@ -44,6 +45,7 @@
PrintLib
MemoryAllocationLib
HiiLib
+ SecureBootVariableLib

[Guids]
gEfiIfrTianoGuid ##
SOMETIMES_PRODUCES ##
GUID
diff --git
a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanup.h
b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanup.h
index c809a7086b..94fbc7d2a4 100644
---
a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanup.h
+++
b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanup.h
@@ -18,6 +18,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/MemoryAllocationLib.h>
#include <Library/HiiLib.h>
#include <Library/PlatformVarCleanupLib.h>
+#include <Library/SecureBootVariableLib.h>

#include <Protocol/Variable.h>
#include <Protocol/VarCheck.h>
diff --git
a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
index 3875d614bb..204f1e00ad 100644
---
a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
+++
b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
@@ -319,90 +319,6 @@ DestroyUserVariableNode (
}
}

-/**
- Create a time based data payload by concatenating the
EFI_VARIABLE_AUTHENTICATION_2
- descriptor with the input data. NO authentication is required in this
function.
-
- @param[in, out] DataSize On input, the size of Data
buffer in
bytes.
- On output, the size of
data
returned in Data
- buffer in bytes.
- @param[in, out] Data On input, Pointer to data
buffer to
be wrapped or
- pointer to NULL to wrap
an
empty payload.
- On output, Pointer to
the new
payload date buffer allocated from pool,
- it's caller's responsibility
to free
the memory after using it.
-
- @retval EFI_SUCCESS Create time based payload
successfully.
- @retval EFI_OUT_OF_RESOURCES There are not enough
memory
resourses to create time based payload.
- @retval EFI_INVALID_PARAMETER The parameter is invalid.
- @retval Others Unexpected error
happens.
-
-**/
-EFI_STATUS
-CreateTimeBasedPayload (
- IN OUT UINTN *DataSize,
- IN OUT UINT8 **Data
- )
-{
- EFI_STATUS Status;
- UINT8 *NewData;
- UINT8 *Payload;
- UINTN PayloadSize;
- EFI_VARIABLE_AUTHENTICATION_2 *DescriptorData;
- UINTN DescriptorSize;
- EFI_TIME Time;
-
- if (Data == NULL || DataSize == NULL) {
- return EFI_INVALID_PARAMETER;
- }
-
- //
- // At user physical presence, the variable does not need to be
signed
but
the
- // parameters to the SetVariable() call still need to be prepared as
authenticated
- // variable. So we create EFI_VARIABLE_AUTHENTICATED_2
descriptor
without certificate
- // data in it.
- //
- Payload = *Data;
- PayloadSize = *DataSize;
-
- DescriptorSize = OFFSET_OF (EFI_VARIABLE_AUTHENTICATION_2,
AuthInfo) + OFFSET_OF (WIN_CERTIFICATE_UEFI_GUID, CertData);
- NewData = (UINT8 *) AllocateZeroPool (DescriptorSize +
PayloadSize);
- if (NewData == NULL) {
- return EFI_OUT_OF_RESOURCES;
- }
-
- if ((Payload != NULL) && (PayloadSize != 0)) {
- CopyMem (NewData + DescriptorSize, Payload, PayloadSize);
- }
-
- DescriptorData = (EFI_VARIABLE_AUTHENTICATION_2 *)
(NewData);
-
- ZeroMem (&Time, sizeof (EFI_TIME));
- Status = gRT->GetTime (&Time, NULL);
- if (EFI_ERROR (Status)) {
- FreePool (NewData);
- return Status;
- }
- Time.Pad1 = 0;
- Time.Nanosecond = 0;
- Time.TimeZone = 0;
- Time.Daylight = 0;
- Time.Pad2 = 0;
- CopyMem (&DescriptorData->TimeStamp, &Time, sizeof
(EFI_TIME));
-
- DescriptorData->AuthInfo.Hdr.dwLength = OFFSET_OF
(WIN_CERTIFICATE_UEFI_GUID, CertData);
- DescriptorData->AuthInfo.Hdr.wRevision = 0x0200;
- DescriptorData->AuthInfo.Hdr.wCertificateType =
WIN_CERT_TYPE_EFI_GUID;
- CopyGuid (&DescriptorData->AuthInfo.CertType,
&gEfiCertPkcs7Guid);
-
- if (Payload != NULL) {
- FreePool (Payload);
- }
-
- *DataSize = DescriptorSize + PayloadSize;
- *Data = NewData;
- return EFI_SUCCESS;
-}
-
/**
Create a counter based data payload by concatenating the
EFI_VARIABLE_AUTHENTICATION
descriptor with the input data. NO authentication is required in
this
function.
--
2.25.1













IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.







Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

Min Xu
 

On 06/24/2021 8:36 AM, James Bottomley wrote:
On Thu, 2021-06-24 at 00:24 +0000, Min Xu wrote:
On 06/22/2021 9:39 PM, Laszlo wrote:
I should clarify: the relevant part of my preference is not that
"IntelTdx.dsc"
contain the *complete* TDVF feature set. The relevant part (for me)
is that "OvmfPkgX64.dsc" *not* be over-complicated for the sake of
TDX, even considering only the "basic" TDVF feature set. It's fine
to implement TDX in two stages ("basic" and "complete"); my point is
that even "basic"
should not over-
complicate "OvmfPkgX64.dsc".
Thanks much for the comments and we don't want to make OvmfPkgX64.dsc
over-complicated either.
We have updated the design slides to V0.95 and Slides 6-15 are
discussing the Config-A and Config-B.
https://edk2.groups.io/g/devel/files/Designs/2021/0611/TDVF_Design_Rev
iew%28v0.95%29.pptx
Your comment is always welcome!
The mailing list still won't give me that file, can you update it in the bugzilla:

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

As well, please?
Sure. TDVF Design Review v0.95 is uploaded to
https://bugzilla.tianocore.org/show_bug.cgi?id=3429
Thanks
Min


Re: 回复: [edk2-devel] [PATCH v1 1/2] MdePkg: MmConfiguration: Moved EFI_MM_RESERVED_MMRAM_REGION to PiMmCis.h

Kun Qin
 

Hi Liming,

Thanks for pointing it out. I will updated the patches shortly.

Regards,
Kun

On 06/20/2021 18:22, gaoliming wrote:
Kun:
There is one header file edk2\MdePkg\Include\PiMultiPhase.h for this
purpose. Can you place the common definition to it?
Thanks
Liming
-----邮件原件-----
发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Kun Qin
发送时间: 2021年6月18日 17:48
收件人: devel@edk2.groups.io
抄送: Michael D Kinney <michael.d.kinney@...>; Liming Gao
<gaoliming@...>; Zhiguang Liu <zhiguang.liu@...>;
Michael Kubacki <michael.kubacki@...>
主题: [edk2-devel] [PATCH v1 1/2] MdePkg: MmConfiguration: Moved
EFI_MM_RESERVED_MMRAM_REGION to PiMmCis.h

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3440

The definition of EFI_MM_RESERVED_MMRAM_REGION, according to PI Spec
1.5
is also referenced in EFI_PEI_MM_CONFIGURATION_PPI. Defining this
structure as is will enforce any potential usage of MM Configuration PPI
interface to include <Protocol/MmConfiguration.h>.

This change moves EFI_MM_RESERVED_MMRAM_REGION definition into
PiMmCis.h,
which is already included in Protocol/MmConfiguration.h. It also paves
way for introducing Ppi/MmConfiguration.h with proper dependency.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Cc: Michael Kubacki <michael.kubacki@...>

Signed-off-by: Kun Qin <kuqin12@...>
---
MdePkg/Include/Pi/PiMmCis.h | 16 ++++++++++++++++
MdePkg/Include/Protocol/MmConfiguration.h | 16 ----------------
2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/MdePkg/Include/Pi/PiMmCis.h b/MdePkg/Include/Pi/PiMmCis.h
index fdf0591a03d6..422a3ea6c2bb 100644
--- a/MdePkg/Include/Pi/PiMmCis.h
+++ b/MdePkg/Include/Pi/PiMmCis.h
@@ -242,6 +242,22 @@ VOID
IN CONST EFI_MM_ENTRY_CONTEXT *MmEntryContext
);

+///
+/// Structure describing a MMRAM region which cannot be used for the
MMRAM heap.
+///
+typedef struct _EFI_MM_RESERVED_MMRAM_REGION {
+ ///
+ /// Starting address of the reserved MMRAM area, as it appears while
MMRAM is open.
+ /// Ignored if MmramReservedSize is 0.
+ ///
+ EFI_PHYSICAL_ADDRESS MmramReservedStart;
+ ///
+ /// Number of bytes occupied by the reserved MMRAM area. A size of
zero indicates the
+ /// last MMRAM area.
+ ///
+ UINT64 MmramReservedSize;
+} EFI_MM_RESERVED_MMRAM_REGION;
+
///
/// Management Mode System Table (MMST)
///
diff --git a/MdePkg/Include/Protocol/MmConfiguration.h
b/MdePkg/Include/Protocol/MmConfiguration.h
index eeb94f64bdf7..d2fb6a13d4af 100644
--- a/MdePkg/Include/Protocol/MmConfiguration.h
+++ b/MdePkg/Include/Protocol/MmConfiguration.h
@@ -21,22 +21,6 @@
0x26eeb3de, 0xb689, 0x492e, {0x80, 0xf0, 0xbe, 0x8b, 0xd7, 0xda,
0x4b,
0xa7 } \
}

-///
-/// Structure describing a MMRAM region which cannot be used for the
MMRAM heap.
-///
-typedef struct _EFI_MM_RESERVED_MMRAM_REGION {
- ///
- /// Starting address of the reserved MMRAM area, as it appears while
MMRAM is open.
- /// Ignored if MmramReservedSize is 0.
- ///
- EFI_PHYSICAL_ADDRESS MmramReservedStart;
- ///
- /// Number of bytes occupied by the reserved MMRAM area. A size of
zero indicates the
- /// last MMRAM area.
- ///
- UINT64 MmramReservedSize;
-} EFI_MM_RESERVED_MMRAM_REGION;
-
typedef struct _EFI_MM_CONFIGURATION_PROTOCOL
EFI_MM_CONFIGURATION_PROTOCOL;

/**
--
2.31.1.windows.1




回复: [edk2-devel] 回复: [edk2][PATCH V3] MdePkg : Add IPMI Macro and Structure Defintions to resolve build errors

gaoliming
 

AbduL:

 This is same to the definition of UINT8 CompletionCode without bitfield.

 

Thanks

Liming

发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 AbduL Lateef
发送时间: 2021623 19:43
收件人: devel@edk2.groups.io; gaoliming@...
抄送: manickavasakam karpagavinayagam <manickavasakamk@...>; isaac.w.oram@...; nathaniel.l.desimone@...; Felixp@...; Harikrishnad@...; manishj@...; zacharyb@...
主题: Re: [edk2-devel] 回复: [edk2][PATCH V3] MdePkg : Add IPMI Macro and Structure Defintions to resolve build errors

 

whats the use of bitfield:8 for UINT8 variable type?

+typedef struct {
+ UINT8 CompletionCode:8;
+} IPMI_SET_BOOT_OPTIONS_RESPONSE;

 

Thanks

AbduL

 

On Mon, Jun 21, 2021 at 7:19 AM gaoliming <gaoliming@...> wrote:

Thanks for you update.

Reviewed-by: Liming Gao <gaoliming@...>

Thanks
Liming
> -----
邮件原件-----
>
发件人: manickavasakam karpagavinayagam <manickavasakamk@...>
>
发送时间: 2021618 23:38
>
收件人: devel@edk2.groups.io
>
抄送: isaac.w.oram@...; nathaniel.l.desimone@...;
> Felixp@...; Harikrishnad@...; manishj@...;
> zacharyb@...; manickavasakamk@...;
> gaoliming@...
>
主题: [edk2][PATCH V3] MdePkg : Add IPMI Macro and Structure Defintions
> to resolve build errors
>
> Build error reported for missing structures
> IPMI_SET_BOOT_OPTIONS_RESPONSE,
> EFI_IPMI_MSG_GET_BMC_EXEC_RSP and
> macros EFI_FIRMWARE_GET_BMC_EXECUTION_CONTEXT
> EFI_FIRMWARE_BMC_IN_FULL_RUNTIME/EFI_FIRMWARE_BMC_IN_FORCE
> D_UPDATE_MODE
> when using
> edk2-platforms\Features\Intel\OutOfBandManagement\IpmiFeaturePkg
>
> MdePkg : Rename IPMI Macro and Structure Defintions
>
> Rename the EFI_IPMI_MSG_GET_BMC_EXEC_RSPB,
> EFI_FIRMWARE_GET_BMC_EXECUTION_CONTEXT
> EFI_FIRMWARE_BMC_IN_FORCED_UPDATE_MODE to
> IPMI_MSG_GET_BMC_EXEC_RSPB,IPMI_GET_BMC_EXECUTION_CONTEXT
> IPMI_BMC_IN_FORCED_UPDATE_MODE
>
> Notes:
> V1 :
> Rename the EFI_IPMI_MSG_GET_BMC_EXEC_RSPB,
> EFI_FIRMWARE_GET_BMC_EXECUTION_CONTEXT
> EFI_FIRMWARE_BMC_IN_FORCED_UPDATE_MODE to
> IPMI_MSG_GET_BMC_EXEC_RSPB,IPMI_GET_BMC_EXECUTION_CONTEXT
> IPMI_BMC_IN_FORCED_UPDATE_MODE
>
> V2:
>
> Remove 0001-MdePkg-Add-IPMI-Macro-and-Structure-Defintions-to-re.patch
>
> V3:
>
> Add Signed-off-by information
>
> Signed-off-by: Manickavasakam Karpagavinayagam
> <manickavasakamk@...>
> ---
>  .../IndustryStandard/IpmiNetFnChassis.h        |  4 ++++
>  .../IndustryStandard/IpmiNetFnFirmware.h       | 18
> ++++++++++++++++++
>  2 files changed, 22 insertions(+)
>
> diff --git a/MdePkg/Include/IndustryStandard/IpmiNetFnChassis.h
> b/MdePkg/Include/IndustryStandard/IpmiNetFnChassis.h
> index 79db55523d..d7cdd3a865 100644
> --- a/MdePkg/Include/IndustryStandard/IpmiNetFnChassis.h
> +++ b/MdePkg/Include/IndustryStandard/IpmiNetFnChassis.h
> @@ -186,6 +186,10 @@ typedef struct {
>    UINT8                                  ParameterData[0];
>
>  } IPMI_SET_BOOT_OPTIONS_REQUEST;
>
>
>
> +typedef struct {
>
> +  UINT8   CompletionCode:8;
>
> +} IPMI_SET_BOOT_OPTIONS_RESPONSE;
>
> +
>
>  //
>
>  //  Definitions for Get System Boot options command
>
>  //
>
> diff --git a/MdePkg/Include/IndustryStandard/IpmiNetFnFirmware.h
> b/MdePkg/Include/IndustryStandard/IpmiNetFnFirmware.h
> index 2d892dbd5a..c4cbe2349b 100644
> --- a/MdePkg/Include/IndustryStandard/IpmiNetFnFirmware.h
> +++ b/MdePkg/Include/IndustryStandard/IpmiNetFnFirmware.h
> @@ -17,4 +17,22 @@
>  // All Firmware commands and their structure definitions to follow here
>
>  //
>
>
>
> +//
----------------------------------------------------------------------------
------------
>
> +//    Definitions for Get BMC Execution Context
>
> +//
----------------------------------------------------------------------------
------------
>
> +#define IPMI_GET_BMC_EXECUTION_CONTEXT  0x23
>
> +
>
> +//
>
> +//  Constants and Structure definitions for "Get Device ID" command to
> follow here
>
> +//
>
> +typedef struct {
>
> +  UINT8   CurrentExecutionContext;
>
> +  UINT8   PartitionPointer;
>
> +} IPMI_MSG_GET_BMC_EXEC_RSP;
>
> +
>
> +//
>
> +// Current Execution Context responses
>
> +//
>
> +#define IPMI_BMC_IN_FORCED_UPDATE_MODE  0x11
>
> +
>
>  #endif
>
> --
> 2.25.0.windows.1
>
>
> Please consider the environment before printing this email.
>
> The information contained in this message may be confidential and
> proprietary to American Megatrends (AMI).  This communication is intended
> to be read only by the individual or entity to whom it is addressed or by
their
> designee. If the reader of this message is not the intended recipient, you
are
> on notice that any distribution of this message, in any form, is strictly
> prohibited.  Please promptly notify the sender by reply e-mail or by
> telephone at 770-246-8600, and then delete or destroy all copies of the
> transmission.







 

--

Thanks and Regards
Abdul Lateef Attar
Bangalore


Re: [PATCH v1 1/5] EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update

Kun Qin
 

Hi Hao,

We have been circulating different ideas internally about how to enforce a warning around this change.

There is an idea of deprecating the old structure and replacing it with a newly defined EFI_MM_COMMUNICATE_HEADER_V2. That way all consumers will be enforced to update their implementation and (hopefully) inspect the compatibility accordingly. In addition, we could add other fields such as signature and/or header version number for further extensibility.

If there are code instances that uses "sizeof(EFI_GUID) + sizeof(UINTN)" in lieu of OFFSET_OF, this implementation is essentially decoupled from the structure definition. So compiler might not be able to help. In that case, runtime protections such as pool guard or stack guard might be useful.

Please let me know if you think header structure V2 is an idea worth to try.

Thanks,
Kun

On 06/15/2021 18:15, Wu, Hao A wrote:
Thanks Kun,
I am a little concerned on whether there will be other missing cases that are not covered by this patch series.
I am also wondering is there any detection can be made to warn the cases that code modification should be made after this structure update.
Otherwise, it will be the burden for the platform owners to review the platform codes following your guide mentioned in this patch.
Hoping others can provide some inputs on this.
Best Regards,
Hao Wu

-----Original Message-----
From: Kun Qin <kuqin12@...>
Sent: Wednesday, June 16, 2021 4:51 AM
To: Wu, Hao A <hao.a.wu@...>; devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@...>; Liming Gao
<gaoliming@...>; Liu, Zhiguang <zhiguang.liu@...>;
Andrew Fish <afish@...>; Laszlo Ersek <lersek@...>; Leif
Lindholm <leif@...>
Subject: Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification:
EFI_MM_COMMUNICATE_HEADER Update

Hi Hao,

Sorry that I missed comments for this patch earlier. You are correct. I only
inspected SmmLockBoxPeiLib. The PEI instance handled mode switch with
```OFFSET_OF ``` function. But DXE instance still have a few use cases that will
be impacted.

I will update this read me file and add a code change patch for this library in
v2. Thanks for pointing this out.

Regards,
Kun

On 06/11/2021 00:46, Wu, Hao A wrote:
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun
Qin
Sent: Thursday, June 10, 2021 9:43 AM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@...>; Liming Gao
<gaoliming@...>; Liu, Zhiguang <zhiguang.liu@...>;
Andrew Fish <afish@...>; Laszlo Ersek <lersek@...>; Leif
Lindholm <leif@...>
Subject: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification:
EFI_MM_COMMUNICATE_HEADER Update

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430

This change includes specification update markdown file that
describes the proposed PI Specification v1.7 Errata A in detail and
potential impact to the existing codebase.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Cc: Andrew Fish <afish@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Leif Lindholm <leif@...>

Signed-off-by: Kun Qin <kuqin12@...>
---
BZ3430-SpecChange.md | 88 ++++++++++++++++++++
1 file changed, 88 insertions(+)

diff --git a/BZ3430-SpecChange.md b/BZ3430-SpecChange.md new file
mode
100644 index 000000000000..33a1ffda447b
--- /dev/null
+++ b/BZ3430-SpecChange.md
@@ -0,0 +1,88 @@
+# Title: Change MessageLength Field of
EFI_MM_COMMUNICATE_HEADER
to
+UINT64
+
+## Status: Draft
+
+## Document: UEFI Platform Initialization Specification Version 1.7
+Errata A
+
+## License
+
+SPDX-License-Identifier: CC-BY-4.0
+
+## Submitter: [TianoCore Community](https://www.tianocore.org)
+
+## Summary of the change
+
+Change the `MessageLength` Field of
`EFI_MM_COMMUNICATE_HEADER`
from UINTN to UINT64 to remove architecture dependency:
+
+```c
+typedef struct {
+ EFI_GUID HeaderGuid;
+ UINT64 MessageLength;
+ UINT8 Data[ANYSIZE_ARRAY];
+} EFI_MM_COMMUNICATE_HEADER;
+```
+
+## Benefits of the change
+
+In PI Spec v1.7 Errata A, Vol.4, Sec 5.7 MM Communication Protocol,
+the
MessageLength field of `EFI_MM_COMMUNICATE_HEADER` (also
defined as
`EFI_SMM_COMMUNICATE_HEADER`) is defined as type UINTN.
+
+But this structure, as a generic definition, could be used for both
+PEI and
DXE MM communication. Thus for a system that supports PEI MM launch,
but operates PEI in 32bit mode and MM foundation in 64bit, the
current `EFI_MM_COMMUNICATE_HEADER` definition will cause
structure
parse error due to UINTN used.
+
+## Impact of the change
+
+This change will impact the known structure consumers including:
+
+```bash
+MdeModulePkg/Core/PiSmmCore/PiSmmIpl
+MdeModulePkg/Application/SmiHandlerProfileInfo
+MdeModulePkg/Application/MemoryProfileInfo
+```
+
+For consumers that are not using
`OFFSET_OF(EFI_MM_COMMUNICATE_HEADER, Data)`, but performing
explicit
addition such as the existing
MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.
c, one will need to change code implementation to match new structure
definition. Otherwise, the code compiled on IA32 architecture will
experience structure field dereference error.
+
+User who currently uses UINTN local variables as place holder of
MessageLength will need to use caution to make cast from UINTN to
UINT64 and vice versa. It is recommended to use `SafeUint64ToUintn`
for such operations when the value is indeterministic.
+
+Note: MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib is
also
consuming this structure, but it handled this size discrepancy
internally. If this

Hello Kun,

Sorry for a question.
I am not sure why the current codes in file SmmLockBoxDxeLib.c will work
properly after the UINTN -> UINT64 change.

For example:
LockBoxGetSmmCommBuffer():
MinimalSizeNeeded = sizeof (EFI_GUID) +
sizeof (UINTN) +
MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_SAVE),
MAX (sizeof
(EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES),
MAX (sizeof
(EFI_SMM_LOCK_BOX_PARAMETER_UPDATE),
MAX (sizeof
(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE),
sizeof
(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)))));

SaveLockBox():
UINT8 TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN)
+ sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)];

Is the series missed changes for SmmLockBoxDxeLib or I got something
wrong?

Best Regards,
Hao Wu


potential spec change is not applied, all applicable PEI MM
communicate callers will need to use the same routine as that of
SmmLockBoxPeiLib to invoke a properly populated
EFI_MM_COMMUNICATE_HEADER to be used in X64 MM foundation.
+
+## Detailed description of the change [normative updates]
+
+### Specification Changes
+
+1. In PI Specification v1.7 Errata A: Vol. 4 Page-91, the definition
+of
`EFI_MM_COMMUNICATE_HEADER` should be changed from current:
+
+```c
+typedef struct {
+ EFI_GUID HeaderGuid;
+ UINTN MessageLength;
+ UINT8 Data[ANYSIZE_ARRAY];
+} EFI_MM_COMMUNICATE_HEADER;
+```
+
+to:
+
+```c
+typedef struct {
+ EFI_GUID HeaderGuid;
+ UINT64 MessageLength;
+ UINT8 Data[ANYSIZE_ARRAY];
+} EFI_MM_COMMUNICATE_HEADER;
+```
+
+### Code Changes
+
+1. Remove the explicit calculation of the offset of `Data` in
`EFI_MM_COMMUNICATE_HEADER`. Thus applicable calculations of
`sizeof(EFI_GUID) + sizeof(UINTN)` should be replaced with
`OFFSET_OF(EFI_MM_COMMUNICATE_HEADER, Data)` or similar
alternatives.
These calculations are identified in:
+
+```bash
+MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.
c
+MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
+```
+
+1. Resolve potentially mismatched type between `UINTN` and `UINT64`.
This would occur when `MessageLength` or its derivitive are used for
local calculation with existing `UINTN` typed variables. Code change
regarding this perspective is per case evaluation: if the variables
involved are all deterministic values, and there is no overflow or
underflow risk, a cast operation (from `UINTN` to `UINT64`) can be
safely used. Otherwise, the calculation will be performed in `UINT64`
bitwidth and then convert to `UINTN` using `SafeUint64*` and
`SafeUint64ToUintn`, respectively. These operations are identified in:
+
+```bash
+MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.
c
+MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
+```
+
+1. After all above changes applied and specification updated,
`MdePkg/Include/Protocol/MmCommunication.h` will need to be
updated
to match new definition that includes the field type update.
--
2.31.1.windows.1




Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

James Bottomley
 

On Thu, 2021-06-24 at 00:24 +0000, Min Xu wrote:
On 06/22/2021 9:39 PM, Laszlo wrote:
I should clarify: the relevant part of my preference is not that
"IntelTdx.dsc"
contain the *complete* TDVF feature set. The relevant part (for me)
is that
"OvmfPkgX64.dsc" *not* be over-complicated for the sake of TDX,
even
considering only the "basic" TDVF feature set. It's fine to
implement TDX in two
stages ("basic" and "complete"); my point is that even "basic"
should not over-
complicate "OvmfPkgX64.dsc".
Thanks much for the comments and we don't want to make OvmfPkgX64.dsc
over-complicated either.
We have updated the design slides to V0.95 and Slides 6-15 are
discussing the
Config-A and Config-B.
https://edk2.groups.io/g/devel/files/Designs/2021/0611/TDVF_Design_Review%28v0.95%29.pptx
Your comment is always welcome!
The mailing list still won't give me that file, can you update it in
the bugzilla:

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

As well, please?

Thanks,

James


Re: SIMPLE_TEXT_OUTPUT_PROTOCOL and Video Resolution

David F.
 

It would be nice to just update the specs to say, when a text mode is
chosen the resolution shall be set to the lowest resolution that
supports the cols and rows of the given text mode. Then over time all
the systems will handle it.

On Wed, Jun 23, 2021 at 3:24 AM Laszlo Ersek <lersek@...> wrote:

On 06/17/21 01:22, Andrew Fish via groups.io wrote:


On Jun 16, 2021, at 2:45 PM, David F. <df7729@...> wrote:

Also, I found if there are 2 GOP handles and you change the mode of
one, the other one doesn't reflect the change (but still doesn't solve
anything with the original question), are you supposed to set the mode
on every handle to keep that part in sync?
A common implementation is to have the Conspliter [1] driver that produces virtual handles that aggregate how many actual devices you have and manages policy.

You should grab the protocols on the gST->ConsoleOutHandle as these are the Spec defined active console devices.


In terms on Simple Text Output Protocol on Graphics this is the default driver in edk2 [2]. These are the config knobs you can set from your DSC file to control defaults.

[Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdVideoHorizontalResolution ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdVideoVerticalResolution ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn ## SOMETIMES_CONSUMES
Note: If you have serial active that may mess with the Conspliter and force it to pick a lower resolution since it has to find the best match between the serial and graphics Simple Text In. So try the Graphics without the serial terminal connect to see if it does different stuff.
Regarding the serial terminal aspect, we discussed the following patch (maybe: workaround?) for that, many years ago, off-list:

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
index a98b690c8b95..ded5513c74a7 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
@@ -115,9 +115,44 @@ TERMINAL_DEV mTerminalDevTemplate = {
};

TERMINAL_CONSOLE_MODE_DATA mTerminalConsoleModeData[] = {
- {80, 25},
- {80, 50},
- {100, 31},
+ { 80, 25 }, // from graphics resolution 640 x 480
+ { 80, 50 }, // from graphics resolution 640 x 960
+ { 100, 25 }, // from graphics resolution 800 x 480
+ { 100, 31 }, // from graphics resolution 800 x 600
+ { 104, 32 }, // from graphics resolution 832 x 624
+ { 120, 33 }, // from graphics resolution 960 x 640
+ { 128, 31 }, // from graphics resolution 1024 x 600
+ { 128, 40 }, // from graphics resolution 1024 x 768
+ { 144, 45 }, // from graphics resolution 1152 x 864
+ { 144, 45 }, // from graphics resolution 1152 x 870
+ { 160, 37 }, // from graphics resolution 1280 x 720
+ { 160, 40 }, // from graphics resolution 1280 x 760
+ { 160, 40 }, // from graphics resolution 1280 x 768
+ { 160, 42 }, // from graphics resolution 1280 x 800
+ { 160, 50 }, // from graphics resolution 1280 x 960
+ { 160, 53 }, // from graphics resolution 1280 x 1024
+ { 170, 40 }, // from graphics resolution 1360 x 768
+ { 170, 40 }, // from graphics resolution 1366 x 768
+ { 175, 55 }, // from graphics resolution 1400 x 1050
+ { 180, 47 }, // from graphics resolution 1440 x 900
+ { 200, 47 }, // from graphics resolution 1600 x 900
+ { 200, 63 }, // from graphics resolution 1600 x 1200
+ { 210, 55 }, // from graphics resolution 1680 x 1050
+ { 240, 56 }, // from graphics resolution 1920 x 1080
+ { 240, 63 }, // from graphics resolution 1920 x 1200
+ { 240, 75 }, // from graphics resolution 1920 x 1440
+ { 250, 105 }, // from graphics resolution 2000 x 2000
+ { 256, 80 }, // from graphics resolution 2048 x 1536
+ { 256, 107 }, // from graphics resolution 2048 x 2048
+ { 320, 75 }, // from graphics resolution 2560 x 1440
+ { 320, 84 }, // from graphics resolution 2560 x 1600
+ { 320, 107 }, // from graphics resolution 2560 x 2048
+ { 350, 110 }, // from graphics resolution 2800 x 2100
+ { 400, 126 }, // from graphics resolution 3200 x 2400
+ { 480, 113 }, // from graphics resolution 3840 x 2160
+ { 512, 113 }, // from graphics resolution 4096 x 2160
+ { 960, 227 }, // from graphics resolution 7680 x 4320
+ { 1024, 227 }, // from graphics resolution 8192 x 4320
//
// New modes can be added here.
//

but the discussion didn't go anywhere over several months, so we've been carrying this patch downstream-only ever since.

Thanks
Laszlo



[1] https://github.com/tianocore/edk2/tree/master/MdeModulePkg/Universal/Console/ConSplitterDxe <https://github.com/tianocore/edk2/tree/master/MdeModulePkg/Universal/Console/ConSplitterDxe>

[2] https://github.com/tianocore/edk2/tree/master/MdeModulePkg/Universal/Console/GraphicsConsoleDxe <https://github.com/tianocore/edk2/tree/master/MdeModulePkg/Universal/Console/GraphicsConsoleDxe>


Thanks,

Andrew Fish

On Tue, Jun 15, 2021 at 11:33 PM David F. via groups.io
<df7729@...> wrote:

Hello,

I've found that most implementation of UEFI don't automatically change
the resolution when setting the mode with STOP (Simple Text Output
Protocol) . You can use GOP to change it after the mode but that
causes other problems. For example, using surface pro 7 in this case,
with 4K screen. The default text mode is 342x96 which puts it in
2736x1824 mode which you'd expect and the text is tiny. But now you
set the mode to 0 which is 80x25 and it actually sets the mode to
2736x1824 if not already in that resolution and uses a 80x25 area in
the center of the screen, still tiny text you can hardly read. If you
then say you want GOP in 640x480 mode (which is available as GOP mode
1 on this system, it will make the font larger but you can't see
anything because it's still offset to the middle of the 2736x1824 area
and you're only seeing the 640x480 upper left of that area on the
screen. Likewise if you have it in 342x96 so it's fully in the upper
left corner of the screen and change the mode to say 800x600
(available as GOP mode 2 on this system) it will make the text
readable but the text can go off the screen in both directions because
it's still 342x96 when the 100x31 STOP mode would be the correct one
(which happens to be mode 2 on this system).

Shouldn't setting the STOP mode handle adjusting the resolution since
that's the main reason you want to change the mode so the size shown
on the screen changes to something you can read.

Any tricks? I've tried a bunch of things, resetting the controller,
using the Reset() protocol function, and other things but nothing
works. As soon as you use STOP to set the mode, it is back to high
resolution and using an area centered in the screen and changing the
resolution after that leaves it in the area centered in the high res
screen and not in the upper left area.

Thanks.









Re: [PATCH v2 1/6] EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update

Kun Qin
 

I can place this file under "CodeFirst" folder on the next round of patch series.

It would be helpful to update the code-first process page so that others can be consistent on the process next time.

Thanks,
Kun

On 06/23/2021 03:02, Laszlo Ersek wrote:
On 06/18/21 11:02, Kun Qin wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430

This change includes specification update markdown file that describes
the proposed PI Specification v1.7 Errata A in detail and potential
impact to the existing codebase.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Cc: Andrew Fish <afish@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Leif Lindholm <leif@...>
Cc: Hao A Wu <hao.a.wu@...>

Signed-off-by: Kun Qin <kuqin12@...>
---

Notes:
v2:
- Updated change impact analysis regarding SmmLockBoxDxeLib [Hao]

BZ3430-SpecChange.md | 90 ++++++++++++++++++++
1 file changed, 90 insertions(+)
Placing such a document in the edk2 root directory looks very strange to me.
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-First-Process#tianocore-github
[...] Specification text changes are held within the affected source
repository, using the GitHub flavor of markdown, in a file (or split
across several files) with .md suffix [...]
The wiki does not seem to specify what directory should contain the spec
change document.
Should we create a "CodeFirst" top-level directory?
Thanks,
Laszlo


Re: [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

Kun Qin
 

Hi Marvin,

I would prefer not to rely on undefined behaviors from different compilers. Instead of using flexible arrays, is it better to remove the `Data` field, pack the structure and follow "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?

In that case, OFFSET_OF will be forced to change to sizeof, and read/write to `Data` will follow the range indicated by MessageLength. But yes, that will enforce developers to update their platform level implementations accordingly.

Regards,
Kun

On 06/23/2021 08:26, Laszlo Ersek wrote:
On 06/23/21 08:54, Marvin Häuser wrote:
On 22.06.21 17:34, Laszlo Ersek wrote:
On 06/18/21 11:37, Marvin Häuser wrote:
On 16.06.21 22:58, Kun Qin wrote:
On 06/16/2021 00:02, Marvin Häuser wrote:
2) Is it feasible yet with the current set of supported compilers to
support flexible arrays?
My impression is that flexible arrays are already supported (as seen
in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
Please correct me if I am wrong.

Would you mind letting me know why this is applicable here? We are
trying to seek ideas on how to catch developer mistakes caused by this
change. So any input is appreciated.
Huh, interesting. Last time I tried I was told about incompatibilities
with MSVC, but I know some have been dropped since then (2005 and 2008
if I recall correctly?), so that'd be great to allow globally.
I too am surprised to see
"UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
flexible array member is a C99 feature, and I didn't even know that we
disallowed it for the sake of particular VS toolchains -- I thought we
had a more general reason than just "not supported by VS versions X
and Y".

The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF()
macro definition for non-gcc / non-clang:

#define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))

borders on undefined behavior as far as I can tell, so its behavior is
totally up to the compiler. It works thus far okay on Visual Studio, but
I couldn't say if it extended correctly to flexible array members.
Yes, it's UB by the standard, but this is actually how MS implements
them (or used to anyway?). I don't see why it'd cause issues with
flexible arrays, as only the start of the array is relevant (which is
constant for all instances of the structure no matter the amount of
elements actually stored). Any specific concern? If so, they could be
addressed by appropriate STATIC_ASSERTs.
No specific concern; my point was that two aspects of the same "class"
of undefined behavior didn't need to be consistent with each other.
Thanks
Laszlo


Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

Min Xu
 

On 06/22/2021 9:39 PM, Laszlo wrote:

I should clarify: the relevant part of my preference is not that "IntelTdx.dsc"
contain the *complete* TDVF feature set. The relevant part (for me) is that
"OvmfPkgX64.dsc" *not* be over-complicated for the sake of TDX, even
considering only the "basic" TDVF feature set. It's fine to implement TDX in two
stages ("basic" and "complete"); my point is that even "basic" should not over-
complicate "OvmfPkgX64.dsc".
Thanks much for the comments and we don't want to make OvmfPkgX64.dsc
over-complicated either.
We have updated the design slides to V0.95 and Slides 6-15 are discussing the
Config-A and Config-B.
https://edk2.groups.io/g/devel/files/Designs/2021/0611/TDVF_Design_Review%28v0.95%29.pptx
Your comment is always welcome!

Thanks!
Min

19681 - 19700 of 96641