| 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
| ||
|
Date: Mon, 1 Jan 2024 15:12:41 -0800
From: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
Chen Yu <yu.c.chen@...el.com>, Len Brown <len.brown@...el.com>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
Stanislaw Gruszka <stanislaw.gruszka@...ux.intel.com>,
Zhang Rui <rui.zhang@...el.com>, Zhao Liu <zhao1.liu@...el.com>,
linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
Zhao Liu <zhao1.liu@...ux.intel.com>
Subject: Re: [PATCH 1/4] thermal: intel: hfi: Refactor enabling code into
helper functions
On Fri, Dec 29, 2023 at 06:22:25PM +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 27, 2023 at 7:28 AM Ricardo Neri
> <ricardo.neri-calderon@...ux.intel.com> wrote:
> >
> > In preparation to add a suspend notifier, wrap the logic to enable HFI and
> > program its memory buffer into helper functions. Both the CPU hotplug
> > callback and the suspend notifier will use it.
>
> No functional impact?
Thank you for your review!
Correct. There is no functional impact. I will update the commit message
in my next version.
>
> > Cc: Chen Yu <yu.c.chen@...el.com>
> > Cc: Len Brown <len.brown@...el.com>
> > Cc: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
> > Cc: Stanislaw Gruszka <stanislaw.gruszka@...ux.intel.com>
> > Cc: Zhang Rui <rui.zhang@...el.com>
> > Cc: Zhao Liu <zhao1.liu@...ux.intel.com>
> > Cc: linux-pm@...r.kernel.org
> > Cc: stable@...r.kernel.org
>
> Please don't CC stable@...r on patch submissions, although you may add
> a Cc: stable tag without actually CCing it for my information, but in
> that case please add a full tag including the earliest stable series
> the patch is intended to apply to.
I see. Sure Rafael, I can do this.
>
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
> > ---
> > drivers/thermal/intel/intel_hfi.c | 46 +++++++++++++++++--------------
> > 1 file changed, 25 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c
> > index c69db6c90869..87ac7b196981 100644
> > --- a/drivers/thermal/intel/intel_hfi.c
> > +++ b/drivers/thermal/intel/intel_hfi.c
> > @@ -347,6 +347,25 @@ static void init_hfi_instance(struct hfi_instance *hfi_instance)
> > hfi_instance->data = hfi_instance->hdr + hfi_features.hdr_size;
> > }
> >
> > +static void hfi_enable(void)
> > +{
> > + u64 msr_val;
> > +
> > + rdmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
> > + msr_val |= HW_FEEDBACK_CONFIG_HFI_ENABLE_BIT;
> > + wrmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
> > +}
> > +
> > +static void hfi_set_hw_table(struct hfi_instance *hfi_instance)
> > +{
> > + phys_addr_t hw_table_pa;
> > + u64 msr_val;
> > +
> > + hw_table_pa = virt_to_phys(hfi_instance->hw_table);
> > + msr_val = hw_table_pa | HW_FEEDBACK_PTR_VALID_BIT;
> > + wrmsrl(MSR_IA32_HW_FEEDBACK_PTR, msr_val);
> > +}
> > +
> > /**
> > * intel_hfi_online() - Enable HFI on @cpu
> > * @cpu: CPU in which the HFI will be enabled
> > @@ -364,8 +383,6 @@ void intel_hfi_online(unsigned int cpu)
> > {
> > struct hfi_instance *hfi_instance;
> > struct hfi_cpu_info *info;
> > - phys_addr_t hw_table_pa;
> > - u64 msr_val;
> > u16 die_id;
> >
> > /* Nothing to do if hfi_instances are missing. */
> > @@ -403,14 +420,16 @@ void intel_hfi_online(unsigned int cpu)
> > /*
> > * Hardware is programmed with the physical address of the first page
> > * frame of the table. Hence, the allocated memory must be page-aligned.
> > + *
> > + * Some processors do not forget the initial address of the HFI table
> > + * even after having been reprogrammed. Keep using the same pages. Do
> > + * not free them.
>
> This comment change does not seem to belong to this patch. I guess it
> needs to go to one of the subsequent patches?
My intention was is to relocate here the comment I deleted in the
subsequent hunk (after some rewording). Sure, I can put this comment in
patch 3/4, which deals with disabling HFI.
>
> > */
> > hfi_instance->hw_table = alloc_pages_exact(hfi_features.nr_table_pages,
> > GFP_KERNEL | __GFP_ZERO);
> > if (!hfi_instance->hw_table)
> > goto unlock;
> >
> > - hw_table_pa = virt_to_phys(hfi_instance->hw_table);
> > -
> > /*
> > * Allocate memory to keep a local copy of the table that
> > * hardware generates.
> > @@ -420,16 +439,6 @@ void intel_hfi_online(unsigned int cpu)
> > if (!hfi_instance->local_table)
> > goto free_hw_table;
> >
> > - /*
> > - * Program the address of the feedback table of this die/package. On
> > - * some processors, hardware remembers the old address of the HFI table
> > - * even after having been reprogrammed and re-enabled. Thus, do not free
> > - * the pages allocated for the table or reprogram the hardware with a
> > - * new base address. Namely, program the hardware only once.
> > - */
> > - msr_val = hw_table_pa | HW_FEEDBACK_PTR_VALID_BIT;
> > - wrmsrl(MSR_IA32_HW_FEEDBACK_PTR, msr_val);
> > -
> > init_hfi_instance(hfi_instance);
> >
> > INIT_DELAYED_WORK(&hfi_instance->update_work, hfi_update_work_fn);
> > @@ -438,13 +447,8 @@ void intel_hfi_online(unsigned int cpu)
> >
> > cpumask_set_cpu(cpu, hfi_instance->cpus);
> >
> > - /*
> > - * Enable the hardware feedback interface and never disable it. See
> > - * comment on programming the address of the table.
> > - */
> > - rdmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
> > - msr_val |= HW_FEEDBACK_CONFIG_HFI_ENABLE_BIT;
> > - wrmsrl(MSR_IA32_HW_FEEDBACK_CONFIG, msr_val);
> > + hfi_set_hw_table(hfi_instance);
> > + hfi_enable();
> >
> > unlock:
> > mutex_unlock(&hfi_instance_lock);
> > --
Powered by blists - more mailing lists