| Message ID | 1466012148-7674-5-git-send-email-james.morse@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 15, 2016 at 7:35 PM, James Morse <james.morse@arm.com> wrote: > On arm64 the cpu with logical id 0 is assumed to be the boot CPU. If a > user hotplugs this CPU out, then uses kexec to boot a new kernel, the new > kernel will assign logical id 0 to a different physical CPU. > This breaks hibernate as hibernate and resume will be attempted on different > CPUs. > > Define a weak symbol arch_hibernation_disable_cpus(), which defaults to > calling disable_nonboot_cpus). Architectures that allow CPU 0 to be > hotplugged can use this to control which CPU is used for hibernate/resume. > > Signed-off-by: James Morse <james.morse@arm.com> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Pavel Machek <pavel@ucw.cz> > --- > If this approach is acceptable, this patch should go with 4&5 via the arm64 > tree. I'm not entirely happy with this, but if you absolutely need it and if you can't think about any cleaner way to achieve the goal, feel free to add my ACK to this patch if necessary. > kernel/power/hibernate.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c > index fca9254280ee..0e668a3207ec 100644 > --- a/kernel/power/hibernate.c > +++ b/kernel/power/hibernate.c > @@ -70,6 +70,16 @@ bool hibernation_available(void) > } > > /** > + * arch_hibernation_disable_cpus - Allow architectures to direct which CPU > + * is used for suspend or resume. > + * @suspend: True during hibernate, false for resume. > + */ > +int __weak arch_hibernation_disable_cpus(__maybe_unused bool suspend) > +{ > + return disable_nonboot_cpus(); > +} > + > +/** > * hibernation_set_ops - Set the global hibernate operations. > * @ops: Hibernation operations to use in subsequent hibernation transitions. > */ > @@ -279,7 +289,7 @@ static int create_image(int platform_mode) > if (error || hibernation_test(TEST_PLATFORM)) > goto Platform_finish; > > - error = disable_nonboot_cpus(); > + error = arch_hibernation_disable_cpus(true); > if (error || hibernation_test(TEST_CPUS)) > goto Enable_cpus; > > @@ -433,7 +443,7 @@ static int resume_target_kernel(bool platform_mode) > if (error) > goto Cleanup; > > - error = disable_nonboot_cpus(); > + error = arch_hibernation_disable_cpus(false); > if (error) > goto Enable_cpus; > > @@ -551,7 +561,7 @@ int hibernation_platform_enter(void) > if (error) > goto Platform_finish; > > - error = disable_nonboot_cpus(); > + error = arch_hibernation_disable_cpus(true); > if (error) > goto Enable_cpus; > > --
Hi Rafael, On 15/06/16 22:10, Rafael J. Wysocki wrote: > On Wed, Jun 15, 2016 at 7:35 PM, James Morse <james.morse@arm.com> wrote: >> On arm64 the cpu with logical id 0 is assumed to be the boot CPU. If a >> user hotplugs this CPU out, then uses kexec to boot a new kernel, the new >> kernel will assign logical id 0 to a different physical CPU. >> This breaks hibernate as hibernate and resume will be attempted on different >> CPUs. >> >> Define a weak symbol arch_hibernation_disable_cpus(), which defaults to >> calling disable_nonboot_cpus). Architectures that allow CPU 0 to be >> hotplugged can use this to control which CPU is used for hibernate/resume. >> >> Signed-off-by: James Morse <james.morse@arm.com> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >> Cc: Pavel Machek <pavel@ucw.cz> >> --- >> If this approach is acceptable, this patch should go with 4&5 via the arm64 >> tree. > > I'm not entirely happy with this, but if you absolutely need it and if > you can't think about any cleaner way to achieve the goal, feel free > to add my ACK to this patch if necessary. I would prefer you to be happy with it. On the weak symbol: After staring at this some more I've made the macro approach work, so the weak symbol can go. (a new version of this series should arrive soon) On the arch-specific odour coming from this, it may also be useful for Chen Yu's 'Use hlt instead of mwait when resuming from hibernation' series[0]. (I don't understand the x86 specifics) We need this to allow kexec and hibernate to interact freely. Preventing kexec if CPU0 is offline because of hibernate doesn't feel right. Fixing it is necessary as the first hint that something is wrong is when we fail to resume and the user looses any data they had in memory. Thanks, James [0] https://lkml.org/lkml/2016/6/25/98
On Tuesday, June 28, 2016 03:51:39 PM James Morse wrote: > Hi Rafael, > > On 15/06/16 22:10, Rafael J. Wysocki wrote: > > On Wed, Jun 15, 2016 at 7:35 PM, James Morse <james.morse@arm.com> wrote: > >> On arm64 the cpu with logical id 0 is assumed to be the boot CPU. If a > >> user hotplugs this CPU out, then uses kexec to boot a new kernel, the new > >> kernel will assign logical id 0 to a different physical CPU. > >> This breaks hibernate as hibernate and resume will be attempted on different > >> CPUs. > >> > >> Define a weak symbol arch_hibernation_disable_cpus(), which defaults to > >> calling disable_nonboot_cpus). Architectures that allow CPU 0 to be > >> hotplugged can use this to control which CPU is used for hibernate/resume. > >> > >> Signed-off-by: James Morse <james.morse@arm.com> > >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > >> Cc: Pavel Machek <pavel@ucw.cz> > >> --- > >> If this approach is acceptable, this patch should go with 4&5 via the arm64 > >> tree. > > > > I'm not entirely happy with this, but if you absolutely need it and if > > you can't think about any cleaner way to achieve the goal, feel free > > to add my ACK to this patch if necessary. > > I would prefer you to be happy with it. > On the weak symbol: After staring at this some more I've made the macro approach > work, so the weak symbol can go. (a new version of this series should arrive soon) > On the arch-specific odour coming from this, it may also be useful for Chen Yu's > 'Use hlt instead of mwait when resuming from hibernation' series[0]. (I don't > understand the x86 specifics) Yes, it looks like both x86 and ARM64 want/need to do something special around disabling/enabling nonboot CPUs during hibernation/restore. Let me comment on the new submission, though. Thanks, Rafael
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c index fca9254280ee..0e668a3207ec 100644 --- a/kernel/power/hibernate.c +++ b/kernel/power/hibernate.c @@ -70,6 +70,16 @@ bool hibernation_available(void) } /** + * arch_hibernation_disable_cpus - Allow architectures to direct which CPU + * is used for suspend or resume. + * @suspend: True during hibernate, false for resume. + */ +int __weak arch_hibernation_disable_cpus(__maybe_unused bool suspend) +{ + return disable_nonboot_cpus(); +} + +/** * hibernation_set_ops - Set the global hibernate operations. * @ops: Hibernation operations to use in subsequent hibernation transitions. */ @@ -279,7 +289,7 @@ static int create_image(int platform_mode) if (error || hibernation_test(TEST_PLATFORM)) goto Platform_finish; - error = disable_nonboot_cpus(); + error = arch_hibernation_disable_cpus(true); if (error || hibernation_test(TEST_CPUS)) goto Enable_cpus; @@ -433,7 +443,7 @@ static int resume_target_kernel(bool platform_mode) if (error) goto Cleanup; - error = disable_nonboot_cpus(); + error = arch_hibernation_disable_cpus(false); if (error) goto Enable_cpus; @@ -551,7 +561,7 @@ int hibernation_platform_enter(void) if (error) goto Platform_finish; - error = disable_nonboot_cpus(); + error = arch_hibernation_disable_cpus(true); if (error) goto Enable_cpus;
On arm64 the cpu with logical id 0 is assumed to be the boot CPU. If a user hotplugs this CPU out, then uses kexec to boot a new kernel, the new kernel will assign logical id 0 to a different physical CPU. This breaks hibernate as hibernate and resume will be attempted on different CPUs. Define a weak symbol arch_hibernation_disable_cpus(), which defaults to calling disable_nonboot_cpus). Architectures that allow CPU 0 to be hotplugged can use this to control which CPU is used for hibernate/resume. Signed-off-by: James Morse <james.morse@arm.com> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Pavel Machek <pavel@ucw.cz> --- If this approach is acceptable, this patch should go with 4&5 via the arm64 tree. kernel/power/hibernate.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)