* RFC: stop implementing set_fs for m68knommu @ 2021-07-05 5:57 Christoph Hellwig 2021-07-05 5:57 ` [PATCH] m68knommu: remove set_fs() Christoph Hellwig 0 siblings, 1 reply; 16+ messages in thread From: Christoph Hellwig @ 2021-07-05 5:57 UTC (permalink / raw) To: geert, gerg; +Cc: linux-m68k, uclinux-dev, torvalds 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] m68knommu: remove set_fs() 2021-07-05 5:57 RFC: stop implementing set_fs for m68knommu Christoph Hellwig @ 2021-07-05 5:57 ` Christoph Hellwig 2021-07-05 8:44 ` Geert Uytterhoeven 0 siblings, 1 reply; 16+ messages in thread From: Christoph Hellwig @ 2021-07-05 5:57 UTC (permalink / raw) To: geert, gerg; +Cc: linux-m68k, uclinux-dev, torvalds 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] m68knommu: remove set_fs() 2021-07-05 5:57 ` [PATCH] m68knommu: remove set_fs() Christoph Hellwig @ 2021-07-05 8:44 ` Geert Uytterhoeven 2021-07-05 8:56 ` Christoph Hellwig 2021-07-05 20:39 ` Linus Torvalds 0 siblings, 2 replies; 16+ messages in thread From: Geert Uytterhoeven @ 2021-07-05 8:44 UTC (permalink / raw) To: Christoph Hellwig Cc: Greg Ungerer, linux-m68k, uClinux development list, Linus Torvalds 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] m68knommu: remove set_fs() 2021-07-05 8:44 ` Geert Uytterhoeven @ 2021-07-05 8:56 ` Christoph Hellwig 2021-07-05 11:33 ` Geert Uytterhoeven 2021-07-05 20:39 ` Linus Torvalds 1 sibling, 1 reply; 16+ messages in thread From: Christoph Hellwig @ 2021-07-05 8:56 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Christoph Hellwig, Greg Ungerer, linux-m68k, uClinux development list, Linus Torvalds 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] m68knommu: remove set_fs() 2021-07-05 8:56 ` Christoph Hellwig @ 2021-07-05 11:33 ` Geert Uytterhoeven 2021-07-05 11:51 ` Christoph Hellwig 0 siblings, 1 reply; 16+ messages in thread From: Geert Uytterhoeven @ 2021-07-05 11:33 UTC (permalink / raw) To: Christoph Hellwig Cc: Greg Ungerer, linux-m68k, uClinux development list, Linus Torvalds 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] m68knommu: remove set_fs() 2021-07-05 11:33 ` Geert Uytterhoeven @ 2021-07-05 11:51 ` Christoph Hellwig 0 siblings, 0 replies; 16+ messages in thread From: Christoph Hellwig @ 2021-07-05 11:51 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Christoph Hellwig, Greg Ungerer, linux-m68k, uClinux development list, Linus Torvalds 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] m68knommu: remove set_fs() 2021-07-05 8:44 ` Geert Uytterhoeven 2021-07-05 8:56 ` Christoph Hellwig @ 2021-07-05 20:39 ` Linus Torvalds 2021-07-06 4:13 ` Christoph Hellwig 2021-07-08 3:40 ` Michael Schmitz 1 sibling, 2 replies; 16+ messages in thread From: Linus Torvalds @ 2021-07-05 20:39 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Christoph Hellwig, Greg Ungerer, linux-m68k, uClinux development list [-- 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] m68knommu: remove set_fs() 2021-07-05 20:39 ` Linus Torvalds @ 2021-07-06 4:13 ` Christoph Hellwig 2021-07-06 18:36 ` Linus Torvalds 2021-07-08 3:40 ` Michael Schmitz 1 sibling, 1 reply; 16+ messages in thread From: Christoph Hellwig @ 2021-07-06 4:13 UTC (permalink / raw) To: Linus Torvalds Cc: Geert Uytterhoeven, Christoph Hellwig, Greg Ungerer, linux-m68k, uClinux development list 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] m68knommu: remove set_fs() 2021-07-06 4:13 ` Christoph Hellwig @ 2021-07-06 18:36 ` Linus Torvalds 2021-07-07 14:25 ` Christoph Hellwig 0 siblings, 1 reply; 16+ messages in thread From: Linus Torvalds @ 2021-07-06 18:36 UTC (permalink / raw) To: Christoph Hellwig Cc: Geert Uytterhoeven, Greg Ungerer, linux-m68k, uClinux development list 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] m68knommu: remove set_fs() 2021-07-06 18:36 ` Linus Torvalds @ 2021-07-07 14:25 ` Christoph Hellwig 2021-07-07 17:41 ` Linus Torvalds 2021-07-08 1:39 ` Michael Schmitz 0 siblings, 2 replies; 16+ messages in thread From: Christoph Hellwig @ 2021-07-07 14:25 UTC (permalink / raw) To: Linus Torvalds Cc: Christoph Hellwig, Geert Uytterhoeven, Greg Ungerer, linux-m68k, uClinux development list 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] m68knommu: remove set_fs() 2021-07-07 14:25 ` Christoph Hellwig @ 2021-07-07 17:41 ` Linus Torvalds 2021-07-08 1:39 ` Michael Schmitz 1 sibling, 0 replies; 16+ messages in thread From: Linus Torvalds @ 2021-07-07 17:41 UTC (permalink / raw) To: Christoph Hellwig Cc: Geert Uytterhoeven, Greg Ungerer, linux-m68k, uClinux development list, Michael Schmitz 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] m68knommu: remove set_fs() 2021-07-07 14:25 ` Christoph Hellwig 2021-07-07 17:41 ` Linus Torvalds @ 2021-07-08 1:39 ` Michael Schmitz 1 sibling, 0 replies; 16+ messages in thread From: Michael Schmitz @ 2021-07-08 1:39 UTC (permalink / raw) To: Christoph Hellwig, Linus Torvalds Cc: Geert Uytterhoeven, Greg Ungerer, linux-m68k, uClinux development list 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] m68knommu: remove set_fs() 2021-07-05 20:39 ` Linus Torvalds 2021-07-06 4:13 ` Christoph Hellwig @ 2021-07-08 3:40 ` Michael Schmitz 2021-07-08 4:14 ` Linus Torvalds 1 sibling, 1 reply; 16+ messages in thread From: Michael Schmitz @ 2021-07-08 3:40 UTC (permalink / raw) To: Linus Torvalds, Geert Uytterhoeven Cc: Christoph Hellwig, Greg Ungerer, linux-m68k, uClinux development list 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] m68knommu: remove set_fs() 2021-07-08 3:40 ` Michael Schmitz @ 2021-07-08 4:14 ` Linus Torvalds 2021-07-08 4:17 ` Christoph Hellwig 2021-07-08 6:33 ` Michael Schmitz 0 siblings, 2 replies; 16+ messages in thread From: Linus Torvalds @ 2021-07-08 4:14 UTC (permalink / raw) To: Michael Schmitz Cc: Geert Uytterhoeven, Christoph Hellwig, Greg Ungerer, linux-m68k, uClinux development list 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] m68knommu: remove set_fs() 2021-07-08 4:14 ` Linus Torvalds @ 2021-07-08 4:17 ` Christoph Hellwig 2021-07-08 6:33 ` Michael Schmitz 1 sibling, 0 replies; 16+ messages in thread From: Christoph Hellwig @ 2021-07-08 4:17 UTC (permalink / raw) To: Linus Torvalds Cc: Michael Schmitz, Geert Uytterhoeven, Christoph Hellwig, Greg Ungerer, linux-m68k, uClinux development list 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] m68knommu: remove set_fs() 2021-07-08 4:14 ` Linus Torvalds 2021-07-08 4:17 ` Christoph Hellwig @ 2021-07-08 6:33 ` Michael Schmitz 1 sibling, 0 replies; 16+ messages in thread From: Michael Schmitz @ 2021-07-08 6:33 UTC (permalink / raw) To: Linus Torvalds Cc: Geert Uytterhoeven, Christoph Hellwig, Greg Ungerer, linux-m68k, uClinux development list 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-07-08 6:33 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-05 5:57 RFC: stop implementing set_fs for m68knommu Christoph Hellwig 2021-07-05 5:57 ` [PATCH] m68knommu: remove set_fs() Christoph Hellwig 2021-07-05 8:44 ` Geert Uytterhoeven 2021-07-05 8:56 ` Christoph Hellwig 2021-07-05 11:33 ` Geert Uytterhoeven 2021-07-05 11:51 ` Christoph Hellwig 2021-07-05 20:39 ` Linus Torvalds 2021-07-06 4:13 ` Christoph Hellwig 2021-07-06 18:36 ` Linus Torvalds 2021-07-07 14:25 ` Christoph Hellwig 2021-07-07 17:41 ` Linus Torvalds 2021-07-08 1:39 ` Michael Schmitz 2021-07-08 3:40 ` Michael Schmitz 2021-07-08 4:14 ` Linus Torvalds 2021-07-08 4:17 ` Christoph Hellwig 2021-07-08 6:33 ` Michael Schmitz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.