Re: [PATCH 6/9] thermal: intel: hfi: Enable Intel Thread Director

From: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
	Len Brown <len.brown@intel.com>,
	Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
	Zhao Liu <zhao1.liu@intel.com>,
	Zhuocheng Ding <zhuocheng.ding@intel.com>,
	x86@kernel.org, linux-pm@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Zhao Liu <zhao1.liu@linux.intel.com>
Subject: Re: [PATCH 6/9] thermal: intel: hfi: Enable Intel Thread Director
Date: Mon, 5 Feb 2024 11:28:47 +0100	[thread overview]
Message-ID: <ZcC4Xy9xBtBsCDLq@linux.intel.com> (raw)
In-Reply-To: <20240203040515.23947-7-ricardo.neri-calderon@linux.intel.com>

On Fri, Feb 02, 2024 at 08:05:12PM -0800, Ricardo Neri wrote:
> Enable Intel Thread Director (ITD) from the CPU hotplug callback: globally
> from CPU0 and then enable the thread-classification hardware in each
> logical processor individually.
> 
> Also, initialize the number of classes supported.
> 
> Currently, a bare-metal machine does not use ITD, but KVM uses the
> attributes of the bare-metal machine to virtualize HFI.
> 
> Cc: Len Brown <len.brown@intel.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> Cc: Zhao Liu <zhao1.liu@linux.intel.com>
> Cc: Zhuocheng Ding <zhuocheng.ding@intel.com>
> Cc: x86@kernel.org
> Cc: linux-pm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> # intel_hfi.c
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Discussion:
> 
> This patch conflicts with a patchset from Stanislaw Gruszka to enable HFI
> only if there are user space entities listening to the thermal netlink
> events. ITD requires that HFI is enabled to function. ITD needs to be
> unconditionally enabled for virtual machines.

Why unconditionally? From what I can tell from KVM patches (please correct
me if I'm wrong) guests need to be modified to utilize HFI/ITD. Do we
also have to enable HFI/ITD if no such guest run on virtual machine ?

> Options to resolve this conflict include a command-line argument for users
> wanting to virtualize HFI or a CONFIG_ option for the same effect. QEMU
> could also learn to listen to thermal netlink event. A blunter option is
> to unconditionally enable HFI when KVM is enabled at build time.

In general similar principle should be applied - do not enable if not
needed. We should be able to get information from KVM when there is
actual need. QEMU registering to thermal events seems to be odd for
me, and I think there must be better solution.

Regards
Stanislaw

> ---
> Patch cherry-picked from the IPC classes patchset.
> ---
> ---
>  arch/x86/include/asm/hfi.h        |  9 ++++++
>  arch/x86/include/asm/msr-index.h  |  6 ++++
>  drivers/thermal/intel/intel_hfi.c | 52 +++++++++++++++++++++++++++++--
>  3 files changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/hfi.h b/arch/x86/include/asm/hfi.h
> index 02ee56dbaeb6..b7fda3e0e8c8 100644
> --- a/arch/x86/include/asm/hfi.h
> +++ b/arch/x86/include/asm/hfi.h
> @@ -23,6 +23,15 @@ union cpuid6_edx {
>  	u32 full;
>  };
>  
> +union cpuid6_ecx {
> +	struct {
> +		u32	dont_care0:8;
> +		u32	nr_classes:8;
> +		u32	dont_care1:16;
> +	} split;
> +	u32 full;
> +};
> +
>  /**
>   * struct hfi_hdr - Header of the HFI table
>   * @perf_updated:	Hardware updated performance capabilities
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 46983fb0b5b3..d74932a0778d 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -1148,6 +1148,12 @@
>  
>  #define MSR_IA32_HW_FEEDBACK_CONFIG     0x17d1
>  #define HW_FEEDBACK_CONFIG_HFI_ENABLE   BIT_ULL(0)
> +#define HW_FEEDBACK_CONFIG_ITD_ENABLE   BIT_ULL(1)
> +
> +#define MSR_IA32_HW_FEEDBACK_THREAD_CONFIG 0x17d4
> +#define HW_FEEDBACK_THREAD_CONFIG_ENABLE BIT_ULL(0)
> +
> +#define MSR_IA32_HW_FEEDBACK_CHAR       0x17d2
>  
>  /* x2APIC locked status */
>  #define MSR_IA32_XAPIC_DISABLE_STATUS	0xBD
> diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c
> index 3c399f3d059f..b69fa234b317 100644
> --- a/drivers/thermal/intel/intel_hfi.c
> +++ b/drivers/thermal/intel/intel_hfi.c
> @@ -33,6 +33,7 @@
>  #include <linux/percpu-defs.h>
>  #include <linux/printk.h>
>  #include <linux/processor.h>
> +#include <linux/sched/topology.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/suspend.h>
> @@ -298,6 +299,10 @@ static void hfi_enable(void)
>  
>  	rdmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
>  	msr_val |= HW_FEEDBACK_CONFIG_HFI_ENABLE;
> +
> +	if (cpu_feature_enabled(X86_FEATURE_ITD))
> +		msr_val |= HW_FEEDBACK_CONFIG_ITD_ENABLE;
> +
>  	wrmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
>  }
>  
> @@ -319,6 +324,10 @@ static void hfi_disable(void)
>  
>  	rdmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
>  	msr_val &= ~HW_FEEDBACK_CONFIG_HFI_ENABLE;
> +
> +	if (cpu_feature_enabled(X86_FEATURE_ITD))
> +		msr_val &= ~HW_FEEDBACK_CONFIG_ITD_ENABLE;
> +
>  	wrmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
>  
>  	/*
> @@ -337,6 +346,30 @@ static void hfi_disable(void)
>  	}
>  }
>  
> +static void hfi_enable_itd_classification(void)
> +{
> +	u64 msr_val;
> +
> +	if (!cpu_feature_enabled(X86_FEATURE_ITD))
> +		return;
> +
> +	rdmsrl(MSR_IA32_HW_FEEDBACK_THREAD_CONFIG, msr_val);
> +	msr_val |= HW_FEEDBACK_THREAD_CONFIG_ENABLE;
> +	wrmsrl(MSR_IA32_HW_FEEDBACK_THREAD_CONFIG, msr_val);
> +}
> +
> +static void hfi_disable_itd_classification(void)
> +{
> +	u64 msr_val;
> +
> +	if (!cpu_feature_enabled(X86_FEATURE_ITD))
> +		return;
> +
> +	rdmsrl(MSR_IA32_HW_FEEDBACK_THREAD_CONFIG, msr_val);
> +	msr_val &= ~HW_FEEDBACK_THREAD_CONFIG_ENABLE;
> +	wrmsrl(MSR_IA32_HW_FEEDBACK_THREAD_CONFIG, msr_val);
> +}
> +
>  /**
>   * intel_hfi_online() - Enable HFI on @cpu
>   * @cpu:	CPU in which the HFI will be enabled
> @@ -377,6 +410,8 @@ void intel_hfi_online(unsigned int cpu)
>  
>  	init_hfi_cpu_index(info);
>  
> +	hfi_enable_itd_classification();
> +
>  	/*
>  	 * Now check if the HFI instance of the package/die of @cpu has been
>  	 * initialized (by checking its header). In such case, all we have to
> @@ -460,6 +495,8 @@ void intel_hfi_offline(unsigned int cpu)
>  	if (!hfi_instance->local_table.hdr)
>  		return;
>  
> +	hfi_disable_itd_classification();
> +
>  	mutex_lock(&hfi_instance_lock);
>  	cpumask_clear_cpu(cpu, hfi_instance->cpus);
>  
> @@ -505,8 +542,14 @@ static __init int hfi_parse_features(void)
>  	 */
>  	hfi_features.class_stride = nr_capabilities;
>  
> -	/* For now, use only one class of the HFI table */
> -	hfi_features.nr_classes = 1;
> +	if (cpu_feature_enabled(X86_FEATURE_ITD)) {
> +		union cpuid6_ecx ecx;
> +
> +		ecx.full = cpuid_ecx(CPUID_HFI_LEAF);
> +		hfi_features.nr_classes = ecx.split.nr_classes;
> +	} else {
> +		hfi_features.nr_classes = 1;
> +	}
>  
>  	/*
>  	 * The header contains change indications for each supported feature.
> @@ -535,11 +578,16 @@ static void hfi_do_enable(void)
>  	/* No locking needed. There is no concurrency with CPU online. */
>  	hfi_set_hw_table(hfi_instance);
>  	hfi_enable();
> +
> +	hfi_enable_itd_classification();
>  }
>  
>  static int hfi_do_disable(void)
>  {
>  	/* No locking needed. There is no concurrency with CPU offline. */
> +
> +	hfi_disable_itd_classification();
> +
>  	hfi_disable();
>  
>  	return 0;
> -- 
> 2.25.1
> 