Add new MTRR Test Point test #314
Conversation
github-actions
bot
added
impact:non-functional
| 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. |
There was a problem hiding this comment.
I don't think the library interface should be tied to "Ready to Boot" as a whole.
There was a problem hiding this comment.
I recommend leaving this generic and let individual functions handle the boot point details.
| **/ | ||
| UINTN | ||
| GetPlatformMtrrCacheData ( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I updated the function to take an enum variable specifying the boot point.
MinPlatformPkg/Library/TestPointMtrrInfoLibNull/TestPointMtrrInfoLibNull.inf
Outdated
Show resolved
Hide resolved
| @param[out] CheckedMtrrs Pointer to the head of an array of VARIABLE_MTRR_INFO structures. | ||
| The caller shall not free this array. | ||
There was a problem hiding this comment.
Please add the Boot parameter here now that it's added to the function.
There was a problem hiding this comment.
This file has a lot of overlap with MinPlatformPkg/Test/Library/TestPointCheckLib/PeiCheckMtrr.c. Have you looked at moving common functionality to a shared .c file with PeiTestPointCheckLib and DxeTestPointCheckLib?
There was a problem hiding this comment.
Good idea. I separated the common functions into TestPointCheckMtrr.c and TestPointCheckMtrr.h.
There was a problem hiding this comment.
In general, the changes should be marked as Mu changes unless they're being cherry-picked from edk2-platforms. I think if the MTRR test point code is consolidated where possible between PEI and DXE, it would be a nice contribution to edk2-platforms.
There was a problem hiding this comment.
I added MU_CHANGE to the previously existing files but not to the new files.
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.
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.PcdTestPointIbvPlatformFeaturePCD to actually run the test. Additionally, a platform will need to create their version of the functionGetPlatformMtrrCacheData. This returns the expected MTRR cache settings will be at ready to boot which will be used for verification.