[PATCH 0/3] riscv: mm: Use hint address in mmap if available

[PATCH 0/3] riscv: mm: Use hint address in mmap if available

[Charlie Jenkins]

On riscv, mmap currently returns an address from the largest address
space that can fit entirely inside of the hint address. This makes it
such that the hint address is almost never returned. This patch raises
the mappable area up to and including the hint address. This allows mmap
to often return the hint address, which allows a performance improvement
over searching for a valid address as well as making the behavior more
similar to other architectures.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
Charlie Jenkins (3):
      riscv: mm: Use hint address in mmap if available
      selftests: riscv: Generalize mm selftests
      docs: riscv: Define behavior of mmap

 Documentation/arch/riscv/vm-layout.rst           | 16 ++--
 arch/riscv/include/asm/processor.h               | 21 ++----
 tools/testing/selftests/riscv/mm/mmap_bottomup.c | 20 +----
 tools/testing/selftests/riscv/mm/mmap_default.c  | 20 +----
 tools/testing/selftests/riscv/mm/mmap_test.h     | 93 +++++++++++++-----------
 5 files changed, 66 insertions(+), 104 deletions(-)
---
base-commit: 556e2d17cae620d549c5474b1ece053430cd50bc
change-id: 20240119-use_mmap_hint_address-f9f4b1b6f5f1
-- 
- Charlie

[PATCH 1/3] riscv: mm: Use hint address in mmap if available

[Charlie Jenkins]

On riscv it is guaranteed that the address returned by mmap is less than
the hint address. Allow mmap to return an address all the way up to
addr, if provided, rather than just up to the lower address space.

This provides a performance benefit as well, allowing mmap to exit after
checking that the address is in range rather than searching for a valid
address.

It is possible to provide an address that uses at most the same number
of bits, however it is significantly more computationally expensive to
provide that number rather than setting the max to be the hint address.
There is the instruction clz/clzw in Zbb that returns the highest set bit
which could be used to performantly implement this, but it would still
be slower than the current implementation. At worst case, half of the
address would not be able to be allocated when a hint address is
provided.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 arch/riscv/include/asm/processor.h | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index f19f861cda54..f3ea5166e3b2 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -22,14 +22,11 @@
 ({								\
 	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)) \
-		mmap_end = VA_USER_SV48;			\
+	if ((_addr) == 0 ||					\
+		(IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) ||	\
+		((_addr + len) > BIT(VA_BITS - 1)))		\
 	else							\
-		mmap_end = VA_USER_SV39;			\
+		mmap_end = (_addr + len);			\
 	mmap_end;						\
 })
 
@@ -39,14 +36,12 @@
 	typeof(addr) _addr = (addr);				\
 	typeof(base) _base = (base);				\
 	unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base);	\
-	if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
+	if ((_addr) == 0 ||					\
+	    (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) ||	\
+	    ((_addr + len) > BIT(VA_BITS - 1)))			\
 		mmap_base = (_base);				\
-	else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \
-		mmap_base = VA_USER_SV57 - rnd_gap;		\
-	else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \
-		mmap_base = VA_USER_SV48 - rnd_gap;		\
 	else							\
-		mmap_base = VA_USER_SV39 - rnd_gap;		\
+		mmap_base = (_addr + len) - rnd_gap;		\
 	mmap_base;						\
 })
 

-- 
2.43.0

Re: [PATCH 1/3] riscv: mm: Use hint address in mmap if available

[Stefan O'Rear]

On Mon, Jan 29, 2024, at 7:37 PM, Charlie Jenkins wrote:
> On riscv it is guaranteed that the address returned by mmap is less than
> the hint address. Allow mmap to return an address all the way up to
> addr, if provided, rather than just up to the lower address space.
>
> This provides a performance benefit as well, allowing mmap to exit after
> checking that the address is in range rather than searching for a valid
> address.
>
> It is possible to provide an address that uses at most the same number
> of bits, however it is significantly more computationally expensive to
> provide that number rather than setting the max to be the hint address.
> There is the instruction clz/clzw in Zbb that returns the highest set bit
> which could be used to performantly implement this, but it would still
> be slower than the current implementation. At worst case, half of the
> address would not be able to be allocated when a hint address is
> provided.
>
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
>  arch/riscv/include/asm/processor.h | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/arch/riscv/include/asm/processor.h 
> b/arch/riscv/include/asm/processor.h
> index f19f861cda54..f3ea5166e3b2 100644
> --- a/arch/riscv/include/asm/processor.h
> +++ b/arch/riscv/include/asm/processor.h
> @@ -22,14 +22,11 @@
>  ({								\
>  	unsigned long mmap_end;					\
>  	typeof(addr) _addr = (addr);				\
> -	if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
> -		mmap_end = STACK_TOP_MAX;			\

Setting mmap_end in the no-hint case seems to have been lost?

-s

> -	else if ((_addr) >= VA_USER_SV57)			\
> -		mmap_end = STACK_TOP_MAX;			\
> -	else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \
> -		mmap_end = VA_USER_SV48;			\
> +	if ((_addr) == 0 ||					\
> +		(IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) ||	\
> +		((_addr + len) > BIT(VA_BITS - 1)))		\
>  	else							\
> -		mmap_end = VA_USER_SV39;			\
> +		mmap_end = (_addr + len);			\
>  	mmap_end;						\
>  })
> 
> @@ -39,14 +36,12 @@
>  	typeof(addr) _addr = (addr);				\
>  	typeof(base) _base = (base);				\
>  	unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base);	\
> -	if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
> +	if ((_addr) == 0 ||					\
> +	    (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) ||	\
> +	    ((_addr + len) > BIT(VA_BITS - 1)))			\
>  		mmap_base = (_base);				\
> -	else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \
> -		mmap_base = VA_USER_SV57 - rnd_gap;		\
> -	else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \
> -		mmap_base = VA_USER_SV48 - rnd_gap;		\
>  	else							\
> -		mmap_base = VA_USER_SV39 - rnd_gap;		\
> +		mmap_base = (_addr + len) - rnd_gap;		\
>  	mmap_base;						\
>  })
> 
>
> -- 
> 2.43.0
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/3] riscv: mm: Use hint address in mmap if available

[Charlie Jenkins]

On Mon, Jan 29, 2024 at 08:53:31PM -0500, Stefan O'Rear wrote:
> On Mon, Jan 29, 2024, at 7:37 PM, Charlie Jenkins wrote:
> > On riscv it is guaranteed that the address returned by mmap is less than
> > the hint address. Allow mmap to return an address all the way up to
> > addr, if provided, rather than just up to the lower address space.
> >
> > This provides a performance benefit as well, allowing mmap to exit after
> > checking that the address is in range rather than searching for a valid
> > address.
> >
> > It is possible to provide an address that uses at most the same number
> > of bits, however it is significantly more computationally expensive to
> > provide that number rather than setting the max to be the hint address.
> > There is the instruction clz/clzw in Zbb that returns the highest set bit
> > which could be used to performantly implement this, but it would still
> > be slower than the current implementation. At worst case, half of the
> > address would not be able to be allocated when a hint address is
> > provided.
> >
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > ---
> >  arch/riscv/include/asm/processor.h | 21 ++++++++-------------
> >  1 file changed, 8 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/processor.h 
> > b/arch/riscv/include/asm/processor.h
> > index f19f861cda54..f3ea5166e3b2 100644
> > --- a/arch/riscv/include/asm/processor.h
> > +++ b/arch/riscv/include/asm/processor.h
> > @@ -22,14 +22,11 @@
> >  ({								\
> >  	unsigned long mmap_end;					\
> >  	typeof(addr) _addr = (addr);				\
> > -	if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
> > -		mmap_end = STACK_TOP_MAX;			\
> 
> Setting mmap_end in the no-hint case seems to have been lost?
> 
> -s

Thanks for catching that, will fix in v2.

- Charlie

> 
> > -	else if ((_addr) >= VA_USER_SV57)			\
> > -		mmap_end = STACK_TOP_MAX;			\
> > -	else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \
> > -		mmap_end = VA_USER_SV48;			\
> > +	if ((_addr) == 0 ||					\
> > +		(IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) ||	\
> > +		((_addr + len) > BIT(VA_BITS - 1)))		\
> >  	else							\
> > -		mmap_end = VA_USER_SV39;			\
> > +		mmap_end = (_addr + len);			\
> >  	mmap_end;						\
> >  })
> > 
> > @@ -39,14 +36,12 @@
> >  	typeof(addr) _addr = (addr);				\
> >  	typeof(base) _base = (base);				\
> >  	unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base);	\
> > -	if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
> > +	if ((_addr) == 0 ||					\
> > +	    (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) ||	\
> > +	    ((_addr + len) > BIT(VA_BITS - 1)))			\
> >  		mmap_base = (_base);				\
> > -	else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \
> > -		mmap_base = VA_USER_SV57 - rnd_gap;		\
> > -	else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \
> > -		mmap_base = VA_USER_SV48 - rnd_gap;		\
> >  	else							\
> > -		mmap_base = VA_USER_SV39 - rnd_gap;		\
> > +		mmap_base = (_addr + len) - rnd_gap;		\
> >  	mmap_base;						\
> >  })
> > 
> >
> > -- 
> > 2.43.0
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/3] riscv: mm: Use hint address in mmap if available

[Yangyu Chen]

> On Jan 30, 2024, at 08:37, Charlie Jenkins <charlie@rivosinc.com> wrote:
> 
> On riscv it is guaranteed that the address returned by mmap is less than
> the hint address. Allow mmap to return an address all the way up to
> addr, if provided, rather than just up to the lower address space.
> 
> This provides a performance benefit as well, allowing mmap to exit after
> checking that the address is in range rather than searching for a valid
> address.
> 
> It is possible to provide an address that uses at most the same number
> of bits, however it is significantly more computationally expensive to
> provide that number rather than setting the max to be the hint address.
> There is the instruction clz/clzw in Zbb that returns the highest set bit
> which could be used to performantly implement this, but it would still
> be slower than the current implementation. At worst case, half of the
> address would not be able to be allocated when a hint address is
> provided.
> 
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
> arch/riscv/include/asm/processor.h | 21 ++++++++-------------
> 1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> index f19f861cda54..f3ea5166e3b2 100644
> --- a/arch/riscv/include/asm/processor.h
> +++ b/arch/riscv/include/asm/processor.h
> @@ -22,14 +22,11 @@
> ({ \
> 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)) \
> - mmap_end = VA_USER_SV48; \
> + if ((_addr) == 0 || \
> + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
> + ((_addr + len) > BIT(VA_BITS - 1))) \

How about replacing BIT(VA_BITS-1) to DEFAULT_MAP_WINDOW to make the code
more general.

> else \
> - mmap_end = VA_USER_SV39; \
> + mmap_end = (_addr + len); \
> mmap_end; \
> })
> 
> @@ -39,14 +36,12 @@
> typeof(addr) _addr = (addr); \
> typeof(base) _base = (base); \
> unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base); \
> - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
> + if ((_addr) == 0 || \
> +    (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
> +    ((_addr + len) > BIT(VA_BITS - 1))) \

Same here.

> mmap_base = (_base); \
> - else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \
> - mmap_base = VA_USER_SV57 - rnd_gap; \
> - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \
> - mmap_base = VA_USER_SV48 - rnd_gap; \
> else \
> - mmap_base = VA_USER_SV39 - rnd_gap; \
> + mmap_base = (_addr + len) - rnd_gap; \
> mmap_base; \
> })
> 
> 

What about not setting the upper bound as x86/arm/powerpc as [1] did?
In this case, user space can directly pass a constant hint address >
BIT(47) to get a mapping in sv57. If you want this, this code also allows
user-space to pass any address larger than TASK_SIZE. You should also
limit the mmap_base to (base) + TASK_SIZE - DEFAULT_MAP_WINDOW.

I’m also aware of the rnd_gap if it is not 0, then we will not get
address mapped to VA_USER_SV39 - rnd_gap.

[1]. https://lore.kernel.org/linux-riscv/tencent_2683632BEE438C6D4854E30BDF9CA0843606@qq.com/

> -- 
> 2.43.0
> 

Re: [PATCH 1/3] riscv: mm: Use hint address in mmap if available

[Charlie Jenkins]

On Tue, Jan 30, 2024 at 10:34:03AM +0800, Yangyu Chen wrote:
> 
> > On Jan 30, 2024, at 08:37, Charlie Jenkins <charlie@rivosinc.com> wrote:
> > 
> > On riscv it is guaranteed that the address returned by mmap is less than
> > the hint address. Allow mmap to return an address all the way up to
> > addr, if provided, rather than just up to the lower address space.
> > 
> > This provides a performance benefit as well, allowing mmap to exit after
> > checking that the address is in range rather than searching for a valid
> > address.
> > 
> > It is possible to provide an address that uses at most the same number
> > of bits, however it is significantly more computationally expensive to
> > provide that number rather than setting the max to be the hint address.
> > There is the instruction clz/clzw in Zbb that returns the highest set bit
> > which could be used to performantly implement this, but it would still
> > be slower than the current implementation. At worst case, half of the
> > address would not be able to be allocated when a hint address is
> > provided.
> > 
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > ---
> > arch/riscv/include/asm/processor.h | 21 ++++++++-------------
> > 1 file changed, 8 insertions(+), 13 deletions(-)
> > 
> > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> > index f19f861cda54..f3ea5166e3b2 100644
> > --- a/arch/riscv/include/asm/processor.h
> > +++ b/arch/riscv/include/asm/processor.h
> > @@ -22,14 +22,11 @@
> > ({ \
> > 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)) \
> > - mmap_end = VA_USER_SV48; \
> > + if ((_addr) == 0 || \
> > + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
> > + ((_addr + len) > BIT(VA_BITS - 1))) \
> 
> How about replacing BIT(VA_BITS-1) to DEFAULT_MAP_WINDOW to make the code
> more general.
> 
> > else \
> > - mmap_end = VA_USER_SV39; \
> > + mmap_end = (_addr + len); \
> > mmap_end; \
> > })
> > 
> > @@ -39,14 +36,12 @@
> > typeof(addr) _addr = (addr); \
> > typeof(base) _base = (base); \
> > unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base); \
> > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
> > + if ((_addr) == 0 || \
> > +    (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
> > +    ((_addr + len) > BIT(VA_BITS - 1))) \
> 
> Same here.
> 
> > mmap_base = (_base); \
> > - else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \
> > - mmap_base = VA_USER_SV57 - rnd_gap; \
> > - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \
> > - mmap_base = VA_USER_SV48 - rnd_gap; \
> > else \
> > - mmap_base = VA_USER_SV39 - rnd_gap; \
> > + mmap_base = (_addr + len) - rnd_gap; \
> > mmap_base; \
> > })
> > 
> > 
> 
> What about not setting the upper bound as x86/arm/powerpc as [1] did?
> In this case, user space can directly pass a constant hint address >
> BIT(47) to get a mapping in sv57. If you want this, this code also allows
> user-space to pass any address larger than TASK_SIZE. You should also
> limit the mmap_base to (base) + TASK_SIZE - DEFAULT_MAP_WINDOW.

No. This suggestion causes a random address to be used if the hint
address is not available. That is why I didn't simply go with your
patch.

This patch both gives your application the benefit of being able to use
a hint address in the hopes that the address is available, as well as
continuing to support the guarantee that an address using more bits than
the hint address is not returned.

- Charlie

> 
> I’m also aware of the rnd_gap if it is not 0, then we will not get
> address mapped to VA_USER_SV39 - rnd_gap.
> 
> [1]. https://lore.kernel.org/linux-riscv/tencent_2683632BEE438C6D4854E30BDF9CA0843606@qq.com/
> 
> > -- 
> > 2.43.0
> > 
> 

Re: [PATCH 1/3] riscv: mm: Use hint address in mmap if available

[Yangyu Chen]

On Mon, 2024-01-29 at 18:50 -0800, Charlie Jenkins wrote:
> On Tue, Jan 30, 2024 at 10:34:03AM +0800, Yangyu Chen wrote:
> > 
> > > On Jan 30, 2024, at 08:37, Charlie Jenkins <charlie@rivosinc.com>
> > > wrote:
> > > 
> > > On riscv it is guaranteed that the address returned by mmap is
> > > less than
> > > the hint address. Allow mmap to return an address all the way up
> > > to
> > > addr, if provided, rather than just up to the lower address
> > > space.
> > > 
> > > This provides a performance benefit as well, allowing mmap to
> > > exit after
> > > checking that the address is in range rather than searching for a
> > > valid
> > > address.
> > > 
> > > It is possible to provide an address that uses at most the same
> > > number
> > > of bits, however it is significantly more computationally
> > > expensive to
> > > provide that number rather than setting the max to be the hint
> > > address.
> > > There is the instruction clz/clzw in Zbb that returns the highest
> > > set bit
> > > which could be used to performantly implement this, but it would
> > > still
> > > be slower than the current implementation. At worst case, half of
> > > the
> > > address would not be able to be allocated when a hint address is
> > > provided.
> > > 
> > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > > ---
> > > arch/riscv/include/asm/processor.h | 21 ++++++++-------------
> > > 1 file changed, 8 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/arch/riscv/include/asm/processor.h
> > > b/arch/riscv/include/asm/processor.h
> > > index f19f861cda54..f3ea5166e3b2 100644
> > > --- a/arch/riscv/include/asm/processor.h
> > > +++ b/arch/riscv/include/asm/processor.h
> > > @@ -22,14 +22,11 @@
> > > ({ \
> > > 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)) \
> > > - mmap_end = VA_USER_SV48; \
> > > + if ((_addr) == 0 || \
> > > + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
> > > + ((_addr + len) > BIT(VA_BITS - 1))) \
> > 
> > How about replacing BIT(VA_BITS-1) to DEFAULT_MAP_WINDOW to make
> > the code
> > more general.
> > 
> > > else \
> > > - mmap_end = VA_USER_SV39; \
> > > + mmap_end = (_addr + len); \
> > > mmap_end; \
> > > })
> > > 
> > > @@ -39,14 +36,12 @@
> > > typeof(addr) _addr = (addr); \
> > > typeof(base) _base = (base); \
> > > unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base); \
> > > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
> > > is_compat_task())) \
> > > + if ((_addr) == 0 || \
> > > +    (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
> > > +    ((_addr + len) > BIT(VA_BITS - 1))) \
> > 
> > Same here.
> > 
> > > mmap_base = (_base); \
> > > - else if (((_addr) >= VA_USER_SV57) && (VA_BITS >=
> > > VA_BITS_SV57)) \
> > > - mmap_base = VA_USER_SV57 - rnd_gap; \
> > > - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
> > > VA_BITS_SV48)) \
> > > - mmap_base = VA_USER_SV48 - rnd_gap; \
> > > else \
> > > - mmap_base = VA_USER_SV39 - rnd_gap; \
> > > + mmap_base = (_addr + len) - rnd_gap; \
> > > mmap_base; \
> > > })
> > > 
> > > 
> > 
> > What about not setting the upper bound as x86/arm/powerpc as [1]
> > did?
> > In this case, user space can directly pass a constant hint address
> > >
> > BIT(47) to get a mapping in sv57. If you want this, this code also
> > allows
> > user-space to pass any address larger than TASK_SIZE. You should
> > also
> > limit the mmap_base to (base) + TASK_SIZE - DEFAULT_MAP_WINDOW.
> 
> No. This suggestion causes a random address to be used if the hint
> address is not available. That is why I didn't simply go with your
> patch.


I think return random address is expected and other ISAs like
x86/arm/powerpc will also return random address if hint is NULL.

Also add CC linux-mm to get more opinions from people who familiar with
mm.

> 
> This patch both gives your application the benefit of being able to
> use
> a hint address in the hopes that the address is available, as well as
> continuing to support the guarantee that an address using more bits
> than
> the hint address is not returned.
> 
> - Charlie
> 
> > 
> > I’m also aware of the rnd_gap if it is not 0, then we will not get
> > address mapped to VA_USER_SV39 - rnd_gap.
> > 
> > [1].
> > https://lore.kernel.org/linux-riscv/tencent_2683632BEE438C6D4854E30BDF9CA0843606@qq.com/
> > 
> > > -- 
> > > 2.43.0
> > > 
> > 

Re: [PATCH 1/3] riscv: mm: Use hint address in mmap if available

[kernel test robot]

Hi Charlie,

kernel test robot noticed the following build errors:

[auto build test ERROR on 556e2d17cae620d549c5474b1ece053430cd50bc]

url:    https://github.com/intel-lab-lkp/linux/commits/Charlie-Jenkins/riscv-mm-Use-hint-address-in-mmap-if-available/20240130-084208
base:   556e2d17cae620d549c5474b1ece053430cd50bc
patch link:    https://lore.kernel.org/r/20240129-use_mmap_hint_address-v1-1-4c74da813ba1%40rivosinc.com
patch subject: [PATCH 1/3] riscv: mm: Use hint address in mmap if available
config: riscv-defconfig (https://download.01.org/0day-ci/archive/20240131/202401310404.eNJvHoC9-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240131/202401310404.eNJvHoC9-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401310404.eNJvHoC9-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from arch/riscv/include/asm/irqflags.h:10,
                    from include/linux/irqflags.h:18,
                    from arch/riscv/include/asm/bitops.h:14,
                    from include/linux/bitops.h:68,
                    from include/linux/kernel.h:23,
                    from mm/mmap.c:12:
   mm/mmap.c: In function 'generic_get_unmapped_area':
>> arch/riscv/include/asm/processor.h:28:9: error: expected expression before 'else'
      28 |         else                                                    \
         |         ^~~~
   mm/mmap.c:1703:40: note: in expansion of macro 'arch_get_mmap_end'
    1703 |         const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
         |                                        ^~~~~~~~~~~~~~~~~
   mm/mmap.c: In function 'generic_get_unmapped_area_topdown':
>> arch/riscv/include/asm/processor.h:28:9: error: expected expression before 'else'
      28 |         else                                                    \
         |         ^~~~
   mm/mmap.c:1751:40: note: in expansion of macro 'arch_get_mmap_end'
    1751 |         const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
         |                                        ^~~~~~~~~~~~~~~~~
--
   In file included from arch/riscv/include/asm/irqflags.h:10,
                    from include/linux/irqflags.h:18,
                    from arch/riscv/include/asm/bitops.h:14,
                    from include/linux/bitops.h:68,
                    from include/linux/thread_info.h:27,
                    from fs/hugetlbfs/inode.c:12:
   fs/hugetlbfs/inode.c: In function 'hugetlb_get_unmapped_area_bottomup':
>> arch/riscv/include/asm/processor.h:28:9: error: expected expression before 'else'
      28 |         else                                                    \
         |         ^~~~
   fs/hugetlbfs/inode.c:173:27: note: in expansion of macro 'arch_get_mmap_end'
     173 |         info.high_limit = arch_get_mmap_end(addr, len, flags);
         |                           ^~~~~~~~~~~~~~~~~
   fs/hugetlbfs/inode.c: In function 'hugetlb_get_unmapped_area_topdown':
>> arch/riscv/include/asm/processor.h:28:9: error: expected expression before 'else'
      28 |         else                                                    \
         |         ^~~~
   fs/hugetlbfs/inode.c:204:35: note: in expansion of macro 'arch_get_mmap_end'
     204 |                 info.high_limit = arch_get_mmap_end(addr, len, flags);
         |                                   ^~~~~~~~~~~~~~~~~
   fs/hugetlbfs/inode.c: In function 'generic_hugetlb_get_unmapped_area':
>> arch/riscv/include/asm/processor.h:28:9: error: expected expression before 'else'
      28 |         else                                                    \
         |         ^~~~
   fs/hugetlbfs/inode.c:219:40: note: in expansion of macro 'arch_get_mmap_end'
     219 |         const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
         |                                        ^~~~~~~~~~~~~~~~~


vim +/else +28 arch/riscv/include/asm/processor.h

add2cc6b6515f7 Charlie Jenkins 2023-08-09  20  
add2cc6b6515f7 Charlie Jenkins 2023-08-09  21  #define arch_get_mmap_end(addr, len, flags)			\
add2cc6b6515f7 Charlie Jenkins 2023-08-09  22  ({								\
add2cc6b6515f7 Charlie Jenkins 2023-08-09  23  	unsigned long mmap_end;					\
add2cc6b6515f7 Charlie Jenkins 2023-08-09  24  	typeof(addr) _addr = (addr);				\
c5712238cfe3f5 Charlie Jenkins 2024-01-29  25  	if ((_addr) == 0 ||					\
c5712238cfe3f5 Charlie Jenkins 2024-01-29  26  		(IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) ||	\
c5712238cfe3f5 Charlie Jenkins 2024-01-29  27  		((_addr + len) > BIT(VA_BITS - 1)))		\
add2cc6b6515f7 Charlie Jenkins 2023-08-09 @28  	else							\
c5712238cfe3f5 Charlie Jenkins 2024-01-29  29  		mmap_end = (_addr + len);			\
add2cc6b6515f7 Charlie Jenkins 2023-08-09  30  	mmap_end;						\
add2cc6b6515f7 Charlie Jenkins 2023-08-09  31  })
add2cc6b6515f7 Charlie Jenkins 2023-08-09  32  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/3] riscv: mm: Use hint address in mmap if available

[kernel test robot]

Hi Charlie,

kernel test robot noticed the following build errors:

[auto build test ERROR on 556e2d17cae620d549c5474b1ece053430cd50bc]

url:    https://github.com/intel-lab-lkp/linux/commits/Charlie-Jenkins/riscv-mm-Use-hint-address-in-mmap-if-available/20240130-084208
base:   556e2d17cae620d549c5474b1ece053430cd50bc
patch link:    https://lore.kernel.org/r/20240129-use_mmap_hint_address-v1-1-4c74da813ba1%40rivosinc.com
patch subject: [PATCH 1/3] riscv: mm: Use hint address in mmap if available
config: riscv-allnoconfig (https://download.01.org/0day-ci/archive/20240131/202401310513.lub8Ilwm-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project fdac7d0b6f74f919d319b31a0680c77f66732586)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240131/202401310513.lub8Ilwm-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401310513.lub8Ilwm-lkp@intel.com/

All errors (new ones prefixed by >>):

>> mm/mmap.c:1703:33: error: expected expression
    1703 |         const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
         |                                        ^
   arch/riscv/include/asm/processor.h:28:2: note: expanded from macro 'arch_get_mmap_end'
      28 |         else                                                    \
         |         ^
   mm/mmap.c:1751:33: error: expected expression
    1751 |         const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
         |                                        ^
   arch/riscv/include/asm/processor.h:28:2: note: expanded from macro 'arch_get_mmap_end'
      28 |         else                                                    \
         |         ^
   2 errors generated.


vim +1703 mm/mmap.c

f6795053dac8d4d Steve Capper           2018-12-06  1683  
^1da177e4c3f415 Linus Torvalds         2005-04-16  1684  /* Get an address range which is currently unmapped.
^1da177e4c3f415 Linus Torvalds         2005-04-16  1685   * For shmat() with addr=0.
^1da177e4c3f415 Linus Torvalds         2005-04-16  1686   *
^1da177e4c3f415 Linus Torvalds         2005-04-16  1687   * Ugly calling convention alert:
^1da177e4c3f415 Linus Torvalds         2005-04-16  1688   * Return value with the low bits set means error value,
^1da177e4c3f415 Linus Torvalds         2005-04-16  1689   * ie
^1da177e4c3f415 Linus Torvalds         2005-04-16  1690   *	if (ret & ~PAGE_MASK)
^1da177e4c3f415 Linus Torvalds         2005-04-16  1691   *		error = ret;
^1da177e4c3f415 Linus Torvalds         2005-04-16  1692   *
^1da177e4c3f415 Linus Torvalds         2005-04-16  1693   * This function "knows" that -ENOMEM has the bits set.
^1da177e4c3f415 Linus Torvalds         2005-04-16  1694   */
^1da177e4c3f415 Linus Torvalds         2005-04-16  1695  unsigned long
4b439e25e29ec33 Christophe Leroy       2022-04-09  1696  generic_get_unmapped_area(struct file *filp, unsigned long addr,
4b439e25e29ec33 Christophe Leroy       2022-04-09  1697  			  unsigned long len, unsigned long pgoff,
4b439e25e29ec33 Christophe Leroy       2022-04-09  1698  			  unsigned long flags)
^1da177e4c3f415 Linus Torvalds         2005-04-16  1699  {
^1da177e4c3f415 Linus Torvalds         2005-04-16  1700  	struct mm_struct *mm = current->mm;
1be7107fbe18eed Hugh Dickins           2017-06-19  1701  	struct vm_area_struct *vma, *prev;
db4fbfb9523c935 Michel Lespinasse      2012-12-11  1702  	struct vm_unmapped_area_info info;
2cb4de085f383cb Christophe Leroy       2022-04-09 @1703  	const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
^1da177e4c3f415 Linus Torvalds         2005-04-16  1704  
f6795053dac8d4d Steve Capper           2018-12-06  1705  	if (len > mmap_end - mmap_min_addr)
^1da177e4c3f415 Linus Torvalds         2005-04-16  1706  		return -ENOMEM;
^1da177e4c3f415 Linus Torvalds         2005-04-16  1707  
06abdfb47ee745a Benjamin Herrenschmidt 2007-05-06  1708  	if (flags & MAP_FIXED)
06abdfb47ee745a Benjamin Herrenschmidt 2007-05-06  1709  		return addr;
06abdfb47ee745a Benjamin Herrenschmidt 2007-05-06  1710  
^1da177e4c3f415 Linus Torvalds         2005-04-16  1711  	if (addr) {
^1da177e4c3f415 Linus Torvalds         2005-04-16  1712  		addr = PAGE_ALIGN(addr);
1be7107fbe18eed Hugh Dickins           2017-06-19  1713  		vma = find_vma_prev(mm, addr, &prev);
f6795053dac8d4d Steve Capper           2018-12-06  1714  		if (mmap_end - len >= addr && addr >= mmap_min_addr &&
1be7107fbe18eed Hugh Dickins           2017-06-19  1715  		    (!vma || addr + len <= vm_start_gap(vma)) &&
1be7107fbe18eed Hugh Dickins           2017-06-19  1716  		    (!prev || addr >= vm_end_gap(prev)))
^1da177e4c3f415 Linus Torvalds         2005-04-16  1717  			return addr;
^1da177e4c3f415 Linus Torvalds         2005-04-16  1718  	}
^1da177e4c3f415 Linus Torvalds         2005-04-16  1719  
db4fbfb9523c935 Michel Lespinasse      2012-12-11  1720  	info.flags = 0;
db4fbfb9523c935 Michel Lespinasse      2012-12-11  1721  	info.length = len;
4e99b02131b280b Heiko Carstens         2013-11-12  1722  	info.low_limit = mm->mmap_base;
f6795053dac8d4d Steve Capper           2018-12-06  1723  	info.high_limit = mmap_end;
db4fbfb9523c935 Michel Lespinasse      2012-12-11  1724  	info.align_mask = 0;
09ef5283fd96ac4 Jaewon Kim             2020-04-10  1725  	info.align_offset = 0;
db4fbfb9523c935 Michel Lespinasse      2012-12-11  1726  	return vm_unmapped_area(&info);
^1da177e4c3f415 Linus Torvalds         2005-04-16  1727  }
4b439e25e29ec33 Christophe Leroy       2022-04-09  1728  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 2/3] selftests: riscv: Generalize mm selftests

[Charlie Jenkins]

The behavior of mmap on riscv is defined to not provide an address that
uses more bits than the hint address, if provided. Make the tests
reflect that.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 tools/testing/selftests/riscv/mm/mmap_bottomup.c | 20 +----
 tools/testing/selftests/riscv/mm/mmap_default.c  | 20 +----
 tools/testing/selftests/riscv/mm/mmap_test.h     | 93 +++++++++++++-----------
 3 files changed, 53 insertions(+), 80 deletions(-)

diff --git a/tools/testing/selftests/riscv/mm/mmap_bottomup.c b/tools/testing/selftests/riscv/mm/mmap_bottomup.c
index 1757d19ca89b..bad8e854263d 100644
--- a/tools/testing/selftests/riscv/mm/mmap_bottomup.c
+++ b/tools/testing/selftests/riscv/mm/mmap_bottomup.c
@@ -8,27 +8,9 @@ TEST(infinite_rlimit)
 {
 // Only works on 64 bit
 #if __riscv_xlen == 64
-	struct addresses mmap_addresses;
-
 	EXPECT_EQ(BOTTOM_UP, memory_layout());
 
-	do_mmaps(&mmap_addresses);
-
-	EXPECT_NE(MAP_FAILED, mmap_addresses.no_hint);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_37_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_38_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_46_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_47_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_55_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_56_addr);
-
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.no_hint);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_37_addr);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_38_addr);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_46_addr);
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.on_47_addr);
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.on_55_addr);
-	EXPECT_GT(1UL << 56, (unsigned long)mmap_addresses.on_56_addr);
+	TEST_MMAPS;
 #endif
 }
 
diff --git a/tools/testing/selftests/riscv/mm/mmap_default.c b/tools/testing/selftests/riscv/mm/mmap_default.c
index c63c60b9397e..a3874778d795 100644
--- a/tools/testing/selftests/riscv/mm/mmap_default.c
+++ b/tools/testing/selftests/riscv/mm/mmap_default.c
@@ -8,27 +8,9 @@ TEST(default_rlimit)
 {
 // Only works on 64 bit
 #if __riscv_xlen == 64
-	struct addresses mmap_addresses;
-
 	EXPECT_EQ(TOP_DOWN, memory_layout());
 
-	do_mmaps(&mmap_addresses);
-
-	EXPECT_NE(MAP_FAILED, mmap_addresses.no_hint);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_37_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_38_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_46_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_47_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_55_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_56_addr);
-
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.no_hint);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_37_addr);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_38_addr);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_46_addr);
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.on_47_addr);
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.on_55_addr);
-	EXPECT_GT(1UL << 56, (unsigned long)mmap_addresses.on_56_addr);
+	TEST_MMAPS;
 #endif
 }
 
diff --git a/tools/testing/selftests/riscv/mm/mmap_test.h b/tools/testing/selftests/riscv/mm/mmap_test.h
index 9b8434f62f57..93face2b3118 100644
--- a/tools/testing/selftests/riscv/mm/mmap_test.h
+++ b/tools/testing/selftests/riscv/mm/mmap_test.h
@@ -4,60 +4,69 @@
 #include <sys/mman.h>
 #include <sys/resource.h>
 #include <stddef.h>
+#include <strings.h>
+#include "../../kselftest_harness.h"
 
 #define TOP_DOWN 0
 #define BOTTOM_UP 1
 
-struct addresses {
-	int *no_hint;
-	int *on_37_addr;
-	int *on_38_addr;
-	int *on_46_addr;
-	int *on_47_addr;
-	int *on_55_addr;
-	int *on_56_addr;
+uint64_t random_addresses[] = {
+	0x19764f0d73b3a9f0, 0x016049584cecef59, 0x3580bdd3562f4acd,
+	0x1164219f20b17da0, 0x07d97fcb40ff2373, 0x76ec528921272ee7,
+	0x4dd48c38a3de3f70, 0x2e11415055f6997d, 0x14b43334ac476c02,
+	0x375a60795aff19f6, 0x47f3051725b8ee1a, 0x4e697cf240494a9f,
+	0x456b59b5c2f9e9d1, 0x101724379d63cb96, 0x7fe9ad31619528c1,
+	0x2f417247c495c2ea, 0x329a5a5b82943a5e, 0x06d7a9d6adcd3827,
+	0x327b0b9ee37f62d5, 0x17c7b1851dfd9b76, 0x006ebb6456ec2cd9,
+	0x00836cd14146a134, 0x00e5c4dcde7126db, 0x004c29feadf75753,
+	0x00d8b20149ed930c, 0x00d71574c269387a, 0x0006ebe4a82acb7a,
+	0x0016135df51f471b, 0x00758bdb55455160, 0x00d0bdd949b13b32,
+	0x00ecea01e7c5f54b, 0x00e37b071b9948b1, 0x0011fdd00ff57ab3,
+	0x00e407294b52f5ea, 0x00567748c200ed20, 0x000d073084651046,
+	0x00ac896f4365463c, 0x00eb0d49a0b26216, 0x0066a2564a982a31,
+	0x002e0d20237784ae, 0x0000554ff8a77a76, 0x00006ce07a54c012,
+	0x000009570516d799, 0x00000954ca15b84d, 0x0000684f0d453379,
+	0x00002ae5816302b5, 0x0000042403fb54bf, 0x00004bad7392bf30,
+	0x00003e73bfa4b5e3, 0x00005442c29978e0, 0x00002803f11286b6,
+	0x000073875d745fc6, 0x00007cede9cb8240, 0x000027df84cc6a4f,
+	0x00006d7e0e74242a, 0x00004afd0b836e02, 0x000047d0e837cd82,
+	0x00003b42405efeda, 0x00001531bafa4c95, 0x00007172cae34ac4,
+	0x0000002732f06b2b, 0x00000012cbf8fd0b, 0x0000001fcc6af0e8,
 };
 
-static inline void do_mmaps(struct addresses *mmap_addresses)
-{
-	/*
-	 * Place all of the hint addresses on the boundaries of mmap
-	 * sv39, sv48, sv57
-	 * User addresses end at 1<<38, 1<<47, 1<<56 respectively
-	 */
-	void *on_37_bits = (void *)(1UL << 37);
-	void *on_38_bits = (void *)(1UL << 38);
-	void *on_46_bits = (void *)(1UL << 46);
-	void *on_47_bits = (void *)(1UL << 47);
-	void *on_55_bits = (void *)(1UL << 55);
-	void *on_56_bits = (void *)(1UL << 56);
 
-	int prot = PROT_READ | PROT_WRITE;
-	int flags = MAP_PRIVATE | MAP_ANONYMOUS;
+#define PROT (PROT_READ | PROT_WRITE)
+#define FLAGS (MAP_PRIVATE | MAP_ANONYMOUS)
+
+/* mmap must return a value that doesn't use more bits than the hint address. */
+static inline unsigned long get_max_value(unsigned long input)
+{
+	unsigned long max_bit = (1UL << (ffsl(input) - 1));
 
-	mmap_addresses->no_hint =
-		mmap(NULL, 5 * sizeof(int), prot, flags, 0, 0);
-	mmap_addresses->on_37_addr =
-		mmap(on_37_bits, 5 * sizeof(int), prot, flags, 0, 0);
-	mmap_addresses->on_38_addr =
-		mmap(on_38_bits, 5 * sizeof(int), prot, flags, 0, 0);
-	mmap_addresses->on_46_addr =
-		mmap(on_46_bits, 5 * sizeof(int), prot, flags, 0, 0);
-	mmap_addresses->on_47_addr =
-		mmap(on_47_bits, 5 * sizeof(int), prot, flags, 0, 0);
-	mmap_addresses->on_55_addr =
-		mmap(on_55_bits, 5 * sizeof(int), prot, flags, 0, 0);
-	mmap_addresses->on_56_addr =
-		mmap(on_56_bits, 5 * sizeof(int), prot, flags, 0, 0);
+	return max_bit + (max_bit - 1);
 }
 
+#define TEST_MMAPS                                                            \
+	({                                                                    \
+		void *mmap_addr;                                              \
+		for (int i = 0; i < ARRAY_SIZE(random_addresses); i++) {      \
+			mmap_addr = mmap((void *)random_addresses[i],         \
+					 5 * sizeof(int), PROT, FLAGS, 0, 0); \
+			EXPECT_NE(MAP_FAILED, mmap_addr);                     \
+			EXPECT_GE((void *)get_max_value(random_addresses[i]), \
+				  mmap_addr);                                 \
+			mmap_addr = mmap((void *)random_addresses[i],         \
+					 5 * sizeof(int), PROT, FLAGS, 0, 0); \
+			EXPECT_NE(MAP_FAILED, mmap_addr);                     \
+			EXPECT_GE((void *)get_max_value(random_addresses[i]), \
+				  mmap_addr);                                 \
+		}                                                             \
+	})
+
 static inline int memory_layout(void)
 {
-	int prot = PROT_READ | PROT_WRITE;
-	int flags = MAP_PRIVATE | MAP_ANONYMOUS;
-
-	void *value1 = mmap(NULL, sizeof(int), prot, flags, 0, 0);
-	void *value2 = mmap(NULL, sizeof(int), prot, flags, 0, 0);
+	void *value1 = mmap(NULL, sizeof(int), PROT, FLAGS, 0, 0);
+	void *value2 = mmap(NULL, sizeof(int), PROT, FLAGS, 0, 0);
 
 	return value2 > value1;
 }

-- 
2.43.0

[PATCH 3/3] docs: riscv: Define behavior of mmap

[Charlie Jenkins]

Define mmap on riscv to not provide an address that uses more bits than
the hint address, if provided.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 Documentation/arch/riscv/vm-layout.rst | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/Documentation/arch/riscv/vm-layout.rst b/Documentation/arch/riscv/vm-layout.rst
index 69ff6da1dbf8..e476b4386bd9 100644
--- a/Documentation/arch/riscv/vm-layout.rst
+++ b/Documentation/arch/riscv/vm-layout.rst
@@ -144,14 +144,8 @@ passing 0 into the hint address parameter of mmap. On CPUs with an address space
 smaller than sv48, the CPU maximum supported address space will be the default.
 
 Software can "opt-in" to receiving VAs from another VA space by providing
-a hint address to mmap. A hint address passed to mmap will cause the largest
-address space that fits entirely into the hint to be used, unless there is no
-space left in the address space. If there is no space available in the requested
-address space, an address in the next smallest available address space will be
-returned.
-
-For example, in order to obtain 48-bit VA space, a hint address greater than
-:code:`1 << 47` must be provided. Note that this is 47 due to sv48 userspace
-ending at :code:`1 << 47` and the addresses beyond this are reserved for the
-kernel. Similarly, to obtain 57-bit VA space addresses, a hint address greater
-than or equal to :code:`1 << 56` must be provided.
+a hint address to mmap. When a hint address is passed to mmap, the returned
+address will never use more bits than the hint address. For example, if a hint
+address of `1 << 40` is passed to mmap, a valid returned address will never use
+bits 41 through 63. If no mappable addresses are available in that range, mmap
+will return `MAP_FAILED`.

-- 
2.43.0

Re: [PATCH 0/3] riscv: mm: Use hint address in mmap if available

[Stefan O'Rear]

On Mon, Jan 29, 2024, at 7:36 PM, Charlie Jenkins wrote:
> On riscv, mmap currently returns an address from the largest address
> space that can fit entirely inside of the hint address. This makes it
> such that the hint address is almost never returned. This patch raises
> the mappable area up to and including the hint address. This allows mmap
> to often return the hint address, which allows a performance improvement
> over searching for a valid address as well as making the behavior more
> similar to other architectures.

This means that if an application or library opts in to Sv48 support by
passing a nonzero hint, it will lose the benefits of ASLR.

Allowing applications to opt in to a VA space smaller than the
architectural minimum seems like an independently useful feature.
Is there a reason to only add it for riscv64?

-s

> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
> Charlie Jenkins (3):
>       riscv: mm: Use hint address in mmap if available
>       selftests: riscv: Generalize mm selftests
>       docs: riscv: Define behavior of mmap
>
>  Documentation/arch/riscv/vm-layout.rst           | 16 ++--
>  arch/riscv/include/asm/processor.h               | 21 ++----
>  tools/testing/selftests/riscv/mm/mmap_bottomup.c | 20 +----
>  tools/testing/selftests/riscv/mm/mmap_default.c  | 20 +----
>  tools/testing/selftests/riscv/mm/mmap_test.h     | 93 +++++++++++++-----------
>  5 files changed, 66 insertions(+), 104 deletions(-)
> ---
> base-commit: 556e2d17cae620d549c5474b1ece053430cd50bc
> change-id: 20240119-use_mmap_hint_address-f9f4b1b6f5f1
> -- 
> - Charlie
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 0/3] riscv: mm: Use hint address in mmap if available

[Charlie Jenkins]

On Mon, Jan 29, 2024 at 09:04:50PM -0500, Stefan O'Rear wrote:
> On Mon, Jan 29, 2024, at 7:36 PM, Charlie Jenkins wrote:
> > On riscv, mmap currently returns an address from the largest address
> > space that can fit entirely inside of the hint address. This makes it
> > such that the hint address is almost never returned. This patch raises
> > the mappable area up to and including the hint address. This allows mmap
> > to often return the hint address, which allows a performance improvement
> > over searching for a valid address as well as making the behavior more
> > similar to other architectures.
> 
> This means that if an application or library opts in to Sv48 support by
> passing a nonzero hint, it will lose the benefits of ASLR.

sv48 is default. However your statement stands for opting into sv57.
If they always pass the same hint address, only the first address will
be deterministic though, correct?

> 
> Allowing applications to opt in to a VA space smaller than the
> architectural minimum seems like an independently useful feature.
> Is there a reason to only add it for riscv64?
> 

If there is interest, it can be added to other architectures as well.

- Charlie

> -s
> 
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > ---
> > Charlie Jenkins (3):
> >       riscv: mm: Use hint address in mmap if available
> >       selftests: riscv: Generalize mm selftests
> >       docs: riscv: Define behavior of mmap
> >
> >  Documentation/arch/riscv/vm-layout.rst           | 16 ++--
> >  arch/riscv/include/asm/processor.h               | 21 ++----
> >  tools/testing/selftests/riscv/mm/mmap_bottomup.c | 20 +----
> >  tools/testing/selftests/riscv/mm/mmap_default.c  | 20 +----
> >  tools/testing/selftests/riscv/mm/mmap_test.h     | 93 +++++++++++++-----------
> >  5 files changed, 66 insertions(+), 104 deletions(-)
> > ---
> > base-commit: 556e2d17cae620d549c5474b1ece053430cd50bc
> > change-id: 20240119-use_mmap_hint_address-f9f4b1b6f5f1
> > -- 
> > - Charlie
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 0/3] riscv: mm: Use hint address in mmap if available

[Stefan O'Rear]

On Mon, Jan 29, 2024, at 9:13 PM, Charlie Jenkins wrote:
> On Mon, Jan 29, 2024 at 09:04:50PM -0500, Stefan O'Rear wrote:
>> On Mon, Jan 29, 2024, at 7:36 PM, Charlie Jenkins wrote:
>> > On riscv, mmap currently returns an address from the largest address
>> > space that can fit entirely inside of the hint address. This makes it
>> > such that the hint address is almost never returned. This patch raises
>> > the mappable area up to and including the hint address. This allows mmap
>> > to often return the hint address, which allows a performance improvement
>> > over searching for a valid address as well as making the behavior more
>> > similar to other architectures.
>> 
>> This means that if an application or library opts in to Sv48 support by
>> passing a nonzero hint, it will lose the benefits of ASLR.
>
> sv48 is default. However your statement stands for opting into sv57.
> If they always pass the same hint address, only the first address will
> be deterministic though, correct?

I think this is correct.

>> 
>> Allowing applications to opt in to a VA space smaller than the
>> architectural minimum seems like an independently useful feature.
>> Is there a reason to only add it for riscv64?
>> 
>
> If there is interest, it can be added to other architectures as well.

I meant as opposed to riscv32.

-s

> - Charlie
>
>> -s
>> 
>> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
>> > ---
>> > Charlie Jenkins (3):
>> >       riscv: mm: Use hint address in mmap if available
>> >       selftests: riscv: Generalize mm selftests
>> >       docs: riscv: Define behavior of mmap
>> >
>> >  Documentation/arch/riscv/vm-layout.rst           | 16 ++--
>> >  arch/riscv/include/asm/processor.h               | 21 ++----
>> >  tools/testing/selftests/riscv/mm/mmap_bottomup.c | 20 +----
>> >  tools/testing/selftests/riscv/mm/mmap_default.c  | 20 +----
>> >  tools/testing/selftests/riscv/mm/mmap_test.h     | 93 +++++++++++++-----------
>> >  5 files changed, 66 insertions(+), 104 deletions(-)
>> > ---
>> > base-commit: 556e2d17cae620d549c5474b1ece053430cd50bc
>> > change-id: 20240119-use_mmap_hint_address-f9f4b1b6f5f1
>> > -- 
>> > - Charlie
>> >
>> >
>> > _______________________________________________
>> > linux-riscv mailing list
>> > linux-riscv@lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 0/3] riscv: mm: Use hint address in mmap if available

[Charlie Jenkins]

On Tue, Jan 30, 2024 at 05:04:55PM -0500, Stefan O'Rear wrote:
> On Mon, Jan 29, 2024, at 9:13 PM, Charlie Jenkins wrote:
> > On Mon, Jan 29, 2024 at 09:04:50PM -0500, Stefan O'Rear wrote:
> >> On Mon, Jan 29, 2024, at 7:36 PM, Charlie Jenkins wrote:
> >> > On riscv, mmap currently returns an address from the largest address
> >> > space that can fit entirely inside of the hint address. This makes it
> >> > such that the hint address is almost never returned. This patch raises
> >> > the mappable area up to and including the hint address. This allows mmap
> >> > to often return the hint address, which allows a performance improvement
> >> > over searching for a valid address as well as making the behavior more
> >> > similar to other architectures.
> >> 
> >> This means that if an application or library opts in to Sv48 support by
> >> passing a nonzero hint, it will lose the benefits of ASLR.
> >
> > sv48 is default. However your statement stands for opting into sv57.
> > If they always pass the same hint address, only the first address will
> > be deterministic though, correct?
> 
> I think this is correct.
> 
> >> 
> >> Allowing applications to opt in to a VA space smaller than the
> >> architectural minimum seems like an independently useful feature.
> >> Is there a reason to only add it for riscv64?
> >> 
> >
> > If there is interest, it can be added to other architectures as well.
> 
> I meant as opposed to riscv32.

That is a good point. I can include rv32 as well.

- Charlie

> 
> -s
> 
> > - Charlie
> >
> >> -s
> >> 
> >> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> >> > ---
> >> > Charlie Jenkins (3):
> >> >       riscv: mm: Use hint address in mmap if available
> >> >       selftests: riscv: Generalize mm selftests
> >> >       docs: riscv: Define behavior of mmap
> >> >
> >> >  Documentation/arch/riscv/vm-layout.rst           | 16 ++--
> >> >  arch/riscv/include/asm/processor.h               | 21 ++----
> >> >  tools/testing/selftests/riscv/mm/mmap_bottomup.c | 20 +----
> >> >  tools/testing/selftests/riscv/mm/mmap_default.c  | 20 +----
> >> >  tools/testing/selftests/riscv/mm/mmap_test.h     | 93 +++++++++++++-----------
> >> >  5 files changed, 66 insertions(+), 104 deletions(-)
> >> > ---
> >> > base-commit: 556e2d17cae620d549c5474b1ece053430cd50bc
> >> > change-id: 20240119-use_mmap_hint_address-f9f4b1b6f5f1
> >> > -- 
> >> > - Charlie
> >> >
> >> >
> >> > _______________________________________________
> >> > linux-riscv mailing list
> >> > linux-riscv@lists.infradead.org
> >> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv