RFC: stop implementing set_fs for m68knommu

RFC: stop implementing set_fs for m68knommu

[Christoph Hellwig]

Hi all,

as a recently build error showed implementing set_fs for m68knommu is
actively wrong given that it already does not distinguish between address
spaces.  Fix this up by not implementing set_fs() at all.

Compile tested only.

[PATCH] m68knommu: remove set_fs()

[Christoph Hellwig]

m68knommu already does not distinguish between kernel and user address
spaces.  Stop defining set_fs() and thus uaccess_kernel() to avoid
confusion.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/m68k/Kconfig                   |  2 +-
 arch/m68k/include/asm/processor.h   |  9 +++++++++
 arch/m68k/include/asm/segment.h     | 13 +++++++------
 arch/m68k/include/asm/thread_info.h | 10 ++++++++++
 arch/m68k/kernel/asm-offsets.c      |  2 ++
 arch/m68k/kernel/entry.S            |  8 ++++----
 arch/m68k/kernel/process.c          |  6 ++++--
 arch/m68k/mm/init.c                 |  3 ++-
 8 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
index 96989ad46f66..1c60037e352a 100644
--- a/arch/m68k/Kconfig
+++ b/arch/m68k/Kconfig
@@ -32,7 +32,7 @@ config M68K
 	select NO_DMA if !MMU && !COLDFIRE
 	select OLD_SIGACTION
 	select OLD_SIGSUSPEND3
-	select SET_FS
+	select SET_FS if MMU
 	select UACCESS_MEMCPY if !MMU
 	select VIRT_TO_BUS
 	select ZONE_DMA
diff --git a/arch/m68k/include/asm/processor.h b/arch/m68k/include/asm/processor.h
index 3750819ac5a1..ac04b5c8fe8d 100644
--- a/arch/m68k/include/asm/processor.h
+++ b/arch/m68k/include/asm/processor.h
@@ -79,7 +79,9 @@ struct thread_struct {
 	unsigned long  ksp;		/* kernel stack pointer */
 	unsigned long  usp;		/* user stack pointer */
 	unsigned short sr;		/* saved status register */
+#ifdef CONFIG_MMU
 	unsigned short fs;		/* saved fs (sfc, dfc) */
+#endif
 	unsigned long  crp[2];		/* cpu root pointer */
 	unsigned long  esp0;		/* points to SR of stack frame */
 	unsigned long  faddr;		/* info about last fault */
@@ -89,11 +91,18 @@ struct thread_struct {
 	unsigned char  fpstate[FPSTATESIZE];  /* floating point state */
 };
 
+#ifdef CONFIG_MMU
 #define INIT_THREAD  {							\
 	.ksp	= sizeof(init_stack) + (unsigned long) init_stack,	\
 	.sr	= PS_S,							\
 	.fs	= __KERNEL_DS,						\
 }
+#else
+#define INIT_THREAD  {							\
+	.ksp	= sizeof(init_stack) + (unsigned long) init_stack,	\
+	.sr	= PS_S,							\
+}
+#endif /* CONFIG_MMU */
 
 /*
  * ColdFire stack format sbould be 0x4 for an aligned usp (will always be
diff --git a/arch/m68k/include/asm/segment.h b/arch/m68k/include/asm/segment.h
index 2b5e68a71ef7..b134820425a5 100644
--- a/arch/m68k/include/asm/segment.h
+++ b/arch/m68k/include/asm/segment.h
@@ -2,19 +2,20 @@
 #ifndef _M68K_SEGMENT_H
 #define _M68K_SEGMENT_H
 
-/* define constants */
 /* Address spaces (FC0-FC2) */
 #define USER_DATA     (1)
+#define USER_PROGRAM  (2)
+#define SUPER_DATA    (5)
+#define SUPER_PROGRAM (6)
+#define CPU_SPACE     (7)
+
+#ifdef CONFIG_MMU
 #ifndef __USER_DS
 #define __USER_DS     (USER_DATA)
 #endif
-#define USER_PROGRAM  (2)
-#define SUPER_DATA    (5)
 #ifndef __KERNEL_DS
 #define __KERNEL_DS   (SUPER_DATA)
 #endif
-#define SUPER_PROGRAM (6)
-#define CPU_SPACE     (7)
 
 #ifndef __ASSEMBLY__
 
@@ -55,5 +56,5 @@ static inline void set_fs(mm_segment_t val)
 #define uaccess_kernel()	(get_fs().seg == KERNEL_DS.seg)
 
 #endif /* __ASSEMBLY__ */
-
+#endif /* CONFIG_MMU */
 #endif /* _M68K_SEGMENT_H */
diff --git a/arch/m68k/include/asm/thread_info.h b/arch/m68k/include/asm/thread_info.h
index 15a757073fa5..8741388d11af 100644
--- a/arch/m68k/include/asm/thread_info.h
+++ b/arch/m68k/include/asm/thread_info.h
@@ -27,19 +27,29 @@
 struct thread_info {
 	struct task_struct	*task;		/* main task structure */
 	unsigned long		flags;
+#ifdef CONFIG_MMU
 	mm_segment_t		addr_limit;	/* thread address space */
+#endif
 	int			preempt_count;	/* 0 => preemptable, <0 => BUG */
 	__u32			cpu;		/* should always be 0 on m68k */
 	unsigned long		tp_value;	/* thread pointer */
 };
 #endif /* __ASSEMBLY__ */
 
+#ifdef CONFIG_MMU
 #define INIT_THREAD_INFO(tsk)			\
 {						\
 	.task		= &tsk,			\
 	.addr_limit	= KERNEL_DS,		\
 	.preempt_count	= INIT_PREEMPT_COUNT,	\
 }
+#else
+#define INIT_THREAD_INFO(tsk)			\
+{						\
+	.task		= &tsk,			\
+	.preempt_count	= INIT_PREEMPT_COUNT,	\
+}
+#endif /* CONFIG_MMU */
 
 #ifndef __ASSEMBLY__
 /* how to get the thread information struct from C */
diff --git a/arch/m68k/kernel/asm-offsets.c b/arch/m68k/kernel/asm-offsets.c
index ccea355052ef..ff9dc90aca93 100644
--- a/arch/m68k/kernel/asm-offsets.c
+++ b/arch/m68k/kernel/asm-offsets.c
@@ -31,7 +31,9 @@ int main(void)
 	DEFINE(THREAD_KSP, offsetof(struct thread_struct, ksp));
 	DEFINE(THREAD_USP, offsetof(struct thread_struct, usp));
 	DEFINE(THREAD_SR, offsetof(struct thread_struct, sr));
+#ifdef CONFIG_MMU
 	DEFINE(THREAD_FS, offsetof(struct thread_struct, fs));
+#endif
 	DEFINE(THREAD_CRP, offsetof(struct thread_struct, crp));
 	DEFINE(THREAD_ESP0, offsetof(struct thread_struct, esp0));
 	DEFINE(THREAD_FPREG, offsetof(struct thread_struct, fp));
diff --git a/arch/m68k/kernel/entry.S b/arch/m68k/kernel/entry.S
index 9dd76fbb7c6b..2e4054e26be6 100644
--- a/arch/m68k/kernel/entry.S
+++ b/arch/m68k/kernel/entry.S
@@ -335,11 +335,11 @@ resume:
 
 	/* save sr */
 	movew	%sr,%a0@(TASK_THREAD+THREAD_SR)
-
+#ifdef CONFIG_MMU
 	/* save fs (sfc,%dfc) (may be pointing to kernel memory) */
 	movec	%sfc,%d0
 	movew	%d0,%a0@(TASK_THREAD+THREAD_FS)
-
+#endif
 	/* save usp */
 	/* it is better to use a movel here instead of a movew 8*) */
 	movec	%usp,%d0
@@ -422,12 +422,12 @@ resume:
 	/* restore user stack pointer */
 	movel	%a1@(TASK_THREAD+THREAD_USP),%a0
 	movel	%a0,%usp
-
+#ifdef CONFIG_MMU
 	/* restore fs (sfc,%dfc) */
 	movew	%a1@(TASK_THREAD+THREAD_FS),%a0
 	movec	%a0,%sfc
 	movec	%a0,%dfc
-
+#endif
 	/* restore status register */
 	movew	%a1@(TASK_THREAD+THREAD_SR),%sr
 
diff --git a/arch/m68k/kernel/process.c b/arch/m68k/kernel/process.c
index db49f9091711..7b725f327658 100644
--- a/arch/m68k/kernel/process.c
+++ b/arch/m68k/kernel/process.c
@@ -92,7 +92,9 @@ void show_regs(struct pt_regs * regs)
 
 void flush_thread(void)
 {
+#ifdef CONFIG_MMU
 	current->thread.fs = __USER_DS;
+#endif
 #ifdef CONFIG_FPU
 	if (!FPU_IS_EMU) {
 		unsigned long zero = 0;
@@ -150,13 +152,13 @@ int copy_thread(unsigned long clone_flags, unsigned long usp, unsigned long arg,
 
 	p->thread.ksp = (unsigned long)frame;
 	p->thread.esp0 = (unsigned long)&frame->regs;
-
+#ifdef CONFIG_MMU
 	/*
 	 * Must save the current SFC/DFC value, NOT the value when
 	 * the parent was last descheduled - RGH  10-08-96
 	 */
 	p->thread.fs = get_fs().seg;
-
+#endif
 	if (unlikely(p->flags & (PF_KTHREAD | PF_IO_WORKER))) {
 		/* kernel thread */
 		memset(frame, 0, sizeof(struct fork_frame));
diff --git a/arch/m68k/mm/init.c b/arch/m68k/mm/init.c
index 5d749e188246..3beec9644ae9 100644
--- a/arch/m68k/mm/init.c
+++ b/arch/m68k/mm/init.c
@@ -72,11 +72,12 @@ void __init paging_init(void)
 	if (!empty_zero_page)
 		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
 		      __func__, PAGE_SIZE, PAGE_SIZE);
-
+#ifdef CONFIG_MMU
 	/*
 	 * Set up SFC/DFC registers (user data space).
 	 */
 	set_fs (USER_DS);
+#endif
 
 	max_zone_pfn[ZONE_DMA] = end_mem >> PAGE_SHIFT;
 	free_area_init(max_zone_pfn);
-- 
2.30.2

Re: [PATCH] m68knommu: remove set_fs()

[Geert Uytterhoeven]

Hi Christoph,

On Mon, Jul 5, 2021 at 7:58 AM Christoph Hellwig <hch@lst.de> wrote:
> m68knommu already does not distinguish between kernel and user address
> spaces.  Stop defining set_fs() and thus uaccess_kernel() to avoid
> confusion.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Thanks for your patch!

> --- a/arch/m68k/Kconfig
> +++ b/arch/m68k/Kconfig
> @@ -32,7 +32,7 @@ config M68K
>         select NO_DMA if !MMU && !COLDFIRE
>         select OLD_SIGACTION
>         select OLD_SIGSUSPEND3
> -       select SET_FS
> +       select SET_FS if MMU

Probably this should be

    select SET_FS if CPU_HAS_ADDRESS_SPACES

instead.  Classic m68k (except for good old 68000) has address spaces,
Coldfire has not.

Which brought my attention to a problem with support for embedded
68EC020 cores: MCPU32  does not select CPU_HAS_ADDRESS_SPACES, but
it also does not select GENERIC_CSUM.  The latter means it will use
arch/m68k/lib/checksum.c, which does contain moves instructions, thus
assuming separate address spaces.  Fortunately MCPU32 is no longer
used since commit a3595962d82495f5 ("m68knommu: remove obsolete 68360
support"), so it can be removed, too.

I guess we want to simplify

    config GENERIC_CSUM
        default y if !CPU_HAS_ADDRESS_SPACES

which would allow us to get rid of the ifndef CONFIG_GENERIC_CSUM in
arch/m68k/lib/Makefile, in favor of lib-$(CPU_HAS_ADDRESS_SPACES).

> --- a/arch/m68k/include/asm/segment.h
> +++ b/arch/m68k/include/asm/segment.h
> @@ -2,19 +2,20 @@
>  #ifndef _M68K_SEGMENT_H
>  #define _M68K_SEGMENT_H
>
> -/* define constants */
>  /* Address spaces (FC0-FC2) */
>  #define USER_DATA     (1)
> +#define USER_PROGRAM  (2)
> +#define SUPER_DATA    (5)
> +#define SUPER_PROGRAM (6)
> +#define CPU_SPACE     (7)
> +
> +#ifdef CONFIG_MMU
>  #ifndef __USER_DS
>  #define __USER_DS     (USER_DATA)
>  #endif
> -#define USER_PROGRAM  (2)
> -#define SUPER_DATA    (5)
>  #ifndef __KERNEL_DS
>  #define __KERNEL_DS   (SUPER_DATA)
>  #endif
> -#define SUPER_PROGRAM (6)
> -#define CPU_SPACE     (7)
>
>  #ifndef __ASSEMBLY__

Just below this is the existing handling for !CPU_HAS_ADDRESS_SPACES.
Probably the [sg]et_fs() definitions there should be made dummies,
reducing the need to add a rather large number of #ifdefs.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] m68knommu: remove set_fs()

[Christoph Hellwig]

On Mon, Jul 05, 2021 at 10:44:51AM +0200, Geert Uytterhoeven wrote:
> > --- a/arch/m68k/Kconfig
> > +++ b/arch/m68k/Kconfig
> > @@ -32,7 +32,7 @@ config M68K
> >         select NO_DMA if !MMU && !COLDFIRE
> >         select OLD_SIGACTION
> >         select OLD_SIGSUSPEND3
> > -       select SET_FS
> > +       select SET_FS if MMU
> 
> Probably this should be
> 
>     select SET_FS if CPU_HAS_ADDRESS_SPACES
> 
> instead.  Classic m68k (except for good old 68000) has address spaces,
> Coldfire has not.

No, at least not yet.  Coldfire and co still rely on set_fs for various
things in the m68k arch code, and also don't provide
__{get,put}_kernel_nofault yet.  So wile m68k with CPU_HAS_ADDRESS_SPACES
will probably one of the hardest ports to get rid of set_fs() for,
m68k/mmu without CPU_HAS_ADDRESS_SPACES would be a logical next step.
I could try to whiteboard code some of it, but I'd need help from dedicated

Re: [PATCH] m68knommu: remove set_fs()

[Geert Uytterhoeven]

Hi Christoph,

On Mon, Jul 5, 2021 at 10:56 AM Christoph Hellwig <hch@lst.de> wrote:
> On Mon, Jul 05, 2021 at 10:44:51AM +0200, Geert Uytterhoeven wrote:
> > > --- a/arch/m68k/Kconfig
> > > +++ b/arch/m68k/Kconfig
> > > @@ -32,7 +32,7 @@ config M68K
> > >         select NO_DMA if !MMU && !COLDFIRE
> > >         select OLD_SIGACTION
> > >         select OLD_SIGSUSPEND3
> > > -       select SET_FS
> > > +       select SET_FS if MMU
> >
> > Probably this should be
> >
> >     select SET_FS if CPU_HAS_ADDRESS_SPACES
> >
> > instead.  Classic m68k (except for good old 68000) has address spaces,
> > Coldfire has not.
>
> No, at least not yet.  Coldfire and co still rely on set_fs for various
> things in the m68k arch code, and also don't provide
> __{get,put}_kernel_nofault yet.  So wile m68k with CPU_HAS_ADDRESS_SPACES
> will probably one of the hardest ports to get rid of set_fs() for,
> m68k/mmu without CPU_HAS_ADDRESS_SPACES would be a logical next step.
> I could try to whiteboard code some of it, but I'd need help from dedicated

M68k/mmu with CPU_HAS_ADDRESS_SPACES needs to use the address spaces
to access user space from kernel space using the "moves" instruction.
Coldfire doesn't.

See arch/m68k/include/asm/uaccess.h.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] m68knommu: remove set_fs()

[Christoph Hellwig]

On Mon, Jul 05, 2021 at 01:33:39PM +0200, Geert Uytterhoeven wrote:
> > No, at least not yet.  Coldfire and co still rely on set_fs for various
> > things in the m68k arch code, and also don't provide
> > __{get,put}_kernel_nofault yet.  So wile m68k with CPU_HAS_ADDRESS_SPACES
> > will probably one of the hardest ports to get rid of set_fs() for,
> > m68k/mmu without CPU_HAS_ADDRESS_SPACES would be a logical next step.
> > I could try to whiteboard code some of it, but I'd need help from dedicated
> 
> M68k/mmu with CPU_HAS_ADDRESS_SPACES needs to use the address spaces
> to access user space from kernel space using the "moves" instruction.
> Coldfire doesn't.
> 
> See arch/m68k/include/asm/uaccess.h.

I know.  That doesn't change that:

 a) even getting rid for MMMU && !CPU_HAS_ADDRESS_SPACES requires further
    work
 b) getting rid of set_fs is possible even for CPU_HAS_ADDRESS_SPACES,
    although it requires a lot of work.

Re: [PATCH] m68knommu: remove set_fs()

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 2850 bytes --]

On Mon, Jul 5, 2021 at 1:46 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Probably this should be
>
>     select SET_FS if CPU_HAS_ADDRESS_SPACES

Actually, I don't think m68k has a single real "set_fs()" at all, and
it should just be converted as-is to not use CONFIG_SET_FS.

Yes, there is a "set_fs()" function, but none of the remaining uses
actually are the traditional kernel style of "use kernel addresses as
user addresses". So as far as the *kernel* is concerned, m68k already
looks like a no-SET_FS architecture, and "set-fs()" is purely a
syntactic thing.

So I think the right thing to do looks something like this:

 - make the rule be that SFC/DFC is always normally USER_DATA

 - the special m68k sequences that need to play with special segments
will always do

        preempt_disable();
        set_segment(..whatever segment they need..);
        .. do the special operation ..
        set_segment(USER_DATA);
        preempt_enable();

 - set_fs() goes away entirely, because the user access functions
always work on USER_DATA and SFC/DFC is always right for them.

Anyway, I'm attaching a COMPLETELY UNTESTED AND ALMOST CERTAINLY VERY
VERY BROKEN patch that is likely not at all correct, but shows what I
think the solution should be.

The important thing is really just the removal of SET_FS entirely, the
rest is me winging it.

NOTE! The above very much assumes that all the special non-USER_DATA
accesses can always be done with preemption disabled. Why? Because I
also made the context switching always just save USER_DATA as the
segment. I didn't *remove* the segment switching - because I'm not
sure if that "just disable preemption across things that do segment
games" is actually valid.

So *if* that preempt_disable/enable is ok, then the segment switching
can just be removed entirely at context switch time.

And if it is *not* ok, then the preempt_disable/enable should go away,
and the context switch should save/restore the actual SFC/DFC value.

Again - let me be very very clear: the only m68k I've ever used was a
68008. It didn't have segments. This patch is COMPLETE GARBAGE. Do not
trust it. Do not use it for anything but a "Linus suggests maybe
something along these lines could work".

This has not even been build-tested, and I'm pretty sure it won't
build as-is. It really is a "Linus did some pattern matching and a few
grep's".

Oh, and if the magical SFC/DFC games can happen in interrupts, then
the "preempt_disable/enable" actually needs to be a full interrupt
disable/enable, or the code needs to re-introduce that "save old
value, restore it". So that's another assumption this example patch
makes - that the SFC/DFC games only happen in process context. Which
may be complete garbage too.

Did I mention that I think this patch is garbage? Because it really is.

                Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 9030 bytes --]

 arch/m68k/Kconfig                |  1 -
 arch/m68k/include/asm/segment.h  | 21 +++------------------
 arch/m68k/include/asm/tlbflush.h |  7 ++++---
 arch/m68k/kernel/process.c       |  2 +-
 arch/m68k/kernel/traps.c         | 16 ++++++++--------
 arch/m68k/mm/cache.c             | 23 ++++++++++++++---------
 arch/m68k/mm/init.c              |  2 +-
 arch/m68k/mm/motorola.c          |  2 +-
 arch/m68k/sun3/config.c          |  2 +-
 arch/m68k/sun3/mmu_emu.c         |  7 ++++---
 10 files changed, 37 insertions(+), 46 deletions(-)

diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
index 96989ad46f66..3a83013f0a96 100644
--- a/arch/m68k/Kconfig
+++ b/arch/m68k/Kconfig
@@ -32,7 +32,6 @@ config M68K
 	select NO_DMA if !MMU && !COLDFIRE
 	select OLD_SIGACTION
 	select OLD_SIGSUSPEND3
-	select SET_FS
 	select UACCESS_MEMCPY if !MMU
 	select VIRT_TO_BUS
 	select ZONE_DMA
diff --git a/arch/m68k/include/asm/segment.h b/arch/m68k/include/asm/segment.h
index 2b5e68a71ef7..a9fabf7d2a2c 100644
--- a/arch/m68k/include/asm/segment.h
+++ b/arch/m68k/include/asm/segment.h
@@ -26,19 +26,9 @@ typedef struct {
 
 #ifdef CONFIG_CPU_HAS_ADDRESS_SPACES
 /*
- * Get/set the SFC/DFC registers for MOVES instructions
+ * Set the SFC/DFC registers for MOVES instructions
  */
-#define USER_DS		MAKE_MM_SEG(__USER_DS)
-#define KERNEL_DS	MAKE_MM_SEG(__KERNEL_DS)
-
-static inline mm_segment_t get_fs(void)
-{
-	mm_segment_t _v;
-	__asm__ ("movec %/dfc,%0":"=r" (_v.seg):);
-	return _v;
-}
-
-static inline void set_fs(mm_segment_t val)
+static inline void set_segment(mm_segment_t val)
 {
 	__asm__ __volatile__ ("movec %0,%/sfc\n\t"
 			      "movec %0,%/dfc\n\t"
@@ -46,14 +36,9 @@ static inline void set_fs(mm_segment_t val)
 }
 
 #else
-#define USER_DS		MAKE_MM_SEG(TASK_SIZE)
-#define KERNEL_DS	MAKE_MM_SEG(0xFFFFFFFF)
-#define get_fs()	(current_thread_info()->addr_limit)
-#define set_fs(x)	(current_thread_info()->addr_limit = (x))
+#define set_segment(x)	((void)(x))
 #endif
 
-#define uaccess_kernel()	(get_fs().seg == KERNEL_DS.seg)
-
 #endif /* __ASSEMBLY__ */
 
 #endif /* _M68K_SEGMENT_H */
diff --git a/arch/m68k/include/asm/tlbflush.h b/arch/m68k/include/asm/tlbflush.h
index a6318ccd308f..3c9e35c7e93f 100644
--- a/arch/m68k/include/asm/tlbflush.h
+++ b/arch/m68k/include/asm/tlbflush.h
@@ -13,13 +13,14 @@ static inline void flush_tlb_kernel_page(void *addr)
 	if (CPU_IS_COLDFIRE) {
 		mmu_write(MMUOR, MMUOR_CNL);
 	} else if (CPU_IS_040_OR_060) {
-		mm_segment_t old_fs = get_fs();
-		set_fs(KERNEL_DS);
+		preempt_disable();
+		set_segment(SUPER_DATA);
 		__asm__ __volatile__(".chip 68040\n\t"
 				     "pflush (%0)\n\t"
 				     ".chip 68k"
 				     : : "a" (addr));
-		set_fs(old_fs);
+		set_segment(USER_DATA);
+		preempt_enable();
 	} else if (CPU_IS_020_OR_030)
 		__asm__ __volatile__("pflush #4,#4,(%0)" : : "a" (addr));
 }
diff --git a/arch/m68k/kernel/process.c b/arch/m68k/kernel/process.c
index db49f9091711..a176d00ffc97 100644
--- a/arch/m68k/kernel/process.c
+++ b/arch/m68k/kernel/process.c
@@ -155,7 +155,7 @@ int copy_thread(unsigned long clone_flags, unsigned long usp, unsigned long arg,
 	 * Must save the current SFC/DFC value, NOT the value when
 	 * the parent was last descheduled - RGH  10-08-96
 	 */
-	p->thread.fs = get_fs().seg;
+	p->thread.fs = USER_DATA.seg;
 
 	if (unlikely(p->flags & (PF_KTHREAD | PF_IO_WORKER))) {
 		/* kernel thread */
diff --git a/arch/m68k/kernel/traps.c b/arch/m68k/kernel/traps.c
index 9e1261462bcc..85863181466c 100644
--- a/arch/m68k/kernel/traps.c
+++ b/arch/m68k/kernel/traps.c
@@ -181,9 +181,9 @@ static inline void access_error060 (struct frame *fp)
 static inline unsigned long probe040(int iswrite, unsigned long addr, int wbs)
 {
 	unsigned long mmusr;
-	mm_segment_t old_fs = get_fs();
 
-	set_fs(MAKE_MM_SEG(wbs));
+	preempt_disable();
+	set_segment(MAKE_MM_SEG(wbs));
 
 	if (iswrite)
 		asm volatile (".chip 68040; ptestw (%0); .chip 68k" : : "a" (addr));
@@ -192,7 +192,8 @@ static inline unsigned long probe040(int iswrite, unsigned long addr, int wbs)
 
 	asm volatile (".chip 68040; movec %%mmusr,%0; .chip 68k" : "=r" (mmusr));
 
-	set_fs(old_fs);
+	set_segment(USER_DATA);
+	preempt_enable();
 
 	return mmusr;
 }
@@ -201,10 +202,9 @@ static inline int do_040writeback1(unsigned short wbs, unsigned long wba,
 				   unsigned long wbd)
 {
 	int res = 0;
-	mm_segment_t old_fs = get_fs();
 
-	/* set_fs can not be moved, otherwise put_user() may oops */
-	set_fs(MAKE_MM_SEG(wbs));
+	preempt_disable();
+	set_segment(MAKE_MM_SEG(wbs));
 
 	switch (wbs & WBSIZ_040) {
 	case BA_SIZE_BYTE:
@@ -218,8 +218,8 @@ static inline int do_040writeback1(unsigned short wbs, unsigned long wba,
 		break;
 	}
 
-	/* set_fs can not be moved, otherwise put_user() may oops */
-	set_fs(old_fs);
+	set_segment(USER_DATA);
+	preempt_enable();
 
 
 	pr_debug("do_040writeback1, res=%d\n", res);
diff --git a/arch/m68k/mm/cache.c b/arch/m68k/mm/cache.c
index b486c0889eec..80e311aa924d 100644
--- a/arch/m68k/mm/cache.c
+++ b/arch/m68k/mm/cache.c
@@ -12,7 +12,7 @@
 #include <asm/traps.h>
 
 
-static unsigned long virt_to_phys_slow(unsigned long vaddr)
+static unsigned long virt_to_phys_slow(unsigned long vaddr, mm_segment_t seg)
 {
 	if (CPU_IS_060) {
 		unsigned long paddr;
@@ -55,7 +55,7 @@ static unsigned long virt_to_phys_slow(unsigned long vaddr)
 		asm volatile ("ptestr %3,%2@,#7,%0\n\t"
 			      "pmove %%psr,%1"
 			      : "=a&" (descaddr), "=m" (mmusr)
-			      : "a" (vaddr), "d" (get_fs().seg));
+			      : "a" (vaddr), "d" (seg.seg));
 		if (mmusr & (MMU_I|MMU_B|MMU_L))
 			return 0;
 		descaddr = phys_to_virt((unsigned long)descaddr);
@@ -73,7 +73,7 @@ static unsigned long virt_to_phys_slow(unsigned long vaddr)
 
 /* Push n pages at kernel virtual address and clear the icache */
 /* RZ: use cpush %bc instead of cpush %dc, cinv %ic */
-void flush_icache_user_range(unsigned long address, unsigned long endaddr)
+static void do_flush_icache_user_range(unsigned long address, unsigned long endaddr, mm_segment_t seg)
 {
 	if (CPU_IS_COLDFIRE) {
 		unsigned long start, end;
@@ -92,7 +92,7 @@ void flush_icache_user_range(unsigned long address, unsigned long endaddr)
 				      ".chip 68040\n\t"
 				      "cpushp %%bc,(%0)\n\t"
 				      ".chip 68k"
-				      : : "a" (virt_to_phys_slow(address)));
+				      : : "a" (virt_to_phys_slow(address, seg)));
 			address += PAGE_SIZE;
 		} while (address < endaddr);
 	} else {
@@ -105,13 +105,18 @@ void flush_icache_user_range(unsigned long address, unsigned long endaddr)
 	}
 }
 
-void flush_icache_range(unsigned long address, unsigned long endaddr)
+void flush_icache_user_range(unsigned long address, unsigned long endaddr)
 {
-	mm_segment_t old_fs = get_fs();
+	do_flush_icache_range(address, endaddr, USER_DATA);
+}
 
-	set_fs(KERNEL_DS);
-	flush_icache_user_range(address, endaddr);
-	set_fs(old_fs);
+void flush_icache_range(unsigned long address, unsigned long endaddr)
+{
+	preempt_disable();
+	set_segment(SUPER_DATA);
+	do_flush_icache_range(address, endaddr, SUPER_DATA);
+	set_segment(USER_DATA);
+	preempt_enable();
 }
 EXPORT_SYMBOL(flush_icache_range);
 
diff --git a/arch/m68k/mm/init.c b/arch/m68k/mm/init.c
index 5d749e188246..4a7a56139013 100644
--- a/arch/m68k/mm/init.c
+++ b/arch/m68k/mm/init.c
@@ -76,7 +76,7 @@ void __init paging_init(void)
 	/*
 	 * Set up SFC/DFC registers (user data space).
 	 */
-	set_fs (USER_DS);
+	set_segment(USER_DATA);
 
 	max_zone_pfn[ZONE_DMA] = end_mem >> PAGE_SHIFT;
 	free_area_init(max_zone_pfn);
diff --git a/arch/m68k/mm/motorola.c b/arch/m68k/mm/motorola.c
index 3a653f0a4188..7af7d5a1ac77 100644
--- a/arch/m68k/mm/motorola.c
+++ b/arch/m68k/mm/motorola.c
@@ -467,7 +467,7 @@ void __init paging_init(void)
 	/*
 	 * Set up SFC/DFC registers
 	 */
-	set_fs(KERNEL_DS);
+	set_segment(USER_DATA);
 
 #ifdef DEBUG
 	printk ("before free_area_init\n");
diff --git a/arch/m68k/sun3/config.c b/arch/m68k/sun3/config.c
index f7dd47232b6c..849457dbb424 100644
--- a/arch/m68k/sun3/config.c
+++ b/arch/m68k/sun3/config.c
@@ -89,7 +89,7 @@ void __init sun3_init(void)
 	sun3_reserved_pmeg[249] = 1;
 	sun3_reserved_pmeg[252] = 1;
 	sun3_reserved_pmeg[253] = 1;
-	set_fs(KERNEL_DS);
+	set_segment(USER_DATA);
 }
 
 /* Without this, Bad Things happen when something calls arch_reset. */
diff --git a/arch/m68k/sun3/mmu_emu.c b/arch/m68k/sun3/mmu_emu.c
index 7aa879b7c7ff..2bcf7e9822e6 100644
--- a/arch/m68k/sun3/mmu_emu.c
+++ b/arch/m68k/sun3/mmu_emu.c
@@ -191,14 +191,15 @@ void __init mmu_emu_init(unsigned long bootmem_end)
 	for(seg = 0; seg < PAGE_OFFSET; seg += SUN3_PMEG_SIZE)
 		sun3_put_segmap(seg, SUN3_INVALID_PMEG);
 
-	set_fs(MAKE_MM_SEG(3));
+	preempt_disable();
+	set_segment(MAKE_MM_SEG(3));
 	for(seg = 0; seg < 0x10000000; seg += SUN3_PMEG_SIZE) {
 		i = sun3_get_segmap(seg);
 		for(j = 1; j < CONTEXTS_NUM; j++)
 			(*(romvec->pv_setctxt))(j, (void *)seg, i);
 	}
-	set_fs(KERNEL_DS);
-
+	set_segment(USER_DATA);
+	preempt_enable();
 }
 
 /* erase the mappings for a dead context.  Uses the pg_dir for hints

Re: [PATCH] m68knommu: remove set_fs()

[Christoph Hellwig]

On Mon, Jul 05, 2021 at 01:39:03PM -0700, Linus Torvalds wrote:
> On Mon, Jul 5, 2021 at 1:46 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> > Probably this should be
> >
> >     select SET_FS if CPU_HAS_ADDRESS_SPACES
> 
> Actually, I don't think m68k has a single real "set_fs()" at all, and
> it should just be converted as-is to not use CONFIG_SET_FS.
> 
> Yes, there is a "set_fs()" function, but none of the remaining uses
> actually are the traditional kernel style of "use kernel addresses as
> user addresses". So as far as the *kernel* is concerned, m68k already
> looks like a no-SET_FS architecture, and "set-fs()" is purely a
> syntactic thing.

It still needs "real" kernel-style set_fs for the mm/maccess.c routines,
but adding __{get,put}_kernel_nofault should not be too hard.

> So I think the right thing to do looks something like this:
> 
>  - make the rule be that SFC/DFC is always normally USER_DATA
> 
>  - the special m68k sequences that need to play with special segments
> will always do
> 
>         preempt_disable();
>         set_segment(..whatever segment they need..);
>         .. do the special operation ..
>         set_segment(USER_DATA);
>         preempt_enable();
> 
>  - set_fs() goes away entirely, because the user access functions
> always work on USER_DATA and SFC/DFC is always right for them.

Yes, that's what I mean with needing a more work.

Re: [PATCH] m68knommu: remove set_fs()

[Linus Torvalds]

On Mon, Jul 5, 2021 at 9:13 PM Christoph Hellwig <hch@lst.de> wrote:
>
> It still needs "real" kernel-style set_fs for the mm/maccess.c routines,
> but adding __{get,put}_kernel_nofault should not be too hard.

Yeah, it's not that it wants set-fs(), it's that I missed that m68k
doesn't have HAVE_GET_KERNEL_NOFAULT.

Implementing __get/put_kernel_nofault() should be fairly
straightforward: they are basically the same thing as the
__get/put_user() functions, except they should just use "move" instead
of "moves".

The m68k uaccess.h file already kind of has support for that, but it's
hardcoded to the CONFIG_CPU_HAS_ADDRESS_SPACES config, rather than
being available as two different versions.

           Linus

Re: [PATCH] m68knommu: remove set_fs()

[Christoph Hellwig]

On Tue, Jul 06, 2021 at 11:36:26AM -0700, Linus Torvalds wrote:
> On Mon, Jul 5, 2021 at 9:13 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > It still needs "real" kernel-style set_fs for the mm/maccess.c routines,
> > but adding __{get,put}_kernel_nofault should not be too hard.
> 
> Yeah, it's not that it wants set-fs(), it's that I missed that m68k
> doesn't have HAVE_GET_KERNEL_NOFAULT.
> 
> Implementing __get/put_kernel_nofault() should be fairly
> straightforward: they are basically the same thing as the
> __get/put_user() functions, except they should just use "move" instead
> of "moves".
> 
> The m68k uaccess.h file already kind of has support for that, but it's
> hardcoded to the CONFIG_CPU_HAS_ADDRESS_SPACES config, rather than
> being available as two different versions.

So I've come up with a whole (compile tested only) series.  We don't
really need the preempt_disable either given the m68k saves and restores
the SFC/DFC registers on context switch:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/m68k-set_fs

Re: [PATCH] m68knommu: remove set_fs()

[Linus Torvalds]

On Wed, Jul 7, 2021 at 7:25 AM Christoph Hellwig <hch@lst.de> wrote:
>
> So I've come up with a whole (compile tested only) series.  We don't
> really need the preempt_disable either given the m68k saves and restores
> the SFC/DFC registers on context switch:

Your commit f349b7ab5f34 ("m68k: hardcode the error value in
__{get,put}_user_asm") is wrong. The fact that
__constant_copy_to_user() passes in a positive "error" value may look
odd, but it is correct.  It's not an error, it's a return value, and
"copy_to_user()" returns not -EFAULT, but the number of characters not
copied (because it needs to be able to restart - think partial
"read/write()" system calls etc).

That said, on x86, we've gotten rid of the small constant cases
entirely, because it just made such a mess of the uaccess.h files, and
we ended up just fixing any performance-sensitive code that used small
constant sizes to do the proper get/put_user() instead.

So one option is to just remove that constant-sized copy code entirely.

Also - can somebody verify that SFC/DFC is never modified in interrupts?

Because if that happens, then the "temporarily modify SFC/DFC"
sequences need to actually save/restore it, rather than just set it
back to USER_DATA.. But yes, you're right that switch_to() will
save/restore it for regular context switches.

Finally, talking about regular context switches - I think this series
should also rename "thread.fs" to "thread.segment" or something like
that.

Apart from those small details, I think that series looks very good.

                   Linus

Re: [PATCH] m68knommu: remove set_fs()

[Michael Schmitz]

Hi Christoph,

On 8/07/21 2:25 am, Christoph Hellwig wrote:
> On Tue, Jul 06, 2021 at 11:36:26AM -0700, Linus Torvalds wrote:
>> On Mon, Jul 5, 2021 at 9:13 PM Christoph Hellwig <hch@lst.de> wrote:
>>> It still needs "real" kernel-style set_fs for the mm/maccess.c routines,
>>> but adding __{get,put}_kernel_nofault should not be too hard.
>> Yeah, it's not that it wants set-fs(), it's that I missed that m68k
>> doesn't have HAVE_GET_KERNEL_NOFAULT.
>>
>> Implementing __get/put_kernel_nofault() should be fairly
>> straightforward: they are basically the same thing as the
>> __get/put_user() functions, except they should just use "move" instead
>> of "moves".
>>
>> The m68k uaccess.h file already kind of has support for that, but it's
>> hardcoded to the CONFIG_CPU_HAS_ADDRESS_SPACES config, rather than
>> being available as two different versions.
> So I've come up with a whole (compile tested only) series.  We don't
> really need the preempt_disable either given the m68k saves and restores
> the SFC/DFC registers on context switch:
>
> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/m68k-set_fs

I tried your patches (on top of m68k 5.14).

Doesn't manage to start init, independent of what disk image I use:

Starting init: /bin/sh exists but couldn't execute it (error -22)
Kernel panic - not syncing: No working init found.  Try passing init= 
option to kernel. See Linux Documentation/admin-guide/init.rst for guidance.
CPU: 0 PID: 1 Comm: sh Not tainted 5.13.0-atari-fpuemu-exitfix+ #1198
Stack from 01031f8c:
         01031f8c 00360249 00360249 002c7032 00000001 000bc486 00000000 
000020c0
         00000000 00000000 00000000 002d3ffa 00355111 002d3f28 00002980 
00000000
         00000000 00000000 00000000 00000000 00000000 00000000 00000000 
00000000
         00000000 00000000 00000000 20000000 00000000
Call Trace: [<002c7032>] panic+0xc0/0x282
  [<000bc486>] kfree+0x0/0x60
  [<000020c0>] try_to_run_init_process+0x0/0x36
  [<002d3ffa>] kernel_init+0xd2/0xd8
  [<002d3f28>] kernel_init+0x0/0xd8
  [<00002980>] ret_from_kernel_thread+0xc/0x14

---[ end Kernel panic - not syncing: No working init found.  Try passing 
init= option to kernel. See Linux Documentation/admin-guide/init.rst for 
guidance. ]---

And I don't think it's the return value issue Linus pointed out - the 
same thing happens with my own (quite similar, though much less 
sophisticated) patches based on what I sent in response to Linus' 
suggestions earlier...

I'll send the last version of mine that did still boot shortly.

Cheers,

     Michael

Re: [PATCH] m68knommu: remove set_fs()

[Michael Schmitz]

Hi Linus,

going back to this one, I missed that bit earlier - the last three hunks
of your patch replaced KERNEL_DS by USER_DATA, everywhere else it's
replaced by SUPER_DATA. Typo, or something too subtle for me to grasp?

Cheers,

    Michael


Am 06.07.21 um 08:39 schrieb Linus Torvalds:
> On Mon, Jul 5, 2021 at 1:46 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> Probably this should be
>>
>>     select SET_FS if CPU_HAS_ADDRESS_SPACES
> Actually, I don't think m68k has a single real "set_fs()" at all, and
> it should just be converted as-is to not use CONFIG_SET_FS.
>
> Yes, there is a "set_fs()" function, but none of the remaining uses
> actually are the traditional kernel style of "use kernel addresses as
> user addresses". So as far as the *kernel* is concerned, m68k already
> looks like a no-SET_FS architecture, and "set-fs()" is purely a
> syntactic thing.
>
> So I think the right thing to do looks something like this:
>
>  - make the rule be that SFC/DFC is always normally USER_DATA
>
>  - the special m68k sequences that need to play with special segments
> will always do
>
>         preempt_disable();
>         set_segment(..whatever segment they need..);
>         .. do the special operation ..
>         set_segment(USER_DATA);
>         preempt_enable();
>
>  - set_fs() goes away entirely, because the user access functions
> always work on USER_DATA and SFC/DFC is always right for them.
>
> Anyway, I'm attaching a COMPLETELY UNTESTED AND ALMOST CERTAINLY VERY
> VERY BROKEN patch that is likely not at all correct, but shows what I
> think the solution should be.
>
> The important thing is really just the removal of SET_FS entirely, the
> rest is me winging it.
>
> NOTE! The above very much assumes that all the special non-USER_DATA
> accesses can always be done with preemption disabled. Why? Because I
> also made the context switching always just save USER_DATA as the
> segment. I didn't *remove* the segment switching - because I'm not
> sure if that "just disable preemption across things that do segment
> games" is actually valid.
>
> So *if* that preempt_disable/enable is ok, then the segment switching
> can just be removed entirely at context switch time.
>
> And if it is *not* ok, then the preempt_disable/enable should go away,
> and the context switch should save/restore the actual SFC/DFC value.
>
> Again - let me be very very clear: the only m68k I've ever used was a
> 68008. It didn't have segments. This patch is COMPLETE GARBAGE. Do not
> trust it. Do not use it for anything but a "Linus suggests maybe
> something along these lines could work".
>
> This has not even been build-tested, and I'm pretty sure it won't
> build as-is. It really is a "Linus did some pattern matching and a few
> grep's".
>
> Oh, and if the magical SFC/DFC games can happen in interrupts, then
> the "preempt_disable/enable" actually needs to be a full interrupt
> disable/enable, or the code needs to re-introduce that "save old
> value, restore it". So that's another assumption this example patch
> makes - that the SFC/DFC games only happen in process context. Which
> may be complete garbage too.
>
> Did I mention that I think this patch is garbage? Because it really is.
>
>                 Linus

Re: [PATCH] m68knommu: remove set_fs()

[Linus Torvalds]

On Wed, Jul 7, 2021 at 8:40 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
>
> going back to this one, I missed that bit earlier - the last three hunks
> of your patch replaced KERNEL_DS by USER_DATA, everywhere else it's
> replaced by SUPER_DATA. Typo, or something too subtle for me to grasp?

So I think the old KERNEL_DS was purely legacy, and isn't what I think
it's really supposed to be. It didn't _matter_, because then execve()
will set it to USER_DS by the time you run any user program, but I
didn't like it.

So I decided that in the new world order, the rules should be really
straightforward and simple:

 - SFC/DFC is always USER_DATA normally, which is how get_user/put_user want it.

 - special functions that actually use SFC/DFC for some temporary
override will set it to that temporary value, and then restore it to
USER_DATA after use.

and that's what I wrote my patch for.

BUT! And this is the important part:

My patch was completely untested garbage. I may have had opinions, I
may have had a plan, but the reality is that without testing (and
fixing things up for the things I had missed - like the code in
mm/maccess.c) that plan is just so much hot air.

In other words, take the above with a big pinch of salt.

And I want to stress once more time: if any of the SFC/DFC
modifications are done in interrupt handlers, that whole "set it back
to USER_DATA" is _wrong_. If they can happen in interrupts, then those
functions that modify SFC/DFC need to save the old value, and restore
it at the end, because otherwise you might have

 - function that uses SFC/DFC:

   set new 'value A'

        <- get interrupt here
           nested function that uses SFC/DFC
           set new value B
           .. do whatever special op
           set SDC/DFC to USER_DATA
        <- interrupt returns

   original SFC/DFC function now has SFC/DFC with USER_DATA
   it _wanted_ to have it with 'value A'

See the problem?

So if nesting can occur - due to interrupts - then all the things that
set a different SFC/DFC really need to save/restore the old one,
rather than set it back blindly to USER_DATA.

Also, finally: my patch had that "preempt_disable/preempt_enable"
around the SFC/DFC modifications. That was hot garbage. Christoph
correctly pointed out that switch_to() will save/restore SFC/DFC, so
there's no real reason to.

Except now that I think about it, I worry about getting scheduled away
*between* the instruction that sets SFC and the one that sets DFC. And
then switch_to() will save just SFC to the thread-struct. And then
restore the (new thread) SFC value to _both_ SFC and DFC.

So task switching doesn't actually save and restore SFC and DFC. It
only really saves SFC, and then it restores both SFC and DFC with the
same value.

Which should be ok, if the m68k code that modifies DFC will _always_
have modified SFC to the new value first. So even if we then schedule
away and back in between the two instructions, it only means that
we'll set DFC then to the value that it will soon be assigned anyway.

But it's all a tiny bit subtle and somewhat confusing.

                  Linus

Re: [PATCH] m68knommu: remove set_fs()

[Christoph Hellwig]

On Wed, Jul 07, 2021 at 09:14:29PM -0700, Linus Torvalds wrote:
> Except now that I think about it, I worry about getting scheduled away
> *between* the instruction that sets SFC and the one that sets DFC. And
> then switch_to() will save just SFC to the thread-struct. And then
> restore the (new thread) SFC value to _both_ SFC and DFC.

arch/m68k/Kconfig:      select ARCH_NO_PREEMPT if !COLDFIRE

So only coldfire can support kernel preemption, and coldfire never uses
SFC/DFC.

Re: [PATCH] m68knommu: remove set_fs()

[Michael Schmitz]

Hi Linus,

Am 08.07.2021 um 16:14 schrieb Linus Torvalds:
> On Wed, Jul 7, 2021 at 8:40 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
>>
>> going back to this one, I missed that bit earlier - the last three hunks
>> of your patch replaced KERNEL_DS by USER_DATA, everywhere else it's
>> replaced by SUPER_DATA. Typo, or something too subtle for me to grasp?
>
> So I think the old KERNEL_DS was purely legacy, and isn't what I think
> it's really supposed to be. It didn't _matter_, because then execve()
> will set it to USER_DS by the time you run any user program, but I
> didn't like it.
>
> So I decided that in the new world order, the rules should be really
> straightforward and simple:
>
>  - SFC/DFC is always USER_DATA normally, which is how get_user/put_user want it.
>
>  - special functions that actually use SFC/DFC for some temporary
> override will set it to that temporary value, and then restore it to
> USER_DATA after use.
>
> and that's what I wrote my patch for.

OK, got it now.

> BUT! And this is the important part:
>
> My patch was completely untested garbage. I may have had opinions, I
> may have had a plan, but the reality is that without testing (and
> fixing things up for the things I had missed - like the code in
> mm/maccess.c) that plan is just so much hot air.
>
> In other words, take the above with a big pinch of salt.
>
> And I want to stress once more time: if any of the SFC/DFC
> modifications are done in interrupt handlers, that whole "set it back
> to USER_DATA" is _wrong_. If they can happen in interrupts, then those
> functions that modify SFC/DFC need to save the old value, and restore
> it at the end, because otherwise you might have
>
>  - function that uses SFC/DFC:
>
>    set new 'value A'
>
>         <- get interrupt here
>            nested function that uses SFC/DFC
>            set new value B
>            .. do whatever special op
>            set SDC/DFC to USER_DATA
>         <- interrupt returns
>
>    original SFC/DFC function now has SFC/DFC with USER_DATA
>    it _wanted_ to have it with 'value A'
>
> See the problem?
>
> So if nesting can occur - due to interrupts - then all the things that
> set a different SFC/DFC really need to save/restore the old one,
> rather than set it back blindly to USER_DATA.

I can't recall any use of get_user() etc. in interrupt handlers, but 
that certainly warrants a closer look.

> Also, finally: my patch had that "preempt_disable/preempt_enable"
> around the SFC/DFC modifications. That was hot garbage. Christoph
> correctly pointed out that switch_to() will save/restore SFC/DFC, so
> there's no real reason to.
>
> Except now that I think about it, I worry about getting scheduled away
> *between* the instruction that sets SFC and the one that sets DFC. And
> then switch_to() will save just SFC to the thread-struct. And then
> restore the (new thread) SFC value to _both_ SFC and DFC.

I wonder whether we can end up scheduling in the return path from an 
interrupt (interrupts return via ret_from_exception on m68k, and that 
has a test for thread info flags relating to reschedule and signals)...

>
> So task switching doesn't actually save and restore SFC and DFC. It
> only really saves SFC, and then it restores both SFC and DFC with the
> same value.
>
> Which should be ok, if the m68k code that modifies DFC will _always_
> have modified SFC to the new value first. So even if we then schedule
> away and back in between the two instructions, it only means that
> we'll set DFC then to the value that it will soon be assigned anyway.
>
> But it's all a tiny bit subtle and somewhat confusing.

Bit too subtle for me still ...

Cheers,

	Michael

>
>                   Linus
>