| Message ID | c23696b455e99b5b111ae559c7998f3c56a664df.1457977402.git.geoff@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Mon, Mar 14, 2016 at 05:48:00PM +0000, Geoff Levand wrote: > 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. > > 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 16 bit immediate value of the HVC instruction > to specify the hcall type. I'm a bit concerned about namespace pollution on the HVC immediate here. Existing users tend allocate a single "random" number to identify the API -- Xen and Jailhouse do this for example. If we start using the HVC immediate to select functions, not just APIs, the space is going to fill up a lot faster, if we have a multiplex multiple APIs through it. (We don't currently seem to multiplex APIs much here, except that we do use HVC for PSCI calls from the guest, and it could be used for additional paravirtualised services in the future). > 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. It would also be preferable to keep the 32-bit and 64-bit APIs the same; we should avoid having them different unless there's a clinching technical reason... There may be some historical context for this that I'm missing... Cheers ---Dave > > Signed-off-by: Geoff Levand <geoff@infradead.org> > Signed-off-by: James Morse <james.morse@arm.com> > --- > arch/arm64/include/asm/virt.h | 27 +++++++++++++++++++++++++++ > arch/arm64/kernel/hyp-stub.S | 32 +++++++++++++++++++++----------- > arch/arm64/kvm/hyp.S | 3 ++- > arch/arm64/kvm/hyp/hyp-entry.S | 9 ++++++--- > 4 files changed, 56 insertions(+), 15 deletions(-) > [...]
Hi, On Tue, 2016-03-15 at 13:50 +0000, Dave Martin wrote: > On Mon, Mar 14, 2016 at 05:48:00PM +0000, Geoff Levand wrote: > > 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. > > > > 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 16 bit immediate value of the HVC instruction > > to specify the hcall type. > > I'm a bit concerned about namespace pollution on the HVC immediate here. > Existing users tend allocate a single "random" number to identify the > API -- Xen and Jailhouse do this for example. > > If we start using the HVC immediate to select functions, not just APIs, > the space is going to fill up a lot faster, if we have a multiplex > multiple APIs through it. This was discussed and concluded that we have 16 bits to fill up, and that is enough. Functions can still be multiplexed through a single HVC immediate if the user chooses to do so. > > (We don't currently seem to multiplex APIs much here, except that we > do use HVC for PSCI calls from the guest, and it could be used for > additional paravirtualised services in the future). > > > 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. > > It would also be preferable to keep the 32-bit and 64-bit APIs the same; > we should avoid having them different unless there's a clinching > technical reason... Please expand on why you see it as preferable. What problems do you see? -Geoff
On Tue, Mar 15, 2016 at 11:15:10AM -0700, Geoff Levand wrote: > Hi, > > On Tue, 2016-03-15 at 13:50 +0000, Dave Martin wrote: > > On Mon, Mar 14, 2016 at 05:48:00PM +0000, Geoff Levand wrote: > > > 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. > > > > > > 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 16 bit immediate value of the HVC instruction > > > to specify the hcall type. > > > > I'm a bit concerned about namespace pollution on the HVC immediate here. > > Existing users tend allocate a single "random" number to identify the > > API -- Xen and Jailhouse do this for example. > > > > If we start using the HVC immediate to select functions, not just APIs, > > the space is going to fill up a lot faster, if we have a multiplex > > multiple APIs through it. > > This was discussed and concluded that we have 16 bits to fill up, > and that is enough. Functions can still be multiplexed through a Enough for what? > single HVC immediate if the user chooses to do so. But KVM can't? The HVC #imm space doesn't seem to be managed, which implies that discovery and/or renumbering mechanisms would be needed if we end up wanting to mux multiple ABIs through there. The tighter limitation on immediate size, and the need for code patching if translation of HVC numbers is needed, mean that this can be harder when using the HVC immediate for demux rather than an ordinary register. Currently, the only other ABI muxed through HVC is PSCI, but it already looks like there is a potential collision -- HVC #0 from EL1 is already KVM_CALL_HYP or a PSCI call, and we rely on knowing whether the call came from the host or guest to demux it properly. This kind of problem is likely to proliferate over time. > > (We don't currently seem to multiplex APIs much here, except that we > > do use HVC for PSCI calls from the guest, and it could be used for > > additional paravirtualised services in the future). > > > > > 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. > > > > It would also be preferable to keep the 32-bit and 64-bit APIs the same; > > we should avoid having them different unless there's a clinching > > technical reason... > > Please expand on why you see it as preferable. What problems do > you see? Fragmentation avoidance is the main argument I see. The architectural constraints and the problem to be solved are basically the same between 32- and 64-bit, AFAICT. Cheers ---Dave
On 16/03/16 13:50, Dave Martin wrote: > On Tue, Mar 15, 2016 at 11:15:10AM -0700, Geoff Levand wrote: >> Hi, >> >> On Tue, 2016-03-15 at 13:50 +0000, Dave Martin wrote: >>> On Mon, Mar 14, 2016 at 05:48:00PM +0000, Geoff Levand wrote: >>>> 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. >>>> >>>> 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 16 bit immediate value of the HVC instruction >>>> to specify the hcall type. >>> >>> I'm a bit concerned about namespace pollution on the HVC immediate here. >>> Existing users tend allocate a single "random" number to identify the >>> API -- Xen and Jailhouse do this for example. >>> >>> If we start using the HVC immediate to select functions, not just APIs, >>> the space is going to fill up a lot faster, if we have a multiplex >>> multiple APIs through it. >> >> This was discussed and concluded that we have 16 bits to fill up, >> and that is enough. Functions can still be multiplexed through a > > Enough for what? > >> single HVC immediate if the user chooses to do so. > > But KVM can't? > > The HVC #imm space doesn't seem to be managed, which implies that > discovery and/or renumbering mechanisms would be needed if we end up > wanting to mux multiple ABIs through there. The tighter limitation > on immediate size, and the need for code patching if translation of > HVC numbers is needed, mean that this can be harder when using the HVC > immediate for demux rather than an ordinary register. > > Currently, the only other ABI muxed through HVC is PSCI, but it > already looks like there is a potential collision -- HVC #0 from EL1 is > already KVM_CALL_HYP or a PSCI call, and we rely on knowing whether > the call came from the host or guest to demux it properly. > > This kind of problem is likely to proliferate over time. > >>> (We don't currently seem to multiplex APIs much here, except that we >>> do use HVC for PSCI calls from the guest, and it could be used for >>> additional paravirtualised services in the future). >>> >>>> 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. >>> >>> It would also be preferable to keep the 32-bit and 64-bit APIs the same; >>> we should avoid having them different unless there's a clinching >>> technical reason... >> >> Please expand on why you see it as preferable. What problems do >> you see? > > Fragmentation avoidance is the main argument I see. The architectural > constraints and the problem to be solved are basically the same between > 32- and 64-bit, AFAICT. +1. I never quite understood why we went from a single HVC immediate + a register indicating the operation to a proliferation of immediate values (and still the need for a register to indicate the operation in most cases). This seems to go in a direction that is diametrically opposite the the "normal" ARM way. That doesn't make it an invalid approach, but uniformity with other APIs (PSCI for example) and the 32bit KVM code seems a highly desirable feature (given that I'll end up maintaining that code). Thanks, M.
Hi Marc, On Wed, 2016-03-16 at 14:09 +0000, Marc Zyngier wrote: > This seems to go in a direction that is diametrically opposite the > the > "normal" ARM way. That doesn't make it an invalid approach, but > uniformity with other APIs (PSCI for example) and the 32bit KVM code > seems a highly desirable feature (given that I'll end up maintaining > that code). We need a way to get the CPU back to the exception level it had on entry to the kernel, and this hcall change is part of my proposed solution. If you could outline something you think would be a better fit, I'll take that and work on an implementation of it. -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/kvm/hyp.S b/arch/arm64/kvm/hyp.S index 0ccdcbb..a598f9e 100644 --- a/arch/arm64/kvm/hyp.S +++ b/arch/arm64/kvm/hyp.S @@ -18,6 +18,7 @@ #include <linux/linkage.h> #include <asm/assembler.h> +#include <asm/virt.h> /* * u64 kvm_call_hyp(void *hypfn, ...); @@ -38,6 +39,6 @@ * arch/arm64/kernel/hyp_stub.S. */ ENTRY(kvm_call_hyp) - hvc #0 + hvc #HVC_CALL_HYP ret ENDPROC(kvm_call_hyp) diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S index 93e8d983..a1edf77 100644 --- a/arch/arm64/kvm/hyp/hyp-entry.S +++ b/arch/arm64/kvm/hyp/hyp-entry.S @@ -43,6 +43,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 @@ -51,14 +52,16 @@ el1_sync: // Guest trapped into EL2 cbnz x3, el1_trap // called HVC /* Here, we're pretty sure the host called HVC. */ + mov x18, x0 restore_x0_to_x3 - /* Check for __hyp_get_vectors */ - cbnz x0, 1f + cmp x18, #HVC_GET_VECTORS + b.ne 1f mrs x0, vbar_el2 b 2f -1: stp lr, xzr, [sp, #-16]! +1: /* Default to HVC_CALL_HYP. */ + push lr, xzr /* * Compute the function address in EL2, and shuffle the parameters.