[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Jintack Lim]

Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
are 11th and 10th bits respectively when E2H is set.  Current code is
unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
may allow guest OS to access physical timer. So, fix it.

Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
---
 arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
 arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
 include/clocksource/arm_arch_timer.h |  6 ++--
 virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
 4 files changed, 103 insertions(+), 6 deletions(-)
 create mode 100644 arch/arm/include/asm/kvm_timer.h
 create mode 100644 arch/arm64/include/asm/kvm_timer.h

diff --git a/arch/arm/include/asm/kvm_timer.h b/arch/arm/include/asm/kvm_timer.h
new file mode 100644
index 0000000..d19d4b3
--- /dev/null
+++ b/arch/arm/include/asm/kvm_timer.h
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2016 - Columbia University
+ * Author: Jintack Lim <jintack@cs.columbia.edu>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __ARM_KVM_TIMER_H__
+#define __ARM_KVM_TIMER_H__
+
+#include <clocksource/arm_arch_timer.h>
+
+static inline u32 __hyp_text get_el1pcten(void)
+{
+	return CNTHCTL_EL1PCTEN_NVHE;
+}
+
+static inline u32 __hyp_text get_el1pcen(void)
+{
+	return CNTHCTL_EL1PCEN_NVHE;
+}
+
+#endif	/* __ARM_KVM_TIMER_H__ */
diff --git a/arch/arm64/include/asm/kvm_timer.h b/arch/arm64/include/asm/kvm_timer.h
new file mode 100644
index 0000000..153f3da
--- /dev/null
+++ b/arch/arm64/include/asm/kvm_timer.h
@@ -0,0 +1,62 @@
+/*
+ * Copyright (C) 2016 - Columbia University
+ * Author: Jintack Lim <jintack@cs.columbia.edu>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __ARM64_KVM_TIMER_H__
+#define __ARM64_KVM_TIMER_H__
+
+#include <asm/kvm_hyp.h>
+#include <clocksource/arm_arch_timer.h>
+
+static inline u32 __hyp_text get_el1pcten_vhe(void)
+{
+	return CNTHCTL_EL1PCTEN_VHE;
+}
+
+static inline u32 __hyp_text get_el1pcten_nvhe(void)
+{
+	return CNTHCTL_EL1PCTEN_NVHE;
+}
+
+static hyp_alternate_select(get_el1pcten_arch,
+			    get_el1pcten_nvhe, get_el1pcten_vhe,
+			    ARM64_HAS_VIRT_HOST_EXTN);
+
+static inline u32 __hyp_text get_el1pten_vhe(void)
+{
+	return CNTHCTL_EL1PTEN_VHE;
+}
+
+static inline u32 __hyp_text get_el1pcen_nvhe(void)
+{
+	return CNTHCTL_EL1PCEN_NVHE;
+}
+
+static hyp_alternate_select(get_el1pcen_arch,
+			    get_el1pcen_nvhe, get_el1pten_vhe,
+			    ARM64_HAS_VIRT_HOST_EXTN);
+
+static inline u32 __hyp_text get_el1pcten(void)
+{
+	return get_el1pcten_arch()();
+}
+
+static inline u32 __hyp_text get_el1pcen(void)
+{
+	return get_el1pcen_arch()();
+}
+
+#endif	/* __ARM64_KVM_TIMER_H__ */
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index caedb74..4094529 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -23,8 +23,10 @@
 #define ARCH_TIMER_CTRL_IT_MASK		(1 << 1)
 #define ARCH_TIMER_CTRL_IT_STAT		(1 << 2)
 
-#define CNTHCTL_EL1PCTEN		(1 << 0)
-#define CNTHCTL_EL1PCEN			(1 << 1)
+#define CNTHCTL_EL1PCTEN_NVHE		(1 << 0)
+#define CNTHCTL_EL1PCEN_NVHE		(1 << 1)
+#define CNTHCTL_EL1PCTEN_VHE		(1 << 10)
+#define CNTHCTL_EL1PTEN_VHE		(1 << 11)
 #define CNTHCTL_EVNTEN			(1 << 2)
 #define CNTHCTL_EVNTDIR			(1 << 3)
 #define CNTHCTL_EVNTI			(0xF << 4)
diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
index 798866a..f3feee0 100644
--- a/virt/kvm/arm/hyp/timer-sr.c
+++ b/virt/kvm/arm/hyp/timer-sr.c
@@ -15,11 +15,11 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <clocksource/arm_arch_timer.h>
 #include <linux/compiler.h>
 #include <linux/kvm_host.h>
 
 #include <asm/kvm_hyp.h>
+#include <asm/kvm_timer.h>
 
 /* vcpu is already in the HYP VA space */
 void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
@@ -37,7 +37,7 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
 
 	/* Allow physical timer/counter access for the host */
 	val = read_sysreg(cnthctl_el2);
-	val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
+	val |= get_el1pcten() | get_el1pcen();
 	write_sysreg(val, cnthctl_el2);
 
 	/* Clear cntvoff for the host */
@@ -55,8 +55,8 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
 	 * Physical counter access is allowed
 	 */
 	val = read_sysreg(cnthctl_el2);
-	val &= ~CNTHCTL_EL1PCEN;
-	val |= CNTHCTL_EL1PCTEN;
+	val &= ~get_el1pcen();
+	val |= get_el1pcten();
 	write_sysreg(val, cnthctl_el2);
 
 	if (timer->enabled) {
-- 
1.9.1

[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Jintack Lim]

Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
are 11th and 10th bits respectively when E2H is set.  Current code is
unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
may allow guest OS to access physical timer. So, fix it.

Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
---
 arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
 arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
 include/clocksource/arm_arch_timer.h |  6 ++--
 virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
 4 files changed, 103 insertions(+), 6 deletions(-)
 create mode 100644 arch/arm/include/asm/kvm_timer.h
 create mode 100644 arch/arm64/include/asm/kvm_timer.h

diff --git a/arch/arm/include/asm/kvm_timer.h b/arch/arm/include/asm/kvm_timer.h
new file mode 100644
index 0000000..d19d4b3
--- /dev/null
+++ b/arch/arm/include/asm/kvm_timer.h
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2016 - Columbia University
+ * Author: Jintack Lim <jintack@cs.columbia.edu>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __ARM_KVM_TIMER_H__
+#define __ARM_KVM_TIMER_H__
+
+#include <clocksource/arm_arch_timer.h>
+
+static inline u32 __hyp_text get_el1pcten(void)
+{
+	return CNTHCTL_EL1PCTEN_NVHE;
+}
+
+static inline u32 __hyp_text get_el1pcen(void)
+{
+	return CNTHCTL_EL1PCEN_NVHE;
+}
+
+#endif	/* __ARM_KVM_TIMER_H__ */
diff --git a/arch/arm64/include/asm/kvm_timer.h b/arch/arm64/include/asm/kvm_timer.h
new file mode 100644
index 0000000..153f3da
--- /dev/null
+++ b/arch/arm64/include/asm/kvm_timer.h
@@ -0,0 +1,62 @@
+/*
+ * Copyright (C) 2016 - Columbia University
+ * Author: Jintack Lim <jintack@cs.columbia.edu>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __ARM64_KVM_TIMER_H__
+#define __ARM64_KVM_TIMER_H__
+
+#include <asm/kvm_hyp.h>
+#include <clocksource/arm_arch_timer.h>
+
+static inline u32 __hyp_text get_el1pcten_vhe(void)
+{
+	return CNTHCTL_EL1PCTEN_VHE;
+}
+
+static inline u32 __hyp_text get_el1pcten_nvhe(void)
+{
+	return CNTHCTL_EL1PCTEN_NVHE;
+}
+
+static hyp_alternate_select(get_el1pcten_arch,
+			    get_el1pcten_nvhe, get_el1pcten_vhe,
+			    ARM64_HAS_VIRT_HOST_EXTN);
+
+static inline u32 __hyp_text get_el1pten_vhe(void)
+{
+	return CNTHCTL_EL1PTEN_VHE;
+}
+
+static inline u32 __hyp_text get_el1pcen_nvhe(void)
+{
+	return CNTHCTL_EL1PCEN_NVHE;
+}
+
+static hyp_alternate_select(get_el1pcen_arch,
+			    get_el1pcen_nvhe, get_el1pten_vhe,
+			    ARM64_HAS_VIRT_HOST_EXTN);
+
+static inline u32 __hyp_text get_el1pcten(void)
+{
+	return get_el1pcten_arch()();
+}
+
+static inline u32 __hyp_text get_el1pcen(void)
+{
+	return get_el1pcen_arch()();
+}
+
+#endif	/* __ARM64_KVM_TIMER_H__ */
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index caedb74..4094529 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -23,8 +23,10 @@
 #define ARCH_TIMER_CTRL_IT_MASK		(1 << 1)
 #define ARCH_TIMER_CTRL_IT_STAT		(1 << 2)
 
-#define CNTHCTL_EL1PCTEN		(1 << 0)
-#define CNTHCTL_EL1PCEN			(1 << 1)
+#define CNTHCTL_EL1PCTEN_NVHE		(1 << 0)
+#define CNTHCTL_EL1PCEN_NVHE		(1 << 1)
+#define CNTHCTL_EL1PCTEN_VHE		(1 << 10)
+#define CNTHCTL_EL1PTEN_VHE		(1 << 11)
 #define CNTHCTL_EVNTEN			(1 << 2)
 #define CNTHCTL_EVNTDIR			(1 << 3)
 #define CNTHCTL_EVNTI			(0xF << 4)
diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
index 798866a..f3feee0 100644
--- a/virt/kvm/arm/hyp/timer-sr.c
+++ b/virt/kvm/arm/hyp/timer-sr.c
@@ -15,11 +15,11 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <clocksource/arm_arch_timer.h>
 #include <linux/compiler.h>
 #include <linux/kvm_host.h>
 
 #include <asm/kvm_hyp.h>
+#include <asm/kvm_timer.h>
 
 /* vcpu is already in the HYP VA space */
 void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
@@ -37,7 +37,7 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
 
 	/* Allow physical timer/counter access for the host */
 	val = read_sysreg(cnthctl_el2);
-	val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
+	val |= get_el1pcten() | get_el1pcen();
 	write_sysreg(val, cnthctl_el2);
 
 	/* Clear cntvoff for the host */
@@ -55,8 +55,8 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
 	 * Physical counter access is allowed
 	 */
 	val = read_sysreg(cnthctl_el2);
-	val &= ~CNTHCTL_EL1PCEN;
-	val |= CNTHCTL_EL1PCTEN;
+	val &= ~get_el1pcen();
+	val |= get_el1pcten();
 	write_sysreg(val, cnthctl_el2);
 
 	if (timer->enabled) {
-- 
1.9.1

Re: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Marc Zyngier]

Hi Jintack,

On 28/11/16 16:46, Jintack Lim wrote:
> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
> are 11th and 10th bits respectively when E2H is set.  Current code is
> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
> may allow guest OS to access physical timer. So, fix it.
> 
> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
> ---
>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>  include/clocksource/arm_arch_timer.h |  6 ++--
>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>  4 files changed, 103 insertions(+), 6 deletions(-)
>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
> 
> diff --git a/arch/arm/include/asm/kvm_timer.h b/arch/arm/include/asm/kvm_timer.h
> new file mode 100644
> index 0000000..d19d4b3
> --- /dev/null
> +++ b/arch/arm/include/asm/kvm_timer.h
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) 2016 - Columbia University
> + * Author: Jintack Lim <jintack@cs.columbia.edu>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __ARM_KVM_TIMER_H__
> +#define __ARM_KVM_TIMER_H__
> +
> +#include <clocksource/arm_arch_timer.h>
> +
> +static inline u32 __hyp_text get_el1pcten(void)
> +{
> +	return CNTHCTL_EL1PCTEN_NVHE;
> +}
> +
> +static inline u32 __hyp_text get_el1pcen(void)
> +{
> +	return CNTHCTL_EL1PCEN_NVHE;
> +}
> +
> +#endif	/* __ARM_KVM_TIMER_H__ */
> diff --git a/arch/arm64/include/asm/kvm_timer.h b/arch/arm64/include/asm/kvm_timer.h
> new file mode 100644
> index 0000000..153f3da
> --- /dev/null
> +++ b/arch/arm64/include/asm/kvm_timer.h
> @@ -0,0 +1,62 @@
> +/*
> + * Copyright (C) 2016 - Columbia University
> + * Author: Jintack Lim <jintack@cs.columbia.edu>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __ARM64_KVM_TIMER_H__
> +#define __ARM64_KVM_TIMER_H__
> +
> +#include <asm/kvm_hyp.h>
> +#include <clocksource/arm_arch_timer.h>
> +
> +static inline u32 __hyp_text get_el1pcten_vhe(void)
> +{
> +	return CNTHCTL_EL1PCTEN_VHE;
> +}
> +
> +static inline u32 __hyp_text get_el1pcten_nvhe(void)
> +{
> +	return CNTHCTL_EL1PCTEN_NVHE;
> +}
> +
> +static hyp_alternate_select(get_el1pcten_arch,
> +			    get_el1pcten_nvhe, get_el1pcten_vhe,
> +			    ARM64_HAS_VIRT_HOST_EXTN);

This is pretty horrid ;-)

First, the inline qualifier doesn't apply here, as the whole 
hyp_alternate_select hack relies on thing not being inlined (it 
actually creates a function pointer).

Then, this gets potentially instantiated in any compilation unit that 
will include this file. GCC is probably clever enough to eliminate it 
if not used, but not without a well deserved warning.

> +
> +static inline u32 __hyp_text get_el1pten_vhe(void)
> +{
> +	return CNTHCTL_EL1PTEN_VHE;
> +}
> +
> +static inline u32 __hyp_text get_el1pcen_nvhe(void)
> +{
> +	return CNTHCTL_EL1PCEN_NVHE;
> +}
> +
> +static hyp_alternate_select(get_el1pcen_arch,
> +			    get_el1pcen_nvhe, get_el1pten_vhe,
> +			    ARM64_HAS_VIRT_HOST_EXTN);
> +
> +static inline u32 __hyp_text get_el1pcten(void)
> +{
> +	return get_el1pcten_arch()();
> +}
> +
> +static inline u32 __hyp_text get_el1pcen(void)
> +{
> +	return get_el1pcen_arch()();
> +}
> +
> +#endif	/* __ARM64_KVM_TIMER_H__ */
> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index caedb74..4094529 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -23,8 +23,10 @@
>  #define ARCH_TIMER_CTRL_IT_MASK		(1 << 1)
>  #define ARCH_TIMER_CTRL_IT_STAT		(1 << 2)
>  
> -#define CNTHCTL_EL1PCTEN		(1 << 0)
> -#define CNTHCTL_EL1PCEN			(1 << 1)
> +#define CNTHCTL_EL1PCTEN_NVHE		(1 << 0)
> +#define CNTHCTL_EL1PCEN_NVHE		(1 << 1)
> +#define CNTHCTL_EL1PCTEN_VHE		(1 << 10)
> +#define CNTHCTL_EL1PTEN_VHE		(1 << 11)
>  #define CNTHCTL_EVNTEN			(1 << 2)
>  #define CNTHCTL_EVNTDIR			(1 << 3)
>  #define CNTHCTL_EVNTI			(0xF << 4)
> diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
> index 798866a..f3feee0 100644
> --- a/virt/kvm/arm/hyp/timer-sr.c
> +++ b/virt/kvm/arm/hyp/timer-sr.c
> @@ -15,11 +15,11 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#include <clocksource/arm_arch_timer.h>
>  #include <linux/compiler.h>
>  #include <linux/kvm_host.h>
>  
>  #include <asm/kvm_hyp.h>
> +#include <asm/kvm_timer.h>
>  
>  /* vcpu is already in the HYP VA space */
>  void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
> @@ -37,7 +37,7 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
>  
>  	/* Allow physical timer/counter access for the host */
>  	val = read_sysreg(cnthctl_el2);
> -	val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
> +	val |= get_el1pcten() | get_el1pcen();
>  	write_sysreg(val, cnthctl_el2);
>  
>  	/* Clear cntvoff for the host */
> @@ -55,8 +55,8 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
>  	 * Physical counter access is allowed
>  	 */
>  	val = read_sysreg(cnthctl_el2);
> -	val &= ~CNTHCTL_EL1PCEN;
> -	val |= CNTHCTL_EL1PCTEN;
> +	val &= ~get_el1pcen();
> +	val |= get_el1pcten();
>  	write_sysreg(val, cnthctl_el2);
>  
>  	if (timer->enabled) {
> 

Trying to solve this myself, I came up with an alternative approach,
which is ugly in its own way (#define in common code, random shifts):

diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
index 798866a..5cacfa8 100644
--- a/virt/kvm/arm/hyp/timer-sr.c
+++ b/virt/kvm/arm/hyp/timer-sr.c
@@ -21,11 +21,41 @@
 
 #include <asm/kvm_hyp.h>
 
+#ifdef CONFIG_ARM64
+static unsigned int __hyp_text get_cnthclt_shift_nvhe(void)
+{
+	return 0;
+}
+
+static unsigned int __hyp_text get_cnthclt_shift_vhe(void)
+{
+	return 10;
+}
+
+static hyp_alternate_select(__get_cnthclt_shift,
+			    get_cnthclt_shift_nvhe, get_cnthclt_shift_vhe,
+			    ARM64_HAS_VIRT_HOST_EXTN)
+
+#define cnthclt_shift()		__get_cnthclt_shift()()
+#else
+#define cnthclt_shift()		(0)
+#endif
+
+static inline void __hyp_text cnthctl_rmw(u32 clr, u32 set)
+{
+	u32 val;
+	int shift = cnthclt_shift();
+
+	val = read_sysreg(cnthctl_el2);
+	val &= ~clr << shift;
+	val |= set << shift;
+	write_sysreg(val, cnthctl_el2);
+}
+
 /* vcpu is already in the HYP VA space */
 void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
-	u64 val;
 
 	if (timer->enabled) {
 		timer->cntv_ctl = read_sysreg_el0(cntv_ctl);
@@ -36,9 +66,7 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
 	write_sysreg_el0(0, cntv_ctl);
 
 	/* Allow physical timer/counter access for the host */
-	val = read_sysreg(cnthctl_el2);
-	val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
-	write_sysreg(val, cnthctl_el2);
+	cnthctl_rmw(0, CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN);
 
 	/* Clear cntvoff for the host */
 	write_sysreg(0, cntvoff_el2);
@@ -48,16 +76,12 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
 {
 	struct kvm *kvm = kern_hyp_va(vcpu->kvm);
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
-	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);
+	cnthctl_rmw(CNTHCTL_EL1PCEN, CNTHCTL_EL1PCTEN);
 
 	if (timer->enabled) {
 		write_sysreg(kvm->arch.timer.cntvoff, cntvoff_el2);

We could make it nicer (read "faster") by introducing a
hyp_alternate_select construct that only returns a value instead
of calling a function. I remember writing something like that
at some point, and dropping it...

Thoughts?

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Marc Zyngier]

Hi Jintack,

On 28/11/16 16:46, Jintack Lim wrote:
> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
> are 11th and 10th bits respectively when E2H is set.  Current code is
> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
> may allow guest OS to access physical timer. So, fix it.
> 
> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
> ---
>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>  include/clocksource/arm_arch_timer.h |  6 ++--
>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>  4 files changed, 103 insertions(+), 6 deletions(-)
>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
> 
> diff --git a/arch/arm/include/asm/kvm_timer.h b/arch/arm/include/asm/kvm_timer.h
> new file mode 100644
> index 0000000..d19d4b3
> --- /dev/null
> +++ b/arch/arm/include/asm/kvm_timer.h
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) 2016 - Columbia University
> + * Author: Jintack Lim <jintack@cs.columbia.edu>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __ARM_KVM_TIMER_H__
> +#define __ARM_KVM_TIMER_H__
> +
> +#include <clocksource/arm_arch_timer.h>
> +
> +static inline u32 __hyp_text get_el1pcten(void)
> +{
> +	return CNTHCTL_EL1PCTEN_NVHE;
> +}
> +
> +static inline u32 __hyp_text get_el1pcen(void)
> +{
> +	return CNTHCTL_EL1PCEN_NVHE;
> +}
> +
> +#endif	/* __ARM_KVM_TIMER_H__ */
> diff --git a/arch/arm64/include/asm/kvm_timer.h b/arch/arm64/include/asm/kvm_timer.h
> new file mode 100644
> index 0000000..153f3da
> --- /dev/null
> +++ b/arch/arm64/include/asm/kvm_timer.h
> @@ -0,0 +1,62 @@
> +/*
> + * Copyright (C) 2016 - Columbia University
> + * Author: Jintack Lim <jintack@cs.columbia.edu>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __ARM64_KVM_TIMER_H__
> +#define __ARM64_KVM_TIMER_H__
> +
> +#include <asm/kvm_hyp.h>
> +#include <clocksource/arm_arch_timer.h>
> +
> +static inline u32 __hyp_text get_el1pcten_vhe(void)
> +{
> +	return CNTHCTL_EL1PCTEN_VHE;
> +}
> +
> +static inline u32 __hyp_text get_el1pcten_nvhe(void)
> +{
> +	return CNTHCTL_EL1PCTEN_NVHE;
> +}
> +
> +static hyp_alternate_select(get_el1pcten_arch,
> +			    get_el1pcten_nvhe, get_el1pcten_vhe,
> +			    ARM64_HAS_VIRT_HOST_EXTN);

This is pretty horrid ;-)

First, the inline qualifier doesn't apply here, as the whole 
hyp_alternate_select hack relies on thing not being inlined (it 
actually creates a function pointer).

Then, this gets potentially instantiated in any compilation unit that 
will include this file. GCC is probably clever enough to eliminate it 
if not used, but not without a well deserved warning.

> +
> +static inline u32 __hyp_text get_el1pten_vhe(void)
> +{
> +	return CNTHCTL_EL1PTEN_VHE;
> +}
> +
> +static inline u32 __hyp_text get_el1pcen_nvhe(void)
> +{
> +	return CNTHCTL_EL1PCEN_NVHE;
> +}
> +
> +static hyp_alternate_select(get_el1pcen_arch,
> +			    get_el1pcen_nvhe, get_el1pten_vhe,
> +			    ARM64_HAS_VIRT_HOST_EXTN);
> +
> +static inline u32 __hyp_text get_el1pcten(void)
> +{
> +	return get_el1pcten_arch()();
> +}
> +
> +static inline u32 __hyp_text get_el1pcen(void)
> +{
> +	return get_el1pcen_arch()();
> +}
> +
> +#endif	/* __ARM64_KVM_TIMER_H__ */
> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index caedb74..4094529 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -23,8 +23,10 @@
>  #define ARCH_TIMER_CTRL_IT_MASK		(1 << 1)
>  #define ARCH_TIMER_CTRL_IT_STAT		(1 << 2)
>  
> -#define CNTHCTL_EL1PCTEN		(1 << 0)
> -#define CNTHCTL_EL1PCEN			(1 << 1)
> +#define CNTHCTL_EL1PCTEN_NVHE		(1 << 0)
> +#define CNTHCTL_EL1PCEN_NVHE		(1 << 1)
> +#define CNTHCTL_EL1PCTEN_VHE		(1 << 10)
> +#define CNTHCTL_EL1PTEN_VHE		(1 << 11)
>  #define CNTHCTL_EVNTEN			(1 << 2)
>  #define CNTHCTL_EVNTDIR			(1 << 3)
>  #define CNTHCTL_EVNTI			(0xF << 4)
> diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
> index 798866a..f3feee0 100644
> --- a/virt/kvm/arm/hyp/timer-sr.c
> +++ b/virt/kvm/arm/hyp/timer-sr.c
> @@ -15,11 +15,11 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#include <clocksource/arm_arch_timer.h>
>  #include <linux/compiler.h>
>  #include <linux/kvm_host.h>
>  
>  #include <asm/kvm_hyp.h>
> +#include <asm/kvm_timer.h>
>  
>  /* vcpu is already in the HYP VA space */
>  void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
> @@ -37,7 +37,7 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
>  
>  	/* Allow physical timer/counter access for the host */
>  	val = read_sysreg(cnthctl_el2);
> -	val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
> +	val |= get_el1pcten() | get_el1pcen();
>  	write_sysreg(val, cnthctl_el2);
>  
>  	/* Clear cntvoff for the host */
> @@ -55,8 +55,8 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
>  	 * Physical counter access is allowed
>  	 */
>  	val = read_sysreg(cnthctl_el2);
> -	val &= ~CNTHCTL_EL1PCEN;
> -	val |= CNTHCTL_EL1PCTEN;
> +	val &= ~get_el1pcen();
> +	val |= get_el1pcten();
>  	write_sysreg(val, cnthctl_el2);
>  
>  	if (timer->enabled) {
> 

Trying to solve this myself, I came up with an alternative approach,
which is ugly in its own way (#define in common code, random shifts):

diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
index 798866a..5cacfa8 100644
--- a/virt/kvm/arm/hyp/timer-sr.c
+++ b/virt/kvm/arm/hyp/timer-sr.c
@@ -21,11 +21,41 @@
 
 #include <asm/kvm_hyp.h>
 
+#ifdef CONFIG_ARM64
+static unsigned int __hyp_text get_cnthclt_shift_nvhe(void)
+{
+	return 0;
+}
+
+static unsigned int __hyp_text get_cnthclt_shift_vhe(void)
+{
+	return 10;
+}
+
+static hyp_alternate_select(__get_cnthclt_shift,
+			    get_cnthclt_shift_nvhe, get_cnthclt_shift_vhe,
+			    ARM64_HAS_VIRT_HOST_EXTN)
+
+#define cnthclt_shift()		__get_cnthclt_shift()()
+#else
+#define cnthclt_shift()		(0)
+#endif
+
+static inline void __hyp_text cnthctl_rmw(u32 clr, u32 set)
+{
+	u32 val;
+	int shift = cnthclt_shift();
+
+	val = read_sysreg(cnthctl_el2);
+	val &= ~clr << shift;
+	val |= set << shift;
+	write_sysreg(val, cnthctl_el2);
+}
+
 /* vcpu is already in the HYP VA space */
 void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
-	u64 val;
 
 	if (timer->enabled) {
 		timer->cntv_ctl = read_sysreg_el0(cntv_ctl);
@@ -36,9 +66,7 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
 	write_sysreg_el0(0, cntv_ctl);
 
 	/* Allow physical timer/counter access for the host */
-	val = read_sysreg(cnthctl_el2);
-	val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
-	write_sysreg(val, cnthctl_el2);
+	cnthctl_rmw(0, CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN);
 
 	/* Clear cntvoff for the host */
 	write_sysreg(0, cntvoff_el2);
@@ -48,16 +76,12 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
 {
 	struct kvm *kvm = kern_hyp_va(vcpu->kvm);
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
-	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);
+	cnthctl_rmw(CNTHCTL_EL1PCEN, CNTHCTL_EL1PCTEN);
 
 	if (timer->enabled) {
 		write_sysreg(kvm->arch.timer.cntvoff, cntvoff_el2);

We could make it nicer (read "faster") by introducing a
hyp_alternate_select construct that only returns a value instead
of calling a function. I remember writing something like that
at some point, and dropping it...

Thoughts?

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Marc Zyngier]

On 28/11/16 17:43, Marc Zyngier wrote:
> Hi Jintack,
> 
> On 28/11/16 16:46, Jintack Lim wrote:
>> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
>> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
>> are 11th and 10th bits respectively when E2H is set.  Current code is
>> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
>> may allow guest OS to access physical timer. So, fix it.
>>
>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
>> ---
>>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>>  include/clocksource/arm_arch_timer.h |  6 ++--
>>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>>  4 files changed, 103 insertions(+), 6 deletions(-)
>>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
>>

[...]

> We could make it nicer (read "faster") by introducing a
> hyp_alternate_select construct that only returns a value instead
> of calling a function. I remember writing something like that
> at some point, and dropping it...

So here's what this could look like (warning, wacky code ahead,
though I fixed a stupid bug that was present in the previous patch).
The generated code is quite nice (no branch, only an extra mov
instruction on the default path)... Of course, completely untested!

diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
index b18e852..d173902 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -121,6 +121,17 @@ typeof(orig) * __hyp_text fname(void)					\
 	return val;							\
 }
 
+#define hyp_alternate_select_const(fname, orig, alt, cond)		\
+inline const typeof(orig) __hyp_text fname(void)			\
+{									\
+	typeof(alt) val = orig;						\
+	asm volatile(ALTERNATIVE("nop		\n",			\
+				 "mov	%0, %1	\n",			\
+				 cond)					\
+		     : "+r" (val) : "r" (alt));				\
+	return val;							\
+}
+
 void __vgic_v2_save_state(struct kvm_vcpu *vcpu);
 void __vgic_v2_restore_state(struct kvm_vcpu *vcpu);
 int __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
index 798866a..7a783af 100644
--- a/virt/kvm/arm/hyp/timer-sr.c
+++ b/virt/kvm/arm/hyp/timer-sr.c
@@ -21,11 +21,29 @@
 
 #include <asm/kvm_hyp.h>
 
+#ifdef CONFIG_ARM64
+static hyp_alternate_select_const(cnthclt_shift, 0, 10,
+				  ARM64_HAS_VIRT_HOST_EXTN)
+
+#else
+#define cnthclt_shift()		(0)
+#endif
+
+static inline void __hyp_text cnthctl_rmw(u32 clr, u32 set)
+{
+	u32 val;
+	int shift = cnthclt_shift();
+
+	val = read_sysreg(cnthctl_el2);
+	val &= ~(clr << shift);
+	val |= set << shift;
+	write_sysreg(val, cnthctl_el2);
+}
+
 /* vcpu is already in the HYP VA space */
 void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
-	u64 val;
 
 	if (timer->enabled) {
 		timer->cntv_ctl = read_sysreg_el0(cntv_ctl);
@@ -36,9 +54,7 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
 	write_sysreg_el0(0, cntv_ctl);
 
 	/* Allow physical timer/counter access for the host */
-	val = read_sysreg(cnthctl_el2);
-	val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
-	write_sysreg(val, cnthctl_el2);
+	cnthctl_rmw(0, CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN);
 
 	/* Clear cntvoff for the host */
 	write_sysreg(0, cntvoff_el2);
@@ -48,16 +64,12 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
 {
 	struct kvm *kvm = kern_hyp_va(vcpu->kvm);
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
-	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);
+	cnthctl_rmw(CNTHCTL_EL1PCEN, CNTHCTL_EL1PCTEN);
 
 	if (timer->enabled) {
 		write_sysreg(kvm->arch.timer.cntvoff, cntvoff_el2);

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Marc Zyngier]

On 28/11/16 17:43, Marc Zyngier wrote:
> Hi Jintack,
> 
> On 28/11/16 16:46, Jintack Lim wrote:
>> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
>> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
>> are 11th and 10th bits respectively when E2H is set.  Current code is
>> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
>> may allow guest OS to access physical timer. So, fix it.
>>
>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
>> ---
>>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>>  include/clocksource/arm_arch_timer.h |  6 ++--
>>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>>  4 files changed, 103 insertions(+), 6 deletions(-)
>>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
>>

[...]

> We could make it nicer (read "faster") by introducing a
> hyp_alternate_select construct that only returns a value instead
> of calling a function. I remember writing something like that
> at some point, and dropping it...

So here's what this could look like (warning, wacky code ahead,
though I fixed a stupid bug that was present in the previous patch).
The generated code is quite nice (no branch, only an extra mov
instruction on the default path)... Of course, completely untested!

diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
index b18e852..d173902 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -121,6 +121,17 @@ typeof(orig) * __hyp_text fname(void)					\
 	return val;							\
 }
 
+#define hyp_alternate_select_const(fname, orig, alt, cond)		\
+inline const typeof(orig) __hyp_text fname(void)			\
+{									\
+	typeof(alt) val = orig;						\
+	asm volatile(ALTERNATIVE("nop		\n",			\
+				 "mov	%0, %1	\n",			\
+				 cond)					\
+		     : "+r" (val) : "r" (alt));				\
+	return val;							\
+}
+
 void __vgic_v2_save_state(struct kvm_vcpu *vcpu);
 void __vgic_v2_restore_state(struct kvm_vcpu *vcpu);
 int __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
index 798866a..7a783af 100644
--- a/virt/kvm/arm/hyp/timer-sr.c
+++ b/virt/kvm/arm/hyp/timer-sr.c
@@ -21,11 +21,29 @@
 
 #include <asm/kvm_hyp.h>
 
+#ifdef CONFIG_ARM64
+static hyp_alternate_select_const(cnthclt_shift, 0, 10,
+				  ARM64_HAS_VIRT_HOST_EXTN)
+
+#else
+#define cnthclt_shift()		(0)
+#endif
+
+static inline void __hyp_text cnthctl_rmw(u32 clr, u32 set)
+{
+	u32 val;
+	int shift = cnthclt_shift();
+
+	val = read_sysreg(cnthctl_el2);
+	val &= ~(clr << shift);
+	val |= set << shift;
+	write_sysreg(val, cnthctl_el2);
+}
+
 /* vcpu is already in the HYP VA space */
 void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
-	u64 val;
 
 	if (timer->enabled) {
 		timer->cntv_ctl = read_sysreg_el0(cntv_ctl);
@@ -36,9 +54,7 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
 	write_sysreg_el0(0, cntv_ctl);
 
 	/* Allow physical timer/counter access for the host */
-	val = read_sysreg(cnthctl_el2);
-	val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
-	write_sysreg(val, cnthctl_el2);
+	cnthctl_rmw(0, CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN);
 
 	/* Clear cntvoff for the host */
 	write_sysreg(0, cntvoff_el2);
@@ -48,16 +64,12 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
 {
 	struct kvm *kvm = kern_hyp_va(vcpu->kvm);
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
-	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);
+	cnthctl_rmw(CNTHCTL_EL1PCEN, CNTHCTL_EL1PCTEN);
 
 	if (timer->enabled) {
 		write_sysreg(kvm->arch.timer.cntvoff, cntvoff_el2);

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Christoffer Dall]

On Mon, Nov 28, 2016 at 06:39:04PM +0000, Marc Zyngier wrote:
> On 28/11/16 17:43, Marc Zyngier wrote:
> > Hi Jintack,
> > 
> > On 28/11/16 16:46, Jintack Lim wrote:
> >> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
> >> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
> >> are 11th and 10th bits respectively when E2H is set.  Current code is
> >> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
> >> may allow guest OS to access physical timer. So, fix it.
> >>
> >> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
> >> ---
> >>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
> >>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
> >>  include/clocksource/arm_arch_timer.h |  6 ++--
> >>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
> >>  4 files changed, 103 insertions(+), 6 deletions(-)
> >>  create mode 100644 arch/arm/include/asm/kvm_timer.h
> >>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
> >>
> 
> [...]
> 
> > We could make it nicer (read "faster") by introducing a
> > hyp_alternate_select construct that only returns a value instead
> > of calling a function. I remember writing something like that
> > at some point, and dropping it...
> 
> So here's what this could look like (warning, wacky code ahead,
> though I fixed a stupid bug that was present in the previous patch).
> The generated code is quite nice (no branch, only an extra mov
> instruction on the default path)... Of course, completely untested!

Isn't this all about determining which bitmask to use, statically, once,
after the system has booted?

How about a good old fashioned static variable, or global struct like
the global one we use for the VGIC, which sets the proper mit mask
during kvm init, and the world-switch code just uses a variable?

Thanks,
-Christoffer

[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Christoffer Dall]

On Mon, Nov 28, 2016 at 06:39:04PM +0000, Marc Zyngier wrote:
> On 28/11/16 17:43, Marc Zyngier wrote:
> > Hi Jintack,
> > 
> > On 28/11/16 16:46, Jintack Lim wrote:
> >> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
> >> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
> >> are 11th and 10th bits respectively when E2H is set.  Current code is
> >> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
> >> may allow guest OS to access physical timer. So, fix it.
> >>
> >> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
> >> ---
> >>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
> >>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
> >>  include/clocksource/arm_arch_timer.h |  6 ++--
> >>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
> >>  4 files changed, 103 insertions(+), 6 deletions(-)
> >>  create mode 100644 arch/arm/include/asm/kvm_timer.h
> >>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
> >>
> 
> [...]
> 
> > We could make it nicer (read "faster") by introducing a
> > hyp_alternate_select construct that only returns a value instead
> > of calling a function. I remember writing something like that
> > at some point, and dropping it...
> 
> So here's what this could look like (warning, wacky code ahead,
> though I fixed a stupid bug that was present in the previous patch).
> The generated code is quite nice (no branch, only an extra mov
> instruction on the default path)... Of course, completely untested!

Isn't this all about determining which bitmask to use, statically, once,
after the system has booted?

How about a good old fashioned static variable, or global struct like
the global one we use for the VGIC, which sets the proper mit mask
during kvm init, and the world-switch code just uses a variable?

Thanks,
-Christoffer

Re: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Jintack Lim]

On Mon, Nov 28, 2016 at 1:39 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 28/11/16 17:43, Marc Zyngier wrote:
>> Hi Jintack,

Hi Marc,

>>
>> On 28/11/16 16:46, Jintack Lim wrote:
>>> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
>>> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
>>> are 11th and 10th bits respectively when E2H is set.  Current code is
>>> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
>>> may allow guest OS to access physical timer. So, fix it.
>>>
>>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
>>> ---
>>>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>>>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>>>  include/clocksource/arm_arch_timer.h |  6 ++--
>>>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>>>  4 files changed, 103 insertions(+), 6 deletions(-)
>>>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>>>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
>>>
>
> [...]
>
>> We could make it nicer (read "faster") by introducing a
>> hyp_alternate_select construct that only returns a value instead
>> of calling a function. I remember writing something like that
>> at some point, and dropping it...
>
> So here's what this could look like (warning, wacky code ahead,
> though I fixed a stupid bug that was present in the previous patch).
> The generated code is quite nice (no branch, only an extra mov
> instruction on the default path)... Of course, completely untested!

This looks much cleaner than my patch.
While we are at it, is it worth to consider that we just need to set
those bits once for VHE case, not for every world switch as an
optimization?

>
> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> index b18e852..d173902 100644
> --- a/arch/arm64/include/asm/kvm_hyp.h
> +++ b/arch/arm64/include/asm/kvm_hyp.h
> @@ -121,6 +121,17 @@ typeof(orig) * __hyp_text fname(void)                                      \
>         return val;                                                     \
>  }
>
> +#define hyp_alternate_select_const(fname, orig, alt, cond)             \
> +inline const typeof(orig) __hyp_text fname(void)                       \
> +{                                                                      \
> +       typeof(alt) val = orig;                                         \
> +       asm volatile(ALTERNATIVE("nop           \n",                    \
> +                                "mov   %0, %1  \n",                    \
> +                                cond)                                  \
> +                    : "+r" (val) : "r" (alt));                         \
> +       return val;                                                     \
> +}
> +
>  void __vgic_v2_save_state(struct kvm_vcpu *vcpu);
>  void __vgic_v2_restore_state(struct kvm_vcpu *vcpu);
>  int __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu);
> diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
> index 798866a..7a783af 100644
> --- a/virt/kvm/arm/hyp/timer-sr.c
> +++ b/virt/kvm/arm/hyp/timer-sr.c
> @@ -21,11 +21,29 @@
>
>  #include <asm/kvm_hyp.h>
>
> +#ifdef CONFIG_ARM64
> +static hyp_alternate_select_const(cnthclt_shift, 0, 10,
> +                                 ARM64_HAS_VIRT_HOST_EXTN)
> +
> +#else
> +#define cnthclt_shift()                (0)
> +#endif
> +
> +static inline void __hyp_text cnthctl_rmw(u32 clr, u32 set)
> +{
> +       u32 val;
> +       int shift = cnthclt_shift();
> +
> +       val = read_sysreg(cnthctl_el2);
> +       val &= ~(clr << shift);
> +       val |= set << shift;
> +       write_sysreg(val, cnthctl_el2);
> +}
> +
>  /* vcpu is already in the HYP VA space */
>  void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
>  {
>         struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> -       u64 val;
>
>         if (timer->enabled) {
>                 timer->cntv_ctl = read_sysreg_el0(cntv_ctl);
> @@ -36,9 +54,7 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
>         write_sysreg_el0(0, cntv_ctl);
>
>         /* Allow physical timer/counter access for the host */
> -       val = read_sysreg(cnthctl_el2);
> -       val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
> -       write_sysreg(val, cnthctl_el2);
> +       cnthctl_rmw(0, CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN);
>
>         /* Clear cntvoff for the host */
>         write_sysreg(0, cntvoff_el2);
> @@ -48,16 +64,12 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
>  {
>         struct kvm *kvm = kern_hyp_va(vcpu->kvm);
>         struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> -       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);
> +       cnthctl_rmw(CNTHCTL_EL1PCEN, CNTHCTL_EL1PCTEN);
>
>         if (timer->enabled) {
>                 write_sysreg(kvm->arch.timer.cntvoff, cntvoff_el2);
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
>

[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Jintack Lim]

On Mon, Nov 28, 2016 at 1:39 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 28/11/16 17:43, Marc Zyngier wrote:
>> Hi Jintack,

Hi Marc,

>>
>> On 28/11/16 16:46, Jintack Lim wrote:
>>> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
>>> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
>>> are 11th and 10th bits respectively when E2H is set.  Current code is
>>> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
>>> may allow guest OS to access physical timer. So, fix it.
>>>
>>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
>>> ---
>>>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>>>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>>>  include/clocksource/arm_arch_timer.h |  6 ++--
>>>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>>>  4 files changed, 103 insertions(+), 6 deletions(-)
>>>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>>>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
>>>
>
> [...]
>
>> We could make it nicer (read "faster") by introducing a
>> hyp_alternate_select construct that only returns a value instead
>> of calling a function. I remember writing something like that
>> at some point, and dropping it...
>
> So here's what this could look like (warning, wacky code ahead,
> though I fixed a stupid bug that was present in the previous patch).
> The generated code is quite nice (no branch, only an extra mov
> instruction on the default path)... Of course, completely untested!

This looks much cleaner than my patch.
While we are at it, is it worth to consider that we just need to set
those bits once for VHE case, not for every world switch as an
optimization?

>
> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> index b18e852..d173902 100644
> --- a/arch/arm64/include/asm/kvm_hyp.h
> +++ b/arch/arm64/include/asm/kvm_hyp.h
> @@ -121,6 +121,17 @@ typeof(orig) * __hyp_text fname(void)                                      \
>         return val;                                                     \
>  }
>
> +#define hyp_alternate_select_const(fname, orig, alt, cond)             \
> +inline const typeof(orig) __hyp_text fname(void)                       \
> +{                                                                      \
> +       typeof(alt) val = orig;                                         \
> +       asm volatile(ALTERNATIVE("nop           \n",                    \
> +                                "mov   %0, %1  \n",                    \
> +                                cond)                                  \
> +                    : "+r" (val) : "r" (alt));                         \
> +       return val;                                                     \
> +}
> +
>  void __vgic_v2_save_state(struct kvm_vcpu *vcpu);
>  void __vgic_v2_restore_state(struct kvm_vcpu *vcpu);
>  int __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu);
> diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
> index 798866a..7a783af 100644
> --- a/virt/kvm/arm/hyp/timer-sr.c
> +++ b/virt/kvm/arm/hyp/timer-sr.c
> @@ -21,11 +21,29 @@
>
>  #include <asm/kvm_hyp.h>
>
> +#ifdef CONFIG_ARM64
> +static hyp_alternate_select_const(cnthclt_shift, 0, 10,
> +                                 ARM64_HAS_VIRT_HOST_EXTN)
> +
> +#else
> +#define cnthclt_shift()                (0)
> +#endif
> +
> +static inline void __hyp_text cnthctl_rmw(u32 clr, u32 set)
> +{
> +       u32 val;
> +       int shift = cnthclt_shift();
> +
> +       val = read_sysreg(cnthctl_el2);
> +       val &= ~(clr << shift);
> +       val |= set << shift;
> +       write_sysreg(val, cnthctl_el2);
> +}
> +
>  /* vcpu is already in the HYP VA space */
>  void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
>  {
>         struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> -       u64 val;
>
>         if (timer->enabled) {
>                 timer->cntv_ctl = read_sysreg_el0(cntv_ctl);
> @@ -36,9 +54,7 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
>         write_sysreg_el0(0, cntv_ctl);
>
>         /* Allow physical timer/counter access for the host */
> -       val = read_sysreg(cnthctl_el2);
> -       val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN;
> -       write_sysreg(val, cnthctl_el2);
> +       cnthctl_rmw(0, CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN);
>
>         /* Clear cntvoff for the host */
>         write_sysreg(0, cntvoff_el2);
> @@ -48,16 +64,12 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
>  {
>         struct kvm *kvm = kern_hyp_va(vcpu->kvm);
>         struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> -       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);
> +       cnthctl_rmw(CNTHCTL_EL1PCEN, CNTHCTL_EL1PCTEN);
>
>         if (timer->enabled) {
>                 write_sysreg(kvm->arch.timer.cntvoff, cntvoff_el2);
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
>

Re: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Marc Zyngier]

On 29/11/16 03:28, Jintack Lim wrote:
> On Mon, Nov 28, 2016 at 1:39 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 28/11/16 17:43, Marc Zyngier wrote:
>>> Hi Jintack,
> 
> Hi Marc,
> 
>>>
>>> On 28/11/16 16:46, Jintack Lim wrote:
>>>> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
>>>> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
>>>> are 11th and 10th bits respectively when E2H is set.  Current code is
>>>> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
>>>> may allow guest OS to access physical timer. So, fix it.
>>>>
>>>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
>>>> ---
>>>>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>>>>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>>>>  include/clocksource/arm_arch_timer.h |  6 ++--
>>>>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>>>>  4 files changed, 103 insertions(+), 6 deletions(-)
>>>>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>>>>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
>>>>
>>
>> [...]
>>
>>> We could make it nicer (read "faster") by introducing a
>>> hyp_alternate_select construct that only returns a value instead
>>> of calling a function. I remember writing something like that
>>> at some point, and dropping it...
>>
>> So here's what this could look like (warning, wacky code ahead,
>> though I fixed a stupid bug that was present in the previous patch).
>> The generated code is quite nice (no branch, only an extra mov
>> instruction on the default path)... Of course, completely untested!
> 
> This looks much cleaner than my patch.
> While we are at it, is it worth to consider that we just need to set
> those bits once for VHE case, not for every world switch as an
> optimization?

Ah! That's a much better idea indeed! And we could stop messing with
cntvoff_el2 as well, as it doesn't need to be restored to zero on exit.
Could you try and respin something along those lines?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Marc Zyngier]

On 29/11/16 03:28, Jintack Lim wrote:
> On Mon, Nov 28, 2016 at 1:39 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 28/11/16 17:43, Marc Zyngier wrote:
>>> Hi Jintack,
> 
> Hi Marc,
> 
>>>
>>> On 28/11/16 16:46, Jintack Lim wrote:
>>>> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
>>>> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
>>>> are 11th and 10th bits respectively when E2H is set.  Current code is
>>>> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
>>>> may allow guest OS to access physical timer. So, fix it.
>>>>
>>>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
>>>> ---
>>>>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>>>>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>>>>  include/clocksource/arm_arch_timer.h |  6 ++--
>>>>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>>>>  4 files changed, 103 insertions(+), 6 deletions(-)
>>>>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>>>>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
>>>>
>>
>> [...]
>>
>>> We could make it nicer (read "faster") by introducing a
>>> hyp_alternate_select construct that only returns a value instead
>>> of calling a function. I remember writing something like that
>>> at some point, and dropping it...
>>
>> So here's what this could look like (warning, wacky code ahead,
>> though I fixed a stupid bug that was present in the previous patch).
>> The generated code is quite nice (no branch, only an extra mov
>> instruction on the default path)... Of course, completely untested!
> 
> This looks much cleaner than my patch.
> While we are at it, is it worth to consider that we just need to set
> those bits once for VHE case, not for every world switch as an
> optimization?

Ah! That's a much better idea indeed! And we could stop messing with
cntvoff_el2 as well, as it doesn't need to be restored to zero on exit.
Could you try and respin something along those lines?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Marc Zyngier]

On 28/11/16 19:42, Christoffer Dall wrote:
> On Mon, Nov 28, 2016 at 06:39:04PM +0000, Marc Zyngier wrote:
>> On 28/11/16 17:43, Marc Zyngier wrote:
>>> Hi Jintack,
>>>
>>> On 28/11/16 16:46, Jintack Lim wrote:
>>>> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
>>>> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
>>>> are 11th and 10th bits respectively when E2H is set.  Current code is
>>>> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
>>>> may allow guest OS to access physical timer. So, fix it.
>>>>
>>>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
>>>> ---
>>>>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>>>>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>>>>  include/clocksource/arm_arch_timer.h |  6 ++--
>>>>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>>>>  4 files changed, 103 insertions(+), 6 deletions(-)
>>>>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>>>>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
>>>>
>>
>> [...]
>>
>>> We could make it nicer (read "faster") by introducing a
>>> hyp_alternate_select construct that only returns a value instead
>>> of calling a function. I remember writing something like that
>>> at some point, and dropping it...
>>
>> So here's what this could look like (warning, wacky code ahead,
>> though I fixed a stupid bug that was present in the previous patch).
>> The generated code is quite nice (no branch, only an extra mov
>> instruction on the default path)... Of course, completely untested!
> 
> Isn't this all about determining which bitmask to use, statically, once,
> after the system has booted?
> 
> How about a good old fashioned static variable, or global struct like
> the global one we use for the VGIC, which sets the proper mit mask
> during kvm init, and the world-switch code just uses a variable?

We could indeed do that (I've been carried away with my tendency for
weird and wonderful hacks).

But as Jintack mentioned, there is a much better approach, which is to
do nothing at all on the VHE path (we can set the permission bits once
and for all). cntvoff_el2 also falls into the same category of things we
should be able to only restore and not bother resetting (as it doesn't
affect the EL2 virtual counter).

Thoughts?

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Marc Zyngier]

On 28/11/16 19:42, Christoffer Dall wrote:
> On Mon, Nov 28, 2016 at 06:39:04PM +0000, Marc Zyngier wrote:
>> On 28/11/16 17:43, Marc Zyngier wrote:
>>> Hi Jintack,
>>>
>>> On 28/11/16 16:46, Jintack Lim wrote:
>>>> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
>>>> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
>>>> are 11th and 10th bits respectively when E2H is set.  Current code is
>>>> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
>>>> may allow guest OS to access physical timer. So, fix it.
>>>>
>>>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
>>>> ---
>>>>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>>>>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>>>>  include/clocksource/arm_arch_timer.h |  6 ++--
>>>>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>>>>  4 files changed, 103 insertions(+), 6 deletions(-)
>>>>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>>>>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
>>>>
>>
>> [...]
>>
>>> We could make it nicer (read "faster") by introducing a
>>> hyp_alternate_select construct that only returns a value instead
>>> of calling a function. I remember writing something like that
>>> at some point, and dropping it...
>>
>> So here's what this could look like (warning, wacky code ahead,
>> though I fixed a stupid bug that was present in the previous patch).
>> The generated code is quite nice (no branch, only an extra mov
>> instruction on the default path)... Of course, completely untested!
> 
> Isn't this all about determining which bitmask to use, statically, once,
> after the system has booted?
> 
> How about a good old fashioned static variable, or global struct like
> the global one we use for the VGIC, which sets the proper mit mask
> during kvm init, and the world-switch code just uses a variable?

We could indeed do that (I've been carried away with my tendency for
weird and wonderful hacks).

But as Jintack mentioned, there is a much better approach, which is to
do nothing at all on the VHE path (we can set the permission bits once
and for all). cntvoff_el2 also falls into the same category of things we
should be able to only restore and not bother resetting (as it doesn't
affect the EL2 virtual counter).

Thoughts?

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Christoffer Dall]

On Tue, Nov 29, 2016 at 09:37:07AM +0000, Marc Zyngier wrote:
> On 28/11/16 19:42, Christoffer Dall wrote:
> > On Mon, Nov 28, 2016 at 06:39:04PM +0000, Marc Zyngier wrote:
> >> On 28/11/16 17:43, Marc Zyngier wrote:
> >>> Hi Jintack,
> >>>
> >>> On 28/11/16 16:46, Jintack Lim wrote:
> >>>> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
> >>>> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
> >>>> are 11th and 10th bits respectively when E2H is set.  Current code is
> >>>> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
> >>>> may allow guest OS to access physical timer. So, fix it.
> >>>>
> >>>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
> >>>> ---
> >>>>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
> >>>>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
> >>>>  include/clocksource/arm_arch_timer.h |  6 ++--
> >>>>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
> >>>>  4 files changed, 103 insertions(+), 6 deletions(-)
> >>>>  create mode 100644 arch/arm/include/asm/kvm_timer.h
> >>>>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
> >>>>
> >>
> >> [...]
> >>
> >>> We could make it nicer (read "faster") by introducing a
> >>> hyp_alternate_select construct that only returns a value instead
> >>> of calling a function. I remember writing something like that
> >>> at some point, and dropping it...
> >>
> >> So here's what this could look like (warning, wacky code ahead,
> >> though I fixed a stupid bug that was present in the previous patch).
> >> The generated code is quite nice (no branch, only an extra mov
> >> instruction on the default path)... Of course, completely untested!
> > 
> > Isn't this all about determining which bitmask to use, statically, once,
> > after the system has booted?
> > 
> > How about a good old fashioned static variable, or global struct like
> > the global one we use for the VGIC, which sets the proper mit mask
> > during kvm init, and the world-switch code just uses a variable?
> 
> We could indeed do that (I've been carried away with my tendency for
> weird and wonderful hacks).
> 
> But as Jintack mentioned, there is a much better approach, which is to
> do nothing at all on the VHE path (we can set the permission bits once
> and for all). cntvoff_el2 also falls into the same category of things we
> should be able to only restore and not bother resetting (as it doesn't
> affect the EL2 virtual counter).
> 
> Thoughts?
> 
Yes, that sounds much better.

I have some patches to get rid of a lot of things, like cntvoff, during
the world-switch for VHE, so if Jintack just wants to focus on the
cnthctl I will catch cntvoff later.

-Christoffer

[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Christoffer Dall]

On Tue, Nov 29, 2016 at 09:37:07AM +0000, Marc Zyngier wrote:
> On 28/11/16 19:42, Christoffer Dall wrote:
> > On Mon, Nov 28, 2016 at 06:39:04PM +0000, Marc Zyngier wrote:
> >> On 28/11/16 17:43, Marc Zyngier wrote:
> >>> Hi Jintack,
> >>>
> >>> On 28/11/16 16:46, Jintack Lim wrote:
> >>>> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
> >>>> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
> >>>> are 11th and 10th bits respectively when E2H is set.  Current code is
> >>>> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
> >>>> may allow guest OS to access physical timer. So, fix it.
> >>>>
> >>>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
> >>>> ---
> >>>>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
> >>>>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
> >>>>  include/clocksource/arm_arch_timer.h |  6 ++--
> >>>>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
> >>>>  4 files changed, 103 insertions(+), 6 deletions(-)
> >>>>  create mode 100644 arch/arm/include/asm/kvm_timer.h
> >>>>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
> >>>>
> >>
> >> [...]
> >>
> >>> We could make it nicer (read "faster") by introducing a
> >>> hyp_alternate_select construct that only returns a value instead
> >>> of calling a function. I remember writing something like that
> >>> at some point, and dropping it...
> >>
> >> So here's what this could look like (warning, wacky code ahead,
> >> though I fixed a stupid bug that was present in the previous patch).
> >> The generated code is quite nice (no branch, only an extra mov
> >> instruction on the default path)... Of course, completely untested!
> > 
> > Isn't this all about determining which bitmask to use, statically, once,
> > after the system has booted?
> > 
> > How about a good old fashioned static variable, or global struct like
> > the global one we use for the VGIC, which sets the proper mit mask
> > during kvm init, and the world-switch code just uses a variable?
> 
> We could indeed do that (I've been carried away with my tendency for
> weird and wonderful hacks).
> 
> But as Jintack mentioned, there is a much better approach, which is to
> do nothing at all on the VHE path (we can set the permission bits once
> and for all). cntvoff_el2 also falls into the same category of things we
> should be able to only restore and not bother resetting (as it doesn't
> affect the EL2 virtual counter).
> 
> Thoughts?
> 
Yes, that sounds much better.

I have some patches to get rid of a lot of things, like cntvoff, during
the world-switch for VHE, so if Jintack just wants to focus on the
cnthctl I will catch cntvoff later.

-Christoffer

Re: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Marc Zyngier]

On 29/11/16 10:47, Christoffer Dall wrote:
> On Tue, Nov 29, 2016 at 09:37:07AM +0000, Marc Zyngier wrote:
>> On 28/11/16 19:42, Christoffer Dall wrote:
>>> On Mon, Nov 28, 2016 at 06:39:04PM +0000, Marc Zyngier wrote:
>>>> On 28/11/16 17:43, Marc Zyngier wrote:
>>>>> Hi Jintack,
>>>>>
>>>>> On 28/11/16 16:46, Jintack Lim wrote:
>>>>>> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
>>>>>> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
>>>>>> are 11th and 10th bits respectively when E2H is set.  Current code is
>>>>>> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
>>>>>> may allow guest OS to access physical timer. So, fix it.
>>>>>>
>>>>>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
>>>>>> ---
>>>>>>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>>>>>>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>>>>>>  include/clocksource/arm_arch_timer.h |  6 ++--
>>>>>>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>>>>>>  4 files changed, 103 insertions(+), 6 deletions(-)
>>>>>>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>>>>>>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
>>>>>>
>>>>
>>>> [...]
>>>>
>>>>> We could make it nicer (read "faster") by introducing a
>>>>> hyp_alternate_select construct that only returns a value instead
>>>>> of calling a function. I remember writing something like that
>>>>> at some point, and dropping it...
>>>>
>>>> So here's what this could look like (warning, wacky code ahead,
>>>> though I fixed a stupid bug that was present in the previous patch).
>>>> The generated code is quite nice (no branch, only an extra mov
>>>> instruction on the default path)... Of course, completely untested!
>>>
>>> Isn't this all about determining which bitmask to use, statically, once,
>>> after the system has booted?
>>>
>>> How about a good old fashioned static variable, or global struct like
>>> the global one we use for the VGIC, which sets the proper mit mask
>>> during kvm init, and the world-switch code just uses a variable?
>>
>> We could indeed do that (I've been carried away with my tendency for
>> weird and wonderful hacks).
>>
>> But as Jintack mentioned, there is a much better approach, which is to
>> do nothing at all on the VHE path (we can set the permission bits once
>> and for all). cntvoff_el2 also falls into the same category of things we
>> should be able to only restore and not bother resetting (as it doesn't
>> affect the EL2 virtual counter).
>>
>> Thoughts?
>>
> Yes, that sounds much better.
> 
> I have some patches to get rid of a lot of things, like cntvoff, during
> the world-switch for VHE, so if Jintack just wants to focus on the
> cnthctl I will catch cntvoff later.

Sound good to me.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Marc Zyngier]

On 29/11/16 10:47, Christoffer Dall wrote:
> On Tue, Nov 29, 2016 at 09:37:07AM +0000, Marc Zyngier wrote:
>> On 28/11/16 19:42, Christoffer Dall wrote:
>>> On Mon, Nov 28, 2016 at 06:39:04PM +0000, Marc Zyngier wrote:
>>>> On 28/11/16 17:43, Marc Zyngier wrote:
>>>>> Hi Jintack,
>>>>>
>>>>> On 28/11/16 16:46, Jintack Lim wrote:
>>>>>> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
>>>>>> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
>>>>>> are 11th and 10th bits respectively when E2H is set.  Current code is
>>>>>> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
>>>>>> may allow guest OS to access physical timer. So, fix it.
>>>>>>
>>>>>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
>>>>>> ---
>>>>>>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>>>>>>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>>>>>>  include/clocksource/arm_arch_timer.h |  6 ++--
>>>>>>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>>>>>>  4 files changed, 103 insertions(+), 6 deletions(-)
>>>>>>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>>>>>>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
>>>>>>
>>>>
>>>> [...]
>>>>
>>>>> We could make it nicer (read "faster") by introducing a
>>>>> hyp_alternate_select construct that only returns a value instead
>>>>> of calling a function. I remember writing something like that
>>>>> at some point, and dropping it...
>>>>
>>>> So here's what this could look like (warning, wacky code ahead,
>>>> though I fixed a stupid bug that was present in the previous patch).
>>>> The generated code is quite nice (no branch, only an extra mov
>>>> instruction on the default path)... Of course, completely untested!
>>>
>>> Isn't this all about determining which bitmask to use, statically, once,
>>> after the system has booted?
>>>
>>> How about a good old fashioned static variable, or global struct like
>>> the global one we use for the VGIC, which sets the proper mit mask
>>> during kvm init, and the world-switch code just uses a variable?
>>
>> We could indeed do that (I've been carried away with my tendency for
>> weird and wonderful hacks).
>>
>> But as Jintack mentioned, there is a much better approach, which is to
>> do nothing at all on the VHE path (we can set the permission bits once
>> and for all). cntvoff_el2 also falls into the same category of things we
>> should be able to only restore and not bother resetting (as it doesn't
>> affect the EL2 virtual counter).
>>
>> Thoughts?
>>
> Yes, that sounds much better.
> 
> I have some patches to get rid of a lot of things, like cntvoff, during
> the world-switch for VHE, so if Jintack just wants to focus on the
> cnthctl I will catch cntvoff later.

Sound good to me.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Jintack Lim]

On Tue, Nov 29, 2016 at 4:36 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 29/11/16 03:28, Jintack Lim wrote:
>> On Mon, Nov 28, 2016 at 1:39 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 28/11/16 17:43, Marc Zyngier wrote:
>>>> Hi Jintack,
>>
>> Hi Marc,
>>
>>>>
>>>> On 28/11/16 16:46, Jintack Lim wrote:
>>>>> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
>>>>> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
>>>>> are 11th and 10th bits respectively when E2H is set.  Current code is
>>>>> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
>>>>> may allow guest OS to access physical timer. So, fix it.
>>>>>
>>>>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
>>>>> ---
>>>>>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>>>>>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>>>>>  include/clocksource/arm_arch_timer.h |  6 ++--
>>>>>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>>>>>  4 files changed, 103 insertions(+), 6 deletions(-)
>>>>>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>>>>>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
>>>>>
>>>
>>> [...]
>>>
>>>> We could make it nicer (read "faster") by introducing a
>>>> hyp_alternate_select construct that only returns a value instead
>>>> of calling a function. I remember writing something like that
>>>> at some point, and dropping it...
>>>
>>> So here's what this could look like (warning, wacky code ahead,
>>> though I fixed a stupid bug that was present in the previous patch).
>>> The generated code is quite nice (no branch, only an extra mov
>>> instruction on the default path)... Of course, completely untested!
>>
>> This looks much cleaner than my patch.
>> While we are at it, is it worth to consider that we just need to set
>> those bits once for VHE case, not for every world switch as an
>> optimization?
>
> Ah! That's a much better idea indeed! And we could stop messing with
> cntvoff_el2 as well, as it doesn't need to be restored to zero on exit.
> Could you try and respin something along those lines?

Yes, I can.

Thanks,
Jintack

>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
>

[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Jintack Lim]

On Tue, Nov 29, 2016 at 4:36 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 29/11/16 03:28, Jintack Lim wrote:
>> On Mon, Nov 28, 2016 at 1:39 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 28/11/16 17:43, Marc Zyngier wrote:
>>>> Hi Jintack,
>>
>> Hi Marc,
>>
>>>>
>>>> On 28/11/16 16:46, Jintack Lim wrote:
>>>>> Bit positions of CNTHCTL_EL2 are changing depending on HCR_EL2.E2H bit.
>>>>> EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is not set, but they
>>>>> are 11th and 10th bits respectively when E2H is set.  Current code is
>>>>> unintentionally setting wrong bits to CNTHCTL_EL2 with E2H set, which
>>>>> may allow guest OS to access physical timer. So, fix it.
>>>>>
>>>>> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
>>>>> ---
>>>>>  arch/arm/include/asm/kvm_timer.h     | 33 +++++++++++++++++++
>>>>>  arch/arm64/include/asm/kvm_timer.h   | 62 ++++++++++++++++++++++++++++++++++++
>>>>>  include/clocksource/arm_arch_timer.h |  6 ++--
>>>>>  virt/kvm/arm/hyp/timer-sr.c          |  8 ++---
>>>>>  4 files changed, 103 insertions(+), 6 deletions(-)
>>>>>  create mode 100644 arch/arm/include/asm/kvm_timer.h
>>>>>  create mode 100644 arch/arm64/include/asm/kvm_timer.h
>>>>>
>>>
>>> [...]
>>>
>>>> We could make it nicer (read "faster") by introducing a
>>>> hyp_alternate_select construct that only returns a value instead
>>>> of calling a function. I remember writing something like that
>>>> at some point, and dropping it...
>>>
>>> So here's what this could look like (warning, wacky code ahead,
>>> though I fixed a stupid bug that was present in the previous patch).
>>> The generated code is quite nice (no branch, only an extra mov
>>> instruction on the default path)... Of course, completely untested!
>>
>> This looks much cleaner than my patch.
>> While we are at it, is it worth to consider that we just need to set
>> those bits once for VHE case, not for every world switch as an
>> optimization?
>
> Ah! That's a much better idea indeed! And we could stop messing with
> cntvoff_el2 as well, as it doesn't need to be restored to zero on exit.
> Could you try and respin something along those lines?

Yes, I can.

Thanks,
Jintack

>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
>

Re: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Suzuki K Poulose]

On 29/11/16 09:36, Marc Zyngier wrote:
> On 29/11/16 03:28, Jintack Lim wrote:
>> On Mon, Nov 28, 2016 at 1:39 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 28/11/16 17:43, Marc Zyngier wrote:
>> This looks much cleaner than my patch.
>> While we are at it, is it worth to consider that we just need to set
>> those bits once for VHE case, not for every world switch as an
>> optimization?
>
> Ah! That's a much better idea indeed! And we could stop messing with
> cntvoff_el2 as well, as it doesn't need to be restored to zero on exit.
> Could you try and respin something along those lines?
>

fyi, we have a static_key based cpus_have_const_cap() for Constant cap
checking (like this case) available in linux-next. May be you could make use
of that instead of alternatives.


Cheers
Suzuki

[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Suzuki K Poulose]

On 29/11/16 09:36, Marc Zyngier wrote:
> On 29/11/16 03:28, Jintack Lim wrote:
>> On Mon, Nov 28, 2016 at 1:39 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 28/11/16 17:43, Marc Zyngier wrote:
>> This looks much cleaner than my patch.
>> While we are at it, is it worth to consider that we just need to set
>> those bits once for VHE case, not for every world switch as an
>> optimization?
>
> Ah! That's a much better idea indeed! And we could stop messing with
> cntvoff_el2 as well, as it doesn't need to be restored to zero on exit.
> Could you try and respin something along those lines?
>

fyi, we have a static_key based cpus_have_const_cap() for Constant cap
checking (like this case) available in linux-next. May be you could make use
of that instead of alternatives.


Cheers
Suzuki

Re: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Jintack Lim]

On Tue, Nov 29, 2016 at 11:53 AM, Suzuki K Poulose
<suzuki.poulose@arm.com> wrote:
> On 29/11/16 09:36, Marc Zyngier wrote:
>>
>> On 29/11/16 03:28, Jintack Lim wrote:
>>>
>>> On Mon, Nov 28, 2016 at 1:39 PM, Marc Zyngier <marc.zyngier@arm.com>
>>> wrote:
>>>>
>>>> On 28/11/16 17:43, Marc Zyngier wrote:
>>>
>>> This looks much cleaner than my patch.
>>> While we are at it, is it worth to consider that we just need to set
>>> those bits once for VHE case, not for every world switch as an
>>> optimization?
>>
>>
>> Ah! That's a much better idea indeed! And we could stop messing with
>> cntvoff_el2 as well, as it doesn't need to be restored to zero on exit.
>> Could you try and respin something along those lines?
>>
>
> fyi, we have a static_key based cpus_have_const_cap() for Constant cap
> checking (like this case) available in linux-next. May be you could make use
> of that instead of alternatives.

Thanks Suzuki. This looks very useful.

Marc, can I write a patch based on linux-next? The commit which has
cpus_have_const_cap() is not in master and next branch in kvm/arm
repo.

>
>
> Cheers
> Suzuki
>

[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Jintack Lim]

On Tue, Nov 29, 2016 at 11:53 AM, Suzuki K Poulose
<suzuki.poulose@arm.com> wrote:
> On 29/11/16 09:36, Marc Zyngier wrote:
>>
>> On 29/11/16 03:28, Jintack Lim wrote:
>>>
>>> On Mon, Nov 28, 2016 at 1:39 PM, Marc Zyngier <marc.zyngier@arm.com>
>>> wrote:
>>>>
>>>> On 28/11/16 17:43, Marc Zyngier wrote:
>>>
>>> This looks much cleaner than my patch.
>>> While we are at it, is it worth to consider that we just need to set
>>> those bits once for VHE case, not for every world switch as an
>>> optimization?
>>
>>
>> Ah! That's a much better idea indeed! And we could stop messing with
>> cntvoff_el2 as well, as it doesn't need to be restored to zero on exit.
>> Could you try and respin something along those lines?
>>
>
> fyi, we have a static_key based cpus_have_const_cap() for Constant cap
> checking (like this case) available in linux-next. May be you could make use
> of that instead of alternatives.

Thanks Suzuki. This looks very useful.

Marc, can I write a patch based on linux-next? The commit which has
cpus_have_const_cap() is not in master and next branch in kvm/arm
repo.

>
>
> Cheers
> Suzuki
>

Re: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Marc Zyngier]

On 29/11/16 21:05, Jintack Lim wrote:
> On Tue, Nov 29, 2016 at 11:53 AM, Suzuki K Poulose
> <suzuki.poulose@arm.com> wrote:
>> On 29/11/16 09:36, Marc Zyngier wrote:
>>>
>>> On 29/11/16 03:28, Jintack Lim wrote:
>>>>
>>>> On Mon, Nov 28, 2016 at 1:39 PM, Marc Zyngier <marc.zyngier@arm.com>
>>>> wrote:
>>>>>
>>>>> On 28/11/16 17:43, Marc Zyngier wrote:
>>>>
>>>> This looks much cleaner than my patch.
>>>> While we are at it, is it worth to consider that we just need to set
>>>> those bits once for VHE case, not for every world switch as an
>>>> optimization?
>>>
>>>
>>> Ah! That's a much better idea indeed! And we could stop messing with
>>> cntvoff_el2 as well, as it doesn't need to be restored to zero on exit.
>>> Could you try and respin something along those lines?
>>>
>>
>> fyi, we have a static_key based cpus_have_const_cap() for Constant cap
>> checking (like this case) available in linux-next. May be you could make use
>> of that instead of alternatives.
> 
> Thanks Suzuki. This looks very useful.
> 
> Marc, can I write a patch based on linux-next? The commit which has
> cpus_have_const_cap() is not in master and next branch in kvm/arm
> repo.

You can. It is just that I won't be able to apply it immediately (I'll
wait for -rc1 to be out, and send it as a fix).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Marc Zyngier]

On 29/11/16 21:05, Jintack Lim wrote:
> On Tue, Nov 29, 2016 at 11:53 AM, Suzuki K Poulose
> <suzuki.poulose@arm.com> wrote:
>> On 29/11/16 09:36, Marc Zyngier wrote:
>>>
>>> On 29/11/16 03:28, Jintack Lim wrote:
>>>>
>>>> On Mon, Nov 28, 2016 at 1:39 PM, Marc Zyngier <marc.zyngier@arm.com>
>>>> wrote:
>>>>>
>>>>> On 28/11/16 17:43, Marc Zyngier wrote:
>>>>
>>>> This looks much cleaner than my patch.
>>>> While we are at it, is it worth to consider that we just need to set
>>>> those bits once for VHE case, not for every world switch as an
>>>> optimization?
>>>
>>>
>>> Ah! That's a much better idea indeed! And we could stop messing with
>>> cntvoff_el2 as well, as it doesn't need to be restored to zero on exit.
>>> Could you try and respin something along those lines?
>>>
>>
>> fyi, we have a static_key based cpus_have_const_cap() for Constant cap
>> checking (like this case) available in linux-next. May be you could make use
>> of that instead of alternatives.
> 
> Thanks Suzuki. This looks very useful.
> 
> Marc, can I write a patch based on linux-next? The commit which has
> cpus_have_const_cap() is not in master and next branch in kvm/arm
> repo.

You can. It is just that I won't be able to apply it immediately (I'll
wait for -rc1 to be out, and send it as a fix).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Jintack Lim]

On Wed, Nov 30, 2016 at 8:31 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 29/11/16 21:05, Jintack Lim wrote:
>> On Tue, Nov 29, 2016 at 11:53 AM, Suzuki K Poulose
>> <suzuki.poulose@arm.com> wrote:
>>> On 29/11/16 09:36, Marc Zyngier wrote:
>>>>
>>>> On 29/11/16 03:28, Jintack Lim wrote:
>>>>>
>>>>> On Mon, Nov 28, 2016 at 1:39 PM, Marc Zyngier <marc.zyngier@arm.com>
>>>>> wrote:
>>>>>>
>>>>>> On 28/11/16 17:43, Marc Zyngier wrote:
>>>>>
>>>>> This looks much cleaner than my patch.
>>>>> While we are at it, is it worth to consider that we just need to set
>>>>> those bits once for VHE case, not for every world switch as an
>>>>> optimization?
>>>>
>>>>
>>>> Ah! That's a much better idea indeed! And we could stop messing with
>>>> cntvoff_el2 as well, as it doesn't need to be restored to zero on exit.
>>>> Could you try and respin something along those lines?
>>>>
>>>
>>> fyi, we have a static_key based cpus_have_const_cap() for Constant cap
>>> checking (like this case) available in linux-next. May be you could make use
>>> of that instead of alternatives.
>>
>> Thanks Suzuki. This looks very useful.
>>
>> Marc, can I write a patch based on linux-next? The commit which has
>> cpus_have_const_cap() is not in master and next branch in kvm/arm
>> repo.
>
> You can. It is just that I won't be able to apply it immediately (I'll
> wait for -rc1 to be out, and send it as a fix).

Ok. Thanks!. I'll send out v2.

>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
>

[PATCH] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Jintack Lim]

On Wed, Nov 30, 2016 at 8:31 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 29/11/16 21:05, Jintack Lim wrote:
>> On Tue, Nov 29, 2016 at 11:53 AM, Suzuki K Poulose
>> <suzuki.poulose@arm.com> wrote:
>>> On 29/11/16 09:36, Marc Zyngier wrote:
>>>>
>>>> On 29/11/16 03:28, Jintack Lim wrote:
>>>>>
>>>>> On Mon, Nov 28, 2016 at 1:39 PM, Marc Zyngier <marc.zyngier@arm.com>
>>>>> wrote:
>>>>>>
>>>>>> On 28/11/16 17:43, Marc Zyngier wrote:
>>>>>
>>>>> This looks much cleaner than my patch.
>>>>> While we are at it, is it worth to consider that we just need to set
>>>>> those bits once for VHE case, not for every world switch as an
>>>>> optimization?
>>>>
>>>>
>>>> Ah! That's a much better idea indeed! And we could stop messing with
>>>> cntvoff_el2 as well, as it doesn't need to be restored to zero on exit.
>>>> Could you try and respin something along those lines?
>>>>
>>>
>>> fyi, we have a static_key based cpus_have_const_cap() for Constant cap
>>> checking (like this case) available in linux-next. May be you could make use
>>> of that instead of alternatives.
>>
>> Thanks Suzuki. This looks very useful.
>>
>> Marc, can I write a patch based on linux-next? The commit which has
>> cpus_have_const_cap() is not in master and next branch in kvm/arm
>> repo.
>
> You can. It is just that I won't be able to apply it immediately (I'll
> wait for -rc1 to be out, and send it as a fix).

Ok. Thanks!. I'll send out v2.

>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
>