All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU
@ 2021-03-05 22:33 Sean Christopherson
  2021-03-08  2:25 ` Xu, Like
  0 siblings, 1 reply; 12+ messages in thread
From: Sean Christopherson @ 2021-03-05 22:33 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Thomas Gleixner, Borislav Petkov, x86
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	H. Peter Anvin, linux-kernel, Like Xu, Paolo Bonzini,
	Jim Mattson, kvm, Dmitry Vyukov, Sean Christopherson

Handle a NULL x86_pmu.guest_get_msrs at invocation instead of patching
in perf_guest_get_msrs_nop() during setup.  If there is no PMU, setup
bails before updating the static calls, leaving x86_pmu.guest_get_msrs
NULL and thus a complete nop.  Ultimately, this causes VMX abort on
VM-Exit due to KVM putting random garbage from the stack into the MSR
load list.

Fixes: abd562df94d1 ("x86/perf: Use static_call for x86_pmu.guest_get_msrs")
Cc: Like Xu <like.xu@linux.intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Jim Mattson <jmattson@google.com>
Cc: kvm@vger.kernel.org
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/events/core.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 6ddeed3cd2ac..ff874461f14c 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -671,7 +671,11 @@ void x86_pmu_disable_all(void)
 
 struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
 {
-	return static_call(x86_pmu_guest_get_msrs)(nr);
+	if (x86_pmu.guest_get_msrs)
+		return static_call(x86_pmu_guest_get_msrs)(nr);
+
+	*nr = 0;
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(perf_guest_get_msrs);
 
@@ -1944,13 +1948,6 @@ static void _x86_pmu_read(struct perf_event *event)
 	x86_perf_event_update(event);
 }
 
-static inline struct perf_guest_switch_msr *
-perf_guest_get_msrs_nop(int *nr)
-{
-	*nr = 0;
-	return NULL;
-}
-
 static int __init init_hw_perf_events(void)
 {
 	struct x86_pmu_quirk *quirk;
@@ -2024,9 +2021,6 @@ static int __init init_hw_perf_events(void)
 	if (!x86_pmu.read)
 		x86_pmu.read = _x86_pmu_read;
 
-	if (!x86_pmu.guest_get_msrs)
-		x86_pmu.guest_get_msrs = perf_guest_get_msrs_nop;
-
 	x86_pmu_static_call_update();
 
 	/*
-- 
2.30.1.766.gb4fecdf3b7-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU
  2021-03-05 22:33 [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU Sean Christopherson
@ 2021-03-08  2:25 ` Xu, Like
  2021-03-08  7:12   ` Dmitry Vyukov
  2021-03-08  8:53   ` Peter Zijlstra
  0 siblings, 2 replies; 12+ messages in thread
From: Xu, Like @ 2021-03-08  2:25 UTC (permalink / raw)
  To: Sean Christopherson, Peter Zijlstra, Dmitry Vyukov
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	H. Peter Anvin, linux-kernel, Like Xu, Paolo Bonzini,
	Jim Mattson, kvm, Dmitry Vyukov,
	Thomas Gleixner
	(x86/pti/timer/core/smp/irq/perf/efi/locking/ras/objtool)
	(x86@kernel.org),
	Borislav Petkov, Thomas Gleixner, Arnaldo Carvalho de Melo,
	Ingo Molnar

On 2021/3/6 6:33, Sean Christopherson wrote:
> Handle a NULL x86_pmu.guest_get_msrs at invocation instead of patching
> in perf_guest_get_msrs_nop() during setup.  If there is no PMU, setup

"If there is no PMU" ...

How to set up this kind of environment,
and what changes are needed in .config or boot parameters ?

> bails before updating the static calls, leaving x86_pmu.guest_get_msrs
> NULL and thus a complete nop.

> Ultimately, this causes VMX abort on
> VM-Exit due to KVM putting random garbage from the stack into the MSR
> load list.
>
> Fixes: abd562df94d1 ("x86/perf: Use static_call for x86_pmu.guest_get_msrs")
> Cc: Like Xu <like.xu@linux.intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Jim Mattson <jmattson@google.com>
> Cc: kvm@vger.kernel.org
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/events/core.c | 16 +++++-----------
>   1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 6ddeed3cd2ac..ff874461f14c 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -671,7 +671,11 @@ void x86_pmu_disable_all(void)
>   
>   struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
>   {
> -	return static_call(x86_pmu_guest_get_msrs)(nr);
> +	if (x86_pmu.guest_get_msrs)
> +		return static_call(x86_pmu_guest_get_msrs)(nr);

How about using "static_call_cond" per commit "452cddbff7" ?

> +
> +	*nr = 0;
> +	return NULL;
>   }
>   EXPORT_SYMBOL_GPL(perf_guest_get_msrs);
>   
> @@ -1944,13 +1948,6 @@ static void _x86_pmu_read(struct perf_event *event)
>   	x86_perf_event_update(event);
>   }
>   
> -static inline struct perf_guest_switch_msr *
> -perf_guest_get_msrs_nop(int *nr)
> -{
> -	*nr = 0;
> -	return NULL;
> -}
> -
>   static int __init init_hw_perf_events(void)
>   {
>   	struct x86_pmu_quirk *quirk;
> @@ -2024,9 +2021,6 @@ static int __init init_hw_perf_events(void)
>   	if (!x86_pmu.read)
>   		x86_pmu.read = _x86_pmu_read;
>   
> -	if (!x86_pmu.guest_get_msrs)
> -		x86_pmu.guest_get_msrs = perf_guest_get_msrs_nop;
> -
>   	x86_pmu_static_call_update();
>   
>   	/*


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU
  2021-03-08  2:25 ` Xu, Like
@ 2021-03-08  7:12   ` Dmitry Vyukov
  2021-03-08  8:35     ` Like Xu
  2021-03-08  8:53   ` Peter Zijlstra
  1 sibling, 1 reply; 12+ messages in thread
From: Dmitry Vyukov @ 2021-03-08  7:12 UTC (permalink / raw)
  To: Xu, Like
  Cc: Sean Christopherson, Peter Zijlstra, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Namhyung Kim, H. Peter Anvin,
	LKML, Like Xu, Paolo Bonzini, Jim Mattson, KVM list,
	Thomas Gleixner
	(x86/pti/timer/core/smp/irq/perf/efi/locking/ras/objtool)
	(x86@kernel.org),
	Borislav Petkov, Arnaldo Carvalho de Melo, Ingo Molnar

On Mon, Mar 8, 2021 at 3:26 AM Xu, Like <like.xu@intel.com> wrote:
>
> On 2021/3/6 6:33, Sean Christopherson wrote:
> > Handle a NULL x86_pmu.guest_get_msrs at invocation instead of patching
> > in perf_guest_get_msrs_nop() during setup.  If there is no PMU, setup
>
> "If there is no PMU" ...
>
> How to set up this kind of environment,
> and what changes are needed in .config or boot parameters ?

Hi Xu,

This can be reproduced in qemu with "-cpu max,-pmu" flag using this reproducer:
https://groups.google.com/g/syzkaller-bugs/c/D8eHw3LIOd0/m/L2G0lVkVBAAJ

> > bails before updating the static calls, leaving x86_pmu.guest_get_msrs
> > NULL and thus a complete nop.
>
> > Ultimately, this causes VMX abort on
> > VM-Exit due to KVM putting random garbage from the stack into the MSR
> > load list.
> >
> > Fixes: abd562df94d1 ("x86/perf: Use static_call for x86_pmu.guest_get_msrs")
> > Cc: Like Xu <like.xu@linux.intel.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Jim Mattson <jmattson@google.com>
> > Cc: kvm@vger.kernel.org
> > Reported-by: Dmitry Vyukov <dvyukov@google.com>
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >   arch/x86/events/core.c | 16 +++++-----------
> >   1 file changed, 5 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> > index 6ddeed3cd2ac..ff874461f14c 100644
> > --- a/arch/x86/events/core.c
> > +++ b/arch/x86/events/core.c
> > @@ -671,7 +671,11 @@ void x86_pmu_disable_all(void)
> >
> >   struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
> >   {
> > -     return static_call(x86_pmu_guest_get_msrs)(nr);
> > +     if (x86_pmu.guest_get_msrs)
> > +             return static_call(x86_pmu_guest_get_msrs)(nr);
>
> How about using "static_call_cond" per commit "452cddbff7" ?
>
> > +
> > +     *nr = 0;
> > +     return NULL;
> >   }
> >   EXPORT_SYMBOL_GPL(perf_guest_get_msrs);
> >
> > @@ -1944,13 +1948,6 @@ static void _x86_pmu_read(struct perf_event *event)
> >       x86_perf_event_update(event);
> >   }
> >
> > -static inline struct perf_guest_switch_msr *
> > -perf_guest_get_msrs_nop(int *nr)
> > -{
> > -     *nr = 0;
> > -     return NULL;
> > -}
> > -
> >   static int __init init_hw_perf_events(void)
> >   {
> >       struct x86_pmu_quirk *quirk;
> > @@ -2024,9 +2021,6 @@ static int __init init_hw_perf_events(void)
> >       if (!x86_pmu.read)
> >               x86_pmu.read = _x86_pmu_read;
> >
> > -     if (!x86_pmu.guest_get_msrs)
> > -             x86_pmu.guest_get_msrs = perf_guest_get_msrs_nop;
> > -
> >       x86_pmu_static_call_update();
> >
> >       /*
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU
  2021-03-08  7:12   ` Dmitry Vyukov
@ 2021-03-08  8:35     ` Like Xu
  2021-03-08  8:51       ` Dmitry Vyukov
  0 siblings, 1 reply; 12+ messages in thread
From: Like Xu @ 2021-03-08  8:35 UTC (permalink / raw)
  To: Dmitry Vyukov, Xu, Like
  Cc: Sean Christopherson, Peter Zijlstra, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Namhyung Kim, H. Peter Anvin,
	LKML, Paolo Bonzini, Jim Mattson, KVM list,
	Thomas Gleixner
	(x86/pti/timer/core/smp/irq/perf/efi/locking/ras/objtool)
	(x86@kernel.org),
	Borislav Petkov, Arnaldo Carvalho de Melo, Ingo Molnar

On 2021/3/8 15:12, Dmitry Vyukov wrote:
> On Mon, Mar 8, 2021 at 3:26 AM Xu, Like <like.xu@intel.com> wrote:
>>
>> On 2021/3/6 6:33, Sean Christopherson wrote:
>>> Handle a NULL x86_pmu.guest_get_msrs at invocation instead of patching
>>> in perf_guest_get_msrs_nop() during setup.  If there is no PMU, setup
>>
>> "If there is no PMU" ...
>>
>> How to set up this kind of environment,
>> and what changes are needed in .config or boot parameters ?
> 
> Hi Xu,
> 
> This can be reproduced in qemu with "-cpu max,-pmu" flag using this reproducer:
> https://groups.google.com/g/syzkaller-bugs/c/D8eHw3LIOd0/m/L2G0lVkVBAAJ

Sorry, I couldn't reproduce any VMX abort with "-cpu max,-pmu".
Doe this patch fix this "unexpected kernel reboot" issue ?

If so, you may add "Tested-by" for more attention.

> 
>>> bails before updating the static calls, leaving x86_pmu.guest_get_msrs
>>> NULL and thus a complete nop.
>>
>>> Ultimately, this causes VMX abort on
>>> VM-Exit due to KVM putting random garbage from the stack into the MSR
>>> load list.
>>>
>>> Fixes: abd562df94d1 ("x86/perf: Use static_call for x86_pmu.guest_get_msrs")
>>> Cc: Like Xu <like.xu@linux.intel.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Jim Mattson <jmattson@google.com>
>>> Cc: kvm@vger.kernel.org
>>> Reported-by: Dmitry Vyukov <dvyukov@google.com>
>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>> ---
>>>    arch/x86/events/core.c | 16 +++++-----------
>>>    1 file changed, 5 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
>>> index 6ddeed3cd2ac..ff874461f14c 100644
>>> --- a/arch/x86/events/core.c
>>> +++ b/arch/x86/events/core.c
>>> @@ -671,7 +671,11 @@ void x86_pmu_disable_all(void)
>>>
>>>    struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
>>>    {
>>> -     return static_call(x86_pmu_guest_get_msrs)(nr);
>>> +     if (x86_pmu.guest_get_msrs)
>>> +             return static_call(x86_pmu_guest_get_msrs)(nr);
>>
>> How about using "static_call_cond" per commit "452cddbff7" ?
>>
>>> +
>>> +     *nr = 0;
>>> +     return NULL;
>>>    }
>>>    EXPORT_SYMBOL_GPL(perf_guest_get_msrs);
>>>
>>> @@ -1944,13 +1948,6 @@ static void _x86_pmu_read(struct perf_event *event)
>>>        x86_perf_event_update(event);
>>>    }
>>>
>>> -static inline struct perf_guest_switch_msr *
>>> -perf_guest_get_msrs_nop(int *nr)
>>> -{
>>> -     *nr = 0;
>>> -     return NULL;
>>> -}
>>> -
>>>    static int __init init_hw_perf_events(void)
>>>    {
>>>        struct x86_pmu_quirk *quirk;
>>> @@ -2024,9 +2021,6 @@ static int __init init_hw_perf_events(void)
>>>        if (!x86_pmu.read)
>>>                x86_pmu.read = _x86_pmu_read;
>>>
>>> -     if (!x86_pmu.guest_get_msrs)
>>> -             x86_pmu.guest_get_msrs = perf_guest_get_msrs_nop;
>>> -
>>>        x86_pmu_static_call_update();
>>>
>>>        /*
>>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU
  2021-03-08  8:35     ` Like Xu
@ 2021-03-08  8:51       ` Dmitry Vyukov
  0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Vyukov @ 2021-03-08  8:51 UTC (permalink / raw)
  To: Like Xu
  Cc: Xu, Like, Sean Christopherson, Peter Zijlstra, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Namhyung Kim, H. Peter Anvin,
	LKML, Paolo Bonzini, Jim Mattson, KVM list,
	Thomas Gleixner
	(x86/pti/timer/core/smp/irq/perf/efi/locking/ras/objtool)
	(x86@kernel.org),
	Borislav Petkov, Arnaldo Carvalho de Melo, Ingo Molnar

On Mon, Mar 8, 2021 at 9:35 AM Like Xu <like.xu@linux.intel.com> wrote:
>
> On 2021/3/8 15:12, Dmitry Vyukov wrote:
> > On Mon, Mar 8, 2021 at 3:26 AM Xu, Like <like.xu@intel.com> wrote:
> >>
> >> On 2021/3/6 6:33, Sean Christopherson wrote:
> >>> Handle a NULL x86_pmu.guest_get_msrs at invocation instead of patching
> >>> in perf_guest_get_msrs_nop() during setup.  If there is no PMU, setup
> >>
> >> "If there is no PMU" ...
> >>
> >> How to set up this kind of environment,
> >> and what changes are needed in .config or boot parameters ?
> >
> > Hi Xu,
> >
> > This can be reproduced in qemu with "-cpu max,-pmu" flag using this reproducer:
> > https://groups.google.com/g/syzkaller-bugs/c/D8eHw3LIOd0/m/L2G0lVkVBAAJ
>
> Sorry, I couldn't reproduce any VMX abort with "-cpu max,-pmu".
> Doe this patch fix this "unexpected kernel reboot" issue ?
>
> If so, you may add "Tested-by" for more attention.

There is an uninit involved. For me it crashed reliably when kernel
compiled with clang 11, but with gcc it worked most of the time.
You may try to add something like:

--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6581,6 +6581,7 @@ static void atomic_switch_perf_msrs(struct vcpu_vmx *vmx)
        struct perf_guest_switch_msr *msrs;

+      nr_msrs = 12345678;
        msrs = perf_guest_get_msrs(&nr_msrs);
+       pr_err("atomic_switch_perf_msrs: msrs=%px nr_msrs=%d\n", msrs, nr_msrs);

Then you will see surprising things.


> >>> bails before updating the static calls, leaving x86_pmu.guest_get_msrs
> >>> NULL and thus a complete nop.
> >>
> >>> Ultimately, this causes VMX abort on
> >>> VM-Exit due to KVM putting random garbage from the stack into the MSR
> >>> load list.
> >>>
> >>> Fixes: abd562df94d1 ("x86/perf: Use static_call for x86_pmu.guest_get_msrs")
> >>> Cc: Like Xu <like.xu@linux.intel.com>
> >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>> Cc: Jim Mattson <jmattson@google.com>
> >>> Cc: kvm@vger.kernel.org
> >>> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> >>> Signed-off-by: Sean Christopherson <seanjc@google.com>
> >>> ---
> >>>    arch/x86/events/core.c | 16 +++++-----------
> >>>    1 file changed, 5 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> >>> index 6ddeed3cd2ac..ff874461f14c 100644
> >>> --- a/arch/x86/events/core.c
> >>> +++ b/arch/x86/events/core.c
> >>> @@ -671,7 +671,11 @@ void x86_pmu_disable_all(void)
> >>>
> >>>    struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
> >>>    {
> >>> -     return static_call(x86_pmu_guest_get_msrs)(nr);
> >>> +     if (x86_pmu.guest_get_msrs)
> >>> +             return static_call(x86_pmu_guest_get_msrs)(nr);
> >>
> >> How about using "static_call_cond" per commit "452cddbff7" ?
> >>
> >>> +
> >>> +     *nr = 0;
> >>> +     return NULL;
> >>>    }
> >>>    EXPORT_SYMBOL_GPL(perf_guest_get_msrs);
> >>>
> >>> @@ -1944,13 +1948,6 @@ static void _x86_pmu_read(struct perf_event *event)
> >>>        x86_perf_event_update(event);
> >>>    }
> >>>
> >>> -static inline struct perf_guest_switch_msr *
> >>> -perf_guest_get_msrs_nop(int *nr)
> >>> -{
> >>> -     *nr = 0;
> >>> -     return NULL;
> >>> -}
> >>> -
> >>>    static int __init init_hw_perf_events(void)
> >>>    {
> >>>        struct x86_pmu_quirk *quirk;
> >>> @@ -2024,9 +2021,6 @@ static int __init init_hw_perf_events(void)
> >>>        if (!x86_pmu.read)
> >>>                x86_pmu.read = _x86_pmu_read;
> >>>
> >>> -     if (!x86_pmu.guest_get_msrs)
> >>> -             x86_pmu.guest_get_msrs = perf_guest_get_msrs_nop;
> >>> -
> >>>        x86_pmu_static_call_update();
> >>>
> >>>        /*
> >>
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU
  2021-03-08  2:25 ` Xu, Like
  2021-03-08  7:12   ` Dmitry Vyukov
@ 2021-03-08  8:53   ` Peter Zijlstra
  2021-03-08 12:01     ` Xu, Like
  2021-03-08 20:40     ` Sean Christopherson
  1 sibling, 2 replies; 12+ messages in thread
From: Peter Zijlstra @ 2021-03-08  8:53 UTC (permalink / raw)
  To: Xu, Like
  Cc: Sean Christopherson, Dmitry Vyukov, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Namhyung Kim, H. Peter Anvin,
	linux-kernel, Like Xu, Paolo Bonzini, Jim Mattson, kvm,
	Thomas Gleixner
	(x86/pti/timer/core/smp/irq/perf/efi/locking/ras/objtool)
	(x86@kernel.org),
	Borislav Petkov, Arnaldo Carvalho de Melo, Ingo Molnar

On Mon, Mar 08, 2021 at 10:25:59AM +0800, Xu, Like wrote:
> On 2021/3/6 6:33, Sean Christopherson wrote:
> > Handle a NULL x86_pmu.guest_get_msrs at invocation instead of patching
> > in perf_guest_get_msrs_nop() during setup.  If there is no PMU, setup
> 
> "If there is no PMU" ...

Then you shouldn't be calling this either ofcourse :-)

> > @@ -671,7 +671,11 @@ void x86_pmu_disable_all(void)
> >   struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
> >   {
> > -	return static_call(x86_pmu_guest_get_msrs)(nr);
> > +	if (x86_pmu.guest_get_msrs)
> > +		return static_call(x86_pmu_guest_get_msrs)(nr);
> 
> How about using "static_call_cond" per commit "452cddbff7" ?

Given the one user in atomic_switch_perf_msrs() that should work because
it doesn't seem to care about nr_msrs when !msrs.

Still, it calling atomic_switch_perf_msrs() and
intel_pmu_lbr_is_enabled() when there isn't a PMU at all is of course, a
complete waste of cycles.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU
  2021-03-08  8:53   ` Peter Zijlstra
@ 2021-03-08 12:01     ` Xu, Like
  2021-03-08 20:40     ` Sean Christopherson
  1 sibling, 0 replies; 12+ messages in thread
From: Xu, Like @ 2021-03-08 12:01 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Sean Christopherson, Dmitry Vyukov, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Namhyung Kim, H. Peter Anvin,
	linux-kernel, Like Xu, Paolo Bonzini, Jim Mattson, kvm,
	Thomas Gleixner
	(x86/pti/timer/core/smp/irq/perf/efi/locking/ras/objtool)
	(x86@kernel.org),
	Borislav Petkov, Arnaldo Carvalho de Melo, Ingo Molnar

On 2021/3/8 16:53, Peter Zijlstra wrote:
> Still, it calling atomic_switch_perf_msrs() and
> intel_pmu_lbr_is_enabled() when there isn't a PMU at all is of course, a
> complete waste of cycles.

This suggestion is reminiscent of a sad regression of optimizing it:

https://lore.kernel.org/kvm/20200619094046.654019-1-vkuznets@redhat.com/
https://lore.kernel.org/kvm/20210209225653.1393771-1-jmattson@google.com/

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU
  2021-03-08  8:53   ` Peter Zijlstra
  2021-03-08 12:01     ` Xu, Like
@ 2021-03-08 20:40     ` Sean Christopherson
  2021-03-09  7:46       ` Peter Zijlstra
  1 sibling, 1 reply; 12+ messages in thread
From: Sean Christopherson @ 2021-03-08 20:40 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Xu, Like, Dmitry Vyukov, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, H. Peter Anvin, linux-kernel, Like Xu,
	Paolo Bonzini, Jim Mattson, kvm,
	Thomas Gleixner
	(x86/pti/timer/core/smp/irq/perf/efi/locking/ras/objtool)
	(x86@kernel.org),
	Borislav Petkov, Arnaldo Carvalho de Melo, Ingo Molnar

On Mon, Mar 08, 2021, Peter Zijlstra wrote:
> On Mon, Mar 08, 2021 at 10:25:59AM +0800, Xu, Like wrote:
> > On 2021/3/6 6:33, Sean Christopherson wrote:
> > > Handle a NULL x86_pmu.guest_get_msrs at invocation instead of patching
> > > in perf_guest_get_msrs_nop() during setup.  If there is no PMU, setup
> > 
> > "If there is no PMU" ...
> 
> Then you shouldn't be calling this either ofcourse :-)

This effectively is KVM's check to find out there is no PMU.  I certainly don't
want to replicate the switch statement in init_hw_perf_events(), plus whatever
is buried in check_hw_exists().  The alternative would be to add X86_FEATURE_PMU
so that KVM can easily check for PMU existence.  I don't really see the point
though, as bare metal KVM, where we really care about performance, is likely to
have a functional PMU, and if it doesn't then I doubt whoever is running KVM
cares much about performance.

> > > @@ -671,7 +671,11 @@ void x86_pmu_disable_all(void)
> > >   struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
> > >   {
> > > -	return static_call(x86_pmu_guest_get_msrs)(nr);
> > > +	if (x86_pmu.guest_get_msrs)
> > > +		return static_call(x86_pmu_guest_get_msrs)(nr);
> > 
> > How about using "static_call_cond" per commit "452cddbff7" ?
> 
> Given the one user in atomic_switch_perf_msrs() that should work because
> it doesn't seem to care about nr_msrs when !msrs.

Uh, that commit quite cleary says:

   NOTE: this is 'obviously' limited to functions with a 'void' return type.

Even if we somehow bypass the (void) cast, IIUC it will compile to a single
'ret',  and return whatever happens to be in RAX, not NULL as is needed.

> Still, it calling atomic_switch_perf_msrs() and
> intel_pmu_lbr_is_enabled() when there isn't a PMU at all is of course, a

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU
  2021-03-08 20:40     ` Sean Christopherson
@ 2021-03-09  7:46       ` Peter Zijlstra
  2021-03-09  8:45         ` Peter Zijlstra
  0 siblings, 1 reply; 12+ messages in thread
From: Peter Zijlstra @ 2021-03-09  7:46 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Xu, Like, Dmitry Vyukov, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, H. Peter Anvin, linux-kernel, Like Xu,
	Paolo Bonzini, Jim Mattson, kvm,
	Thomas Gleixner
	(x86/pti/timer/core/smp/irq/perf/efi/locking/ras/objtool)
	(x86@kernel.org),
	Borislav Petkov, Arnaldo Carvalho de Melo, Ingo Molnar

On Mon, Mar 08, 2021 at 12:40:44PM -0800, Sean Christopherson wrote:
> On Mon, Mar 08, 2021, Peter Zijlstra wrote:

> > Given the one user in atomic_switch_perf_msrs() that should work because
> > it doesn't seem to care about nr_msrs when !msrs.
> 
> Uh, that commit quite cleary says:

D0h! I got static_call_cond() and __static_call_return0 mixed up.
Anyway, let me see if I can make something work here.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU
  2021-03-09  7:46       ` Peter Zijlstra
@ 2021-03-09  8:45         ` Peter Zijlstra
  2021-03-09 17:05           ` Sean Christopherson
  0 siblings, 1 reply; 12+ messages in thread
From: Peter Zijlstra @ 2021-03-09  8:45 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Xu, Like, Dmitry Vyukov, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, H. Peter Anvin, linux-kernel, Like Xu,
	Paolo Bonzini, Jim Mattson, kvm,
	Thomas Gleixner
	(x86/pti/timer/core/smp/irq/perf/efi/locking/ras/objtool)
	(x86@kernel.org),
	Borislav Petkov, Arnaldo Carvalho de Melo, Ingo Molnar

On Tue, Mar 09, 2021 at 08:46:49AM +0100, Peter Zijlstra wrote:
> On Mon, Mar 08, 2021 at 12:40:44PM -0800, Sean Christopherson wrote:
> > On Mon, Mar 08, 2021, Peter Zijlstra wrote:
> 
> > > Given the one user in atomic_switch_perf_msrs() that should work because
> > > it doesn't seem to care about nr_msrs when !msrs.
> > 
> > Uh, that commit quite cleary says:
> 
> D0h! I got static_call_cond() and __static_call_return0 mixed up.
> Anyway, let me see if I can make something work here.

Does this work? I can never seem to start a VM, and if I do accidentally
manage, then it never contains the things I need :/

---
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 6ddeed3cd2ac..fadcecd73e1a 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -81,7 +81,11 @@ DEFINE_STATIC_CALL_NULL(x86_pmu_swap_task_ctx, *x86_pmu.swap_task_ctx);
 DEFINE_STATIC_CALL_NULL(x86_pmu_drain_pebs,   *x86_pmu.drain_pebs);
 DEFINE_STATIC_CALL_NULL(x86_pmu_pebs_aliases, *x86_pmu.pebs_aliases);
 
-DEFINE_STATIC_CALL_NULL(x86_pmu_guest_get_msrs,  *x86_pmu.guest_get_msrs);
+/*
+ * This one is magic, it will get called even when PMU init fails (because
+ * there is no PMU), in which case it should simply return NULL.
+ */
+__DEFINE_STATIC_CALL(x86_pmu_guest_get_msrs, *x86_pmu.guest_get_msrs, __static_call_return0);
 
 u64 __read_mostly hw_cache_event_ids
 				[PERF_COUNT_HW_CACHE_MAX]
@@ -1944,13 +1948,6 @@ static void _x86_pmu_read(struct perf_event *event)
 	x86_perf_event_update(event);
 }
 
-static inline struct perf_guest_switch_msr *
-perf_guest_get_msrs_nop(int *nr)
-{
-	*nr = 0;
-	return NULL;
-}
-
 static int __init init_hw_perf_events(void)
 {
 	struct x86_pmu_quirk *quirk;
@@ -2024,9 +2021,6 @@ static int __init init_hw_perf_events(void)
 	if (!x86_pmu.read)
 		x86_pmu.read = _x86_pmu_read;
 
-	if (!x86_pmu.guest_get_msrs)
-		x86_pmu.guest_get_msrs = perf_guest_get_msrs_nop;
-
 	x86_pmu_static_call_update();
 
 	/*

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU
  2021-03-09  8:45         ` Peter Zijlstra
@ 2021-03-09 17:05           ` Sean Christopherson
  2021-03-09 17:07             ` Dmitry Vyukov
  0 siblings, 1 reply; 12+ messages in thread
From: Sean Christopherson @ 2021-03-09 17:05 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Xu, Like, Dmitry Vyukov, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, H. Peter Anvin, linux-kernel, Like Xu,
	Paolo Bonzini, Jim Mattson, kvm,
	Thomas Gleixner
	(x86/pti/timer/core/smp/irq/perf/efi/locking/ras/objtool)
	(x86@kernel.org),
	Borislav Petkov, Arnaldo Carvalho de Melo, Ingo Molnar

On Tue, Mar 09, 2021, Peter Zijlstra wrote:
> On Tue, Mar 09, 2021 at 08:46:49AM +0100, Peter Zijlstra wrote:
> > On Mon, Mar 08, 2021 at 12:40:44PM -0800, Sean Christopherson wrote:
> > > On Mon, Mar 08, 2021, Peter Zijlstra wrote:
> > 
> > > > Given the one user in atomic_switch_perf_msrs() that should work because
> > > > it doesn't seem to care about nr_msrs when !msrs.
> > > 
> > > Uh, that commit quite cleary says:
> > 
> > D0h! I got static_call_cond() and __static_call_return0 mixed up.
> > Anyway, let me see if I can make something work here.
> 
> Does this work? I can never seem to start a VM, and if I do accidentally
> manage, then it never contains the things I need :/

Yep, once I found the dependencies in tip/sched/core (thank tip-bot!).  I'll
send v2 your way.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU
  2021-03-09 17:05           ` Sean Christopherson
@ 2021-03-09 17:07             ` Dmitry Vyukov
  0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Vyukov @ 2021-03-09 17:07 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Peter Zijlstra, Xu, Like, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, H. Peter Anvin, LKML, Like Xu,
	Paolo Bonzini, Jim Mattson, KVM list,
	Thomas Gleixner
	(x86/pti/timer/core/smp/irq/perf/efi/locking/ras/objtool)
	(x86@kernel.org),
	Borislav Petkov, Arnaldo Carvalho de Melo, Ingo Molnar

On Tue, Mar 9, 2021 at 6:05 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Mar 09, 2021, Peter Zijlstra wrote:
> > On Tue, Mar 09, 2021 at 08:46:49AM +0100, Peter Zijlstra wrote:
> > > On Mon, Mar 08, 2021 at 12:40:44PM -0800, Sean Christopherson wrote:
> > > > On Mon, Mar 08, 2021, Peter Zijlstra wrote:
> > >
> > > > > Given the one user in atomic_switch_perf_msrs() that should work because
> > > > > it doesn't seem to care about nr_msrs when !msrs.
> > > >
> > > > Uh, that commit quite cleary says:
> > >
> > > D0h! I got static_call_cond() and __static_call_return0 mixed up.
> > > Anyway, let me see if I can make something work here.
> >
> > Does this work? I can never seem to start a VM, and if I do accidentally
> > manage, then it never contains the things I need :/
>
> Yep, once I found the dependencies in tip/sched/core (thank tip-bot!).  I'll
> send v2 your way.

If you are resending, please also add the syzbot Reported-by tag. Thanks.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-03-09 17:08 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-05 22:33 [PATCH] x86/perf: Fix guest_get_msrs static call if there is no PMU Sean Christopherson
2021-03-08  2:25 ` Xu, Like
2021-03-08  7:12   ` Dmitry Vyukov
2021-03-08  8:35     ` Like Xu
2021-03-08  8:51       ` Dmitry Vyukov
2021-03-08  8:53   ` Peter Zijlstra
2021-03-08 12:01     ` Xu, Like
2021-03-08 20:40     ` Sean Christopherson
2021-03-09  7:46       ` Peter Zijlstra
2021-03-09  8:45         ` Peter Zijlstra
2021-03-09 17:05           ` Sean Christopherson
2021-03-09 17:07             ` Dmitry Vyukov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.