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

Add new MTRR Test Point test #314

Open
wants to merge 4 commits into
base: dev/202405
Choose a base branch
from
Open

Conversation

kenlautner
Copy link
Contributor

@kenlautner kenlautner commented Dec 31, 2024

Description

Add a new MTRR Test Point test that runs at Ready to Boot. This test checks the systems MTRRs at Ready to Boot versus the expected MTRR settings the platform publishes.

For details on how to complete these options and their meaning refer to CONTRIBUTING.md.

  • Impacts functionality?
  • Impacts security?
  • Breaking change?
  • Includes tests?
  • Includes documentation?
  • Backport to release branch?

How This Was Tested

Tested on Intel physical platforms. MTRRs were correctly read and validated at Ready to Boot.

Integration Instructions

Enable bit 6 in byte 4 of the gMinPlatformPkgTokenSpaceGuid.PcdTestPointIbvPlatformFeature PCD to actually run the test. Additionally, a platform will need to create their version of the function GetPlatformMtrrCacheData. This returns the expected MTRR cache settings will be at ready to boot which will be used for verification.

@github-actions github-actions bot added impact:non-functional Does not have a functional impact impact:testing Affects testing type:backport Backport changes in a dev branch PR to its release branch. labels Dec 31, 2024
Comment on lines +34 to +37
UINTN
GetPlatformMtrrCacheData (
OUT VARIABLE_MTRR_INFO **CheckedMtrrs
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
UINTN
GetPlatformMtrrCacheData (
OUT VARIABLE_MTRR_INFO **CheckedMtrrs
);
UINTN
EFIAPI
GetPlatformMtrrCacheData (
OUT VARIABLE_MTRR_INFO **CheckedMtrrs
);

Comment on lines +14 to +15
#ifndef _TEST_POINT_MTRR_INFO_LIB_H_
#define _TEST_POINT_MTRR_INFO_LIB_H_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#ifndef _TEST_POINT_MTRR_INFO_LIB_H_
#define _TEST_POINT_MTRR_INFO_LIB_H_
#ifndef TEST_POINT_MTRR_INFO_LIB_H_
#define TEST_POINT_MTRR_INFO_LIB_H_

Comment on lines +4 to +7
An interface for platforms to define the expected MTRR cache types for specific
regions by Ready To Boot. Create an array of VARIABLE_MTRR_INFO structures for every
MTRR range that you want to validate. If any of the checked regions don't have the
matching caching type the test will report an error for the failing range and return.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the library interface should be tied to "Ready to Boot" as a whole.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend leaving this generic and let individual functions handle the boot point details.


**/
UINTN
GetPlatformMtrrCacheData (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this interface should be scoped to the point the MTRR settings are valid. Either a boot point parameter or separate function for each boot point.

Comment on lines +4 to +5
# Null implementation of TestPointMtrrInfoLib.h that doesn't return any MTRRs that we want to check.
# This is for compatibility purposes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Null implementation of TestPointMtrrInfoLib.h that doesn't return any MTRRs that we want to check.
# This is for compatibility purposes.
# Null implementation of the TestPointMtrrInfoLib class that doesn't return any MTRRs that we want to check.
# This is for compatibility purposes.

@@ -0,0 +1,411 @@
/** @file

Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is authored by Intel as BSD-2-Clause-Patent but not in edk2-platforms yet?

**/

#include <Uefi.h>
#include <PiPei.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PiPei.h should not be included in a file targeting DXE.

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 impact:testing Affects testing type:backport Backport changes in a dev branch PR to its release branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants