linux-kernel - Re: [PATCH] KVM: x86: Adjust counter sample period after a wrmsr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0c66eae3-8983-0632-6d39-fd335620b76a@linux.intel.com>
Date:   Mon, 24 Feb 2020 14:56:08 +0800
From:   Like Xu <like.xu@...ux.intel.com>
To:     Paolo Bonzini <pbonzini@...hat.com>,
        Eric Hankland <ehankland@...gle.com>,
        Jim Mattson <jmattson@...gle.com>,
        Peter Shier <pshier@...gle.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH] KVM: x86: Adjust counter sample period after a wrmsr

Hi Hankland,

On 2020/2/22 15:34, Paolo Bonzini wrote:
> On 22/02/20 03:34, Eric Hankland wrote:
>> The sample_period of a counter tracks when that counter will
>> overflow and set global status/trigger a PMI. However this currently
>> only gets set when the initial counter is created or when a counter is
>> resumed; this updates the sample period after a wrmsr so running
>> counters will accurately reflect their new value.
>>
>> Signed-off-by: Eric Hankland <ehankland@...gle.com>
>> ---
>>   arch/x86/kvm/pmu.c           | 4 ++--
>>   arch/x86/kvm/pmu.h           | 8 ++++++++
>>   arch/x86/kvm/vmx/pmu_intel.c | 6 ++++++
>>   3 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
>> index bcc6a73d6628..d1f8ca57d354 100644
>> --- a/arch/x86/kvm/pmu.c
>> +++ b/arch/x86/kvm/pmu.c
>> @@ -111,7 +111,7 @@ static void pmc_reprogram_counter(struct kvm_pmc *pmc, u32 type,
>>   		.config = config,
>>   	};
>>   
>> -	attr.sample_period = (-pmc->counter) & pmc_bitmask(pmc);
>> +	attr.sample_period = get_sample_period(pmc, pmc->counter);
>>   
>>   	if (in_tx)
>>   		attr.config |= HSW_IN_TX;
>> @@ -158,7 +158,7 @@ static bool pmc_resume_counter(struct kvm_pmc *pmc)
>>   
>>   	/* recalibrate sample period and check if it's accepted by perf core */
>>   	if (perf_event_period(pmc->perf_event,
>> -			(-pmc->counter) & pmc_bitmask(pmc)))
>> +			      get_sample_period(pmc, pmc->counter)))
>>   		return false;
>>   
>>   	/* reuse perf_event to serve as pmc_reprogram_counter() does*/
>> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
>> index 13332984b6d5..354b8598b6c1 100644
>> --- a/arch/x86/kvm/pmu.h
>> +++ b/arch/x86/kvm/pmu.h
>> @@ -129,6 +129,15 @@ static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
>>   	return NULL;
>>   }
>>   
>> +static inline u64 get_sample_period(struct kvm_pmc *pmc, u64 counter_value)
>> +{
>> +	u64 sample_period = (-counter_value) & pmc_bitmask(pmc);
>> +
>> +	if (!sample_period)
>> +		sample_period = pmc_bitmask(pmc) + 1;
>> +	return sample_period;
>> +}
>> +
>>   void reprogram_gp_counter(struct kvm_pmc *pmc, u64 eventsel);
>>   void reprogram_fixed_counter(struct kvm_pmc *pmc, u8 ctrl, int fixed_idx);
>>   void reprogram_counter(struct kvm_pmu *pmu, int pmc_idx);
>> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
>> index fd21cdb10b79..e933541751fb 100644
>> --- a/arch/x86/kvm/vmx/pmu_intel.c
>> +++ b/arch/x86/kvm/vmx/pmu_intel.c
>> @@ -263,9 +263,15 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>   			if (!msr_info->host_initiated)
>>   				data = (s64)(s32)data;
>>   			pmc->counter += data - pmc_read_counter(pmc);
>> +			if (pmc->perf_event)
>> +				perf_event_period(pmc->perf_event,
>> +						  get_sample_period(pmc, data));
>>   			return 0;
>>   		} else if ((pmc = get_fixed_pmc(pmu, msr))) {
>>   			pmc->counter += data - pmc_read_counter(pmc);
>> +			if (pmc->perf_event)
>> +				perf_event_period(pmc->perf_event,
>> +						  get_sample_period(pmc, data));
>>   			return 0;
>>   		} else if ((pmc = get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0))) {
>>   			if (data == pmc->eventsel)

Although resetting the running counters is allowed,
it is not recommended to do it.

The motivation of this patch looks good to me.

However, it does hurt performance due to more frequent calls to 
perf_event_period() and we just took the perf_event_ctx_lock in the 
perf_event_read_value().

Thanks,
Like Xu

>>
> 
> Queued, thanks.
> 
> Paolo
>