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@xxxxxxx>
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Cc: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
> Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
> ---
> 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
