BOOKE KVM calling load_up_fpu from C?

BOOKE KVM calling load_up_fpu from C?

[Michael Neuling]

Scott,

I was looking at changing how load_up_fpu works and I found this in
arch/powerpc/kvm/booke.h:

static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu)
{
#ifdef CONFIG_PPC_FPU
	if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) {
		load_up_fpu();
		current->thread.regs->msr |= MSR_FP;
	}
#endif
}

I'm wondering how this is suppose to work since load_up_fpu is suppose
to have MSR in R12?

Mikey

RE: BOOKE KVM calling load_up_fpu from C?

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+bharat.bhushan=3Dfreescale.com@lists.ozlabs.org] On Behalf Of Mic=
hael
> Neuling
> Sent: Tuesday, February 12, 2013 8:59 AM
> To: Wood Scott-B07421
> Cc: linuxppc-dev@lists.ozlabs.org
> Subject: BOOKE KVM calling load_up_fpu from C?
>=20
> Scott,
>=20
> I was looking at changing how load_up_fpu works and I found this in
> arch/powerpc/kvm/booke.h:
>=20
> static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu) { #ifdef
> CONFIG_PPC_FPU
> 	if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) {
> 		load_up_fpu();
> 		current->thread.regs->msr |=3D MSR_FP;
> 	}
> #endif
> }
>=20
> I'm wondering how this is suppose to work since load_up_fpu is suppose to=
 have
> MSR in R12?

Is not the load_up_fpu() does mfmsr:

_GLOBAL(load_up_fpu)
        mfmsr   r5
        ori     r5,r5,MSR_FP
#ifdef CONFIG_VSX
BEGIN_FTR_SECTION
        oris    r5,r5,MSR_VSX@h
END_FTR_SECTION_IFSET(CPU_FTR_VSX)
#endif
        SYNC
        MTMSRD(r5)                      /* enable use of fpu now */
        isync
<snip>

-Bharat

>=20
> Mikey
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: BOOKE KVM calling load_up_fpu from C?

[Michael Neuling]

Bhushan Bharat-R65777 <R65777@freescale.com> wrote:

> 
> 
> > -----Original Message-----
> > From: Linuxppc-dev [mailto:linuxppc-dev-
> > bounces+bharat.bhushan=freescale.com@lists.ozlabs.org] On Behalf Of Michael
> > Neuling
> > Sent: Tuesday, February 12, 2013 8:59 AM
> > To: Wood Scott-B07421
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Subject: BOOKE KVM calling load_up_fpu from C?
> > 
> > Scott,
> > 
> > I was looking at changing how load_up_fpu works and I found this in
> > arch/powerpc/kvm/booke.h:
> > 
> > static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu) { #ifdef
> > CONFIG_PPC_FPU
> > 	if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) {
> > 		load_up_fpu();
> > 		current->thread.regs->msr |= MSR_FP;
> > 	}
> > #endif
> > }
> > 
> > I'm wondering how this is suppose to work since load_up_fpu is suppose to have
> > MSR in R12?
> 
> Is not the load_up_fpu() does mfmsr:
> 
> _GLOBAL(load_up_fpu)
>         mfmsr   r5
>         ori     r5,r5,MSR_FP
> #ifdef CONFIG_VSX
> BEGIN_FTR_SECTION
>         oris    r5,r5,MSR_VSX@h
> END_FTR_SECTION_IFSET(CPU_FTR_VSX)
> #endif
>         SYNC
>         MTMSRD(r5)                      /* enable use of fpu now */
>         isync
> <snip>

Look further down...

#ifdef CONFIG_PPC32
	mfspr	r5,SPRN_SPRG_THREAD		/* current task's THREAD (phys) */
	lwz	r4,THREAD_FPEXC_MODE(r5)
	ori	r9,r9,MSR_FP		/* enable FP for current */
	or	r9,r9,r4
#else
	ld	r4,PACACURRENT(r13)
	addi	r5,r4,THREAD		/* Get THREAD */
	lwz	r4,THREAD_FPEXC_MODE(r5)
	ori	r12,r12,MSR_FP
	or	r12,r12,r4
	std	r12,_MSR(r1)
#endif

R12 is loaded with SRR1 in the exception prolog before load_up_fpu is
called.  It's the MSR of the user process, not the current MSR.

Mikey

RE: BOOKE KVM calling load_up_fpu from C?

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Michael Neuling [mailto:mikey@neuling.org]
> Sent: Tuesday, February 12, 2013 9:16 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> Subject: Re: BOOKE KVM calling load_up_fpu from C?
>=20
> Bhushan Bharat-R65777 <R65777@freescale.com> wrote:
>=20
> >
> >
> > > -----Original Message-----
> > > From: Linuxppc-dev [mailto:linuxppc-dev-
> > > bounces+bharat.bhushan=3Dfreescale.com@lists.ozlabs.org] On Behalf Of
> > > bounces+Michael
> > > Neuling
> > > Sent: Tuesday, February 12, 2013 8:59 AM
> > > To: Wood Scott-B07421
> > > Cc: linuxppc-dev@lists.ozlabs.org
> > > Subject: BOOKE KVM calling load_up_fpu from C?
> > >
> > > Scott,
> > >
> > > I was looking at changing how load_up_fpu works and I found this in
> > > arch/powerpc/kvm/booke.h:
> > >
> > > static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu) {
> > > #ifdef CONFIG_PPC_FPU
> > > 	if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) {
> > > 		load_up_fpu();
> > > 		current->thread.regs->msr |=3D MSR_FP;
> > > 	}
> > > #endif
> > > }
> > >
> > > I'm wondering how this is suppose to work since load_up_fpu is
> > > suppose to have MSR in R12?
> >
> > Is not the load_up_fpu() does mfmsr:
> >
> > _GLOBAL(load_up_fpu)
> >         mfmsr   r5
> >         ori     r5,r5,MSR_FP
> > #ifdef CONFIG_VSX
> > BEGIN_FTR_SECTION
> >         oris    r5,r5,MSR_VSX@h
> > END_FTR_SECTION_IFSET(CPU_FTR_VSX)
> > #endif
> >         SYNC
> >         MTMSRD(r5)                      /* enable use of fpu now */
> >         isync
> > <snip>
>=20
> Look further down...
>=20
> #ifdef CONFIG_PPC32
> 	mfspr	r5,SPRN_SPRG_THREAD		/* current task's THREAD (phys) */
> 	lwz	r4,THREAD_FPEXC_MODE(r5)
> 	ori	r9,r9,MSR_FP		/* enable FP for current */
> 	or	r9,r9,r4
> #else
> 	ld	r4,PACACURRENT(r13)
> 	addi	r5,r4,THREAD		/* Get THREAD */
> 	lwz	r4,THREAD_FPEXC_MODE(r5)
> 	ori	r12,r12,MSR_FP
> 	or	r12,r12,r4
> 	std	r12,_MSR(r1)
> #endif
>=20
> R12 is loaded with SRR1 in the exception prolog before load_up_fpu is cal=
led.

Yes it is SRR1 not MSR.
Also on 32bit it looks like that R9 is assumed to have SRR1.

-Bharat

> It's the MSR of the user process, not the current MSR.
>=20
> Mikey

Re: BOOKE KVM calling load_up_fpu from C?

[Michael Neuling]

Bhushan Bharat-R65777 <R65777@freescale.com> wrote:

> 
> 
> > -----Original Message-----
> > From: Michael Neuling [mailto:mikey@neuling.org]
> > Sent: Tuesday, February 12, 2013 9:16 AM
> > To: Bhushan Bharat-R65777
> > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> > Subject: Re: BOOKE KVM calling load_up_fpu from C?
> > 
> > Bhushan Bharat-R65777 <R65777@freescale.com> wrote:
> > 
> > >
> > >
> > > > -----Original Message-----
> > > > From: Linuxppc-dev [mailto:linuxppc-dev-
> > > > bounces+bharat.bhushan=freescale.com@lists.ozlabs.org] On Behalf Of
> > > > bounces+Michael
> > > > Neuling
> > > > Sent: Tuesday, February 12, 2013 8:59 AM
> > > > To: Wood Scott-B07421
> > > > Cc: linuxppc-dev@lists.ozlabs.org
> > > > Subject: BOOKE KVM calling load_up_fpu from C?
> > > >
> > > > Scott,
> > > >
> > > > I was looking at changing how load_up_fpu works and I found this in
> > > > arch/powerpc/kvm/booke.h:
> > > >
> > > > static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu) {
> > > > #ifdef CONFIG_PPC_FPU
> > > > 	if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) {
> > > > 		load_up_fpu();
> > > > 		current->thread.regs->msr |= MSR_FP;
> > > > 	}
> > > > #endif
> > > > }
> > > >
> > > > I'm wondering how this is suppose to work since load_up_fpu is
> > > > suppose to have MSR in R12?
> > >
> > > Is not the load_up_fpu() does mfmsr:
> > >
> > > _GLOBAL(load_up_fpu)
> > >         mfmsr   r5
> > >         ori     r5,r5,MSR_FP
> > > #ifdef CONFIG_VSX
> > > BEGIN_FTR_SECTION
> > >         oris    r5,r5,MSR_VSX@h
> > > END_FTR_SECTION_IFSET(CPU_FTR_VSX)
> > > #endif
> > >         SYNC
> > >         MTMSRD(r5)                      /* enable use of fpu now */
> > >         isync
> > > <snip>
> > 
> > Look further down...
> > 
> > #ifdef CONFIG_PPC32
> > 	mfspr	r5,SPRN_SPRG_THREAD		/* current task's THREAD (phys) */
> > 	lwz	r4,THREAD_FPEXC_MODE(r5)
> > 	ori	r9,r9,MSR_FP		/* enable FP for current */
> > 	or	r9,r9,r4
> > #else
> > 	ld	r4,PACACURRENT(r13)
> > 	addi	r5,r4,THREAD		/* Get THREAD */
> > 	lwz	r4,THREAD_FPEXC_MODE(r5)
> > 	ori	r12,r12,MSR_FP
> > 	or	r12,r12,r4
> > 	std	r12,_MSR(r1)
> > #endif
> > 
> > R12 is loaded with SRR1 in the exception prolog before load_up_fpu is called.
> 
> Yes it is SRR1 not MSR.

Yes, SRR1 == the MSR of the user process, not the current MSR.

> Also on 32bit it looks like that R9 is assumed to have SRR1.

Yep that too.  

So any idea how it's suppose to work or is it broken?

Mikey

RE: BOOKE KVM calling load_up_fpu from C?

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Michael Neuling [mailto:mikey@neuling.org]
> Sent: Tuesday, February 12, 2013 9:46 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> Subject: Re: BOOKE KVM calling load_up_fpu from C?
>=20
> Bhushan Bharat-R65777 <R65777@freescale.com> wrote:
>=20
> >
> >
> > > -----Original Message-----
> > > From: Michael Neuling [mailto:mikey@neuling.org]
> > > Sent: Tuesday, February 12, 2013 9:16 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> > > Subject: Re: BOOKE KVM calling load_up_fpu from C?
> > >
> > > Bhushan Bharat-R65777 <R65777@freescale.com> wrote:
> > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Linuxppc-dev [mailto:linuxppc-dev-
> > > > > bounces+bharat.bhushan=3Dfreescale.com@lists.ozlabs.org] On Behal=
f
> > > > > bounces+Of Michael
> > > > > Neuling
> > > > > Sent: Tuesday, February 12, 2013 8:59 AM
> > > > > To: Wood Scott-B07421
> > > > > Cc: linuxppc-dev@lists.ozlabs.org
> > > > > Subject: BOOKE KVM calling load_up_fpu from C?
> > > > >
> > > > > Scott,
> > > > >
> > > > > I was looking at changing how load_up_fpu works and I found this
> > > > > in
> > > > > arch/powerpc/kvm/booke.h:
> > > > >
> > > > > static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu) {
> > > > > #ifdef CONFIG_PPC_FPU
> > > > > 	if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) {
> > > > > 		load_up_fpu();
> > > > > 		current->thread.regs->msr |=3D MSR_FP;
> > > > > 	}
> > > > > #endif
> > > > > }
> > > > >
> > > > > I'm wondering how this is suppose to work since load_up_fpu is
> > > > > suppose to have MSR in R12?
> > > >
> > > > Is not the load_up_fpu() does mfmsr:
> > > >
> > > > _GLOBAL(load_up_fpu)
> > > >         mfmsr   r5
> > > >         ori     r5,r5,MSR_FP
> > > > #ifdef CONFIG_VSX
> > > > BEGIN_FTR_SECTION
> > > >         oris    r5,r5,MSR_VSX@h
> > > > END_FTR_SECTION_IFSET(CPU_FTR_VSX)
> > > > #endif
> > > >         SYNC
> > > >         MTMSRD(r5)                      /* enable use of fpu now */
> > > >         isync
> > > > <snip>
> > >
> > > Look further down...
> > >
> > > #ifdef CONFIG_PPC32
> > > 	mfspr	r5,SPRN_SPRG_THREAD		/* current task's THREAD (phys) */
> > > 	lwz	r4,THREAD_FPEXC_MODE(r5)
> > > 	ori	r9,r9,MSR_FP		/* enable FP for current */
> > > 	or	r9,r9,r4
> > > #else
> > > 	ld	r4,PACACURRENT(r13)
> > > 	addi	r5,r4,THREAD		/* Get THREAD */
> > > 	lwz	r4,THREAD_FPEXC_MODE(r5)
> > > 	ori	r12,r12,MSR_FP
> > > 	or	r12,r12,r4
> > > 	std	r12,_MSR(r1)
> > > #endif
> > >
> > > R12 is loaded with SRR1 in the exception prolog before load_up_fpu is
> called.
> >
> > Yes it is SRR1 not MSR.
>=20
> Yes, SRR1 =3D=3D the MSR of the user process, not the current MSR.
>=20
> > Also on 32bit it looks like that R9 is assumed to have SRR1.
>=20
> Yep that too.
>=20
> So any idea how it's suppose to work or is it broken?

To me this looks wrong. And this seems to works because the thread->reg->ms=
r is not actually used to write SRR1 (and eventually the thread MSR) when d=
oing rfi to enter guest. Infact Guest(shadow_msr) MSR is used as SRR1 and w=
hich will have proper MSR (including FP set).

But Yes, Scott is right person to comment, So let us wait for him comment.

Thanks
-Bharat

Re: BOOKE KVM calling load_up_fpu from C?

[Scott Wood]

On 02/12/2013 03:01:07 AM, Bhushan Bharat-R65777 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Michael Neuling [mailto:mikey@neuling.org]
> > Sent: Tuesday, February 12, 2013 9:46 AM
> > To: Bhushan Bharat-R65777
> > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> > Subject: Re: BOOKE KVM calling load_up_fpu from C?
> >
> > Bhushan Bharat-R65777 <R65777@freescale.com> wrote:
> >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Michael Neuling [mailto:mikey@neuling.org]
> > > > Sent: Tuesday, February 12, 2013 9:16 AM
> > > > To: Bhushan Bharat-R65777
> > > > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> > > > Subject: Re: BOOKE KVM calling load_up_fpu from C?
> > > >
> > > > Look further down...
> > > >
> > > > #ifdef CONFIG_PPC32
> > > > 	mfspr	r5,SPRN_SPRG_THREAD		/* current =20
> task's THREAD (phys) */
> > > > 	lwz	r4,THREAD_FPEXC_MODE(r5)
> > > > 	ori	r9,r9,MSR_FP		/* enable FP for =20
> current */
> > > > 	or	r9,r9,r4
> > > > #else
> > > > 	ld	r4,PACACURRENT(r13)
> > > > 	addi	r5,r4,THREAD		/* Get THREAD */
> > > > 	lwz	r4,THREAD_FPEXC_MODE(r5)
> > > > 	ori	r12,r12,MSR_FP
> > > > 	or	r12,r12,r4
> > > > 	std	r12,_MSR(r1)
> > > > #endif
> > > >
> > > > R12 is loaded with SRR1 in the exception prolog before =20
> load_up_fpu is
> > called.
> > >
> > > Yes it is SRR1 not MSR.
> >
> > Yes, SRR1 =3D=3D the MSR of the user process, not the current MSR.
> >
> > > Also on 32bit it looks like that R9 is assumed to have SRR1.
> >
> > Yep that too.
> >
> > So any idea how it's suppose to work or is it broken?
>=20
> To me this looks wrong. And this seems to works because the =20
> thread->reg->msr is not actually used to write SRR1 (and eventually =20
> the thread MSR) when doing rfi to enter guest. Infact =20
> Guest(shadow_msr) MSR is used as SRR1 and which will have proper MSR =20
> (including FP set).
>=20
> But Yes, Scott is right person to comment, So let us wait for him =20
> comment.

I don't think it's actually a problem on 32-bit, since r9 is modified =20
but never actually used for anything.  On 64-bit, though, there's a =20
store to the caller's stack frame (yuck) which the kvm/booke.h caller =20
is not prepared for.  Indeed, book3s's kvmppc_load_up_fpu creates an =20
interrupt-like stack frame, but does not load r9 or r12.

It would be really nice if assumptions like these were put in a code =20
comment above load_up_fpu...  and if we didn't have so many random =20
differences between 32-bit and 64-bit. :-P

-Scott=

Re: BOOKE KVM calling load_up_fpu from C?

[Michael Neuling]

Scott Wood <scottwood@freescale.com> wrote:

> On 02/12/2013 03:01:07 AM, Bhushan Bharat-R65777 wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Michael Neuling [mailto:mikey@neuling.org]
> > > Sent: Tuesday, February 12, 2013 9:46 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> > > Subject: Re: BOOKE KVM calling load_up_fpu from C?
> > >
> > > Bhushan Bharat-R65777 <R65777@freescale.com> wrote:
> > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Michael Neuling [mailto:mikey@neuling.org]
> > > > > Sent: Tuesday, February 12, 2013 9:16 AM
> > > > > To: Bhushan Bharat-R65777
> > > > > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> > > > > Subject: Re: BOOKE KVM calling load_up_fpu from C?
> > > > >
> > > > > Look further down...
> > > > >
> > > > > #ifdef CONFIG_PPC32
> > > > > 	mfspr	r5,SPRN_SPRG_THREAD		/* current  
> > task's THREAD (phys) */
> > > > > 	lwz	r4,THREAD_FPEXC_MODE(r5)
> > > > > 	ori	r9,r9,MSR_FP		/* enable FP for  
> > current */
> > > > > 	or	r9,r9,r4
> > > > > #else
> > > > > 	ld	r4,PACACURRENT(r13)
> > > > > 	addi	r5,r4,THREAD		/* Get THREAD */
> > > > > 	lwz	r4,THREAD_FPEXC_MODE(r5)
> > > > > 	ori	r12,r12,MSR_FP
> > > > > 	or	r12,r12,r4
> > > > > 	std	r12,_MSR(r1)
> > > > > #endif
> > > > >
> > > > > R12 is loaded with SRR1 in the exception prolog before  
> > load_up_fpu is
> > > called.
> > > >
> > > > Yes it is SRR1 not MSR.
> > >
> > > Yes, SRR1 == the MSR of the user process, not the current MSR.
> > >
> > > > Also on 32bit it looks like that R9 is assumed to have SRR1.
> > >
> > > Yep that too.
> > >
> > > So any idea how it's suppose to work or is it broken?
> > 
> > To me this looks wrong. And this seems to works because the  
> > thread->reg->msr is not actually used to write SRR1 (and eventually  
> > the thread MSR) when doing rfi to enter guest. Infact  
> > Guest(shadow_msr) MSR is used as SRR1 and which will have proper MSR  
> > (including FP set).
> > 
> > But Yes, Scott is right person to comment, So let us wait for him  
> > comment.
> 
> I don't think it's actually a problem on 32-bit, since r9 is modified  
> but never actually used for anything.  On 64-bit, though, there's a  
> store to the caller's stack frame (yuck) which the kvm/booke.h caller  
> is not prepared for.  Indeed, book3s's kvmppc_load_up_fpu creates an  
> interrupt-like stack frame, but does not load r9 or r12.

Yep.

> It would be really nice if assumptions like these were put in a code  
> comment above load_up_fpu...  and if we didn't have so many random  
> differences between 32-bit and 64-bit. :-P

Yep.. I won't NACK that patch when you send it :-)

It was pretty much assumed that load_up_fpu was going to be called right
after the exception prolog.  Calling it any other way was going to be
tricky.

Mikey

RE: BOOKE KVM calling load_up_fpu from C?

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, February 13, 2013 12:03 AM
> To: Bhushan Bharat-R65777
> Cc: Michael Neuling; Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> Subject: Re: BOOKE KVM calling load_up_fpu from C?
>=20
> On 02/12/2013 03:01:07 AM, Bhushan Bharat-R65777 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Michael Neuling [mailto:mikey@neuling.org]
> > > Sent: Tuesday, February 12, 2013 9:46 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> > > Subject: Re: BOOKE KVM calling load_up_fpu from C?
> > >
> > > Bhushan Bharat-R65777 <R65777@freescale.com> wrote:
> > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Michael Neuling [mailto:mikey@neuling.org]
> > > > > Sent: Tuesday, February 12, 2013 9:16 AM
> > > > > To: Bhushan Bharat-R65777
> > > > > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> > > > > Subject: Re: BOOKE KVM calling load_up_fpu from C?
> > > > >
> > > > > Look further down...
> > > > >
> > > > > #ifdef CONFIG_PPC32
> > > > > 	mfspr	r5,SPRN_SPRG_THREAD		/* current
> > task's THREAD (phys) */
> > > > > 	lwz	r4,THREAD_FPEXC_MODE(r5)
> > > > > 	ori	r9,r9,MSR_FP		/* enable FP for
> > current */
> > > > > 	or	r9,r9,r4
> > > > > #else
> > > > > 	ld	r4,PACACURRENT(r13)
> > > > > 	addi	r5,r4,THREAD		/* Get THREAD */
> > > > > 	lwz	r4,THREAD_FPEXC_MODE(r5)
> > > > > 	ori	r12,r12,MSR_FP
> > > > > 	or	r12,r12,r4
> > > > > 	std	r12,_MSR(r1)
> > > > > #endif
> > > > >
> > > > > R12 is loaded with SRR1 in the exception prolog before
> > load_up_fpu is
> > > called.
> > > >
> > > > Yes it is SRR1 not MSR.
> > >
> > > Yes, SRR1 =3D=3D the MSR of the user process, not the current MSR.
> > >
> > > > Also on 32bit it looks like that R9 is assumed to have SRR1.
> > >
> > > Yep that too.
> > >
> > > So any idea how it's suppose to work or is it broken?
> >
> > To me this looks wrong. And this seems to works because the
> > thread->reg->msr is not actually used to write SRR1 (and eventually
> > the thread MSR) when doing rfi to enter guest. Infact
> > Guest(shadow_msr) MSR is used as SRR1 and which will have proper MSR
> > (including FP set).
> >
> > But Yes, Scott is right person to comment, So let us wait for him
> > comment.
>=20
> I don't think it's actually a problem on 32-bit, since r9 is modified but=
 never
> actually used for anything.

Is not the epilog loads srr1 in r9 and load_up_fpu() changes r9 and then r9=
 is written back in srr1 ?


>  On 64-bit, though, there's a store to the caller's
> stack frame (yuck) which the kvm/booke.h caller is not prepared for.

So if caller is using r12 then it can lead to come corruption, right ?

>  Indeed,
> book3s's kvmppc_load_up_fpu creates an interrupt-like stack frame, but do=
es not
> load r9 or r12.
>=20
> It would be really nice if assumptions like these were put in a code comm=
ent
> above load_up_fpu...  and if we didn't have so many random differences be=
tween
> 32-bit and 64-bit. :-P

:)

Thanks
-Bharat

>=20
> -Scott

Re: BOOKE KVM calling load_up_fpu from C?

[Scott Wood]

On 02/12/2013 07:18:14 PM, Bhushan Bharat-R65777 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, February 13, 2013 12:03 AM
> > To: Bhushan Bharat-R65777
> > Cc: Michael Neuling; Wood Scott-B07421; =20
> linuxppc-dev@lists.ozlabs.org
> > Subject: Re: BOOKE KVM calling load_up_fpu from C?
> >
> > On 02/12/2013 03:01:07 AM, Bhushan Bharat-R65777 wrote:
> > > To me this looks wrong. And this seems to works because the
> > > thread->reg->msr is not actually used to write SRR1 (and =20
> eventually
> > > the thread MSR) when doing rfi to enter guest. Infact
> > > Guest(shadow_msr) MSR is used as SRR1 and which will have proper =20
> MSR
> > > (including FP set).
> > >
> > > But Yes, Scott is right person to comment, So let us wait for him
> > > comment.
> >
> > I don't think it's actually a problem on 32-bit, since r9 is =20
> modified but never
> > actually used for anything.
>=20
> Is not the epilog loads srr1 in r9 and load_up_fpu() changes r9 and =20
> then r9 is written back in srr1 ?

What epilog?  We're talking about the case where it's called from C =20
code.

When it's called from an exception handler, then r9 is used, but in =20
that case it's also initialized before calling load_up_fpu, by the =20
prolog.

> >  On 64-bit, though, there's a store to the caller's
> > stack frame (yuck) which the kvm/booke.h caller is not prepared for.
>=20
> So if caller is using r12 then it can lead to come corruption, right ?

No, r12 is a volatile register in the ABI, as is r9.  The issue is that =20
the stack can be corrupted.

-Scott=

RE: BOOKE KVM calling load_up_fpu from C?

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, February 13, 2013 6:53 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; Michael Neuling; linuxppc-dev@lists.ozlabs.org
> Subject: Re: BOOKE KVM calling load_up_fpu from C?
>=20
> On 02/12/2013 07:18:14 PM, Bhushan Bharat-R65777 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Wednesday, February 13, 2013 12:03 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: Michael Neuling; Wood Scott-B07421;
> > linuxppc-dev@lists.ozlabs.org
> > > Subject: Re: BOOKE KVM calling load_up_fpu from C?
> > >
> > > On 02/12/2013 03:01:07 AM, Bhushan Bharat-R65777 wrote:
> > > > To me this looks wrong. And this seems to works because the
> > > > thread->reg->msr is not actually used to write SRR1 (and
> > eventually
> > > > the thread MSR) when doing rfi to enter guest. Infact
> > > > Guest(shadow_msr) MSR is used as SRR1 and which will have proper
> > MSR
> > > > (including FP set).
> > > >
> > > > But Yes, Scott is right person to comment, So let us wait for him
> > > > comment.
> > >
> > > I don't think it's actually a problem on 32-bit, since r9 is
> > modified but never
> > > actually used for anything.
> >
> > Is not the epilog loads srr1 in r9 and load_up_fpu() changes r9 and
> > then r9 is written back in srr1 ?
>=20
> What epilog?  We're talking about the case where it's called from C code
>=20
> When it's called from an exception handler, then r9 is used, but in that =
case
> it's also initialized before calling load_up_fpu, by the prolog.

Agree. Was just confirming the exception handler case.

>=20
> > >  On 64-bit, though, there's a store to the caller's stack frame
> > > (yuck) which the kvm/booke.h caller is not prepared for.
> >
> > So if caller is using r12 then it can lead to come corruption, right ?
>=20
> No, r12 is a volatile register in the ABI, as is r9.  The issue is that t=
he
> stack can be corrupted.

Ok, Thanks
-Bharat

>=20
> -Scott

RE: BOOKE KVM calling load_up_fpu from C?

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, February 13, 2013 6:53 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; Michael Neuling; linuxppc-dev@lists.ozlabs.org
> Subject: Re: BOOKE KVM calling load_up_fpu from C?
>=20
> On 02/12/2013 07:18:14 PM, Bhushan Bharat-R65777 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Wednesday, February 13, 2013 12:03 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: Michael Neuling; Wood Scott-B07421;
> > linuxppc-dev@lists.ozlabs.org
> > > Subject: Re: BOOKE KVM calling load_up_fpu from C?
> > >
> > > On 02/12/2013 03:01:07 AM, Bhushan Bharat-R65777 wrote:
> > > > To me this looks wrong. And this seems to works because the
> > > > thread->reg->msr is not actually used to write SRR1 (and
> > eventually
> > > > the thread MSR) when doing rfi to enter guest. Infact
> > > > Guest(shadow_msr) MSR is used as SRR1 and which will have proper
> > MSR
> > > > (including FP set).
> > > >
> > > > But Yes, Scott is right person to comment, So let us wait for him
> > > > comment.
> > >
> > > I don't think it's actually a problem on 32-bit, since r9 is
> > modified but never
> > > actually used for anything.
> >
> > Is not the epilog loads srr1 in r9 and load_up_fpu() changes r9 and
> > then r9 is written back in srr1 ?
>=20
> What epilog?  We're talking about the case where it's called from C code.
>=20
> When it's called from an exception handler, then r9 is used, but in that =
case
> it's also initialized before calling load_up_fpu, by the prolog.
>=20
> > >  On 64-bit, though, there's a store to the caller's stack frame
> > > (yuck) which the kvm/booke.h caller is not prepared for.
> >
> > So if caller is using r12 then it can lead to come corruption, right ?
>=20
> No, r12 is a volatile register in the ABI, as is r9.  The issue is that t=
he
> stack can be corrupted.

What do you mean by stack is corrupted?
My understanding is that when calling the assembly function from C function=
 then stack frame will not be pushed and assembly function uses the caller =
stack frame.  Example function1() calls function2() which calls assembly_ro=
utine()


functio1()=20

|---------------------|
|     Stack Frame 1   |
| <function1 caller   |
|  registers etc>     |
|---------------------|


Calls function 2

|----------------------|
|     Stack Frame 2    |
| <function1 registers |
|   etc >              |
|----------------------|
|     Stack Frame 1    |
| <function1 caller    |
|  registers etc>      |
|----------------------|

calls assembly_routine();
Now no stack frame push; And the assembly_routine() changes register values=
 saved in stack. So when stack will be unrolled then registers of function1=
() will get corrupted, right?

Thanks
-Bharat

Re: BOOKE KVM calling load_up_fpu from C?

[Scott Wood]

On 02/12/2013 10:17:00 PM, Bhushan Bharat-R65777 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, February 13, 2013 6:53 AM
> > To: Bhushan Bharat-R65777
> > Cc: Wood Scott-B07421; Michael Neuling; =20
> linuxppc-dev@lists.ozlabs.org
> > Subject: Re: BOOKE KVM calling load_up_fpu from C?
> >
> > On 02/12/2013 07:18:14 PM, Bhushan Bharat-R65777 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Wednesday, February 13, 2013 12:03 AM
> > > > To: Bhushan Bharat-R65777
> > > > Cc: Michael Neuling; Wood Scott-B07421;
> > > linuxppc-dev@lists.ozlabs.org
> > > > Subject: Re: BOOKE KVM calling load_up_fpu from C?
> > > >
> > > >  On 64-bit, though, there's a store to the caller's stack frame
> > > > (yuck) which the kvm/booke.h caller is not prepared for.
> > >
> > > So if caller is using r12 then it can lead to come corruption, =20
> right ?
> >
> > No, r12 is a volatile register in the ABI, as is r9.  The issue is =20
> that the
> > stack can be corrupted.
>=20
> What do you mean by stack is corrupted?

load_up_fpu() makes assumptions about the caller's stack frame that =20
aren't true when called from C code.

> My understanding is that when calling the assembly function from C =20
> function then stack frame will not be pushed and assembly function =20
> uses the caller stack frame.

Huh?  Assembly functions obey the same ABI as C functions (at least, =20
asm functions meant to be callable from C do).  If the above were true, =20
how would C code know that it's calling an asm function, and how would =20
it know how much stack to create and which portions would be clobbered?

The issue with load_up_fpu() is that it was apparently not meant to be =20
called directly from C code.

-Scott=