| Message ID | 20200130132558.10201-1-maz@kernel.org |
|---|---|
| State | New |
| Headers | show |
Hi, On 1/30/20 1:25 PM, Marc Zyngier wrote: > From: Alexandru Elisei <alexandru.elisei@arm.com> > > According to the ARM ARM, registers CNT{P,V}_TVAL_EL0 have bits [63:32] > RES0 [1]. When reading the register, the value is truncated to the least > significant 32 bits [2], and on writes, TimerValue is treated as a signed > 32-bit integer [1, 2]. > > When the guest behaves correctly and writes 32-bit values, treating TVAL > as an unsigned 64 bit register works as expected. However, things start > to break down when the guest writes larger values, because > (u64)0x1_ffff_ffff = 8589934591. but (s32)0x1_ffff_ffff = -1, and the > former will cause the timer interrupt to be asserted in the future, but > the latter will cause it to be asserted now. Let's treat TVAL as a > signed 32-bit register on writes, to match the behaviour described in > the architecture, and the behaviour experimentally exhibited by the > virtual timer on a non-vhe host. > > [1] Arm DDI 0487E.a, section D13.8.18 > [2] Arm DDI 0487E.a, section D11.2.4 > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > [maz: replaced the read-side mask with lower_32_bits] > Signed-off-by: Marc Zyngier <maz@kernel.org> > Fixes: 8fa761624871 ("KVM: arm/arm64: arch_timer: Fix CNTP_TVAL calculation") > Link: https://lore.kernel.org/r/20200127103652.2326-1-alexandru.elisei@arm.com > --- > virt/kvm/arm/arch_timer.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > index f182b2380345..c6c2a9dde00c 100644 > --- a/virt/kvm/arm/arch_timer.c > +++ b/virt/kvm/arm/arch_timer.c > @@ -805,6 +805,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu, > switch (treg) { > case TIMER_REG_TVAL: > val = timer->cnt_cval - kvm_phys_timer_read() + timer->cntvoff; > + val &= lower_32_bits(val); This is correct, but how about making it val = lower_32_bits(val) for more clarity? Apologies for not spotting it earlier :( Thanks, Alex > break; > > case TIMER_REG_CTL: > @@ -850,7 +851,7 @@ static void kvm_arm_timer_write(struct kvm_vcpu *vcpu, > { > switch (treg) { > case TIMER_REG_TVAL: > - timer->cnt_cval = kvm_phys_timer_read() - timer->cntvoff + val; > + timer->cnt_cval = kvm_phys_timer_read() - timer->cntvoff + (s32)val; > break; > > case TIMER_REG_CTL:
On 2020-01-30 14:11, Alexandru Elisei wrote: > Hi, > > On 1/30/20 1:25 PM, Marc Zyngier wrote: >> From: Alexandru Elisei <alexandru.elisei@arm.com> >> >> According to the ARM ARM, registers CNT{P,V}_TVAL_EL0 have bits >> [63:32] >> RES0 [1]. When reading the register, the value is truncated to the >> least >> significant 32 bits [2], and on writes, TimerValue is treated as a >> signed >> 32-bit integer [1, 2]. >> >> When the guest behaves correctly and writes 32-bit values, treating >> TVAL >> as an unsigned 64 bit register works as expected. However, things >> start >> to break down when the guest writes larger values, because >> (u64)0x1_ffff_ffff = 8589934591. but (s32)0x1_ffff_ffff = -1, and the >> former will cause the timer interrupt to be asserted in the future, >> but >> the latter will cause it to be asserted now. Let's treat TVAL as a >> signed 32-bit register on writes, to match the behaviour described in >> the architecture, and the behaviour experimentally exhibited by the >> virtual timer on a non-vhe host. >> >> [1] Arm DDI 0487E.a, section D13.8.18 >> [2] Arm DDI 0487E.a, section D11.2.4 >> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >> [maz: replaced the read-side mask with lower_32_bits] >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> Fixes: 8fa761624871 ("KVM: arm/arm64: arch_timer: Fix CNTP_TVAL >> calculation") >> Link: >> https://lore.kernel.org/r/20200127103652.2326-1-alexandru.elisei@arm.com >> --- >> virt/kvm/arm/arch_timer.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c >> index f182b2380345..c6c2a9dde00c 100644 >> --- a/virt/kvm/arm/arch_timer.c >> +++ b/virt/kvm/arm/arch_timer.c >> @@ -805,6 +805,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu >> *vcpu, >> switch (treg) { >> case TIMER_REG_TVAL: >> val = timer->cnt_cval - kvm_phys_timer_read() + timer->cntvoff; >> + val &= lower_32_bits(val); > > This is correct, but how about making it val = lower_32_bits(val) for > more > clarity? Apologies for not spotting it earlier :( That's what it should have been, but I obviously typoed it. As it passed all my tests, I didn't notice the issue. I'll queue a cleanup once Paolo has a chance to pull this. M.
On 30/01/20 14:25, Marc Zyngier wrote:
> git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git tags/kvmarm-5.6
Pulled, thanks!
Paolo
Hi Paolo, Please find below the pull request for KVM/arm for Linux 5.6. No new feature this time around, but a large spectrum of interesting fixes. I'm certainly pleased that people are focussing on these corner cases, as some of them are pretty hard to debug. So kudos to them! Please pull, M. The following changes since commit fd6988496e79a6a4bdb514a4655d2920209eb85d: Linux 5.5-rc4 (2019-12-29 15:29:16 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git tags/kvmarm-5.6 for you to fetch changes up to 4a267aa707953a9a73d1f5dc7f894dd9024a92be: KVM: arm64: Treat emulated TVAL TimerValue as a signed 32-bit integer (2020-01-28 13:09:31 +0000) ---------------------------------------------------------------- KVM/arm updates for Linux 5.6 - Fix MMIO sign extension - Fix HYP VA tagging on tag space exhaustion - Fix PSTATE/CPSR handling when generating exception - Fix MMU notifier's advertizing of young pages - Fix poisoned page handling - Fix PMU SW event handling - Fix TVAL register access - Fix AArch32 external abort injection - Fix ITS unmapped collection handling - Various cleanups ---------------------------------------------------------------- Alexandru Elisei (1): KVM: arm64: Treat emulated TVAL TimerValue as a signed 32-bit integer Andrew Jones (1): arm64: KVM: Add UAPI notes for swapped registers Christoffer Dall (1): KVM: arm64: Only sign-extend MMIO up to register width Eric Auger (5): KVM: arm/arm64: vgic-its: Fix restoration of unmapped collections KVM: arm64: pmu: Don't increment SW_INCR if PMCR.E is unset KVM: arm64: pmu: Don't mark a counter as chained if the odd one is disabled KVM: arm64: pmu: Fix chained SW_INCR counters KVM: arm64: pmu: Only handle supported event counters Gavin Shan (1): KVM: arm/arm64: Fix young bit from mmu notifier James Morse (3): KVM: arm/arm64: Re-check VMA on detecting a poisoned page KVM: arm: Fix DFSR setting for non-LPAE aarch32 guests KVM: arm: Make inject_abt32() inject an external abort instead Marc Zyngier (1): KVM: arm/arm64: Cleanup MMIO handling Mark Brown (1): arm64: KVM: Annotate guest entry/exit as a single function Mark Rutland (3): KVM: arm64: Correct PSTATE on exception entry KVM: arm/arm64: Correct CPSR on exception entry KVM: arm/arm64: Correct AArch32 SPSR on exception entry Russell King (1): arm64: kvm: Fix IDMAP overlap with HYP VA Shannon Zhao (1): KVM: ARM: Call hyp_cpu_pm_exit at the right place YueHaibing (1): KVM: arm: Remove duplicate include Zenghui Yu (3): KVM: arm/arm64: vgic: Handle GICR_PENDBASER.PTZ filed as RAZ KVM: arm/arm64: vgic-its: Properly check the unmapped coll in DISCARD handler KVM: arm/arm64: vgic: Drop the kvm_vgic_register_mmio_region() Documentation/virt/kvm/api.txt | 9 +++ arch/arm/include/asm/kvm_emulate.h | 27 +++++++- arch/arm/include/asm/kvm_host.h | 12 ++-- arch/arm/include/asm/kvm_hyp.h | 1 + arch/arm/include/asm/kvm_mmio.h | 26 ------- arch/arm64/include/asm/kvm_emulate.h | 40 ++++++++++- arch/arm64/include/asm/kvm_host.h | 12 ++-- arch/arm64/include/asm/kvm_mmio.h | 29 -------- arch/arm64/include/asm/ptrace.h | 1 + arch/arm64/include/uapi/asm/kvm.h | 12 +++- arch/arm64/include/uapi/asm/ptrace.h | 1 + arch/arm64/kvm/hyp/entry.S | 7 +- arch/arm64/kvm/inject_fault.c | 70 +++++++++++++++++-- arch/arm64/kvm/va_layout.c | 56 +++++++-------- virt/kvm/arm/aarch32.c | 131 ++++++++++++++++++++++++++++++----- virt/kvm/arm/arch_timer.c | 3 +- virt/kvm/arm/arm.c | 4 +- virt/kvm/arm/mmio.c | 68 +++++++----------- virt/kvm/arm/mmu.c | 24 +++---- virt/kvm/arm/pmu.c | 114 ++++++++++++++++++------------ virt/kvm/arm/vgic/vgic-its.c | 6 +- virt/kvm/arm/vgic/vgic-mmio-v3.c | 5 +- virt/kvm/arm/vgic/vgic-mmio.h | 5 -- 23 files changed, 421 insertions(+), 242 deletions(-) delete mode 100644 arch/arm/include/asm/kvm_mmio.h delete mode 100644 arch/arm64/include/asm/kvm_mmio.h