All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: arm64: Add missing index for trapping debug registers
@ 2021-05-14  1:49 ` Ricardo Koller
  0 siblings, 0 replies; 6+ messages in thread
From: Ricardo Koller @ 2021-05-14  1:49 UTC (permalink / raw)
  To: kvmarm; +Cc: maz, linux-arm-kernel

Trapping an access to debug register <n> (like bvr<n>, bcr<n>, wvr<n>,
wcr<n>) results in storing and loading values from the vcpu copy at
index 0 (irrespective of <n>). So, this guest test fails:

  /* traps and wrongly stores 0x123 into vcpu->bvr[0] */
  write_sysreg(dbgbvr1_el1, 0x123);
  /* reads 0 from the real bvr[1] without trapping */
  GUEST_ASSERT(read_sysreg(dbgbvr1_el1) == 0x123); /* check fails */

Fix this by setting the register index in macro DBG_BCR_BVR_WCR_WVR_EL1
to <n>.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/sys_regs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 76ea2800c33e..e4ec9edd49fa 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -935,13 +935,13 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
 #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
 	{ SYS_DESC(SYS_DBGBVRn_EL1(n)),					\
-	  trap_bvr, reset_bvr, 0, 0, get_bvr, set_bvr },		\
+	  trap_bvr, reset_bvr, n, 0, get_bvr, set_bvr },		\
 	{ SYS_DESC(SYS_DBGBCRn_EL1(n)),					\
-	  trap_bcr, reset_bcr, 0, 0, get_bcr, set_bcr },		\
+	  trap_bcr, reset_bcr, n, 0, get_bcr, set_bcr },		\
 	{ SYS_DESC(SYS_DBGWVRn_EL1(n)),					\
-	  trap_wvr, reset_wvr, 0, 0,  get_wvr, set_wvr },		\
+	  trap_wvr, reset_wvr, n, 0,  get_wvr, set_wvr },		\
 	{ SYS_DESC(SYS_DBGWCRn_EL1(n)),					\
-	  trap_wcr, reset_wcr, 0, 0,  get_wcr, set_wcr }
+	  trap_wcr, reset_wcr, n, 0,  get_wcr, set_wcr }
 
 #define PMU_SYS_REG(r)						\
 	SYS_DESC(r), .reset = reset_unknown, .visibility = pmu_visibility
-- 
2.31.1.751.gd2f1c929bd-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

* [PATCH] KVM: arm64: Add missing index for trapping debug registers
@ 2021-05-14  1:49 ` Ricardo Koller
  0 siblings, 0 replies; 6+ messages in thread
From: Ricardo Koller @ 2021-05-14  1:49 UTC (permalink / raw)
  To: kvmarm
  Cc: maz, james.morse, alexandru.elisei, drjones, suzuki.poulose,
	linux-arm-kernel, Ricardo Koller

Trapping an access to debug register <n> (like bvr<n>, bcr<n>, wvr<n>,
wcr<n>) results in storing and loading values from the vcpu copy at
index 0 (irrespective of <n>). So, this guest test fails:

  /* traps and wrongly stores 0x123 into vcpu->bvr[0] */
  write_sysreg(dbgbvr1_el1, 0x123);
  /* reads 0 from the real bvr[1] without trapping */
  GUEST_ASSERT(read_sysreg(dbgbvr1_el1) == 0x123); /* check fails */

Fix this by setting the register index in macro DBG_BCR_BVR_WCR_WVR_EL1
to <n>.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/sys_regs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 76ea2800c33e..e4ec9edd49fa 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -935,13 +935,13 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
 #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
 	{ SYS_DESC(SYS_DBGBVRn_EL1(n)),					\
-	  trap_bvr, reset_bvr, 0, 0, get_bvr, set_bvr },		\
+	  trap_bvr, reset_bvr, n, 0, get_bvr, set_bvr },		\
 	{ SYS_DESC(SYS_DBGBCRn_EL1(n)),					\
-	  trap_bcr, reset_bcr, 0, 0, get_bcr, set_bcr },		\
+	  trap_bcr, reset_bcr, n, 0, get_bcr, set_bcr },		\
 	{ SYS_DESC(SYS_DBGWVRn_EL1(n)),					\
-	  trap_wvr, reset_wvr, 0, 0,  get_wvr, set_wvr },		\
+	  trap_wvr, reset_wvr, n, 0,  get_wvr, set_wvr },		\
 	{ SYS_DESC(SYS_DBGWCRn_EL1(n)),					\
-	  trap_wcr, reset_wcr, 0, 0,  get_wcr, set_wcr }
+	  trap_wcr, reset_wcr, n, 0,  get_wcr, set_wcr }
 
 #define PMU_SYS_REG(r)						\
 	SYS_DESC(r), .reset = reset_unknown, .visibility = pmu_visibility
-- 
2.31.1.751.gd2f1c929bd-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: [PATCH] KVM: arm64: Add missing index for trapping debug registers
  2021-05-14  1:49 ` Ricardo Koller
@ 2021-05-14  8:19   ` Marc Zyngier
  -1 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2021-05-14  8:19 UTC (permalink / raw)
  To: Ricardo Koller; +Cc: kvmarm, linux-arm-kernel

Hi Ricardo,

On Fri, 14 May 2021 02:49:06 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> Trapping an access to debug register <n> (like bvr<n>, bcr<n>, wvr<n>,
> wcr<n>) results in storing and loading values from the vcpu copy at
> index 0 (irrespective of <n>). So, this guest test fails:
> 
>   /* traps and wrongly stores 0x123 into vcpu->bvr[0] */
>   write_sysreg(dbgbvr1_el1, 0x123);
>   /* reads 0 from the real bvr[1] without trapping */
>   GUEST_ASSERT(read_sysreg(dbgbvr1_el1) == 0x123); /* check fails */

Bummer... But nice catch! I broke it in 03fdfb2690099 ("KVM: arm64:
Don't write junk to sysregs on reset").

> Fix this by setting the register index in macro DBG_BCR_BVR_WCR_WVR_EL1
> to <n>.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 76ea2800c33e..e4ec9edd49fa 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -935,13 +935,13 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
>  #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
>  	{ SYS_DESC(SYS_DBGBVRn_EL1(n)),					\
> -	  trap_bvr, reset_bvr, 0, 0, get_bvr, set_bvr },		\
> +	  trap_bvr, reset_bvr, n, 0, get_bvr, set_bvr },		\
>  	{ SYS_DESC(SYS_DBGBCRn_EL1(n)),					\
> -	  trap_bcr, reset_bcr, 0, 0, get_bcr, set_bcr },		\
> +	  trap_bcr, reset_bcr, n, 0, get_bcr, set_bcr },		\
>  	{ SYS_DESC(SYS_DBGWVRn_EL1(n)),					\
> -	  trap_wvr, reset_wvr, 0, 0,  get_wvr, set_wvr },		\
> +	  trap_wvr, reset_wvr, n, 0,  get_wvr, set_wvr },		\
>  	{ SYS_DESC(SYS_DBGWCRn_EL1(n)),					\
> -	  trap_wcr, reset_wcr, 0, 0,  get_wcr, set_wcr }
> +	  trap_wcr, reset_wcr, n, 0,  get_wcr, set_wcr }
>  
>  #define PMU_SYS_REG(r)						\
>  	SYS_DESC(r), .reset = reset_unknown, .visibility = pmu_visibility

Unfortunately, I don't think that's the right fix either, and just
setting it to the debug register index will do the wrong thing in
reset_sys_reg_descs().

The reason is that the debug registers don't live in the per-vcpu
sysreg array, but instead in some private structure, owing to the fact
that we support both guest and host-side debugging. The value '0' here
is in indication that the shadow registers are "somewhere else".

I think the correct fix is to use the register encoding itself, which
contains everything we need (for all the debug regs, the CRm field is
the debug register index). That's what it should have been the first
place.

Could you please give the following patch a go?

Thanks,

	M.

From cfca75f1e2ab5dd1933de59b90f31293821fd2de Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Fri, 14 May 2021 09:05:41 +0100
Subject: [PATCH] KVM: arm64: Fix debug register indexing

Commit 03fdfb2690099 ("KVM: arm64: Don't write junk to sysregs on
reset") flipped the register number to 0 for all the debug registers
in the sysreg table, hereby indicating that these registers live
in a separate shadow structure.

However, the author of this patch failed to realise that all the
accessors are using that particular index instead of the register
encoding, resulting in all the registers hitting index 0. Not quite
a valid implementation of the architecture...

Address the issue by fixing all the accessors to use the CRm field
of the encoding, which contains the debug register index.

Fixes: 03fdfb2690099 ("KVM: arm64: Don't write junk to sysregs on reset")
Reported-by: Ricardo Koller <ricarkol@google.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/kvm/sys_regs.c | 42 +++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 76ea2800c33e..1a7968ad078c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -399,14 +399,14 @@ static bool trap_bvr(struct kvm_vcpu *vcpu,
 		     struct sys_reg_params *p,
 		     const struct sys_reg_desc *rd)
 {
-	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
+	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
 
 	if (p->is_write)
 		reg_to_dbg(vcpu, p, rd, dbg_reg);
 	else
 		dbg_to_reg(vcpu, p, rd, dbg_reg);
 
-	trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
+	trace_trap_reg(__func__, rd->CRm, p->is_write, *dbg_reg);
 
 	return true;
 }
@@ -414,7 +414,7 @@ static bool trap_bvr(struct kvm_vcpu *vcpu,
 static int set_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
 
 	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -424,7 +424,7 @@ static int set_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
 
 	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -434,21 +434,21 @@ static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static void reset_bvr(struct kvm_vcpu *vcpu,
 		      const struct sys_reg_desc *rd)
 {
-	vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg] = rd->val;
+	vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm] = rd->val;
 }
 
 static bool trap_bcr(struct kvm_vcpu *vcpu,
 		     struct sys_reg_params *p,
 		     const struct sys_reg_desc *rd)
 {
-	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
+	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
 
 	if (p->is_write)
 		reg_to_dbg(vcpu, p, rd, dbg_reg);
 	else
 		dbg_to_reg(vcpu, p, rd, dbg_reg);
 
-	trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
+	trace_trap_reg(__func__, rd->CRm, p->is_write, *dbg_reg);
 
 	return true;
 }
@@ -456,7 +456,7 @@ static bool trap_bcr(struct kvm_vcpu *vcpu,
 static int set_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
 
 	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -467,7 +467,7 @@ static int set_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
 
 	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -477,22 +477,22 @@ static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static void reset_bcr(struct kvm_vcpu *vcpu,
 		      const struct sys_reg_desc *rd)
 {
-	vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg] = rd->val;
+	vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm] = rd->val;
 }
 
 static bool trap_wvr(struct kvm_vcpu *vcpu,
 		     struct sys_reg_params *p,
 		     const struct sys_reg_desc *rd)
 {
-	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
+	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
 
 	if (p->is_write)
 		reg_to_dbg(vcpu, p, rd, dbg_reg);
 	else
 		dbg_to_reg(vcpu, p, rd, dbg_reg);
 
-	trace_trap_reg(__func__, rd->reg, p->is_write,
-		vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg]);
+	trace_trap_reg(__func__, rd->CRm, p->is_write,
+		vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm]);
 
 	return true;
 }
@@ -500,7 +500,7 @@ static bool trap_wvr(struct kvm_vcpu *vcpu,
 static int set_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
 
 	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -510,7 +510,7 @@ static int set_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
 
 	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -520,21 +520,21 @@ static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static void reset_wvr(struct kvm_vcpu *vcpu,
 		      const struct sys_reg_desc *rd)
 {
-	vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg] = rd->val;
+	vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm] = rd->val;
 }
 
 static bool trap_wcr(struct kvm_vcpu *vcpu,
 		     struct sys_reg_params *p,
 		     const struct sys_reg_desc *rd)
 {
-	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
+	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
 
 	if (p->is_write)
 		reg_to_dbg(vcpu, p, rd, dbg_reg);
 	else
 		dbg_to_reg(vcpu, p, rd, dbg_reg);
 
-	trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
+	trace_trap_reg(__func__, rd->CRm, p->is_write, *dbg_reg);
 
 	return true;
 }
@@ -542,7 +542,7 @@ static bool trap_wcr(struct kvm_vcpu *vcpu,
 static int set_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
 
 	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -552,7 +552,7 @@ static int set_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
 
 	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -562,7 +562,7 @@ static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static void reset_wcr(struct kvm_vcpu *vcpu,
 		      const struct sys_reg_desc *rd)
 {
-	vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg] = rd->val;
+	vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm] = rd->val;
 }
 
 static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
-- 
2.30.2


-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

* Re: [PATCH] KVM: arm64: Add missing index for trapping debug registers
@ 2021-05-14  8:19   ` Marc Zyngier
  0 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2021-05-14  8:19 UTC (permalink / raw)
  To: Ricardo Koller
  Cc: kvmarm, james.morse, alexandru.elisei, drjones, suzuki.poulose,
	linux-arm-kernel

Hi Ricardo,

On Fri, 14 May 2021 02:49:06 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> Trapping an access to debug register <n> (like bvr<n>, bcr<n>, wvr<n>,
> wcr<n>) results in storing and loading values from the vcpu copy at
> index 0 (irrespective of <n>). So, this guest test fails:
> 
>   /* traps and wrongly stores 0x123 into vcpu->bvr[0] */
>   write_sysreg(dbgbvr1_el1, 0x123);
>   /* reads 0 from the real bvr[1] without trapping */
>   GUEST_ASSERT(read_sysreg(dbgbvr1_el1) == 0x123); /* check fails */

Bummer... But nice catch! I broke it in 03fdfb2690099 ("KVM: arm64:
Don't write junk to sysregs on reset").

> Fix this by setting the register index in macro DBG_BCR_BVR_WCR_WVR_EL1
> to <n>.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 76ea2800c33e..e4ec9edd49fa 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -935,13 +935,13 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
>  #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
>  	{ SYS_DESC(SYS_DBGBVRn_EL1(n)),					\
> -	  trap_bvr, reset_bvr, 0, 0, get_bvr, set_bvr },		\
> +	  trap_bvr, reset_bvr, n, 0, get_bvr, set_bvr },		\
>  	{ SYS_DESC(SYS_DBGBCRn_EL1(n)),					\
> -	  trap_bcr, reset_bcr, 0, 0, get_bcr, set_bcr },		\
> +	  trap_bcr, reset_bcr, n, 0, get_bcr, set_bcr },		\
>  	{ SYS_DESC(SYS_DBGWVRn_EL1(n)),					\
> -	  trap_wvr, reset_wvr, 0, 0,  get_wvr, set_wvr },		\
> +	  trap_wvr, reset_wvr, n, 0,  get_wvr, set_wvr },		\
>  	{ SYS_DESC(SYS_DBGWCRn_EL1(n)),					\
> -	  trap_wcr, reset_wcr, 0, 0,  get_wcr, set_wcr }
> +	  trap_wcr, reset_wcr, n, 0,  get_wcr, set_wcr }
>  
>  #define PMU_SYS_REG(r)						\
>  	SYS_DESC(r), .reset = reset_unknown, .visibility = pmu_visibility

Unfortunately, I don't think that's the right fix either, and just
setting it to the debug register index will do the wrong thing in
reset_sys_reg_descs().

The reason is that the debug registers don't live in the per-vcpu
sysreg array, but instead in some private structure, owing to the fact
that we support both guest and host-side debugging. The value '0' here
is in indication that the shadow registers are "somewhere else".

I think the correct fix is to use the register encoding itself, which
contains everything we need (for all the debug regs, the CRm field is
the debug register index). That's what it should have been the first
place.

Could you please give the following patch a go?

Thanks,

	M.

From cfca75f1e2ab5dd1933de59b90f31293821fd2de Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Fri, 14 May 2021 09:05:41 +0100
Subject: [PATCH] KVM: arm64: Fix debug register indexing

Commit 03fdfb2690099 ("KVM: arm64: Don't write junk to sysregs on
reset") flipped the register number to 0 for all the debug registers
in the sysreg table, hereby indicating that these registers live
in a separate shadow structure.

However, the author of this patch failed to realise that all the
accessors are using that particular index instead of the register
encoding, resulting in all the registers hitting index 0. Not quite
a valid implementation of the architecture...

Address the issue by fixing all the accessors to use the CRm field
of the encoding, which contains the debug register index.

Fixes: 03fdfb2690099 ("KVM: arm64: Don't write junk to sysregs on reset")
Reported-by: Ricardo Koller <ricarkol@google.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/kvm/sys_regs.c | 42 +++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 76ea2800c33e..1a7968ad078c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -399,14 +399,14 @@ static bool trap_bvr(struct kvm_vcpu *vcpu,
 		     struct sys_reg_params *p,
 		     const struct sys_reg_desc *rd)
 {
-	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
+	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
 
 	if (p->is_write)
 		reg_to_dbg(vcpu, p, rd, dbg_reg);
 	else
 		dbg_to_reg(vcpu, p, rd, dbg_reg);
 
-	trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
+	trace_trap_reg(__func__, rd->CRm, p->is_write, *dbg_reg);
 
 	return true;
 }
@@ -414,7 +414,7 @@ static bool trap_bvr(struct kvm_vcpu *vcpu,
 static int set_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
 
 	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -424,7 +424,7 @@ static int set_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
 
 	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -434,21 +434,21 @@ static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static void reset_bvr(struct kvm_vcpu *vcpu,
 		      const struct sys_reg_desc *rd)
 {
-	vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg] = rd->val;
+	vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm] = rd->val;
 }
 
 static bool trap_bcr(struct kvm_vcpu *vcpu,
 		     struct sys_reg_params *p,
 		     const struct sys_reg_desc *rd)
 {
-	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
+	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
 
 	if (p->is_write)
 		reg_to_dbg(vcpu, p, rd, dbg_reg);
 	else
 		dbg_to_reg(vcpu, p, rd, dbg_reg);
 
-	trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
+	trace_trap_reg(__func__, rd->CRm, p->is_write, *dbg_reg);
 
 	return true;
 }
@@ -456,7 +456,7 @@ static bool trap_bcr(struct kvm_vcpu *vcpu,
 static int set_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
 
 	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -467,7 +467,7 @@ static int set_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
 
 	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -477,22 +477,22 @@ static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static void reset_bcr(struct kvm_vcpu *vcpu,
 		      const struct sys_reg_desc *rd)
 {
-	vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg] = rd->val;
+	vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm] = rd->val;
 }
 
 static bool trap_wvr(struct kvm_vcpu *vcpu,
 		     struct sys_reg_params *p,
 		     const struct sys_reg_desc *rd)
 {
-	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
+	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
 
 	if (p->is_write)
 		reg_to_dbg(vcpu, p, rd, dbg_reg);
 	else
 		dbg_to_reg(vcpu, p, rd, dbg_reg);
 
-	trace_trap_reg(__func__, rd->reg, p->is_write,
-		vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg]);
+	trace_trap_reg(__func__, rd->CRm, p->is_write,
+		vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm]);
 
 	return true;
 }
@@ -500,7 +500,7 @@ static bool trap_wvr(struct kvm_vcpu *vcpu,
 static int set_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
 
 	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -510,7 +510,7 @@ static int set_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
 
 	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -520,21 +520,21 @@ static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static void reset_wvr(struct kvm_vcpu *vcpu,
 		      const struct sys_reg_desc *rd)
 {
-	vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg] = rd->val;
+	vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm] = rd->val;
 }
 
 static bool trap_wcr(struct kvm_vcpu *vcpu,
 		     struct sys_reg_params *p,
 		     const struct sys_reg_desc *rd)
 {
-	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
+	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
 
 	if (p->is_write)
 		reg_to_dbg(vcpu, p, rd, dbg_reg);
 	else
 		dbg_to_reg(vcpu, p, rd, dbg_reg);
 
-	trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
+	trace_trap_reg(__func__, rd->CRm, p->is_write, *dbg_reg);
 
 	return true;
 }
@@ -542,7 +542,7 @@ static bool trap_wcr(struct kvm_vcpu *vcpu,
 static int set_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
 
 	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -552,7 +552,7 @@ static int set_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
+	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
 
 	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
 		return -EFAULT;
@@ -562,7 +562,7 @@ static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 static void reset_wcr(struct kvm_vcpu *vcpu,
 		      const struct sys_reg_desc *rd)
 {
-	vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg] = rd->val;
+	vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm] = rd->val;
 }
 
 static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
-- 
2.30.2


-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: [PATCH] KVM: arm64: Add missing index for trapping debug registers
  2021-05-14  8:19   ` Marc Zyngier
@ 2021-05-14 17:53     ` Ricardo Koller
  -1 siblings, 0 replies; 6+ messages in thread
From: Ricardo Koller @ 2021-05-14 17:53 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: kvmarm, linux-arm-kernel

On Fri, May 14, 2021 at 09:19:29AM +0100, Marc Zyngier wrote:
> Hi Ricardo,
> 
> On Fri, 14 May 2021 02:49:06 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > Trapping an access to debug register <n> (like bvr<n>, bcr<n>, wvr<n>,
> > wcr<n>) results in storing and loading values from the vcpu copy at
> > index 0 (irrespective of <n>). So, this guest test fails:
> > 
> >   /* traps and wrongly stores 0x123 into vcpu->bvr[0] */
> >   write_sysreg(dbgbvr1_el1, 0x123);
> >   /* reads 0 from the real bvr[1] without trapping */
> >   GUEST_ASSERT(read_sysreg(dbgbvr1_el1) == 0x123); /* check fails */
> 
> Bummer... But nice catch! I broke it in 03fdfb2690099 ("KVM: arm64:
> Don't write junk to sysregs on reset").
> 
> > Fix this by setting the register index in macro DBG_BCR_BVR_WCR_WVR_EL1
> > to <n>.
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 76ea2800c33e..e4ec9edd49fa 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -935,13 +935,13 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >  /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
> >  #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
> >  	{ SYS_DESC(SYS_DBGBVRn_EL1(n)),					\
> > -	  trap_bvr, reset_bvr, 0, 0, get_bvr, set_bvr },		\
> > +	  trap_bvr, reset_bvr, n, 0, get_bvr, set_bvr },		\
> >  	{ SYS_DESC(SYS_DBGBCRn_EL1(n)),					\
> > -	  trap_bcr, reset_bcr, 0, 0, get_bcr, set_bcr },		\
> > +	  trap_bcr, reset_bcr, n, 0, get_bcr, set_bcr },		\
> >  	{ SYS_DESC(SYS_DBGWVRn_EL1(n)),					\
> > -	  trap_wvr, reset_wvr, 0, 0,  get_wvr, set_wvr },		\
> > +	  trap_wvr, reset_wvr, n, 0,  get_wvr, set_wvr },		\
> >  	{ SYS_DESC(SYS_DBGWCRn_EL1(n)),					\
> > -	  trap_wcr, reset_wcr, 0, 0,  get_wcr, set_wcr }
> > +	  trap_wcr, reset_wcr, n, 0,  get_wcr, set_wcr }
> >  
> >  #define PMU_SYS_REG(r)						\
> >  	SYS_DESC(r), .reset = reset_unknown, .visibility = pmu_visibility
> 
> Unfortunately, I don't think that's the right fix either, and just
> setting it to the debug register index will do the wrong thing in
> reset_sys_reg_descs().
> 

I see, got it.

> The reason is that the debug registers don't live in the per-vcpu
> sysreg array, but instead in some private structure, owing to the fact
> that we support both guest and host-side debugging. The value '0' here
> is in indication that the shadow registers are "somewhere else".
> 
> I think the correct fix is to use the register encoding itself, which
> contains everything we need (for all the debug regs, the CRm field is
> the debug register index). That's what it should have been the first
> place.
> 
> Could you please give the following patch a go?
> 

I just tried it and it fixes the test described in the commit message:
wr(bvr1),rd(bvr1).

Thanks,
Ricardo

> Thanks,
> 
> 	M.
> 
> From cfca75f1e2ab5dd1933de59b90f31293821fd2de Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Fri, 14 May 2021 09:05:41 +0100
> Subject: [PATCH] KVM: arm64: Fix debug register indexing
> 
> Commit 03fdfb2690099 ("KVM: arm64: Don't write junk to sysregs on
> reset") flipped the register number to 0 for all the debug registers
> in the sysreg table, hereby indicating that these registers live
> in a separate shadow structure.
> 
> However, the author of this patch failed to realise that all the
> accessors are using that particular index instead of the register
> encoding, resulting in all the registers hitting index 0. Not quite
> a valid implementation of the architecture...
> 
> Address the issue by fixing all the accessors to use the CRm field
> of the encoding, which contains the debug register index.
> 
> Fixes: 03fdfb2690099 ("KVM: arm64: Don't write junk to sysregs on reset")
> Reported-by: Ricardo Koller <ricarkol@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  arch/arm64/kvm/sys_regs.c | 42 +++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 76ea2800c33e..1a7968ad078c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -399,14 +399,14 @@ static bool trap_bvr(struct kvm_vcpu *vcpu,
>  		     struct sys_reg_params *p,
>  		     const struct sys_reg_desc *rd)
>  {
> -	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
> +	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
>  
>  	if (p->is_write)
>  		reg_to_dbg(vcpu, p, rd, dbg_reg);
>  	else
>  		dbg_to_reg(vcpu, p, rd, dbg_reg);
>  
> -	trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
> +	trace_trap_reg(__func__, rd->CRm, p->is_write, *dbg_reg);
>  
>  	return true;
>  }
> @@ -414,7 +414,7 @@ static bool trap_bvr(struct kvm_vcpu *vcpu,
>  static int set_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  		const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
>  
>  	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -424,7 +424,7 @@ static int set_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  	const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
>  
>  	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -434,21 +434,21 @@ static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static void reset_bvr(struct kvm_vcpu *vcpu,
>  		      const struct sys_reg_desc *rd)
>  {
> -	vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg] = rd->val;
> +	vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm] = rd->val;
>  }
>  
>  static bool trap_bcr(struct kvm_vcpu *vcpu,
>  		     struct sys_reg_params *p,
>  		     const struct sys_reg_desc *rd)
>  {
> -	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
> +	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
>  
>  	if (p->is_write)
>  		reg_to_dbg(vcpu, p, rd, dbg_reg);
>  	else
>  		dbg_to_reg(vcpu, p, rd, dbg_reg);
>  
> -	trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
> +	trace_trap_reg(__func__, rd->CRm, p->is_write, *dbg_reg);
>  
>  	return true;
>  }
> @@ -456,7 +456,7 @@ static bool trap_bcr(struct kvm_vcpu *vcpu,
>  static int set_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  		const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
>  
>  	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -467,7 +467,7 @@ static int set_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  	const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
>  
>  	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -477,22 +477,22 @@ static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static void reset_bcr(struct kvm_vcpu *vcpu,
>  		      const struct sys_reg_desc *rd)
>  {
> -	vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg] = rd->val;
> +	vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm] = rd->val;
>  }
>  
>  static bool trap_wvr(struct kvm_vcpu *vcpu,
>  		     struct sys_reg_params *p,
>  		     const struct sys_reg_desc *rd)
>  {
> -	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
> +	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
>  
>  	if (p->is_write)
>  		reg_to_dbg(vcpu, p, rd, dbg_reg);
>  	else
>  		dbg_to_reg(vcpu, p, rd, dbg_reg);
>  
> -	trace_trap_reg(__func__, rd->reg, p->is_write,
> -		vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg]);
> +	trace_trap_reg(__func__, rd->CRm, p->is_write,
> +		vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm]);
>  
>  	return true;
>  }
> @@ -500,7 +500,7 @@ static bool trap_wvr(struct kvm_vcpu *vcpu,
>  static int set_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  		const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
>  
>  	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -510,7 +510,7 @@ static int set_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  	const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
>  
>  	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -520,21 +520,21 @@ static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static void reset_wvr(struct kvm_vcpu *vcpu,
>  		      const struct sys_reg_desc *rd)
>  {
> -	vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg] = rd->val;
> +	vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm] = rd->val;
>  }
>  
>  static bool trap_wcr(struct kvm_vcpu *vcpu,
>  		     struct sys_reg_params *p,
>  		     const struct sys_reg_desc *rd)
>  {
> -	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
> +	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
>  
>  	if (p->is_write)
>  		reg_to_dbg(vcpu, p, rd, dbg_reg);
>  	else
>  		dbg_to_reg(vcpu, p, rd, dbg_reg);
>  
> -	trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
> +	trace_trap_reg(__func__, rd->CRm, p->is_write, *dbg_reg);
>  
>  	return true;
>  }
> @@ -542,7 +542,7 @@ static bool trap_wcr(struct kvm_vcpu *vcpu,
>  static int set_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  		const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
>  
>  	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -552,7 +552,7 @@ static int set_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  	const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
>  
>  	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -562,7 +562,7 @@ static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static void reset_wcr(struct kvm_vcpu *vcpu,
>  		      const struct sys_reg_desc *rd)
>  {
> -	vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg] = rd->val;
> +	vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm] = rd->val;
>  }
>  
>  static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> -- 
> 2.30.2
> 
> 
> -- 
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

* Re: [PATCH] KVM: arm64: Add missing index for trapping debug registers
@ 2021-05-14 17:53     ` Ricardo Koller
  0 siblings, 0 replies; 6+ messages in thread
From: Ricardo Koller @ 2021-05-14 17:53 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: kvmarm, james.morse, alexandru.elisei, drjones, suzuki.poulose,
	linux-arm-kernel

On Fri, May 14, 2021 at 09:19:29AM +0100, Marc Zyngier wrote:
> Hi Ricardo,
> 
> On Fri, 14 May 2021 02:49:06 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > Trapping an access to debug register <n> (like bvr<n>, bcr<n>, wvr<n>,
> > wcr<n>) results in storing and loading values from the vcpu copy at
> > index 0 (irrespective of <n>). So, this guest test fails:
> > 
> >   /* traps and wrongly stores 0x123 into vcpu->bvr[0] */
> >   write_sysreg(dbgbvr1_el1, 0x123);
> >   /* reads 0 from the real bvr[1] without trapping */
> >   GUEST_ASSERT(read_sysreg(dbgbvr1_el1) == 0x123); /* check fails */
> 
> Bummer... But nice catch! I broke it in 03fdfb2690099 ("KVM: arm64:
> Don't write junk to sysregs on reset").
> 
> > Fix this by setting the register index in macro DBG_BCR_BVR_WCR_WVR_EL1
> > to <n>.
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 76ea2800c33e..e4ec9edd49fa 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -935,13 +935,13 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >  /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
> >  #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
> >  	{ SYS_DESC(SYS_DBGBVRn_EL1(n)),					\
> > -	  trap_bvr, reset_bvr, 0, 0, get_bvr, set_bvr },		\
> > +	  trap_bvr, reset_bvr, n, 0, get_bvr, set_bvr },		\
> >  	{ SYS_DESC(SYS_DBGBCRn_EL1(n)),					\
> > -	  trap_bcr, reset_bcr, 0, 0, get_bcr, set_bcr },		\
> > +	  trap_bcr, reset_bcr, n, 0, get_bcr, set_bcr },		\
> >  	{ SYS_DESC(SYS_DBGWVRn_EL1(n)),					\
> > -	  trap_wvr, reset_wvr, 0, 0,  get_wvr, set_wvr },		\
> > +	  trap_wvr, reset_wvr, n, 0,  get_wvr, set_wvr },		\
> >  	{ SYS_DESC(SYS_DBGWCRn_EL1(n)),					\
> > -	  trap_wcr, reset_wcr, 0, 0,  get_wcr, set_wcr }
> > +	  trap_wcr, reset_wcr, n, 0,  get_wcr, set_wcr }
> >  
> >  #define PMU_SYS_REG(r)						\
> >  	SYS_DESC(r), .reset = reset_unknown, .visibility = pmu_visibility
> 
> Unfortunately, I don't think that's the right fix either, and just
> setting it to the debug register index will do the wrong thing in
> reset_sys_reg_descs().
> 

I see, got it.

> The reason is that the debug registers don't live in the per-vcpu
> sysreg array, but instead in some private structure, owing to the fact
> that we support both guest and host-side debugging. The value '0' here
> is in indication that the shadow registers are "somewhere else".
> 
> I think the correct fix is to use the register encoding itself, which
> contains everything we need (for all the debug regs, the CRm field is
> the debug register index). That's what it should have been the first
> place.
> 
> Could you please give the following patch a go?
> 

I just tried it and it fixes the test described in the commit message:
wr(bvr1),rd(bvr1).

Thanks,
Ricardo

> Thanks,
> 
> 	M.
> 
> From cfca75f1e2ab5dd1933de59b90f31293821fd2de Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Fri, 14 May 2021 09:05:41 +0100
> Subject: [PATCH] KVM: arm64: Fix debug register indexing
> 
> Commit 03fdfb2690099 ("KVM: arm64: Don't write junk to sysregs on
> reset") flipped the register number to 0 for all the debug registers
> in the sysreg table, hereby indicating that these registers live
> in a separate shadow structure.
> 
> However, the author of this patch failed to realise that all the
> accessors are using that particular index instead of the register
> encoding, resulting in all the registers hitting index 0. Not quite
> a valid implementation of the architecture...
> 
> Address the issue by fixing all the accessors to use the CRm field
> of the encoding, which contains the debug register index.
> 
> Fixes: 03fdfb2690099 ("KVM: arm64: Don't write junk to sysregs on reset")
> Reported-by: Ricardo Koller <ricarkol@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  arch/arm64/kvm/sys_regs.c | 42 +++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 76ea2800c33e..1a7968ad078c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -399,14 +399,14 @@ static bool trap_bvr(struct kvm_vcpu *vcpu,
>  		     struct sys_reg_params *p,
>  		     const struct sys_reg_desc *rd)
>  {
> -	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
> +	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
>  
>  	if (p->is_write)
>  		reg_to_dbg(vcpu, p, rd, dbg_reg);
>  	else
>  		dbg_to_reg(vcpu, p, rd, dbg_reg);
>  
> -	trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
> +	trace_trap_reg(__func__, rd->CRm, p->is_write, *dbg_reg);
>  
>  	return true;
>  }
> @@ -414,7 +414,7 @@ static bool trap_bvr(struct kvm_vcpu *vcpu,
>  static int set_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  		const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
>  
>  	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -424,7 +424,7 @@ static int set_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  	const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
>  
>  	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -434,21 +434,21 @@ static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static void reset_bvr(struct kvm_vcpu *vcpu,
>  		      const struct sys_reg_desc *rd)
>  {
> -	vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg] = rd->val;
> +	vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm] = rd->val;
>  }
>  
>  static bool trap_bcr(struct kvm_vcpu *vcpu,
>  		     struct sys_reg_params *p,
>  		     const struct sys_reg_desc *rd)
>  {
> -	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
> +	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
>  
>  	if (p->is_write)
>  		reg_to_dbg(vcpu, p, rd, dbg_reg);
>  	else
>  		dbg_to_reg(vcpu, p, rd, dbg_reg);
>  
> -	trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
> +	trace_trap_reg(__func__, rd->CRm, p->is_write, *dbg_reg);
>  
>  	return true;
>  }
> @@ -456,7 +456,7 @@ static bool trap_bcr(struct kvm_vcpu *vcpu,
>  static int set_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  		const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
>  
>  	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -467,7 +467,7 @@ static int set_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  	const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
>  
>  	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -477,22 +477,22 @@ static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static void reset_bcr(struct kvm_vcpu *vcpu,
>  		      const struct sys_reg_desc *rd)
>  {
> -	vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg] = rd->val;
> +	vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm] = rd->val;
>  }
>  
>  static bool trap_wvr(struct kvm_vcpu *vcpu,
>  		     struct sys_reg_params *p,
>  		     const struct sys_reg_desc *rd)
>  {
> -	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
> +	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
>  
>  	if (p->is_write)
>  		reg_to_dbg(vcpu, p, rd, dbg_reg);
>  	else
>  		dbg_to_reg(vcpu, p, rd, dbg_reg);
>  
> -	trace_trap_reg(__func__, rd->reg, p->is_write,
> -		vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg]);
> +	trace_trap_reg(__func__, rd->CRm, p->is_write,
> +		vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm]);
>  
>  	return true;
>  }
> @@ -500,7 +500,7 @@ static bool trap_wvr(struct kvm_vcpu *vcpu,
>  static int set_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  		const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
>  
>  	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -510,7 +510,7 @@ static int set_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  	const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
>  
>  	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -520,21 +520,21 @@ static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static void reset_wvr(struct kvm_vcpu *vcpu,
>  		      const struct sys_reg_desc *rd)
>  {
> -	vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg] = rd->val;
> +	vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm] = rd->val;
>  }
>  
>  static bool trap_wcr(struct kvm_vcpu *vcpu,
>  		     struct sys_reg_params *p,
>  		     const struct sys_reg_desc *rd)
>  {
> -	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
> +	u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
>  
>  	if (p->is_write)
>  		reg_to_dbg(vcpu, p, rd, dbg_reg);
>  	else
>  		dbg_to_reg(vcpu, p, rd, dbg_reg);
>  
> -	trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
> +	trace_trap_reg(__func__, rd->CRm, p->is_write, *dbg_reg);
>  
>  	return true;
>  }
> @@ -542,7 +542,7 @@ static bool trap_wcr(struct kvm_vcpu *vcpu,
>  static int set_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  		const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
>  
>  	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -552,7 +552,7 @@ static int set_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  	const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
> +	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
>  
>  	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
>  		return -EFAULT;
> @@ -562,7 +562,7 @@ static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  static void reset_wcr(struct kvm_vcpu *vcpu,
>  		      const struct sys_reg_desc *rd)
>  {
> -	vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg] = rd->val;
> +	vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm] = rd->val;
>  }
>  
>  static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> -- 
> 2.30.2
> 
> 
> -- 
> Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

end of thread, other threads:[~2021-05-14 17:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-14  1:49 [PATCH] KVM: arm64: Add missing index for trapping debug registers Ricardo Koller
2021-05-14  1:49 ` Ricardo Koller
2021-05-14  8:19 ` Marc Zyngier
2021-05-14  8:19   ` Marc Zyngier
2021-05-14 17:53   ` Ricardo Koller
2021-05-14 17:53     ` Ricardo Koller

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.