Re: [PATCH] KVM: arm/arm64: Don't enable/disable physical timer access on VHE

[Jintack Lim]

On Mon, Nov 20, 2017 at 6:16 AM, Christoffer Dall
 wrote:
> After the timer optimization rework we accidentally end up calling
> physical timer enable/disable functions on VHE systems, which is neither
> needed nor correct, since the CNTHCTL_EL2 register format is
> different when HCR_EL2.E2H is set.
>
> The CNTHCTL_EL2 is initialized when CPUs become online in
> kvm_timer_init_vhe() and we don't have to call these functions on VHE
> systems, which also allows us to inline the non-VHE functionality.
>
> Reported-by: Jintack Lim 
> Signed-off-by: Christoffer Dall 

Reviewed-by: Jintack Lim 

Thanks,
Jintack

> ---
>  include/kvm/arm_arch_timer.h |  3 ---
>  virt/kvm/arm/arch_timer.c|  6 --
>  virt/kvm/arm/hyp/timer-sr.c  | 48 
> ++--
>  3 files changed, 20 insertions(+), 37 deletions(-)
>
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index 01ee473517e2..6e45608b2399 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -93,7 +93,4 @@ void kvm_timer_init_vhe(void);
>  #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer)
>  #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer)
>
> -void enable_el1_phys_timer_access(void);
> -void disable_el1_phys_timer_access(void);
> -
>  #endif
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 4151250ce8da..190c99ed1b73 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -479,9 +479,6 @@ void kvm_timer_vcpu_load(struct kvm_vcpu *vcpu)
>
> vtimer_restore_state(vcpu);
>
> -   if (has_vhe())
> -   disable_el1_phys_timer_access();
> -
> /* Set the background timer for the physical timer emulation. */
> phys_timer_emulate(vcpu);
>  }
> @@ -510,9 +507,6 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
> if (unlikely(!timer->enabled))
> return;
>
> -   if (has_vhe())
> -   enable_el1_phys_timer_access();
> -
> vtimer_save_state(vcpu);
>
> /*
> diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
> index f39861639f08..f24404b3c8df 100644
> --- a/virt/kvm/arm/hyp/timer-sr.c
> +++ b/virt/kvm/arm/hyp/timer-sr.c
> @@ -27,42 +27,34 @@ void __hyp_text __kvm_timer_set_cntvoff(u32 cntvoff_low, 
> u32 cntvoff_high)
> write_sysreg(cntvoff, cntvoff_el2);
>  }
>
> -void __hyp_text enable_el1_phys_timer_access(void)
> -{
> -   u64 val;
> -
> -   /* Allow physical timer/counter access for the host */
> -   val = read_sysreg(cnthctl_el2);
> -   val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
> -   write_sysreg(val, cnthctl_el2);
> -}
> -
> -void __hyp_text disable_el1_phys_timer_access(void)
> -{
> -   u64 val;
> -
> -   /*
> -* Disallow physical timer access for the guest
> -* Physical counter access is allowed
> -*/
> -   val = read_sysreg(cnthctl_el2);
> -   val &= ~CNTHCTL_EL1PCEN;
> -   val |= CNTHCTL_EL1PCTEN;
> -   write_sysreg(val, cnthctl_el2);
> -}
> -
>  void __hyp_text __timer_disable_traps(struct kvm_vcpu *vcpu)
>  {
> /*
>  * We don't need to do this for VHE since the host kernel runs in EL2
>  * with HCR_EL2.TGE ==1, which makes those bits have no impact.
>  */
> -   if (!has_vhe())
> -   enable_el1_phys_timer_access();
> +   if (!has_vhe()) {
> +   u64 val;
> +
> +   /* Allow physical timer/counter access for the host */
> +   val = read_sysreg(cnthctl_el2);
> +   val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
> +   write_sysreg(val, cnthctl_el2);
> +   }
>  }
>
>  void __hyp_text __timer_enable_traps(struct kvm_vcpu *vcpu)
>  {
> -   if (!has_vhe())
> -   disable_el1_phys_timer_access();
> +   if (!has_vhe()) {
> +   u64 val;
> +
> +   /*
> +* Disallow physical timer access for the guest
> +* Physical counter access is allowed
> +*/
> +   val = read_sysreg(cnthctl_el2);
> +   val &= ~CNTHCTL_EL1PCEN;
> +   val |= CNTHCTL_EL1PCTEN;
> +   write_sysreg(val, cnthctl_el2);
> +   }
>  }
> --
> 2.14.2
>
>

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH] KVM: arm/arm64: Don't enable/disable physical timer access on VHE

[Christoffer Dall]

After the timer optimization rework we accidentally end up calling
physical timer enable/disable functions on VHE systems, which is neither
needed nor correct, since the CNTHCTL_EL2 register format is
different when HCR_EL2.E2H is set.

The CNTHCTL_EL2 is initialized when CPUs become online in
kvm_timer_init_vhe() and we don't have to call these functions on VHE
systems, which also allows us to inline the non-VHE functionality.

Reported-by: Jintack Lim 
Signed-off-by: Christoffer Dall 
---
 include/kvm/arm_arch_timer.h |  3 ---
 virt/kvm/arm/arch_timer.c|  6 --
 virt/kvm/arm/hyp/timer-sr.c  | 48 ++--
 3 files changed, 20 insertions(+), 37 deletions(-)

diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index 01ee473517e2..6e45608b2399 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -93,7 +93,4 @@ void kvm_timer_init_vhe(void);
 #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer)
 #define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer)
 
-void enable_el1_phys_timer_access(void);
-void disable_el1_phys_timer_access(void);
-
 #endif
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 4151250ce8da..190c99ed1b73 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -479,9 +479,6 @@ void kvm_timer_vcpu_load(struct kvm_vcpu *vcpu)
 
vtimer_restore_state(vcpu);
 
-   if (has_vhe())
-   disable_el1_phys_timer_access();
-
/* Set the background timer for the physical timer emulation. */
phys_timer_emulate(vcpu);
 }
@@ -510,9 +507,6 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
if (unlikely(!timer->enabled))
return;
 
-   if (has_vhe())
-   enable_el1_phys_timer_access();
-
vtimer_save_state(vcpu);
 
/*
diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
index f39861639f08..f24404b3c8df 100644
--- a/virt/kvm/arm/hyp/timer-sr.c
+++ b/virt/kvm/arm/hyp/timer-sr.c
@@ -27,42 +27,34 @@ void __hyp_text __kvm_timer_set_cntvoff(u32 cntvoff_low, 
u32 cntvoff_high)
write_sysreg(cntvoff, cntvoff_el2);
 }
 
-void __hyp_text enable_el1_phys_timer_access(void)
-{
-   u64 val;
-
-   /* Allow physical timer/counter access for the host */
-   val = read_sysreg(cnthctl_el2);
-   val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
-   write_sysreg(val, cnthctl_el2);
-}
-
-void __hyp_text disable_el1_phys_timer_access(void)
-{
-   u64 val;
-
-   /*
-* Disallow physical timer access for the guest
-* Physical counter access is allowed
-*/
-   val = read_sysreg(cnthctl_el2);
-   val &= ~CNTHCTL_EL1PCEN;
-   val |= CNTHCTL_EL1PCTEN;
-   write_sysreg(val, cnthctl_el2);
-}
-
 void __hyp_text __timer_disable_traps(struct kvm_vcpu *vcpu)
 {
/*
 * We don't need to do this for VHE since the host kernel runs in EL2
 * with HCR_EL2.TGE ==1, which makes those bits have no impact.
 */
-   if (!has_vhe())
-   enable_el1_phys_timer_access();
+   if (!has_vhe()) {
+   u64 val;
+
+   /* Allow physical timer/counter access for the host */
+   val = read_sysreg(cnthctl_el2);
+   val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
+   write_sysreg(val, cnthctl_el2);
+   }
 }
 
 void __hyp_text __timer_enable_traps(struct kvm_vcpu *vcpu)
 {
-   if (!has_vhe())
-   disable_el1_phys_timer_access();
+   if (!has_vhe()) {
+   u64 val;
+
+   /*
+* Disallow physical timer access for the guest
+* Physical counter access is allowed
+*/
+   val = read_sysreg(cnthctl_el2);
+   val &= ~CNTHCTL_EL1PCEN;
+   val |= CNTHCTL_EL1PCTEN;
+   write_sysreg(val, cnthctl_el2);
+   }
 }
-- 
2.14.2

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm