| Message ID | 20231222082711.454374-1-leobras@redhat.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [RFC,1/1] riscv: Improve arch_get_mmap_end() macro | expand |
On Fri, Dec 22, 2023 at 4:27 PM Leonardo Bras <leobras@redhat.com> wrote: > > This macro caused me some confusion, which took some reviewer's time to > make it clear, so I propose adding a short comment in code to avoid > confusion in the future. > > Also, added some improvements to the macro, such as removing the > assumption of VA_USER_SV57 being the largest address space. > > Signed-off-by: Leonardo Bras <leobras@redhat.com> > --- > arch/riscv/include/asm/processor.h | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > index f19f861cda549..2278e2a8362af 100644 > --- a/arch/riscv/include/asm/processor.h > +++ b/arch/riscv/include/asm/processor.h > @@ -18,15 +18,21 @@ > #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) > #define STACK_TOP_MAX TASK_SIZE_64 > > +/* > + * addr is a hint to the maximum userspace address that mmap should provide, so > + * this macro needs to return the largest address space available so that > + * mmap_end < addr, being mmap_end the top of that address space. > + * See Documentation/arch/riscv/vm-layout.rst for more details. > + */ > #define arch_get_mmap_end(addr, len, flags) \ > ({ \ > unsigned long mmap_end; \ > typeof(addr) _addr = (addr); \ > if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > mmap_end = STACK_TOP_MAX; \ > - else if ((_addr) >= VA_USER_SV57) \ > - mmap_end = STACK_TOP_MAX; \ > - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ > + else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \ > + mmap_end = VA_USER_SV57; \ It's clearer. LGTM. Reviewed-by: Guo Ren <guoren@kernel.org> > + else if (((_addr) >= VA_USER_SV48) && (VA_BITS >= VA_BITS_SV48)) \ > mmap_end = VA_USER_SV48; \ > else \ > mmap_end = VA_USER_SV39; \ > -- > 2.43.0 >
On Fri, Dec 22, 2023 at 07:48:36PM +0800, Guo Ren wrote: > On Fri, Dec 22, 2023 at 4:27 PM Leonardo Bras <leobras@redhat.com> wrote: > > > > This macro caused me some confusion, which took some reviewer's time to > > make it clear, so I propose adding a short comment in code to avoid > > confusion in the future. > > > > Also, added some improvements to the macro, such as removing the > > assumption of VA_USER_SV57 being the largest address space. > > > > Signed-off-by: Leonardo Bras <leobras@redhat.com> > > --- > > arch/riscv/include/asm/processor.h | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > > index f19f861cda549..2278e2a8362af 100644 > > --- a/arch/riscv/include/asm/processor.h > > +++ b/arch/riscv/include/asm/processor.h > > @@ -18,15 +18,21 @@ > > #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) > > #define STACK_TOP_MAX TASK_SIZE_64 > > > > +/* > > + * addr is a hint to the maximum userspace address that mmap should provide, so > > + * this macro needs to return the largest address space available so that > > + * mmap_end < addr, being mmap_end the top of that address space. > > + * See Documentation/arch/riscv/vm-layout.rst for more details. > > + */ > > #define arch_get_mmap_end(addr, len, flags) \ > > ({ \ > > unsigned long mmap_end; \ > > typeof(addr) _addr = (addr); \ > > if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > mmap_end = STACK_TOP_MAX; \ > > - else if ((_addr) >= VA_USER_SV57) \ > > - mmap_end = STACK_TOP_MAX; \ > > - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ > > + else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \ > > + mmap_end = VA_USER_SV57; \ > It's clearer. > > LGTM. > Reviewed-by: Guo Ren <guoren@kernel.org> Thanks! Leo > > > + else if (((_addr) >= VA_USER_SV48) && (VA_BITS >= VA_BITS_SV48)) \ > > mmap_end = VA_USER_SV48; \ > > else \ > > mmap_end = VA_USER_SV39; \ > > -- > > 2.43.0 > > > > > -- > Best Regards > Guo Ren >
diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h index f19f861cda549..2278e2a8362af 100644 --- a/arch/riscv/include/asm/processor.h +++ b/arch/riscv/include/asm/processor.h @@ -18,15 +18,21 @@ #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) #define STACK_TOP_MAX TASK_SIZE_64 +/* + * addr is a hint to the maximum userspace address that mmap should provide, so + * this macro needs to return the largest address space available so that + * mmap_end < addr, being mmap_end the top of that address space. + * See Documentation/arch/riscv/vm-layout.rst for more details. + */ #define arch_get_mmap_end(addr, len, flags) \ ({ \ unsigned long mmap_end; \ typeof(addr) _addr = (addr); \ if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ mmap_end = STACK_TOP_MAX; \ - else if ((_addr) >= VA_USER_SV57) \ - mmap_end = STACK_TOP_MAX; \ - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ + else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \ + mmap_end = VA_USER_SV57; \ + else if (((_addr) >= VA_USER_SV48) && (VA_BITS >= VA_BITS_SV48)) \ mmap_end = VA_USER_SV48; \ else \ mmap_end = VA_USER_SV39; \
This macro caused me some confusion, which took some reviewer's time to make it clear, so I propose adding a short comment in code to avoid confusion in the future. Also, added some improvements to the macro, such as removing the assumption of VA_USER_SV57 being the largest address space. Signed-off-by: Leonardo Bras <leobras@redhat.com> --- arch/riscv/include/asm/processor.h | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)