Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers

[Scott Wood <scottwood@freescale.com>]

On 07/23/2012 06:19 AM, Bharat Bhushan wrote:
> IAC/DAC are defined as 32 bit while they are 64 bit wide. So ONE_REG
> interface is added to set/get them.
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
> v2:
>  - Using copy_to/from_user() apis.
> 
>  arch/powerpc/include/asm/kvm.h      |   12 ++++++
>  arch/powerpc/include/asm/kvm_host.h |   28 ++++++++++++++-
>  arch/powerpc/kvm/booke.c            |   64 +++++++++++++++++++++++++++++++++-
>  arch/powerpc/kvm/booke_emulate.c    |    8 ++--
>  4 files changed, 104 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/asm/kvm.h
> index 1bea4d8..3c14202 100644
> --- a/arch/powerpc/include/asm/kvm.h
> +++ b/arch/powerpc/include/asm/kvm.h
> @@ -221,6 +221,12 @@ struct kvm_sregs {
>  
>  			__u32 dbsr;	/* KVM_SREGS_E_UPDATE_DBSR */
>  			__u32 dbcr[3];
> +			/*
> +			 * iac/dac registers are 64bit wide, while this API
> +			 * interface provides only lower 32 bits on 64 bit
> +			 * processors. ONE_REG interface is added for 64bit
> +			 * iac/dac registers.
> +			 */
>  			__u32 iac[4];
>  			__u32 dac[2];
>  			__u32 dvc[2];
> @@ -326,5 +332,11 @@ struct kvm_book3e_206_tlb_params {
>  };
>  
>  #define KVM_REG_PPC_HIOR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x1)
> +#define KVM_REG_PPC_IAC1	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x2)
> +#define KVM_REG_PPC_IAC2	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x3)
> +#define KVM_REG_PPC_IAC3	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x4)
> +#define KVM_REG_PPC_IAC4	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x5)
> +#define KVM_REG_PPC_DAC1	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x6)
> +#define KVM_REG_PPC_DAC2	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x7)
>  
>  #endif /* __LINUX_KVM_POWERPC_H */
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index 43cac48..2c0f015 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -342,6 +342,31 @@ struct kvmppc_slb {
>  	bool class	: 1;
>  };
>  
> +#ifdef CONFIG_BOOKE
> +# ifdef CONFIG_PPC_FSL_BOOK3E
> +#define KVMPPC_IAC_NUM	2
> +#define KVMPPC_DAC_NUM	2
> +# else
> +#define KVMPPC_IAC_NUM	4
> +#define KVMPPC_DAC_NUM	2
> +# endif
> +#define KVMPPC_MAX_IAC	4
> +#define KVMPPC_MAX_DAC	2
> +#endif
> +
> +struct kvmppc_debug_reg {
> +#ifdef CONFIG_BOOKE
> +	u32 dbcr0;
> +	u32 dbcr1;
> +	u32 dbcr2;
> +#ifdef CONFIG_KVM_E500MC
> +	u32 dbcr4;
> +#endif
> +	u64 iac[KVMPPC_MAX_IAC];
> +	u64 dac[KVMPPC_MAX_DAC];
> +#endif
> +};

Is there any reason for this to be a separate struct?

> +
>  struct kvm_vcpu_arch {
>  	ulong host_stack;
>  	u32 host_pid;
> @@ -436,9 +461,8 @@ struct kvm_vcpu_arch {
>  
>  	u32 ccr0;
>  	u32 ccr1;
> -	u32 dbcr0;
> -	u32 dbcr1;
>  	u32 dbsr;
> +	struct kvmppc_debug_reg dbg_reg;
>  
>  	u64 mmcr[3];
>  	u32 pmc[8];
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index be71b8e..fa23158 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -1353,12 +1353,72 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
>  
>  int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
>  {
> -	return -EINVAL;
> +	int r = -EINVAL;
> +
> +	switch(reg->id) {
> +	case KVM_REG_PPC_IAC1:
> +		r = copy_to_user((u64 __user *)(long)reg->addr,
> +				 &vcpu->arch.dbg_reg.iac[0], sizeof(u64));
> +		break;
> +	case KVM_REG_PPC_IAC2:
> +		r = copy_to_user((u64 __user *)(long)reg->addr,
> +				 &vcpu->arch.dbg_reg.iac[1], sizeof(u64));
> +		break;
> +	case KVM_REG_PPC_IAC3:
> +		r = copy_to_user((u64 __user *)(long)reg->addr,
> +				 &vcpu->arch.dbg_reg.iac[2], sizeof(u64));
> +		break;
> +	case KVM_REG_PPC_IAC4:
> +		r = copy_to_user((u64 __user *)(long)reg->addr,
> +				 &vcpu->arch.dbg_reg.iac[3], sizeof(u64));
> +		break;

This will exceed the array size if userspace asks to access IAC3/4 on an
e500-family chip.

Why not just set the array at the max size unconditionally?

-Scott