* [PATCH] KVM: x86/pmu: Prevent zero period event from being repeatedly released
2022-12-07 7:15 [PATCH kvm-unit-tests] x86/pmu: Add Intel Guest Transactional (commited) cycles testcase Like Xu
@ 2022-12-07 7:15 ` Like Xu
2022-12-07 16:52 ` Sean Christopherson
2022-12-23 16:58 ` [PATCH] " Paolo Bonzini
2022-12-07 15:51 ` [PATCH kvm-unit-tests] x86/pmu: Add Intel Guest Transactional (commited) cycles testcase Yang, Weijiang
2022-12-07 17:32 ` Sean Christopherson
2 siblings, 2 replies; 6+ messages in thread
From: Like Xu @ 2022-12-07 7:15 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel
From: Like Xu <likexu@tencent.com>
The current vPMU can reuse the same pmc->perf_event for the same
hardware event via pmc_pause/resume_counter(), but this optimization
does not apply to a portion of the TSX events (e.g., "event=0x3c,in_tx=1,
in_tx_cp=1"), where event->attr.sample_period is legally zero at creation,
thus making the perf call to perf_event_period() meaningless (no need to
adjust sample period in this case), and instead causing such reusable
perf_events to be repeatedly released and created.
Avoid releasing zero sample_period events by checking is_sampling_event()
to follow the previously enable/disable optimization.
Signed-off-by: Like Xu <likexu@tencent.com>
---
arch/x86/kvm/pmu.c | 3 ++-
arch/x86/kvm/pmu.h | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index 684393c22105..eb594620dd75 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -238,7 +238,8 @@ static bool pmc_resume_counter(struct kvm_pmc *pmc)
return false;
/* recalibrate sample period and check if it's accepted by perf core */
- if (perf_event_period(pmc->perf_event,
+ if (is_sampling_event(pmc->perf_event) &&
+ perf_event_period(pmc->perf_event,
get_sample_period(pmc, pmc->counter)))
return false;
diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index 85ff3c0588ba..cdb91009701d 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -140,7 +140,8 @@ static inline u64 get_sample_period(struct kvm_pmc *pmc, u64 counter_value)
static inline void pmc_update_sample_period(struct kvm_pmc *pmc)
{
- if (!pmc->perf_event || pmc->is_paused)
+ if (!pmc->perf_event || pmc->is_paused ||
+ !is_sampling_event(pmc->perf_event))
return;
perf_event_period(pmc->perf_event,
--
2.38.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: x86/pmu: Prevent zero period event from being repeatedly released
2022-12-07 7:15 ` [PATCH] KVM: x86/pmu: Prevent zero period event from being repeatedly released Like Xu
@ 2022-12-07 16:52 ` Sean Christopherson
2022-12-23 16:58 ` [PATCH] " Paolo Bonzini
1 sibling, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2022-12-07 16:52 UTC (permalink / raw)
To: Like Xu; +Cc: Paolo Bonzini, kvm, linux-kernel
Please don't mix kernel and KVM-unit-tests patches in the same "series", for those
of us that have become dependent on b4, mixing patches for two separate repos
makes life miserable.
The best alternative I have come up with is to post the KVM patch(es), and then
provide a lore link in the KUT patch(es). It means waiting a few minutes before
sending the KUT if you want to double check that you got the lore link right,
but I find that it's fairly easy to account for that in my workflow.
On Wed, Dec 07, 2022, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
>
> The current vPMU can reuse the same pmc->perf_event for the same
> hardware event via pmc_pause/resume_counter(), but this optimization
> does not apply to a portion of the TSX events (e.g., "event=0x3c,in_tx=1,
> in_tx_cp=1"), where event->attr.sample_period is legally zero at creation,
> thus making the perf call to perf_event_period() meaningless (no need to
> adjust sample period in this case), and instead causing such reusable
> perf_events to be repeatedly released and created.
>
> Avoid releasing zero sample_period events by checking is_sampling_event()
> to follow the previously enable/disable optimization.
>
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
> arch/x86/kvm/pmu.c | 3 ++-
> arch/x86/kvm/pmu.h | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index 684393c22105..eb594620dd75 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -238,7 +238,8 @@ static bool pmc_resume_counter(struct kvm_pmc *pmc)
> return false;
>
> /* recalibrate sample period and check if it's accepted by perf core */
> - if (perf_event_period(pmc->perf_event,
> + if (is_sampling_event(pmc->perf_event) &&
> + perf_event_period(pmc->perf_event,
> get_sample_period(pmc, pmc->counter)))
> return false;
>
> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> index 85ff3c0588ba..cdb91009701d 100644
> --- a/arch/x86/kvm/pmu.h
> +++ b/arch/x86/kvm/pmu.h
> @@ -140,7 +140,8 @@ static inline u64 get_sample_period(struct kvm_pmc *pmc, u64 counter_value)
>
> static inline void pmc_update_sample_period(struct kvm_pmc *pmc)
> {
> - if (!pmc->perf_event || pmc->is_paused)
> + if (!pmc->perf_event || pmc->is_paused ||
> + !is_sampling_event(pmc->perf_event))
> return;
>
> perf_event_period(pmc->perf_event,
> --
> 2.38.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Prevent zero period event from being repeatedly released
2022-12-07 7:15 ` [PATCH] KVM: x86/pmu: Prevent zero period event from being repeatedly released Like Xu
2022-12-07 16:52 ` Sean Christopherson
@ 2022-12-23 16:58 ` Paolo Bonzini
1 sibling, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2022-12-23 16:58 UTC (permalink / raw)
To: Like Xu; +Cc: Sean Christopherson, kvm, linux-kernel
Queued, thanks. Please resubmit the test though.
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH kvm-unit-tests] x86/pmu: Add Intel Guest Transactional (commited) cycles testcase
2022-12-07 7:15 [PATCH kvm-unit-tests] x86/pmu: Add Intel Guest Transactional (commited) cycles testcase Like Xu
2022-12-07 7:15 ` [PATCH] KVM: x86/pmu: Prevent zero period event from being repeatedly released Like Xu
@ 2022-12-07 15:51 ` Yang, Weijiang
2022-12-07 17:32 ` Sean Christopherson
2 siblings, 0 replies; 6+ messages in thread
From: Yang, Weijiang @ 2022-12-07 15:51 UTC (permalink / raw)
To: Like Xu; +Cc: kvm, linux-kernel, Sean Christopherson, Paolo Bonzini
On 12/7/2022 3:15 PM, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
>
> On Intel platforms with TSX feature, pmu users in guest can collect
> the commited or total transactional cycles for a tsx-enabled workload,
> adding new test cases to cover them, as they are not strictly the same
> as normal hardware events from the KVM implementation point of view.
>
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
> x86/pmu.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/x86/pmu.c b/x86/pmu.c
> index 72c2c9c..d4c6813 100644
> --- a/x86/pmu.c
> +++ b/x86/pmu.c
> @@ -20,7 +20,7 @@
>
> typedef struct {
> uint32_t ctr;
> - uint32_t config;
> + uint64_t config;
> uint64_t count;
> int idx;
> } pmu_counter_t;
> @@ -547,6 +547,76 @@ static void check_emulated_instr(void)
> report_prefix_pop();
> }
>
> +#define _XBEGIN_STARTED (~0u)
> +
> +static inline int _xbegin(void)
> +{
> + int ret = _XBEGIN_STARTED;
> + asm volatile(".byte 0xc7,0xf8 ; .long 0" : "+a" (ret) :: "memory");
> + return ret;
> +}
> +
> +static inline void _xend(void)
> +{
> + asm volatile(".byte 0x0f,0x01,0xd5" ::: "memory");
> +}
> +
> +int *ptr;
> +
> +static void tsx_fault(void)
> +{
> + int value = 0;
> +
> + ptr = NULL;
> + if(_xbegin() == _XBEGIN_STARTED) {
> + value++;
> + // causes abort
> + *ptr = value;
> + _xend();
> + }
> +}
> +
> +static void tsx_normal(void)
> +{
> + int value = 0;
> +
> + if(_xbegin() == _XBEGIN_STARTED) {
> + value++;
> + _xend();
> + }
> +}
> +
> +static void check_tsx_cycles(void)
> +{
> + pmu_counter_t cnt;
> + int i;
> +
> + if (!this_cpu_has(X86_FEATURE_RTM) || !this_cpu_has(X86_FEATURE_HLE))
> + return;
Since the test case is for xbegin/xend, HLE check may omit as it's for
other X-instructions.
> +
> + report_prefix_push("TSX cycles");
> +
> + for (i = 0; i < pmu.nr_gp_counters; i++) {
> + cnt.ctr = MSR_GP_COUNTERx(i);
> +
> + if (i == 2)
> + /* Transactional cycles commited only on gp counter 2 */
> + cnt.config = EVNTSEL_OS | EVNTSEL_USR | 0x30000003c;
> + else
> + /* Transactional cycles */
> + cnt.config = EVNTSEL_OS | EVNTSEL_USR | 0x10000003c;
> +
> + start_event(&cnt);
> + tsx_fault();
> + tsx_normal();
> + stop_event(&cnt);
> +
> + report(cnt.count > 0, "gp cntr-%d", i);
The purpose is to collect total cycles, why not print out the data here
for each GP counter?
> + }
> +
> + report_prefix_pop();
> +}
> +
> static void check_counters(void)
> {
> if (is_fep_available())
> @@ -559,6 +629,7 @@ static void check_counters(void)
> check_counter_overflow();
> check_gp_counter_cmask();
> check_running_counter_wrmsr();
> + check_tsx_cycles();
> }
>
> static void do_unsupported_width_counter_write(void *index)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH kvm-unit-tests] x86/pmu: Add Intel Guest Transactional (commited) cycles testcase
2022-12-07 7:15 [PATCH kvm-unit-tests] x86/pmu: Add Intel Guest Transactional (commited) cycles testcase Like Xu
2022-12-07 7:15 ` [PATCH] KVM: x86/pmu: Prevent zero period event from being repeatedly released Like Xu
2022-12-07 15:51 ` [PATCH kvm-unit-tests] x86/pmu: Add Intel Guest Transactional (commited) cycles testcase Yang, Weijiang
@ 2022-12-07 17:32 ` Sean Christopherson
2 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2022-12-07 17:32 UTC (permalink / raw)
To: Like Xu; +Cc: Paolo Bonzini, kvm, linux-kernel
On Wed, Dec 07, 2022, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
>
> On Intel platforms with TSX feature, pmu users in guest can collect
> the commited or total transactional cycles for a tsx-enabled workload,
> adding new test cases to cover them, as they are not strictly the same
> as normal hardware events from the KVM implementation point of view.
>
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
> x86/pmu.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/x86/pmu.c b/x86/pmu.c
> index 72c2c9c..d4c6813 100644
> --- a/x86/pmu.c
> +++ b/x86/pmu.c
> @@ -20,7 +20,7 @@
>
> typedef struct {
> uint32_t ctr;
> - uint32_t config;
> + uint64_t config;
> uint64_t count;
> int idx;
> } pmu_counter_t;
> @@ -547,6 +547,76 @@ static void check_emulated_instr(void)
> report_prefix_pop();
> }
>
> +#define _XBEGIN_STARTED (~0u)
> +
> +static inline int
This should be "unsigned int". EAX can yield a negative value, e.g. via "XABORT
0xff", which is why the compiler instrinsics use this explicit, unsigned value
(that relies on reserved bits in the abort status).
> _xbegin(void)
These having nothing to do with the PMU, i.e. belong in processor.h.
The naming is also non-stanard, i.e. drop the underscore. I assume you're
trying to match the compiler instrinsics, but that's bound to do more harm than
good. Either use the instrinsics or write code that aligns with KUT's style.
Using instrinsics is probably a bad idea because eventually we'll want to do
something weird, e.g. provide a bogus fallback address. And having to go lookup
gcc/clang documentation is rather annoything.
> +{
> + int ret = _XBEGIN_STARTED;
Newline after declarations.
> + asm volatile(".byte 0xc7,0xf8 ; .long 0" : "+a" (ret) :: "memory");
This is just mean.
unsigned int ret = XBEGIN_STARTED;
asm volatile("xbegin 1f\n\t"
"1:\n\t"
: "+a" (ret) :: "memory");
return ret;
> + return ret;
> +}
> +
> +static inline void _xend(void)
> +{
> + asm volatile(".byte 0x0f,0x01,0xd5" ::: "memory");
Like XBEGIN, use the mnemonic.
> +}
> +
> +int *ptr;
I'm honestly at a loss for words.
> +static void tsx_fault(void)
s/fault/abort. Yes, a fault causes an abort, but no fault is ever observed by
software. Though I don't quite understand why helpers are needed in the first
place.
> +{
> + int value = 0;
> +
> + ptr = NULL;
> + if(_xbegin() == _XBEGIN_STARTED) {
Space after the "if".
> + value++;
> + // causes abort
> + *ptr = value;
/* Generate a non-canonical #GP to trigger ABORT. */
(int *)NONCANONICAL) = 0;
> + _xend();
Why bother with XEND?
> + }
> +}
> +
> +static void tsx_normal(void)
> +{
> + int value = 0;
> +
> + if(_xbegin() == _XBEGIN_STARTED) {
> + value++;
What's the purpose of incrementing an arbitrary value?
> + _xend();
Does this test rely on the region being successfully committed? If so, the test
is guaranteed to be flaky, e.g. due to a host IRQ at the "wrong" time. Assuming
success is not required, please add a comment describing the requirements.
> + }
> +}
> +
> +static void check_tsx_cycles(void)
> +{
> + pmu_counter_t cnt;
> + int i;
> +
> + if (!this_cpu_has(X86_FEATURE_RTM) || !this_cpu_has(X86_FEATURE_HLE))
> + return;
> +
> + report_prefix_push("TSX cycles");
> +
> + for (i = 0; i < pmu.nr_gp_counters; i++) {
> + cnt.ctr = MSR_GP_COUNTERx(i);
> +
> + if (i == 2)
> + /* Transactional cycles commited only on gp counter 2 */
> + cnt.config = EVNTSEL_OS | EVNTSEL_USR | 0x30000003c;
> + else
> + /* Transactional cycles */
> + cnt.config = EVNTSEL_OS | EVNTSEL_USR | 0x10000003c;
> +
> + start_event(&cnt);
> + tsx_fault();
> + tsx_normal();
As a above, why bother with helpers? Can't this just be:
start_event(&cnt);
/* Generate a non-canonical #GP to trigger ABORT. */
if (xbegin() == XBEGIN_STARTED)
*(int *)NONCANONICAL = 0;
/* <comment about what requirements of this code> */
if (xbegin() == XBEGIN_STARTED)
xend();
stop_event(&cnt);
> + stop_event(&cnt);
> +
> + report(cnt.count > 0, "gp cntr-%d", i);
> + }
> +
> + report_prefix_pop();
> +}
> +
> static void check_counters(void)
> {
> if (is_fep_available())
> @@ -559,6 +629,7 @@ static void check_counters(void)
> check_counter_overflow();
> check_gp_counter_cmask();
> check_running_counter_wrmsr();
> + check_tsx_cycles();
> }
>
> static void do_unsupported_width_counter_write(void *index)
> --
> 2.38.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread