date:20230814

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

2023-08-14 Thread Andrew Fish via groups.io

We also support Xcode clang so that means we also support Mach-O executables 
that get converted to PE/COFF. The is a tool called mtoc (mach-o to coff) in a 
crufty old open source project that does the conversion. 

The reason you are having issues is due to security hardening as the self 
modifying code is a security risk. It is kind of hard to imagine a case in UEFI 
that the self modifying code is worth the security risk?. I know Linux does 
some patching but those are really hot paths that get used a lot, I don’t see 
that being a pattern that would be common in firmware. The only case I can 
think you might want SMC is if you were trying to make an UEFI based stress 
test of some kind? 

It might be helpful if you could explain why you can’t use a dispatch table or 
just define a UEFI Protocol and construct it on the fly to meet your 
configuration? To me saying you need Self Modifying Code is kind of like saying 
you need to write it in assembler since the C compiler is not smart enough, and 
most of the times people think that they are wrong.  

Thanks,

Andrew Fish

> On Aug 14, 2023, at 8:06 PM, Chao Li  wrote:
> 
> Hi Liming, Bob and Yuwei
> 
> There is a need that some code wants to supports Self-Modification, because 
> some program behavior may not be determined during compilation, and I think 
> this demand may be very popular. 
> 
> The permise of Self-Modification is that the section has executable and 
> writable permissions. Adding a new section and giving it executable and 
> writable permissions is a better way, and the 'pragma seg_code' is recognized 
> in Microsoft VS compiler but GCC doesn't. If use the GCC as the compiler, the 
> '.section name flags' of GNU GAS are acceptable.
> 
> But there is a problem, if converting from elf to efi, the user-defined 
> section with W+X or A+W+X will be droped, Elf64Convert.c will scan the file 
> section permission of elf, if the section is A+X, it will be classified into 
> the .text section, if the section is A+W , then it will be classified into 
> the .data section, if the section is A+W+X or W+X, then it will be 
> droped(Elf64Convert.c, line 272 to 325).
> 
> That is:
> 
> If using the VS compiler, the user-defined with executable and writable 
> sections may be perserved, but GCC elf to efi conversion may not.
> 
> 
> 
> Hope hearback from you and discuss the necessity of SMC(Slef-Modifying-Code) 
> and how to implement it.
> 
> 
> 
> 
> Thanks,
> Chao
> 

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

[edk2-devel] About EDK2 supports Self Modifying Code

2023-08-14 Thread Chao Li


Hi Liming, Bob and Yuwei

There is a need that some code wants to supports Self-Modification, 
because some program behavior may not be determined during compilation, 
and I think this demand may be very popular.


The permise of Self-Modification is that the section has executable and 
writable permissions. Adding a new section and giving it executable and 
writable permissions is a better way, and the 'pragma seg_code' is 
recognized in Microsoft VS compiler but GCC doesn't. If use the GCC as 
the compiler, the '.section name flags' of GNU GAS are acceptable.


But there is a problem, if converting from elf to efi, the user-defined 
section with W+X or A+W+X will be droped, Elf64Convert.c will scan the 
file section permission of elf, if the section is A+X, it will be 
classified into the .text section, if the section is A+W , then it will 
be classified into the .data section, if the section is A+W+X or W+X, 
then it will be droped(Elf64Convert.c, line 272 to 325).


That is:

If using the VS compiler, the user-defined with executable and writable 
sections may be perserved, but GCC elf to efi conversion may not.



Hope hearback from you and discuss the necessity of 
SMC(Slef-Modifying-Code) and how to implement it.




Thanks,
Chao


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

Re: [edk2-devel] [PATCH v1 0/7] Uncrustify GoogleTest update

2023-08-14 Thread Michael Kubacki

I don't see the package maintainers actually in Cc for these patches. 
You might want to resend the series with them copied as their email 
rules might otherwise not bring the emails to their attention.


Thanks,
Michael

On 8/9/2023 5:32 PM, VivianNK wrote:

Introduce cpp-specific formatting rules to Uncrustify for readability:
  * indent_class: indent lines inside of a class
  * indent_extern: indent lines inside of an extern

Apply all Uncrustify formatting rules defined in the config to existing
cpp and header files (GoogleTest files).

Moving forward all GoogleTest files will be included in uncrustify
formatting checks, leading to more consistency and greater readability.

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

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




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

[edk2-devel] Event: TianoCore Bug Triage - APAC / NAMO - Tuesday, August 15, 2023 #cal-reminder

2023-08-14 Thread Group Notification

*Reminder: TianoCore Bug Triage - APAC / NAMO*

*When:*
Tuesday, August 15, 2023
6:30pm to 7:30pm
(UTC-07:00) America/Los Angeles

*Where:*
https://teams.microsoft.com/l/meetup-join/19%3ameeting_OTk1YzJhN2UtOGQwNi00NjY4LWEwMTktY2JiODRlYTY1NmY0%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%226e4ce4c4-1242-431b-9a51-92cd01a5df3c%22%7d

*Organizer:* Liming Gao gaolim...@byosoft.com.cn ( 
gaolim...@byosoft.com.cn?subject=Re:%20Event:%20TianoCore%20Bug%20Triage%20-%20APAC%20%2F%20NAMO
 )

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

*Description:*

TianoCore Bug Triage - APAC / NAMO

Hosted by Liming Gao



Microsoft Teams meeting

*Join on your computer or mobile app*

Click here to join the meeting ( 
https://teams.microsoft.com/l/meetup-join/19%3ameeting_OTk1YzJhN2UtOGQwNi00NjY4LWEwMTktY2JiODRlYTY1NmY0%40thread.v2/0?context=%7b%22Tid%22%3a%2246c98d88-e344-4ed4-8496-4ed7712e255d%22%2c%22Oid%22%3a%226e4ce4c4-1242-431b-9a51-92cd01a5df3c%22%7d
 )

*Join with a video conferencing device*

te...@conf.intel.com

Video Conference ID: 116 062 094 0

Alternate VTC dialing instructions ( 
https://conf.intel.com/teams/?conf=1160620940&ivr=teams&d=conf.intel.com&test=test_call
 )

*Or call in (audio only)*

+1 916-245-6934,,77463821# ( tel:+19162456934,,77463821# ) United States, 
Sacramento

Phone Conference ID: 774 638 21#

Find a local number ( 
https://dialin.teams.microsoft.com/d195d438-2daa-420e-b9ea-da26f9d1d6d5?id=77463821
 ) | Reset PIN ( https://mysettings.lync.com/pstnconferencing )

Learn More ( https://aka.ms/JoinTeamsMeeting ) | Meeting options ( 
https://teams.microsoft.com/meetingOptions/?organizerId=b286b53a-1218-4db3-bfc9-3d4c5aa7669e&tenantId=46c98d88-e344-4ed4-8496-4ed7712e255d&threadId=19_meeting_OTUyZTg2NjgtNDhlNS00ODVlLTllYTUtYzg1OTNjNjdiZjFh@thread.v2&messageId=0&language=en-US
 )


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

Re: [edk2-devel] [PATCH v1 0/7] Add DebugMacroCheck

2023-08-14 Thread Andrew Fish via groups.io




> On Aug 14, 2023, at 5:26 PM, Michael Kubacki  
> wrote:
> 
> On 8/14/2023 8:23 PM, Andrew Fish via groups.io wrote:
>>> On Aug 14, 2023, at 3:51 PM, Pedro Falcato  wrote:
>>> 
>>> On Mon, Aug 14, 2023 at 9:49 PM Michael Kubacki
>>> mailto:mikub...@linux.microsoft.com>> wrote:
 
 From: Michael Kubacki 
 
 Adds a new script and build plugin called DebugMacroCheck.
 
 The script verifies that the number of print specifiers match the
 number of arguments in DEBUG() calls.
 
 Overview:
 
 - Build plugin: BuildPlugin/DebugMacroCheckBuildPlugin.py
  - Runs on any build target that is not NO-TARGET
 - Standalone script: DebugMacroCheck.py
  - Run `DebugMacroCheck.py --help` to see command line options
 - Unit tests:
  - Tests/test_DebugMacroCheck.py
  - Can be run with:
`python -m unittest discover -s ./.pytool/Plugin/DebugMacroCheck/tests 
 -v`
  - Also visible in VS Code Test Explorer
 
 Background:
 
 The tool has been constantly run against edk2 derived code for about
 a year now. During that time, its found over 20 issues in edk2, over
 50 issues in various vendor code, and numerous other issues specific
 to Project Mu.
 
 See the following series for a batch of issues previously fixed in
 edk2 discovered by the tool:
 
  https://edk2.groups.io/g/devel/message/93104
 
 I've received interest from vendors to place it in edk2 to
 immediately find issues in the upstream and make it easier for edk2
 consumers to directly acquire it. That led to this patch series.
 
 This would run in edk2 as a build plugin. All issues in the edk2
 codebase have been resolved so this would find new issues before
 they are merged into the codebase.
>>> 
>>> Hi,
>>> 
>>> I really like this change but I cannot stop and think that if DEBUG
>>> and PrintLib were ISO C compliant, we could be using normal interfaces
>>> with normal argument types and the compiler's intrinsic knowledge of
>>> printf-like functions.
>>> Have you explored that option for future code? See e.g
>>> https://godbolt.org/z/4e8d3WToT (I don't 
>>> know what MSVC uses, if
>>> anything).
>>> 
>> I have a dream that we add an eft_print as an attribute format archetype and 
>> then do what you recommend. After all clang and gcc are open source.
> I agree that would be preferred. I did something in similar in GCC at the 
> time, but I couldn't find an equivalent in VS. The issues kept appearing so 
> this was a cross-platform way to address it.
> 

This is a case that CI can help :)

Thanks,

Andrew Fish 

> I uploaded some usage examples to the PR for reference:
> https://github.com/tianocore/edk2/pull/4736
> 
> Thanks,
> Michael
>> Thanks,
>> Andrew Fish
>>> --
>>> Pedro
>>> 
>>> 
>> 



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

Re: [edk2-devel] OVMF build.sh change is incorrect

2023-08-14 Thread Mike Beaton

Perhaps I can briefly clarify:

"This now no longer works" was too brief - of course the listed command
does build and start QEMU. But, to clarify, a rebuild is not what is wanted
here, both for the additional time it takes, and for the fact that it
resets (rebuilds) the NVRAM of the VM.

The advantage of using `build.sh -a ... -b ... qemu ...` to launch QEMU
without building (as was previously possible) is that it automatically
selects the correct qemu command and correct built OVMF BIOS binary to
match the related build command.

Mike

On Sun, 13 Aug 2023 at 11:13, Mike Beaton  wrote:

> I believe this change
> https://github.com/tianocore/edk2/commit/173a7a7daaad560cd69e1000faca1d2b91774c46
> may have misunderstood the purpose of the previous code.
>
> I used to frequently use:
>
> `./build.sh -a X64 -b RELEASE` (or whichever arch and build target I
> required) to build OVMF
>
> and then:
>
> `./build.sh -a X64 -b RELEASE qemu {my-qemu-flags}` to run OVMF
>
> This now no longer works, since the second command also attempts to
> rebuild OVMF every time you run it.
>
> I believe the previous behaviour was intended and more useful.
>
> Mike Beaton
>
>

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

[edk2-devel] OVMF build.sh change is incorrect

2023-08-14 Thread Mike Beaton

I believe this change
https://github.com/tianocore/edk2/commit/173a7a7daaad560cd69e1000faca1d2b91774c46
may have misunderstood the purpose of the previous code.

I used to frequently use:

`./build.sh -a X64 -b RELEASE` (or whichever arch and build target I
required) to build OVMF

and then:

`./build.sh -a X64 -b RELEASE qemu {my-qemu-flags}` to run OVMF

This now no longer works, since the second command also attempts to rebuild
OVMF every time you run it.

I believe the previous behaviour was intended and more useful.

Mike Beaton


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107749): https://edk2.groups.io/g/devel/message/107749
Mute This Topic: https://groups.io/mt/100750154/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 0/7] Add DebugMacroCheck

2023-08-14 Thread Pedro Falcato

On Tue, Aug 15, 2023 at 1:23 AM Andrew (EFI) Fish  wrote:
>
>
>
> On Aug 14, 2023, at 3:51 PM, Pedro Falcato  wrote:
>
> On Mon, Aug 14, 2023 at 9:49 PM Michael Kubacki
>  wrote:
>
>
> From: Michael Kubacki 
>
> Adds a new script and build plugin called DebugMacroCheck.
>
> The script verifies that the number of print specifiers match the
> number of arguments in DEBUG() calls.
>
> Overview:
>
> - Build plugin: BuildPlugin/DebugMacroCheckBuildPlugin.py
>  - Runs on any build target that is not NO-TARGET
> - Standalone script: DebugMacroCheck.py
>  - Run `DebugMacroCheck.py --help` to see command line options
> - Unit tests:
>  - Tests/test_DebugMacroCheck.py
>  - Can be run with:
>`python -m unittest discover -s ./.pytool/Plugin/DebugMacroCheck/tests -v`
>  - Also visible in VS Code Test Explorer
>
> Background:
>
> The tool has been constantly run against edk2 derived code for about
> a year now. During that time, its found over 20 issues in edk2, over
> 50 issues in various vendor code, and numerous other issues specific
> to Project Mu.
>
> See the following series for a batch of issues previously fixed in
> edk2 discovered by the tool:
>
>  https://edk2.groups.io/g/devel/message/93104
>
> I've received interest from vendors to place it in edk2 to
> immediately find issues in the upstream and make it easier for edk2
> consumers to directly acquire it. That led to this patch series.
>
> This would run in edk2 as a build plugin. All issues in the edk2
> codebase have been resolved so this would find new issues before
> they are merged into the codebase.
>
>
> Hi,
>
> I really like this change but I cannot stop and think that if DEBUG
> and PrintLib were ISO C compliant, we could be using normal interfaces
> with normal argument types and the compiler's intrinsic knowledge of
> printf-like functions.
> Have you explored that option for future code? See e.g
> https://godbolt.org/z/4e8d3WToT (I don't know what MSVC uses, if
> anything).
>
>
> I have a dream that we add an eft_print as an attribute format archetype and 
> then do what you recommend. After all clang and gcc are open source.

I don't think the upstream compiler folks are willing to support our
broken printf variant. Nor should we encourage things like

VOID
Foo (
  UINTN Val
)
{
  DEBUG ((DEBUG_INFO, "%Lx", (UINT64) Val);
}

while not providing anything that looks but doesn't look like normal C
printf semantics.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107748): https://edk2.groups.io/g/devel/message/107748
Mute This Topic: https://groups.io/mt/100745693/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 0/7] Add DebugMacroCheck

2023-08-14 Thread Andrew Fish via groups.io



> On Aug 14, 2023, at 3:51 PM, Pedro Falcato  wrote:
> 
> On Mon, Aug 14, 2023 at 9:49 PM Michael Kubacki
> mailto:mikub...@linux.microsoft.com>> wrote:
>> 
>> From: Michael Kubacki 
>> 
>> Adds a new script and build plugin called DebugMacroCheck.
>> 
>> The script verifies that the number of print specifiers match the
>> number of arguments in DEBUG() calls.
>> 
>> Overview:
>> 
>> - Build plugin: BuildPlugin/DebugMacroCheckBuildPlugin.py
>>  - Runs on any build target that is not NO-TARGET
>> - Standalone script: DebugMacroCheck.py
>>  - Run `DebugMacroCheck.py --help` to see command line options
>> - Unit tests:
>>  - Tests/test_DebugMacroCheck.py
>>  - Can be run with:
>>`python -m unittest discover -s ./.pytool/Plugin/DebugMacroCheck/tests -v`
>>  - Also visible in VS Code Test Explorer
>> 
>> Background:
>> 
>> The tool has been constantly run against edk2 derived code for about
>> a year now. During that time, its found over 20 issues in edk2, over
>> 50 issues in various vendor code, and numerous other issues specific
>> to Project Mu.
>> 
>> See the following series for a batch of issues previously fixed in
>> edk2 discovered by the tool:
>> 
>>  https://edk2.groups.io/g/devel/message/93104
>> 
>> I've received interest from vendors to place it in edk2 to
>> immediately find issues in the upstream and make it easier for edk2
>> consumers to directly acquire it. That led to this patch series.
>> 
>> This would run in edk2 as a build plugin. All issues in the edk2
>> codebase have been resolved so this would find new issues before
>> they are merged into the codebase.
> 
> Hi,
> 
> I really like this change but I cannot stop and think that if DEBUG
> and PrintLib were ISO C compliant, we could be using normal interfaces
> with normal argument types and the compiler's intrinsic knowledge of
> printf-like functions.
> Have you explored that option for future code? See e.g
> https://godbolt.org/z/4e8d3WToT (I don't know what MSVC uses, if
> anything).
> 

I have a dream that we add an eft_print as an attribute format archetype and 
then do what you recommend. After all clang and gcc are open source. 

Thanks,

Andrew Fish

> -- 
> Pedro
> 
> 
> 



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

[edk2-devel] Now: Tools, CI, Code base construction meeting series - Monday, August 14, 2023 #cal-notice

2023-08-14 Thread Group Notification

*Tools, CI, Code base construction meeting series*

*When:*
Monday, August 14, 2023
4:30pm to 5:30pm
(UTC-07:00) America/Los Angeles

*Where:*
https://teams.microsoft.com/l/meetup-join/19%3ameeting_ZDI2ZDg4NmMtMjI1My00MzI5LWFmYjAtMGQyNjUzNTBjZGYw%40thread.v2/0?context=%7b%22Tid%22%3a%2272f988bf-86f1-41af-91ab-2d7cd011db47%22%2c%22Oid%22%3a%2223af6561-6e1c-450d-b917-d9d674eb3cb6%22%7d

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

*Description:*

TianoCore community,

Microsoft and Intel will be hosting a series of open meetings to discuss build, 
CI, tools, and other related topics. If you are interested, have ideas/opinions 
please join us. These meetings will be Monday 4:30pm Pacific Time on Microsoft 
Teams.

MS Teams Link in following discussion: * 
https://github.com/tianocore/edk2/discussions/2614

Anyone is welcome to join.

* tianocore/edk2: EDK II (github.com)
* tianocore/edk2-basetools: EDK II BaseTools Python tools as a PIP module 
(github.com) https://github.com/tianocore/edk2-basetools
* tianocore/edk2-pytool-extensions: Extensions to the edk2 build system 
allowing for a more robust and plugin based build system and tool execution 
environment (github.com) https://github.com/tianocore/edk2-pytool-extensions
* tianocore/edk2-pytool-library: Python library package that supports UEFI 
development (github.com) https://github.com/tianocore/edk2-pytool-library

MS Teams Browser Clients * 
https://docs.microsoft.com/en-us/microsoftteams/get-clients?tabs=Windows#browser-client


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

Re: [edk2-devel] [PATCH v1 0/7] Add DebugMacroCheck

2023-08-14 Thread Pedro Falcato

On Mon, Aug 14, 2023 at 9:49 PM Michael Kubacki
 wrote:
>
> From: Michael Kubacki 
>
> Adds a new script and build plugin called DebugMacroCheck.
>
> The script verifies that the number of print specifiers match the
> number of arguments in DEBUG() calls.
>
> Overview:
>
> - Build plugin: BuildPlugin/DebugMacroCheckBuildPlugin.py
>   - Runs on any build target that is not NO-TARGET
> - Standalone script: DebugMacroCheck.py
>   - Run `DebugMacroCheck.py --help` to see command line options
> - Unit tests:
>   - Tests/test_DebugMacroCheck.py
>   - Can be run with:
> `python -m unittest discover -s ./.pytool/Plugin/DebugMacroCheck/tests -v`
>   - Also visible in VS Code Test Explorer
>
> Background:
>
> The tool has been constantly run against edk2 derived code for about
> a year now. During that time, its found over 20 issues in edk2, over
> 50 issues in various vendor code, and numerous other issues specific
> to Project Mu.
>
> See the following series for a batch of issues previously fixed in
> edk2 discovered by the tool:
>
>   https://edk2.groups.io/g/devel/message/93104
>
> I've received interest from vendors to place it in edk2 to
> immediately find issues in the upstream and make it easier for edk2
> consumers to directly acquire it. That led to this patch series.
>
> This would run in edk2 as a build plugin. All issues in the edk2
> codebase have been resolved so this would find new issues before
> they are merged into the codebase.

Hi,

I really like this change but I cannot stop and think that if DEBUG
and PrintLib were ISO C compliant, we could be using normal interfaces
with normal argument types and the compiler's intrinsic knowledge of
printf-like functions.
Have you explored that option for future code? See e.g
https://godbolt.org/z/4e8d3WToT (I don't know what MSVC uses, if
anything).

-- 
Pedro


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

[edk2-devel] Event: Tools, CI, Code base construction meeting series - Monday, August 14, 2023 #cal-reminder

2023-08-14 Thread Group Notification

*Reminder: Tools, CI, Code base construction meeting series*

*When:*
Monday, August 14, 2023
4:30pm to 5:30pm
(UTC-07:00) America/Los Angeles

*Where:*
https://teams.microsoft.com/l/meetup-join/19%3ameeting_ZDI2ZDg4NmMtMjI1My00MzI5LWFmYjAtMGQyNjUzNTBjZGYw%40thread.v2/0?context=%7b%22Tid%22%3a%2272f988bf-86f1-41af-91ab-2d7cd011db47%22%2c%22Oid%22%3a%2223af6561-6e1c-450d-b917-d9d674eb3cb6%22%7d

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

*Description:*

TianoCore community,

Microsoft and Intel will be hosting a series of open meetings to discuss build, 
CI, tools, and other related topics. If you are interested, have ideas/opinions 
please join us. These meetings will be Monday 4:30pm Pacific Time on Microsoft 
Teams.

MS Teams Link in following discussion: * 
https://github.com/tianocore/edk2/discussions/2614

Anyone is welcome to join.

* tianocore/edk2: EDK II (github.com)
* tianocore/edk2-basetools: EDK II BaseTools Python tools as a PIP module 
(github.com) https://github.com/tianocore/edk2-basetools
* tianocore/edk2-pytool-extensions: Extensions to the edk2 build system 
allowing for a more robust and plugin based build system and tool execution 
environment (github.com) https://github.com/tianocore/edk2-pytool-extensions
* tianocore/edk2-pytool-library: Python library package that supports UEFI 
development (github.com) https://github.com/tianocore/edk2-pytool-library

MS Teams Browser Clients * 
https://docs.microsoft.com/en-us/microsoftteams/get-clients?tabs=Windows#browser-client


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

[edk2-devel] [PATCH v1 7/7] .pytool/Plugin: Add DebugMacroCheck

2023-08-14 Thread Michael Kubacki

From: Michael Kubacki 

Adds a plugin that finds debug macro formatting issues. These errors
often creep into debug prints in error conditions not frequently
executed and make debug more difficult when they are encountered.

The code can be as a standalone script which is useful to find
problems in a large codebase that has not been checked before or as
a CI plugin that notifies a developer of an error right away.

The script was already used to find numerous issues in edk2 in the
past so there's not many code fixes in this change. More details
are available in the readme file:

.pytool\Plugin\DebugMacroCheck\Readme.md

Cc: Sean Brogan 
Cc: Michael D Kinney 
Cc: Liming Gao 
Signed-off-by: Michael Kubacki 
---
 .pytool/Plugin/DebugMacroCheck/BuildPlugin/DebugMacroCheckBuildPlugin.py | 127 
+++
 .pytool/Plugin/DebugMacroCheck/BuildPlugin/DebugMacroCheck_plug_in.yaml  |  11 
+
 .pytool/Plugin/DebugMacroCheck/DebugMacroCheck.py| 859 

 .pytool/Plugin/DebugMacroCheck/Readme.md | 253 
++
 .pytool/Plugin/DebugMacroCheck/tests/DebugMacroDataSet.py| 674 
+++
 .pytool/Plugin/DebugMacroCheck/tests/MacroTest.py| 131 
+++
 .pytool/Plugin/DebugMacroCheck/tests/__init__.py |   0
 .pytool/Plugin/DebugMacroCheck/tests/test_DebugMacroCheck.py | 201 
+
 8 files changed, 2256 insertions(+)

diff --git 
a/.pytool/Plugin/DebugMacroCheck/BuildPlugin/DebugMacroCheckBuildPlugin.py 
b/.pytool/Plugin/DebugMacroCheck/BuildPlugin/DebugMacroCheckBuildPlugin.py
new file mode 100644
index ..b1544666025e
--- /dev/null
+++ b/.pytool/Plugin/DebugMacroCheck/BuildPlugin/DebugMacroCheckBuildPlugin.py
@@ -0,0 +1,127 @@
+# @file DebugMacroCheckBuildPlugin.py
+#
+# A build plugin that checks if DEBUG macros are formatted properly.
+#
+# In particular, that print format specifiers are defined
+# with the expected number of arguments in the variable
+# argument list.
+#
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+import logging
+import os
+import pathlib
+import sys
+import yaml
+
+# Import the build plugin
+plugin_file = pathlib.Path(__file__)
+sys.path.append(str(plugin_file.parent.parent))
+
+# flake8 (E402): Ignore flake8 module level import not at top of file
+import DebugMacroCheck  # noqa: E402
+
+from edk2toolext import edk2_logging   # noqa: E402
+from edk2toolext.environment.plugintypes.uefi_build_plugin import \
+IUefiBuildPlugin   # noqa: E402
+from edk2toolext.environment.uefi_build import UefiBuilder # noqa: E402
+from edk2toollib.uefi.edk2.path_utilities import Edk2Path  # noqa: E402
+from pathlib import Path   # noqa: E402
+
+
+class DebugMacroCheckBuildPlugin(IUefiBuildPlugin):
+
+def do_pre_build(self, builder: UefiBuilder) -> int:
+"""Debug Macro Check pre-build functionality.
+
+The plugin is invoked in pre-build since it can operate independently
+of build tools and to notify the user of any errors earlier in the
+build process to reduce feedback time.
+
+Args:
+builder (UefiBuilder): A UEFI builder object for this build.
+
+Returns:
+int: The number of debug macro errors found. Zero indicates the
+check either did not run or no errors were found.
+"""
+
+# Check if disabled in the environment
+env_disable = builder.env.GetValue("DISABLE_DEBUG_MACRO_CHECK")
+if env_disable:
+return 0
+
+# Only run on targets with compilation
+build_target = builder.env.GetValue("TARGET").lower()
+if "no-target" in build_target:
+return 0
+
+pp = builder.pp.split(os.pathsep)
+edk2 = Edk2Path(builder.ws, pp)
+package = edk2.GetContainingPackage(
+builder.mws.join(builder.ws,
+ builder.env.GetValue(
+"ACTIVE_PLATFORM")))
+package_path = Path(
+  edk2.GetAbsolutePathOnThisSystemFromEdk2RelativePath(
+package))
+
+# Every debug macro is printed at DEBUG logging level.
+# Ensure the level is above DEBUG while executing the macro check
+# plugin to avoid flooding the log handler.
+handler_level_context = []
+for h in logging.getLogger().handlers:
+if h.level < logging.INFO:
+handler_level_context.append((h, h.level))
+h.setLevel(logging.INFO)
+
+edk2_logging.log_progress("Checking DEBUG Macros")
+
+# There are two ways to specify macro substitution data for this
+# plugin.

[edk2-devel] [PATCH v1 6/7] OvmfPkg/PlatformCI: Disable DebugMacroCheck

2023-08-14 Thread Michael Kubacki

From: Michael Kubacki 

Disables the DebugMacroCheck CI plugin to reduce CI checks performed
in the package.

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Jordan Justen 
Cc: Gerd Hoffmann 
Signed-off-by: Michael Kubacki 
---
 OvmfPkg/PlatformCI/PlatformBuildLib.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/OvmfPkg/PlatformCI/PlatformBuildLib.py 
b/OvmfPkg/PlatformCI/PlatformBuildLib.py
index 1ada935d3cb4..f829738cdda4 100644
--- a/OvmfPkg/PlatformCI/PlatformBuildLib.py
+++ b/OvmfPkg/PlatformCI/PlatformBuildLib.py
@@ -170,6 +170,7 @@ class PlatformBuilder( UefiBuilder, BuildSettingsManager):
 self.env.SetValue("PRODUCT_NAME", "OVMF", "Platform Hardcoded")
 self.env.SetValue("MAKE_STARTUP_NSH", "FALSE", "Default to false")
 self.env.SetValue("QEMU_HEADLESS", "FALSE", "Default to false")
+self.env.SetValue("DISABLE_DEBUG_MACRO_CHECK", "TRUE", "Disable by 
default")
 return 0
 
 def PlatformPreBuild(self):
-- 
2.41.0.windows.3



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

[edk2-devel] [PATCH v1 5/7] DynamicTablesPkg.ci.yaml: Add debug macro exception

2023-08-14 Thread Michael Kubacki

From: Michael Kubacki 

Adds a CI YAML entry to acknowledge a case where custom strings
contain print specifiers for a single debug macro.

Cc: Sami Mujawar 
Cc: Alexei Fedorov 
Cc: Pierre Gondois 
Signed-off-by: Michael Kubacki 
---
 DynamicTablesPkg/DynamicTablesPkg.ci.yaml | 8 
 1 file changed, 8 insertions(+)

diff --git a/DynamicTablesPkg/DynamicTablesPkg.ci.yaml 
b/DynamicTablesPkg/DynamicTablesPkg.ci.yaml
index 5addf8626841..1d41d44bbf33 100644
--- a/DynamicTablesPkg/DynamicTablesPkg.ci.yaml
+++ b/DynamicTablesPkg/DynamicTablesPkg.ci.yaml
@@ -130,5 +130,13 @@
  # should be ignore
 "AdditionalIncludePaths": [] # Additional paths to spell check
  # (wildcards supported)
+},
+
+"DebugMacroCheck": {
+  "StringSubstitutions": {
+  # 
DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
+  # Reason: Debug format strings are dynamically set.
+  "Parser[Index].Format": "%d"
+  }
 }
 }
-- 
2.41.0.windows.3



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

[edk2-devel] [PATCH v1 4/7] ArmVirtPkg.ci.yaml: Add debug macro exception

2023-08-14 Thread Michael Kubacki

From: Michael Kubacki 

Adds a CI YAML entry to acknowledge a case where a macro is expanded
that contains a print specifier.

Cc: Ard Biesheuvel 
Cc: Leif Lindholm 
Cc: Sami Mujawar 
Cc: Gerd Hoffmann 
Signed-off-by: Michael Kubacki 
---
 ArmVirtPkg/ArmVirtPkg.ci.yaml | 8 
 1 file changed, 8 insertions(+)

diff --git a/ArmVirtPkg/ArmVirtPkg.ci.yaml b/ArmVirtPkg/ArmVirtPkg.ci.yaml
index 1e799dc4e194..506b0e72f0bb 100644
--- a/ArmVirtPkg/ArmVirtPkg.ci.yaml
+++ b/ArmVirtPkg/ArmVirtPkg.ci.yaml
@@ -125,5 +125,13 @@
 ],   # words to extend to the dictionary for this package
 "IgnoreStandardPaths": [],   # Standard Plugin defined paths that 
should be ignore
 "AdditionalIncludePaths": [] # Additional paths to spell check 
(wildcards supported)
+},
+
+"DebugMacroCheck": {
+  "StringSubstitutions": {
+  # DynamicTablesPkg/Include/ConfigurationManagerObject.h
+  # Reason: Expansion of macro that contains a print specifier.
+  "FMT_CM_OBJECT_ID": "0x%lx"
+  }
 }
 }
-- 
2.41.0.windows.3



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

[edk2-devel] [PATCH v1 3/7] SecurityPkg.ci.yaml: Add debug macro exception

2023-08-14 Thread Michael Kubacki

From: Michael Kubacki 

Adds a CI YAML entry to acknowledge a case where a single argument
is matched to a format specifier with a ternary operator.

Cc: Jiewen Yao 
Cc: Jian J Wang 
Signed-off-by: Michael Kubacki 
---
 SecurityPkg/SecurityPkg.ci.yaml | 9 +
 1 file changed, 9 insertions(+)

diff --git a/SecurityPkg/SecurityPkg.ci.yaml b/SecurityPkg/SecurityPkg.ci.yaml
index 2138b0a5e21b..3f03762bd6f9 100644
--- a/SecurityPkg/SecurityPkg.ci.yaml
+++ b/SecurityPkg/SecurityPkg.ci.yaml
@@ -106,5 +106,14 @@
 
 "Defines": {
 "BLD_*_CONTINUOUS_INTEGRATION": "TRUE",
+},
+
+"DebugMacroCheck": {
+  "StringSubstitutions": {
+  # SecurityPkg/IntelTdx/TdTcg2Dxe/TdTcg2Dxe.c
+  # Reason: Acknowledging use of two format specifiers in string with 
one argument
+  # Replace ternary operator in debug string with single 
specifier
+  'Index == COLUME_SIZE/2 ? " | %02x" : " %02x"': "%d"
+  }
 }
 }
-- 
2.41.0.windows.3



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

[edk2-devel] [PATCH v1 2/7] pip-requirements.txt: Add regex

2023-08-14 Thread Michael Kubacki

From: Michael Kubacki 

regex is a popular PIP module for regular expression support.

https://pypi.org/project/regex/

This change adds regex for the upcoming DebugMacroCheck plugin.

Cc: Sean Brogan 
Cc: Michael D Kinney 
Cc: Liming Gao 
Signed-off-by: Michael Kubacki 
---
 pip-requirements.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pip-requirements.txt b/pip-requirements.txt
index c221576bef3a..0f615f9be28a 100644
--- a/pip-requirements.txt
+++ b/pip-requirements.txt
@@ -17,4 +17,4 @@ edk2-pytool-extensions~=0.23.2
 edk2-basetools==0.1.48
 antlr4-python3-runtime==4.7.1
 lcov-cobertura==2.0.2
-
+regex==2023.8.8
-- 
2.41.0.windows.3



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

[edk2-devel] [PATCH v1 1/7] RedfishPkg/PlatformHostInterfaceBmcUsbNicLib: Fix DEBUG macro args

2023-08-14 Thread Michael Kubacki

From: Michael Kubacki 

Some macros added have a mismatched number of  print specifiers to
arguments.

Cc: Abner Chang 
Cc: Nickle Wang 
Cc: Igor Kulchytskyy 
Signed-off-by: Michael Kubacki 
---
 
RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInterfaceBmcUsbNicLib.c
 | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git 
a/RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInterfaceBmcUsbNicLib.c
 
b/RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInterfaceBmcUsbNicLib.c
index d18d83b93808..95900579118b 100644
--- 
a/RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInterfaceBmcUsbNicLib.c
+++ 
b/RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInterfaceBmcUsbNicLib.c
@@ -65,10 +65,10 @@ ProbeRedfishCredentialBootstrap (
(ResponseData.CompletionCode == 
REDFISH_IPMI_COMP_CODE_BOOTSTRAP_CREDENTIAL_DISABLED)
   ))
   {
-DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, "Redfish Credentail 
Bootstrapping is supported\n", __func__));
+DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, "Redfish Credential 
Bootstrapping is supported\n"));
 ReturnBool = TRUE;
   } else {
-DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, "Redfish Credentail 
Bootstrapping is not supported\n", __func__));
+DEBUG ((DEBUG_REDFISH_HOST_INTERFACE, "Redfish Credential 
Bootstrapping is not supported\n"));
 ReturnBool = FALSE;
   }
 
@@ -645,7 +645,7 @@ HostInterfaceIpmiCheckMacAddress (
 &ResponseDataSize
 );
   if (EFI_ERROR (Status)) {
-DEBUG ((DEBUG_ERROR, "- Fails to send command.\n", ChannelNum));
+DEBUG ((DEBUG_ERROR, "- Channel %d fails to send command.\n", 
ChannelNum));
 continue;
   }
 
@@ -1084,7 +1084,7 @@ CheckBmcUsbNicOnHandles (
 (VOID **)&DevicePath
 );
 if (EFI_ERROR (Status)) {
-  DEBUG ((DEBUG_ERROR, "Failed to locate SNP on %d handle.\n", 
__func__, Index));
+  DEBUG ((DEBUG_ERROR, "Failed to locate SNP on %d handle.\n", Index));
   continue;
 }
 
-- 
2.41.0.windows.3



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

[edk2-devel] [PATCH v1 0/7] Add DebugMacroCheck

2023-08-14 Thread Michael Kubacki

From: Michael Kubacki 

Adds a new script and build plugin called DebugMacroCheck.

The script verifies that the number of print specifiers match the
number of arguments in DEBUG() calls.

Overview:

- Build plugin: BuildPlugin/DebugMacroCheckBuildPlugin.py
  - Runs on any build target that is not NO-TARGET
- Standalone script: DebugMacroCheck.py
  - Run `DebugMacroCheck.py --help` to see command line options
- Unit tests:
  - Tests/test_DebugMacroCheck.py
  - Can be run with:
`python -m unittest discover -s ./.pytool/Plugin/DebugMacroCheck/tests -v`
  - Also visible in VS Code Test Explorer

Background:

The tool has been constantly run against edk2 derived code for about
a year now. During that time, its found over 20 issues in edk2, over
50 issues in various vendor code, and numerous other issues specific
to Project Mu.

See the following series for a batch of issues previously fixed in
edk2 discovered by the tool:

  https://edk2.groups.io/g/devel/message/93104

I've received interest from vendors to place it in edk2 to
immediately find issues in the upstream and make it easier for edk2
consumers to directly acquire it. That led to this patch series.

This would run in edk2 as a build plugin. All issues in the edk2
codebase have been resolved so this would find new issues before
they are merged into the codebase.

The script is meant to be portable so it can be run as a build plugin
or dropped as a standalone script into other environments alongside
the unit tests.

Series Overview:

- Fixes outstanding issues in RedfishPkg
- Adds the `regex` PIP module to pip-requirements.txt
- Adds exceptions for debug macro usage in ArmVirtPkg,
  DynamicTablesPkg, and SecurityPkg
- Disables the plugin in OvmfPkg per maintainer's previous
  preferences
- Adds the plugin

The plugin (this series) is running with passing CI results as shown
in this PR:
  https://github.com/tianocore/edk2/pull/4736

Cc: Abner Chang 
Cc: Alexei Fedorov 
Cc: Ard Biesheuvel 
Cc: Gerd Hoffmann 
Cc: Igor Kulchytskyy 
Cc: Jian J Wang 
Cc: Jiewen Yao 
Cc: Jordan Justen 
Cc: Leif Lindholm 
Cc: Liming Gao 
Cc: Michael D Kinney 
Cc: Nickle Wang 
Cc: Pierre Gondois 
Cc: Sami Mujawar 
Cc: Sean Brogan 

Michael Kubacki (7):
  RedfishPkg/PlatformHostInterfaceBmcUsbNicLib: Fix DEBUG macro args
  pip-requirements.txt: Add regex
  SecurityPkg.ci.yaml: Add debug macro exception
  ArmVirtPkg.ci.yaml: Add debug macro exception
  DynamicTablesPkg.ci.yaml: Add debug macro exception
  OvmfPkg/PlatformCI: Disable DebugMacroCheck
  .pytool/Plugin: Add DebugMacroCheck

 
RedfishPkg/Library/PlatformHostInterfaceBmcUsbNicLib/PlatformHostInterfaceBmcUsbNicLib.c
 |   8 +-
 .pytool/Plugin/DebugMacroCheck/BuildPlugin/DebugMacroCheckBuildPlugin.py   
  | 127 +++
 .pytool/Plugin/DebugMacroCheck/BuildPlugin/DebugMacroCheck_plug_in.yaml
  |  11 +
 .pytool/Plugin/DebugMacroCheck/DebugMacroCheck.py  
  | 859 
 .pytool/Plugin/DebugMacroCheck/Readme.md   
  | 253 ++
 .pytool/Plugin/DebugMacroCheck/tests/DebugMacroDataSet.py  
  | 674 +++
 .pytool/Plugin/DebugMacroCheck/tests/MacroTest.py  
  | 131 +++
 .pytool/Plugin/DebugMacroCheck/tests/__init__.py   
  |   0
 .pytool/Plugin/DebugMacroCheck/tests/test_DebugMacroCheck.py   
  | 201 +
 ArmVirtPkg/ArmVirtPkg.ci.yaml  
  |   8 +
 DynamicTablesPkg/DynamicTablesPkg.ci.yaml  
  |   8 +
 OvmfPkg/PlatformCI/PlatformBuildLib.py 
  |   1 +
 SecurityPkg/SecurityPkg.ci.yaml
  |   9 +
 pip-requirements.txt   
  |   2 +-
 14 files changed, 2287 insertions(+), 5 deletions(-)
 create mode 100644 
.pytool/Plugin/DebugMacroCheck/BuildPlugin/DebugMacroCheckBuildPlugin.py
 create mode 100644 
.pytool/Plugin/DebugMacroCheck/BuildPlugin/DebugMacroCheck_plug_in.yaml
 create mode 100644 .pytool/Plugin/DebugMacroCheck/DebugMacroCheck.py
 create mode 100644 .pytool/Plugin/DebugMacroCheck/Readme.md
 create mode 100644 .pytool/Plugin/DebugMacroCheck/tests/DebugMacroDataSet.py
 create mode 100644 .pytool/Plugin/DebugMacroCheck/tests/MacroTest.py
 create mode 100644 .pytool/Plugin/DebugMacroCheck/tests/__init__.py
 create mode 100644 .pytool/Plugin/DebugMacroCheck/tests/test_DebugMacroCheck.py

-- 
2.41.0.windows.3



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

Re: [edk2-devel] [PATCH edk2-platforms v3 0/3] Platform/QemuSbsa: add GIC ITS

2023-08-14 Thread Graeme Gregory

Patch series seems sane to me.

Reviewed-by: Graeme Gregory 

(I seem to be moving further and further from ARM development and even
further from SBSA machine development so Ill probably remove myself
as a reviewer for this machine)

On Wed, Jul 19, 2023 at 02:08:39PM +0200, Marcin Juszkiewicz wrote:
> SBSA Reference Platform can have GIC ITS present. And when it has then
> we can have complex PCI Express setup (and some other things).
> 
> First patch adds support for GIC ITS. Address is read from TF-A via SMC
> call. IORT is generated, MADT has ITS information. Linux boots and sees
> GIC ITS as expected. SMMU information is also provided in IORT and used.
> 
> Second patch introduces PcdSmmuBase variable to avoid using magic number
> in IORT generation.
> 
> Third patch takes care of system where GIC ITS is not present (like QEMU
> 8.0). If GIC ITS address is not set then there is no mention of it in
> MADT and there is no IORT, Linux boots.
> 
> Changes since v2:
> - no ITS == no IORT
> 
> Changes since v1:
> - IORT is generated in C
> - no ITS == no ITS node in IORT
> - introduced PcdSmmuBase
> 
> Marcin Juszkiewicz (2):
>   Platform/QemuSbsa: add dynamic PcdSmmuBase
>   Platform/SbsaQemu: handle systems without GIC ITS
> 
> Shashi Mallela (1):
>   Platform/SbsaQemu: add GIC ITS support
> 
>  Silicon/Qemu/SbsaQemu/SbsaQemu.dec|   4 +
>  Platform/Qemu/SbsaQemu/SbsaQemu.dsc   |   4 +
>  .../Qemu/SbsaQemu/AcpiTables/AcpiTables.inf   |   1 +
>  .../SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf   |   2 +
>  .../SbsaQemuPlatformDxe.inf   |   1 +
>  .../Include/IndustryStandard/SbsaQemuAcpi.h   |  11 +
>  .../Include/IndustryStandard/SbsaQemuSmc.h|   1 +
>  .../Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c | 208 +-
>  .../SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c |  10 +
>  9 files changed, 241 insertions(+), 1 deletion(-)
> 
> -- 
> 2.41.0
> 


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

[edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

2023-08-14 Thread Henz, Patrick

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

XhciDxe uses the timer functionality provided by the
boot services table to detect timeout conditions. This
breaks the driver's ExitBootServices call back, as
CoreExitBootServices halts the timer before signaling
the ExitBootServices event. If the host controller
fails to halt in the call back, the timeout condition
will never occur and the boot gets stuck in an indefinite
spin loop. Use the free running timer provided by
TimerLib to calculate timeouts, avoiding the potential
hang.

Signed-off-by: Patrick Henz 
Reviewed-by:
---
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c  | 56 +++
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h  | 22 
 MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf |  2 +
 MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 67 +--
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 68 +---
 5 files changed, 129 insertions(+), 86 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c 
b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
index 62682dd27c..1dcbe20512 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
@@ -1,6 +1,7 @@
 /** @file
   The XHCI controller driver.
 
+(C) Copyright 2023 Hewlett Packard Enterprise Development LP
 Copyright (c) 2011 - 2022, Intel Corporation. All rights reserved.
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -2294,3 +2295,58 @@ XhcDriverBindingStop (
 
   return EFI_SUCCESS;
 }
+
+/**
+  Computes and returns the elapsed time in nanoseconds since
+  PreviousTick. The value of PreviousTick is overwritten with the
+  current performance counter value.
+
+  @param  PreviousTickPointer to PreviousTick count.
+  @return The elapsed time in nanoseconds since PreviousCount.
+  PreviousCount is overwritten with the current performance
+  counter value.
+**/
+UINT64
+XhcGetElapsedTime (
+  IN OUT UINT64  *PreviousTick
+  )
+{
+  UINT64  StartValue;
+  UINT64  EndValue;
+  UINT64  CurrentTick;
+  UINT64  Delta;
+
+  GetPerformanceCounterProperties (&StartValue, &EndValue);
+
+  CurrentTick = GetPerformanceCounter ();
+
+  //
+  // Determine if the counter is counting up or down
+  //
+  if (StartValue < EndValue) {
+//
+// Counter counts upwards, check for an overflow condition
+//
+if (*PreviousTick > CurrentTick) {
+  Delta = (EndValue - *PreviousTick) + CurrentTick;
+} else {
+  Delta = CurrentTick - *PreviousTick;
+}
+  } else {
+//
+// Counter counts downwards, check for an underflow condition
+//
+if (*PreviousTick < CurrentTick) {
+  Delta = (StartValue - CurrentTick) + *PreviousTick;
+} else {
+  Delta = *PreviousTick - CurrentTick;
+}
+  }
+
+  //
+  // Set PreviousTick to CurrentTick
+  //
+  *PreviousTick = CurrentTick;
+
+  return GetTimeInNanoSecond (Delta);
+}
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h 
b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
index ca223bd20c..77feb66647 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
@@ -2,6 +2,7 @@
 
   Provides some data structure definitions used by the XHCI host controller 
driver.
 
+(C) Copyright 2023 Hewlett Packard Enterprise Development LP
 Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.
 Copyright (c) Microsoft Corporation.
 SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -26,6 +27,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -37,6 +39,10 @@ typedef struct _USB_DEV_CONTEXTUSB_DEV_CONTEXT;
 #include "ComponentName.h"
 #include "UsbHcMem.h"
 
+//
+// Converts a count from microseconds to nanoseconds
+//
+#define XHC_MICROSECOND_TO_NANOSECOND(Time)  ((UINT64)(Time) * 1000)
 //
 // The unit is microsecond, setting it as 1us.
 //
@@ -720,4 +726,20 @@ XhcAsyncIsochronousTransfer (
   IN VOID*Context
   );
 
+/**
+  Computes and returns the elapsed time in nanoseconds since
+  PreviousTick. The value of PreviousTick is overwritten with the
+  current performance counter value.
+
+  @param  PreviousTickPointer to PreviousTick count.
+  before calling this function.
+  @return The elapsed time in nanoseconds since PreviousCount.
+  PreviousCount is overwritten with the current performance
+  counter value.
+**/
+UINT64
+XhcGetElapsedTime (
+  IN OUT UINT64  *PreviousTick
+  );
+
 #endif
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf 
b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
index 5865d86822..18ef87916a 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciDxe.inf
@@ -3,6 +3,7 @@
 #  It implements the interfaces of monitoring the status of all ports and 
transferring
 #  Control, Bulk, Interrupt and Isochronous requests to those attached usb 
LS/FS/HS/SS devices.
 #
+#  (C) Copyright 2023 Hewlett Packard Enterprise Development LP
 #  Copyright (c) 2

Re: [edk2-devel] [edk2-platforms][PATCH V3-1] IpmiFeaturePkg:Provided multiple IPMI interface support in DXE and SMM

2023-08-14 Thread Huang, Yanbo

Hi Arun,

When I apply these changes in intel platform, I will hit some "warning treated 
as error" issues, so I file a bug in tianocore: 
https://bugzilla.tianocore.org/show_bug.cgi?id=4522
Could you please help to check it?
Thanks!

Best Regards,
Yanbo Huang


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

Re: [edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue

2023-08-14 Thread Ranbir Singh

When code is compiled in RELEASE mode, the ASSERT (Interval != 0);
statement gets NULL'ified. Hence for Coverity it simply doesn't exist.
Further, IMO Coverity seems to look at a function in isolation whether it
is safe or not. It is however not necessary to fix all issues pointed out
by Coverity if something looks to be a false positive or something is done
deliberately.

I will go by returning 1 and can still keep ASSERT if you prefer that way.
I will put a change post ASSERT statement only, so that DEBUG mode still
catches if wrongly '0' is sent.

Should I do this or not - update the function header as well to reflect the
minimum 'Interval' valid value as 1 and that if 0 is wrongly sent, it will
be treated as 1 only in RELEASE mode ?

On Mon, Aug 14, 2023 at 2:09 PM Wu, Hao A  wrote:

> My take is that:
>
> For all the possible calling scenario of UhciConvertPollRate() in current
>
> UhciDxe driver implementation, it is guaranteed that the input parameter
>
> 'Interval' will not be 0.
>
>
>
> I think this is why the "ASSERT (Interval != 0);" statement is put here to
>
> indicate such case will never happen and notify developers for future
> changes
>
> in this driver that something is wrong.
>
>
>
> I do not know why Coverity is unable to figure this out.
>
>
>
> My personal preference is to return 1 (i.e. 1 << 0) as if the minimum
> allowed
>
> value for 'Interval' (which is 1) is being passed into this
> UhciConvertPollRate
>
> function if anything does go wrong brought by future changes.
>
>
>
> Best Regards,
>
> Hao Wu
>
>
>
> *From:* Ranbir Singh 
> *Sent:* Monday, August 14, 2023 3:49 PM
> *To:* Wu, Hao A 
> *Cc:* devel@edk2.groups.io; Ni, Ray ; Veeresh Sangolli <
> veeresh.sango...@dellteam.com>
> *Subject:* Re: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT
> Coverity issue
>
>
>
>
>
> On Thu, Aug 10, 2023 at 8:09 AM Wu, Hao A  wrote:
>
> > -Original Message-
> > From: Ranbir Singh 
> > Sent: Monday, July 17, 2023 7:39 PM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Wu, Hao A ; Ni, Ray ; Veeresh
> > Sangolli 
> > Subject: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT
> > Coverity issue
> >
> > From: Ranbir Singh 
> >
> > The function UhciConvertPollRate has a check
> >
> > ASSERT (Interval != 0);
> >
> > but this comes into play only in DEBUG mode. In Release mode, there is
> > no handling if the Interval parameter value is ZERO. To avoid shifting
> > by a negative amount later in the code flow in this undesirable case,
> > it is better to handle it as well by simply returning ZERO.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4211
> >
> > Cc: Hao A Wu 
> > Cc: Ray Ni 
> > Co-authored-by: Veeresh Sangolli 
> > Signed-off-by: Ranbir Singh 
> > Signed-off-by: Ranbir Singh 
> > ---
> >  MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > index c08f9496969b..8ddef4b68ccf 100644
> > --- a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > +++ b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > @@ -214,6 +214,10 @@ UhciConvertPollRate (
> >
> >
> >ASSERT (Interval != 0);
> >
> >
> >
> > +  if (Interval == 0) {
> >
> > +return 0;
>
>
> Return 0 will cause further issues within UhciLinkQhToFrameList() &
> UhciUnlinkQhFromFrameList() where the returned value is being used in the
> below
> 'for' statement:
>
> for (Index = 0; Index < UHCI_FRAME_NUM; Index += Qh->Interval) {
>
>
>
> One way is to prohibit UhciCreateQh() to proceed normally by checking if
> Interval parameter value is 0 at the very beginning and may be add a DEBUG
> statement and/or an ASSERT as well - return value then has to be NULL from
> this function for this specific case of invalid Interval parameter value
> being received as 0. That should take care of the issue Hao pointed above
> in for loop.
>
>
>
> UhciCreateAsyncReq() may also need to introduce a Polling Interval
> parameter value check similar to as it exists in
> Uhci2AsyncInterruptTransfer() for a future direct call scenario.
>
>
>
> My thought is to return 1 (i.e. 1 << 0) as if the minimum allowed value for
> 'Interval' (which is 1) is being passed into this UhciConvertPollRate
> function.
>
>
>
> If we choose to modify UhciCreateQh(), then we may have status quo
> in UhciConvertPollRate(). However, we can still pursue the return 1 (i.e. 1
> << 0) option here in which case I guess I should update the function header
> as well to reflect the minimum 'Interval' valid value as 1 and that if 0 is
> wrongly sent, it will be treated as 1 only. *When we do that in RELEASE
> mode, should we still retain ASSERT ?* I think NO.
>
>
>
> If we choose to simply modify UhciConvertPollRate() as stated, then we
> can have the status quo in UhciCreateQh() and UhciCreateAsyncReq().
>
>
>
>
> Best Regards,
> Hao Wu
>
>
> >
> > +  }
> >
> > +
> >
> >//
> >
> >// Find the index

Re: [edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue

2023-08-14 Thread Wu, Hao A

My take is that:
For all the possible calling scenario of UhciConvertPollRate() in current
UhciDxe driver implementation, it is guaranteed that the input parameter
'Interval' will not be 0.

I think this is why the "ASSERT (Interval != 0);" statement is put here to
indicate such case will never happen and notify developers for future changes
in this driver that something is wrong.

I do not know why Coverity is unable to figure this out.

My personal preference is to return 1 (i.e. 1 << 0) as if the minimum allowed
value for 'Interval' (which is 1) is being passed into this UhciConvertPollRate
function if anything does go wrong brought by future changes.

Best Regards,
Hao Wu

From: Ranbir Singh 
Sent: Monday, August 14, 2023 3:49 PM
To: Wu, Hao A 
Cc: devel@edk2.groups.io; Ni, Ray ; Veeresh Sangolli 

Subject: Re: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT 
Coverity issue

On Thu, Aug 10, 2023 at 8:09 AM Wu, Hao A 
mailto:hao.a...@intel.com>> wrote:
> -Original Message-
> From: Ranbir Singh mailto:rsi...@ventanamicro.com>>
> Sent: Monday, July 17, 2023 7:39 PM
> To: devel@edk2.groups.io; 
> rsi...@ventanamicro.com
> Cc: Wu, Hao A mailto:hao.a...@intel.com>>; Ni, Ray 
> mailto:ray...@intel.com>>; Veeresh
> Sangolli mailto:veeresh.sango...@dellteam.com>>
> Subject: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT
> Coverity issue
>
> From: Ranbir Singh mailto:ranbir.sin...@dell.com>>
>
> The function UhciConvertPollRate has a check
>
> ASSERT (Interval != 0);
>
> but this comes into play only in DEBUG mode. In Release mode, there is
> no handling if the Interval parameter value is ZERO. To avoid shifting
> by a negative amount later in the code flow in this undesirable case,
> it is better to handle it as well by simply returning ZERO.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4211
>
> Cc: Hao A Wu mailto:hao.a...@intel.com>>
> Cc: Ray Ni mailto:ray...@intel.com>>
> Co-authored-by: Veeresh Sangolli 
> mailto:veeresh.sango...@dellteam.com>>
> Signed-off-by: Ranbir Singh 
> mailto:ranbir.sin...@dell.com>>
> Signed-off-by: Ranbir Singh 
> mailto:rsi...@ventanamicro.com>>
> ---
>  MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> index c08f9496969b..8ddef4b68ccf 100644
> --- a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> @@ -214,6 +214,10 @@ UhciConvertPollRate (
>
>
>ASSERT (Interval != 0);
>
>
>
> +  if (Interval == 0) {
>
> +return 0;

Return 0 will cause further issues within UhciLinkQhToFrameList() &
UhciUnlinkQhFromFrameList() where the returned value is being used in the below
'for' statement:

for (Index = 0; Index < UHCI_FRAME_NUM; Index += Qh->Interval) {

One way is to prohibit UhciCreateQh() to proceed normally by checking if 
Interval parameter value is 0 at the very beginning and may be add a DEBUG 
statement and/or an ASSERT as well - return value then has to be NULL from this 
function for this specific case of invalid Interval parameter value being 
received as 0. That should take care of the issue Hao pointed above in for loop.

UhciCreateAsyncReq() may also need to introduce a Polling Interval parameter 
value check similar to as it exists in Uhci2AsyncInterruptTransfer() for a 
future direct call scenario.

My thought is to return 1 (i.e. 1 << 0) as if the minimum allowed value for
'Interval' (which is 1) is being passed into this UhciConvertPollRate function.

If we choose to modify UhciCreateQh(), then we may have status quo in 
UhciConvertPollRate(). However, we can still pursue the return 1 (i.e. 1 << 0) 
option here in which case I guess I should update the function header as well 
to reflect the minimum 'Interval' valid value as 1 and that if 0 is wrongly 
sent, it will be treated as 1 only. When we do that in RELEASE mode, should we 
still retain ASSERT ? I think NO.

If we choose to simply modify UhciConvertPollRate() as stated, then we can have 
the status quo in UhciCreateQh() and UhciCreateAsyncReq().

Best Regards,
Hao Wu

>
> +  }
>
> +
>
>//
>
>// Find the index (1 based) of the highest non-zero bit
>
>//
>
> --
> 2.34.1

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107729): https://edk2.groups.io/g/devel/message/107729
Mute This Topic: https://groups.io/mt/100212109/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] OvmfPkg/Bhyve: build platform info HOB

2023-08-14 Thread Corvin Köhne

On Thu, 2023-08-10 at 15:54 -0400, Michael Kubacki wrote:
> I'm not sure why test results are not being reported that may be a 
> larger issue.
> 
> I pulled the branch locally, run uncrustify, and it automatically
> fixed 
> the code to pass.
> 
> The issue is the following function call:
> 
>    BuildPlatformInfoHob();
> 
> Should be:
> 
>    BuildPlatformInfoHob ();
> 

I already run uncrustify locally but it doesn't complain about it.
Thank you!

Should I resend an updated patch?
Can this change make it into edk-stable202308? Otherwise, fwcfg won't
be usable by bhyve in this version.


-- 
Kind regards,
Corvin


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




signature.asc
Description: This is a digitally signed message part

Re: [edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue

2023-08-14 Thread Ranbir Singh

On Thu, Aug 10, 2023 at 8:09 AM Wu, Hao A  wrote:

> > -Original Message-
> > From: Ranbir Singh 
> > Sent: Monday, July 17, 2023 7:39 PM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Wu, Hao A ; Ni, Ray ; Veeresh
> > Sangolli 
> > Subject: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT
> > Coverity issue
> >
> > From: Ranbir Singh 
> >
> > The function UhciConvertPollRate has a check
> >
> > ASSERT (Interval != 0);
> >
> > but this comes into play only in DEBUG mode. In Release mode, there is
> > no handling if the Interval parameter value is ZERO. To avoid shifting
> > by a negative amount later in the code flow in this undesirable case,
> > it is better to handle it as well by simply returning ZERO.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4211
> >
> > Cc: Hao A Wu 
> > Cc: Ray Ni 
> > Co-authored-by: Veeresh Sangolli 
> > Signed-off-by: Ranbir Singh 
> > Signed-off-by: Ranbir Singh 
> > ---
> >  MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > index c08f9496969b..8ddef4b68ccf 100644
> > --- a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > +++ b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > @@ -214,6 +214,10 @@ UhciConvertPollRate (
> >
> >
> >ASSERT (Interval != 0);
> >
> >
> >
> > +  if (Interval == 0) {
> >
> > +return 0;
>
>
> Return 0 will cause further issues within UhciLinkQhToFrameList() &
> UhciUnlinkQhFromFrameList() where the returned value is being used in the
> below
> 'for' statement:
>
> for (Index = 0; Index < UHCI_FRAME_NUM; Index += Qh->Interval) {
>
>
One way is to prohibit UhciCreateQh() to proceed normally by checking if
Interval parameter value is 0 at the very beginning and may be add a DEBUG
statement and/or an ASSERT as well - return value then has to be NULL from
this function for this specific case of invalid Interval parameter value
being received as 0. That should take care of the issue Hao pointed above
in for loop.

UhciCreateAsyncReq() may also need to introduce a Polling Interval
parameter value check similar to as it exists in
Uhci2AsyncInterruptTransfer() for a future direct call scenario.


> My thought is to return 1 (i.e. 1 << 0) as if the minimum allowed value for
> 'Interval' (which is 1) is being passed into this UhciConvertPollRate
> function.
>

If we choose to modify UhciCreateQh(), then we may have status quo
in UhciConvertPollRate(). However, we can still pursue the return 1 (i.e. 1
<< 0) option here in which case I guess I should update the function header
as well to reflect the minimum 'Interval' valid value as 1 and that if 0 is
wrongly sent, it will be treated as 1 only. *When we do that in RELEASE
mode, should we still retain ASSERT ?* I think NO.

If we choose to simply modify UhciConvertPollRate() as stated, then we
can have the status quo in UhciCreateQh() and UhciCreateAsyncReq().


> Best Regards,
> Hao Wu
>
>
> >
> > +  }
> >
> > +
> >
> >//
> >
> >// Find the index (1 based) of the highest non-zero bit
> >
> >//
> >
> > --
> > 2.34.1
>
>


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

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

[edk2-devel] About EDK2 supports Self Modifying Code

Re: [edk2-devel] [PATCH v1 0/7] Uncrustify GoogleTest update

[edk2-devel] Event: TianoCore Bug Triage - APAC / NAMO - Tuesday, August 15, 2023 #cal-reminder

Re: [edk2-devel] [PATCH v1 0/7] Add DebugMacroCheck

Re: [edk2-devel] OVMF build.sh change is incorrect

[edk2-devel] OVMF build.sh change is incorrect

Re: [edk2-devel] [PATCH v1 0/7] Add DebugMacroCheck

Re: [edk2-devel] [PATCH v1 0/7] Add DebugMacroCheck

[edk2-devel] Now: Tools, CI, Code base construction meeting series - Monday, August 14, 2023 #cal-notice

Re: [edk2-devel] [PATCH v1 0/7] Add DebugMacroCheck

[edk2-devel] Event: Tools, CI, Code base construction meeting series - Monday, August 14, 2023 #cal-reminder

[edk2-devel] [PATCH v1 7/7] .pytool/Plugin: Add DebugMacroCheck

[edk2-devel] [PATCH v1 6/7] OvmfPkg/PlatformCI: Disable DebugMacroCheck

[edk2-devel] [PATCH v1 5/7] DynamicTablesPkg.ci.yaml: Add debug macro exception

[edk2-devel] [PATCH v1 4/7] ArmVirtPkg.ci.yaml: Add debug macro exception

[edk2-devel] [PATCH v1 3/7] SecurityPkg.ci.yaml: Add debug macro exception

[edk2-devel] [PATCH v1 2/7] pip-requirements.txt: Add regex

[edk2-devel] [PATCH v1 1/7] RedfishPkg/PlatformHostInterfaceBmcUsbNicLib: Fix DEBUG macro args

[edk2-devel] [PATCH v1 0/7] Add DebugMacroCheck

Re: [edk2-devel] [PATCH edk2-platforms v3 0/3] Platform/QemuSbsa: add GIC ITS

[edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

Re: [edk2-devel] [edk2-platforms][PATCH V3-1] IpmiFeaturePkg:Provided multiple IPMI interface support in DXE and SMM

Re: [edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue

Re: [edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue

Re: [edk2-devel] [PATCH v1 1/1] OvmfPkg/Bhyve: build platform info HOB

Re: [edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue

27 matches

Site Navigation

Mail list logo

Footer information