Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro

[Vitaly Cheptsov via Groups.Io]

Laszlo,

I have already mentioned that the documentation is sufficient as _Static_assert 
is C standard, so I do not plan to make a V3 for this patch. The patch is merge 
ready.

As for usage examples I have an opposing opinion to yours and believe it is 
based on very good reasons. Not using STATIC_ASSERT in the current release will 
make the feature optionally available and let people test it in their setups. 
In case they notice it does not work for them they will have 3 months grace 
period to report it to us and consider making a change. This will also give 
them 3 months grace period of VERIFY_SIZE_OF macro removal in favour of 
STATIC_ASSERT. Making the change now will let people do seamless transition to 
the new feature and will avoid obstacles you are currently trying to create. 
Thus STATIC_ASSERT usage and VERIFY_SIZE_OF removal must both be separate 
patchsets with potentially separate BZs.

Thanks for understanding,
Vitaly

> 16 авг. 2019 г., в 19:33, Laszlo Ersek  написал(а):
> 
> 
> On 08/15/19 03:59, Gao, Liming wrote:
>> Vitaly:
>>  As you know, edk2 201908 stable tag will start soft freeze tomorrow. Dose 
>> this patch plan to catch this stable tag?
>> If yes, please ask the feedback from Tianocore Stewards. I have cc this 
>> patch to all Stewards.
> 
> If a feature patch (or series) is fully reviewed before the soft feature
> freeze (by the respective package maintainers), it can be merged during
> the soft feature freeze.
> 
> However, I don't think this patch is mature enough for that. As I've
> just said up-thread, I'd like to see STATIC_ASSERT being put to use at
> once (in the same series, not in the same patch). In addition, the
> documentation should be improved (the (constant-expression ,
> string-literal) parameter list seems absent, or at least insufficiently
> documented).
> 
> In turn, I doubt a v3 posting could be reviewed with enough care before
> we enter the soft feature freeze. I'd suggest to submit the v3 series as
> soon as we start the next development cycle.
> 
> Thanks
> Laszlo
> 
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Yao, 
>> Jiewen
>> Sent: Thursday, August 15, 2019 9:05 AM
>> To: devel@edk2.groups.io; vit9...@protonmail.com; Kinney, Michael D 
>> 
>> Cc: Laszlo Ersek 
>> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro
>> 
>> Good input.
>> I think we should separate the work to convert all EDKII code to use 
>> STATIC_ASSERT.
>> We can do that work once we add STATIC_ASSERT.
>> 
>> I recommend:
>> 
>> 1)  Step 1: Add STATIS_ASSERT - this patch and this Bugzilla
>> 
>> 2)  Step 2: Convert VERIFY_SIZE_OF to STATIS_ASSERT, and remove 
>> VERIFY_SIZE_OF – the other patch and the other Bugzilla
>> 
>> 3)  Step 3: Scan the rest, if there is need. – Another patch and another 
>> Bugzilla
>> 
>> Thank you
>> Yao Jiewen
>> 
>> From: devel@edk2.groups.io 
>> [mailto:devel@edk2.groups.io] On Behalf Of Vitaly Cheptosv via Groups.Io
>> Sent: Thursday, August 15, 2019 12:23 AM
>> To: Kinney, Michael D 
>> mailto:michael.d.kin...@intel.com>>
>> Cc: devel@edk2.groups.io; Laszlo Ersek 
>> mailto:ler...@redhat.com>>
>> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro
>> 
>> 
>> Michael, Liming, Laszlo,
>> 
>> Static assertions via _Static_assert are standard C11 functionality, thus 
>> any at least C11 (ISO/IEC 9899 2011) conforming compiler is required to 
>> support the second argument with the diagnostic description.
>> 
>> The notation without the message currently is only present in C++, not in C, 
>> thus the two-argument notation is the only allowed notation for 
>> _Static_assert for at least C17 (ISO/IEC 9899 2018) and below.
>> 
>> In the bottom of this message I included a quote from C17 for the relevant 
>> section (6.7.10).
>> 
>> GCC and CLANG (including Xcode) appear to be conforming to the standard for 
>> this section, and MSVC compiler static_assert extension also supports the 
>> diagnostic message argument. This is pretty much all we care about.
>> 
>> As for examples, I see little reason to clarify STATIC_ASSERT behaviour 
>> outside of the standard reference in its description and actual usage in the 
>> source code, but can do that just fine if you think that it may help 
>> somebody.
>> 
>> I fully agree that VERIFY_SIZE_OF usage should be converted to 
>> STATIC_ASSERT, and in fact I also suggest VERIFY_SIZE_OF to be entirely 
>> removed from Base.h. This should be fairly costless, as apparently it is 
>> only used in Base.h and MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c, 
>> which I can replace in the same patch set.
>> 
>> As for select ASSERT usage switching to STATIC_ASSERT, this would also be 
>> great, as let us be honest, the use of ASSERT in EDK II codebase is very 
>> questioning. In fact, this was one of the reasons we introduced our own 
>> static assertions some time ago. However, 

Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro

[Vitaly Cheptsov via Groups.Io]

Andrew, Mike,

Actually thanks for making me recheck it. I have just installed doxygen, and 
can confirm that it can generate parameters for non-functional macros. This was 
my main reason for going with /// comment style, which is used for all other 
plain macros. I have just submitted V3 version with this fixed.

With C language modernisation I fully agree that it needs to be done 
reasonably, and that is why I also try to be very realistic on what people 
actually use. For instance, one of the issues that caused several problems is 
the use of 1-sized arrays instead of flexible array members. A couple years ago 
that was still there just because there still existed some compilers that did 
not support [] syntax.

Best wishes,
Vitaly

> 17 авг. 2019 г., в 1:58, Andrew Fish  написал(а):
> 
> 
> 
>> On Aug 16, 2019, at 2:40 PM, Vitaly Cheptsov via Groups.Io 
>> mailto:vit9696=protonmail@groups.io>> 
>> wrote:
>> 
>> Mike,
>> 
>> I missed your message while writing mine, but I am afraid I disagree with 
>> the functional macro usage for this feature.
>> 
>> I explicitly quoted C standard static_assert definition in one of my 
>> previous messages, and I want EDK II to be as close to standard C as 
>> possible.
>> 
>> This will avoid a lot of confusion for newcomers and will let us later adopt 
>> a more flexible single and double argument interface when it gets 
>> standardised.
>> 
>> For these reasons altogether, I am not positive the macro could get a 
>> doxygen documentation as it is not designed to have any arguments in the 
>> first place.
>> 
> 
> Vitaly,
> 
> I don't understand your logic? It is always possible to write a comment in C 
> code?
> 
> In terms of the C standard and the EFI type system we kind of have a long 
> history of how the code ended up like it is. The (U)EFI spec defined its own 
> type system (and ABI) as a way of specifying interoperability so the code got 
> built on top of that. 20 years ago we shied away from having and EFI code 
> base produce definitions of standard C things as we wanted to make it easier 
> to import chunks of code in OS loaders that needed to get ported to EFI from 
> lots of different vendors. Also one of the early compilers that we 
> standardized on was VC2003 and that does not even fully support C99. For some 
> reason it seems VC2008 was also a popular target for some time. I don't think 
> VC++ got around to C99 until 2013/2015. So that is kind how the edk2 ended up 
> with its own type system. 
> 
> I'm all for modernization of the C code as long we are thoughtful about 
> compatibility. For example I still see that VS2008 is a supported 
> BaseTools/Conf/tools_def.template.
> 
> Thanks,
> 
> Andrew Fish
> 
> 
>> Best wishes,
>> Vitaly
>> 
>> On сб, авг. 17, 2019 at 00:04, Kinney, Michael D > <mailto:michael.d.kin...@intel.com>> wrote:
>>> 
>>> Laszlo,
>>> 
>>> I agree that better comments/documentation of STATIC_ASSERT()
>>> for EDK II usages is required. For example, EDK II defines
>>> the ASSERT() macro which is based on the standard C function
>>> assert(), but we still document it fully for EDK II usage.
>>> 
>>> /**
>>> Macro that calls DebugAssert() if an expression evaluates to FALSE.
>>> 
>>> If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
>>> bit of PcdDebugProperyMask is set, then this macro evaluates the Boolean
>>> expression specified by Expression. If Expression evaluates to FALSE, then
>>> DebugAssert() is called passing in the source filename, source line number,
>>> and Expression.
>>> 
>>> @param Expression Boolean expression.
>>> 
>>> **/
>>> #if !defined(MDEPKG_NDEBUG)
>>> #define ASSERT(Expression) \
>>> do { \
>>> if (DebugAssertEnabled ()) { \
>>> if (!(Expression)) { \
>>> _ASSERT (Expression); \
>>> ANALYZER_UNREACHABLE (); \
>>> } \
>>> } \
>>> } while (FALSE)
>>> #else
>>> #define ASSERT(Expression)
>>> #endif
>>> 
>>> I would like to see the macro documentation for
>>> STATIC_ASSERT() with the Doxygen style description of the
>>> parameters. The fact I asked if STATIC_ASSERT() supported
>>> the 2nd message parameter should have been a trigger
>>> for me to ask for the more complete macro comment block.
>>> The fact that this macro can be directly mapped to
>>> built in compiler name makes the implementation simple,
>>> but other implementations

[edk2-devel] [PATCH v1 1/3] MdeModulePkg/ResetUtilityLib: Use STATIC_ASSERT macro

[Vitaly Cheptsov via Groups.Io]

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

Use new STATIC_ASSERT macro instead of VERIFY_SIZE_OF.

Signed-off-by: Vitaly Cheptsov 
---
 MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c 
b/MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c
index 2b5af4b95a..bb151d0331 100644
--- a/MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c
+++ b/MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c
@@ -20,7 +20,10 @@ typedef struct {
 } RESET_UTILITY_GUID_SPECIFIC_RESET_DATA;
 #pragma pack()
 
-VERIFY_SIZE_OF (RESET_UTILITY_GUID_SPECIFIC_RESET_DATA, 18);
+STATIC_ASSERT (
+  sizeof (RESET_UTILITY_GUID_SPECIFIC_RESET_DATA) == 18,
+  "sizeof (RESET_UTILITY_GUID_SPECIFIC_RESET_DATA) is expected to be 18 bytes"
+  );
 
 /**
   This is a shorthand helper function to reset with reset type and a subtype
-- 
2.20.1 (Apple Git-117)


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

View/Reply Online (#45870): https://edk2.groups.io/g/devel/message/45870
Mute This Topic: https://groups.io/mt/32918009/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

[edk2-devel] [PATCH v1 2/3] MdePkg: Use STATIC_ASSERT macro

[Vitaly Cheptsov via Groups.Io]

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

Use new STATIC_ASSERT macro instead of VERIFY_SIZE_OF.

Signed-off-by: Vitaly Cheptsov 
---
 MdePkg/Include/Base.h | 79 ++--
 1 file changed, 40 insertions(+), 39 deletions(-)

diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index ec096133ba..d871422cd6 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -41,45 +41,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
 #define VERIFY_SIZE_OF(TYPE, Size) extern UINT8 
_VerifySizeof##TYPE[(sizeof(TYPE) == (Size)) / (sizeof(TYPE) == (Size))]
 
-//
-// Verify that ProcessorBind.h produced UEFI Data Types that are compliant with
-// Section 2.3.1 of the UEFI 2.3 Specification.
-//
-VERIFY_SIZE_OF (BOOLEAN, 1);
-VERIFY_SIZE_OF (INT8, 1);
-VERIFY_SIZE_OF (UINT8, 1);
-VERIFY_SIZE_OF (INT16, 2);
-VERIFY_SIZE_OF (UINT16, 2);
-VERIFY_SIZE_OF (INT32, 4);
-VERIFY_SIZE_OF (UINT32, 4);
-VERIFY_SIZE_OF (INT64, 8);
-VERIFY_SIZE_OF (UINT64, 8);
-VERIFY_SIZE_OF (CHAR8, 1);
-VERIFY_SIZE_OF (CHAR16, 2);
-
-//
-// The following three enum types are used to verify that the compiler
-// configuration for enum types is compliant with Section 2.3.1 of the
-// UEFI 2.3 Specification. These enum types and enum values are not
-// intended to be used. A prefix of '__' is used avoid conflicts with
-// other types.
-//
-typedef enum {
-  __VerifyUint8EnumValue = 0xff
-} __VERIFY_UINT8_ENUM_SIZE;
-
-typedef enum {
-  __VerifyUint16EnumValue = 0x
-} __VERIFY_UINT16_ENUM_SIZE;
-
-typedef enum {
-  __VerifyUint32EnumValue = 0x
-} __VERIFY_UINT32_ENUM_SIZE;
-
-VERIFY_SIZE_OF (__VERIFY_UINT8_ENUM_SIZE, 4);
-VERIFY_SIZE_OF (__VERIFY_UINT16_ENUM_SIZE, 4);
-VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4);
-
 //
 // The Microsoft* C compiler can removed references to unreferenced data items
 //  if the /OPT:REF linker option is used. We defined a macro as this is a
@@ -857,6 +818,46 @@ typedef UINTN  *BASE_LIST;
   #define STATIC_ASSERT _Static_assert
 #endif
 
+//
+// Verify that ProcessorBind.h produced UEFI Data Types that are compliant with
+// Section 2.3.1 of the UEFI 2.3 Specification.
+//
+
+STATIC_ASSERT (sizeof (BOOLEAN) == 1, "sizeof (BOOLEAN) does not meet UEFI 
Specification Data Type requirements");
+STATIC_ASSERT (sizeof (INT8)== 1, "sizeof (INT8) does not meet UEFI 
Specification Data Type requirements");
+STATIC_ASSERT (sizeof (UINT8)   == 1, "sizeof (UINT8) does not meet UEFI 
Specification Data Type requirements");
+STATIC_ASSERT (sizeof (INT16)   == 2, "sizeof (INT16) does not meet UEFI 
Specification Data Type requirements");
+STATIC_ASSERT (sizeof (UINT16)  == 2, "sizeof (UINT16) does not meet UEFI 
Specification Data Type requirements");
+STATIC_ASSERT (sizeof (INT32)   == 4, "sizeof (INT32) does not meet UEFI 
Specification Data Type requirements");
+STATIC_ASSERT (sizeof (UINT32)  == 4, "sizeof (UINT32) does not meet UEFI 
Specification Data Type requirements");
+STATIC_ASSERT (sizeof (INT64)   == 8, "sizeof (INT64) does not meet UEFI 
Specification Data Type requirements");
+STATIC_ASSERT (sizeof (UINT64)  == 8, "sizeof (UINT64) does not meet UEFI 
Specification Data Type requirements");
+STATIC_ASSERT (sizeof (CHAR8)   == 1, "sizeof (CHAR8) does not meet UEFI 
Specification Data Type requirements");
+STATIC_ASSERT (sizeof (CHAR16)  == 2, "sizeof (CHAR16) does not meet UEFI 
Specification Data Type requirements");
+
+//
+// The following three enum types are used to verify that the compiler
+// configuration for enum types is compliant with Section 2.3.1 of the
+// UEFI 2.3 Specification. These enum types and enum values are not
+// intended to be used. A prefix of '__' is used avoid conflicts with
+// other types.
+//
+typedef enum {
+  __VerifyUint8EnumValue = 0xff
+} __VERIFY_UINT8_ENUM_SIZE;
+
+typedef enum {
+  __VerifyUint16EnumValue = 0x
+} __VERIFY_UINT16_ENUM_SIZE;
+
+typedef enum {
+  __VerifyUint32EnumValue = 0x
+} __VERIFY_UINT32_ENUM_SIZE;
+
+STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not 
meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (sizeof (__VERIFY_UINT16_ENUM_SIZE) == 4, "Size of enum does not 
meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not 
meet UEFI Specification Data Type requirements");
+
 /**
   Macro that returns a pointer to the data structure that contains a specified 
field of
   that data structure.  This is a lightweight method to hide information by 
placing a
-- 
2.20.1 (Apple Git-117)


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

View/Reply Online (#45873): https://edk2.groups.io/g/devel/message/45873
Mute This Topic: https://groups.io/mt/32918012/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital 

[edk2-devel] [PATCH v1 3/3] MdePkg: Drop VERIFY_SIZE_OF in favour of STATIC_ASSERT

[Vitaly Cheptsov via Groups.Io]

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

New STATIC_ASSERT macro supersedes VERIFY_SIZE_OF as being more functional.

Signed-off-by: Vitaly Cheptsov 
---
 MdePkg/Include/Base.h | 13 -
 1 file changed, 13 deletions(-)

diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index d871422cd6..ed85b98318 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -28,19 +28,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #pragma warning ( disable : 4200 )
 #endif
 
-/**
-  Verifies the storage size of a given data type.
-
-  This macro generates a divide by zero error or a zero size array declaration 
in
-  the preprocessor if the size is incorrect.  These are declared as "extern" so
-  the space for these arrays will not be in the modules.
-
-  @param  TYPE  The date type to determine the size of.
-  @param  Size  The expected size for the TYPE.
-
-**/
-#define VERIFY_SIZE_OF(TYPE, Size) extern UINT8 
_VerifySizeof##TYPE[(sizeof(TYPE) == (Size)) / (sizeof(TYPE) == (Size))]
-
 //
 // The Microsoft* C compiler can removed references to unreferenced data items
 //  if the /OPT:REF linker option is used. We defined a macro as this is a
-- 
2.20.1 (Apple Git-117)


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

View/Reply Online (#45872): https://edk2.groups.io/g/devel/message/45872
Mute This Topic: https://groups.io/mt/32918011/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

[edk2-devel] [PATCH v1 0/3] Replace VERIFY_SIZE_OF with STATIC_ASSERT

[Vitaly Cheptsov via Groups.Io]

Things to note:

- This patchset should go after STATIC_ASSERT implementation:
  https://edk2.groups.io/g/devel/topic/32917749
- It is suggested that unlike the previous patch, which in my
  opinion should appear in edk2-stable201908, this patchset should
  land in edk2-stable2019011. This will let more people to comment
  whether they are ready to use it as is.

Vitaly Cheptsov (3):
  MdeModulePkg/ResetUtilityLib: Use STATIC_ASSERT macro
  MdePkg: Use STATIC_ASSERT macro
  MdePkg: Drop VERIFY_SIZE_OF in favour of STATIC_ASSERT

 MdePkg/Include/Base.h   | 92 +---
 MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c |  5 +-
 2 files changed, 44 insertions(+), 53 deletions(-)

-- 
2.20.1 (Apple Git-117)


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

View/Reply Online (#45871): https://edk2.groups.io/g/devel/message/45871
Mute This Topic: https://groups.io/mt/32918010/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro

[Vitaly Cheptsov via Groups.Io]

Laszlo,

I am very glad to you for expressing a different opinion as this lets me view 
the situation from different angles.

I understand your concerns, and believe that most of them should actually be 
addressed in a way you explain. In fact, I plan to submit more patches myself 
for everyone's benefit.

The exact situation with static assertions is that they are not coming too 
early, but actually too late. We have been using static assertions in UEFI code 
for quite some time already, and I believe we are not alone. All of us will 
benefit from legacy code removal once this patch lands upstream.

For your claim that this code is not well tested I should mention that the 
patch is based on one of the open-source projects I maintain, which everyone 
can track, and which I believe have gotten reasonable attention from different 
people with different compilers.

For dead code I believe that in EDK II we do not have a good definition for 
that term as normally done in serious industrial projects like aerospace or 
military that have no dead code requirement in their SDL. Primarily because EDK 
II is a library for others to rely on, it is not a self contained system where 
dead code term is usually defined, standardised and verified against.

Whether it is liked or not, the fact EDK II gets continual development is only 
because different companies, academia, and individuals use its code. I feel bad 
for these people having to fork, and believe that most value in EDK is what it 
gives to the outside, not the inside. So supporting a new interface a number of 
projects use and need makes most sense to me.

I do not want to make more changes to core code for multiple reasons as you see 
above. One of them indeed being some necessary discussion for the use inside 
EDK II. But I do not believe this a good stopper from giving a working 
interface to others, which unlike EDK II, actually have defined compilers, 
infrastructure, and requirements.

Hopefully I pointed out to enough reasons to leave you with some doubts and 
permit this patch to land in as an exception from your personal standpoint. 
Thank you for understanding and being constructive.

Cheers,
Vitaly

On пт, авг. 16, 2019 at 22:38, Laszlo Ersek  wrote:

> On 08/16/19 19:23, vit9...@protonmail.com wrote:
>> Laszlo,
>>
>> I have already mentioned that the documentation is sufficient as
>> _Static_assert is C standard
>
> Yes, in a release of the ISO C standard that edk2 does not target.
>
> In addition, edk2 already has several restrictions in place against
> standards-conformant code. (Such as bit-shifting of UINT64 values,
> initialization of structures with automatic storage duration, structure
> assignment, maybe more.)
>
>> so I do not plan to make a V3 for this patch.
>
> I find that regrettable.
>
>> The patch is merge ready.
>
> Such statements are usually made when people that comment on a patch
> arrive at a consensus. The patch may be merge-ready from your
> perspective and from Mike's. It is not merge-ready from my perspective.
> I hope I'm allowed to comment (constructively) on patches that aren't
> strictly aimed at the subsystems I co-maintain.
>
>> As for usage examples I have an opposing opinion to yours and believe
>> it is based on very good reasons. Not using STATIC_ASSERT in the
>> current release will make the feature optionally available and let
>> people test it in their setups.
>
> Not using STATIC_ASSERT in the current stable release makes the
> STATIC_ASSERT macro definition *dead code* in edk2 proper. I understand
> that edk2 is a "kit", and quite explicitly caters to out-of-tree
> platforms. That's not a positive trait of edk2 however; it's a negative
> one, in my judgement. Whatever we add to the core of edk2, we should
> exercise as diligently as we can *inside* of edk2.
>
>> In case they notice it does not work for them they will have 3 months
>> grace period to report it to us and consider making a change.
>
> That is what the feature freezes are for. The feature is reviewed before
> the soft feature freeze, merged (at the latest) during the soft feature
> freeze, and bugs can still be fixed during the hard feature freeze. The
> community is expected to test diligently during the hard feature freeze.
> Perhaps we should extend the hard feature freeze.
>
> My problem is not that the change is not "in your face". I'm all for
> avoiding regressions. My problem is that the code is dead and untestable
> without platform changes, even though it could be put to great use in
> core code at once. If you think that's too risky, this close to the
> stable tag, then one solution is to resubmit at the beginning of the
> next development cycle (again with additional patches that utilize the
> STATIC_ASSERT macro at once). Developers will then have close to three
> months to report and fix issues.
>
> Another solution would be to conditionally keep VERIFY_SIZE_OF, vs.
> using STATIC_ASSERT, for expressing the build-time invariants. 

[edk2-devel] [PATCH v3 1/1] MdePkg: Add STATIC_ASSERT macro

[Vitaly Cheptsov via Groups.Io]

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

Provide a macro for compile time assertions.
Equivalent to C11 static_assert macro from assert.h.

Signed-off-by: Vitaly Cheptsov 
---
 MdePkg/Include/Base.h | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index ce20b5f01d..ec096133ba 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -843,6 +843,20 @@ typedef UINTN  *BASE_LIST;
 #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
 #endif
 
+/**
+  Portable definition for compile time assertions.
+  Equivalent to C11 static_assert macro from assert.h.
+
+  @param  Expression  Boolean expression.
+  @param  Message Raised compiler diagnostic message when expression is 
false.
+
+**/
+#ifdef _MSC_EXTENSIONS
+  #define STATIC_ASSERT static_assert
+#else
+  #define STATIC_ASSERT _Static_assert
+#endif
+
 /**
   Macro that returns a pointer to the data structure that contains a specified 
field of
   that data structure.  This is a lightweight method to hide information by 
placing a
-- 
2.20.1 (Apple Git-117)


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

View/Reply Online (#45868): https://edk2.groups.io/g/devel/message/45868
Mute This Topic: https://groups.io/mt/32917749/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf

[Vitaly Cheptsov via Groups.Io]

Hi Donald,

Glad to hear it helped a little, and sorry for some outdated quotes.

Your clarification regarding model range is very helpful. Xeon W being client 
and thus having client clock makes sense, though I must say it was quite not 
obvious. I was referring to the same SDM table, and it would have been great to 
have vertical range binding instead of the misleading CPUID.

With that, however, I still do not see a way to dynamically determine the 
difference between Xeon client and server.

For us it is needed as we use TimerLib differently. For you TimerLib is a part 
of BSP, so you face no issue statically setting the clock frequency as a PCD. 
For us TimerLib is used as a part of an UEFI application compatible with 
different hardware, and in fact not just Intel. We have such "generic TimerLib" 
library, and to determine CPU frequency, as a fallback to CPUID 15H, it relies 
on ACPI PM timer. This is not great for two reasons:
- we have to maintain it ourselves, while we would have liked that be part of 
EDK II with different vendors contributing to the project.
- we still cannot find an obvious way to determine crystal clock when it is not 
reported, in particular for Xeon W and Xeon Scalable case.

I guess this is now out of the scope of this patch and perhaps even this exact 
library, but it will be helpful to hear relevant technical details on the issue 
and opinions on such "generic TimerLib" library to later appear in EDK II.

Best regards,
Vitaly

> 15 авг. 2019 г., в 11:40, Kuo, Donald  написал(а):
> 
> Hi Vitaly,
>
> Appreciated for reviewing very detail. And looks your captured some piece of 
> code is older version. And should be ok, I do got your point.
>
> Item #1
> -  I do missed RegEax error handling in case for 
> CpuidCoreClockCalculateTscFrequency () should return 0 and EFI_UNSUPPORTED. 
> AR: Will update it.
>
> Item #2:
> -  Actually the information is from SDM Table 18-85
> o   As we know CPUID signature could be hard to identify processor XTAL 
> frequency is? So we only check CPUID Leaf 0x15
> o   And the PCD will be defined depends on platform specific and during 
> project early development. Based on SDM, Intel processor for CPUID.15h EAX 
> and EBX is enumerated, but ECX could be possible not enumerated. So that is 
> why we based on  SDM Table 18-85 to create PCD as below:
> §  Intel Xeon Server family: 25MHz
> §  Intel Core and Intel Xeon W family: 24MHz
> §  Intel Atom : 19.2MHz
> §  So regards the “06_55h” might cause the confused, we could review it 
> whether remove it??
> Item #3:
> -  Again, the XTAL frequency is optional and should be define in 
> early phase of new project. Default should still use AcpiTimer.
> oPlatform / developer owner should well know the CPU generation XTAL 
> frequency is? Server / Client / Atom ?
> o   For those CPU not supported should still use AcpiTimerLib (default)
>
> Thanks,
> Donald
>   <>
>  <>From: vit9696 via [] [mailto:vit9696=protonmail.com 
> @[]] 
> Sent: Thursday, August 15, 2019 3:20 PM
> To: Kuo, Donald mailto:donald@intel.com>>; 
> devel@edk2.groups.io 
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by 
> using CPUID(0x15) TSC leaf
>
> Hello,
> 
> Thank you for the patch! I reviewed the code and noticed select issues 
> explained below.
> 
> 1. The following construction:
> 
> if (RegEbx == 0) {
> DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock Frequency 
> !!\n"));
> ASSERT (RegEbx != 0);
> }
> 
> Does not look good to me, and in my opinion, should be written differently:
> 
> if (RegEbx == 0 || RegEax == 0) {
> DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock Frequency 
> !!\n"));
> ASSERT (RegEbx != 0);
>   ASSERT (RegEax != 0);
>   return 0;
> }
> 
> The reason for the above code being wrong is potential division by zero 
> below, which behaviour is undefined (and in fact unknown due to unspecified 
> interrupt table state). Even though the intent is to not permit the use of 
> this library on an unsupported platform, runtime checks and assertions are 
> essentially NO-OPs, should be separate and not confused. For this to work 
> properly the call to CpuidCoreClockCalculateTscFrequency should happen in 
> library constructor with EFI_UNSUPPORTED return on 
> CpuidCoreClockCalculateTscFrequency returning 0.
> 
> 2. The notes about crystal clock frequency for 06_55H CPU signature:
> "2500 - Intel Xeon Processor Scalable Family with CPUID signature 
> 06_55H(25MHz).\n"
> # Intel Xeon Processor Scalable Family with CPUID signature 06_55H = 2500 
> (25MHz)
> 
> are misleading in both this library and Intel SDM. Intel Xeon W processors 
> have the same CPUID signature (06_55H), they report 0 crystal clock 
> frequency, and their crystal clock frequency is 24 MHz. This should at least 
> be mentioned in the lower part mentioning Intel Xeon W Processor 
> Family(24MHz).
> 

Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf

[Vitaly Cheptsov via Groups.Io]

Hello,

Thank you for the patch! I reviewed the code and noticed select issues 
explained below.

1. The following construction:

if (RegEbx == 0) {
DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock Frequency 
!!\n"));
ASSERT (RegEbx != 0);
}

Does not look good to me, and in my opinion, should be written differently:

if (RegEbx == 0 || RegEax == 0 ) {
DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock Frequency 
!!\n"));
ASSERT (RegEbx != 0);
ASSERT (RegEax != 0);
return 0;
}

The reason for the above code being wrong is potential division by zero below, 
which behaviour is undefined (and in fact unknown due to unspecified interrupt 
table state). Even though the intent is to not permit the use of this library 
on an unsupported platform, runtime checks and assertions are essentially 
NO-OPs, should be separate and not confused. For this to work properly the call 
to CpuidCoreClockCalculateTscFrequency should happen in library constructor 
with EFI_UNSUPPORTED return on CpuidCoreClockCalculateTscFrequency returning 0.

2. The notes about crystal clock frequency for 06_55H CPU signature:
"2500 - Intel Xeon Processor Scalable Family with CPUID signature 
06_55H(25MHz).\n"
# Intel Xeon Processor Scalable Family with CPUID signature 06_55H = 2500 
(25MHz)

are misleading in both this library and Intel SDM. Intel Xeon W processors have 
the same CPUID signature ( 06_55H) , they report 0 crystal clock frequency, and 
their crystal clock frequency is 24 MHz. This should at least be mentioned in 
the lower part mentioning Intel Xeon W Processor Family(24MHz).

Actually, given that many Intel guys are here, I wonder whether anybody knows 
if there is any better approach to distinguish Xeon Scalable CPUs and Xeon W 
CPUs besides using brand string or using marketing frequency from CPUID 16H to 
determine crystal clock frequency (this is what Linux does at the moment)?

3. Intel Atom Denverton with CPUID signature (06_5FH), also known as Goldmont 
X, reports 0 crystal clock frequency as well, and has 25 MHz frequency. This is 
missing from SDM, but once again I believe it should be included in the two 
places from the above to avoid confusion.

Besides these 3 points, honestly, the library itself appears to be very limited 
for anything but embedding it into the firmware with known hardware, which 
already works just fine by defining the PCD. Missing 0 crystal clock frequency 
handling in runtime with hardcoding the PCD value looks very bad, because the 
number of modern Intel CPU models reporting 0 crystal clock frequency and 
having non 24 MHz frequency is quite far from 0. This makes the library 
essentially impossible to use in any UEFI application or third-party product 
even when targeting Skylake+ generation. To resolute this I vote for additional 
detection functionality to be added to the library to obtain crystal clock 
frequency when the CPUID reports 0. The PCD could be the last resort when no 
other method works. My opinion is that this should be resolved prior to merging 
the patch.

Best regards,
Vitaly

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

View/Reply Online (#45689): https://edk2.groups.io/g/devel/message/45689
Mute This Topic: https://groups.io/mt/32839184/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] Determining TSC frequency programmatically

[Vitaly Cheptsov via Groups.Io]

Hello,

I initially raised this question in a new TimerLib patch[1], but as the 
discussion was getting more distracted, I decided to create a separate thread 
in hopes new people could join.

The issue is that our UEFI bootloader needs to obtain TSC frequency to pass it 
to our specialised operating system that uses TSC for scheduling on x86.

For a while we went with ACPI power management timer to measure the frequency, 
but as modern Intel CPUs support CPUID 15H leaf (CPUID_TIME_STAMP_COUNTER) we 
try to use where possible for better accuracy. The issue with this CPUID leaf 
is that the crystal clock frequency returned in ECX register is optional and 
therefore can be 0. Intel SDM suggests to use a static value in this case[2], 
but it is completely opaque on how to match the running CPU with its static 
value from SDM.

Initially we went with CPUID model checking, but this failed badly for Xeon 
Scalable and Xeon W, as they share the CPUID (06_55H) but have different 
crystal clock frequencies (25 MHz vs 24 MHz accordingly). Donald Kuo gave a 
good hint in the previous thread that client CPUs usually get 24 MHz crystal 
clock, server CPUs have 25, and Atoms have 19.2. This, however, does not make 
the situation easier as we do not see a way to determine CPU vertical segment 
without e.g. parsing the CPUID brand string.

Apparently, we are not alone, and different open-source operating systems have 
different workarounds to this issue. For example, Linux kernel went with using 
marketing frequency from CPUID 16H leaf (CPUID_PROCESSOR_FREQUENCY)[3], and BSD 
flavours fallback to older methods when neither crystal clock frequency can be 
obtained through CPUID 15H, nor unambiguous CPUID models exist to be able to 
use static values.

Another issue we see with EDK II TimerLib implementations for x86 is that they 
are very model specific. As Michael Kinney said, the situation is not a problem 
when you use TimerLib for BSP bringup, as you already know the CPU family you 
target, however, it is not great for other uses, although they may be 
discouraged. In our opinion, regardless of the use, this approach has a number 
of design issues.

All modern Intel x86 CPUs have virtual TSC counter that has fixed frequency. In 
fact, this is true for most, if not all, modern x86 CPUs by other vendors as 
well. This makes us believe that EDK II should have generic TscTimerLib for 
x86, which will work anywhere when given valid TSC frequency, and 
TscFrequencyLib, which would determine TSC frequency in a vendor-specific 
method. Ideally there exists GenericTscFrequencyLib that can do this for a wide 
majority of CPUs through different methods at runtime, including CPUID 15H, 
ACPI power management, vendor-specific extensions, etc.

To summarise:
- We need to know how to match current running CPU with its crystal clock 
frequency when CPUID 15H reports 0 ECX.
- We would like to suggest to split TSC-based TimerLib in two libraries: 
generic with timer implementation and platform-specific with TSC frequency 
discovery.
- We wonder whether a generic vendor-supported TSC frequency discovery library 
working on a wide range of CPUs is feasible to have in EDK II mainline.

Best regards,
Vitaly

[1] 
https://edk2.groups.io/g/devel/topic/patch_ueficpupkg_adding_a/32839184?p=,,,20,0,0,0::recentpostdate%2Fsticky,,,20,2,0,32839184
 

[PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf
[2] 18.7.3 Determining the Processor Base Frequency
Table 18-75. Nominal Core Crystal Clock Frequency
[3] https://lore.kernel.org/lkml/20190509055417.13152-1-dr...@endlessm.com/ 



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

View/Reply Online (#45750): https://edk2.groups.io/g/devel/message/45750
Mute This Topic: https://groups.io/mt/32891598/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

[edk2-devel] [PATCH v3 0/1] MdePkg: Add STATIC_ASSERT macro

[Vitaly Cheptsov via Groups.Io]

Implements https://bugzilla.tianocore.org/show_bug.cgi?id=3D2048.

Things to note:
- _Static_assert is a standard C11 keyword and thus is available
on every modern compiler (including Apple Clang, Clang, and GCC).
See: https://en.cppreference.com/w/c/keyword/_Static_assert
- static_assert is a hack to support MSVC, which implements static
assertions with this vendor-specific keyword starting from at least
VS 2010 to date.
- V3 of the patch addresses the confusion with doxygen, which I
expected to not be able to handle @param for a macro with no arguments.
- The replacement of VERIFY_SIZE_OF will be submitted in a separate
patch series, and I request the colleagues to test this on their setups.
As for myself I can report that it works for me with CLANG38, GCC5,
VS2017, and XCODE5. 


Vitaly Cheptsov (1):
  MdePkg: Add STATIC_ASSERT macro

 MdePkg/Include/Base.h | 14 ++
 1 file changed, 14 insertions(+)

-- 
2.20.1 (Apple Git-117)


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

View/Reply Online (#45867): https://edk2.groups.io/g/devel/message/45867
Mute This Topic: https://groups.io/mt/32917747/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro

[Vitaly Cheptsov via Groups.Io]

Mike,

I missed your message while writing mine, but I am afraid I disagree with the 
functional macro usage for this feature.

I explicitly quoted C standard static_assert definition in one of my previous 
messages, and I want EDK II to be as close to standard C as possible.

This will avoid a lot of confusion for newcomers and will let us later adopt a 
more flexible single and double argument interface when it gets standardised.

For these reasons altogether, I am not positive the macro could get a doxygen 
documentation as it is not designed to have any arguments in the first place.

Best wishes,
Vitaly

On сб, авг. 17, 2019 at 00:04, Kinney, Michael D  
wrote:

> Laszlo,
>
> I agree that better comments/documentation of STATIC_ASSERT()
> for EDK II usages is required. For example, EDK II defines
> the ASSERT() macro which is based on the standard C function
> assert(), but we still document it fully for EDK II usage.
>
> /**
> Macro that calls DebugAssert() if an expression evaluates to FALSE.
>
> If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
> bit of PcdDebugProperyMask is set, then this macro evaluates the Boolean
> expression specified by Expression. If Expression evaluates to FALSE, then
> DebugAssert() is called passing in the source filename, source line number,
> and Expression.
>
> @param Expression Boolean expression.
>
> **/
> #if !defined(MDEPKG_NDEBUG)
> #define ASSERT(Expression) \
> do { \
> if (DebugAssertEnabled ()) { \
> if (!(Expression)) { \
> _ASSERT (Expression); \
> ANALYZER_UNREACHABLE (); \
> } \
> } \
> } while (FALSE)
> #else
> #define ASSERT(Expression)
> #endif
>
> I would like to see the macro documentation for
> STATIC_ASSERT() with the Doxygen style description of the
> parameters. The fact I asked if STATIC_ASSERT() supported
> the 2nd message parameter should have been a trigger
> for me to ask for the more complete macro comment block.
> The fact that this macro can be directly mapped to
> built in compiler name makes the implementation simple,
> but other implementations are possible for compilers
> that do not support this feature directly. This is why
> the complete description of the EDK II version is required.
>
> I would like to see a V3 with the complete description.
>
> In general, I agree it is better if there is code that
> uses a new feature in the code base, so the feature can
> be tested. A second option we can consider going forward
> is for unit test code to be submitted with a new feature,
> so even if there are no consumers from the EDK II repos,
> the feature can still be tested as the EDK II repos are
> maintained. A third option is for community members to
> provide Tested-by responses to the feature along with
> statements in the Bugzilla that clearly documents how the
> the feature was tested.
>
> Best regards,
>
> Mike
>
>> -Original Message-
>> From: devel@edk2.groups.io
>> [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
>> Sent: Friday, August 16, 2019 12:39 PM
>> To: vit9...@protonmail.com
>> Cc: devel@edk2.groups.io; leif.lindh...@linaro.org;
>> af...@apple.com
>> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add
>> STATIC_ASSERT macro
>>
>> On 08/16/19 19:23, vit9...@protonmail.com wrote:
>> > Laszlo,
>> >
>> > I have already mentioned that the documentation is
>> sufficient as
>> > _Static_assert is C standard
>>
>> Yes, in a release of the ISO C standard that edk2 does
>> not target.
>>
>> In addition, edk2 already has several restrictions in
>> place against standards-conformant code. (Such as bit-
>> shifting of UINT64 values, initialization of structures
>> with automatic storage duration, structure assignment,
>> maybe more.)
>>
>> > so I do not plan to make a V3 for this patch.
>>
>> I find that regrettable.
>>
>> > The patch is merge ready.
>>
>> Such statements are usually made when people that
>> comment on a patch arrive at a consensus. The patch may
>> be merge-ready from your perspective and from Mike's.
>> It is not merge-ready from my perspective.
>> I hope I'm allowed to comment (constructively) on
>> patches that aren't strictly aimed at the subsystems I
>> co-maintain.
>>
>> > As for usage examples I have an opposing opinion to
>> yours and believe
>> > it is based on very good reasons. Not using
>> STATIC_ASSERT in the
>> > current release will make the feature optionally
>> available and let
>> > people test it in their setups.
>>
>> Not using STATIC_ASSERT in the current stable release
>> makes the STATIC_ASSERT macro definition *dead code* in
>> edk2 proper. I understand that edk2 is a "kit", and
>> quite explicitly caters to out-of-tree platforms.
>> That's not a positive trait of edk2 however; it's a
>> negative one, in my judgement. Whatever we add to the
>> core of edk2, we should exercise as diligently as we
>> can *inside* of edk2.
>>
>> > In case they notice it does not work for them they
>> will have 3 months
>> > grace period to report it to us and consider 

Re: [edk2-devel] Determining TSC frequency programmatically

[Vitaly Cheptsov via Groups.Io]

Andrew,

Thanks for the reminder for CpuDxe.

I think we saw CpuDxe calculation some time ago, and it was most likely a 
reason we went with a very similar approach to determine the TSC frequency as a 
fallback mechanism. It, however, is in fact prone to issues, putting aside the 
non-guaranteed period and CPU frequency match. Basically, there are two 
problems:

1. The code implemented in C is subject to minor time drifts, which results in 
slightly different results across the reboots. The calculation based on CPUID 
15H is more accurate.
2. The lack of TPL changes during frequency measurement results in EFI_EVENTs 
potentially interrupting the process and thus making the resulting value very 
inaccurate. Since the value is cached there is not always a possibility to 
avoid it.

EmulatorPkg is a bit of a special case, which we think we know about :), 
actually thanks for fixing it up on macOS, this is appreciated!

All in all, I would say that there really needs to exist a decent library, 
which most likely CpuDxe should make a use of as well.

Best wishes,
Vitaly

> 16 авг. 2019 г., в 21:35, Andrew Fish  написал(а):
> 
> Vitaly,
> 
> As Mike mentioned platforms can know more info about how they are constructed 
> thus you may not want to have a lot of generic discovery code floating about 
> if you don't really need it. 
> 
> One option could be to pass up the TSC Frequency/Period via some EFI 
> mechanism so generic code can be told by platform specific code. 
> 
> The PI spec already has an abstraction for a CPU based timer that is 
> architecture neutral. The CPU Architectural Protocol has a GetTimerValue() 
> member function. 
> https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Protocol/Cpu.h#L220
>  
> <https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Protocol/Cpu.h#L220>
> 
> For X86 it returns TSC
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/CpuDxe/CpuDxe.c#L289 
> <https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/CpuDxe/CpuDxe.c#L289>
> 
> EFI Systems are not required to implement PI so we usually don't encourage 
> generic EFI code to go after PI APIs. 
> 
> I'd also point out that using TSC can break in things like the EmulatorPkg as 
> you end up running in ring 0 and TSC access is blocked. 
> https://github.com/tianocore/edk2/blob/master/EmulatorPkg/CpuRuntimeDxe/Cpu.c#L352
>  
> <https://github.com/tianocore/edk2/blob/master/EmulatorPkg/CpuRuntimeDxe/Cpu.c#L352>
> https://github.com/tianocore/edk2/blob/master/EmulatorPkg/Unix/Host/EmuThunk.c#L250
>  
> <https://github.com/tianocore/edk2/blob/master/EmulatorPkg/Unix/Host/EmuThunk.c#L250>
> 
> 
> I would point that a library that did TSC frequency discovery would likely be 
> useful for the UefiCpuPkg CpuDxe driver. 
> 
> Thanks,
> 
> Andrew Fish
> 
>> On Aug 15, 2019, at 2:10 PM, Vitaly Cheptsov via Groups.Io 
>> mailto:vit9696=protonmail@groups.io>> 
>> wrote:
>> 
>> Hello,
>> 
>> I initially raised this question in a new TimerLib patch[1], but as the 
>> discussion was getting more distracted, I decided to create a separate 
>> thread in hopes new people could join.
>> 
>> The issue is that our UEFI bootloader needs to obtain TSC frequency to pass 
>> it to our specialised operating system that uses TSC for scheduling on x86.
>> 
>> For a while we went with ACPI power management timer to measure the 
>> frequency, but as modern Intel CPUs support CPUID 15H leaf 
>> (CPUID_TIME_STAMP_COUNTER) we try to use where possible for better accuracy. 
>> The issue with this CPUID leaf is that the crystal clock frequency returned 
>> in ECX register is optional and therefore can be 0. Intel SDM suggests to 
>> use a static value in this case[2], but it is completely opaque on how to 
>> match the running CPU with its static value from SDM.
>> 
>> Initially we went with CPUID model checking, but this failed badly for Xeon 
>> Scalable and Xeon W, as they share the CPUID (06_55H) but have different 
>> crystal clock frequencies (25 MHz vs 24 MHz accordingly). Donald Kuo gave a 
>> good hint in the previous thread that client CPUs usually get 24 MHz crystal 
>> clock, server CPUs have 25, and Atoms have 19.2. This, however, does not 
>> make the situation easier as we do not see a way to determine CPU vertical 
>> segment without e.g. parsing the CPUID brand string.
>> 
>> Apparently, we are not alone, and different open-source operating systems 
>> have different workarounds to this issue. For example, Linux kernel went 
>> with using marketing frequency from CPUID 16H leaf 
>> (CPUID_PROCESSOR_FREQUENCY)[3], and BSD flavours fallback to older metho

Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf

[Vitaly Cheptsov via Groups.Io]

Donald, Michael,

Yes, I see that such a use is quite new and unexpected very well by know. I 
expanded my thoughts in a separate e-mail thread[1] as the consideration opened 
up an apparently separate problem partially related to the patch. Perhaps, we 
could continue the discussion there some time later.

Best regards,
Vitaly

[1] Determining TSC frequency programmatically
https://edk2.groups.io/g/devel/topic/determining_tsc_frequency/32891598?p=,,,100,0,0,0::recentpostdate%2Fsticky,,,100,2,0,32891598

> 16 авг. 2019 г., в 9:56, Kuo, Donald  написал(а):
> 
> Hi Vitaly,
>
> UEFI Application does be another scope. And regards your question on “a way 
> to dynamically determine the difference between Xeon client and server” … is 
> not in current design-in, for long term goal we could consider if there is 
> proper way to identify CPU SKU dynamically.
>
> Thanks for sharing your thought and it could be an enhancement on TimerLib in 
> the future. J
>
> Thanks,
> Donald
>   <>
>  <>From: Kinney, Michael D 
> Sent: Friday, August 16, 2019 12:23 AM
> To: devel@edk2.groups.io; vit9...@protonmail.com; Kuo, Donald 
> ; Kinney, Michael D 
> Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by 
> using CPUID(0x15) TSC leaf
>
> Vitaly,
>
> When implementing a UEFI Application, if you want maximum compatibility, you 
> should use UEFI Services/Protocols and minimize as many HW assumptions as 
> possible.
>
> I understand, especially for accurate time and clock related services, the 
> that the UEFI Specification defined Services/Protocols can be challenging to 
> use.
>
> When adding content that uses HW such as TSC or CPUID the UEFI App should be 
> implemented with good detection logic to know it is safe to use that HW, and 
> contain the fallback logic to use an alternate mechanism if the required HW 
> is not present.  This is a very different approach than some early FW 
> initialization code that can safely make some HW assumptions that helps keep 
> the FW init code small and efficient.  This does imply that different 
> libraries may be required for FW init and UEFI Applications.
>
> Thanks,
>
> Mike
>
> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> 
> [mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>] On Behalf Of 
> Vitaly Cheptsov via Groups.Io
> Sent: Thursday, August 15, 2019 5:10 AM
> To: Kuo, Donald mailto:donald@intel.com>>
> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by 
> using CPUID(0x15) TSC leaf
>
> Hi Donald,
> 
> Glad to hear it helped a little, and sorry for some outdated quotes.
> 
> Your clarification regarding model range is very helpful. Xeon W being client 
> and thus having client clock makes sense, though I must say it was quite not 
> obvious. I was referring to the same SDM table, and it would have been great 
> to have vertical range binding instead of the misleading CPUID.
> 
> With that, however, I still do not see a way to dynamically determine the 
> difference between Xeon client and server.
> 
> For us it is needed as we use TimerLib differently. For you TimerLib is a 
> part of BSP, so you face no issue statically setting the clock frequency as a 
> PCD. For us TimerLib is used as a part of an UEFI application compatible with 
> different hardware, and in fact not just Intel. We have such "generic 
> TimerLib" library, and to determine CPU frequency, as a fallback to CPUID 
> 15H, it relies on ACPI PM timer. This is not great for two reasons:
> - we have to maintain it ourselves, while we would have liked that be part of 
> EDK II with different vendors contributing to the project.
> - we still cannot find an obvious way to determine crystal clock when it is 
> not reported, in particular for Xeon W and Xeon Scalable case.
> 
> I guess this is now out of the scope of this patch and perhaps even this 
> exact library, but it will be helpful to hear relevant technical details on 
> the issue and opinions on such "generic TimerLib" library to later appear in 
> EDK II.
> 
> Best regards,
> Vitaly
> 
> 15 авг. 2019 г., в 11:40, Kuo, Donald  <mailto:donald@intel.com>> написал(а):
>
> Hi Vitaly,
>
> Appreciated for reviewing very detail. And looks your captured some piece of 
> code is older version. And should be ok, I do got your point.
>
> Item #1
> -  I do missed RegEax error handling in case for 
> CpuidCoreClockCalculateTscFrequency () should return 0 and EFI_UNSUPPORTED. 
> AR: Will update it.
>
> Item #2:
> -  Actually the information is from SDM Table 18-85
> o   As

Re: [edk2-devel] [PATCH v3 1/1] MdePkg: Add STATIC_ASSERT macro

[Vitaly Cheptsov via Groups.Io]

Liming, sounds great, thank you.

> 9 сент. 2019 г., в 10:57, Gao, Liming  написал(а):
> 
> Vitaly:
>   I see https://edk2.groups.io/g/devel/message/45743 
> <https://edk2.groups.io/g/devel/message/45743> and 
> https://edk2.groups.io/g/devel/message/45691 
> <https://edk2.groups.io/g/devel/message/45691> gives RB.
>
>   If no other comments, I will push this change this week.
>
> Thanks
> Liming
> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> 
> [mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>] On Behalf Of 
> Vitaly Cheptsov via Groups.Io
> Sent: Sunday, September 08, 2019 12:31 AM
> To: vit9696 mailto:vit9...@protonmail.com>>; 
> devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Subject: Re: [edk2-devel] [PATCH v3 1/1] MdePkg: Add STATIC_ASSERT macro
>
> Hello,
> 
> Given that new EDK II stable has already landed, and it was suggested to 
> merge this right afterwards, it seems to me about the time to merge this 
> patch.
> 
> Best wishes,
> Vitaly 
> 


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

View/Reply Online (#47053): https://edk2.groups.io/g/devel/message/47053
Mute This Topic: https://groups.io/mt/32917749/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [PATCH v3 1/1] MdePkg: Add STATIC_ASSERT macro

[Vitaly Cheptsov via Groups.Io]

Hello,

Given that new EDK II stable has already landed, and it was suggested to merge 
this right afterwards, it seems to me about the time to merge this patch.

Best wishes,
Vitaly

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

View/Reply Online (#47003): https://edk2.groups.io/g/devel/message/47003
Mute This Topic: https://groups.io/mt/32917749/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 092/105] .mailmap: Add an entry for Vitaly Cheptsov

[Vitaly Cheptsov via Groups.Io]

Reviewed-by: Vitaly Cheptsov 
В пт, дек. 6, 2019 в 14:26, Philippe Mathieu-Daude  пишет:

> We use .mailmap to display contributors email addresses in an
> uniform format.
>
> Add an entry for Vitaly Cheptsov to have his name and email address
> displayed properly in the git history.
>
> Cc: Vitaly Cheptsov 
> Signed-off-by: Philippe Mathieu-Daude 
> ---
> [Due to MTA restricting the recipient list to 100, I can not Cc all the
> named developers in the cover. Therefore I'm adapting the explaination
> from the cover in each patch]
>
> This patch won't get merged if Vitaly Cheptsov doesn't give his
> approval, by replying to this patch with:
> Reviewed-by: Vitaly Cheptsov 
>
> If you think this patch is inappropriate, you don't need to justify,
> reply with:
> NAcked-by: Vitaly Cheptsov 
> or simply:
> NACK
>
> If your Firstname Lastname order is incorrect, tell me and I will fix it.
>
> You can also ignore this mail, but I might resend it and keep bothering
> you.
>
> Regards,
>
> Phil.
> ---
> .mailmap | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/.mailmap b/.mailmap
> index 9ff770c545bc..f2bc63d7b9ed 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -176,3 +176,4 @@ Ting Ye  
> 
> Ting Ye  
> Tomas Pilar 
> Vanguput Narendra 
> +Vitaly Cheptsov  Vitaly Cheptsov via Groups.Io 
> 
> --
> 2.21.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#51947): https://edk2.groups.io/g/devel/message/51947
Mute This Topic: https://groups.io/mt/67468446/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

Liming, Jiewen, 

I am personally fine to resubmit the patch changing the defaults to TRUE, but 
does it actually make sense in any other environment but some special testing 
platform? I cannot imagine any production platform that would need it enabled, 
the only use case is to perform analysis on the trusted data usage or something 
similar, as I explained on BZ.

As for assertions, I would expect that all PCDs we introduce are meant to 
control unexpected ASSERT behaviour. I.e. where ASSERTs are potentially used to 
signalise about issues coming not only from trusted sources (usage contract 
violation) but also from untrusted sources (external data). Such assertions 
cannot be used in production environments, as they may break software, and thus 
we have to implement code to disable them.

Best regards,
Vitaly

> 21 окт. 2019 г., в 7:28, Yao, Jiewen  написал(а):
> 
> 
> Hi Mike
> I remember we have discussed it before.
> 
> The general concern is that: how many additional PCD we need introduce, to 
> control different ASSERT in different modules ?
> 
> We may want to enable *some* assert in some modules, but disable *some other* 
> assert.
> E.g. the assert for linkedlist in base lib is another example...
> 
> What is your thought?
> 
> Thank you
> Yao Jiewen
> 
>> -Original Message-
>> From: Gao, Liming 
>> Sent: Monday, October 21, 2019 11:17 AM
>> To: devel@edk2.groups.io; vit9...@protonmail.com
>> Cc: Yao, Jiewen ; Wang, Jian J ;
>> Gao, Liming ; Kinney, Michael D
>> 
>> Subject: RE: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe 
>> string
>> constraint assertions
>> 
>> Include more people.
>> 
>> Basically, to keep the compatible behavior, PcdAssertOnSafeStringConstraints
>> default value should be TRUE.
>> The different platform can configure it.
>> 
>> Thanks
>> Liming
>>> -Original Message-
>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>>> Vitaly Cheptsov via Groups.Io
>>> Sent: Sunday, October 20, 2019 9:06 PM
>>> To: devel@edk2.groups.io
>>> Subject: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe string
>>> constraint assertions
>>> 
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054
>>> 
>>> Runtime data checks are not meant to cause debug assertions
>>> unless explicitly needed by some debug code (thus the PCD)
>>> as this breaks debug builds validating data with BaseLib.
>>> 
>>> Signed-off-by: Vitaly Cheptsov >
>>> ---
>>> MdePkg/MdePkg.dec   |  6 ++
>>> MdePkg/Library/BaseLib/BaseLib.inf  | 11 ++-
>>> MdePkg/Library/BaseLib/SafeString.c |  4 +++-
>>> MdePkg/MdePkg.uni   |  6 ++
>>> 4 files changed, 21 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
>>> index 3fd7d1634c..dda2cdf401 100644
>>> --- a/MdePkg/MdePkg.dec
>>> +++ b/MdePkg/MdePkg.dec
>>> @@ -2221,6 +2221,12 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
>>>  # @Prompt Memory Address of GuidedExtractHandler Table.
>>> 
>>> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|0x1
>>> 00|UINT64|0x30001015
>>> 
>>> +  ## Indicates if safe string constraint violation should assert.
>>> +  #   TRUE  - Safe string constraint violation causes assertion.
>>> +  #   FALSE - Safe string constraint violation does not cause 
>>> assertion.
>>> +  # @Prompt Enable safe string constraint violation assertions.
>>> +
>>> gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints|FALSE|BOOL
>>> EAN|0x002e
>>> +
>>> [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>>>  ## This value is used to set the base address of PCI express hierarchy.
>>>  # @Prompt PCI Express Base Address.
>>> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf
>>> b/MdePkg/Library/BaseLib/BaseLib.inf
>>> index 3586beb0ab..bc98bc6134 100644
>>> --- a/MdePkg/Library/BaseLib/BaseLib.inf
>>> +++ b/MdePkg/Library/BaseLib/BaseLib.inf
>>> @@ -390,11 +390,12 @@ [LibraryClasses]
>>>  BaseMemoryLib
>>> 
>>> [Pcd]
>>> -  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength  ##
>>> SOMETIMES_CONSUMES
>>> -  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength ##
>>> SOMETIMES_CONSUMES
>>> -  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength   ##

Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

Jiewen,

Your explanation makes good sense in the context of "This API is NOT design to 
handle untrusted input.", however, I believe this is not how it is should work 
or at least this is not how we would like it to behave.

In Unix world there are similar hardened interfaces, for example, strlcpy or 
strlcat[1]. These interfaces behave quite similar to what you have in BaseLib 
SafeString, but in the first place they are meant to handle untrusted input. To 
my experience this situation happens no less often (and in fact much more) than 
the need to do extra error checking in trusted code, and I expect EDK2 to 
follow suit. I.e. implement something that can be used not only to check for 
programmer errors within the module, but also perform the validation of 
external data. After all, if these were just the assertions, I would have 
expected for return value to be void not RETURN_STATUS.

If we consider all the options we need to do either of the following:
- reimplement most of the functions in the caller code, which is non-trivial, 
increases code size, reduces readability, is more prone to errors, makes 
reviewing more time consuming, etc.
- reimplement all these functions in a separate library, which solves some of 
the issues, but still increases code size and results in separate interfaces 
with different contracts
- add a pcd to disable assertions, which will disable the debugging handlers 
and will effectively make these functions behave as runtime checks for 
untrusted data, numerous people me included expect them do.

Last suggestion makes most sense to me. Therefore, I believe that even in the 
situation we have different opinions on whether this should be on by default, 
we should at least see the benefit for having an option to make this 
configurable in other projects. In this case I can update the patch in the next 
days to preserve the original behaviour and resubmit it as a v2.

Best wishes,
Vitaly

[1] https://linux.die.net/man/3/strlcpy

> 21 окт. 2019 г., в 11:07, Yao, Jiewen  написал(а):
> 
> 
> Hi Vitaly
> We have discussed the ASSERT usage when we added the first version of code.
> 
> C11 standard supports runtime violation handler registration.
> We investigated the behavior of
> (1) MS Visual Studio - http://msdn.microsoft.com/en-us/library/bb288454.aspx
> (2) SafeCLib - http://sourceforge.net/projects/safeclib/
> (3) Slibc - https://code.google.com/p/slibc/
> 
> The default behavior is:
> (1) Call Debugger
> (2) Print error
> (3) Call abort()
> 
> As conclusion, we believe it is *caller's responsibility* to make sure the 
> caller inputs right parameter, instead of let callee check and return error.
> 
> In order to catch such error as early as possible, we decide to use ASSERT, 
> because it is something never happen. (We still use error handling followed 
> by to handle the release build.)
> 
> This API is NOT design to handle untrusted input. As such, we believe it is 
> expected ASSERT behavior.
> 
> 
> We have other debate, such as if we need ASSERT 2 bytes alignment CHAR16 
> process, or if we need ASSERT address alignment for MMIO access.
> Per our experience, it is much better to let caller guarantee that, instead 
> of callee check that. And ASSERT is a good way to catch the issue as early as 
> possible.
> 
> 
> 
> Thank you
> Yao Jiewen
> 
>> -Original Message-
>> From: devel@edk2.groups.io  On Behalf Of Vitaly
>> Cheptsov via Groups.Io
>> Sent: Monday, October 21, 2019 3:28 PM
>> To: Yao, Jiewen ; Gao, Liming 
>> Cc: devel@edk2.groups.io; Wang, Jian J ; Kinney,
>> Michael D 
>> Subject: Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe 
>> string
>> constraint assertions
>> 
>> Liming, Jiewen,
>> 
>> I am personally fine to resubmit the patch changing the defaults to TRUE, but
>> does it actually make sense in any other environment but some special testing
>> platform? I cannot imagine any production platform that would need it 
>> enabled,
>> the only use case is to perform analysis on the trusted data usage or 
>> something
>> similar, as I explained on BZ.
>> 
>> As for assertions, I would expect that all PCDs we introduce are meant to
>> control unexpected ASSERT behaviour. I.e. where ASSERTs are potentially used
>> to signalise about issues coming not only from trusted sources (usage 
>> contract
>> violation) but also from untrusted sources (external data). Such assertions
>> cannot be used in production environments, as they may break software, and
>> thus we have to implement code to disable them.
>> 
>> Best regards,
>> Vitaly
>> 
>>> 21 окт. 2019 г., в 7:28, Yao, Jiewen  написал(а):
>>> 
>>> 

Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

;> >
>> >
>> > Hi Vitaly
>> > We have discussed the ASSERT usage when we added the first version of code.
>> >
>> > C11 standard supports runtime violation handler registration.
>> > We investigated the behavior of
>> > (1) MS Visual Studio - 
>> > http://msdn.microsoft.com/en-us/library/bb288454.aspx
>> > (2) SafeCLib - http://sourceforge.net/projects/safeclib/
>> > (3) Slibc - https://code.google.com/p/slibc/
>> >
>> > The default behavior is:
>> > (1) Call Debugger
>> > (2) Print error
>> > (3) Call abort()
>> >
>> > As conclusion, we believe it is *caller's responsibility* to make sure the 
>> > caller
>> inputs right parameter, instead of let callee check and return error.
>> >
>> > In order to catch such error as early as possible, we decide to use ASSERT,
>> because it is something never happen. (We still use error handling followed 
>> by to
>> handle the release build.)
>> >
>> > This API is NOT design to handle untrusted input. As such, we believe it is
>> expected ASSERT behavior.
>> >
>> >
>> > We have other debate, such as if we need ASSERT 2 bytes alignment CHAR16
>> process, or if we need ASSERT address alignment for MMIO access.
>> > Per our experience, it is much better to let caller guarantee that, 
>> > instead of
>> callee check that. And ASSERT is a good way to catch the issue as early as
>> possible.
>> >
>> >
>> >
>> > Thank you
>> > Yao Jiewen
>> >
>> >> -Original Message-
>> >> From: devel@edk2.groups.io  On Behalf Of Vitaly
>> >> Cheptsov via Groups.Io
>> >> Sent: Monday, October 21, 2019 3:28 PM
>> >> To: Yao, Jiewen ; Gao, Liming
>> 
>> >> Cc: devel@edk2.groups.io; Wang, Jian J ; Kinney,
>> >> Michael D 
>> >> Subject: Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe
>> string
>> >> constraint assertions
>> >>
>> >> Liming, Jiewen,
>> >>
>> >> I am personally fine to resubmit the patch changing the defaults to TRUE, 
>> >> but
>> >> does it actually make sense in any other environment but some special
>> testing
>> >> platform? I cannot imagine any production platform that would need it
>> enabled,
>> >> the only use case is to perform analysis on the trusted data usage or
>> something
>> >> similar, as I explained on BZ.
>> >>
>> >> As for assertions, I would expect that all PCDs we introduce are meant to
>> >> control unexpected ASSERT behaviour. I.e. where ASSERTs are potentially
>> used
>> >> to signalise about issues coming not only from trusted sources (usage
>> contract
>> >> violation) but also from untrusted sources (external data). Such 
>> >> assertions
>> >> cannot be used in production environments, as they may break software, and
>> >> thus we have to implement code to disable them.
>> >>
>> >> Best regards,
>> >> Vitaly
>> >>
>> >>> 21 окт. 2019 г., в 7:28, Yao, Jiewen  написал(а):
>> >>>
>> >>>
>> >>> Hi Mike
>> >>> I remember we have discussed it before.
>> >>>
>> >>> The general concern is that: how many additional PCD we need introduce,
>> to
>> >> control different ASSERT in different modules ?
>> >>>
>> >>> We may want to enable *some* assert in some modules, but disable *some
>> >> other* assert.
>> >>> E.g. the assert for linkedlist in base lib is another example...
>> >>>
>> >>> What is your thought?
>> >>>
>> >>> Thank you
>> >>> Yao Jiewen
>> >>>
>> >>>> -Original Message-
>> >>>> From: Gao, Liming 
>> >>>> Sent: Monday, October 21, 2019 11:17 AM
>> >>>> To: devel@edk2.groups.io; vit9...@protonmail.com
>> >>>> Cc: Yao, Jiewen ; Wang, Jian J
>> >> ;
>> >>>> Gao, Liming ; Kinney, Michael D
>> >>>> 
>> >>>> Subject: RE: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe
>> >> string
>> >>>> constraint assertions
>> >>>>
>> >>>> Include more people.
>> >>>>
>> >>&

[edk2-devel] [PATCH v2 0/1] MdePkg: Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

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

Requesting for merge in edk2-stable2019011.

Vitaly Cheptsov (1):
  MdePkg: Add PCD to disable safe string constraint assertions

 MdePkg/MdePkg.dec   |  6 ++
 MdePkg/Library/BaseLib/BaseLib.inf  | 11 ++-
 MdePkg/Library/BaseLib/SafeString.c |  4 +++-
 MdePkg/MdePkg.uni   |  6 ++
 4 files changed, 21 insertions(+), 6 deletions(-)

-- 
2.21.0 (Apple Git-122)


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

View/Reply Online (#49328): https://edk2.groups.io/g/devel/message/49328
Mute This Topic: https://groups.io/mt/36392293/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

[edk2-devel] [PATCH v2 1/1] MdePkg: Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

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

Runtime data checks are not meant to cause debug assertions
unless explicitly needed by some debug code (thus the PCD)
as this breaks debug builds validating data with BaseLib.

Signed-off-by: Vitaly Cheptsov >
---
 MdePkg/MdePkg.dec   |  6 ++
 MdePkg/Library/BaseLib/BaseLib.inf  | 11 ++-
 MdePkg/Library/BaseLib/SafeString.c |  4 +++-
 MdePkg/MdePkg.uni   |  6 ++
 4 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 3fd7d1634c..c1671333f6 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2221,6 +2221,12 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
   # @Prompt Memory Address of GuidedExtractHandler Table.
   
gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|0x100|UINT64|0x30001015
 
+  ## Indicates if safe string constraint violation should assert.
+  #   TRUE  - Safe string constraint violation causes assertion.
+  #   FALSE - Safe string constraint violation does not cause assertion.
+  # @Prompt Enable safe string constraint violation assertions.
+  
gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints|TRUE|BOOLEAN|0x002e
+
 [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## This value is used to set the base address of PCI express hierarchy.
   # @Prompt PCI Express Base Address.
diff --git a/MdePkg/Library/BaseLib/BaseLib.inf 
b/MdePkg/Library/BaseLib/BaseLib.inf
index 3586beb0ab..bc98bc6134 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -390,11 +390,12 @@ [LibraryClasses]
   BaseMemoryLib
 
 [Pcd]
-  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength  ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength   ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask   ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType   ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints   ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength  ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask  ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType  ## 
SOMETIMES_CONSUMES
 
 [FeaturePcd]
   gEfiMdePkgTokenSpaceGuid.PcdVerifyNodeInList  ## CONSUMES
diff --git a/MdePkg/Library/BaseLib/SafeString.c 
b/MdePkg/Library/BaseLib/SafeString.c
index 7dc03d2caa..56b5e34a8d 100644
--- a/MdePkg/Library/BaseLib/SafeString.c
+++ b/MdePkg/Library/BaseLib/SafeString.c
@@ -14,7 +14,9 @@
 
 #define SAFE_STRING_CONSTRAINT_CHECK(Expression, Status)  \
   do { \
-ASSERT (Expression); \
+if (PcdGetBool (PcdAssertOnSafeStringConstraints)) { \
+  ASSERT (Expression); \
+} \
 if (!(Expression)) { \
   return Status; \
 } \
diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
index 5c1fa24065..425b66bb43 100644
--- a/MdePkg/MdePkg.uni
+++ b/MdePkg/MdePkg.uni
@@ -287,6 +287,12 @@
 
 #string STR_gEfiMdePkgTokenSpaceGuid_PcdGuidedExtractHandlerTableAddress_HELP  
#language en-US "This value is used to set the available memory address to 
store Guided Extract Handlers. The required memory space is decided by the 
value of PcdMaximumGuidedExtractHandler."
 
+#string STR_gEfiMdePkgTokenSpaceGuid_PcdAssertOnSafeStringConstraints_PROMPT  
#language en-US "Enable safe string constraint violation assertions"
+
+#string STR_gEfiMdePkgTokenSpaceGuid_PcdAssertOnSafeStringConstraints_HELP  
#language en-US "Indicates if safe string constraint violation should 
assert.\n"
+   
"TRUE  - Safe string constraint violation causes assertion.\n"
+   
"FALSE - Safe string constraint violation does not cause assertion."
+
 #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_PROMPT  
#language en-US "PCI Express Base Address"
 
 #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_HELP  #language 
en-US "This value is used to set the base address of PCI express hierarchy."
-- 
2.21.0 (Apple Git-122)


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

View/Reply Online (#49327): https://edk2.groups.io/g/devel/message/49327
Mute This Topic: https://groups.io/mt/36392292/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

[edk2-devel] [PATCH v3 0/1] MdePkg: Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

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

Requesting for merge in edk2-stable2019011.

Changes since V1:
- Enable assertions by default to preserve the original behaviour
- Fix bugzilla reference link

Vitaly Cheptsov (1):
  MdePkg: Add PCD to disable safe string constraint assertions

 MdePkg/MdePkg.dec   |  6 ++
 MdePkg/Library/BaseLib/BaseLib.inf  | 11 ++-
 MdePkg/Library/BaseLib/SafeString.c |  4 +++-
 MdePkg/MdePkg.uni   |  6 ++
 4 files changed, 21 insertions(+), 6 deletions(-)

-- 
2.21.0 (Apple Git-122)


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

View/Reply Online (#49329): https://edk2.groups.io/g/devel/message/49329
Mute This Topic: https://groups.io/mt/36392350/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

[edk2-devel] [PATCH v3 1/1] MdePkg: Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

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

Runtime data checks are not meant to cause debug assertions
unless explicitly needed by some debug code (thus the PCD)
as this breaks debug builds validating data with BaseLib.

Signed-off-by: Vitaly Cheptsov >
---
 MdePkg/MdePkg.dec   |  6 ++
 MdePkg/Library/BaseLib/BaseLib.inf  | 11 ++-
 MdePkg/Library/BaseLib/SafeString.c |  4 +++-
 MdePkg/MdePkg.uni   |  6 ++
 4 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 3fd7d1634c..c1671333f6 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2221,6 +2221,12 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
   # @Prompt Memory Address of GuidedExtractHandler Table.
   
gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|0x100|UINT64|0x30001015
 
+  ## Indicates if safe string constraint violation should assert.
+  #   TRUE  - Safe string constraint violation causes assertion.
+  #   FALSE - Safe string constraint violation does not cause assertion.
+  # @Prompt Enable safe string constraint violation assertions.
+  
gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints|TRUE|BOOLEAN|0x002e
+
 [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## This value is used to set the base address of PCI express hierarchy.
   # @Prompt PCI Express Base Address.
diff --git a/MdePkg/Library/BaseLib/BaseLib.inf 
b/MdePkg/Library/BaseLib/BaseLib.inf
index 3586beb0ab..bc98bc6134 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -390,11 +390,12 @@ [LibraryClasses]
   BaseMemoryLib
 
 [Pcd]
-  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength  ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength   ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask   ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType   ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints   ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength  ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask  ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType  ## 
SOMETIMES_CONSUMES
 
 [FeaturePcd]
   gEfiMdePkgTokenSpaceGuid.PcdVerifyNodeInList  ## CONSUMES
diff --git a/MdePkg/Library/BaseLib/SafeString.c 
b/MdePkg/Library/BaseLib/SafeString.c
index 7dc03d2caa..56b5e34a8d 100644
--- a/MdePkg/Library/BaseLib/SafeString.c
+++ b/MdePkg/Library/BaseLib/SafeString.c
@@ -14,7 +14,9 @@
 
 #define SAFE_STRING_CONSTRAINT_CHECK(Expression, Status)  \
   do { \
-ASSERT (Expression); \
+if (PcdGetBool (PcdAssertOnSafeStringConstraints)) { \
+  ASSERT (Expression); \
+} \
 if (!(Expression)) { \
   return Status; \
 } \
diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
index 5c1fa24065..425b66bb43 100644
--- a/MdePkg/MdePkg.uni
+++ b/MdePkg/MdePkg.uni
@@ -287,6 +287,12 @@
 
 #string STR_gEfiMdePkgTokenSpaceGuid_PcdGuidedExtractHandlerTableAddress_HELP  
#language en-US "This value is used to set the available memory address to 
store Guided Extract Handlers. The required memory space is decided by the 
value of PcdMaximumGuidedExtractHandler."
 
+#string STR_gEfiMdePkgTokenSpaceGuid_PcdAssertOnSafeStringConstraints_PROMPT  
#language en-US "Enable safe string constraint violation assertions"
+
+#string STR_gEfiMdePkgTokenSpaceGuid_PcdAssertOnSafeStringConstraints_HELP  
#language en-US "Indicates if safe string constraint violation should 
assert.\n"
+   
"TRUE  - Safe string constraint violation causes assertion.\n"
+   
"FALSE - Safe string constraint violation does not cause assertion."
+
 #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_PROMPT  
#language en-US "PCI Express Base Address"
 
 #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_HELP  #language 
en-US "This value is used to set the base address of PCI express hierarchy."
-- 
2.21.0 (Apple Git-122)


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

View/Reply Online (#49330): https://edk2.groups.io/g/devel/message/49330
Mute This Topic: https://groups.io/mt/36392351/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [PATCH v3 1/1] MdePkg: Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

I agree that we want this PCD mentioned in BaseLib header, but most likely it 
should be done in a shorter form.

What about something like this?

  Unless PcdAssertOnSafeStringConstraints is set to FALSE:
  If String is NULL, then ASSERT().
  If Data is NULL, then ASSERT().
  If String is not aligned in a 16-bit boundary, then ASSERT().
  If PcdMaximumUnicodeStringLength is not zero, and String contains more than
  PcdMaximumUnicodeStringLength Unicode characters, not including the
  Null-terminator, then ASSERT().

Best wishes,
Vitaly

> 22 окт. 2019 г., в 10:52, Yao, Jiewen  написал(а):
> 
> 
> In BaseLib.h, we have below comments in each safe string function header.
> ===
>  If an error would be returned, then the function will also ASSERT().
> ===
> 
> As I mentioned earlier, the original purpose is to let the caller guarantee 
> the correctness of the input. Similar to Microsoft Visual Studio strcpy_s() 
> behavior.
> 
> 
> If we decide to change the purpose and change the design, I recommend we add 
> clarification in the function header as well.
> 
> For example:
> ===
> The PcdAssertOnSafeStringConstraints is used to control the behavior of the 
> runtime violation.
> When PcdAssertOnSafeStringConstraints is TRUE, if an error would be returned, 
> then the function will also ASSERT().
> When PcdAssertOnSafeStringConstraints is FALSE, if an error would be 
> returned, then the function will NOT ASSERT().
> ===
> 
> 
> Hi Michael D Kinney
> Do you have some comment?
> 
> Thank you
> Yao Jiewen
> 
> 
>> -Original Message-
>> From: devel@edk2.groups.io  On Behalf Of Vitaly
>> Cheptsov via Groups.Io
>> Sent: Tuesday, October 22, 2019 3:20 PM
>> To: devel@edk2.groups.io
>> Subject: [edk2-devel] [PATCH v3 1/1] MdePkg: Add PCD to disable safe string
>> constraint assertions
>> 
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054
>> 
>> Runtime data checks are not meant to cause debug assertions
>> unless explicitly needed by some debug code (thus the PCD)
>> as this breaks debug builds validating data with BaseLib.
>> 
>> Signed-off-by: Vitaly Cheptsov >
>> ---
>> MdePkg/MdePkg.dec   |  6 ++
>> MdePkg/Library/BaseLib/BaseLib.inf  | 11 ++-
>> MdePkg/Library/BaseLib/SafeString.c |  4 +++-
>> MdePkg/MdePkg.uni   |  6 ++
>> 4 files changed, 21 insertions(+), 6 deletions(-)
>> 
>> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
>> index 3fd7d1634c..c1671333f6 100644
>> --- a/MdePkg/MdePkg.dec
>> +++ b/MdePkg/MdePkg.dec
>> @@ -2221,6 +2221,12 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
>>   # @Prompt Memory Address of GuidedExtractHandler Table.
>> 
>> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|0x100
>> |UINT64|0x30001015
>> 
>> +  ## Indicates if safe string constraint violation should assert.
>> +  #   TRUE  - Safe string constraint violation causes assertion.
>> +  #   FALSE - Safe string constraint violation does not cause assertion.
>> +  # @Prompt Enable safe string constraint violation assertions.
>> +
>> gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints|TRUE|BOOLEA
>> N|0x002e
>> +
>> [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>>   ## This value is used to set the base address of PCI express hierarchy.
>>   # @Prompt PCI Express Base Address.
>> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf
>> b/MdePkg/Library/BaseLib/BaseLib.inf
>> index 3586beb0ab..bc98bc6134 100644
>> --- a/MdePkg/Library/BaseLib/BaseLib.inf
>> +++ b/MdePkg/Library/BaseLib/BaseLib.inf
>> @@ -390,11 +390,12 @@ [LibraryClasses]
>>   BaseMemoryLib
>> 
>> [Pcd]
>> -  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength  ##
>> SOMETIMES_CONSUMES
>> -  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength ##
>> SOMETIMES_CONSUMES
>> -  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength   ##
>> SOMETIMES_CONSUMES
>> -  gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask   ##
>> SOMETIMES_CONSUMES
>> -  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType   ##
>> SOMETIMES_CONSUMES
>> +  gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints   ##
>> SOMETIMES_CONSUMES
>> +  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength ##
>> SOMETIMES_CONSUMES
>> +  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength##
>> SOMETIMES_CONSUMES
>> +  gEfiMdePkgTokenSpaceGuid.PcdMaximum

[edk2-devel] [PATCH v1 0/1] MdePkg: Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

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

Requesting for merge in edk2-stable2019011.

Vitaly Cheptsov (1):
  MdePkg: Add PCD to disable safe string constraint assertions

 MdePkg/MdePkg.dec   |  6 ++
 MdePkg/Library/BaseLib/BaseLib.inf  | 11 ++-
 MdePkg/Library/BaseLib/SafeString.c |  4 +++-
 MdePkg/MdePkg.uni   |  6 ++
 4 files changed, 21 insertions(+), 6 deletions(-)

-- 
2.21.0 (Apple Git-122)


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

View/Reply Online (#49254): https://edk2.groups.io/g/devel/message/49254
Mute This Topic: https://groups.io/mt/35943316/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

[edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

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

Runtime data checks are not meant to cause debug assertions
unless explicitly needed by some debug code (thus the PCD)
as this breaks debug builds validating data with BaseLib.

Signed-off-by: Vitaly Cheptsov >
---
 MdePkg/MdePkg.dec   |  6 ++
 MdePkg/Library/BaseLib/BaseLib.inf  | 11 ++-
 MdePkg/Library/BaseLib/SafeString.c |  4 +++-
 MdePkg/MdePkg.uni   |  6 ++
 4 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 3fd7d1634c..dda2cdf401 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2221,6 +2221,12 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
   # @Prompt Memory Address of GuidedExtractHandler Table.
   
gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|0x100|UINT64|0x30001015
 
+  ## Indicates if safe string constraint violation should assert.
+  #   TRUE  - Safe string constraint violation causes assertion.
+  #   FALSE - Safe string constraint violation does not cause assertion.
+  # @Prompt Enable safe string constraint violation assertions.
+  
gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints|FALSE|BOOLEAN|0x002e
+
 [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## This value is used to set the base address of PCI express hierarchy.
   # @Prompt PCI Express Base Address.
diff --git a/MdePkg/Library/BaseLib/BaseLib.inf 
b/MdePkg/Library/BaseLib/BaseLib.inf
index 3586beb0ab..bc98bc6134 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -390,11 +390,12 @@ [LibraryClasses]
   BaseMemoryLib
 
 [Pcd]
-  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength  ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength   ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask   ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType   ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints   ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength  ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask  ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType  ## 
SOMETIMES_CONSUMES
 
 [FeaturePcd]
   gEfiMdePkgTokenSpaceGuid.PcdVerifyNodeInList  ## CONSUMES
diff --git a/MdePkg/Library/BaseLib/SafeString.c 
b/MdePkg/Library/BaseLib/SafeString.c
index 7dc03d2caa..56b5e34a8d 100644
--- a/MdePkg/Library/BaseLib/SafeString.c
+++ b/MdePkg/Library/BaseLib/SafeString.c
@@ -14,7 +14,9 @@
 
 #define SAFE_STRING_CONSTRAINT_CHECK(Expression, Status)  \
   do { \
-ASSERT (Expression); \
+if (PcdGetBool (PcdAssertOnSafeStringConstraints)) { \
+  ASSERT (Expression); \
+} \
 if (!(Expression)) { \
   return Status; \
 } \
diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
index 5c1fa24065..425b66bb43 100644
--- a/MdePkg/MdePkg.uni
+++ b/MdePkg/MdePkg.uni
@@ -287,6 +287,12 @@
 
 #string STR_gEfiMdePkgTokenSpaceGuid_PcdGuidedExtractHandlerTableAddress_HELP  
#language en-US "This value is used to set the available memory address to 
store Guided Extract Handlers. The required memory space is decided by the 
value of PcdMaximumGuidedExtractHandler."
 
+#string STR_gEfiMdePkgTokenSpaceGuid_PcdAssertOnSafeStringConstraints_PROMPT  
#language en-US "Enable safe string constraint violation assertions"
+
+#string STR_gEfiMdePkgTokenSpaceGuid_PcdAssertOnSafeStringConstraints_HELP  
#language en-US "Indicates if safe string constraint violation should 
assert.\n"
+   
"TRUE  - Safe string constraint violation causes assertion.\n"
+   
"FALSE - Safe string constraint violation does not cause assertion."
+
 #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_PROMPT  
#language en-US "PCI Express Base Address"
 
 #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_HELP  #language 
en-US "This value is used to set the base address of PCI express hierarchy."
-- 
2.21.0 (Apple Git-122)


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

View/Reply Online (#49255): https://edk2.groups.io/g/devel/message/49255
Mute This Topic: https://groups.io/mt/35943317/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [PATCH V3 1/2] MdePkg/UefiDevicePathLib: Separate the device path lib

[Vitaly Cheptsov via Groups.Io]

Reviewed-by: Vitaly Cheptsov 

On Wed, Dec 18, 2019 at 05:10, Zhichao Gao  wrote:

> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2298
>
> UefiDevicePathLibOptionalDevicePathProtocol's implementation isn't
> fit its description. It should be implement as blow:
> Try to find the DevicePathProtocol, if found then use it to implement
> the interface. Else, use the local interface. It should not have the
> depex and ASSERT of gEfiDevicePathUtilitiesProtocolGuid when not find
> the DevicePathProtocol.
>
> Add a mandatory one to force using the DevicePathUtilities,
> DevicePathToText and DevicePathFromText protocol.
>
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Vitaly Cheptsov 
> Tested-by: Zhichao Gao 
> Signed-off-by: Zhichao Gao 
> ---
>
> V2:
> Optional one should always return EFI_SUCCESS in its constructor.
> Change the description of optional one's uni file.
> Fix the copyright date of mandatory one's uni file.
>
> V3:
> Remove the Status variable in
> UefiDevicePathLibOptionalDevicePathProtocolConstructor.
> The Status would cause GCC build fail because the variable is
> initialized but not used. Since it is useless for the constructor,
> directly remove it.
>
> ...DevicePathLibMandatoryDevicePathProtocol.c | 469 ++
> ...vicePathLibMandatoryDevicePathProtocol.inf | 86 
> ...vicePathLibMandatoryDevicePathProtocol.uni | 18 +
> ...iDevicePathLibOptionalDevicePathProtocol.c | 21 +-
> ...evicePathLibOptionalDevicePathProtocol.inf | 5 +-
> ...evicePathLibOptionalDevicePathProtocol.uni | 6 +-
> 6 files changed, 585 insertions(+), 20 deletions(-)
> create mode 100644 
> MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.c
> create mode 100644 
> MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.inf
> create mode 100644 
> MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.uni
>
> diff --git 
> a/MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.c
>  
> b/MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.c
> new file mode 100644
> index 00..fa27110fd4
> --- /dev/null
> +++ 
> b/MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.c
> @@ -0,0 +1,469 @@
> +/** @file
> + Device Path services. The thing to remember is device paths are built out of
> + nodes. The device path is terminated by an end node that is length
> + sizeof(EFI_DEVICE_PATH_PROTOCOL). That would be why there is 
> sizeof(EFI_DEVICE_PATH_PROTOCOL)
> + all over this file.
> +
> + The only place where multi-instance device paths are supported is in
> + environment varibles. Multi-instance device paths should never be placed
> + on a Handle.
> +
> + Copyright (c) 2019, Intel Corporation. All rights reserved.
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +
> +#include "UefiDevicePathLib.h"
> +
> +GLOBAL_REMOVE_IF_UNREFERENCED EFI_DEVICE_PATH_UTILITIES_PROTOCOL 
> *mDevicePathLibDevicePathUtilities = NULL;
> +GLOBAL_REMOVE_IF_UNREFERENCED EFI_DEVICE_PATH_TO_TEXT_PROTOCOL 
> *mDevicePathLibDevicePathToText = NULL;
> +GLOBAL_REMOVE_IF_UNREFERENCED EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL 
> *mDevicePathLibDevicePathFromText = NULL;
> +
> +/**
> + The constructor function caches the pointer to DevicePathUtilites protocol,
> + DevicePathToText protocol and DevicePathFromText protocol.
> +
> + The constructor function locates these three protocols from protocol 
> database.
> + It will caches the pointer to local protocol instance if that operation 
> fails
> + and it will always return EFI_SUCCESS.
> +
> + @param ImageHandle The firmware allocated handle for the EFI image.
> + @param SystemTable A pointer to the EFI System Table.
> +
> + @retval EFI_SUCCESS The constructor always returns EFI_SUCCESS.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +UefiDevicePathLibMandatoryDevicePathProtocolConstructor (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + EFI_STATUS Status;
> +
> + Status = gBS->LocateProtocol (
> + ,
> + NULL,
> + (VOID**) 
> + );
> + ASSERT_EFI_ERROR (Status);
> + ASSERT (mDevicePathLibDevicePathUtilities != NULL);
> +
> + Status = gBS->LocateProtocol (
> + ,
> + NULL,
> + (VOID**) 
> + );
> + ASSERT_EFI_ERROR (Status);
> + ASSERT (mDevicePathLibDevicePathToText != NULL);
> +
> + Status = gBS->LocateProtocol (
> + ,
> + NULL,
> + (VOID**) 
> + );
> + ASSERT_EFI_ERROR (Status);
> + ASSERT (mDevicePathLibDevicePathFromText != NULL);
> +
> + return Status;
> +}
> +
> +/**
> + Returns the size of a device path in bytes.
> +
> + This function returns the size, in bytes, of the device path data structure
> + specified by DevicePath including the end of device path node.
> + If DevicePath is NULL or invalid, then 0 is returned.
> +
> + @param DevicePath A pointer to a device path data structure.
> +
> + @retval 0 If DevicePath is NULL or invalid.
> + @retval Others The size of a device path in bytes.
> +
> +**/

Re: [edk2-devel] [PATCH V3 0/2] *MdePkg/UefiDevicePathLib: Separate the lib instances

[Vitaly Cheptsov via Groups.Io]

This makes very good sense to me, thank you for taking your time to fix it.
I am slightly unsure whether if checks with subsequent assertions are really 
needed in mandatory version, as asserting in the constructor will trigger 
missing protocol very early anyway, but I do not think it is important.

Best wishes,
Vitaly

On Wed, Dec 18, 2019 at 05:10, Zhichao Gao  wrote:

> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2298
>
> The UefiDevicePathLibOptionalDevicePathProtocolConstructor's implementation
> isn't match with its instance name.
> Remove the ASSERT and depex of the gEfiDevicePathUtilitiesProtocolGuid
> because of "Optional".
>
> Add a mandatory instance to force using the DevicePathUtilities,
> DevicePathToText and DevicePathFromText protocol with the ASSERT
> and depex.
>
> V2:
> The optional lib instance's construction should return success all the
> time.
> Change the desciption of the optional lib uni file.
> Change the copyright date of the mandatory one's uni file.
>
> V3:
> Remove the Status variable in 
> UefiDevicePathLibOptionalDevicePathProtocolConstructor.
> The Status would cause GCC build fail because the variable is initialized but 
> not used.
> Since it is useless for the constructor, directly remove it.
>
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Vitaly Cheptsov 
> Signed-off-by: Zhichao Gao 
>
> Zhichao Gao (2):
> MdePkg/UefiDevicePathLib: Separate the device path lib
> MdePkg/dsc: Add UefiDevicePathLibMandatoryDevicePathProtocol for build
>
> ...DevicePathLibMandatoryDevicePathProtocol.c | 469 ++
> ...vicePathLibMandatoryDevicePathProtocol.inf | 86 
> ...vicePathLibMandatoryDevicePathProtocol.uni | 18 +
> ...iDevicePathLibOptionalDevicePathProtocol.c | 21 +-
> ...evicePathLibOptionalDevicePathProtocol.inf | 5 +-
> ...evicePathLibOptionalDevicePathProtocol.uni | 6 +-
> MdePkg/MdePkg.dsc | 3 +-
> 7 files changed, 587 insertions(+), 21 deletions(-)
> create mode 100644 
> MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.c
> create mode 100644 
> MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.inf
> create mode 100644 
> MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.uni
>
> --
> 2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52334): https://edk2.groups.io/g/devel/message/52334
Mute This Topic: https://groups.io/mt/68779976/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V3 2/2] MdePkg/dsc: Add UefiDevicePathLibMandatoryDevicePathProtocol for build

[Vitaly Cheptsov via Groups.Io]

Reviewed-by: Vitaly Cheptsov 

On Wed, Dec 18, 2019 at 05:10, Zhichao Gao  wrote:

> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2298
>
> Add the new instance lib for build.
>
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Vitaly Cheptsov 
> Reviewed-by: Liming Gao 
> Tested-by: Zhichao Gao 
> Signed-off-by: Zhichao Gao 
> ---
> MdePkg/MdePkg.dsc | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc
> index 0aeafaaacc..9e9813dd27 100644
> --- a/MdePkg/MdePkg.dsc
> +++ b/MdePkg/MdePkg.dsc
> @@ -1,7 +1,7 @@
> ## @file
> # EFI/PI MdePkg Package
> #
> -# Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.
> +# Copyright (c) 2007 - 2019, Intel Corporation. All rights reserved.
> # Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
> #
> # SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -99,6 +99,7 @@
> MdePkg/Library/UefiDebugLibStdErr/UefiDebugLibStdErr.inf
> MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
> MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibOptionalDevicePathProtocol.inf
> + 
> MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.inf
> MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.inf
> MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
> MdePkg/Library/UefiLib/UefiLib.inf
> --
> 2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52336): https://edk2.groups.io/g/devel/message/52336
Mute This Topic: https://groups.io/mt/68779981/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for CLANGPDB

[Vitaly Cheptsov via Groups.Io]

Liming,

We did run several of our projects based on EDK II in X64 mode, DEBUG, RELEASE, 
NOOPT. Noticed no change from XCODE5.

We also tried building several EDK builtin packages like CryptoPkg, MdePkg, 
MdeModulePkg.

Best wishes,
Vitaly

В пн, февр. 10, 2020 в 16:47, Gao, Liming  пишет:

> Vitaly:
> This change is good. Can you your test for it? I verify this patch for Ovmf 
> platform on Windows. It can make ovmf pass build with CLANGPDB.
>
> Thanks
> Liming
>> -Original Message-
>> From: devel@edk2.groups.io  On Behalf Of Vitaly 
>> Cheptsov via Groups.Io
>> Sent: Monday, February 10, 2020 6:59 PM
>> To: devel@edk2.groups.io
>> Subject: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for CLANGPDB
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2397
>>
>> Signed-off-by: Vitaly Cheptsov 
>> ---
>> BaseTools/Conf/tools_def.template | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/BaseTools/Conf/tools_def.template 
>> b/BaseTools/Conf/tools_def.template
>> index feee2bbf16..6bf6c5768e 100755
>> --- a/BaseTools/Conf/tools_def.template
>> +++ b/BaseTools/Conf/tools_def.template
>> @@ -2755,11 +2755,11 @@ RELEASE_CLANG38_AARCH64_DLINK_FLAGS = 
>> DEF(CLANG38_AARCH64_DLINK_FLAGS) -flto -Wl
>> DEFINE CLANGPDB_IA32_PREFIX = ENV(CLANG_BIN)
>> DEFINE CLANGPDB_X64_PREFIX = ENV(CLANG_BIN)
>>
>> -DEFINE CLANGPDB_IA32_TARGET = -target i686-unknown-windows
>> -DEFINE CLANGPDB_X64_TARGET = -target x86_64-unknown-windows
>> +DEFINE CLANGPDB_IA32_TARGET = -target i686-unknown-windows-gnu
>> +DEFINE CLANGPDB_X64_TARGET = -target x86_64-unknown-windows-gnu
>>
>> DEFINE CLANGPDB_WARNING_OVERRIDES = -Wno-parentheses-equality 
>> -Wno-tautological-compare -Wno-tautological-constant-out-
>> of-range-compare -Wno-empty-body -Wno-unused-const-variable -Wno-varargs 
>> -Wno-unknown-warning-option -Wno-microsoft-enum-
>> forward-reference
>> -DEFINE CLANGPDB_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) 
>> DEF(CLANGPDB_WARNING_OVERRIDES) -fno-stack-protector -
>> mms-bitfields -Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas 
>> -Wno-incompatible-library-redeclaration -fno-
>> asynchronous-unwind-tables -mno-implicit-float 
>> -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang -
>> funsigned-char -fno-ms-extensions -Wno-null-dereference -fms-compatibility 
>> -mno-stack-arg-probe
>> +DEFINE CLANGPDB_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) 
>> DEF(CLANGPDB_WARNING_OVERRIDES) -fno-stack-protector -
>> fno-asynchronous-unwind-tables -funsigned-char 
>> -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang -Wno-
>> address -Wno-shift-negative-value -Wno-unknown-pragmas 
>> -Wno-incompatible-library-redeclaration -Wno-null-dereference -mno-
>> implicit-float -mms-bitfields -mno-stack-arg-probe -nostdlib -nostdlibinc
>>
>> ###
>> # CLANGPDB IA32 definitions
>> --
>> 2.21.1 (Apple Git-122.3)
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>>
>> View/Reply Online (#54130): https://edk2.groups.io/g/devel/message/54130
>> Mute This Topic: https://groups.io/mt/71134286/1759384
>> Group Owner: devel+ow...@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [liming@intel.com]
>> -=-=-=-=-=-=
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#54159): https://edk2.groups.io/g/devel/message/54159
Mute This Topic: https://groups.io/mt/71134286/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for CLANGPDB

[Vitaly Cheptsov via Groups.Io]

Liu,

Thanks for explanation, it does make sense now. As for no need to 
-DNO_MSABI_VA_FUNCS I agree, but it will not make much difference, because from 
what I understand the VA_ARG implementation is chosen based on EFIAPI presence 
when generic __builtin’s are used.

Best,
Vitaly

> 12 февр. 2020 г., в 04:38, Liu, Zhiguang  написал(а):
> 
> Hi Vitaly,
> After your patch to Switch to GNU mode for CLANGPDB, the build option 
> -DNO_MSABI_VA_FUNCS is not required. I will send another patch to remove it.
> And for you question, this is a patch set that resolves BZ 2415, and the 
> second patch 21821933aea284cd3dfea6994bd4b83bd9739fc9 has direct influence to 
> CLANG38.
>
> Thanks
> Zhiguang
>
> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> 
> mailto:devel@edk2.groups.io>> On Behalf Of Vitaly 
> Cheptsov via Groups.Io
> Sent: Tuesday, February 11, 2020 3:09 PM
> To: Gao, Liming mailto:liming@intel.com>>; Liu, 
> Zhiguang mailto:zhiguang@intel.com>>; Shi, 
> Steven mailto:steven@intel.com>>
> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Subject: Re: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for 
> CLANGPDB
>
> Liming,
>
> Done. As a side note, I am not positive how can 
> 7990438f1437f47990a8890dee51978cb8dbc25c[1] resolve BZ 2415[2]. The bug was 
> about CLANG38, and the toolchain updated was CLANGPDB. While it makes sense 
> to update CLANGPDB with this flag to stay clean (it will not make a 
> difference for clang in GNU mode), CLANGPDB has nothing to do to CLANG38.
>
> Best wishes,
> Vitaly
>
> [1] https://bugzilla.tianocore.org/show_bug.cgi?id=2415 
> <https://bugzilla.tianocore.org/show_bug.cgi?id=2415>
> [2] 
> https://github.com/tianocore/edk2/commit/7990438f1437f47990a8890dee51978cb8dbc25c
>  
> <https://github.com/tianocore/edk2/commit/7990438f1437f47990a8890dee51978cb8dbc25c>
>
> 
> 
> 11 февр. 2020 г., в 09:02, Gao, Liming  <mailto:liming@intel.com>> написал(а):
>
> Vitaly:
>   Can you update this patch based on the latest edk2 trunk? I will catch it 
> for edk2 Q1 stable tag.
>
> Thanks
> Liming
> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> 
> mailto:devel@edk2.groups.io>> On Behalf Of Liming Gao
> Sent: Tuesday, February 11, 2020 1:34 PM
> To: vit9696 mailto:vit9...@protonmail.com>>; 
> devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Subject: Re: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for 
> CLANGPDB
>
> Reviewed-by: Liming Gao mailto:liming@intel.com>>
>
> From: vit9696 mailto:vit9...@protonmail.com>> 
> Sent: Tuesday, February 11, 2020 3:23 AM
> To: Gao, Liming mailto:liming@intel.com>>; 
> devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Subject: RE: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for 
> CLANGPDB
>
> Liming,
>
> We did run several of our projects based on EDK II in X64 mode, DEBUG, 
> RELEASE, NOOPT. Noticed no change from XCODE5.
>
> We also tried building several EDK builtin packages like CryptoPkg, MdePkg, 
> MdeModulePkg.
>
> Best wishes,
> Vitaly
>
> В пн, февр. 10, 2020 в 16:47, Gao, Liming  <mailto:liming@intel.com>> пишет:
> Vitaly:
> This change is good. Can you your test for it? I verify this patch for Ovmf 
> platform on Windows. It can make ovmf pass build with CLANGPDB.
> 
> Thanks
> Liming
> > -Original Message-
> > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> 
> > mailto:devel@edk2.groups.io>> On Behalf Of Vitaly 
> > Cheptsov via Groups.Io
> > Sent: Monday, February 10, 2020 6:59 PM
> > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> > Subject: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for CLANGPDB
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2397 
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=2397>
> >
> > Signed-off-by: Vitaly Cheptsov  > <mailto:vit9...@protonmail.com>>
> > ---
> > BaseTools/Conf/tools_def.template | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/BaseTools/Conf/tools_def.template 
> > b/BaseTools/Conf/tools_def.template
> > index feee2bbf16..6bf6c5768e 100755
> > --- a/BaseTools/Conf/tools_def.template
> > +++ b/BaseTools/Conf/tools_def.template
> > @@ -2755,11 +2755,11 @@ RELEASE_CLANG38_AARCH64_DLINK_FLAGS = 
> > DEF(CLANG38_AARCH64_DLINK_FLAGS) -flto -Wl
> > DEFINE CLANGPDB_IA32_PREFIX = ENV(CLANG_BIN)
> > DEFINE CLANGPDB_X64_PREFIX = ENV(CLANG_BIN)
> >
> > -DEFINE CLANGPDB_

Re: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for CLANGPDB

[Vitaly Cheptsov via Groups.Io]

Liming,

Done. As a side note, I am not positive how can 
7990438f1437f47990a8890dee51978cb8dbc25c[1] resolve BZ 2415[2]. The bug was 
about CLANG38, and the toolchain updated was CLANGPDB. While it makes sense to 
update CLANGPDB with this flag to stay clean (it will not make a difference for 
clang in GNU mode), CLANGPDB has nothing to do to CLANG38.

Best wishes,
Vitaly

[1] https://bugzilla.tianocore.org/show_bug.cgi?id=2415 
<https://bugzilla.tianocore.org/show_bug.cgi?id=2415>
[2] 
https://github.com/tianocore/edk2/commit/7990438f1437f47990a8890dee51978cb8dbc25c
 
<https://github.com/tianocore/edk2/commit/7990438f1437f47990a8890dee51978cb8dbc25c>


> 11 февр. 2020 г., в 09:02, Gao, Liming  написал(а):
> 
> Vitaly:
>   Can you update this patch based on the latest edk2 trunk? I will catch it 
> for edk2 Q1 stable tag.
>
> Thanks
> Liming
> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> 
> mailto:devel@edk2.groups.io>> On Behalf Of Liming Gao
> Sent: Tuesday, February 11, 2020 1:34 PM
> To: vit9696 mailto:vit9...@protonmail.com>>; 
> devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Subject: Re: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for 
> CLANGPDB
>
> Reviewed-by: Liming Gao mailto:liming@intel.com>>
>
> From: vit9696 mailto:vit9...@protonmail.com>> 
> Sent: Tuesday, February 11, 2020 3:23 AM
> To: Gao, Liming mailto:liming@intel.com>>; 
> devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Subject: RE: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for 
> CLANGPDB
>
> Liming,
>
> We did run several of our projects based on EDK II in X64 mode, DEBUG, 
> RELEASE, NOOPT. Noticed no change from XCODE5.
>
> We also tried building several EDK builtin packages like CryptoPkg, MdePkg, 
> MdeModulePkg.
>
> Best wishes,
> Vitaly
>
> В пн, февр. 10, 2020 в 16:47, Gao, Liming  <mailto:liming@intel.com>> пишет:
> Vitaly:
> This change is good. Can you your test for it? I verify this patch for Ovmf 
> platform on Windows. It can make ovmf pass build with CLANGPDB.
> 
> Thanks
> Liming
> > -Original Message-
> > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> 
> > mailto:devel@edk2.groups.io>> On Behalf Of Vitaly 
> > Cheptsov via Groups.Io
> > Sent: Monday, February 10, 2020 6:59 PM
> > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> > Subject: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for CLANGPDB
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2397 
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=2397>
> >
> > Signed-off-by: Vitaly Cheptsov  > <mailto:vit9...@protonmail.com>>
> > ---
> > BaseTools/Conf/tools_def.template | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/BaseTools/Conf/tools_def.template 
> > b/BaseTools/Conf/tools_def.template
> > index feee2bbf16..6bf6c5768e 100755
> > --- a/BaseTools/Conf/tools_def.template
> > +++ b/BaseTools/Conf/tools_def.template
> > @@ -2755,11 +2755,11 @@ RELEASE_CLANG38_AARCH64_DLINK_FLAGS = 
> > DEF(CLANG38_AARCH64_DLINK_FLAGS) -flto -Wl
> > DEFINE CLANGPDB_IA32_PREFIX = ENV(CLANG_BIN)
> > DEFINE CLANGPDB_X64_PREFIX = ENV(CLANG_BIN)
> >
> > -DEFINE CLANGPDB_IA32_TARGET = -target i686-unknown-windows
> > -DEFINE CLANGPDB_X64_TARGET = -target x86_64-unknown-windows
> > +DEFINE CLANGPDB_IA32_TARGET = -target i686-unknown-windows-gnu
> > +DEFINE CLANGPDB_X64_TARGET = -target x86_64-unknown-windows-gnu
> >
> > DEFINE CLANGPDB_WARNING_OVERRIDES = -Wno-parentheses-equality 
> > -Wno-tautological-compare -Wno-tautological-constant-out-
> > of-range-compare -Wno-empty-body -Wno-unused-const-variable -Wno-varargs 
> > -Wno-unknown-warning-option -Wno-microsoft-enum-
> > forward-reference
> > -DEFINE CLANGPDB_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) 
> > DEF(CLANGPDB_WARNING_OVERRIDES) -fno-stack-protector -
> > mms-bitfields -Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas 
> > -Wno-incompatible-library-redeclaration -fno-
> > asynchronous-unwind-tables -mno-implicit-float 
> > -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang -
> > funsigned-char -fno-ms-extensions -Wno-null-dereference -fms-compatibility 
> > -mno-stack-arg-probe
> > +DEFINE CLANGPDB_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) 
> > DEF(CLANGPDB_WARNING_OVERRIDES) -fno-stack-protector -
> > fno-asynchronous-unwind-tables -funsigned-char 
> > -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang -Wno-
> > address -Wno-s

[edk2-devel] [PATCH V2 0/1] BaseTools: Switch to GNU mode for CLANGPDB

[Vitaly Cheptsov via Groups.Io]

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

I did not include __clang__ removal in this patch series, as it is
also used in arm and aarch64 code, which makes me believe they could
be using specialised toolchains.

V2 performed a rebase onto recent master commits.

Request this to be merged in edk2-stable202002.

Vitaly Cheptsov (1):
  BaseTools: Switch to GNU mode for CLANGPDB

 BaseTools/Conf/tools_def.template | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.21.1 (Apple Git-122.3)


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

View/Reply Online (#54184): https://edk2.groups.io/g/devel/message/54184
Mute This Topic: https://groups.io/mt/71160540/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for CLANGPDB

[Vitaly Cheptsov via Groups.Io]

Liming,

Yes, perhaps that was unclear from my message. From what I understand 
__builtin_va_start behaves the same way as __builtin_ms_va__start for EFIAPI 
functions, and these are currently the only functions where variadic arguments 
are allowed.

For non-EFIAPI functions the calling conventions may be different from 
Microsoft 64-bit, i.e. they are System V for CLANG, CLANGPDB, and GCC. If you 
use variadic arguments in non-EFIAPI functions (which I believe is currently 
unsupported), you will end up with different calling conventions for variadic 
arguments as well. In this case the use of __builtin_va_start will work fine 
here, as System V variadic arguments will be used for non-EFIAPI, and Microsoft 
64-bit for EFIAPI, while __builtin_ms_va_start may technically break non-EFIAPI.

Perhaps I do not fully understand the underlying mechanism, but that is how I 
have always thought it works.

Best wishes,
Vitaly

On Wed, Feb 12, 2020 at 17:01, Gao, Liming  wrote:

> Vitaly:
>
>   With this change, X64 GCC and CLANG tool chain will use below VA_START 
> definition.
>
> #define VA_START(Marker, Parameter)  __builtin_ms_va_start (Marker, Parameter)
>
> Thanks
>
> Liming
>
> From: devel@edk2.groups.io   On Behalf Of Vitaly 
> Cheptsov via Groups.Io
> Sent: Wednesday, February 12, 2020 4:08 PM
> To: Liu, Zhiguang 
> Cc: devel@edk2.groups.io; Gao, Liming ; Shi, Steven 
> 
> Subject: Re: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for 
> CLANGPDB
>
> Liu,
>
> Thanks for explanation, it does make sense now. As for no need to 
> -DNO_MSABI_VA_FUNCS I agree, but it will not make much difference, because 
> from what I understand the VA_ARG implementation is chosen based on EFIAPI 
> presence when generic __builtin’s are used.
>
> Best,
>
> Vitaly
>
>> 12 февр. 2020 г., в 04:38, Liu, Zhiguang  написал(а):
>>
>> Hi Vitaly,
>>
>> After your patch to Switch to GNU mode for CLANGPDB, the build option 
>> -DNO_MSABI_VA_FUNCS is not required. I will send another patch to remove it.
>>
>> And for you question, this is a patch set that resolves BZ 2415, and the 
>> second patch 21821933aea284cd3dfea6994bd4b83bd9739fc9 has direct influence 
>> to CLANG38.
>>
>> Thanks
>>
>> Zhiguang
>>
>> From: devel@edk2.groups.io  On Behalf Of Vitaly 
>> Cheptsov via Groups.Io
>> Sent: Tuesday, February 11, 2020 3:09 PM
>> To: Gao, Liming ; Liu, Zhiguang 
>> ; Shi, Steven 
>> Cc: devel@edk2.groups.io
>> Subject: Re: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for 
>> CLANGPDB
>>
>> Liming,
>>
>> Done. As a side note, I am not positive how can 
>> 7990438f1437f47990a8890dee51978cb8dbc25c[1] resolve BZ 2415[2]. The bug was 
>> about CLANG38, and the toolchain updated was CLANGPDB. While it makes sense 
>> to update CLANGPDB with this flag to stay clean (it will not make a 
>> difference for clang in GNU mode), CLANGPDB has nothing to do to CLANG38.
>>
>> Best wishes,
>>
>> Vitaly
>>
>> [1] https://bugzilla.tianocore.org/show_bug.cgi?id=2415
>>
>> [2] 
>> https://github.com/tianocore/edk2/commit/7990438f1437f47990a8890dee51978cb8dbc25c
>>
>>> 11 февр. 2020 г., в 09:02, Gao, Liming  написал(а):
>>>
>>> Vitaly:
>>>
>>>   Can you update this patch based on the latest edk2 trunk? I will catch it 
>>> for edk2 Q1 stable tag.
>>>
>>> Thanks
>>>
>>> Liming
>>>
>>> From: devel@edk2.groups.io  On Behalf Of Liming Gao
>>> Sent: Tuesday, February 11, 2020 1:34 PM
>>> To: vit9696 ; devel@edk2.groups.io
>>> Subject: Re: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for 
>>> CLANGPDB
>>>
>>> Reviewed-by: Liming Gao 
>>>
>>> From: vit9696 
>>> Sent: Tuesday, February 11, 2020 3:23 AM
>>> To: Gao, Liming ; devel@edk2.groups.io
>>> Subject: RE: [edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for 
>>> CLANGPDB
>>>
>>> Liming,
>>>
>>> We did run several of our projects based on EDK II in X64 mode, DEBUG, 
>>> RELEASE, NOOPT. Noticed no change from XCODE5.
>>>
>>> We also tried building several EDK builtin packages like CryptoPkg, MdePkg, 
>>> MdeModulePkg.
>>>
>>> Best wishes,
>>>
>>> Vitaly
>>>
>>> В пн, февр. 10, 2020 в 16:47, Gao, Liming  пишет:
>>>
>>>> Vitaly:
>>>> This change is good. Can you your test for it? I verify this patch for 
>>>> Ovmf platform on Windows. It can make ovmf pass build 

Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

Hi Mike,

Any progress with this? We would really benefit from this landing in the next 
stable release.

Best,
Vitaly

> 8 янв. 2020 г., в 19:35, Kinney, Michael D  
> написал(а):
> 
> 
> Hi Vitaly,
> 
> Thanks for the additional background.  I would like
> a couple extra day to review the PCD name and the places
> the PCD might potentially be used.
> 
> If we find other APIs where ASSERT() behavior is only
> valuable during dev/debug to quickly identify misuse
> with trusted data and the API provides predicable
> return behavior when ASSERT() is disabled, then I would
> like to have a pattern we can potentially apply to all
> these APIs across all packages.
> 
> Thanks,
> 
> Mike
> 
>> -Original Message-----
>> From: devel@edk2.groups.io  On
>> Behalf Of Vitaly Cheptsov via Groups.Io
>> Sent: Monday, January 6, 2020 10:44 AM
>> To: Kinney, Michael D 
>> Cc: devel@edk2.groups.io
>> Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to
>> disable safe string constraint assertions
>> 
>> Hi Mike,
>> 
>> Yes, the primary use case is for UEFI Applications. We
>> do not want to disable ASSERT’s completely, as
>> assertions that make sense, i.e. the ones signalising
>> about interface misuse, are helpful for debugging.
>> 
>> I have already explained in the BZ that basically all
>> safe string constraint assertions make no sense for
>> handling untrusted data. We find this use case very
>> logical, as these functions behave properly with
>> assertions disabled and cover all these error
>> conditions by the return statuses. In such situation is
>> not useful for these functions to assert, as we end up
>> inefficiently reimplementing the logic. I would have
>> liked the approach of discussing the interfaces
>> individually, but I struggle to find any that makes
>> sense from this point of view.
>> 
>> AsciiStrToGuid will ASSERT when the length of the
>> passed string is odd. Functions that cannot, ahem,
>> parse, for us are pretty much useless.
>> AsciiStrCatS will ASSERT when the appended string does
>> not fit the buffer. For us this logic makes this
>> function pretty much equivalent to deprecated and thus
>> unavailable AsciiStrCat, except it is also slower.
>> 
>> My original suggestion was to remove the assertions
>> entirely, but several people here said that they use
>> them to verify usage errors when handling trusted data.
>> This makes good sense to me, so we suggest to support
>> both cases by introducing a PCD in this patch.
>> 
>> Best wishes,
>> Vitaly
>> 
>>> 6 янв. 2020 г., в 21:28, Kinney, Michael D
>>  написал(а):
>>> 
>>> 
>>> Hi Vitaly,
>>> 
>>> Is the use case for UEFI Applications?
>>> 
>>> There is a different mechanism to disable all
>> ASSERT()
>>> statements  within a UEFI Application.
>>> 
>>> If a component is consuming data from an untrusted
>> source,
>>> then that component is required to verify the
>> untrusted
>>> data before passing it to a function that clearly
>> documents
>>> is input requirements.  If this approach is followed,
>> then
>>> the BaseLib functions can be used "as is" as long as
>> the
>>> ASSERT() conditions are verified before calling.
>>> 
>>> If there are some APIs that currently document their
>> ASSERT()
>>> behavior and we think that ASSERT() behavior is
>> incorrect and
>>> should be handled by an existing error return value,
>> then we
>>> should discuss each of those APIs individually.
>>> 
>>> Mike
>>> 
>>> 
>>>> -Original Message-
>>>> From: devel@edk2.groups.io  On
>>>> Behalf Of Vitaly Cheptsov via Groups.Io
>>>> Sent: Friday, January 3, 2020 9:13 AM
>>>> To: devel@edk2.groups.io
>>>> Subject: [edk2-devel] [PATCH v3 0/1] Add PCD to
>> disable
>>>> safe string constraint assertions
>>>> 
>>>> REF:
>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=2054
>>>> 
>>>> Requesting for merge in edk2-stable202002.
>>>> 
>>>> Changes since V1:
>>>> - Enable assertions by default to preserve the
>> original
>>>> behaviour
>>>> - Fix bugzilla reference link
>>>> - Update documentation in BaseLib.h
>>>> 
>>>> Vitaly Cheptsov (1):
>>>> MdePkg: Add PCD to disable safe stri

Re: [edk2-devel] [PATCH 0/1] Use _MSC_VER to determine MSVC compiler

[Vitaly Cheptsov via Groups.Io]

4 февр. 2020 г., в 09:56, Gao, Liming <liming@intel.com> написал(а):Vitaly:  Yes. I think we should have better solution in OpenSSL to support EDK2 usage. But, this is a long term solution. For now, I will try the solution to remove -fms-compatibility option in CLANGPDB tool chain. ThanksLimingFrom: vit9696 <vit9...@protonmail.com> Sent: Monday, February 3, 2020 7:29 PMTo: Gao, Liming <liming@intel.com>; devel@edk2.groups.io; Marvin Häuser <marvin.haeu...@outlook.com>Subject: RE: [edk2-devel] [PATCH 0/1] Use _MSC_VER to determine MSVC compiler Liming, I believe it is reasonable to fix OpenSSL, but most likely it is faster to address EDK II code at first. In future we should still stick to _MSC_VER, but for now just ensuring that any toolchain by MSVC does not define _MSC_EXTENSIONS should work too. I believe that -fms-compatibility option is not needed for CLANGPDB, as CLANGPDB is literally using clang, and that worked fine before in CLANG38 and XCODE5. We may still have to update some preprocessor conditions to include __clang__ near __GNUC__ if __GNUC__ is left undefined even when we remove -fms-compatibility, but that sounds fine for me. We will investigate that on our own and submit a new patch, but help from other parties is strongly appreciated. We mostly use XCODE5 and are not well aware of other platforms. Best wishes,Vitaly On Mon, Feb 3, 2020 at 11:00, Gao, Liming <liming@intel.com> wrote:Vitaly:This change will cause CryptoPkg openssl build failure, because OpensslLib.inf undefines _MSC_VER macro to make sure openssl source code be built in edk2 project without using MS VS functions.Now, I have no good solution to fix this issue. One way is to use defined(_MSC_EXTENSIONS) && !defined(__clang__), another way is to investigate whether we can remove -fms-compatibility option in CLANGPDB.ThanksLiming> -Original Message-> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov via Groups.Io> Sent: Saturday, February 1, 2020 5:17 AM> To: devel@edk2.groups.io> Subject: [edk2-devel] [PATCH 0/1] Use _MSC_VER to determine MSVC compiler>> Patch details are explained in https://bugzilla.tianocore.org/show_bug.cgi?id=2397.> We request this to be merged in edk2-stable202002.>> Vitaly Cheptsov (1):> MdePkg: Use _MSC_VER to determine MSVC compiler>> MdePkg/Include/AArch64/ProcessorBind.h | 2 +-> MdePkg/Include/Arm/ProcessorBind.h | 8 > MdePkg/Include/Base.h | 10 +-> MdePkg/Include/Ia32/ProcessorBind.h | 6 +++---> MdePkg/Include/X64/ProcessorBind.h | 6 +++---> 5 files changed, 16 insertions(+), 16 deletions(-)>> --> 2.21.1 (Apple Git-122.3)>>> -=-=-=-=-=-=> Groups.io Links: You receive all messages sent to this group.>> View/Reply Online (#53623): https://edk2.groups.io/g/devel/message/53623> Mute This Topic: https://groups.io/mt/70882954/1759384> Group Owner: devel+ow...@edk2.groups.io> Unsubscribe: https://edk2.groups.io/g/devel/unsub [liming@intel.com]> -=-=-=-=-=-=

_._,_._,_

Groups.io Links:


You receive all messages sent to this group.





View/Reply Online (#53835) |





|



Mute This Topic


| New Topic





Your Subscription |
Contact Group Owner |

Unsubscribe

 [arch...@mail-archive.com]
_._,_._,_



clangpdb.diff
Description: Binary data



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [PATCH 0/1] Use _MSC_VER to determine MSVC compiler

[Vitaly Cheptsov via Groups.Io]

Liming,

Correct, I believe it is not just Base.h but several other files. I plan to 
include the removal of __clang__ references in my patchset as well, since after 
the target change all the use of clang will be in GNU mode.

In addition to that, I believe that in GNU mode it should be also possible to 
support ARM and AARCH64 in CLANGPDB, but I would rather not work on this as I 
do not have the hardware for validation.

Best wishes,
Vitaly

> 6 февр. 2020 г., в 11:22, Gao, Liming  написал(а):
> 
> Vitaly:
>   We also find _MSC_VER is defined in Windows, but not in Linux. Your 
> analysis explains it. When use i686-unknown-windows-gnu option, __GNUC__ 
> macro will be defined. If so, we don’t need to append the check for defined 
> (__clang__) in Base.h. And, this change can remove -fno-ms-extensions and 
> -fms-compatibility option. Then, CLANGPDB can keep the same behavior in 
> Windows/Linux/MacOs host OS.
>
> #if defined (__GNUC__) || defined (__clang__)
> è
> #if defined (__GNUC__)
>
> Thanks
> Liming
> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> 
> mailto:devel@edk2.groups.io>> On Behalf Of Vitaly 
> Cheptsov via Groups.Io
> Sent: Thursday, February 6, 2020 8:17 AM
> To: Gao, Liming mailto:liming@intel.com>>
> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Marvin Häuser 
> mailto:marvin.haeu...@outlook.com>>; Laszlo 
> Ersek mailto:ler...@redhat.com>>; Kinney, Michael D 
> mailto:michael.d.kin...@intel.com>>
> Subject: Re: [edk2-devel] [PATCH 0/1] Use _MSC_VER to determine MSVC compiler
>
> Liming,
>
> We performed the initial exploration of CLANGPDB toolchain issue on our end 
> and believe we can suggest a solid solution.
>
> In addition to all the issues I mentioned in the BZ[1] there are several more.
>
> 1. CLANGPDB uses -target x86_64-unknown-windows, and this basically means 
> different behaviour for Windows and other operating systems:
> - On Windows it will "attach" to installed Visual Studio and will gather the 
> parameters from this installation, i.e. it will define _MSC_VER to installed 
> Visual Studio version. For example, for me it is implicitly passing 
> -fms-compatibility-version=19.16.27026 and setting full triple to 
> x86_64-unknown-windows-msvc19.16.27026.
> - On Mac and Linux it will obviously not find Visual Studio, and as a result 
> the full triple will be x86_64-unknown-windows-msvc with _MSC_VER macro not 
> being defined.
>
> There basically is no control to it except -U_MSC_VER, which is ugly, as 
> different include directories, other defines will still happen between 
> installations.
>
> 2. EDK II relies on UINT32_MAX being a valid value for enum. This is not the 
> case in the specification, as it requires enum to be either INT32 or UINT32:
>
> Element of a standard ANSI C enum type declaration. Type INT32.or UINT32. 
> ANSI C does not define the size of sign of an enum so they should never be 
> used in structures. ANSI C integer promotion rules make INT32 or UINT32 
> interchangeable when passed as an argument to a function.
>
> However, since I am not positive that no existing code relies on this, it is 
> best to preserve the current behaviour. Supporting this is valid for GNU 
> flavour or as a Microsoft Extension. Disabling -fms-compatibility will result 
> in a compile error for enums having 0x values, like in Base.h.
>
> All in all, we believe that CLANGPDB simply has an overlook in the -target 
> argument due to a misconsideration of the clang triple implementation. 
> Normally for target only 3 words are provided, but for Windows it is crucial 
> to have 4, as there are different drivers with different automatics. To 
> resolve the problem, we should use GNU targets i686-unknown-windows-gnu and 
> x86_64-unknown-windows-gnu. This is basically the only and the least hurtful 
> solution, as using MSVC mode will define _MSC_EXTENSIONS, which already 
> breaks many places and will require a heavy codebase refactoring, and 
> randomly define _MSC_VER and use Visual Studio headers and configuration, 
> which makes reproducible builds on different operating systems questionable 
> if not impossible.
> 
> 
> I will submit another patch that will replace this one later this week. In 
> addition to GNU targets I additionally pass -nostdlib and -nostdlibinc so 
> that a freestanding target is used and only builtin headers are accessible 
> (like stdint.h, stddef.h, and stdbool.h). This is not required but an extra 
> safety measure. Our initial validation of the changes found no issues with 
> our projects. We also performed building of most common EDK II packages like 
> CryptoPkg, MdePkg, and MdeModulePkg. While t

[edk2-devel] [PATCH 0/1] Use _MSC_VER to determine MSVC compiler

[Vitaly Cheptsov via Groups.Io]

Patch details are explained in 
https://bugzilla.tianocore.org/show_bug.cgi?id=2397.
We request this to be merged in edk2-stable202002.

Vitaly Cheptsov (1):
  MdePkg: Use _MSC_VER to determine MSVC compiler

 MdePkg/Include/AArch64/ProcessorBind.h |  2 +-
 MdePkg/Include/Arm/ProcessorBind.h |  8 
 MdePkg/Include/Base.h  | 10 +-
 MdePkg/Include/Ia32/ProcessorBind.h|  6 +++---
 MdePkg/Include/X64/ProcessorBind.h |  6 +++---
 5 files changed, 16 insertions(+), 16 deletions(-)

-- 
2.21.1 (Apple Git-122.3)


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

View/Reply Online (#53623): https://edk2.groups.io/g/devel/message/53623
Mute This Topic: https://groups.io/mt/70882954/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

[edk2-devel] [PATCH 1/1] MdePkg: Use _MSC_VER to determine MSVC compiler

[Vitaly Cheptsov via Groups.Io]

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

CLANGPDB toolchain, implemented by clang compiler, will also
define currently used _MSC_EXTENSIONS macro, which does not
behave correctly in many situations as explained in the report.

Signed-off-by: Vitaly Cheptsov 
---
 MdePkg/Include/AArch64/ProcessorBind.h |  2 +-
 MdePkg/Include/Arm/ProcessorBind.h |  8 
 MdePkg/Include/Base.h  | 10 +-
 MdePkg/Include/Ia32/ProcessorBind.h|  6 +++---
 MdePkg/Include/X64/ProcessorBind.h |  6 +++---
 5 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/MdePkg/Include/AArch64/ProcessorBind.h 
b/MdePkg/Include/AArch64/ProcessorBind.h
index 896bf273ac..fe6b28ec9c 100644
--- a/MdePkg/Include/AArch64/ProcessorBind.h
+++ b/MdePkg/Include/AArch64/ProcessorBind.h
@@ -24,7 +24,7 @@
 #pragma pack()
 #endif
 
-#if defined(_MSC_EXTENSIONS)
+#if defined(_MSC_VER)
 
 //
 // Disable some level 4 compilation warnings (same as IA32 and X64)
diff --git a/MdePkg/Include/Arm/ProcessorBind.h 
b/MdePkg/Include/Arm/ProcessorBind.h
index 1264b44b46..f5a5969d4a 100644
--- a/MdePkg/Include/Arm/ProcessorBind.h
+++ b/MdePkg/Include/Arm/ProcessorBind.h
@@ -22,7 +22,7 @@
 #pragma pack()
 #endif
 
-#if defined(_MSC_EXTENSIONS)
+#if defined(_MSC_VER)
 
 //
 // Disable some level 4 compilation warnings (same as IA32 and X64)
@@ -74,11 +74,11 @@
 //
 // RVCT and MSFT don't support the __builtin_unreachable() macro
 //
-#if defined(__ARMCC_VERSION) || defined(_MSC_EXTENSIONS)
+#if defined(__ARMCC_VERSION) || defined(_MSC_VER)
 #define UNREACHABLE()
 #endif
 
-#if defined(_MSC_EXTENSIONS)
+#if defined(_MSC_VER)
   //
   // use Microsoft* C compiler dependent integer width types
   //
@@ -212,7 +212,7 @@ typedef INT32   INTN;
 #define GCC_ASM_IMPORT(name)
 
   #endif
-#elif defined(_MSC_EXTENSIONS)
+#elif defined(_MSC_VER)
   //
   // PRESERVE8 is not supported by the MSFT assembler.
   //
diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index 321d729c04..bda630a2dc 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -21,7 +21,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 //
 #include 
 
-#if defined(_MSC_EXTENSIONS)
+#if defined(_MSC_VER)
 //
 // Disable warning when last field of data structure is a zero sized array.
 //
@@ -33,7 +33,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 //  if the /OPT:REF linker option is used. We defined a macro as this is a
 //  a non standard extension
 //
-#if defined(_MSC_EXTENSIONS) && _MSC_VER < 1800 && !defined (MDE_CPU_EBC)
+#if defined(_MSC_VER) && _MSC_VER < 1800 && !defined (MDE_CPU_EBC)
   ///
   /// Remove global variable from the linked image if there are no references 
to
   /// it after all compiler and linker optimizations have been performed.
@@ -92,7 +92,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 /// flagged with this attribute.
 ///
 #define NORETURN  __attribute__((noreturn))
-  #elif defined(_MSC_EXTENSIONS) && !defined(MDE_CPU_EBC)
+  #elif defined(_MSC_VER) && !defined(MDE_CPU_EBC)
 ///
 /// Signal compilers and analyzers that the function cannot return.
 /// It is up to the compiler to remove any code past a call to functions
@@ -799,7 +799,7 @@ typedef UINTN  *BASE_LIST;
 **/
 #ifdef MDE_CPU_EBC
   #define STATIC_ASSERT(Expression, Message)
-#elif defined(_MSC_EXTENSIONS)
+#elif defined(_MSC_VER)
   #define STATIC_ASSERT static_assert
 #else
   #define STATIC_ASSERT _Static_assert
@@ -1256,7 +1256,7 @@ typedef UINTN RETURN_STATUS;
 #define SIGNATURE_64(A, B, C, D, E, F, G, H) \
 (SIGNATURE_32 (A, B, C, D) | ((UINT64) (SIGNATURE_32 (E, F, G, H)) << 32))
 
-#if defined(_MSC_EXTENSIONS) && !defined (__INTEL_COMPILER) && !defined 
(MDE_CPU_EBC)
+#if defined(_MSC_VER) && !defined (__INTEL_COMPILER) && !defined (MDE_CPU_EBC)
   void * _ReturnAddress(void);
   #pragma intrinsic(_ReturnAddress)
   /**
diff --git a/MdePkg/Include/Ia32/ProcessorBind.h 
b/MdePkg/Include/Ia32/ProcessorBind.h
index fa4b7e8e98..bf6a8b09f5 100644
--- a/MdePkg/Include/Ia32/ProcessorBind.h
+++ b/MdePkg/Include/Ia32/ProcessorBind.h
@@ -49,7 +49,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #endif
 
 
-#if defined(_MSC_EXTENSIONS)
+#if defined(_MSC_VER)
 
 //
 // Disable warning that make it impossible to compile at /W4
@@ -110,7 +110,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #endif
 
 
-#if defined(_MSC_EXTENSIONS)
+#if defined(_MSC_VER)
 
   //
   // use Microsoft C compiler dependent integer width types
@@ -276,7 +276,7 @@ typedef INT32   INTN;
   ///
   /// If EFIAPI is already defined, then we use that definition.
   ///
-#elif defined(_MSC_EXTENSIONS)
+#elif defined(_MSC_VER)
   ///
   /// Microsoft* compiler specific method for EFIAPI calling convention.
   ///
diff --git a/MdePkg/Include/X64/ProcessorBind.h 
b/MdePkg/Include/X64/ProcessorBind.h
index 387e9c5c9c..2e8ac6c8d5 100644
--- a/MdePkg/Include/X64/ProcessorBind.h
+++ b/MdePkg/Include/X64/ProcessorBind.h
@@ -63,7 +63,7 @@
 

Re: [edk2-devel] [PATCH 0/1] Use _MSC_VER to determine MSVC compiler

[Vitaly Cheptsov via Groups.Io]

Liming,

I believe it is reasonable to fix OpenSSL, but most likely it is faster to 
address EDK II code at first. In future we should still stick to _MSC_VER, but 
for now just ensuring that any toolchain by MSVC does not define 
_MSC_EXTENSIONS should work too.

I believe that -fms-compatibility option is not needed for CLANGPDB, as 
CLANGPDB is literally using clang, and that worked fine before in CLANG38 and 
XCODE5. We may still have to update some preprocessor conditions to include 
__clang__ near __GNUC__ if __GNUC__ is left undefined even when we remove 
-fms-compatibility, but that sounds fine for me.

We will investigate that on our own and submit a new patch, but help from other 
parties is strongly appreciated. We mostly use XCODE5 and are not well aware of 
other platforms.

Best wishes,
Vitaly

On Mon, Feb 3, 2020 at 11:00, Gao, Liming  wrote:

> Vitaly:
> This change will cause CryptoPkg openssl build failure, because 
> OpensslLib.inf undefines _MSC_VER macro to make sure openssl source code be 
> built in edk2 project without using MS VS functions.
>
> Now, I have no good solution to fix this issue. One way is to use 
> defined(_MSC_EXTENSIONS) && !defined(__clang__), another way is to 
> investigate whether we can remove -fms-compatibility option in CLANGPDB.
>
> Thanks
> Liming
>> -Original Message-----
>> From: devel@edk2.groups.io  On Behalf Of Vitaly 
>> Cheptsov via Groups.Io
>> Sent: Saturday, February 1, 2020 5:17 AM
>> To: devel@edk2.groups.io
>> Subject: [edk2-devel] [PATCH 0/1] Use _MSC_VER to determine MSVC compiler
>>
>> Patch details are explained in 
>> https://bugzilla.tianocore.org/show_bug.cgi?id=2397.
>> We request this to be merged in edk2-stable202002.
>>
>> Vitaly Cheptsov (1):
>> MdePkg: Use _MSC_VER to determine MSVC compiler
>>
>> MdePkg/Include/AArch64/ProcessorBind.h | 2 +-
>> MdePkg/Include/Arm/ProcessorBind.h | 8 
>> MdePkg/Include/Base.h | 10 +-
>> MdePkg/Include/Ia32/ProcessorBind.h | 6 +++---
>> MdePkg/Include/X64/ProcessorBind.h | 6 +++---
>> 5 files changed, 16 insertions(+), 16 deletions(-)
>>
>> --
>> 2.21.1 (Apple Git-122.3)
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>>
>> View/Reply Online (#53623): https://edk2.groups.io/g/devel/message/53623
>> Mute This Topic: https://groups.io/mt/70882954/1759384
>> Group Owner: devel+ow...@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [liming@intel.com]
>> -=-=-=-=-=-=
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53653): https://edk2.groups.io/g/devel/message/53653
Mute This Topic: https://groups.io/mt/70882954/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

Replying as per Liming's request for this to be merged into edk2-stable202002.

On Mon, Feb 10, 2020 at 14:12, vit9696  wrote:

> Hello,
>
> It has been quite some time since we submitted the patch with so far no 
> negative response. As I mentioned previously, my team will strongly benefit 
> from its landing in EDK II mainline. Since it does not add any regressions 
> and can be viewed as a feature implementation for the rest of EDK II users, I 
> request this to be merged upstream in edk2-stable202002.
>
> Best wishes,
> Vitaly
>
>> 27 янв. 2020 г., в 12:47, vit9696  написал(а):
>>
>>
>> Hi Mike,
>>
>> Any progress with this? We would really benefit from this landing in the 
>> next stable release.
>>
>> Best,
>> Vitaly
>>
>>> 8 янв. 2020 г., в 19:35, Kinney, Michael D  
>>> написал(а):
>>>
>>>
>>> Hi Vitaly,
>>>
>>> Thanks for the additional background. I would like
>>> a couple extra day to review the PCD name and the places
>>> the PCD might potentially be used.
>>>
>>> If we find other APIs where ASSERT() behavior is only
>>> valuable during dev/debug to quickly identify misuse
>>> with trusted data and the API provides predicable
>>> return behavior when ASSERT() is disabled, then I would
>>> like to have a pattern we can potentially apply to all
>>> these APIs across all packages.
>>>
>>> Thanks,
>>>
>>> Mike
>>>
>>>> -Original Message-
>>>> From: devel@edk2.groups.io  On
>>>> Behalf Of Vitaly Cheptsov via Groups.Io
>>>> Sent: Monday, January 6, 2020 10:44 AM
>>>> To: Kinney, Michael D 
>>>> Cc: devel@edk2.groups.io
>>>> Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to
>>>> disable safe string constraint assertions
>>>>
>>>> Hi Mike,
>>>>
>>>> Yes, the primary use case is for UEFI Applications. We
>>>> do not want to disable ASSERT’s completely, as
>>>> assertions that make sense, i.e. the ones signalising
>>>> about interface misuse, are helpful for debugging.
>>>>
>>>> I have already explained in the BZ that basically all
>>>> safe string constraint assertions make no sense for
>>>> handling untrusted data. We find this use case very
>>>> logical, as these functions behave properly with
>>>> assertions disabled and cover all these error
>>>> conditions by the return statuses. In such situation is
>>>> not useful for these functions to assert, as we end up
>>>> inefficiently reimplementing the logic. I would have
>>>> liked the approach of discussing the interfaces
>>>> individually, but I struggle to find any that makes
>>>> sense from this point of view.
>>>>
>>>> AsciiStrToGuid will ASSERT when the length of the
>>>> passed string is odd. Functions that cannot, ahem,
>>>> parse, for us are pretty much useless.
>>>> AsciiStrCatS will ASSERT when the appended string does
>>>> not fit the buffer. For us this logic makes this
>>>> function pretty much equivalent to deprecated and thus
>>>> unavailable AsciiStrCat, except it is also slower.
>>>>
>>>> My original suggestion was to remove the assertions
>>>> entirely, but several people here said that they use
>>>> them to verify usage errors when handling trusted data.
>>>> This makes good sense to me, so we suggest to support
>>>> both cases by introducing a PCD in this patch.
>>>>
>>>> Best wishes,
>>>> Vitaly
>>>>
>>>>> 6 янв. 2020 г., в 21:28, Kinney, Michael D
>>>>  написал(а):
>>>>>
>>>>>
>>>>> Hi Vitaly,
>>>>>
>>>>> Is the use case for UEFI Applications?
>>>>>
>>>>> There is a different mechanism to disable all
>>>> ASSERT()
>>>>> statements within a UEFI Application.
>>>>>
>>>>> If a component is consuming data from an untrusted
>>>> source,
>>>>> then that component is required to verify the
>>>> untrusted
>>>>> data before passing it to a function that clearly
>>>> documents
>>>>> is input requirements. If this approach is followed,
>>>> then
>>>>> the BaseLib functions can be used "as is" as long as
>>>&g

Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers

[Vitaly Cheptsov via Groups.Io]

Replying as per Liming's request for this to be merged into edk2-stable202002.

On Mon, Feb 10, 2020 at 13:18, Vitaly Cheptsov  wrote:

> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2510
>
> Some firmwares:
> - Report Shift modifier even when they report upper-case unicode letter.
> - Report Ctrl modifier with "shifted" UniChar (i.e. X - 'A' + 1).
>
> This change provides support for these firmwares preserving the compatibility
> with the previous input handling.
>
> Signed-off-by: Michael Belyaev 
> Reviewed-by: Vitaly Cheptsov 
> ---
> ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c | 37 
> ++--
> ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c | 28 
> ++-
> ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c | 6 
> ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c | 11 
> +++---
> 4 files changed, 58 insertions(+), 24 deletions(-)
>
> diff --git 
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c 
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> index df530f1119..9615a4dfbd 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> @@ -1381,8 +1381,8 @@ MainCommandDisplayHelp (
> continue;
> }
>
> - if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) ||
> - (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
> + if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0)
> + || (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
> //
> // For consoles that don't support/report shift state,
> // CTRL+W is translated to L'W' - L'A' + 1.
> @@ -1390,14 +1390,17 @@ MainCommandDisplayHelp (
> if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
> break;
> }
> - } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0) 
> &&
> - ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED | 
> EFI_RIGHT_CONTROL_PRESSED)) != 0) &&
> - ((KeyData.KeyState.KeyShiftState & ~(EFI_SHIFT_STATE_VALID | 
> EFI_LEFT_CONTROL_PRESSED | EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> + } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> + && ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED | 
> EFI_RIGHT_CONTROL_PRESSED)) != 0)
> + && ((KeyData.KeyState.KeyShiftState & ~(EFI_SHIFT_STATE_VALID | 
> EFI_LEFT_CONTROL_PRESSED | EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> //
> // For consoles that supports/reports shift state,
> // make sure that only CONTROL shift key is pressed.
> + // For some consoles that report shift state,
> + // CTRL+W is still translated to L'W' - L'A' + 1.
> //
> - if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 'W')) {
> + if ((KeyData.Key.UnicodeChar == L'w') || (KeyData.Key.UnicodeChar == L'W')
> + || (KeyData.Key.UnicodeChar == L'w' - L'a' + 1) || (KeyData.Key.UnicodeChar 
> == L'W' - L'A' + 1)) {
> break;
> }
> }
> @@ -1834,7 +1837,8 @@ MainEditorKeyInput (
> EFI_KEY_DATA KeyData;
> EFI_STATUS Status;
> EFI_SIMPLE_POINTER_STATE MouseState;
> - BOOLEAN NoShiftState;
> + BOOLEAN NoModifierState;
> + BOOLEAN ShiftOnlyState;
>
> do {
>
> @@ -1886,17 +1890,28 @@ MainEditorKeyInput (
> //
> StatusBarSetRefresh();
> //
> - // NoShiftState: TRUE when no shift key is pressed.
> + // NoModifierState: TRUE when no modifier key is pressed.
> //
> - NoShiftState = ((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 
> 0) || (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID);
> + NoModifierState = ((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) 
> == 0)
> + || (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID);
> + //
> + // ShiftOnlyState: TRUE when no modifier key except Shift is pressed.
> + //
> + ShiftOnlyState = ((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) 
> == 0)
> + || ((KeyData.KeyState.KeyShiftState
> + & ~(EFI_SHIFT_STATE_VALID | EFI_LEFT_SHIFT_PRESSED | 
> EFI_RIGHT_SHIFT_PRESSED)) == 0);
> //
> // dispatch to different components' key handling function
> //
> if (EFI_NOT_FOUND != MenuBarDispatchControlHotKey()) {
> Status = EFI_SUCCESS;
> - } else if (NoShiftState && ((KeyData.Key.ScanCode == SCAN_NULL) || 
> ((KeyData.Key.ScanCode >= SCAN_UP) && (KeyData.Key.ScanCode <= 
> SCAN_PAGE_DOWN {
> + } else if ((ShiftOnlyState && (KeyData.Key.ScanCode == SCAN_NULL))
> + || (NoModifierState && (KeyData.Key.ScanCode >= SCAN_UP) && 
> (KeyData.Key.ScanCode <= SCAN_PAGE_DOWN))) {
> + //
> + // alphanumeric keys with or without shift, or arrow keys without shift
> + //
> Status = FileBufferHandleInput ();
> - } else if (NoShiftState && (KeyData.Key.ScanCode >= SCAN_F1) && 
> (KeyData.Key.ScanCode <= SCAN_F12)) {
> + } else if (NoModifierState && (KeyData.Key.ScanCode >= SCAN_F1) && 
> (KeyData.Key.ScanCode <= SCAN_F12)) {
> Status = MenuBarDispatchFunctionKey ();
> } else {
> StatusBarSetStatusString (L"Unknown Command");
> diff 

Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers

[Vitaly Cheptsov via Groups.Io]

Zhichao,

Thanks for your review. The comment is correct, as ShiftOnlyState means the 
state where only shift (and no other modifiers) can be pressed or not.

Best wishes,
Vitaly

On Wed, Feb 19, 2020 at 09:55, Gao, Zhichao  wrote:

> Hi Vitaly,
>
> See the comment below:
>
>> -Original Message-
>> From: devel@edk2.groups.io  On Behalf Of Vitaly
>> Cheptsov via Groups.Io
>> Sent: Monday, February 10, 2020 6:18 PM
>> To: devel@edk2.groups.io
>> Subject: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with
>> separately reported modifiers
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2510
>>
>> Some firmwares:
>> - Report Shift modifier even when they report upper-case unicode letter.
>> - Report Ctrl modifier with "shifted" UniChar (i.e. X - 'A' + 1).
>>
>> This change provides support for these firmwares preserving the compatibility
>> with the previous input handling.
>>
>> Signed-off-by: Michael Belyaev 
>> Reviewed-by: Vitaly Cheptsov 
>> ---
>> ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c | 37
>> ++--
>> ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c | 28
>> ++-
>> ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c | 6 
>> ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c | 11
>> +++---
>> 4 files changed, 58 insertions(+), 24 deletions(-)
>>
>> diff --git
>> a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
>> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
>> index df530f1119..9615a4dfbd 100644
>> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
>> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
>> @@ -1381,8 +1381,8 @@ MainCommandDisplayHelp (
>> continue;
>> }
>>
>> - if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) ||
>> - (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
>> + if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0)
>> + || (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
>> //
>> // For consoles that don't support/report shift state,
>> // CTRL+W is translated to L'W' - L'A' + 1.
>> @@ -1390,14 +1390,17 @@ MainCommandDisplayHelp (
>> if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
>> break;
>> }
>> - } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
>> &&
>> - ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
>> EFI_RIGHT_CONTROL_PRESSED)) != 0) &&
>> - ((KeyData.KeyState.KeyShiftState & ~(EFI_SHIFT_STATE_VALID |
>> EFI_LEFT_CONTROL_PRESSED | EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
>> + } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
>> + && ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
>> EFI_RIGHT_CONTROL_PRESSED)) != 0)
>> + && ((KeyData.KeyState.KeyShiftState &
>> + ~(EFI_SHIFT_STATE_VALID | EFI_LEFT_CONTROL_PRESSED |
>> + EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
>> //
>> // For consoles that supports/reports shift state,
>> // make sure that only CONTROL shift key is pressed.
>> + // For some consoles that report shift state,
>> + // CTRL+W is still translated to L'W' - L'A' + 1.
>> //
>> - if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 'W'))
>> {
>> + if ((KeyData.Key.UnicodeChar == L'w') || (KeyData.Key.UnicodeChar == L'W')
>> + || (KeyData.Key.UnicodeChar == L'w' - L'a' + 1) ||
>> + (KeyData.Key.UnicodeChar == L'W' - L'A' + 1)) {
>> break;
>> }
>> }
>> @@ -1834,7 +1837,8 @@ MainEditorKeyInput (
>> EFI_KEY_DATA KeyData;
>> EFI_STATUS Status;
>> EFI_SIMPLE_POINTER_STATE MouseState;
>> - BOOLEAN NoShiftState;
>> + BOOLEAN NoModifierState;
>> + BOOLEAN ShiftOnlyState;
>>
>> do {
>>
>> @@ -1886,17 +1890,28 @@ MainEditorKeyInput (
>> //
>> StatusBarSetRefresh();
>> //
>> - // NoShiftState: TRUE when no shift key is pressed.
>> + // NoModifierState: TRUE when no modifier key is pressed.
>> //
>> - NoShiftState = ((KeyData.KeyState.KeyShiftState &
>> EFI_SHIFT_STATE_VALID) == 0) || (KeyData.KeyState.KeyShiftState ==
>> EFI_SHIFT_STATE_VALID);
>> + NoModifierState = ((KeyData.KeyState.KeyShiftState &
>> EFI_SHIFT_STATE_VALID) == 0)
>> + || (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID);
>> + //
>> + // ShiftOnlyState: 

[edk2-devel] [PATCH 0/1] ShellPkg: Add support for input with separately reported modifiers

[Vitaly Cheptsov via Groups.Io]

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

Some firmwares:
- Report Shift modifier even when they report upper-case unicode letter.
- Report Ctrl modifier with "shifted" UniChar (i.e. X - 'A' + 1).

This change provides support for these firmwares preserving the compatibility
with the previous input handling.

We request this to be merged in edk2-stable202002.

Vitaly Cheptsov (1):
  ShellPkg: Add support for input with separately reported modifiers

 ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c   | 37 
++--
 ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c  | 28 
++-
 ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c   |  6 
 ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c | 11 +++---
 4 files changed, 58 insertions(+), 24 deletions(-)

-- 
2.21.1 (Apple Git-122.3)


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

View/Reply Online (#54123): https://edk2.groups.io/g/devel/message/54123
Mute This Topic: https://groups.io/mt/71133730/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

Hello,

It has been quite some time since we submitted the patch with so far no 
negative response. As I mentioned previously, my team will strongly benefit 
from its landing in EDK II mainline. Since it does not add any regressions and 
can be viewed as a feature implementation for the rest of EDK II users, I 
request this to be merged upstream in edk2-stable202002.

Best wishes,
Vitaly

> 27 янв. 2020 г., в 12:47, vit9696  написал(а):
> 
> 
> Hi Mike,
> 
> Any progress with this? We would really benefit from this landing in the next 
> stable release.
> 
> Best,
> Vitaly
> 
>> 8 янв. 2020 г., в 19:35, Kinney, Michael D  
>> написал(а):
>> 
>> 
>> Hi Vitaly,
>> 
>> Thanks for the additional background.  I would like
>> a couple extra day to review the PCD name and the places
>> the PCD might potentially be used.
>> 
>> If we find other APIs where ASSERT() behavior is only
>> valuable during dev/debug to quickly identify misuse
>> with trusted data and the API provides predicable
>> return behavior when ASSERT() is disabled, then I would
>> like to have a pattern we can potentially apply to all
>> these APIs across all packages.
>> 
>> Thanks,
>> 
>> Mike
>> 
>>> -Original Message-
>>> From: devel@edk2.groups.io  On
>>> Behalf Of Vitaly Cheptsov via Groups.Io
>>> Sent: Monday, January 6, 2020 10:44 AM
>>> To: Kinney, Michael D 
>>> Cc: devel@edk2.groups.io
>>> Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to
>>> disable safe string constraint assertions
>>> 
>>> Hi Mike,
>>> 
>>> Yes, the primary use case is for UEFI Applications. We
>>> do not want to disable ASSERT’s completely, as
>>> assertions that make sense, i.e. the ones signalising
>>> about interface misuse, are helpful for debugging.
>>> 
>>> I have already explained in the BZ that basically all
>>> safe string constraint assertions make no sense for
>>> handling untrusted data. We find this use case very
>>> logical, as these functions behave properly with
>>> assertions disabled and cover all these error
>>> conditions by the return statuses. In such situation is
>>> not useful for these functions to assert, as we end up
>>> inefficiently reimplementing the logic. I would have
>>> liked the approach of discussing the interfaces
>>> individually, but I struggle to find any that makes
>>> sense from this point of view.
>>> 
>>> AsciiStrToGuid will ASSERT when the length of the
>>> passed string is odd. Functions that cannot, ahem,
>>> parse, for us are pretty much useless.
>>> AsciiStrCatS will ASSERT when the appended string does
>>> not fit the buffer. For us this logic makes this
>>> function pretty much equivalent to deprecated and thus
>>> unavailable AsciiStrCat, except it is also slower.
>>> 
>>> My original suggestion was to remove the assertions
>>> entirely, but several people here said that they use
>>> them to verify usage errors when handling trusted data.
>>> This makes good sense to me, so we suggest to support
>>> both cases by introducing a PCD in this patch.
>>> 
>>> Best wishes,
>>> Vitaly
>>> 
>>>> 6 янв. 2020 г., в 21:28, Kinney, Michael D
>>>  написал(а):
>>>> 
>>>> 
>>>> Hi Vitaly,
>>>> 
>>>> Is the use case for UEFI Applications?
>>>> 
>>>> There is a different mechanism to disable all
>>> ASSERT()
>>>> statements  within a UEFI Application.
>>>> 
>>>> If a component is consuming data from an untrusted
>>> source,
>>>> then that component is required to verify the
>>> untrusted
>>>> data before passing it to a function that clearly
>>> documents
>>>> is input requirements.  If this approach is followed,
>>> then
>>>> the BaseLib functions can be used "as is" as long as
>>> the
>>>> ASSERT() conditions are verified before calling.
>>>> 
>>>> If there are some APIs that currently document their
>>> ASSERT()
>>>> behavior and we think that ASSERT() behavior is
>>> incorrect and
>>>> should be handled by an existing error return value,
>>> then we
>>>> should discuss each of those APIs individually.
>>>> 
>>>> Mike
>>>> 
>>>> 
>>>>> -Original Messag

[edk2-devel] [PATCH 1/1] BaseTools: Switch to GNU mode for CLANGPDB

[Vitaly Cheptsov via Groups.Io]

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

Signed-off-by: Vitaly Cheptsov 
---
 BaseTools/Conf/tools_def.template | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/BaseTools/Conf/tools_def.template 
b/BaseTools/Conf/tools_def.template
index feee2bbf16..6bf6c5768e 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -2755,11 +2755,11 @@ RELEASE_CLANG38_AARCH64_DLINK_FLAGS = 
DEF(CLANG38_AARCH64_DLINK_FLAGS) -flto -Wl
 DEFINE CLANGPDB_IA32_PREFIX  = ENV(CLANG_BIN)
 DEFINE CLANGPDB_X64_PREFIX   = ENV(CLANG_BIN)
 
-DEFINE CLANGPDB_IA32_TARGET  = -target i686-unknown-windows
-DEFINE CLANGPDB_X64_TARGET   = -target x86_64-unknown-windows
+DEFINE CLANGPDB_IA32_TARGET  = -target i686-unknown-windows-gnu
+DEFINE CLANGPDB_X64_TARGET   = -target x86_64-unknown-windows-gnu
 
 DEFINE CLANGPDB_WARNING_OVERRIDES= -Wno-parentheses-equality 
-Wno-tautological-compare -Wno-tautological-constant-out-of-range-compare 
-Wno-empty-body -Wno-unused-const-variable -Wno-varargs 
-Wno-unknown-warning-option -Wno-microsoft-enum-forward-reference
-DEFINE CLANGPDB_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) 
DEF(CLANGPDB_WARNING_OVERRIDES) -fno-stack-protector -mms-bitfields 
-Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas 
-Wno-incompatible-library-redeclaration -fno-asynchronous-unwind-tables 
-mno-implicit-float  
-ftrap-function=undefined_behavior_has_been_optimized_away_by_clang 
-funsigned-char -fno-ms-extensions -Wno-null-dereference -fms-compatibility 
-mno-stack-arg-probe
+DEFINE CLANGPDB_ALL_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) 
DEF(CLANGPDB_WARNING_OVERRIDES) -fno-stack-protector 
-fno-asynchronous-unwind-tables -funsigned-char 
-ftrap-function=undefined_behavior_has_been_optimized_away_by_clang 
-Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas 
-Wno-incompatible-library-redeclaration -Wno-null-dereference 
-mno-implicit-float -mms-bitfields -mno-stack-arg-probe -nostdlib -nostdlibinc
 
 ###
 # CLANGPDB IA32 definitions
-- 
2.21.1 (Apple Git-122.3)


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

View/Reply Online (#54130): https://edk2.groups.io/g/devel/message/54130
Mute This Topic: https://groups.io/mt/71134286/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

[edk2-devel] [PATCH 0/1] BaseTools: Switch to GNU mode for CLANGPDB

[Vitaly Cheptsov via Groups.Io]

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

I did not include __clang__ removal in this patch series, as it is
also used in arm and aarch64 code, which makes me believe they could
be using specialised toolchains.

Request this to be merged in edk2-stable202002.

Vitaly Cheptsov (1):
  BaseTools: Switch to GNU mode for CLANGPDB

 BaseTools/Conf/tools_def.template | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.21.1 (Apple Git-122.3)


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

View/Reply Online (#54131): https://edk2.groups.io/g/devel/message/54131
Mute This Topic: https://groups.io/mt/71134287/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

[edk2-devel] [PATCH 0/1] BaseTools: Switch to GNU mode for CLANGPDB

[Vitaly Cheptsov via Groups.Io]

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

I did not include __clang__ removal in this patch series, as it is
also used in arm and aarch64 code, which makes me believe they could
be using specialised toolchains.

Request this to be merged in edk2-stable202002.

Vitaly Cheptsov (1):
  BaseTools: Switch to GNU mode for CLANGPDB

 BaseTools/Conf/tools_def.template | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.21.1 (Apple Git-122.3)


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

View/Reply Online (#54134): https://edk2.groups.io/g/devel/message/54134
Mute This Topic: https://groups.io/mt/71134287/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

Michael,

Generalising the approach makes good sense to me, but we need to make an 
obvious distinguishable difference between:
- precondition and invariant assertions (i.e. assertions, where function will 
NOT work if they are violated)
- constraint asserts (i.e. assertions, which allow us to spot unintentional 
behaviour when parsing untrusted data, but which do not break function 
behaviour).

As we want to use this outside of SafeString,  I suggest the following:
- Introduce DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40 bit for 
PcdDebugPropertyMask instead of PcdAssertOnSafeStringConstraints.
- Introduce DebugAssertConstraintEnabled DebugLib function to check for 
DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED.
- Introduce ASSERT_CONSTRAINT macro, that will assert only if 
DebugConstraintAssertEnabled returns true.
- Change SafeString ASSERTS in SAFE_STRING_CONSTRAINT_CHECK to 
ASSERT_CONSTRAINTs.
- Use ASSERT_CONSTRAINT in other places where necessary.

I believe this way lines best with EDK II design. If there are no objections, I 
can submit the patch in the beginning of next week.

Best wishes,
Vitaly

> 14 февр. 2020 г., в 20:00, Kinney, Michael D  
> написал(а):
> 
> Vitaly,
>
> I want to make sure a feature PCD can be used to disable ASSERT() behavior in 
> more than just safe string functions in BaseLib.
>
> Can we consider changing the name and description of 
> PcdAssertOnSafeStringConstraints to be more generic, so if we find other lib 
> APIs, the name will make sense?
>
> Maybe something like: PcdEnableLibraryAssertChecks?  Default is TRUE.  Can 
> set to FALSE in DSC file to disable ASSERT() checks.
>
> Thanks,
>
> Mike
>
>
>
> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> 
> mailto:devel@edk2.groups.io>> On Behalf Of Vitaly 
> Cheptsov via Groups.Io
> Sent: Friday, February 14, 2020 3:55 AM
> To: Kinney, Michael D  <mailto:michael.d.kin...@intel.com>>; Gao, Liming  <mailto:liming@intel.com>>; Gao, Zhichao  <mailto:zhichao@intel.com>>; devel@edk2.groups.io 
> <mailto:devel@edk2.groups.io>
> Cc: Marvin Häuser  <mailto:marvin.haeu...@outlook.com>>; Laszlo Ersek  <mailto:ler...@redhat.com>>
> Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string 
> constraint assertions
>
> Replying as per Liming's request for this to be merged into edk2-stable202002.
>
> On Mon, Feb 10, 2020 at 14:12, vit9696  <mailto:vit9...@protonmail.com>> wrote:
> Hello,
> 
> It has been quite some time since we submitted the patch with so far no 
> negative response. As I mentioned previously, my team will strongly benefit 
> from its landing in EDK II mainline. Since it does not add any regressions 
> and can be viewed as a feature implementation for the rest of EDK II users, I 
> request this to be merged upstream in edk2-stable202002.
> 
> Best wishes,
> Vitaly
> 
> > 27 янв. 2020 г., в 12:47, vit9696  > <mailto:vit9...@protonmail.com>> написал(а):
> > 
> > 
> > Hi Mike,
> > 
> > Any progress with this? We would really benefit from this landing in the 
> > next stable release.
> > 
> > Best,
> > Vitaly
> > 
> >> 8 янв. 2020 г., в 19:35, Kinney, Michael D  >> <mailto:michael.d.kin...@intel.com>> написал(а):
> >> 
> >> 
> >> Hi Vitaly,
> >> 
> >> Thanks for the additional background. I would like
> >> a couple extra day to review the PCD name and the places
> >> the PCD might potentially be used.
> >> 
> >> If we find other APIs where ASSERT() behavior is only
> >> valuable during dev/debug to quickly identify misuse
> >> with trusted data and the API provides predicable
> >> return behavior when ASSERT() is disabled, then I would
> >> like to have a pattern we can potentially apply to all
> >> these APIs across all packages.
> >> 
> >> Thanks,
> >> 
> >> Mike
> >> 
> >>> -Original Message-
> >>> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> 
> >>> mailto:devel@edk2.groups.io>> On
> >>> Behalf Of Vitaly Cheptsov via Groups.Io
> >>> Sent: Monday, January 6, 2020 10:44 AM
> >>> To: Kinney, Michael D  >>> <mailto:michael.d.kin...@intel.com>>
> >>> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> >>> Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to
> >>> disable safe string constraint assertions
> >>> 
> >>> Hi Mike,
> >>> 
> >>> Yes, the primary use case is for UEFI Applications. We
> >

Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

Michael,

The suggested changes make sense to me. I will prepare the patch in the next 
days. I guess the only question left is whether disabling assertions also 
disables constraint assertions. I think this should be the case for backwards 
compatibility, despite being slightly unintuitive.

Best,
Vitaly

On Sat, Feb 15, 2020 at 01:50, Kinney, Michael D  
wrote:

> Hi Vitaly,
>
> I agree that this proposal makes a lot of sense.  We recently added a new 
> assert type called STATIC_ASSERT() for assert conditions that can be tested 
> at build time.
>
> A new assert type for checks that can be removed and the API still guarantees 
> that it fails gracefully with a proper return code is a good idea.  Given we 
> have STATIC_ASSERT(), how about naming the new macro CONSTRAINT_ASSERT()?
>
> We also want the default to be enabled.  The current use of bit 0x40 in 
> PcdDebugPropertyMask  is always clear, so we want the asserts enabled when 
> 0x40 is clear.  We can change the name of the define bit to 
> DEBUG_PROPERTY_CONTRAINT_ASSERT_DISABLED so bit 0x40 needs to be set in 
> PcdDebugPropertyMask to disable these types of asserts.
>
> This approach does require all the DebugLib implementations to be updated 
> with the new  DebugConstraintAssertDisabled() API.
>
> Mike
>
> From: vit9696 
> Sent: Friday, February 14, 2020 9:38 AM
> To: Kinney, Michael D 
> Cc: devel@edk2.groups.io; Gao, Liming ; Gao, Zhichao 
> ; Marvin Häuser ; Laszlo 
> Ersek 
> Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string 
> constraint assertions
>
> Michael,
>
> Generalising the approach makes good sense to me, but we need to make an 
> obvious distinguishable difference between:
>
> - precondition and invariant assertions (i.e. assertions, where function will 
> NOT work if they are violated)
>
> - constraint asserts (i.e. assertions, which allow us to spot unintentional 
> behaviour when parsing untrusted data, but which do not break function 
> behaviour).
>
> As we want to use this outside of SafeString,  I suggest the following:
>
> - Introduce DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40 bit for 
> PcdDebugPropertyMask instead of PcdAssertOnSafeStringConstraints.
>
> - Introduce DebugAssertConstraintEnabled DebugLib function to check for 
> DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED.
>
> - Introduce ASSERT_CONSTRAINT macro, that will assert only if 
> DebugConstraintAssertEnabled returns true.
>
> - Change SafeString ASSERTS in SAFE_STRING_CONSTRAINT_CHECK to 
> ASSERT_CONSTRAINTs.
>
> - Use ASSERT_CONSTRAINT in other places where necessary.
>
> I believe this way lines best with EDK II design. If there are no objections, 
> I can submit the patch in the beginning of next week.
>
> Best wishes,
>
> Vitaly
>
>> 14 февр. 2020 г., в 20:00, Kinney, Michael D  
>> написал(а):
>>
>> Vitaly,
>>
>> I want to make sure a feature PCD can be used to disable ASSERT() behavior 
>> in more than just safe string functions in BaseLib.
>>
>> Can we consider changing the name and description of 
>> PcdAssertOnSafeStringConstraints to be more generic, so if we find other lib 
>> APIs, the name will make sense?
>>
>> Maybe something like: PcdEnableLibraryAssertChecks?  Default is TRUE.  Can 
>> set to FALSE in DSC file to disable ASSERT() checks.
>>
>> Thanks,
>>
>> Mike
>>
>> From: devel@edk2.groups.io  On Behalf Of Vitaly 
>> Cheptsov via Groups.Io
>> Sent: Friday, February 14, 2020 3:55 AM
>> To: Kinney, Michael D ; Gao, Liming 
>> ; Gao, Zhichao ; 
>> devel@edk2.groups.io
>> Cc: Marvin Häuser ; Laszlo Ersek 
>> 
>> Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string 
>> constraint assertions
>>
>> Replying as per Liming's request for this to be merged into 
>> edk2-stable202002.
>>
>> On Mon, Feb 10, 2020 at 14:12, vit9696  wrote:
>>
>>> Hello,
>>>
>>> It has been quite some time since we submitted the patch with so far no 
>>> negative response. As I mentioned previously, my team will strongly benefit 
>>> from its landing in EDK II mainline. Since it does not add any regressions 
>>> and can be viewed as a feature implementation for the rest of EDK II users, 
>>> I request this to be merged upstream in edk2-stable202002.
>>>
>>> Best wishes,
>>> Vitaly
>>>
>>>> 27 янв. 2020 г., в 12:47, vit9696  написал(а):
>>>>
>>>>
>>>> Hi Mike,
>>>>
>>>> Any progress with this? We would really benefit from this landing in the 
>>>> next 

Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

g2 != NULL, ErrorExit, Status = EFI_INVALID_PARAMETER);
>   REQUIRE_ACTION(Arg3 != NULL, ErrorExit, Status = EFI_INVALID_PARAMETER);
>
> ErrorExit:
>   return Status;
>
> If you use CONSTRAINT_ASSERT();
>
>   if (Arg1 == NULL || Arg2 == NULL || Arg3 == NULL) {
>CONSTRAINT_ASSERT (Arg1 != NULL);
>CONSTRAINT_ASSERT (Arg2 != NULL);
>CONSTRAINT_ASSERT (Arg3 != NULL);
>return EFI_INVALID_PARAMETER;
>  }
>
> I'd note error processing args on entry is the simplest case.  In a more 
> complex case when cleanup is required the goto label is more useful.
>
> I guess we could argue for less typing and more symmetry and say use ASSERT, 
> CONSTRAINT, and REQUIRE. I guess you could add an ASSERT_ACTION too.
>
> The AssertMacros.h versions also support _quiet (skip the print) and _string 
> (add a string to the print) so you end up with:
> REQUIRE
> REQUIRE_STRING
>
> REQUIRE_QUIET
>
> REQUIRE_ACTION
>
> REQUIRE_ACTION_STRING
>
> REQUIRE_ACTION_QUIET
>
> We could also end up with
> CONSTRAINT
> CONSTRAINT_STRING
>
> CONSTRAINT_QUIET
>
> I think the main idea behind _QUIET is you can silence things that are too 
> noisy, and you can easily make noise things show up by removing the _QUIET to 
> debug.
>
> I'd thought I throw out the other forms for folks to think about. I'm 
> probably biased as I used to looking at code and seeing things like 
> require_action_string(Arg1 != NULL, ErrorExit, Status = 
> EFI_INVALID_PARAMETER, "1st Arg1 check");
>
> Thanks,
>
> Andrew Fish
>
> PS The old debug macros had 2 versions of CONSTRAINT check and verify. The 
> check version was compiled out on a release build, the verify version always 
> does the check and just skips the DEBUG print.
>
>> Mike
>>
>> From: vit9696 
>> Sent: Friday, February 14, 2020 9:38 AM
>> To: Kinney, Michael D 
>> Cc: devel@edk2.groups.io; Gao, Liming ; Gao, Zhichao 
>> ; Marvin Häuser ; Laszlo 
>> Ersek 
>> Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string 
>> constraint assertions
>>
>> Michael,
>>
>> Generalising the approach makes good sense to me, but we need to make an 
>> obvious distinguishable difference between:
>> - precondition and invariant assertions (i.e. assertions, where function 
>> will NOT work if they are violated)
>> - constraint asserts (i.e. assertions, which allow us to spot unintentional 
>> behaviour when parsing untrusted data, but which do not break function 
>> behaviour).
>>
>> As we want to use this outside of SafeString,  I suggest the following:
>> - Introduce DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40 bit for 
>> PcdDebugPropertyMask instead of PcdAssertOnSafeStringConstraints.
>> - Introduce DebugAssertConstraintEnabled DebugLib function to check for 
>> DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED.
>> - Introduce ASSERT_CONSTRAINT macro, that will assert only if 
>> DebugConstraintAssertEnabled returns true.
>> - Change SafeString ASSERTS in SAFE_STRING_CONSTRAINT_CHECK to 
>> ASSERT_CONSTRAINTs.
>> - Use ASSERT_CONSTRAINT in other places where necessary.
>>
>> I believe this way lines best with EDK II design. If there are no 
>> objections, I can submit the patch in the beginning of next week.
>>
>> Best wishes,
>> Vitaly
>>
>>> 14 февр. 2020 г., в 20:00, Kinney, Michael D  
>>> написал(а):
>>>
>>> Vitaly,
>>>
>>> I want to make sure a feature PCD can be used to disable ASSERT() behavior 
>>> in more than just safe string functions in BaseLib.
>>>
>>> Can we consider changing the name and description of 
>>> PcdAssertOnSafeStringConstraints to be more generic, so if we find other 
>>> lib APIs, the name will make sense?
>>>
>>> Maybe something like: PcdEnableLibraryAssertChecks?  Default is TRUE.  Can 
>>> set to FALSE in DSC file to disable ASSERT() checks.
>>>
>>> Thanks,
>>>
>>> Mike
>>>
>>> From: devel@edk2.groups.io  On Behalf Of Vitaly 
>>> Cheptsov via Groups.Io
>>> Sent: Friday, February 14, 2020 3:55 AM
>>> To: Kinney, Michael D ; Gao, Liming 
>>> ; Gao, Zhichao ; 
>>> devel@edk2.groups.io
>>> Cc: Marvin Häuser ; Laszlo Ersek 
>>> 
>>> Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string 
>>> constraint assertions
>>>
>>> Replying as per Liming's request for this to be merged into 
>>> edk2-stable202002.
>>>
>>> On Mon, Feb 10, 2020 at 14:1

Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

It seems that edk2.groups.io  has hit the limits for 
message size due to quotation levels (?), so I send another copy of my previous 
message to have it visible from the web-interface.

Andrew,

It is ok, as we are all here for mutual benefit, no worries. But you are right 
that we indeed want a solution for BZ 2054[1] to land as early as possible. It 
is great that after more than half a year :) we have some productive 
discussion, so I am all open for it.

* The compliant about a breaking change in DebugLib is honestly fair, and I 
wonder whether we could avoid it. The reason it potentially breaks is the 
introduction of DebugConstraintAssertEnabled function that will need to land in 
every DebugLib implementation. I think we can look differently at this problem. 
Currently functions like DebugAssertEnabled, DebugPrintEnabled, 
DebugClearMemoryEnabled, and so on are copy-pasted from one DebugLib to another 
introducing almost 100 lines of exactly the same code every DebugLib 
implementation should write. It can be argued that one can implement these 
functions differently, but neither the description in DebugLib.h permits this, 
nor I have seen any implementation doing it so far. I believe we should check 
Pcd values directly in DebugLib.h header, which has a lot of pros including the 
backwards compatibility resolution:

— reduction of code copy-pasting.
— making compilers without LTO produce smaller binaries.
— making third-party DebugLib implementations more flexible and easier to 
maintain.
— resolving the problem of third-party DebugLib implementations not working 
with CONSTRAINT_ASSERT.

To make it less intrusive we can provide an iterative approach:
1. Introduce Pcd checking macros like DEBUG_PRINT_ENABLED, DEBUG_ASSERT_ENABLED:
#define DEBUG_PRINT_ENABLED() (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) & 
DEBUG_PROPERTY_DEBUG_PRINT_ENABLED) != 0)
2. Switch DEBUG, ASSERT, and other related macros to use these new macros by 
replacing calls like DebugPrintEnabled() with macros like DEBUG_PRINT_ENABLED().
3. Introduce DEBUG_CONSTRAINT_ASSERT_ENABLED and CONSTRAINT_ASSERT as discussed 
previously except DebugConstraintAssertEnabled is now a macro.
4. Remove DebugPrintEnabled and other functions from EDK II DebugLib 
implementations and header.

Step 4 can be done through deprecation phase. However, I think these functions 
are not used anywhere but inside DebugLib implementations anyway, and the way 
they are structured should let still legacy third-party DebugLib 
implementations still having these functions to compile fine even without the 
immediate refactoring.

* The point of AssertMacros and changing the world is valid, but to be frank 
with you I have an opinion that we should not incorporate this experience in 
EDK II. I do not know if these macros are used in Mac EFI code, as my only 
experience with them was during XNU kernelspace programming, but I do not have 
a memory of them bringing enough benefit. Basically, macro constructions that 
cannot be easily expressed with normal C functions, especially ones changing 
control flow with something like a goto, are unintuitive and overcomplicated. 
They make the code harder to read, especially to those not familiar with the 
rest of the codebase, for little to no reason. While there certainly may be 
some positive sides in these macros, like improved structural separation by 
introducing different categories of checks, I am afraid we do not need them as 
is. Even if we do implement something close in the future, I believe they 
should rather exist in a separate library and be an opt-in for newly 
contributed code.

* The point of caller/callee control on the constraint violation is slightly 
tricky. We have already spent some time trying to nail it down, and I believe 
there is a good level of logic that can be split in two parts:

1. Assertion type choose. Which to write ASSERT or CONSTRAINT_ASSERT?

You can rely on a simple thought experiment here. Does failing this assertion 
make the function work in the conditions it was not meant to be? I.e. will the 
function break or crash or will some other spec/doc-compliant implementations 
of this potentially do. When the answer is affirmative, it is an ASSERT, 
otherwise it is a CONSTRAINT_ASSERT. Examples in the BZ mentioning SafeString 
give a good grasp of the use cases.

2. Assertion handling choice. When do we want ASSERT or CONSTRAINT_ASSERT to be 
enabled?

This is more likely what your question was about. First is that 
CONSTRAINT_ASSERTs only make sense to enable in DEBUG builds where ASSERTs are 
enabled. So the question comes to whether DEBUG builds should behave the same 
way as RELEASE builds functionality-wise or not. Let’s take an another simple 
thought experiment: should we halt in a build when external untrusted data 
theoretically exists and is not valid? If the answer is yes, then 
CONSTRAINT_ASSERT should be enabled in a DEBUG build, otherwise no. 

Re: [edk2-devel] Patch List for 202002 stable tag

[Vitaly Cheptsov via Groups.Io]

Liming, no problem from our side. The patch is now reviewed and I believe I 
provided all the necessarily material regarding its status.

In case Ray would rather postpone it, I give no objection to this without prior 
notice. There is no problem from our side if EDK II team wants to prioritise 
other issues, we can always merge it right after the stable tag lands.

Best wishes,
Vitaly

В чт, февр. 20, 2020 в 09:58, Gao, Liming  пишет:

> Ray, Zhichao and Vitaly:
>
>   Thanks. BZ is the good place to catch all discussion. Because we are close 
> to edk2 stable tag 202002, can you make the agreement soon for BZ 2510?
>
> Thanks
>
> Liming
>
> From: Ni, Ray 
> Sent: Thursday, February 20, 2020 11:13 AM
> To: Gao, Liming ; vit9696 
> Cc: Kinney, Michael D ; Laszlo Ersek 
> ; Guptha, Soumya K ; 
> l...@nuviainc.com; af...@apple.com; devel@edk2.groups.io; Marvin Häuser 
> ; Gao, Zhichao 
> Subject: RE: Patch List for 202002 stable tag
>
> Liming,
>
> I provided my comments in the BZ.
>
> From: Gao, Liming 
> Sent: Thursday, February 20, 2020 9:17 AM
> To: vit9696 
> Cc: Kinney, Michael D ; Laszlo Ersek 
> ; Guptha, Soumya K ; 
> l...@nuviainc.com; af...@apple.com; devel@edk2.groups.io; Marvin Häuser 
> ; Ni, Ray ; Gao, Zhichao 
> 
> Subject: RE: Patch List for 202002 stable tag
>
> Vitaly:
>
>   I add my comments.
>
> Zhichao and Ray:
>
>Can you give your opinion for BZ 
> https://bugzilla.tianocore.org/show_bug.cgi?id=2510? Is it a bug fix or 
> feature enhancement?
>
> Thanks
>
> Liming
>
> From: vit9696 
> Sent: Thursday, February 20, 2020 2:09 AM
> To: Gao, Liming 
> Cc: Kinney, Michael D ; Laszlo Ersek 
> ; Guptha, Soumya K ; 
> l...@nuviainc.com; af...@apple.com; devel@edk2.groups.io; Marvin Häuser 
> 
> Subject: Re: Patch List for 202002 stable tag
>
> Liming,
>
> Thanks for pinging me about this!
>
> With the PCD[1][2] I fully agree. The fact that it did not manage to land is 
> mainly due to a sudden discussion that arose after complete silence for 
> almost half a year, which was sort of unexpected. I will use this message as 
> a suggestion to include this change as one of the primary goals for 202005 
> and kindly ask others to help to agree on the actual implementation. This bug 
> strongly concerns us and we believe the fact that it does not (yet) cause 
> issues to everyone is mainly coincidence.
>
> [Liming] You can also present the topic in Tiano Design meeting to collect 
> the feedback. Ni, Ray is the meeting host. You can send the topic to him.
>
> With the Shell patch, the fact that I cannot enter upper case letters or use 
> hotkeys in the editor sounds like a bug to me. The way the actual commit 
> message is written reflects the change of the internal logic in the codebase 
> (it adds support of specific behaviour handling on the target). In my 
> opinion, it should not necessarily include the word «Fix» to be qualified as 
> a bugfix, this is what bugzilla is for.
>
> [Liming] If this fix is the bug, I agree it follows the process to catch this 
> stable tag. I include ShellPkg maintainers (Ray Ni and Zhichao Gao) to give 
> the opinion for the bug or not.
>
> I am personally ok with deferring it to a next stable tag, but if the 
> reasoning for this is «Feature planning freeze» dates, they do not strictly 
> apply due to the reasons I stated above. So far the patch received only one 
> review comment, which in fact was due to a minor misinterpretation. We also 
> did some fairly extensive testing on our side before the submission (that’s 
> why it actually took us a few more days). Unless the team has a lot of 
> important work for the release, we can postpone the merge, otherwise I think 
> it should be safe to merge this.
>
> Best wishes,
>
> Vitaly
>
> [1] https://bugzilla.tianocore.org/show_bug.cgi?id=2054
>
> [2] https://edk2.groups.io/g/devel/topic/69401948
>
>> 19 февр. 2020 г., в 18:39, Gao, Liming  написал(а):
>>
>> Mike and Laszlo:
>>  Thanks for your comments.
>>
>> Vitaly:
>>  You request below two patches to catch 202002 stable tag. I agree with Mike 
>> and Laszlo comments. They are not ready to catch this stable tag. The first 
>> one is under discussion. The second one is like the enhancement or feature 
>> instead of the bug fix. It is submitted after Feb 7th Feature Planning 
>> Freeze. So, I suggest to defer them to next stable tag 202005.
>>
>> https://edk2.groups.io/g/devel/topic/patch_v3_0_1_add_pcd_to/69401948 [PATCH 
>> v3 0/1] Add PCD to disable safe string constraint assertions (solution under 
>> discussion)
>> https://edk2.groups.io/g/devel/message/54122 [PATCH 1/1] ShellPkg: Add 
>> support for input with separately reported modifiers (under review, is this 
>> a feature or bug in the discussion)
>>
>> Thanks
>> Liming
>>
>>> -Original Message-
>>> From: Kinney, Michael D 
>>> Sent: Wednesday, February 19, 2020 4:43 AM
>>> To: Laszlo Ersek ; Gao, Liming ; 
>>> Guptha, Soumya K ;
>>> l...@nuviainc.com; af...@apple.com; Kinney, Michael D 
>>> 

[edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without device paths

[Vitaly Cheptsov via Groups.Io]

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

Doing this reduces the amount of needless work during device
connection and resolves issues with firmwares that freeze
when connecting handles without device paths.

Signed-off-by: Vitaly Cheptsov 
---
 ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c 
b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
index b6e7c952fa..083aac0dba 100644
--- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
+++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
@@ -32,8 +32,8 @@ ConnectAllEfi (
   UINTN   Index;
 
   Status = gBS->LocateHandleBuffer (
-  AllHandles,
-  NULL,
+  ByProtocol,
+  ,
   NULL,
   ,
   
-- 
2.21.0 (Apple Git-122.2)


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

View/Reply Online (#53170): https://edk2.groups.io/g/devel/message/53170
Mute This Topic: https://groups.io/mt/69653841/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

[edk2-devel] [PATCH 0/1] ShellPkg: Do not connect handles without device paths

[Vitaly Cheptsov via Groups.Io]

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

Doing this reduces the amount of needless work during device
connection and resolves issues with firmwares that freeze
when connecting handles without device paths.

We request this to be merged in edk2-stable202002.

Vitaly Cheptsov (1):
  ShellPkg: Do not connect handles without device paths

 ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.21.0 (Apple Git-122.2)


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

View/Reply Online (#53171): https://edk2.groups.io/g/devel/message/53171
Mute This Topic: https://groups.io/mt/69653842/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without device paths

[Vitaly Cheptsov via Groups.Io]

Hi,

‘Freeze’ means it is hung up forever, and we believe it is caused by an invalid 
memory access in the firmware code.

Using loading command with ‘-nc’ (no connect) will work fine, but that is kind 
of expected, as the issue is caused exactly by the connect logic.

We do not believe it is possible for device controller to exist without a 
device path protocol in the wild. Would you please provide an example? If there 
is something we do not know about, I can imagine introducing a PCD for this 
logic.

Best wishes,
Vitaly

> 13 янв. 2020 г., в 11:11, Gao, Zhichao  написал(а):
> 
> 
> Hi,
> 
> What does 'freeze' mean? System blocked for a while or it hung up forever?
> 
> The change would affect the operation of the 'load' command. If some drivers 
> need to connect to the device controller but it doesn't have a device path 
> protocol, the behavior of 'load' may be incorrect.
> Is the option '-nc' working for your request?
> 
> Thanks,
> Zhichao
> 
>> -Original Message-
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Vitaly Cheptsov via Groups.Io
>> Sent: Monday, January 13, 2020 5:39 AM
>> To: devel@edk2.groups.io
>> Subject: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without
>> device paths
>> 
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2460
>> 
>> Doing this reduces the amount of needless work during device connection
>> and resolves issues with firmwares that freeze when connecting handles
>> without device paths.
>> 
>> Signed-off-by: Vitaly Cheptsov 
>> ---
>> ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
>> b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
>> index b6e7c952fa..083aac0dba 100644
>> --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
>> +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
>> @@ -32,8 +32,8 @@ ConnectAllEfi (
>>   UINTN   Index;
>> 
>>   Status = gBS->LocateHandleBuffer (
>> -  AllHandles,
>> -  NULL,
>> +  ByProtocol,
>> +  ,
>>   NULL,
>>   ,
>>   
>> --
>> 2.21.0 (Apple Git-122.2)
>> 
>> 
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>> 
>> View/Reply Online (#53170): https://edk2.groups.io/g/devel/message/53170
>> Mute This Topic: https://groups.io/mt/69653841/1768756
>> Group Owner: devel+ow...@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub
>> [zhichao@intel.com]
>> -=-=-=-=-=-=
> 


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

View/Reply Online (#53187): https://edk2.groups.io/g/devel/message/53187
Mute This Topic: https://groups.io/mt/69653841/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without device paths

[Vitaly Cheptsov via Groups.Io]

Thanks all for your input,

These explanations seem sufficient to us that it is not a good idea to change 
the behaviour for everyone. Even so, we still need this to be configurable in 
some way, as having to patch EDK II is impracticable.

We believe there are three possible routes to approach this problem:

- Introduce a separate ControllerConnectionLib library for this function. While 
it is small, we found several places in our code that need to call it beyond 
UEFI Shell. This way different implementations could be used depending on the 
chosen library.
- Introduce a ConnectRequiresDevicePath PCD, which will choose the preferred 
logic.
- Introduce a -dp Shell argument for affected commands the way Lazslo suggested.

We believe either route or a combination of multiple routes have their own 
benefits, and would suggest either going with 1+2 or with 3. Any approach is 
fine for us.

We will submit V2 of the patch after hearing the opinions.

Best wishes,

Vitaly

On Mon, Jan 13, 2020 at 20:55, Laszlo Ersek  wrote:

> On 01/13/20 12:56, Ni, Ray wrote:
>> We shouldn't assume that a DriverBindingStart() can only start on a handle 
>> with device path installed. DevicePath protocol is just a special protocol.
>> It's possible that a bus driver starts on a host controller handle and 
>> creates multiple children, each with only a Specific_IO protocol installed.
>> Certain device driver can start on the children handle and open the 
>> Specific_IO protocol BY_DRIVER.
>> I am not sure if certain today's network drivers may work like this. It's 
>> allowed per UEFI spec.
>
> I agree.
>
> Under "10.2 EFI Device Path Protocol", the spec says, "If the handle
> does not logically map to a physical device, the handle may not
> necessarily support the device path protocol."
>
> I think gBS->ConnectController() and
> EFI_DRIVER_BINDING_PROTOCOL.Supported() should work on such handles.
>
> If we'd like to work around related issues in drivers, then I'd suggest
> new command line options for the "load", "connect", "reconnect" shell
> commands (maybe more), for filtering out handles that do not carry
> device paths. Such command line options could be added as an extension,
> IIUC, such as "-_option". I.e., I believe it's not necessary to start
> with UEFI Shell Spec updates.
>
> Thanks
> Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53201): https://edk2.groups.io/g/devel/message/53201
Mute This Topic: https://groups.io/mt/69653841/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without device paths

[Vitaly Cheptsov via Groups.Io]

Ray,

We are quite reluctant to have patches in EDK II for a large amount of widely 
adopted firmwares. Patches eventually break and require maintenance cost, and 
currently we are trying to get rid of them all. We believe that EDK II Shell is 
supposed to work on real world platforms and not only the ones that 
theoretically support the specification. It is always hard to adopt changes 
based on third-party bugs, and we very well understand your concern, yet it is 
something we have to do to stay beneficial to the end user.

Best wishes,
Vitaly

On Tue, Jan 14, 2020 at 05:53, Ni, Ray  wrote:

> Vitaly,
>
> I still have concern to modify the EDKII code to workaround a firmware bug.
>
> Can you just change in your local version?
>
> Thanks,
>
> Ray
>
> From: devel@edk2.groups.io   On Behalf Of Vitaly 
> Cheptsov via Groups.Io
> Sent: Tuesday, January 14, 2020 4:47 AM
> To: Laszlo Ersek ; devel@edk2.groups.io; Ni, Ray 
> ; Gao, Zhichao 
> Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles 
> without device paths
>
> Thanks all for your input,
>
> These explanations seem sufficient to us that it is not a good idea to change 
> the behaviour for everyone. Even so, we still need this to be configurable in 
> some way, as having to patch EDK II is impracticable.
>
> We believe there are three possible routes to approach this problem:
>
> -  Introduce a separate ControllerConnectionLib library for this function. 
> While it is small, we found several places in our code that need to call it 
> beyond UEFI Shell. This way different implementations could be used depending 
> on the chosen library.
> -  Introduce a ConnectRequiresDevicePath PCD, which will choose the preferred 
> logic.
> -  Introduce a -dp Shell argument for affected commands the way Lazslo 
> suggested.
>
> We believe either route or a combination of multiple routes have their own 
> benefits, and would suggest either going with 1+2 or with 3. Any approach is 
> fine for us.
>
> We will submit V2 of the patch after hearing the opinions.
>
> Best wishes,
>
> Vitaly
>
> On Mon, Jan 13, 2020 at 20:55, Laszlo Ersek  wrote:
>
>> On 01/13/20 12:56, Ni, Ray wrote:
>>> We shouldn't assume that a DriverBindingStart() can only start on a handle 
>>> with device path installed. DevicePath protocol is just a special protocol.
>>> It's possible that a bus driver starts on a host controller handle and 
>>> creates multiple children, each with only a Specific_IO protocol installed.
>>> Certain device driver can start on the children handle and open the 
>>> Specific_IO protocol BY_DRIVER.
>>> I am not sure if certain today's network drivers may work like this. It's 
>>> allowed per UEFI spec.
>>
>> I agree.
>>
>> Under "10.2 EFI Device Path Protocol", the spec says, "If the handle
>> does not logically map to a physical device, the handle may not
>> necessarily support the device path protocol."
>>
>> I think gBS->ConnectController() and
>> EFI_DRIVER_BINDING_PROTOCOL.Supported() should work on such handles.
>>
>> If we'd like to work around related issues in drivers, then I'd suggest
>> new command line options for the "load", "connect", "reconnect" shell
>> commands (maybe more), for filtering out handles that do not carry
>> device paths. Such command line options could be added as an extension,
>> IIUC, such as "-_option". I.e., I believe it's not necessary to start
>> with UEFI Shell Spec updates.
>>
>> Thanks
>> Laszlo
>
> 
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53210): https://edk2.groups.io/g/devel/message/53210
Mute This Topic: https://groups.io/mt/69653841/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles without device paths

[Vitaly Cheptsov via Groups.Io]

In addition to my previous letter I have to mention a couple more newly 
discovered details.

1. UEFI Shell (ShellPkg) actually has 3 functions dedicated to connecting 
controllers and essentially doing the same thing:

- ConnectAllEfi
ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c

This one locates all handles and runs connect on them. It is the one we 
mentioned in the bug.

- LoadPciRomConnectAllDriversToAllControllers
ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c

This one is similar to ConnectAllEfi. The only difference is that it 
additionally checks for Ctrl+C via ShellGetExecutionBreakFlag.

- ConnectControllers
ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c

This one is more complex, as it supports explicitly connecting specified 
controllers, however, for connecting all controllers it locates handles with 
gEfiDevicePathProtocolGuid. I.e. exactly what we ask. I believe that all these 
functions should behave the same way in correspondence to 
gBS->ConnectController at the very least, and most likely there should be a 
library responsible for connection and disconnection.

2. We also checked EFI 1.1 Shell, and confirmed that it consistently has checks 
for device path presence before running connect on the handle[1][2]. This makes 
us believe that our proposal is not really a firmware bug, but rather a 
limitation of the legacy specification.

Considering all these discoveries, we believe our suggested change is legit 
depending on the minimum supported UEFI version. Therefore we propose 
submitting a ControllerConnection library with 5 functions:

- ConnectController
- DisconnectController
- GetAllControllers
- ConnectAllControllers
- DisconnectAllControllers

Adopting it throughout the code will let the distribitor to be responsible for 
choosing what is right for his applications (or firmwares).

Best wishes,
Vitaly

[1] 
https://sourceforge.net/p/efi-shell/code/64/tree/trunk/Shell/LoadPciRom/LoadPciRom.c#l528
[2] 
https://sourceforge.net/p/efi-shell/code/64/tree/trunk/Shell/load/load.c#l312


> 14 янв. 2020 г., в 11:36, vit9696  написал(а):
> 
> Ray,
> 
> We are quite reluctant to have patches in EDK II for a large amount of widely 
> adopted firmwares. Patches eventually break and require maintenance cost, and 
> currently we are trying to get rid of them all. We believe that EDK II Shell 
> is supposed to work on real world platforms and not only the ones that 
> theoretically support the specification. It is always hard to adopt changes 
> based on third-party bugs, and we very well understand your concern, yet it 
> is something we have to do to stay beneficial to the end user.
> 
> Best wishes,
> Vitaly
> 
> On Tue, Jan 14, 2020 at 05:53, Ni, Ray  <mailto:ray...@intel.com>> wrote:
>> 
>> Vitaly,
>> 
>> I still have concern to modify the EDKII code to workaround a firmware bug.
>> 
>> Can you just change in your local version?
>> 
>>
>> Thanks,
>> 
>> Ray
>> 
>>
>> From: devel@edk2.groups.io  On Behalf Of Vitaly 
>> Cheptsov via Groups.Io
>> Sent: Tuesday, January 14, 2020 4:47 AM
>> To: Laszlo Ersek ; devel@edk2.groups.io; Ni, Ray 
>> ; Gao, Zhichao 
>> Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Do not connect handles 
>> without device paths
>> 
>>
>> Thanks all for your input,
>>
>> These explanations seem sufficient to us that it is not a good idea to 
>> change the behaviour for everyone. Even so, we still need this to be 
>> configurable in some way, as having to patch EDK II is impracticable.
>>
>> We believe there are three possible routes to approach this problem:
>>
>> Introduce a separate ControllerConnectionLib library for this function. 
>> While it is small, we found several places in our code that need to call it 
>> beyond UEFI Shell. This way different implementations could be used 
>> depending on the chosen library.
>> Introduce a ConnectRequiresDevicePath PCD, which will choose the preferred 
>> logic.
>> Introduce a -dp Shell argument for affected commands the way Lazslo 
>> suggested.
>>
>> We believe either route or a combination of multiple routes have their own 
>> benefits, and would suggest either going with 1+2 or with 3. Any approach is 
>> fine for us.
>>
>> We will submit V2 of the patch after hearing the opinions.
>>
>> Best wishes,
>> Vitaly
>>
>> On Mon, Jan 13, 2020 at 20:55, Laszlo Ersek > <mailto:ler...@redhat.com>> wrote:
>> 
>> On 01/13/20 12:56, Ni, Ray wrote:
>> > We shouldn't assume that a DriverBindingStart() can only start on a handle 
>> > with device path installed. DevicePath protocol is just a special protocol.
>> 

[edk2-devel] [PATCH v1 0/1] MdePkg: Do not use CreateEventEx unless required

[Vitaly Cheptsov via Groups.Io]

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

Vitaly Cheptsov (1):
  MdePkg: Do not use CreateEventEx unless required

 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c |  3 +--
 MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c |  5 ++---
 MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLib.c  |  5 ++---
 MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c|  5 ++---
 MdePkg/Library/UefiDebugLibDebugPortProtocol/DebugLibConstructor.c |  5 ++---
 MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c|  5 ++---
 MdePkg/Library/UefiRuntimeLib/RuntimeLib.c | 10 
--
 7 files changed, 15 insertions(+), 23 deletions(-)

-- 
2.21.0 (Apple Git-122.2)


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

View/Reply Online (#52840): https://edk2.groups.io/g/devel/message/52840
Mute This Topic: https://groups.io/mt/69403120/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

[edk2-devel] [PATCH v1 1/1] MdePkg: Do not use CreateEventEx unless required

[Vitaly Cheptsov via Groups.Io]

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

There are many firmwares in the wild not supporting CreateEventEx,
including devices less than 5 years old.

Signed-off-by: Vitaly Cheptsov 
---
 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c |  3 +--
 MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c |  5 ++---
 MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLib.c  |  5 ++---
 MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c|  5 ++---
 MdePkg/Library/UefiDebugLibDebugPortProtocol/DebugLibConstructor.c |  5 ++---
 MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c|  5 ++---
 MdePkg/Library/UefiRuntimeLib/RuntimeLib.c | 10 
--
 7 files changed, 15 insertions(+), 23 deletions(-)

diff --git a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c 
b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
index 862c6bff09..cc79843b1c 100644
--- a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
+++ b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
@@ -77,9 +77,8 @@ DxeRuntimeDebugLibSerialPortConstructor (
 return Status;
   }
 
-  return SystemTable->BootServices->CreateEventEx (EVT_NOTIFY_SIGNAL,
+  return SystemTable->BootServices->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES,
   TPL_NOTIFY, ExitBootServicesEvent, NULL,
-  ,
   );
 }
 
diff --git a/MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c 
b/MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c
index 6e784763be..7e5852e641 100644
--- a/MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c
+++ b/MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c
@@ -124,12 +124,11 @@ DxeRuntimePciExpressLibConstructor (
   //
   // Register SetVirtualAddressMap () notify function
   //
-  Status = gBS->CreateEventEx (
-  EVT_NOTIFY_SIGNAL,
+  Status = gBS->CreateEvent (
+  EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE,
   TPL_NOTIFY,
   DxeRuntimePciExpressLibVirtualNotify,
   NULL,
-  ,
   
   );
   ASSERT_EFI_ERROR (Status);
diff --git a/MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLib.c 
b/MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLib.c
index 2f503ecffe..b0dbdec0cf 100644
--- a/MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLib.c
+++ b/MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLib.c
@@ -109,12 +109,11 @@ DxeRuntimePciSegmentLibConstructor (
   //
   // Register SetVirtualAddressMap () notify function
   //
-  Status = gBS->CreateEventEx (
-  EVT_NOTIFY_SIGNAL,
+  Status = gBS->CreateEvent (
+  EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE,
   TPL_NOTIFY,
   DxeRuntimePciSegmentLibVirtualNotify,
   NULL,
-  ,
   
   );
   ASSERT_EFI_ERROR (Status);
diff --git a/MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c 
b/MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c
index ed73f92818..b4ac17cf55 100644
--- a/MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c
+++ b/MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c
@@ -64,12 +64,11 @@ DxeDebugLibConstructor(
 {
   mDebugST = SystemTable;
 
-  SystemTable->BootServices->CreateEventEx (
-EVT_NOTIFY_SIGNAL,
+  SystemTable->BootServices->CreateEvent (
+EVT_SIGNAL_EXIT_BOOT_SERVICES,
 TPL_NOTIFY,
 ExitBootServicesCallback,
 NULL,
-,
 
 );
 
diff --git a/MdePkg/Library/UefiDebugLibDebugPortProtocol/DebugLibConstructor.c 
b/MdePkg/Library/UefiDebugLibDebugPortProtocol/DebugLibConstructor.c
index 6ea0912f2b..96fc1c422f 100644
--- a/MdePkg/Library/UefiDebugLibDebugPortProtocol/DebugLibConstructor.c
+++ b/MdePkg/Library/UefiDebugLibDebugPortProtocol/DebugLibConstructor.c
@@ -64,12 +64,11 @@ DxeDebugLibConstructor(
 {
   mDebugBS = SystemTable->BootServices;
 
-  mDebugBS->CreateEventEx (
-  EVT_NOTIFY_SIGNAL,
+  mDebugBS->CreateEvent (
+  EVT_SIGNAL_EXIT_BOOT_SERVICES,
   TPL_NOTIFY,
   ExitBootServicesCallback,
   NULL,
-  ,
   
   );
 
diff --git a/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c 
b/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
index ed73f92818..b4ac17cf55 100644
--- a/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
+++ b/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
@@ -64,12 +64,11 @@ DxeDebugLibConstructor(
 {
   mDebugST = SystemTable;
 
-  

[edk2-devel] [PATCH v3 1/1] MdePkg: Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

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

Runtime data checks are not meant to cause debug assertions
unless explicitly needed by some debug code (thus the PCD)
as this breaks debug builds validating data with BaseLib.

Signed-off-by: Vitaly Cheptsov 
---
 MdePkg/MdePkg.dec   |  6 ++
 MdePkg/Library/BaseLib/BaseLib.inf  | 11 +--
 MdePkg/Include/Library/BaseLib.h| 74 +---
 MdePkg/Library/BaseLib/SafeString.c |  4 +-
 MdePkg/MdePkg.uni   |  6 ++
 5 files changed, 71 insertions(+), 30 deletions(-)

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index d022cc5e3e..0191b7a08b 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2221,6 +2221,12 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
   # @Prompt Memory Address of GuidedExtractHandler Table.
   
gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|0x100|UINT64|0x30001015
 
+  ## Indicates if safe string constraint violation should assert.
+  #   TRUE  - Safe string constraint violation causes assertion.
+  #   FALSE - Safe string constraint violation does not cause assertion.
+  # @Prompt Enable safe string constraint violation assertions.
+  
gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints|TRUE|BOOLEAN|0x002e
+
 [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## This value is used to set the base address of PCI express hierarchy.
   # @Prompt PCI Express Base Address.
diff --git a/MdePkg/Library/BaseLib/BaseLib.inf 
b/MdePkg/Library/BaseLib/BaseLib.inf
index 3586beb0ab..bc98bc6134 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -390,11 +390,12 @@ [LibraryClasses]
   BaseMemoryLib
 
 [Pcd]
-  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength  ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength   ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask   ## 
SOMETIMES_CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType   ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints   ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength  ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask  ## 
SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType  ## 
SOMETIMES_CONSUMES
 
 [FeaturePcd]
   gEfiMdePkgTokenSpaceGuid.PcdVerifyNodeInList  ## CONSUMES
diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 2a75bc023f..c413ca5f57 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -189,6 +189,8 @@ StrnSizeS (
 
   If Destination is not aligned on a 16-bit boundary, then ASSERT().
   If Source is not aligned on a 16-bit boundary, then ASSERT().
+
+  Unless PcdAssertOnSafeStringConstraints is set to FALSE:
   If an error would be returned, then the function will also ASSERT().
 
   If an error is returned, then the Destination is unmodified.
@@ -225,6 +227,8 @@ StrCpyS (
 
   If Length > 0 and Destination is not aligned on a 16-bit boundary, then 
ASSERT().
   If Length > 0 and Source is not aligned on a 16-bit boundary, then ASSERT().
+
+  Unless PcdAssertOnSafeStringConstraints is set to FALSE:
   If an error would be returned, then the function will also ASSERT().
 
   If an error is returned, then the Destination is unmodified.
@@ -263,6 +267,8 @@ StrnCpyS (
 
   If Destination is not aligned on a 16-bit boundary, then ASSERT().
   If Source is not aligned on a 16-bit boundary, then ASSERT().
+
+  Unless PcdAssertOnSafeStringConstraints is set to FALSE:
   If an error would be returned, then the function will also ASSERT().
 
   If an error is returned, then the Destination is unmodified.
@@ -303,6 +309,8 @@ StrCatS (
 
   If Destination is not aligned on a 16-bit boundary, then ASSERT().
   If Source is not aligned on a 16-bit boundary, then ASSERT().
+
+  Unless PcdAssertOnSafeStringConstraints is set to FALSE:
   If an error would be returned, then the function will also ASSERT().
 
   If an error is returned, then the Destination is unmodified.
@@ -350,9 +358,11 @@ StrnCatS (
   be ignored. Then, the function stops at the first character that is a not a
   valid decimal character or a Null-terminator, whichever one comes first.
 
+  If String is not aligned in a 16-bit boundary, then ASSERT().
+
+  Unless PcdAssertOnSafeStringConstraints is set to FALSE:
   If String is NULL, then ASSERT().
   If Data is NULL, then ASSERT().
-  If String is not aligned in a 16-bit boundary, then ASSERT().
   If PcdMaximumUnicodeStringLength is not zero, and String contains 

[edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

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

Requesting for merge in edk2-stable202002.

Changes since V1:
- Enable assertions by default to preserve the original behaviour
- Fix bugzilla reference link
- Update documentation in BaseLib.h

Vitaly Cheptsov (1):
  MdePkg: Add PCD to disable safe string constraint assertions

 MdePkg/MdePkg.dec   |  6 ++
 MdePkg/Library/BaseLib/BaseLib.inf  | 11 +--
 MdePkg/Include/Library/BaseLib.h| 74 +---
 MdePkg/Library/BaseLib/SafeString.c |  4 +-
 MdePkg/MdePkg.uni   |  6 ++
 5 files changed, 71 insertions(+), 30 deletions(-)

-- 
2.21.0 (Apple Git-122.2)


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

View/Reply Online (#52837): https://edk2.groups.io/g/devel/message/52837
Mute This Topic: https://groups.io/mt/69401948/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov via Groups.Io]

Hi Mike,

Yes, the primary use case is for UEFI Applications. We do not want to disable 
ASSERT’s completely, as assertions that make sense, i.e. the ones signalising 
about interface misuse, are helpful for debugging.

I have already explained in the BZ that basically all safe string constraint 
assertions make no sense for handling untrusted data. We find this use case 
very logical, as these functions behave properly with assertions disabled and 
cover all these error conditions by the return statuses. In such situation is 
not useful for these functions to assert, as we end up inefficiently 
reimplementing the logic. I would have liked the approach of discussing the 
interfaces individually, but I struggle to find any that makes sense from this 
point of view.

AsciiStrToGuid will ASSERT when the length of the passed string is odd. 
Functions that cannot, ahem, parse, for us are pretty much useless.
AsciiStrCatS will ASSERT when the appended string does not fit the buffer. For 
us this logic makes this function pretty much equivalent to deprecated and thus 
unavailable AsciiStrCat, except it is also slower.

My original suggestion was to remove the assertions entirely, but several 
people here said that they use them to verify usage errors when handling 
trusted data. This makes good sense to me, so we suggest to support both cases 
by introducing a PCD in this patch.

Best wishes,
Vitaly

> 6 янв. 2020 г., в 21:28, Kinney, Michael D  
> написал(а):
> 
> 
> Hi Vitaly,
> 
> Is the use case for UEFI Applications?
> 
> There is a different mechanism to disable all ASSERT()
> statements  within a UEFI Application.
> 
> If a component is consuming data from an untrusted source,
> then that component is required to verify the untrusted
> data before passing it to a function that clearly documents
> is input requirements.  If this approach is followed, then
> the BaseLib functions can be used "as is" as long as the
> ASSERT() conditions are verified before calling.
> 
> If there are some APIs that currently document their ASSERT()
> behavior and we think that ASSERT() behavior is incorrect and
> should be handled by an existing error return value, then we
> should discuss each of those APIs individually.
> 
> Mike
> 
> 
>> -Original Message-
>> From: devel@edk2.groups.io  On
>> Behalf Of Vitaly Cheptsov via Groups.Io
>> Sent: Friday, January 3, 2020 9:13 AM
>> To: devel@edk2.groups.io
>> Subject: [edk2-devel] [PATCH v3 0/1] Add PCD to disable
>> safe string constraint assertions
>> 
>> REF:
>> https://bugzilla.tianocore.org/show_bug.cgi?id=2054
>> 
>> Requesting for merge in edk2-stable202002.
>> 
>> Changes since V1:
>> - Enable assertions by default to preserve the original
>> behaviour
>> - Fix bugzilla reference link
>> - Update documentation in BaseLib.h
>> 
>> Vitaly Cheptsov (1):
>>  MdePkg: Add PCD to disable safe string constraint
>> assertions
>> 
>> MdePkg/MdePkg.dec   |  6 ++
>> MdePkg/Library/BaseLib/BaseLib.inf  | 11 +--
>> MdePkg/Include/Library/BaseLib.h| 74
>> +---
>> MdePkg/Library/BaseLib/SafeString.c |  4 +-
>> MdePkg/MdePkg.uni   |  6 ++
>> 5 files changed, 71 insertions(+), 30 deletions(-)
>> 
>> --
>> 2.21.0 (Apple Git-122.2)
>> 
>> 
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this
>> group.
>> 
>> View/Reply Online (#52837):
>> https://edk2.groups.io/g/devel/message/52837
>> Mute This Topic: https://groups.io/mt/69401948/1643496
>> Group Owner: devel+ow...@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub
>> [michael.d.kin...@intel.com]
>> -=-=-=-=-=-=
> 


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

View/Reply Online (#52943): https://edk2.groups.io/g/devel/message/52943
Mute This Topic: https://groups.io/mt/69401948/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

[edk2-devel] [PATCH v2 1/1] MdePkg: Do not use CreateEventEx unless required

[Vitaly Cheptsov via Groups.Io]

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

There are many firmwares in the wild not supporting CreateEventEx,
including devices less than 5 years old.

Signed-off-by: Vitaly Cheptsov 
---
 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf   
|  3 ---
 MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf 
|  4 
 MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLibSegmentInfo.inf 
|  3 ---
 MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf   
|  3 ---
 MdePkg/Library/UefiDebugLibDebugPortProtocol/UefiDebugLibDebugPortProtocol.inf 
|  3 ---
 MdePkg/Library/UefiDebugLibStdErr/UefiDebugLibStdErr.inf   
|  3 ---
 MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf   
|  5 -
 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c 
|  3 +--
 MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c 
|  5 ++---
 MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLib.c  
|  5 ++---
 MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c
|  5 ++---
 MdePkg/Library/UefiDebugLibDebugPortProtocol/DebugLibConstructor.c 
|  5 ++---
 MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
|  5 ++---
 MdePkg/Library/UefiRuntimeLib/RuntimeLib.c 
| 10 --
 14 files changed, 15 insertions(+), 47 deletions(-)

diff --git 
a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf 
b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
index 655c1c14c1..31d169ad7c 100644
--- 
a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
+++ 
b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
@@ -41,9 +41,6 @@ [LibraryClasses]
   PrintLib
   SerialPortLib
 
-[Guids]
-  gEfiEventExitBootServicesGuid ## CONSUMES ## Event
-
 [Pcd]
   gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue ## SOMETIMES_CONSUMES
   gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask ## CONSUMES
diff --git a/MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf 
b/MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf
index 45bfe9dc6f..8d2ba1d187 100644
--- a/MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf
+++ b/MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf
@@ -47,7 +47,3 @@ [LibraryClasses]
 
 [Pcd]
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress  ## CONSUMES
-
-[Guids]
-  gEfiEventVirtualAddressChangeGuid ## CONSUMES ## Event
-
diff --git 
a/MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLibSegmentInfo.inf
 
b/MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLibSegmentInfo.inf
index f6445f4abb..ae9f11b697 100644
--- 
a/MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLibSegmentInfo.inf
+++ 
b/MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLibSegmentInfo.inf
@@ -45,6 +45,3 @@ [LibraryClasses]
   MemoryAllocationLib
   DxeServicesTableLib
   UefiBootServicesTableLib
-
-[Guids]
-  gEfiEventVirtualAddressChangeGuid ## CONSUMES ## Event
diff --git a/MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf 
b/MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf
index b577d52ac6..53bbc8ce3f 100644
--- a/MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf
+++ b/MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf
@@ -46,9 +46,6 @@ [LibraryClasses]
   PrintLib
   DebugPrintErrorLevelLib
 
-[Guids]
-  gEfiEventExitBootServicesGuid ## CONSUMES
-
 [Pcd]
   gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue## 
SOMETIMES_CONSUMES
   gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask## CONSUMES
diff --git 
a/MdePkg/Library/UefiDebugLibDebugPortProtocol/UefiDebugLibDebugPortProtocol.inf
 
b/MdePkg/Library/UefiDebugLibDebugPortProtocol/UefiDebugLibDebugPortProtocol.inf
index ff09a12ce4..e12a1025c6 100644
--- 
a/MdePkg/Library/UefiDebugLibDebugPortProtocol/UefiDebugLibDebugPortProtocol.inf
+++ 
b/MdePkg/Library/UefiDebugLibDebugPortProtocol/UefiDebugLibDebugPortProtocol.inf
@@ -46,9 +46,6 @@ [LibraryClasses]
   PrintLib
   DebugPrintErrorLevelLib
 
-[Guids]
-  gEfiEventExitBootServicesGuid ## CONSUMES
-
 [Protocols]
   gEfiDebugPortProtocolGuid ## CONSUMES
 
diff --git a/MdePkg/Library/UefiDebugLibStdErr/UefiDebugLibStdErr.inf 
b/MdePkg/Library/UefiDebugLibStdErr/UefiDebugLibStdErr.inf
index 11f7594626..5ecb971a0a 100644
--- a/MdePkg/Library/UefiDebugLibStdErr/UefiDebugLibStdErr.inf
+++ b/MdePkg/Library/UefiDebugLibStdErr/UefiDebugLibStdErr.inf
@@ -44,9 +44,6 @@ [LibraryClasses]
   PrintLib
   DebugPrintErrorLevelLib
 
-[Guids]
-  gEfiEventExitBootServicesGuid ## CONSUMES
-
 [Pcd]
   

[edk2-devel] [PATCH v2 0/1] MdePkg: Do not use CreateEventEx unless required

[Vitaly Cheptsov via Groups.Io]

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

Changes from V1:
- Dropped GUIDs from inf files

Requesting for merge in edk2-stable202002

Vitaly Cheptsov (1):
  MdePkg: Do not use CreateEventEx unless required

 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf   
|  3 ---
 MdePkg/Library/DxeRuntimePciExpressLib/DxeRuntimePciExpressLib.inf 
|  4 
 MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLibSegmentInfo.inf 
|  3 ---
 MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf   
|  3 ---
 MdePkg/Library/UefiDebugLibDebugPortProtocol/UefiDebugLibDebugPortProtocol.inf 
|  3 ---
 MdePkg/Library/UefiDebugLibStdErr/UefiDebugLibStdErr.inf   
|  3 ---
 MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf   
|  5 -
 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c 
|  3 +--
 MdePkg/Library/DxeRuntimePciExpressLib/PciExpressLib.c 
|  5 ++---
 MdePkg/Library/PciSegmentLibSegmentInfo/DxeRuntimePciSegmentLib.c  
|  5 ++---
 MdePkg/Library/UefiDebugLibConOut/DebugLibConstructor.c
|  5 ++---
 MdePkg/Library/UefiDebugLibDebugPortProtocol/DebugLibConstructor.c 
|  5 ++---
 MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c
|  5 ++---
 MdePkg/Library/UefiRuntimeLib/RuntimeLib.c 
| 10 --
 14 files changed, 15 insertions(+), 47 deletions(-)

-- 
2.21.0 (Apple Git-122.2)


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

View/Reply Online (#52979): https://edk2.groups.io/g/devel/message/52979
Mute This Topic: https://groups.io/mt/69499712/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers

[Vitaly Cheptsov via groups.io]

Zhichao,

This is correct. I did not notice the message previously, but otherwise 
everything outlined here is accurate. Let me know if further input can be 
performed from my side for this to land.

Best regards,
Vitaly

> 2 апр. 2020 г., в 11:28, Gao, Zhichao  написал(а):
> 
> Summarize the issue, if anything incorrect, please help to correct it. Thanks.
>
> Background:
> Uefi spec ambiguous description:
> > When interpreting the data from this function, it should be noted that if a 
> > class
> > of printable characters that are normally adjusted by shift modifiers (e.g. 
> > Shift
> > Key + "f" key) would be presented solely as a KeyData.Key.UnicodeChar 
> > without the
> > associated shift state. So in the previous example of a Shift Key + "f" key 
> > being
> > pressed, the only pertinent data returned would be KeyData.Key.UnicodeChar 
> > with
> > the value of "F". This of course would not typically be the case for 
> > non-printable
> > characters such as the pressing of the Right Shift Key + F10 key since the
> > corresponding returned data would be reflected both in the
> > KeyData.KeyState.KeyShiftState and KeyData.Key.ScanCode values.
>
> Some firmware vendor see the “if” as an option to implement the keyboard 
> driver and they choose to implement in the other way.
> Ideal: shift + UnicodeKey/UnScancodeKey == report as ==> “shifted” 
> UnicodeKey/UnScancodeKey
> Ctrl + UnicodeKey == report as ==> UnicodeKey – ‘A’/’a’ + 1
> Other way: shift + UnicodeKey/UnScancodeKey == report as ==> shift state + 
> “shifted” UnicodeKey/UnScancodeKey
> Ctrl + UnicodeKey == report as ==> ctrl state + UnicodeKey – 
> ‘A’/’a’ + 1
> I.e. do not remove the shift state.
> That would cause the shell edit/Hexedit missing handling the “shifted” key 
> when run shell in such firmware.
>
> Vitaly’s solution add the support the handle “other implementation”.
>
> If we take Vitaly’s solution, we should fixed all the applications in edk2 
> not only in shell edit/Hexeidt function.
> And when we catch another request same with this issue, we would follow the 
> same progress.
>
> Thanks,
> Zhichao
>
> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> 
> [mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>] On Behalf Of Gao, 
> Zhichao
> Sent: Thursday, April 2, 2020 2:57 PM
> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; 
> vit9...@protonmail.com <mailto:vit9...@protonmail.com>; Rothman, Michael A 
> mailto:michael.a.roth...@intel.com>>
> Cc: Andrew Fish mailto:af...@apple.com>>; Laszlo Ersek 
> mailto:ler...@redhat.com>>; Marvin Häuser 
> mailto:mhaeu...@outlook.de>>; Kinney, Michael D 
> mailto:michael.d.kin...@intel.com>>; Ni, Ray 
> mailto:ray...@intel.com>>
> Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with 
> separately reported modifiers
>
> Hi Micheal Rothman,
>
> Can you help to review this issue? Because of the uefi spec issue, some 
> firmware implemented the different behavior.
> Should the edk2 code to handle such issue? If yes, here maybe the situation:
> 1.   Change for all apps -> uefi spec update and accept  such behavior 
> with description -> Done.
> 2.   Change for all apps -> uefi spec update to remove the ambiguous and 
> reject other behavior -> removal the change in first step.
>
> Hi Vitaly,
>
> I used to think it is an additional support for different implementation 
> because of the spec. But if we approve this patch, all the app in edk2 using 
> the combo key function should be update.
> Using shell’s ctrl+’c’ as an example, it need to update at the same time. 
> Same with SCT tool.
>
> Thanks,
> Zhichao
>  <> 
> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> 
> [mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>] On Behalf Of 
> Vitaly Cheptsov via Groups.Io
> Sent: Friday, March 27, 2020 7:01 PM
> To: Gao, Zhichao mailto:zhichao@intel.com>>
> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Andrew Fish 
> mailto:af...@apple.com>>; Laszlo Ersek  <mailto:ler...@redhat.com>>; Marvin Häuser  <mailto:mhaeu...@outlook.de>>; Kinney, Michael D  <mailto:michael.d.kin...@intel.com>>; Ni, Ray  <mailto:ray...@intel.com>>
> Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with 
> separately reported modifiers
>
> Hello,
>
> Requesting to merge this into edk2-stable202005 for the reasons explained in 
> BZ[1]. I assume there is no real objection for this, only the a

Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers

[Vitaly Cheptsov via Groups.Io]

Hello,

Requesting to merge this into edk2-stable202005 for the reasons explained in 
BZ[1]. I assume there is no real objection for this, only the approach we make 
such changes in the future, but this can be postponed as we encounter more of 
such problems.

Best regards,
Vitaly

[1] https://bugzilla.tianocore.org/show_bug.cgi?id=2510

> 20 февр. 2020 г., в 03:27, Gao, Zhichao  написал(а):
> 
> Sorry for my mistake. Then I have no other comments for this patch.
> Reviewed-by: Zhichao Gao  <mailto:zhichao@intel.com>>
>
> Thanks,
> Zhichao
>
>  <>From: vit9696  
> Sent: Wednesday, February 19, 2020 8:15 PM
> To: Gao, Zhichao ; devel@edk2.groups.io
> Subject: RE: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with 
> separately reported modifiers
>
> Zhichao,
>
> Thanks for your review. The comment is correct, as ShiftOnlyState means the 
> state where only shift (and no other modifiers) can be pressed or not.
>
> Best wishes,
> Vitaly
>
> On Wed, Feb 19, 2020 at 09:55, Gao, Zhichao  <mailto:zhichao@intel.com>> wrote:
> Hi Vitaly,
> 
> See the comment below:
> 
> > -Original Message-
> > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> 
> > mailto:devel@edk2.groups.io>> On Behalf Of Vitaly
> > Cheptsov via Groups.Io
> > Sent: Monday, February 10, 2020 6:18 PM
> > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> > Subject: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with
> > separately reported modifiers
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2510 
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=2510>
> >
> > Some firmwares:
> > - Report Shift modifier even when they report upper-case unicode letter.
> > - Report Ctrl modifier with "shifted" UniChar (i.e. X - 'A' + 1).
> >
> > This change provides support for these firmwares preserving the 
> > compatibility
> > with the previous input handling.
> >
> > Signed-off-by: Michael Belyaev  > <mailto:usrs...@icloud.com>>
> > Reviewed-by: Vitaly Cheptsov  > <mailto:vit9...@protonmail.com>>
> > ---
> > ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c | 37
> > ++--
> > ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c | 28
> > ++-
> > ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c | 6 
> > ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c | 11
> > +++---
> > 4 files changed, 58 insertions(+), 24 deletions(-)
> >
> > diff --git
> > a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> > b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> > index df530f1119..9615a4dfbd 100644
> > --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> > +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> > @@ -1381,8 +1381,8 @@ MainCommandDisplayHelp (
> > continue;
> > }
> >
> > - if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) ||
> > - (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
> > + if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0)
> > + || (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
> > //
> > // For consoles that don't support/report shift state,
> > // CTRL+W is translated to L'W' - L'A' + 1.
> > @@ -1390,14 +1390,17 @@ MainCommandDisplayHelp (
> > if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
> > break;
> > }
> > - } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> > &&
> > - ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> > EFI_RIGHT_CONTROL_PRESSED)) != 0) &&
> > - ((KeyData.KeyState.KeyShiftState & ~(EFI_SHIFT_STATE_VALID |
> > EFI_LEFT_CONTROL_PRESSED | EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> > + } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> > + && ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> > EFI_RIGHT_CONTROL_PRESSED)) != 0)
> > + && ((KeyData.KeyState.KeyShiftState &
> > + ~(EFI_SHIFT_STATE_VALID | EFI_LEFT_CONTROL_PRESSED |
> > + EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> > //
> > // For consoles that supports/reports shift state,
> > // make sure that only CONTROL shift key is pressed.
> > + // For some consoles that report shift state,
> > + // CTRL+W is still translated to L'W' - L'A' + 1.
> > //
> > - if ((KeyData.K

Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers

[Vitaly Cheptsov via groups.io]

mware implemented the different behavior.
> Should the edk2 code to handle such issue? If yes, here maybe the situation:
> 1.   Change for all apps -> uefi spec update and accept  such behavior 
> with description -> Done.
> 2.   Change for all apps -> uefi spec update to remove the ambiguous and 
> reject other behavior -> removal the change in first step.
>
> Hi Vitaly,
>
> I used to think it is an additional support for different implementation 
> because of the spec. But if we approve this patch, all the app in edk2 using 
> the combo key function should be update.
> Using shell’s ctrl+’c’ as an example, it need to update at the same time. 
> Same with SCT tool.
>
> Thanks,
> Zhichao
>
> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> 
> [mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>] On Behalf Of 
> Vitaly Cheptsov via Groups.Io
> Sent: Friday, March 27, 2020 7:01 PM
> To: Gao, Zhichao mailto:zhichao@intel.com>>
> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Andrew Fish 
> mailto:af...@apple.com>>; Laszlo Ersek  <mailto:ler...@redhat.com>>; Marvin Häuser  <mailto:mhaeu...@outlook.de>>; Kinney, Michael D  <mailto:michael.d.kin...@intel.com>>; Ni, Ray  <mailto:ray...@intel.com>>
> Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with 
> separately reported modifiers
>
> Hello,
>
> Requesting to merge this into edk2-stable202005 for the reasons explained in 
> BZ[1]. I assume there is no real objection for this, only the approach we 
> make such changes in the future, but this can be postponed as we encounter 
> more of such problems.
>
> Best regards,
> Vitaly
>
> [1] https://bugzilla.tianocore.org/show_bug.cgi?id=2510 
> <https://bugzilla.tianocore.org/show_bug.cgi?id=2510>
>
> 
> 20 февр. 2020 г., в 03:27, Gao, Zhichao  <mailto:zhichao@intel.com>> написал(а):
>
> Sorry for my mistake. Then I have no other comments for this patch.
> Reviewed-by: Zhichao Gao  <mailto:zhichao@intel.com>>
>
> Thanks,
> Zhichao
>
>  <>From: vit9696 mailto:vit9...@protonmail.com>> 
> Sent: Wednesday, February 19, 2020 8:15 PM
> To: Gao, Zhichao mailto:zhichao@intel.com>>; 
> devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Subject: RE: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with 
> separately reported modifiers
>
> Zhichao,
>
> Thanks for your review. The comment is correct, as ShiftOnlyState means the 
> state where only shift (and no other modifiers) can be pressed or not.
>
> Best wishes,
> Vitaly
>
> On Wed, Feb 19, 2020 at 09:55, Gao, Zhichao  <mailto:zhichao@intel.com>> wrote:
> Hi Vitaly,
> 
> See the comment below:
> 
> > -Original Message-
> > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> 
> > mailto:devel@edk2.groups.io>> On Behalf Of Vitaly
> > Cheptsov via Groups.Io
> > Sent: Monday, February 10, 2020 6:18 PM
> > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> > Subject: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with
> > separately reported modifiers
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2510 
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=2510>
> >
> > Some firmwares:
> > - Report Shift modifier even when they report upper-case unicode letter.
> > - Report Ctrl modifier with "shifted" UniChar (i.e. X - 'A' + 1).
> >
> > This change provides support for these firmwares preserving the 
> > compatibility
> > with the previous input handling.
> >
> > Signed-off-by: Michael Belyaev  > <mailto:usrs...@icloud.com>>
> > Reviewed-by: Vitaly Cheptsov  > <mailto:vit9...@protonmail.com>>
> > ---
> > ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c | 37
> > ++--
> > ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c | 28
> > ++-
> > ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c | 6 
> > ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c | 11
> > +++---
> > 4 files changed, 58 insertions(+), 24 deletions(-)
> >
> > diff --git
> > a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> > b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> > index df530f1119..9615a4dfbd 100644
> > --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> > +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor

Re: [edk2-devel] [PATCH 1/2] BaseTools: Change CLANGPDB target to reduce image size

[Vitaly Cheptsov via Groups.Io]

Hello,

While I can imagine this landing as an absolutely last effort resort, at the 
moment the problem is unclear and unjustified. Do not rush with this patch 
please and proceed with the discussion in BZ.

Best wishes,
Vitaly 

> 24 марта 2020 г., в 09:20, Zhiguang Liu  написал(а):
> 
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2628
> 
> Using target i686-unknown-windows will generate smaller IA32 FV.
> 
> CC: Vitaly Cheptsov 
> CC: Liming Gao 
> CC: Bob Feng 
> Signed-off-by: Zhiguang Liu 
> ---
> BaseTools/Conf/tools_def.template | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/BaseTools/Conf/tools_def.template 
> b/BaseTools/Conf/tools_def.template
> index 2b17d3b297..3d6f6b4b4c 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -2755,7 +2755,7 @@ RELEASE_CLANG38_AARCH64_DLINK_FLAGS = 
> DEF(CLANG38_AARCH64_DLINK_FLAGS) -flto -Wl
> DEFINE CLANGPDB_IA32_PREFIX  = ENV(CLANG_BIN)
> DEFINE CLANGPDB_X64_PREFIX   = ENV(CLANG_BIN)
> 
> -DEFINE CLANGPDB_IA32_TARGET  = -target i686-unknown-windows-gnu
> +DEFINE CLANGPDB_IA32_TARGET  = -target i686-unknown-windows
> DEFINE CLANGPDB_X64_TARGET   = -target x86_64-unknown-windows-gnu
> 
> DEFINE CLANGPDB_WARNING_OVERRIDES= -Wno-parentheses-equality 
> -Wno-tautological-compare -Wno-tautological-constant-out-of-range-compare 
> -Wno-empty-body -Wno-unused-const-variable -Wno-varargs 
> -Wno-unknown-warning-option -Wno-microsoft-enum-forward-reference
> --
> 2.25.1.windows.1
> 


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

View/Reply Online (#56128): https://edk2.groups.io/g/devel/message/56128
Mute This Topic: https://groups.io/mt/72512013/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



signature.asc
Description: OpenPGP digital signature

Re: [edk2-devel] [Patch v9 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test

[Vitaly Cheptsov via groups.io]

Time to get this land, thx Mike!Reviewed-By: Vitaly Cheptsov On Wed, May 20, 2020 at 23:10, Michael D Kinney  wrote:  Use the safe string function StrCpyS() in BaseLib to test theSAFE_STRING_CONSTRAINT_CHECK() macro.Cc: Andrew Fish Cc: Ard Biesheuvel Cc: Bret Barkelew Cc: Brian J. Johnson Cc: Chasel Chiu Cc: Jordan Justen Cc: Laszlo Ersek Cc: Leif Lindholm Cc: Liming Gao Cc: Marvin H?user Cc: Michael D Kinney Cc: Vincent Zimmer Cc: Zhichao Gao Cc: Jiewen Yao Cc: Vitaly Cheptsov Signed-off-by: Michael D Kinney Reviewed-by: Philippe Mathieu-Daude --- .../UnitTest/Library/BaseLib/Base64UnitTest.c | 107 ++ 1 file changed, 107 insertions(+)diff --git a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.cindex 8952f9da6c..2c4266491c 100644--- a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c+++ b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c@@ -290,6 +290,99 @@ RfcDecodeTest(   return UNIT_TEST_PASSED; }+#define SOURCE_STRING  L"Hello"++STATIC+UNIT_TEST_STATUS+EFIAPI+SafeStringContraintCheckTest (+  IN UNIT_TEST_CONTEXT  Context+  )+{+  RETURN_STATUS  Status;+  CHAR16 Destination[20];+  CHAR16 AllZero[20];++  //+  // Zero buffer used to verify Destination is not modified+  //+  ZeroMem (AllZero, sizeof (AllZero));++  //+  // Positive test case copy source unicode string to destination+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), SOURCE_STRING);+  UT_ASSERT_NOT_EFI_ERROR (Status);+  UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING));++  //+  // Positive test case with DestMax the same as Source size+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16), SOURCE_STRING);+  UT_ASSERT_NOT_EFI_ERROR (Status);+  UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING));++  //+  // Negative test case with Destination NULL+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (NULL, sizeof (Destination) / sizeof (CHAR16), SOURCE_STRING);+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER);+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero));++  //+  // Negative test case with Source NULL+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), NULL);+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER);+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero));++  //+  // Negative test case with DestMax too big+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, MAX_UINTN, SOURCE_STRING);+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER);+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero));++  //+  // Negative test case with DestMax 0+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, 0, SOURCE_STRING);+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER);+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero));++  //+  // Negative test case with DestMax smaller than Source size+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, 1, SOURCE_STRING);+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero));++  //+  // Negative test case with DestMax smaller than Source size by one character+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16) - 1, SOURCE_STRING);+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero));++  //+  // Negative test case with overlapping Destination and Source+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), Destination);+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_ACCESS_DENIED);+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero));++  return UNIT_TEST_PASSED;+}+ /**   Initialze the unit test framework, suite, and unit tests for the   Base64 conversion APIs of BaseLib and run the unit tests.@@ -309,6 +402,7 @@ UnitTestingEntry (   UNIT_TEST_FRAMEWORK_HANDLE  Fw;   UNIT_TEST_SUITE_HANDLE  b64EncodeTests;   UNIT_TEST_SUITE_HANDLE  b64DecodeTests;+  UNIT_TEST_SUITE_HANDLE  SafeStringTests;   Fw = NULL;@@ -367,6 +461,19 @@ UnitTestingEntry (   AddTestCase (b64DecodeTests, "Incorrectly placed padding character", "Error4", RfcDecodeTest, NULL, CleanUpB64TestContext, );   AddTestCase (b64DecodeTests, "Too small of output buffer", "Error5", RfcDecodeTest, NULL, CleanUpB64TestContext, );+  //+  // Populate the safe string Unit Test Suite.+  //+  Status = CreateUnitTestSuite (, Fw, "Safe String", "BaseLib.SafeString", NULL, NULL);+  if 

Re: [edk2-devel] [Patch v9 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test

[Vitaly Cheptsov via groups.io]

Time to get this land, thx Mike!Reviewed-By: Vitaly Cheptsov On Wed, May 20, 2020 at 23:10, Michael D Kinney  wrote:  Use the safe string function StrCpyS() in BaseLib to test theSAFE_STRING_CONSTRAINT_CHECK() macro.Cc: Andrew Fish Cc: Ard Biesheuvel Cc: Bret Barkelew Cc: Brian J. Johnson Cc: Chasel Chiu Cc: Jordan Justen Cc: Laszlo Ersek Cc: Leif Lindholm Cc: Liming Gao Cc: Marvin H?user Cc: Michael D Kinney Cc: Vincent Zimmer Cc: Zhichao Gao Cc: Jiewen Yao Cc: Vitaly Cheptsov Signed-off-by: Michael D Kinney Reviewed-by: Philippe Mathieu-Daude --- .../UnitTest/Library/BaseLib/Base64UnitTest.c | 107 ++ 1 file changed, 107 insertions(+)diff --git a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.cindex 8952f9da6c..2c4266491c 100644--- a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c+++ b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c@@ -290,6 +290,99 @@ RfcDecodeTest(   return UNIT_TEST_PASSED; }+#define SOURCE_STRING  L"Hello"++STATIC+UNIT_TEST_STATUS+EFIAPI+SafeStringContraintCheckTest (+  IN UNIT_TEST_CONTEXT  Context+  )+{+  RETURN_STATUS  Status;+  CHAR16 Destination[20];+  CHAR16 AllZero[20];++  //+  // Zero buffer used to verify Destination is not modified+  //+  ZeroMem (AllZero, sizeof (AllZero));++  //+  // Positive test case copy source unicode string to destination+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), SOURCE_STRING);+  UT_ASSERT_NOT_EFI_ERROR (Status);+  UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING));++  //+  // Positive test case with DestMax the same as Source size+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16), SOURCE_STRING);+  UT_ASSERT_NOT_EFI_ERROR (Status);+  UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING));++  //+  // Negative test case with Destination NULL+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (NULL, sizeof (Destination) / sizeof (CHAR16), SOURCE_STRING);+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER);+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero));++  //+  // Negative test case with Source NULL+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), NULL);+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER);+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero));++  //+  // Negative test case with DestMax too big+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, MAX_UINTN, SOURCE_STRING);+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER);+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero));++  //+  // Negative test case with DestMax 0+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, 0, SOURCE_STRING);+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER);+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero));++  //+  // Negative test case with DestMax smaller than Source size+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, 1, SOURCE_STRING);+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero));++  //+  // Negative test case with DestMax smaller than Source size by one character+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16) - 1, SOURCE_STRING);+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero));++  //+  // Negative test case with overlapping Destination and Source+  //+  ZeroMem (Destination, sizeof (Destination));+  Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), Destination);+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_ACCESS_DENIED);+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero));++  return UNIT_TEST_PASSED;+}+ /**   Initialze the unit test framework, suite, and unit tests for the   Base64 conversion APIs of BaseLib and run the unit tests.@@ -309,6 +402,7 @@ UnitTestingEntry (   UNIT_TEST_FRAMEWORK_HANDLE  Fw;   UNIT_TEST_SUITE_HANDLE  b64EncodeTests;   UNIT_TEST_SUITE_HANDLE  b64DecodeTests;+  UNIT_TEST_SUITE_HANDLE  SafeStringTests;   Fw = NULL;@@ -367,6 +461,19 @@ UnitTestingEntry (   AddTestCase (b64DecodeTests, "Incorrectly placed padding character", "Error4", RfcDecodeTest, NULL, CleanUpB64TestContext, );   AddTestCase (b64DecodeTests, "Too small of output buffer", "Error5", RfcDecodeTest, NULL, CleanUpB64TestContext, );+  //+  // Populate the safe string Unit Test Suite.+  //+  Status = CreateUnitTestSuite (, Fw, "Safe String", "BaseLib.SafeString", 

Re: [edk2-devel] [Patch v8 1/2] MdePkg: Fix SafeString performing assertions on runtime checks

[Vitaly Cheptsov via groups.io]

Mike,

Looks perfect to me. For everyone: the only change from V7 is an addition of 
DEBUG_VERBOSE message, which can indeed be useful.

Best wishes,
Vitaly

> 20 мая 2020 г., в 06:01, Michael D Kinney  
> написал(а):
> 
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054
> 
> Runtime checks returned via status return code should not work as
> assertions to permit parsing not trusted data with SafeString
> interfaces.  Replace ASSERT() with a DEBUG_VERBOSE message.
> 
> Cc: Andrew Fish 
> Cc: Ard Biesheuvel 
> Cc: Bret Barkelew 
> Cc: Brian J. Johnson 
> Cc: Chasel Chiu 
> Cc: Jordan Justen 
> Cc: Laszlo Ersek 
> Cc: Leif Lindholm 
> Cc: Liming Gao 
> Cc: Marvin H?user 
> Cc: Michael D Kinney 
> Cc: Vincent Zimmer 
> Cc: Zhichao Gao 
> Cc: Jiewen Yao 
> Signed-off-by: Vitaly Cheptsov 
> ---
> MdePkg/Include/Library/BaseLib.h| 111 ---
> MdePkg/Library/BaseLib/SafeString.c | 115 +---
> 2 files changed, 3 insertions(+), 223 deletions(-)
> 
> diff --git a/MdePkg/Include/Library/BaseLib.h 
> b/MdePkg/Include/Library/BaseLib.h
> index b0bbe8cef8..8e7b87cbda 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -216,7 +216,6 @@ StrnSizeS (
> 
>   If Destination is not aligned on a 16-bit boundary, then ASSERT().
>   If Source is not aligned on a 16-bit boundary, then ASSERT().
> -  If an error would be returned, then the function will also ASSERT().
> 
>   If an error is returned, then the Destination is unmodified.
> 
> @@ -252,7 +251,6 @@ StrCpyS (
> 
>   If Length > 0 and Destination is not aligned on a 16-bit boundary, then 
> ASSERT().
>   If Length > 0 and Source is not aligned on a 16-bit boundary, then ASSERT().
> -  If an error would be returned, then the function will also ASSERT().
> 
>   If an error is returned, then the Destination is unmodified.
> 
> @@ -290,7 +288,6 @@ StrnCpyS (
> 
>   If Destination is not aligned on a 16-bit boundary, then ASSERT().
>   If Source is not aligned on a 16-bit boundary, then ASSERT().
> -  If an error would be returned, then the function will also ASSERT().
> 
>   If an error is returned, then the Destination is unmodified.
> 
> @@ -330,7 +327,6 @@ StrCatS (
> 
>   If Destination is not aligned on a 16-bit boundary, then ASSERT().
>   If Source is not aligned on a 16-bit boundary, then ASSERT().
> -  If an error would be returned, then the function will also ASSERT().
> 
>   If an error is returned, then the Destination is unmodified.
> 
> @@ -377,12 +373,7 @@ StrnCatS (
>   be ignored. Then, the function stops at the first character that is a not a
>   valid decimal character or a Null-terminator, whichever one comes first.
> 
> -  If String is NULL, then ASSERT().
> -  If Data is NULL, then ASSERT().
>   If String is not aligned in a 16-bit boundary, then ASSERT().
> -  If PcdMaximumUnicodeStringLength is not zero, and String contains more than
> -  PcdMaximumUnicodeStringLength Unicode characters, not including the
> -  Null-terminator, then ASSERT().
> 
>   If String has no valid decimal digits in the above format, then 0 is stored
>   at the location pointed to by Data.
> @@ -433,12 +424,7 @@ StrDecimalToUintnS (
>   be ignored. Then, the function stops at the first character that is a not a
>   valid decimal character or a Null-terminator, whichever one comes first.
> 
> -  If String is NULL, then ASSERT().
> -  If Data is NULL, then ASSERT().
>   If String is not aligned in a 16-bit boundary, then ASSERT().
> -  If PcdMaximumUnicodeStringLength is not zero, and String contains more than
> -  PcdMaximumUnicodeStringLength Unicode characters, not including the
> -  Null-terminator, then ASSERT().
> 
>   If String has no valid decimal digits in the above format, then 0 is stored
>   at the location pointed to by Data.
> @@ -494,12 +480,7 @@ StrDecimalToUint64S (
>   the first character that is a not a valid hexadecimal character or NULL,
>   whichever one comes first.
> 
> -  If String is NULL, then ASSERT().
> -  If Data is NULL, then ASSERT().
>   If String is not aligned in a 16-bit boundary, then ASSERT().
> -  If PcdMaximumUnicodeStringLength is not zero, and String contains more than
> -  PcdMaximumUnicodeStringLength Unicode characters, not including the
> -  Null-terminator, then ASSERT().
> 
>   If String has no valid hexadecimal digits in the above format, then 0 is
>   stored at the location pointed to by Data.
> @@ -555,12 +536,7 @@ StrHexToUintnS (
>   the first character that is a not a valid hexadecimal character or NULL,
>   whichever one comes first.
> 
> -  If String is NULL, then ASSERT().
> -  If Data is NULL, then ASSERT().
>   If String is not aligned in a 16-bit boundary, then ASSERT().
> -  If PcdMaximumUnicodeStringLength is not zero, and String contains more than
> -  PcdMaximumUnicodeStringLength Unicode characters, not including the
> -  Null-terminator, then ASSERT().
> 
>   If String has no valid hexadecimal digits in the above 

Re: [edk2-devel] [Patch v8 2/2] MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test

[Vitaly Cheptsov via groups.io]

Mike,

This looks generally good to me, but there are few issues.

1. There is a mistake in the description here:

+  // Negative test case with DestMax smaller than Source size
+  //
+  Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), 
Destination);

This should read «Negative test case with Destination and Source matching» 
probably or something alike.

2. For completeness I would also add zeroing before running every StrCpyS test, 
like this:

+  ZeroMem (Destination, sizeof (Destination));
+  Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16), 
SOURCE_STRING);
+  UT_ASSERT_NOT_EFI_ERROR (Status);
+  UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING));

Otherwise UT_ASSERT_MEM_EQUAL will miss the check of StrCpyS doing anything — 
Destination could remain unchanged since case 1.

3. I will also make sense to check the Destination string remains unchanged on 
failure for all the other cases, like this:

+  CHAR16 Destination[20];
+  CHAR16 AllZero[20];
+
+  ZeroMem (AllZero, sizeof (AllZero));

+  //
+  // Negative test case with DestMax too big
+  //
+  ZeroMem (Destination, sizeof (Destination));
+  Status = StrCpyS (Destination, MAX_UINTN, SOURCE_STRING);
+  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER);
+  UT_ASSERT_MEM_EQUAL (Destination, AllZero, sizeof (AllZero));

This behaviour was very unintuitive to me when I met with EDK II BaseLib, 
because I expected something like strlcpy behaviour, where the string is 
truncated to fit, so it is definitely worth testing to ensure we do not break 
things.

Best wishes,
Vitaly

> 20 мая 2020 г., в 06:11, Michael D Kinney  
> написал(а):
> 
> 
> Use the safe string function StrCpyS() in BaseLib to test the
> SAFE_STRING_CONSTRAINT_CHECK() macro.
> 
> Cc: Andrew Fish 
> Cc: Ard Biesheuvel 
> Cc: Bret Barkelew 
> Cc: Brian J. Johnson 
> Cc: Chasel Chiu 
> Cc: Jordan Justen 
> Cc: Laszlo Ersek 
> Cc: Leif Lindholm 
> Cc: Liming Gao 
> Cc: Marvin H?user 
> Cc: Michael D Kinney 
> Cc: Vincent Zimmer 
> Cc: Zhichao Gao 
> Cc: Jiewen Yao 
> Cc: Vitaly Cheptsov 
> Signed-off-by: Michael D Kinney 
> ---
> .../UnitTest/Library/BaseLib/Base64UnitTest.c | 85 +++
> 1 file changed, 85 insertions(+)
> 
> diff --git a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c 
> b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c
> index 8952f9da6c..5aced69e0d 100644
> --- a/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c
> +++ b/MdePkg/Test/UnitTest/Library/BaseLib/Base64UnitTest.c
> @@ -290,6 +290,77 @@ RfcDecodeTest(
>   return UNIT_TEST_PASSED;
> }
> 
> +#define SOURCE_STRING  L"Hello"
> +
> +STATIC
> +UNIT_TEST_STATUS
> +EFIAPI
> +SafeStringContraintCheckTest (
> +  IN UNIT_TEST_CONTEXT  Context
> +  )
> +{
> +  RETURN_STATUS  Status;
> +  CHAR16 Destination[20];
> +
> +  //
> +  // Positive test case copy source unicode string to destination
> +  //
> +  Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), 
> SOURCE_STRING);
> +  UT_ASSERT_NOT_EFI_ERROR (Status);
> +  UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING));
> +
> +  //
> +  // Positive test case with DestMax the same as Source size
> +  //
> +  Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16), 
> SOURCE_STRING);
> +  UT_ASSERT_NOT_EFI_ERROR (Status);
> +  UT_ASSERT_MEM_EQUAL (Destination, SOURCE_STRING, sizeof (SOURCE_STRING));
> +
> +  //
> +  // Negative test case with Destination NULL
> +  //
> +  Status = StrCpyS (NULL, sizeof (Destination) / sizeof (CHAR16), 
> SOURCE_STRING);
> +  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER);
> +
> +  //
> +  // Negative test case with Source NULL
> +  //
> +  Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), 
> NULL);
> +  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER);
> +
> +  //
> +  // Negative test case with DestMax too big
> +  //
> +  Status = StrCpyS (Destination, MAX_UINTN, SOURCE_STRING);
> +  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER);
> +
> +  //
> +  // Negative test case with DestMax 0
> +  //
> +  Status = StrCpyS (Destination, 0, SOURCE_STRING);
> +  UT_ASSERT_STATUS_EQUAL (Status, RETURN_INVALID_PARAMETER);
> +
> +  //
> +  // Negative test case with DestMax smaller than Source size
> +  //
> +  Status = StrCpyS (Destination, 1, SOURCE_STRING);
> +  UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
> +
> +  //
> +  // Negative test case with DestMax smaller than Source size by one 
> character
> +  //
> +  Status = StrCpyS (Destination, sizeof (SOURCE_STRING) / sizeof (CHAR16) - 
> 1, SOURCE_STRING);
> +  UT_ASSERT_STATUS_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
> +
> +  //
> +  // Negative test case with DestMax smaller than Source size
> +  //
> +  Status = StrCpyS (Destination, sizeof (Destination) / sizeof (CHAR16), 
> Destination);
> +  UT_ASSERT_STATUS_EQUAL (Status, RETURN_ACCESS_DENIED);
> +
> 

Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix CPU stack guard support by aligning GDT buffer

[Vitaly Cheptsov via groups.io]

Just to make it clear, this is an immediate solution that is good enough to fix 
the bug. However, a more proper solution would be to introduce the _Alignas ( 
https://en.cppreference.com/w/c/language/_Alignas ) concept to EDK II. I would 
suggest the following macro in Base.h:

/**
Enforce custom alignment for a variable definition.
Similar to C11 alignas macro from stdalign.h, except it must be functional to 
support MSVC.

@param  Alignment  Numeric alignment to require.
**/
#ifdef _MSC_EXTENSIONS
#define ALIGNAS(Alignment) __declspec(align(Alignment))
#else
#define ALIGNAS(Alignment) _Alignas(Alignment)
#endif

If there is no disagreement on this, I can imagine submitting an update after 
this patch is merged.


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

Re: [edk2-devel] Question about UEFI, AddressSanitizer and MMU mappings

[Vitaly Cheptsov via groups.io]

Hello,

I have some experience porting LLVM Sanitizers and am very interested in this 
project coming live to UEFI as well. I had success with both entirely static 
shadow memory allocation and dynamic on-demand allocation. For ASan in the UEFI 
my personal idea would be trying to avoid page-fault allocation of the shadow 
memory, but rather adapting the allocators to not only allocate the "origin" 
memory but also "shadow memory" with a known shift and base (which are 
configurable now).

The parts that seem difficult to me are concurrency and trying to make ASan 
work in whitelist mode (i.e. forbidding all accesses that are not 
greenlighted), but otherwise it should be rather straight-forward if we do not 
include fake stack in the task and focus on DXE at first. All in all, I can be 
a co-mentor in this task and am ready to help as needed.

Best wishes,
Vitaly


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