| Message ID | 1422660828.21823.40.camel@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 30, 2015 at 03:33:48PM -0800, Geoff Levand wrote: > 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? > > 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? > > 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 <geoff@infradead.org> > --- > 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(-) > > diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h > index 7a5df52..eb10368 100644 > --- a/arch/arm64/include/asm/virt.h > +++ b/arch/arm64/include/asm/virt.h > @@ -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? Thanks, -Christoffer
Hi Christoffer, On Thu, 2015-02-19 at 21:57 +0100, Christoffer Dall wrote: > On Fri, Jan 30, 2015 at 03:33:48PM -0800, Geoff Levand wrote: > > 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. > > 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. > > 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 <geoff@infradead.org> > > --- > > 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(-) > > > > diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h > > index 7a5df52..eb10368 100644 > > --- a/arch/arm64/include/asm/virt.h > > +++ b/arch/arm64/include/asm/virt.h > > @@ -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? 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. -Geoff
On Wed, Feb 25, 2015 at 02:09:30PM -0800, Geoff Levand wrote: > Hi Christoffer, > > On Thu, 2015-02-19 at 21:57 +0100, Christoffer Dall wrote: > > On Fri, Jan 30, 2015 at 03:33:48PM -0800, Geoff Levand wrote: > > > 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. > > > > 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. > 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? > > > 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 <geoff@infradead.org> > > > --- > > > 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(-) > > > > > > diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h > > > index 7a5df52..eb10368 100644 > > > --- a/arch/arm64/include/asm/virt.h > > > +++ b/arch/arm64/include/asm/virt.h > > > @@ -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? > > 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.? -Christoffer
Hi Christoffer, On Mon, 2015-03-02 at 14:13 -0800, Christoffer Dall wrote: > On Wed, Feb 25, 2015 at 02:09:30PM -0800, Geoff Levand wrote: > > 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? 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. > > > > + * 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? > > > > 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.? OK, I will change these to x9, x10. I'll post a v8 patch set soon. -Geoff
Hi Geoff, On 03/02/2015 06:22 PM, Geoff Levand wrote: > Hi Christoffer, > > On Mon, 2015-03-02 at 14:13 -0800, Christoffer Dall wrote: >> On Wed, Feb 25, 2015 at 02:09:30PM -0800, Geoff Levand wrote: >>> 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? > > 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 Chris
Hi Christopher, On Tue, 2015-03-03 at 16:47 -0500, Christopher Covington wrote: > On 03/02/2015 06:22 PM, Geoff Levand wrote: > > 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 On looking at the SMC document, I found this: The SMC instruction encodes an immediate value as defined by the ARM architecture [1][2]. The size of this and mechanism to access the immediate value differ between the ARM instruction sets. Additionally, it is time consuming for 32-bit Secure Monitor code to access this immediate value. Consequently: o An SMC immediate value of Zero must be used. o All other SMC immediate values are reserved. The first problem of differing access methods does not exist for our case, the kernel will always use the same method. As for the second problem, the current implementation already reads esr_el2. The new code just adds an AND instruction to mask the ISS field. I don't think this would be more overhead than shifting registers. One alternative would be to use a high register, say x7, and limit the hcalls to args x0-x6, but I don't think this gains much over using the immediate. -Geoff
diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h index 7a5df52..eb10368 100644 --- a/arch/arm64/include/asm/virt.h +++ b/arch/arm64/include/asm/virt.h @@ -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. + */ + +/* + * HVC_CALL_HYP - Execute a hyp routine. + */ + +#define HVC_CALL_HYP 0 + +/* + * 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 + #define BOOT_CPU_MODE_EL1 (0xe11) #define BOOT_CPU_MODE_EL2 (0xe12) diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S index a272f33..017ab519 100644 --- a/arch/arm64/kernel/hyp-stub.S +++ b/arch/arm64/kernel/hyp-stub.S @@ -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 + 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 - cbz x0, 1f - msr vbar_el2, x0 // Set vbar_el2 + + 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 ENDPROC(el1_sync) @@ -100,11 +109,12 @@ ENDPROC(\label) * initialisation entry point. */ -ENTRY(__hyp_get_vectors) - mov x0, xzr - // fall through ENTRY(__hyp_set_vectors) - hvc #0 + hvc #HVC_SET_VECTORS ret -ENDPROC(__hyp_get_vectors) ENDPROC(__hyp_set_vectors) + +ENTRY(__hyp_get_vectors) + hvc #HVC_GET_VECTORS + ret +ENDPROC(__hyp_get_vectors) diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c index 3425f31..7043fd7 100644 --- a/arch/arm64/kernel/psci.c +++ b/arch/arm64/kernel/psci.c @@ -123,7 +123,8 @@ static noinline int __invoke_psci_fn_hvc(u64 function_id, u64 arg0, u64 arg1, __asmeq("%3", "x3") "hvc #0\n" : "+r" (function_id) - : "r" (arg0), "r" (arg1), "r" (arg2)); + : "r" (arg0), "r" (arg1), "r" (arg2) + : "x17", "x18"); return function_id; } diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S index c0d8202..42c9851 100644 --- a/arch/arm64/kvm/hyp.S +++ b/arch/arm64/kvm/hyp.S @@ -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,18 @@ el1_sync: // Guest trapped into EL2 cbnz x3, el1_trap // called HVC /* Here, we're pretty sure the host called HVC. */ + mov x18, x0 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 -1: push lr, xzr +1: /* Default to HVC_CALL_HYP. */ + + push lr, xzr /* * Compute the function address in EL2, and shuffle the parameters.
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 <geoff@infradead.org> --- 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(-)