| Message ID | 20220227162831.674483-14-guoren@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | riscv: compat: Add COMPAT mode support for rv64 | expand |
Hi Arnd, On Mon, Feb 28, 2022 at 12:30 AM <guoren@kernel.org> wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > If the current task is in COMPAT mode, set SR_UXL_32 in status for > returning userspace. We need CONFIG _COMPAT to prevent compiling > errors with rv32 defconfig. > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > Signed-off-by: Guo Ren <guoren@kernel.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Palmer Dabbelt <palmer@dabbelt.com> > --- > arch/riscv/kernel/process.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c > index 03ac3aa611f5..54787ca9806a 100644 > --- a/arch/riscv/kernel/process.c > +++ b/arch/riscv/kernel/process.c > @@ -97,6 +97,11 @@ void start_thread(struct pt_regs *regs, unsigned long pc, > } > regs->epc = pc; > regs->sp = sp; > + FIxup: + #ifdef CONFIG_COMPAT > + if (is_compat_task()) > + regs->status = (regs->status & ~SR_UXL) | SR_UXL_32; > + else > + regs->status = (regs->status & ~SR_UXL) | SR_UXL_64; + #endif We still need "#ifdef CONFIG_COMPAT" here, because for rv32 we can't set SR_UXL at all. SR_UXL is BIT[32, 33]. > } > > void flush_thread(void) > -- > 2.25.1 >
On 11/03/2022 02:38, Guo Ren wrote: > Hi Arnd, > > On Mon, Feb 28, 2022 at 12:30 AM <guoren@kernel.org> wrote: >> >> From: Guo Ren <guoren@linux.alibaba.com> >> >> If the current task is in COMPAT mode, set SR_UXL_32 in status for >> returning userspace. We need CONFIG _COMPAT to prevent compiling >> errors with rv32 defconfig. >> >> Signed-off-by: Guo Ren <guoren@linux.alibaba.com> >> Signed-off-by: Guo Ren <guoren@kernel.org> >> Cc: Arnd Bergmann <arnd@arndb.de> >> Cc: Palmer Dabbelt <palmer@dabbelt.com> >> --- >> arch/riscv/kernel/process.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c >> index 03ac3aa611f5..54787ca9806a 100644 >> --- a/arch/riscv/kernel/process.c >> +++ b/arch/riscv/kernel/process.c >> @@ -97,6 +97,11 @@ void start_thread(struct pt_regs *regs, unsigned long pc, >> } >> regs->epc = pc; >> regs->sp = sp; >> + > FIxup: > > + #ifdef CONFIG_COMPAT >> + if (is_compat_task()) >> + regs->status = (regs->status & ~SR_UXL) | SR_UXL_32; >> + else >> + regs->status = (regs->status & ~SR_UXL) | SR_UXL_64; > + #endif > > We still need "#ifdef CONFIG_COMPAT" here, because for rv32 we can't > set SR_UXL at all. SR_UXL is BIT[32, 33]. would an if (IS_ENABLED(CONFIG_COMPAT)) { } around the lot be better than an #ifdef here? >> } >> >> void flush_thread(void) >> -- >> 2.25.1 >> > >
On Fri, Mar 11, 2022 at 9:38 PM Ben Dooks <ben.dooks@codethink.co.uk> wrote: > > On 11/03/2022 02:38, Guo Ren wrote: > > Hi Arnd, > > > > On Mon, Feb 28, 2022 at 12:30 AM <guoren@kernel.org> wrote: > >> > >> From: Guo Ren <guoren@linux.alibaba.com> > >> > >> If the current task is in COMPAT mode, set SR_UXL_32 in status for > >> returning userspace. We need CONFIG _COMPAT to prevent compiling > >> errors with rv32 defconfig. > >> > >> Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > >> Signed-off-by: Guo Ren <guoren@kernel.org> > >> Cc: Arnd Bergmann <arnd@arndb.de> > >> Cc: Palmer Dabbelt <palmer@dabbelt.com> > >> --- > >> arch/riscv/kernel/process.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c > >> index 03ac3aa611f5..54787ca9806a 100644 > >> --- a/arch/riscv/kernel/process.c > >> +++ b/arch/riscv/kernel/process.c > >> @@ -97,6 +97,11 @@ void start_thread(struct pt_regs *regs, unsigned long pc, > >> } > >> regs->epc = pc; > >> regs->sp = sp; > >> + > > FIxup: > > > > + #ifdef CONFIG_COMPAT > >> + if (is_compat_task()) > >> + regs->status = (regs->status & ~SR_UXL) | SR_UXL_32; > >> + else > >> + regs->status = (regs->status & ~SR_UXL) | SR_UXL_64; > > + #endif > > > > We still need "#ifdef CONFIG_COMPAT" here, because for rv32 we can't > > set SR_UXL at all. SR_UXL is BIT[32, 33]. > > would an if (IS_ENABLED(CONFIG_COMPAT)) { } around the lot be better > than an #ifdef here? I don't think, seems #ifdef CONFIG_COMPAT is more commonly used in arch/* > > >> } > >> > >> void flush_thread(void) > >> -- > >> 2.25.1 > >> > > > > > > > -- > Ben Dooks http://www.codethink.co.uk/ > Senior Engineer Codethink - Providing Genius > > https://www.codethink.co.uk/privacy.html
On Sat, Mar 12, 2022 at 3:13 AM Guo Ren <guoren@kernel.org> wrote: > On Fri, Mar 11, 2022 at 9:38 PM Ben Dooks <ben.dooks@codethink.co.uk> wrote: > > On 11/03/2022 02:38, Guo Ren wrote: > > >> --- a/arch/riscv/kernel/process.c > > >> +++ b/arch/riscv/kernel/process.c > > >> @@ -97,6 +97,11 @@ void start_thread(struct pt_regs *regs, unsigned long pc, > > >> } > > >> regs->epc = pc; > > >> regs->sp = sp; > > >> + > > > FIxup: > > > > > > + #ifdef CONFIG_COMPAT > > >> + if (is_compat_task()) > > >> + regs->status = (regs->status & ~SR_UXL) | SR_UXL_32; > > >> + else > > >> + regs->status = (regs->status & ~SR_UXL) | SR_UXL_64; > > > + #endif > > > > > > We still need "#ifdef CONFIG_COMPAT" here, because for rv32 we can't > > > set SR_UXL at all. SR_UXL is BIT[32, 33]. > > > > would an if (IS_ENABLED(CONFIG_COMPAT)) { } around the lot be better > > than an #ifdef here? > > I don't think, seems #ifdef CONFIG_COMPAT is more commonly used in arch/* We used to require an #ifdef check around is_compat_task(), so there are a lot of stale #ifdefs that could be removed. In general, 'if (IS_ENABLED())' is considered more readable than #ifdef inside of a function. In this case there are a number of better ways to write the function if you want to get into the details: - firstly, you should remove the #ifdef check around the definition of SR_UXL, otherwise the IS_ENABLED() check does not work. - you can use an 'if (!IS_ENABLED(CONFIG_COMPAT)) \\ return;' ahead of the assignment since that is at the end of the function. - you can remove the bit masking since 'regs->status' is initialized above it, adding in only the one bit, shortening it to if (IS_ENABLED(CONFIG_COMPAT)) regs->status |= is_compat_task()) ? SR_UXL_32 : SR_UXL_64; - to make this more logical, I would suggest always assigning the SR_UXL bits regardless of CONFIG_COMPAT, and instead make it something like if (IS_ENABLED(CONFIG_32BIT) || is_compat_task()) regs->status = | SR_UXL_32; else regs->status = | SR_UXL_64; Arnd
On Sat, Mar 12, 2022 at 4:36 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Sat, Mar 12, 2022 at 3:13 AM Guo Ren <guoren@kernel.org> wrote: > > On Fri, Mar 11, 2022 at 9:38 PM Ben Dooks <ben.dooks@codethink.co.uk> wrote: > > > On 11/03/2022 02:38, Guo Ren wrote: > > > >> --- a/arch/riscv/kernel/process.c > > > >> +++ b/arch/riscv/kernel/process.c > > > >> @@ -97,6 +97,11 @@ void start_thread(struct pt_regs *regs, unsigned long pc, > > > >> } > > > >> regs->epc = pc; > > > >> regs->sp = sp; > > > >> + > > > > FIxup: > > > > > > > > + #ifdef CONFIG_COMPAT > > > >> + if (is_compat_task()) > > > >> + regs->status = (regs->status & ~SR_UXL) | SR_UXL_32; > > > >> + else > > > >> + regs->status = (regs->status & ~SR_UXL) | SR_UXL_64; > > > > + #endif > > > > > > > > We still need "#ifdef CONFIG_COMPAT" here, because for rv32 we can't > > > > set SR_UXL at all. SR_UXL is BIT[32, 33]. > > > > > > would an if (IS_ENABLED(CONFIG_COMPAT)) { } around the lot be better > > > than an #ifdef here? > > > > I don't think, seems #ifdef CONFIG_COMPAT is more commonly used in arch/* > > We used to require an #ifdef check around is_compat_task(), so there are > a lot of stale #ifdefs that could be removed. In general, 'if (IS_ENABLED())' > is considered more readable than #ifdef inside of a function. In this case > there are a number of better ways to write the function if you want to get > into the details: > > - firstly, you should remove the #ifdef check around the definition of > SR_UXL, otherwise the IS_ENABLED() check does not work. > > - you can use an 'if (!IS_ENABLED(CONFIG_COMPAT)) \\ return;' ahead of the > assignment since that is at the end of the function. > > - you can remove the bit masking since 'regs->status' is initialized above it, > adding in only the one bit, shortening it to > > if (IS_ENABLED(CONFIG_COMPAT)) > regs->status |= is_compat_task()) ? SR_UXL_32 : SR_UXL_64; > > - to make this more logical, I would suggest always assigning the SR_UXL > bits regardless of CONFIG_COMPAT, and instead make it something like > > if (IS_ENABLED(CONFIG_32BIT) || is_compat_task()) > regs->status = | SR_UXL_32; > else > regs->status = | SR_UXL_64; When CONFIG_32BIT=y, regs->status is 32bit width but SR_UXL_32 is 34bit width. That's wrong in type. (Only CONFIG_64BIT has SR_UXL). > > Arnd
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c index 03ac3aa611f5..54787ca9806a 100644 --- a/arch/riscv/kernel/process.c +++ b/arch/riscv/kernel/process.c @@ -97,6 +97,11 @@ void start_thread(struct pt_regs *regs, unsigned long pc, } regs->epc = pc; regs->sp = sp; + + if (is_compat_task()) + regs->status = (regs->status & ~SR_UXL) | SR_UXL_32; + else + regs->status = (regs->status & ~SR_UXL) | SR_UXL_64; } void flush_thread(void)