Re: [PATCH] m68knommu: remove set_fs()

[Michael Schmitz <schmitzmic@gmail.com>]

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