| Message ID | 20201117065036.519662-1-alex.hung@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mtrr: add tests for enable bits in MTRR_DEF_TYPE MSR | expand |
On 17/11/2020 06:50, Alex Hung wrote: > Signed-off-by: Alex Hung <alex.hung@canonical.com> > --- > src/bios/mtrr/mtrr.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c > index c9b77941..ca1dbc2c 100644 > --- a/src/bios/mtrr/mtrr.c > +++ b/src/bios/mtrr/mtrr.c > @@ -594,6 +594,35 @@ static int mtrr_deinit(fwts_framework *fw) > return FWTS_OK; > } > > +static int mtrr_test0(fwts_framework *fw) > +{ > + uint64_t mtrr_def_msr; Can you add an empty line here before the if statement > + if (fwts_cpu_readmsr(fw, 0, MTRR_DEF_TYPE_MSR, &mtrr_def_msr) != FWTS_OK) { > + fwts_failed(fw, LOG_LEVEL_CRITICAL, > + "MTRRMSRDefaultNotAvailable", > + "MTRR_DEF_TYPE_MSR cannot be read from CPU."); > + return FWTS_OK; is FWTS_OK correct to return when the msr read failed? > + } > + > + if (mtrr_def_msr & 0x800) Can you #define the 0x800 magic value to add more context to what that value is? > + fwts_passed(fw, "MTRRs enabled flag is set in MTRR_DEF_TYPE MSR correctly."); > + else > + fwts_failed(fw, LOG_LEVEL_CRITICAL, > + "MTRRDisabled", > + "MTRRs enabled flag is clear in MTRR_DEF_TYPE MSR and " > + "all MTRRs are disabled "); > + > + if (mtrr_def_msr & 0x400) Same here with the 0x400 value > + fwts_passed(fw, "fixed MTRRs enabled flag is set in MTRR_DEF_TYPE MSR correctly."); > + else > + fwts_failed(fw, LOG_LEVEL_CRITICAL, > + "MTRRFixedRangeDisabled", > + "Fixed MTRRs enabled flag is clear in MTRR_DEF_TYPE MSR and " > + "all Fixed-range MTRRs are disabled "); > + > + return FWTS_OK; > +} > + > static int mtrr_test1(fwts_framework *fw) > { > return validate_iomem(fw); > @@ -658,6 +687,7 @@ static int mtrr_test3(fwts_framework *fw) > } > > static fwts_framework_minor_test mtrr_tests[] = { > + { mtrr_test0, "Validate MTRR default enabled." }, > { mtrr_test1, "Validate the kernel MTRR IOMEM setup." }, > { mtrr_test2, "Validate the MTRR setup across all processors." }, > { mtrr_test3, "Test for AMD MtrrFixDramModEn being cleared by the BIOS." }, >
diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c index c9b77941..ca1dbc2c 100644 --- a/src/bios/mtrr/mtrr.c +++ b/src/bios/mtrr/mtrr.c @@ -594,6 +594,35 @@ static int mtrr_deinit(fwts_framework *fw) return FWTS_OK; } +static int mtrr_test0(fwts_framework *fw) +{ + uint64_t mtrr_def_msr; + if (fwts_cpu_readmsr(fw, 0, MTRR_DEF_TYPE_MSR, &mtrr_def_msr) != FWTS_OK) { + fwts_failed(fw, LOG_LEVEL_CRITICAL, + "MTRRMSRDefaultNotAvailable", + "MTRR_DEF_TYPE_MSR cannot be read from CPU."); + return FWTS_OK; + } + + if (mtrr_def_msr & 0x800) + fwts_passed(fw, "MTRRs enabled flag is set in MTRR_DEF_TYPE MSR correctly."); + else + fwts_failed(fw, LOG_LEVEL_CRITICAL, + "MTRRDisabled", + "MTRRs enabled flag is clear in MTRR_DEF_TYPE MSR and " + "all MTRRs are disabled "); + + if (mtrr_def_msr & 0x400) + fwts_passed(fw, "fixed MTRRs enabled flag is set in MTRR_DEF_TYPE MSR correctly."); + else + fwts_failed(fw, LOG_LEVEL_CRITICAL, + "MTRRFixedRangeDisabled", + "Fixed MTRRs enabled flag is clear in MTRR_DEF_TYPE MSR and " + "all Fixed-range MTRRs are disabled "); + + return FWTS_OK; +} + static int mtrr_test1(fwts_framework *fw) { return validate_iomem(fw); @@ -658,6 +687,7 @@ static int mtrr_test3(fwts_framework *fw) } static fwts_framework_minor_test mtrr_tests[] = { + { mtrr_test0, "Validate MTRR default enabled." }, { mtrr_test1, "Validate the kernel MTRR IOMEM setup." }, { mtrr_test2, "Validate the MTRR setup across all processors." }, { mtrr_test3, "Test for AMD MtrrFixDramModEn being cleared by the BIOS." },
Signed-off-by: Alex Hung <alex.hung@canonical.com> --- src/bios/mtrr/mtrr.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)