|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 2/2] xtf/vpmu: MSR read/write tests for VPMU
On 04/05/17 22:33, Mohit Gambhir wrote:
> This patch tests VPMU functionality in the hypervisor on Intel machines.
> The tests write to all valid bits in the MSRs that get exposed to the guests
> when VPMU is enabled. The tests also write invalid values to the bits
> that should be masked and expect the wrmsr call to fault.
>
> The tests are currently unsupported for AMD machines and PV guests.
>
> Signed-off-by: Mohit Gambhir <mohit.gambhir@xxxxxxxxxx>
> ---
> tests/vpmu/Makefile | 9 ++
> tests/vpmu/main.c | 442
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 451 insertions(+)
> create mode 100644 tests/vpmu/Makefile
> create mode 100644 tests/vpmu/main.c
>
> diff --git a/tests/vpmu/Makefile b/tests/vpmu/Makefile
> new file mode 100644
> index 0000000..d457ab8
> --- /dev/null
> +++ b/tests/vpmu/Makefile
> @@ -0,0 +1,9 @@
> +include $(ROOT)/build/common.mk
> +
> +NAME := vpmu
> +CATEGORY := functional
I have added an in-development category for uses in cases like this.
You will pick it up if you rebase (as well as picking up your first
patch, which I've committed).
> +TEST-ENVS := hvm64
> +
> +obj-perenv += main.o
> +
> +include $(ROOT)/build/gen.mk
> diff --git a/tests/vpmu/main.c b/tests/vpmu/main.c
Thinking more about the eventual usecases here, I think this test would
better as vpmu-intel rather than just vpmu. Were an AMD variant to
eventually appear, it probably wouldn’t share any code, and thus the
test would want to be vpmu-amd instead.
I am less sure about the PV side of things. I know the interface is
hypercall / shared memory based rather than MSR based, but the
underlying mechanism and values are still the same, so I optimistically
hope they can share the same primary codebase as their HVM counterparts.
> new file mode 100644
> index 0000000..c2f6dec
> --- /dev/null
> +++ b/tests/vpmu/main.c
> @@ -0,0 +1,442 @@
> <snip>
> +static void test_valid_msr_write(uint32_t idx, uint64_t wval, uint64_t erval)
> +{
> + uint64_t rval = 0;
What are wval, erval or rval? (I'm trying to make a point, but) you
have a large number of very short, almost identical variable names which
don't convey obvious meanings.
(see below for a suggestion in a context with more explanation)
> +
> + if( wrmsr_safe(idx, wval) )
> + xtf_failure("Fail: wrmsr(0x%08x, 0x%016"PRIx64") got unexpected
> fault"
> + "\n", idx, wval);
For the sake of line length, I wouldn't bother splitting this string
like this. However, (again from context), a block of 8 and 16
characters in a wrmsr() expression are obviously hex, so the 0x prefix
doesn't provide any extra useful information.
I'd recommend simply
"Fail: wrmsr(%08x, %016"PRIx64") got unexpected fault\n"
in this case.
> +
> + /* check to see if the values were written correctly */
/* Check to see if the values were written correctly. */ please.
> + if ( rdmsr_safe(idx, &rval) )
> + xtf_failure("Fail: rdmsr(0x%08x, 0x016%"PRIxPTR") got unexpected
> fault"
> + "\n", idx, (uintptr_t) &rval);
The pointer to rval is not relevant. It will be a compile time constant
and has nothing to do with whether the read succeeds or fails.
"Fail: rdmsr(%08x) got unexpected fault\n" is perfectly fine.
> +
> + /* check if read value is equal to expected value */
> + if ( rval != erval )
If the read failed, this check is illogical, as you are not comparing
erval to anything meaningful. This check needs to be an else clause to
the rdmsr_safe().
In fact, if you had left rval uninitialised, the compiler would have
helped you by pointing out that the comparison used possibly
uninitialised data.
> + xtf_failure("Fail: rdmsr mismatch idx 0x%08x, wval 0x%016"PRIx64
> + ", rval 0x%016"PRIx64"\n", idx, wval, rval);
Naming wval and rval in the text might be meaningful to you as the
author of the test, but it isn't meaningful to anyone else looking at
test logs.
This basic naming scheme I have used elsewhere in XTF is based around
expectations, and what you got. These are mostly context agnostic, but
convey obvious meaning to people reading the code.
Judging by the callers, you appear to be trying to write a specific
value, then confirm that a (possibly different) value gets read back.
If so, I'd suggest using 'val', 'exp_read' and 'got' as your variables,
and this as the text:
"Fail: rdmsr(%08x) expected %016"PRIx64", got %016"PRIx64"\n"
Finally, doing so should highlight that you aren't printing out the
useful information in the case that the read mismatches with the
expectation.
> +}
> +
> +static void test_invalid_msr_write(uint32_t idx, uint64_t wval)
> +{
> + /* wrmsr_safe must return false after faulting */
> + if( !wrmsr_safe(idx, wval) )
> + xtf_failure("Fail: wrmsr(0x%08x, 0x%016"PRIx64") did not fault.\n",
The trailing punctuation isn't useful here.
> + idx, wval);
> +}
> +
> +static void test_transparent_msr_write(uint32_t idx, uint64_t wval)
What does transparent mean in this context?
> +{
> + uint64_t rval1 = 0, rval2 = 0;
> +
> + /* read current value */
> + if ( rdmsr_safe(idx, &rval1) )
> + xtf_failure("Fail: rdmsr(0x%08x, 0x016%"PRIxPTR") got unexpected
> fault"
> + "\n", idx, (uintptr_t)&rval1);
> +
> + /* wrmsr should not fault but should not write anything either */
> + if ( wrmsr_safe(idx, wval) )
> + xtf_failure("Fail: wrmsr(0x%08x, 0x%016"PRIx64") got unexpected
> fault"
> + "\n", idx, wval);
> +
> + /* read new value */
> + if ( rdmsr_safe(idx, &rval2) )
> + xtf_failure("Fail: rdmsr(0x%08x, 0x016%"PRIxPTR") got unexpected
> fault"
> + "\n", idx, (uintptr_t) &rval2);
> +
> + /* Check if the new value is the same as the one before wrmsr */
> + if ( rval1 != rval2 )
> + xtf_failure("Fail: rdmsr mismatch idx 0x%08x, wval 0x%016"PRIx64
> + ", rval 0x%016"PRIx64"\n", idx, rval1, rval2);
> +}
> +
> +static void test_intel_pmu_ver1(uint8_t ng, uint8_t ngb)
What are ng and ngb?
> +{
> + /*
> + * Intel Sofwtare Development Manual Vol. 3B,
> + * Section 18.2.1 - Architectural Performance Monitoring Version 1
> + *
> + * MSRs made available by the VPMU
> + *
> + * PMCx (start at address 0xc1)
> + * PERFEVENTSELx (start at address 0x186)
> + */
> +
> + uint32_t idx;
> + uint64_t wval = 0;
> + uint64_t erval = 0;
> + unsigned i;
> +
> + printk("Testing version 1\n");
> +
> + /* for all general purpose counters */
> + for ( i = 0; i < ng; i++ )
> + {
> + /* test writing to PMCx */
> + idx = MSR_PMC(i);
> +
> + /* test we can write to all valid bits in the counters */
> + wval = ((1ull << 32) - 1);
> +
> + /* Bit 32 is sign extended to the the high order bits */
> + erval = ((1ull << ngb) - 1);
> +
> + test_valid_msr_write(idx, wval, erval);
I'm not convinced by the usefulness of breaking out idx. What is wrong
with:
test_valid_msr_write(MSR_PMC(i), wval, erval);
here?
> +
> + /* set all valid bits in MSR_EVENTSELx */
> + idx = MSR_PERFEVTSEL(i);
> + wval = ((1ull << 32) - 1) ^ (PERFEVENTSELx_ENABLE_ANYTHREAD |
> + PERFEVENTSELx_ENABLE_PCB);
> +
> + test_valid_msr_write(idx, wval, wval);
> +
> + /* test writing an invalid value and assert that it faults */
> + wval = ~((1ull << 32) - 1);
> +
> + test_invalid_msr_write(idx, wval);
> + }
> +}
> +
> +static void test_intel_pmu_ver2(uint8_t ng, uint8_t nf)
> +{
> + /*
> + * Intel Sofwtare Development Manual Vol. 3B,
> + * Section 18.2.2 - Architectural Performance Monitoring Version 2
> + *
> + * MSRs made available by the VPMU -
> + *
> + * FIXED_CTRx (start at address 0x309)
> + * FIXED_CTR_CTRL
> + * PERF_GLOBAL_CTRL
> + * PERF_GLOBAL_STATUS (read-only)
> + * PERF_GLOBAL_OVF_CTRL
> + * DEBUGCTL_Freeze_LBR_ON_PMI
> + * DEBUGCTL_Freeze_PerfMon_On_PMI
> + */
> +
> + uint32_t idx;
> + uint64_t wval;
> + uint64_t rval;
> + unsigned i;
> +
> + printk("Testing version 2\n");
> +
> + for ( i = 0; i < nf; i++ )
> + {
> + /* test writing to FIXED_CTRx */
> + idx = MSR_FIXED_CTR(i);
> +
> + wval = (1ull << 32) - 1;
> +
> + test_valid_msr_write(idx, wval, wval);
> +
> + /* test invalid write to FIXED_CTRx */
> + wval = ~wval;
> +
> + test_invalid_msr_write(idx, wval);
> + }
> +
> + /* test FIXED_CTR_CTRL */
> + idx = MSR_FIXED_CTR_CTRL;
> +
> + /* enable all fixed counters */
> + wval = 0;
> +
> + for ( i = 0; i < nf; i++ )
> + wval |= (FIXED_CTR_ENABLE << (FIXED_CTR_CTL_BITS * i));
> +
> + test_valid_msr_write(idx, wval, wval);
> +
> + /* invert wval to test writing an invalid value */
> + wval = ~wval;
> +
> + test_invalid_msr_write(idx, wval);
> +
> + /* test PERF_GLOBAL_CTRL */
> + idx = MSR_PERF_GLOBAL_CTRL;
> +
> + wval = 0;
> +
> + /* set all fixed function counters enable bits */
> + for ( i=0; i < nf; i ++ )
i = 0 please.
> + wval |= ((uint64_t)1 << (32 + i));
1ull
> +
> + /* set all general purpose counters enable bits*/
> + for ( i = 0; i < ng; i++ )
> + wval |= (1 << i);
1u
> +
> + test_valid_msr_write(idx, wval, wval);
> +
> + /* invert wval to test invalid write to MSR_PERF_GLOBAL_CTRL*/
> + wval = ~wval;
> +
> + test_invalid_msr_write(idx, wval);
> +
> + /* test PERF_GLOBAL_OVF_CTRL */
> + idx = MSR_PERF_GLOBAL_OVF_CTRL;
> +
> + /* set all valid bits in MSR_PERF_GLOBAL_OVF_CTRL */
> + wval = 0xC000000000000000 | (((1ULL << nf) - 1) << 32) | ((1ULL << ng) -
> 1);
> +
> + /*
> + * Writing to MSR_PERF_GLOBAL_OVF_CTRL clears
> + * MSR_PERF_GLOBAL_STATUS but but always returns 0 when read so
> + * it is tested as a transparent write
> + */
> +
> + test_transparent_msr_write(idx, wval);
> +
> + /* invert wval to test invalid write to MSR_PERF_GLOBAL_OVF_CTRL*/
> + wval = ~wval;
> +
> + test_invalid_msr_write(idx, wval);
> +
> + /* test PERF_GLOBAL_STATUS (read-only) */
> + idx = MSR_PERF_GLOBAL_STATUS;
> +
> + if ( rdmsr_safe(idx, &rval) )
> + {
> + xtf_failure("Error: test_intel_pmu_ver2: "
> + "rdmsr_safe for MSR 0x%x resulted in a fault!\n", idx);
> + }
> +
> + /* try to write the PERF_GLOBAL_STATUS register and expect it to fail*/
> + test_invalid_msr_write(idx, wval);
> +
> + /* test DEBUGCTL */
> + idx = MSR_DEBUGCTL;
> +
> + /* Test DEBUGCTL facilities enabled by v2 */
> + wval = DEBUGCTL_Freeze_LBR_ON_PMI | DEBUGCTL_Freeze_PerfMon_On_PMI;
> +
> + test_valid_msr_write(idx, wval, wval);
> +
> +}
> +
> +static void test_intel_pmu_ver3(uint8_t ng, uint8_t nf)
> +{
> + /*
> + * Intel Sofwtare Development Manual Vol. 3B
> + * Section 18.2.3 Architectural Performance Monitoring Version 3
> + *
> + * MSRs made available by the VPMU
> + *
> + * PERFEVENTSELx.ANYTHREAD (Bit 21)
> + * FIXED_CTR_CTRL.ANYTHREADx (Bit 2, 6, 11)
> + *
> + * Version 3 introduces ANYTHREAD bit but VPMU does not support it to
> + * ensure that a VCPU isn't able to read values from physical resources
> that
> + * are not allocated to it. This test case validates that we are unable
> to
> + * write to .ANYTHREAD bit in PERFEVENTSELx and FIXED_CTR_CTRL
> + */
> +
> + uint64_t wval;
> + uint32_t idx;
> + unsigned i;
> +
> + printk("Testing version 3\n");
> +
> + /* test PERFEVENTSELx.ANYTHREAD is disabled */
> + for ( i = 0; i < ng; i++ )
> + {
> + idx = MSR_PERFEVTSEL(i);
> +
> + wval = EVENT_UOPS_RETIRED_ANYTHREAD;
> +
> + test_invalid_msr_write(idx, wval);
> + }
> +
> + /* test FIXED_CTR_CTL.ANYTHREAD is disabled */
> + idx = MSR_FIXED_CTR_CTRL;
> +
> + wval = 0;
> +
> + for ( i = 0; i < nf; i++ )
> + wval |= (FIXED_CTR_ENABLE_ANYTHREAD << (4 * i)) ;
> +
> + test_invalid_msr_write(idx, wval);
> +}
> +
> +static void test_full_width_cnts(uint8_t ng, uint8_t ngb, uint8_t pdcm)
> +{
> + uint64_t caps, wval;
> + uint32_t idx;
> + unsigned i;
> +
> + if ( rdmsr_safe(MSR_PERF_CAPABILITIES, &caps) )
> + {
> + if (pdcm) {
> + xtf_failure("Fail: Fault while reading MSR_PERF_CAPABILITIES\n");
> + }
> + return;
> + }
> + else if ( !pdcm )
> + {
> + /*
> + * MSR_PERF_CAPABILITIES should nto be readable without PDCM
> + * bit set in CPUID.
> + * XENBUG: However, at the time of writing this test, Xen
> + * leaks PERF_CAPABILITIES into guest view which makes it a known
> + * error. Do not fail but report the error.
> + */
> + xtf_error("Error: MSR_PERF_CAPABILITIES readable while PDCM is 0\n");
> + }
> +
> +
> + if ( !(caps & PERF_CAPABILITIES_Full_Width) )
> + {
> + printk("Info: Full width counters not supported\n");
> + return;
> + }
> +
> + /* test writes to full width counters */
> + for ( i = 0; i < ng; i++)
> + {
> + idx = MSR_A_PMC(i);
> +
> + wval = ((1ull << ngb) - 1) ;
> +
> + test_valid_msr_write(idx, wval, wval);
> +
> + /* invert wval to test invalid write to MSR_PERF_GLOBAL_OVF_CTRL */
> + wval = ~wval;
> +
> + test_invalid_msr_write(idx, wval);
> + }
> +}
> +
> +void test_main(void)
> +{
> + /* Architectural Performance Monitoring Version */
> + uint8_t ver;
> + /* Number of general purpose counter registers */
> + uint8_t ngregs;
> + /* Number of fixed function counter registers */
> + uint8_t nfregs;
> + /* Bit width of general-purpose, performance monitoring counter */
> + uint8_t ngbits;
> + /* Performance and debug capabilties MSR*/
> + uint8_t pdcm;
> +
> + /* cpuid variables */
> + uint32_t leaf = 0x0a, subleaf = 0;
As with the idx comment, I am not convinced of the value of breaking out
leaf and subleaf like this.
> + uint32_t eax, ebx, ecx, edx;
> +
> + if ( vendor_is_amd )
> + return xtf_skip("Skip: VPMU testing for AMD currently
> unsupported\n");
> +
> + if (max_leaf < leaf)
if ( max_leaf < leaf )
> + return xtf_skip("Skip: Unable to retrieve PMU information from
> cpuid\n");
max_leaf < 0xa is synonymous with finding 0 in eax. Both of them mean
that Architectural PMU is unavailable. I'd word this as:
"Skip: Architectural PMU unavailable\n"
> +
> + /*
> + * Inter Software Development Manual Vol 2A
> + * Table 3-8 Information Returned by CPUID Instruction
> + */
> +
> + cpuid_count(leaf, subleaf, &eax, &ebx, &ecx, &edx);
> +
> + /* Extract the version ID - EAX 7:0 */
> + ver = (eax & 0xff);
And skip with the 0 case here, rather than complicating the logic below.
> +
> + /* Extract number of general purpose counter regs - EAX 15:8 */
> + ngregs = (eax >> 8) & 0xff;
> +
> + /* Extract number of fixed function counter regs - EDX 4:0 */
> + nfregs = edx & 0x1f;
> +
> + /* Extract number of bits in general purpose counter registers bits */
> + ngbits = (eax >> 16) & 0xff;
> +
> + /* Retrieve Perf & Debug Capabilties MSR availability*/
> + leaf = 0x01;
> +
> + printk("Info: PMU Version %u, Genereal purpose counters %u, Fixed
> function "
> + "counters %u, Counter width %u\n", ver, ngregs, nfregs, ngbits);
> +
> + cpuid_count(leaf, subleaf, &eax, &ebx, &ecx, &edx);
> +
> + /* Extract Performance & Debug Capabilties MSR from ECX bit 15 */
> + pdcm = (ecx >> 15) & 0x01;
Use this extra hunk:
diff --git a/arch/x86/include/arch/cpuid.h b/arch/x86/include/arch/cpuid.h
index bff55d4..8cb9a78 100644
--- a/arch/x86/include/arch/cpuid.h
+++ b/arch/x86/include/arch/cpuid.h
@@ -75,6 +75,7 @@ static inline bool cpu_has(unsigned int feature)
#define cpu_has_sse2 cpu_has(X86_FEATURE_SSE2)
#define cpu_has_vmx cpu_has(X86_FEATURE_VMX)
#define cpu_has_smx cpu_has(X86_FEATURE_SMX)
+#define cpu_has_pdcm cpu_has(X86_FEATURE_PCDM)
#define cpu_has_pcid cpu_has(X86_FEATURE_PCID)
#define cpu_has_xsave cpu_has(X86_FEATURE_XSAVE)
#define cpu_has_avx cpu_has(X86_FEATURE_AVX)
to avoid special casing a feature flag check.
> +
> + printk("Info: Perf & Debug Capability MSR %u\n", pdcm);
> +
> + switch (ver)
> + {
> +
> + default:
> + /*
> + * Version 4 facilities are not supported by Xen.
> + * VPMU emulates version 3. Fall through
> + */
> + xtf_warning("Test doesn't support version %u\n", ver);
The indentation of this entire switch statement looks suspect.
> +
> + case 3:
> + /* test version 3 facilities */
> + test_intel_pmu_ver3( ngregs, nfregs);
> +
> + /* fall through */
> +
> + case 2:
> + /* test version 2 facilities */
> + test_intel_pmu_ver2(ngregs, nfregs);
> +
> + /* test version 1 facilities */
> + test_intel_pmu_ver1(ngregs, ngbits);
> +
> + /* test full width counters */
> + test_full_width_cnts(ngregs, ngbits, pdcm);
> +
> + break;
> +
> + case 1:
> + return xtf_skip("Skip: VPMU version 1 unsupported in Xen\n");
I don't think I got my point across before.
Xen choosing to not support version 1 on real hardware has no relevance
to what version Xen emulates to the guest.
There is nothing wrong with Xen emulating version 1 to a guest.
> +
> + case 0:
> + return xtf_skip("Skip: VPMU not enabled in Xen\n");
> + }
> +
> + return xtf_success(NULL);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
I gave this test a spin.
(d4) --- Xen Test Framework ---
(d4) Environment: HVM 64bit (Long mode 4 levels)
(d4) Test vpmu
(d4) Info: PMU Version 3, Genereal purpose counters 4, Fixed function
counters 3, Counter width 48
(d4) Info: Perf & Debug Capability MSR 0
(d4) Testing version 3
(d4) Testing version 2
(XEN) vpmu_intel.c:602:d4v0 Can not write readonly MSR:
MSR_PERF_GLOBAL_STATUS(0x38E)!
(d4) Fail: rdmsr mismatch idx 0x000001d9, wval 0x0000000000001800, rval
0x0000000000000000
(d4) Testing version 1
(d4) Error: MSR_PERF_CAPABILITIES readable while PDCM is 0
(d4) Test result: FAILURE
Is MSR_PERF_GLOBAL_STATUS read only, or read write? Xen and your test
disagree on this point.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |