[PATCH v2 0/3] OpenRISC fixes for 5.9

[PATCH v2 0/3] OpenRISC fixes for 5.9

[Stafford Horne]

Changes since v1:

 - Now a series, v1 was only the "Reserve memblock for initrd" patch
 - Added fixes for compiler issues pointed out by the kbuild robot

This is a few fixes found during testing 5.9.

Stafford Horne (3):
  openrisc: Reserve memblock for initrd
  openrisc: Fix cache API compile issue when not inlining
  openrisc: Fix issue with get_user for 64-bit values

 arch/openrisc/include/asm/uaccess.h | 16 ++++++++--------
 arch/openrisc/kernel/setup.c        | 10 ++++++++++
 arch/openrisc/mm/cache.c            |  2 +-
 3 files changed, 19 insertions(+), 9 deletions(-)

-- 
2.26.2

[PATCH v2 1/3] openrisc: Reserve memblock for initrd

[Stafford Horne]

Recently OpenRISC added support for external initrd images, but I found
some instability when using larger buildroot initrd images. It turned
out that I forgot to reserve the memblock space for the initrd image.

This patch fixes the instability issue by reserving memblock space.

Fixes: ff6c923dbec3 ("openrisc: Add support for external initrd images")
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 arch/openrisc/kernel/setup.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c
index b18e775f8be3..13c87f1f872b 100644
--- a/arch/openrisc/kernel/setup.c
+++ b/arch/openrisc/kernel/setup.c
@@ -80,6 +80,16 @@ static void __init setup_memory(void)
 	 */
 	memblock_reserve(__pa(_stext), _end - _stext);
 
+#ifdef CONFIG_BLK_DEV_INITRD
+	/* Then reserve the initrd, if any */
+	if (initrd_start && (initrd_end > initrd_start)) {
+		unsigned long aligned_start = ALIGN_DOWN(initrd_start, PAGE_SIZE);
+		unsigned long aligned_end = ALIGN(initrd_end, PAGE_SIZE);
+
+		memblock_reserve(__pa(aligned_start), aligned_end - aligned_start);
+	}
+#endif /* CONFIG_BLK_DEV_INITRD */
+
 	early_init_fdt_reserve_self();
 	early_init_fdt_scan_reserved_mem();
 
-- 
2.26.2

Re: [PATCH v2 1/3] openrisc: Reserve memblock for initrd

[Stafford Horne]

On Sat, Sep 05, 2020 at 10:19:33PM +0900, Stafford Horne wrote:
> Recently OpenRISC added support for external initrd images, but I found
> some instability when using larger buildroot initrd images. It turned
> out that I forgot to reserve the memblock space for the initrd image.
> 
> This patch fixes the instability issue by reserving memblock space.
> 
> Fixes: ff6c923dbec3 ("openrisc: Add support for external initrd images")
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
Forgot to mention:

Changes since v1:
  - Updated to use separate variables as suggested by Mike.

>  arch/openrisc/kernel/setup.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c
> index b18e775f8be3..13c87f1f872b 100644
> --- a/arch/openrisc/kernel/setup.c
> +++ b/arch/openrisc/kernel/setup.c
> @@ -80,6 +80,16 @@ static void __init setup_memory(void)
>  	 */
>  	memblock_reserve(__pa(_stext), _end - _stext);
>  
> +#ifdef CONFIG_BLK_DEV_INITRD
> +	/* Then reserve the initrd, if any */
> +	if (initrd_start && (initrd_end > initrd_start)) {
> +		unsigned long aligned_start = ALIGN_DOWN(initrd_start, PAGE_SIZE);
> +		unsigned long aligned_end = ALIGN(initrd_end, PAGE_SIZE);
> +
> +		memblock_reserve(__pa(aligned_start), aligned_end - aligned_start);
> +	}
> +#endif /* CONFIG_BLK_DEV_INITRD */
> +
>  	early_init_fdt_reserve_self();
>  	early_init_fdt_scan_reserved_mem();
>  
> -- 
> 2.26.2
> 

Re: [PATCH v2 1/3] openrisc: Reserve memblock for initrd

[Mike Rapoport]

On Sat, Sep 05, 2020 at 10:19:33PM +0900, Stafford Horne wrote:
> Recently OpenRISC added support for external initrd images, but I found
> some instability when using larger buildroot initrd images. It turned
> out that I forgot to reserve the memblock space for the initrd image.
> 
> This patch fixes the instability issue by reserving memblock space.
> 
> Fixes: ff6c923dbec3 ("openrisc: Add support for external initrd images")
> Signed-off-by: Stafford Horne <shorne@gmail.com>

Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>

> ---
>  arch/openrisc/kernel/setup.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c
> index b18e775f8be3..13c87f1f872b 100644
> --- a/arch/openrisc/kernel/setup.c
> +++ b/arch/openrisc/kernel/setup.c
> @@ -80,6 +80,16 @@ static void __init setup_memory(void)
>  	 */
>  	memblock_reserve(__pa(_stext), _end - _stext);
>  
> +#ifdef CONFIG_BLK_DEV_INITRD
> +	/* Then reserve the initrd, if any */
> +	if (initrd_start && (initrd_end > initrd_start)) {
> +		unsigned long aligned_start = ALIGN_DOWN(initrd_start, PAGE_SIZE);
> +		unsigned long aligned_end = ALIGN(initrd_end, PAGE_SIZE);
> +
> +		memblock_reserve(__pa(aligned_start), aligned_end - aligned_start);
> +	}
> +#endif /* CONFIG_BLK_DEV_INITRD */
> +
>  	early_init_fdt_reserve_self();
>  	early_init_fdt_scan_reserved_mem();
>  
> -- 
> 2.26.2
> 

-- 
Sincerely yours,
Mike.

[PATCH v2 2/3] openrisc: Fix cache API compile issue when not inlining

[Stafford Horne]

I found this when compiling a kbuild random config with GCC 11.  The
config enables CONFIG_DEBUG_SECTION_MISMATCH, which sets CFLAGS
-fno-inline-functions-called-once. This causes the call to cache_loop in
cache.c to not be inlined causing the below compile error.

    In file included from arch/openrisc/mm/cache.c:13:
    arch/openrisc/mm/cache.c: In function 'cache_loop':
    ./arch/openrisc/include/asm/spr.h:16:27: warning: 'asm' operand 0 probably does not match constraints
       16 | #define mtspr(_spr, _val) __asm__ __volatile__ (  \
	  |                           ^~~~~~~
    arch/openrisc/mm/cache.c:25:3: note: in expansion of macro 'mtspr'
       25 |   mtspr(reg, line);
	  |   ^~~~~
    ./arch/openrisc/include/asm/spr.h:16:27: error: impossible constraint in 'asm'
       16 | #define mtspr(_spr, _val) __asm__ __volatile__ (  \
	  |                           ^~~~~~~
    arch/openrisc/mm/cache.c:25:3: note: in expansion of macro 'mtspr'
       25 |   mtspr(reg, line);
	  |   ^~~~~
    make[1]: *** [scripts/Makefile.build:283: arch/openrisc/mm/cache.o] Error 1

The asm constraint "K" requires a immediate constant argument to mtspr,
however because of no inlining a register argument is passed causing a
failure.  Fix this by using __always_inline.

Link: https://lore.kernel.org/lkml/202008200453.ohnhqkjQ%25lkp@intel.com/
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 arch/openrisc/mm/cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/openrisc/mm/cache.c b/arch/openrisc/mm/cache.c
index 08f56af387ac..534a52ec5e66 100644
--- a/arch/openrisc/mm/cache.c
+++ b/arch/openrisc/mm/cache.c
@@ -16,7 +16,7 @@
 #include <asm/cacheflush.h>
 #include <asm/tlbflush.h>
 
-static void cache_loop(struct page *page, const unsigned int reg)
+static __always_inline void cache_loop(struct page *page, const unsigned int reg)
 {
 	unsigned long paddr = page_to_pfn(page) << PAGE_SHIFT;
 	unsigned long line = paddr & ~(L1_CACHE_BYTES - 1);
-- 
2.26.2

[PATCH v2 3/3] openrisc: Fix issue with get_user for 64-bit values

[Stafford Horne]

A build failure was raised by kbuild with the following error.

    drivers/android/binder.c: Assembler messages:
    drivers/android/binder.c:3861: Error: unrecognized keyword/register name `l.lwz ?ap,4(r24)'
    drivers/android/binder.c:3866: Error: unrecognized keyword/register name `l.addi ?ap,r0,0'

The issue is with 64-bit get_user() calls on openrisc.  I traced this to
a problem where in the internally in the get_user macros there is a cast
to long __gu_val this causes GCC to think the get_user call is 32-bit.
This binder code is really long and GCC allocates register r30, which
triggers the issue. The 64-bit get_user asm tries to get the 64-bit pair
register, which for r30 overflows the general register names and returns
the dummy register ?ap.

The fix is to just remove the assignment to the 32-bit temporary
variable __gu_val.

Link: https://lore.kernel.org/lkml/202008200453.ohnhqkjQ%25lkp@intel.com/
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 arch/openrisc/include/asm/uaccess.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h
index f0390211236b..4a8976dda1a5 100644
--- a/arch/openrisc/include/asm/uaccess.h
+++ b/arch/openrisc/include/asm/uaccess.h
@@ -165,19 +165,19 @@ struct __large_struct {
 
 #define __get_user_nocheck(x, ptr, size)			\
 ({								\
-	long __gu_err, __gu_val;				\
-	__get_user_size(__gu_val, (ptr), (size), __gu_err);	\
-	(x) = (__force __typeof__(*(ptr)))__gu_val;		\
+	long __gu_err;						\
+	__get_user_size((x), (ptr), (size), __gu_err);		\
 	__gu_err;						\
 })
 
 #define __get_user_check(x, ptr, size)					\
 ({									\
-	long __gu_err = -EFAULT, __gu_val = 0;				\
+	long __gu_err = -EFAULT;					\
 	const __typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
-	if (access_ok(__gu_addr, size))			\
-		__get_user_size(__gu_val, __gu_addr, (size), __gu_err);	\
-	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
+	if (access_ok(__gu_addr, size))					\
+		__get_user_size((x), __gu_addr, (size), __gu_err);	\
+	else								\
+		(x) = 0;						\
 	__gu_err;							\
 })
 
@@ -191,7 +191,7 @@ do {									\
 	case 2: __get_user_asm(x, ptr, retval, "l.lhz"); break;		\
 	case 4: __get_user_asm(x, ptr, retval, "l.lwz"); break;		\
 	case 8: __get_user_asm2(x, ptr, retval); break;			\
-	default: (x) = __get_user_bad();				\
+	default: (x) = (__typeof__(x)) __get_user_bad();		\
 	}								\
 } while (0)
 
-- 
2.26.2

Re: [PATCH v2 3/3] openrisc: Fix issue with get_user for 64-bit values

[Luc Van Oostenryck]

On Sat, Sep 05, 2020 at 10:19:35PM +0900, Stafford Horne wrote:

Hi,

The change for 64-bit get_user() looks good to me.
But I wonder, given that openrisc is big-endian, what will happen
you have the opposite situation:
	u32 *ptr;
	u64 val;
	...
	get_user(val, ptr);

Won't you end with the value in the most significant part of
the register pair?

-- Luc

Re: [PATCH v2 3/3] openrisc: Fix issue with get_user for 64-bit values

[Stafford Horne]

On Sat, Sep 05, 2020 at 03:57:14PM +0200, Luc Van Oostenryck wrote:
> On Sat, Sep 05, 2020 at 10:19:35PM +0900, Stafford Horne wrote:
> 
> Hi,
> 
> The change for 64-bit get_user() looks good to me.
> But I wonder, given that openrisc is big-endian, what will happen
> you have the opposite situation:
> 	u32 *ptr;
> 	u64 val;
> 	...
> 	get_user(val, ptr);
> 
> Won't you end with the value in the most significant part of
> the register pair?

Hi Luc,

The get_user function uses the size of the ptr to determine how to do the load ,
so this case would not use the 64-bit pair register logic.  I think it should be
ok, the end result would be the same as c code:

  var = *ptr;

-Stafford

Re: [OpenRISC] [PATCH v2 3/3] openrisc: Fix issue with get_user for 64-bit values

[Luc Van Oostenryck]

On Sun, Sep 06, 2020 at 06:34:08AM +0900, Stafford Horne wrote:
> On Sat, Sep 05, 2020 at 03:57:14PM +0200, Luc Van Oostenryck wrote:
> > On Sat, Sep 05, 2020 at 10:19:35PM +0900, Stafford Horne wrote:
> > 
> > Hi,
> > 
> > The change for 64-bit get_user() looks good to me.
> > But I wonder, given that openrisc is big-endian, what will happen
> > you have the opposite situation:
> > 	u32 *ptr;
> > 	u64 val;
> > 	...
> > 	get_user(val, ptr);
> > 
> > Won't you end with the value in the most significant part of
> > the register pair?
> 
> Hi Luc,
> 
> The get_user function uses the size of the ptr to determine how to do the load ,
> so this case would not use the 64-bit pair register logic.  I think it should be
> ok, the end result would be the same as c code:
> 
>   var = *ptr;

Hi,

Sorry to insist but both won't give the same result.
The problem comes from the output part of the asm: "=r" (x).

The following code:
	u32 getp(u32 *ptr)
	{
		u64 val;
		val = *ptr;
		return val;
	}
will compile to something like:
	getp:
		l.jr	r9
		l.lwz	r11, 0(r3)

The load is written to r11, which is what is returned. OK.

But the get_user() code with a u32 pointer *and* a u64 destination
is equivalent to something like:
	u32 getl(u32 *ptr)
	{
		u64 val;

		asm("l.lwz %0,0(%1)" : "=r"(val) : "r"(ptr));
		return val;
	}
and this compiles to:
	getl:
		l.lwz	r17,0(r3)
		l.jr	r9
		l.or	r11, r19, r19

The load is written to r17 but what is returned is the content of r19.
Not good.

I think that, in the get_user() code:
* if the pointer is a pointer to a 64-bit quantity, then variable
  used in as the output in the asm needs to be a 64-bit variable
* if the pointer is a pointer to a 32-bit quantity, then variable
  used in as the output in the asm needs to be a 64-bit variable
At least one way to guarantee this is to use a temporary variable
that matches the size of the pointer.

-- Luc

Re: [OpenRISC] [PATCH v2 3/3] openrisc: Fix issue with get_user for 64-bit values

[Stafford Horne]

On Sun, Sep 06, 2020 at 02:22:28AM +0200, Luc Van Oostenryck wrote:
> On Sun, Sep 06, 2020 at 06:34:08AM +0900, Stafford Horne wrote:
> > On Sat, Sep 05, 2020 at 03:57:14PM +0200, Luc Van Oostenryck wrote:
> > > On Sat, Sep 05, 2020 at 10:19:35PM +0900, Stafford Horne wrote:
> > > 
> > > Hi,
> > > 
> > > The change for 64-bit get_user() looks good to me.
> > > But I wonder, given that openrisc is big-endian, what will happen
> > > you have the opposite situation:
> > > 	u32 *ptr;
> > > 	u64 val;
> > > 	...
> > > 	get_user(val, ptr);
> > > 
> > > Won't you end with the value in the most significant part of
> > > the register pair?
> > 
> > Hi Luc,
> > 
> > The get_user function uses the size of the ptr to determine how to do the load ,
> > so this case would not use the 64-bit pair register logic.  I think it should be
> > ok, the end result would be the same as c code:
> > 
> >   var = *ptr;
> 
> Hi,
> 
> Sorry to insist but both won't give the same result.
> The problem comes from the output part of the asm: "=r" (x).
> 
> The following code:
> 	u32 getp(u32 *ptr)
> 	{
> 		u64 val;
> 		val = *ptr;
> 		return val;
> 	}
> will compile to something like:
> 	getp:
> 		l.jr	r9
> 		l.lwz	r11, 0(r3)
> 
> The load is written to r11, which is what is returned. OK.
> 
> But the get_user() code with a u32 pointer *and* a u64 destination
> is equivalent to something like:
> 	u32 getl(u32 *ptr)
> 	{
> 		u64 val;
> 
> 		asm("l.lwz %0,0(%1)" : "=r"(val) : "r"(ptr));
> 		return val;
> 	}
> and this compiles to:
> 	getl:
> 		l.lwz	r17,0(r3)
> 		l.jr	r9
> 		l.or	r11, r19, r19
> 
> The load is written to r17 but what is returned is the content of r19.
> Not good.
> 
> I think that, in the get_user() code:
> * if the pointer is a pointer to a 64-bit quantity, then variable
>   used in as the output in the asm needs to be a 64-bit variable
> * if the pointer is a pointer to a 32-bit quantity, then variable
>   used in as the output in the asm needs to be a 64-bit variable
> At least one way to guarantee this is to use a temporary variable
> that matches the size of the pointer.

Hello,

Thanks for taking the time to explain.  I see your point, it makes sense I will
fix this up.

-Stafford

Re: [PATCH v2 0/3] OpenRISC fixes for 5.9

[Stafford Horne]

On Sat, Sep 05, 2020 at 10:19:32PM +0900, Stafford Horne wrote:
> Changes since v1:
> 
>  - Now a series, v1 was only the "Reserve memblock for initrd" patch
>  - Added fixes for compiler issues pointed out by the kbuild robot
Forgot to mention:
  - Updated "Reserve memblock for initrd", as per Mike's comments

> This is a few fixes found during testing 5.9.
> 
> Stafford Horne (3):
>   openrisc: Reserve memblock for initrd
>   openrisc: Fix cache API compile issue when not inlining
>   openrisc: Fix issue with get_user for 64-bit values
> 
>  arch/openrisc/include/asm/uaccess.h | 16 ++++++++--------
>  arch/openrisc/kernel/setup.c        | 10 ++++++++++
>  arch/openrisc/mm/cache.c            |  2 +-
>  3 files changed, 19 insertions(+), 9 deletions(-)
> 
> -- 
> 2.26.2
>