| Message ID | 20200430144831.59194-2-dbrazdil@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Split off nVHE hyp code | expand |
On Thu, 30 Apr 2020 15:48:17 +0100, David Brazdil <dbrazdil@google.com> wrote: > > From: Quentin Perret <qperret@google.com> > > Currently, the arm64 KVM code provides __kvm_call_hyp assembly procedure which > does nothing but call the HVC instruction. This is used to call functions by > their pointer in EL2 under nVHE, and abused by __cpu_init_hyp_mode to pass > a data pointer. The hyp-stub code, on the other hand, has its own assembly > procedures for (re)setting hyp vectors. > > In preparation for a clean-up of the KVM hypercall interface, unify all HVC > users behind __kvm_call_hyp and remove comments about expected meaning of > arguments. But the arguments still have a meaning, don't they? See below. > > No functional changes intended. > > Signed-off-by: Quentin Perret <qperret@google.com> > Signed-off-by: David Brazdil <dbrazdil@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 12 ++++++----- > arch/arm64/include/asm/virt.h | 33 ++++++++++++++++++++++++++++-- > arch/arm64/kernel/hyp-stub.S | 34 ------------------------------- > arch/arm64/kvm/hyp.S | 13 +----------- > 4 files changed, 39 insertions(+), 53 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 32c8a675e5a4..e61143d6602d 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -25,6 +25,7 @@ > #include <asm/kvm.h> > #include <asm/kvm_asm.h> > #include <asm/thread_info.h> > +#include <asm/virt.h> > > #define __KVM_HAVE_ARCH_INTC_INITIALIZED > > @@ -446,7 +447,8 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva); > void kvm_arm_halt_guest(struct kvm *kvm); > void kvm_arm_resume_guest(struct kvm *kvm); > > -u64 __kvm_call_hyp(void *hypfn, ...); > +#define kvm_call_hyp_nvhe(hypfn, ...) \ > + __kvm_call_hyp((unsigned long)kvm_ksym_ref(hypfn), ##__VA_ARGS__) > > /* > * The couple of isb() below are there to guarantee the same behaviour > @@ -459,7 +461,7 @@ u64 __kvm_call_hyp(void *hypfn, ...); > f(__VA_ARGS__); \ > isb(); \ > } else { \ > - __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \ > + kvm_call_hyp_nvhe(f, ##__VA_ARGS__); \ > } \ > } while(0) > > @@ -471,8 +473,7 @@ u64 __kvm_call_hyp(void *hypfn, ...); > ret = f(__VA_ARGS__); \ > isb(); \ > } else { \ > - ret = __kvm_call_hyp(kvm_ksym_ref(f), \ > - ##__VA_ARGS__); \ > + ret = kvm_call_hyp_nvhe(f, ##__VA_ARGS__); \ > } \ > \ > ret; \ > @@ -551,7 +552,8 @@ static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr, > * cpus_have_const_cap() wrapper. > */ > BUG_ON(!system_capabilities_finalized()); > - __kvm_call_hyp((void *)pgd_ptr, hyp_stack_ptr, vector_ptr, tpidr_el2); > + __kvm_call_hyp((unsigned long)pgd_ptr, hyp_stack_ptr, vector_ptr, > + tpidr_el2); > > /* > * Disabling SSBD on a non-VHE system requires us to enable SSBS > diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h > index 61fd26752adc..fdc11f819b06 100644 > --- a/arch/arm64/include/asm/virt.h > +++ b/arch/arm64/include/asm/virt.h > @@ -62,8 +62,37 @@ > */ > extern u32 __boot_cpu_mode[2]; > > -void __hyp_set_vectors(phys_addr_t phys_vector_base); > -void __hyp_reset_vectors(void); > +/* Make HVC call into the hypervisor. */ > +extern u64 __kvm_call_hyp(unsigned long arg, ...); > + > +/* > + * __hyp_set_vectors: Call this after boot to set the initial hypervisor > + * vectors as part of hypervisor installation. On an SMP system, this should > + * be called on each CPU. > + * > + * @phys_vector_base must be the physical address of the new vector table, and > + * must be 2KB aligned. > + * > + * Before calling this, you must check that the stub hypervisor is installed > + * everywhere, by waiting for any secondary CPUs to be brought up and then > + * checking that is_hyp_mode_available() is true. > + * > + * If not, there is a pre-existing hypervisor, some CPUs failed to boot, or > + * something else went wrong... in such cases, trying to install a new > + * hypervisor is unlikely to work as desired. > + * > + * When you call into your shiny new hypervisor, sp_el2 will contain junk, > + * so you will need to set that to something sensible at the new hypervisor's > + * initialisation entry point. > + */ > +static inline void __hyp_set_vectors(phys_addr_t phys_vector_base) > +{ > + __kvm_call_hyp(HVC_SET_VECTORS, phys_vector_base); > +} > +static inline void __hyp_reset_vectors(void) > +{ > + __kvm_call_hyp(HVC_RESET_VECTORS); > +} > > /* Reports the availability of HYP mode */ > static inline bool is_hyp_mode_available(void) > diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S > index e473ead806ed..78d4ec5c4290 100644 > --- a/arch/arm64/kernel/hyp-stub.S > +++ b/arch/arm64/kernel/hyp-stub.S > @@ -84,37 +84,3 @@ ENDPROC(\label) > invalid_vector el1_irq_invalid > invalid_vector el1_fiq_invalid > invalid_vector el1_error_invalid > - > -/* > - * __hyp_set_vectors: Call this after boot to set the initial hypervisor > - * vectors as part of hypervisor installation. On an SMP system, this should > - * be called on each CPU. > - * > - * x0 must be the physical address of the new vector table, and must be > - * 2KB aligned. > - * > - * Before calling this, you must check that the stub hypervisor is installed > - * everywhere, by waiting for any secondary CPUs to be brought up and then > - * checking that is_hyp_mode_available() is true. > - * > - * If not, there is a pre-existing hypervisor, some CPUs failed to boot, or > - * something else went wrong... in such cases, trying to install a new > - * hypervisor is unlikely to work as desired. > - * > - * When you call into your shiny new hypervisor, sp_el2 will contain junk, > - * so you will need to set that to something sensible at the new hypervisor's > - * initialisation entry point. > - */ > - > -ENTRY(__hyp_set_vectors) > - mov x1, x0 > - mov x0, #HVC_SET_VECTORS > - hvc #0 > - ret > -ENDPROC(__hyp_set_vectors) > - > -ENTRY(__hyp_reset_vectors) > - mov x0, #HVC_RESET_VECTORS > - hvc #0 > - ret > -ENDPROC(__hyp_reset_vectors) > diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S > index 3c79a1124af2..f6c9501ddfc9 100644 > --- a/arch/arm64/kvm/hyp.S > +++ b/arch/arm64/kvm/hyp.S > @@ -11,22 +11,11 @@ > #include <asm/cpufeature.h> > > /* > - * u64 __kvm_call_hyp(void *hypfn, ...); > + * u64 __kvm_call_hyp(unsigned long arg, ...); > * > * This is not really a variadic function in the classic C-way and care must > * be taken when calling this to ensure parameters are passed in registers > * only, since the stack will change between the caller and the callee. > - * > - * Call the function with the first argument containing a pointer to the > - * function you wish to call in Hyp mode, and subsequent arguments will be > - * passed as x0, x1, and x2 (a maximum of 3 arguments in addition to the > - * function pointer can be passed). The function being called must be mapped > - * in Hyp mode (see init_hyp_mode in arch/arm/kvm/arm.c). Return values are > - * passed in x0. > - * > - * A function pointer with a value less than 0xfff has a special meaning, > - * and is used to implement hyp stubs in the same way as in > - * arch/arm64/kernel/hyp_stub.S. I don't think any of this becomes obsolete with this patch (apart from the reference to 32bit), and only changes with patch #2. Or am I misunderstanding something? > */ > SYM_FUNC_START(__kvm_call_hyp) > hvc #0 > -- > 2.26.1 > > Thanks, M.
On Thursday 07 May 2020 at 15:01:07 (+0100), Marc Zyngier wrote: > > /* > > - * u64 __kvm_call_hyp(void *hypfn, ...); > > + * u64 __kvm_call_hyp(unsigned long arg, ...); > > * > > * This is not really a variadic function in the classic C-way and care must > > * be taken when calling this to ensure parameters are passed in registers > > * only, since the stack will change between the caller and the callee. > > - * > > - * Call the function with the first argument containing a pointer to the > > - * function you wish to call in Hyp mode, and subsequent arguments will be > > - * passed as x0, x1, and x2 (a maximum of 3 arguments in addition to the > > - * function pointer can be passed). The function being called must be mapped > > - * in Hyp mode (see init_hyp_mode in arch/arm/kvm/arm.c). Return values are > > - * passed in x0. > > - * > > - * A function pointer with a value less than 0xfff has a special meaning, > > - * and is used to implement hyp stubs in the same way as in > > - * arch/arm64/kernel/hyp_stub.S. > > I don't think any of this becomes obsolete with this patch (apart from > the reference to 32bit), and only changes with patch #2. Or am I > misunderstanding something? Nope, I think you're right. To be fair, this patch has changed quite a bit since the first version (which did change that comment a little later IIRC), but David has done all the hard work on top so I'll let him answer that one. And David, feel free to take the authorship for this patch -- I barely recognize it (for the better), so it's more than fair if you get the credit :) Thanks, Quentin
On Thu, May 07, 2020 at 03:36:33PM +0100, Quentin Perret wrote: > On Thursday 07 May 2020 at 15:01:07 (+0100), Marc Zyngier wrote: > > > /* > > > - * u64 __kvm_call_hyp(void *hypfn, ...); > > > + * u64 __kvm_call_hyp(unsigned long arg, ...); > > > * > > > * This is not really a variadic function in the classic C-way and care must > > > * be taken when calling this to ensure parameters are passed in registers > > > * only, since the stack will change between the caller and the callee. > > > - * > > > - * Call the function with the first argument containing a pointer to the > > > - * function you wish to call in Hyp mode, and subsequent arguments will be > > > - * passed as x0, x1, and x2 (a maximum of 3 arguments in addition to the > > > - * function pointer can be passed). The function being called must be mapped > > > - * in Hyp mode (see init_hyp_mode in arch/arm/kvm/arm.c). Return values are > > > - * passed in x0. > > > - * > > > - * A function pointer with a value less than 0xfff has a special meaning, > > > - * and is used to implement hyp stubs in the same way as in > > > - * arch/arm64/kernel/hyp_stub.S. > > > > I don't think any of this becomes obsolete with this patch (apart from > > the reference to 32bit), and only changes with patch #2. Or am I > > misunderstanding something? > > Nope, I think you're right. To be fair, this patch has changed quite > a bit since the first version (which did change that comment a little > later IIRC), but David has done all the hard work on top so I'll let > him answer that one. They have a different meaning in the three different contexts: 1) hyp-stub HVC: hcall ID + 3 args 2) hyp-init HVC: 4 args (no hcall ID) 3) host HVC: function pointer + 3 args The patch was meant to have all three use the same HVC routine, eventually converging on host HVCs also using 'hcall ID + 3 args' in patch 02, but it is a bit forced at this point. I can drop this patch from the series and we can clean this up later if it still makes sense. > > And David, feel free to take the authorship for this patch -- I barely > recognize it (for the better), so it's more than fair if you get the > credit :)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 32c8a675e5a4..e61143d6602d 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -25,6 +25,7 @@ #include <asm/kvm.h> #include <asm/kvm_asm.h> #include <asm/thread_info.h> +#include <asm/virt.h> #define __KVM_HAVE_ARCH_INTC_INITIALIZED @@ -446,7 +447,8 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva); void kvm_arm_halt_guest(struct kvm *kvm); void kvm_arm_resume_guest(struct kvm *kvm); -u64 __kvm_call_hyp(void *hypfn, ...); +#define kvm_call_hyp_nvhe(hypfn, ...) \ + __kvm_call_hyp((unsigned long)kvm_ksym_ref(hypfn), ##__VA_ARGS__) /* * The couple of isb() below are there to guarantee the same behaviour @@ -459,7 +461,7 @@ u64 __kvm_call_hyp(void *hypfn, ...); f(__VA_ARGS__); \ isb(); \ } else { \ - __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__); \ + kvm_call_hyp_nvhe(f, ##__VA_ARGS__); \ } \ } while(0) @@ -471,8 +473,7 @@ u64 __kvm_call_hyp(void *hypfn, ...); ret = f(__VA_ARGS__); \ isb(); \ } else { \ - ret = __kvm_call_hyp(kvm_ksym_ref(f), \ - ##__VA_ARGS__); \ + ret = kvm_call_hyp_nvhe(f, ##__VA_ARGS__); \ } \ \ ret; \ @@ -551,7 +552,8 @@ static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr, * cpus_have_const_cap() wrapper. */ BUG_ON(!system_capabilities_finalized()); - __kvm_call_hyp((void *)pgd_ptr, hyp_stack_ptr, vector_ptr, tpidr_el2); + __kvm_call_hyp((unsigned long)pgd_ptr, hyp_stack_ptr, vector_ptr, + tpidr_el2); /* * Disabling SSBD on a non-VHE system requires us to enable SSBS diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h index 61fd26752adc..fdc11f819b06 100644 --- a/arch/arm64/include/asm/virt.h +++ b/arch/arm64/include/asm/virt.h @@ -62,8 +62,37 @@ */ extern u32 __boot_cpu_mode[2]; -void __hyp_set_vectors(phys_addr_t phys_vector_base); -void __hyp_reset_vectors(void); +/* Make HVC call into the hypervisor. */ +extern u64 __kvm_call_hyp(unsigned long arg, ...); + +/* + * __hyp_set_vectors: Call this after boot to set the initial hypervisor + * vectors as part of hypervisor installation. On an SMP system, this should + * be called on each CPU. + * + * @phys_vector_base must be the physical address of the new vector table, and + * must be 2KB aligned. + * + * Before calling this, you must check that the stub hypervisor is installed + * everywhere, by waiting for any secondary CPUs to be brought up and then + * checking that is_hyp_mode_available() is true. + * + * If not, there is a pre-existing hypervisor, some CPUs failed to boot, or + * something else went wrong... in such cases, trying to install a new + * hypervisor is unlikely to work as desired. + * + * When you call into your shiny new hypervisor, sp_el2 will contain junk, + * so you will need to set that to something sensible at the new hypervisor's + * initialisation entry point. + */ +static inline void __hyp_set_vectors(phys_addr_t phys_vector_base) +{ + __kvm_call_hyp(HVC_SET_VECTORS, phys_vector_base); +} +static inline void __hyp_reset_vectors(void) +{ + __kvm_call_hyp(HVC_RESET_VECTORS); +} /* Reports the availability of HYP mode */ static inline bool is_hyp_mode_available(void) diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S index e473ead806ed..78d4ec5c4290 100644 --- a/arch/arm64/kernel/hyp-stub.S +++ b/arch/arm64/kernel/hyp-stub.S @@ -84,37 +84,3 @@ ENDPROC(\label) invalid_vector el1_irq_invalid invalid_vector el1_fiq_invalid invalid_vector el1_error_invalid - -/* - * __hyp_set_vectors: Call this after boot to set the initial hypervisor - * vectors as part of hypervisor installation. On an SMP system, this should - * be called on each CPU. - * - * x0 must be the physical address of the new vector table, and must be - * 2KB aligned. - * - * Before calling this, you must check that the stub hypervisor is installed - * everywhere, by waiting for any secondary CPUs to be brought up and then - * checking that is_hyp_mode_available() is true. - * - * If not, there is a pre-existing hypervisor, some CPUs failed to boot, or - * something else went wrong... in such cases, trying to install a new - * hypervisor is unlikely to work as desired. - * - * When you call into your shiny new hypervisor, sp_el2 will contain junk, - * so you will need to set that to something sensible at the new hypervisor's - * initialisation entry point. - */ - -ENTRY(__hyp_set_vectors) - mov x1, x0 - mov x0, #HVC_SET_VECTORS - hvc #0 - ret -ENDPROC(__hyp_set_vectors) - -ENTRY(__hyp_reset_vectors) - mov x0, #HVC_RESET_VECTORS - hvc #0 - ret -ENDPROC(__hyp_reset_vectors) diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S index 3c79a1124af2..f6c9501ddfc9 100644 --- a/arch/arm64/kvm/hyp.S +++ b/arch/arm64/kvm/hyp.S @@ -11,22 +11,11 @@ #include <asm/cpufeature.h> /* - * u64 __kvm_call_hyp(void *hypfn, ...); + * u64 __kvm_call_hyp(unsigned long arg, ...); * * This is not really a variadic function in the classic C-way and care must * be taken when calling this to ensure parameters are passed in registers * only, since the stack will change between the caller and the callee. - * - * Call the function with the first argument containing a pointer to the - * function you wish to call in Hyp mode, and subsequent arguments will be - * passed as x0, x1, and x2 (a maximum of 3 arguments in addition to the - * function pointer can be passed). The function being called must be mapped - * in Hyp mode (see init_hyp_mode in arch/arm/kvm/arm.c). Return values are - * passed in x0. - * - * A function pointer with a value less than 0xfff has a special meaning, - * and is used to implement hyp stubs in the same way as in - * arch/arm64/kernel/hyp_stub.S. */ SYM_FUNC_START(__kvm_call_hyp) hvc #0