-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[PATCH 0/4] OvmfPkg/PlatformPei: rewrite MemTypeInfo HOB production logic #629
Merged
Conversation
This file contains 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
In commit d42fdd6 ("OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation", 2020-03-12), we enabled the boot-to-boot tracking of the usages of various UEFI memory types. Both whitepapers listed in that commit recommend that BS Code/Data type memory *not* be tracked. This recommendation was confirmed by Jiewen in the following two messages as well: [1] https://edk2.groups.io/g/devel/message/55741 http://mid.mail-archive.com/74D8A39837DF1E4DA445A8C0B3885C503F97B579@shsmsx102.ccr.corp.intel.com [2] https://edk2.groups.io/g/devel/message/55749 http://mid.mail-archive.com/74D8A39837DF1E4DA445A8C0B3885C503F97BDC5@shsmsx102.ccr.corp.intel.com While tracking BS Code/Data type memory has one benefit (it de-fragments the UEFI memory map), the downsides outweigh it. Spikes in BS Data type memory usage are not uncommon in particular, and they may have the following consequences: - such reboots during normal boot that look "spurious" to the end user, and have no SMM security benefit, - a large BS Data record in MemoryTypeInformation may cause issues when the DXE Core tries to prime the according bin(s), but the system's RAM size has been reduced meanwhile. Removing the BS Code/Data entries from MemoryTypeInformation leads to a bit more fragmentation in the UEFI memory map, but that should be harmless. Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2706 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200508121651.16045-2-lersek@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
The previous patch has no effect -- i.e., it cannot stop the tracking of BS Code/Data in MemTypeInfo -- if the virtual machine already has a MemoryTypeInformation UEFI variable. In that case, our current logic allows the DXE IPL PEIM to translate the UEFI variable to the HOB, and that translation is verbatim. If the variable already contains records for BS Code/Data, the issues listed in the previous patch persist for the virtual machine. For this reason, *always* install PlatformPei's own MemTypeInfo HOB. This prevents the DXE IPL PEIM's variable-to-HOB translation. In PlatformPei, consume the records in the MemoryTypeInformation UEFI variable as hints: - Ignore all memory types for which we wouldn't by default install records in the HOB. This hides BS Code/Data from any existent MemoryTypeInformation variable. - For the memory types that our defaults cover, enable the records in the UEFI variable to increase (and *only* to increase) the page counts. This lets the MemoryTypeInformation UEFI variable function as designed, but it eliminates a reboot when such a new OVMF binary is deployed (a) that has higher memory consumption than tracked by the virtual machine's UEFI variable previously, *but* (b) whose defaults also reflect those higher page counts. Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2706 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200508121651.16045-3-lersek@redhat.com> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
Some OvmfPkg modules already depend on "EmbeddedPkg.dec"; thus, replace the open-coded memory type info defaults in the source code with the EmbeddedPkg PCDs that stand for the same purpose. Consequently, platform builders can override these values with the "--pcd" option of "build", without source code updates. While at it, sort the memory type names alphabetically. Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2706 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200508121651.16045-4-lersek@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
Any new OVMF binary (containing commit d42fdd6, and built with SMM_REQUIRE) is likely to reboot during its first boot, regardless of whether the variable store is logically empty, or it contains a MemoryTypeInformation variable from an earlier OVMF binary. This "reboot on first boot after OVMF upgrade" occurs despite having eliminated BS Code/Data tracking in earlier parts of this series. Meaning that we've outgrown the bins of those memory types too that matter for SMM security. Eliminating said reboot will make an upgrade to edk2-stable202005 more comfortable for users. Increase the defaults empirically. (The total doesn't exceed 3MB by much.) Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Philippe Mathieu-Daudé <philmd@redhat.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2706 Signed-off-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <20200508121651.16045-5-lersek@redhat.com> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
https://edk2.groups.io/g/devel/message/58876
http://mid.mail-archive.com/20200508121651.16045-1-lersek@redhat.com
https://bugzilla.tianocore.org/show_bug.cgi?id=2706