[PATCH] Thermal: intel: hfi: Give HFI instances package scope

[PATCH] Thermal: intel: hfi: Give HFI instances package scope

[Zhang Rui]

The Intel Software Developer's Manual defines the scope of HFI (registers
and memory buffer) as package. Use package scope* in the software
representation of an HFI instance.

Using die scope in HFI instances has the effect of creating multiple,
conflicting, instances for the same package: each instance allocates its
own memory buffer and configures the same package-level registers.
Specifically, only one of the allocated memory buffers can be set in the
MSR_IA32_HW_FEEDBACK_PTR register. CPUs get incorrect HFI data from the
table.

The problem does not affect current HFI-capable platforms because they
all have single-die processors.

* We used die scope for HFI instances because there have been processors
in which packages where enumerated as dies. None of those systems support
HFI. If such a system emerged we would need to quirk it.

Co-developed-by: Chen Yu <yu.c.chen@intel.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/intel/intel_hfi.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c
index a180a98bb9f1..5b18a46a10b0 100644
--- a/drivers/thermal/intel/intel_hfi.c
+++ b/drivers/thermal/intel/intel_hfi.c
@@ -401,10 +401,10 @@ static void hfi_disable(void)
  * intel_hfi_online() - Enable HFI on @cpu
  * @cpu:	CPU in which the HFI will be enabled
  *
- * Enable the HFI to be used in @cpu. The HFI is enabled at the die/package
- * level. The first CPU in the die/package to come online does the full HFI
+ * Enable the HFI to be used in @cpu. The HFI is enabled at the package
+ * level. The first CPU in the package to come online does the full HFI
  * initialization. Subsequent CPUs will just link themselves to the HFI
- * instance of their die/package.
+ * instance of their package.
  *
  * This function is called before enabling the thermal vector in the local APIC
  * in order to ensure that @cpu has an associated HFI instance when it receives
@@ -414,31 +414,31 @@ void intel_hfi_online(unsigned int cpu)
 {
 	struct hfi_instance *hfi_instance;
 	struct hfi_cpu_info *info;
-	u16 die_id;
+	u16 pkg_id;
 
 	/* Nothing to do if hfi_instances are missing. */
 	if (!hfi_instances)
 		return;
 
 	/*
-	 * Link @cpu to the HFI instance of its package/die. It does not
+	 * Link @cpu to the HFI instance of its package. It does not
 	 * matter whether the instance has been initialized.
 	 */
 	info = &per_cpu(hfi_cpu_info, cpu);
-	die_id = topology_logical_die_id(cpu);
+	pkg_id = topology_logical_package_id(cpu);
 	hfi_instance = info->hfi_instance;
 	if (!hfi_instance) {
-		if (die_id >= max_hfi_instances)
+		if (pkg_id >= max_hfi_instances)
 			return;
 
-		hfi_instance = &hfi_instances[die_id];
+		hfi_instance = &hfi_instances[pkg_id];
 		info->hfi_instance = hfi_instance;
 	}
 
 	init_hfi_cpu_index(info);
 
 	/*
-	 * Now check if the HFI instance of the package/die of @cpu has been
+	 * Now check if the HFI instance of the package of @cpu has been
 	 * initialized (by checking its header). In such case, all we have to
 	 * do is to add @cpu to this instance's cpumask and enable the instance
 	 * if needed.
@@ -504,7 +504,7 @@ void intel_hfi_online(unsigned int cpu)
  *
  * On some processors, hardware remembers previous programming settings even
  * after being reprogrammed. Thus, keep HFI enabled even if all CPUs in the
- * die/package of @cpu are offline. See note in intel_hfi_online().
+ * package of @cpu are offline. See note in intel_hfi_online().
  */
 void intel_hfi_offline(unsigned int cpu)
 {
@@ -674,9 +674,13 @@ void __init intel_hfi_init(void)
 	if (hfi_parse_features())
 		return;
 
-	/* There is one HFI instance per die/package. */
-	max_hfi_instances = topology_max_packages() *
-			    topology_max_dies_per_package();
+	/*
+	 * Note: HFI resources are managed at the physical package scope.
+	 * There could be platforms that enumerate packages as Linux dies.
+	 * Special handling would be needed if this happens on an HFI-capable
+	 * platform.
+	 */
+	max_hfi_instances = topology_max_packages();
 
 	/*
 	 * This allocation may fail. CPU hotplug callbacks must check
-- 
2.34.1

Re: [PATCH] Thermal: intel: hfi: Give HFI instances package scope

[Rafael J. Wysocki]

On Wed, Jul 3, 2024 at 7:55 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> The Intel Software Developer's Manual defines the scope of HFI (registers
> and memory buffer) as package. Use package scope* in the software

"as a package"

> representation of an HFI instance.
>
> Using die scope in HFI instances has the effect of creating multiple,
> conflicting, instances for the same package: each instance allocates its
> own memory buffer and configures the same package-level registers.
> Specifically, only one of the allocated memory buffers can be set in the
> MSR_IA32_HW_FEEDBACK_PTR register. CPUs get incorrect HFI data from the
> table.
>
> The problem does not affect current HFI-capable platforms because they
> all have single-die processors.
>
> * We used die scope for HFI instances because there have been processors
> in which packages where enumerated as dies. None of those systems support

"were"

> HFI. If such a system emerged we would need to quirk it.
>
> Co-developed-by: Chen Yu <yu.c.chen@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Ricardo, any concerns?

> ---
>  drivers/thermal/intel/intel_hfi.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c
> index a180a98bb9f1..5b18a46a10b0 100644
> --- a/drivers/thermal/intel/intel_hfi.c
> +++ b/drivers/thermal/intel/intel_hfi.c
> @@ -401,10 +401,10 @@ static void hfi_disable(void)
>   * intel_hfi_online() - Enable HFI on @cpu
>   * @cpu:       CPU in which the HFI will be enabled
>   *
> - * Enable the HFI to be used in @cpu. The HFI is enabled at the die/package
> - * level. The first CPU in the die/package to come online does the full HFI
> + * Enable the HFI to be used in @cpu. The HFI is enabled at the package
> + * level. The first CPU in the package to come online does the full HFI
>   * initialization. Subsequent CPUs will just link themselves to the HFI
> - * instance of their die/package.
> + * instance of their package.
>   *
>   * This function is called before enabling the thermal vector in the local APIC
>   * in order to ensure that @cpu has an associated HFI instance when it receives
> @@ -414,31 +414,31 @@ void intel_hfi_online(unsigned int cpu)
>  {
>         struct hfi_instance *hfi_instance;
>         struct hfi_cpu_info *info;
> -       u16 die_id;
> +       u16 pkg_id;
>
>         /* Nothing to do if hfi_instances are missing. */
>         if (!hfi_instances)
>                 return;
>
>         /*
> -        * Link @cpu to the HFI instance of its package/die. It does not
> +        * Link @cpu to the HFI instance of its package. It does not
>          * matter whether the instance has been initialized.
>          */
>         info = &per_cpu(hfi_cpu_info, cpu);
> -       die_id = topology_logical_die_id(cpu);
> +       pkg_id = topology_logical_package_id(cpu);
>         hfi_instance = info->hfi_instance;
>         if (!hfi_instance) {
> -               if (die_id >= max_hfi_instances)
> +               if (pkg_id >= max_hfi_instances)
>                         return;
>
> -               hfi_instance = &hfi_instances[die_id];
> +               hfi_instance = &hfi_instances[pkg_id];
>                 info->hfi_instance = hfi_instance;
>         }
>
>         init_hfi_cpu_index(info);
>
>         /*
> -        * Now check if the HFI instance of the package/die of @cpu has been
> +        * Now check if the HFI instance of the package of @cpu has been
>          * initialized (by checking its header). In such case, all we have to
>          * do is to add @cpu to this instance's cpumask and enable the instance
>          * if needed.
> @@ -504,7 +504,7 @@ void intel_hfi_online(unsigned int cpu)
>   *
>   * On some processors, hardware remembers previous programming settings even
>   * after being reprogrammed. Thus, keep HFI enabled even if all CPUs in the
> - * die/package of @cpu are offline. See note in intel_hfi_online().
> + * package of @cpu are offline. See note in intel_hfi_online().
>   */
>  void intel_hfi_offline(unsigned int cpu)
>  {
> @@ -674,9 +674,13 @@ void __init intel_hfi_init(void)
>         if (hfi_parse_features())
>                 return;
>
> -       /* There is one HFI instance per die/package. */
> -       max_hfi_instances = topology_max_packages() *
> -                           topology_max_dies_per_package();
> +       /*
> +        * Note: HFI resources are managed at the physical package scope.
> +        * There could be platforms that enumerate packages as Linux dies.
> +        * Special handling would be needed if this happens on an HFI-capable
> +        * platform.
> +        */
> +       max_hfi_instances = topology_max_packages();
>
>         /*
>          * This allocation may fail. CPU hotplug callbacks must check
> --
> 2.34.1
>
>

Re: [PATCH] Thermal: intel: hfi: Give HFI instances package scope

[Ricardo Neri]

On Wed, Jul 03, 2024 at 01:33:03PM +0200, Rafael J. Wysocki wrote:
> On Wed, Jul 3, 2024 at 7:55 AM Zhang Rui <rui.zhang@intel.com> wrote:
> >
> > The Intel Software Developer's Manual defines the scope of HFI (registers
> > and memory buffer) as package. Use package scope* in the software
> 
> "as a package"
> 
> > representation of an HFI instance.
> >
> > Using die scope in HFI instances has the effect of creating multiple,
> > conflicting, instances for the same package: each instance allocates its
> > own memory buffer and configures the same package-level registers.
> > Specifically, only one of the allocated memory buffers can be set in the
> > MSR_IA32_HW_FEEDBACK_PTR register. CPUs get incorrect HFI data from the
> > table.
> >
> > The problem does not affect current HFI-capable platforms because they
> > all have single-die processors.
> >
> > * We used die scope for HFI instances because there have been processors
> > in which packages where enumerated as dies. None of those systems support
> 
> "were"
> 
> > HFI. If such a system emerged we would need to quirk it.
> >
> > Co-developed-by: Chen Yu <yu.c.chen@intel.com>
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> 
> Ricardo, any concerns?

No concerns. IMO, it is important to document why we were using die scope
before. Rui has done it.

This patch looks good to me.

FWIW, Reviewed-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Re: [PATCH] Thermal: intel: hfi: Give HFI instances package scope

[Rafael J. Wysocki]

On Tue, Jul 9, 2024 at 4:13 AM Ricardo Neri
<ricardo.neri-calderon@linux.intel.com> wrote:
>
> On Wed, Jul 03, 2024 at 01:33:03PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Jul 3, 2024 at 7:55 AM Zhang Rui <rui.zhang@intel.com> wrote:
> > >
> > > The Intel Software Developer's Manual defines the scope of HFI (registers
> > > and memory buffer) as package. Use package scope* in the software
> >
> > "as a package"
> >
> > > representation of an HFI instance.
> > >
> > > Using die scope in HFI instances has the effect of creating multiple,
> > > conflicting, instances for the same package: each instance allocates its
> > > own memory buffer and configures the same package-level registers.
> > > Specifically, only one of the allocated memory buffers can be set in the
> > > MSR_IA32_HW_FEEDBACK_PTR register. CPUs get incorrect HFI data from the
> > > table.
> > >
> > > The problem does not affect current HFI-capable platforms because they
> > > all have single-die processors.
> > >
> > > * We used die scope for HFI instances because there have been processors
> > > in which packages where enumerated as dies. None of those systems support
> >
> > "were"
> >
> > > HFI. If such a system emerged we would need to quirk it.
> > >
> > > Co-developed-by: Chen Yu <yu.c.chen@intel.com>
> > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> >
> > Ricardo, any concerns?
>
> No concerns. IMO, it is important to document why we were using die scope
> before. Rui has done it.
>
> This patch looks good to me.
>
> FWIW, Reviewed-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Applied as 6.11 material, thanks!