Re: [PATCH v6 09/12] KVM: arm64: introduce vcpu->arch.debug_ptr

["Alex Bennée" <alex.bennee@linaro.org>]

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

> On Fri, Jun 19, 2015 at 01:23:47PM +0100, Alex Bennée wrote:
>> This introduces a level of indirection for the debug registers. Instead
>> of using the sys_regs[] directly we store registers in a structure in
>> the vcpu. As we are no longer tied to the layout of the sys_regs[] we
>> can make the copies size appropriate for control and value registers.
>> 
>> This also entails updating the sys_regs code to access this new
>> structure. Instead of passing a register index we now pass an offset
>> into the kvm_guest_debug_arch structure.
>> 
>> We also need to ensure the GET/SET_ONE_REG ioctl operations store the
>> registers in their correct location.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> 
>> ---
>> v6:
>>   - fix up some ws issues
>>   - correct clobber info
>>   - re-word commentary in kvm_host.h
>>   - fix endian access issues for aarch32 fields
>>   - revert all KVM_GET/SET_ONE_REG to 64bit (also see ABI update)
>> ---
<snip>
>>  
>> +/* Used when AArch32 kernels trap to mapped debug registers */
>> +static inline bool trap_debug32(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_params *p,
>> +				const struct sys_reg_desc *rd)
>> +{
>> +	__u32 *r = (__u32 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);
>
> This still looks like something that's asking for BE trouble.  Why not
> access the register as a __u64 as it is and then only special-case it
> somehow for the XVR thingy...  Perhaps a separate function, see below.
>
>> +	if (p->is_write) {
>> +		*r = *vcpu_reg(vcpu, p->Rt);
>> +		vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
>> +	} else {
>> +		*vcpu_reg(vcpu, p->Rt) = *r;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +static inline bool trap_debug64(struct kvm_vcpu *vcpu,
>> +				const struct sys_reg_params *p,
>> +				const struct sys_reg_desc *rd)
>> +{
>> +	__u64 *r = (__u64 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);
>> +	if (p->is_write) {
>> +		*r = *vcpu_reg(vcpu, p->Rt);
>> +		vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
>> +	} else {
>> +		*vcpu_reg(vcpu, p->Rt) = *r;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +static inline void reset_debug64(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
>> +{
>> +	__u64 *r = (__u64 *) ((void * )&vcpu->arch.vcpu_debug_state + rd->reg);
>> +	*r = rd->val;
>> +}
>> +
>>  static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>>  {
>>  	u64 amair;
>> @@ -240,16 +277,20 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>>  #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
>>  	/* DBGBVRn_EL1 */						\
>>  	{ Op0(0b10), Op1(0b000), CRn(0b0000), CRm((n)), Op2(0b100),	\
>> -	  trap_debug_regs, reset_val, (DBGBVR0_EL1 + (n)), 0 },		\
>> +	  trap_debug64, reset_debug64,					\
>> +	  offsetof(struct kvm_guest_debug_arch, dbg_bvr[(n)]), 0 },	\
>>  	/* DBGBCRn_EL1 */						\
>>  	{ Op0(0b10), Op1(0b000), CRn(0b0000), CRm((n)), Op2(0b101),	\
>> -	  trap_debug_regs, reset_val, (DBGBCR0_EL1 + (n)), 0 },		\
>> +	  trap_debug64, reset_debug64,					\
>> +	  offsetof(struct kvm_guest_debug_arch, dbg_bcr[(n)]), 0},	\
>>  	/* DBGWVRn_EL1 */						\
>>  	{ Op0(0b10), Op1(0b000), CRn(0b0000), CRm((n)), Op2(0b110),	\
>> -	  trap_debug_regs, reset_val, (DBGWVR0_EL1 + (n)), 0 },		\
>> +	  trap_debug64, reset_debug64,					\
>> +	  offsetof(struct kvm_guest_debug_arch, dbg_wvr[(n)]), 0 },	\
>>  	/* DBGWCRn_EL1 */						\
>>  	{ Op0(0b10), Op1(0b000), CRn(0b0000), CRm((n)), Op2(0b111),	\
>> -	  trap_debug_regs, reset_val, (DBGWCR0_EL1 + (n)), 0 }
>> +	  trap_debug64, reset_debug64,					\
>> +	  offsetof(struct kvm_guest_debug_arch, dbg_wcr[(n)]), 0}
>>  
>>  /*
>>   * Architected system registers.
>> @@ -502,42 +543,51 @@ static bool trap_dbgidr(struct kvm_vcpu *vcpu,
>>  	}
>>  }
>>  
>> -static bool trap_debug32(struct kvm_vcpu *vcpu,
>> -			 const struct sys_reg_params *p,
>> -			 const struct sys_reg_desc *r)
>> -{
>> -	if (p->is_write) {
>> -		vcpu_cp14(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
>> -		vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
>> -	} else {
>> -		*vcpu_reg(vcpu, p->Rt) = vcpu_cp14(vcpu, r->reg);
>> -	}
>> -
>> -	return true;
>> -}
>> +/* AArch32 debug register mappings
>> + *
>> + * AArch32 DBGBVRn is mapped to DBGBVRn_EL1[31:0]
>> + * AArch32 DBGBXVRn is mapped to DBGBVRn_EL1[63:32]
>> + *
>> + * All control registers and watchpoint value registers are mapped to
>> + * the lower 32 bits of their AArch64 equivalents.
>> + *
>> + * We also need to ensure we deal with endian differences when
>> + * mapping a partial AArch64 register.
>> + */
>>  
>> -#define DBG_BCR_BVR_WCR_WVR(n)					\
>> -	/* DBGBVRn */						\
>> -	{ Op1( 0), CRn( 0), CRm((n)), Op2( 4), trap_debug32,	\
>> -	  NULL, (cp14_DBGBVR0 + (n) * 2) },			\
>> -	/* DBGBCRn */						\
>> -	{ Op1( 0), CRn( 0), CRm((n)), Op2( 5), trap_debug32,	\
>> -	  NULL, (cp14_DBGBCR0 + (n) * 2) },			\
>> -	/* DBGWVRn */						\
>> -	{ Op1( 0), CRn( 0), CRm((n)), Op2( 6), trap_debug32,	\
>> -	  NULL, (cp14_DBGWVR0 + (n) * 2) },			\
>> -	/* DBGWCRn */						\
>> -	{ Op1( 0), CRn( 0), CRm((n)), Op2( 7), trap_debug32,	\
>> -	  NULL, (cp14_DBGWCR0 + (n) * 2) }
>> -
>> -#define DBGBXVR(n)						\
>> -	{ Op1( 0), CRn( 1), CRm((n)), Op2( 1), trap_debug32,	\
>> -	  NULL, cp14_DBGBXVR0 + n * 2 }
>> +#ifdef CONFIG_CPU_BIG_ENDIAN
>> +#define DBG_AA32_LOW_OFFSET	sizeof(__u32)
>> +#define DBG_AA32_HIGH_OFFSET	0
>> +#else
>> +#define DBG_AA32_LOW_OFFSET	0
>> +#define DBG_AA32_HIGH_OFFSET	sizeof(__u32)
>> +#endif
>> +
>> +#define DBG_BCR_BVR_WCR_WVR(n)						\
>> +	/* DBGBVRn */							\
>> +	{ Op1( 0), CRn( 0), CRm((n)), Op2( 4), trap_debug32,		\
>> +	  NULL, offsetof(struct kvm_guest_debug_arch, dbg_bvr[(n)])	\
>> +	  + DBG_AA32_LOW_OFFSET },					\
>> +	/* DBGBCRn */							\
>> +	{ Op1( 0), CRn( 0), CRm((n)), Op2( 5), trap_debug32,		\
>> +	  NULL, offsetof(struct kvm_guest_debug_arch, dbg_bcr[(n)]) },	\
>
> why doesn't this need + DBG_AA32_LOW_OFFSET?

It didn't before as it was a 32bit register, but of course last version
I moved it back to 64 bit and failed to catch that. Thanks!

>
>> +	/* DBGWVRn */							\
>> +	{ Op1( 0), CRn( 0), CRm((n)), Op2( 6), trap_debug32,		\
>> +	  NULL, offsetof(struct kvm_guest_debug_arch, dbg_wvr[(n)])	\
>> +	  + DBG_AA32_LOW_OFFSET },					\
>> +	/* DBGWCRn */							\
>> +	{ Op1( 0), CRn( 0), CRm((n)), Op2( 7), trap_debug32,		\
>> +	  NULL, offsetof(struct kvm_guest_debug_arch, dbg_wcr[(n)]) }
>
> ditto ?
>
> I find this quite hard to read and adding this offset on the separate
> line doesn't seem to help.
>
> Perhaps you should just bite the bullet and have separate accessor
> functions for the wvr/wcr/bcr/bvr arrays and just pass the register
> number.

I suspect it would be cleaner reading for the cost of more boilerplate
code. Should I share the access functions between Aarch64/Aarch32 modes
as well?


>
> Thanks,
> -Christoffer

-- 
Alex Bennée