Re: [kvm-unit-tests PATCH 1/2] x86/pmu: Update rdpmc testcase to cover #GP and emulation path

[Like Xu]

On 6/10/2022 5:36 am, Sean Christopherson wrote:
> On Mon, Sep 05, 2022, Like Xu wrote:
> > From: Like Xu <likexu@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > ---
> >   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