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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJZ5v0gVFPNK=_=Tnr0QJ-yBWfSWc+wuJrE079Fz+Ba1P57TmA@mail.gmail.com>
Date: Wed, 3 Jul 2024 13:33:03 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Zhang Rui <rui.zhang@...el.com>, ricardo.neri-calderon@...ux.intel.com
Cc: rafael.j.wysocki@...el.com, linux-kernel@...r.kernel.org, 
	linux-pm@...r.kernel.org
Subject: Re: [PATCH] Thermal: intel: hfi: Give HFI instances package scope

On Wed, Jul 3, 2024 at 7:55 AM Zhang Rui <rui.zhang@...el.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@...el.com>
> Signed-off-by: Chen Yu <yu.c.chen@...el.com>
> Signed-off-by: Zhang Rui <rui.zhang@...el.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
>
>