Re: [PATCH v2 3/3] powerpc: Fix update form addressing in inline assembly

[Segher Boessenkool]

Hi!

On Tue, Oct 20, 2020 at 07:40:09AM +0000, Christophe Leroy wrote:
> In several places, inline assembly uses the "%Un" modifier
> to enable the use of instruction with update form addressing,
> but the associated "<>" constraint is missing.
> 
> As mentioned in previous patch, this fails with gcc 4.9, so
> "<>" can't be used directly.
> 
> Use UPD_CONSTR macro everywhere %Un modifier is used.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>

Oh well, it will be easy enough to remove this wart later, so

Reviewed-by: Segher Boessenkool <segher@xxxxxxxxxxxxxxxxxxx>

> --- a/arch/powerpc/include/asm/book3s/32/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
> @@ -525,7 +525,7 @@ static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
>  		stw%U0%X0 %2,%0\n\
>  		eieio\n\
>  		stw%U1%X1 %L2,%1"
> -	: "=m" (*ptep), "=m" (*((unsigned char *)ptep+4))
> +	: "=m"UPD_CONSTR (*ptep), "=m"UPD_CONSTR (*((unsigned char *)ptep+4))
>  	: "r" (pte) : "memory");

Here it would pre-increment ptep+4.  That can never be something useful
afaics?  The order the two operands are (either or not) pre-modified in
the asm is not specified (GCC does not parse the asm template, by
design), so I fail to see how this could ever work.

> --- a/arch/powerpc/include/asm/nohash/pgtable.h
> +++ b/arch/powerpc/include/asm/nohash/pgtable.h
> @@ -200,7 +200,7 @@ static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
>  			stw%U0%X0 %2,%0\n\
>  			eieio\n\
>  			stw%U1%X1 %L2,%1"
> -		: "=m" (*ptep), "=m" (*((unsigned char *)ptep+4))
> +		: "=m"UPD_CONSTR (*ptep), "=m"UPD_CONSTR (*((unsigned char *)ptep+4))
>  		: "r" (pte) : "memory");

Same here.

The rest looks fine.


Segher