Re: [PATCH v7 0/5] powerpc/64: memcmp() optimization
Hi Naveen, On Wed, Jun 06, 2018 at 12:06:09PM +0530, Naveen N. Rao wrote: > Simon Guo wrote: > >Hi Michael, > >On Tue, Jun 05, 2018 at 12:16:22PM +1000, Michael Ellerman wrote: > >>Hi Simon, > >> > >>wei.guo.si...@gmail.com writes: > >>> From: Simon Guo > >>> > >>> There is some room to optimize memcmp() in powerpc 64 bits version for > >>> following 2 cases: > >>> (1) Even src/dst addresses are not aligned with 8 bytes at the beginning, > >>> memcmp() can align them and go with .Llong comparision mode without > >>> fallback to .Lshort comparision mode do compare buffer byte by byte. > >>> (2) VMX instructions can be used to speed up for large size comparision, > >>> currently the threshold is set for 4K bytes. Notes the VMX instructions > >>> will lead to VMX regs save/load penalty. This patch set includes a > >>> patch to add a 32 bytes pre-checking to minimize the penalty. > >>> > >>> It did the similar with glibc commit dec4a7105e (powerpc: > >>Improve memcmp > performance for POWER8). Thanks Cyril Bur's > >>information. > >>> This patch set also updates memcmp selftest case to make it compiled and > >>> incorporate large size comparison case. > >> > >>I'm seeing a few crashes with this applied, I haven't had time to look > >>into what is happening yet, sorry. > >> > > > >The bug is due to memcmp() invokes a C function enter_vmx_ops() > >who will load some PIC value based on r2. > > > >memcmp() doesn't use r2 and if the memcmp() is invoked from kernel > >itself, everything is fine. But if memcmp() is invoked from > >modules[test_user_copy], r2 will be required to be setup > >correctly. Otherwise the enter_vmx_ops() will refer to an > >incorrect/unexisting data location based on wrong r2 value. > > > >Following patch will fix this issue: > > > >diff --git a/arch/powerpc/lib/memcmp_64.S b/arch/powerpc/lib/memcmp_64.S > >index 5eba49744a5a..24d093fa89bb 100644 > >--- a/arch/powerpc/lib/memcmp_64.S > >+++ b/arch/powerpc/lib/memcmp_64.S > >@@ -102,7 +102,7 @@ > > * 2) src/dst has different offset to the 8 bytes boundary. The handlers > > * are named like .Ldiffoffset_ > > */ > >-_GLOBAL(memcmp) > >+_GLOBAL_TOC(memcmp) > >cmpdi cr1,r5,0 > > > >/* Use the short loop if the src/dst addresses are not > >-- > > > >It means the memcmp() fun entry will have additional 2 instructions. Is there > >any way to save these 2 instructions when the memcmp() is actually invoked > >from kernel itself? > > That will be the case. We will end up entering the function via the > local entry point skipping the first two instructions. The Global > entry point is only used for cross-module calls. > Yes. Thanks :) - Simon
Re: [PATCH v6 2/4] powerpc/64: enhance memcmp() with VMX instruction for long bytes comparision
Hi segher, On Wed, May 30, 2018 at 05:03:21PM +0800, Simon Guo wrote: > On Wed, May 30, 2018 at 03:35:40AM -0500, Segher Boessenkool wrote: > > On Wed, May 30, 2018 at 04:14:02PM +0800, Simon Guo wrote: > > > Hi Segher, > > > On Mon, May 28, 2018 at 06:05:59AM -0500, Segher Boessenkool wrote: > > > > On Fri, May 25, 2018 at 12:07:34PM +0800, wei.guo.si...@gmail.com wrote: > > > > > + /* save and restore cr0 */ > > > > > + mfocrf r5,64 > > > > > + EXIT_VMX_OPS > > > > > + mtocrf 64,r5 > > > > > + b .LcmpAB_lightweight > > > > > > > > That's cr1, not cr0. You can use mcrf instead, it is cheaper (esp. if > > > > you have it in a non-volatile CR field before so you need only one, if > > > > any). > > > > > > > You are right :) How about using mtcr/mfcr instead, I think they are > > > fast as well and more readable. > > > > Those are much worse than m[ft]ocrf. > > > > You probably should just shuffle things around so that EXIT_VMX_OPS > > does not clobber the CR field you need to keep. > Let me use mcrf then :) I now felt unformatable to use mcrf like: mcrf7,0 since I cannot 100% confident that compiler will not use CR7 or other CR# in exit_vmx_ops(). Can we switch back to mfocrf/mtocrf with correct CR0 value? mfocrf r5,128 ... mtocrf 128,r5 Thanks, - Simon
Re: [PATCH v7 0/5] powerpc/64: memcmp() optimization
Hi Michael, On Tue, Jun 05, 2018 at 12:16:22PM +1000, Michael Ellerman wrote: > Hi Simon, > > wei.guo.si...@gmail.com writes: > > From: Simon Guo > > > > There is some room to optimize memcmp() in powerpc 64 bits version for > > following 2 cases: > > (1) Even src/dst addresses are not aligned with 8 bytes at the beginning, > > memcmp() can align them and go with .Llong comparision mode without > > fallback to .Lshort comparision mode do compare buffer byte by byte. > > (2) VMX instructions can be used to speed up for large size comparision, > > currently the threshold is set for 4K bytes. Notes the VMX instructions > > will lead to VMX regs save/load penalty. This patch set includes a > > patch to add a 32 bytes pre-checking to minimize the penalty. > > > > It did the similar with glibc commit dec4a7105e (powerpc: Improve memcmp > > performance for POWER8). Thanks Cyril Bur's information. > > This patch set also updates memcmp selftest case to make it compiled and > > incorporate large size comparison case. > > I'm seeing a few crashes with this applied, I haven't had time to look > into what is happening yet, sorry. > The bug is due to memcmp() invokes a C function enter_vmx_ops() who will load some PIC value based on r2. memcmp() doesn't use r2 and if the memcmp() is invoked from kernel itself, everything is fine. But if memcmp() is invoked from modules[test_user_copy], r2 will be required to be setup correctly. Otherwise the enter_vmx_ops() will refer to an incorrect/unexisting data location based on wrong r2 value. Following patch will fix this issue: diff --git a/arch/powerpc/lib/memcmp_64.S b/arch/powerpc/lib/memcmp_64.S index 5eba49744a5a..24d093fa89bb 100644 --- a/arch/powerpc/lib/memcmp_64.S +++ b/arch/powerpc/lib/memcmp_64.S @@ -102,7 +102,7 @@ * 2) src/dst has different offset to the 8 bytes boundary. The handlers * are named like .Ldiffoffset_ */ -_GLOBAL(memcmp) +_GLOBAL_TOC(memcmp) cmpdi cr1,r5,0 /* Use the short loop if the src/dst addresses are not -- It means the memcmp() fun entry will have additional 2 instructions. Is there any way to save these 2 instructions when the memcmp() is actually invoked from kernel itself? Again thanks for finding this issue. Thanks, - Simon > [ 2471.300595] kselftest: Running tests in user > [ 2471.302785] calling test_user_copy_init+0x0/0xd14 [test_user_copy] @ 44883 > [ 2471.302892] Unable to handle kernel paging request for data at address > 0xc00818553005 > [ 2471.303014] Faulting instruction address: 0xc001f29c > [ 2471.303119] Oops: Kernel access of bad area, sig: 11 [#1] > [ 2471.303193] LE SMP NR_CPUS=2048 NUMA PowerNV > [ 2471.303256] Modules linked in: test_user_copy(+) vxlan ip6_udp_tunnel > udp_tunnel 8021q bridge stp llc dummy test_printf test_firmware vmx_crypto > crct10dif_vpmsum crct10dif_common crc32c_vpmsum veth [last unloaded: > test_static_key_base] > [ 2471.303532] CPU: 4 PID: 44883 Comm: modprobe Tainted: GW > 4.17.0-rc3-gcc7x-g7204012 #1 > [ 2471.303644] NIP: c001f29c LR: c001f6e4 CTR: > > [ 2471.303754] REGS: c01fddc2b560 TRAP: 0300 Tainted: GW > (4.17.0-rc3-gcc7x-g7204012) > [ 2471.303873] MSR: 92009033 CR: > 24222844 XER: > [ 2471.303996] CFAR: c001f6e0 DAR: c00818553005 DSISR: 4000 > IRQMASK: 0 > [ 2471.303996] GPR00: c001f6e4 c01fddc2b7e0 c00818529900 > 0200 > [ 2471.303996] GPR04: c01fe4b90020 ffe0 > 03fe01b48000 > [ 2471.303996] GPR08: 8000 c00818553005 c01fddc28000 > c00818520df0 > [ 2471.303996] GPR12: c009c430 c01fbc00 2000 > > [ 2471.303996] GPR16: c01fddc2bc20 0030 c01f7ba0 > 0001 > [ 2471.303996] GPR20: c0c772b0 c10b4018 > > [ 2471.303996] GPR24: c00818521c98 > c01fe4b9 > [ 2471.303996] GPR28: fff4 0200 92009033 > 92009033 > [ 2471.304930] NIP [c001f29c] msr_check_and_set+0x3c/0xc0 > [ 2471.305008] LR [c001f6e4] enable_kernel_altivec+0x44/0x100 > [ 2471.305084] Call Trace: > [ 2471.305122] [c01fddc2b7e0] [c009baa8] > __copy_tofrom_user_base+0x9c/0x574 (unreliable) > [ 2471.305240] [c01fddc2b860] [c001f6e4] > enable_kernel_altivec+0x44/0x100 > [ 2471.305336] [c01fddc2b890] [c009ce40] enter_vmx_ops+0x50/0x70 > [ 2471.305418] [c01fddc2b8b0] [c009c768] memcmp+0x338/0x6
Re: [PATCH v7 0/5] powerpc/64: memcmp() optimization
Hi Michael, On Tue, Jun 05, 2018 at 12:16:22PM +1000, Michael Ellerman wrote: > Hi Simon, > > wei.guo.si...@gmail.com writes: > > From: Simon Guo > > > > There is some room to optimize memcmp() in powerpc 64 bits version for > > following 2 cases: > > (1) Even src/dst addresses are not aligned with 8 bytes at the beginning, > > memcmp() can align them and go with .Llong comparision mode without > > fallback to .Lshort comparision mode do compare buffer byte by byte. > > (2) VMX instructions can be used to speed up for large size comparision, > > currently the threshold is set for 4K bytes. Notes the VMX instructions > > will lead to VMX regs save/load penalty. This patch set includes a > > patch to add a 32 bytes pre-checking to minimize the penalty. > > > > It did the similar with glibc commit dec4a7105e (powerpc: Improve memcmp > > performance for POWER8). Thanks Cyril Bur's information. > > This patch set also updates memcmp selftest case to make it compiled and > > incorporate large size comparison case. > > I'm seeing a few crashes with this applied, I haven't had time to look > into what is happening yet, sorry. Sorry I didn't catch this in my testing. I will check the root cause and update later. Thanks, - Simon > > [ 2471.300595] kselftest: Running tests in user > [ 2471.302785] calling test_user_copy_init+0x0/0xd14 [test_user_copy] @ 44883 > [ 2471.302892] Unable to handle kernel paging request for data at address > 0xc00818553005 > [ 2471.303014] Faulting instruction address: 0xc001f29c > [ 2471.303119] Oops: Kernel access of bad area, sig: 11 [#1] > [ 2471.303193] LE SMP NR_CPUS=2048 NUMA PowerNV > [ 2471.303256] Modules linked in: test_user_copy(+) vxlan ip6_udp_tunnel > udp_tunnel 8021q bridge stp llc dummy test_printf test_firmware vmx_crypto > crct10dif_vpmsum crct10dif_common crc32c_vpmsum veth [last unloaded: > test_static_key_base] > [ 2471.303532] CPU: 4 PID: 44883 Comm: modprobe Tainted: GW > 4.17.0-rc3-gcc7x-g7204012 #1 > [ 2471.303644] NIP: c001f29c LR: c001f6e4 CTR: > > [ 2471.303754] REGS: c01fddc2b560 TRAP: 0300 Tainted: GW > (4.17.0-rc3-gcc7x-g7204012) > [ 2471.303873] MSR: 92009033 CR: > 24222844 XER: > [ 2471.303996] CFAR: c001f6e0 DAR: c00818553005 DSISR: 4000 > IRQMASK: 0 > [ 2471.303996] GPR00: c001f6e4 c01fddc2b7e0 c00818529900 > 0200 > [ 2471.303996] GPR04: c01fe4b90020 ffe0 > 03fe01b48000 > [ 2471.303996] GPR08: 8000 c00818553005 c01fddc28000 > c00818520df0 > [ 2471.303996] GPR12: c009c430 c01fbc00 2000 > > [ 2471.303996] GPR16: c01fddc2bc20 0030 c01f7ba0 > 0001 > [ 2471.303996] GPR20: c0c772b0 c10b4018 > > [ 2471.303996] GPR24: c00818521c98 > c01fe4b9 > [ 2471.303996] GPR28: fff4 0200 92009033 > 92009033 > [ 2471.304930] NIP [c001f29c] msr_check_and_set+0x3c/0xc0 > [ 2471.305008] LR [c001f6e4] enable_kernel_altivec+0x44/0x100 > [ 2471.305084] Call Trace: > [ 2471.305122] [c01fddc2b7e0] [c009baa8] > __copy_tofrom_user_base+0x9c/0x574 (unreliable) > [ 2471.305240] [c01fddc2b860] [c001f6e4] > enable_kernel_altivec+0x44/0x100 > [ 2471.305336] [c01fddc2b890] [c009ce40] enter_vmx_ops+0x50/0x70 > [ 2471.305418] [c01fddc2b8b0] [c009c768] memcmp+0x338/0x680 > [ 2471.305501] [c01fddc2b9b0] [c00818520190] > test_user_copy_init+0x188/0xd14 [test_user_copy] > [ 2471.305617] [c01fddc2ba60] [c000de20] > do_one_initcall+0x90/0x560 > [ 2471.305710] [c01fddc2bb30] [c0200630] do_init_module+0x90/0x260 > [ 2471.305795] [c01fddc2bbc0] [c01fec88] load_module+0x1a28/0x1ce0 > [ 2471.305875] [c01fddc2bd70] [c01ff1e8] > sys_finit_module+0xc8/0x110 > [ 2471.305983] [c01fddc2be30] [c000b528] system_call+0x58/0x6c > [ 2471.306066] Instruction dump: > [ 2471.306112] fba1ffe8 fbc1fff0 fbe1fff8 f8010010 f821ff81 7c7d1b78 6000 > 6000 > [ 2471.306216] 7fe000a6 3d220003 39299705 7ffeeb78 <8929> 2f89 > 419e0044 6000 > [ 2471.306326] ---[ end trace daf8d409e65b9841 ]--- > > And: > > [ 19.096709] test_bpf: test_skb_segment: success in skb_segment! > [ 19.096799] initcall test_bpf_init+0x0/0xae0 [test_bpf] returned 0 after > 591217 usecs > [ 19.
Re: [PATCH v4 04/29] KVM: PPC: Book3S PR: Move kvmppc_save_tm/kvmppc_restore_tm to separate file
On Wed, May 30, 2018 at 09:40:27AM +1000, Paul Mackerras wrote: > On Wed, May 23, 2018 at 03:01:47PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > It is a simple patch just for moving kvmppc_save_tm/kvmppc_restore_tm() > > functionalities to tm.S. There is no logic change. The reconstruct of > > those APIs will be done in later patches to improve readability. > > > > It is for preparation of reusing those APIs on both HV/PR PPC KVM. > > > > Some slight change during move the functions includes: > > - surrounds some HV KVM specific code with CONFIG_KVM_BOOK3S_HV_POSSIBLE > > for compilation. > > - use _GLOBAL() to define kvmppc_save_tm/kvmppc_restore_tm() > > > > Signed-off-by: Simon Guo > > --- > > arch/powerpc/kvm/Makefile | 3 + > > arch/powerpc/kvm/book3s_hv_rmhandlers.S | 322 > > arch/powerpc/kvm/tm.S | 363 > > > > 3 files changed, 366 insertions(+), 322 deletions(-) > > create mode 100644 arch/powerpc/kvm/tm.S > > > > diff --git a/arch/powerpc/kvm/Makefile b/arch/powerpc/kvm/Makefile > > index 4b19da8..f872c04 100644 > > --- a/arch/powerpc/kvm/Makefile > > +++ b/arch/powerpc/kvm/Makefile > > @@ -63,6 +63,9 @@ kvm-pr-y := \ > > book3s_64_mmu.o \ > > book3s_32_mmu.o > > > > +kvm-book3s_64-builtin-objs-$(CONFIG_KVM_BOOK3S_64_HANDLER) += \ > > + tm.o > > + > > ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE > > kvm-book3s_64-builtin-objs-$(CONFIG_KVM_BOOK3S_64_HANDLER) += \ > > book3s_rmhandlers.o > > diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S > > b/arch/powerpc/kvm/book3s_hv_rmhandlers.S > > index 5e6e493..4db2b10 100644 > > --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S > > +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S > > @@ -39,8 +39,6 @@ BEGIN_FTR_SECTION;\ > > extsw reg, reg; \ > > END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300) > > > > -#define VCPU_GPRS_TM(reg) (((reg) * ULONG_SIZE) + VCPU_GPR_TM) > > - > > /* Values in HSTATE_NAPPING(r13) */ > > #define NAPPING_CEDE 1 > > #define NAPPING_NOVCPU 2 > > @@ -3119,326 +3117,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC) > > mr r4,r31 > > blr > > > > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > > -/* > > - * Save transactional state and TM-related registers. > > - * Called with r9 pointing to the vcpu struct. > > - * This can modify all checkpointed registers, but > > - * restores r1, r2 and r9 (vcpu pointer) before exit. > > - */ > > -kvmppc_save_tm: > > - mflrr0 > > - std r0, PPC_LR_STKOFF(r1) > > - stdur1, -PPC_MIN_STKFRM(r1) > > - > > - /* Turn on TM. */ > > - mfmsr r8 > > - li r0, 1 > > - rldimi r8, r0, MSR_TM_LG, 63-MSR_TM_LG > > - mtmsrd r8 > > - > > - ld r5, VCPU_MSR(r9) > > - rldicl. r5, r5, 64 - MSR_TS_S_LG, 62 > > - beq 1f /* TM not active in guest. */ > > - > > - std r1, HSTATE_HOST_R1(r13) > > - li r3, TM_CAUSE_KVM_RESCHED > > - > > -BEGIN_FTR_SECTION > > - lbz r0, HSTATE_FAKE_SUSPEND(r13) /* Were we fake suspended? */ > > - cmpwi r0, 0 > > - beq 3f > > - rldicl. r8, r8, 64 - MSR_TS_S_LG, 62 /* Did we actually hrfid? */ > > - beq 4f > > -BEGIN_FTR_SECTION_NESTED(96) > > - bl pnv_power9_force_smt4_catch > > -END_FTR_SECTION_NESTED(CPU_FTR_P9_TM_XER_SO_BUG, CPU_FTR_P9_TM_XER_SO_BUG, > > 96) > > - nop > > - b 6f > > -3: > > - /* Emulation of the treclaim instruction needs TEXASR before treclaim */ > > - mfspr r6, SPRN_TEXASR > > - std r6, VCPU_ORIG_TEXASR(r9) > > -6: > > -END_FTR_SECTION_IFSET(CPU_FTR_P9_TM_HV_ASSIST) > > It worries me that we now have this TM hypervisor assist stuff in a > place where it could be active with PR KVM. I think it would be > better to factor out the TM assist code into a separate function which > then calls kvmppc_save_tm if it needs to do an actual treclaim. I'll > look at doing that. Paul, Thanks for the info. Please let me know if anything I can help. BR, - Simon
Re: [PATCH v6 2/4] powerpc/64: enhance memcmp() with VMX instruction for long bytes comparision
On Wed, May 30, 2018 at 03:35:40AM -0500, Segher Boessenkool wrote: > On Wed, May 30, 2018 at 04:14:02PM +0800, Simon Guo wrote: > > Hi Segher, > > On Mon, May 28, 2018 at 06:05:59AM -0500, Segher Boessenkool wrote: > > > On Fri, May 25, 2018 at 12:07:34PM +0800, wei.guo.si...@gmail.com wrote: > > > > + /* save and restore cr0 */ > > > > + mfocrf r5,64 > > > > + EXIT_VMX_OPS > > > > + mtocrf 64,r5 > > > > + b .LcmpAB_lightweight > > > > > > That's cr1, not cr0. You can use mcrf instead, it is cheaper (esp. if > > > you have it in a non-volatile CR field before so you need only one, if > > > any). > > > > > You are right :) How about using mtcr/mfcr instead, I think they are > > fast as well and more readable. > > Those are much worse than m[ft]ocrf. > > You probably should just shuffle things around so that EXIT_VMX_OPS > does not clobber the CR field you need to keep. Let me use mcrf then :) Thanks, - Simon
Re: [PATCH v6 1/4] powerpc/64: Align bytes before fall back to .Lshort in powerpc64 memcmp()
On Wed, May 30, 2018 at 03:27:39AM -0500, Segher Boessenkool wrote: > Hi! > > On Wed, May 30, 2018 at 04:11:50PM +0800, Simon Guo wrote: > > On Mon, May 28, 2018 at 05:35:12AM -0500, Segher Boessenkool wrote: > > > On Fri, May 25, 2018 at 12:07:33PM +0800, wei.guo.si...@gmail.com wrote: > > > If this doesn't use cr0 anymore, you can do rlwinm r6,r6,0,7 instead of > > > andi r6,r6,7 . > > > > > CR0 is used at .Lno_short handling. > > Tricky. > > > > > + subfc. r5,r6,r5 > > > > > > Why subfc? You don't use the carry. > > OK. I will use subfc instead. > > I meant subf -- no carry. If you want CR0 set there is subf. just fine. > > > > > + bgt cr0,8f > > > > + li r3,-1 > > > > +8: > > > > + blr > > > > > > blelr > > > li r3,-1 > > > blr > > Sure. That looks more impact. > > Should have been bgtlr of course -- well check please :-) Yes :) Thanks, - Simon
Re: [PATCH v6 2/4] powerpc/64: enhance memcmp() with VMX instruction for long bytes comparision
Hi Michael, On Mon, May 28, 2018 at 09:59:29PM +1000, Michael Ellerman wrote: > Hi Simon, > > wei.guo.si...@gmail.com writes: > > diff --git a/arch/powerpc/lib/memcmp_64.S b/arch/powerpc/lib/memcmp_64.S > > index f20e883..4ba7bb6 100644 > > --- a/arch/powerpc/lib/memcmp_64.S > > +++ b/arch/powerpc/lib/memcmp_64.S > > @@ -174,6 +235,13 @@ _GLOBAL(memcmp) > > blr > > > > .Llong: > > +#ifdef CONFIG_ALTIVEC > > + /* Try to use vmx loop if length is equal or greater than 4K */ > > + cmpldi cr6,r5,VMX_THRESH > > + bge cr6,.Lsameoffset_vmx_cmp > > + > > Here we decide to use vmx, but we don't do any CPU feature checks. > > > > @@ -332,7 +400,94 @@ _GLOBAL(memcmp) > > 8: > > blr > > > > +#ifdef CONFIG_ALTIVEC > > +.Lsameoffset_vmx_cmp: > > + /* Enter with src/dst addrs has the same offset with 8 bytes > > +* align boundary > > +*/ > > + ENTER_VMX_OPS > > + beq cr1,.Llong_novmx_cmp > > + > > +3: > > + /* need to check whether r4 has the same offset with r3 > > +* for 16 bytes boundary. > > +*/ > > + xor r0,r3,r4 > > + andi. r0,r0,0xf > > + bne .Ldiffoffset_vmx_cmp_start > > + > > + /* len is no less than 4KB. Need to align with 16 bytes further. > > +*/ > > + andi. rA,r3,8 > > + LD rA,0,r3 > > + beq 4f > > + LD rB,0,r4 > > + cmpld cr0,rA,rB > > + addir3,r3,8 > > + addir4,r4,8 > > + addir5,r5,-8 > > + > > + beq cr0,4f > > + /* save and restore cr0 */ > > + mfocrf r5,64 > > + EXIT_VMX_OPS > > + mtocrf 64,r5 > > + b .LcmpAB_lightweight > > + > > +4: > > + /* compare 32 bytes for each loop */ > > + srdir0,r5,5 > > + mtctr r0 > > + clrldi r5,r5,59 > > + li off16,16 > > + > > +.balign 16 > > +5: > > + lvx v0,0,r3 > > + lvx v1,0,r4 > > + vcmpequd. v0,v0,v1 > > vcmpequd is only available on Power8 and later CPUs. > > Which means this will crash on Power7 or earlier. > > Something like this should fix it I think. > > diff --git a/arch/powerpc/lib/memcmp_64.S b/arch/powerpc/lib/memcmp_64.S > index 96eb08b2be2e..0a11ff14dcd9 100644 > --- a/arch/powerpc/lib/memcmp_64.S > +++ b/arch/powerpc/lib/memcmp_64.S > @@ -236,9 +236,11 @@ _GLOBAL(memcmp) > > .Llong: > #ifdef CONFIG_ALTIVEC > +BEGIN_FTR_SECTION > /* Try to use vmx loop if length is equal or greater than 4K */ > cmpldi cr6,r5,VMX_THRESH > bge cr6,.Lsameoffset_vmx_cmp > +END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S) > > .Llong_novmx_cmp: > #endif Thanks for the good catch! I will update that. > > > There's another problem which is that old toolchains don't know about > vcmpequd. To fix that we'll need to add a macro that uses .long to > construct the instruction. Right. I will add the corresponding macros. Thanks for your review. BR, - Simon
Re: [PATCH v6 2/4] powerpc/64: enhance memcmp() with VMX instruction for long bytes comparision
Hi Segher, On Mon, May 28, 2018 at 06:05:59AM -0500, Segher Boessenkool wrote: > On Fri, May 25, 2018 at 12:07:34PM +0800, wei.guo.si...@gmail.com wrote: > > + /* save and restore cr0 */ > > + mfocrf r5,64 > > + EXIT_VMX_OPS > > + mtocrf 64,r5 > > + b .LcmpAB_lightweight > > That's cr1, not cr0. You can use mcrf instead, it is cheaper (esp. if > you have it in a non-volatile CR field before so you need only one, if any). > You are right :) How about using mtcr/mfcr instead, I think they are fast as well and more readable. > > + vcmpequb. v7,v9,v10 > > + bnl cr6,.Ldiffoffset_vmx_diff_found > > In other places you say bf 24,... Dunno which is more readable, but > please pick one? I will update to bnl for other cases. > > > Segher Thanks for your review. BR, - Simon
Re: [PATCH v6 1/4] powerpc/64: Align bytes before fall back to .Lshort in powerpc64 memcmp()
Hi Segher, On Mon, May 28, 2018 at 05:35:12AM -0500, Segher Boessenkool wrote: > On Fri, May 25, 2018 at 12:07:33PM +0800, wei.guo.si...@gmail.com wrote: > > _GLOBAL(memcmp) > > cmpdi cr1,r5,0 > > > > - /* Use the short loop if both strings are not 8B aligned */ > > - or r6,r3,r4 > > + /* Use the short loop if the src/dst addresses are not > > +* with the same offset of 8 bytes align boundary. > > +*/ > > + xor r6,r3,r4 > > andi. r6,r6,7 > > > > - /* Use the short loop if length is less than 32B */ > > - cmpdi cr6,r5,31 > > + /* Fall back to short loop if compare at aligned addrs > > +* with less than 8 bytes. > > +*/ > > + cmpdi cr6,r5,7 > > > > beq cr1,.Lzero > > - bne .Lshort > > - bgt cr6,.Llong > > + bgt cr6,.Lno_short > > If this doesn't use cr0 anymore, you can do rlwinm r6,r6,0,7 instead of > andi r6,r6,7 . > CR0 is used at .Lno_short handling. > > +.Lsameoffset_8bytes_make_align_start: > > + /* attempt to compare bytes not aligned with 8 bytes so that > > +* rest comparison can run based on 8 bytes alignment. > > +*/ > > + andi. r6,r3,7 > > + > > + /* Try to compare the first double word which is not 8 bytes aligned: > > +* load the first double word at (src & ~7UL) and shift left appropriate > > +* bits before comparision. > > +*/ > > + clrlwi r6,r3,29 > > + rlwinm r6,r6,3,0,28 > > Those last two lines are together just > rlwinm r6,r3,3,0x1c > Yes. I will combine them. > > + subfc. r5,r6,r5 > > Why subfc? You don't use the carry. OK. I will use subfc instead. > > > + rlwinm r6,r6,3,0,28 > > That's > slwi r6,r6,3 Yes. > > > + bgt cr0,8f > > + li r3,-1 > > +8: > > + blr > > blelr > li r3,-1 > blr Sure. That looks more impact. > > (and more of the same things elsewhere). > > > Segher Thanks for your good comments. BR, - Simon
Re: [PATCH] KVM: PPC: remove mmio_vsx_tx_sx_enabled in PR KVM MMIO emulation
On Thu, May 24, 2018 at 05:01:26PM +0800, wei.guo.si...@gmail.com wrote: > From: Simon Guo > > Originally PR KVM MMIO emulation uses only 0~31#(5 bits) for VSR > reg number, and use mmio_vsx_tx_sx_enabled field together for > 0~63# VSR regs. > > Currently PR KVM MMIO emulation is reimplemented with analyse_instr() > assistence. analyse_instr() returns 0~63 for VSR register number, so > it is not necessary to use additional mmio_vsx_tx_sx_enabled field > any more. > > This patch extends related reg bits(expand io_gpr to u16 from u8 > and use 6 bits for VSR reg#), so that mmio_vsx_tx_sx_enabled can > be removed. > > Signed-off-by: Simon Guo It applies to HV KVM too. I will correct the commit message in next version. Thanks, - Simon
Re: [PATCH v5 2/4] powerpc/64: enhance memcmp() with VMX instruction for long bytes comparision
Hi Michael, On Thu, May 24, 2018 at 05:44:33PM +1000, Michael Ellerman wrote: > Hi Simon, > > wei.guo.si...@gmail.com writes: > > From: Simon Guo > > > > This patch add VMX primitives to do memcmp() in case the compare size > > exceeds 4K bytes. KSM feature can benefit from this. > > You say "exceeds 4K" here. > it should be >= 4k. I will correct the message. > > diff --git a/arch/powerpc/lib/memcmp_64.S b/arch/powerpc/lib/memcmp_64.S > > index f20e883..6303bbf 100644 > > --- a/arch/powerpc/lib/memcmp_64.S > > +++ b/arch/powerpc/lib/memcmp_64.S > > @@ -27,12 +27,73 @@ > > #define LH lhbrx > > #define LW lwbrx > > #define LD ldbrx > > +#define LVSlvsr > > +#define VPERM(_VRT,_VRA,_VRB,_VRC) \ > > + vperm _VRT,_VRB,_VRA,_VRC > > #else > > #define LH lhzx > > #define LW lwzx > > #define LD ldx > > +#define LVSlvsl > > +#define VPERM(_VRT,_VRA,_VRB,_VRC) \ > > + vperm _VRT,_VRA,_VRB,_VRC > > #endif > > > > +#define VMX_OPS_THRES 4096 > > THRES == 4096 > > BTW, can we call it VMX_THRESH ? > Sure. I will update it. > > +#define ENTER_VMX_OPS \ > > + mflrr0; \ > > + std r3,-STACKFRAMESIZE+STK_REG(R31)(r1); \ > > + std r4,-STACKFRAMESIZE+STK_REG(R30)(r1); \ > > + std r5,-STACKFRAMESIZE+STK_REG(R29)(r1); \ > > + std r0,16(r1); \ > > + stdur1,-STACKFRAMESIZE(r1); \ > > + bl enter_vmx_ops; \ > > + cmpwi cr1,r3,0; \ > > + ld r0,STACKFRAMESIZE+16(r1); \ > > + ld r3,STK_REG(R31)(r1); \ > > + ld r4,STK_REG(R30)(r1); \ > > + ld r5,STK_REG(R29)(r1); \ > > + addir1,r1,STACKFRAMESIZE; \ > > + mtlrr0 > > + > > +#define EXIT_VMX_OPS \ > > + mflrr0; \ > > + std r3,-STACKFRAMESIZE+STK_REG(R31)(r1); \ > > + std r4,-STACKFRAMESIZE+STK_REG(R30)(r1); \ > > + std r5,-STACKFRAMESIZE+STK_REG(R29)(r1); \ > > + std r0,16(r1); \ > > + stdur1,-STACKFRAMESIZE(r1); \ > > + bl exit_vmx_ops; \ > > + ld r0,STACKFRAMESIZE+16(r1); \ > > + ld r3,STK_REG(R31)(r1); \ > > + ld r4,STK_REG(R30)(r1); \ > > + ld r5,STK_REG(R29)(r1); \ > > + addir1,r1,STACKFRAMESIZE; \ > > + mtlrr0 > > + > > +/* > > + * LD_VSR_CROSS16B load the 2nd 16 bytes for _vaddr which is unaligned with > > + * 16 bytes boundary and permute the result with the 1st 16 bytes. > > + > > + *| y y y y y y y y y y y y y 0 1 2 | 3 4 5 6 7 8 9 a b c d e f z z z > > | > > + *^ ^ > > ^ > > + * 0x10 0x20 > > 0xbbb30 > > + * ^ > > + *_vaddr > > + * > > + * > > + * _vmask is the mask generated by LVS > > + * _v1st_qw is the 1st aligned QW of current addr which is already loaded. > > + * for example: 0xy012 for big endian > > + * _v2nd_qw is the 2nd aligned QW of cur _vaddr to be loaded. > > + * for example: 0x3456789abcdefzzz for big endian > > + * The permute result is saved in _v_res. > > + * for example: 0x0123456789abcdef for big endian. > > + */ > > +#define LD_VSR_CROSS16B(_vaddr,_vmask,_v1st_qw,_v2nd_qw,_v_res) \ > > +lvx _v2nd_qw,_vaddr,off16; \ > > +VPERM(_v_res,_v1st_qw,_v2nd_qw,_vmask) > > + > > /* > > * There are 2 categories for memcmp: > > * 1) src/dst has the same offset to the 8 bytes boundary. The handlers > > @@ -174,6 +235,13 @@ _GLOBAL(memcmp) > > blr > > > > .Llong: > > +#ifdef CONFIG_ALTIVEC > > + /* Try to use vmx loop if length is larger than 4K */ > > + cmpldi cr6,r5,VMX_OPS_THRES > > + bge cr6,.Lsameoffset_vmx_cmp > > Here we compare the length to 4K and if it's greater *or equal* then we > go to the VMX case. Or am I reading it backward? > > So we should say "if the size is 4K or more we do VMX" shouldn't we? Yes. Again I need reword the comment to "equal or greater than 4K" here. Thanks, - Simon
Re: [PATCH v3 24/29] KVM: PPC: Book3S PR: Support TAR handling for PR KVM HTM.
Hi Paul, On Tue, May 22, 2018 at 09:44:47PM +1000, Paul Mackerras wrote: > On Mon, May 21, 2018 at 12:09:41PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > Currently guest kernel doesn't handle TAR fac unavailable and it always > > runs with TAR bit on. PR KVM will lazily enable TAR. TAR is not a > > frequent-use reg and it is not included in SVCPU struct. > > > > Due to the above, the checkpointed TAR val might be a bogus TAR val. > > To solve this issue, we will make vcpu->arch.fscr tar bit consistent > > with shadow_fscr when TM enabled. > > > > At the end of emulating treclaim., the correct TAR val need to be loaded > > into reg if FSCR_TAR bit is on. > > At the beginning of emulating trechkpt., TAR needs to be flushed so that > > the right tar val can be copy into tar_tm. > > > > Tested with: > > tools/testing/selftests/powerpc/tm/tm-tar > > tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar (remove DSCR/PPR > > related testing). > > With this patch, a 32-bit powermac compile with PR KVM enabled gives > this error: > > arch/powerpc/kvm/book3s_pr.c:58:12: error: ‘kvmppc_handle_fac’ declared > ‘static’ but never defined [-Werror=unused-function] > static int kvmppc_handle_fac(struct kvm_vcpu *vcpu, ulong fac); > ^ > cc1: all warnings being treated as errors > scripts/Makefile.build:312: recipe for target 'arch/powerpc/kvm/book3s_pr.o' > failed > > The easy fix is a #ifdef CONFIG_PPC_BOOK3S_64 around the forward > static definition. Thx for find that. I will fix that and send V4 includes that simple fix. BR, - Simon
Re: [PATCH v3 5/7] KVM: PPC: reimplements LOAD_VSX/STORE_VSX instruction mmio emulation with analyse_intr() input
Hi Paul, On Tue, May 22, 2018 at 07:41:51PM +1000, Paul Mackerras wrote: > On Mon, May 21, 2018 at 01:24:24PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > This patch reimplements LOAD_VSX/STORE_VSX instruction MMIO emulation with > > analyse_intr() input. It utilizes VSX_FPCONV/VSX_SPLAT/SIGNEXT exported > > by analyse_instr() and handle accordingly. > > > > When emulating VSX store, the VSX reg will need to be flushed so that > > the right reg val can be retrieved before writing to IO MEM. > > When I tested this patch set with the MMIO emulation test program I > have, I got a host crash on the first test that used a VSX instruction > with a register number >= 32, that is, a VMX register. The crash was > that it hit the BUG() at line 1193 of arch/powerpc/kvm/powerpc.c. > > The reason it hit the BUG() is that vcpu->arch.io_gpr was 0xa3. > What's happening here is that analyse_instr gives a register numbers > in the range 32 - 63 for VSX instructions which access VMX registers. > When 35 is ORed with 0x80 (KVM_MMIO_REG_VSX) we get 0xa3. > > The old code didn't pass the high bit of the register number to > kvmppc_handle_vsx_load/store, but instead passed it via the > vcpu->arch.mmio_vsx_tx_sx_enabled field. With your patch set we still > set and use that field, so the patch below on top of your patches is > the quick fix. Ideally we would get rid of that field and just use > the high (0x20) bit of the register number instead, but that can be > cleaned up later. > > If you like, I will fold the patch below into this patch and push the > series to my kvm-ppc-next branch. > > Paul. Sorry my test missed this kind of cases. Please go ahead to fold the patch as you suggested. Thanks for point it out. If you like, I can do the clean up work. If I understand correctly, we need to expand io_gpr to u16 from u8 so that reg number can use 6 bits and leave room for other reg flag bits. BR, - Simon > --- > diff --git a/arch/powerpc/kvm/emulate_loadstore.c > b/arch/powerpc/kvm/emulate_loadstore.c > index 0165fcd..afde788 100644 > --- a/arch/powerpc/kvm/emulate_loadstore.c > +++ b/arch/powerpc/kvm/emulate_loadstore.c > @@ -242,8 +242,8 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu) > } > > emulated = kvmppc_handle_vsx_load(run, vcpu, > - KVM_MMIO_REG_VSX|op.reg, io_size_each, > - 1, op.type & SIGNEXT); > + KVM_MMIO_REG_VSX | (op.reg & 0x1f), > + io_size_each, 1, op.type & SIGNEXT); > break; > } > #endif > @@ -363,7 +363,7 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu) > } > > emulated = kvmppc_handle_vsx_store(run, vcpu, > - op.reg, io_size_each, 1); > + op.reg & 0x1f, io_size_each, 1); > break; > } > #endif
Re: [PATCH v4 3/4] powerpc/64: add 32 bytes prechecking before using VMX optimization on memcmp()
Hi Michael, On Fri, May 18, 2018 at 12:13:52AM +1000, Michael Ellerman wrote: > wei.guo.si...@gmail.com writes: > > From: Simon Guo > > > > This patch is based on the previous VMX patch on memcmp(). > > > > To optimize ppc64 memcmp() with VMX instruction, we need to think about > > the VMX penalty brought with: If kernel uses VMX instruction, it needs > > to save/restore current thread's VMX registers. There are 32 x 128 bits > > VMX registers in PPC, which means 32 x 16 = 512 bytes for load and store. > > > > The major concern regarding the memcmp() performance in kernel is KSM, > > who will use memcmp() frequently to merge identical pages. So it will > > make sense to take some measures/enhancement on KSM to see whether any > > improvement can be done here. Cyril Bur indicates that the memcmp() for > > KSM has a higher possibility to fail (unmatch) early in previous bytes > > in following mail. > > https://patchwork.ozlabs.org/patch/817322/#1773629 > > And I am taking a follow-up on this with this patch. > > > > Per some testing, it shows KSM memcmp() will fail early at previous 32 > > bytes. More specifically: > > - 76% cases will fail/unmatch before 16 bytes; > > - 83% cases will fail/unmatch before 32 bytes; > > - 84% cases will fail/unmatch before 64 bytes; > > So 32 bytes looks a better choice than other bytes for pre-checking. > > > > This patch adds a 32 bytes pre-checking firstly before jumping into VMX > > operations, to avoid the unnecessary VMX penalty. And the testing shows > > ~20% improvement on memcmp() average execution time with this patch. > > > > The detail data and analysis is at: > > https://github.com/justdoitqd/publicFiles/blob/master/memcmp/README.md > > > > Any suggestion is welcome. > > Thanks for digging into that, really great work. > > I'm inclined to make this not depend on KSM though. It seems like a good > optimisation to do in general. > > So can we just call it the 'pre-check' or something, and always do it? > Sound reasonable to me. I will expand the change to .Ldiffoffset_vmx_cmp case and test accordingly. Thanks, - Simon
Re: [PATCH v2 05/10] KVM: PPC: reimplement non-SIMD LOAD/STORE instruction mmio emulation with analyse_intr() input
Hi Paul, On Thu, May 17, 2018 at 09:49:18AM +1000, Paul Mackerras wrote: > On Mon, May 07, 2018 at 02:20:11PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > This patch reimplements non-SIMD LOAD/STORE instruction MMIO emulation > > with analyse_intr() input. It utilizes the BYTEREV/UPDATE/SIGNEXT > > properties exported by analyse_instr() and invokes > > kvmppc_handle_load(s)/kvmppc_handle_store() accordingly. > > > > It also move CACHEOP type handling into the skeleton. > > > > instruction_type within kvm_ppc.h is renamed to avoid conflict with > > sstep.h. > > > > Suggested-by: Paul Mackerras > > Signed-off-by: Simon Guo > > Looks pretty good, but one comment below... > > > - case OP_31_XOP_LWAUX: > > - emulated = kvmppc_handle_loads(run, vcpu, rt, 4, 1); > > - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed); > > - break; > > + if (emulated == EMULATE_DONE) > > + goto out; > > Shouldn't this also goto out if emulated == EMULATE_DO_MMIO? Yes. Let me update this. Thanks, - Simon
Re: [PATCH v2 07/10] KVM: PPC: reimplement LOAD_FP/STORE_FP instruction mmio emulation with analyse_intr() input
On Thu, May 17, 2018 at 09:52:07AM +1000, Paul Mackerras wrote: > On Mon, May 07, 2018 at 02:20:13PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > This patch reimplements LOAD_FP/STORE_FP instruction MMIO emulation with > > analyse_intr() input. It utilizes the FPCONV/UPDATE properties exported by > > analyse_instr() and invokes kvmppc_handle_load(s)/kvmppc_handle_store() > > accordingly. > > > > For FP store MMIO emulation, the FP regs need to be flushed firstly so > > that the right FP reg vals can be read from vcpu->arch.fpr, which will > > be stored into MMIO data. > > > > Suggested-by: Paul Mackerras > > Signed-off-by: Simon Guo > > One comment below, otherwise looks good... > > > arch/powerpc/kvm/emulate_loadstore.c | 197 > > +++ > > 1 file changed, 40 insertions(+), 157 deletions(-) > > > > diff --git a/arch/powerpc/kvm/emulate_loadstore.c > > b/arch/powerpc/kvm/emulate_loadstore.c > > index 2a91845..5a6571c 100644 > > --- a/arch/powerpc/kvm/emulate_loadstore.c > > +++ b/arch/powerpc/kvm/emulate_loadstore.c > > @@ -138,6 +138,22 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu) > > > > break; > > } > > +#ifdef CONFIG_PPC_FPU > > + case LOAD_FP: > > + if (kvmppc_check_fp_disabled(vcpu)) > > + return EMULATE_DONE; > > + > > + if (op.type & FPCONV) > > + vcpu->arch.mmio_sp64_extend = 1; > > + > > + emulated = kvmppc_handle_load(run, vcpu, > > + KVM_MMIO_REG_FPR|op.reg, size, 1); > > You need to check the SIGNEXT flag and call kvmppc_handle_loads if it > is set, because of the lfiwax case: > > > - case OP_31_XOP_LFIWAX: > > - if (kvmppc_check_fp_disabled(vcpu)) > > - return EMULATE_DONE; > > - emulated = kvmppc_handle_loads(run, vcpu, > > - KVM_MMIO_REG_FPR|rt, 4, 1); > > - break; Yes. I need to handle that. Thanks for point it out. BR, - Simon
Re: [PATCH v2 29/30] KVM: PPC: add KVM_SET_ONE_REG/KVM_GET_ONE_REG to async ioctl
Hi Paul,
On Tue, May 15, 2018 at 04:15:26PM +1000, Paul Mackerras wrote:
> On Wed, Feb 28, 2018 at 01:52:37AM +0800, wei.guo.si...@gmail.com wrote:
> > From: Simon Guo
> >
> > In both HV/PR KVM, the KVM_SET_ONE_REG/KVM_GET_ONE_REG ioctl should
> > be able to perform without load vcpu. This patch adds
> > KVM_SET_ONE_REG/KVM_GET_ONE_REG implementation to async ioctl
> > function.
> >
> > Signed-off-by: Simon Guo
> > ---
> > arch/powerpc/kvm/powerpc.c | 13 +
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> > index 7987fa3..6afd004 100644
> > --- a/arch/powerpc/kvm/powerpc.c
> > +++ b/arch/powerpc/kvm/powerpc.c
> > @@ -1619,6 +1619,19 @@ long kvm_arch_vcpu_async_ioctl(struct file *filp,
> > return -EFAULT;
> > return kvm_vcpu_ioctl_interrupt(vcpu, &irq);
> > }
> > +
> > + if ((ioctl == KVM_SET_ONE_REG) || (ioctl == KVM_GET_ONE_REG)) {
> > + struct kvm_one_reg reg;
> > +
> > + if (copy_from_user(®, argp, sizeof(reg)))
> > + return -EFAULT;
> > +
> > + if (ioctl == KVM_SET_ONE_REG)
> > + return kvm_vcpu_ioctl_set_one_reg(vcpu, ®);
> > + else
> > + return kvm_vcpu_ioctl_get_one_reg(vcpu, ®);
> > + }
> > +
> > return -ENOIOCTLCMD;
> > }
>
> This seems dangerous to me, since now we can have set/get one_reg
> running in parallel with vcpu execution. Is there a really compelling
> reason to do this? If not I'd rather not make this change.
I will remove this patch.
Thanks,
- Simon
Re: [PATCH v2 18/30] KVM: PPC: Book3S PR: always fail transaction in guest privilege state
Hi Paul,
On Tue, May 15, 2018 at 04:07:55PM +1000, Paul Mackerras wrote:
> On Wed, Feb 28, 2018 at 01:52:26AM +0800, wei.guo.si...@gmail.com wrote:
> > From: Simon Guo
> >
> > Currently kernel doesn't use transaction memory.
> > And there is an issue for privilege guest that:
> > tbegin/tsuspend/tresume/tabort TM instructions can impact MSR TM bits
> > without trap into PR host. So following code will lead to a false mfmsr
> > result:
> > tbegin <- MSR bits update to Transaction active.
> > beq <- failover handler branch
> > mfmsr <- still read MSR bits from magic page with
> > transaction inactive.
> >
> > It is not an issue for non-privilege guest since its mfmsr is not patched
> > with magic page and will always trap into PR host.
> >
> > This patch will always fail tbegin attempt for privilege guest, so that
> > the above issue is prevented. It is benign since currently (guest) kernel
> > doesn't initiate a transaction.
> >
> > Test case:
> > https://github.com/justdoitqd/publicFiles/blob/master/test_tbegin_pr.c
> >
> > Signed-off-by: Simon Guo
> > ---
> > arch/powerpc/include/asm/kvm_book3s.h | 2 ++
> > arch/powerpc/kvm/book3s_emulate.c | 43
> > +++
> > arch/powerpc/kvm/book3s_pr.c | 11 -
> > 3 files changed, 55 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_book3s.h
> > b/arch/powerpc/include/asm/kvm_book3s.h
> > index 2ecb6a3..9690280 100644
> > --- a/arch/powerpc/include/asm/kvm_book3s.h
> > +++ b/arch/powerpc/include/asm/kvm_book3s.h
> > @@ -258,9 +258,11 @@ extern void kvmppc_copy_from_svcpu(struct kvm_vcpu
> > *vcpu,
> > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > void kvmppc_save_tm_pr(struct kvm_vcpu *vcpu);
> > void kvmppc_restore_tm_pr(struct kvm_vcpu *vcpu);
> > +void kvmppc_restore_tm_sprs(struct kvm_vcpu *vcpu);
> > #else
> > static inline void kvmppc_save_tm_pr(struct kvm_vcpu *vcpu) {}
> > static inline void kvmppc_restore_tm_pr(struct kvm_vcpu *vcpu) {}
> > +static inline void kvmppc_restore_tm_sprs(struct kvm_vcpu *vcpu) {}
> > #endif
> >
> > extern int kvm_irq_bypass;
> > diff --git a/arch/powerpc/kvm/book3s_emulate.c
> > b/arch/powerpc/kvm/book3s_emulate.c
> > index a03533d..90b5f59 100644
> > --- a/arch/powerpc/kvm/book3s_emulate.c
> > +++ b/arch/powerpc/kvm/book3s_emulate.c
> > @@ -23,6 +23,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include "book3s.h"
> >
> > #define OP_19_XOP_RFID 18
> > @@ -47,6 +48,8 @@
> > #define OP_31_XOP_EIOIO854
> > #define OP_31_XOP_SLBMFEE 915
> >
> > +#define OP_31_XOP_TBEGIN 654
> > +
> > /* DCBZ is actually 1014, but we patch it to 1010 so we get a trap */
> > #define OP_31_XOP_DCBZ 1010
> >
> > @@ -362,6 +365,46 @@ int kvmppc_core_emulate_op_pr(struct kvm_run *run,
> > struct kvm_vcpu *vcpu,
> >
> > break;
> > }
> > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > + case OP_31_XOP_TBEGIN:
> > + {
> > + if (!cpu_has_feature(CPU_FTR_TM))
> > + break;
> > +
> > + if (!(kvmppc_get_msr(vcpu) & MSR_TM)) {
> > + kvmppc_trigger_fac_interrupt(vcpu, FSCR_TM_LG);
> > + emulated = EMULATE_AGAIN;
> > + break;
> > + }
> > +
> > + if (!(kvmppc_get_msr(vcpu) & MSR_PR)) {
> > + preempt_disable();
> > + vcpu->arch.cr = (CR0_TBEGIN_FAILURE |
> > + (vcpu->arch.cr & ~(CR0_MASK << CR0_SHIFT)));
> > +
> > + vcpu->arch.texasr = (TEXASR_FS | TEXASR_EX |
> > + (((u64)(TM_CAUSE_EMULATE |
> > TM_CAUSE_PERSISTENT))
> > +<< TEXASR_FC_LG));
> > +
> > + if ((inst >> 21) & 0x1)
> > + vcpu->arch.texasr |= TEXASR_ROT;
> > +
> > + if (kvmppc_get_msr(vcpu) & MSR_PR)
> > + vcpu->arch.texasr |= TEXASR_PR;
>
> This if statement seems unnecessary, since we only get here when
> MSR_PR is clear.
Yes. I will remove that.
Thanks,
- Simon
Re: [PATCH v2 17/30] KVM: PPC: Book3S PR: make mtspr/mfspr emulation behavior based on active TM SPRs
On Tue, May 15, 2018 at 04:07:03PM +1000, Paul Mackerras wrote:
> On Wed, Feb 28, 2018 at 01:52:25AM +0800, wei.guo.si...@gmail.com wrote:
> > From: Simon Guo
> >
> > The mfspr/mtspr on TM SPRs(TEXASR/TFIAR/TFHAR) are non-privileged
> > instructions and can be executed at PR KVM guest without trapping
> > into host in problem state. We only emulate mtspr/mfspr
> > texasr/tfiar/tfhar at guest PR=0 state.
> >
> > When we are emulating mtspr tm sprs at guest PR=0 state, the emulation
> > result need to be visible to guest PR=1 state. That is, the actual TM
> > SPR val should be loaded into actual registers.
> >
> > We already flush TM SPRs into vcpu when switching out of CPU, and load
> > TM SPRs when switching back.
> >
> > This patch corrects mfspr()/mtspr() emulation for TM SPRs to make the
> > actual source/dest based on actual TM SPRs.
> >
> > Signed-off-by: Simon Guo
> > ---
> > arch/powerpc/include/asm/kvm_book3s.h | 1 +
> > arch/powerpc/kvm/book3s_emulate.c | 54
> > ---
> > arch/powerpc/kvm/book3s_pr.c | 2 +-
> > 3 files changed, 46 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_book3s.h
> > b/arch/powerpc/include/asm/kvm_book3s.h
> > index 5911c3b..2ecb6a3 100644
> > --- a/arch/powerpc/include/asm/kvm_book3s.h
> > +++ b/arch/powerpc/include/asm/kvm_book3s.h
> > @@ -208,6 +208,7 @@ extern long kvmppc_hv_get_dirty_log_radix(struct kvm
> > *kvm,
> > extern void kvmppc_book3s_dequeue_irqprio(struct kvm_vcpu *vcpu,
> > unsigned int vec);
> > extern void kvmppc_inject_interrupt(struct kvm_vcpu *vcpu, int vec, u64
> > flags);
> > +extern void kvmppc_trigger_fac_interrupt(struct kvm_vcpu *vcpu, ulong fac);
> > extern void kvmppc_set_bat(struct kvm_vcpu *vcpu, struct kvmppc_bat *bat,
> >bool upper, u32 val);
> > extern void kvmppc_giveup_ext(struct kvm_vcpu *vcpu, ulong msr);
> > diff --git a/arch/powerpc/kvm/book3s_emulate.c
> > b/arch/powerpc/kvm/book3s_emulate.c
> > index 63af17f..a03533d 100644
> > --- a/arch/powerpc/kvm/book3s_emulate.c
> > +++ b/arch/powerpc/kvm/book3s_emulate.c
> > @@ -523,13 +523,35 @@ int kvmppc_core_emulate_mtspr_pr(struct kvm_vcpu
> > *vcpu, int sprn, ulong spr_val)
> > break;
> > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > case SPRN_TFHAR:
> > - vcpu->arch.tfhar = spr_val;
> > - break;
> > case SPRN_TEXASR:
> > - vcpu->arch.texasr = spr_val;
> > - break;
> > case SPRN_TFIAR:
> > - vcpu->arch.tfiar = spr_val;
> > + if (!cpu_has_feature(CPU_FTR_TM))
> > + break;
> > +
> > + if (!(kvmppc_get_msr(vcpu) & MSR_TM)) {
> > + kvmppc_trigger_fac_interrupt(vcpu, FSCR_TM_LG);
> > + emulated = EMULATE_AGAIN;
> > + break;
> > + }
> > +
> > + if (MSR_TM_ACTIVE(kvmppc_get_msr(vcpu))) {
> > + /* it is illegal to mtspr() TM regs in
> > +* other than non-transactional state.
> > +*/
> > + kvmppc_core_queue_program(vcpu, SRR1_PROGTM);
> > + emulated = EMULATE_AGAIN;
>
> According to the architecture, mtspr to TFHAR is permitted in
> suspended state.
oh.. I rescan the ISA and find the corresponding statement:
"If an attempt is made to execute mtspr specifying a TM
SPR in other than Non-transactional state, with the
exception of TFAR in suspended state, a TM Bad Thing
type Program interrupt is generated."
It mentiones "TFAR" instead of "TFHAR" -- So it looks a typo
in the ISA book. I will correct the code.
Thanks,
- Simon
Re: [PATCH v2 07/30] KVM: PPC: Book3S PR: add C function wrapper for _kvmppc_save/restore_tm()
On Tue, May 15, 2018 at 04:05:48PM +1000, Paul Mackerras wrote: > On Wed, Feb 28, 2018 at 01:37:14AM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > Currently _kvmppc_save/restore_tm() APIs can only be invoked from > > assembly function. This patch adds C function wrappers for them so > > that they can be safely called from C function. > > > > Signed-off-by: Simon Guo > > Some relatively minor comments below: > > > diff --git a/arch/powerpc/kvm/tm.S b/arch/powerpc/kvm/tm.S > > index 2d6fe5b..269dd11 100644 > > --- a/arch/powerpc/kvm/tm.S > > +++ b/arch/powerpc/kvm/tm.S > > @@ -35,7 +35,7 @@ > > * This can modify all checkpointed registers, but > > * restores r1, r2 before exit. > > */ > > -_GLOBAL(kvmppc_save_tm) > > +_GLOBAL(__kvmppc_save_tm) > > mflrr0 > > std r0, PPC_LR_STKOFF(r1) > > > > @@ -149,6 +149,52 @@ _GLOBAL(kvmppc_save_tm) > > blr > > > > /* > > + * _kvmppc_save_tm() is a wrapper around __kvmppc_save_tm(), so that it can > > + * be invoked from C function by PR KVM only. > > + */ > > +_GLOBAL(_kvmppc_save_tm_pr) > > The comment doesn't match the actual function name. I will correct that. Thanks for point it out. > > > + mflrr5 > > + std r5, PPC_LR_STKOFF(r1) > > + stdur1, -SWITCH_FRAME_SIZE(r1) > > + SAVE_NVGPRS(r1) > > + > > + /* save MSR since TM/math bits might be impacted > > +* by __kvmppc_save_tm(). > > +*/ > > + mfmsr r5 > > + SAVE_GPR(5, r1) > > + > > + /* also save DSCR/CR so that it can be recovered later */ > > + mfspr r6, SPRN_DSCR > > + SAVE_GPR(6, r1) > > + > > + mfcrr7 > > + stw r7, _CCR(r1) > > + > > + bl __kvmppc_save_tm > > + > > + ld r7, _CCR(r1) > > + mtcrr7 > > + > > + REST_GPR(6, r1) > > + mtspr SPRN_DSCR, r6 > > + > > + /* need preserve current MSR's MSR_TS bits */ > > + REST_GPR(5, r1) > > + mfmsr r6 > > + rldicl r6, r6, 64 - MSR_TS_S_LG, 62 > > + rldimi r5, r6, MSR_TS_S_LG, 63 - MSR_TS_T_LG > > + mtmsrd r5 > > + > > + REST_NVGPRS(r1) > > + addir1, r1, SWITCH_FRAME_SIZE > > + ld r5, PPC_LR_STKOFF(r1) > > + mtlrr5 > > + blr > > + > > +EXPORT_SYMBOL_GPL(_kvmppc_save_tm_pr); > > + > > +/* > > * Restore transactional state and TM-related registers. > > * Called with: > > * - r3 pointing to the vcpu struct. > > @@ -158,7 +204,7 @@ _GLOBAL(kvmppc_save_tm) > > * This potentially modifies all checkpointed registers. > > * It restores r1, r2 from the PACA. > > */ > > -_GLOBAL(kvmppc_restore_tm) > > +_GLOBAL(__kvmppc_restore_tm) > > mflrr0 > > std r0, PPC_LR_STKOFF(r1) > > > > @@ -186,6 +232,7 @@ _GLOBAL(kvmppc_restore_tm) > > rldicl. r5, r5, 64 - MSR_TS_S_LG, 62 > > beqlr /* TM not active in guest */ > > std r1, HSTATE_SCRATCH2(r13) > > + std r3, HSTATE_SCRATCH1(r13) > > Why do we need to save r3 here and restore it below? I think it is a legacy change when I did TM save/restore on each trap from guest to host. It can be removed now. > > > > > /* Make sure the failure summary is set, otherwise we'll program check > > * when we trechkpt. It's possible that this might have been not set > > @@ -262,6 +309,7 @@ _GLOBAL(kvmppc_restore_tm) > > ld r29, HSTATE_DSCR(r13) > > mtspr SPRN_DSCR, r29 > > #endif > > + ld r3, HSTATE_SCRATCH1(r13) > > ld r1, HSTATE_SCRATCH2(r13) > > ld r2, PACATMSCRATCH(r13) > > > > @@ -273,4 +321,47 @@ _GLOBAL(kvmppc_restore_tm) > > mtlrr0 > > blr > > > > +/* > > + * _kvmppc_restore_tm() is a wrapper around __kvmppc_restore_tm(), so that > > it > > + * can be invoked from C function by PR KVM only. > > + */ > > +_GLOBAL(_kvmppc_restore_tm_pr) > > Again, comment doesn't match the actual function name. I will correct that. Thanks, - Simon
Re: [PATCH v2 00/30] KVM: PPC: Book3S PR: Transaction memory support on PR KVM
On Tue, May 15, 2018 at 04:01:54PM +1000, Paul Mackerras wrote: > On Wed, Feb 28, 2018 at 01:37:07AM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > In current days, many OS distributions have utilized transaction > > memory functionality. In PowerPC, HV KVM supports TM. But PR KVM > > does not. > > > > The drive for the transaction memory support of PR KVM is the > > openstack Continuous Integration testing - They runs a HV(hypervisor) > > KVM(as level 1) and then run PR KVM(as level 2) on top of that. > > > > This patch set add transaction memory support on PR KVM. > > This patch set is starting to look pretty nice. I only have a few > comments on some of the patches. > > Unfortunately, these patches collide with the patches that have gone > upstream to add the hypervisor assistance for TM on POWER9, so some of > the patches will need to be rebased (in particular patches 2 and 4). Paul, Thanks for your kind guide/patience during developing this patch set. I will rebase and rework your comments. BR, - Simon
Re: [PATCH 11/11] KVM: PPC: reconstruct LOAD_VSX/STORE_VSX instruction mmio emulation with analyse_intr() input
On Thu, May 03, 2018 at 04:26:12PM +1000, Paul Mackerras wrote: > On Wed, Apr 25, 2018 at 07:54:44PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > This patch reconstructs LOAD_VSX/STORE_VSX instruction MMIO emulation with > > analyse_intr() input. It utilizes VSX_FPCONV/VSX_SPLAT/SIGNEXT exported > > by analyse_instr() and handle accordingly. > > > > When emulating VSX store, the VSX reg will need to be flushed so that > > the right reg val can be retrieved before writing to IO MEM. > > > > Suggested-by: Paul Mackerras > > Signed-off-by: Simon Guo > > Looks good, except that you shouldn't need the special case for > stxsiwx. With size=4 and element_size=8, kvmppc_handle_vsx_store > should just do the right thing, as far as I can see. Yes. Let me test after update. Thanks, - Simon
Re: [PATCH 10/11] KVM: PPC: reconstruct LOAD_VMX/STORE_VMX instruction mmio emulation with analyse_intr() input
On Thu, May 03, 2018 at 04:17:15PM +1000, Paul Mackerras wrote:
> On Wed, Apr 25, 2018 at 07:54:43PM +0800, wei.guo.si...@gmail.com wrote:
> > From: Simon Guo
> >
> > This patch reconstructs LOAD_VMX/STORE_VMX instruction MMIO emulation with
> > analyse_intr() input. When emulating the store, the VMX reg will need to
> > be flushed so that the right reg val can be retrieved before writing to
> > IO MEM.
> >
> > Suggested-by: Paul Mackerras
> > Signed-off-by: Simon Guo
>
> This looks fine for lvx and stvx, but now we are also doing something
> for the vector element loads and stores (lvebx, stvebx, lvehx, stvehx,
> etc.) without having the logic to insert or extract the correct
> element in the vector register image. We need either to generate an
> error for the element load/store instructions, or handle them
> correctly.
Yes. I will consider that.
>
> > diff --git a/arch/powerpc/kvm/emulate_loadstore.c
> > b/arch/powerpc/kvm/emulate_loadstore.c
> > index 2dbdf9a..0bfee2f 100644
> > --- a/arch/powerpc/kvm/emulate_loadstore.c
> > +++ b/arch/powerpc/kvm/emulate_loadstore.c
> > @@ -160,6 +160,27 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> > KVM_MMIO_REG_FPR|op.reg, size, 1);
> > break;
> > #endif
> > +#ifdef CONFIG_ALTIVEC
> > + case LOAD_VMX:
> > + if (kvmppc_check_altivec_disabled(vcpu))
> > + return EMULATE_DONE;
> > +
> > + /* VMX access will need to be size aligned */
>
> This comment isn't quite right; it isn't that the address needs to be
> size-aligned, it's that the hardware forcibly aligns it. So I would
> say something like /* Hardware enforces alignment of VMX accesses */.
>
I will update that.
> > + vcpu->arch.vaddr_accessed &= ~((unsigned long)size - 1);
> > + vcpu->arch.paddr_accessed &= ~((unsigned long)size - 1);
> > +
> > + if (size == 16) {
> > + vcpu->arch.mmio_vmx_copy_nums = 2;
> > + emulated = kvmppc_handle_load128_by2x64(run,
> > + vcpu, KVM_MMIO_REG_VMX|op.reg,
> > + 1);
> > + } else if (size <= 8)
> > + emulated = kvmppc_handle_load(run, vcpu,
> > + KVM_MMIO_REG_VMX|op.reg,
> > + size, 1);
> > +
> > + break;
> > +#endif
> > case STORE:
> > if (op.type & UPDATE) {
> > vcpu->arch.mmio_ra = op.update_reg;
> > @@ -197,6 +218,36 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> > VCPU_FPR(vcpu, op.reg), size, 1);
> > break;
> > #endif
> > +#ifdef CONFIG_ALTIVEC
> > + case STORE_VMX:
> > + if (kvmppc_check_altivec_disabled(vcpu))
> > + return EMULATE_DONE;
> > +
> > + /* VMX access will need to be size aligned */
> > + vcpu->arch.vaddr_accessed &= ~((unsigned long)size - 1);
> > + vcpu->arch.paddr_accessed &= ~((unsigned long)size - 1);
> > +
> > + /* if it is PR KVM, the FP/VEC/VSX registers need to
> > +* be flushed so that kvmppc_handle_store() can read
> > +* actual VMX vals from vcpu->arch.
> > +*/
> > + if (!is_kvmppc_hv_enabled(vcpu->kvm))
>
> As before, I suggest just testing that the function pointer isn't
> NULL.
Got it.
Thanks,
- Simon
Re: [PATCH 09/11] KVM: PPC: reconstruct LOAD_FP/STORE_FP instruction mmio emulation with analyse_intr() input
On Thu, May 03, 2018 at 04:10:49PM +1000, Paul Mackerras wrote: > On Wed, Apr 25, 2018 at 07:54:42PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > This patch reconstructs LOAD_FP/STORE_FP instruction MMIO emulation with > > analyse_intr() input. It utilizes the FPCONV/UPDATE properties exported by > > analyse_instr() and invokes kvmppc_handle_load(s)/kvmppc_handle_store() > > accordingly. > > > > The FP regs need to be flushed so that the right FP reg vals can be read > > from vcpu->arch.fpr. > > This only applies for store instructions; it would be clearer if you > said that explicitly. I will correct this message. > > > > > Suggested-by: Paul Mackerras > > Signed-off-by: Simon Guo > > Same comment about updating RA as for the other patches. Otherwise > this looks fine. > > Paul. Thanks, - Simon
Re: [PATCH 08/11] KVM: PPC: add giveup_ext() hook for PPC KVM ops
On Thu, May 03, 2018 at 04:08:17PM +1000, Paul Mackerras wrote:
> On Wed, Apr 25, 2018 at 07:54:41PM +0800, wei.guo.si...@gmail.com wrote:
> > From: Simon Guo
> >
> > Currently HV will save math regs(FP/VEC/VSX) when trap into host. But
> > PR KVM will only save math regs when qemu task switch out of CPU.
> >
> > To emulate FP/VEC/VSX load, PR KVM need to flush math regs firstly and
> > then be able to update saved VCPU FPR/VEC/VSX area reasonably.
> >
> > This patch adds the giveup_ext() to KVM ops (an empty one for HV KVM)
> > and kvmppc_complete_mmio_load() can invoke that hook to flush math
> > regs accordingly.
> >
> > Math regs flush is also necessary for STORE, which will be covered
> > in later patch within this patch series.
> >
> > Signed-off-by: Simon Guo
>
> I don't see where you have provided a function for Book E.
>
> I would suggest you only set the function pointer to non-NULL when the
> function is actually needed, i.e. for PR KVM.
Got it.
>
> It seems to me that this means that emulation of FP/VMX/VSX loads is
> currently broken for PR KVM for the case where kvm_io_bus_read() is
> able to supply the data, and the emulation of FP/VMX/VSX stores is
> broken for PR KVM for all cases. Do you agree?
>
Yes. I think so.
> > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > index 5b875ba..7eb5507 100644
> > --- a/arch/powerpc/kvm/book3s_hv.c
> > +++ b/arch/powerpc/kvm/book3s_hv.c
> > @@ -2084,6 +2084,10 @@ static int kvmhv_set_smt_mode(struct kvm *kvm,
> > unsigned long smt_mode,
> > return err;
> > }
> >
> > +static void kvmhv_giveup_ext(struct kvm_vcpu *vcpu, ulong msr)
> > +{
> > +}
> > +
> > static void unpin_vpa(struct kvm *kvm, struct kvmppc_vpa *vpa)
> > {
> > if (vpa->pinned_addr)
> > @@ -4398,6 +4402,7 @@ static int kvmhv_configure_mmu(struct kvm *kvm,
> > struct kvm_ppc_mmuv3_cfg *cfg)
> > .configure_mmu = kvmhv_configure_mmu,
> > .get_rmmu_info = kvmhv_get_rmmu_info,
> > .set_smt_mode = kvmhv_set_smt_mode,
> > + .giveup_ext = kvmhv_giveup_ext,
> > };
> >
> > static int kvm_init_subcore_bitmap(void)
>
> I think HV KVM could leave this pointer as NULL, and then...
ok.
>
> > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> > index 17f0315..e724601 100644
> > --- a/arch/powerpc/kvm/powerpc.c
> > +++ b/arch/powerpc/kvm/powerpc.c
> > @@ -1061,6 +1061,9 @@ static void kvmppc_complete_mmio_load(struct kvm_vcpu
> > *vcpu,
> > kvmppc_set_gpr(vcpu, vcpu->arch.io_gpr, gpr);
> > break;
> > case KVM_MMIO_REG_FPR:
> > + if (!is_kvmppc_hv_enabled(vcpu->kvm))
> > + vcpu->kvm->arch.kvm_ops->giveup_ext(vcpu, MSR_FP);
> > +
>
> This could become
> if (vcpu->kvm->arch.kvm_ops->giveup_ext)
> vcpu->kvm->arch.kvm_ops->giveup_ext(vcpu, MSR_FP);
>
> and you wouldn't need to fix Book E explicitly.
Yes
>
> Paul.
Thanks,
- Simon
Re: [PATCH 07/11] KVM: PPC: reconstruct non-SIMD LOAD/STORE instruction mmio emulation with analyse_intr() input
On Thu, May 03, 2018 at 04:03:46PM +1000, Paul Mackerras wrote:
> On Wed, Apr 25, 2018 at 07:54:40PM +0800, wei.guo.si...@gmail.com wrote:
> > From: Simon Guo
> >
> > This patch reconstructs non-SIMD LOAD/STORE instruction MMIO emulation
> > with analyse_intr() input. It utilizes the BYTEREV/UPDATE/SIGNEXT
> > properties exported by analyse_instr() and invokes
> > kvmppc_handle_load(s)/kvmppc_handle_store() accordingly.
> >
> > It also move CACHEOP type handling into the skeleton.
> >
> > instruction_type within sstep.h is renamed to avoid conflict with
> > kvm_ppc.h.
>
> I'd prefer to change the one in kvm_ppc.h, especially since that one
> isn't exactly about the type of instruction, but more about the type
> of interrupt led to us trying to fetch the instruction.
>
Agreed.
> > Suggested-by: Paul Mackerras
> > Signed-off-by: Simon Guo
> > ---
> > arch/powerpc/include/asm/sstep.h | 2 +-
> > arch/powerpc/kvm/emulate_loadstore.c | 282
> > +++
> > 2 files changed, 51 insertions(+), 233 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/sstep.h
> > b/arch/powerpc/include/asm/sstep.h
> > index ab9d849..0a1a312 100644
> > --- a/arch/powerpc/include/asm/sstep.h
> > +++ b/arch/powerpc/include/asm/sstep.h
> > @@ -23,7 +23,7 @@
> > #define IS_RFID(instr) (((instr) & 0xfc0007fe) == 0x4c24)
> > #define IS_RFI(instr) (((instr) & 0xfc0007fe) == 0x4c64)
> >
> > -enum instruction_type {
> > +enum analyse_instruction_type {
> > COMPUTE,/* arith/logical/CR op, etc. */
> > LOAD, /* load and store types need to be contiguous */
> > LOAD_MULTI,
> > diff --git a/arch/powerpc/kvm/emulate_loadstore.c
> > b/arch/powerpc/kvm/emulate_loadstore.c
> > index 90b9692..aaaf872 100644
> > --- a/arch/powerpc/kvm/emulate_loadstore.c
> > +++ b/arch/powerpc/kvm/emulate_loadstore.c
> > @@ -31,9 +31,12 @@
> > #include
> > #include
> > #include
> > +#include
> > #include "timing.h"
> > #include "trace.h"
> >
> > +int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> > + unsigned int instr);
>
> You shouldn't need this prototype here, since there's one in sstep.h.
>
Yes.
> > #ifdef CONFIG_PPC_FPU
> > static bool kvmppc_check_fp_disabled(struct kvm_vcpu *vcpu)
> > {
> > @@ -84,8 +87,9 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> > struct kvm_run *run = vcpu->run;
> > u32 inst;
> > int ra, rs, rt;
> > - enum emulation_result emulated;
> > + enum emulation_result emulated = EMULATE_FAIL;
> > int advance = 1;
> > + struct instruction_op op;
> >
> > /* this default type might be overwritten by subcategories */
> > kvmppc_set_exit_type(vcpu, EMULATED_INST_EXITS);
> > @@ -114,144 +118,64 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> > vcpu->arch.mmio_update_ra = 0;
> > vcpu->arch.mmio_host_swabbed = 0;
> >
> > - switch (get_op(inst)) {
> > - case 31:
> > - switch (get_xop(inst)) {
> > - case OP_31_XOP_LWZX:
> > - emulated = kvmppc_handle_load(run, vcpu, rt, 4, 1);
> > - break;
> > -
> > - case OP_31_XOP_LWZUX:
> > - emulated = kvmppc_handle_load(run, vcpu, rt, 4, 1);
> > - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> > - break;
> > -
> > - case OP_31_XOP_LBZX:
> > - emulated = kvmppc_handle_load(run, vcpu, rt, 1, 1);
> > - break;
> > + emulated = EMULATE_FAIL;
> > + vcpu->arch.regs.msr = vcpu->arch.shared->msr;
> > + vcpu->arch.regs.ccr = vcpu->arch.cr;
> > + if (analyse_instr(&op, &vcpu->arch.regs, inst) == 0) {
> > + int type = op.type & INSTR_TYPE_MASK;
> > + int size = GETSIZE(op.type);
> >
> > - case OP_31_XOP_LBZUX:
> > - emulated = kvmppc_handle_load(run, vcpu, rt, 1, 1);
> > - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> > - break;
> > + switch (type) {
> > + case LOAD: {
> > + int instr_byte_swap = op.type & BYTEREV;
> >
> > - case OP_31_XOP_STDX:
> > -
Re: [PATCH 04/11] KVM: PPC: fix incorrect element_size for stxsiwx in analyse_instr
On Thu, May 03, 2018 at 03:50:47PM +1000, Paul Mackerras wrote: > On Wed, Apr 25, 2018 at 07:54:37PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > stwsiwx will place contents of word element 1 of VSR into word > > storage of EA. So the element size of stwsiwx should be 4. > > > > This patch correct the size from 8 to 4. > > > > Signed-off-by: Simon Guo > > --- > > arch/powerpc/lib/sstep.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c > > index 34d68f1..151d484 100644 > > --- a/arch/powerpc/lib/sstep.c > > +++ b/arch/powerpc/lib/sstep.c > > @@ -2178,7 +2178,7 @@ int analyse_instr(struct instruction_op *op, const > > struct pt_regs *regs, > > case 140: /* stxsiwx */ > > op->reg = rd | ((instr & 1) << 5); > > op->type = MKOP(STORE_VSX, 0, 4); > > - op->element_size = 8; > > + op->element_size = 4; > > I made the element_size be 8 deliberately because this way, with > size=4 but element_size=8, the code will naturally choose the correct > word (the least-significant word of the left half) of the register to > store into memory. With this change you then need the special case in > a later patch for stxsiwx, which you shouldn't need if you don't make > this change. > > Paul. Thanks for point out. I will update accordingly. Thanks, - Simon
Re: [PATCH 05/11] KVM: PPC: add GPR RA update skeleton for MMIO emulation
On Thu, May 03, 2018 at 03:58:14PM +1000, Paul Mackerras wrote: > On Wed, Apr 25, 2018 at 07:54:38PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > To optimize kvm emulation code with analyse_instr, adds new > > mmio_update_ra flag to aid with GPR RA update. > > > > This patch arms RA update at load/store emulation path for both > > qemu mmio emulation or coalesced mmio emulation. > > It's not clear to me why you need to do this. The existing code > writes RA at the point where the instruction is decoded. In later > patches, you change that so the RA update occurs after the MMIO > operation is performed. Is there a particular reason why you made > that change? > > Paul. I wanted to avoid the case that GPR RA was updated even when EMULATE_FAIL. But if it is not mandatory, I can update RA in kvmppc_emulate_loadstore() instead.. Thanks, - Simon
Re: [PATCH 03/11] KVM: PPC: Fix a mmio_host_swabbed uninitialized usage issue when VMX store
On Thu, May 03, 2018 at 03:48:26PM +1000, Paul Mackerras wrote: > On Wed, Apr 25, 2018 at 07:54:36PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > When KVM emulates VMX store, it will invoke kvmppc_get_vmx_data() to > > retrieve VMX reg val. kvmppc_get_vmx_data() will check mmio_host_swabbed > > to decide which double word of vr[] to be used. But the > > mmio_host_swabbed can be uninitiazed during VMX store procedure: > > > > kvmppc_emulate_loadstore > > \- kvmppc_handle_store128_by2x64 > > \- kvmppc_get_vmx_data > > > > This patch corrects this by using kvmppc_need_byteswap() to choose > > double word of vr[] and initialized mmio_host_swabbed to avoid invisble > > trouble. > > > > Signed-off-by: Simon Guo > > The patch is correct, but I think the patch description needs to say > that vcpu->arch.mmio_host_swabbed is not meant to be used at all for > emulation of store instructions, and this patch makes that true for > VMX stores. I will revise the commit message accordingly. > > Paul. Thanks, - Simon
Re: [PATCH 02/11] KVM: PPC: mov nip/ctr/lr/xer registers to pt_regs in kvm_vcpu_arch
On Thu, May 03, 2018 at 03:46:01PM +1000, Paul Mackerras wrote: > On Wed, Apr 25, 2018 at 07:54:35PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > This patch moves nip/ctr/lr/xer registers from scattered places in > > kvm_vcpu_arch to pt_regs structure. > > > > cr register is "unsigned long" in pt_regs and u32 in vcpu->arch. > > It will need more consideration and may move in later patches. > > > > Signed-off-by: Simon Guo > > Mostly looks fine; some nits below. > > > diff --git a/arch/powerpc/kernel/asm-offsets.c > > b/arch/powerpc/kernel/asm-offsets.c > > index e8a78a5..731f7d4 100644 > > --- a/arch/powerpc/kernel/asm-offsets.c > > +++ b/arch/powerpc/kernel/asm-offsets.c > > @@ -431,14 +431,14 @@ int main(void) > > #ifdef CONFIG_ALTIVEC > > OFFSET(VCPU_VRS, kvm_vcpu, arch.vr.vr); > > #endif > > - OFFSET(VCPU_XER, kvm_vcpu, arch.xer); > > - OFFSET(VCPU_CTR, kvm_vcpu, arch.ctr); > > - OFFSET(VCPU_LR, kvm_vcpu, arch.lr); > > + OFFSET(VCPU_XER, kvm_vcpu, arch.regs.xer); > > + OFFSET(VCPU_CTR, kvm_vcpu, arch.regs.ctr); > > + OFFSET(VCPU_LR, kvm_vcpu, arch.regs.link); > > #ifdef CONFIG_PPC_BOOK3S > > OFFSET(VCPU_TAR, kvm_vcpu, arch.tar); > > #endif > > - OFFSET(VCPU_CR, kvm_vcpu, arch.cr); > > - OFFSET(VCPU_PC, kvm_vcpu, arch.nip); > > This should be arch.pc; arch.nip doesn't exist. Yes. That was introduced during patch split. I will correct them. > > > + OFFSET(VCPU_CR, kvm_vcpu, arch.regs.ccr); > > I thought the patch description said you weren't moving CR at this > stage? Sorry about that. thanks for pointing out. > > > + OFFSET(VCPU_PC, kvm_vcpu, arch.regs.nip); > > #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE > > OFFSET(VCPU_MSR, kvm_vcpu, arch.shregs.msr); > > OFFSET(VCPU_SRR0, kvm_vcpu, arch.shregs.srr0); > > @@ -693,11 +693,11 @@ int main(void) > > #endif /* CONFIG_PPC_BOOK3S_64 */ > > > > #else /* CONFIG_PPC_BOOK3S */ > > - OFFSET(VCPU_CR, kvm_vcpu, arch.cr); > > - OFFSET(VCPU_XER, kvm_vcpu, arch.xer); > > - OFFSET(VCPU_LR, kvm_vcpu, arch.lr); > > - OFFSET(VCPU_CTR, kvm_vcpu, arch.ctr); > > - OFFSET(VCPU_PC, kvm_vcpu, arch.pc); > > + OFFSET(VCPU_CR, kvm_vcpu, arch.regs.ccr); > > Once again VCPU_CR should not be changed. Yep. > > > + OFFSET(VCPU_XER, kvm_vcpu, arch.regs.xer); > > + OFFSET(VCPU_LR, kvm_vcpu, arch.regs.link); > > + OFFSET(VCPU_CTR, kvm_vcpu, arch.regs.ctr); > > + OFFSET(VCPU_PC, kvm_vcpu, arch.regs.nip); > > OFFSET(VCPU_SPRG9, kvm_vcpu, arch.sprg9); > > OFFSET(VCPU_LAST_INST, kvm_vcpu, arch.last_inst); > > OFFSET(VCPU_FAULT_DEAR, kvm_vcpu, arch.fault_dear); > > Paul. Thanks, - Simon
Re: [PATCH 01/11] KVM: PPC: add pt_regs into kvm_vcpu_arch and move vcpu->arch.gpr[] into it
On Thu, May 03, 2018 at 03:34:01PM +1000, Paul Mackerras wrote: > On Wed, Apr 25, 2018 at 07:54:34PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > Current regs are scattered at kvm_vcpu_arch structure and it will > > be more neat to organize them into pt_regs structure. > > > > Also it will enable reconstruct MMIO emulation code with > > "reimplement" would be clearer than "reconstruct" here, I think. > ok. > > @@ -438,7 +438,7 @@ int main(void) > > OFFSET(VCPU_TAR, kvm_vcpu, arch.tar); > > #endif > > OFFSET(VCPU_CR, kvm_vcpu, arch.cr); > > - OFFSET(VCPU_PC, kvm_vcpu, arch.pc); > > + OFFSET(VCPU_PC, kvm_vcpu, arch.nip); > > This hunk shouldn't be in this patch. Yes. sorry my patch split has some issue in patch 00/01. > > Paul. Thanks, - Simon
Re: [PATCH 00/11] KVM: PPC: reconstruct mmio emulation with analyse_instr()
On Thu, May 03, 2018 at 03:31:17PM +1000, Paul Mackerras wrote: > On Wed, Apr 25, 2018 at 07:54:33PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > We already have analyse_instr() which analyzes instructions for the > > instruction > > type, size, addtional flags, etc. What kvmppc_emulate_loadstore() did is > > somehow > > duplicated and it will be good to utilize analyse_instr() to reconstruct the > > code. The advantage is that the code logic will be shared and more clean to > > be > > maintained. > > > > This patch series reconstructs kvmppc_emulate_loadstore() for various > > load/store > > instructions. > > > > The testcase locates at: > > https://github.com/justdoitqd/publicFiles/blob/master/test_mmio.c > > > > - Tested at both PR/HV KVM. > > - Also tested with little endian host & big endian guest. > > > > Tested instruction list: > > lbz lbzu lbzx ld ldbrx > > ldu ldx lfd lfdu lfdx > > lfiwax lfiwzx lfs lfsu lfsx > > lha lhau lhax lhbrx lhz > > lhzu lhzx lvx lwax lwbrx > > lwz lwzu lwzx lxsdx lxsiwax > > lxsiwzx lxsspx lxvd2x lxvdsx lxvw4x > > stb stbu stbx std stdbrx > > stdu stdx stfd stfdu stfdx > > stfiwx stfs stfsx sth sthbrx > > sthu sthx stvx stw stwbrx > > stwu stwx stxsdx stxsiwx stxsspx > > stxvd2x stxvw4x > > Thanks for doing this. It's nice to see that this makes the code 260 > lines smaller. > > I have some comments on the individual patches, which I will give in > replies to those patches. > > Paul. Thanks for those good comment. I will check them. BR, - Simon
Re: [PATCH 01/11] KVM: PPC: add pt_regs into kvm_vcpu_arch and move vcpu->arch.gpr[] into it
On Fri, Apr 27, 2018 at 11:47:21AM +0800, kbuild test robot wrote:
> Hi Simon,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on powerpc/next]
> [also build test ERROR on v4.17-rc2 next-20180426]
> [if your patch is applied to the wrong git tree, please drop us a note to
> help improve the system]
>
> url:
> https://github.com/0day-ci/linux/commits/wei-guo-simon-gmail-com/KVM-PPC-add-pt_regs-into-kvm_vcpu_arch-and-move-vcpu-arch-gpr-into-it/20180427-055410
> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> config: powerpc-defconfig (attached as .config)
> compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
> wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=powerpc
>
> Note: the
> linux-review/wei-guo-simon-gmail-com/KVM-PPC-add-pt_regs-into-kvm_vcpu_arch-and-move-vcpu-arch-gpr-into-it/20180427-055410
> HEAD 92a7de2f1920f80f57d625d6d07731a00ea99161 builds fine.
> It only hurts bisectibility.
>
> All error/warnings (new ones prefixed by >>):
>
>In file included from arch/powerpc/include/asm/kvm_book3s.h:271:0,
> from arch/powerpc/kernel/asm-offsets.c:57:
>arch/powerpc/include/asm/kvm_book3s_64.h: In function
> 'copy_from_checkpoint':
> >> arch/powerpc/include/asm/kvm_book3s_64.h:493:20: error: 'struct
> >> kvm_vcpu_arch' has no member named 'gpr'; did you mean 'qpr'?
> memcpy(vcpu->arch.gpr, vcpu->arch.gpr_tm,
>^~~
>qpr
>arch/powerpc/include/asm/kvm_book3s_64.h:494:27: error: 'struct
> kvm_vcpu_arch' has no member named 'gpr'; did you mean 'qpr'?
> sizeof(vcpu->arch.gpr));
> ^~~
> qpr
>arch/powerpc/include/asm/kvm_book3s_64.h: In function 'copy_to_checkpoint':
>arch/powerpc/include/asm/kvm_book3s_64.h:510:39: error: 'struct
> kvm_vcpu_arch' has no member named 'gpr'; did you mean 'qpr'?
> memcpy(vcpu->arch.gpr_tm, vcpu->arch.gpr,
> ^~~
> qpr
>arch/powerpc/include/asm/kvm_book3s_64.h:511:27: error: 'struct
> kvm_vcpu_arch' has no member named 'gpr'; did you mean 'qpr'?
> sizeof(vcpu->arch.gpr));
> ^~~
> qpr
>In file included from arch/powerpc/kernel/asm-offsets.c:30:0:
>arch/powerpc/kernel/asm-offsets.c: In function 'main':
> >> include/linux/compiler-gcc.h:170:2: error: 'struct kvm_vcpu_arch' has no
> >> member named 'nip'
> __builtin_offsetof(a, b)
> ^
>include/linux/kbuild.h:6:62: note: in definition of macro 'DEFINE'
> asm volatile("\n.ascii \"->" #sym " %0 " #val "\"" : : "i" (val))
> ^~~
>include/linux/stddef.h:17:32: note: in expansion of macro
> '__compiler_offsetof'
> #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
>^~~
> >> include/linux/kbuild.h:11:14: note: in expansion of macro 'offsetof'
> DEFINE(sym, offsetof(struct str, mem))
> ^~~~
> >> arch/powerpc/kernel/asm-offsets.c:441:2: note: in expansion of macro
> >> 'OFFSET'
> OFFSET(VCPU_PC, kvm_vcpu, arch.nip);
> ^~
>make[2]: *** [arch/powerpc/kernel/asm-offsets.s] Error 1
>make[2]: Target '__build' not remade because of errors.
>make[1]: *** [prepare0] Error 2
>make[1]: Target 'prepare' not remade because of errors.
>make: *** [sub-make] Error 2
>
> vim +493 arch/powerpc/include/asm/kvm_book3s_64.h
>
> 4bb3c7a0 Paul Mackerras 2018-03-21 481
> 4bb3c7a0 Paul Mackerras 2018-03-21 482 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> 4bb3c7a0 Paul Mackerras 2018-03-21 483 static inline void
> copy_from_checkpoint(struct kvm_vcpu *vcpu)
> 4bb3c7a0 Paul Mackerras 2018-03-21 484 {
> 4bb3c7a0 Paul Mackerras 2018-03-21 485 vcpu->arch.cr =
> vcpu->arch.cr_tm;
> 4bb3c7a0 Paul Mackerras 2018-03-21 486 vcpu->arch.xer =
> vcpu->arch.xer_tm;
> 4bb3c7a0 Paul Mackerras 2018-03-21 487 vcpu->arch.lr =
> vcpu->arch.lr_tm;
> 4bb3c7a0 Paul Mackerras 2018-03-21 488 vcpu->arch.ctr =
> vcpu->arch.ctr_tm;
> 4bb3c7a0 Paul Mackerras 2018-03-21 489 vcpu->arch.amr =
> vcpu->arch.amr_tm;
> 4bb3c7a0 Paul Mackerras 2018-03-21 490 vcpu->arch.ppr =
> vcpu->arch.ppr_tm;
> 4bb3c7a0 Paul Mackerras 2018-03-21 491 vcpu->arch.dscr =
> vcpu->arch.dscr_tm;
> 4bb3c7a0 Paul Mackerras 2018-03-21 492 vcpu->arch.tar =
> vcpu->arch.tar_tm;
> 4bb3c7a0 Paul Mackerras 2018-03-21 @493 memcpy(vcpu->arch.gpr,
> vcpu->arch.gpr_tm,
> 4bb3c7a0 Paul Mackerras 2018-03-21 494 sizeo
Re: [PATCH] KVM: PPC: Book3S PR: close a race window when SVCPU pointer is hold before kvmppc_copy_from_svcpu()
Hi Alex,
On Wed, Jan 31, 2018 at 10:28:05AM +0100, Alexander Graf wrote:
>
>
> On 31.01.18 05:23, wei.guo.si...@gmail.com wrote:
> > From: Simon Guo
> >
> > commit 40fdd8c88c4a ("KVM: PPC: Book3S: PR: Make svcpu -> vcpu store
> > preempt savvy") and commit 3d3319b45eea ("KVM: PPC: Book3S: PR: Enable
> > interrupts earlier") is trying to turns on preemption early when
> > return into highmem guest exit handler.
> >
> > However there is a race window in following example at
> > arch/powerpc/kvm/book3s_interrupts.S:
> >
> > highmem guest exit handler:
> > ...
> > 195 GET_SHADOW_VCPU(r4)
> > 196 bl FUNC(kvmppc_copy_from_svcpu)
> > ...
> > 239 bl FUNC(kvmppc_handle_exit_pr)
> >
> > If there comes a preemption between line 195 and 196, line 196
> > may hold an invalid SVCPU reference with following sequence:
> > 1) Qemu task T1 runs at GET_SHADOW_VCPU(r4) at line 195, on CPU A.
> > 2) T1 is preempted and switch out CPU A. As a result, it checks
> > CPU A's svcpu->in_use (=1 at present) and flush cpu A's svcpu to
> > T1's vcpu.
> > 3) Another task T2 switches into CPU A and it may update CPU A's
> > svcpu->in_use into 1.
> > 4) T1 is scheduled into CPU B. But it still holds CPU A's svcpu
> > reference as R4. Then it executes kvmppc_copy_from_svcpu() with
> > R4 and it will corrupt T1's VCPU with T2's content. T2's VCPU
> > will also be impacted.
> >
> > This patch moves the svcpu->in_use into VCPU so that the vcpus
> > sharing the same svcpu can work properly and fix the above case.
> >
> > Signed-off-by: Simon Guo
>
> Sorry, the previous version would only compile on 32bit PPC ;). Please
> find the fixed one which just uses svcpu_get() and _put() here:
>
>
> https://github.com/agraf/linux-2.6/commit/f9e3ca44c9a9d4930d6dccaacb518734746059c3
>
>
> Alex
Your solution looks better than mine :)
Unfortunately somehow I cannot reproduce my issue without the fix. So
I cannot test it currently.
Reviewed-by: Simon Guo
Thanks,
- Simon
Re: [PATCH 25/26] KVM: PPC: Book3S PR: Support TAR handling for PR KVM HTM.
Hi Paul, On Wed, Jan 24, 2018 at 03:02:58PM +1100, Paul Mackerras wrote: > On Thu, Jan 11, 2018 at 06:11:38PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > Currently guest kernel doesn't handle TAR fac unavailable and it always > > runs with TAR bit on. PR KVM will lazily enable TAR. TAR is not a > > frequent-use reg and it is not included in SVCPU struct. > > > > To make it work for transaction memory at PR KVM: > > 1). Flush/giveup TAR at kvmppc_save_tm_pr(). > > 2) If we are receiving a TAR fac unavail exception inside a transaction, > > the checkpointed TAR might be a TAR value from another process. So we need > > treclaim the transaction, then load the desired TAR value into reg, and > > perform trecheckpoint. > > 3) Load TAR facility at kvmppc_restore_tm_pr() when TM active. > > The reason we always loads TAR when restoring TM is that: > > If we don't do this way, when there is a TAR fac unavailable exception > > during TM active: > > case 1: it is the 1st TAR fac unavail exception after tbegin. > > vcpu->arch.tar should be reloaded as checkpoint tar val. > > case 2: it is the 2nd or later TAR fac unavail exception after tbegin. > > vcpu->arch.tar_tm should be reloaded as checkpoint tar val. > > There will be unnecessary difficulty to handle the above 2 cases. > > > > at the end of emulating treclaim., the correct TAR val need to be loaded > > into reg if FSCR_TAR bit is on. > > at the beginning of emulating trechkpt., TAR needs to be flushed so that > > the right tar val can be copy into tar_tm. > > Would it be simpler always to load up TAR when guest_MSR[TM] is 1? > > Paul. Sure. it will have a similar solution with math regs. Thanks for the suggestion, BR - Simon
Re: [PATCH 23/26] KVM: PPC: Book3S PR: add emulation for tabort. for privilege guest
Hi Paul, On Tue, Jan 23, 2018 at 08:44:16PM +1100, Paul Mackerras wrote: > On Thu, Jan 11, 2018 at 06:11:36PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > Currently privilege guest will be run with TM disabled. > > > > Although the privilege guest cannot initiate a new transaction, > > it can use tabort to terminate its problem state's transaction. > > So it is still necessary to emulate tabort. for privilege guest. > > > > This patch adds emulation for tabort. of privilege guest. > > > > Tested with: > > https://github.com/justdoitqd/publicFiles/blob/master/test_tabort.c > > > > Signed-off-by: Simon Guo > > --- > > arch/powerpc/include/asm/kvm_book3s.h | 1 + > > arch/powerpc/kvm/book3s_emulate.c | 31 +++ > > arch/powerpc/kvm/book3s_pr.c | 2 +- > > 3 files changed, 33 insertions(+), 1 deletion(-) > > > > diff --git a/arch/powerpc/include/asm/kvm_book3s.h > > b/arch/powerpc/include/asm/kvm_book3s.h > > index 524cd82..8bd454c 100644 > > --- a/arch/powerpc/include/asm/kvm_book3s.h > > +++ b/arch/powerpc/include/asm/kvm_book3s.h > > @@ -258,6 +258,7 @@ extern void kvmppc_copy_from_svcpu(struct kvm_vcpu > > *vcpu, > > void kvmppc_save_tm_pr(struct kvm_vcpu *vcpu); > > void kvmppc_restore_tm_pr(struct kvm_vcpu *vcpu); > > void kvmppc_restore_tm_sprs(struct kvm_vcpu *vcpu); > > +void kvmppc_save_tm_sprs(struct kvm_vcpu *vcpu); > > Why do you add this declaration, and change it from "static inline" to > "inline" below, when this patch doesn't use it? Also, making it > "inline" is pointless if it has a caller outside the source file where > it's defined (if gcc wants to inline uses of it inside the same source > file, it will do so anyway even without the "inline" keyword.) > > Paul. It is a leave over of my previous rework. Sorry and I will remove them. Thanks, - Simon
Re: [PATCH 21/26] KVM: PPC: Book3S PR: adds emulation for treclaim.
Hi Paul,
On Tue, Jan 23, 2018 at 08:23:23PM +1100, Paul Mackerras wrote:
> On Thu, Jan 11, 2018 at 06:11:34PM +0800, wei.guo.si...@gmail.com wrote:
> > From: Simon Guo
> >
> > This patch adds support for "treclaim." emulation when PR KVM guest
> > executes treclaim. and traps to host.
> >
> > We will firstly doing treclaim. and save TM checkpoint and doing
> > treclaim. Then it is necessary to update vcpu current reg content
> > with checkpointed vals. When rfid into guest again, those vcpu
> > current reg content(now the checkpoint vals) will be loaded into
> > regs.
> >
> > Signed-off-by: Simon Guo
> > ---
> > arch/powerpc/include/asm/reg.h| 4 +++
> > arch/powerpc/kvm/book3s_emulate.c | 66
> > ++-
> > 2 files changed, 69 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> > index 6c293bc..b3bcf6b 100644
> > --- a/arch/powerpc/include/asm/reg.h
> > +++ b/arch/powerpc/include/asm/reg.h
> > @@ -244,12 +244,16 @@
> > #define SPRN_TEXASR0x82/* Transaction EXception & Summary */
> > #define SPRN_TEXASRU 0x83/* '' '' ''Upper 32 */
> > #define TEXASR_FC_LG (63 - 7)/* Failure Code */
> > +#define TEXASR_AB_LG (63 - 31) /* Abort */
> > +#define TEXASR_SU_LG (63 - 32) /* Suspend */
> > #define TEXASR_HV_LG (63 - 34) /* Hypervisor state*/
> > #define TEXASR_PR_LG (63 - 35) /* Privilege level */
> > #define TEXASR_FS_LG (63 - 36) /* failure summary */
> > #define TEXASR_EX_LG (63 - 37) /* TFIAR exact bit */
> > #define TEXASR_ROT_LG (63 - 38) /* ROT bit */
> > #define TEXASR_FC (ASM_CONST(0xFF) << TEXASR_FC_LG)
> > +#define TEXASR_AB __MASK(TEXASR_AB_LG)
> > +#define TEXASR_SU __MASK(TEXASR_SU_LG)
> > #define TEXASR_HV __MASK(TEXASR_HV_LG)
> > #define TEXASR_PR __MASK(TEXASR_PR_LG)
> > #define TEXASR_FS __MASK(TEXASR_FS_LG)
>
> It would be good to collect up all the modifications you need to make
> to reg.h into a single patch at the beginning of the patch series --
> that will make it easier to merge it all.
>
OK.
> > diff --git a/arch/powerpc/kvm/book3s_emulate.c
> > b/arch/powerpc/kvm/book3s_emulate.c
> > index 1eb1900..51c0e20 100644
> > --- a/arch/powerpc/kvm/book3s_emulate.c
> > +++ b/arch/powerpc/kvm/book3s_emulate.c
>
> [snip]
>
> > @@ -127,6 +130,42 @@ void kvmppc_copyfrom_vcpu_tm(struct kvm_vcpu *vcpu)
> > vcpu->arch.vrsave = vcpu->arch.vrsave_tm;
> > }
> >
> > +static void kvmppc_emulate_treclaim(struct kvm_vcpu *vcpu, int ra_val)
> > +{
> > + unsigned long guest_msr = kvmppc_get_msr(vcpu);
> > + int fc_val = ra_val ? ra_val : 1;
> > +
> > + kvmppc_save_tm_pr(vcpu);
> > +
> > + preempt_disable();
> > + kvmppc_copyfrom_vcpu_tm(vcpu);
> > + preempt_enable();
> > +
> > + /*
> > +* treclaim need quit to non-transactional state.
> > +*/
> > + guest_msr &= ~(MSR_TS_MASK);
> > + kvmppc_set_msr(vcpu, guest_msr);
> > +
> > + preempt_disable();
> > + tm_enable();
> > + vcpu->arch.texasr = mfspr(SPRN_TEXASR);
> > + vcpu->arch.texasr &= ~TEXASR_FC;
> > + vcpu->arch.texasr |= ((u64)fc_val << TEXASR_FC_LG);
>
> You're doing failure recording here unconditionally, but the
> architecture says that treclaim. only does failure recording if
> TEXASR_FS is not already set.
>
I need add that. And the CR0 setting is also missed.
Thanks for the catch.
[snip]
BR,
- Simon
Re: [PATCH 22/26] KVM: PPC: Book3S PR: add emulation for trechkpt in PR KVM.
Hi Paul,
On Tue, Jan 23, 2018 at 08:36:44PM +1100, Paul Mackerras wrote:
> On Thu, Jan 11, 2018 at 06:11:35PM +0800, wei.guo.si...@gmail.com wrote:
> > From: Simon Guo
> >
> > This patch adds host emulation when guest PR KVM executes "trechkpt.",
> > which is a privileged instruction and will trap into host.
> >
> > We firstly copy vcpu ongoing content into vcpu tm checkpoint
> > content, then perform kvmppc_restore_tm_pr() to do trechkpt.
> > with updated vcpu tm checkpoint vals.
> >
> > Signed-off-by: Simon Guo
>
> [snip]
>
> > +static void kvmppc_emulate_trchkpt(struct kvm_vcpu *vcpu)
> > +{
> > + unsigned long guest_msr = kvmppc_get_msr(vcpu);
> > +
> > + preempt_disable();
> > + vcpu->arch.save_msr_tm = MSR_TS_S;
> > + vcpu->arch.save_msr_tm &= ~(MSR_FP | MSR_VEC | MSR_VSX);
>
> This looks odd, since you are clearing bits when you have just set
> save_msr_tm to a constant value that doesn't have these bits set.
> This could be taken as a sign that the previous line has a bug and you
> meant "|=" or something similar instead of "=". I think you probably
> did mean "=", in which case you should remove the line clearing
> FP/VEC/VSX.
I will rework and remove "save_msr_tm" from the code.
Thanks,
- Simon
Re: [PATCH 19/26] KVM: PPC: Book3S PR: always fail transaction in guest privilege state
Hi Paul, On Tue, Jan 23, 2018 at 07:30:33PM +1100, Paul Mackerras wrote: > On Thu, Jan 11, 2018 at 06:11:32PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > Currently kernel doesn't use transaction memory. > > And there is an issue for privilege guest that: > > tbegin/tsuspend/tresume/tabort TM instructions can impact MSR TM bits > > without trap into PR host. So following code will lead to a false mfmsr > > result: > > tbegin <- MSR bits update to Transaction active. > > beq <- failover handler branch > > mfmsr <- still read MSR bits from magic page with > > transaction inactive. > > > > It is not an issue for non-privilege guest since its mfmsr is not patched > > with magic page and will always trap into PR host. > > > > This patch will always fail tbegin attempt for privilege guest, so that > > the above issue is prevented. It is benign since currently (guest) kernel > > doesn't initiate a transaction. > > > > Test case: > > https://github.com/justdoitqd/publicFiles/blob/master/test_tbegin_pr.c > > > > Signed-off-by: Simon Guo > > You need to handle the case where MSR_TM is not set in the guest MSR, > and give the guest a facility unavailable interrupt. Thanks for the catch. > > [snip] > > > --- a/arch/powerpc/kvm/book3s_pr.c > > +++ b/arch/powerpc/kvm/book3s_pr.c > > @@ -255,7 +255,7 @@ static inline void kvmppc_save_tm_sprs(struct kvm_vcpu > > *vcpu) > > tm_disable(); > > } > > > > -static inline void kvmppc_restore_tm_sprs(struct kvm_vcpu *vcpu) > > +inline void kvmppc_restore_tm_sprs(struct kvm_vcpu *vcpu) > > You should probably remove the 'inline' here too. OK. BR, - Simon
Re: [PATCH 18/26] KVM: PPC: Book3S PR: make mtspr/mfspr emulation behavior based on active TM SPRs
Hi Paul,
On Tue, Jan 23, 2018 at 07:17:45PM +1100, Paul Mackerras wrote:
> On Thu, Jan 11, 2018 at 06:11:31PM +0800, wei.guo.si...@gmail.com wrote:
> > From: Simon Guo
> >
> > The mfspr/mtspr on TM SPRs(TEXASR/TFIAR/TFHAR) are non-privileged
> > instructions and can be executed at PR KVM guest without trapping
> > into host in problem state. We only emulate mtspr/mfspr
> > texasr/tfiar/tfhar at guest PR=0 state.
> >
> > When we are emulating mtspr tm sprs at guest PR=0 state, the emulation
> > result need to be visible to guest PR=1 state. That is, the actual TM
> > SPR val should be loaded into actual registers.
> >
> > We already flush TM SPRs into vcpu when switching out of CPU, and load
> > TM SPRs when switching back.
> >
> > This patch corrects mfspr()/mtspr() emulation for TM SPRs to make the
> > actual source/dest based on actual TM SPRs.
> >
> > Signed-off-by: Simon Guo
> > ---
> > arch/powerpc/kvm/book3s_emulate.c | 35 +++
> > 1 file changed, 27 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_emulate.c
> > b/arch/powerpc/kvm/book3s_emulate.c
> > index e096d01..c2836330 100644
> > --- a/arch/powerpc/kvm/book3s_emulate.c
> > +++ b/arch/powerpc/kvm/book3s_emulate.c
> > @@ -521,13 +521,26 @@ int kvmppc_core_emulate_mtspr_pr(struct kvm_vcpu
> > *vcpu, int sprn, ulong spr_val)
> > break;
> > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > case SPRN_TFHAR:
> > - vcpu->arch.tfhar = spr_val;
> > - break;
> > case SPRN_TEXASR:
> > - vcpu->arch.texasr = spr_val;
> > - break;
> > case SPRN_TFIAR:
> > - vcpu->arch.tfiar = spr_val;
> > + if (MSR_TM_ACTIVE(kvmppc_get_msr(vcpu))) {
> > + /* it is illegal to mtspr() TM regs in
> > +* other than non-transactional state.
> > +*/
> > + kvmppc_core_queue_program(vcpu, SRR1_PROGTM);
> > + emulated = EMULATE_AGAIN;
> > + break;
> > + }
>
> We also need to check that the guest has TM enabled in the guest MSR,
> and give them a facility unavailable interrupt if not.
>
> > +
> > + tm_enable();
> > + if (sprn == SPRN_TFHAR)
> > + mtspr(SPRN_TFHAR, spr_val);
> > + else if (sprn == SPRN_TEXASR)
> > + mtspr(SPRN_TEXASR, spr_val);
> > + else
> > + mtspr(SPRN_TFIAR, spr_val);
> > + tm_disable();
>
> I haven't seen any checks that we are on a CPU that has TM. What
> happens if a guest does a mtmsrd with TM=1 and then a mtspr to TEXASR
> when running on a POWER7 (assuming the host kernel was compiled with
> CONFIG_PPC_TRANSACTIONAL_MEM=y)?
>
> Ideally, if the host CPU does not have TM functionality, these mtsprs
> would be treated as no-ops and attempts to set the TM or TS fields in
> the guest MSR would be ignored.
>
> > +
> > break;
> > #endif
> > #endif
> > @@ -674,13 +687,19 @@ int kvmppc_core_emulate_mfspr_pr(struct kvm_vcpu
> > *vcpu, int sprn, ulong *spr_val
> > break;
> > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > case SPRN_TFHAR:
> > - *spr_val = vcpu->arch.tfhar;
> > + tm_enable();
> > + *spr_val = mfspr(SPRN_TFHAR);
> > + tm_disable();
> > break;
> > case SPRN_TEXASR:
> > - *spr_val = vcpu->arch.texasr;
> > + tm_enable();
> > + *spr_val = mfspr(SPRN_TEXASR);
> > + tm_disable();
> > break;
> > case SPRN_TFIAR:
> > - *spr_val = vcpu->arch.tfiar;
> > + tm_enable();
> > + *spr_val = mfspr(SPRN_TFIAR);
> > + tm_disable();
> > break;
>
> These need to check MSR_TM in the guest MSR, and become no-ops on
> machines without TM capability.
Thanks for the above catches. I will rework later.
BR,
- Simon
Re: [PATCH 17/26] KVM: PPC: Book3S PR: add math support for PR KVM HTM
Hi Paul, On Tue, Jan 23, 2018 at 06:29:27PM +1100, Paul Mackerras wrote: > On Thu, Jan 11, 2018 at 06:11:30PM +0800, wei.guo.si...@gmail.com wrote: > > ines: 219 > > > > From: Simon Guo > > > > The math registers will be saved into vcpu->arch.fp/vr and corresponding > > vcpu->arch.fp_tm/vr_tm area. > > > > We flush or giveup the math regs into vcpu->arch.fp/vr before saving > > transaction. After transaction is restored, the math regs will be loaded > > back into regs. > > It looks to me that you are loading up the math regs on every vcpu > load, not just those with an active transaction. That seems like > overkill. > > > If there is a FP/VEC/VSX unavailable exception during transaction active > > state, the math checkpoint content might be incorrect and we need to do > > treclaim./load the correct checkpoint val/trechkpt. sequence to retry the > > transaction. > > I would prefer a simpler approach where just before entering the > guest, we check if the guest MSR TM bit is set, and if so we make sure > that whichever math regs are enabled in the guest MSR are actually > loaded on the CPU, that is, that guest_owned_ext has the same bits set > as the guest MSR. Then we never have to handle a FP/VEC/VSX > unavailable interrupt with a transaction active (other than by simply > passing it on to the guest). Good idea. I will rework as this way. Thanks, - Simon
Re: [PATCH 16/26] KVM: PPC: Book3S PR: add transaction memory save/restore skeleton for PR KVM
Hi Paul,
On Tue, Jan 23, 2018 at 05:04:09PM +1100, Paul Mackerras wrote:
> On Thu, Jan 11, 2018 at 06:11:29PM +0800, wei.guo.si...@gmail.com wrote:
> > From: Simon Guo
> >
> > The transaction memory checkpoint area save/restore behavior is
> > triggered when VCPU qemu process is switching out/into CPU. ie.
> > at kvmppc_core_vcpu_put_pr() and kvmppc_core_vcpu_load_pr().
> >
> > MSR TM active state is determined by TS bits:
> > active: 10(transactional) or 01 (suspended)
> > inactive: 00 (non-transactional)
> > We don't "fake" TM functionality for guest. We "sync" guest virtual
> > MSR TM active state(10 or 01) with shadow MSR. That is to say,
> > we don't emulate a transactional guest with a TM inactive MSR.
> >
> > TM SPR support(TFIAR/TFAR/TEXASR) has already been supported by
> > commit 9916d57e64a4 ("KVM: PPC: Book3S PR: Expose TM registers").
> > Math register support (FPR/VMX/VSX) will be done at subsequent
> > patch.
> >
> > - TM save:
> > When kvmppc_save_tm_pr() is invoked, whether TM context need to
> > be saved can be determined by current host MSR state:
> > * TM active - save TM context
> > * TM inactive - no need to do so and only save TM SPRs.
> >
> > - TM restore:
> > However when kvmppc_restore_tm_pr() is invoked, there is an
> > issue to determine whether TM restore should be performed.
> > The TM active host MSR val saved in kernel stack is not loaded yet.
>
> I don't follow this exactly. What is the value saved on the kernel
> stack?
>
> I get that we may not have done the sync from the shadow MSR back to
> the guest MSR, since that is done in kvmppc_handle_exit_pr() with
> interrupts enabled and we might be unloading because we got
> preempted. In that case we would have svcpu->in_use = 1, and we
> should in fact do the sync of the TS bits from shadow_msr to the vcpu
> MSR value in kvmppc_copy_from_svcpu(). If you did that then both the
> load and put functions could just rely on the vcpu's MSR value.
>
Yes. that looks more clean and simpler!
> > We don't know whether there is a transaction to be restored from
> > current host MSR TM status at kvmppc_restore_tm_pr(). To solve this
> > issue, we save current MSR into vcpu->arch.save_msr_tm at
> > kvmppc_save_tm_pr(), and kvmppc_restore_tm_pr() check TS bits of
> > vcpu->arch.save_msr_tm to decide whether to do TM restore.
> >
> > Signed-off-by: Simon Guo
> > Suggested-by: Paul Mackerras
> > ---
> > arch/powerpc/include/asm/kvm_book3s.h | 6 +
> > arch/powerpc/include/asm/kvm_host.h | 1 +
> > arch/powerpc/kvm/book3s_pr.c | 41
> > +++
> > 3 files changed, 48 insertions(+)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_book3s.h
> > b/arch/powerpc/include/asm/kvm_book3s.h
> > index 9a66700..d8dbfa5 100644
> > --- a/arch/powerpc/include/asm/kvm_book3s.h
> > +++ b/arch/powerpc/include/asm/kvm_book3s.h
> > @@ -253,6 +253,12 @@ extern void kvmppc_copy_to_svcpu(struct
> > kvmppc_book3s_shadow_vcpu *svcpu,
> > struct kvm_vcpu *vcpu);
> > extern void kvmppc_copy_from_svcpu(struct kvm_vcpu *vcpu,
> >struct kvmppc_book3s_shadow_vcpu *svcpu);
> > +
> > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > +void kvmppc_save_tm_pr(struct kvm_vcpu *vcpu);
> > +void kvmppc_restore_tm_pr(struct kvm_vcpu *vcpu);
> > +#endif
>
> It would be cleaner at the point where you use these if you added a
> #else clause to define a null version for the case when transactional
> memory support is not configured, like this:
>
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +void kvmppc_save_tm_pr(struct kvm_vcpu *vcpu);
> +void kvmppc_restore_tm_pr(struct kvm_vcpu *vcpu);
> +#else
> +static inline void kvmppc_save_tm_pr(struct kvm_vcpu *vcpu) {}
> +static inline void kvmppc_restore_tm_pr(struct kvm_vcpu *vcpu) {}
> +#endif
>
> That way you don't need the #ifdef at the call site.
>
Thanks for the tip.
> > @@ -131,6 +135,10 @@ static void kvmppc_core_vcpu_put_pr(struct kvm_vcpu
> > *vcpu)
> > if (kvmppc_is_split_real(vcpu))
> > kvmppc_unfixup_split_real(vcpu);
> >
> > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > + kvmppc_save_tm_pr(vcpu);
> > +#endif
> > +
> > kvmppc_giveup_ext(vcpu, MSR_FP | MSR_VEC | MSR_VSX);
> > kvmppc_giveup_fac(vcpu, FSCR_TAR_LG);
>
> I think you should do these giveup_ext/giveup_fac calls before calling
> kvmppc_save_tm_pr, because the treclaim in kvmppc_save_tm_pr will
> modify all the FP/VEC/VSX registers and the TAR.
I handled giveup_ext/giveup_fac() within kvmppc_save_tm_pr(), so that
other place (like kvmppc_emulate_treclaim() can invoke
kvmppc_save_tm_pr() easily). But I think moving the calls sequence as
you suggested above will be more readable.
Thanks,
- Simon
Re: [PATCH 04/26] KVM: PPC: Book3S PR: add C function wrapper for _kvmppc_save/restore_tm()
Hi Paul, On Tue, Jan 23, 2018 at 04:49:16PM +1100, Paul Mackerras wrote: > On Thu, Jan 11, 2018 at 06:11:17PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > Currently _kvmppc_save/restore_tm() APIs can only be invoked from > > assembly function. This patch adds C function wrappers for them so > > that they can be safely called from C function. > > > > Signed-off-by: Simon Guo > > [snip] > > > --- a/arch/powerpc/include/asm/asm-prototypes.h > > +++ b/arch/powerpc/include/asm/asm-prototypes.h > > @@ -126,4 +126,11 @@ unsigned long __init prom_init(unsigned long r3, > > unsigned long r4, > > void _mcount(void); > > unsigned long prepare_ftrace_return(unsigned long parent, unsigned long > > ip); > > > > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > > +/* Transaction memory related */ > > +struct kvm_vcpu; > > +void _kvmppc_restore_tm_pr(struct kvm_vcpu *vcpu, u64 guest_msr); > > +void _kvmppc_save_tm_pr(struct kvm_vcpu *vcpu, u64 guest_msr); > > +#endif > > It's not generally necessary to have ifdefs around function > declarations. If the function is never defined because the feature > is not configured in, that is fine. > Got it. Thanks. > > @@ -149,6 +149,58 @@ _GLOBAL(kvmppc_save_tm) > > blr > > > > /* > > + * _kvmppc_save_tm() is a wrapper around __kvmppc_save_tm(), so that it can > > + * be invoked from C function by PR KVM only. > > + */ > > +_GLOBAL(_kvmppc_save_tm_pr) > > + mflrr5 > > + std r5, PPC_LR_STKOFF(r1) > > + stdur1, -SWITCH_FRAME_SIZE(r1) > > + SAVE_NVGPRS(r1) > > + > > + /* save MSR since TM/math bits might be impacted > > +* by __kvmppc_save_tm(). > > +*/ > > + mfmsr r5 > > + SAVE_GPR(5, r1) > > + > > + /* also save DSCR/CR so that it can be recovered later */ > > + mfspr r6, SPRN_DSCR > > + SAVE_GPR(6, r1) > > + > > + mfcrr7 > > + stw r7, _CCR(r1) > > + > > + /* allocate stack frame for __kvmppc_save_tm since > > +* it will save LR into its stackframe and we don't > > +* want to corrupt _kvmppc_save_tm_pr's. > > +*/ > > + stdur1, -PPC_MIN_STKFRM(r1) > > You don't need to do this. In the PowerPC ELF ABI, functions always > save their LR (i.e. their return address) in their *caller's* stack > frame, not their own. You have established a stack frame for > _kvmppc_save_tm_pr above, and that is sufficient. Same comment > applies for _kvmppc_restore_tm_pr. Ah..yes. I need remove that. Thanks, - Simon
Re: [PATCH 02/26] KVM: PPC: Book3S PR: add new parameter (guest MSR) for kvmppc_save_tm()/kvmppc_restore_tm()
Hi Paul, On Tue, Jan 23, 2018 at 04:42:09PM +1100, Paul Mackerras wrote: > On Thu, Jan 11, 2018 at 06:11:15PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > HV KVM and PR KVM need different MSR source to indicate whether > > treclaim. or trecheckpoint. is necessary. > > > > This patch add new parameter (guest MSR) for these kvmppc_save_tm/ > > kvmppc_restore_tm() APIs: > > - For HV KVM, it is VCPU_MSR > > - For PR KVM, it is current host MSR or VCPU_SHADOW_SRR1 > > > > This enhancement enables these 2 APIs to be reused by PR KVM later. > > And the patch keeps HV KVM logic unchanged. > > > > This patch also reworks kvmppc_save_tm()/kvmppc_restore_tm() to > > have a clean ABI: r3 for vcpu and r4 for guest_msr. > > > > Signed-off-by: Simon Guo > > Question: why do you switch from using HSTATE_HOST_R1 to HSTATE_SCRATCH2 > > > @@ -42,11 +45,11 @@ _GLOBAL(kvmppc_save_tm) > > rldimi r8, r0, MSR_TM_LG, 63-MSR_TM_LG > > mtmsrd r8 > > > > - ld r5, VCPU_MSR(r9) > > - rldicl. r5, r5, 64 - MSR_TS_S_LG, 62 > > + rldicl. r4, r4, 64 - MSR_TS_S_LG, 62 > > beq 1f /* TM not active in guest. */ > > > > - std r1, HSTATE_HOST_R1(r13) > > + std r1, HSTATE_SCRATCH2(r13) > > ... here? > > > @@ -166,17 +173,17 @@ _GLOBAL(kvmppc_restore_tm) > > * The user may change these outside of a transaction, so they must > > * always be context switched. > > */ > > - ld r5, VCPU_TFHAR(r4) > > - ld r6, VCPU_TFIAR(r4) > > - ld r7, VCPU_TEXASR(r4) > > + ld r5, VCPU_TFHAR(r3) > > + ld r6, VCPU_TFIAR(r3) > > + ld r7, VCPU_TEXASR(r3) > > mtspr SPRN_TFHAR, r5 > > mtspr SPRN_TFIAR, r6 > > mtspr SPRN_TEXASR, r7 > > > > - ld r5, VCPU_MSR(r4) > > + mr r5, r4 > > rldicl. r5, r5, 64 - MSR_TS_S_LG, 62 > > beqlr /* TM not active in guest */ > > - std r1, HSTATE_HOST_R1(r13) > > + std r1, HSTATE_SCRATCH2(r13) > > and here? > > Please add a paragraph to the patch description explaining why you are > making that change. In subsequent patches, kvmppc_save_tm/kvmppc_restore_tm() will be invoked by wrapper function who setup addtional stack frame and update R1(then update HSTATE_HOST_R1 with addtional offset). Although HSTATE_HOST_R1 is now used safely(always PPC_STL before entering guest and PPC_LL in kvmppc_interrupt_pr()), I worried a future usage will take an assumption on HSTATE_HOST_R1 value and bring trouble. As a result, in kvmppc_save_tm/kvmppc_restore_tm() case, I choose HSTATE_SCRATCH2 to restore the r1. I will update the commit message. Thanks, - Simon > > Paul.
Re: [PATCH 13/26] KVM: PPC: Book3S PR: adds new kvmppc_copyto_vcpu_tm/kvmppc_copyfrom_vcpu_tm API for PR KVM.
Hi Paul, On Tue, Jan 23, 2018 at 04:52:19PM +1100, Paul Mackerras wrote: > On Thu, Jan 11, 2018 at 06:11:26PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > This patch adds 2 new APIs: kvmppc_copyto_vcpu_tm() and > > kvmppc_copyfrom_vcpu_tm(). These 2 APIs will be used to copy from/to TM > > data between VCPU_TM/VCPU area. > > > > PR KVM will use these APIs for treclaim. or trchkpt. emulation. > > > > Signed-off-by: Simon Guo > > Reviewed-by: Paul Mackerras > > Actually, I take that back. You have missed XER. :) Thanks for the catch. I will fix that. > > Paul. BR, - Simon
Re: [PATCH 00/26] KVM: PPC: Book3S PR: Transaction memory support on PR KVM
Hi Paul, On Tue, Jan 23, 2018 at 04:38:32PM +1100, Paul Mackerras wrote: > On Thu, Jan 11, 2018 at 06:11:13PM +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > In current days, many OS distributions have utilized transaction > > memory functionality. In PowerPC, HV KVM supports TM. But PR KVM > > does not. > > > > The drive for the transaction memory support of PR KVM is the > > openstack Continuous Integration testing - They runs a HV(hypervisor) > > KVM(as level 1) and then run PR KVM(as level 2) on top of that. > > > > This patch set add transaction memory support on PR KVM. > > Thanks for the patch set. It mostly looks good, though I have some > comments on the individual patches. > > I don't see where you are implementing support for userspace accessing > the TM checkpointed register values using the GET_ONE_REG/SET_ONE_REG > API. This would mean that you couldn't migrate a guest that was in > the middle of a transaction. We will need to have the one_reg API > access to the TM checkpoint implemented, though there will be a > difficulty in that kvmppc_get_one_reg() and kvmppc_set_one_reg() are > called with the vcpu context loaded. With your scheme of having the > TM checkpoint stored in the CPU while the vcpu context is loaded, the > values you want to access in kvmppc_get/set_one_reg are inaccessible > since they're stored in the CPU. You would have to arrange for > kvmppc_get/set_one_reg to be called without the vcpu context loaded > (recent patches in the kvm next branch probably make that easier) or > else explicitly unload and reload the vcpu context in those functions. > (This is easier in HV KVM since the checkpoint is not in the CPU at > the point of doing kvmppc_get/set_one_reg.) Thanks for point it out. I didn't think about it before and will investigate. I plan to work out this PR KVM HTM kvmppc_get/set_one_reg() (and the KVM_SET_REGS you mentioned in another mail) with seperate patch/patch set, so that the reworked V2 of current patches can be sent out in parallel. In case it is not appropriate for you, please let me know. > > There is also complexity added because it's possible for the guest to > have TM, FP, VEC and VSX all enabled from its point of view but to > have FP/VEC/VSX not actually enabled in the hardware when the guest is > running. As you note in your patch descriptions, this means that the > guest can do tbegin and create a checkpoint with bogus values for the > FP/VEC/VSX registers. Rather than trying to detect and fix up this > situation after the fact, I would suggest that if the guest has TM > enabled then we make sure that the real FP/VEC/VSX bits in the MSR > match what the guest thinks it has. That way we would avoid the bogus > checkpoint problem. (There is still the possibility of getting bogus > checkpointed FP/VEC/VSX registers if the guest does tbegin with the > FP/VEC/VSX bits clear in the MSR, but that is the guest's problem to > deal with.) Good idea. I will look into kvmppc_set_msr_pr() / kvmppc_giveup_ext() to simplify the solution. Thanks for your review and time. BR, - Simon
Re: [PATCH 00/26] KVM: PPC: Book3S PR: Transaction memory support on PR KVM
Hi Gustavo, On Thu, Jan 11, 2018 at 11:56:59AM -0200, Gustavo Romero wrote: > Hi Simon, > > On 01/11/2018 08:11 AM, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > In current days, many OS distributions have utilized transaction > > memory functionality. In PowerPC, HV KVM supports TM. But PR KVM > > does not. > > > > The drive for the transaction memory support of PR KVM is the > > openstack Continuous Integration testing - They runs a HV(hypervisor) > > KVM(as level 1) and then run PR KVM(as level 2) on top of that. > > > > This patch set add transaction memory support on PR KVM. > > Is this correct to assume that this emulation mode will just kick in on P9 > with kernel TM workarounds and HV KVM will continue to be used on POWER8 > since HV KVM is supported on POWER8 hosts? As Ben mentioned, this patch set aims to enhancement PR KVM on Power8 to support transaction memory. Thanks, - Simon > > > Regards, > Gustavo > > > Test cases performed: > > linux/tools/testing/selftests/powerpc/tm/tm-syscall > > linux/tools/testing/selftests/powerpc/tm/tm-fork > > linux/tools/testing/selftests/powerpc/tm/tm-vmx-unavail > > linux/tools/testing/selftests/powerpc/tm/tm-tmspr > > linux/tools/testing/selftests/powerpc/tm/tm-signal-msr-resv > > linux/tools/testing/selftests/powerpc/math/vsx_preempt > > linux/tools/testing/selftests/powerpc/math/fpu_signal > > linux/tools/testing/selftests/powerpc/math/vmx_preempt > > linux/tools/testing/selftests/powerpc/math/fpu_syscall > > linux/tools/testing/selftests/powerpc/math/vmx_syscall > > linux/tools/testing/selftests/powerpc/math/fpu_preempt > > linux/tools/testing/selftests/powerpc/math/vmx_signal > > linux/tools/testing/selftests/powerpc/ptrace/ptrace-tm-gpr > > linux/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-gpr > > linux/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx > > linux/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr > > linux/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx > > https://github.com/justdoitqd/publicFiles/blob/master/test_tbegin_pr.c > > https://github.com/justdoitqd/publicFiles/blob/master/test_tabort.c > > https://github.com/justdoitqd/publicFiles/blob/master/test_kvm_htm_cap.c > > > > Simon Guo (25): > > KVM: PPC: Book3S PR: Move kvmppc_save_tm/kvmppc_restore_tm to separate > > file > > KVM: PPC: Book3S PR: add new parameter (guest MSR) for > > kvmppc_save_tm()/kvmppc_restore_tm() > > KVM: PPC: Book3S PR: turn on FP/VSX/VMX MSR bits in kvmppc_save_tm() > > KVM: PPC: Book3S PR: add C function wrapper for > > _kvmppc_save/restore_tm() > > KVM: PPC: Book3S PR: In PR KVM suspends Transactional state when > > inject an interrupt. > > KVM: PPC: Book3S PR: PR KVM pass through MSR TM/TS bits to shadow_msr. > > KVM: PPC: Book3S PR: add TEXASR related macros > > KVM: PPC: Book3S PR: Sync TM bits to shadow msr for problem state > > guest > > KVM: PPC: Book3S PR: implement RFID TM behavior to suppress change > > from S0 to N0 > > KVM: PPC: Book3S PR: set MSR HV bit accordingly for PPC970 and others. > > KVM: PPC: Book3S PR: prevent TS bits change in kvmppc_interrupt_pr() > > powerpc: export symbol msr_check_and_set(). > > KVM: PPC: Book3S PR: adds new > > kvmppc_copyto_vcpu_tm/kvmppc_copyfrom_vcpu_tm API for PR KVM. > > KVM: PPC: Book3S PR: export tm_enable()/tm_disable/tm_abort() APIs > > KVM: PPC: Book3S PR: add kvmppc_save/restore_tm_sprs() APIs > > KVM: PPC: Book3S PR: add transaction memory save/restore skeleton for > > PR KVM > > KVM: PPC: Book3S PR: add math support for PR KVM HTM > > KVM: PPC: Book3S PR: make mtspr/mfspr emulation behavior based on > > active TM SPRs > > KVM: PPC: Book3S PR: always fail transaction in guest privilege state > > KVM: PPC: Book3S PR: enable NV reg restore for reading TM SPR at guest > > privilege state > > KVM: PPC: Book3S PR: adds emulation for treclaim. > > KVM: PPC: Book3S PR: add emulation for trechkpt in PR KVM. > > KVM: PPC: Book3S PR: add emulation for tabort. for privilege guest > > KVM: PPC: Book3S PR: add guard code to prevent returning to guest with > > PR=0 and Transactional state > > KVM: PPC: Book3S PR: enable HTM for PR KVM for KVM_CHECK_EXTENSION > > ioctl > > > > arch/powerpc/include/asm/asm-prototypes.h | 10 + > > arch/powerpc/include/asm/kvm_book3s.h | 8 + > > arch/powerpc/include/asm/kvm_host.h | 3 + > > arch/powerpc/inc
Re: [PATCH v3 3/3] powerpc:selftest update memcmp_64 selftest for VMX implementation
Hi Cyril, Thanks for the review. On Mon, Oct 16, 2017 at 02:32:58PM +1100, Cyril Bur wrote: > On Fri, 2017-10-13 at 12:30 +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > This patch adjust selftest memcmp_64 so that memcmp selftest can be > > compiled successfully. > > > > Do they not compile at the moment? > Since I renamed enter/exit_vmx_copy() to enter/exit_vmx_ops() in patch 2nd, this patch is necessary to make selftest compiled based on patch 2nd. > > It also adds testcases for: > > - memcmp over 4K bytes size. > > - s1/s2 with different/random offset on 16 bytes boundary. > > - enter/exit_vmx_ops pairness. > > > > This is a great idea, just a thought though - perhaps it might make > more sense to have each condition be tested for in a separate binary > rather than a single binary that tests everything. > I am not sure whether it is better to seperate those test cases on the same memcmp func. Currently the time to execute this single test bin is within 35 secs, and keeping them in one exec will make people test more comprehensively. So Personally I prefer to keep them in one at present. Thanks, - Simon
Re: [PATCH] selftests/powerpc: fix build error in powerpc ptrace selftests.
Hi Michael, On Tue, Oct 10, 2017 at 09:10:32PM +1100, Michael Ellerman wrote: > wei.guo.si...@gmail.com writes: > > > From: Simon Guo > > > > GCC 7 will take "r2" in clobber list as an error will it will get following > > build errors for powerpc ptrace selftests even with -fno-pic option: > > ptrace-tm-vsx.c: In function ‘tm_vsx’: > > ptrace-tm-vsx.c:42:2: error: PIC register clobbered by ‘r2’ in ‘asm’ > > asm __volatile__( > > ^~~ > > make[1]: *** [ptrace-tm-vsx] Error 1 > > ptrace-tm-spd-vsx.c: In function ‘tm_spd_vsx’: > > ptrace-tm-spd-vsx.c:55:2: error: PIC register clobbered by ‘r2’ in ‘asm’ > > asm __volatile__( > > ^~~ > > make[1]: *** [ptrace-tm-spd-vsx] Error 1 > > ptrace-tm-spr.c: In function ‘tm_spr’: > > ptrace-tm-spr.c:46:2: error: PIC register clobbered by ‘r2’ in ‘asm’ > > asm __volatile__( > > ^~~ > > > > This patch fix the build error by removing "r2" out of clobber list. > > But do any of the blocks clobber r2? If so then it should be in the > clobber list. I see none of them clobbers r2, and neither does those assembly functions which those blocks calls, like "loadvsx". For the change on tools/testing/selftests/powerpc/ptrace/Makefile, it can be ignored since I noticed recent commit a3c01050584da3 "selftests/powerpc: Force ptrace tests to build -fno-pie". Please let me know if you want a new v2 to remove that change on ptrace/Makefile. Thanks, - Simon
Re: [PATCH v2 2/3] powerpc/64: enhance memcmp() with VMX instruction for long bytes comparision
On Wed, Sep 27, 2017 at 09:43:44AM +, David Laight wrote: > From: Segher Boessenkool > > Sent: 27 September 2017 10:28 > ... > > You also need nasty code to deal with the start and end of strings, with > > conditional branches and whatnot, which quickly overwhelms the benefit > > of using vector registers at all. This tradeoff also changes with newer > > ISA versions. > > The goal posts keep moving. > For instance with modern intel x86 cpus 'rep movsb' is by far the fastest > way to copy data (from cached memory). > > > Things have to become *really* cheap before it will be good to often use > > vector registers in the kernel though. > > I've had thoughts about this in the past. > If the vector registers belong to the current process then you might > get away with just saving the ones you want to use. > If they belong to a different process then you also need to tell the > FPU save code where you've saved the registers. > Then the IPI code can recover all the correct values. > > On X86 all the AVX registers are caller saved, the system call > entry could issue the instruction that invalidates them all. > Kernel code running in the context of a user process could then > use the registers without saving them. > It would only need to set a mark to ensure they are invalidated > again on return to user (might be cheap enough to do anyway). > Dunno about PPC though. I am not aware of any ppc instruction which can set a "mark" or provide any high granularity flag against single or subgroup of vec regs' validity. But ppc experts may want to correct me. Thanks, - Simon
Re: [PATCH v2 2/3] powerpc/64: enhance memcmp() with VMX instruction for long bytes comparision
Hi Michael, On Wed, Sep 27, 2017 at 01:38:09PM +1000, Michael Ellerman wrote: > Segher Boessenkool writes: > > > On Tue, Sep 26, 2017 at 03:34:36PM +1000, Michael Ellerman wrote: > >> Cyril Bur writes: > >> > This was written for userspace which doesn't have to explicitly enable > >> > VMX in order to use it - we need to be smarter in the kernel. > >> > >> Well the kernel has to do it for them after a trap, which is actually > >> even more expensive, so arguably the glibc code should be smarter too > >> and the threshold before using VMX should probably be higher than in the > >> kernel (to cover the cost of the trap). > > > > A lot of userspace code uses V*X, more and more with newer CPUs and newer > > compiler versions. If you already paid the price for using vector > > registers you do not need to again :-) > > True, but you don't know if you've paid the price already. > > You also pay the price on every context switch (more state to switch), > so it's not free even once enabled. Which is why the kernel will > eventually turn it off if it's unused again. > > But now that I've actually looked at the glibc version, it does do some > checks for minimum length before doing any vector instructions, so Looks the glibc version will use VSX instruction and lead to trap in a 9 bytes size cmp with src/dst 16 bytes aligned. 132 /* Now both rSTR1 and rSTR2 are aligned to QW. */ 133 .align 4 134 L(qw_align): 135 vspltisbv0, 0 136 srdi. r6, rN, 6 137 li r8, 16 138 li r10, 32 139 li r11, 48 140 ble cr0, L(lessthan64) 141 mtctr r6 142 vspltisbv8, 0 143 vspltisbv6, 0 144 /* Aligned vector loop. */ 145 .align 4 line 135 is the VSX instruction causing trap. Did I miss anything? > that's probably all we want. The exact trade off between checking some > bytes without vector vs turning on vector depends on your input data, so > it's tricky to tune in general. Discussed offline with Cyril. The plan is to use (>=4KB) as the minimum len before vector regs steps at v3. Cyril will consolidate his existing work on KSM optimization later, which is probably making 64bytes comparison-ahead to determine whether it is an early or late matching pattern. Cyril has also some other valuable comments and I will rework on v3. Is it OK for you? Thanks, - Simon
Re: [PATCH v2 3/3] powerpc:selftest update memcmp_64 selftest for VMX implementation
Hi David, On Mon, Sep 25, 2017 at 09:30:28AM +, David Laight wrote: > From: wei.guo.si...@gmail.com > > Sent: 21 September 2017 00:35 > > This patch adjust selftest memcmp_64 so that memcmp selftest can be > > compiled successfully. > ... > > #define ITERATIONS 1 > > > > +#define LARGE_SIZE (5 * 1024) > > +#define LARGE_ITERATIONS 1000 > ... > > Measuring performance by doing a lot of iterations isn't ideal > and is pretty pointless. > Cold cache performance can be more useful. > Also you don't really want any dynamic branch prediction logic > tuned to the exact test you keep doing. I think the (orignal) selftest aims at full coverage of functionality correctness, since each iteration generates a new data set by random. Thanks, - Simon
Re: [PATCH v2 2/3] powerpc/64: enhance memcmp() with VMX instruction for long bytes comparision
Hi Cyril, On Sat, Sep 23, 2017 at 12:06:48AM +1000, Cyril Bur wrote: > On Thu, 2017-09-21 at 07:34 +0800, wei.guo.si...@gmail.com wrote: > > From: Simon Guo > > > > This patch add VMX primitives to do memcmp() in case the compare size > > exceeds 4K bytes. > > > > Hi Simon, > > Sorry I didn't see this sooner, I've actually been working on a kernel > version of glibc commit dec4a7105e (powerpc: Improve memcmp performance > for POWER8) unfortunately I've been distracted and it still isn't done. Thanks for sync with me. Let's consolidate our effort together :) I have a quick check on glibc commit dec4a7105e. Looks the aligned case comparison with VSX is launched without rN size limitation, which means it will have a VSX reg load penalty even when the length is 9 bytes. It did some optimization when src/dest addrs don't have the same offset on 8 bytes alignment boundary. I need to read more closely. > I wonder if we can consolidate our efforts here. One thing I did come > across in my testing is that for memcmp() that will fail early (I > haven't narrowed down the the optimal number yet) the cost of enabling > VMX actually turns out to be a performance regression, as such I've > added a small check of the first 64 bytes to the start before enabling > VMX to ensure the penalty is worth taking. Will there still be a penalty if the 65th byte differs? > > Also, you should consider doing 4K and greater, KSM (Kernel Samepage > Merging) uses PAGE_SIZE which can be as small as 4K. Currently the VMX will only be applied when size exceeds 4K. Are you suggesting a bigger threshold than 4K? We can sync more offline for v3. Thanks, - Simon
Re: [PATCH v2 2/3] powerpc/64: enhance memcmp() with VMX instruction for long bytes comparision
Hi, On Thu, Sep 21, 2017 at 07:34:39AM +0800, wei.guo.si...@gmail.com wrote: > From: Simon Guo > > This patch add VMX primitives to do memcmp() in case the compare size > exceeds 4K bytes. > > Test result with following test program(replace the "^>" with ""): > -- I missed the exit_vmx_ops() part and need to rework on v3. Thanks, - Simon
Re: [PATCH v1 0/3] powerpc: memcmp() optimization
Hi Chris, On Tue, Sep 19, 2017 at 02:21:33PM +0200, Christophe LEROY wrote: > Hi > > Could you in the email/patch subject and in the commit texts write > powerpc/64 instead of powerpc as it doesn't apply to powerpc/32 > > Christophe > Sure. I will update in v2. BR, - Simon
Re: [PATCH v1 1/3] powerpc: Align bytes before fall back to .Lshort in powerpc memcmp
On Tue, Sep 19, 2017 at 10:12:50AM +, David Laight wrote: > From: wei.guo.si...@gmail.com > > Sent: 19 September 2017 11:04 > > Currently memcmp() in powerpc will fall back to .Lshort (compare per byte > > mode) if either src or dst address is not 8 bytes aligned. It can be > > opmitized if both addresses are with the same offset with 8 bytes boundary. > > > > memcmp() can align the src/dst address with 8 bytes firstly and then > > compare with .Llong mode. > > Why not mask both addresses with ~7 and mask/shift the read value to ignore > the unwanted high (BE) or low (LE) bits. > > The same can be done at the end of the compare with any final, partial word. > > David > Yes. That will be better. A prototyping shows ~5% improvement on 32 bytes size comparison with v1. I will rework on v2. Thanks for the suggestion. BR, - Simon
Re: Build failures in powerpc ptrace selftests
Hi Seth,
On Wed, Aug 30, 2017 at 08:05:25AM -0500, Seth Forshee wrote:
> With gcc 7 from Ubuntu 17.10 I'm getting the follwing error building the
> ptrace selftests for powerpc:
>
> ptrace-tm-vsx.c: In function ‘tm_vsx’:
> ptrace-tm-vsx.c:42:2: error: PIC register clobbered by ‘r2’ in ‘asm’
> asm __volatile__(
> ^~~
> make[1]: *** [ptrace-tm-vsx] Error 1
> ptrace-tm-spd-vsx.c: In function ‘tm_spd_vsx’:
> ptrace-tm-spd-vsx.c:55:2: error: PIC register clobbered by ‘r2’ in ‘asm’
> asm __volatile__(
> ^~~
> make[1]: *** [ptrace-tm-spd-vsx] Error 1
> ptrace-tm-spr.c: In function ‘tm_spr’:
> ptrace-tm-spr.c:46:2: error: PIC register clobbered by ‘r2’ in ‘asm’
> asm __volatile__(
> ^~~
>
> Best I can tell gcc now considers any inline asm which clobbers r2 an
> error, and I get it even with -fno-pic. I'm not familiar with powerpc
> asm, but it's not obvious to me why r2 is in the clobber list for any of
> these.
Thanks for reporting this issue.
In PPC ABI, r2 is used as TOC pointer and it might be changed by inter-module
function calls. I believe that is the reason why it was put into clobber list
originally.
In our case, dual entry is used to save TOC pointer (r2) update for function
calls within one module. There is no r2 change and I think r2 can be moved out
of clobber list.
I don't have a GCC-7 environment. If possible, could you help to try following
patch?
Thanks.
- Simon
diff --git a/tools/testing/selftests/powerpc/ptrace/Makefile
b/tools/testing/selftests/powerpc/ptrace/Makefile
index fe6bc60..c836f6d 100644
--- a/tools/testing/selftests/powerpc/ptrace/Makefile
+++ b/tools/testing/selftests/powerpc/ptrace/Makefile
@@ -6,7 +6,7 @@ include ../../lib.mk
all: $(TEST_PROGS)
-CFLAGS += -m64 -I../../../../../usr/include -I../tm -mhtm
+CFLAGS += -m64 -I../../../../../usr/include -I../tm -mhtm -fno-pic
$(TEST_PROGS): ../harness.c ../utils.c ../lib/reg.S ptrace.h
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c
b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c
index 0df3c23..277dade 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c
@@ -79,8 +79,8 @@ void tm_spd_vsx(void)
: [res] "=r" (result), [texasr] "=r" (texasr)
: [fp_load] "r" (fp_load), [fp_load_ckpt] "r" (fp_load_ckpt),
[sprn_texasr] "i" (SPRN_TEXASR)
- : "memory", "r0", "r1", "r2", "r3", "r4",
- "r8", "r9", "r10", "r11"
+ : "memory", "r0", "r1", "r3", "r4",
+ "r7", "r8", "r9", "r10", "r11"
);
if (result) {
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c
b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c
index 94e57cb..51427a2 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c
@@ -76,8 +76,7 @@ void tm_spr(void)
: [tfhar] "=r" (tfhar), [res] "=r" (result),
[texasr] "=r" (texasr), [cptr1] "=r" (cptr1)
: [sprn_texasr] "i" (SPRN_TEXASR)
- : "memory", "r0", "r1", "r2", "r3", "r4",
- "r8", "r9", "r10", "r11", "r31"
+ : "memory", "r0", "r8", "r31"
);
/* There are 2 32bit instructions before tbegin. */
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c
b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c
index b4081e2..17c23ca 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c
@@ -67,7 +67,7 @@ void tm_vsx(void)
: [res] "=r" (result), [texasr] "=r" (texasr)
: [fp_load] "r" (fp_load), [fp_load_ckpt] "r" (fp_load_ckpt),
[sprn_texasr] "i" (SPRN_TEXASR), [cptr1] "r" (&cptr[1])
- : "memory", "r0", "r1", "r2", "r3", "r4",
+ : "memory", "r0", "r1", "r3", "r4",
"r7", "r8", "r9", "r10", "r11"
);
Re: [PATCH v15 03/15] selftests/powerpc: Add ptrace tests for EBB
On Fri, Oct 07, 2016 at 08:44:48AM +1100, Michael Ellerman wrote: > wei.guo.si...@gmail.com writes: > > > From: Anshuman Khandual > > > > This patch adds ptrace interface test for EBB/PMU specific > > registers. This also adds some generic ptrace interface > > based helper functions to be used by other patches later > > on in the series. > > This is consistently failing for me on a P8 Tuleta (pvr 004b 0201): > > # ./ptrace-ebb > test: ptrace_ebb_pmu > tags: git_version:v4.8-rc5-176-g89cf1de0ae90 > EBBRR: 100059f8 > EBBHR: 100053cc; expected: 100053cc > BESCR: 8001 > SIAR: 100012d0 > SDAR: 3fff7e4cc000 > SIER: 300; expected: 200 > MMCR2: 0; expected: 0 > MMCR0: 18080; expected: 18080 > failure: ptrace_ebb_pmu > > cheers Michael, Yes.. SIER has different value in baremetal and virtual machine due to different MSR[HV] value. I will correct it. Originally I only tested in virtual BE/LE machines. Currently all tests cases (with fix) passed on one baremetal P8 machine with LE OS installed. And I will try to find another baremetal with BE OS installed to test. Thanks for indicating it. BR, Simon
Re: [PATCH v14 13/15] selftests/powerpc: Add ptrace tests for TM SPR registers
Hi Cyril,
On Wed, Sep 14, 2016 at 03:04:12PM +1000, Cyril Bur wrote:
> On Mon, 2016-09-12 at 15:33 +0800, wei.guo.si...@gmail.com wrote:
> > From: Anshuman Khandual
> >
> > This patch adds ptrace interface test for TM SPR registers. This
> > also adds ptrace interface based helper functions related to TM
> > SPR registers access.
> >
>
> I'm seeing this one fail a lot, it does occasionally succeed but fails
> a lot on my test setup.
>
> I use qemu on a power8 for most of my testing:
> qemu-system-ppc64 --enable-kvm -machine pseries,accel=kvm,usb=off -m
> 4096 -realtime mlock=off -smp 4,sockets=1,cores=2,threads=2 -nographic
> -vga none
>
>
> > Signed-off-by: Anshuman Khandual
> > Signed-off-by: Simon Guo
> > ---
> > tools/testing/selftests/powerpc/ptrace/Makefile| 3 +-
> > .../selftests/powerpc/ptrace/ptrace-tm-spr.c | 186
> > +
> > tools/testing/selftests/powerpc/ptrace/ptrace.h| 35
> > 3 files changed, 223 insertions(+), 1 deletion(-)
> > create mode 100644 tools/testing/selftests/powerpc/ptrace/ptrace-tm-
> > spr.c
> >
> > diff --git a/tools/testing/selftests/powerpc/ptrace/Makefile
> > b/tools/testing/selftests/powerpc/ptrace/Makefile
> > index 797840a..f34670e 100644
> > --- a/tools/testing/selftests/powerpc/ptrace/Makefile
> > +++ b/tools/testing/selftests/powerpc/ptrace/Makefile
> > @@ -1,7 +1,8 @@
> > TEST_PROGS := ptrace-ebb ptrace-gpr ptrace-tm-gpr ptrace-tm-spd-gpr
> > \
> > ptrace-tar ptrace-tm-tar ptrace-tm-spd-tar ptrace-vsx ptrace-tm-vsx
> > \
> > -ptrace-tm-spd-vsx
> > +ptrace-tm-spd-vsx ptrace-tm-spr
> >
> > +include ../../lib.mk
> >
> > all: $(TEST_PROGS)
> > CFLAGS += -m64
> > diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c
> > b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c
> > new file mode 100644
> > index 000..2863070
> > --- /dev/null
> > +++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c
> > @@ -0,0 +1,186 @@
> > +/*
> > + * Ptrace test TM SPR registers
> > + *
> > + * Copyright (C) 2015 Anshuman Khandual, IBM Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +#include "ptrace.h"
> > +
> > +/* Tracee and tracer shared data */
> > +struct shared {
> > + int flag;
> > + struct tm_spr_regs regs;
> > +};
> > +unsigned long tfhar;
> > +
> > +int shm_id;
> > +volatile struct shared *cptr, *pptr;
> > +
> > +int shm_id1;
> > +volatile int *cptr1, *pptr1;
> > +
> > +#define TM_SCHED 0xde018c01
> > +#define TM_KVM_SCHED 0xe001ac01
> > +
> > +int validate_tm_spr(struct tm_spr_regs *regs)
> > +{
> > + if (regs->tm_tfhar != tfhar)
> > + return TEST_FAIL;
> > +
> > + if ((regs->tm_texasr != TM_SCHED) && (regs->tm_texasr !=
> > TM_KVM_SCHED))
> > + return TEST_FAIL;
>
> The above condition fails, should this test try again if this condition
> is true, rather than fail?
>
I reproduced the failure with your configuration. Besides treclaim, there are
many other reasons that may lead to the transaction failure according to ISA.
At least I observed following Texasr values:
11801
18801
1a801
I noticed some FIAR locates at IPI handling related code. My previous
configuration
is 4 sockets/each with only 1 core/1 thread. That is probably the reason I
always
passed the test in the old configuration.(per my understanding, IPI is limited
within
threads). So I removed the checking regarding specfied TEXASR value.
And I think I have reworked all your other comments.
I will send out v15 soon. Again thanks for your code inspection.
BR,
- Simon
Re: [PATCH 2/2] powerpc: tm: Enable transactional memory (TM) lazily for userspace
On Wed, Sep 14, 2016 at 06:02:16PM +1000, Cyril Bur wrote:
> @@ -954,8 +963,16 @@ static inline void __switch_to_tm(struct task_struct
> *prev,
> struct task_struct *new)
> {
> if (cpu_has_feature(CPU_FTR_TM)) {
> - tm_enable();
> - tm_reclaim_task(prev);
> + if (tm_enabled(prev) || tm_enabled(new))
> + tm_enable();
> +
> + if (tm_enabled(prev)) {
> + prev->thread.load_tm++;
> + tm_reclaim_task(prev);
> + if (!MSR_TM_ACTIVE(prev->thread.regs->msr) &&
> prev->thread.load_tm == 0)
> + prev->thread.regs->msr &= ~MSR_TM;
> + }
Hi Cyril,
If MSR_TM_ACTIVE(), is it better to reset load_tm to 0?
Other looks good to me.
Thanks,
- Simon
Re: [PATCH v14 13/15] selftests/powerpc: Add ptrace tests for TM SPR registers
On Wed, Sep 14, 2016 at 03:04:12PM +1000, Cyril Bur wrote: > On Mon, 2016-09-12 at 15:33 +0800, wei.guo.si...@gmail.com wrote: > > From: Anshuman Khandual > > > > This patch adds ptrace interface test for TM SPR registers. This > > also adds ptrace interface based helper functions related to TM > > SPR registers access. > > > > I'm seeing this one fail a lot, it does occasionally succeed but fails > a lot on my test setup. > > I use qemu on a power8 for most of my testing: > qemu-system-ppc64 --enable-kvm -machine pseries,accel=kvm,usb=off -m > 4096 -realtime mlock=off -smp 4,sockets=1,cores=2,threads=2 -nographic > -vga none Hi Cyril, Sorry for the late response. I am just back from a vacation. Strangely the case always succeed in my machine. I will try to reproduce it with your configuration. > > + if ((regs->tm_texasr != TM_SCHED) && (regs->tm_texasr != > > TM_KVM_SCHED)) > > + return TEST_FAIL; > > The above condition fails, should this test try again if this condition > is true, rather than fail? If you have the value of "regs->tm_texasr" in hand, please let me know. Thanks for the elaborated comment in other mails and I will work on that. BR, - Simon
Re: [PATCH v14 00/15] selftests/powerpc: Add ptrace tests for ppc registers
Hi Cyril,
On Tue, Sep 13, 2016 at 03:49:10PM +1000, Cyril Bur wrote:
> Thanks for putting the effort in to get these merged! I have a few
> remarks that apply to more than one patch which I'll say here.
>
> I'm not sure #defining the TM instructions as .long for the selftests
> is useful. Compilers these days know about the instructions 'tbegin.'
> 'tsuspend.' and the like, I would question anyone using a compiler old
> enough not to know about these...
I agree. But let me check with original author Anshuman firstly.
>
> There are a few assembly fpu register load functions that could be
> consolidated into those in math/ and even some in tm/
Will rework that.
>
> Doing while(ptr); to wait for another thread should be
>
> while(ptr)
> asm volatile("" : : : "memory");
>
> Documentation/volatile-considered-harmful.txt for reasons why.
> Even knowing this I did it your way without thinking in a selftest I
> wrote doing similar things and it turns out that it didn't work [the
> way we both expect it would].
You are right.
>
> Having said all that, I'm aware that these are selftests and this
> series could be nicer but I won't lose any sleep if they were merged
> almost as is. Thanks for your work!
>
> Finally, they didn't compile for me, I did a git rebase --exec with my
> build scripts and:
>
> selftests/powerpc: Add ptrace tests for EBB
> [snip]
> *** No rule to make target 'ptrace.S', needed by 'ptrace-ebb'.
> (that appears fixed by subsequent patch)
>
> selftests/powerpc: Add ptrace tests for GPR/FPR registers
> Seems to have failed horribly and those problems continue...
>
> I applied these to powerpc-next at:
> commit c6935931c1894ff857616ff8549b61236a19148f
> Author: Linus Torvalds
> Date: Sun Sep 4 14:31:46 2016 -0700
>
> Linux 4.8-rc5
>
> Should I have based on something else?
I didn't reproduce the latter error and I also applied on c69359.
My build script is only one line:
make -C tools/testing/selftests TARGETS=powerpc 1>/dev/null
Did I miss anything with your build script?
Anyway I need to fix that.
Thanks for the sharing. Most are good comments and I will rework that.
BR,
- Simon
Re: [PATCH] powerpc: set used_vsr/used_vr/used_spe in sigreturn path when MSR bits are active
On Tue, Jul 26, 2016 at 04:06:01PM +0800, wei.guo.si...@gmail.com wrote: > From: Simon Guo > > Normally, when MSR[VSX/VR/SPE] bits = 1, the used_vsr/used_vr/used_spe > bit have already been set. However signal frame locates at user space > and it is controlled by user application. It is up to kernel to make > sure used_vsr/used_vr/used_spe(in kernel)=1 and consistent with MSR > bits. > > For example, CRIU application, who utilizes sigreturn to restore > checkpointed process, will lead to the case where MSR[VSX] bit is > active in signal frame, but used_vsx bit is not set. (the same applies > to VR/SPE). > > This patch will reinforce this at kernel by always setting used_* bit > when MSR related bits are active in signal frame and we are doing > sigreturn. > > This patch is based on Ben's Proposal. > Hi Michael, Just a ping for this patch. The history locates at: http://linuxppc.10917.n7.nabble.com/PATCH-v4-powerpc-Export-thread-struct-used-vr-used-vsr-to-user-space-td110147.html#a110161 If any addtional work required, please let me know. Thanks, - Simon
Re: [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common.
On Fri, Sep 09, 2016 at 08:52:52PM +1000, Michael Ellerman wrote:
> I do - Sorry Simon but your patch just adds too many #ifdefs.
>
> Any time you have to do something like:
>
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> }
> +#endif
>
> It should be a sign that something has gone wrong :)
>
> Does Cyril's series to rework the TM structures help at all with this
> warning?
Hi Michael,
What cppchecker complains is only concerned with GPR:
gpr32_get/set_common() which is used by tm_cgpr32_get()/gpr32_get().
Cyril's patch changes TM FPR/VR/VSX state saving location to be consistent with
GPR's. It doesn't actually modify TM GPR behavior.
So Cyril's work doesn't relate with this cppchecker complaining issue.
Thanks for the feedback regarding too many ifdefs. Is following implemention
better for this issue?
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index bf91658..cf48e98 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -2065,34 +2065,14 @@ static const struct user_regset_view
user_ppc_native_view = {
static int gpr32_get_common(struct task_struct *target,
const struct user_regset *regset,
unsigned int pos, unsigned int count,
- void *kbuf, void __user *ubuf, bool tm_active)
+ void *kbuf, void __user *ubuf,
+ unsigned long *regs)
{
- const unsigned long *regs = &target->thread.regs->gpr[0];
- const unsigned long *ckpt_regs;
compat_ulong_t *k = kbuf;
compat_ulong_t __user *u = ubuf;
compat_ulong_t reg;
int i;
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
- ckpt_regs = &target->thread.ckpt_regs.gpr[0];
-#endif
- if (tm_active) {
- regs = ckpt_regs;
- } else {
- if (target->thread.regs == NULL)
- return -EIO;
-
- if (!FULL_REGS(target->thread.regs)) {
- /*
-* We have a partial register set.
-* Fill 14-31 with bogus values.
-*/
- for (i = 14; i < 32; i++)
- target->thread.regs->gpr[i] = NV_REG_POISON;
- }
- }
-
pos /= sizeof(reg);
count /= sizeof(reg);
@@ -2133,29 +2113,13 @@ static int gpr32_get_common(struct task_struct *target,
static int gpr32_set_common(struct task_struct *target,
const struct user_regset *regset,
unsigned int pos, unsigned int count,
-const void *kbuf, const void __user *ubuf, bool tm_active)
+const void *kbuf, const void __user *ubuf,
+unsigned long *regs)
{
- unsigned long *regs = &target->thread.regs->gpr[0];
- unsigned long *ckpt_regs;
const compat_ulong_t *k = kbuf;
const compat_ulong_t __user *u = ubuf;
compat_ulong_t reg;
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
- ckpt_regs = &target->thread.ckpt_regs.gpr[0];
-#endif
-
- if (tm_active) {
- regs = ckpt_regs;
- } else {
- regs = &target->thread.regs->gpr[0];
-
- if (target->thread.regs == NULL)
- return -EIO;
-
- CHECK_FULL_REGS(target->thread.regs);
- }
-
pos /= sizeof(reg);
count /= sizeof(reg);
@@ -2220,7 +2184,7 @@ static int tm_cgpr32_get(struct task_struct *target,
unsigned int pos, unsigned int count,
void *kbuf, void __user *ubuf)
{
- return gpr32_get_common(target, regset, pos, count, kbuf, ubuf, 1);
+ return gpr32_get_common(target, regset, pos, count, kbuf, ubuf,
&target->thread.ckpt_regs.gpr[0]);
}
static int tm_cgpr32_set(struct task_struct *target,
@@ -2228,7 +2192,7 @@ static int tm_cgpr32_set(struct task_struct *target,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
- return gpr32_set_common(target, regset, pos, count, kbuf, ubuf, 1);
+ return gpr32_set_common(target, regset, pos, count, kbuf, ubuf,
&target->thread.ckpt_regs.gpr[0]);
}
#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
@@ -2237,7 +2201,18 @@ static int gpr32_get(struct task_struct *target,
unsigned int pos, unsigned int count,
void *kbuf, void __user *ubuf)
{
- return gpr32_get_common(target, regset, pos, count, kbuf, ubuf, 0);
+ if (target->thread.regs == NULL)
+ return -EIO;
+
+ if (!FULL_REGS(target->thread.regs)) {
+ /*
+* We have a partial register set.
+* Fill 14-31 with bogus values.
+*/
+ for (i = 14; i < 32; i++)
+ target->thread.regs->gpr[i] = NV_REG_POISON;
+
Re: [PATCH] powerpc/ptrace: Fix cppcheck issue in gpr32_set_common/gpr32_get_common.
Hi Daniel, On Wed, Aug 24, 2016 at 12:21:23PM +1000, Daniel Axtens wrote: > Hi Simon, > > > The ckpt_regs usage in gpr32_set_common/gpr32_get_common() > > will lead to cppcheck error. > > > > [arch/powerpc/kernel/ptrace.c:2062]: (error) Uninitialized variable: > > ckpt_regs > > [arch/powerpc/kernel/ptrace.c:2130]: (error) Uninitialized variable: > > ckpt_regs > > > > A straightforward fix to clean it. > > I'm always happy to see cppcheck warnings fixed :) Thanks for raising this issue :) > > > static int gpr32_get_common(struct task_struct *target, > > const struct user_regset *regset, > > unsigned int pos, unsigned int count, > > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > > void *kbuf, void __user *ubuf, bool tm_active) > > +#else > > + void *kbuf, void __user *ubuf) > > +#endif > > I wonder if it might be possible to avoid some of the ifdefs and general > churn by making the tm_active argument __maybe_unused rather than > ifdefing around it? > > In particular, it would mean the two hunks in the function definitions > and these these two hunks at the call site would be unnecessary: I think keeping tm_active argument for "ifndef CONFIG_PPC_TRANSACTIONAL_MEM" case (with __maybe_unused prefix) will be somehow strange -- Whatever value is provided in the caller function for tm_active, programmer might be puzzled and cost sometime to think about it. I don't like to use "__maybe_unused" to bypass this warning. Thanks, - Simon
Re: [PATCH v3] powerpc: signals: Discard transaction state from signal frames
a sighandler and suspended on return
> +from the sighandler to the kernel will get reclaimed and discarded.
>
> Failure cause codes used by kernel
> ==
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index b6aa378..a7daf74 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -1226,7 +1226,21 @@ long sys_rt_sigreturn(int r3, int r4, int r5, int r6,
> int r7, int r8,
> (regs->gpr[1] + __SIGNAL_FRAMESIZE + 16);
> if (!access_ok(VERIFY_READ, rt_sf, sizeof(*rt_sf)))
> goto bad;
> +
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> + /*
> + * If there is a transactional state then throw it away.
> + * The purpose of a sigreturn is to destroy all traces of the
> + * signal frame, this includes any transactional state created
> + * within in. We only check for suspended as we can never be
> + * active in the kernel, we are active, there is nothing better to
> + * do than go ahead and Bad Thing later.
> + * The cause is not important as there will never be a
> + * recheckpoint so it's not user visible.
> + */
> + if (MSR_TM_SUSPENDED(mfmsr()))
> + tm_reclaim_current(0);
> +
> if (__get_user(tmp, &rt_sf->uc.uc_link))
> goto bad;
> uc_transact = (struct ucontext __user *)(uintptr_t)tmp;
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 7e49984..70409bb 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -676,7 +676,21 @@ int sys_rt_sigreturn(unsigned long r3, unsigned long r4,
> unsigned long r5,
> if (__copy_from_user(&set, &uc->uc_sigmask, sizeof(set)))
> goto badframe;
> set_current_blocked(&set);
> +
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> + /*
> + * If there is a transactional state then throw it away.
> + * The purpose of a sigreturn is to destroy all traces of the
> + * signal frame, this includes any transactional state created
> + * within in. We only check for suspended as we can never be
> + * active in the kernel, we are active, there is nothing better to
> + * do than go ahead and Bad Thing later.
> + * The cause is not important as there will never be a
> + * recheckpoint so it's not user visible.
> + */
> + if (MSR_TM_SUSPENDED(mfmsr()))
> + tm_reclaim_current(0);
> +
> if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
> goto badframe;
> if (MSR_TM_ACTIVE(msr)) {
> --
> 2.9.3
>
Acked-by: Simon Guo
Thanks,
- Simon
Re: [PATCH] powerpc: signals: Discard transaction state from signal frames
Hi Cyril, On Mon, Aug 22, 2016 at 05:32:06PM +1000, Cyril Bur wrote: > diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c > index b6aa378..31e4e15 100644 > --- a/arch/powerpc/kernel/signal_32.c > +++ b/arch/powerpc/kernel/signal_32.c > @@ -1226,7 +1226,19 @@ long sys_rt_sigreturn(int r3, int r4, int r5, int r6, > int r7, int r8, > (regs->gpr[1] + __SIGNAL_FRAMESIZE + 16); > if (!access_ok(VERIFY_READ, rt_sf, sizeof(*rt_sf))) > goto bad; > + > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM > + /* > + * If there is a transactional/suspended state then throw it away. > + * The purpose of a sigreturn is to destroy all traces of the > + * signal frame, this includes any transactional state created > + * within in. > + * The cause is not important as there will never be a > + * recheckpoint so it's not user visible. > + */ > + if (MSR_TM_ACTIVE(mfmsr())) > + tm_reclaim_current(0); > + Maybe a little picky here: Per my understanding, the TRANSACTIONAL state will be failed in system call common entry. The only expected state to prevent here is SUSPEND state. Should we use MSR_TM_SUSPENDED(mfmsr()) here and BUG_ON MSR_TM_TRANSACTIONAL(mfmsr())? -- If it is transactional state, something is wrong with kernel. Others looks good to me. Thanks, - Simon
Re: [PATCH v3 19/21] powerpc: tm: Always use fp_state and vr_state to store live registers
On Wed, Aug 17, 2016 at 01:43:21PM +1000, Cyril Bur wrote: > There is currently an inconsistency as to how the entire CPU register > state is saved and restored when a thread uses transactional memory > (TM). > > Using transactional memory results in the CPU having duplicated > (almost all) of its register state. This duplication results in a set > of registers which can be considered 'live', those being currently > modified by the instructions being executed and another set that is > frozen at a point in time. > > On context switch, both sets of state have to be saved and (later) > restored. These two states are often called a variety of different > things. Common terms for the state which only exists after has entered > a transaction (performed a TBEGIN instruction) in hardware is the > 'transactional' or 'speculative'. > > Between a TBEGIN and a TEND or TABORT (or an event that causes the > hardware to abort), regardless of the use of TSUSPEND the > transactional state can be referred to as the live state. > > The second state is often to referred to as the 'checkpointed' state > and is a duplication of the live state when the TBEGIN instruction is > executed. This state is kept in the hardware and will be rolled back > to on transaction failure. > > Currently all the registers stored in pt_regs are ALWAYS the live > registers, that is, when a thread has transactional registers their > values are stored in pt_regs and the checkpointed state is in > ckpt_regs. A strange opposite is true for fp_state. When a thread is > non transactional fp_state holds the live registers. When a thread > has initiated a transaction fp_state holds the checkpointed state and > transact_fp becomes the structure which holds the live state (at this > point it is a transactional state). The same is true for vr_state > > This method creates confusion as to where the live state is, in some > circumstances it requires extra work to determine where to put the > live state and prevents the use of common functions designed (probably > before TM) to save the live state. > > With this patch pt_regs, fp_state and vr_state all represent the > same thing and the other structures [pending rename] are for > checkpointed state. > > Signed-off-by: Cyril Bur Acked-by: Simon Guo Thanks, - Simon
Re: [PATCH v2 18/20] powerpc: tm: Always use fp_state and vr_state to store live registers
Hi Cyril, On Mon, Aug 15, 2016 at 05:25:53PM +1000, Cyril Bur wrote: > > There are 2 "giveall_all()" in above path: > > __switch_to() > > giveup_all() // first time > > __switch_to_tm() > > tm_reclaim_task() > > tm_reclaim_thread() > > giveup_all() // again > > We should remove the one in __switch_to(). > > I don't think that will be possible, if a thread is not transactional > the second giveup_all() won't get called so we'll need to preserve the > one in __switch_to(). > I don't think removing the second is a good idea as we can enter > tm_reclaim_thread() from other means than __switch_to_tm(). > I did think that perhaps __switch_to_tm() could call giveup_all() in > the non transactional case but on reflection, doing nothing on the non > transactional case is cleaner. > The two calls are annoying but in the case where two calls are made, > the second should realise that nothing needs to be done and exit > quickly. Ah, yes. I somehow ignored non-transactional case > > > And another question, for following code in tm_reclaim_thread(): > > /* Having done the reclaim, we now have the checkpointed > > * FP/VSX values in the registers. These might be valid > > * even if we have previously called enable_kernel_fp() or > > * flush_fp_to_thread(), so update thr->regs->msr to > > * indicate their current validity. > > */ > > thr->regs->msr |= msr_diff; > > > > Does it imply the task being switched out of CPU, with TIF_RESTORE_TM > > bit set, might end with MSR_FP enabled? (I thought MSR_FP should > > not > > be enabled for a switched out task, as specified in > > flush_fp_to_thread()) > > Correct! I mistakenly thought it was solving a problem but you're > right. What you say is correct but it breaks signals, I've been > hesitating on how to get signals to work with the correct solution > here, I wasn't happy with my ideas but looks like it's pretty much the > only way to go unfortunately. Currently there will be an oops on flush_fp_to_thread() with this patch, when ptrace a transaction application. I think originally there is no giveup_all() call before tm_reclaim_thread(), so TIF_RESTORE_TM bit is not set. And finally thr->regs->msr is not marked with MSR_FP enabled. [ 363.473851] kernel BUG at arch/powerpc/kernel/process.c:191! [ 363.474242] Oops: Exception in kernel mode, sig: 5 [#1] [ 363.474595] SMP NR_CPUS=2048 NUMA pSeries [ 363.474956] Modules linked in: isofs sg virtio_balloon ip_tables xfs libcrc32c sr_mod cdrom sd_mod virtio_net ibmvscsi scsi_transport_srp virtio_pci virtio_ring virtio [ 363.476021] CPU: 0 PID: 3018 Comm: ptrace-tm-gpr Not tainted 4.8.0-rc1+ #5 [ 363.476547] task: c00072c66b80 task.stack: c0007e64c000 [ 363.476949] NIP: c0017650 LR: c000dc10 CTR: c0011538 [ 363.477428] REGS: c0007e64f960 TRAP: 0700 Not tainted (4.8.0-rc1+) [ 363.477948] MSR: 80029033 CR: 4288 XER: 2000 [ 363.478488] CFAR: c0017620 SOFTE: 1 GPR00: c000dc10 c0007e64fbe0 c0f3d800 c00072c65600 GPR04: c08706a8 0108 GPR08: 010008410180 0001 0001 c0870f18 GPR12: cfe8 GPR16: GPR20: GPR24: 010008410180 GPR28: 0108 c00072c65600 [ 363.482451] NIP [c0017650] flush_fp_to_thread+0x50/0x70 [ 363.482855] LR [c000dc10] fpr_get+0x40/0x120 [ 363.483194] Call Trace: [ 363.483362] [c0007e64fbe0] [c0007e64fcf0] 0xc0007e64fcf0 (unreliable) [ 363.483872] [c0007e64fc00] [c000dc10] fpr_get+0x40/0x120 [ 363.484304] [c0007e64fd60] [c0011578] arch_ptrace+0x628/0x7e0 [ 363.485811] [c0007e64fde0] [c00c3340] SyS_ptrace+0xa0/0x180 [ 363.486276] [c0007e64fe30] [c0009404] system_call+0x38/0xec [ 363.486725] Instruction dump: [ 363.486927] 40820010 4e800020 6000 6042 7c0802a6 f8010010 f821ffe1 e92d0210 [ 363.487459] 7c694a78 7d290074 7929d182 69290001 <0b09> 4b25 38210020 e8010010 [ 363.488005] ---[ end trace 58650a99c675b48c ]--- [ 363.490137] [ 365.490279] Kernel panic - not syncing: Fatal exception Thanks, - Simon
Re: [PATCH v2 18/20] powerpc: tm: Always use fp_state and vr_state to store live registers
On Fri, Aug 12, 2016 at 09:28:17AM +1000, Cyril Bur wrote: > @@ -846,7 +834,9 @@ static void tm_reclaim_thread(struct thread_struct *thr, > if (!MSR_TM_SUSPENDED(mfmsr())) > return; > > - tm_reclaim(thr, thr->regs->msr, cause); > + giveup_all(container_of(thr, struct task_struct, thread)); > + > + tm_reclaim(thr, msr_diff, cause); > > /* Having done the reclaim, we now have the checkpointed >* FP/VSX values in the registers. These might be valid > @@ -1189,11 +1171,11 @@ struct task_struct *__switch_to(struct task_struct > *prev, >*/ > save_sprs(&prev->thread); > > - __switch_to_tm(prev); > - > /* Save FPU, Altivec, VSX and SPE state */ > giveup_all(prev); > > + __switch_to_tm(prev, new); > + > /* >* We can't take a PMU exception inside _switch() since there is a >* window where the kernel stack SLB and the kernel stack are out There are 2 "giveall_all()" in above path: __switch_to() giveup_all() // first time __switch_to_tm() tm_reclaim_task() tm_reclaim_thread() giveup_all() // again We should remove the one in __switch_to(). And another question, for following code in tm_reclaim_thread(): /* Having done the reclaim, we now have the checkpointed * FP/VSX values in the registers. These might be valid * even if we have previously called enable_kernel_fp() or * flush_fp_to_thread(), so update thr->regs->msr to * indicate their current validity. */ thr->regs->msr |= msr_diff; Does it imply the task being switched out of CPU, with TIF_RESTORE_TM bit set, might end with MSR_FP enabled? (I thought MSR_FP should not be enabled for a switched out task, as specified in flush_fp_to_thread()) Thanks, - Simon
Re: [PATCH] ppc64: allow ptrace to set TM bits
Hi Laurent,
On Fri, Jul 29, 2016 at 11:51:22AM +0200, Laurent Dufour wrote:
> static int set_user_msr(struct task_struct *task, unsigned long msr)
> {
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> + if (!(task->thread.regs->msr & MSR_TM)) {
> + /* If TM is not available, discard TM bits changes */
> + msr &= ~(MSR_TM | MSR_TS_MASK);
> + }
> +#endif
I am not sure whether following is an issue:
Per PowerISA, any exception/interrupt will disable MSR[TM] bit
automatically and mark MSR_TS to be suspended when it is
transactional. It is possible that MSR[TM] = 0 and MSR[MSR_TS] != 0
(suspended).
Will set_user_msr() be able to escape from the above?
For example, one user space application encountered
page fault during transaction, its task->thread.regs->msr & MSR_TM == 0
and MSR[MSR_TS] == suspended. Then it is being traced and
set_user_msr() is invoked on it. I think it will be incorrect to
clear its MSR_TS_MASK bits.
Please correct me if I am wrong.
Thanks,
- Simon
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [v4] powerpc: Export thread_struct.used_vr/used_vsr to user space
On Thu, Jul 21, 2016 at 08:57:29PM +1000, Michael Ellerman wrote: > Can one of you send a properly formatted and signed-off patch. I will work on that. Thanks, Simon ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/5] powerpc: tm: Always use fp_state and vr_state to store live registers
On Mon, Jul 18, 2016 at 11:28:30AM +1000, Cyril Bur wrote: > On Sun, 17 Jul 2016 11:25:43 +0800 > > The aim of this patch is to ensure that pt_regs, fp_state and vr_state always > hold a threads 'live' registers. So, after a recheckpoint fp_state is where > the > the state should be. tm_reclaim_thread() does a save_all() before doing the > reclaim. > > This means that the call to restore_math() is a replacement for all deleted > lines above it. > > I added it here because I'd prefer to be safe but I left that comment in > because I suspect restore_math() will be called later and we can get away with > not calling it here. > > > And, should the thread's MSR now set FP bit in tm_recheckpoint(), to > > indicate that FP register content is "fresh" in contrast to thread.fp_state? > > > > I'm not sure what you mean by 'fresh'. You do highlight that we'll have to be > sure that the MSR bits are off (so that restore_math() doesn't assume the > registers are already loaded) which makes me think that tm_reclaim_thread() > should be doing a giveup_all(), I'll fix that. > > I hope that helps, > Thanks Cyril. The explanation is detail and helpful. - Simon ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/5] powerpc: tm: Always use fp_state and vr_state to store live registers
Hi Cyril,
On Wed, Jun 08, 2016 at 02:00:34PM +1000, Cyril Bur wrote:
> @@ -917,24 +907,10 @@ static inline void tm_recheckpoint_new_task(struct
> task_struct *new)
>"(new->msr 0x%lx, new->origmsr 0x%lx)\n",
>new->pid, new->thread.regs->msr, msr);
>
> - /* This loads the checkpointed FP/VEC state, if used */
> tm_recheckpoint(&new->thread, msr);
>
> - /* This loads the speculative FP/VEC state, if used */
> - if (msr & MSR_FP) {
> - do_load_up_transact_fpu(&new->thread);
> - new->thread.regs->msr |=
> - (MSR_FP | new->thread.fpexc_mode);
> - }
> -#ifdef CONFIG_ALTIVEC
> - if (msr & MSR_VEC) {
> - do_load_up_transact_altivec(&new->thread);
> - new->thread.regs->msr |= MSR_VEC;
> - }
> -#endif
> - /* We may as well turn on VSX too since all the state is restored now */
> - if (msr & MSR_VSX)
> - new->thread.regs->msr |= MSR_VSX;
> + /* Won't restore math get called later? */
> + restore_math(new->thread.regs);
I have some question for the "restore_math" in tm_recheckpoint_new_task().
Per my understanding, now after tm_recheckpoint, the fp_state content
is obsolete. However restore_math() will try to restore FP/Vec/VSX state
from fp_state, (orginally it is right, since fp_state was the valid
checkpointed state and consistent with FP register ). Should we remove
the restore_math() here?
And, should the thread's MSR now set FP bit in tm_recheckpoint(), to
indicate that FP register content is "fresh" in contrast to thread.fp_state?
Please correct me if I am wrong.
Thanks,
- Simon
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [v4] powerpc: Export thread_struct.used_vr/used_vsr to user space
Michael, Ben, On Fri, Jul 08, 2016 at 08:02:42PM +1000, Michael Ellerman wrote: > Benjamin Herrenschmidt writes: > > > On Thu, 2016-07-07 at 23:21 +1000, Benjamin Herrenschmidt wrote: > >> > >> I think the right fix is that if a restore_sigcontext() has the MSR > >> bits set, > >> it should set the corresponding used_* flag. > > > > Something like this: > > > > (totally untested) > > Simon/Laurent, can you guys test this and let me know if it works for > your usecase. > Sorry for the late reply. I tested Ben's patch and the fix works for the CRIU case mentioned before. Thanks, Simon ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [v4] powerpc: Export thread_struct.used_vr/used_vsr to user space
On Fri, Jul 08, 2016 at 08:02:42PM +1000, Michael Ellerman wrote: > Benjamin Herrenschmidt writes: > > > On Thu, 2016-07-07 at 23:21 +1000, Benjamin Herrenschmidt wrote: > >> > >> I think the right fix is that if a restore_sigcontext() has the MSR > >> bits set, > >> it should set the corresponding used_* flag. > > > > Something like this: > > > > (totally untested) > > Simon/Laurent, can you guys test this and let me know if it works for > your usecase. Just notice this note. Yes I will test it on CRIU and update later. Thanks, - Simon ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [v4] powerpc: Export thread_struct.used_vr/used_vsr to user space
On Thu, Jul 07, 2016 at 11:21:18PM +1000, Benjamin Herrenschmidt wrote: > I think the right fix is that if a restore_sigcontext() has the MSR bits set, > it should set the corresponding used_* flag. > > Or is there a reason why that won't work ? That sounds reaonable to me. I will prepare a patch based on that. Michael, Ben, Laurent, Thanks the discussion and proposal. - Simon ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RESEND, v2] powerpc: Export thread_struct.used_vr/used_vsr to user space
Hi Michael,
On Tue, Jul 05, 2016 at 03:40:40PM +1000, Michael Ellerman wrote:
> On Wed, 2016-06-04 at 07:00:12 UTC, Simon Guo wrote:
> > These 2 fields track whether user process has used Altivec/VSX
> > registers or not. They are used by kernel to setup signal frame
> > on user stack correctly regarding vector part.
> >
> > CRIU(Checkpoint and Restore In User space) builds signal frame
> > for restored process. It will need this export information to
> > setup signal frame correctly. And CRIU will need to restore these
> > 2 fields for the restored process.
> >
> > Signed-off-by: Simon Guo
> > Reviewed-by: Laurent Dufour
> > @@ -176,6 +176,17 @@ struct pt_regs {
> > #define PTRACE_GETREGS64 0x16
> > #define PTRACE_SETREGS64 0x17
> >
> > +/*
> > + * Get or set some register used bit.
> > + * The flags will be saved in a 32 bit data.
> > + * Currently it is only used for VR/VSR usage.
> > + */
> > +#define PTRACE_GET_REGS_USAGE0x1e
> > +#define PTRACE_SET_REGS_USAGE0x1f
> > +
> > +#define PTRACE_REGS_USAGE_VR_BIT 0x0001
> > +#define PTRACE_REGS_USAGE_VSR_BIT 0x0002
>
>
> It looks like you just made up this new ptrace ABI ?
>
> Or is it used on other arches ? (no AFAICS)
>
> How do other arches handle this ?
>
> I'm a bit wary of adding new ptrace ABIs.
>
> If we do want to do this, I'd at least think the mask should be u64, to give
> us
> more capacity to add new registers.
>
> cheers
It is only used on PowerPc currently. I had better rename
it to:
#define PPC_PTRACE_GET_REGS_USAGE0x96
#define PPC_PTRACE_SET_REGS_USAGE0x97
I will change the mask into u64.
Thanks,
- Simon
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/5] powerpc: tm: Always use fp_state and vr_state to store live registers
hi Cyril, On Wed, Jun 08, 2016 at 02:00:34PM +1000, Cyril Bur wrote: > @@ -1108,11 +1084,11 @@ struct task_struct *__switch_to(struct task_struct > *prev, >*/ > save_sprs(&prev->thread); > > - __switch_to_tm(prev); > - > /* Save FPU, Altivec, VSX and SPE state */ > giveup_all(prev); > > + __switch_to_tm(prev); > + There should be a bug. giveup_all() will clear MSR[FP] bit. __switch_to_tm() reads that bit to decide whether the FP register needs to be flushed to thread_struct. === tm_reclaim() (invoked by __switch_to_tm) andi. r0, r4, MSR_FP beq dont_backup_fp addir7, r3, THREAD_CKFPSTATE SAVE_32FPRS_VSRS(0, R6, R7) /* r6 scratch, r7 transact fp state */ mffsfr0 stfdfr0,FPSTATE_FPSCR(r7) dont_backup_fp: = But now the __switch_to_tm() is moved behind giveup_all(). So __switch_to_tm() loses MSR[FP] and cannot decide whether saving ckpt FPU or not. The same applies to VMX/VSX. Thanks, - Simon ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RESEND][PATCH v2] powerpc: Export thread_struct.used_vr/used_vsr to user space
On Tue, Jun 21, 2016 at 02:30:06PM +0800, Simon Guo wrote: > Hi Michael, > On Wed, Apr 06, 2016 at 03:00:12PM +0800, Simon Guo wrote: > > These 2 fields track whether user process has used Altivec/VSX > > registers or not. They are used by kernel to setup signal frame > > on user stack correctly regarding vector part. > > > > CRIU(Checkpoint and Restore In User space) builds signal frame > > for restored process. It will need this export information to > > setup signal frame correctly. And CRIU will need to restore these > > 2 fields for the restored process. > > > > Signed-off-by: Simon Guo > > Reviewed-by: Laurent Dufour > > --- > > Just a kind reminder per our previous discussion. > If possible, please help pull this in during 4.8 merge window. Some CRIU work > items are pending for it. > > Have a nice day. > > Thanks, > - Simon + linuxppc-dev list Thanks, - Simon (IBM LTC) ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RESEND][PATCH v2] powerpc: Export thread_struct.used_vr/used_vsr to user space
Hi Michael, On Wed, Apr 06, 2016 at 03:00:12PM +0800, Simon Guo wrote: > These 2 fields track whether user process has used Altivec/VSX > registers or not. They are used by kernel to setup signal frame > on user stack correctly regarding vector part. > > CRIU(Checkpoint and Restore In User space) builds signal frame > for restored process. It will need this export information to > setup signal frame correctly. And CRIU will need to restore these > 2 fields for the restored process. > > Signed-off-by: Simon Guo > Reviewed-by: Laurent Dufour > --- Is there any chance that this patch can be merged upstream soon? The corresponding work of CRIU has to follow after that. Thanks, Simon ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[RESEND][PATCH v2] powerpc: Export thread_struct.used_vr/used_vsr to user space
These 2 fields track whether user process has used Altivec/VSX
registers or not. They are used by kernel to setup signal frame
on user stack correctly regarding vector part.
CRIU(Checkpoint and Restore In User space) builds signal frame
for restored process. It will need this export information to
setup signal frame correctly. And CRIU will need to restore these
2 fields for the restored process.
Signed-off-by: Simon Guo
Reviewed-by: Laurent Dufour
---
arch/powerpc/include/uapi/asm/ptrace.h | 11 ++
arch/powerpc/kernel/ptrace.c | 39 ++
arch/powerpc/kernel/ptrace32.c | 2 ++
3 files changed, 52 insertions(+)
diff --git a/arch/powerpc/include/uapi/asm/ptrace.h
b/arch/powerpc/include/uapi/asm/ptrace.h
index 8036b38..d5afe95 100644
--- a/arch/powerpc/include/uapi/asm/ptrace.h
+++ b/arch/powerpc/include/uapi/asm/ptrace.h
@@ -176,6 +176,17 @@ struct pt_regs {
#define PTRACE_GETREGS64 0x16
#define PTRACE_SETREGS64 0x17
+/*
+ * Get or set some register used bit.
+ * The flags will be saved in a 32 bit data.
+ * Currently it is only used for VR/VSR usage.
+ */
+#define PTRACE_GET_REGS_USAGE0x1e
+#define PTRACE_SET_REGS_USAGE0x1f
+
+#define PTRACE_REGS_USAGE_VR_BIT 0x0001
+#define PTRACE_REGS_USAGE_VSR_BIT 0x0002
+
/* Calls to trace a 64bit program from a 32bit program */
#define PPC_PTRACE_PEEKTEXT_3264 0x95
#define PPC_PTRACE_PEEKDATA_3264 0x94
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 30a03c0..39b8ff2 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -1755,6 +1755,45 @@ long arch_ptrace(struct task_struct *child, long request,
REGSET_SPE, 0, 35 * sizeof(u32),
datavp);
#endif
+ case PTRACE_GET_REGS_USAGE:
+ {
+ u32 *u32_datap = (u32 *)datavp;
+ u32 reg_usage = 0;
+
+ if (addr != sizeof(u32))
+ return -EINVAL;
+
+#ifdef CONFIG_ALTIVEC
+ if (child->thread.used_vr)
+ reg_usage |= PTRACE_REGS_USAGE_VR_BIT;
+#endif
+#ifdef CONFIG_VSX
+ if (child->thread.used_vsr)
+ reg_usage |= PTRACE_REGS_USAGE_VSR_BIT;
+#endif
+ return put_user(reg_usage, u32_datap);
+ }
+ case PTRACE_SET_REGS_USAGE:
+ {
+ u32 *u32_datap = (u32 *)datavp;
+ u32 reg_usage = 0;
+
+ if (addr != sizeof(u32))
+ return -EINVAL;
+
+ ret = get_user(reg_usage, u32_datap);
+ if (ret)
+ return ret;
+#ifdef CONFIG_ALTIVEC
+ child->thread.used_vr =
+ !!(reg_usage & PTRACE_REGS_USAGE_VR_BIT);
+#endif
+#ifdef CONFIG_VSX
+ child->thread.used_vsr =
+ !!(reg_usage & PTRACE_REGS_USAGE_VSR_BIT);
+#endif
+ break;
+ }
default:
ret = ptrace_request(child, request, addr, data);
diff --git a/arch/powerpc/kernel/ptrace32.c b/arch/powerpc/kernel/ptrace32.c
index f52b7db..ff359a1 100644
--- a/arch/powerpc/kernel/ptrace32.c
+++ b/arch/powerpc/kernel/ptrace32.c
@@ -305,6 +305,8 @@ long compat_arch_ptrace(struct task_struct *child,
compat_long_t request,
case PPC_PTRACE_GETHWDBGINFO:
case PPC_PTRACE_SETHWDEBUG:
case PPC_PTRACE_DELHWDEBUG:
+ case PTRACE_GET_REGS_USAGE:
+ case PTRACE_SET_REGS_USAGE:
ret = arch_ptrace(child, request, addr, data);
break;
--
2.7.0
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] powerpc: Export thread_struct.used_vr/used_vsr to user space
These 2 fields track whether user process has used Altivec/VSX registers
or not. They are used by kernel to setup signal frame on user stack
correctly regarding vector part.
CRIU(Checkpoint and Restore In User space) builds signal frame for
restored process. It will need this export information to setup signal
frame correctly. And CRIU will need to restore these 2 fields for
the restored process.
Signed-off-by: Simon Guo
Reviewed-by: Laurent Dufour
---
arch/powerpc/include/uapi/asm/ptrace.h | 11 ++
arch/powerpc/kernel/ptrace.c | 39 ++
arch/powerpc/kernel/ptrace32.c | 2 ++
3 files changed, 52 insertions(+)
diff --git a/arch/powerpc/include/uapi/asm/ptrace.h
b/arch/powerpc/include/uapi/asm/ptrace.h
index 8036b38..4456b8f 100644
--- a/arch/powerpc/include/uapi/asm/ptrace.h
+++ b/arch/powerpc/include/uapi/asm/ptrace.h
@@ -176,6 +176,17 @@ struct pt_regs {
#define PTRACE_GETREGS64 0x16
#define PTRACE_SETREGS64 0x17
+/*
+ * Get or set some register used bit.
+ * The flags will be saved in a 32 bit data.
+ * Currently it is only used for VR/VSR usage.
+ */
+#define PTRACE_GET_REGS_USAGE0x1e
+#define PTRACE_SET_REGS_USAGE0x1f
+
+#define PTRACE_REGS_USAGE_BIT_VR 0x0
+#define PTRACE_REGS_USAGE_BIT_VSR 0x1
+
/* Calls to trace a 64bit program from a 32bit program */
#define PPC_PTRACE_PEEKTEXT_3264 0x95
#define PPC_PTRACE_PEEKDATA_3264 0x94
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 30a03c0..43d3db3b 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -1755,6 +1755,45 @@ long arch_ptrace(struct task_struct *child, long request,
REGSET_SPE, 0, 35 * sizeof(u32),
datavp);
#endif
+ case PTRACE_GET_REGS_USAGE:
+ {
+ u32 *u32_datap = (u32 *)datavp;
+ u32 reg_usage = 0;
+
+ if (addr != sizeof(u32))
+ return -EINVAL;
+
+#ifdef CONFIG_ALTIVEC
+ if (child->thread.used_vr)
+ reg_usage |= (1 << PTRACE_REGS_USAGE_BIT_VR);
+#endif
+#ifdef CONFIG_VSX
+ if (child->thread.used_vsr)
+ reg_usage |= (1 << PTRACE_REGS_USAGE_BIT_VSR);
+#endif
+ return put_user(reg_usage, u32_datap);
+ }
+ case PTRACE_SET_REGS_USAGE:
+ {
+ u32 *u32_datap = (u32 *)datavp;
+ u32 reg_usage = 0;
+
+ if (addr != sizeof(u32))
+ return -EINVAL;
+
+ ret = get_user(reg_usage, u32_datap);
+ if (ret)
+ return ret;
+#ifdef CONFIG_ALTIVEC
+ child->thread.used_vr =
+ ((reg_usage >> PTRACE_REGS_USAGE_BIT_VR) & 1);
+#endif
+#ifdef CONFIG_VSX
+ child->thread.used_vsr =
+ ((reg_usage >> PTRACE_REGS_USAGE_BIT_VSR) & 1);
+#endif
+ break;
+ }
default:
ret = ptrace_request(child, request, addr, data);
diff --git a/arch/powerpc/kernel/ptrace32.c b/arch/powerpc/kernel/ptrace32.c
index f52b7db..ff359a1 100644
--- a/arch/powerpc/kernel/ptrace32.c
+++ b/arch/powerpc/kernel/ptrace32.c
@@ -305,6 +305,8 @@ long compat_arch_ptrace(struct task_struct *child,
compat_long_t request,
case PPC_PTRACE_GETHWDBGINFO:
case PPC_PTRACE_SETHWDEBUG:
case PPC_PTRACE_DELHWDEBUG:
+ case PTRACE_GET_REGS_USAGE:
+ case PTRACE_SET_REGS_USAGE:
ret = arch_ptrace(child, request, addr, data);
break;
--
2.7.0
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] powerpc: Export thread_struct.used_vr/used_vsr to user space
These 2 fields track whether user process has used Altivec/VSX registers
or not. They are used by kernel to setup signal frame on user stack
correctly regarding vector part.
CRIU(Checkpoint and Restore In User space) builds signal frame for
restored process. It will need this export information to setup signal
frame correctly. And CRIU will need to restore these 2 fields for
the restored process.
Signed-off-by: Simon Guo
Reviewed-by: Laurent Dufour
---
arch/powerpc/include/uapi/asm/ptrace.h | 11 ++
arch/powerpc/kernel/ptrace.c | 39 ++
arch/powerpc/kernel/ptrace32.c | 2 ++
3 files changed, 52 insertions(+)
diff --git a/arch/powerpc/include/uapi/asm/ptrace.h
b/arch/powerpc/include/uapi/asm/ptrace.h
index 8036b38..4456b8f 100644
--- a/arch/powerpc/include/uapi/asm/ptrace.h
+++ b/arch/powerpc/include/uapi/asm/ptrace.h
@@ -176,6 +176,17 @@ struct pt_regs {
#define PTRACE_GETREGS64 0x16
#define PTRACE_SETREGS64 0x17
+/*
+ * Get or set some register used bit.
+ * The flags will be saved in a 32 bit data.
+ * Currently it is only used for VR/VSR usage.
+ */
+#define PTRACE_GET_REGS_USAGE0x1e
+#define PTRACE_SET_REGS_USAGE0x1f
+
+#define PTRACE_REGS_USAGE_BIT_VR 0x0
+#define PTRACE_REGS_USAGE_BIT_VSR 0x1
+
/* Calls to trace a 64bit program from a 32bit program */
#define PPC_PTRACE_PEEKTEXT_3264 0x95
#define PPC_PTRACE_PEEKDATA_3264 0x94
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 30a03c0..43d3db3b 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -1755,6 +1755,45 @@ long arch_ptrace(struct task_struct *child, long request,
REGSET_SPE, 0, 35 * sizeof(u32),
datavp);
#endif
+ case PTRACE_GET_REGS_USAGE:
+ {
+ u32 *u32_datap = (u32 *)datavp;
+ u32 reg_usage = 0;
+
+ if (addr != sizeof(u32))
+ return -EINVAL;
+
+#ifdef CONFIG_ALTIVEC
+ if (child->thread.used_vr)
+ reg_usage |= (1 << PTRACE_REGS_USAGE_BIT_VR);
+#endif
+#ifdef CONFIG_VSX
+ if (child->thread.used_vsr)
+ reg_usage |= (1 << PTRACE_REGS_USAGE_BIT_VSR);
+#endif
+ return put_user(reg_usage, u32_datap);
+ }
+ case PTRACE_SET_REGS_USAGE:
+ {
+ u32 *u32_datap = (u32 *)datavp;
+ u32 reg_usage = 0;
+
+ if (addr != sizeof(u32))
+ return -EINVAL;
+
+ ret = get_user(reg_usage, u32_datap);
+ if (ret)
+ return ret;
+#ifdef CONFIG_ALTIVEC
+ child->thread.used_vr =
+ ((reg_usage >> PTRACE_REGS_USAGE_BIT_VR) & 1);
+#endif
+#ifdef CONFIG_VSX
+ child->thread.used_vsr =
+ ((reg_usage >> PTRACE_REGS_USAGE_BIT_VSR) & 1);
+#endif
+ break;
+ }
default:
ret = ptrace_request(child, request, addr, data);
diff --git a/arch/powerpc/kernel/ptrace32.c b/arch/powerpc/kernel/ptrace32.c
index f52b7db..ff359a1 100644
--- a/arch/powerpc/kernel/ptrace32.c
+++ b/arch/powerpc/kernel/ptrace32.c
@@ -305,6 +305,8 @@ long compat_arch_ptrace(struct task_struct *child,
compat_long_t request,
case PPC_PTRACE_GETHWDBGINFO:
case PPC_PTRACE_SETHWDEBUG:
case PPC_PTRACE_DELHWDEBUG:
+ case PTRACE_GET_REGS_USAGE:
+ case PTRACE_SET_REGS_USAGE:
ret = arch_ptrace(child, request, addr, data);
break;
--
2.7.0
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] powerpc: correct VSX used_vsr comment
used_vsr flag is set if process has used VSX register,
instead of Altivec register.
This patch corrects the wrong comment.
Signed-off-by: Simon Guo
---
arch/powerpc/include/asm/processor.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/processor.h
b/arch/powerpc/include/asm/processor.h
index ac23308..a3a25e4 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -244,7 +244,7 @@ struct thread_struct {
#endif /* CONFIG_ALTIVEC */
#ifdef CONFIG_VSX
/* VSR status */
- int used_vsr; /* set if process has used altivec */
+ int used_vsr; /* set if process has used VSX */
#endif /* CONFIG_VSX */
#ifdef CONFIG_SPE
unsigned long evr[32];/* upper 32-bits of SPE regs */
--
1.8.3.1
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
