Messages in this thread | | | Date | Wed, 19 Oct 2022 16:50:53 +0800 | Subject | Re: [kvm-unit-tests PATCH 1/2] x86/pmu: Update rdpmc testcase to cover #GP and emulation path | From | Like Xu <> |
| |
On 6/10/2022 5:36 am, Sean Christopherson wrote: > On Mon, Sep 05, 2022, Like Xu wrote: >> From: Like Xu <likexu@tencent.com> >> >> Specifying an unsupported PMC encoding will cause a #GP(0). >> All testcases should be passed when the KVM_FEP prefix is added. >> >> Signed-off-by: Like Xu <likexu@tencent.com> >> --- >> lib/x86/processor.h | 5 ++++- >> x86/pmu.c | 13 +++++++++++++ >> 2 files changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/lib/x86/processor.h b/lib/x86/processor.h >> index 10bca27..9c490d9 100644 >> --- a/lib/x86/processor.h >> +++ b/lib/x86/processor.h >> @@ -441,7 +441,10 @@ static inline int wrmsr_safe(u32 index, u64 val) >> static inline uint64_t rdpmc(uint32_t index) >> { >> uint32_t a, d; >> - asm volatile ("rdpmc" : "=a"(a), "=d"(d) : "c"(index)); >> + if (is_fep_available()) >> + asm volatile (KVM_FEP "rdpmc" : "=a"(a), "=d"(d) : "c"(index)); >> + else >> + asm volatile ("rdpmc" : "=a"(a), "=d"(d) : "c"(index)); > > Hmm, not sure how I feel about the idea of always use FEP in a common helper when > it's available. Part of me likes the idea, but part of me is worried that it > will cause confusion due to not being explicit. > > Unless there's a pressing need to force emulation, let's punt the FEP stuff for > now. More below.
Some security researchers are very interested in these corners.
To my limited testing, most KVM emulation code (at least arch/x86/kvm/emulate.c) are not adequately covered by test cases, and perhaps some will move them to the user space.
> >> return a | ((uint64_t)d << 32); >> } >> >> diff --git a/x86/pmu.c b/x86/pmu.c >> index 203a9d4..11607c0 100644 >> --- a/x86/pmu.c >> +++ b/x86/pmu.c >> @@ -758,12 +758,25 @@ static bool pmu_is_detected(void) >> return detect_intel_pmu(); >> } >> >> +static void rdpmc_unsupported_counter(void *data) >> +{ >> + rdpmc(64); >> +} >> + >> +static void check_rdpmc_cause_gp(void) > > Maybe check_invalid_rdpmc_gp()? There are multiple reasons RDPMC can #GP, the > one that is being relied on to guarantee #GP is specifically that the PMC is > invalid.
Applied.
> dd
p, :D
> >> +{ >> + report(test_for_exception(GP_VECTOR, rdpmc_unsupported_counter, NULL), > > I'd really like to move away from test_for_exception() and use ASM_TRY(). Ignoring > FEP for the moment, the most extensible solution is to provide a safe variant: > > static inline int rdpmc_safe(u32 index, uint64_t *val) > { > uint32_t a, d; > > asm volatile (ASM_TRY("1f") > "rdpmc" > : "=a"(a), "=d"(d) : "c"(index));
asm volatile (ASM_TRY("1f") "rdpmc\n\t" "1:" : "=a"(a), "=d"(d) : "c"(index) : "memory");
, otherwise the compiler will complain.
> *val = (uint64_t)a | ((uint64_t)d << 32); > return exception_vector(); > } > > static inline uint64_t rdpmc(uint32_t index) > { > uint64_t val; > int vector = rdpmc_safe(index, &val); > > assert_msg(!vector, "Unexpected %s on RDPMC(%d)", > exception_mnemonic(vector), index); > return val; > }
Applied.
> > > For long-term emulation validation, the best idea I have at this point is to do > add a config knob to opt-in to using FEP in _all_ common helpers (where "all" > means everything KVM actually emulates). It'd take some macro magic, but it'd > be easier to maintain (no need to have two paths in every helper) and would be > controllable.
With both hands up in favour. Leave it to you, as this involves a wider change.
> >> + "rdpmc with invalid PMC index raises #GP"); >> +} >> + >> int main(int ac, char **av) >> { >> setup_vm(); >> handle_irq(PC_VECTOR, cnt_overflow); >> buf = malloc(N*64); >> >> + check_rdpmc_cause_gp(); >> + >> if (!pmu_is_detected()) >> return report_summary(); >> >> -- >> 2.37.3 >>
| |