[PATCH 1/2] arm64: kvm: reuse existing cache type/info related macros

[PATCH 1/2] arm64: kvm: reuse existing cache type/info related macros

[Sudeep Holla]

We already have various macros related to cache type and bitfields in
CLIDR system register. We can replace some of the hardcoded values
here using those existing macros.

This patch reuses those existing cache type/info related macros and
replaces the hardcorded values. It also removes some of the comments
that become trivial with the macro names.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/include/asm/cachetype.h |  7 +++++++
 arch/arm64/kernel/cacheinfo.c      |  7 -------
 arch/arm64/kvm/sys_regs.c          | 27 +++++++++++++--------------
 3 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/include/asm/cachetype.h b/arch/arm64/include/asm/cachetype.h
index f5588692f1d4..f58b5e3df6b8 100644
--- a/arch/arm64/include/asm/cachetype.h
+++ b/arch/arm64/include/asm/cachetype.h
@@ -39,6 +39,13 @@
 
 extern unsigned long __icache_flags;
 
+#define MAX_CACHE_LEVEL			7	/* Max 7 level supported */
+/* Ctypen, bits[3(n - 1) + 2 : 3(n - 1)], for n = 1 to 7 */
+#define CLIDR_CTYPE_SHIFT(level)	(3 * (level - 1))
+#define CLIDR_CTYPE_MASK(level)		(7 << CLIDR_CTYPE_SHIFT(level))
+#define CLIDR_CTYPE(clidr, level)	\
+	(((clidr) & CLIDR_CTYPE_MASK(level)) >> CLIDR_CTYPE_SHIFT(level))
+
 /*
  * NumSets, bits[27:13] - (Number of sets in cache) - 1
  * Associativity, bits[12:3] - (Associativity of cache) - 1
diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
index 3f2250fc391b..a460208b08cf 100644
--- a/arch/arm64/kernel/cacheinfo.c
+++ b/arch/arm64/kernel/cacheinfo.c
@@ -26,13 +26,6 @@
 #include <asm/cachetype.h>
 #include <asm/processor.h>
 
-#define MAX_CACHE_LEVEL			7	/* Max 7 level supported */
-/* Ctypen, bits[3(n - 1) + 2 : 3(n - 1)], for n = 1 to 7 */
-#define CLIDR_CTYPE_SHIFT(level)	(3 * (level - 1))
-#define CLIDR_CTYPE_MASK(level)		(7 << CLIDR_CTYPE_SHIFT(level))
-#define CLIDR_CTYPE(clidr, level)	\
-	(((clidr) & CLIDR_CTYPE_MASK(level)) >> CLIDR_CTYPE_SHIFT(level))
-
 static inline enum cache_type get_cache_type(int level)
 {
 	u64 clidr;
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 87e7e6608cd8..5dca1f10340f 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -21,11 +21,13 @@
  */
 
 #include <linux/bsearch.h>
+#include <linux/cacheinfo.h>
 #include <linux/kvm_host.h>
 #include <linux/mm.h>
 #include <linux/uaccess.h>
 
 #include <asm/cacheflush.h>
+#include <asm/cachetype.h>
 #include <asm/cputype.h>
 #include <asm/debug-monitors.h>
 #include <asm/esr.h>
@@ -59,7 +61,7 @@
 static u32 cache_levels;
 
 /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
-#define CSSELR_MAX 12
+#define CSSELR_MAX 	((MAX_CACHE_LEVEL - 1) >> 1)
 
 /* Which cache CCSIDR represents depends on CSSELR value. */
 static u32 get_ccsidr(u32 csselr)
@@ -68,9 +70,7 @@ static u32 get_ccsidr(u32 csselr)
 
 	/* Make sure noone else changes CSSELR during this! */
 	local_irq_disable();
-	write_sysreg(csselr, csselr_el1);
-	isb();
-	ccsidr = read_sysreg(ccsidr_el1);
+	ccsidr = cache_get_ccsidr(csselr);
 	local_irq_enable();
 
 	return ccsidr;
@@ -1960,19 +1960,18 @@ static bool is_valid_cache(u32 val)
 		return false;
 
 	/* Bottom bit is Instruction or Data bit.  Next 3 bits are level. */
-	level = (val >> 1);
-	ctype = (cache_levels >> (level * 3)) & 7;
+	level = (val >> 1) + 1;
+	ctype = CLIDR_CTYPE(cache_levels, level);
 
 	switch (ctype) {
-	case 0: /* No cache */
-		return false;
-	case 1: /* Instruction cache only */
-		return (val & 1);
-	case 2: /* Data cache only */
-	case 4: /* Unified cache */
-		return !(val & 1);
-	case 3: /* Separate instruction and data caches */
+	case CACHE_TYPE_INST:
+		return (val & CACHE_TYPE_INST);
+	case CACHE_TYPE_DATA:
+	case CACHE_TYPE_UNIFIED:
+		return !(val & CACHE_TYPE_INST);
+	case CACHE_TYPE_SEPARATE:
 		return true;
+	case CACHE_TYPE_NOCACHE:
 	default: /* Reserved: we can't know instruction or data. */
 		return false;
 	}
-- 
2.7.4

[PATCH 1/2] arm64: kvm: reuse existing cache type/info related macros

[Sudeep Holla]

We already have various macros related to cache type and bitfields in
CLIDR system register. We can replace some of the hardcoded values
here using those existing macros.

This patch reuses those existing cache type/info related macros and
replaces the hardcorded values. It also removes some of the comments
that become trivial with the macro names.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/include/asm/cachetype.h |  7 +++++++
 arch/arm64/kernel/cacheinfo.c      |  7 -------
 arch/arm64/kvm/sys_regs.c          | 27 +++++++++++++--------------
 3 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/include/asm/cachetype.h b/arch/arm64/include/asm/cachetype.h
index f5588692f1d4..f58b5e3df6b8 100644
--- a/arch/arm64/include/asm/cachetype.h
+++ b/arch/arm64/include/asm/cachetype.h
@@ -39,6 +39,13 @@
 
 extern unsigned long __icache_flags;
 
+#define MAX_CACHE_LEVEL			7	/* Max 7 level supported */
+/* Ctypen, bits[3(n - 1) + 2 : 3(n - 1)], for n = 1 to 7 */
+#define CLIDR_CTYPE_SHIFT(level)	(3 * (level - 1))
+#define CLIDR_CTYPE_MASK(level)		(7 << CLIDR_CTYPE_SHIFT(level))
+#define CLIDR_CTYPE(clidr, level)	\
+	(((clidr) & CLIDR_CTYPE_MASK(level)) >> CLIDR_CTYPE_SHIFT(level))
+
 /*
  * NumSets, bits[27:13] - (Number of sets in cache) - 1
  * Associativity, bits[12:3] - (Associativity of cache) - 1
diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
index 3f2250fc391b..a460208b08cf 100644
--- a/arch/arm64/kernel/cacheinfo.c
+++ b/arch/arm64/kernel/cacheinfo.c
@@ -26,13 +26,6 @@
 #include <asm/cachetype.h>
 #include <asm/processor.h>
 
-#define MAX_CACHE_LEVEL			7	/* Max 7 level supported */
-/* Ctypen, bits[3(n - 1) + 2 : 3(n - 1)], for n = 1 to 7 */
-#define CLIDR_CTYPE_SHIFT(level)	(3 * (level - 1))
-#define CLIDR_CTYPE_MASK(level)		(7 << CLIDR_CTYPE_SHIFT(level))
-#define CLIDR_CTYPE(clidr, level)	\
-	(((clidr) & CLIDR_CTYPE_MASK(level)) >> CLIDR_CTYPE_SHIFT(level))
-
 static inline enum cache_type get_cache_type(int level)
 {
 	u64 clidr;
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 87e7e6608cd8..5dca1f10340f 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -21,11 +21,13 @@
  */
 
 #include <linux/bsearch.h>
+#include <linux/cacheinfo.h>
 #include <linux/kvm_host.h>
 #include <linux/mm.h>
 #include <linux/uaccess.h>
 
 #include <asm/cacheflush.h>
+#include <asm/cachetype.h>
 #include <asm/cputype.h>
 #include <asm/debug-monitors.h>
 #include <asm/esr.h>
@@ -59,7 +61,7 @@
 static u32 cache_levels;
 
 /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
-#define CSSELR_MAX 12
+#define CSSELR_MAX 	((MAX_CACHE_LEVEL - 1) >> 1)
 
 /* Which cache CCSIDR represents depends on CSSELR value. */
 static u32 get_ccsidr(u32 csselr)
@@ -68,9 +70,7 @@ static u32 get_ccsidr(u32 csselr)
 
 	/* Make sure noone else changes CSSELR during this! */
 	local_irq_disable();
-	write_sysreg(csselr, csselr_el1);
-	isb();
-	ccsidr = read_sysreg(ccsidr_el1);
+	ccsidr = cache_get_ccsidr(csselr);
 	local_irq_enable();
 
 	return ccsidr;
@@ -1960,19 +1960,18 @@ static bool is_valid_cache(u32 val)
 		return false;
 
 	/* Bottom bit is Instruction or Data bit.  Next 3 bits are level. */
-	level = (val >> 1);
-	ctype = (cache_levels >> (level * 3)) & 7;
+	level = (val >> 1) + 1;
+	ctype = CLIDR_CTYPE(cache_levels, level);
 
 	switch (ctype) {
-	case 0: /* No cache */
-		return false;
-	case 1: /* Instruction cache only */
-		return (val & 1);
-	case 2: /* Data cache only */
-	case 4: /* Unified cache */
-		return !(val & 1);
-	case 3: /* Separate instruction and data caches */
+	case CACHE_TYPE_INST:
+		return (val & CACHE_TYPE_INST);
+	case CACHE_TYPE_DATA:
+	case CACHE_TYPE_UNIFIED:
+		return !(val & CACHE_TYPE_INST);
+	case CACHE_TYPE_SEPARATE:
 		return true;
+	case CACHE_TYPE_NOCACHE:
 	default: /* Reserved: we can't know instruction or data. */
 		return false;
 	}
-- 
2.7.4

[PATCH 2/2] arm64: kvm: upgrade csselr and ccsidr to 64-bit values

[Sudeep Holla]

csselr and ccsidr are treated as 64-bit values already elsewhere in the
kernel. It also aligns well with the architecture extensions that allow
64-bit format for ccsidr.

This patch upgrades the existing accesses to csselr and ccsidr from
32-bit to 64-bit in preparation to add support to those extensions.

Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 5dca1f10340f..a3559a8a2b0c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -58,15 +58,15 @@
  */
 
 /* 3 bits per cache level, as per CLIDR, but non-existent caches always 0 */
-static u32 cache_levels;
+static u64 cache_levels;
 
 /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
 #define CSSELR_MAX 	((MAX_CACHE_LEVEL - 1) >> 1)
 
 /* Which cache CCSIDR represents depends on CSSELR value. */
-static u32 get_ccsidr(u32 csselr)
+static u64 get_ccsidr(u64 csselr)
 {
-	u32 ccsidr;
+	u64 ccsidr;
 
 	/* Make sure noone else changes CSSELR during this! */
 	local_irq_disable();
@@ -1952,9 +1952,9 @@ static int set_invariant_sys_reg(u64 id, void __user *uaddr)
 	return 0;
 }
 
-static bool is_valid_cache(u32 val)
+static bool is_valid_cache(u64 val)
 {
-	u32 level, ctype;
+	u64 level, ctype;
 
 	if (val >= CSSELR_MAX)
 		return false;
@@ -1979,8 +1979,8 @@ static bool is_valid_cache(u32 val)
 
 static int demux_c15_get(u64 id, void __user *uaddr)
 {
-	u32 val;
-	u32 __user *uval = uaddr;
+	u64 val;
+	u64 __user *uval = uaddr;
 
 	/* Fail if we have unknown bits set. */
 	if (id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK|KVM_REG_ARM_COPROC_MASK
@@ -2004,8 +2004,8 @@ static int demux_c15_get(u64 id, void __user *uaddr)
 
 static int demux_c15_set(u64 id, void __user *uaddr)
 {
-	u32 val, newval;
-	u32 __user *uval = uaddr;
+	u64 val, newval;
+	u64 __user *uval = uaddr;
 
 	/* Fail if we have unknown bits set. */
 	if (id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK|KVM_REG_ARM_COPROC_MASK
-- 
2.7.4

[PATCH 2/2] arm64: kvm: upgrade csselr and ccsidr to 64-bit values

[Sudeep Holla]

csselr and ccsidr are treated as 64-bit values already elsewhere in the
kernel. It also aligns well with the architecture extensions that allow
64-bit format for ccsidr.

This patch upgrades the existing accesses to csselr and ccsidr from
32-bit to 64-bit in preparation to add support to those extensions.

Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 5dca1f10340f..a3559a8a2b0c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -58,15 +58,15 @@
  */
 
 /* 3 bits per cache level, as per CLIDR, but non-existent caches always 0 */
-static u32 cache_levels;
+static u64 cache_levels;
 
 /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
 #define CSSELR_MAX 	((MAX_CACHE_LEVEL - 1) >> 1)
 
 /* Which cache CCSIDR represents depends on CSSELR value. */
-static u32 get_ccsidr(u32 csselr)
+static u64 get_ccsidr(u64 csselr)
 {
-	u32 ccsidr;
+	u64 ccsidr;
 
 	/* Make sure noone else changes CSSELR during this! */
 	local_irq_disable();
@@ -1952,9 +1952,9 @@ static int set_invariant_sys_reg(u64 id, void __user *uaddr)
 	return 0;
 }
 
-static bool is_valid_cache(u32 val)
+static bool is_valid_cache(u64 val)
 {
-	u32 level, ctype;
+	u64 level, ctype;
 
 	if (val >= CSSELR_MAX)
 		return false;
@@ -1979,8 +1979,8 @@ static bool is_valid_cache(u32 val)
 
 static int demux_c15_get(u64 id, void __user *uaddr)
 {
-	u32 val;
-	u32 __user *uval = uaddr;
+	u64 val;
+	u64 __user *uval = uaddr;
 
 	/* Fail if we have unknown bits set. */
 	if (id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK|KVM_REG_ARM_COPROC_MASK
@@ -2004,8 +2004,8 @@ static int demux_c15_get(u64 id, void __user *uaddr)
 
 static int demux_c15_set(u64 id, void __user *uaddr)
 {
-	u32 val, newval;
-	u32 __user *uval = uaddr;
+	u64 val, newval;
+	u64 __user *uval = uaddr;
 
 	/* Fail if we have unknown bits set. */
 	if (id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK|KVM_REG_ARM_COPROC_MASK
-- 
2.7.4

Re: [PATCH 1/2] arm64: kvm: reuse existing cache type/info related macros

[Will Deacon]

On Fri, Jan 20, 2017 at 10:50:09AM +0000, Sudeep Holla wrote:
> We already have various macros related to cache type and bitfields in
> CLIDR system register. We can replace some of the hardcoded values
> here using those existing macros.
> 
> This patch reuses those existing cache type/info related macros and
> replaces the hardcorded values. It also removes some of the comments
> that become trivial with the macro names.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  arch/arm64/include/asm/cachetype.h |  7 +++++++
>  arch/arm64/kernel/cacheinfo.c      |  7 -------
>  arch/arm64/kvm/sys_regs.c          | 27 +++++++++++++--------------
>  3 files changed, 20 insertions(+), 21 deletions(-)

I assume both of these will go via kvm-arm.

Will

[PATCH 1/2] arm64: kvm: reuse existing cache type/info related macros

[Will Deacon]

On Fri, Jan 20, 2017 at 10:50:09AM +0000, Sudeep Holla wrote:
> We already have various macros related to cache type and bitfields in
> CLIDR system register. We can replace some of the hardcoded values
> here using those existing macros.
> 
> This patch reuses those existing cache type/info related macros and
> replaces the hardcorded values. It also removes some of the comments
> that become trivial with the macro names.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  arch/arm64/include/asm/cachetype.h |  7 +++++++
>  arch/arm64/kernel/cacheinfo.c      |  7 -------
>  arch/arm64/kvm/sys_regs.c          | 27 +++++++++++++--------------
>  3 files changed, 20 insertions(+), 21 deletions(-)

I assume both of these will go via kvm-arm.

Will

Re: [PATCH 1/2] arm64: kvm: reuse existing cache type/info related macros

[Christoffer Dall]

On Fri, Jan 20, 2017 at 10:50:09AM +0000, Sudeep Holla wrote:
> We already have various macros related to cache type and bitfields in
> CLIDR system register. We can replace some of the hardcoded values
> here using those existing macros.
> 
> This patch reuses those existing cache type/info related macros and
> replaces the hardcorded values. It also removes some of the comments
> that become trivial with the macro names.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  arch/arm64/include/asm/cachetype.h |  7 +++++++
>  arch/arm64/kernel/cacheinfo.c      |  7 -------
>  arch/arm64/kvm/sys_regs.c          | 27 +++++++++++++--------------
>  3 files changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cachetype.h b/arch/arm64/include/asm/cachetype.h
> index f5588692f1d4..f58b5e3df6b8 100644
> --- a/arch/arm64/include/asm/cachetype.h
> +++ b/arch/arm64/include/asm/cachetype.h
> @@ -39,6 +39,13 @@
>  
>  extern unsigned long __icache_flags;
>  
> +#define MAX_CACHE_LEVEL			7	/* Max 7 level supported */
> +/* Ctypen, bits[3(n - 1) + 2 : 3(n - 1)], for n = 1 to 7 */
> +#define CLIDR_CTYPE_SHIFT(level)	(3 * (level - 1))
> +#define CLIDR_CTYPE_MASK(level)		(7 << CLIDR_CTYPE_SHIFT(level))
> +#define CLIDR_CTYPE(clidr, level)	\
> +	(((clidr) & CLIDR_CTYPE_MASK(level)) >> CLIDR_CTYPE_SHIFT(level))
> +
>  /*
>   * NumSets, bits[27:13] - (Number of sets in cache) - 1
>   * Associativity, bits[12:3] - (Associativity of cache) - 1
> diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
> index 3f2250fc391b..a460208b08cf 100644
> --- a/arch/arm64/kernel/cacheinfo.c
> +++ b/arch/arm64/kernel/cacheinfo.c
> @@ -26,13 +26,6 @@
>  #include <asm/cachetype.h>
>  #include <asm/processor.h>
>  
> -#define MAX_CACHE_LEVEL			7	/* Max 7 level supported */
> -/* Ctypen, bits[3(n - 1) + 2 : 3(n - 1)], for n = 1 to 7 */
> -#define CLIDR_CTYPE_SHIFT(level)	(3 * (level - 1))
> -#define CLIDR_CTYPE_MASK(level)		(7 << CLIDR_CTYPE_SHIFT(level))
> -#define CLIDR_CTYPE(clidr, level)	\
> -	(((clidr) & CLIDR_CTYPE_MASK(level)) >> CLIDR_CTYPE_SHIFT(level))
> -
>  static inline enum cache_type get_cache_type(int level)
>  {
>  	u64 clidr;
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 87e7e6608cd8..5dca1f10340f 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -21,11 +21,13 @@
>   */
>  
>  #include <linux/bsearch.h>
> +#include <linux/cacheinfo.h>
>  #include <linux/kvm_host.h>
>  #include <linux/mm.h>
>  #include <linux/uaccess.h>
>  
>  #include <asm/cacheflush.h>
> +#include <asm/cachetype.h>
>  #include <asm/cputype.h>
>  #include <asm/debug-monitors.h>
>  #include <asm/esr.h>
> @@ -59,7 +61,7 @@
>  static u32 cache_levels;
>  
>  /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
> -#define CSSELR_MAX 12
> +#define CSSELR_MAX 	((MAX_CACHE_LEVEL - 1) >> 1)

Did you mean '<< 1' here?

>  
>  /* Which cache CCSIDR represents depends on CSSELR value. */
>  static u32 get_ccsidr(u32 csselr)
> @@ -68,9 +70,7 @@ static u32 get_ccsidr(u32 csselr)
>  
>  	/* Make sure noone else changes CSSELR during this! */
>  	local_irq_disable();
> -	write_sysreg(csselr, csselr_el1);
> -	isb();
> -	ccsidr = read_sysreg(ccsidr_el1);
> +	ccsidr = cache_get_ccsidr(csselr);
>  	local_irq_enable();
>  
>  	return ccsidr;
> @@ -1960,19 +1960,18 @@ static bool is_valid_cache(u32 val)
>  		return false;
>  
>  	/* Bottom bit is Instruction or Data bit.  Next 3 bits are level. */
> -	level = (val >> 1);
> -	ctype = (cache_levels >> (level * 3)) & 7;
> +	level = (val >> 1) + 1;
> +	ctype = CLIDR_CTYPE(cache_levels, level);
>  
>  	switch (ctype) {
> -	case 0: /* No cache */
> -		return false;
> -	case 1: /* Instruction cache only */
> -		return (val & 1);
> -	case 2: /* Data cache only */
> -	case 4: /* Unified cache */
> -		return !(val & 1);
> -	case 3: /* Separate instruction and data caches */
> +	case CACHE_TYPE_INST:
> +		return (val & CACHE_TYPE_INST);
> +	case CACHE_TYPE_DATA:
> +	case CACHE_TYPE_UNIFIED:
> +		return !(val & CACHE_TYPE_INST);
> +	case CACHE_TYPE_SEPARATE:
>  		return true;
> +	case CACHE_TYPE_NOCACHE:
>  	default: /* Reserved: we can't know instruction or data. */
>  		return false;
>  	}
> -- 
> 2.7.4
> 

Otherwise this looks correct to me.

Thanks,
-Christoffer

[PATCH 1/2] arm64: kvm: reuse existing cache type/info related macros

[Christoffer Dall]

On Fri, Jan 20, 2017 at 10:50:09AM +0000, Sudeep Holla wrote:
> We already have various macros related to cache type and bitfields in
> CLIDR system register. We can replace some of the hardcoded values
> here using those existing macros.
> 
> This patch reuses those existing cache type/info related macros and
> replaces the hardcorded values. It also removes some of the comments
> that become trivial with the macro names.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  arch/arm64/include/asm/cachetype.h |  7 +++++++
>  arch/arm64/kernel/cacheinfo.c      |  7 -------
>  arch/arm64/kvm/sys_regs.c          | 27 +++++++++++++--------------
>  3 files changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cachetype.h b/arch/arm64/include/asm/cachetype.h
> index f5588692f1d4..f58b5e3df6b8 100644
> --- a/arch/arm64/include/asm/cachetype.h
> +++ b/arch/arm64/include/asm/cachetype.h
> @@ -39,6 +39,13 @@
>  
>  extern unsigned long __icache_flags;
>  
> +#define MAX_CACHE_LEVEL			7	/* Max 7 level supported */
> +/* Ctypen, bits[3(n - 1) + 2 : 3(n - 1)], for n = 1 to 7 */
> +#define CLIDR_CTYPE_SHIFT(level)	(3 * (level - 1))
> +#define CLIDR_CTYPE_MASK(level)		(7 << CLIDR_CTYPE_SHIFT(level))
> +#define CLIDR_CTYPE(clidr, level)	\
> +	(((clidr) & CLIDR_CTYPE_MASK(level)) >> CLIDR_CTYPE_SHIFT(level))
> +
>  /*
>   * NumSets, bits[27:13] - (Number of sets in cache) - 1
>   * Associativity, bits[12:3] - (Associativity of cache) - 1
> diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
> index 3f2250fc391b..a460208b08cf 100644
> --- a/arch/arm64/kernel/cacheinfo.c
> +++ b/arch/arm64/kernel/cacheinfo.c
> @@ -26,13 +26,6 @@
>  #include <asm/cachetype.h>
>  #include <asm/processor.h>
>  
> -#define MAX_CACHE_LEVEL			7	/* Max 7 level supported */
> -/* Ctypen, bits[3(n - 1) + 2 : 3(n - 1)], for n = 1 to 7 */
> -#define CLIDR_CTYPE_SHIFT(level)	(3 * (level - 1))
> -#define CLIDR_CTYPE_MASK(level)		(7 << CLIDR_CTYPE_SHIFT(level))
> -#define CLIDR_CTYPE(clidr, level)	\
> -	(((clidr) & CLIDR_CTYPE_MASK(level)) >> CLIDR_CTYPE_SHIFT(level))
> -
>  static inline enum cache_type get_cache_type(int level)
>  {
>  	u64 clidr;
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 87e7e6608cd8..5dca1f10340f 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -21,11 +21,13 @@
>   */
>  
>  #include <linux/bsearch.h>
> +#include <linux/cacheinfo.h>
>  #include <linux/kvm_host.h>
>  #include <linux/mm.h>
>  #include <linux/uaccess.h>
>  
>  #include <asm/cacheflush.h>
> +#include <asm/cachetype.h>
>  #include <asm/cputype.h>
>  #include <asm/debug-monitors.h>
>  #include <asm/esr.h>
> @@ -59,7 +61,7 @@
>  static u32 cache_levels;
>  
>  /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
> -#define CSSELR_MAX 12
> +#define CSSELR_MAX 	((MAX_CACHE_LEVEL - 1) >> 1)

Did you mean '<< 1' here?

>  
>  /* Which cache CCSIDR represents depends on CSSELR value. */
>  static u32 get_ccsidr(u32 csselr)
> @@ -68,9 +70,7 @@ static u32 get_ccsidr(u32 csselr)
>  
>  	/* Make sure noone else changes CSSELR during this! */
>  	local_irq_disable();
> -	write_sysreg(csselr, csselr_el1);
> -	isb();
> -	ccsidr = read_sysreg(ccsidr_el1);
> +	ccsidr = cache_get_ccsidr(csselr);
>  	local_irq_enable();
>  
>  	return ccsidr;
> @@ -1960,19 +1960,18 @@ static bool is_valid_cache(u32 val)
>  		return false;
>  
>  	/* Bottom bit is Instruction or Data bit.  Next 3 bits are level. */
> -	level = (val >> 1);
> -	ctype = (cache_levels >> (level * 3)) & 7;
> +	level = (val >> 1) + 1;
> +	ctype = CLIDR_CTYPE(cache_levels, level);
>  
>  	switch (ctype) {
> -	case 0: /* No cache */
> -		return false;
> -	case 1: /* Instruction cache only */
> -		return (val & 1);
> -	case 2: /* Data cache only */
> -	case 4: /* Unified cache */
> -		return !(val & 1);
> -	case 3: /* Separate instruction and data caches */
> +	case CACHE_TYPE_INST:
> +		return (val & CACHE_TYPE_INST);
> +	case CACHE_TYPE_DATA:
> +	case CACHE_TYPE_UNIFIED:
> +		return !(val & CACHE_TYPE_INST);
> +	case CACHE_TYPE_SEPARATE:
>  		return true;
> +	case CACHE_TYPE_NOCACHE:
>  	default: /* Reserved: we can't know instruction or data. */
>  		return false;
>  	}
> -- 
> 2.7.4
> 

Otherwise this looks correct to me.

Thanks,
-Christoffer

Re: [PATCH 2/2] arm64: kvm: upgrade csselr and ccsidr to 64-bit values

[Christoffer Dall]

On Fri, Jan 20, 2017 at 10:50:10AM +0000, Sudeep Holla wrote:
> csselr and ccsidr are treated as 64-bit values already elsewhere in the
> kernel. It also aligns well with the architecture extensions that allow
> 64-bit format for ccsidr.
> 
> This patch upgrades the existing accesses to csselr and ccsidr from
> 32-bit to 64-bit in preparation to add support to those extensions.
> 
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 5dca1f10340f..a3559a8a2b0c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -58,15 +58,15 @@
>   */
>  
>  /* 3 bits per cache level, as per CLIDR, but non-existent caches always 0 */
> -static u32 cache_levels;
> +static u64 cache_levels;
>  
>  /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
>  #define CSSELR_MAX 	((MAX_CACHE_LEVEL - 1) >> 1)
>  
>  /* Which cache CCSIDR represents depends on CSSELR value. */
> -static u32 get_ccsidr(u32 csselr)
> +static u64 get_ccsidr(u64 csselr)
>  {
> -	u32 ccsidr;
> +	u64 ccsidr;
>  
>  	/* Make sure noone else changes CSSELR during this! */
>  	local_irq_disable();
> @@ -1952,9 +1952,9 @@ static int set_invariant_sys_reg(u64 id, void __user *uaddr)
>  	return 0;
>  }
>  
> -static bool is_valid_cache(u32 val)
> +static bool is_valid_cache(u64 val)
>  {
> -	u32 level, ctype;
> +	u64 level, ctype;
>  
>  	if (val >= CSSELR_MAX)
>  		return false;
> @@ -1979,8 +1979,8 @@ static bool is_valid_cache(u32 val)
>  
>  static int demux_c15_get(u64 id, void __user *uaddr)
>  {
> -	u32 val;
> -	u32 __user *uval = uaddr;
> +	u64 val;
> +	u64 __user *uval = uaddr;
>  
>  	/* Fail if we have unknown bits set. */
>  	if (id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK|KVM_REG_ARM_COPROC_MASK
> @@ -2004,8 +2004,8 @@ static int demux_c15_get(u64 id, void __user *uaddr)
>  
>  static int demux_c15_set(u64 id, void __user *uaddr)
>  {
> -	u32 val, newval;
> -	u32 __user *uval = uaddr;
> +	u64 val, newval;
> +	u64 __user *uval = uaddr;

Doesn't converting these uval pointers to u64 cause us to break the ABI
as we'll now be reading/writing 64-bit values to userspace with the
get_user and put_user following the declarations?

>  
>  	/* Fail if we have unknown bits set. */
>  	if (id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK|KVM_REG_ARM_COPROC_MASK
> -- 
> 2.7.4
> 

Thanks,
-Christoffer

[PATCH 2/2] arm64: kvm: upgrade csselr and ccsidr to 64-bit values

[Christoffer Dall]

On Fri, Jan 20, 2017 at 10:50:10AM +0000, Sudeep Holla wrote:
> csselr and ccsidr are treated as 64-bit values already elsewhere in the
> kernel. It also aligns well with the architecture extensions that allow
> 64-bit format for ccsidr.
> 
> This patch upgrades the existing accesses to csselr and ccsidr from
> 32-bit to 64-bit in preparation to add support to those extensions.
> 
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 5dca1f10340f..a3559a8a2b0c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -58,15 +58,15 @@
>   */
>  
>  /* 3 bits per cache level, as per CLIDR, but non-existent caches always 0 */
> -static u32 cache_levels;
> +static u64 cache_levels;
>  
>  /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
>  #define CSSELR_MAX 	((MAX_CACHE_LEVEL - 1) >> 1)
>  
>  /* Which cache CCSIDR represents depends on CSSELR value. */
> -static u32 get_ccsidr(u32 csselr)
> +static u64 get_ccsidr(u64 csselr)
>  {
> -	u32 ccsidr;
> +	u64 ccsidr;
>  
>  	/* Make sure noone else changes CSSELR during this! */
>  	local_irq_disable();
> @@ -1952,9 +1952,9 @@ static int set_invariant_sys_reg(u64 id, void __user *uaddr)
>  	return 0;
>  }
>  
> -static bool is_valid_cache(u32 val)
> +static bool is_valid_cache(u64 val)
>  {
> -	u32 level, ctype;
> +	u64 level, ctype;
>  
>  	if (val >= CSSELR_MAX)
>  		return false;
> @@ -1979,8 +1979,8 @@ static bool is_valid_cache(u32 val)
>  
>  static int demux_c15_get(u64 id, void __user *uaddr)
>  {
> -	u32 val;
> -	u32 __user *uval = uaddr;
> +	u64 val;
> +	u64 __user *uval = uaddr;
>  
>  	/* Fail if we have unknown bits set. */
>  	if (id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK|KVM_REG_ARM_COPROC_MASK
> @@ -2004,8 +2004,8 @@ static int demux_c15_get(u64 id, void __user *uaddr)
>  
>  static int demux_c15_set(u64 id, void __user *uaddr)
>  {
> -	u32 val, newval;
> -	u32 __user *uval = uaddr;
> +	u64 val, newval;
> +	u64 __user *uval = uaddr;

Doesn't converting these uval pointers to u64 cause us to break the ABI
as we'll now be reading/writing 64-bit values to userspace with the
get_user and put_user following the declarations?

>  
>  	/* Fail if we have unknown bits set. */
>  	if (id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK|KVM_REG_ARM_COPROC_MASK
> -- 
> 2.7.4
> 

Thanks,
-Christoffer

Re: [PATCH 1/2] arm64: kvm: reuse existing cache type/info related macros

[Sudeep Holla]

On 23/01/17 21:05, Christoffer Dall wrote:
> On Fri, Jan 20, 2017 at 10:50:09AM +0000, Sudeep Holla wrote:
>> We already have various macros related to cache type and bitfields in
>> CLIDR system register. We can replace some of the hardcoded values
>> here using those existing macros.
>>
>> This patch reuses those existing cache type/info related macros and
>> replaces the hardcorded values. It also removes some of the comments
>> that become trivial with the macro names.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  arch/arm64/include/asm/cachetype.h |  7 +++++++
>>  arch/arm64/kernel/cacheinfo.c      |  7 -------
>>  arch/arm64/kvm/sys_regs.c          | 27 +++++++++++++--------------
>>  3 files changed, 20 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/cachetype.h b/arch/arm64/include/asm/cachetype.h
>> index f5588692f1d4..f58b5e3df6b8 100644
>> --- a/arch/arm64/include/asm/cachetype.h
>> +++ b/arch/arm64/include/asm/cachetype.h
>> @@ -39,6 +39,13 @@
>>  
>>  extern unsigned long __icache_flags;
>>  
>> +#define MAX_CACHE_LEVEL			7	/* Max 7 level supported */
>> +/* Ctypen, bits[3(n - 1) + 2 : 3(n - 1)], for n = 1 to 7 */
>> +#define CLIDR_CTYPE_SHIFT(level)	(3 * (level - 1))
>> +#define CLIDR_CTYPE_MASK(level)		(7 << CLIDR_CTYPE_SHIFT(level))
>> +#define CLIDR_CTYPE(clidr, level)	\
>> +	(((clidr) & CLIDR_CTYPE_MASK(level)) >> CLIDR_CTYPE_SHIFT(level))
>> +
>>  /*
>>   * NumSets, bits[27:13] - (Number of sets in cache) - 1
>>   * Associativity, bits[12:3] - (Associativity of cache) - 1
>> diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
>> index 3f2250fc391b..a460208b08cf 100644
>> --- a/arch/arm64/kernel/cacheinfo.c
>> +++ b/arch/arm64/kernel/cacheinfo.c
>> @@ -26,13 +26,6 @@
>>  #include <asm/cachetype.h>
>>  #include <asm/processor.h>
>>  
>> -#define MAX_CACHE_LEVEL			7	/* Max 7 level supported */
>> -/* Ctypen, bits[3(n - 1) + 2 : 3(n - 1)], for n = 1 to 7 */
>> -#define CLIDR_CTYPE_SHIFT(level)	(3 * (level - 1))
>> -#define CLIDR_CTYPE_MASK(level)		(7 << CLIDR_CTYPE_SHIFT(level))
>> -#define CLIDR_CTYPE(clidr, level)	\
>> -	(((clidr) & CLIDR_CTYPE_MASK(level)) >> CLIDR_CTYPE_SHIFT(level))
>> -
>>  static inline enum cache_type get_cache_type(int level)
>>  {
>>  	u64 clidr;
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 87e7e6608cd8..5dca1f10340f 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -21,11 +21,13 @@
>>   */
>>  
>>  #include <linux/bsearch.h>
>> +#include <linux/cacheinfo.h>
>>  #include <linux/kvm_host.h>
>>  #include <linux/mm.h>
>>  #include <linux/uaccess.h>
>>  
>>  #include <asm/cacheflush.h>
>> +#include <asm/cachetype.h>
>>  #include <asm/cputype.h>
>>  #include <asm/debug-monitors.h>
>>  #include <asm/esr.h>
>> @@ -59,7 +61,7 @@
>>  static u32 cache_levels;
>>  
>>  /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
>> -#define CSSELR_MAX 12
>> +#define CSSELR_MAX 	((MAX_CACHE_LEVEL - 1) >> 1)
> 
> Did you mean '<< 1' here?
> 

Ah right, sorry for the stupid mistake.

-- 
Regards,
Sudeep

[PATCH 1/2] arm64: kvm: reuse existing cache type/info related macros

[Sudeep Holla]

On 23/01/17 21:05, Christoffer Dall wrote:
> On Fri, Jan 20, 2017 at 10:50:09AM +0000, Sudeep Holla wrote:
>> We already have various macros related to cache type and bitfields in
>> CLIDR system register. We can replace some of the hardcoded values
>> here using those existing macros.
>>
>> This patch reuses those existing cache type/info related macros and
>> replaces the hardcorded values. It also removes some of the comments
>> that become trivial with the macro names.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  arch/arm64/include/asm/cachetype.h |  7 +++++++
>>  arch/arm64/kernel/cacheinfo.c      |  7 -------
>>  arch/arm64/kvm/sys_regs.c          | 27 +++++++++++++--------------
>>  3 files changed, 20 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/cachetype.h b/arch/arm64/include/asm/cachetype.h
>> index f5588692f1d4..f58b5e3df6b8 100644
>> --- a/arch/arm64/include/asm/cachetype.h
>> +++ b/arch/arm64/include/asm/cachetype.h
>> @@ -39,6 +39,13 @@
>>  
>>  extern unsigned long __icache_flags;
>>  
>> +#define MAX_CACHE_LEVEL			7	/* Max 7 level supported */
>> +/* Ctypen, bits[3(n - 1) + 2 : 3(n - 1)], for n = 1 to 7 */
>> +#define CLIDR_CTYPE_SHIFT(level)	(3 * (level - 1))
>> +#define CLIDR_CTYPE_MASK(level)		(7 << CLIDR_CTYPE_SHIFT(level))
>> +#define CLIDR_CTYPE(clidr, level)	\
>> +	(((clidr) & CLIDR_CTYPE_MASK(level)) >> CLIDR_CTYPE_SHIFT(level))
>> +
>>  /*
>>   * NumSets, bits[27:13] - (Number of sets in cache) - 1
>>   * Associativity, bits[12:3] - (Associativity of cache) - 1
>> diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
>> index 3f2250fc391b..a460208b08cf 100644
>> --- a/arch/arm64/kernel/cacheinfo.c
>> +++ b/arch/arm64/kernel/cacheinfo.c
>> @@ -26,13 +26,6 @@
>>  #include <asm/cachetype.h>
>>  #include <asm/processor.h>
>>  
>> -#define MAX_CACHE_LEVEL			7	/* Max 7 level supported */
>> -/* Ctypen, bits[3(n - 1) + 2 : 3(n - 1)], for n = 1 to 7 */
>> -#define CLIDR_CTYPE_SHIFT(level)	(3 * (level - 1))
>> -#define CLIDR_CTYPE_MASK(level)		(7 << CLIDR_CTYPE_SHIFT(level))
>> -#define CLIDR_CTYPE(clidr, level)	\
>> -	(((clidr) & CLIDR_CTYPE_MASK(level)) >> CLIDR_CTYPE_SHIFT(level))
>> -
>>  static inline enum cache_type get_cache_type(int level)
>>  {
>>  	u64 clidr;
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 87e7e6608cd8..5dca1f10340f 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -21,11 +21,13 @@
>>   */
>>  
>>  #include <linux/bsearch.h>
>> +#include <linux/cacheinfo.h>
>>  #include <linux/kvm_host.h>
>>  #include <linux/mm.h>
>>  #include <linux/uaccess.h>
>>  
>>  #include <asm/cacheflush.h>
>> +#include <asm/cachetype.h>
>>  #include <asm/cputype.h>
>>  #include <asm/debug-monitors.h>
>>  #include <asm/esr.h>
>> @@ -59,7 +61,7 @@
>>  static u32 cache_levels;
>>  
>>  /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
>> -#define CSSELR_MAX 12
>> +#define CSSELR_MAX 	((MAX_CACHE_LEVEL - 1) >> 1)
> 
> Did you mean '<< 1' here?
> 

Ah right, sorry for the stupid mistake.

-- 
Regards,
Sudeep

Re: [PATCH 2/2] arm64: kvm: upgrade csselr and ccsidr to 64-bit values

[Sudeep Holla]

On 23/01/17 21:08, Christoffer Dall wrote:
> On Fri, Jan 20, 2017 at 10:50:10AM +0000, Sudeep Holla wrote:
>> csselr and ccsidr are treated as 64-bit values already elsewhere in the
>> kernel. It also aligns well with the architecture extensions that allow
>> 64-bit format for ccsidr.
>>
>> This patch upgrades the existing accesses to csselr and ccsidr from
>> 32-bit to 64-bit in preparation to add support to those extensions.
>>
>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 5dca1f10340f..a3559a8a2b0c 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c

[..]

>> @@ -2004,8 +2004,8 @@ static int demux_c15_get(u64 id, void __user *uaddr)
>>  
>>  static int demux_c15_set(u64 id, void __user *uaddr)
>>  {
>> -	u32 val, newval;
>> -	u32 __user *uval = uaddr;
>> +	u64 val, newval;
>> +	u64 __user *uval = uaddr;
> 
> Doesn't converting these uval pointers to u64 cause us to break the ABI
> as we'll now be reading/writing 64-bit values to userspace with the
> get_user and put_user following the declarations?
> 

Yes, I too have similar concern. IIUC it is always read via kvm_one_reg
structure. I could not find any specific user for this register to cross
check.

-- 
Regards,
Sudeep

[PATCH 2/2] arm64: kvm: upgrade csselr and ccsidr to 64-bit values

[Sudeep Holla]

On 23/01/17 21:08, Christoffer Dall wrote:
> On Fri, Jan 20, 2017 at 10:50:10AM +0000, Sudeep Holla wrote:
>> csselr and ccsidr are treated as 64-bit values already elsewhere in the
>> kernel. It also aligns well with the architecture extensions that allow
>> 64-bit format for ccsidr.
>>
>> This patch upgrades the existing accesses to csselr and ccsidr from
>> 32-bit to 64-bit in preparation to add support to those extensions.
>>
>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 5dca1f10340f..a3559a8a2b0c 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c

[..]

>> @@ -2004,8 +2004,8 @@ static int demux_c15_get(u64 id, void __user *uaddr)
>>  
>>  static int demux_c15_set(u64 id, void __user *uaddr)
>>  {
>> -	u32 val, newval;
>> -	u32 __user *uval = uaddr;
>> +	u64 val, newval;
>> +	u64 __user *uval = uaddr;
> 
> Doesn't converting these uval pointers to u64 cause us to break the ABI
> as we'll now be reading/writing 64-bit values to userspace with the
> get_user and put_user following the declarations?
> 

Yes, I too have similar concern. IIUC it is always read via kvm_one_reg
structure. I could not find any specific user for this register to cross
check.

-- 
Regards,
Sudeep

Re: [PATCH 2/2] arm64: kvm: upgrade csselr and ccsidr to 64-bit values

[Christoffer Dall]

On Tue, Jan 24, 2017 at 10:15:38AM +0000, Sudeep Holla wrote:
> 
> 
> On 23/01/17 21:08, Christoffer Dall wrote:
> > On Fri, Jan 20, 2017 at 10:50:10AM +0000, Sudeep Holla wrote:
> >> csselr and ccsidr are treated as 64-bit values already elsewhere in the
> >> kernel. It also aligns well with the architecture extensions that allow
> >> 64-bit format for ccsidr.
> >>
> >> This patch upgrades the existing accesses to csselr and ccsidr from
> >> 32-bit to 64-bit in preparation to add support to those extensions.
> >>
> >> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> >> Cc: Marc Zyngier <marc.zyngier@arm.com>
> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >> ---
> >>  arch/arm64/kvm/sys_regs.c | 18 +++++++++---------
> >>  1 file changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >> index 5dca1f10340f..a3559a8a2b0c 100644
> >> --- a/arch/arm64/kvm/sys_regs.c
> >> +++ b/arch/arm64/kvm/sys_regs.c
> 
> [..]
> 
> >> @@ -2004,8 +2004,8 @@ static int demux_c15_get(u64 id, void __user *uaddr)
> >>  
> >>  static int demux_c15_set(u64 id, void __user *uaddr)
> >>  {
> >> -	u32 val, newval;
> >> -	u32 __user *uval = uaddr;
> >> +	u64 val, newval;
> >> +	u64 __user *uval = uaddr;
> > 
> > Doesn't converting these uval pointers to u64 cause us to break the ABI
> > as we'll now be reading/writing 64-bit values to userspace with the
> > get_user and put_user following the declarations?
> > 
> 
> Yes, I too have similar concern. IIUC it is always read via kvm_one_reg
> structure. I could not find any specific user for this register to cross
> check.
> 

Not sure it matters which interface we get the userspace pointer from?

This patch is definitely changing the write from a 32-bit write to a
64-bit write and there's a specific check prior to the put_user() call
which checks that userspace intended a 32-bit value and presumably
provided a 32-bit pointer.

So I think the only way to return 64-bit AArch32 system register values
to userspace (if that is the intention) is to define a new ID for 64-bit
CCSIDR registers and handle them separately.

Thanks,
-Christoffer

[PATCH 2/2] arm64: kvm: upgrade csselr and ccsidr to 64-bit values

[Christoffer Dall]

On Tue, Jan 24, 2017 at 10:15:38AM +0000, Sudeep Holla wrote:
> 
> 
> On 23/01/17 21:08, Christoffer Dall wrote:
> > On Fri, Jan 20, 2017 at 10:50:10AM +0000, Sudeep Holla wrote:
> >> csselr and ccsidr are treated as 64-bit values already elsewhere in the
> >> kernel. It also aligns well with the architecture extensions that allow
> >> 64-bit format for ccsidr.
> >>
> >> This patch upgrades the existing accesses to csselr and ccsidr from
> >> 32-bit to 64-bit in preparation to add support to those extensions.
> >>
> >> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> >> Cc: Marc Zyngier <marc.zyngier@arm.com>
> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >> ---
> >>  arch/arm64/kvm/sys_regs.c | 18 +++++++++---------
> >>  1 file changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >> index 5dca1f10340f..a3559a8a2b0c 100644
> >> --- a/arch/arm64/kvm/sys_regs.c
> >> +++ b/arch/arm64/kvm/sys_regs.c
> 
> [..]
> 
> >> @@ -2004,8 +2004,8 @@ static int demux_c15_get(u64 id, void __user *uaddr)
> >>  
> >>  static int demux_c15_set(u64 id, void __user *uaddr)
> >>  {
> >> -	u32 val, newval;
> >> -	u32 __user *uval = uaddr;
> >> +	u64 val, newval;
> >> +	u64 __user *uval = uaddr;
> > 
> > Doesn't converting these uval pointers to u64 cause us to break the ABI
> > as we'll now be reading/writing 64-bit values to userspace with the
> > get_user and put_user following the declarations?
> > 
> 
> Yes, I too have similar concern. IIUC it is always read via kvm_one_reg
> structure. I could not find any specific user for this register to cross
> check.
> 

Not sure it matters which interface we get the userspace pointer from?

This patch is definitely changing the write from a 32-bit write to a
64-bit write and there's a specific check prior to the put_user() call
which checks that userspace intended a 32-bit value and presumably
provided a 32-bit pointer.

So I think the only way to return 64-bit AArch32 system register values
to userspace (if that is the intention) is to define a new ID for 64-bit
CCSIDR registers and handle them separately.

Thanks,
-Christoffer

Re: [PATCH 2/2] arm64: kvm: upgrade csselr and ccsidr to 64-bit values

[Sudeep Holla]

On 24/01/17 10:30, Christoffer Dall wrote:
> On Tue, Jan 24, 2017 at 10:15:38AM +0000, Sudeep Holla wrote:
>>
>>
>> On 23/01/17 21:08, Christoffer Dall wrote:
>>> On Fri, Jan 20, 2017 at 10:50:10AM +0000, Sudeep Holla wrote:
>>>> csselr and ccsidr are treated as 64-bit values already elsewhere in the
>>>> kernel. It also aligns well with the architecture extensions that allow
>>>> 64-bit format for ccsidr.
>>>>
>>>> This patch upgrades the existing accesses to csselr and ccsidr from
>>>> 32-bit to 64-bit in preparation to add support to those extensions.
>>>>
>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>>> ---
>>>>  arch/arm64/kvm/sys_regs.c | 18 +++++++++---------
>>>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>>> index 5dca1f10340f..a3559a8a2b0c 100644
>>>> --- a/arch/arm64/kvm/sys_regs.c
>>>> +++ b/arch/arm64/kvm/sys_regs.c
>>
>> [..]
>>
>>>> @@ -2004,8 +2004,8 @@ static int demux_c15_get(u64 id, void __user *uaddr)
>>>>  
>>>>  static int demux_c15_set(u64 id, void __user *uaddr)
>>>>  {
>>>> -	u32 val, newval;
>>>> -	u32 __user *uval = uaddr;
>>>> +	u64 val, newval;
>>>> +	u64 __user *uval = uaddr;
>>>
>>> Doesn't converting these uval pointers to u64 cause us to break the ABI
>>> as we'll now be reading/writing 64-bit values to userspace with the
>>> get_user and put_user following the declarations?
>>>
>>
>> Yes, I too have similar concern. IIUC it is always read via kvm_one_reg
>> structure. I could not find any specific user for this register to cross
>> check.
>>
> 
> Not sure it matters which interface we get the userspace pointer from?
> 

Agreed.

> This patch is definitely changing the write from a 32-bit write to a
> 64-bit write and there's a specific check prior to the put_user() call
> which checks that userspace intended a 32-bit value and presumably
> provided a 32-bit pointer.
> 

I see you point, I missed to see that check(just to be sure KVM_REG_SIZE
check right ?).

> So I think the only way to return 64-bit AArch32 system register values
> to userspace (if that is the intention) is to define a new ID for 64-bit
> CCSIDR registers and handle them separately.
> 

I will add KVM_REG_ARM_DEMUX_ID_CCSIDR_64B or something similar.
Thanks for the review.

-- 
Regards,
Sudeep

[PATCH 2/2] arm64: kvm: upgrade csselr and ccsidr to 64-bit values

[Sudeep Holla]

On 24/01/17 10:30, Christoffer Dall wrote:
> On Tue, Jan 24, 2017 at 10:15:38AM +0000, Sudeep Holla wrote:
>>
>>
>> On 23/01/17 21:08, Christoffer Dall wrote:
>>> On Fri, Jan 20, 2017 at 10:50:10AM +0000, Sudeep Holla wrote:
>>>> csselr and ccsidr are treated as 64-bit values already elsewhere in the
>>>> kernel. It also aligns well with the architecture extensions that allow
>>>> 64-bit format for ccsidr.
>>>>
>>>> This patch upgrades the existing accesses to csselr and ccsidr from
>>>> 32-bit to 64-bit in preparation to add support to those extensions.
>>>>
>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>>> ---
>>>>  arch/arm64/kvm/sys_regs.c | 18 +++++++++---------
>>>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>>> index 5dca1f10340f..a3559a8a2b0c 100644
>>>> --- a/arch/arm64/kvm/sys_regs.c
>>>> +++ b/arch/arm64/kvm/sys_regs.c
>>
>> [..]
>>
>>>> @@ -2004,8 +2004,8 @@ static int demux_c15_get(u64 id, void __user *uaddr)
>>>>  
>>>>  static int demux_c15_set(u64 id, void __user *uaddr)
>>>>  {
>>>> -	u32 val, newval;
>>>> -	u32 __user *uval = uaddr;
>>>> +	u64 val, newval;
>>>> +	u64 __user *uval = uaddr;
>>>
>>> Doesn't converting these uval pointers to u64 cause us to break the ABI
>>> as we'll now be reading/writing 64-bit values to userspace with the
>>> get_user and put_user following the declarations?
>>>
>>
>> Yes, I too have similar concern. IIUC it is always read via kvm_one_reg
>> structure. I could not find any specific user for this register to cross
>> check.
>>
> 
> Not sure it matters which interface we get the userspace pointer from?
> 

Agreed.

> This patch is definitely changing the write from a 32-bit write to a
> 64-bit write and there's a specific check prior to the put_user() call
> which checks that userspace intended a 32-bit value and presumably
> provided a 32-bit pointer.
> 

I see you point, I missed to see that check(just to be sure KVM_REG_SIZE
check right ?).

> So I think the only way to return 64-bit AArch32 system register values
> to userspace (if that is the intention) is to define a new ID for 64-bit
> CCSIDR registers and handle them separately.
> 

I will add KVM_REG_ARM_DEMUX_ID_CCSIDR_64B or something similar.
Thanks for the review.

-- 
Regards,
Sudeep

Re: [PATCH 2/2] arm64: kvm: upgrade csselr and ccsidr to 64-bit values

[Christoffer Dall]

On Tue, Jan 24, 2017 at 10:55:24AM +0000, Sudeep Holla wrote:
> 
> 
> On 24/01/17 10:30, Christoffer Dall wrote:
> > On Tue, Jan 24, 2017 at 10:15:38AM +0000, Sudeep Holla wrote:
> >>
> >>
> >> On 23/01/17 21:08, Christoffer Dall wrote:
> >>> On Fri, Jan 20, 2017 at 10:50:10AM +0000, Sudeep Holla wrote:
> >>>> csselr and ccsidr are treated as 64-bit values already elsewhere in the
> >>>> kernel. It also aligns well with the architecture extensions that allow
> >>>> 64-bit format for ccsidr.
> >>>>
> >>>> This patch upgrades the existing accesses to csselr and ccsidr from
> >>>> 32-bit to 64-bit in preparation to add support to those extensions.
> >>>>
> >>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> >>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
> >>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >>>> ---
> >>>>  arch/arm64/kvm/sys_regs.c | 18 +++++++++---------
> >>>>  1 file changed, 9 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >>>> index 5dca1f10340f..a3559a8a2b0c 100644
> >>>> --- a/arch/arm64/kvm/sys_regs.c
> >>>> +++ b/arch/arm64/kvm/sys_regs.c
> >>
> >> [..]
> >>
> >>>> @@ -2004,8 +2004,8 @@ static int demux_c15_get(u64 id, void __user *uaddr)
> >>>>  
> >>>>  static int demux_c15_set(u64 id, void __user *uaddr)
> >>>>  {
> >>>> -	u32 val, newval;
> >>>> -	u32 __user *uval = uaddr;
> >>>> +	u64 val, newval;
> >>>> +	u64 __user *uval = uaddr;
> >>>
> >>> Doesn't converting these uval pointers to u64 cause us to break the ABI
> >>> as we'll now be reading/writing 64-bit values to userspace with the
> >>> get_user and put_user following the declarations?
> >>>
> >>
> >> Yes, I too have similar concern. IIUC it is always read via kvm_one_reg
> >> structure. I could not find any specific user for this register to cross
> >> check.
> >>
> > 
> > Not sure it matters which interface we get the userspace pointer from?
> > 
> 
> Agreed.
> 
> > This patch is definitely changing the write from a 32-bit write to a
> > 64-bit write and there's a specific check prior to the put_user() call
> > which checks that userspace intended a 32-bit value and presumably
> > provided a 32-bit pointer.
> > 
> 
> I see you point, I missed to see that check(just to be sure KVM_REG_SIZE
> check right ?).

yes.


> 
> > So I think the only way to return 64-bit AArch32 system register values
> > to userspace (if that is the intention) is to define a new ID for 64-bit
> > CCSIDR registers and handle them separately.
> > 
> 
> I will add KVM_REG_ARM_DEMUX_ID_CCSIDR_64B or something similar.
> Thanks for the review.
> 

Cool, thanks.
-Christoffer

[PATCH 2/2] arm64: kvm: upgrade csselr and ccsidr to 64-bit values

[Christoffer Dall]

On Tue, Jan 24, 2017 at 10:55:24AM +0000, Sudeep Holla wrote:
> 
> 
> On 24/01/17 10:30, Christoffer Dall wrote:
> > On Tue, Jan 24, 2017 at 10:15:38AM +0000, Sudeep Holla wrote:
> >>
> >>
> >> On 23/01/17 21:08, Christoffer Dall wrote:
> >>> On Fri, Jan 20, 2017 at 10:50:10AM +0000, Sudeep Holla wrote:
> >>>> csselr and ccsidr are treated as 64-bit values already elsewhere in the
> >>>> kernel. It also aligns well with the architecture extensions that allow
> >>>> 64-bit format for ccsidr.
> >>>>
> >>>> This patch upgrades the existing accesses to csselr and ccsidr from
> >>>> 32-bit to 64-bit in preparation to add support to those extensions.
> >>>>
> >>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> >>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
> >>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >>>> ---
> >>>>  arch/arm64/kvm/sys_regs.c | 18 +++++++++---------
> >>>>  1 file changed, 9 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >>>> index 5dca1f10340f..a3559a8a2b0c 100644
> >>>> --- a/arch/arm64/kvm/sys_regs.c
> >>>> +++ b/arch/arm64/kvm/sys_regs.c
> >>
> >> [..]
> >>
> >>>> @@ -2004,8 +2004,8 @@ static int demux_c15_get(u64 id, void __user *uaddr)
> >>>>  
> >>>>  static int demux_c15_set(u64 id, void __user *uaddr)
> >>>>  {
> >>>> -	u32 val, newval;
> >>>> -	u32 __user *uval = uaddr;
> >>>> +	u64 val, newval;
> >>>> +	u64 __user *uval = uaddr;
> >>>
> >>> Doesn't converting these uval pointers to u64 cause us to break the ABI
> >>> as we'll now be reading/writing 64-bit values to userspace with the
> >>> get_user and put_user following the declarations?
> >>>
> >>
> >> Yes, I too have similar concern. IIUC it is always read via kvm_one_reg
> >> structure. I could not find any specific user for this register to cross
> >> check.
> >>
> > 
> > Not sure it matters which interface we get the userspace pointer from?
> > 
> 
> Agreed.
> 
> > This patch is definitely changing the write from a 32-bit write to a
> > 64-bit write and there's a specific check prior to the put_user() call
> > which checks that userspace intended a 32-bit value and presumably
> > provided a 32-bit pointer.
> > 
> 
> I see you point, I missed to see that check(just to be sure KVM_REG_SIZE
> check right ?).

yes.


> 
> > So I think the only way to return 64-bit AArch32 system register values
> > to userspace (if that is the intention) is to define a new ID for 64-bit
> > CCSIDR registers and handle them separately.
> > 
> 
> I will add KVM_REG_ARM_DEMUX_ID_CCSIDR_64B or something similar.
> Thanks for the review.
> 

Cool, thanks.
-Christoffer