| 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
| ||
|
Message-ID: <20240104030215.GA12718@ranerica-svr.sc.intel.com>
Date: Wed, 3 Jan 2024 19:02:15 -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
Subject: Re: [PATCH v2 4/4] thermal: intel: hfi: Add a suspend notifier
On Wed, Jan 03, 2024 at 02:38:26PM +0100, Rafael J. Wysocki wrote:
> On Wed, Jan 3, 2024 at 2:34 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
> >
> > The subject should say "add a PM notifier" to indicate that
> > hibernation is covered too.
> >
> > On Wed, Jan 3, 2024 at 5:13 AM Ricardo Neri
> > <ricardo.neri-calderon@...ux.intel.com> wrote:
> > >
> > > The kernel gives the HFI hardware a memory region that the latter uses to
> > > provide updates to the HFI table. The kernel allocates this memory region
> > > at boot. It remains constant throughout runtime time.
> > >
> > > When resuming from suspend or hibernation, the restore kernel allocates a
> >
> > The restore kernel is only used during resume from hibernation, so
> > this particular problem is hibernation-specific.
> >
> > It is possible, at least in principle, that the address of the HFI
> > table is "lost" by the processor during resume from "deep" suspend
> > (ACPI S3), in which case it may not survive the firmware-driven part
> > of the suspend-resume cycle. It is thus prudent to disable HFI on
> > suspend and re-enable it on resume for the boot CPU (under the
> > assumption that the other CPUs will be taken care of by CPU offline),
> > but for a somewhat different reason than in the hibernation case.
> >
> > > second memory buffer and reprograms the HFI hardware with the new location
> > > as part of a normal boot. The location of the second memory buffer may
> > > differ from the one allocated by the image kernel. Subsequently, when the
> > > restore kernel transfers control to the image kernel, the second buffer
> > > becomes invalid, potentially leading to memory corruption if the hardware
> > > writes to it (hardware continues using the buffer from the restore kernel).
> > >
> > > Add a suspend notifier to disable all HFI instances before jumping to the
> > > image kernel and enable them once the image kernel has been restored. Use
> > > the memory buffer that the image kernel allocated.
> > >
> > > For non-boot CPUs, rely on the CPU hotplug callbacks as CPUs are disabled
> > > and enabled during suspend and resume, respectively.
> > >
> > > The CPU hotplug callbacks do not cover the boot CPU. Handle the HFI
> > > instance of the boot CPU from the suspend notifier callback.
> > >
> > > 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 # 6.1
> > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
> > > --
> > > Changes since v1:
> > > * Moved registration of the suspend notifier towards the end of
> > > intel_hfi_init(). (Stan)
> > > * Renamed hfi_do_pm_[enable|disable]() to hfi_do_[enable|disable](). Stan
> > > will use these functions outside the suspend notifier. (Stan)
> > > * Added locking to calls to hfi_[enable|disable]() from the suspend
> > > notifier. (Rafael)
> > > ---
> > > drivers/thermal/intel/intel_hfi.c | 62 +++++++++++++++++++++++++++++++
> > > 1 file changed, 62 insertions(+)
> > >
> > > diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c
> > > index 22445403b520..8d6e4f8dc67a 100644
> > > --- a/drivers/thermal/intel/intel_hfi.c
> > > +++ b/drivers/thermal/intel/intel_hfi.c
> > > @@ -30,11 +30,13 @@
> > > #include <linux/kernel.h>
> > > #include <linux/math.h>
> > > #include <linux/mutex.h>
> > > +#include <linux/notifier.h>
> > > #include <linux/percpu-defs.h>
> > > #include <linux/printk.h>
> > > #include <linux/processor.h>
> > > #include <linux/slab.h>
> > > #include <linux/spinlock.h>
> > > +#include <linux/suspend.h>
> > > #include <linux/string.h>
> > > #include <linux/topology.h>
> > > #include <linux/workqueue.h>
> > > @@ -571,6 +573,60 @@ static __init int hfi_parse_features(void)
> > > return 0;
> > > }
> > >
> > > +static void hfi_do_enable(void *info)
> > > +{
> > > + struct hfi_instance *hfi_instance = info;
> > > +
> > > + hfi_set_hw_table(hfi_instance);
> > > + hfi_enable();
> > > +}
> > > +
> > > +static void hfi_do_disable(void *info)
> > > +{
> > > + hfi_disable();
> > > +}
> > > +
> > > +static int hfi_pm_notify(struct notifier_block *nb,
> > > + unsigned long mode, void *unused)
> > > +{
> > > + struct hfi_cpu_info *info = &per_cpu(hfi_cpu_info, 0);
> > > + struct hfi_instance *hfi = info->hfi_instance;
> > > + int ret = 0;
> > > +
> > > + /* HFI may not be in use. */
> > > + if (!hfi)
> > > + return ret;
> > > +
> > > + mutex_lock(&hfi_instance_lock);
> > > + /*
> > > + * Only handle the HFI instance of the package of the boot CPU. The
> > > + * instances of other packages are handled in the CPU hotplug callbacks.
> > > + */
> > > + switch (mode) {
> > > + case PM_HIBERNATION_PREPARE:
> > > + case PM_SUSPEND_PREPARE:
> > > + case PM_RESTORE_PREPARE:
> > > + ret = smp_call_function_single(0, hfi_do_disable, NULL, true);
> > > + break;
> > > +
> > > + case PM_POST_RESTORE:
> > > + case PM_POST_HIBERNATION:
> > > + case PM_POST_SUSPEND:
> > > + ret = smp_call_function_single(0, hfi_do_enable, hfi, true);
> > > + break;
> >
> > Because this handles the boot CPU only, one has to wonder if it should
> > be a syscore op rather than a PM notifier.
> >
> > It does not sleep AFAICS, so it can run in that context, and it is
> > guaranteed to run on the boot CPU then, so it is not necessary to
> > force that. Moreover, syscore ops are guaranteed to be
> > non-concurrent, so locking is not needed.
> >
> > In addition, disabling HFI from a PM notifier is generally observable
> > by user space, because PM notifiers run before user space is frozen,
> > but doing it from a syscore op wouldn't be.
Yes, we only have to handle the boot CPU. The rest are handled via CPU
offline. Then syscore ops look like a good fit for me.
>
> One more thing: PM notifiers run on all variants of system suspend and
> resume, including suspend-to-idle in which case HFI need not be
> disabled/enabled IIUC and syscore ops only run in hibernation and
> "deep" suspend cycles, so they cover the cases in which the special
> handling is really needed and don't add useless overhead otherwise.
I verified that the HFI configuration survives suspend-to-idle. No extra
handling is needed.
Powered by blists - more mailing lists