[PATCH] powerpc: Improve comment explaining why we modify VRSAVE

[PATCH] powerpc: Improve comment explaining why we modify VRSAVE

[Anton Blanchard]

The comment explaining why we modify VRSAVE is misleading, glibc
does rely on the behaviour. Update the comment.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

diff --git a/arch/powerpc/kernel/vector.S b/arch/powerpc/kernel/vector.S
index 1c2e7a3..3907fcf 100644
--- a/arch/powerpc/kernel/vector.S
+++ b/arch/powerpc/kernel/vector.S
@@ -70,10 +70,11 @@ _GLOBAL(load_up_altivec)
 	MTMSRD(r5)			/* enable use of AltiVec now */
 	isync
 
-	/* Hack: if we get an altivec unavailable trap with VRSAVE
-	 * set to all zeros, we assume this is a broken application
-	 * that fails to set it properly, and thus we switch it to
-	 * all 1's
+	/*
+	 * While userspace in general ignores VRSAVE, glibc uses it as a
+	 * boolean to optimise userspace context save/restore. Whenever we
+	 * take an altivec unavailable exception we must set VRSAVE to
+	 * something non zero. Set it to all 1s.
 	 */
 	mfspr	r4,SPRN_VRSAVE
 	cmpwi	0,r4,0

Re: [PATCH] powerpc: Improve comment explaining why we modify VRSAVE

[Cyril Bur]

On Fri, 20 May 2016 04:41:34 +1000
Anton Blanchard via Linuxppc-dev <linuxppc-dev@lists.ozlabs.org> wrote:

> The comment explaining why we modify VRSAVE is misleading, glibc
> does rely on the behaviour. Update the comment.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>
> ---
> 
> diff --git a/arch/powerpc/kernel/vector.S b/arch/powerpc/kernel/vector.S
> index 1c2e7a3..3907fcf 100644
> --- a/arch/powerpc/kernel/vector.S
> +++ b/arch/powerpc/kernel/vector.S
> @@ -70,10 +70,11 @@ _GLOBAL(load_up_altivec)
>  	MTMSRD(r5)			/* enable use of AltiVec now */
>  	isync
>  
> -	/* Hack: if we get an altivec unavailable trap with VRSAVE
> -	 * set to all zeros, we assume this is a broken application
> -	 * that fails to set it properly, and thus we switch it to
> -	 * all 1's
> +	/*
> +	 * While userspace in general ignores VRSAVE, glibc uses it as a
> +	 * boolean to optimise userspace context save/restore. Whenever we
> +	 * take an altivec unavailable exception we must set VRSAVE to
> +	 * something non zero. Set it to all 1s.

I always assumed this was a little more complicated and that it revolved
around trying to adhere to the programming note in 5.3.3 of the ISA:

"The VRSAVE register can be used to indicate which VRs are currently being used
by a program. If this is done, the operating system could save only those VRs
when an 'interrupt' occurs, and could restore only those VRs when resuming the
interrupted program."

In this scenario we don't trust userspace to do anything sane (IE the old
comment saying 'broken application' because zero here can't be correct) and
we're loading all 1's because we're now switching all (32) altivec registers.
Obviously we're still going to switch all 32 even if userspace adheres to the
note, doing so doesn't violate the note.

Admittedly the note really does imply that it's userspaces responsibility to
set it, however, if we're going to change it to something other than zero, it
might be worth noting that all 1's is 'best' in case anyone does ever follow
that note.

Perhaps ending with "... Set it to all 1s as a best effort to adhere to the
programming note in '5.3.3 VR Save Register' of the ISA"

Having said all that, a reminder that glibc does look at it is very welcome
here!

Reviewed-by: Cyril Bur <cyrilbur@gmail.com>

>  	 */
>  	mfspr	r4,SPRN_VRSAVE
>  	cmpwi	0,r4,0
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: powerpc: Improve comment explaining why we modify VRSAVE

[Michael Ellerman]

On Thu, 2016-19-05 at 18:41:34 UTC, Unknown sender due to SPF wrote:
> The comment explaining why we modify VRSAVE is misleading, glibc
> does rely on the behaviour. Update the comment.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>
> Reviewed-by: Cyril Bur <cyrilbur@gmail.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/dd57023747e33572b31867f890

cheers