Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c4e78b67-bfcb-3671-3de9-252b97062c27@gmail.com>
Date:   Wed, 19 Oct 2022 16:50:53 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.3.3
Subject: Re: [kvm-unit-tests PATCH 1/2] x86/pmu: Update rdpmc testcase to
 cover #GP and emulation path
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220905123946.95223-1-likexu@tencent.com>
 <20220905123946.95223-6-likexu@tencent.com> <Yz347iKzq7cbjMdw@google.com>
From:   Like Xu <like.xu.linux@gmail.com>
In-Reply-To: <Yz347iKzq7cbjMdw@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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
>>