linux-kernel - Re: [PATCH v2 2/9] powerpc/kvm: make hypervisor state restore a function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 18 May 2016 12:37:56 +0530
From:	Shreyas B Prabhu <shreyas@...ux.vnet.ibm.com>
To:	ego@...ux.vnet.ibm.com
CC:	mpe@...erman.id.au, linuxppc-dev@...ts.ozlabs.org,
	paulus@...abs.org, linux-kernel@...r.kernel.org, mikey@...ling.org
Subject: Re: [PATCH v2 2/9] powerpc/kvm: make hypervisor state restore a function

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
>>