KVM ARM Archive on lore.kernel.org
 help / color / Atom feed
From: Marc Zyngier <maz@kernel.org>
To: David Brazdil <dbrazdil@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Will Deacon <will@kernel.org>,
	kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH 01/15] arm64: kvm: Unify users of HVC instruction
Date: Thu, 07 May 2020 15:01:07 +0100
Message-ID: <878si3j13w.wl-maz@kernel.org> (raw)
In-Reply-To: <20200430144831.59194-2-dbrazdil@google.com>

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.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

  reply index

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-30 14:48 [PATCH 00/15] Split off nVHE hyp code David Brazdil
2020-04-30 14:48 ` [PATCH 01/15] arm64: kvm: Unify users of HVC instruction David Brazdil
2020-05-07 14:01   ` Marc Zyngier [this message]
2020-05-07 14:36     ` Quentin Perret
2020-05-13 10:30       ` David Brazdil
2020-04-30 14:48 ` [PATCH 02/15] arm64: kvm: Formalize host-hyp hypcall ABI David Brazdil
2020-05-07 13:10   ` Marc Zyngier
2020-05-07 13:33     ` Quentin Perret
2020-05-10 10:16       ` Marc Zyngier
2020-05-13 10:48         ` David Brazdil
2020-04-30 14:48 ` [PATCH 03/15] arm64: kvm: Fix symbol dependency in __hyp_call_panic_nvhe David Brazdil
2020-05-07 13:22   ` Marc Zyngier
2020-05-07 14:36     ` David Brazdil
2020-05-10 11:14       ` Marc Zyngier
2020-04-30 14:48 ` [PATCH 04/15] arm64: kvm: Add build rules for separate nVHE object files David Brazdil
2020-05-07 13:46   ` Marc Zyngier
2020-04-30 14:48 ` [PATCH 05/15] arm64: kvm: Build hyp-entry.S separately for VHE/nVHE David Brazdil
2020-05-07 15:07   ` Marc Zyngier
2020-05-07 15:15     ` Marc Zyngier
2020-04-30 14:48 ` [PATCH 06/15] arm64: kvm: Move __smccc_workaround_1_smc to .rodata David Brazdil
2020-05-11 10:04   ` Marc Zyngier
2020-05-11 10:29     ` Will Deacon
2020-04-30 14:48 ` [PATCH 07/15] arm64: kvm: Split hyp/tlb.c to VHE/nVHE David Brazdil
2020-04-30 14:48 ` [PATCH 08/15] arm64: kvm: Split hyp/switch.c " David Brazdil
2020-04-30 14:48 ` [PATCH 09/15] arm64: kvm: Split hyp/debug-sr.c " David Brazdil
2020-04-30 14:48 ` [PATCH 10/15] arm64: kvm: Split hyp/sysreg-sr.c " David Brazdil
2020-04-30 14:48 ` [PATCH 11/15] arm64: kvm: Split hyp/timer-sr.c " David Brazdil
2020-04-30 14:48 ` [PATCH 12/15] arm64: kvm: Compile remaining hyp/ files for both VHE/nVHE David Brazdil
2020-04-30 14:48 ` [PATCH 13/15] arm64: kvm: Add comments around __hyp_text_ symbol aliases David Brazdil
2020-04-30 14:48 ` [PATCH 14/15] arm64: kvm: Remove __hyp_text macro, use build rules instead David Brazdil
2020-04-30 14:48 ` [PATCH 15/15] arm64: kvm: Lift instrumentation restrictions on VHE David Brazdil
2020-04-30 17:53 ` [PATCH 00/15] Split off nVHE hyp code Marc Zyngier
2020-04-30 18:57   ` David Brazdil

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=878si3j13w.wl-maz@kernel.org \
    --to=maz@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=dbrazdil@google.com \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

KVM ARM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvmarm/0 kvmarm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvmarm kvmarm/ https://lore.kernel.org/kvmarm \
		kvmarm@lists.cs.columbia.edu
	public-inbox-index kvmarm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/edu.columbia.cs.lists.kvmarm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git