Message ID | 1442501171-24484-1-git-send-email-marc.zyngier@arm.com |
---|---|
State | New |
Headers | show |
On 17/09/2015 16:46, Marc Zyngier wrote: > Hi Paolo, > > We've had a "nice" collection of fixes trickling in this week, and > since both Christoffer and I are away next week, I've decided to send > everything your way a bit early. Sure, thanks. I'll send the pull request to Linus tomorrow. Paolo Fairly random stuff to be honnest, > but a negative diffstat can't be that bad! :-) > > Thanks, > > M. > > The following changes since commit 0c0672922dcc70ffba11d96385e98e42fb3ae08d: > > arm/arm64: KVM: Fix PSCI affinity info return value for non valid cores (2015-09-04 17:02:48 +0100) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git tags/kvm-arm-for-4.3-rc2-2 > > for you to fetch changes up to ef748917b529847277f07c98c55e1c0ce416449f: > > arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS' (2015-09-17 13:13:27 +0100) > > ---------------------------------------------------------------- > Second set of KVM/ARM changes for 4.3-rc2 > > - Workaround for a Cortex-A57 erratum > - Bug fix for the debugging infrastructure > - Fix for 32bit guests with more than 4GB of address space > on a 32bit host > - A number of fixes for the (unusual) case when we don't use > the in-kernel GIC emulation > - Removal of ThumbEE handling on arm64, since these have been > dropped from the architecture before anyone actually ever > built a CPU > - Remove the KVM_ARM_MAX_VCPUS limitation which has become > fairly pointless > > ---------------------------------------------------------------- > Marc Zyngier (3): > arm64: KVM: Fix user access for debug registers > arm64: KVM: Disable virtual timer even if the guest is not using it > arm: KVM: Disable virtual timer even if the guest is not using it > > Marek Majtyka (1): > arm: KVM: Fix incorrect device to IPA mapping > > Ming Lei (1): > arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS' > > Pavel Fedin (1): > arm/arm64: KVM: vgic: Check for !irqchip_in_kernel() when mapping resources > > Will Deacon (2): > KVM: arm64: add workaround for Cortex-A57 erratum #852523 > arm64: KVM: Remove all traces of the ThumbEE registers > > arch/arm/include/asm/kvm_host.h | 8 ++------ > arch/arm/kvm/Kconfig | 11 ----------- > arch/arm/kvm/arm.c | 2 +- > arch/arm/kvm/interrupts_head.S | 6 ++++-- > arch/arm/kvm/mmu.c | 6 ++++-- > arch/arm64/include/asm/kvm_arm.h | 1 - > arch/arm64/include/asm/kvm_asm.h | 4 +--- > arch/arm64/include/asm/kvm_host.h | 8 ++------ > arch/arm64/kvm/Kconfig | 11 ----------- > arch/arm64/kvm/hyp.S | 31 ++++++++++--------------------- > arch/arm64/kvm/sys_regs.c | 15 ++++----------- > include/kvm/arm_vgic.h | 6 +----- > virt/kvm/arm/vgic-v3.c | 2 +- > 13 files changed, 30 insertions(+), 81 deletions(-) >
On 17/09/15 16:02, Paolo Bonzini wrote: > > > On 17/09/2015 16:46, Marc Zyngier wrote: >> When running a guest with the architected timer disabled (with QEMU and >> the kernel_irqchip=off option, for example), it is important to make >> sure the timer gets turned off. Otherwise, the guest may try to >> enable it anyway, leading to a screaming HW interrupt. >> >> The fix is to unconditionally turn off the virtual timer on guest >> exit. >> >> Cc: stable@vger.kernel.org >> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> arch/arm64/kvm/hyp.S | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S >> index 39aa322..60a83e2 100644 >> --- a/arch/arm64/kvm/hyp.S >> +++ b/arch/arm64/kvm/hyp.S >> @@ -562,8 +562,6 @@ >> mrs x3, cntv_ctl_el0 >> and x3, x3, #3 >> str w3, [x0, #VCPU_TIMER_CNTV_CTL] >> - bic x3, x3, #1 // Clear Enable >> - msr cntv_ctl_el0, x3 >> >> isb >> >> @@ -571,6 +569,9 @@ >> str x3, [x0, #VCPU_TIMER_CNTV_CVAL] >> >> 1: >> + // Disable the virtual timer >> + msr cntv_ctl_el0, xzr >> + >> // Allow physical timer/counter access for the host >> mrs x2, cnthctl_el2 >> orr x2, x2, #3 >> > > It looks like here in restore_timer_state: > > ldr w2, [x0, #VCPU_TIMER_CNTV_CTL] > and x2, x2, #3 > msr cntv_ctl_el0, x2 > > the "and" would be unnecessary if kvm_arm_timer_set_reg remembered to > do it. Something like this, which would also make the code more > consistent between arm and arm64... > > diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S > index 702740d37465..93e322b4d242 100644 > --- a/arch/arm/kvm/interrupts_head.S > +++ b/arch/arm/kvm/interrupts_head.S > @@ -514,6 +514,7 @@ ARM_BE8(rev r6, r6 ) > beq 1f > > mrc p15, 0, r2, c14, c3, 1 @ CNTV_CTL > + and r2, r2, #3 I don't think we need this. Exposing the ISTATUS bit to the kernel (or even userspace) is not really a problem (that's actually an interesting piece of information), and restoring it is not possible since it is read-only. We should drop the equivalent 'and' from the arm64 version. > str r2, [vcpu, #VCPU_TIMER_CNTV_CTL] > bic r2, #1 @ Clear ENABLE > mcr p15, 0, r2, c14, c3, 1 @ CNTV_CTL > @@ -566,7 +567,6 @@ ARM_BE8(rev r6, r6 ) > isb > > ldr r2, [vcpu, #VCPU_TIMER_CNTV_CTL] > - and r2, r2, #3 > mcr p15, 0, r2, c14, c3, 1 @ CNTV_CTL > 1: > .endm > diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S > index 10915aaf0b01..bfcd3f3a947b 100644 > --- a/arch/arm64/kvm/hyp.S > +++ b/arch/arm64/kvm/hyp.S > @@ -887,7 +887,6 @@ alternative_endif > isb > > ldr w2, [x0, #VCPU_TIMER_CNTV_CTL] > - and x2, x2, #3 > msr cntv_ctl_el0, x2 > 1: > .endm > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > index 98c95f2fcba4..9b03c9f5abbf 100644 > --- a/virt/kvm/arm/arch_timer.c > +++ b/virt/kvm/arm/arch_timer.c > @@ -218,7 +218,7 @@ int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value) > > switch (regid) { > case KVM_REG_ARM_TIMER_CTL: > - timer->cntv_ctl = value; > + timer->cntv_ctl = value & (ARCH_TIMER_CTRL_IT_MASK | ARCH_TIMER_CTRL_ENABLE); > break; > case KVM_REG_ARM_TIMER_CNT: > vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - value; > Otherwise looks pretty good. Can you send an updated patch? Thanks, M.
On 17/09/2015 17:28, Marc Zyngier wrote: > > diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S > > index 702740d37465..93e322b4d242 100644 > > --- a/arch/arm/kvm/interrupts_head.S > > +++ b/arch/arm/kvm/interrupts_head.S > > @@ -514,6 +514,7 @@ ARM_BE8(rev r6, r6 ) > > beq 1f > > > > mrc p15, 0, r2, c14, c3, 1 @ CNTV_CTL > > + and r2, r2, #3 > > I don't think we need this. Exposing the ISTATUS bit to the kernel (or > even userspace) is not really a problem (that's actually an interesting > piece of information), and restoring it is not possible since it is > read-only. > > We should drop the equivalent 'and' from the arm64 version. Ok. I'll resend the thing as a proper patch. Paolo