| Message ID | 20220905123946.95223-6-likexu@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,1/2] x86/pmu: Update rdpmc testcase to cover #GP and emulation path | expand |
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. > 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. dd > +{ > + 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)); *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; } 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. > + "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 >
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 >>
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)); 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) +{ + report(test_for_exception(GP_VECTOR, rdpmc_unsupported_counter, NULL), + "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();