linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP
@ 2017-04-19 17:44 Ganapatrao Kulkarni
  2017-04-20  8:49 ` Mark Rutland
  0 siblings, 1 reply; 13+ messages in thread
From: Ganapatrao Kulkarni @ 2017-04-19 17:44 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel
  Cc: Will.Deacon, catalin.marinas, mark.rutland, acme,
	alexander.shishkin, peterz, mingo, jnair, gpkulkarni

commit d98ecda (arm64: perf: Count EL2 events if the kernel is running in HYP)
is returning error for perf syscall with mixed attribute set for exclude_kernel
and exclude_hv. This change is breaking some applications (observed with hhvm)
when ran on VHE enabled platforms.

Adding fix to consider only exclude_kernel attribute when kernel is
running in HYP. Also adding sysfs file to notify the bhehaviour
of attribute exclude_hv.

Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
---

Changelog:

V2:
 - Changes as per Will Deacon's suggestion.

V1: Initial patch

 arch/arm64/kernel/perf_event.c | 28 ++++++++++++++++++++++++----
 include/linux/perf/arm_pmu.h   |  1 +
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 57ae9d9..b35b94c 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -535,6 +535,25 @@
 	.attrs = armv8_pmuv3_format_attrs,
 };
 
+static ssize_t armv8_pmuv3_exclude_hv_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", is_kernel_in_hyp_mode() ? 1 : 0);
+}
+
+static struct device_attribute armv8_pmuv3_exclude_hv_attr =
+		__ATTR(exclude_hv, 0444, armv8_pmuv3_exclude_hv_show, NULL);
+
+static struct attribute *armv8_pmuv3_attrs[] = {
+	&armv8_pmuv3_exclude_hv_attr.attr,
+	NULL,
+};
+
+static struct attribute_group armv8_pmuv3_attr_group = {
+	.name = "attr",
+	.attrs = armv8_pmuv3_attrs,
+};
+
 /*
  * Perf Events' indices
  */
@@ -871,14 +890,13 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
 
 	if (attr->exclude_idle)
 		return -EPERM;
-	if (is_kernel_in_hyp_mode() &&
-	    attr->exclude_kernel != attr->exclude_hv)
-		return -EINVAL;
+	if (is_kernel_in_hyp_mode() && !attr->exclude_kernel)
+		config_base |= ARMV8_PMU_INCLUDE_EL2;
 	if (attr->exclude_user)
 		config_base |= ARMV8_PMU_EXCLUDE_EL0;
 	if (!is_kernel_in_hyp_mode() && attr->exclude_kernel)
 		config_base |= ARMV8_PMU_EXCLUDE_EL1;
-	if (!attr->exclude_hv)
+	if (!is_kernel_in_hyp_mode() && !attr->exclude_hv)
 		config_base |= ARMV8_PMU_INCLUDE_EL2;
 
 	/*
@@ -1008,6 +1026,8 @@ static int armv8_pmuv3_init(struct arm_pmu *cpu_pmu)
 		&armv8_pmuv3_events_attr_group;
 	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_FORMATS] =
 		&armv8_pmuv3_format_attr_group;
+	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_ATTR] =
+		&armv8_pmuv3_attr_group;
 	return armv8pmu_probe_pmu(cpu_pmu);
 }
 
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index 8462da2..a26ffc7 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -81,6 +81,7 @@ enum armpmu_attr_groups {
 	ARMPMU_ATTR_GROUP_COMMON,
 	ARMPMU_ATTR_GROUP_EVENTS,
 	ARMPMU_ATTR_GROUP_FORMATS,
+	ARMPMU_ATTR_GROUP_ATTR,
 	ARMPMU_NR_ATTR_GROUPS
 };
 
-- 
1.8.1.4

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

* Re: [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP
  2017-04-19 17:44 [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP Ganapatrao Kulkarni
@ 2017-04-20  8:49 ` Mark Rutland
  2017-04-20  9:26   ` Ganapatrao Kulkarni
  0 siblings, 1 reply; 13+ messages in thread
From: Mark Rutland @ 2017-04-20  8:49 UTC (permalink / raw)
  To: Ganapatrao Kulkarni
  Cc: linux-kernel, linux-arm-kernel, Will.Deacon, catalin.marinas,
	acme, alexander.shishkin, peterz, mingo, jnair, gpkulkarni

On Wed, Apr 19, 2017 at 11:14:06PM +0530, Ganapatrao Kulkarni wrote:
> commit d98ecda (arm64: perf: Count EL2 events if the kernel is running in HYP)
> is returning error for perf syscall with mixed attribute set for exclude_kernel
> and exclude_hv. This change is breaking some applications (observed with hhvm)
> when ran on VHE enabled platforms.
> 
> Adding fix to consider only exclude_kernel attribute when kernel is
> running in HYP. Also adding sysfs file to notify the bhehaviour
> of attribute exclude_hv.
> 
> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
> ---
> 
> Changelog:
> 
> V2:
>  - Changes as per Will Deacon's suggestion.
> 
> V1: Initial patch
> 
>  arch/arm64/kernel/perf_event.c | 28 ++++++++++++++++++++++++----
>  include/linux/perf/arm_pmu.h   |  1 +
>  2 files changed, 25 insertions(+), 4 deletions(-)
> 
> @@ -871,14 +890,13 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
>  
>  	if (attr->exclude_idle)
>  		return -EPERM;
> -	if (is_kernel_in_hyp_mode() &&
> -	    attr->exclude_kernel != attr->exclude_hv)
> -		return -EINVAL;
> +	if (is_kernel_in_hyp_mode() && !attr->exclude_kernel)
> +		config_base |= ARMV8_PMU_INCLUDE_EL2;
>  	if (attr->exclude_user)
>  		config_base |= ARMV8_PMU_EXCLUDE_EL0;
>  	if (!is_kernel_in_hyp_mode() && attr->exclude_kernel)
>  		config_base |= ARMV8_PMU_EXCLUDE_EL1;
> -	if (!attr->exclude_hv)
> +	if (!is_kernel_in_hyp_mode() && !attr->exclude_hv)
>  		config_base |= ARMV8_PMU_INCLUDE_EL2;

This isn't quite what Will suggested.

The idea was that userspace would read sysfs, then use that to determine
the correct exclusion parameters [1,2]. This logic was not expected to
change; it correctly validates whether we can provide what the user
requests.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/499224.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/499493.html

>  
>  	/*
> @@ -1008,6 +1026,8 @@ static int armv8_pmuv3_init(struct arm_pmu *cpu_pmu)
>  		&armv8_pmuv3_events_attr_group;
>  	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_FORMATS] =
>  		&armv8_pmuv3_format_attr_group;
> +	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_ATTR] =
> +		&armv8_pmuv3_attr_group;
>  	return armv8pmu_probe_pmu(cpu_pmu);
>  }
>  
> diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
> index 8462da2..a26ffc7 100644
> --- a/include/linux/perf/arm_pmu.h
> +++ b/include/linux/perf/arm_pmu.h
> @@ -81,6 +81,7 @@ enum armpmu_attr_groups {
>  	ARMPMU_ATTR_GROUP_COMMON,
>  	ARMPMU_ATTR_GROUP_EVENTS,
>  	ARMPMU_ATTR_GROUP_FORMATS,
> +	ARMPMU_ATTR_GROUP_ATTR,
>  	ARMPMU_NR_ATTR_GROUPS
>  };
>  
> -- 
> 1.8.1.4
> 

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

* Re: [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP
  2017-04-20  8:49 ` Mark Rutland
@ 2017-04-20  9:26   ` Ganapatrao Kulkarni
  2017-04-24 15:45     ` Will Deacon
  0 siblings, 1 reply; 13+ messages in thread
From: Ganapatrao Kulkarni @ 2017-04-20  9:26 UTC (permalink / raw)
  To: Mark Rutland, Andrew.Pinski
  Cc: Ganapatrao Kulkarni, linux-kernel, linux-arm-kernel, Will Deacon,
	Catalin Marinas, acme, alexander.shishkin, peterz, Ingo Molnar,
	jnair

On Thu, Apr 20, 2017 at 2:19 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Apr 19, 2017 at 11:14:06PM +0530, Ganapatrao Kulkarni wrote:
>> commit d98ecda (arm64: perf: Count EL2 events if the kernel is running in HYP)
>> is returning error for perf syscall with mixed attribute set for exclude_kernel
>> and exclude_hv. This change is breaking some applications (observed with hhvm)
>> when ran on VHE enabled platforms.
>>
>> Adding fix to consider only exclude_kernel attribute when kernel is
>> running in HYP. Also adding sysfs file to notify the bhehaviour
>> of attribute exclude_hv.
>>
>> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
>> ---
>>
>> Changelog:
>>
>> V2:
>>  - Changes as per Will Deacon's suggestion.
>>
>> V1: Initial patch
>>
>>  arch/arm64/kernel/perf_event.c | 28 ++++++++++++++++++++++++----
>>  include/linux/perf/arm_pmu.h   |  1 +
>>  2 files changed, 25 insertions(+), 4 deletions(-)
>>
>> @@ -871,14 +890,13 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
>>
>>       if (attr->exclude_idle)
>>               return -EPERM;
>> -     if (is_kernel_in_hyp_mode() &&
>> -         attr->exclude_kernel != attr->exclude_hv)
>> -             return -EINVAL;
>> +     if (is_kernel_in_hyp_mode() && !attr->exclude_kernel)
>> +             config_base |= ARMV8_PMU_INCLUDE_EL2;
>>       if (attr->exclude_user)
>>               config_base |= ARMV8_PMU_EXCLUDE_EL0;
>>       if (!is_kernel_in_hyp_mode() && attr->exclude_kernel)
>>               config_base |= ARMV8_PMU_EXCLUDE_EL1;
>> -     if (!attr->exclude_hv)
>> +     if (!is_kernel_in_hyp_mode() && !attr->exclude_hv)
>>               config_base |= ARMV8_PMU_INCLUDE_EL2;
>
> This isn't quite what Will suggested.
>
> The idea was that userspace would read sysfs, then use that to determine
> the correct exclusion parameters [1,2]. This logic was not expected to
> change; it correctly validates whether we can provide what the user
> requests.

OK, if you are ok with sysfs part, i can send next version with that
change only?.

>
> Thanks,
> Mark.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/499224.html
> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/499493.html
>
>>
>>       /*
>> @@ -1008,6 +1026,8 @@ static int armv8_pmuv3_init(struct arm_pmu *cpu_pmu)
>>               &armv8_pmuv3_events_attr_group;
>>       cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_FORMATS] =
>>               &armv8_pmuv3_format_attr_group;
>> +     cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_ATTR] =
>> +             &armv8_pmuv3_attr_group;
>>       return armv8pmu_probe_pmu(cpu_pmu);
>>  }
>>
>> diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
>> index 8462da2..a26ffc7 100644
>> --- a/include/linux/perf/arm_pmu.h
>> +++ b/include/linux/perf/arm_pmu.h
>> @@ -81,6 +81,7 @@ enum armpmu_attr_groups {
>>       ARMPMU_ATTR_GROUP_COMMON,
>>       ARMPMU_ATTR_GROUP_EVENTS,
>>       ARMPMU_ATTR_GROUP_FORMATS,
>> +     ARMPMU_ATTR_GROUP_ATTR,
>>       ARMPMU_NR_ATTR_GROUPS
>>  };
>>
>> --
>> 1.8.1.4
>>

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

* Re: [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP
  2017-04-20  9:26   ` Ganapatrao Kulkarni
@ 2017-04-24 15:45     ` Will Deacon
  2017-04-25  3:43       ` Ganapatrao Kulkarni
  0 siblings, 1 reply; 13+ messages in thread
From: Will Deacon @ 2017-04-24 15:45 UTC (permalink / raw)
  To: Ganapatrao Kulkarni
  Cc: Mark Rutland, Andrew.Pinski, Ganapatrao Kulkarni, linux-kernel,
	linux-arm-kernel, Catalin Marinas, acme, alexander.shishkin,
	peterz, Ingo Molnar, jnair

On Thu, Apr 20, 2017 at 02:56:50PM +0530, Ganapatrao Kulkarni wrote:
> On Thu, Apr 20, 2017 at 2:19 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Wed, Apr 19, 2017 at 11:14:06PM +0530, Ganapatrao Kulkarni wrote:
> >> commit d98ecda (arm64: perf: Count EL2 events if the kernel is running in HYP)
> >> is returning error for perf syscall with mixed attribute set for exclude_kernel
> >> and exclude_hv. This change is breaking some applications (observed with hhvm)
> >> when ran on VHE enabled platforms.
> >>
> >> Adding fix to consider only exclude_kernel attribute when kernel is
> >> running in HYP. Also adding sysfs file to notify the bhehaviour
> >> of attribute exclude_hv.
> >>
> >> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
> >> ---
> >>
> >> Changelog:
> >>
> >> V2:
> >>  - Changes as per Will Deacon's suggestion.
> >>
> >> V1: Initial patch
> >>
> >>  arch/arm64/kernel/perf_event.c | 28 ++++++++++++++++++++++++----
> >>  include/linux/perf/arm_pmu.h   |  1 +
> >>  2 files changed, 25 insertions(+), 4 deletions(-)
> >>
> >> @@ -871,14 +890,13 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
> >>
> >>       if (attr->exclude_idle)
> >>               return -EPERM;
> >> -     if (is_kernel_in_hyp_mode() &&
> >> -         attr->exclude_kernel != attr->exclude_hv)
> >> -             return -EINVAL;
> >> +     if (is_kernel_in_hyp_mode() && !attr->exclude_kernel)
> >> +             config_base |= ARMV8_PMU_INCLUDE_EL2;
> >>       if (attr->exclude_user)
> >>               config_base |= ARMV8_PMU_EXCLUDE_EL0;
> >>       if (!is_kernel_in_hyp_mode() && attr->exclude_kernel)
> >>               config_base |= ARMV8_PMU_EXCLUDE_EL1;
> >> -     if (!attr->exclude_hv)
> >> +     if (!is_kernel_in_hyp_mode() && !attr->exclude_hv)
> >>               config_base |= ARMV8_PMU_INCLUDE_EL2;
> >
> > This isn't quite what Will suggested.
> >
> > The idea was that userspace would read sysfs, then use that to determine
> > the correct exclusion parameters [1,2]. This logic was not expected to
> > change; it correctly validates whether we can provide what the user
> > requests.
> 
> OK, if you are ok with sysfs part, i can send next version with that
> change only?.

I think the sysfs part is still a little dodgy, since you still expose the
"exclude_hv" file with a value of 0 when not running at EL2, which would
imply that exclude_hv is forced to zero. I don't think that's correct.

Will

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

* Re: [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP
  2017-04-24 15:45     ` Will Deacon
@ 2017-04-25  3:43       ` Ganapatrao Kulkarni
  2017-04-25 16:53         ` Will Deacon
  0 siblings, 1 reply; 13+ messages in thread
From: Ganapatrao Kulkarni @ 2017-04-25  3:43 UTC (permalink / raw)
  To: Will Deacon
  Cc: Mark Rutland, Andrew.Pinski, Ganapatrao Kulkarni, linux-kernel,
	linux-arm-kernel, Catalin Marinas, acme, alexander.shishkin,
	peterz, Ingo Molnar, jnair

On Mon, Apr 24, 2017 at 9:15 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Apr 20, 2017 at 02:56:50PM +0530, Ganapatrao Kulkarni wrote:
>> On Thu, Apr 20, 2017 at 2:19 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Wed, Apr 19, 2017 at 11:14:06PM +0530, Ganapatrao Kulkarni wrote:
>> >> commit d98ecda (arm64: perf: Count EL2 events if the kernel is running in HYP)
>> >> is returning error for perf syscall with mixed attribute set for exclude_kernel
>> >> and exclude_hv. This change is breaking some applications (observed with hhvm)
>> >> when ran on VHE enabled platforms.
>> >>
>> >> Adding fix to consider only exclude_kernel attribute when kernel is
>> >> running in HYP. Also adding sysfs file to notify the bhehaviour
>> >> of attribute exclude_hv.
>> >>
>> >> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
>> >> ---
>> >>
>> >> Changelog:
>> >>
>> >> V2:
>> >>  - Changes as per Will Deacon's suggestion.
>> >>
>> >> V1: Initial patch
>> >>
>> >>  arch/arm64/kernel/perf_event.c | 28 ++++++++++++++++++++++++----
>> >>  include/linux/perf/arm_pmu.h   |  1 +
>> >>  2 files changed, 25 insertions(+), 4 deletions(-)
>> >>
>> >> @@ -871,14 +890,13 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
>> >>
>> >>       if (attr->exclude_idle)
>> >>               return -EPERM;
>> >> -     if (is_kernel_in_hyp_mode() &&
>> >> -         attr->exclude_kernel != attr->exclude_hv)
>> >> -             return -EINVAL;
>> >> +     if (is_kernel_in_hyp_mode() && !attr->exclude_kernel)
>> >> +             config_base |= ARMV8_PMU_INCLUDE_EL2;
>> >>       if (attr->exclude_user)
>> >>               config_base |= ARMV8_PMU_EXCLUDE_EL0;
>> >>       if (!is_kernel_in_hyp_mode() && attr->exclude_kernel)
>> >>               config_base |= ARMV8_PMU_EXCLUDE_EL1;
>> >> -     if (!attr->exclude_hv)
>> >> +     if (!is_kernel_in_hyp_mode() && !attr->exclude_hv)
>> >>               config_base |= ARMV8_PMU_INCLUDE_EL2;
>> >
>> > This isn't quite what Will suggested.
>> >
>> > The idea was that userspace would read sysfs, then use that to determine
>> > the correct exclusion parameters [1,2]. This logic was not expected to
>> > change; it correctly validates whether we can provide what the user
>> > requests.
>>
>> OK, if you are ok with sysfs part, i can send next version with that
>> change only?.
>
> I think the sysfs part is still a little dodgy, since you still expose the
> "exclude_hv" file with a value of 0 when not running at EL2, which would
> imply that exclude_hv is forced to zero. I don't think that's correct.

okay, i can make exclude_hv visible only when kernel booted in EL2.
is it ok to have empty directory "attr" when kernel booted to EL1?
attr can be place holder for any other miscellaneous attributes, that
can be added in future.

>
> Will

thanks
Ganapat

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

* Re: [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP
  2017-04-25  3:43       ` Ganapatrao Kulkarni
@ 2017-04-25 16:53         ` Will Deacon
  2017-04-26  6:53           ` Jayachandran C.
  0 siblings, 1 reply; 13+ messages in thread
From: Will Deacon @ 2017-04-25 16:53 UTC (permalink / raw)
  To: Ganapatrao Kulkarni
  Cc: Mark Rutland, Andrew.Pinski, Ganapatrao Kulkarni, linux-kernel,
	linux-arm-kernel, Catalin Marinas, acme, alexander.shishkin,
	peterz, Ingo Molnar, jnair

On Tue, Apr 25, 2017 at 09:13:40AM +0530, Ganapatrao Kulkarni wrote:
> On Mon, Apr 24, 2017 at 9:15 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Thu, Apr 20, 2017 at 02:56:50PM +0530, Ganapatrao Kulkarni wrote:
> >> On Thu, Apr 20, 2017 at 2:19 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > On Wed, Apr 19, 2017 at 11:14:06PM +0530, Ganapatrao Kulkarni wrote:
> >> >> commit d98ecda (arm64: perf: Count EL2 events if the kernel is running in HYP)
> >> >> is returning error for perf syscall with mixed attribute set for exclude_kernel
> >> >> and exclude_hv. This change is breaking some applications (observed with hhvm)
> >> >> when ran on VHE enabled platforms.
> >> >>
> >> >> Adding fix to consider only exclude_kernel attribute when kernel is
> >> >> running in HYP. Also adding sysfs file to notify the bhehaviour
> >> >> of attribute exclude_hv.
> >> >>
> >> >> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
> >> >> ---
> >> >>
> >> >> Changelog:
> >> >>
> >> >> V2:
> >> >>  - Changes as per Will Deacon's suggestion.
> >> >>
> >> >> V1: Initial patch
> >> >>
> >> >>  arch/arm64/kernel/perf_event.c | 28 ++++++++++++++++++++++++----
> >> >>  include/linux/perf/arm_pmu.h   |  1 +
> >> >>  2 files changed, 25 insertions(+), 4 deletions(-)
> >> >>
> >> >> @@ -871,14 +890,13 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
> >> >>
> >> >>       if (attr->exclude_idle)
> >> >>               return -EPERM;
> >> >> -     if (is_kernel_in_hyp_mode() &&
> >> >> -         attr->exclude_kernel != attr->exclude_hv)
> >> >> -             return -EINVAL;
> >> >> +     if (is_kernel_in_hyp_mode() && !attr->exclude_kernel)
> >> >> +             config_base |= ARMV8_PMU_INCLUDE_EL2;
> >> >>       if (attr->exclude_user)
> >> >>               config_base |= ARMV8_PMU_EXCLUDE_EL0;
> >> >>       if (!is_kernel_in_hyp_mode() && attr->exclude_kernel)
> >> >>               config_base |= ARMV8_PMU_EXCLUDE_EL1;
> >> >> -     if (!attr->exclude_hv)
> >> >> +     if (!is_kernel_in_hyp_mode() && !attr->exclude_hv)
> >> >>               config_base |= ARMV8_PMU_INCLUDE_EL2;
> >> >
> >> > This isn't quite what Will suggested.
> >> >
> >> > The idea was that userspace would read sysfs, then use that to determine
> >> > the correct exclusion parameters [1,2]. This logic was not expected to
> >> > change; it correctly validates whether we can provide what the user
> >> > requests.
> >>
> >> OK, if you are ok with sysfs part, i can send next version with that
> >> change only?.
> >
> > I think the sysfs part is still a little dodgy, since you still expose the
> > "exclude_hv" file with a value of 0 when not running at EL2, which would
> > imply that exclude_hv is forced to zero. I don't think that's correct.
> 
> okay, i can make exclude_hv visible only when kernel booted in EL2.
> is it ok to have empty directory "attr" when kernel booted to EL1?
> attr can be place holder for any other miscellaneous attributes, that
> can be added in future.

Sounds good to me, although I'll seek comment from the other perf folks
before merging anything with ABI implications.

Will

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

* Re: [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP
  2017-04-25 16:53         ` Will Deacon
@ 2017-04-26  6:53           ` Jayachandran C.
       [not found]             ` <CY4PR07MB3415C0CEBB9D3DF0C249DC6793110@CY4PR07MB3415.namprd07.prod.outlook.com>
  0 siblings, 1 reply; 13+ messages in thread
From: Jayachandran C. @ 2017-04-26  6:53 UTC (permalink / raw)
  To: Will Deacon
  Cc: Ganapatrao Kulkarni, Mark Rutland, Andrew.Pinski,
	Catalin Marinas, linux-kernel, acme, alexander.shishkin, peterz,
	Ingo Molnar, jnair, Ganapatrao Kulkarni, linux-arm-kernel

Hi Will,

On Tue, Apr 25, 2017 at 10:23 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Apr 25, 2017 at 09:13:40AM +0530, Ganapatrao Kulkarni wrote:
>> On Mon, Apr 24, 2017 at 9:15 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > On Thu, Apr 20, 2017 at 02:56:50PM +0530, Ganapatrao Kulkarni wrote:
>> >> On Thu, Apr 20, 2017 at 2:19 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> > On Wed, Apr 19, 2017 at 11:14:06PM +0530, Ganapatrao Kulkarni wrote:
>> >> >> commit d98ecda (arm64: perf: Count EL2 events if the kernel is running in HYP)
>> >> >> is returning error for perf syscall with mixed attribute set for exclude_kernel
>> >> >> and exclude_hv. This change is breaking some applications (observed with hhvm)
>> >> >> when ran on VHE enabled platforms.
>> >> >>
>> >> >> Adding fix to consider only exclude_kernel attribute when kernel is
>> >> >> running in HYP. Also adding sysfs file to notify the bhehaviour
>> >> >> of attribute exclude_hv.
>> >> >>
>> >> >> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
>> >> >> ---
>> >> >>
>> >> >> Changelog:
>> >> >>
>> >> >> V2:
>> >> >>  - Changes as per Will Deacon's suggestion.
>> >> >>
>> >> >> V1: Initial patch
>> >> >>
>> >> >>  arch/arm64/kernel/perf_event.c | 28 ++++++++++++++++++++++++----
>> >> >>  include/linux/perf/arm_pmu.h   |  1 +
>> >> >>  2 files changed, 25 insertions(+), 4 deletions(-)
>> >> >>
>> >> >> @@ -871,14 +890,13 @@ static int armv8pmu_set_event_filter(struct hw_perf_event *event,
>> >> >>
>> >> >>       if (attr->exclude_idle)
>> >> >>               return -EPERM;
>> >> >> -     if (is_kernel_in_hyp_mode() &&
>> >> >> -         attr->exclude_kernel != attr->exclude_hv)
>> >> >> -             return -EINVAL;
>> >> >> +     if (is_kernel_in_hyp_mode() && !attr->exclude_kernel)
>> >> >> +             config_base |= ARMV8_PMU_INCLUDE_EL2;
>> >> >>       if (attr->exclude_user)
>> >> >>               config_base |= ARMV8_PMU_EXCLUDE_EL0;
>> >> >>       if (!is_kernel_in_hyp_mode() && attr->exclude_kernel)
>> >> >>               config_base |= ARMV8_PMU_EXCLUDE_EL1;
>> >> >> -     if (!attr->exclude_hv)
>> >> >> +     if (!is_kernel_in_hyp_mode() && !attr->exclude_hv)
>> >> >>               config_base |= ARMV8_PMU_INCLUDE_EL2;
>> >> >
>> >> > This isn't quite what Will suggested.
>> >> >
>> >> > The idea was that userspace would read sysfs, then use that to determine
>> >> > the correct exclusion parameters [1,2]. This logic was not expected to
>> >> > change; it correctly validates whether we can provide what the user
>> >> > requests.
>> >>
>> >> OK, if you are ok with sysfs part, i can send next version with that
>> >> change only?.
>> >
>> > I think the sysfs part is still a little dodgy, since you still expose the
>> > "exclude_hv" file with a value of 0 when not running at EL2, which would
>> > imply that exclude_hv is forced to zero. I don't think that's correct.
>>
>> okay, i can make exclude_hv visible only when kernel booted in EL2.
>> is it ok to have empty directory "attr" when kernel booted to EL1?
>> attr can be place holder for any other miscellaneous attributes, that
>> can be added in future.
>
> Sounds good to me, although I'll seek comment from the other perf folks
> before merging anything with ABI implications.

Do you really think this is the solution given:
- this is an arm64 specific sysfs interface that is tied to the perf API
- the perf API documentation has to be updated for this
- All the applications that use the perf API have to be modified to
check this sysfs interface
- If the application fails to do so, a very narrow corner case
(exclude_hv != exclude_kernel and VHE enabled) fails.

Any application that really cares can already do see if exclude_hv !=
exclude_kernel case works by calling perf_open_event() with those
options and checking the return value.

Hope I am mistake here, otherwise this does not sound like a good idea.

JC.

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

* Re: [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP
       [not found]             ` <CY4PR07MB3415C0CEBB9D3DF0C249DC6793110@CY4PR07MB3415.namprd07.prod.outlook.com>
@ 2017-04-26 10:10               ` Will Deacon
  2017-04-26 13:41                 ` Jayachandran C
  0 siblings, 1 reply; 13+ messages in thread
From: Will Deacon @ 2017-04-26 10:10 UTC (permalink / raw)
  To: Pinski, Andrew
  Cc: Jayachandran C.,
	Ganapatrao Kulkarni, Mark Rutland, Catalin Marinas, linux-kernel,
	acme, alexander.shishkin, peterz, Ingo Molnar, Nair,
	Jayachandran, Kulkarni, Ganapatrao, linux-arm-kernel

On Wed, Apr 26, 2017 at 07:22:46AM +0000, Pinski, Andrew wrote:
> On 4/25/2017 11:53 PM, Jayachandran C. wrote:
> > On Tue, Apr 25, 2017 at 10:23 PM, Will Deacon <will.deacon@arm.com> wrote:
> >> On Tue, Apr 25, 2017 at 09:13:40AM +0530, Ganapatrao Kulkarni wrote:
> >>> On Mon, Apr 24, 2017 at 9:15 PM, Will Deacon <will.deacon@arm.com> wrote:
> >>>> On Thu, Apr 20, 2017 at 02:56:50PM +0530, Ganapatrao Kulkarni wrote:
> >>>>> OK, if you are ok with sysfs part, i can send next version with that
> >>>>> change only?.
> >>>> I think the sysfs part is still a little dodgy, since you still expose the
> >>>> "exclude_hv" file with a value of 0 when not running at EL2, which would
> >>>> imply that exclude_hv is forced to zero. I don't think that's correct.
> >>> okay, i can make exclude_hv visible only when kernel booted in EL2.
> >>> is it ok to have empty directory "attr" when kernel booted to EL1?
> >>> attr can be place holder for any other miscellaneous attributes, that
> >>> can be added in future.
> >> Sounds good to me, although I'll seek comment from the other perf folks
> >> before merging anything with ABI implications.
> > Do you really think this is the solution given:
> > - this is an arm64 specific sysfs interface that is tied to the perf API

That's why I want feedback from others. The intention would be that this can
be used by other PMUs as well, since it's not uncommon that parts of the
sizeable perf_event_attr structure are not used by a given PMU.

> > - the perf API documentation has to be updated for this

So? If having to update documentation means we shouldn't change the kernel,
then we may as well all find new jobs.

> > - All the applications that use the perf API have to be modified to
> > check this sysfs interface
> > - If the application fails to do so, a very narrow corner case
> > (exclude_hv != exclude_kernel and VHE enabled) fails.

See below, but apparently people care about it.

> > Any application that really cares can already do see if exclude_hv !=
> > exclude_kernel case works by calling perf_open_event() with those
> > options and checking the return value.

That's a good point: there is *something* userspace can do, although that
would be arm64-specific and doesn't really help with the state-space
explosion you get with combinations of invalid/unused perf_event_attr
fields.

> An example of an application which needs to changed is HHVM. Currently 
> it sets exclude_hv to true but exclude_kernel to false as it does not 
> care about the hypervisor associated perf events associated with the 
> code, only the kernel and userspace associated evnts.
> Yes we could submit a patch to use the sysfs interface to check but it 
> would look funny and the facebook folks might reject the patch as it is 
> ARM64 specific in generic code.  Note this is how all of this discussion 
> started was HHVM's call to perf_open_event was failing.

Hmm, if you're saying that HHVM won't be changed to use the sysfs stuff,
then why are we bothering?

Not sure where this leaves us.

Will

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

* Re: [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP
  2017-04-26 10:10               ` Will Deacon
@ 2017-04-26 13:41                 ` Jayachandran C
  2017-04-27 17:37                   ` Will Deacon
  0 siblings, 1 reply; 13+ messages in thread
From: Jayachandran C @ 2017-04-26 13:41 UTC (permalink / raw)
  To: Will Deacon
  Cc: Pinski, Andrew, Jayachandran C.,
	Ganapatrao Kulkarni, Mark Rutland, Catalin Marinas, linux-kernel,
	acme, alexander.shishkin, peterz, Ingo Molnar, Nair,
	Jayachandran, Kulkarni, Ganapatrao, linux-arm-kernel

On Wed, Apr 26, 2017 at 11:10:21AM +0100, Will Deacon wrote:
> On Wed, Apr 26, 2017 at 07:22:46AM +0000, Pinski, Andrew wrote:
> > On 4/25/2017 11:53 PM, Jayachandran C. wrote:
> > > On Tue, Apr 25, 2017 at 10:23 PM, Will Deacon <will.deacon@arm.com> wrote:
> > >> On Tue, Apr 25, 2017 at 09:13:40AM +0530, Ganapatrao Kulkarni wrote:
> > >>> On Mon, Apr 24, 2017 at 9:15 PM, Will Deacon <will.deacon@arm.com> wrote:
> > >>>> On Thu, Apr 20, 2017 at 02:56:50PM +0530, Ganapatrao Kulkarni wrote:
> > >>>>> OK, if you are ok with sysfs part, i can send next version with that
> > >>>>> change only?.
> > >>>> I think the sysfs part is still a little dodgy, since you still expose the
> > >>>> "exclude_hv" file with a value of 0 when not running at EL2, which would
> > >>>> imply that exclude_hv is forced to zero. I don't think that's correct.
> > >>> okay, i can make exclude_hv visible only when kernel booted in EL2.
> > >>> is it ok to have empty directory "attr" when kernel booted to EL1?
> > >>> attr can be place holder for any other miscellaneous attributes, that
> > >>> can be added in future.
> > >> Sounds good to me, although I'll seek comment from the other perf folks
> > >> before merging anything with ABI implications.
> > > Do you really think this is the solution given:
> > > - this is an arm64 specific sysfs interface that is tied to the perf API
> 
> That's why I want feedback from others. The intention would be that this can
> be used by other PMUs as well, since it's not uncommon that parts of the
> sizeable perf_event_attr structure are not used by a given PMU.
> 
> > > - the perf API documentation has to be updated for this
> 
> So? If having to update documentation means we shouldn't change the kernel,
> then we may as well all find new jobs.
> 
> > > - All the applications that use the perf API have to be modified to
> > > check this sysfs interface
> > > - If the application fails to do so, a very narrow corner case
> > > (exclude_hv != exclude_kernel and VHE enabled) fails.
> 
> See below, but apparently people care about it.
> 
> > > Any application that really cares can already do see if exclude_hv !=
> > > exclude_kernel case works by calling perf_open_event() with those
> > > options and checking the return value.
> 
> That's a good point: there is *something* userspace can do, although that
> would be arm64-specific and doesn't really help with the state-space
> explosion you get with combinations of invalid/unused perf_event_attr
> fields.
> 
> > An example of an application which needs to changed is HHVM. Currently 
> > it sets exclude_hv to true but exclude_kernel to false as it does not 
> > care about the hypervisor associated perf events associated with the 
> > code, only the kernel and userspace associated evnts.
> > Yes we could submit a patch to use the sysfs interface to check but it 
> > would look funny and the facebook folks might reject the patch as it is 
> > ARM64 specific in generic code.  Note this is how all of this discussion 
> > started was HHVM's call to perf_open_event was failing.
> 
> Hmm, if you're saying that HHVM won't be changed to use the sysfs stuff,
> then why are we bothering?
> 
> Not sure where this leaves us.

If my understanding is correct, the sysfs suggestion above is going to
add API complexity without solving the issue. Ignoring the exclude_hv if
it cannot be honored would be a better solution.

If that is not acceptable (which seems to be the case - but I do not
see a reason for that), I think the better option for the application
is to check if the platform supports the mode exclusion it wants by
using the perf_event_open API itself.

Thanks,
JC.

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

* Re: [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP
  2017-04-26 13:41                 ` Jayachandran C
@ 2017-04-27 17:37                   ` Will Deacon
  2017-04-28 13:46                     ` Jayachandran C
  0 siblings, 1 reply; 13+ messages in thread
From: Will Deacon @ 2017-04-27 17:37 UTC (permalink / raw)
  To: Jayachandran C
  Cc: Pinski, Andrew, Jayachandran C.,
	Ganapatrao Kulkarni, Mark Rutland, Catalin Marinas, linux-kernel,
	acme, alexander.shishkin, peterz, Ingo Molnar, Nair,
	Jayachandran, Kulkarni, Ganapatrao, linux-arm-kernel

On Wed, Apr 26, 2017 at 01:41:42PM +0000, Jayachandran C wrote:
> On Wed, Apr 26, 2017 at 11:10:21AM +0100, Will Deacon wrote:
> > On Wed, Apr 26, 2017 at 07:22:46AM +0000, Pinski, Andrew wrote:
> > > On 4/25/2017 11:53 PM, Jayachandran C. wrote:
> > > > On Tue, Apr 25, 2017 at 10:23 PM, Will Deacon <will.deacon@arm.com> wrote:
> > > >> On Tue, Apr 25, 2017 at 09:13:40AM +0530, Ganapatrao Kulkarni wrote:
> > > >>> On Mon, Apr 24, 2017 at 9:15 PM, Will Deacon <will.deacon@arm.com> wrote:
> > > >>>> On Thu, Apr 20, 2017 at 02:56:50PM +0530, Ganapatrao Kulkarni wrote:
> > > >>>>> OK, if you are ok with sysfs part, i can send next version with that
> > > >>>>> change only?.
> > > >>>> I think the sysfs part is still a little dodgy, since you still expose the
> > > >>>> "exclude_hv" file with a value of 0 when not running at EL2, which would
> > > >>>> imply that exclude_hv is forced to zero. I don't think that's correct.
> > > >>> okay, i can make exclude_hv visible only when kernel booted in EL2.
> > > >>> is it ok to have empty directory "attr" when kernel booted to EL1?
> > > >>> attr can be place holder for any other miscellaneous attributes, that
> > > >>> can be added in future.
> > > >> Sounds good to me, although I'll seek comment from the other perf folks
> > > >> before merging anything with ABI implications.
> > > > Do you really think this is the solution given:
> > > > - this is an arm64 specific sysfs interface that is tied to the perf API
> > 
> > That's why I want feedback from others. The intention would be that this can
> > be used by other PMUs as well, since it's not uncommon that parts of the
> > sizeable perf_event_attr structure are not used by a given PMU.
> > 
> > > > - the perf API documentation has to be updated for this
> > 
> > So? If having to update documentation means we shouldn't change the kernel,
> > then we may as well all find new jobs.
> > 
> > > > - All the applications that use the perf API have to be modified to
> > > > check this sysfs interface
> > > > - If the application fails to do so, a very narrow corner case
> > > > (exclude_hv != exclude_kernel and VHE enabled) fails.
> > 
> > See below, but apparently people care about it.
> > 
> > > > Any application that really cares can already do see if exclude_hv !=
> > > > exclude_kernel case works by calling perf_open_event() with those
> > > > options and checking the return value.
> > 
> > That's a good point: there is *something* userspace can do, although that
> > would be arm64-specific and doesn't really help with the state-space
> > explosion you get with combinations of invalid/unused perf_event_attr
> > fields.
> > 
> > > An example of an application which needs to changed is HHVM. Currently 
> > > it sets exclude_hv to true but exclude_kernel to false as it does not 
> > > care about the hypervisor associated perf events associated with the 
> > > code, only the kernel and userspace associated evnts.
> > > Yes we could submit a patch to use the sysfs interface to check but it 
> > > would look funny and the facebook folks might reject the patch as it is 
> > > ARM64 specific in generic code.  Note this is how all of this discussion 
> > > started was HHVM's call to perf_open_event was failing.
> > 
> > Hmm, if you're saying that HHVM won't be changed to use the sysfs stuff,
> > then why are we bothering?
> > 
> > Not sure where this leaves us.
> 
> If my understanding is correct, the sysfs suggestion above is going to
> add API complexity without solving the issue. Ignoring the exclude_hv if
> it cannot be honored would be a better solution.

Better for HHVM, sure, but I don't think it's better in general. It means
that we silently do the opposite of what the user has requested in some
configurations.

Will

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

* Re: [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP
  2017-04-27 17:37                   ` Will Deacon
@ 2017-04-28 13:46                     ` Jayachandran C
  2017-04-28 16:38                       ` Will Deacon
  0 siblings, 1 reply; 13+ messages in thread
From: Jayachandran C @ 2017-04-28 13:46 UTC (permalink / raw)
  To: Will Deacon
  Cc: Pinski, Andrew, Jayachandran C.,
	Ganapatrao Kulkarni, Mark Rutland, Catalin Marinas, linux-kernel,
	acme, alexander.shishkin, peterz, Ingo Molnar, Nair,
	Jayachandran, Kulkarni, Ganapatrao, linux-arm-kernel

On Thu, Apr 27, 2017 at 06:37:59PM +0100, Will Deacon wrote:
> On Wed, Apr 26, 2017 at 01:41:42PM +0000, Jayachandran C wrote:
> > On Wed, Apr 26, 2017 at 11:10:21AM +0100, Will Deacon wrote:
> > > On Wed, Apr 26, 2017 at 07:22:46AM +0000, Pinski, Andrew wrote:
> > > > On 4/25/2017 11:53 PM, Jayachandran C. wrote:
> > > > > On Tue, Apr 25, 2017 at 10:23 PM, Will Deacon <will.deacon@arm.com> wrote:
> > > > >> On Tue, Apr 25, 2017 at 09:13:40AM +0530, Ganapatrao Kulkarni wrote:
> > > > >>> On Mon, Apr 24, 2017 at 9:15 PM, Will Deacon <will.deacon@arm.com> wrote:
> > > > >>>> On Thu, Apr 20, 2017 at 02:56:50PM +0530, Ganapatrao Kulkarni wrote:
> > > > >>>>> OK, if you are ok with sysfs part, i can send next version with that
> > > > >>>>> change only?.
> > > > >>>> I think the sysfs part is still a little dodgy, since you still expose the
> > > > >>>> "exclude_hv" file with a value of 0 when not running at EL2, which would
> > > > >>>> imply that exclude_hv is forced to zero. I don't think that's correct.
> > > > >>> okay, i can make exclude_hv visible only when kernel booted in EL2.
> > > > >>> is it ok to have empty directory "attr" when kernel booted to EL1?
> > > > >>> attr can be place holder for any other miscellaneous attributes, that
> > > > >>> can be added in future.
> > > > >> Sounds good to me, although I'll seek comment from the other perf folks
> > > > >> before merging anything with ABI implications.
> > > > > Do you really think this is the solution given:
> > > > > - this is an arm64 specific sysfs interface that is tied to the perf API
> > > 
> > > That's why I want feedback from others. The intention would be that this can
> > > be used by other PMUs as well, since it's not uncommon that parts of the
> > > sizeable perf_event_attr structure are not used by a given PMU.
> > > 
> > > > > - the perf API documentation has to be updated for this
> > > 
> > > So? If having to update documentation means we shouldn't change the kernel,
> > > then we may as well all find new jobs.
> > > 
> > > > > - All the applications that use the perf API have to be modified to
> > > > > check this sysfs interface
> > > > > - If the application fails to do so, a very narrow corner case
> > > > > (exclude_hv != exclude_kernel and VHE enabled) fails.
> > > 
> > > See below, but apparently people care about it.
> > > 
> > > > > Any application that really cares can already do see if exclude_hv !=
> > > > > exclude_kernel case works by calling perf_open_event() with those
> > > > > options and checking the return value.
> > > 
> > > That's a good point: there is *something* userspace can do, although that
> > > would be arm64-specific and doesn't really help with the state-space
> > > explosion you get with combinations of invalid/unused perf_event_attr
> > > fields.
> > > 
> > > > An example of an application which needs to changed is HHVM. Currently 
> > > > it sets exclude_hv to true but exclude_kernel to false as it does not 
> > > > care about the hypervisor associated perf events associated with the 
> > > > code, only the kernel and userspace associated evnts.
> > > > Yes we could submit a patch to use the sysfs interface to check but it 
> > > > would look funny and the facebook folks might reject the patch as it is 
> > > > ARM64 specific in generic code.  Note this is how all of this discussion 
> > > > started was HHVM's call to perf_open_event was failing.
> > > 
> > > Hmm, if you're saying that HHVM won't be changed to use the sysfs stuff,
> > > then why are we bothering?
> > > 
> > > Not sure where this leaves us.
> > 
> > If my understanding is correct, the sysfs suggestion above is going to
> > add API complexity without solving the issue. Ignoring the exclude_hv if
> > it cannot be honored would be a better solution.
> 
> Better for HHVM, sure, but I don't think it's better in general. It means
> that we silently do the opposite of what the user has requested in some
> configurations.

If my understanding is correct, when is_kernel_in_hyp_mode() is true,
the kernel is in EL2 and there is no real hypervisor with hvc calls
from kernel. Ignoring the exclude_hv would be correct.

When kernel is in EL1, it would be correct to consider exclude_hv to
skip events in EL2 (reached with hvc).

I don't see the issue, can you please give more detail on the config
with unexpected behavior?

JC.

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

* Re: [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP
  2017-04-28 13:46                     ` Jayachandran C
@ 2017-04-28 16:38                       ` Will Deacon
  2017-05-01 16:10                         ` Jayachandran C
  0 siblings, 1 reply; 13+ messages in thread
From: Will Deacon @ 2017-04-28 16:38 UTC (permalink / raw)
  To: Jayachandran C
  Cc: Pinski, Andrew, Jayachandran C.,
	Ganapatrao Kulkarni, Mark Rutland, Catalin Marinas, linux-kernel,
	acme, alexander.shishkin, peterz, Ingo Molnar, Nair,
	Jayachandran, Kulkarni, Ganapatrao, linux-arm-kernel

Hi guys,

On Fri, Apr 28, 2017 at 01:46:24PM +0000, Jayachandran C wrote:
> On Thu, Apr 27, 2017 at 06:37:59PM +0100, Will Deacon wrote:
> > > If my understanding is correct, the sysfs suggestion above is going to
> > > add API complexity without solving the issue. Ignoring the exclude_hv if
> > > it cannot be honored would be a better solution.
> > 
> > Better for HHVM, sure, but I don't think it's better in general. It means
> > that we silently do the opposite of what the user has requested in some
> > configurations.
> 
> If my understanding is correct, when is_kernel_in_hyp_mode() is true,
> the kernel is in EL2 and there is no real hypervisor with hvc calls
> from kernel. Ignoring the exclude_hv would be correct.
> 
> When kernel is in EL1, it would be correct to consider exclude_hv to
> skip events in EL2 (reached with hvc).
> 
> I don't see the issue, can you please give more detail on the config
> with unexpected behavior?

This got me thinking, so I tried to look at the history of exclude_hv. It
turns out it was added in 0475f9ea8e2c ("perf_counters: allow users to
count user, kernel and/or hypervisor events") for PowerPC, not x86 (where
this doesn't seem to be supported).

Notably, it looks like it's always ignored for the x86 CPU PMU, and ignored
on PowerPC when a hypervisor is not present. I think that backs up your
suggestion that we should ignore it when is_kernel_in_hyp_mode() is true.

In which case, I withdraw my objection to ignoring exclude_hv when running
in hyp mode, but please add a comment explaining the rationale!

Will

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

* Re: [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP
  2017-04-28 16:38                       ` Will Deacon
@ 2017-05-01 16:10                         ` Jayachandran C
  0 siblings, 0 replies; 13+ messages in thread
From: Jayachandran C @ 2017-05-01 16:10 UTC (permalink / raw)
  To: Will Deacon
  Cc: Pinski, Andrew, Jayachandran C.,
	Ganapatrao Kulkarni, Mark Rutland, Catalin Marinas, linux-kernel,
	acme, alexander.shishkin, peterz, Ingo Molnar, Nair,
	Jayachandran, Kulkarni, Ganapatrao, linux-arm-kernel

On Fri, Apr 28, 2017 at 05:38:23PM +0100, Will Deacon wrote:
> Hi guys,
> 
> On Fri, Apr 28, 2017 at 01:46:24PM +0000, Jayachandran C wrote:
> > On Thu, Apr 27, 2017 at 06:37:59PM +0100, Will Deacon wrote:
> > > > If my understanding is correct, the sysfs suggestion above is going to
> > > > add API complexity without solving the issue. Ignoring the exclude_hv if
> > > > it cannot be honored would be a better solution.
> > > 
> > > Better for HHVM, sure, but I don't think it's better in general. It means
> > > that we silently do the opposite of what the user has requested in some
> > > configurations.
> > 
> > If my understanding is correct, when is_kernel_in_hyp_mode() is true,
> > the kernel is in EL2 and there is no real hypervisor with hvc calls
> > from kernel. Ignoring the exclude_hv would be correct.
> > 
> > When kernel is in EL1, it would be correct to consider exclude_hv to
> > skip events in EL2 (reached with hvc).
> > 
> > I don't see the issue, can you please give more detail on the config
> > with unexpected behavior?
> 
> This got me thinking, so I tried to look at the history of exclude_hv. It
> turns out it was added in 0475f9ea8e2c ("perf_counters: allow users to
> count user, kernel and/or hypervisor events") for PowerPC, not x86 (where
> this doesn't seem to be supported).
> 
> Notably, it looks like it's always ignored for the x86 CPU PMU, and ignored
> on PowerPC when a hypervisor is not present. I think that backs up your
> suggestion that we should ignore it when is_kernel_in_hyp_mode() is true.
> 
> In which case, I withdraw my objection to ignoring exclude_hv when running
> in hyp mode, but please add a comment explaining the rationale!

Thanks, we will send out an updated patch with a commit message summarizing
this disucssion.

JC

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

end of thread, other threads:[~2017-05-01 16:10 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-19 17:44 [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP Ganapatrao Kulkarni
2017-04-20  8:49 ` Mark Rutland
2017-04-20  9:26   ` Ganapatrao Kulkarni
2017-04-24 15:45     ` Will Deacon
2017-04-25  3:43       ` Ganapatrao Kulkarni
2017-04-25 16:53         ` Will Deacon
2017-04-26  6:53           ` Jayachandran C.
     [not found]             ` <CY4PR07MB3415C0CEBB9D3DF0C249DC6793110@CY4PR07MB3415.namprd07.prod.outlook.com>
2017-04-26 10:10               ` Will Deacon
2017-04-26 13:41                 ` Jayachandran C
2017-04-27 17:37                   ` Will Deacon
2017-04-28 13:46                     ` Jayachandran C
2017-04-28 16:38                       ` Will Deacon
2017-05-01 16:10                         ` Jayachandran C

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).