| 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: <CAAhSdy3+0dReF9mj2aLo725Yc6co3sKNjjRw2QUCe0K_iWipFQ@mail.gmail.com>
Date: Wed, 23 Aug 2023 19:14:23 +0530
From: Anup Patel <anup@...infault.org>
To: Nick Hu <nick.hu@...ive.com>
Cc: paul.walmsley@...ive.com, palmer@...belt.com,
aou@...s.berkeley.edu, leyfoon.tan@...rfivetech.com,
mason.huo@...rfivetech.com, conor.dooley@...rochip.com,
jeeheng.sia@...rfivetech.com, linux-riscv@...ts.infradead.org,
linux-kernel@...r.kernel.org, zong.li@...ive.com,
Andy Chiu <andy.chiu@...ive.com>,
Andrew Jones <ajones@...tanamicro.com>
Subject: Re: [PATCH v2] riscv: suspend: Add syscore ops for suspend
On Wed, Aug 23, 2023 at 8:38 AM Nick Hu <nick.hu@...ive.com> wrote:
>
> Hi Anup
>
> Thanks for your feedbacks
> On Tue, Aug 22, 2023 at 5:17 PM Anup Patel <anup@...infault.org> wrote:
> >
> > On Tue, Aug 22, 2023 at 1:28 PM Nick Hu <nick.hu@...ive.com> wrote:
> > >
> > > Hi Anup
> > >
> > >
> > > On Tue, Aug 22, 2023 at 2:16 PM Anup Patel <anup@...infault.org> wrote:
> > > >
> > > > On Tue, Aug 22, 2023 at 11:17 AM Nick Hu <nick.hu@...ive.com> wrote:
> > > > >
> > > > > If a task is the one who performs the suspend flow and it also do some
> > > > > floating or vector operations before the suspend, we might lose the FPU
> > > > > and vector states when it goes to the non-retention system suspend state.
> > > > > Add syscore ops to save and restore the FPU and vector states of the
> > > > > current task to fix the issue above.
> > > >
> > > > This only applies to non-retentive system suspend so why do we need
> > > > this before SBI system suspend is merged ?
> > > >
> > > > Regards,
> > > > Anup
> > > >
> > > >
> > > How about hibernation?
> >
> > If this is for hibernation then the commit description should say so.
> >
> Actually this commit is for the suspend-to-ram. I ask about
> hibernation here because I think hibernation may also have the same
> issue too.
> If hibernation doesn't need it, I can defer this patch until we have
> system suspend.
> I send this patch because I believe we will support system suspend in
> the kernel soon or later and the hibernation might need it now.
Okay, understood.
>
> > Adding syscore_ops would mean these callbacks will be called for
> > both suspend-to-ram and hibernate cases. Other architectures don't
> > have this save/restore for suspend-to-ram because it is generally
> > called from idle task.
> >
> Do you think we don't have to consider the case in the patch?
I was comparing other architectures but I did not find any explicit
saving of FP state for suspend-to-ram. Although, I did find explicit
FP save for hibernate.
Functionally, it is fine to always save FP state for suspend-to-ram
but I am wondering if this redundant work in suspend-to-ram path.
Regards,
Anup
>
> Regards,
> Nick
> > Why not do the save/restore in save_processor_state() and
> > restore_processor_state() just like arch/mips/power/cpu.c ?
> >
> > Regards,
> > Anup
> >
> >
> > >
> > > Regards,
> > > Nick
> > > > >
> > > > > Co-developed-by: Andy Chiu <andy.chiu@...ive.com>
> > > > > Signed-off-by: Andy Chiu <andy.chiu@...ive.com>
> > > > > Signed-off-by: Nick Hu <nick.hu@...ive.com>
> > > > > ---
> > > > > Changes in v2:
> > > > > a) Add Co-developed-by and adjust the order of signed-off
> > > > > b) Rephrase the commit message
> > > > >
> > > > > arch/riscv/kernel/suspend.c | 45 +++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 45 insertions(+)
> > > > >
> > > > > diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
> > > > > index 3c89b8ec69c4..ff69ff8a1974 100644
> > > > > --- a/arch/riscv/kernel/suspend.c
> > > > > +++ b/arch/riscv/kernel/suspend.c
> > > > > @@ -4,9 +4,14 @@
> > > > > * Copyright (c) 2022 Ventana Micro Systems Inc.
> > > > > */
> > > > >
> > > > > +#include <linux/cpu_pm.h>
> > > > > #include <linux/ftrace.h>
> > > > > +#include <linux/thread_info.h>
> > > > > +#include <linux/syscore_ops.h>
> > > > > #include <asm/csr.h>
> > > > > #include <asm/suspend.h>
> > > > > +#include <asm/switch_to.h>
> > > > > +#include <asm/vector.h>
> > > > >
> > > > > void suspend_save_csrs(struct suspend_context *context)
> > > > > {
> > > > > @@ -85,3 +90,43 @@ int cpu_suspend(unsigned long arg,
> > > > >
> > > > > return rc;
> > > > > }
> > > > > +
> > > > > +static int riscv_cpu_suspend(void)
> > > > > +{
> > > > > + struct task_struct *cur_task = get_current();
> > > > > + struct pt_regs *regs = task_pt_regs(cur_task);
> > > > > +
> > > > > + if (has_fpu()) {
> > > > > + if (unlikely(regs->status & SR_SD))
> > > > > + fstate_save(cur_task, regs);
> > > > > + }
> > > > > + if (has_vector()) {
> > > > > + if (unlikely(regs->status & SR_SD))
> > > > > + riscv_v_vstate_save(cur_task, regs);
> > > > > + }
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static void riscv_cpu_resume(void)
> > > > > +{
> > > > > + struct task_struct *cur_task = get_current();
> > > > > + struct pt_regs *regs = task_pt_regs(cur_task);
> > > > > +
> > > > > + if (has_fpu())
> > > > > + fstate_restore(cur_task, regs);
> > > > > + if (has_vector())
> > > > > + riscv_v_vstate_restore(cur_task, regs);
> > > > > +}
> > > > > +
> > > > > +static struct syscore_ops riscv_cpu_syscore_ops = {
> > > > > + .suspend = riscv_cpu_suspend,
> > > > > + .resume = riscv_cpu_resume,
> > > > > +};
> > > > > +
> > > > > +static int __init riscv_cpu_suspend_init(void)
> > > > > +{
> > > > > + register_syscore_ops(&riscv_cpu_syscore_ops);
> > > > > + return 0;
> > > > > +}
> > > > > +arch_initcall(riscv_cpu_suspend_init);
> > > > > --
> > > > > 2.34.1
> > > > >
Powered by blists - more mailing lists