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 <[email protected]>
---
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)
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 <[email protected]>
> ---
> 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)
>
Queued, thanks.
Paolo
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 <[email protected]>
>> ---
>> 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
>
Hi Like -
Thanks for the feedback - is your recommendation to do the read and
period change at the same time and only take the lock once or is there
another way around this while still handling writes correctly?
Eric
On 2020/2/25 8:08, Eric Hankland wrote:
> Hi Like -
>
> Thanks for the feedback - is your recommendation to do the read and
> period change at the same time and only take the lock once or is there
> another way around this while still handling writes correctly?
For non-running counters(the most common situation), we have too many
chances to reflect their new periods. In this case, calling
perf_event_period() in the trap of counter msr is redundant and burdensome.
A better way is to check if this counter is running via
pmc_speculative_in_use (), and if so,
just trigger kvm_make_request(KVM_REQ_PMU, pmc->vcpu).
Thanks,
Like Xu
>
> Eric
>