linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Ricardo Koller <ricarkol@google.com>
To: Marc Zyngier <maz@kernel.org>
Cc: kvmarm@lists.cs.columbia.edu, james.morse@arm.com,
	alexandru.elisei@arm.com, drjones@redhat.com,
	suzuki.poulose@arm.com, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] KVM: arm64: Add missing index for trapping debug registers
Date: Fri, 14 May 2021 10:53:03 -0700	[thread overview]
Message-ID: <YJ64/5AQz0ISl0oX@google.com> (raw)
In-Reply-To: <87bl9dohgu.wl-maz@kernel.org>

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

      reply	other threads:[~2021-05-14 17:55 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-14  1:49 [PATCH] KVM: arm64: Add missing index for trapping debug registers Ricardo Koller
2021-05-14  8:19 ` Marc Zyngier
2021-05-14 17:53   ` Ricardo Koller [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YJ64/5AQz0ISl0oX@google.com \
    --to=ricarkol@google.com \
    --cc=alexandru.elisei@arm.com \
    --cc=drjones@redhat.com \
    --cc=james.morse@arm.com \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=maz@kernel.org \
    --cc=suzuki.poulose@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).