Messages in this thread Patch in this message | | | Subject | Re: [PATCH] KVM: arm64: Use the correct timer for accessing CNT | From | James Morse <> | Date | Tue, 17 Mar 2020 13:59:38 +0000 |
| |
Hi KarimAllah,
On 3/16/20 9:39 AM, KarimAllah Ahmed wrote: > Use the physical timer object when reading the physical timer counter > instead of using the virtual timer object. This is only visible when > reading it from user-space as kvm_arm_timer_get_reg() is only executed on > the get register patch from user-space.
Have you seen this go wrong?
I agree this looks like this was a typo introduced by: 84135d3d1 ("KVM: arm/arm64: consolidate arch timer trap handlers") -----------------%<----------------- case KVM_REG_ARM_PTIMER_CNT: - return kvm_phys_timer_read(); + return kvm_arm_timer_read(vcpu, + vcpu_vtimer(vcpu), TIMER_REG_CNT); -----------------%<-----------------
This would be a problem when the guest reads the physical counter directly, (which doesn't get trapped), and the VMM makes this API call and gets a number in a totally different ball-park.
Can the VMM actually read these registers with this path?
kvm_arm_get_reg() gets to filter out the coproc registers that aren't in the sys_reg[], it also uses is_timer_reg() to spot the timer/counter registers, but is_timer_reg() only matches three of them: | case KVM_REG_ARM_TIMER_CTL: | case KVM_REG_ARM_TIMER_CNT: | case KVM_REG_ARM_TIMER_CVAL:
KVM_REG_ARM_PTIMER_CNT is not one of them.
It looks like when the VMM tries to read this, it fails is_timer_reg(), and matches in the sys_regs[] and is handled by access_arch_timer(), which uses kvm_arm_timer_read_sysreg() -> kvm_arm_timer_read(), bypassing this bug.
... this looks like a bug in dead code ...
Thanks!
James
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > index 0d9438e..93bd59b 100644 > --- a/virt/kvm/arm/arch_timer.c > +++ b/virt/kvm/arm/arch_timer.c > @@ -788,7 +788,7 @@ u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid) > vcpu_ptimer(vcpu), TIMER_REG_CTL); > case KVM_REG_ARM_PTIMER_CNT: > return kvm_arm_timer_read(vcpu, > - vcpu_vtimer(vcpu), TIMER_REG_CNT); > + vcpu_ptimer(vcpu), TIMER_REG_CNT); > case KVM_REG_ARM_PTIMER_CVAL: > return kvm_arm_timer_read(vcpu, > vcpu_ptimer(vcpu), TIMER_REG_CVAL); >
| |