linux-kernel - Re: [PATCH v5 08/12] KVM: arm64: re-factor hyp.S debug register code

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:34:46 +0100
From:	Alex Bennée <alex.bennee@...aro.org>
To:	Christoffer Dall <christoffer.dall@...aro.org>
Cc:	kvm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	kvmarm@...ts.cs.columbia.edu, marc.zyngier@....com,
	peter.maydell@...aro.org, agraf@...e.de, drjones@...hat.com,
	pbonzini@...hat.com, zhichao.huang@...aro.org,
	jan.kiszka@...mens.com, dahi@...ux.vnet.ibm.com,
	r65777@...escale.com, bp@...e.de, Gleb Natapov <gleb@...nel.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 08/12] KVM: arm64: re-factor hyp.S debug register code


Christoffer Dall <christoffer.dall@...aro.org> writes:

> On Fri, May 29, 2015 at 10:30:24AM +0100, Alex Bennée wrote:
>> This is a pre-cursor to sharing the code with the guest debug support.
>> This replaces the big macro that fishes data out of a fixed location
>> with a more general helper macro to restore a set of debug registers. It
>> uses macro substitution so it can be re-used for debug control and value
>> registers. It does however rely on the debug registers being 64 bit
>> aligned (as they happen to be in the hyp ABI).

Oops I'd better fix that commit comment.

>> 
>> Signed-off-by: Alex Bennée <alex.bennee@...aro.org>
>> 
>> ---
>> v3:
>>   - return to the patch series
>>   - add save and restore targets
>>   - change register use and document
>> v4:
>>   - keep original setup/restore names
>>   - don't use split u32/u64 structure yet
>> ---
>>  arch/arm64/kvm/hyp.S | 519 ++++++++++++++-------------------------------------
>>  1 file changed, 140 insertions(+), 379 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>> index 74e63d8..9c4897d 100644
>> --- a/arch/arm64/kvm/hyp.S
>> +++ b/arch/arm64/kvm/hyp.S
>
>
> [...]
>
>> @@ -465,195 +318,52 @@
>>  	msr	mdscr_el1,	x25
>>  .endm
>>  
>> -.macro restore_debug
>> -	// x2: base address for cpu context
>> -	// x3: tmp register
>> -
>> -	mrs	x26, id_aa64dfr0_el1
>> -	ubfx	x24, x26, #12, #4	// Extract BRPs
>> -	ubfx	x25, x26, #20, #4	// Extract WRPs
>> -	mov	w26, #15
>> -	sub	w24, w26, w24		// How many BPs to skip
>> -	sub	w25, w26, w25		// How many WPs to skip
>> -
>> -	add	x3, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
>> +.macro restore_debug type
>> +        // x4: pointer to register set
>> +        // x5: number of registers to skip
>
> nit: use tabs instead of spaces here...
>
>> +	// x6..x22 trashed
>>  
>
> [...]
>
>> @@ -887,12 +597,63 @@ __restore_sysregs:
>>  	restore_sysregs
>>  	ret
>>  
>> +/* Save debug state */
>>  __save_debug:
>> -	save_debug
>> +	// x0: base address for vcpu context
>> +	// x2: ptr to current CPU context
>> +	// x4/x5: trashed
>
> I had a bunch of questions here which I think you missed last time
> around:
>  1. why do we need the vcpu context?

We don't, I'll drop that

>  2. what does 'current' mean here?

Either the host or vcpu context depending which way we are currently going.

>  3. you're also trashing everything that save_debug trashes, so x4/5,
>     x6-x22 trashed would be more correct

OK

>
>> +
>> +	mrs	x26, id_aa64dfr0_el1
>> +	ubfx	x24, x26, #12, #4	// Extract BRPs
>> +	ubfx	x25, x26, #20, #4	// Extract WRPs
>> +	mov	w26, #15
>> +	sub	w24, w26, w24		// How many BPs to skip
>> +	sub	w25, w26, w25		// How many WPs to skip
>> +
>> +	mov     x5, x24
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
>> +	save_debug dbgbcr
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
>> +	save_debug dbgbvr
>> +
>> +	mov     x5, x25
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
>> +	save_debug dbgwcr
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
>> +	save_debug dbgwvr
>> +
>> +	mrs	x21, mdccint_el1
>> +	str	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
>>  	ret
>>  
>> +/* Restore debug state */
>>  __restore_debug:
>> -	restore_debug
>> +	// x0: base address for cpu context
>> +	// x2: ptr to current CPU context
>> +	// x4/x5: trashed
>
> and you missed these comments too, basically same stuff, but why was it
> 'cpu' here and not 'vcpu' like above?

Again we use the functions both for restoring host and vcpu debug context.

>
> note again, that you're explicitly touching x24, xx25, and x26 here as
> well, so shouldn't they be listed as trashed?
>
>> +
>> +	mrs	x26, id_aa64dfr0_el1
>> +	ubfx	x24, x26, #12, #4	// Extract BRPs
>> +	ubfx	x25, x26, #20, #4	// Extract WRPs
>> +	mov	w26, #15
>> +	sub	w24, w26, w24		// How many BPs to skip
>> +	sub	w25, w26, w25		// How many WPs to skip
>> +
>> +	mov     x5, x24
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBCR0_EL1)
>> +	restore_debug dbgbcr
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGBVR0_EL1)
>> +	restore_debug dbgbvr
>> +
>> +	mov     x5, x25
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWCR0_EL1)
>> +	restore_debug dbgwcr
>> +	add	x4, x2, #CPU_SYSREG_OFFSET(DBGWVR0_EL1)
>> +	restore_debug dbgwvr
>> +
>> +	ldr	x21, [x2, #CPU_SYSREG_OFFSET(MDCCINT_EL1)]
>> +	msr	mdccint_el1, x21
>> +
>>  	ret
>>  
>>  __save_fpsimd:
>
> Thanks,
> -Christoffer

-- 
Alex Bennée
--
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/