Closed
Conversation
… most of the time. We should favor "always", not "force", for inclusivity. Also to match gcc/clang terminology. We should, later, move all code out of headers, and __forceinline will go away. There is not much, really. There will only be extern ALWAYS_INLINE. Plain inline, and static inline, are really, mostly, historical accidents we can avoid now. Now that we have LTCG and extern ALWAYS_INLINE. We don't need code in headers, to make it inlinable. However code in headers has the advantage of not needing to be built for every client combination. Look at what WIL does, where it changes its namespace for every define it is sensitive to. Related work items: #54710354, #57987809
clang: Change ReadNoFence to just volatile. volatile_accessors presents a slight porting problem and duplicated code. This fixes part of it, by changing NoFence to just be volatile. No casts are required. It is how NoFence works, except on ARM64EC with default compiler flags, or ARM64 with non-default compiler flags, where volatile might be slower, but still correct. UEFI will never target ARM64EC, and again, it works either way, just possibly faster or slower. This was Oliver's recommendation. Cleanup and repair of ReadAcquire/WriteRelease, which will be further dealt with, in a later PR. ---- This pull request is a code cleanup aimed at improving clang compatibility by replacing custom non-fence read/write functions with direct volatile accesses. The PR removes redundant helper functions (ReadNoFence, WriteNoFence, etc.) in favor of direct volatile accesses and marks several structure fields as volatile to ensure correct memory semantics under clang. It also adds documentation for clang-specific build instructions. - **`MsvmPkg/Include/Private/EfiNt.h` and `MsvmPkg/Include/Synchronization.h`**: Removed non-fence inline functions and simplified read/write operations. - **`MsvmPkg/Include/Vmbus/VmbusPacketFormat.h` and `VmbusPacketInterface.h`**: Updated struct field definitions by adding volatile qualifiers. - **`MsvmPkg/EmclDxe/RingBuffer.c` and `Emcl.c`**: Replaced calls to ReadNoFence/WriteNoFence with direct volatile field accesses. - **`MsvmPkg/Docs/Clang.txt`**: Added documentation outlining clang build setup and commands. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #54710354, #57987809
Clang: Braces initializing subobjects (and maybe not scalars, though that is allowed).
ExcludeMainFvFromMeasurementLib.c:19:
suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
(EFI_PHYSICAL_ADDRESS) FixedPcdGet64 (PcdFvBaseAddress),
VariableDxe.c: suggest braces around initialization of subobject
[-Werror,-Wmissing-braces]
EFI_GUID hyperVGuid = HYPERV_PRIVATE_NAMESPACE;
^~~~~~~~~~~~~~~~~~~~~~~~
VariableDxe.c: expanded from macro 'HYPERV_PRIVATE_NAMESPACE'
{ 0x610b9e98, 0xc6f6, 0x47f8, 0x8b, 0x47, 0x2d, 0x2d, 0xa0, 0xd5, 0x2a, 0x91 }
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
VpcivscDxe.c: error: suggest braces around initialization of subobject
[-Werror,-Wmissing-braces]
0, // Slot
^
{}
etc.
I think this is a nice feature of the language and Clang is too warning-prone, but oh well.
Related work items: #57987809
…ported. This is a separate library to easily ensure there is one everything, and no duplicates. ---- Temporary stubbing of assembly routines to support Clang builds until the port is completed. This PR adds a temporary library with stub functions for assembly routines, enabling Clang compilation support and updating various module manifests to integrate it. - `MsvmPkg/Library/ClangTemporaryLib/ClangTemporaryLib.c`: Introduces dummy implementations for multiple assembly entry points under a Clang-specific flag. - `MsvmPkg/Library/ClangTemporaryLib/ClangTemporaryLib.inf`: Provides the module descriptor for the new temporary library. - Multiple project manifest files (e.g., `HostVisibilityLib.inf`, `CpuDxe.inf`, `EfiHvDxe.inf`, `BdDebugLib.inf`, `DxeBdLib.inf`, and DSC files) are updated to reference `ClangTemporaryLib`. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #54710354, #57987809, #60275790
Clang: Fix `ui64` suffix.
```
HvTimerLib.c: error: invalid suffix 'ui64' on integer constant
Stall100ns(MicroSeconds * 10ui64);
^
```
Use portable `stdint.h` and `UINT64_C` for constant of type `uint64_t`, presumed to be like `UINT64`.
There are 2-3 ways to define `uint64_t` and `UINT64`, so they are hypothetically different.
- unsigned __int64, very old option in Microsoft and Digital compilers
- unsigned long long, very old option in other compilers, and portable since C99
- unsigned long on all Unix 64bit targets
`__int64` and `long long` are likely transparent aliases of each other on systems with `__int64`, so this might be two options, not three. (i.e. see if the mangle the same or not in C++, they probably do).
Related work items: #57987809
… And update ARMv7 to ARMv8.0 ReadAcquire/WriteRelease are duplicated in EfiNt.h and Synchronization.h and not portable. Fix both. **NOTE: This also changes the code sequences.** The original Windows ARM64 code was copied from ARM32. I reported this a few years ago, spurring Windows to update. That is where the `__dmb` came from. ARM64 v1.0 is ARMv8.0 and offers ldar/stlr. Use that instead. vmbus.sys on the other side, has already moved ahead similarly, and beyond (to `ldapr`).
A few months ago, either because of ARM64 port, or more likely, compiler update, a use of: _InterlockedExchange64 got replaced by InterlockedCompareExchange64. Because _InterlockedExchange64 was no longer linking. These are not drop-in replacements, and the resulting code is not convincingly correct. The reason this would change from linking to not, is because this was inlining a sequence that ARM says not to use going forward, on newer hardware. The library this calls, on mainstream Windows, dynamically detects hardware and choses ARMv8.0 instructions or ARMv8.1. UEFI does not presently have this library. Our interlocked code, upstream, including InterlockedExchangeCompare64 is largely assembly, unchanged for years, and not following ARM's recent guidance. It does not matter if we inline or call EFI, we are not following guidance either way. None of the three states follow guidance: - main before this PR - this PR - before the PR this is basically undoing In this PR we ask the compiler to inline the code, so it links. Sticking with ARMv8.0 code. We need to visit this more. i.e. to be adaptive at runtime to 8.0 vs. 8.1, in the upstream code, and here. Upstream should probably add InterlockedExchange64 as well. Note that the problem was added to fix the ARM64 build, but the problem is portable. This could be a shipping race in AMD64. This will require a little more clang porting work also. Thank @<Oliver Smith-Denny> again for showing me where to set compiler/linker options. ---- #### AI description (iteration 2) #### PR Classification Bug fix addressing atomic operation usage and ARM build configuration. #### PR Summary This pull request corrects the implementation of atomic flag exchanges in the Vmbus driver and updates ARM compilation settings. - `MsvmPkg/VmbusDxe/VmbusRoot.c`: Changed the flag pointer type from UINT64* to INT64* and replaced the use of InterlockedCompareExchange64 with _InterlockedExchange64 for proper atomic operations. - `MsvmPkg/MsvmPkgAARCH64.dsc`: Updated the compiler flags by adding `/d2overrideInterlockedIntrinsArm64-` to override inline interlocked sequences on ARM. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #57987809, #60275790
…tor.asm. So that CpuAsm.nasm can be unmodified from upstream.
…arrierWithoutFence. Renamed MemoryBarrierWithoutFence to CompilerBarrier. We might consider also calling it CompilerReadWriteBarrier. There's no great answer here, as it is officially deprecated, does not do what people think, and has a portable replacement, that tries to have a clearer meaning, but does not work on Msvc. (I'm not sure the meaning is clearer. I had to look deeper. But ultimately, stdatomic provides same functionality, with portable standard names..except on Msvc, that does not work.) Moving it to assembly is probably a good idea, to strengthen it. Or rather, both _ReadWriteBarrier, and call AsmCompilerBarrier, asm being, really really, compiler cannot see through it. However that is not hte current semantic. This PR is "cleanup and port", not "strengthen". I'll PR "strengthen" separately, later, maybe. Related work items: #57987809, #60275790
Types `long` and `unsigned long` should be avoided when either specific size or pointer-sized is needed. And then arguably avoid others such as long long. The best types are in `stdint.h`. Second past are the NT/EFI types. There are few places to use long, mainly with the Msvc intrinsics. This also fixes the two definitions of `NTSTATUS` to be the same. Later we will single instance them. ---- This pull request is a code cleanup that enhances portability by updating legacy integer type definitions to fixed-width types. The changes standardize integer types across the codebase to improve consistency and support clang cleanup efforts. - `MsvmPkg/Include/Private/EfiNt.h`: Replaces legacy type definitions with fixed-width types from <stdint.h>, updates include directives, and redefines NTSTATUS. - `MsvmPkg/Sec/X64/SecP.h`: Changes function return types from "long long" to "int64_t" for better type consistency. - `MsvmPkg/Include/Vmbus/NtStatus.h`: Updates NTSTATUS definition to use int32_t. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #60275790
… over-read. In this case the warning points out where the code really does not do what the author intended, including an array over-read. Related work items: #60275790
This was interesting. Back when I worked for a few weeks with EFI on Xbox, I removed all EFIAPI. Surely nobody would again make the NT/x86 mistake of having multiple calling conventions. I recall at the time, gcc for EFI was, like, Cygwin gcc. But now I see, gcc for EFI on Linux/amd64, is just regular gcc, targeting SysV with `-DEFIAPI=attribute((ms))` (not the exact spelling) on the command line. Therefore keeping the default calling convention but selectively changing it only as required by EFI. As long as all functions and function pointers are declared correctly, no problem (same as NT/x86). This leads to errors when you mix them up -- function pointer parameter (callback) with function with different calling convention. Clang does not have this problem, as it uses a different target instead, therefore the default calling convention matches EFIAPI. Msvc of course does not have this problem. We need to then be mindful to declare all assembly functions correctly, i.e. generally with EFIAPI, assuming they all work with Msvc and Clang, and none were interfacing only with gcc. Gcc is really a wildcard. This could be Cygwin/Msys gcc also, with a different default.
This is only to change the comment character from semicolon to pound, as the rest of the file is already doing. We will soon delete this file and use .nasm instead. Related work items: #57987809
This is a _little_ heavy handed, but, really, the extra size is very little, like maybe 12K per PE. If you figure maybe 6 sections: text data rdata reloc rsrc pdata, which really could be just text data rdata (the rest can be merged into rdata, depending on discardability), and on average 2K lost per section. And then even if you do not have execute-in-place, loading is just copying, which is super nice. This is how probably all other load formats work, i.e. Linux and Mac, where kernel doesn't participate so much in user loading (yeah I know, there's no kernel or user here). Thanks to @<Oliver Smith-Denny> for the answers here, seeing the module in the error and knowing where we can put compiler/linker flags, and knowing why we have the redundant 4K. Error fixed by this was: ``` stuart_build -c MsvmPkg\PlatformBuild.py BLD_*_LEGACY_DEBUGGER=1 TARGET=RELEASE TOOL_CHAIN_TAG=CLANGPDB or, same result: stuart_build -c MsvmPkg\PlatformBuild.py TOOL_CHAIN_TAG=CLANGPDB ERROR - Compiler #7000 from : Failed to generate FV ERROR - Compiler #7000 from : Failed to execute command INFO - GenFv: ERROR 3000: Invalid INFO - PE image Section-Alignment and File-Alignment do not match : Q:\src\hyperv.uefi\Build\MsvmX64\RELEASE_CLANGPDB\FV\Ffs\52C05B14-0B98-496c-BC3B-04B50211D680PeiCore\52C05B14-0B98-496c-BC3B-04B50211D680.ffs. INFO - GenFv: ERROR 3000: Invalid INFO - Could not rebase Q:\src\hyperv.uefi\Build\MsvmX64\RELEASE_CLANGPDB\FV\Ffs\52C05B14-0B98-496c-BC3B-04B50211D680PeiCore\52C05B14-0B98-496c-BC3B-04B50211D680.ffs. ``` And, look closely, for "PeiCore" in there, then `dir /s/b PeiCore.dll PeiCore.efi` and `link /dump /headers`, but still, I didn't see the module name buried, and was not hopeful I'd figure out where to put the linker flags, and didn't know why Msft worked and Clang did not -- all addressed by Oliver and obvious once you see it. For reference, in os.2020, you'd either change makefile.def (seriously, this is the right setting, for everyone), or: `LINKER_FLAGS=$(LINKER_FLAGS) ...` MSBuild users can enquire, as SQL and SQLPAL also do this globally. Related work items: #54710354, #57987809
…tant.
Gcc rejects 4 character constants.
Use `SIGNATURE_32` like the rest of UEFI.
Tested like this:
```
C:\s>type 1.c
int a = 'GBDK';
int d = SIGNATURE_32('G','B','D','K');
int e = SIGNATURE_32('K','D','B','G');
C:\s>cl /c /FAsc 1.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.43.34810 for x64
C:\s>more 1.cod
a DD 04742444bH
d DD 04b444247H
e DD 04742444bH
```
Thus showing which form is equivalent.
I pondered if "DataBlock" was in there, 'DB' that is not likely it, it is "Kernel DeBuGGer".
Related work items: #60275790
Related work items: #57987809
Gcc warns about every pragma warning. Clang silently ignores them, but there is an area (not in this PR) where it sorta matters. Related work items: #57987809
#### AI description (iteration 1) #### PR Classification This PR makes an API change by removing the EFIAPI calling convention from a function pointer typedef to ensure consistent behavior with GCC. #### PR Summary The pull request removes the EFIAPI modifier from the FILTER_ROUTINE typedef in `MsvmPkg/Library/DeviceBootManagerLib/DeviceBootManagerLib.c`, aligning the calling convention with GCC requirements and supporting virtual UEFI operation at the intended privilege levels. - `MsvmPkg/Library/DeviceBootManagerLib/DeviceBootManagerLib.c`: Removed EFIAPI from the FILTER_ROUTINE typedef declaration. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #41399366, #57987809
Related work items: #57987809
…ded. Related work items: #60275790
The code is attempting to use space at the end of buffer as scratch, and sets it to 0xff. Or rather, it computes size it might want to use, but then uses a different size and ignores the computed size. This PR does not "fix", does not make it do what it might intend. It keeps the existing behavior and throws out the computation of other data, that it does not really use. Gcc warns about essentially unused locals. This is separated from other cases, because those probably did what the author meant, and this one probably does not. Related work items: #57987809
…ure. Related work items: #57987809
… not needed." This reverts commit 3ba9392041a0412fe0e84def09e284491cf53e02. It seems unnecessarily risky. While is true, the barrier is redundant given its stated purpose, the barrier does more than is stated. And it is both _extremely_ cheap, and costly in risk if we need it but don't do it. "extremely cheap": it impedes compiler optimization, very little. Msvc is very conservative, and assume aliasing everywhere, so quite likely the barrier never did anything for Msvc. But clang/gcc are less conservative -- we should probably build with whatever is the flag that makes them like Msvc, really. There's too much legacy code and thinking and too little stress testing, and too little to gain. `-fno-strict-aliasing` I think it is. We should ponder this, seriously, unless upstream already does it, which will be clear in the logs. Related work items: #57987809
This is small debt I recently knowingly introduced, in the interest of fixing a possible bug, in all Msvc builds. Related work items: #57987809
Gcc/Clang: STATIC_ASSERT repair and cleanup. - Repair: Earlier I missed some of the single parameter STATIC_ASSERTs. - Cleanup: Introduce STATIC_ASSERT_1 that repeats its first parameter, switch STATIC_ASSERT with second empty parameter and C_ASSERT to it. Tested at least with one assert and one compiler, where before they did not show any message and after they nicely rendered the expression. Imho they should be showing the expression, but I guess that is what the message is for. Low tech and terse: STATIC_ASSERT_1, '1' means one parameter. Alternatives, not taken but not clearly worse or better: STATIC_ASSERT one parameter STATIC_ASSERT_MSG with second parameter, message. STATIC_ASSERT_MESSAGE spelled out name STATIC_ASSERT_2 for two parameters, perhaps the rare case STATIC_ASSERT variadic implemented in the compiler maybe in newer standard. In an alternate universe you can overload C preprocessor macros _easily_ on number of parameters. Actually, I think you already can, but not easily, via dispatching within an a variadic one. We can come back to this later, except, `STATIC_ASSERT` is taken by upstream. We'd have to work with them or make a new name. ---- This PR focuses on code cleanup and build fix by replacing and standardizing static assertion macros for improved Clang/GCC compatibility. The changes replace calls to the old STATIC_ASSERT (and C_ASSERT) macros with the new STATIC_ASSERT_1 macro across multiple modules to address Clang build issues for hyperv.uefi AARCH64. - `MsvmPkg/PlatformPei/Hv.c`, `StorvscDxe/VstorageProtocol.h`, and related files now use STATIC_ASSERT_1 for type and size checks. - Assertion macros have been updated in `kdnetinterface.h`, `SmbiosPlatform.c`, and `ChannelMessages.h` to ensure consistent static assertions. - A new header file, `StaticAssert1.h`, is introduced to define the STATIC_ASSERT_1 macro for unified usage. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #57987809
Gcc is even pickier than Clang, at least with whatever flags we are using.
```
C:\s>type 1.c
typedef struct { int ProcessorSteppingId : 4; } PROCESSOR_SIGNATURE;
typedef struct { int ProcessorFpu : 1; } PROCESSOR_FEATURE_FLAGS;
typedef struct {
PROCESSOR_SIGNATURE Signature;
PROCESSOR_FEATURE_FLAGS FeatureFlags;
} PROCESSOR_ID_DATA;
typedef struct {
int i;
PROCESSOR_ID_DATA ProcessorId;
} SMBIOS_TABLE_TYPE4;
struct { SMBIOS_TABLE_TYPE4 Formatted; } clangWarn = {0, 0};
struct { SMBIOS_TABLE_TYPE4 Formatted; } gccWarn = {{0, {0}}};
struct { SMBIOS_TABLE_TYPE4 Formatted; } noWarn = {{0, {{0}}}};
C:\s>cl /c /Wall 1.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.43.34810 for x64
1.c
C:\s>"C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\Llvm\x64\bin\clang.exe" -c 1.c -Wall
1.c:14:60: warning: suggest braces around initialization of subobject [-Wmissing-braces]
14 | struct { SMBIOS_TABLE_TYPE4 Formatted; } clangWarn = {0, 0};
| ^
| {}
1.c:14:60: warning: suggest braces around initialization of subobject [-Wmissing-braces]
14 | struct { SMBIOS_TABLE_TYPE4 Formatted; } clangWarn = {0, 0};
| ^
| {}
2 warnings generated.
$ gcc -c -Wall 1.c
1.c:14:56: warning: missing braces around initializer [-Wmissing-braces]
14 | struct { SMBIOS_TABLE_TYPE4 Formatted; } clangWarn = {0, 0};
| ^
| { -
| {{0}}}
1.c:15:56: warning: missing braces around initializer [-Wmissing-braces]
15 | struct { SMBIOS_TABLE_TYPE4 Formatted; } gccWarn = {{0, {0}}};
| ^
| {}
1.c:15:56: warning: missing braces around initializer [-Wmissing-braces]
15 | struct { SMBIOS_TABLE_TYPE4 Formatted; } gccWarn = {{0, {0}}};
| ^
| {}
```
Related work items: #57987809
It is useful to search for "===" to find merge conflicts. Not to find comments. Nor to find DebugPrint. In this PR, change the comments to "---". Leave DebugPrint alone. ---- #### AI description (iteration 1) #### PR Classification This pull request is a code cleanup that removes redundant apparent merge markers from comment blocks. #### PR Summary The changes standardize comment divider styles across multiple files to improve clarity and maintain code consistency. - In `MsvmPkg/Include/Hv/HvGuestCpuid.h`, replaced "====" markers with "----" for clarity. - In `MsvmPkg/AziHsmDxe/AziHsmBKS3.c`, updated internal TPM comments to use "----" markers. - In `MsvmPkg/Include/Guid/FrontPageEventDataStruct.h`, changed "====" dividers to "----" for a cleaner look. - In `MsvmPkg/Library/MsPlatBdsLib/BdsPlatform.c`, revised comment delimitation from "====" to "----". <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #60275790
``` lld-link: error: duplicate symbol: gEventIndexDiscarded >>> defined at Q:\src\hyperv.uefi\MsvmPkg\VmbfsDxe\VmbfsDxe.c >>> VmbfsDxe.lib(VmbfsDxe.obj) >>> defined at Q:\src\hyperv.uefi\MsvmPkg\VmbfsDxe\VmbfsFile.c >>> VmbfsDxe.lib(VmbfsFile.obj) ld-link: warning: /align specified without /driver; image may not run lld-link: error: duplicate symbol: mInternalEventServices >>> defined at Q:\src\hyperv.uefi\MsvmPkg\Library\EmclLib\EmclLib.c >>> EmclLib.lib(EmclLib.obj) >>> defined at Q:\src\hyperv.uefi\MsvmPkg\NetvscDxe\NetvscDxe.c >>> NetvscDxe.lib(NetvscDxe.obj) ``` etc. One thing may be controversial is I used `__declspec(selectany)` instead of perhaps `static`. This is probably more like the original, and more efficient. There's only of each, any set, gets them "all", instead of getting them multiple times. Oh and it should be in some common header to ensure it always has the same type. Alternatively, we could sprinkle `__declspec (selectany)` all over for these fixes. Alternatively, put these all into a library and remove the non-standard `__declspec (selectany)`. Call is MsGlobals? MsGlobalVariables? I am surprised this did not build with Clang. Related work items: #57987809, #60275790
Rename `__cpuid` to `MsCpuid` and implement it on top of `AsmCpuid`. Related work items: #57987809, #60275790
Gcc tends to warn, if an inline function "maybe" not inlinable. We can fix this by making ALWAYS_INLINE always empty for gcc. Or pragma off the warning. But, in this PR, just..well, I went back and forth. The best approach, really is to move the code out of .h to .c. Let the compiler decide. We do use LTCG. inline is no longer useful in non-template code, assuming single build variant. But that is an unreadable diff. AI does not assist. Maybe do it anyway. It has the advantage of being C89. Another approach is to #if the inline code and include it only once, without inline. In this PR, use "statiic inline" in .h, and nothing in .c. Related work items: #57987809
Use ZeroMem instead.
```
#define INIT {...}
typedef struct A { ... } A;
A a = (A)INIT;
```
is nice, but kinda not allowed in EFI.
Instead:
```
#define INIT {...}
typedef struct A { ... } A;
static A s = INIT;
A a;
CopyMem(&a, &s, sizeof(a));
```
or `ASSIGN_STRUCT(&a, &s)`
In this case, if we instantiate more variations, we could avoid the copy.
It requires a bit of a trick though, with designated initializers, in order to not break the `INIT` abstraction.
Like this:
```
typedef struct WrapA { A a; } WrapA;
static WrapA s {.a = INIT, .a.foo = 1};
```
The copying in this case is pretty small. I think it is OK.
Related work items: #57987809
…emset. Compilers are prone to output calls to memset and memcpy. Gcc is a little more prone, but Clang is a "problem" too. (Not clearly a problem. Compiler writers are good candidates for writing these functions.) Therefore they have been provided. Use them. It is easy to eliminate most of the calls, but difficult to eliminate them all. This presents the dilema then to eliminate some or not. In some cases, the copy or set is only made explicit, not removed. And other times I noticed excessive copying, easiiy removed. Given that presently upstream AMD64 is broken, and upstream AMD64 community does not like using this library by default, the present approach is a hybrid. Eliminate what we can. Use the library when we can. Hope that covers everything. Which it does seem to. Also submit upstream patches. Related work items: #57987809
…KS3.c). `char big[99]= "small"` is `memcpy` and/or `memset`. In this case, the copy is not needed. (In other cases we often add CopyMem/ZeroMem calls as part of the fix). Related work items: #57987809
Related work items: #57987809
…dLogger During SEC phase, the Advanced Logger writes a small pointer structure to the address specified by this PCD. The original address (0xFA000000) sits in the MMIO gap, which is not backed by real RAM. On normal VMs this works because the hypervisor/paravisor maps the access. On hardware-isolated VMs without a paravisor (TDX or SNP "NoManagementVtl"), guest memory pages must be explicitly accepted before use, and SEC has no ability to do that. Writing to an unaccepted address causes a fatal fault (#VE on TDX, #VC on SNP) that SEC cannot handle, crashing the VM at boot. Setting this to 0 makes the logger skip the write in SEC. PEI Core detects the 0 and allocates its own log buffer from accepted RAM, so logging still works from PEI onward. NOTE: I still need to test this on a regular test machine to ensure EfiDiagnostics is not broken ---- #### AI description (iteration 1) #### PR Classification Bug fix aimed at preventing SEC phase memory faults on NoManagementVtl CVMs. #### PR Summary This pull request disables the SEC pre-memory logger by setting `PcdAdvancedLoggerBase` to 0, ensuring that logging skips the problematic MMIO region during SEC and uses accepted RAM in PEI. - `MsvmPkg/MsvmPkgX64.dsc`: Updated the Advanced Logger configuration by changing `PcdAdvancedLoggerBase` from 0xFA000000 to 0 with detailed comments explaining the rationale. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #53782343
Related work items: #57987809
Light handed alternative to !15020076
We forked around 2016 or earlier.
In 2019 and 2020 mErrorCodeFlag got two more bits.
Gcc/clang fail to link due to the duplicate symbols.
Remove ours in C and link to upstream/fork.
Update ours in assembly.
Here is the history of the two bits:
Ours:
```
commit ee188cdbb20c13f2361192fa1f07192eea8835ef
Author: Larry Cleeton <lcleeton@ntdev.microsoft.com>
Date: Wed Jan 13 12:50:27 2016 -0800
checkpoint - tries to load a boot loader!
```
Upstream:
```
commit 5277540e37e88a1a69f9517c4ad895051b4b3ed3
Author: Tom Lendacky <thomas.lendacky@amd.com>
Date: Wed Aug 12 15:21:36 2020 -0500
UefiCpuPkg/CpuExceptionHandler: Add base support for the #VC exception
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
Add base support to handle #VC exceptions. Update the common exception
-CONST UINT32 mErrorCodeFlag = 0x00227d00;
+CONST UINT32 mErrorCodeFlag = 0x20227d00;
+ "Reserved",
+ "Reserved",
+ "Reserved",
+ "Reserved",
+ "Reserved",
+ "Reserved",
+ "Reserved",
+ "#VC - VMM Communication"
```
```
commit 0d25074cbcc272532d6ca5a47974ac5b31f4b6ec
Author: Jiewen Yao <jiewen.yao@intel.com>
Date: Fri Feb 22 21:30:35 2019 +0800
UefiCpuPkg/ExceptionLib: Add CET support.
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1521
Add information dump for Control Protection exception.
-CONST UINT32 mErrorCodeFlag = 0x00027d00;
+CONST UINT32 mErrorCodeFlag = 0x00227d00;
- "#VE - Virtualization"
+ "#VE - Virtualization",
+ "#CP - Control Protection"
};
```
Related work items: #57987809
…chitecture tag. Related work items: #57987809
- Gcc LTO checks function signatures. Fix them. - Add/remove stubs. Related work items: #57987809
Make the lld-link flags not the gcc flags. Set gcc flags. Note that gcc flags are sometimes filealign:64K, rendering it, not great. We should use windows targeting/cygwin/mingwin tools instead, so that there is no ELF=>PE conversion, and sectioAlign and filAalign can be unequal. Related work items: #57987809
Upcoming module update breaks this code. Make it more resililent. Remove 33 lines, add 11, no loss of meaning or clarity, except we no longer list all the zeroed member data. Related work items: #57987809
Also for Msvc in MsKdDebugPkg. Related work items: #57987809
…vive submodule update. We use MU CpuDebugAssert. Upcoming submodule update removes it. Replace for now with NtAssert. Neither really work as intended, but they keep us going. As debuggers get replaced this might go away. Related work items: #57987809
Related work items: #54710354
- Standard. - Shorter. - I saw other PRs doing it. - A few cases deliberately deferred, since they will not compile (still using __FUNCTION__ as concatenable string literal, instead of non-concatenable char array; stuff not in clang build?) ---- This pull request is a code cleanup that standardizes debug logging by replacing all instances of `__FUNCTION__` with `__func__`. The pull request updates the logging macros across the codebase to use `__func__`, ensuring more consistent and portable debug messages without changing the underlying functionality. - `MsvmPkg/AcpiPlatformDxe/Dsdt.c`, `Tcg2PreInitLibPei.c`, and similar files now use `__func__` in their DEBUG statements. - Modules under `AziHsmDxe` have been modified to uniformly report errors and status using `__func__`. - `VmbusDxe` and `VmbusRoot.c` files have updated logging for memory operations and protocol communications. - Various libraries (e.g., `MsvmRngLib`, `Tpm2DeviceLib`) and unit test files also reflect the change for improved log consistency. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #57987809
Add Gcc and Clang builds. Clang on Windows and Linux. Gcc only on Linux, but could also be done on Windows. Gcc debug builds skipped because, ELF to COFF conversion yields large images, and there is an upstream calling convention nismatch to fix. Matrix build is replaced with each over parameter, because matrix build cannot change pool type, without more harder to read diff involving moving the code into a new template. Templates cannot be ranges of file (not like a C++ function). The PR programming language is PR-hostile. ---- Pipeline update to support cross-platform builds with Linux, Clang, and GCC. This PR refactors the build pipeline configuration to enable dynamic job creation for multiple toolchain and platform combinations—as required by the AARCH64 Clang build work item. - **`MsvmPkg/OneBranch/Build-And-Publish.yaml`**: Introduces a new `Builds` parameter that defines build configurations for Windows (VS2022, CLANGPDB) and Linux (Clang, GCC) targets. - **`MsvmPkg/OneBranch/Build-And-Publish.yaml`**: Replaces the traditional fixed matrix with an iterative `each` loop to dynamically generate jobs based on the new build configuration objects. - **`MsvmPkg/OneBranch/Build-And-Publish.yaml`**: Implements conditional platform-specific steps (e.g., apt-get setup for Linux and selective symbol publishing) to ensure appropriate execution per toolchain. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #57987809
Fix debug build for AziHsm
Windows ES builds and supports a clang toolset. Add that to our build matrix. Remove most of the quoting. Use verbose instead of dumping logs after. I kept the VisualStudio2022 clang toolset for now. I believe it goes away in VS2026, so we'll remove it here then eventually too. Related work items: #57987809
Edit comments. Related work items: #57987809
This PR fixes what was missed in this PR: https://dev.azure.com/microsoft/OS/_git/hyperv.uefi/pullrequest/10689820?path=/MsvmPkg/MsvmPkgX64.fdf&version=GBofficial/main&line=33&lineEnd=34&lineStartColumn=1&lineEndColumn=1&type=2&lineStyle=plain&_a=files&iteration=11&base=0 That is, while we split the DXE FV apart from Main FV, we forgot to skip measuring it. This results in 38 new measurements going to PCR2. This PR adds three things: - Ensure that ExcludeFvsFromMeasurementLib includes DXE FV. But again, by default we don't want to exclude measuring this. We want to make sure MainFV and DxeFV are in PCR0, so that we don't get any drivers from _our_ platform into PCR2 - Issue a `PeiServicesInstallFvInfoPpi` to make `Tcg2Pei` aware of the DXE FV. This was the key thing missing - Add the SHA384 library to ARM64 + remove SHA1 - Rename the Exclude PCD Attached are raw boot measurements for arm64 and amd64 to prove that this fix works: [raw-boot-measurement-amd64.txt](https://microsoft.visualstudio.com/8d47e068-03c8-4cdc-aa9b-fc6929290322/_apis/git/repositories/1ad2e189-11bd-479f-8f25-a739cc641dfa/pullRequests/15013511/attachments/raw-boot-measurement-amd64.txt) [raw-boot-measurement-arm64.txt](https://microsoft.visualstudio.com/8d47e068-03c8-4cdc-aa9b-fc6929290322/_apis/git/repositories/1ad2e189-11bd-479f-8f25-a739cc641dfa/pullRequests/15013511/attachments/raw-boot-measurement-arm64.txt) Related work items: #61409673
See microsoft/mu_basecore#1712 Related work items: #57987809
This PR adds the AIP protocol to SnpDxe. Previously, we took a 202502 submodule update, which modifies `DxeNetLib`'s `NetLibDetectMediaWaitTimeout()` function to poll instead of returning EFI_NO_MEDIA when the AIP protocol is absent. This causes Gen2 VMs with an unplugged NIC to hang in boot for a while. By implementing the AIP protocol, whenever our NIC is unplugged, we can immediately reflect `MediaState = EFI_NO_MEDIA` to achieve what upstream code used to do. ---- #### AI description (iteration 1) #### PR Classification This PR introduces a new feature by adding support for the Adapter Information Protocol (AIP) to the SnpDxe driver, enhancing media state reporting to address a UEFI regression. #### PR Summary The changes integrate the AIP protocol into the NetvscDxe driver to quickly assess network media status and streamline protocol management. - **`MsvmPkg/NetvscDxe/AdapterInformation.c`**: New file implementing AIP functions (GetInformation, SetInformation, GetSupportedTypes) for media state reporting. - **`MsvmPkg/NetvscDxe/Snp.h`**: Added AIP function declarations and a macro to retrieve the SNP driver from the AIP instance. - **`MsvmPkg/NetvscDxe/Snp.c`**: Modified to install/uninstall the AIP protocol alongside the SNP protocol and initialize its handlers. - **`MsvmPkg/NetvscDxe/NetvscDxe.inf`**: Updated to produce the AIP protocol and include the new file. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #61421898
It is an old optimization, that we never use. Remove it. Related work items: #57987809
It used by a lot of the Masm, and derivation might be used by Nasm. You really cannot write decent ABI-following code with Masm without it. Though the ABI in UEFI is a little unclear wrt unwind, and Nasm does not really "work". Related work items: #57987809
Convert macamd64.inc to macamd64.nasm to reduce porting work. ---- #### AI description (iteration 4) #### PR Classification This pull request adds a new feature by introducing a NASM portability layer to support Clang builds. #### PR Summary The changes introduce a comprehensive assembly macro file that provides AMD64 macros to ease the transition from MASM to NASM, aligning with the Clang build requirements for hyperv.uefi AARCH64. - `MsvmPkg/Include/macamd64.nasm`: Newly added file defining macros for register pushes, stack allocation, register and xmm saving, and function prologue/epilogue management. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot --> Related work items: #57987809
- Factor common code to `common.yaml`. ChatGPT said this was impossible. Claude was not sure.
It is imho very unfortunate this has to be another separate file, but this the best I could do. It appears the upstream pipelines must be extended at the stages/jobs level, not at toplevel. This yet another programming language is suspiciously inadequate.
- Tune `apt-get install`, i.e. only install what is needed, per-configuration.
- "Rewrite" vpack parameterization.
- PR and Salted builds have different vpack names.
- So we only have one per name.
- Gradual removal of quotes, and spaces.
- Reduce gratuitious use of PowerShell and unnecessary parameters.
- Copy logs to artifacts just once. The first copy does not show up.
- Add echoing of multi-line scripts (single line scripts already echo by default).
Confirmed no secrets occur. This is useful for debugging/development, because ADO has variables and parameters, and the difference is not at all clear, despite repeated documentation trying to make them clear. We should probably overwhelmingly favor parameters, because the "preprocessed" yaml is available to view. This PR does not complete such conversion (i.e. eliminate vars.yml and replace with constant parameters). Later.
- Canonical ADO booleans are `True` and `False`. Use them.
- vpack stuff appears to be almost completely untestable outside of master, except that the official pipeline can be run against PRs, with approval. I will queue that up.
Related work items: #57987809
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.