| Message ID | 20240203040515.23947-7-ricardo.neri-calderon@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | thermal: intel: hfi: Prework for the virtualization of HFI | expand |
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 >
On Mon, Feb 05, 2024 at 11:28:47AM +0100, Stanislaw Gruszka wrote: > 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 ? You are correct. ITD needs to be enabled unconditionally iff we can't know when a guest machine is running. > > > 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. Agreed, Zhao had suggested to enable/disable when virtual machines are built/destroyed. > QEMU registering to thermal events seems to be odd for > me, and I think there must be better solution. Right.
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;