arm64: Add new hcall HVC_CALL_FUNC

To allow the assembler macros defined in proc-macros.S to be used outside the mm code move the proc-macros.S file from arch/arm64/mm/ to arch/arm64/include/asm/ and fix up any preprocessor includes to use the new file location.

Signed-off-by Geoff Levand <[email protected]>

---
 arch/arm64/include/asm/proc-macros.S | 54 ++++++++++++++++++++++++++++++++++++
 arch/arm64/mm/cache.S                |  3 +-
 arch/arm64/mm/proc-macros.S          | 54 ------------------------------------
 arch/arm64/mm/proc.S                 |  3 +-

To allow for additional hcalls to be defined and to make the arm64 hcall API more consistent across exception vector routines, change the hcall implementations to use the ISS field of the ESR_EL2 register to specify the hcall type.

The existing arm64 hcall implementations are limited in that they only allow for two distinct hcalls; with the x0 register either zero, or not zero. Also, the API of the hyp-stub exception vector routines and the KVM exception vector routines differ; hyp-stub uses a non-zero value in x0 to implement __hyp_set_vectors, whereas KVM uses it to implement kvm_call_hyp.

Define three new preprocessor macros HVC_GET_VECTORS, HVC_SET_VECTORS and HVC_CALL_HYP and to be used as hcall type specifiers and convert the existing __hyp_get_vectors(), __hyp_set_vectors() and kvm_call_hyp() routines to use these new macros when executing an HVC call. Also change the corresponding hyp-stub and KVM el1_sync exception vector routines to use these new macros.

Signed-off-by Geoff Levand <[email protected]>

arch/arm64/include/asm/virt.h +9 -0

index 7a5df52..99c319c 100644
@@ -21,6 +21,26 @@
 #define BOOT_CPU_MODE_EL1	(0xe11)
 #define BOOT_CPU_MODE_EL2	(0xe12)
 
+/*
+ * HVC_GET_VECTORS - Return the value of the vbar_el2 register.
+ */
+
+#define HVC_GET_VECTORS 1
+
+/*
+ * HVC_SET_VECTORS - Set the value of the vbar_el2 register.
+ *

arch/arm64/include/asm/virt.h +0 -0

 /*

arch/arm64/kernel/hyp-stub.S +10 -6

index a272f33..e3db3fd 100644
@@ -22,6 +22,7 @@
 #include <linux/irqchip/arm-gic-v3.h>
 
 #include <asm/assembler.h>
+#include <asm/kvm_arm.h>
 #include <asm/ptrace.h>
 #include <asm/virt.h>
 

@@ -53,14 +54,22 @@ ENDPROC(__hyp_stub_vectors)
 	.align 11
 
 el1_sync:
-	mrs	x1, esr_el2
-	lsr	x1, x1, #26
-	cmp	x1, #0x16
-	b.ne	2f				// Not an HVC trap
-	cbz	x0, 1f
-	msr	vbar_el2, x0			// Set vbar_el2
+	mrs	x18, esr_el2
+	lsr	x17, x18, #ESR_ELx_EC_SHIFT
+	and	x18, x18, #ESR_ELx_ISS_MASK
+
+	cmp     x17, #ESR_ELx_EC_HVC64
+	b.ne    2f				// Not an HVC trap
+
+	cmp	x18, #HVC_GET_VECTORS
+	b.ne	1f

arch/arm64/kvm/hyp.S +3 -4

index c0d8202..1916c89 100644
@@ -27,6 +27,7 @@
 #include <asm/kvm_asm.h>
 #include <asm/kvm_mmu.h>
 #include <asm/memory.h>
+#include <asm/virt.h>
 
 #define CPU_GP_REG_OFFSET(x)	(CPU_GP_REGS + x)
 #define CPU_XREG_OFFSET(x)	CPU_GP_REG_OFFSET(CPU_USER_PT_REGS + 8*x)

@@ -1106,12 +1107,9 @@ __hyp_panic_str:
  * in Hyp mode (see init_hyp_mode in arch/arm/kvm/arm.c).  Return values are
  * passed in r0 and r1.
  *
- * A function pointer with a value of 0 has a special meaning, and is
- * used to implement __hyp_get_vectors in the same way as in
- * arch/arm64/kernel/hyp_stub.S.
  */
 ENTRY(kvm_call_hyp)
-	hvc	#0
+	hvc	#HVC_CALL_HYP
 	ret
 ENDPROC(kvm_call_hyp)
 

@@ -1142,6 +1140,7 @@ el1_sync:					// Guest trapped into EL2
 
 	mrs	x1, esr_el2
 	lsr	x2, x1, #ESR_ELx_EC_SHIFT
+	and	x0, x1, #ESR_ELx_ISS_MASK
 
 	cmp	x2, #ESR_ELx_EC_HVC64
 	b.ne	el1_trap

arch/arm64/kvm/hyp.S +0 -2

 	pop	x2, x3
 	pop	x0, x1
 
-	/* Check for __hyp_get_vectors */
-	cbnz	x0, 1f

Quoted 17 lines · Geoff Levand

To allow for additional hcalls to be defined and to make the arm64 hcall API more consistent across exception vector routines, change the hcall implementations to use the ISS field of the ESR_EL2 register to specify the hcall type.

The existing arm64 hcall implementations are limited in that they only allow for two distinct hcalls; with the x0 register either zero, or not zero. Also, the API of the hyp-stub exception vector routines and the KVM exception vector routines differ; hyp-stub uses a non-zero value in x0 to implement __hyp_set_vectors, whereas KVM uses it to implement kvm_call_hyp.

Define three new preprocessor macros HVC_GET_VECTORS, HVC_SET_VECTORS and HVC_CALL_HYP and to be used as hcall type specifiers and convert the existing __hyp_get_vectors(), __hyp_set_vectors() and kvm_call_hyp() routines to use these new macros when executing an HVC call. Also change the corresponding hyp-stub and KVM el1_sync exception vector routines to use these new macros.

Quoted 1 line

Signed-off-by Geoff Levand <[email protected]>

Using the #imm value for HVC to separate what gets called looks fine to me. However, I'd like to see a review from Marc/Christoffer on this patch.

Some comments below:

Quoted 23 lines

arch/arm64/include/asm/virt.h +15 -0

index 7a5df52..99c319c 100644
@@ -21,6 +21,26 @@
 #define BOOT_CPU_MODE_EL1	(0xe11)
 #define BOOT_CPU_MODE_EL2	(0xe12)
 
+/*
+ * HVC_GET_VECTORS - Return the value of the vbar_el2 register.
+ */
+
+#define HVC_GET_VECTORS 1
+
+/*
+ * HVC_SET_VECTORS - Set the value of the vbar_el2 register.
+ *
+ * @x0: Physical address of the new vector table.
+ */
+
+#define HVC_SET_VECTORS 2
+
+/*

arch/arm64/include/asm/virt.h line 42

Quoted 4 lines

arch/arm64/include/asm/virt.h +4 -0

+ * HVC_CALL_HYP - Execute a hyp routine.
+ */
+
+#define HVC_CALL_HYP 3

I think you can ignore this case (make it the default), just define it as 0 as that's the normal use-case after initialisation and avoid checking it explicitly.

Quoted 41 lines

arch/arm64/include/asm/virt.h +0 -0

 /*

arch/arm64/kernel/hyp-stub.S +16 -7

index a272f33..e3db3fd 100644
@@ -22,6 +22,7 @@
 #include <linux/irqchip/arm-gic-v3.h>
 
 #include <asm/assembler.h>
+#include <asm/kvm_arm.h>
 #include <asm/ptrace.h>
 #include <asm/virt.h>
 

@@ -53,14 +54,22 @@ ENDPROC(__hyp_stub_vectors)
 	.align 11
 
 el1_sync:
-	mrs	x1, esr_el2
-	lsr	x1, x1, #26
-	cmp	x1, #0x16
-	b.ne	2f				// Not an HVC trap
-	cbz	x0, 1f
-	msr	vbar_el2, x0			// Set vbar_el2
+	mrs	x18, esr_el2
+	lsr	x17, x18, #ESR_ELx_EC_SHIFT
+	and	x18, x18, #ESR_ELx_ISS_MASK
+
+	cmp     x17, #ESR_ELx_EC_HVC64
+	b.ne    2f				// Not an HVC trap
+
+	cmp	x18, #HVC_GET_VECTORS
+	b.ne	1f
+	mrs	x0, vbar_el2
 	b	2f
-1:	mrs	x0, vbar_el2			// Return vbar_el2
+
+1:	cmp	x18, #HVC_SET_VECTORS
+	b.ne	2f
+	msr	vbar_el2, x0
+
 2:	eret

arch/arm64/kernel/hyp-stub.S ENDPROC(__hyp_stub_vectors) · line 74

Quoted 1 line

arch/arm64/kernel/hyp-stub.S +0 -0

 ENDPROC(el1_sync)

You seem to be using x17 and x18 here freely. Do you have any guarantees that the caller saved/restored those registers? I guess you assume they are temporary registers and the caller first branches to a function (like __kvm_hyp_call) and expects them to be corrupted. But I'm not sure that's always the case. Take for example the __invoke_psci_fn_hvc where the function is in C (we should change this for other reasons).

Quoted 37 lines

arch/arm64/kvm/hyp.S +3 -4

index c0d8202..1916c89 100644
@@ -27,6 +27,7 @@
 #include <asm/kvm_asm.h>
 #include <asm/kvm_mmu.h>
 #include <asm/memory.h>
+#include <asm/virt.h>
 
 #define CPU_GP_REG_OFFSET(x)	(CPU_GP_REGS + x)
 #define CPU_XREG_OFFSET(x)	CPU_GP_REG_OFFSET(CPU_USER_PT_REGS + 8*x)

@@ -1106,12 +1107,9 @@ __hyp_panic_str:
  * in Hyp mode (see init_hyp_mode in arch/arm/kvm/arm.c).  Return values are
  * passed in r0 and r1.
  *
- * A function pointer with a value of 0 has a special meaning, and is
- * used to implement __hyp_get_vectors in the same way as in
- * arch/arm64/kernel/hyp_stub.S.
  */
 ENTRY(kvm_call_hyp)
-	hvc	#0
+	hvc	#HVC_CALL_HYP
 	ret
 ENDPROC(kvm_call_hyp)
 

@@ -1142,6 +1140,7 @@ el1_sync:					// Guest trapped into EL2
 
 	mrs	x1, esr_el2
 	lsr	x2, x1, #ESR_ELx_EC_SHIFT
+	and	x0, x1, #ESR_ELx_ISS_MASK
 
 	cmp	x2, #ESR_ELx_EC_HVC64
 	b.ne	el1_trap

@@ -1150,15 +1149,19 @@ el1_sync:					// Guest trapped into EL2
 	cbnz	x3, el1_trap			// called HVC
 

arch/arm64/kvm/hyp.S el1_sync: // Guest trapped into EL2 · line 1152

Quoted 2 lines

arch/arm64/kvm/hyp.S +1 -0

 	/* Here, we're pretty sure the host called HVC. */
+	mov	x18, x0

Same comment here about corrupting x18. If it is safe, maybe add some comments in the calling place.

Quoted 11 lines

arch/arm64/kvm/hyp.S +2 -2

 	pop	x2, x3
 	pop	x0, x1
 
-	/* Check for __hyp_get_vectors */
-	cbnz	x0, 1f
+	cmp	x18, #HVC_GET_VECTORS
+	b.ne	1f
 	mrs	x0, vbar_el2
 	b	2f
 

arch/arm64/kvm/hyp.S el1_sync: // Guest trapped into EL2 · line 1164

Quoted 5 lines

arch/arm64/kvm/hyp.S +4 -1

-1:	push	lr, xzr
+1:	cmp	x18, #HVC_CALL_HYP
+	b.ne	2f
+
+	push	lr, xzr

At this point, we expect either HVC_GET_VECTORS or HVC_CALL_HYP. I think you can simply assume HVC_CALL_HYP as default and ignore the additional cmp.

Quoted 42 lines · Catalin Marinas

Quoted 41 lines · Geoff Levand

/*

arch/arm64/kernel/hyp-stub.S +16 -7

index a272f33..e3db3fd 100644
@@ -22,6 +22,7 @@
 #include <linux/irqchip/arm-gic-v3.h>
 
 #include <asm/assembler.h>
+#include <asm/kvm_arm.h>
 #include <asm/ptrace.h>
 #include <asm/virt.h>
 

@@ -53,14 +54,22 @@ ENDPROC(__hyp_stub_vectors)
 	.align 11
 
 el1_sync:
-	mrs	x1, esr_el2
-	lsr	x1, x1, #26
-	cmp	x1, #0x16
-	b.ne	2f				// Not an HVC trap
-	cbz	x0, 1f
-	msr	vbar_el2, x0			// Set vbar_el2
+	mrs	x18, esr_el2
+	lsr	x17, x18, #ESR_ELx_EC_SHIFT
+	and	x18, x18, #ESR_ELx_ISS_MASK
+
+	cmp     x17, #ESR_ELx_EC_HVC64
+	b.ne    2f				// Not an HVC trap
+
+	cmp	x18, #HVC_GET_VECTORS
+	b.ne	1f
+	mrs	x0, vbar_el2
 	b	2f
-1:	mrs	x0, vbar_el2			// Return vbar_el2
+
+1:	cmp	x18, #HVC_SET_VECTORS
+	b.ne	2f
+	msr	vbar_el2, x0
+
 2:	eret

Quoted 8 lines

arch/arm64/kernel/hyp-stub.S ENDPROC(__hyp_stub_vectors) · line 74

Quoted 1 line

arch/arm64/kernel/hyp-stub.S +0 -0

 ENDPROC(el1_sync)

You seem to be using x17 and x18 here freely. Do you have any guarantees that the caller saved/restored those registers? I guess you assume they are temporary registers and the caller first branches to a function (like __kvm_hyp_call) and expects them to be corrupted. But I'm not sure that's always the case. Take for example the __invoke_psci_fn_hvc where the function is in C (we should change this for other reasons).

Yes, I assume the compiler will not expect them to be preserved. I missed __invoke_psci_fn_hvc. Can we just add x17 and x18 to the clobbered list?

        asm volatile(
                        __asmeq("%0", "x0")
                        __asmeq("%1", "x1")
                        __asmeq("%2", "x2")
                        __asmeq("%3", "x3")
                        "hvc    #0\n"
                : "+r" (function_id)

To allow for additional hcalls to be defined and to make the arm64 hcall API more consistent across exception vector routines, change the hcall implementations to use the ISS field of the ESR_EL2 register to specify the hcall type.

The existing arm64 hcall implementations are limited in that they only allow for two distinct hcalls; with the x0 register either zero, or not zero. Also, the API of the hyp-stub exception vector routines and the KVM exception vector routines differ; hyp-stub uses a non-zero value in x0 to implement __hyp_set_vectors, whereas KVM uses it to implement kvm_call_hyp.

Define three new preprocessor macros HVC_CALL_HYP, HVC_GET_VECTORS, and HVC_SET_VECTORS to be used as hcall type specifiers and convert the existing __hyp_get_vectors(), __hyp_set_vectors() and kvm_call_hyp() routines to use these new macros when executing an HVC call. Also change the corresponding hyp-stub and KVM el1_sync exception vector routines to use these new macros.

Signed-off-by Geoff Levand <[email protected]>

---
 arch/arm64/include/asm/virt.h | 27 +++++++++++++++++++++++++++
 arch/arm64/kernel/hyp-stub.S  | 32 +++++++++++++++++++++-----------
 arch/arm64/kernel/psci.c      |  3 ++-
 arch/arm64/kvm/hyp.S          | 16 +++++++++-------
 4 files changed, 59 insertions(+), 19 deletions(-)

arch/arm64/include/asm/virt.h +0 -0

index 7a5df52..eb10368 100644

Quoted 3 lines · Geoff Levand

To allow for additional hcalls to be defined and to make the arm64 hcall API more consistent across exception vector routines, change the hcall implementations to use the ISS field of the ESR_EL2 register to specify the hcall type.

how does this make things more consistent? Do we have other examples of things using the immediate field which I'm missing?

Quoted 5 lines

The existing arm64 hcall implementations are limited in that they only allow for two distinct hcalls; with the x0 register either zero, or not zero. Also, the API of the hyp-stub exception vector routines and the KVM exception vector routines differ; hyp-stub uses a non-zero value in x0 to implement __hyp_set_vectors, whereas KVM uses it to implement kvm_call_hyp.

this seems orthogonal to the use of the immediate field vs. x0 though, so why it the immediate field preferred again?

Quoted 20 lines

Define three new preprocessor macros HVC_CALL_HYP, HVC_GET_VECTORS, and HVC_SET_VECTORS to be used as hcall type specifiers and convert the existing __hyp_get_vectors(), __hyp_set_vectors() and kvm_call_hyp() routines to use these new macros when executing an HVC call. Also change the corresponding hyp-stub and KVM el1_sync exception vector routines to use these new macros.

Signed-off-by Geoff Levand <[email protected]>

---
 arch/arm64/include/asm/virt.h | 27 +++++++++++++++++++++++++++
 arch/arm64/kernel/hyp-stub.S  | 32 +++++++++++++++++++++-----------
 arch/arm64/kernel/psci.c      |  3 ++-
 arch/arm64/kvm/hyp.S          | 16 +++++++++-------
 4 files changed, 59 insertions(+), 19 deletions(-)

arch/arm64/include/asm/virt.h +0 -0

index 7a5df52..eb10368 100644

arch/arm64/include/asm/virt.h line 26

Quoted 9 lines

arch/arm64/include/asm/virt.h +6 -0

@@ -18,6 +18,33 @@
 #ifndef __ASM__VIRT_H
 #define __ASM__VIRT_H
 
+/*
+ * The arm64 hcall implementation uses the ISS field of the ESR_EL2 register to
+ * specify the hcall type.  The exception handlers are allowed to use registers
+ * x17 and x18 in their implementation.  Any routine issuing an hcall must not
+ * expect these registers to be preserved.
+ */

I thought the existing use of registers were based on the arm procedure call standard so we didn't have to worry about adding more caller-save registers.

Don't we now have to start adding code around callers to make sure callers know that x17 and x18 may be clobbered?

Hi Christoffer,

Quoted 7 lines · Christoffer Dall

Quoted 3 lines · Geoff Levand

To allow for additional hcalls to be defined and to make the arm64 hcall API more consistent across exception vector routines, change the hcall implementations to use the ISS field of the ESR_EL2 register to specify the hcall type.

how does this make things more consistent? Do we have other examples of things using the immediate field which I'm missing?

As I detail in the next paragraph, by consistent I mean in the API exposed, not in the implementation. This point is really secondary to the need for more hyper calls as I discuss below.

Quoted 8 lines

Quoted 5 lines

The existing arm64 hcall implementations are limited in that they only allow for two distinct hcalls; with the x0 register either zero, or not zero. Also, the API of the hyp-stub exception vector routines and the KVM exception vector routines differ; hyp-stub uses a non-zero value in x0 to implement __hyp_set_vectors, whereas KVM uses it to implement kvm_call_hyp.

this seems orthogonal to the use of the immediate field vs. x0 though, so why it the immediate field preferred again?

When a CPU is reset via cpu_soft_restart() we need to execute the caller supplied reset routine at the exception level the kernel was entered at. So, for a kernel entered at EL2, we need a way to execute that routine at EL2.

The current hyp-stub vector implementation, which uses x0, is limited to two hyper calls; __hyp_get_vectors and __hyp_set_vectors. To support cpu_soft_restart() we need a third hyper call, one which allows for code to be executed at EL2. My proposed use of the immediate value of the hvc instruction will allow for 2^16 distinct hyper calls.

Quoted 34 lines

Quoted 20 lines

Define three new preprocessor macros HVC_CALL_HYP, HVC_GET_VECTORS, and HVC_SET_VECTORS to be used as hcall type specifiers and convert the existing __hyp_get_vectors(), __hyp_set_vectors() and kvm_call_hyp() routines to use these new macros when executing an HVC call. Also change the corresponding hyp-stub and KVM el1_sync exception vector routines to use these new macros.

Signed-off-by Geoff Levand <[email protected]>

---
 arch/arm64/include/asm/virt.h | 27 +++++++++++++++++++++++++++
 arch/arm64/kernel/hyp-stub.S  | 32 +++++++++++++++++++++-----------
 arch/arm64/kernel/psci.c      |  3 ++-
 arch/arm64/kvm/hyp.S          | 16 +++++++++-------
 4 files changed, 59 insertions(+), 19 deletions(-)

arch/arm64/include/asm/virt.h +0 -0

index 7a5df52..eb10368 100644

arch/arm64/include/asm/virt.h line 26

Quoted 9 lines

arch/arm64/include/asm/virt.h +6 -0

@@ -18,6 +18,33 @@
 #ifndef __ASM__VIRT_H
 #define __ASM__VIRT_H
 
+/*
+ * The arm64 hcall implementation uses the ISS field of the ESR_EL2 register to
+ * specify the hcall type.  The exception handlers are allowed to use registers
+ * x17 and x18 in their implementation.  Any routine issuing an hcall must not
+ * expect these registers to be preserved.
+ */

I thought the existing use of registers were based on the arm procedure call standard so we didn't have to worry about adding more caller-save registers.

Quoted 2 lines

Don't we now have to start adding code around callers to make sure callers know that x17 and x18 may be clobbered?

We use x17 and x18 to allow hyper calls to work without a stack, which is needed for cpu_soft_restart(). The procedure call standard says that these are temporary registers, so a C compiler should not expect these to be preserved.

Quoted 6 lines · Geoff Levand

The current hyp-stub vector implementation, which uses x0, is limited to two hyper calls; __hyp_get_vectors and __hyp_set_vectors. To support cpu_soft_restart() we need a third hyper call, one which allows for code to be executed at EL2. My proposed use of the immediate value of the hvc instruction will allow for 2^16 distinct hyper calls.

right, but using x0 allows for 2^64 distinct hypercalls. Just to be clear, I'm fine with using immediate field if there are no good reasons not to, I was just curious as to what direct benefit it has. After thinking about it a bit, from my point of view, the benefit would be the clarity that x0 is first argument like a normal procedure call, so no need to shift things around. Is this part of the equation or am I missing the overall purpose here?

Quoted 13 lines

Quoted 12 lines

Quoted 5 lines

+ * The arm64 hcall implementation uses the ISS field of the ESR_EL2 register to
+ * specify the hcall type. The exception handlers are allowed to use registers
+ * x17 and x18 in their implementation. Any routine issuing an hcall must not
+ * expect these registers to be preserved.
+ */

I thought the existing use of registers were based on the arm procedure call standard so we didn't have to worry about adding more caller-save registers.

Don't we now have to start adding code around callers to make sure callers know that x17 and x18 may be clobbered?

Quoted 5 lines

We use x17 and x18 to allow hyper calls to work without a stack, which is needed for cpu_soft_restart(). The procedure call standard says that these are temporary registers, so a C compiler should not expect these to be preserved.

Then why not use r9-15 or r0-r7 as the AACPS clearly specifies as caller preserve instead of the registers which may have special meaning etc.?

Hi Christoffer,

Quoted 8 lines · Christoffer Dall

Quoted 6 lines · Geoff Levand

The current hyp-stub vector implementation, which uses x0, is limited to two hyper calls; __hyp_get_vectors and __hyp_set_vectors. To support cpu_soft_restart() we need a third hyper call, one which allows for code to be executed at EL2. My proposed use of the immediate value of the hvc instruction will allow for 2^16 distinct hyper calls.

Quoted 7 lines

right, but using x0 allows for 2^64 distinct hypercalls. Just to be clear, I'm fine with using immediate field if there are no good reasons not to, I was just curious as to what direct benefit it has. After thinking about it a bit, from my point of view, the benefit would be the clarity that x0 is first argument like a normal procedure call, so no need to shift things around. Is this part of the equation or am I missing the overall purpose here?

Yes, in general it will make marshaling of args, etc. easier. Also, to me, if we are going to change the implementation it seems to be the most natural way.

Quoted 3 lines · Geoff Levand

Yes, in general it will make marshaling of args, etc. easier. Also, to me, if we are going to change the implementation it seems to be the most natural way.

From reading the architecture documentation, I too expected the hypervisor call instruction's immediate and the instruction specific syndrome to be used. However I vaguely recall someone pointing out that reading the exception syndrome register and extracting the instruction specific syndrome is bound to take longer than simply using a general purpose register.

One might also consider alignment with the SMC Calling Convention document [1], which while originally written for SMC, is also used for HVC by PSCI [2].

1. http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0028a/index.html
2. http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0022c/index.html

arch/arm64/include/asm/virt.h +0 -0

index 99c319c..4f23a48 100644
@@ -41,6 +41,19 @@
 
 #define HVC_CALL_HYP 3

arch/arm64/include/asm/virt.h +4 -0

+ */
+
+#define HVC_CALL_FUNC 4
+
 #ifndef __ASSEMBLY__
 
 /*

arch/arm64/kernel/hyp-stub.S +0 -0

index e3db3fd..b5d36e7 100644

arch/arm64/kernel/hyp-stub.S +6 -0

+	b.ne    1f
+	msr     vbar_el2, x0
+	b       2f
+
+1:	cmp     x18, #HVC_CALL_FUNC
+	b.ne    2f

Quoted 8 lines · Geoff Levand

arch/arm64/include/asm/virt.h +0 -0

index 99c319c..4f23a48 100644
@@ -41,6 +41,19 @@
 
 #define HVC_CALL_HYP 3
 

arch/arm64/include/asm/virt.h line 52

Quoted 9 lines

arch/arm64/include/asm/virt.h +9 -0

+/*
+ * HVC_CALL_FUNC - Execute a function at EL2.
+ *
+ * @x0: Physical address of the function to be executed.
+ * @x1: Passed as the first argument to the function.
+ * @x2: Passed as the second argument to the function.
+ * @x3: Passed as the third argument to the function.
+ *
+ * The called function must preserve the contents of register x18.

Can you pick a register that's normally callee saved?

Quoted 10 lines

arch/arm64/include/asm/virt.h +9 -0

+1:	cmp     x18, #HVC_CALL_FUNC
+	b.ne    2f
+	mov     x18, lr
+	mov     lr, x0
+	mov     x0, x1
+	mov     x1, x2
+	mov     x2, x3
+	blr     lr
+	mov     lr, x18

Quoted 2 lines

2: eret
ENDPROC(el1_sync)

What is the calling convention for this HVC? You mentioned x18 above but what about other registers that the called function may corrupt (x18 is a temporary register, so it's not expected to be callee saved).

Quoted 8 lines · Geoff Levand

arch/arm64/include/asm/virt.h +0 -0

index 99c319c..4f23a48 100644
@@ -41,6 +41,19 @@
 
 #define HVC_CALL_HYP 3
 

arch/arm64/include/asm/virt.h line 52

Quoted 9 lines

arch/arm64/include/asm/virt.h +9 -0

+/*
+ * HVC_CALL_FUNC - Execute a function at EL2.
+ *
+ * @x0: Physical address of the function to be executed.
+ * @x1: Passed as the first argument to the function.
+ * @x2: Passed as the second argument to the function.
+ * @x3: Passed as the third argument to the function.
+ *
+ * The called function must preserve the contents of register x18.

Can you pick a register that's normally callee saved?

Quoted 15 lines

arch/arm64/include/asm/virt.h +4 -0

+ */
+
+#define HVC_CALL_FUNC 4
+
 #ifndef __ASSEMBLY__
 
 /*

arch/arm64/kernel/hyp-stub.S +0 -0

index e3db3fd..b5d36e7 100644
@@ -66,9 +66,20 @@ el1_sync:
 	mrs	x0, vbar_el2
 	b	2f
 

arch/arm64/kernel/hyp-stub.S el1_sync: · line 69

Quoted 4 lines

arch/arm64/kernel/hyp-stub.S +1 -3

-1:	cmp	x18, #HVC_SET_VECTORS
-	b.ne	2f
-	msr	vbar_el2, x0
+1:	cmp     x18, #HVC_SET_VECTORS

This line doesn't seem to have any change, apart from some whitespace. Or did you want to drop the label?

Quoted 14 lines

arch/arm64/kernel/hyp-stub.S +13 -0

+	b.ne    1f
+	msr     vbar_el2, x0
+	b       2f
+
+1:	cmp     x18, #HVC_CALL_FUNC
+	b.ne    2f
+	mov     x18, lr
+	mov     lr, x0
+	mov     x0, x1
+	mov     x1, x2
+	mov     x2, x3
+	blr     lr
+	mov     lr, x18
 

arch/arm64/kernel/hyp-stub.S el1_sync: · line 85

Quoted 2 lines

arch/arm64/kernel/hyp-stub.S +0 -0

 2:	eret
 ENDPROC(el1_sync)

What is the calling convention for this HVC? You mentioned x18 above but what about other registers that the called function may corrupt (x18 is a temporary register, so it's not expected to be callee saved).

When a CPU is reset it needs to be put into the exception level it had when it entered the kernel. Update cpu_reset() to accept an argument el2_switch which signals cpu_reset() to enter the soft reset address at EL2. If el2_switch is not set the soft reset address will be entered at EL1.

Update cpu_soft_restart() and soft_restart() to pass the return of is_hyp_mode_available() as the el2_switch value to cpu_reset(). Also update the comments of cpu_reset(), cpu_soft_restart() and soft_restart() to reflect this change.

Signed-off-by Geoff Levand <[email protected]>

---
 arch/arm64/include/asm/proc-fns.h |  4 ++--
 arch/arm64/kernel/process.c       | 10 ++++++++-
 arch/arm64/mm/proc.S              | 47 +++++++++++++++++++++++++++++----------
 3 files changed, 46 insertions(+), 15 deletions(-)

arch/arm64/include/asm/proc-fns.h +2 -2

index 9a8fd84..339394d 100644
@@ -32,8 +32,8 @@ extern void cpu_cache_off(void);
 extern void cpu_do_idle(void);
 extern void cpu_do_switch_mm(unsigned long pgd_phys, struct mm_struct *mm);
 extern void cpu_reset(unsigned long addr) __attribute__((noreturn));
-void cpu_soft_restart(phys_addr_t cpu_reset,
-		unsigned long addr) __attribute__((noreturn));
+void cpu_soft_restart(phys_addr_t cpu_reset, unsigned long el2_switch,
+		      unsigned long addr) __attribute__((noreturn));
 extern void cpu_do_suspend(struct cpu_suspend_ctx *ptr);
 extern u64 cpu_do_resume(phys_addr_t ptr, u64 idmap_ttbr);
 

arch/arm64/kernel/process.c +1 -0

index fde9923..371bbf1 100644
@@ -50,6 +50,7 @@
 #include <asm/mmu_context.h>
 #include <asm/processor.h>
 #include <asm/stacktrace.h>
+#include <asm/virt.h>
 
 #ifdef CONFIG_CC_STACKPROTECTOR
 #include <linux/stackprotector.h>

Quoted 45 lines · Geoff Levand

When a CPU is reset it needs to be put into the exception level it had when it entered the kernel. Update cpu_reset() to accept an argument el2_switch which signals cpu_reset() to enter the soft reset address at EL2. If el2_switch is not set the soft reset address will be entered at EL1.

Update cpu_soft_restart() and soft_restart() to pass the return of is_hyp_mode_available() as the el2_switch value to cpu_reset(). Also update the comments of cpu_reset(), cpu_soft_restart() and soft_restart() to reflect this change.

Signed-off-by Geoff Levand <[email protected]>

---
 arch/arm64/include/asm/proc-fns.h |  4 ++--
 arch/arm64/kernel/process.c       | 10 ++++++++-
 arch/arm64/mm/proc.S              | 47 +++++++++++++++++++++++++++++----------
 3 files changed, 46 insertions(+), 15 deletions(-)

arch/arm64/include/asm/proc-fns.h +2 -2

index 9a8fd84..339394d 100644
@@ -32,8 +32,8 @@ extern void cpu_cache_off(void);
 extern void cpu_do_idle(void);
 extern void cpu_do_switch_mm(unsigned long pgd_phys, struct mm_struct *mm);
 extern void cpu_reset(unsigned long addr) __attribute__((noreturn));
-void cpu_soft_restart(phys_addr_t cpu_reset,
-		unsigned long addr) __attribute__((noreturn));
+void cpu_soft_restart(phys_addr_t cpu_reset, unsigned long el2_switch,
+		      unsigned long addr) __attribute__((noreturn));
 extern void cpu_do_suspend(struct cpu_suspend_ctx *ptr);
 extern u64 cpu_do_resume(phys_addr_t ptr, u64 idmap_ttbr);
 

arch/arm64/kernel/process.c +1 -0

index fde9923..371bbf1 100644
@@ -50,6 +50,7 @@
 #include <asm/mmu_context.h>
 #include <asm/processor.h>
 #include <asm/stacktrace.h>
+#include <asm/virt.h>
 
 #ifdef CONFIG_CC_STACKPROTECTOR
 #include <linux/stackprotector.h>

arch/arm64/kernel/process.c EXPORT_SYMBOL(__stack_chk_guard); · line 67

Quoted 8 lines

arch/arm64/kernel/process.c +4 -1

@@ -60,7 +61,14 @@ EXPORT_SYMBOL(__stack_chk_guard);
 void soft_restart(unsigned long addr)
 {
 	setup_mm_for_reboot();
-	cpu_soft_restart(virt_to_phys(cpu_reset), addr);
+
+	/* TODO: Remove this conditional when KVM can support CPU restart. */
+	if (IS_ENABLED(CONFIG_KVM))
+		cpu_soft_restart(virt_to_phys(cpu_reset), 0, addr);

If we haven't torn down KVM, doesn't that mean that KVM is active at EL2 (with MMU and caches on) at this point?

If that's the case then we cannot possibly try to call kexec(), because we cannot touch the memory used by the page tables for those EL2 mappings. Things will explode if we do.

Hi Mark,

Quoted 58 lines · Mark Rutland

Quoted 45 lines · Geoff Levand

When a CPU is reset it needs to be put into the exception level it had when it entered the kernel. Update cpu_reset() to accept an argument el2_switch which signals cpu_reset() to enter the soft reset address at EL2. If el2_switch is not set the soft reset address will be entered at EL1.

Update cpu_soft_restart() and soft_restart() to pass the return of is_hyp_mode_available() as the el2_switch value to cpu_reset(). Also update the comments of cpu_reset(), cpu_soft_restart() and soft_restart() to reflect this change.

Signed-off-by Geoff Levand <[email protected]>

---
 arch/arm64/include/asm/proc-fns.h |  4 ++--
 arch/arm64/kernel/process.c       | 10 ++++++++-
 arch/arm64/mm/proc.S              | 47 +++++++++++++++++++++++++++++----------
 3 files changed, 46 insertions(+), 15 deletions(-)

arch/arm64/include/asm/proc-fns.h +2 -2

index 9a8fd84..339394d 100644
@@ -32,8 +32,8 @@ extern void cpu_cache_off(void);
 extern void cpu_do_idle(void);
 extern void cpu_do_switch_mm(unsigned long pgd_phys, struct mm_struct *mm);
 extern void cpu_reset(unsigned long addr) __attribute__((noreturn));
-void cpu_soft_restart(phys_addr_t cpu_reset,
-		unsigned long addr) __attribute__((noreturn));
+void cpu_soft_restart(phys_addr_t cpu_reset, unsigned long el2_switch,
+		      unsigned long addr) __attribute__((noreturn));
 extern void cpu_do_suspend(struct cpu_suspend_ctx *ptr);
 extern u64 cpu_do_resume(phys_addr_t ptr, u64 idmap_ttbr);
 

arch/arm64/kernel/process.c +1 -0

index fde9923..371bbf1 100644
@@ -50,6 +50,7 @@
 #include <asm/mmu_context.h>
 #include <asm/processor.h>
 #include <asm/stacktrace.h>
+#include <asm/virt.h>
 
 #ifdef CONFIG_CC_STACKPROTECTOR
 #include <linux/stackprotector.h>

arch/arm64/kernel/process.c EXPORT_SYMBOL(__stack_chk_guard); · line 67

Quoted 8 lines

arch/arm64/kernel/process.c +4 -1

@@ -60,7 +61,14 @@ EXPORT_SYMBOL(__stack_chk_guard);
 void soft_restart(unsigned long addr)
 {
 	setup_mm_for_reboot();
-	cpu_soft_restart(virt_to_phys(cpu_reset), addr);
+
+	/* TODO: Remove this conditional when KVM can support CPU restart. */
+	if (IS_ENABLED(CONFIG_KVM))
+		cpu_soft_restart(virt_to_phys(cpu_reset), 0, addr);

If we haven't torn down KVM, doesn't that mean that KVM is active at EL2 (with MMU and caches on) at this point?

Quoted 3 lines

If that's the case then we cannot possibly try to call kexec(), because we cannot touch the memory used by the page tables for those EL2 mappings. Things will explode if we do.

This conditional is just if KVM, do things the old way (don't try to switch exception levels). It is to handle the system shutdown case.

Another patch in this series '[PATCH 7/8] arm64/kexec: Add checks for KVM' assures kexec cannot happen when KVM is configured.

Quoted 2 lines · Geoff Levand

This conditional is just if KVM, do things the old way (don't try to switch exception levels). It is to handle the system shutdown case.

Having grepped treewide for soft_restart, other than kexec there are no users for arm64. So surely kexec is the only case to cater for at the moment?

Quoted 2 lines

Another patch in this series '[PATCH 7/8] arm64/kexec: Add checks for KVM' assures kexec cannot happen when KVM is configured.

It would be better to just move this earlier (or event better, implement kvm teardown).

ENTRY(cpu_reset)

arch/arm64/mm/proc.S +2 -3

-	mrs	x1, sctlr_el1
-	bic	x1, x1, #1
-	msr	sctlr_el1, x1			// disable the MMU
+	mrs	x2, sctlr_el1
+	bic	x2, x2, #1

Quoted 11 lines · Geoff Levand

ENTRY(cpu_reset)

arch/arm64/mm/proc.S +5 -4

-	mrs	x1, sctlr_el1
-	bic	x1, x1, #1
-	msr	sctlr_el1, x1			// disable the MMU
+	mrs	x2, sctlr_el1
+	bic	x2, x2, #1
+	msr	sctlr_el1, x2			// disable the MMU
 	isb
-	ret	x0
+
+	cbz	x0, 1f				// el2_switch?

arch/arm64/mm/proc.S ENTRY(cpu_cache_off) · line 86

Quoted 5 lines

arch/arm64/mm/proc.S +5 -0

+	mov	x0, x1
+	mov	x1, xzr
+	mov	x2, xzr
+	mov	x3, xzr
+	hvc	#HVC_CALL_FUNC			// no return

If that's the only user of HVC_CALL_FUNC, why do we bother with arguments, calling convention?

Add three new files, kexec.h, machine_kexec.c and relocate_kernel.S to the arm64 architecture that add support for the kexec re-boot mechanism (CONFIG_KEXEC) on arm64 platforms.

Signed-off-by Geoff Levand <[email protected]>

---
 arch/arm64/Kconfig                  |   9 ++
 arch/arm64/include/asm/kexec.h      |  47 +++++++++++
 arch/arm64/kernel/Makefile          |   1 +
 arch/arm64/kernel/machine_kexec.c   | 155 ++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/relocate_kernel.S | 160 ++++++++++++++++++++++++++++++++++++
 include/uapi/linux/kexec.h          |   1 +
 6 files changed, 373 insertions(+)
 create mode 100644 arch/arm64/include/asm/kexec.h
 create mode 100644 arch/arm64/kernel/machine_kexec.c
 create mode 100644 arch/arm64/kernel/relocate_kernel.S

arch/arm64/Kconfig +9 -0

index b1f9a20..d9eb9cd 100644
@@ -474,6 +474,15 @@ config SECCOMP
          and the task is only allowed to execute a few safe syscalls
          defined by each seccomp mode.
 
+config KEXEC
+       depends on (!SMP || PM_SLEEP_SMP)
+       bool "kexec system call"
+       ---help---
+         kexec is a system call that implements the ability to shutdown your
+         current kernel, and to start another kernel.  It is like a reboot
+         but it is independent of the system firmware.   And like a reboot
+         you can start any kernel with it, not just Linux.
+

arch/arm64/Kconfig +16 -0

+/**
+ * kexec_is_dtb - Helper routine to check the device tree header signature.
+ */
+static bool kexec_is_dtb(const void *dtb)
+{
+       __be32 magic;
+
+       return get_user(magic, (__be32 *)dtb) ? false :
+               (be32_to_cpu(magic) == OF_DT_HEADER);
+}
+
+/**
+ * kexec_find_dtb_seg - Helper routine to find the dtb segment.
+ */
+static const struct kexec_segment *kexec_find_dtb_seg(
+       const struct kimage *image)

Add runtime checks that fail the arm64 kexec syscall for situations that would result in system instability do to problems in the KVM kernel support. These checks should be removed when the KVM problems are resolved fixed.

Signed-off-by Geoff Levand <[email protected]>

---
 arch/arm64/kernel/machine_kexec.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

arch/arm64/kernel/machine_kexec.c +3 -0

index 3d84759..a36459d 100644
@@ -16,6 +16,9 @@
 #include <asm/cacheflush.h>
 #include <asm/system_misc.h>
 
+/* TODO: Remove this include when KVM can support a kexec reboot. */
+#include <asm/virt.h>
+
 /* Global variables for the relocate_kernel routine. */
 extern const unsigned char relocate_new_kernel[];
 extern const unsigned long relocate_new_kernel_size;

Quoted 27 lines · Geoff Levand

Add runtime checks that fail the arm64 kexec syscall for situations that would result in system instability do to problems in the KVM kernel support. These checks should be removed when the KVM problems are resolved fixed.

Signed-off-by Geoff Levand <[email protected]>

---
 arch/arm64/kernel/machine_kexec.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

arch/arm64/kernel/machine_kexec.c +3 -0

index 3d84759..a36459d 100644
@@ -16,6 +16,9 @@
 #include <asm/cacheflush.h>
 #include <asm/system_misc.h>
 
+/* TODO: Remove this include when KVM can support a kexec reboot. */
+#include <asm/virt.h>
+
 /* Global variables for the relocate_kernel routine. */
 extern const unsigned char relocate_new_kernel[];
 extern const unsigned long relocate_new_kernel_size;

@@ -100,6 +103,13 @@ int machine_kexec_prepare(struct kimage *image)
 
      kexec_image_info(image);
 

arch/arm64/kernel/machine_kexec.c int machine_kexec_prepare(struct kimage *image) · line 111

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c +6 -0

+     /* TODO: Remove this message when KVM can support a kexec reboot. */
+     if (IS_ENABLED(CONFIG_KVM) && is_hyp_mode_available()) {
+             pr_err("%s: Your kernel is configured with KVM support (CONFIG_KVM=y) which currently does not allow for kexec re-boot.\n",
+                     __func__);
+             return -ENOSYS;
+     }

If you really don't want to implement KVM teardown, surely this should be at the start of the series, so we don't have a point in the middle where things may explode in this case?

Quoted 28 lines · Mark Rutland

Quoted 27 lines · Geoff Levand

Add runtime checks that fail the arm64 kexec syscall for situations that would result in system instability do to problems in the KVM kernel support. These checks should be removed when the KVM problems are resolved fixed.

Signed-off-by Geoff Levand <[email protected]>

---
 arch/arm64/kernel/machine_kexec.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

arch/arm64/kernel/machine_kexec.c +3 -0

index 3d84759..a36459d 100644
@@ -16,6 +16,9 @@
 #include <asm/cacheflush.h>
 #include <asm/system_misc.h>
 
+/* TODO: Remove this include when KVM can support a kexec reboot. */
+#include <asm/virt.h>
+
 /* Global variables for the relocate_kernel routine. */
 extern const unsigned char relocate_new_kernel[];
 extern const unsigned long relocate_new_kernel_size;

@@ -100,6 +103,13 @@ int machine_kexec_prepare(struct kimage *image)
 
      kexec_image_info(image);
 

Quoted 11 lines

arch/arm64/kernel/machine_kexec.c int machine_kexec_prepare(struct kimage *image) · line 111

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c +6 -0

+     /* TODO: Remove this message when KVM can support a kexec reboot. */
+     if (IS_ENABLED(CONFIG_KVM) && is_hyp_mode_available()) {
+             pr_err("%s: Your kernel is configured with KVM support (CONFIG_KVM=y) which currently does not allow for kexec re-boot.\n",
+                     __func__);
+             return -ENOSYS;
+     }

If you really don't want to implement KVM teardown, surely this should be at the start of the series, so we don't have a point in the middle where things may explode in this case?

So this caters to support systems that don't support KVM (don't boot in EL2) but is configured with both KVM and KEXEC?

Why not just make the kexec config dependent on !KVM ?

Quoted 27 lines · Geoff Levand

Add runtime checks that fail the arm64 kexec syscall for situations that would result in system instability do to problems in the KVM kernel support. These checks should be removed when the KVM problems are resolved fixed.

Signed-off-by Geoff Levand <[email protected]>

---
 arch/arm64/kernel/machine_kexec.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

arch/arm64/kernel/machine_kexec.c +3 -0

index 3d84759..a36459d 100644
@@ -16,6 +16,9 @@
 #include <asm/cacheflush.h>
 #include <asm/system_misc.h>
 
+/* TODO: Remove this include when KVM can support a kexec reboot. */
+#include <asm/virt.h>
+
 /* Global variables for the relocate_kernel routine. */
 extern const unsigned char relocate_new_kernel[];
 extern const unsigned long relocate_new_kernel_size;

@@ -100,6 +103,13 @@ int machine_kexec_prepare(struct kimage *image)
 
 	kexec_image_info(image);
 

arch/arm64/kernel/machine_kexec.c int machine_kexec_prepare(struct kimage *image) · line 111

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c +6 -0

+	/* TODO: Remove this message when KVM can support a kexec reboot. */
+	if (IS_ENABLED(CONFIG_KVM) && is_hyp_mode_available()) {
+		pr_err("%s: Your kernel is configured with KVM support (CONFIG_KVM=y) which currently does not allow for kexec re-boot.\n",
+			__func__);
+		return -ENOSYS;
+	}

If you really don't want to implement KVM teardown, surely this should be at the start of the series, so we don't have a point in the middle where things may explode in this case?

Quoted 27 lines · Geoff Levand

Add runtime checks that fail the arm64 kexec syscall for situations that would result in system instability do to problems in the KVM kernel support. These checks should be removed when the KVM problems are resolved fixed.

Signed-off-by Geoff Levand <[email protected]>

---
  arch/arm64/kernel/machine_kexec.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

arch/arm64/kernel/machine_kexec.c +3 -0

index 3d84759..a36459d 100644
@@ -16,6 +16,9 @@
  #include <asm/cacheflush.h>
  #include <asm/system_misc.h>
 
+/* TODO: Remove this include when KVM can support a kexec reboot. */
+#include <asm/virt.h>
+
  /* Global variables for the relocate_kernel routine. */
  extern const unsigned char relocate_new_kernel[];
  extern const unsigned long relocate_new_kernel_size;

@@ -100,6 +103,13 @@ int machine_kexec_prepare(struct kimage *image)
 
  	kexec_image_info(image);
 

arch/arm64/kernel/machine_kexec.c int machine_kexec_prepare(struct kimage *image) · line 111

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c +6 -0

+	/* TODO: Remove this message when KVM can support a kexec reboot. */
+	if (IS_ENABLED(CONFIG_KVM) && is_hyp_mode_available()) {
+		pr_err("%s: Your kernel is configured with KVM support (CONFIG_KVM=y) which currently does not allow for kexec re-boot.\n",
+			__func__);
+		return -ENOSYS;
+	}

If you really don't want to implement KVM teardown, surely this should be at the start of the series, so we don't have a point in the middle where things may explode in this case?

Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 27 lines · Geoff Levand

Add runtime checks that fail the arm64 kexec syscall for situations that would result in system instability do to problems in the KVM kernel support. These checks should be removed when the KVM problems are resolved fixed.

Signed-off-by Geoff Levand <[email protected]>

---
  arch/arm64/kernel/machine_kexec.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

arch/arm64/kernel/machine_kexec.c +3 -0

index 3d84759..a36459d 100644
@@ -16,6 +16,9 @@
  #include <asm/cacheflush.h>
  #include <asm/system_misc.h>
 
+/* TODO: Remove this include when KVM can support a kexec reboot. */
+#include <asm/virt.h>
+
  /* Global variables for the relocate_kernel routine. */
  extern const unsigned char relocate_new_kernel[];
  extern const unsigned long relocate_new_kernel_size;

@@ -100,6 +103,13 @@ int machine_kexec_prepare(struct kimage *image)
 
  	kexec_image_info(image);
 

arch/arm64/kernel/machine_kexec.c int machine_kexec_prepare(struct kimage *image) · line 111

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c +6 -0

+	/* TODO: Remove this message when KVM can support a kexec reboot. */
+	if (IS_ENABLED(CONFIG_KVM) && is_hyp_mode_available()) {
+		pr_err("%s: Your kernel is configured with KVM support (CONFIG_KVM=y) which currently does not allow for kexec re-boot.\n",
+			__func__);
+		return -ENOSYS;
+	}

If you really don't want to implement KVM teardown, surely this should be at the start of the series, so we don't have a point in the middle where things may explode in this case?

Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hello,

Quoted 28 lines · Mark Rutland

Quoted 27 lines · Geoff Levand

Add runtime checks that fail the arm64 kexec syscall for situations that would result in system instability do to problems in the KVM kernel support. These checks should be removed when the KVM problems are resolved fixed.

Signed-off-by Geoff Levand <[email protected]>

---
  arch/arm64/kernel/machine_kexec.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

arch/arm64/kernel/machine_kexec.c +3 -0

index 3d84759..a36459d 100644
@@ -16,6 +16,9 @@
  #include <asm/cacheflush.h>
  #include <asm/system_misc.h>
 
+/* TODO: Remove this include when KVM can support a kexec reboot. */
+#include <asm/virt.h>
+
  /* Global variables for the relocate_kernel routine. */
  extern const unsigned char relocate_new_kernel[];
  extern const unsigned long relocate_new_kernel_size;

@@ -100,6 +103,13 @@ int machine_kexec_prepare(struct kimage *image)
 
  	kexec_image_info(image);
 

Quoted 10 lines

arch/arm64/kernel/machine_kexec.c int machine_kexec_prepare(struct kimage *image) · line 111

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c +6 -0

+	/* TODO: Remove this message when KVM can support a kexec reboot. */
+	if (IS_ENABLED(CONFIG_KVM) && is_hyp_mode_available()) {
+		pr_err("%s: Your kernel is configured with KVM support (CONFIG_KVM=y) which currently does not allow for kexec re-boot.\n",
+			__func__);
+		return -ENOSYS;
+	}

If you really don't want to implement KVM teardown, surely this should be at the start of the series, so we don't have a point in the middle where things may explode in this case?

I'm going to fix this KVM issue (teardown) in cooperation with Geoff.

Looking into kvm init code, kvm_arch_init() in particular, I guess that teardown function (kvm_arch_exit()) should do
(reverting kvm_timer_hyp_init() per cpu)
- stop arch timer

(reverting cpu_init_hyp_mode() per cpu)
- flush TLB
- jump into identical mapping (using boot_hyp_pgd?)
- disable MMU?

- restore vbar_el2 to __hyp_stub_vectors (or NULL?)

(reverting kvm_mmu_init())
- Do we need to free page tables and a bounce(trampoline) page?

Is this good enough for safely shutting down kvm? Do I miss anything essential, or can I skip anyhing?

Quoted 42 lines · AKASHI Takahiro

Hello,

Quoted 28 lines · Mark Rutland

Quoted 27 lines · Geoff Levand

Add runtime checks that fail the arm64 kexec syscall for situations that would result in system instability do to problems in the KVM kernel support. These checks should be removed when the KVM problems are resolved fixed.

Signed-off-by Geoff Levand <[email protected]>

---
  arch/arm64/kernel/machine_kexec.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

arch/arm64/kernel/machine_kexec.c +3 -0

index 3d84759..a36459d 100644
@@ -16,6 +16,9 @@
  #include <asm/cacheflush.h>
  #include <asm/system_misc.h>
 
+/* TODO: Remove this include when KVM can support a kexec reboot. */
+#include <asm/virt.h>
+
  /* Global variables for the relocate_kernel routine. */
  extern const unsigned char relocate_new_kernel[];
  extern const unsigned long relocate_new_kernel_size;

@@ -100,6 +103,13 @@ int machine_kexec_prepare(struct kimage *image)
 
  	kexec_image_info(image);
 

Quoted 10 lines

arch/arm64/kernel/machine_kexec.c int machine_kexec_prepare(struct kimage *image) · line 111

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c +6 -0

+	/* TODO: Remove this message when KVM can support a kexec reboot. */
+	if (IS_ENABLED(CONFIG_KVM) && is_hyp_mode_available()) {
+		pr_err("%s: Your kernel is configured with KVM support (CONFIG_KVM=y) which currently does not allow for kexec re-boot.\n",
+			__func__);
+		return -ENOSYS;
+	}

If you really don't want to implement KVM teardown, surely this should be at the start of the series, so we don't have a point in the middle where things may explode in this case?

Quoted 6 lines

I'm going to fix this KVM issue (teardown) in cooperation with Geoff.

Looking into kvm init code, kvm_arch_init() in particular, I guess that teardown function (kvm_arch_exit()) should do
(reverting kvm_timer_hyp_init() per cpu)
- stop arch timer

No need, there shouldn't be any guest running at that point.

Quoted 4 lines

(reverting cpu_init_hyp_mode() per cpu)
- flush TLB
- jump into identical mapping (using boot_hyp_pgd?)
- disable MMU?

Yes, and for that, you need to go back to an idmap

Quoted 1 line

- restore vbar_el2 to __hyp_stub_vectors (or NULL?)

It doesn't matter, you want to stay in HYP for the next kernel.

Quoted 62 lines · Marc Zyngier

Quoted 42 lines · AKASHI Takahiro

Hello,

Quoted 28 lines · Mark Rutland

Quoted 27 lines · Geoff Levand

Add runtime checks that fail the arm64 kexec syscall for situations that would result in system instability do to problems in the KVM kernel support. These checks should be removed when the KVM problems are resolved fixed.

Signed-off-by Geoff Levand <[email protected]>

---
   arch/arm64/kernel/machine_kexec.c | 10 ++++++++++
   1 file changed, 10 insertions(+)

arch/arm64/kernel/machine_kexec.c +3 -0

index 3d84759..a36459d 100644
@@ -16,6 +16,9 @@
   #include <asm/cacheflush.h>
   #include <asm/system_misc.h>
 
+/* TODO: Remove this include when KVM can support a kexec reboot. */
+#include <asm/virt.h>
+
   /* Global variables for the relocate_kernel routine. */
   extern const unsigned char relocate_new_kernel[];
   extern const unsigned long relocate_new_kernel_size;

@@ -100,6 +103,13 @@ int machine_kexec_prepare(struct kimage *image)
 
   	kexec_image_info(image);
 

Quoted 10 lines

arch/arm64/kernel/machine_kexec.c int machine_kexec_prepare(struct kimage *image) · line 111

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c +6 -0

+	/* TODO: Remove this message when KVM can support a kexec reboot. */
+	if (IS_ENABLED(CONFIG_KVM) && is_hyp_mode_available()) {
+		pr_err("%s: Your kernel is configured with KVM support (CONFIG_KVM=y) which currently does not allow for kexec re-boot.\n",
+			__func__);
+		return -ENOSYS;
+	}

If you really don't want to implement KVM teardown, surely this should be at the start of the series, so we don't have a point in the middle where things may explode in this case?

Quoted 6 lines

I'm going to fix this KVM issue (teardown) in cooperation with Geoff.

Looking into kvm init code, kvm_arch_init() in particular, I guess that teardown function (kvm_arch_exit()) should do
(reverting kvm_timer_hyp_init() per cpu)
- stop arch timer

No need, there shouldn't be any guest running at that point.

Quoted 4 lines

(reverting cpu_init_hyp_mode() per cpu)
- flush TLB
- jump into identical mapping (using boot_hyp_pgd?)
- disable MMU?

Yes, and for that, you need to go back to an idmap

Quoted 1 line

- restore vbar_el2 to __hyp_stub_vectors (or NULL?)

Quoted 1 line

It doesn't matter, you want to stay in HYP for the next kernel.

Well, it depends on how the teardown is implemented. I'd imagined we'd have KVM tear itself down (restoring the hyp stub) as part of shut down. Later kexec/soft_restart would call the stub to get back to EL2.

That way kexec doesn't need to know anything about KVM, and it has one path that works regardless of whether KVM is compiled into the kernel.

Mark.

Hello Marc, Mark

Thank you for your useful comments.
I need to look at Marc's commit(5a677ce044f) more carefully about trampoline code.

Quoted 63 lines · Mark Rutland

Quoted 62 lines · Marc Zyngier

Quoted 42 lines · AKASHI Takahiro

Hello,

Quoted 28 lines · Mark Rutland

Quoted 27 lines · Geoff Levand

Add runtime checks that fail the arm64 kexec syscall for situations that would result in system instability do to problems in the KVM kernel support. These checks should be removed when the KVM problems are resolved fixed.

Signed-off-by Geoff Levand <[email protected]>

---
   arch/arm64/kernel/machine_kexec.c | 10 ++++++++++
   1 file changed, 10 insertions(+)

arch/arm64/kernel/machine_kexec.c +3 -0

index 3d84759..a36459d 100644
@@ -16,6 +16,9 @@
   #include <asm/cacheflush.h>
   #include <asm/system_misc.h>
 
+/* TODO: Remove this include when KVM can support a kexec reboot. */
+#include <asm/virt.h>
+
   /* Global variables for the relocate_kernel routine. */
   extern const unsigned char relocate_new_kernel[];
   extern const unsigned long relocate_new_kernel_size;

@@ -100,6 +103,13 @@ int machine_kexec_prepare(struct kimage *image)
 
   	kexec_image_info(image);
 

Quoted 10 lines

arch/arm64/kernel/machine_kexec.c int machine_kexec_prepare(struct kimage *image) · line 111

Quoted 6 lines

arch/arm64/kernel/machine_kexec.c +6 -0

+	/* TODO: Remove this message when KVM can support a kexec reboot. */
+	if (IS_ENABLED(CONFIG_KVM) && is_hyp_mode_available()) {
+		pr_err("%s: Your kernel is configured with KVM support (CONFIG_KVM=y) which currently does not allow for kexec re-boot.\n",
+			__func__);
+		return -ENOSYS;
+	}

If you really don't want to implement KVM teardown, surely this should be at the start of the series, so we don't have a point in the middle where things may explode in this case?

Quoted 6 lines

I'm going to fix this KVM issue (teardown) in cooperation with Geoff.

Looking into kvm init code, kvm_arch_init() in particular, I guess that teardown function (kvm_arch_exit()) should do
(reverting kvm_timer_hyp_init() per cpu)
- stop arch timer

No need, there shouldn't be any guest running at that point.

Quoted 4 lines

(reverting cpu_init_hyp_mode() per cpu)
- flush TLB
- jump into identical mapping (using boot_hyp_pgd?)
- disable MMU?

Yes, and for that, you need to go back to an idmap

Quoted 1 line

- restore vbar_el2 to __hyp_stub_vectors (or NULL?)

Quoted 8 lines

Quoted 1 line

It doesn't matter, you want to stay in HYP for the next kernel.

Well, it depends on how the teardown is implemented. I'd imagined we'd have KVM tear itself down (restoring the hyp stub) as part of shut down. Later kexec/soft_restart would call the stub to get back to EL2.

That way kexec doesn't need to know anything about KVM, and it has one path that works regardless of whether KVM is compiled into the kernel.

Initially, I thought that we would define kvm_arch_exit() and call it somewhere in the middle of kexec path (no idea yet).
But Geoff suggested me to implement a new hvc call, HVC_CPU_SHUTDOWN(??), and make it called via cpu_notifier(CPU_DYING_FROZEN) initiated by machine_shutdown() from kernel_kexec().
(As you know, a hook is already there for cpu online in kvm/arm.c.)

Initially, I thought that we would define kvm_arch_exit() and call it somewhere in the middle of kexec path (no idea yet).
But Geoff suggested me to implement a new hvc call, HVC_CPU_SHUTDOWN(??), and make it called via cpu_notifier(CPU_DYING_FROZEN) initiated by machine_shutdown() from kernel_kexec().

Hi Takahiro.

Quoted 5 lines · AKASHI Takahiro

Initially, I thought that we would define kvm_arch_exit() and call it somewhere in the middle of kexec path (no idea yet).
But Geoff suggested me to implement a new hvc call, HVC_CPU_SHUTDOWN(??), and make it called via cpu_notifier(CPU_DYING_FROZEN) initiated by machine_shutdown() from kernel_kexec().

As an initial implementation we can hook into the CPU_DYING_FROZEN notifier sent to hyp_init_cpu_notify(). The longer term solution should use kvm_arch_hardware_enable() and kvm_arch_hardware_disable().

The calls to cpu_notifier(CPU_DYING_FROZEN) are part of cpu hot plug, and independent of kexec. If someone were to add spin-table cpu un-plug, then it would be used for that also. It seems we should be able to test without kexec by using cpu hot plug.

To tear down KVM you need to get back to hyp mode, and hence the need for HVC_CPU_SHUTDOWN. The sequence I envisioned would be like this:

cpu_notifier(CPU_DYING_FROZEN)
-> kvm_cpu_shutdown()
prepare for hvc
-> HVC_CPU_SHUTDOWN
now in hyp mode, do KVM tear down, restore default exception vectors

Once the default exception vectors are restored soft_restart() can then execute the cpu_reset routine in EL2.

Some notes are here for those with access: https://cards.linaro.org/browse/KWG-611

-Geoff

Quoted 3 lines · Geoff Levand

As an initial implementation we can hook into the CPU_DYING_FROZEN notifier sent to hyp_init_cpu_notify(). The longer term solution should use kvm_arch_hardware_enable() and kvm_arch_hardware_disable().

Are these two different approaches?

kernel_exec() -> machine_shutdown() -> disable_nonboot_cpu()

    -> _cpu_down() -> cpu_notify_nofail(CPU_DEAD|...)

On the other hand, kvm already has a hook into kvm_arch_hardware_disable():
(ignoring kvm_usage_count here)
kvm_cpu_hotplug(CPU_DYING) -> hardware_disable()

    -> hardware_disable_nolock() -> kvm_arch_hardware_disable()

So it seems that we don't have to add a new hook at hyp_init_cpu_notify() if kvm_arch_hardware_disable() is properly implemented.

disable_nonboot_cpu() will not inovke cpu hotplug on *boot* cpu, and we should handle it in a separate way though.

Hi Takahiro,

Quoted 6 lines · AKASHI Takahiro

Quoted 3 lines · Geoff Levand

As an initial implementation we can hook into the CPU_DYING_FROZEN notifier sent to hyp_init_cpu_notify(). The longer term solution should use kvm_arch_hardware_enable() and kvm_arch_hardware_disable().

Are these two different approaches?

Yes, these are two different solutions, One initial work-around, and a more involved proper solution. Hooking into the CPU_DYING_FROZEN notifier would be a initial fix. The proper solution would be to move the KVM setup to kvm_arch_hardware_enable(), and the shutdown to kvm_arch_hardware_disable().

Quoted 10 lines

kernel_exec() -> machine_shutdown() -> disable_nonboot_cpu()

     -> _cpu_down() -> cpu_notify_nofail(CPU_DEAD|...)

On the other hand, kvm already has a hook into kvm_arch_hardware_disable():
(ignoring kvm_usage_count here)
kvm_cpu_hotplug(CPU_DYING) -> hardware_disable()

     -> hardware_disable_nolock() -> kvm_arch_hardware_disable()

So it seems that we don't have to add a new hook at hyp_init_cpu_notify() if kvm_arch_hardware_disable() is properly implemented.

Yes, that is correct. But, as above, you would also need to update the KVM startup to use kvm_arch_hardware_enable().

Quoted 2 lines

disable_nonboot_cpu() will not inovke cpu hotplug on *boot* cpu, and we should handle it in a separate way though.

IIRC, the secondary cpus go through PSCI on shutdown, and that path is working OK. Maybe I am mistaken though.

The primary cpu shutdown (hyp stubs restored) is what is missing. The primary cpu goes through cpu_soft_restart(), and that is what is currently failing.

-Geoff

Quoted 31 lines · Geoff Levand

Hi Takahiro,

Quoted 6 lines · AKASHI Takahiro

Quoted 3 lines · Geoff Levand

As an initial implementation we can hook into the CPU_DYING_FROZEN notifier sent to hyp_init_cpu_notify(). The longer term solution should use kvm_arch_hardware_enable() and kvm_arch_hardware_disable().

Are these two different approaches?

Yes, these are two different solutions, One initial work-around, and a more involved proper solution. Hooking into the CPU_DYING_FROZEN notifier would be a initial fix. The proper solution would be to move the KVM setup to kvm_arch_hardware_enable(), and the shutdown to kvm_arch_hardware_disable().

Quoted 10 lines

kernel_exec() -> machine_shutdown() -> disable_nonboot_cpu()

     -> _cpu_down() -> cpu_notify_nofail(CPU_DEAD|...)

On the other hand, kvm already has a hook into kvm_arch_hardware_disable():
(ignoring kvm_usage_count here)
kvm_cpu_hotplug(CPU_DYING) -> hardware_disable()

     -> hardware_disable_nolock() -> kvm_arch_hardware_disable()

So it seems that we don't have to add a new hook at hyp_init_cpu_notify() if kvm_arch_hardware_disable() is properly implemented.

Yes, that is correct. But, as above, you would also need to update the KVM startup to use kvm_arch_hardware_enable().

Quoted 5 lines

Quoted 2 lines

disable_nonboot_cpu() will not inovke cpu hotplug on *boot* cpu, and we should handle it in a separate way though.

IIRC, the secondary cpus go through PSCI on shutdown, and that path is working OK. Maybe I am mistaken though.

If so, why should we add a hook at hyp_init_cpu_notify() as initial work-around?

Quoted 3 lines

The primary cpu shutdown (hyp stubs restored) is what is missing. The primary cpu goes through cpu_soft_restart(), and that is what is currently failing.

Yeah, we will call teardown function manually in soft_restart();

This series adds the core support for kexec re-boots on arm64. This v7 of the series is mainly just a rebase to the latest arm64 for-next/core branch (v3.19-rc4), and a few very minor changes requested for v6.

Several things are known to have problems on kexec re-boot:

spin-table

FIX: Upgrade system firmware to provide PSCI enable method support or add missing spin-table support to the kernel.

ACPI
----

PROBLEM: The kernel for ACPI based systems does not export a device tree to the standard user space location of 'proc/device-tree'. Current applications expect to access device tree information from this standard location.

WORK-AROUND: Disable ACPI in firmware, OR pass 'acpi=off' on the first stage kernel command line, OR pass a user specified DTB using the kexec --dtb option.

FIX: FIX: An interface to expose a binary device tree to user space has been proposed. User kexec utilities will need to be updated to add support for this new interface.

Hi Geoff,

Quoted 3 lines · Geoff Levand

This series adds the core support for kexec re-boots on arm64. This v7 of the series is mainly just a rebase to the latest arm64 for-next/core branch (v3.19-rc4), and a few very minor changes requested for v6.

I haven't looked at the series in detail before, so some of my comments may have already been discussed.

Quoted 3 lines

Several things are known to have problems on kexec re-boot:

spin-table

I think that's not too bad, for complete kexec support (SMP->SMP) we can require some CPU unplug mechanism and PSCI is one of them.

Quoted 2 lines

FIX: Upgrade system firmware to provide PSCI enable method support or add missing spin-table support to the kernel.

What's the missing spin-table support?

Quoted 13 lines

ACPI
----

PROBLEM: The kernel for ACPI based systems does not export a device tree to the standard user space location of 'proc/device-tree'. Current applications expect to access device tree information from this standard location.

WORK-AROUND: Disable ACPI in firmware, OR pass 'acpi=off' on the first stage kernel command line, OR pass a user specified DTB using the kexec --dtb option.

FIX: FIX: An interface to expose a binary device tree to user space has been proposed. User kexec utilities will need to be updated to add support for this new interface.

So the fix here is to boot the second stage kernel with dtb, which means that we mandate the existence of a DT file for any ACPI system. Are there plans to make the kexec'ed kernel reuse the ACPI tables?

Hi Geoff,

Quoted 3 lines · Geoff Levand

This series adds the core support for kexec re-boots on arm64. This v7 of the series is mainly just a rebase to the latest arm64 for-next/core branch (v3.19-rc4), and a few very minor changes requested for v6.

I haven't looked at the series in detail before, so some of my comments may have already been discussed.

Quoted 3 lines

Several things are known to have problems on kexec re-boot:

spin-table

I think that's not too bad, for complete kexec support (SMP->SMP) we can require some CPU unplug mechanism and PSCI is one of them.

Quoted 2 lines

FIX: Upgrade system firmware to provide PSCI enable method support or add missing spin-table support to the kernel.

What's the missing spin-table support?

Quoted 3 lines · Geoff Levand

This series adds the core support for kexec re-boots on arm64. This v7 of the series is mainly just a rebase to the latest arm64 for-next/core branch (v3.19-rc4), and a few very minor changes requested for v6.

I haven't looked at the series in detail before, so some of my comments may have already been discussed.

Quoted 3 lines

Several things are known to have problems on kexec re-boot:

spin-table

I think that's not too bad, for complete kexec support (SMP->SMP) we can require some CPU unplug mechanism and PSCI is one of them.

Quoted 2 lines

FIX: Upgrade system firmware to provide PSCI enable method support or add missing spin-table support to the kernel.

What's the missing spin-table support?

Quoted 11 lines

ACPI
----

PROBLEM: The kernel for ACPI based systems does not export a device tree to the standard user space location of 'proc/device-tree'. Current applications expect to access device tree information from this standard location.

WORK-AROUND: Disable ACPI in firmware, OR pass 'acpi=off' on the first stage kernel command line, OR pass a user specified DTB using the kexec --dtb option.

Quoted 3 lines

FIX: FIX: An interface to expose a binary device tree to user space has been proposed. User kexec utilities will need to be updated to add support for this new interface.

So the fix here is to boot the second stage kernel with dtb, which means that we mandate the existence of a DT file for any ACPI system. Are there plans to make the kexec'ed kernel reuse the ACPI tables?