Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1402736 > unrolled thread
| Started by | Gautham R Shenoy <ego@linux.vnet.ibm.com> |
|---|---|
| First post | 2016-05-18 08:30 +0200 |
| Last post | 2016-05-19 16:30 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v2 2/9] powerpc/kvm: make hypervisor state restore a function Gautham R Shenoy <ego@linux.vnet.ibm.com> - 2016-05-18 08:30 +0200
Re: [PATCH v2 2/9] powerpc/kvm: make hypervisor state restore a function Shreyas B Prabhu <shreyas@linux.vnet.ibm.com> - 2016-05-18 09:10 +0200
Re: [PATCH v2 2/9] powerpc/kvm: make hypervisor state restore a function Gautham R Shenoy <ego@linux.vnet.ibm.com> - 2016-05-19 16:30 +0200
| From | Gautham R Shenoy <ego@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-05-18 08:30 +0200 |
| Subject | Re: [PATCH v2 2/9] powerpc/kvm: make hypervisor state restore a function |
| Message-ID | <rA39v-6Ng-5@gated-at.bofh.it> |
Hi Shreyas, On Tue, May 03, 2016 at 01:54:31PM +0530, Shreyas B. Prabhu wrote: > In the current code, when the thread wakes up in reset vector, some > of the state restore code and check for whether a thread needs to > branch to kvm is duplicated. Reorder the code such that this > duplication is avoided. > > At a higher level this is what the change looks like- I have manually verified that the code flow in the new patch is has the same effect as whatever we were doing earlier. There a couple of comments inline. > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S > index 7716ceb..7ebfbb0 100644 > --- a/arch/powerpc/kernel/exceptions-64s.S > +++ b/arch/powerpc/kernel/exceptions-64s.S > @@ -107,25 +107,8 @@ BEGIN_FTR_SECTION > beq 9f > > cmpwi cr3,r13,2 > + bl power7_restore_hyp_resource > > - /* > - * Check if last bit of HSPGR0 is set. This indicates whether we are > - * waking up from winkle. > - */ > - GET_PACA(r13) > - clrldi r5,r13,63 > - clrrdi r13,r13,1 > - cmpwi cr4,r5,1 > - mtspr SPRN_HSPRG0,r13 > - > - lbz r0,PACA_THREAD_IDLE_STATE(r13) > - cmpwi cr2,r0,PNV_THREAD_NAP > - bgt cr2,8f /* Either sleep or Winkle */ > - > - /* Waking up from nap should not cause hypervisor state loss */ > - bgt cr3,. > - > - /* Waking up from nap */ > li r0,PNV_THREAD_RUNNING > stb r0,PACA_THREAD_IDLE_STATE(r13) /* Clear thread state */ > > @@ -143,13 +126,9 @@ BEGIN_FTR_SECTION > > /* Return SRR1 from power7_nap() */ > mfspr r3,SPRN_SRR1 > - beq cr3,2f > - b power7_wakeup_noloss > -2: b power7_wakeup_loss > - > - /* Fast Sleep wakeup on PowerNV */ > -8: GET_PACA(r13) In the old code, we do a GET_PACA(r13) before invoking the power7_wakeup_tb_loss. In the new code we don't. Can you explain this omission ? [..snip..] > @@ -420,33 +451,9 @@ common_exit: > > hypervisor_state_restored: > > - li r5,PNV_THREAD_RUNNING > - stb r5,PACA_THREAD_IDLE_STATE(r13) > - > mtspr SPRN_SRR1,r16 > -#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE > - li r0,KVM_HWTHREAD_IN_KERNEL > - stb r0,HSTATE_HWTHREAD_STATE(r13) > - /* Order setting hwthread_state vs. testing hwthread_req */ > - sync > - lbz r0,HSTATE_HWTHREAD_REQ(r13) > - cmpwi r0,0 > - beq 6f > - b kvm_start_guest > -6: > -#endif > - > - REST_NVGPRS(r1) > - REST_GPR(2, r1) > - ld r3,_CCR(r1) > - ld r4,_MSR(r1) > - ld r5,_NIP(r1) > - addi r1,r1,INT_FRAME_SIZE > - mtcr r3 > - mfspr r3,SPRN_SRR1 /* Return SRR1 */ > - mtspr SPRN_SRR1,r4 > - mtspr SPRN_SRR0,r5 > - rfid > + mtlr r17 > + blr Perhaps you could add a comment against this blr to indicate that we go back to the reset vector right after the call to power7_restore_hyp_resource. > > fastsleep_workaround_at_exit: > li r3,1 > -- > 2.4.11 >
[toc] | [next] | [standalone]
| From | Shreyas B Prabhu <shreyas@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-05-18 09:10 +0200 |
| Subject | Re: [PATCH v2 2/9] powerpc/kvm: make hypervisor state restore a function |
| Message-ID | <rA3Md-7f3-5@gated-at.bofh.it> |
| In reply to | #1402736 |
Hi Gautham, On 05/18/2016 11:55 AM, Gautham R Shenoy wrote: > Hi Shreyas, > > On Tue, May 03, 2016 at 01:54:31PM +0530, Shreyas B. Prabhu wrote: >> In the current code, when the thread wakes up in reset vector, some >> of the state restore code and check for whether a thread needs to >> branch to kvm is duplicated. Reorder the code such that this >> duplication is avoided. >> >> At a higher level this is what the change looks like- > > I have manually verified that the code flow in the new patch is has > the same effect as whatever we were doing earlier. There a couple of > comments inline. > Thanks for the review! >> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S >> index 7716ceb..7ebfbb0 100644 >> --- a/arch/powerpc/kernel/exceptions-64s.S >> +++ b/arch/powerpc/kernel/exceptions-64s.S >> @@ -107,25 +107,8 @@ BEGIN_FTR_SECTION >> beq 9f >> >> cmpwi cr3,r13,2 >> + bl power7_restore_hyp_resource >> >> - /* >> - * Check if last bit of HSPGR0 is set. This indicates whether we are >> - * waking up from winkle. >> - */ >> - GET_PACA(r13) >> - clrldi r5,r13,63 >> - clrrdi r13,r13,1 >> - cmpwi cr4,r5,1 >> - mtspr SPRN_HSPRG0,r13 >> - >> - lbz r0,PACA_THREAD_IDLE_STATE(r13) >> - cmpwi cr2,r0,PNV_THREAD_NAP >> - bgt cr2,8f /* Either sleep or Winkle */ >> - >> - /* Waking up from nap should not cause hypervisor state loss */ >> - bgt cr3,. >> - >> - /* Waking up from nap */ >> li r0,PNV_THREAD_RUNNING >> stb r0,PACA_THREAD_IDLE_STATE(r13) /* Clear thread state */ >> >> @@ -143,13 +126,9 @@ BEGIN_FTR_SECTION >> >> /* Return SRR1 from power7_nap() */ >> mfspr r3,SPRN_SRR1 >> - beq cr3,2f >> - b power7_wakeup_noloss >> -2: b power7_wakeup_loss >> - >> - /* Fast Sleep wakeup on PowerNV */ >> -8: GET_PACA(r13) > > In the old code, we do a GET_PACA(r13) before invoking the > power7_wakeup_tb_loss. In the new code we don't. Can you explain > this omission ? GET_PACA(13) is the called in the beginning of power7_restore_hyp_resource. So r13 contains pointer to PACA when power7_wakeup_tb_loss invoked later in the same function. > > > [..snip..] > >> @@ -420,33 +451,9 @@ common_exit: >> >> hypervisor_state_restored: >> >> - li r5,PNV_THREAD_RUNNING >> - stb r5,PACA_THREAD_IDLE_STATE(r13) >> - >> mtspr SPRN_SRR1,r16 >> -#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE >> - li r0,KVM_HWTHREAD_IN_KERNEL >> - stb r0,HSTATE_HWTHREAD_STATE(r13) >> - /* Order setting hwthread_state vs. testing hwthread_req */ >> - sync >> - lbz r0,HSTATE_HWTHREAD_REQ(r13) >> - cmpwi r0,0 >> - beq 6f >> - b kvm_start_guest >> -6: >> -#endif >> - >> - REST_NVGPRS(r1) >> - REST_GPR(2, r1) >> - ld r3,_CCR(r1) >> - ld r4,_MSR(r1) >> - ld r5,_NIP(r1) >> - addi r1,r1,INT_FRAME_SIZE >> - mtcr r3 >> - mfspr r3,SPRN_SRR1 /* Return SRR1 */ >> - mtspr SPRN_SRR1,r4 >> - mtspr SPRN_SRR0,r5 >> - rfid >> + mtlr r17 >> + blr > > > Perhaps you could add a comment against this blr to indicate that we > go back to the reset vector right after the call to > power7_restore_hyp_resource. Ok. I'll do that. > >> >> fastsleep_workaround_at_exit: >> li r3,1 >> -- >> 2.4.11 >>
[toc] | [prev] | [next] | [standalone]
| From | Gautham R Shenoy <ego@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-05-19 16:30 +0200 |
| Message-ID | <rAx7E-Wa-3@gated-at.bofh.it> |
| In reply to | #1402753 |
Hi Shreyas, On Wed, May 18, 2016 at 12:37:56PM +0530, Shreyas B Prabhu wrote: [..snip..] > >> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S > >> index 7716ceb..7ebfbb0 100644 > >> --- a/arch/powerpc/kernel/exceptions-64s.S > >> +++ b/arch/powerpc/kernel/exceptions-64s.S > >> @@ -107,25 +107,8 @@ BEGIN_FTR_SECTION > >> beq 9f > >> > >> cmpwi cr3,r13,2 > >> + bl power7_restore_hyp_resource > >> > >> - /* > >> - * Check if last bit of HSPGR0 is set. This indicates whether we are > >> - * waking up from winkle. > >> - */ > >> - GET_PACA(r13) > >> - clrldi r5,r13,63 > >> - clrrdi r13,r13,1 > >> - cmpwi cr4,r5,1 > >> - mtspr SPRN_HSPRG0,r13 > >> - > >> - lbz r0,PACA_THREAD_IDLE_STATE(r13) > >> - cmpwi cr2,r0,PNV_THREAD_NAP > >> - bgt cr2,8f /* Either sleep or Winkle */ > >> - > >> - /* Waking up from nap should not cause hypervisor state loss */ > >> - bgt cr3,. > >> - > >> - /* Waking up from nap */ > >> li r0,PNV_THREAD_RUNNING > >> stb r0,PACA_THREAD_IDLE_STATE(r13) /* Clear thread state */ > >> > >> @@ -143,13 +126,9 @@ BEGIN_FTR_SECTION > >> > >> /* Return SRR1 from power7_nap() */ > >> mfspr r3,SPRN_SRR1 > >> - beq cr3,2f > >> - b power7_wakeup_noloss > >> -2: b power7_wakeup_loss > >> - > >> - /* Fast Sleep wakeup on PowerNV */ > >> -8: GET_PACA(r13) > > > > In the old code, we do a GET_PACA(r13) before invoking the > > power7_wakeup_tb_loss. In the new code we don't. Can you explain > > this omission ? > > GET_PACA(13) is the called in the beginning of > power7_restore_hyp_resource. So r13 contains pointer to PACA when > power7_wakeup_tb_loss invoked later in the same function. Ah, I see it now. So the GET_PACA(r13) at 8: was anyway redundant in the older code. You can add my Reviewed-by: to this patch. -- Thanks and Regards gautham.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web