| Message ID | 20160520044134.62712b79@kryten (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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
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
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
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> ---