[PATCH 0/3 v2] perf: precise mode and exclude_guest

[PATCH 0/3 v2] perf: precise mode and exclude_guest

[David Ahern]

Hopefully this wraps up the precise mode-exclude_guest dependency.
I'm sure someone will let me know if I screwed up the attribution
in the second patch.

David Ahern (2):
  perf tool: precise mode requires exclude_guest
  perf tool: give user better message if precise is not supported

Peter Zijlstra (1):
  perf: require exclude_guest to use PEBS - kernel side enforcement

v2: updated commit messages for patches 1 and 2

 arch/x86/kernel/cpu/perf_event.c |    6 ++++++
 tools/perf/builtin-record.c      |    5 +++++
 tools/perf/builtin-top.c         |    4 ++++
 tools/perf/util/parse-events.c   |    3 +++
 4 files changed, 18 insertions(+)

-- 
1.7.10.1

[PATCH 1/3] perf tool: precise mode requires exclude_guest

[David Ahern]

Summary of events per Peter:

  "Intel PEBS in VT-x context uses the DS address as a guest linear address,
  even though its programmed by the host as a host linear address. This
  either results in guest memory corruption and or the hardware faulting and
  'crashing' the virtual machine.  Therefore we have to disable PEBS on VT-x
  enter and re-enable on VT-x exit, enforcing a strict exclude_guest.

  AMB IBS does work but doesn't currently support exclude_* at all,
  setting an exclude_* bit will make it fail."

This patch handles userspace perf command, setting the exclude_guest
attribute if precise mode is requested, but only if a user has not
specified a request for guest or host only profiling (G or H attribute).

Kernel side AMD currently ignores all exclude_* bits, so there is no impact
to existing IBS code paths. Robert Richter has a patch where IBS code will
return EINVAL if an exclude_* bit is set. When this goes in it means use
of :p on AMD with IBS will first fail with EINVAL (because exclude_guest
will be set). Then the existing fallback code within perf will unset
exclude_guest and try again. The second attempt will succeed if the CPU
supports IBS profiling.

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Robert Richter <robert.richter@amd.com>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: Avi Kivity <avi@redhat.com>
Link: https://lkml.org/lkml/2012/7/9/264
---
 tools/perf/util/parse-events.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 44afcf4..696cc7e 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -694,6 +694,9 @@ static int get_event_modifier(struct event_modifier *mod, char *str,
 			eH = 0;
 		} else if (*str == 'p') {
 			precise++;
+			/* use of precise requires exclude_guest */
+			if (!exclude_GH)
+				eG = 1;
 		} else
 			break;
 
-- 
1.7.10.1

[PATCH 2/3] perf: require exclude_guest to use PEBS - kernel side enforcement

[David Ahern]

From: Peter Zijlstra <peterz@infradead.org>

Per Peter:
  "Intel PEBS in VT-x context uses the DS address as a guest linear address,
  even though its programmed by the host as a host linear address. This
  either results in guest memory corruption and or the hardware faulting and
  'crashing' the virtual machine.  Therefore we have to disable PEBS on VT-x
  enter and re-enable on VT-x exit, enforcing a strict exclude_guest.

This patch enforces exclude_guest kernel side.

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Robert Richter <robert.richter@amd.com>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: Avi Kivity <avi@redhat.com>
Link: https://lkml.org/lkml/2012/7/9/298
---
 arch/x86/kernel/cpu/perf_event.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 915b876..4bc96c5 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -338,6 +338,9 @@ int x86_setup_perfctr(struct perf_event *event)
 		/* BTS is currently only allowed for user-mode. */
 		if (!attr->exclude_kernel)
 			return -EOPNOTSUPP;
+
+		if (!attr->exclude_guest)
+			return -EOPNOTSUPP;
 	}
 
 	hwc->config |= config;
@@ -380,6 +383,9 @@ int x86_pmu_hw_config(struct perf_event *event)
 	if (event->attr.precise_ip) {
 		int precise = 0;
 
+		if (!event->attr.exclude_guest)
+				return -EOPNOTSUPP;
+
 		/* Support for constant skid */
 		if (x86_pmu.pebs_active && !x86_pmu.pebs_broken) {
 			precise++;
-- 
1.7.10.1

[PATCH 3/3 v2] perf tool: give user better message if precise is not supported

[David Ahern]

Platforms (e.g., VM's) without support for precise mode get a confusing
error message. e.g.,
$ perf record -e cycles:p -a -- sleep 1

  Error: sys_perf_event_open() syscall returned with 95 (Operation not
  supported).  /bin/dmesg may provide additional information.

  No hardware sampling interrupt available. No APIC? If so then you can
  boot the kernel with the "lapic" boot parameter to force-enable it.
  sleep: Terminated

which is not clear that precise mode might be the root problem. With this
patch:

$ perf record -e cycles:p -fo /tmp/perf.data -- sleep 1
  Error:
  'precise' request may not be supported. Try removing 'p' modifier
  sleep: Terminated

v2: softened message to 'may not be' supported per Robert's suggestion

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Robert Richter <robert.richter@amd.com>
---
 tools/perf/builtin-record.c |    5 +++++
 tools/perf/builtin-top.c    |    4 ++++
 2 files changed, 9 insertions(+)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index c643ed6..385e272 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -294,6 +294,11 @@ try_again:
 					  perf_evsel__name(pos));
 				rc = -err;
 				goto out;
+			} else if ((err == EOPNOTSUPP) && (attr->precise_ip)) {
+				ui__error("\'precise\' request may not be supported. "
+					  "Try removing 'p' modifier\n");
+				rc = -err;
+				goto out;
 			}
 
 			printf("\n");
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 5550754..1d1c98c 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -976,6 +976,10 @@ try_again:
 				ui__error("Too many events are opened.\n"
 					    "Try again after reducing the number of events\n");
 				goto out_err;
+			} else if ((err == EOPNOTSUPP) && (attr->precise_ip)) {
+				ui__error("\'precise\' request may not be supported. "
+					  "Try removing 'p' modifier\n");
+				goto out_err;
 			}
 
 			ui__error("The sys_perf_event_open() syscall "
-- 
1.7.10.1

Re: [PATCH 1/3] perf tool: precise mode requires exclude_guest

[Robert Richter]

On 12.09.12 09:16:28, David Ahern wrote:
> Summary of events per Peter:
> 
>   "Intel PEBS in VT-x context uses the DS address as a guest linear address,
>   even though its programmed by the host as a host linear address. This
>   either results in guest memory corruption and or the hardware faulting and
>   'crashing' the virtual machine.  Therefore we have to disable PEBS on VT-x
>   enter and re-enable on VT-x exit, enforcing a strict exclude_guest.
> 
>   AMB IBS does work but doesn't currently support exclude_* at all,
>   setting an exclude_* bit will make it fail."
> 
> This patch handles userspace perf command, setting the exclude_guest
> attribute if precise mode is requested, but only if a user has not
> specified a request for guest or host only profiling (G or H attribute).
> 
> Kernel side AMD currently ignores all exclude_* bits, so there is no impact
> to existing IBS code paths. Robert Richter has a patch where IBS code will
> return EINVAL if an exclude_* bit is set. When this goes in it means use
> of :p on AMD with IBS will first fail with EINVAL (because exclude_guest
> will be set). Then the existing fallback code within perf will unset
> exclude_guest and try again. The second attempt will succeed if the CPU
> supports IBS profiling.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Robert Richter <robert.richter@amd.com>
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: Avi Kivity <avi@redhat.com>
> Link: https://lkml.org/lkml/2012/7/9/264
> ---
>  tools/perf/util/parse-events.c |    3 +++
>  1 file changed, 3 insertions(+)

Acked-by: Robert Richter <robert.richter@amd.com>

I tested the patch set with AMD IBS and it works fine.

-Robert

> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 44afcf4..696cc7e 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -694,6 +694,9 @@ static int get_event_modifier(struct event_modifier *mod, char *str,
>  			eH = 0;
>  		} else if (*str == 'p') {
>  			precise++;
> +			/* use of precise requires exclude_guest */
> +			if (!exclude_GH)
> +				eG = 1;
>  		} else
>  			break;
>  
> -- 
> 1.7.10.1
> 
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 1/3] perf tool: precise mode requires exclude_guest

[Arnaldo Carvalho de Melo]

Em Wed, Sep 12, 2012 at 07:46:55PM +0200, Robert Richter escreveu:
> On 12.09.12 09:16:28, David Ahern wrote:
> > 
> > Kernel side AMD currently ignores all exclude_* bits, so there is no impact
> > to existing IBS code paths. Robert Richter has a patch where IBS code will
> > return EINVAL if an exclude_* bit is set. When this goes in it means use
> > of :p on AMD with IBS will first fail with EINVAL (because exclude_guest
> > will be set). Then the existing fallback code within perf will unset
> > exclude_guest and try again. The second attempt will succeed if the CPU
> > supports IBS profiling.
> > 
> > Signed-off-by: David Ahern <dsahern@gmail.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Robert Richter <robert.richter@amd.com>
> > Cc: Gleb Natapov <gleb@redhat.com>
> > Cc: Avi Kivity <avi@redhat.com>
> > Link: https://lkml.org/lkml/2012/7/9/264
> > ---
> >  tools/perf/util/parse-events.c |    3 +++
> >  1 file changed, 3 insertions(+)
> 
> Acked-by: Robert Richter <robert.richter@amd.com>
> 
> I tested the patch set with AMD IBS and it works fine.

Ok, I think that in this specific patchset I can add:

Tested-by: Robert Richter <robert.richter@amd.com>
Reviewed-by: Robert Richter <robert.richter@amd.com>

Instead of an Acked-by, may I?

- Arnaldo

Re: [PATCH 2/3] perf: require exclude_guest to use PEBS - kernel side enforcement

[Namhyung Kim]

Hi David,

On Wed, 12 Sep 2012 09:16:29 -0600, David Ahern wrote:
> From: Peter Zijlstra <peterz@infradead.org>
>
> Per Peter:
>   "Intel PEBS in VT-x context uses the DS address as a guest linear address,
>   even though its programmed by the host as a host linear address. This
>   either results in guest memory corruption and or the hardware faulting and
>   'crashing' the virtual machine.  Therefore we have to disable PEBS on VT-x
>   enter and re-enable on VT-x exit, enforcing a strict exclude_guest.
>
> This patch enforces exclude_guest kernel side.
>
[snip]
> @@ -380,6 +383,9 @@ int x86_pmu_hw_config(struct perf_event *event)
>  	if (event->attr.precise_ip) {
>  		int precise = 0;
>  
> +		if (!event->attr.exclude_guest)
> +				return -EOPNOTSUPP;

I see a whitespace problem here. :)

Thanks,
Namhyung


> +
>  		/* Support for constant skid */
>  		if (x86_pmu.pebs_active && !x86_pmu.pebs_broken) {
>  			precise++;

Re: [PATCH 2/3] perf: require exclude_guest to use PEBS - kernel side enforcement

[David Ahern]

On 9/12/12 8:38 PM, Namhyung Kim wrote:
> Hi David,
>
> On Wed, 12 Sep 2012 09:16:29 -0600, David Ahern wrote:
>> From: Peter Zijlstra <peterz@infradead.org>
>>
>> Per Peter:
>>    "Intel PEBS in VT-x context uses the DS address as a guest linear address,
>>    even though its programmed by the host as a host linear address. This
>>    either results in guest memory corruption and or the hardware faulting and
>>    'crashing' the virtual machine.  Therefore we have to disable PEBS on VT-x
>>    enter and re-enable on VT-x exit, enforcing a strict exclude_guest.
>>
>> This patch enforces exclude_guest kernel side.
>>
> [snip]
>> @@ -380,6 +383,9 @@ int x86_pmu_hw_config(struct perf_event *event)
>>   	if (event->attr.precise_ip) {
>>   		int precise = 0;
>>
>> +		if (!event->attr.exclude_guest)
>> +				return -EOPNOTSUPP;
>
> I see a whitespace problem here. :)

grr.... an extra freaking tab.

Re: [PATCH 2/3] perf: require exclude_guest to use PEBS - kernel side enforcement

[Arnaldo Carvalho de Melo]

Em Wed, Sep 12, 2012 at 10:33:36PM -0600, David Ahern escreveu:
> On 9/12/12 8:38 PM, Namhyung Kim wrote:
> >I see a whitespace problem here. :)
> 
> grr.... an extra freaking tab.

As people say down here, it happens, even in the best families... :-P

- Arnaldo

$ shutdown now tho

Re: [PATCH 1/3] perf tool: precise mode requires exclude_guest

[Robert Richter]

On 12.09.12 11:50:57, Arnaldo Carvalho de Melo wrote:
> Em Wed, Sep 12, 2012 at 07:46:55PM +0200, Robert Richter escreveu:
> > On 12.09.12 09:16:28, David Ahern wrote:
> > > 
> > > Kernel side AMD currently ignores all exclude_* bits, so there is no impact
> > > to existing IBS code paths. Robert Richter has a patch where IBS code will
> > > return EINVAL if an exclude_* bit is set. When this goes in it means use
> > > of :p on AMD with IBS will first fail with EINVAL (because exclude_guest
> > > will be set). Then the existing fallback code within perf will unset
> > > exclude_guest and try again. The second attempt will succeed if the CPU
> > > supports IBS profiling.
> > > 
> > > Signed-off-by: David Ahern <dsahern@gmail.com>
> > > Cc: Ingo Molnar <mingo@kernel.org>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Robert Richter <robert.richter@amd.com>
> > > Cc: Gleb Natapov <gleb@redhat.com>
> > > Cc: Avi Kivity <avi@redhat.com>
> > > Link: https://lkml.org/lkml/2012/7/9/264
> > > ---
> > >  tools/perf/util/parse-events.c |    3 +++
> > >  1 file changed, 3 insertions(+)
> > 
> > Acked-by: Robert Richter <robert.richter@amd.com>
> > 
> > I tested the patch set with AMD IBS and it works fine.
> 
> Ok, I think that in this specific patchset I can add:
> 
> Tested-by: Robert Richter <robert.richter@amd.com>
> Reviewed-by: Robert Richter <robert.richter@amd.com>
> 
> Instead of an Acked-by, may I?

I am fine with this.

Thanks,

-Robert

> 
> - Arnaldo
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center