[PATCH v3 0/5] arm64: Patching branches for fun and profit

[PATCH v3 0/5] arm64: Patching branches for fun and profit

[Marc Zyngier]

The current alternative instruction framework is not kind to branches,
potentially leading to all kind of hacks in the code that uses
alternatives. This series expands it to deal with immediate branches
(for a start), and applies it to the VGIC world switch.

Patch #1 adds the required infrastructure to extract the immediate
from an instruction.

Patch #2 allows the use of an immediate b or bl instruction as an
alternative, computing the target branch as the instruction is being
patched in.

Patch #3 defines a feature framework that works exactly like the CPU
errata infrastructure (and shares a lot with it).

Patch #4 adds detection of the system register GICv3 CPU interface.

Patch #5 enables dynamic patching of the KVM code.

This has been tested with GICv3 on a FastModel.

* From v1:
- Fixed missing cpu_to_le32 when writing back the instruction
- Added Will's Acks.

* From v2:
- Replaced cpu_to_le32 with aarch_write_insn, which is the proper API
  and takes care of the potential pitfall if the text is read-only.
  Thanks to Tixy for suggesting this.

Marc Zyngier (5):
  arm64: insn: Add aarch64_insn_decode_immediate
  arm64: alternative: Allow immediate branch as alternative instruction
  arm64: Extract feature parsing code from cpu_errata.c
  arm64: alternative: Introduce feature for GICv3 CPU interface
  arm64: KVM: Switch vgic save/restore to alternative_insn

 arch/arm/include/asm/kvm_host.h     |  5 ---
 arch/arm64/include/asm/cpufeature.h | 23 ++++++++++-
 arch/arm64/include/asm/insn.h       |  1 +
 arch/arm64/include/asm/kvm_host.h   | 23 -----------
 arch/arm64/kernel/Makefile          |  2 +-
 arch/arm64/kernel/alternative.c     | 55 ++++++++++++++++++++++++-
 arch/arm64/kernel/asm-offsets.c     |  1 -
 arch/arm64/kernel/cpu_errata.c      | 36 ++---------------
 arch/arm64/kernel/cpufeature.c      | 63 +++++++++++++++++++++++++++++
 arch/arm64/kernel/cpuinfo.c         |  1 +
 arch/arm64/kernel/insn.c            | 81 +++++++++++++++++++++++++++++--------
 arch/arm64/kvm/hyp.S                | 18 ++-------
 virt/kvm/arm/vgic.c                 |  3 --
 13 files changed, 214 insertions(+), 98 deletions(-)
 create mode 100644 arch/arm64/kernel/cpufeature.c

-- 
2.1.4

[PATCH v3 1/5] arm64: insn: Add aarch64_insn_decode_immediate

[Marc Zyngier]

Patching an instruction sometimes requires extracting the immediate
field from this instruction. To facilitate this, and avoid
potential duplication of code, add aarch64_insn_decode_immediate
as the reciprocal to aarch64_insn_encode_immediate.

Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/insn.h |  1 +
 arch/arm64/kernel/insn.c      | 81 ++++++++++++++++++++++++++++++++++---------
 2 files changed, 66 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index d2f4942..f81b328 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -285,6 +285,7 @@ bool aarch64_insn_is_nop(u32 insn);
 int aarch64_insn_read(void *addr, u32 *insnp);
 int aarch64_insn_write(void *addr, u32 insn);
 enum aarch64_insn_encoding_class aarch64_get_insn_class(u32 insn);
+u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn);
 u32 aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
 				  u32 insn, u64 imm);
 u32 aarch64_insn_gen_branch_imm(unsigned long pc, unsigned long addr,
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index c8eca88..9249020 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -265,23 +265,13 @@ int __kprobes aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt)
 	return aarch64_insn_patch_text_sync(addrs, insns, cnt);
 }
 
-u32 __kprobes aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
-				  u32 insn, u64 imm)
+static int __kprobes aarch64_get_imm_shift_mask(enum aarch64_insn_imm_type type,
+						u32 *maskp, int *shiftp)
 {
-	u32 immlo, immhi, lomask, himask, mask;
+	u32 mask;
 	int shift;
 
 	switch (type) {
-	case AARCH64_INSN_IMM_ADR:
-		lomask = 0x3;
-		himask = 0x7ffff;
-		immlo = imm & lomask;
-		imm >>= 2;
-		immhi = imm & himask;
-		imm = (immlo << 24) | (immhi);
-		mask = (lomask << 24) | (himask);
-		shift = 5;
-		break;
 	case AARCH64_INSN_IMM_26:
 		mask = BIT(26) - 1;
 		shift = 0;
@@ -320,9 +310,68 @@ u32 __kprobes aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
 		shift = 16;
 		break;
 	default:
-		pr_err("aarch64_insn_encode_immediate: unknown immediate encoding %d\n",
-			type);
-		return 0;
+		return -EINVAL;
+	}
+
+	*maskp = mask;
+	*shiftp = shift;
+
+	return 0;
+}
+
+#define ADR_IMM_HILOSPLIT	2
+#define ADR_IMM_SIZE		SZ_2M
+#define ADR_IMM_LOMASK		((1 << ADR_IMM_HILOSPLIT) - 1)
+#define ADR_IMM_HIMASK		((ADR_IMM_SIZE >> ADR_IMM_HILOSPLIT) - 1)
+#define ADR_IMM_LOSHIFT		29
+#define ADR_IMM_HISHIFT		5
+
+u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn)
+{
+	u32 immlo, immhi, mask;
+	int shift;
+
+	switch (type) {
+	case AARCH64_INSN_IMM_ADR:
+		shift = 0;
+		immlo = (insn >> ADR_IMM_LOSHIFT) & ADR_IMM_LOMASK;
+		immhi = (insn >> ADR_IMM_HISHIFT) & ADR_IMM_HIMASK;
+		insn = (immhi << ADR_IMM_HILOSPLIT) | immlo;
+		mask = ADR_IMM_SIZE - 1;
+		break;
+	default:
+		if (aarch64_get_imm_shift_mask(type, &mask, &shift) < 0) {
+			pr_err("aarch64_insn_decode_immediate: unknown immediate encoding %d\n",
+			       type);
+			return 0;
+		}
+	}
+
+	return (insn >> shift) & mask;
+}
+
+u32 __kprobes aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
+				  u32 insn, u64 imm)
+{
+	u32 immlo, immhi, mask;
+	int shift;
+
+	switch (type) {
+	case AARCH64_INSN_IMM_ADR:
+		shift = 0;
+		immlo = (imm & ADR_IMM_LOMASK) << ADR_IMM_LOSHIFT;
+		imm >>= ADR_IMM_HILOSPLIT;
+		immhi = (imm & ADR_IMM_HIMASK) << ADR_IMM_HISHIFT;
+		imm = immlo | immhi;
+		mask = ((ADR_IMM_LOMASK << ADR_IMM_LOSHIFT) |
+			(ADR_IMM_HIMASK << ADR_IMM_HISHIFT));
+		break;
+	default:
+		if (aarch64_get_imm_shift_mask(type, &mask, &shift) < 0) {
+			pr_err("aarch64_insn_encode_immediate: unknown immediate encoding %d\n",
+			       type);
+			return 0;
+		}
 	}
 
 	/* Update the immediate field. */
-- 
2.1.4

[PATCH v3 2/5] arm64: alternative: Allow immediate branch as alternative instruction

[Marc Zyngier]

Since all immediate branches are PC-relative on Aarch64, these
instructions cannot be used as an alternative with the simplistic
approach we currently have (the immediate has been computed from
the .altinstr_replacement section, and end-up being completely off
if we insert it directly).

This patch handles the b and bl instructions in a different way,
using the insn framework to recompute the immediate, and generate
the right displacement.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kernel/alternative.c | 55 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index ad7821d..21033bb 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -24,6 +24,7 @@
 #include <asm/cacheflush.h>
 #include <asm/alternative.h>
 #include <asm/cpufeature.h>
+#include <asm/insn.h>
 #include <linux/stop_machine.h>
 
 extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
@@ -33,6 +34,48 @@ struct alt_region {
 	struct alt_instr *end;
 };
 
+/*
+ * Decode the imm field of a b/bl instruction, and return the byte
+ * offset as a signed value (so it can be used when computing a new
+ * branch target).
+ */
+static s32 get_branch_offset(u32 insn)
+{
+	s32 imm = aarch64_insn_decode_immediate(AARCH64_INSN_IMM_26, insn);
+
+	/* sign-extend the immediate before turning it into a byte offset */
+	return (imm << 6) >> 4;
+}
+
+static u32 get_alt_insn(u8 *insnptr, u8 *altinsnptr)
+{
+	u32 insn;
+
+	aarch64_insn_read(altinsnptr, &insn);
+
+	/* Stop the world on instructions we don't support... */
+	BUG_ON(aarch64_insn_is_cbz(insn));
+	BUG_ON(aarch64_insn_is_cbnz(insn));
+	BUG_ON(aarch64_insn_is_bcond(insn));
+	/* ... and there is probably more. */
+
+	if (aarch64_insn_is_b(insn) || aarch64_insn_is_bl(insn)) {
+		enum aarch64_insn_branch_type type;
+		unsigned long target;
+
+		if (aarch64_insn_is_b(insn))
+			type = AARCH64_INSN_BRANCH_NOLINK;
+		else
+			type = AARCH64_INSN_BRANCH_LINK;
+
+		target = (unsigned long)altinsnptr + get_branch_offset(insn);
+		insn = aarch64_insn_gen_branch_imm((unsigned long)insnptr,
+						   target, type);
+	}
+
+	return insn;
+}
+
 static int __apply_alternatives(void *alt_region)
 {
 	struct alt_instr *alt;
@@ -40,16 +83,24 @@ static int __apply_alternatives(void *alt_region)
 	u8 *origptr, *replptr;
 
 	for (alt = region->begin; alt < region->end; alt++) {
+		u32 insn;
+		int i;
+
 		if (!cpus_have_cap(alt->cpufeature))
 			continue;
 
-		BUG_ON(alt->alt_len > alt->orig_len);
+		BUG_ON(alt->alt_len != alt->orig_len);
 
 		pr_info_once("patching kernel code\n");
 
 		origptr = (u8 *)&alt->orig_offset + alt->orig_offset;
 		replptr = (u8 *)&alt->alt_offset + alt->alt_offset;
-		memcpy(origptr, replptr, alt->alt_len);
+
+		for (i = 0; i < alt->alt_len; i += sizeof(insn)) {
+			insn = get_alt_insn(origptr + i, replptr + i);
+			aarch64_insn_write(origptr + i, insn);
+		}
+
 		flush_icache_range((uintptr_t)origptr,
 				   (uintptr_t)(origptr + alt->alt_len));
 	}
-- 
2.1.4

[PATCH v3 3/5] arm64: Extract feature parsing code from cpu_errata.c

[Marc Zyngier]

As we detect more architectural features at runtime, it makes
sense to reuse the existing framework whilst avoiding to call
a feature an erratum...

This patch extract the core capability parsing, moves it into
a new file (cpufeature.c), and let the CPU errata detection code
use it.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/cpufeature.h | 15 ++++++++++++
 arch/arm64/kernel/Makefile          |  2 +-
 arch/arm64/kernel/cpu_errata.c      | 36 ++++------------------------
 arch/arm64/kernel/cpufeature.c      | 47 +++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/cpuinfo.c         |  1 +
 5 files changed, 68 insertions(+), 33 deletions(-)
 create mode 100644 arch/arm64/kernel/cpufeature.c

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index b6c16d5..6ae35d1 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -28,6 +28,18 @@
 
 #ifndef __ASSEMBLY__
 
+struct arm64_cpu_capabilities {
+	const char *desc;
+	u16 capability;
+	bool (*matches)(const struct arm64_cpu_capabilities *);
+	union {
+		struct {	/* To be used for erratum handling only */
+			u32 midr_model;
+			u32 midr_range_min, midr_range_max;
+		};
+	};
+};
+
 extern DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
 
 static inline bool cpu_have_feature(unsigned int num)
@@ -51,7 +63,10 @@ static inline void cpus_set_cap(unsigned int num)
 		__set_bit(num, cpu_hwcaps);
 }
 
+void check_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
+			    const char *info);
 void check_local_cpu_errata(void);
+void check_local_cpu_features(void);
 bool cpu_supports_mixed_endian_el0(void);
 bool system_supports_mixed_endian_el0(void);
 
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index d5e7074..b12e15b 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -17,7 +17,7 @@ arm64-obj-y		:= debug-monitors.o entry.o irq.o fpsimd.o		\
 			   sys.o stacktrace.o time.o traps.o io.o vdso.o	\
 			   hyp-stub.o psci.o psci-call.o cpu_ops.o insn.o	\
 			   return_address.o cpuinfo.o cpu_errata.o		\
-			   alternative.o cacheinfo.o
+			   cpufeature.o alternative.o cacheinfo.o
 
 arm64-obj-$(CONFIG_COMPAT)		+= sys32.o kuser32.o signal32.o 	\
 					   sys_compat.o entry32.o		\
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index fa62637..a66f4fa 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -16,8 +16,6 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#define pr_fmt(fmt) "alternatives: " fmt
-
 #include <linux/types.h>
 #include <asm/cpu.h>
 #include <asm/cputype.h>
@@ -26,27 +24,11 @@
 #define MIDR_CORTEX_A53 MIDR_CPU_PART(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A53)
 #define MIDR_CORTEX_A57 MIDR_CPU_PART(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A57)
 
-/*
- * Add a struct or another datatype to the union below if you need
- * different means to detect an affected CPU.
- */
-struct arm64_cpu_capabilities {
-	const char *desc;
-	u16 capability;
-	bool (*is_affected)(struct arm64_cpu_capabilities *);
-	union {
-		struct {
-			u32 midr_model;
-			u32 midr_range_min, midr_range_max;
-		};
-	};
-};
-
 #define CPU_MODEL_MASK (MIDR_IMPLEMENTOR_MASK | MIDR_PARTNUM_MASK | \
 			MIDR_ARCHITECTURE_MASK)
 
 static bool __maybe_unused
-is_affected_midr_range(struct arm64_cpu_capabilities *entry)
+is_affected_midr_range(const struct arm64_cpu_capabilities *entry)
 {
 	u32 midr = read_cpuid_id();
 
@@ -59,12 +41,12 @@ is_affected_midr_range(struct arm64_cpu_capabilities *entry)
 }
 
 #define MIDR_RANGE(model, min, max) \
-	.is_affected = is_affected_midr_range, \
+	.matches = is_affected_midr_range, \
 	.midr_model = model, \
 	.midr_range_min = min, \
 	.midr_range_max = max
 
-struct arm64_cpu_capabilities arm64_errata[] = {
+const struct arm64_cpu_capabilities arm64_errata[] = {
 #if	defined(CONFIG_ARM64_ERRATUM_826319) || \
 	defined(CONFIG_ARM64_ERRATUM_827319) || \
 	defined(CONFIG_ARM64_ERRATUM_824069)
@@ -97,15 +79,5 @@ struct arm64_cpu_capabilities arm64_errata[] = {
 
 void check_local_cpu_errata(void)
 {
-	struct arm64_cpu_capabilities *cpus = arm64_errata;
-	int i;
-
-	for (i = 0; cpus[i].desc; i++) {
-		if (!cpus[i].is_affected(&cpus[i]))
-			continue;
-
-		if (!cpus_have_cap(cpus[i].capability))
-			pr_info("enabling workaround for %s\n", cpus[i].desc);
-		cpus_set_cap(cpus[i].capability);
-	}
+	check_cpu_capabilities(arm64_errata, "enabling workaround for");
 }
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
new file mode 100644
index 0000000..3d9967e
--- /dev/null
+++ b/arch/arm64/kernel/cpufeature.c
@@ -0,0 +1,47 @@
+/*
+ * Contains CPU feature definitions
+ *
+ * Copyright (C) 2015 ARM Ltd.
+ *
+ * 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/>.
+ */
+
+#define pr_fmt(fmt) "alternatives: " fmt
+
+#include <linux/types.h>
+#include <asm/cpu.h>
+#include <asm/cpufeature.h>
+
+static const struct arm64_cpu_capabilities arm64_features[] = {
+	{},
+};
+
+void check_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
+			    const char *info)
+{
+	int i;
+
+	for (i = 0; caps[i].desc; i++) {
+		if (!caps[i].matches(&caps[i]))
+			continue;
+
+		if (!cpus_have_cap(caps[i].capability))
+			pr_info("%s %s\n", info, caps[i].desc);
+		cpus_set_cap(caps[i].capability);
+	}
+}
+
+void check_local_cpu_features(void)
+{
+	check_cpu_capabilities(arm64_features, "detected feature");
+}
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 9298556..75d5a86 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -236,6 +236,7 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
 	cpuinfo_detect_icache_policy(info);
 
 	check_local_cpu_errata();
+	check_local_cpu_features();
 	update_cpu_features(info);
 }
 
-- 
2.1.4

[PATCH v3 4/5] arm64: alternative: Introduce feature for GICv3 CPU interface

[Marc Zyngier]

Add a new item to the feature set (ARM64_HAS_SYSREG_GIC_CPUIF)
to indicate that we have a system register GIC CPU interface

This will help KVM switching to alternative instruction patching.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/cpufeature.h |  8 +++++++-
 arch/arm64/kernel/cpufeature.c      | 16 ++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 6ae35d1..d9e57b5 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -23,8 +23,9 @@
 
 #define ARM64_WORKAROUND_CLEAN_CACHE		0
 #define ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE	1
+#define ARM64_HAS_SYSREG_GIC_CPUIF		2
 
-#define ARM64_NCAPS				2
+#define ARM64_NCAPS				3
 
 #ifndef __ASSEMBLY__
 
@@ -37,6 +38,11 @@ struct arm64_cpu_capabilities {
 			u32 midr_model;
 			u32 midr_range_min, midr_range_max;
 		};
+
+		struct {	/* Feature register checking */
+			u64 register_mask;
+			u64 register_value;
+		};
 	};
 };
 
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 3d9967e..b0bea2b3 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -22,7 +22,23 @@
 #include <asm/cpu.h>
 #include <asm/cpufeature.h>
 
+static bool
+has_id_aa64pfr0_feature(const struct arm64_cpu_capabilities *entry)
+{
+	u64 val;
+
+	val = read_cpuid(id_aa64pfr0_el1);
+	return (val & entry->register_mask) == entry->register_value;
+}
+
 static const struct arm64_cpu_capabilities arm64_features[] = {
+	{
+		.desc = "system register GIC CPU interface",
+		.capability = ARM64_HAS_SYSREG_GIC_CPUIF,
+		.matches = has_id_aa64pfr0_feature,
+		.register_mask = (0xf << 24),
+		.register_value = (1 << 24),
+	},
 	{},
 };
 
-- 
2.1.4

[PATCH v3 4/5] arm64: alternative: Introduce feature for GICv3 CPU interface

[Christoffer Dall]

On Fri, Mar 27, 2015 at 01:09:24PM +0000, Marc Zyngier wrote:
> Add a new item to the feature set (ARM64_HAS_SYSREG_GIC_CPUIF)
> to indicate that we have a system register GIC CPU interface
> 
> This will help KVM switching to alternative instruction patching.
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> Acked-by: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/include/asm/cpufeature.h |  8 +++++++-
>  arch/arm64/kernel/cpufeature.c      | 16 ++++++++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 6ae35d1..d9e57b5 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -23,8 +23,9 @@
>  
>  #define ARM64_WORKAROUND_CLEAN_CACHE		0
>  #define ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE	1
> +#define ARM64_HAS_SYSREG_GIC_CPUIF		2
>  
> -#define ARM64_NCAPS				2
> +#define ARM64_NCAPS				3
>  
>  #ifndef __ASSEMBLY__
>  
> @@ -37,6 +38,11 @@ struct arm64_cpu_capabilities {
>  			u32 midr_model;
>  			u32 midr_range_min, midr_range_max;
>  		};
> +
> +		struct {	/* Feature register checking */
> +			u64 register_mask;
> +			u64 register_value;
> +		};
>  	};
>  };
>  
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 3d9967e..b0bea2b3 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -22,7 +22,23 @@
>  #include <asm/cpu.h>
>  #include <asm/cpufeature.h>
>  
> +static bool
> +has_id_aa64pfr0_feature(const struct arm64_cpu_capabilities *entry)
> +{
> +	u64 val;
> +
> +	val = read_cpuid(id_aa64pfr0_el1);

is this preferred compared to fishing it out of cpuinfo ?

> +	return (val & entry->register_mask) == entry->register_value;
> +}
> +
>  static const struct arm64_cpu_capabilities arm64_features[] = {
> +	{
> +		.desc = "system register GIC CPU interface",
> +		.capability = ARM64_HAS_SYSREG_GIC_CPUIF,
> +		.matches = has_id_aa64pfr0_feature,
> +		.register_mask = (0xf << 24),
> +		.register_value = (1 << 24),

I don't know if it's worth defining these masks in some header file.
The only other place I could see them used was in head.S.

> +	},
>  	{},
>  };
>  
> -- 
> 2.1.4
> 

Besides the nits, this looks good to me.

-Christoffer

[PATCH v3 4/5] arm64: alternative: Introduce feature for GICv3 CPU interface

[Marc Zyngier]

On 14/05/15 12:25, Christoffer Dall wrote:
> On Fri, Mar 27, 2015 at 01:09:24PM +0000, Marc Zyngier wrote:
>> Add a new item to the feature set (ARM64_HAS_SYSREG_GIC_CPUIF)
>> to indicate that we have a system register GIC CPU interface
>>
>> This will help KVM switching to alternative instruction patching.
>>
>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>> Acked-by: Will Deacon <will.deacon@arm.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/include/asm/cpufeature.h |  8 +++++++-
>>  arch/arm64/kernel/cpufeature.c      | 16 ++++++++++++++++
>>  2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>> index 6ae35d1..d9e57b5 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -23,8 +23,9 @@
>>  
>>  #define ARM64_WORKAROUND_CLEAN_CACHE		0
>>  #define ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE	1
>> +#define ARM64_HAS_SYSREG_GIC_CPUIF		2
>>  
>> -#define ARM64_NCAPS				2
>> +#define ARM64_NCAPS				3
>>  
>>  #ifndef __ASSEMBLY__
>>  
>> @@ -37,6 +38,11 @@ struct arm64_cpu_capabilities {
>>  			u32 midr_model;
>>  			u32 midr_range_min, midr_range_max;
>>  		};
>> +
>> +		struct {	/* Feature register checking */
>> +			u64 register_mask;
>> +			u64 register_value;
>> +		};
>>  	};
>>  };
>>  
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 3d9967e..b0bea2b3 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -22,7 +22,23 @@
>>  #include <asm/cpu.h>
>>  #include <asm/cpufeature.h>
>>  
>> +static bool
>> +has_id_aa64pfr0_feature(const struct arm64_cpu_capabilities *entry)
>> +{
>> +	u64 val;
>> +
>> +	val = read_cpuid(id_aa64pfr0_el1);
> 
> is this preferred compared to fishing it out of cpuinfo ?

Probably for the moment, yes. At some point, we should be able to have a
consolidated set of features, consistent across all CPUs in the system.
Once we have that, we should revisit this detection mecanism.

>> +	return (val & entry->register_mask) == entry->register_value;
>> +}
>> +
>>  static const struct arm64_cpu_capabilities arm64_features[] = {
>> +	{
>> +		.desc = "system register GIC CPU interface",
>> +		.capability = ARM64_HAS_SYSREG_GIC_CPUIF,
>> +		.matches = has_id_aa64pfr0_feature,
>> +		.register_mask = (0xf << 24),
>> +		.register_value = (1 << 24),
> 
> I don't know if it's worth defining these masks in some header file.
> The only other place I could see them used was in head.S.

Mark was looking at this a while ago. Maybe a task for a sleepless
night? ;-)

Thanks,

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

[PATCH v3 4/5] arm64: alternative: Introduce feature for GICv3 CPU interface

[Christoffer Dall]

On Thu, May 28, 2015 at 10:27:14AM +0100, Marc Zyngier wrote:
> On 14/05/15 12:25, Christoffer Dall wrote:
> > On Fri, Mar 27, 2015 at 01:09:24PM +0000, Marc Zyngier wrote:
> >> Add a new item to the feature set (ARM64_HAS_SYSREG_GIC_CPUIF)
> >> to indicate that we have a system register GIC CPU interface
> >>
> >> This will help KVM switching to alternative instruction patching.
> >>
> >> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> >> Acked-by: Will Deacon <will.deacon@arm.com>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  arch/arm64/include/asm/cpufeature.h |  8 +++++++-
> >>  arch/arm64/kernel/cpufeature.c      | 16 ++++++++++++++++
> >>  2 files changed, 23 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> >> index 6ae35d1..d9e57b5 100644
> >> --- a/arch/arm64/include/asm/cpufeature.h
> >> +++ b/arch/arm64/include/asm/cpufeature.h
> >> @@ -23,8 +23,9 @@
> >>  
> >>  #define ARM64_WORKAROUND_CLEAN_CACHE		0
> >>  #define ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE	1
> >> +#define ARM64_HAS_SYSREG_GIC_CPUIF		2
> >>  
> >> -#define ARM64_NCAPS				2
> >> +#define ARM64_NCAPS				3
> >>  
> >>  #ifndef __ASSEMBLY__
> >>  
> >> @@ -37,6 +38,11 @@ struct arm64_cpu_capabilities {
> >>  			u32 midr_model;
> >>  			u32 midr_range_min, midr_range_max;
> >>  		};
> >> +
> >> +		struct {	/* Feature register checking */
> >> +			u64 register_mask;
> >> +			u64 register_value;
> >> +		};
> >>  	};
> >>  };
> >>  
> >> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> >> index 3d9967e..b0bea2b3 100644
> >> --- a/arch/arm64/kernel/cpufeature.c
> >> +++ b/arch/arm64/kernel/cpufeature.c
> >> @@ -22,7 +22,23 @@
> >>  #include <asm/cpu.h>
> >>  #include <asm/cpufeature.h>
> >>  
> >> +static bool
> >> +has_id_aa64pfr0_feature(const struct arm64_cpu_capabilities *entry)
> >> +{
> >> +	u64 val;
> >> +
> >> +	val = read_cpuid(id_aa64pfr0_el1);
> > 
> > is this preferred compared to fishing it out of cpuinfo ?
> 
> Probably for the moment, yes. At some point, we should be able to have a
> consolidated set of features, consistent across all CPUs in the system.
> Once we have that, we should revisit this detection mecanism.
> 
> >> +	return (val & entry->register_mask) == entry->register_value;
> >> +}
> >> +
> >>  static const struct arm64_cpu_capabilities arm64_features[] = {
> >> +	{
> >> +		.desc = "system register GIC CPU interface",
> >> +		.capability = ARM64_HAS_SYSREG_GIC_CPUIF,
> >> +		.matches = has_id_aa64pfr0_feature,
> >> +		.register_mask = (0xf << 24),
> >> +		.register_value = (1 << 24),
> > 
> > I don't know if it's worth defining these masks in some header file.
> > The only other place I could see them used was in head.S.
> 
> Mark was looking at this a while ago. Maybe a task for a sleepless
> night? ;-)
> 

yeah, you can add this to the patch if it helps:

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

-Christoffer

[PATCH v3 4/5] arm64: alternative: Introduce feature for GICv3 CPU interface

[Marc Zyngier]

On 28/05/15 14:02, Christoffer Dall wrote:
> On Thu, May 28, 2015 at 10:27:14AM +0100, Marc Zyngier wrote:
>> On 14/05/15 12:25, Christoffer Dall wrote:
>>> On Fri, Mar 27, 2015 at 01:09:24PM +0000, Marc Zyngier wrote:
>>>> Add a new item to the feature set (ARM64_HAS_SYSREG_GIC_CPUIF)
>>>> to indicate that we have a system register GIC CPU interface
>>>>
>>>> This will help KVM switching to alternative instruction patching.
>>>>
>>>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>>>> Acked-by: Will Deacon <will.deacon@arm.com>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>  arch/arm64/include/asm/cpufeature.h |  8 +++++++-
>>>>  arch/arm64/kernel/cpufeature.c      | 16 ++++++++++++++++
>>>>  2 files changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>>>> index 6ae35d1..d9e57b5 100644
>>>> --- a/arch/arm64/include/asm/cpufeature.h
>>>> +++ b/arch/arm64/include/asm/cpufeature.h
>>>> @@ -23,8 +23,9 @@
>>>>  
>>>>  #define ARM64_WORKAROUND_CLEAN_CACHE		0
>>>>  #define ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE	1
>>>> +#define ARM64_HAS_SYSREG_GIC_CPUIF		2
>>>>  
>>>> -#define ARM64_NCAPS				2
>>>> +#define ARM64_NCAPS				3
>>>>  
>>>>  #ifndef __ASSEMBLY__
>>>>  
>>>> @@ -37,6 +38,11 @@ struct arm64_cpu_capabilities {
>>>>  			u32 midr_model;
>>>>  			u32 midr_range_min, midr_range_max;
>>>>  		};
>>>> +
>>>> +		struct {	/* Feature register checking */
>>>> +			u64 register_mask;
>>>> +			u64 register_value;
>>>> +		};
>>>>  	};
>>>>  };
>>>>  
>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>>> index 3d9967e..b0bea2b3 100644
>>>> --- a/arch/arm64/kernel/cpufeature.c
>>>> +++ b/arch/arm64/kernel/cpufeature.c
>>>> @@ -22,7 +22,23 @@
>>>>  #include <asm/cpu.h>
>>>>  #include <asm/cpufeature.h>
>>>>  
>>>> +static bool
>>>> +has_id_aa64pfr0_feature(const struct arm64_cpu_capabilities *entry)
>>>> +{
>>>> +	u64 val;
>>>> +
>>>> +	val = read_cpuid(id_aa64pfr0_el1);
>>>
>>> is this preferred compared to fishing it out of cpuinfo ?
>>
>> Probably for the moment, yes. At some point, we should be able to have a
>> consolidated set of features, consistent across all CPUs in the system.
>> Once we have that, we should revisit this detection mecanism.
>>
>>>> +	return (val & entry->register_mask) == entry->register_value;
>>>> +}
>>>> +
>>>>  static const struct arm64_cpu_capabilities arm64_features[] = {
>>>> +	{
>>>> +		.desc = "system register GIC CPU interface",
>>>> +		.capability = ARM64_HAS_SYSREG_GIC_CPUIF,
>>>> +		.matches = has_id_aa64pfr0_feature,
>>>> +		.register_mask = (0xf << 24),
>>>> +		.register_value = (1 << 24),
>>>
>>> I don't know if it's worth defining these masks in some header file.
>>> The only other place I could see them used was in head.S.
>>
>> Mark was looking at this a while ago. Maybe a task for a sleepless
>> night? ;-)
>>
> 
> yeah, you can add this to the patch if it helps:
> 
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

Thanks. I'll look at getting these two patches in once the dependencies
are merged.

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

[PATCH v3 5/5] arm64: KVM: Switch vgic save/restore to alternative_insn

[Marc Zyngier]

So far, we configured the world-switch by having a small array
of pointers to the save and restore functions, depending on the
GIC used on the platform.

Loading these values each time is a bit silly (they never change),
and it makes sense to rely on the instruction patching instead.

This leads to a nice cleanup of the code.

Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_host.h   |  5 -----
 arch/arm64/include/asm/kvm_host.h | 23 -----------------------
 arch/arm64/kernel/asm-offsets.c   |  1 -
 arch/arm64/kvm/hyp.S              | 18 ++++--------------
 virt/kvm/arm/vgic.c               |  3 ---
 5 files changed, 4 insertions(+), 46 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 41008cd..2fcad3b 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -225,11 +225,6 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
 	return 0;
 }
 
-static inline void vgic_arch_setup(const struct vgic_params *vgic)
-{
-	BUG_ON(vgic->type != VGIC_V2);
-}
-
 int kvm_perf_init(void);
 int kvm_perf_teardown(void);
 
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 8ac3c70..b0aa3a4 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -228,29 +228,6 @@ struct vgic_sr_vectors {
 	void	*restore_vgic;
 };
 
-static inline void vgic_arch_setup(const struct vgic_params *vgic)
-{
-	extern struct vgic_sr_vectors __vgic_sr_vectors;
-
-	switch(vgic->type)
-	{
-	case VGIC_V2:
-		__vgic_sr_vectors.save_vgic	= __save_vgic_v2_state;
-		__vgic_sr_vectors.restore_vgic	= __restore_vgic_v2_state;
-		break;
-
-#ifdef CONFIG_ARM_GIC_V3
-	case VGIC_V3:
-		__vgic_sr_vectors.save_vgic	= __save_vgic_v3_state;
-		__vgic_sr_vectors.restore_vgic	= __restore_vgic_v3_state;
-		break;
-#endif
-
-	default:
-		BUG();
-	}
-}
-
 static inline void kvm_arch_hardware_disable(void) {}
 static inline void kvm_arch_hardware_unsetup(void) {}
 static inline void kvm_arch_sync_events(struct kvm *kvm) {}
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 14dd3d1..cf9d97b 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -128,7 +128,6 @@ int main(void)
   DEFINE(VCPU_VGIC_CPU,		offsetof(struct kvm_vcpu, arch.vgic_cpu));
   DEFINE(VGIC_SAVE_FN,		offsetof(struct vgic_sr_vectors, save_vgic));
   DEFINE(VGIC_RESTORE_FN,	offsetof(struct vgic_sr_vectors, restore_vgic));
-  DEFINE(VGIC_SR_VECTOR_SZ,	sizeof(struct vgic_sr_vectors));
   DEFINE(VGIC_V2_CPU_HCR,	offsetof(struct vgic_cpu, vgic_v2.vgic_hcr));
   DEFINE(VGIC_V2_CPU_VMCR,	offsetof(struct vgic_cpu, vgic_v2.vgic_vmcr));
   DEFINE(VGIC_V2_CPU_MISR,	offsetof(struct vgic_cpu, vgic_v2.vgic_misr));
diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index 5befd01..0d55a53 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -17,8 +17,10 @@
 
 #include <linux/linkage.h>
 
+#include <asm/alternative-asm.h>
 #include <asm/asm-offsets.h>
 #include <asm/assembler.h>
+#include <asm/cpufeature.h>
 #include <asm/debug-monitors.h>
 #include <asm/esr.h>
 #include <asm/fpsimdmacros.h>
@@ -808,10 +810,7 @@
  * Call into the vgic backend for state saving
  */
 .macro save_vgic_state
-	adr	x24, __vgic_sr_vectors
-	ldr	x24, [x24, VGIC_SAVE_FN]
-	kern_hyp_va	x24
-	blr	x24
+	alternative_insn "bl __save_vgic_v2_state", "bl __save_vgic_v3_state", ARM64_HAS_SYSREG_GIC_CPUIF
 	mrs	x24, hcr_el2
 	mov	x25, #HCR_INT_OVERRIDE
 	neg	x25, x25
@@ -828,10 +827,7 @@
 	orr	x24, x24, #HCR_INT_OVERRIDE
 	orr	x24, x24, x25
 	msr	hcr_el2, x24
-	adr	x24, __vgic_sr_vectors
-	ldr	x24, [x24, #VGIC_RESTORE_FN]
-	kern_hyp_va	x24
-	blr	x24
+	alternative_insn "bl __restore_vgic_v2_state", "bl __restore_vgic_v3_state", ARM64_HAS_SYSREG_GIC_CPUIF
 .endm
 
 .macro save_timer_state
@@ -1062,12 +1058,6 @@ ENTRY(__kvm_flush_vm_context)
 	ret
 ENDPROC(__kvm_flush_vm_context)
 
-	// struct vgic_sr_vectors __vgi_sr_vectors;
-	.align 3
-ENTRY(__vgic_sr_vectors)
-	.skip	VGIC_SR_VECTOR_SZ
-ENDPROC(__vgic_sr_vectors)
-
 __kvm_hyp_panic:
 	// Guess the context by looking at VTTBR:
 	// If zero, then we're already a host.
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 0cc6ab6..23d3c81 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1903,9 +1903,6 @@ int kvm_vgic_hyp_init(void)
 		goto out_free_irq;
 	}
 
-	/* Callback into for arch code for setup */
-	vgic_arch_setup(vgic);
-
 	on_each_cpu(vgic_init_maintenance_interrupt, NULL, 1);
 
 	return 0;
-- 
2.1.4

[PATCH v3 5/5] arm64: KVM: Switch vgic save/restore to alternative_insn

[Christoffer Dall]

On Fri, Mar 27, 2015 at 01:09:25PM +0000, Marc Zyngier wrote:
> So far, we configured the world-switch by having a small array
> of pointers to the save and restore functions, depending on the
> GIC used on the platform.
> 
> Loading these values each time is a bit silly (they never change),
> and it makes sense to rely on the instruction patching instead.
> 
> This leads to a nice cleanup of the code.
> 
> Acked-by: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

I gave this a quick spin on Juno as well and works as expected:

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>