Re: [PATCH v5 5/5] Auto-detect whether a FPU exists
On Thu, Aug 09, 2018 at 12:02:58AM -0700, Christoph Hellwig wrote: > On Thu, Aug 09, 2018 at 02:43:36PM +0800, Alan Kao wrote: > > It does look a little bit weird. Should I send a v6 for this? > > Yes, please resend the series or just this patch. > > I think the hswap.h definition should go away and we should just > keep the switch_to.h one, even if that means including the header > in another C file. > It turns out that only cpufeature.c and switch_to.h are affected by hwcap.h. As switch_to.h already had extern has_fpu declaration, the one in hwcap.h is redundant and can be removed safely. I will resend just this patch, and mark it as v6.
Re: [PATCH v5 5/5] Auto-detect whether a FPU exists
On Thu, Aug 09, 2018 at 12:02:58AM -0700, Christoph Hellwig wrote: > On Thu, Aug 09, 2018 at 02:43:36PM +0800, Alan Kao wrote: > > It does look a little bit weird. Should I send a v6 for this? > > Yes, please resend the series or just this patch. > > I think the hswap.h definition should go away and we should just > keep the switch_to.h one, even if that means including the header > in another C file. > It turns out that only cpufeature.c and switch_to.h are affected by hwcap.h. As switch_to.h already had extern has_fpu declaration, the one in hwcap.h is redundant and can be removed safely. I will resend just this patch, and mark it as v6.
Re: [PATCH v5 5/5] Auto-detect whether a FPU exists
On Thu, Aug 09, 2018 at 02:43:36PM +0800, Alan Kao wrote: > It does look a little bit weird. Should I send a v6 for this? Yes, please resend the series or just this patch. I think the hswap.h definition should go away and we should just keep the switch_to.h one, even if that means including the header in another C file.
Re: [PATCH v5 5/5] Auto-detect whether a FPU exists
On Thu, Aug 09, 2018 at 02:43:36PM +0800, Alan Kao wrote: > It does look a little bit weird. Should I send a v6 for this? Yes, please resend the series or just this patch. I think the hswap.h definition should go away and we should just keep the switch_to.h one, even if that means including the header in another C file.
Re: [PATCH v5 5/5] Auto-detect whether a FPU exists
On Wed, Aug 08, 2018 at 11:31:24PM -0700, Christoph Hellwig wrote: > > extern unsigned long elf_hwcap; > > +extern bool has_fpu; > > #endif > > Doesn't this conflict with the !CONFIG_CPU stub in switch_to.h? switch_to.h did include asm/hwcap.h, but the !CONFIG_FPU stub +#define has_fpu false always shows later than +extern bool has fpu so actually no warning during compilation. > > It seems like we should only have this definition in one place to start > with. It does look a little bit weird. Should I send a v6 for this? > > Otherwise this looks fine to me: > > Reviewed-by: Christoph Hellwig > Thanks for all the feedback. Alan
Re: [PATCH v5 5/5] Auto-detect whether a FPU exists
On Wed, Aug 08, 2018 at 11:31:24PM -0700, Christoph Hellwig wrote: > > extern unsigned long elf_hwcap; > > +extern bool has_fpu; > > #endif > > Doesn't this conflict with the !CONFIG_CPU stub in switch_to.h? switch_to.h did include asm/hwcap.h, but the !CONFIG_FPU stub +#define has_fpu false always shows later than +extern bool has fpu so actually no warning during compilation. > > It seems like we should only have this definition in one place to start > with. It does look a little bit weird. Should I send a v6 for this? > > Otherwise this looks fine to me: > > Reviewed-by: Christoph Hellwig > Thanks for all the feedback. Alan
Re: [PATCH v5 5/5] Auto-detect whether a FPU exists
> extern unsigned long elf_hwcap; > +extern bool has_fpu; > #endif Doesn't this conflict with the !CONFIG_CPU stub in switch_to.h? It seems like we should only have this definition in one place to start with. Otherwise this looks fine to me: Reviewed-by: Christoph Hellwig
Re: [PATCH v5 5/5] Auto-detect whether a FPU exists
> extern unsigned long elf_hwcap; > +extern bool has_fpu; > #endif Doesn't this conflict with the !CONFIG_CPU stub in switch_to.h? It seems like we should only have this definition in one place to start with. Otherwise this looks fine to me: Reviewed-by: Christoph Hellwig
[PATCH v5 5/5] Auto-detect whether a FPU exists
We expect that a kernel with CONFIG_FPU=y can still support no-FPU
machines. To do so, the kernel should first examine the existence of a
FPU, then do nothing if a FPU does exist; otherwise, it should
disable/bypass all FPU-related functions.
In this patch, a new global variable, has_fpu, is created and determined
when parsing the hardware capability from device tree during booting.
This variable is used in those FPU-related functions.
Signed-off-by: Alan Kao
Cc: Greentime Hu
Cc: Vincent Chen
Cc: Zong Li
Cc: Nick Hu
---
arch/riscv/include/asm/hwcap.h | 1 +
arch/riscv/include/asm/switch_to.h | 9 +
arch/riscv/kernel/cpufeature.c | 8
arch/riscv/kernel/process.c| 4 +++-
arch/riscv/kernel/signal.c | 6 --
5 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 8a4ed7bbcbea..b0da2cbfb468 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -33,5 +33,6 @@ enum {
};
extern unsigned long elf_hwcap;
+extern bool has_fpu;
#endif
#endif
diff --git a/arch/riscv/include/asm/switch_to.h
b/arch/riscv/include/asm/switch_to.h
index 093050b03543..7943f991bd86 100644
--- a/arch/riscv/include/asm/switch_to.h
+++ b/arch/riscv/include/asm/switch_to.h
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#ifdef CONFIG_FPU
extern void __fstate_save(struct task_struct *save_to);
@@ -56,13 +57,12 @@ static inline void __switch_to_aux(struct task_struct *prev,
fstate_restore(next, task_pt_regs(next));
}
-#define DEFAULT_SSTATUS (SR_SPIE | SR_FS_INITIAL)
-
+extern bool has_fpu;
#else
+#define has_fpu false
#define fstate_save(task, regs) do { } while (0)
#define fstate_restore(task, regs) do { } while (0)
#define __switch_to_aux(__prev, __next) do { } while (0)
-#define DEFAULT_SSTATUS (SR_SPIE | SR_FS_OFF)
#endif
extern struct task_struct *__switch_to(struct task_struct *,
@@ -72,7 +72,8 @@ extern struct task_struct *__switch_to(struct task_struct *,
do { \
struct task_struct *__prev = (prev);\
struct task_struct *__next = (next);\
- __switch_to_aux(__prev, __next);\
+ if (has_fpu)\
+ __switch_to_aux(__prev, __next);\
((last) = __switch_to(__prev, __next)); \
} while (0)
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 17011a870044..46942e635266 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -22,6 +22,9 @@
#include
unsigned long elf_hwcap __read_mostly;
+#ifdef CONFIG_FPU
+bool has_fpu __read_mostly;
+#endif
void riscv_fill_hwcap(void)
{
@@ -58,4 +61,9 @@ void riscv_fill_hwcap(void)
elf_hwcap |= isa2hwcap[(unsigned char)(isa[i])];
pr_info("elf_hwcap is 0x%lx", elf_hwcap);
+
+#ifdef CONFIG_FPU
+ if (elf_hwcap & (COMPAT_HWCAP_ISA_F | COMPAT_HWCAP_ISA_D))
+ has_fpu = true;
+#endif
}
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index 3820d89e2db9..97155aee9e71 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -83,7 +83,9 @@ void show_regs(struct pt_regs *regs)
void start_thread(struct pt_regs *regs, unsigned long pc,
unsigned long sp)
{
- regs->sstatus = DEFAULT_SSTATUS;
+ regs->sstatus = SR_SPIE;
+ if (has_fpu)
+ regs->sstatus |= SR_FS_INITIAL;
regs->sepc = pc;
regs->sp = sp;
set_fs(USER_DS);
diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
index 2450b824d799..f9b5e7e352ef 100644
--- a/arch/riscv/kernel/signal.c
+++ b/arch/riscv/kernel/signal.c
@@ -98,7 +98,8 @@ static long restore_sigcontext(struct pt_regs *regs,
/* sc_regs is structured the same as the start of pt_regs */
err = __copy_from_user(regs, >sc_regs, sizeof(sc->sc_regs));
/* Restore the floating-point state. */
- err |= restore_fp_state(regs, >sc_fpregs);
+ if (has_fpu)
+ err |= restore_fp_state(regs, >sc_fpregs);
return err;
}
@@ -150,7 +151,8 @@ static long setup_sigcontext(struct rt_sigframe __user
*frame,
/* sc_regs is structured the same as the start of pt_regs */
err = __copy_to_user(>sc_regs, regs, sizeof(sc->sc_regs));
/* Save the floating-point state. */
- err |= save_fp_state(regs, >sc_fpregs);
+ if (has_fpu)
+ err |= save_fp_state(regs, >sc_fpregs);
return err;
}
--
2.18.0
[PATCH v5 5/5] Auto-detect whether a FPU exists
We expect that a kernel with CONFIG_FPU=y can still support no-FPU
machines. To do so, the kernel should first examine the existence of a
FPU, then do nothing if a FPU does exist; otherwise, it should
disable/bypass all FPU-related functions.
In this patch, a new global variable, has_fpu, is created and determined
when parsing the hardware capability from device tree during booting.
This variable is used in those FPU-related functions.
Signed-off-by: Alan Kao
Cc: Greentime Hu
Cc: Vincent Chen
Cc: Zong Li
Cc: Nick Hu
---
arch/riscv/include/asm/hwcap.h | 1 +
arch/riscv/include/asm/switch_to.h | 9 +
arch/riscv/kernel/cpufeature.c | 8
arch/riscv/kernel/process.c| 4 +++-
arch/riscv/kernel/signal.c | 6 --
5 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 8a4ed7bbcbea..b0da2cbfb468 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -33,5 +33,6 @@ enum {
};
extern unsigned long elf_hwcap;
+extern bool has_fpu;
#endif
#endif
diff --git a/arch/riscv/include/asm/switch_to.h
b/arch/riscv/include/asm/switch_to.h
index 093050b03543..7943f991bd86 100644
--- a/arch/riscv/include/asm/switch_to.h
+++ b/arch/riscv/include/asm/switch_to.h
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#ifdef CONFIG_FPU
extern void __fstate_save(struct task_struct *save_to);
@@ -56,13 +57,12 @@ static inline void __switch_to_aux(struct task_struct *prev,
fstate_restore(next, task_pt_regs(next));
}
-#define DEFAULT_SSTATUS (SR_SPIE | SR_FS_INITIAL)
-
+extern bool has_fpu;
#else
+#define has_fpu false
#define fstate_save(task, regs) do { } while (0)
#define fstate_restore(task, regs) do { } while (0)
#define __switch_to_aux(__prev, __next) do { } while (0)
-#define DEFAULT_SSTATUS (SR_SPIE | SR_FS_OFF)
#endif
extern struct task_struct *__switch_to(struct task_struct *,
@@ -72,7 +72,8 @@ extern struct task_struct *__switch_to(struct task_struct *,
do { \
struct task_struct *__prev = (prev);\
struct task_struct *__next = (next);\
- __switch_to_aux(__prev, __next);\
+ if (has_fpu)\
+ __switch_to_aux(__prev, __next);\
((last) = __switch_to(__prev, __next)); \
} while (0)
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 17011a870044..46942e635266 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -22,6 +22,9 @@
#include
unsigned long elf_hwcap __read_mostly;
+#ifdef CONFIG_FPU
+bool has_fpu __read_mostly;
+#endif
void riscv_fill_hwcap(void)
{
@@ -58,4 +61,9 @@ void riscv_fill_hwcap(void)
elf_hwcap |= isa2hwcap[(unsigned char)(isa[i])];
pr_info("elf_hwcap is 0x%lx", elf_hwcap);
+
+#ifdef CONFIG_FPU
+ if (elf_hwcap & (COMPAT_HWCAP_ISA_F | COMPAT_HWCAP_ISA_D))
+ has_fpu = true;
+#endif
}
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index 3820d89e2db9..97155aee9e71 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -83,7 +83,9 @@ void show_regs(struct pt_regs *regs)
void start_thread(struct pt_regs *regs, unsigned long pc,
unsigned long sp)
{
- regs->sstatus = DEFAULT_SSTATUS;
+ regs->sstatus = SR_SPIE;
+ if (has_fpu)
+ regs->sstatus |= SR_FS_INITIAL;
regs->sepc = pc;
regs->sp = sp;
set_fs(USER_DS);
diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
index 2450b824d799..f9b5e7e352ef 100644
--- a/arch/riscv/kernel/signal.c
+++ b/arch/riscv/kernel/signal.c
@@ -98,7 +98,8 @@ static long restore_sigcontext(struct pt_regs *regs,
/* sc_regs is structured the same as the start of pt_regs */
err = __copy_from_user(regs, >sc_regs, sizeof(sc->sc_regs));
/* Restore the floating-point state. */
- err |= restore_fp_state(regs, >sc_fpregs);
+ if (has_fpu)
+ err |= restore_fp_state(regs, >sc_fpregs);
return err;
}
@@ -150,7 +151,8 @@ static long setup_sigcontext(struct rt_sigframe __user
*frame,
/* sc_regs is structured the same as the start of pt_regs */
err = __copy_to_user(>sc_regs, regs, sizeof(sc->sc_regs));
/* Save the floating-point state. */
- err |= save_fp_state(regs, >sc_fpregs);
+ if (has_fpu)
+ err |= save_fp_state(regs, >sc_fpregs);
return err;
}
--
2.18.0
