linux-kernel - Re: [PATCH] KVM: arm64: fix misleading comments in save/restore

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:	Thu, 04 Jun 2015 11:01:41 +0100
From:	Marc Zyngier <marc.zyngier@....com>
To:	Christoffer Dall <christoffer.dall@...aro.org>,
	Alex Bennée 
	<alex.bennee@...aro.org>
CC:	"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"kvmarm@...ts.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>,
	Gleb Natapov <gleb@...nel.org>,
	Paolo Bonzini <pbonzini@...hat.com>,
	Catalin Marinas <Catalin.Marinas@....com>,
	Will Deacon <Will.Deacon@....com>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] KVM: arm64: fix misleading comments in save/restore

On 04/06/15 10:34, Christoffer Dall wrote:
> On Thu, May 28, 2015 at 10:43:06AM +0100, Alex Bennée wrote:
>> The elr_el2 and spsr_el2 registers in fact contain the processor state
>> before entry into the hypervisor code.
> 
> be careful with your use of the hypervisor, in the KVM design the
> hypervisor is split across EL1 and EL2.
> 
>> In the case of guest state it
>> could be in either el0 or el1.
> 
> true
> 
>>
>> Signed-off-by: Alex Bennée <alex.bennee@...aro.org>
>> ---
>>  arch/arm64/kvm/hyp.S | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>> index d755922..1940a4c 100644
>> --- a/arch/arm64/kvm/hyp.S
>> +++ b/arch/arm64/kvm/hyp.S
>> @@ -50,8 +50,8 @@
>>  	stp	x29, lr, [x3, #80]
>>  
>>  	mrs	x19, sp_el0
>> -	mrs	x20, elr_el2		// EL1 PC
>> -	mrs	x21, spsr_el2		// EL1 pstate
>> +	mrs	x20, elr_el2		// PC before hyp entry
>> +	mrs	x21, spsr_el2		// pstate before hyp entry
>>  
>>  	stp	x19, x20, [x3, #96]
>>  	str	x21, [x3, #112]
>> @@ -82,8 +82,8 @@
>>  	ldr	x21, [x3, #16]
>>  
>>  	msr	sp_el0, x19
>> -	msr	elr_el2, x20 				// EL1 PC
>> -	msr	spsr_el2, x21 				// EL1 pstate
>> +	msr	elr_el2, x20 		// PC to restore
>> +	msr	spsr_el2, x21 		// pstate to restore
> 
> I don't feel like 'to restore' is much more meaningful here.
> 
> I would actually vote for removin the comments all together, since one
> should really understand the code as opposed to the comments when
> reading this kind of stuff.
> 
> Meh, I'm not sure.  Your patch is definitely better than doing nothing.
> 
> Marc?

While I definitely agree that people should pay more attention to the
code rather than blindly trusting comments, I still think there is some
value in disambiguating the exception entry/return, because this bit of
code assumes some intimate knowledge of the ARMv8 exception model.

As for the comments themselves, I'd rather have some wording that
clearly indicate that we're dealing with guest information, i.e:

	mrs	x20, elr_el2		// Guest PC
	mrs	x21, spsr_el2		// Guest pstate

(and the same for the exception return). The "before hyp entry" and "to
restore" are not really useful (all the registers we are
saving/restoring fall into these categories). What I wanted to convey
here was that despite using an EL2 register, we are dealing with guest
registers.

Would this address your concerns?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/