All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
@ 2012-07-23 11:31 ` Bharat Bhushan
  0 siblings, 0 replies; 18+ messages in thread
From: Bharat Bhushan @ 2012-07-23 11:19 UTC (permalink / raw)
  To: kvm-ppc, agraf, kvm, scottwood; +Cc: Bharat Bhushan, Bharat Bhushan

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
+};
+
 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;
+	case KVM_REG_PPC_DAC1:
+		r = copy_to_user((u64 __user *)(long)reg->addr,
+				 &vcpu->arch.dbg_reg.dac[0], sizeof(u64));
+		break;
+	case KVM_REG_PPC_DAC2:
+		r = copy_to_user((u64 __user *)(long)reg->addr,
+				 &vcpu->arch.dbg_reg.dac[1], sizeof(u64));
+		break;
+	default:
+		break;
+	}
+	return r;
 }
 
 int kvm_vcpu_ioctl_set_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_from_user(&vcpu->arch.dbg_reg.iac[0],
+			     (u64 __user *)(long)reg->addr, sizeof(u64));
+		break;
+	case KVM_REG_PPC_IAC2:
+		r = copy_from_user(&vcpu->arch.dbg_reg.iac[1],
+			     (u64 __user *)(long)reg->addr, sizeof(u64));
+		break;
+	case KVM_REG_PPC_IAC3:
+		r = copy_from_user(&vcpu->arch.dbg_reg.iac[2],
+			     (u64 __user *)(long)reg->addr, sizeof(u64));
+		break;
+	case KVM_REG_PPC_IAC4:
+		r = copy_from_user(&vcpu->arch.dbg_reg.iac[3],
+			     (u64 __user *)(long)reg->addr, sizeof(u64));
+		break;
+	case KVM_REG_PPC_DAC1:
+		r = copy_from_user(&vcpu->arch.dbg_reg.dac[0],
+			     (u64 __user *)(long)reg->addr, sizeof(u64));
+		break;
+	case KVM_REG_PPC_DAC2:
+		r = copy_from_user(&vcpu->arch.dbg_reg.dac[1],
+			     (u64 __user *)(long)reg->addr, sizeof(u64));
+		break;
+	default:
+		break;
+	}
+	return r;
 }
 
 int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
diff --git a/arch/powerpc/kvm/booke_emulate.c b/arch/powerpc/kvm/booke_emulate.c
index 5a66ade..cc99a0b 100644
--- a/arch/powerpc/kvm/booke_emulate.c
+++ b/arch/powerpc/kvm/booke_emulate.c
@@ -133,10 +133,10 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val)
 		vcpu->arch.csrr1 = spr_val;
 		break;
 	case SPRN_DBCR0:
-		vcpu->arch.dbcr0 = spr_val;
+		vcpu->arch.dbg_reg.dbcr0 = spr_val;
 		break;
 	case SPRN_DBCR1:
-		vcpu->arch.dbcr1 = spr_val;
+		vcpu->arch.dbg_reg.dbcr1 = spr_val;
 		break;
 	case SPRN_DBSR:
 		vcpu->arch.dbsr &= ~spr_val;
@@ -266,10 +266,10 @@ int kvmppc_booke_emulate_mfspr(struct kvm_vcpu *vcpu, int sprn, ulong *spr_val)
 		*spr_val = vcpu->arch.csrr1;
 		break;
 	case SPRN_DBCR0:
-		*spr_val = vcpu->arch.dbcr0;
+		*spr_val = vcpu->arch.dbg_reg.dbcr0;
 		break;
 	case SPRN_DBCR1:
-		*spr_val = vcpu->arch.dbcr1;
+		*spr_val = vcpu->arch.dbg_reg.dbcr1;
 		break;
 	case SPRN_DBSR:
 		*spr_val = vcpu->arch.dbsr;
-- 
1.7.0.4

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
@ 2012-07-23 11:31 ` Bharat Bhushan
  0 siblings, 0 replies; 18+ messages in thread
From: Bharat Bhushan @ 2012-07-23 11:31 UTC (permalink / raw)
  To: kvm-ppc, agraf, kvm, scottwood; +Cc: Bharat Bhushan, Bharat Bhushan

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
+};
+
 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;
+	case KVM_REG_PPC_DAC1:
+		r = copy_to_user((u64 __user *)(long)reg->addr,
+				 &vcpu->arch.dbg_reg.dac[0], sizeof(u64));
+		break;
+	case KVM_REG_PPC_DAC2:
+		r = copy_to_user((u64 __user *)(long)reg->addr,
+				 &vcpu->arch.dbg_reg.dac[1], sizeof(u64));
+		break;
+	default:
+		break;
+	}
+	return r;
 }
 
 int kvm_vcpu_ioctl_set_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_from_user(&vcpu->arch.dbg_reg.iac[0],
+			     (u64 __user *)(long)reg->addr, sizeof(u64));
+		break;
+	case KVM_REG_PPC_IAC2:
+		r = copy_from_user(&vcpu->arch.dbg_reg.iac[1],
+			     (u64 __user *)(long)reg->addr, sizeof(u64));
+		break;
+	case KVM_REG_PPC_IAC3:
+		r = copy_from_user(&vcpu->arch.dbg_reg.iac[2],
+			     (u64 __user *)(long)reg->addr, sizeof(u64));
+		break;
+	case KVM_REG_PPC_IAC4:
+		r = copy_from_user(&vcpu->arch.dbg_reg.iac[3],
+			     (u64 __user *)(long)reg->addr, sizeof(u64));
+		break;
+	case KVM_REG_PPC_DAC1:
+		r = copy_from_user(&vcpu->arch.dbg_reg.dac[0],
+			     (u64 __user *)(long)reg->addr, sizeof(u64));
+		break;
+	case KVM_REG_PPC_DAC2:
+		r = copy_from_user(&vcpu->arch.dbg_reg.dac[1],
+			     (u64 __user *)(long)reg->addr, sizeof(u64));
+		break;
+	default:
+		break;
+	}
+	return r;
 }
 
 int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
diff --git a/arch/powerpc/kvm/booke_emulate.c b/arch/powerpc/kvm/booke_emulate.c
index 5a66ade..cc99a0b 100644
--- a/arch/powerpc/kvm/booke_emulate.c
+++ b/arch/powerpc/kvm/booke_emulate.c
@@ -133,10 +133,10 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val)
 		vcpu->arch.csrr1 = spr_val;
 		break;
 	case SPRN_DBCR0:
-		vcpu->arch.dbcr0 = spr_val;
+		vcpu->arch.dbg_reg.dbcr0 = spr_val;
 		break;
 	case SPRN_DBCR1:
-		vcpu->arch.dbcr1 = spr_val;
+		vcpu->arch.dbg_reg.dbcr1 = spr_val;
 		break;
 	case SPRN_DBSR:
 		vcpu->arch.dbsr &= ~spr_val;
@@ -266,10 +266,10 @@ int kvmppc_booke_emulate_mfspr(struct kvm_vcpu *vcpu, int sprn, ulong *spr_val)
 		*spr_val = vcpu->arch.csrr1;
 		break;
 	case SPRN_DBCR0:
-		*spr_val = vcpu->arch.dbcr0;
+		*spr_val = vcpu->arch.dbg_reg.dbcr0;
 		break;
 	case SPRN_DBCR1:
-		*spr_val = vcpu->arch.dbcr1;
+		*spr_val = vcpu->arch.dbg_reg.dbcr1;
 		break;
 	case SPRN_DBSR:
 		*spr_val = vcpu->arch.dbsr;
-- 
1.7.0.4



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
  2012-07-23 11:31 ` Bharat Bhushan
@ 2012-07-23 15:42   ` Scott Wood
  -1 siblings, 0 replies; 18+ messages in thread
From: Scott Wood @ 2012-07-23 15:42 UTC (permalink / raw)
  To: Bharat Bhushan; +Cc: kvm-ppc, agraf, kvm, Bharat Bhushan

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
@ 2012-07-23 15:42   ` Scott Wood
  0 siblings, 0 replies; 18+ messages in thread
From: Scott Wood @ 2012-07-23 15:42 UTC (permalink / raw)
  To: Bharat Bhushan; +Cc: kvm-ppc, agraf, kvm, Bharat Bhushan

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



^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
  2012-07-23 15:42   ` Scott Wood
@ 2012-07-23 15:48     ` Bhushan Bharat-R65777
  -1 siblings, 0 replies; 18+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-07-23 15:48 UTC (permalink / raw)
  To: Wood Scott-B07421; +Cc: kvm-ppc, agraf, kvm



> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Monday, July 23, 2012 9:12 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; agraf@suse.de; kvm@vger.kernel.org; Bhushan Bharat-
> R65777
> Subject: Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug
> registers
> 
> 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?

No specific reason, The rest of debug ( which will follow sometime soon) uses this structure and looks to make code look easy.

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

No , is not the array size already set to maximum. But yes we should not let IAC3/4 being accessed for e500 (FSL_BOOKE).

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

This is already coded this way. Please see the struct is this patch.

Thanks
-Bharat

> 
> -Scott


^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
@ 2012-07-23 15:48     ` Bhushan Bharat-R65777
  0 siblings, 0 replies; 18+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-07-23 15:48 UTC (permalink / raw)
  To: Wood Scott-B07421; +Cc: kvm-ppc, agraf, kvm

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogV29vZCBTY290dC1CMDc0
MjENCj4gU2VudDogTW9uZGF5LCBKdWx5IDIzLCAyMDEyIDk6MTIgUE0NCj4gVG86IEJodXNoYW4g
QmhhcmF0LVI2NTc3Nw0KPiBDYzoga3ZtLXBwY0B2Z2VyLmtlcm5lbC5vcmc7IGFncmFmQHN1c2Uu
ZGU7IGt2bUB2Z2VyLmtlcm5lbC5vcmc7IEJodXNoYW4gQmhhcmF0LQ0KPiBSNjU3NzcNCj4gU3Vi
amVjdDogUmU6IFtQQVRDSCB2Ml0gYm9va2U6IEFkZGVkIE9ORV9SRUcgaW50ZXJmYWNlIGZvciBJ
QUMvREFDIGRlYnVnDQo+IHJlZ2lzdGVycw0KPiANCj4gT24gMDcvMjMvMjAxMiAwNjoxOSBBTSwg
QmhhcmF0IEJodXNoYW4gd3JvdGU6DQo+ID4gSUFDL0RBQyBhcmUgZGVmaW5lZCBhcyAzMiBiaXQg
d2hpbGUgdGhleSBhcmUgNjQgYml0IHdpZGUuIFNvIE9ORV9SRUcNCj4gPiBpbnRlcmZhY2UgaXMg
YWRkZWQgdG8gc2V0L2dldCB0aGVtLg0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTogQmhhcmF0IEJo
dXNoYW4gPGJoYXJhdC5iaHVzaGFuQGZyZWVzY2FsZS5jb20+DQo+ID4gLS0tDQo+ID4gdjI6DQo+
ID4gIC0gVXNpbmcgY29weV90by9mcm9tX3VzZXIoKSBhcGlzLg0KPiA+DQo+ID4gIGFyY2gvcG93
ZXJwYy9pbmNsdWRlL2FzbS9rdm0uaCAgICAgIHwgICAxMiArKysrKysNCj4gPiAgYXJjaC9wb3dl
cnBjL2luY2x1ZGUvYXNtL2t2bV9ob3N0LmggfCAgIDI4ICsrKysrKysrKysrKysrLQ0KPiA+ICBh
cmNoL3Bvd2VycGMva3ZtL2Jvb2tlLmMgICAgICAgICAgICB8ICAgNjQgKysrKysrKysrKysrKysr
KysrKysrKysrKysrKysrKysrLQ0KPiA+ICBhcmNoL3Bvd2VycGMva3ZtL2Jvb2tlX2VtdWxhdGUu
YyAgICB8ICAgIDggKystLQ0KPiA+ICA0IGZpbGVzIGNoYW5nZWQsIDEwNCBpbnNlcnRpb25zKCsp
LCA4IGRlbGV0aW9ucygtKQ0KPiA+DQo+ID4gZGlmZiAtLWdpdCBhL2FyY2gvcG93ZXJwYy9pbmNs
dWRlL2FzbS9rdm0uaA0KPiA+IGIvYXJjaC9wb3dlcnBjL2luY2x1ZGUvYXNtL2t2bS5oIGluZGV4
IDFiZWE0ZDguLjNjMTQyMDIgMTAwNjQ0DQo+ID4gLS0tIGEvYXJjaC9wb3dlcnBjL2luY2x1ZGUv
YXNtL2t2bS5oDQo+ID4gKysrIGIvYXJjaC9wb3dlcnBjL2luY2x1ZGUvYXNtL2t2bS5oDQo+ID4g
QEAgLTIyMSw2ICsyMjEsMTIgQEAgc3RydWN0IGt2bV9zcmVncyB7DQo+ID4NCj4gPiAgCQkJX191
MzIgZGJzcjsJLyogS1ZNX1NSRUdTX0VfVVBEQVRFX0RCU1IgKi8NCj4gPiAgCQkJX191MzIgZGJj
clszXTsNCj4gPiArCQkJLyoNCj4gPiArCQkJICogaWFjL2RhYyByZWdpc3RlcnMgYXJlIDY0Yml0
IHdpZGUsIHdoaWxlIHRoaXMgQVBJDQo+ID4gKwkJCSAqIGludGVyZmFjZSBwcm92aWRlcyBvbmx5
IGxvd2VyIDMyIGJpdHMgb24gNjQgYml0DQo+ID4gKwkJCSAqIHByb2Nlc3NvcnMuIE9ORV9SRUcg
aW50ZXJmYWNlIGlzIGFkZGVkIGZvciA2NGJpdA0KPiA+ICsJCQkgKiBpYWMvZGFjIHJlZ2lzdGVy
cy4NCj4gPiArCQkJICovDQo+ID4gIAkJCV9fdTMyIGlhY1s0XTsNCj4gPiAgCQkJX191MzIgZGFj
WzJdOw0KPiA+ICAJCQlfX3UzMiBkdmNbMl07DQo+ID4gQEAgLTMyNiw1ICszMzIsMTEgQEAgc3Ry
dWN0IGt2bV9ib29rM2VfMjA2X3RsYl9wYXJhbXMgeyAgfTsNCj4gPg0KPiA+ICAjZGVmaW5lIEtW
TV9SRUdfUFBDX0hJT1IJKEtWTV9SRUdfUFBDIHwgS1ZNX1JFR19TSVpFX1U2NCB8IDB4MSkNCj4g
PiArI2RlZmluZSBLVk1fUkVHX1BQQ19JQUMxCShLVk1fUkVHX1BQQyB8IEtWTV9SRUdfU0laRV9V
NjQgfCAweDIpDQo+ID4gKyNkZWZpbmUgS1ZNX1JFR19QUENfSUFDMgkoS1ZNX1JFR19QUEMgfCBL
Vk1fUkVHX1NJWkVfVTY0IHwgMHgzKQ0KPiA+ICsjZGVmaW5lIEtWTV9SRUdfUFBDX0lBQzMJKEtW
TV9SRUdfUFBDIHwgS1ZNX1JFR19TSVpFX1U2NCB8IDB4NCkNCj4gPiArI2RlZmluZSBLVk1fUkVH
X1BQQ19JQUM0CShLVk1fUkVHX1BQQyB8IEtWTV9SRUdfU0laRV9VNjQgfCAweDUpDQo+ID4gKyNk
ZWZpbmUgS1ZNX1JFR19QUENfREFDMQkoS1ZNX1JFR19QUEMgfCBLVk1fUkVHX1NJWkVfVTY0IHwg
MHg2KQ0KPiA+ICsjZGVmaW5lIEtWTV9SRUdfUFBDX0RBQzIJKEtWTV9SRUdfUFBDIHwgS1ZNX1JF
R19TSVpFX1U2NCB8IDB4NykNCj4gPg0KPiA+ICAjZW5kaWYgLyogX19MSU5VWF9LVk1fUE9XRVJQ
Q19IICovDQo+ID4gZGlmZiAtLWdpdCBhL2FyY2gvcG93ZXJwYy9pbmNsdWRlL2FzbS9rdm1faG9z
dC5oDQo+ID4gYi9hcmNoL3Bvd2VycGMvaW5jbHVkZS9hc20va3ZtX2hvc3QuaA0KPiA+IGluZGV4
IDQzY2FjNDguLjJjMGYwMTUgMTAwNjQ0DQo+ID4gLS0tIGEvYXJjaC9wb3dlcnBjL2luY2x1ZGUv
YXNtL2t2bV9ob3N0LmgNCj4gPiArKysgYi9hcmNoL3Bvd2VycGMvaW5jbHVkZS9hc20va3ZtX2hv
c3QuaA0KPiA+IEBAIC0zNDIsNiArMzQyLDMxIEBAIHN0cnVjdCBrdm1wcGNfc2xiIHsNCj4gPiAg
CWJvb2wgY2xhc3MJOiAxOw0KPiA+ICB9Ow0KPiA+DQo+ID4gKyNpZmRlZiBDT05GSUdfQk9PS0UN
Cj4gPiArIyBpZmRlZiBDT05GSUdfUFBDX0ZTTF9CT09LM0UNCj4gPiArI2RlZmluZSBLVk1QUENf
SUFDX05VTQkyDQo+ID4gKyNkZWZpbmUgS1ZNUFBDX0RBQ19OVU0JMg0KPiA+ICsjIGVsc2UNCj4g
PiArI2RlZmluZSBLVk1QUENfSUFDX05VTQk0DQo+ID4gKyNkZWZpbmUgS1ZNUFBDX0RBQ19OVU0J
Mg0KPiA+ICsjIGVuZGlmDQo+ID4gKyNkZWZpbmUgS1ZNUFBDX01BWF9JQUMJNA0KPiA+ICsjZGVm
aW5lIEtWTVBQQ19NQVhfREFDCTINCj4gPiArI2VuZGlmDQo+ID4gKw0KPiA+ICtzdHJ1Y3Qga3Zt
cHBjX2RlYnVnX3JlZyB7DQo+ID4gKyNpZmRlZiBDT05GSUdfQk9PS0UNCj4gPiArCXUzMiBkYmNy
MDsNCj4gPiArCXUzMiBkYmNyMTsNCj4gPiArCXUzMiBkYmNyMjsNCj4gPiArI2lmZGVmIENPTkZJ
R19LVk1fRTUwME1DDQo+ID4gKwl1MzIgZGJjcjQ7DQo+ID4gKyNlbmRpZg0KPiA+ICsJdTY0IGlh
Y1tLVk1QUENfTUFYX0lBQ107DQo+ID4gKwl1NjQgZGFjW0tWTVBQQ19NQVhfREFDXTsNCj4gPiAr
I2VuZGlmDQo+ID4gK307DQo+IA0KPiBJcyB0aGVyZSBhbnkgcmVhc29uIGZvciB0aGlzIHRvIGJl
IGEgc2VwYXJhdGUgc3RydWN0Pw0KDQpObyBzcGVjaWZpYyByZWFzb24sIFRoZSByZXN0IG9mIGRl
YnVnICggd2hpY2ggd2lsbCBmb2xsb3cgc29tZXRpbWUgc29vbikgdXNlcyB0aGlzIHN0cnVjdHVy
ZSBhbmQgbG9va3MgdG8gbWFrZSBjb2RlIGxvb2sgZWFzeS4NCg0KPiANCj4gPiArDQo+ID4gIHN0
cnVjdCBrdm1fdmNwdV9hcmNoIHsNCj4gPiAgCXVsb25nIGhvc3Rfc3RhY2s7DQo+ID4gIAl1MzIg
aG9zdF9waWQ7DQo+ID4gQEAgLTQzNiw5ICs0NjEsOCBAQCBzdHJ1Y3Qga3ZtX3ZjcHVfYXJjaCB7
DQo+ID4NCj4gPiAgCXUzMiBjY3IwOw0KPiA+ICAJdTMyIGNjcjE7DQo+ID4gLQl1MzIgZGJjcjA7
DQo+ID4gLQl1MzIgZGJjcjE7DQo+ID4gIAl1MzIgZGJzcjsNCj4gPiArCXN0cnVjdCBrdm1wcGNf
ZGVidWdfcmVnIGRiZ19yZWc7DQo+ID4NCj4gPiAgCXU2NCBtbWNyWzNdOw0KPiA+ICAJdTMyIHBt
Y1s4XTsNCj4gPiBkaWZmIC0tZ2l0IGEvYXJjaC9wb3dlcnBjL2t2bS9ib29rZS5jIGIvYXJjaC9w
b3dlcnBjL2t2bS9ib29rZS5jIGluZGV4DQo+ID4gYmU3MWI4ZS4uZmEyMzE1OCAxMDA2NDQNCj4g
PiAtLS0gYS9hcmNoL3Bvd2VycGMva3ZtL2Jvb2tlLmMNCj4gPiArKysgYi9hcmNoL3Bvd2VycGMv
a3ZtL2Jvb2tlLmMNCj4gPiBAQCAtMTM1MywxMiArMTM1Myw3MiBAQCBpbnQga3ZtX2FyY2hfdmNw
dV9pb2N0bF9zZXRfc3JlZ3Moc3RydWN0DQo+ID4ga3ZtX3ZjcHUgKnZjcHUsDQo+ID4NCj4gPiAg
aW50IGt2bV92Y3B1X2lvY3RsX2dldF9vbmVfcmVnKHN0cnVjdCBrdm1fdmNwdSAqdmNwdSwgc3Ry
dWN0DQo+ID4ga3ZtX29uZV9yZWcgKnJlZykgIHsNCj4gPiAtCXJldHVybiAtRUlOVkFMOw0KPiA+
ICsJaW50IHIgPSAtRUlOVkFMOw0KPiA+ICsNCj4gPiArCXN3aXRjaChyZWctPmlkKSB7DQo+ID4g
KwljYXNlIEtWTV9SRUdfUFBDX0lBQzE6DQo+ID4gKwkJciA9IGNvcHlfdG9fdXNlcigodTY0IF9f
dXNlciAqKShsb25nKXJlZy0+YWRkciwNCj4gPiArCQkJCSAmdmNwdS0+YXJjaC5kYmdfcmVnLmlh
Y1swXSwgc2l6ZW9mKHU2NCkpOw0KPiA+ICsJCWJyZWFrOw0KPiA+ICsJY2FzZSBLVk1fUkVHX1BQ
Q19JQUMyOg0KPiA+ICsJCXIgPSBjb3B5X3RvX3VzZXIoKHU2NCBfX3VzZXIgKikobG9uZylyZWct
PmFkZHIsDQo+ID4gKwkJCQkgJnZjcHUtPmFyY2guZGJnX3JlZy5pYWNbMV0sIHNpemVvZih1NjQp
KTsNCj4gPiArCQlicmVhazsNCj4gPiArCWNhc2UgS1ZNX1JFR19QUENfSUFDMzoNCj4gPiArCQly
ID0gY29weV90b191c2VyKCh1NjQgX191c2VyICopKGxvbmcpcmVnLT5hZGRyLA0KPiA+ICsJCQkJ
ICZ2Y3B1LT5hcmNoLmRiZ19yZWcuaWFjWzJdLCBzaXplb2YodTY0KSk7DQo+ID4gKwkJYnJlYWs7
DQo+ID4gKwljYXNlIEtWTV9SRUdfUFBDX0lBQzQ6DQo+ID4gKwkJciA9IGNvcHlfdG9fdXNlcigo
dTY0IF9fdXNlciAqKShsb25nKXJlZy0+YWRkciwNCj4gPiArCQkJCSAmdmNwdS0+YXJjaC5kYmdf
cmVnLmlhY1szXSwgc2l6ZW9mKHU2NCkpOw0KPiA+ICsJCWJyZWFrOw0KPiANCj4gVGhpcyB3aWxs
IGV4Y2VlZCB0aGUgYXJyYXkgc2l6ZSBpZiB1c2Vyc3BhY2UgYXNrcyB0byBhY2Nlc3MgSUFDMy80
IG9uIGFuIGU1MDAtDQo+IGZhbWlseSBjaGlwLg0KDQpObyAsIGlzIG5vdCB0aGUgYXJyYXkgc2l6
ZSBhbHJlYWR5IHNldCB0byBtYXhpbXVtLiBCdXQgeWVzIHdlIHNob3VsZCBub3QgbGV0IElBQzMv
NCBiZWluZyBhY2Nlc3NlZCBmb3IgZTUwMCAoRlNMX0JPT0tFKS4NCg0KPiANCj4gV2h5IG5vdCBq
dXN0IHNldCB0aGUgYXJyYXkgYXQgdGhlIG1heCBzaXplIHVuY29uZGl0aW9uYWxseT8NCg0KVGhp
cyBpcyBhbHJlYWR5IGNvZGVkIHRoaXMgd2F5LiBQbGVhc2Ugc2VlIHRoZSBzdHJ1Y3QgaXMgdGhp
cyBwYXRjaC4NCg0KVGhhbmtzDQotQmhhcmF0DQoNCj4gDQo+IC1TY290dA0KDQo

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
  2012-07-23 15:48     ` Bhushan Bharat-R65777
@ 2012-07-23 16:02       ` Scott Wood
  -1 siblings, 0 replies; 18+ messages in thread
From: Scott Wood @ 2012-07-23 16:02 UTC (permalink / raw)
  To: Bhushan Bharat-R65777; +Cc: Wood Scott-B07421, kvm-ppc, agraf, kvm

On 07/23/2012 10:48 AM, Bhushan Bharat-R65777 wrote:
> 
> 
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Monday, July 23, 2012 9:12 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; agraf@suse.de; kvm@vger.kernel.org; Bhushan Bharat-
>> R65777
>> Subject: Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug
>> registers
>>
>> This will exceed the array size if userspace asks to access IAC3/4 on an e500-
>> family chip.
> 
> No , is not the array size already set to maximum. But yes we should not let IAC3/4 being accessed for e500 (FSL_BOOKE).
> 
>>
>> Why not just set the array at the max size unconditionally?
> 
> This is already coded this way. Please see the struct is this patch.

Sorry, I misread the array declaration.

-Scott

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
@ 2012-07-23 16:02       ` Scott Wood
  0 siblings, 0 replies; 18+ messages in thread
From: Scott Wood @ 2012-07-23 16:02 UTC (permalink / raw)
  To: Bhushan Bharat-R65777; +Cc: Wood Scott-B07421, kvm-ppc, agraf, kvm

On 07/23/2012 10:48 AM, Bhushan Bharat-R65777 wrote:
> 
> 
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Monday, July 23, 2012 9:12 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; agraf@suse.de; kvm@vger.kernel.org; Bhushan Bharat-
>> R65777
>> Subject: Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug
>> registers
>>
>> This will exceed the array size if userspace asks to access IAC3/4 on an e500-
>> family chip.
> 
> No , is not the array size already set to maximum. But yes we should not let IAC3/4 being accessed for e500 (FSL_BOOKE).
> 
>>
>> Why not just set the array at the max size unconditionally?
> 
> This is already coded this way. Please see the struct is this patch.

Sorry, I misread the array declaration.

-Scott



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
  2012-07-23 15:48     ` Bhushan Bharat-R65777
@ 2012-07-23 17:41       ` Alexander Graf
  -1 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2012-07-23 17:41 UTC (permalink / raw)
  To: Bhushan Bharat-R65777; +Cc: Wood Scott-B07421, kvm-ppc, kvm

On 07/23/2012 05:48 PM, Bhushan Bharat-R65777 wrote:
>
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Monday, July 23, 2012 9:12 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; agraf@suse.de; kvm@vger.kernel.org; Bhushan Bharat-
>> R65777
>> Subject: Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug
>> registers
>>
>> 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?
> No specific reason, The rest of debug ( which will follow sometime soon) uses this structure and looks to make code look easy.

So why not use an fsl / booke specific struct for the debug patches 
then? Debug registers are really nothing common between book3s and 
booke, so we shouldn't treat them as such by using the same struct 
definition.


Alex

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
@ 2012-07-23 17:41       ` Alexander Graf
  0 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2012-07-23 17:41 UTC (permalink / raw)
  To: Bhushan Bharat-R65777; +Cc: Wood Scott-B07421, kvm-ppc, kvm

On 07/23/2012 05:48 PM, Bhushan Bharat-R65777 wrote:
>
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Monday, July 23, 2012 9:12 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; agraf@suse.de; kvm@vger.kernel.org; Bhushan Bharat-
>> R65777
>> Subject: Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug
>> registers
>>
>> 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?
> No specific reason, The rest of debug ( which will follow sometime soon) uses this structure and looks to make code look easy.

So why not use an fsl / booke specific struct for the debug patches 
then? Debug registers are really nothing common between book3s and 
booke, so we shouldn't treat them as such by using the same struct 
definition.


Alex


^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
  2012-07-23 17:41       ` Alexander Graf
@ 2012-07-24  1:04         ` Bhushan Bharat-R65777
  -1 siblings, 0 replies; 18+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-07-24  1:04 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Wood Scott-B07421, kvm-ppc, kvm



> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Monday, July 23, 2012 11:12 PM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug
> registers
> 
> On 07/23/2012 05:48 PM, Bhushan Bharat-R65777 wrote:
> >
> >> -----Original Message-----
> >> From: Wood Scott-B07421
> >> Sent: Monday, July 23, 2012 9:12 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: kvm-ppc@vger.kernel.org; agraf@suse.de; kvm@vger.kernel.org;
> >> Bhushan Bharat-
> >> R65777
> >> Subject: Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC
> >> debug registers
> >>
> >> 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?
> > No specific reason, The rest of debug ( which will follow sometime soon) uses
> this structure and looks to make code look easy.
> 
> So why not use an fsl / booke specific struct for the debug patches then? Debug
> registers are really nothing common between book3s and booke, so we shouldn't
> treat them as such by using the same struct definition.
> 

All elements of struct are under #ifdef CONFIG_BOOKE? So for book3s it is as good as void, only struct type if declared. Do you want the struct type also under config_booke ?

Thanks
-Bharat


^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
@ 2012-07-24  1:04         ` Bhushan Bharat-R65777
  0 siblings, 0 replies; 18+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-07-24  1:04 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Wood Scott-B07421, kvm-ppc, kvm

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQWxleGFuZGVyIEdyYWYg
W21haWx0bzphZ3JhZkBzdXNlLmRlXQ0KPiBTZW50OiBNb25kYXksIEp1bHkgMjMsIDIwMTIgMTE6
MTIgUE0NCj4gVG86IEJodXNoYW4gQmhhcmF0LVI2NTc3Nw0KPiBDYzogV29vZCBTY290dC1CMDc0
MjE7IGt2bS1wcGNAdmdlci5rZXJuZWwub3JnOyBrdm1Admdlci5rZXJuZWwub3JnDQo+IFN1Ympl
Y3Q6IFJlOiBbUEFUQ0ggdjJdIGJvb2tlOiBBZGRlZCBPTkVfUkVHIGludGVyZmFjZSBmb3IgSUFD
L0RBQyBkZWJ1Zw0KPiByZWdpc3RlcnMNCj4gDQo+IE9uIDA3LzIzLzIwMTIgMDU6NDggUE0sIEJo
dXNoYW4gQmhhcmF0LVI2NTc3NyB3cm90ZToNCj4gPg0KPiA+PiAtLS0tLU9yaWdpbmFsIE1lc3Nh
Z2UtLS0tLQ0KPiA+PiBGcm9tOiBXb29kIFNjb3R0LUIwNzQyMQ0KPiA+PiBTZW50OiBNb25kYXks
IEp1bHkgMjMsIDIwMTIgOToxMiBQTQ0KPiA+PiBUbzogQmh1c2hhbiBCaGFyYXQtUjY1Nzc3DQo+
ID4+IENjOiBrdm0tcHBjQHZnZXIua2VybmVsLm9yZzsgYWdyYWZAc3VzZS5kZTsga3ZtQHZnZXIu
a2VybmVsLm9yZzsNCj4gPj4gQmh1c2hhbiBCaGFyYXQtDQo+ID4+IFI2NTc3Nw0KPiA+PiBTdWJq
ZWN0OiBSZTogW1BBVENIIHYyXSBib29rZTogQWRkZWQgT05FX1JFRyBpbnRlcmZhY2UgZm9yIElB
Qy9EQUMNCj4gPj4gZGVidWcgcmVnaXN0ZXJzDQo+ID4+DQo+ID4+IE9uIDA3LzIzLzIwMTIgMDY6
MTkgQU0sIEJoYXJhdCBCaHVzaGFuIHdyb3RlOg0KPiA+Pj4gSUFDL0RBQyBhcmUgZGVmaW5lZCBh
cyAzMiBiaXQgd2hpbGUgdGhleSBhcmUgNjQgYml0IHdpZGUuIFNvIE9ORV9SRUcNCj4gPj4+IGlu
dGVyZmFjZSBpcyBhZGRlZCB0byBzZXQvZ2V0IHRoZW0uDQo+ID4+Pg0KPiA+Pj4gU2lnbmVkLW9m
Zi1ieTogQmhhcmF0IEJodXNoYW4gPGJoYXJhdC5iaHVzaGFuQGZyZWVzY2FsZS5jb20+DQo+ID4+
PiAtLS0NCj4gPj4+IHYyOg0KPiA+Pj4gICAtIFVzaW5nIGNvcHlfdG8vZnJvbV91c2VyKCkgYXBp
cy4NCj4gPj4+DQo+ID4+PiAgIGFyY2gvcG93ZXJwYy9pbmNsdWRlL2FzbS9rdm0uaCAgICAgIHwg
ICAxMiArKysrKysNCj4gPj4+ICAgYXJjaC9wb3dlcnBjL2luY2x1ZGUvYXNtL2t2bV9ob3N0Lmgg
fCAgIDI4ICsrKysrKysrKysrKysrLQ0KPiA+Pj4gICBhcmNoL3Bvd2VycGMva3ZtL2Jvb2tlLmMg
ICAgICAgICAgICB8ICAgNjQNCj4gKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrLQ0K
PiA+Pj4gICBhcmNoL3Bvd2VycGMva3ZtL2Jvb2tlX2VtdWxhdGUuYyAgICB8ICAgIDggKystLQ0K
PiA+Pj4gICA0IGZpbGVzIGNoYW5nZWQsIDEwNCBpbnNlcnRpb25zKCspLCA4IGRlbGV0aW9ucygt
KQ0KPiA+Pj4NCj4gPj4+IGRpZmYgLS1naXQgYS9hcmNoL3Bvd2VycGMvaW5jbHVkZS9hc20va3Zt
LmgNCj4gPj4+IGIvYXJjaC9wb3dlcnBjL2luY2x1ZGUvYXNtL2t2bS5oIGluZGV4IDFiZWE0ZDgu
LjNjMTQyMDIgMTAwNjQ0DQo+ID4+PiAtLS0gYS9hcmNoL3Bvd2VycGMvaW5jbHVkZS9hc20va3Zt
LmgNCj4gPj4+ICsrKyBiL2FyY2gvcG93ZXJwYy9pbmNsdWRlL2FzbS9rdm0uaA0KPiA+Pj4gQEAg
LTIyMSw2ICsyMjEsMTIgQEAgc3RydWN0IGt2bV9zcmVncyB7DQo+ID4+Pg0KPiA+Pj4gICAJCQlf
X3UzMiBkYnNyOwkvKiBLVk1fU1JFR1NfRV9VUERBVEVfREJTUiAqLw0KPiA+Pj4gICAJCQlfX3Uz
MiBkYmNyWzNdOw0KPiA+Pj4gKwkJCS8qDQo+ID4+PiArCQkJICogaWFjL2RhYyByZWdpc3RlcnMg
YXJlIDY0Yml0IHdpZGUsIHdoaWxlIHRoaXMgQVBJDQo+ID4+PiArCQkJICogaW50ZXJmYWNlIHBy
b3ZpZGVzIG9ubHkgbG93ZXIgMzIgYml0cyBvbiA2NCBiaXQNCj4gPj4+ICsJCQkgKiBwcm9jZXNz
b3JzLiBPTkVfUkVHIGludGVyZmFjZSBpcyBhZGRlZCBmb3IgNjRiaXQNCj4gPj4+ICsJCQkgKiBp
YWMvZGFjIHJlZ2lzdGVycy4NCj4gPj4+ICsJCQkgKi8NCj4gPj4+ICAgCQkJX191MzIgaWFjWzRd
Ow0KPiA+Pj4gICAJCQlfX3UzMiBkYWNbMl07DQo+ID4+PiAgIAkJCV9fdTMyIGR2Y1syXTsNCj4g
Pj4+IEBAIC0zMjYsNSArMzMyLDExIEBAIHN0cnVjdCBrdm1fYm9vazNlXzIwNl90bGJfcGFyYW1z
IHsgIH07DQo+ID4+Pg0KPiA+Pj4gICAjZGVmaW5lIEtWTV9SRUdfUFBDX0hJT1IJKEtWTV9SRUdf
UFBDIHwgS1ZNX1JFR19TSVpFX1U2NCB8IDB4MSkNCj4gPj4+ICsjZGVmaW5lIEtWTV9SRUdfUFBD
X0lBQzEJKEtWTV9SRUdfUFBDIHwgS1ZNX1JFR19TSVpFX1U2NCB8IDB4MikNCj4gPj4+ICsjZGVm
aW5lIEtWTV9SRUdfUFBDX0lBQzIJKEtWTV9SRUdfUFBDIHwgS1ZNX1JFR19TSVpFX1U2NCB8IDB4
MykNCj4gPj4+ICsjZGVmaW5lIEtWTV9SRUdfUFBDX0lBQzMJKEtWTV9SRUdfUFBDIHwgS1ZNX1JF
R19TSVpFX1U2NCB8IDB4NCkNCj4gPj4+ICsjZGVmaW5lIEtWTV9SRUdfUFBDX0lBQzQJKEtWTV9S
RUdfUFBDIHwgS1ZNX1JFR19TSVpFX1U2NCB8IDB4NSkNCj4gPj4+ICsjZGVmaW5lIEtWTV9SRUdf
UFBDX0RBQzEJKEtWTV9SRUdfUFBDIHwgS1ZNX1JFR19TSVpFX1U2NCB8IDB4NikNCj4gPj4+ICsj
ZGVmaW5lIEtWTV9SRUdfUFBDX0RBQzIJKEtWTV9SRUdfUFBDIHwgS1ZNX1JFR19TSVpFX1U2NCB8
IDB4NykNCj4gPj4+DQo+ID4+PiAgICNlbmRpZiAvKiBfX0xJTlVYX0tWTV9QT1dFUlBDX0ggKi8g
ZGlmZiAtLWdpdA0KPiA+Pj4gYS9hcmNoL3Bvd2VycGMvaW5jbHVkZS9hc20va3ZtX2hvc3QuaA0K
PiA+Pj4gYi9hcmNoL3Bvd2VycGMvaW5jbHVkZS9hc20va3ZtX2hvc3QuaA0KPiA+Pj4gaW5kZXgg
NDNjYWM0OC4uMmMwZjAxNSAxMDA2NDQNCj4gPj4+IC0tLSBhL2FyY2gvcG93ZXJwYy9pbmNsdWRl
L2FzbS9rdm1faG9zdC5oDQo+ID4+PiArKysgYi9hcmNoL3Bvd2VycGMvaW5jbHVkZS9hc20va3Zt
X2hvc3QuaA0KPiA+Pj4gQEAgLTM0Miw2ICszNDIsMzEgQEAgc3RydWN0IGt2bXBwY19zbGIgew0K
PiA+Pj4gICAJYm9vbCBjbGFzcwk6IDE7DQo+ID4+PiAgIH07DQo+ID4+Pg0KPiA+Pj4gKyNpZmRl
ZiBDT05GSUdfQk9PS0UNCj4gPj4+ICsjIGlmZGVmIENPTkZJR19QUENfRlNMX0JPT0szRQ0KPiA+
Pj4gKyNkZWZpbmUgS1ZNUFBDX0lBQ19OVU0JMg0KPiA+Pj4gKyNkZWZpbmUgS1ZNUFBDX0RBQ19O
VU0JMg0KPiA+Pj4gKyMgZWxzZQ0KPiA+Pj4gKyNkZWZpbmUgS1ZNUFBDX0lBQ19OVU0JNA0KPiA+
Pj4gKyNkZWZpbmUgS1ZNUFBDX0RBQ19OVU0JMg0KPiA+Pj4gKyMgZW5kaWYNCj4gPj4+ICsjZGVm
aW5lIEtWTVBQQ19NQVhfSUFDCTQNCj4gPj4+ICsjZGVmaW5lIEtWTVBQQ19NQVhfREFDCTINCj4g
Pj4+ICsjZW5kaWYNCj4gPj4+ICsNCj4gPj4+ICtzdHJ1Y3Qga3ZtcHBjX2RlYnVnX3JlZyB7DQo+
ID4+PiArI2lmZGVmIENPTkZJR19CT09LRQ0KPiA+Pj4gKwl1MzIgZGJjcjA7DQo+ID4+PiArCXUz
MiBkYmNyMTsNCj4gPj4+ICsJdTMyIGRiY3IyOw0KPiA+Pj4gKyNpZmRlZiBDT05GSUdfS1ZNX0U1
MDBNQw0KPiA+Pj4gKwl1MzIgZGJjcjQ7DQo+ID4+PiArI2VuZGlmDQo+ID4+PiArCXU2NCBpYWNb
S1ZNUFBDX01BWF9JQUNdOw0KPiA+Pj4gKwl1NjQgZGFjW0tWTVBQQ19NQVhfREFDXTsNCj4gPj4+
ICsjZW5kaWYNCj4gPj4+ICt9Ow0KPiA+PiBJcyB0aGVyZSBhbnkgcmVhc29uIGZvciB0aGlzIHRv
IGJlIGEgc2VwYXJhdGUgc3RydWN0Pw0KPiA+IE5vIHNwZWNpZmljIHJlYXNvbiwgVGhlIHJlc3Qg
b2YgZGVidWcgKCB3aGljaCB3aWxsIGZvbGxvdyBzb21ldGltZSBzb29uKSB1c2VzDQo+IHRoaXMg
c3RydWN0dXJlIGFuZCBsb29rcyB0byBtYWtlIGNvZGUgbG9vayBlYXN5Lg0KPiANCj4gU28gd2h5
IG5vdCB1c2UgYW4gZnNsIC8gYm9va2Ugc3BlY2lmaWMgc3RydWN0IGZvciB0aGUgZGVidWcgcGF0
Y2hlcyB0aGVuPyBEZWJ1Zw0KPiByZWdpc3RlcnMgYXJlIHJlYWxseSBub3RoaW5nIGNvbW1vbiBi
ZXR3ZWVuIGJvb2szcyBhbmQgYm9va2UsIHNvIHdlIHNob3VsZG4ndA0KPiB0cmVhdCB0aGVtIGFz
IHN1Y2ggYnkgdXNpbmcgdGhlIHNhbWUgc3RydWN0IGRlZmluaXRpb24uDQo+IA0KDQpBbGwgZWxl
bWVudHMgb2Ygc3RydWN0IGFyZSB1bmRlciAjaWZkZWYgQ09ORklHX0JPT0tFPyBTbyBmb3IgYm9v
azNzIGl0IGlzIGFzIGdvb2QgYXMgdm9pZCwgb25seSBzdHJ1Y3QgdHlwZSBpZiBkZWNsYXJlZC4g
RG8geW91IHdhbnQgdGhlIHN0cnVjdCB0eXBlIGFsc28gdW5kZXIgY29uZmlnX2Jvb2tlID8NCg0K
VGhhbmtzDQotQmhhcmF0DQoNCg=


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
  2012-07-24  1:04         ` Bhushan Bharat-R65777
@ 2012-07-24 12:46           ` Alexander Graf
  -1 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2012-07-24 12:46 UTC (permalink / raw)
  To: Bhushan Bharat-R65777; +Cc: Wood Scott-B07421, kvm-ppc, kvm

On 07/24/2012 03:04 AM, Bhushan Bharat-R65777 wrote:
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Monday, July 23, 2012 11:12 PM
>> To: Bhushan Bharat-R65777
>> Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>> Subject: Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug
>> registers
>>
>> On 07/23/2012 05:48 PM, Bhushan Bharat-R65777 wrote:
>>>> -----Original Message-----
>>>> From: Wood Scott-B07421
>>>> Sent: Monday, July 23, 2012 9:12 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: kvm-ppc@vger.kernel.org; agraf@suse.de; kvm@vger.kernel.org;
>>>> Bhushan Bharat-
>>>> R65777
>>>> Subject: Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC
>>>> debug registers
>>>>
>>>> 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?
>>> No specific reason, The rest of debug ( which will follow sometime soon) uses
>> this structure and looks to make code look easy.
>>
>> So why not use an fsl / booke specific struct for the debug patches then? Debug
>> registers are really nothing common between book3s and booke, so we shouldn't
>> treat them as such by using the same struct definition.
>>
> All elements of struct are under #ifdef CONFIG_BOOKE? So for book3s it is as good as void, only struct type if declared. Do you want the struct type also under config_booke ?

struct kvmppc_booke_debug_reg {
    <lots of defines>
};

struct kvmppc_book3s_debug_reg {
    <lots of other defines>
};

void booke_foo() {
   struct kvmppc_booke_debug_reg r;
   r.dbcr0 = 0;
}

vs

struct kvmppc_debug_reg {
#ifdef CONFIG_BOOKE
   <lots of defines>
#else
   <lots of other defines>
#endif
};

void booke_foo() {
   struct kvmppc_debug_reg r;
   r.dbcr0 = 0;
}

Which one has less #ifdefs? Keep in mind that the less #ifdefs you have, 
the more readable and maintainable your code becomes, because config 
options have less effect on your syntax.


Alex

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
@ 2012-07-24 12:46           ` Alexander Graf
  0 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2012-07-24 12:46 UTC (permalink / raw)
  To: Bhushan Bharat-R65777; +Cc: Wood Scott-B07421, kvm-ppc, kvm

On 07/24/2012 03:04 AM, Bhushan Bharat-R65777 wrote:
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Monday, July 23, 2012 11:12 PM
>> To: Bhushan Bharat-R65777
>> Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>> Subject: Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug
>> registers
>>
>> On 07/23/2012 05:48 PM, Bhushan Bharat-R65777 wrote:
>>>> -----Original Message-----
>>>> From: Wood Scott-B07421
>>>> Sent: Monday, July 23, 2012 9:12 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: kvm-ppc@vger.kernel.org; agraf@suse.de; kvm@vger.kernel.org;
>>>> Bhushan Bharat-
>>>> R65777
>>>> Subject: Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC
>>>> debug registers
>>>>
>>>> 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?
>>> No specific reason, The rest of debug ( which will follow sometime soon) uses
>> this structure and looks to make code look easy.
>>
>> So why not use an fsl / booke specific struct for the debug patches then? Debug
>> registers are really nothing common between book3s and booke, so we shouldn't
>> treat them as such by using the same struct definition.
>>
> All elements of struct are under #ifdef CONFIG_BOOKE? So for book3s it is as good as void, only struct type if declared. Do you want the struct type also under config_booke ?

struct kvmppc_booke_debug_reg {
    <lots of defines>
};

struct kvmppc_book3s_debug_reg {
    <lots of other defines>
};

void booke_foo() {
   struct kvmppc_booke_debug_reg r;
   r.dbcr0 = 0;
}

vs

struct kvmppc_debug_reg {
#ifdef CONFIG_BOOKE
   <lots of defines>
#else
   <lots of other defines>
#endif
};

void booke_foo() {
   struct kvmppc_debug_reg r;
   r.dbcr0 = 0;
}

Which one has less #ifdefs? Keep in mind that the less #ifdefs you have, 
the more readable and maintainable your code becomes, because config 
options have less effect on your syntax.


Alex


^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
  2012-07-24 12:46           ` Alexander Graf
@ 2012-07-24 13:26             ` Bhushan Bharat-R65777
  -1 siblings, 0 replies; 18+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-07-24 13:26 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Wood Scott-B07421, kvm-ppc, kvm

> >>>>> +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?
> >>> No specific reason, The rest of debug ( which will follow sometime
> >>> soon) uses
> >> this structure and looks to make code look easy.
> >>
> >> So why not use an fsl / booke specific struct for the debug patches
> >> then? Debug registers are really nothing common between book3s and
> >> booke, so we shouldn't treat them as such by using the same struct
> definition.
> >>
> > All elements of struct are under #ifdef CONFIG_BOOKE? So for book3s it is as
> good as void, only struct type if declared. Do you want the struct type also
> under config_booke ?
> 
> struct kvmppc_booke_debug_reg {
>     <lots of defines>
> };
> 
> struct kvmppc_book3s_debug_reg {
>     <lots of other defines>
> };
> 
> void booke_foo() {
>    struct kvmppc_booke_debug_reg r;

kvmppc_booke_debug_reg or kvmppc_book3s_debug_reg ?

Thanks
-Bharat

>    r.dbcr0 = 0;
> }
> 
> vs
> 
> struct kvmppc_debug_reg {
> #ifdef CONFIG_BOOKE
>    <lots of defines>
> #else
>    <lots of other defines>
> #endif
> };
> 
> void booke_foo() {
>    struct kvmppc_debug_reg r;
>    r.dbcr0 = 0;
> }
> 
> Which one has less #ifdefs? Keep in mind that the less #ifdefs you have, the
> more readable and maintainable your code becomes, because config options have
> less effect on your syntax.
> 
> 
> Alex
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body
> of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html


^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
@ 2012-07-24 13:26             ` Bhushan Bharat-R65777
  0 siblings, 0 replies; 18+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-07-24 13:26 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Wood Scott-B07421, kvm-ppc, kvm

PiA+Pj4+PiArc3RydWN0IGt2bXBwY19kZWJ1Z19yZWcgew0KPiA+Pj4+PiArI2lmZGVmIENPTkZJ
R19CT09LRQ0KPiA+Pj4+PiArCXUzMiBkYmNyMDsNCj4gPj4+Pj4gKwl1MzIgZGJjcjE7DQo+ID4+
Pj4+ICsJdTMyIGRiY3IyOw0KPiA+Pj4+PiArI2lmZGVmIENPTkZJR19LVk1fRTUwME1DDQo+ID4+
Pj4+ICsJdTMyIGRiY3I0Ow0KPiA+Pj4+PiArI2VuZGlmDQo+ID4+Pj4+ICsJdTY0IGlhY1tLVk1Q
UENfTUFYX0lBQ107DQo+ID4+Pj4+ICsJdTY0IGRhY1tLVk1QUENfTUFYX0RBQ107DQo+ID4+Pj4+
ICsjZW5kaWYNCj4gPj4+Pj4gK307DQo+ID4+Pj4gSXMgdGhlcmUgYW55IHJlYXNvbiBmb3IgdGhp
cyB0byBiZSBhIHNlcGFyYXRlIHN0cnVjdD8NCj4gPj4+IE5vIHNwZWNpZmljIHJlYXNvbiwgVGhl
IHJlc3Qgb2YgZGVidWcgKCB3aGljaCB3aWxsIGZvbGxvdyBzb21ldGltZQ0KPiA+Pj4gc29vbikg
dXNlcw0KPiA+PiB0aGlzIHN0cnVjdHVyZSBhbmQgbG9va3MgdG8gbWFrZSBjb2RlIGxvb2sgZWFz
eS4NCj4gPj4NCj4gPj4gU28gd2h5IG5vdCB1c2UgYW4gZnNsIC8gYm9va2Ugc3BlY2lmaWMgc3Ry
dWN0IGZvciB0aGUgZGVidWcgcGF0Y2hlcw0KPiA+PiB0aGVuPyBEZWJ1ZyByZWdpc3RlcnMgYXJl
IHJlYWxseSBub3RoaW5nIGNvbW1vbiBiZXR3ZWVuIGJvb2szcyBhbmQNCj4gPj4gYm9va2UsIHNv
IHdlIHNob3VsZG4ndCB0cmVhdCB0aGVtIGFzIHN1Y2ggYnkgdXNpbmcgdGhlIHNhbWUgc3RydWN0
DQo+IGRlZmluaXRpb24uDQo+ID4+DQo+ID4gQWxsIGVsZW1lbnRzIG9mIHN0cnVjdCBhcmUgdW5k
ZXIgI2lmZGVmIENPTkZJR19CT09LRT8gU28gZm9yIGJvb2szcyBpdCBpcyBhcw0KPiBnb29kIGFz
IHZvaWQsIG9ubHkgc3RydWN0IHR5cGUgaWYgZGVjbGFyZWQuIERvIHlvdSB3YW50IHRoZSBzdHJ1
Y3QgdHlwZSBhbHNvDQo+IHVuZGVyIGNvbmZpZ19ib29rZSA/DQo+IA0KPiBzdHJ1Y3Qga3ZtcHBj
X2Jvb2tlX2RlYnVnX3JlZyB7DQo+ICAgICA8bG90cyBvZiBkZWZpbmVzPg0KPiB9Ow0KPiANCj4g
c3RydWN0IGt2bXBwY19ib29rM3NfZGVidWdfcmVnIHsNCj4gICAgIDxsb3RzIG9mIG90aGVyIGRl
ZmluZXM+DQo+IH07DQo+IA0KPiB2b2lkIGJvb2tlX2ZvbygpIHsNCj4gICAgc3RydWN0IGt2bXBw
Y19ib29rZV9kZWJ1Z19yZWcgcjsNCg0Ka3ZtcHBjX2Jvb2tlX2RlYnVnX3JlZyBvciBrdm1wcGNf
Ym9vazNzX2RlYnVnX3JlZyA/DQoNClRoYW5rcw0KLUJoYXJhdA0KDQo+ICAgIHIuZGJjcjAgPSAw
Ow0KPiB9DQo+IA0KPiB2cw0KPiANCj4gc3RydWN0IGt2bXBwY19kZWJ1Z19yZWcgew0KPiAjaWZk
ZWYgQ09ORklHX0JPT0tFDQo+ICAgIDxsb3RzIG9mIGRlZmluZXM+DQo+ICNlbHNlDQo+ICAgIDxs
b3RzIG9mIG90aGVyIGRlZmluZXM+DQo+ICNlbmRpZg0KPiB9Ow0KPiANCj4gdm9pZCBib29rZV9m
b28oKSB7DQo+ICAgIHN0cnVjdCBrdm1wcGNfZGVidWdfcmVnIHI7DQo+ICAgIHIuZGJjcjAgPSAw
Ow0KPiB9DQo+IA0KPiBXaGljaCBvbmUgaGFzIGxlc3MgI2lmZGVmcz8gS2VlcCBpbiBtaW5kIHRo
YXQgdGhlIGxlc3MgI2lmZGVmcyB5b3UgaGF2ZSwgdGhlDQo+IG1vcmUgcmVhZGFibGUgYW5kIG1h
aW50YWluYWJsZSB5b3VyIGNvZGUgYmVjb21lcywgYmVjYXVzZSBjb25maWcgb3B0aW9ucyBoYXZl
DQo+IGxlc3MgZWZmZWN0IG9uIHlvdXIgc3ludGF4Lg0KPiANCj4gDQo+IEFsZXgNCj4gDQo+IC0t
DQo+IFRvIHVuc3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNj
cmliZSBrdm0tcHBjIiBpbiB0aGUgYm9keQ0KPiBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZn
ZXIua2VybmVsLm9yZyBNb3JlIG1ham9yZG9tbyBpbmZvIGF0DQo+IGh0dHA6Ly92Z2VyLmtlcm5l
bC5vcmcvbWFqb3Jkb21vLWluZm8uaHRtbA0KDQo

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
  2012-07-24 13:26             ` Bhushan Bharat-R65777
@ 2012-07-24 14:56               ` Alexander Graf
  -1 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2012-07-24 14:56 UTC (permalink / raw)
  To: Bhushan Bharat-R65777; +Cc: Wood Scott-B07421, kvm-ppc, kvm

On 07/24/2012 03:26 PM, Bhushan Bharat-R65777 wrote:
>>>>>>> +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?
>>>>> No specific reason, The rest of debug ( which will follow sometime
>>>>> soon) uses
>>>> this structure and looks to make code look easy.
>>>>
>>>> So why not use an fsl / booke specific struct for the debug patches
>>>> then? Debug registers are really nothing common between book3s and
>>>> booke, so we shouldn't treat them as such by using the same struct
>> definition.
>>> All elements of struct are under #ifdef CONFIG_BOOKE? So for book3s it is as
>> good as void, only struct type if declared. Do you want the struct type also
>> under config_booke ?
>>
>> struct kvmppc_booke_debug_reg {
>>      <lots of defines>
>> };
>>
>> struct kvmppc_book3s_debug_reg {
>>      <lots of other defines>
>> };
>>
>> void booke_foo() {
>>     struct kvmppc_booke_debug_reg r;
> kvmppc_booke_debug_reg or kvmppc_book3s_debug_reg ?

In booke_foo() you certainly will never want to use struct 
kvmppc_book3s_debug_reg, no?


Alex


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers
@ 2012-07-24 14:56               ` Alexander Graf
  0 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2012-07-24 14:56 UTC (permalink / raw)
  To: Bhushan Bharat-R65777; +Cc: Wood Scott-B07421, kvm-ppc, kvm

On 07/24/2012 03:26 PM, Bhushan Bharat-R65777 wrote:
>>>>>>> +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?
>>>>> No specific reason, The rest of debug ( which will follow sometime
>>>>> soon) uses
>>>> this structure and looks to make code look easy.
>>>>
>>>> So why not use an fsl / booke specific struct for the debug patches
>>>> then? Debug registers are really nothing common between book3s and
>>>> booke, so we shouldn't treat them as such by using the same struct
>> definition.
>>> All elements of struct are under #ifdef CONFIG_BOOKE? So for book3s it is as
>> good as void, only struct type if declared. Do you want the struct type also
>> under config_booke ?
>>
>> struct kvmppc_booke_debug_reg {
>>      <lots of defines>
>> };
>>
>> struct kvmppc_book3s_debug_reg {
>>      <lots of other defines>
>> };
>>
>> void booke_foo() {
>>     struct kvmppc_booke_debug_reg r;
> kvmppc_booke_debug_reg or kvmppc_book3s_debug_reg ?

In booke_foo() you certainly will never want to use struct 
kvmppc_book3s_debug_reg, no?


Alex


^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2012-07-24 14:56 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-23 11:19 [PATCH v2] booke: Added ONE_REG interface for IAC/DAC debug registers Bharat Bhushan
2012-07-23 11:31 ` Bharat Bhushan
2012-07-23 15:42 ` Scott Wood
2012-07-23 15:42   ` Scott Wood
2012-07-23 15:48   ` Bhushan Bharat-R65777
2012-07-23 15:48     ` Bhushan Bharat-R65777
2012-07-23 16:02     ` Scott Wood
2012-07-23 16:02       ` Scott Wood
2012-07-23 17:41     ` Alexander Graf
2012-07-23 17:41       ` Alexander Graf
2012-07-24  1:04       ` Bhushan Bharat-R65777
2012-07-24  1:04         ` Bhushan Bharat-R65777
2012-07-24 12:46         ` Alexander Graf
2012-07-24 12:46           ` Alexander Graf
2012-07-24 13:26           ` Bhushan Bharat-R65777
2012-07-24 13:26             ` Bhushan Bharat-R65777
2012-07-24 14:56             ` Alexander Graf
2012-07-24 14:56               ` Alexander Graf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.