Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert Downgrading Page.c Error Prints #1095

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

os-d
Copy link
Contributor

@os-d os-d commented Aug 7, 2024

Description

This reverts commit 76f8060.

This commit was from an earlier era of Project Mu where we were not as strict with memory protections. Now that we are more strict, these are important prints (and failures if alignment of a RUNTIME entry is wrong). This was confirmed by booting on Q35 and analyzing the few prints that showed up.

The first was that Rust code was not being linked as we expected, that is fixed here for pure Rust repos: microsoft/mu_devops#356 and here for mu_basecore based repos: #1098. This may get fixed in a different way that is more broad. The in-depth issue was that /BASE:0 was not getting set for the Rust linker. It is failing to allocate pages when loading the image. The reason is that the ImageBase field in the PE/COFF optional header is set, so we try to load at that address:

if ((PcdGetBool (PcdImageLargeAddressLoad) && ((Image->ImageContext.ImageAddress) >= 0x100000)) ||
Image->ImageContext.RelocationsStripped)
{
Status = CoreAllocatePages (
AllocateAddress,
(EFI_MEMORY_TYPE)(Image->ImageContext.ImageCodeMemoryType),
Image->NumberOfPages,
&Image->ImageContext.ImageAddress
);
}
if (EFI_ERROR (Status) && !Image->ImageContext.RelocationsStripped) {
Status = CoreAllocatePages (
AllocateAnyPages,
(EFI_MEMORY_TYPE)(Image->ImageContext.ImageCodeMemoryType),
Image->NumberOfPages,
&Image->ImageContext.ImageAddress
);
}
(ImageAddress gets populated from ImageBase:
if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
//
// Use PE32 offset
//
ImageContext->ImageAddress = Hdr.Pe32->OptionalHeader.ImageBase;
} else {
//
// Use PE32+ offset
//
ImageContext->ImageAddress = Hdr.Pe32Plus->OptionalHeader.ImageBase;
}
).

Per the PE/COFF spec, this is only intended for use by Windows and I don't think we should really be honoring it, but probably things are relying on it:

image

UefiHidDxe and HelloWorldRustDxe are the only EFI binaries in the mu_tiano_platforms build that set ImageBase. For everything else it is 0, so we don't enter that codepath (what happens in that codepath is the ImageBase provided, 0x140000000 for both modules, is where we attempt to load the modules to, which isn't in our memory tree (or one of those two is already loaded there) and so we fail).Setting /BASE:0 causes MSVC to set the ImageBase to 0 (in ElfConvert we always set the ImageBase to 0:

NtHdr->Pe32Plus.OptionalHeader.ImageBase = 0;
).

The other set of failures was from gDS->SetMemorySpaceCapabilities() trying to set attributes in gMemoryMap on GCD memory types that don't exist there and an edk2 PR has been put up to fix that: tianocore/edk2#6062.

There are no other failures seen and when these failures do occur, we want to debug them. Closes #1091.

For each item, place an "x" in between [ and ] if true. Example: [x].
(you can also check items in the GitHub UI)

  • Impacts functionality?
    • Functionality - Does the change ultimately impact how firmware functions?
    • Examples: Add a new library, publish a new PPI, update an algorithm, ...
  • Impacts security?
    • Security - Does the change have a direct security impact on an application,
      flow, or firmware?
    • Examples: Crypto algorithm change, buffer overflow fix, parameter
      validation improvement, ...
  • Breaking change?
    • Breaking change - Will anyone consuming this change experience a break
      in build or boot behavior?
    • Examples: Add a new library class, move a module to a different repo, call
      a function in a new library class in a pre-existing module, ...
  • Includes tests?
    • Tests - Does the change include any explicit test code?
    • Examples: Unit tests, integration tests, robot tests, ...
  • Includes documentation?
    • Documentation - Does the change contain explicit documentation additions
      outside direct code modifications (and comments)?
    • Examples: Update readme file, add feature readme file, link to documentation
      on an a separate Web page, ...

How This Was Tested

Tested on Q35.

Integration Instructions

N/A.

@github-actions github-actions bot added the impact:non-functional Does not have a functional impact label Aug 7, 2024
@os-d os-d requested a review from makubacki August 7, 2024 22:04
@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Please upload report for BASE (release/202405@63fc132). Learn more about missing BASE report.

Files Patch % Lines
MdeModulePkg/Core/Dxe/Mem/Page.c 0.00% 5 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##             release/202405    #1095   +/-   ##
=================================================
  Coverage                  ?    0.68%           
=================================================
  Files                     ?      643           
  Lines                     ?   219670           
  Branches                  ?     1356           
=================================================
  Hits                      ?     1502           
  Misses                    ?   218121           
  Partials                  ?       47           
Flag Coverage Δ
MdeModulePkg 0.68% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@os-d os-d requested a review from kuqin12 August 7, 2024 23:41
This reverts commit 76f8060.

This commit was from an earlier era of Project Mu where we were
not as strict with memory protections. Now that we are more strict,
these are important prints (and failures if alignment of a RUNTIME
entry is wrong). This was confirmed by booting on Q35 and analyzing
the few prints that showed up.

The first was that Rust code was not being linked as we expected,
that is fixed here: microsoft/mu_devops#356.
This may get fixed in a different way that is more broad. See the
PR for this commit for more details on the failure seen.

The other set of failures was from gDS->SetMemorySpaceCapabilities()
trying to set attributes in gMemoryMap on GCD memory types that
don't exist there and an edk2 PR has been put up to fix that:
tianocore/edk2#6062.

There are no other failures seen and when these failures do occur,
we want to debug them.
@os-d os-d force-pushed the osde/revert_page_prints branch from a81517e to 9fa1997 Compare August 8, 2024 16:30
@os-d os-d enabled auto-merge (rebase) August 8, 2024 16:31
@os-d os-d merged commit f0075d1 into microsoft:release/202405 Aug 8, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:non-functional Does not have a functional impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants