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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
