[PATCH] perf/x86/intel/uncore: Fix for iio mapping on Skylake Server

[PATCH] perf/x86/intel/uncore: Fix for iio mapping on Skylake Server

[alexander.antonov]

From: Alexander Antonov <alexander.antonov@linux.intel.com>

Introduced early attributes /sys/devices/uncore_iio_<pmu_idx>/die* are
initialized by skx_iio_set_mapping(), however, for example, for multiple
segment platforms skx_iio_get_topology() returns -EPERM before a list of
attributes in skx_iio_mapping_group will have been initialized.
As a result the list is being NULL. Thus the warning
"sysfs: (bin_)attrs not set by subsystem for group: uncore_iio_*/" appears
and uncore_iio pmus are not available in sysfs. Clear IIO attr_update
to properly handle the cases when topology information cannot be
retrieved.

Fixes: bb42b3d39781 ("perf/x86/intel/uncore: Expose an Uncore unit to IIO PMON mapping")
Reported-by: Kyle Meyer <kyle.meyer@hpe.com>
Suggested-by: Kan Liang <kan.liang@linux.intel.com>
Reviewed-by: Alexei Budankov <alexey.budankov@linux.intel.com>
Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
---
 arch/x86/events/intel/uncore_snbep.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index 62e88ad919ff..ccfa1d6b6aa0 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -3749,7 +3749,9 @@ static int skx_iio_set_mapping(struct intel_uncore_type *type)
 
 	ret = skx_iio_get_topology(type);
 	if (ret)
-		return ret;
+		goto clear_attr_update;
+
+	ret = -ENOMEM;
 
 	/* One more for NULL. */
 	attrs = kcalloc((uncore_max_dies() + 1), sizeof(*attrs), GFP_KERNEL);
@@ -3781,8 +3783,9 @@ static int skx_iio_set_mapping(struct intel_uncore_type *type)
 	kfree(eas);
 	kfree(attrs);
 	kfree(type->topology);
+clear_attr_update:
 	type->attr_update = NULL;
-	return -ENOMEM;
+	return ret;
 }
 
 static void skx_iio_cleanup_mapping(struct intel_uncore_type *type)

base-commit: a1b8638ba1320e6684aa98233c15255eb803fac7
-- 
2.19.1

[tip: perf/core] perf/x86/intel/uncore: Fix for iio mapping on Skylake Server

[tip-bot2 for Alexander Antonov]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     f797f05d917ffef94249ee0aec4c14a5b50517b2
Gitweb:        https://git.kernel.org/tip/f797f05d917ffef94249ee0aec4c14a5b50517b2
Author:        Alexander Antonov <alexander.antonov@linux.intel.com>
AuthorDate:    Mon, 28 Sep 2020 13:21:33 +03:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Tue, 29 Sep 2020 09:57:02 +02:00

perf/x86/intel/uncore: Fix for iio mapping on Skylake Server

Introduced early attributes /sys/devices/uncore_iio_<pmu_idx>/die* are
initialized by skx_iio_set_mapping(), however, for example, for multiple
segment platforms skx_iio_get_topology() returns -EPERM before a list of
attributes in skx_iio_mapping_group will have been initialized.
As a result the list is being NULL. Thus the warning
"sysfs: (bin_)attrs not set by subsystem for group: uncore_iio_*/" appears
and uncore_iio pmus are not available in sysfs. Clear IIO attr_update
to properly handle the cases when topology information cannot be
retrieved.

Fixes: bb42b3d39781 ("perf/x86/intel/uncore: Expose an Uncore unit to IIO PMON mapping")
Reported-by: Kyle Meyer <kyle.meyer@hpe.com>
Suggested-by: Kan Liang <kan.liang@linux.intel.com>
Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Alexei Budankov <alexey.budankov@linux.intel.com>
Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
Link: https://lkml.kernel.org/r/20200928102133.61041-1-alexander.antonov@linux.intel.com
---
 arch/x86/events/intel/uncore_snbep.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index 495056f..3f1e75f 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -3754,7 +3754,9 @@ static int skx_iio_set_mapping(struct intel_uncore_type *type)
 
 	ret = skx_iio_get_topology(type);
 	if (ret)
-		return ret;
+		goto clear_attr_update;
+
+	ret = -ENOMEM;
 
 	/* One more for NULL. */
 	attrs = kcalloc((uncore_max_dies() + 1), sizeof(*attrs), GFP_KERNEL);
@@ -3786,8 +3788,9 @@ err:
 	kfree(eas);
 	kfree(attrs);
 	kfree(type->topology);
+clear_attr_update:
 	type->attr_update = NULL;
-	return -ENOMEM;
+	return ret;
 }
 
 static void skx_iio_cleanup_mapping(struct intel_uncore_type *type)

Re: [PATCH] perf/x86/intel/uncore: Fix for iio mapping on Skylake Server

[Meyer, Kyle]

Hello Alexander,

Do you plan on supporting multiple segment platforms?

Thanks,
Kyle Meyer

________________________________________
From: alexander.antonov@linux.intel.com <alexander.antonov@linux.intel.com>
Sent: Monday, September 28, 2020 5:21 AM
To: peterz@infradead.org; linux-kernel@vger.kernel.org; x86@kernel.org
Cc: alexander.shishkin@linux.intel.com; kan.liang@linux.intel.com; alexey.budankov@linux.intel.com; ak@linux.intel.com; acme@kernel.org; mingo@redhat.com; alexander.antonov@linux.intel.com; Meyer, Kyle; Anderson, Russ
Subject: [PATCH] perf/x86/intel/uncore: Fix for iio mapping on Skylake Server

From: Alexander Antonov <alexander.antonov@linux.intel.com>

Introduced early attributes /sys/devices/uncore_iio_<pmu_idx>/die* are
initialized by skx_iio_set_mapping(), however, for example, for multiple
segment platforms skx_iio_get_topology() returns -EPERM before a list of
attributes in skx_iio_mapping_group will have been initialized.
As a result the list is being NULL. Thus the warning
"sysfs: (bin_)attrs not set by subsystem for group: uncore_iio_*/" appears
and uncore_iio pmus are not available in sysfs. Clear IIO attr_update
to properly handle the cases when topology information cannot be
retrieved.

Fixes: bb42b3d39781 ("perf/x86/intel/uncore: Expose an Uncore unit to IIO PMON mapping")
Reported-by: Kyle Meyer <kyle.meyer@hpe.com>
Suggested-by: Kan Liang <kan.liang@linux.intel.com>
Reviewed-by: Alexei Budankov <alexey.budankov@linux.intel.com>
Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
---
 arch/x86/events/intel/uncore_snbep.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index 62e88ad919ff..ccfa1d6b6aa0 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -3749,7 +3749,9 @@ static int skx_iio_set_mapping(struct intel_uncore_type *type)

        ret = skx_iio_get_topology(type);
        if (ret)
-               return ret;
+               goto clear_attr_update;
+
+       ret = -ENOMEM;

        /* One more for NULL. */
        attrs = kcalloc((uncore_max_dies() + 1), sizeof(*attrs), GFP_KERNEL);
@@ -3781,8 +3783,9 @@ static int skx_iio_set_mapping(struct intel_uncore_type *type)
        kfree(eas);
        kfree(attrs);
        kfree(type->topology);
+clear_attr_update:
        type->attr_update = NULL;
-       return -ENOMEM;
+       return ret;
 }

 static void skx_iio_cleanup_mapping(struct intel_uncore_type *type)

base-commit: a1b8638ba1320e6684aa98233c15255eb803fac7
--
2.19.1

Re: [PATCH] perf/x86/intel/uncore: Fix for iio mapping on Skylake Server

[Alexander Antonov]

Hello Kyle,

Currently we do not have plans on supporting the Uncore units to IIO PMON
mapping on multiple segment platforms due to a variety of such platforms.
It would be great if you describe your case, I mean how you configure 
segments
on your platform. It will help to cover your configuration and determine a
common approach for the mapping algorithm.

Thanks,
Alexander

On 10/09/2020 05:11 PM, Meyer, Kyle wrote:
> Hello Alexander,
>
> Do you plan on supporting multiple segment platforms?
>
> Thanks,
> Kyle Meyer
>
> ________________________________________
> From: alexander.antonov@linux.intel.com <alexander.antonov@linux.intel.com>
> Sent: Monday, September 28, 2020 5:21 AM
> To: peterz@infradead.org; linux-kernel@vger.kernel.org; x86@kernel.org
> Cc: alexander.shishkin@linux.intel.com; kan.liang@linux.intel.com; alexey.budankov@linux.intel.com; ak@linux.intel.com; acme@kernel.org; mingo@redhat.com; alexander.antonov@linux.intel.com; Meyer, Kyle; Anderson, Russ
> Subject: [PATCH] perf/x86/intel/uncore: Fix for iio mapping on Skylake Server
>
> From: Alexander Antonov <alexander.antonov@linux.intel.com>
>
> Introduced early attributes /sys/devices/uncore_iio_<pmu_idx>/die* are
> initialized by skx_iio_set_mapping(), however, for example, for multiple
> segment platforms skx_iio_get_topology() returns -EPERM before a list of
> attributes in skx_iio_mapping_group will have been initialized.
> As a result the list is being NULL. Thus the warning
> "sysfs: (bin_)attrs not set by subsystem for group: uncore_iio_*/" appears
> and uncore_iio pmus are not available in sysfs. Clear IIO attr_update
> to properly handle the cases when topology information cannot be
> retrieved.
>
> Fixes: bb42b3d39781 ("perf/x86/intel/uncore: Expose an Uncore unit to IIO PMON mapping")
> Reported-by: Kyle Meyer <kyle.meyer@hpe.com>
> Suggested-by: Kan Liang <kan.liang@linux.intel.com>
> Reviewed-by: Alexei Budankov <alexey.budankov@linux.intel.com>
> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
> Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
> ---
>   arch/x86/events/intel/uncore_snbep.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
> index 62e88ad919ff..ccfa1d6b6aa0 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -3749,7 +3749,9 @@ static int skx_iio_set_mapping(struct intel_uncore_type *type)
>
>          ret = skx_iio_get_topology(type);
>          if (ret)
> -               return ret;
> +               goto clear_attr_update;
> +
> +       ret = -ENOMEM;
>
>          /* One more for NULL. */
>          attrs = kcalloc((uncore_max_dies() + 1), sizeof(*attrs), GFP_KERNEL);
> @@ -3781,8 +3783,9 @@ static int skx_iio_set_mapping(struct intel_uncore_type *type)
>          kfree(eas);
>          kfree(attrs);
>          kfree(type->topology);
> +clear_attr_update:
>          type->attr_update = NULL;
> -       return -ENOMEM;
> +       return ret;
>   }
>
>   static void skx_iio_cleanup_mapping(struct intel_uncore_type *type)
>
> base-commit: a1b8638ba1320e6684aa98233c15255eb803fac7
> --
> 2.19.1
>

Re: [PATCH] perf/x86/intel/uncore: Fix for iio mapping on Skylake Server

[Meyer, Kyle]

Hello Alexander,

PCI Uncore mapping details including domains:

On HUB'd based systems each Socket is on it's own Segment, and we use
MMFCG space (0x80000000-0x8FFFFFFF) for segment zero. All the other
segments we use the UV-HUBs ConfigPassthru space (a 64bit config space).
For a 2 chassis, 8 socket system it looks like this: (On a 32 socket
system everything quadruples.)

$ dmsg | grep MMCONFIG
[    5.128486] PCI: MMCONFIG for domain 0000 [bus 00-fe] at [mem 0x80000000-0x8fefffff] (base 0x80000000)
[    5.132003] PCI: MMCONFIG for domain 0001 [bus 00-fe] at [mem 0xfff00000000-0xfff0fefffff] (base 0xfff00000000)
[    5.136003] PCI: MMCONFIG for domain 0002 [bus 00-fe] at [mem 0xfff30000000-0xfff3fefffff] (base 0xfff30000000)
[    5.140002] PCI: MMCONFIG for domain 0003 [bus 00-fe] at [mem 0xfff20000000-0xfff2fefffff] (base 0xfff20000000)
[    5.144003] PCI: MMCONFIG for domain 0004 [bus 00-fe] at [mem 0xfff50000000-0xfff5fefffff] (base 0xfff50000000)
[    5.148003] PCI: MMCONFIG for domain 0005 [bus 00-fe] at [mem 0xfff40000000-0xfff4fefffff] (base 0xfff40000000)
[    5.152003] PCI: MMCONFIG for domain 0006 [bus 00-fe] at [mem 0xfff70000000-0xfff7fefffff] (base 0xfff70000000)
[    5.156003] PCI: MMCONFIG for domain 0007 [bus 00-fe] at [mem 0xfff60000000-0xfff6fefffff] (base 0xfff60000000)
[    5.160207] PCI: MMCONFIG at [mem 0x80000000-0x8fefffff] reserved in E820
[    5.164000] PCI: MMCONFIG at [mem 0xfff00000000-0xfff0fefffff] reserved in E820
[    5.172003] PCI: MMCONFIG at [mem 0xfff30000000-0xfff3fefffff] reserved in E820
[    5.184001] PCI: MMCONFIG at [mem 0xfff20000000-0xfff2fefffff] reserved in E820
[    5.192001] PCI: MMCONFIG at [mem 0xfff50000000-0xfff5fefffff] reserved in E820
[    5.200001] PCI: MMCONFIG at [mem 0xfff40000000-0xfff4fefffff] reserved in E820
[    5.208001] PCI: MMCONFIG at [mem 0xfff70000000-0xfff7fefffff] reserved in E820
[    5.216002] PCI: MMCONFIG at [mem 0xfff60000000-0xfff6fefffff] reserved in E820

On hubless systems like cooperhawk we put multiple sockets under a segment
(all sockets in a given chassis reside on the same segment). This also
assigns up to 4 sockets per segment depending on how many sockets are
enabled and use a larger MMCFG space (0x80000000-0x9FFFFFFF) thus

# dmesg | grep MMCONFIG
[    5.760989] PCI: MMCONFIG for domain 0000 [bus 00-ff] at [mem 0x80000000-0x8fffffff] (base 0x80000000)
[    5.763803] PCI: MMCONFIG for domain 0001 [bus 00-ff] at [mem 0x90000000-0x9fffffff] (base 0x90000000)
[    5.767807] PCI: MMCONFIG at [mem 0x80000000-0x8fffffff] reserved in E820
[    5.771805] PCI: MMCONFIG at [mem 0x90000000-0x9fffffff] reserved in E820

Notes:

This last approach is how it is done on a Intel whitebox too. But that
doesn't scale to the number of sockets we support on a HUB'd based system
so we use a different approach.

For Intel to make this flexible, they need to get the PCIe topology
from the ACPI tables. (MCFG/DSDT/SSDT has all the info they need).
Intel does support segments in their default BIOSes starting with
CooperLake so they should also support segments in their OS drivers/tools.

Thanks,
Kyle Meyer

________________________________________
From: Alexander Antonov <alexander.antonov@linux.intel.com>
Sent: Tuesday, October 13, 2020 9:37 AM
To: Meyer, Kyle; peterz@infradead.org; linux-kernel@vger.kernel.org; x86@kernel.org
Cc: alexander.shishkin@linux.intel.com; kan.liang@linux.intel.com; alexey.budankov@linux.intel.com; ak@linux.intel.com; acme@kernel.org; mingo@redhat.com; Anderson, Russ
Subject: Re: [PATCH] perf/x86/intel/uncore: Fix for iio mapping on Skylake Server

Hello Kyle,

Currently we do not have plans on supporting the Uncore units to IIO PMON
mapping on multiple segment platforms due to a variety of such platforms.
It would be great if you describe your case, I mean how you configure
segments
on your platform. It will help to cover your configuration and determine a
common approach for the mapping algorithm.

Thanks,
Alexander

On 10/09/2020 05:11 PM, Meyer, Kyle wrote:
> Hello Alexander,
>
> Do you plan on supporting multiple segment platforms?
>
> Thanks,
> Kyle Meyer
>
> ________________________________________
> From: alexander.antonov@linux.intel.com <alexander.antonov@linux.intel.com>
> Sent: Monday, September 28, 2020 5:21 AM
> To: peterz@infradead.org; linux-kernel@vger.kernel.org; x86@kernel.org
> Cc: alexander.shishkin@linux.intel.com; kan.liang@linux.intel.com; alexey.budankov@linux.intel.com; ak@linux.intel.com; acme@kernel.org; mingo@redhat.com; alexander.antonov@linux.intel.com; Meyer, Kyle; Anderson, Russ
> Subject: [PATCH] perf/x86/intel/uncore: Fix for iio mapping on Skylake Server
>
> From: Alexander Antonov <alexander.antonov@linux.intel.com>
>
> Introduced early attributes /sys/devices/uncore_iio_<pmu_idx>/die* are
> initialized by skx_iio_set_mapping(), however, for example, for multiple
> segment platforms skx_iio_get_topology() returns -EPERM before a list of
> attributes in skx_iio_mapping_group will have been initialized.
> As a result the list is being NULL. Thus the warning
> "sysfs: (bin_)attrs not set by subsystem for group: uncore_iio_*/" appears
> and uncore_iio pmus are not available in sysfs. Clear IIO attr_update
> to properly handle the cases when topology information cannot be
> retrieved.
>
> Fixes: bb42b3d39781 ("perf/x86/intel/uncore: Expose an Uncore unit to IIO PMON mapping")
> Reported-by: Kyle Meyer <kyle.meyer@hpe.com>
> Suggested-by: Kan Liang <kan.liang@linux.intel.com>
> Reviewed-by: Alexei Budankov <alexey.budankov@linux.intel.com>
> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
> Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
> ---
>   arch/x86/events/intel/uncore_snbep.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
> index 62e88ad919ff..ccfa1d6b6aa0 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -3749,7 +3749,9 @@ static int skx_iio_set_mapping(struct intel_uncore_type *type)
>
>          ret = skx_iio_get_topology(type);
>          if (ret)
> -               return ret;
> +               goto clear_attr_update;
> +
> +       ret = -ENOMEM;
>
>          /* One more for NULL. */
>          attrs = kcalloc((uncore_max_dies() + 1), sizeof(*attrs), GFP_KERNEL);
> @@ -3781,8 +3783,9 @@ static int skx_iio_set_mapping(struct intel_uncore_type *type)
>          kfree(eas);
>          kfree(attrs);
>          kfree(type->topology);
> +clear_attr_update:
>          type->attr_update = NULL;
> -       return -ENOMEM;
> +       return ret;
>   }
>
>   static void skx_iio_cleanup_mapping(struct intel_uncore_type *type)
>
> base-commit: a1b8638ba1320e6684aa98233c15255eb803fac7
> --
> 2.19.1
>