From: Marc Zyngier <maz@kernel.org>
To: Andrew Scull <ascull@google.com>
Cc: kernel-team@android.com, suzuki.poulose@arm.com,
catalin.marinas@arm.com, james.morse@arm.com,
linux-arm-kernel@lists.infradead.org,
Sudeep Holla <sudeep.holla@arm.com>,
David Brazdil <dbrazdil@google.com>,
will@kernel.org, kvmarm@lists.cs.columbia.edu,
julien.thierry.kdev@gmail.com
Subject: Re: [PATCH v3 16/18] KVM: arm64: nVHE: Migrate hyp interface to SMCCC
Date: Mon, 07 Sep 2020 14:47:59 +0100 [thread overview]
Message-ID: <87pn6xlmuo.wl-maz@kernel.org> (raw)
In-Reply-To: <20200903135307.251331-17-ascull@google.com>
On Thu, 03 Sep 2020 14:53:05 +0100,
Andrew Scull <ascull@google.com> wrote:
>
> Rather than passing arbitrary function pointers to run at hyp, define
> and equivalent set of SMCCC functions.
>
> Since the SMCCC functions are strongly tied to the original function
> prototypes, it is not expected for the host to ever call an invalid ID
> but a warning is raised if this does ever occur.
>
> As __kvm_vcpu_run is used for every switch between the host and a guest,
> it is explicitly singled out to be identified before the other function
> IDs to improve the performance of the hot path.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> Signed-off-by: David Brazdil <dbrazdil@google.com>
Who is the author? If it is a co-development, use the ad-hoc tag.
> ---
> arch/arm64/include/asm/kvm_asm.h | 24 ++++++
> arch/arm64/include/asm/kvm_host.h | 25 ++++---
> arch/arm64/kvm/arm.c | 2 +-
> arch/arm64/kvm/hyp.S | 24 ++----
> arch/arm64/kvm/hyp/nvhe/hyp-main.c | 113 +++++++++++++++++++++++++----
> 5 files changed, 145 insertions(+), 43 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 4bbde3d3989c..4a73f1349151 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -38,6 +38,30 @@
>
> #define __SMCCC_WORKAROUND_1_SMC_SZ 36
>
> +#define KVM_HOST_SMCCC_ID(id) \
> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> + ARM_SMCCC_SMC_64, \
> + ARM_SMCCC_OWNER_STANDARD_HYP, \
> + (id))
> +
> +#define KVM_HOST_SMCCC_FUNC(name) KVM_HOST_SMCCC_ID(__KVM_HOST_SMCCC_FUNC_##name)
> +
> +#define __KVM_HOST_SMCCC_FUNC___kvm_hyp_init 0
> +#define __KVM_HOST_SMCCC_FUNC___kvm_flush_vm_context 1
> +#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid_ipa 2
> +#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_vmid 3
> +#define __KVM_HOST_SMCCC_FUNC___kvm_tlb_flush_local_vmid 4
> +#define __KVM_HOST_SMCCC_FUNC___kvm_timer_set_cntvoff 5
> +#define __KVM_HOST_SMCCC_FUNC___kvm_vcpu_run 6
> +#define __KVM_HOST_SMCCC_FUNC___kvm_enable_ssbs 7
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_get_ich_vtr_el2 8
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_read_vmcr 9
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_write_vmcr 10
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_init_lrs 11
> +#define __KVM_HOST_SMCCC_FUNC___kvm_get_mdcr_el2 12
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_save_aprs 13
> +#define __KVM_HOST_SMCCC_FUNC___vgic_v3_restore_aprs 14
> +
> #ifndef __ASSEMBLY__
>
> #include <linux/mm.h>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 16adbefde1cc..82c941cf8890 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -11,6 +11,7 @@
> #ifndef __ARM64_KVM_HOST_H__
> #define __ARM64_KVM_HOST_H__
>
> +#include <linux/arm-smccc.h>
> #include <linux/bitmap.h>
> #include <linux/types.h>
> #include <linux/jump_label.h>
> @@ -479,18 +480,20 @@ 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, ...);
> +u64 __kvm_call_hyp_init(phys_addr_t pgd_ptr,
> + unsigned long hyp_stack_ptr,
> + unsigned long vector_ptr,
> + unsigned long tpidr_el2);
>
> -#define kvm_call_hyp_nvhe(f, ...) \
> - do { \
> - DECLARE_KVM_NVHE_SYM(f); \
> - __kvm_call_hyp(kvm_ksym_ref_nvhe(f), ##__VA_ARGS__); \
> - } while(0)
> -
> -#define kvm_call_hyp_nvhe_ret(f, ...) \
> +#define kvm_call_hyp_nvhe(f, ...) \
> ({ \
> - DECLARE_KVM_NVHE_SYM(f); \
> - __kvm_call_hyp(kvm_ksym_ref_nvhe(f), ##__VA_ARGS__); \
> + struct arm_smccc_res res; \
> + \
> + arm_smccc_1_1_hvc(KVM_HOST_SMCCC_FUNC(f), \
> + ##__VA_ARGS__, &res); \
> + WARN_ON(res.a0 != SMCCC_RET_SUCCESS); \
> + \
> + res.a1; \
> })
>
> /*
> @@ -516,7 +519,7 @@ u64 __kvm_call_hyp(void *hypfn, ...);
> ret = f(__VA_ARGS__); \
> isb(); \
> } else { \
> - ret = kvm_call_hyp_nvhe_ret(f, ##__VA_ARGS__); \
> + ret = kvm_call_hyp_nvhe(f, ##__VA_ARGS__); \
nit: Just inline the whole macro here.
> } \
> \
> ret; \
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 6b7180072c8d..49aa08bd26de 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1288,7 +1288,7 @@ static void cpu_init_hyp_mode(void)
> * 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_init(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/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 3c79a1124af2..12aa426f7559 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -11,24 +11,12 @@
> #include <asm/cpufeature.h>
>
> /*
> - * u64 __kvm_call_hyp(void *hypfn, ...);
> - *
> - * 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.
> + * u64 __kvm_call_hyp_init(phys_addr_t pgd_ptr,
> + * unsigned long hyp_stack_ptr,
> + * unsigned long vector_ptr,
> + * unsigned long tpidr_el2);
> */
> -SYM_FUNC_START(__kvm_call_hyp)
> +SYM_FUNC_START(__kvm_call_hyp_init)
> hvc #0
> ret
> -SYM_FUNC_END(__kvm_call_hyp)
> +SYM_FUNC_END(__kvm_call_hyp_init)
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index c8938e09f585..13093df70c87 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> @@ -12,24 +12,111 @@
> #include <asm/kvm_hyp.h>
> #include <asm/kvm_mmu.h>
>
> -typedef unsigned long (*hypcall_fn_t)
> - (unsigned long, unsigned long, unsigned long);
> +#include <kvm/arm_hypercalls.h>
> +
> +static void handle_host_hcall(unsigned long func_id,
> + struct kvm_cpu_context *host_ctxt)
> +{
> + unsigned long ret = 0;
> +
> + /*
> + * __kvm_vcpu_run is a hot path of the context switch so identify it
> + * quickly before searching through the other functions IDs.
> + */
> + if (func_id == KVM_HOST_SMCCC_FUNC(__kvm_vcpu_run)) {
> + struct kvm_vcpu *vcpu =
> + (struct kvm_vcpu *)host_ctxt->regs.regs[1];
> +
> + ret = __kvm_vcpu_run(vcpu);
> + goto out;
> + }
This is terribly ugly. How does it behave if you keep it in the
switch(), and make it function 0, for example?
> +
> + switch (func_id) {
> + case KVM_HOST_SMCCC_FUNC(__kvm_flush_vm_context):
> + __kvm_flush_vm_context();
> + break;
> + case KVM_HOST_SMCCC_FUNC(__kvm_tlb_flush_vmid_ipa): {
> + struct kvm_s2_mmu *mmu =
> + (struct kvm_s2_mmu *)host_ctxt->regs.regs[1];
> + phys_addr_t ipa = host_ctxt->regs.regs[2];
> + int level = host_ctxt->regs.regs[3];
> +
> + __kvm_tlb_flush_vmid_ipa(mmu, ipa, level);
> + break;
> + }
nit: The formatting hurts. If you have to use braces, don't introduce
extra indentation. And given how many times you extract a s2_mmu from
the first second argument, consider using a helper.
> + case KVM_HOST_SMCCC_FUNC(__kvm_tlb_flush_vmid): {
> + struct kvm_s2_mmu *mmu =
> + (struct kvm_s2_mmu *)host_ctxt->regs.regs[1];
> +
> + __kvm_tlb_flush_vmid(mmu);
> + break;
> + }
> + case KVM_HOST_SMCCC_FUNC(__kvm_tlb_flush_local_vmid): {
> + struct kvm_s2_mmu *mmu =
> + (struct kvm_s2_mmu *)host_ctxt->regs.regs[1];
> +
> + __kvm_tlb_flush_local_vmid(mmu);
> + break;
> + }
> + case KVM_HOST_SMCCC_FUNC(__kvm_timer_set_cntvoff): {
> + u64 cntvoff = host_ctxt->regs.regs[1];
> +
> + __kvm_timer_set_cntvoff(cntvoff);
> + break;
> + }
> + case KVM_HOST_SMCCC_FUNC(__kvm_enable_ssbs):
> + __kvm_enable_ssbs();
> + break;
> + case KVM_HOST_SMCCC_FUNC(__vgic_v3_get_ich_vtr_el2):
> + ret = __vgic_v3_get_ich_vtr_el2();
> + break;
> + case KVM_HOST_SMCCC_FUNC(__vgic_v3_read_vmcr):
> + ret = __vgic_v3_read_vmcr();
> + break;
> + case KVM_HOST_SMCCC_FUNC(__vgic_v3_write_vmcr): {
> + u32 vmcr = host_ctxt->regs.regs[1];
> +
> + __vgic_v3_write_vmcr(vmcr);
> + break;
> + }
> + case KVM_HOST_SMCCC_FUNC(__vgic_v3_init_lrs):
> + __vgic_v3_init_lrs();
> + break;
> + case KVM_HOST_SMCCC_FUNC(__kvm_get_mdcr_el2):
> + ret = __kvm_get_mdcr_el2();
> + break;
> + case KVM_HOST_SMCCC_FUNC(__vgic_v3_save_aprs): {
> + struct vgic_v3_cpu_if *cpu_if =
> + (struct vgic_v3_cpu_if *)host_ctxt->regs.regs[1];
> +
> + __vgic_v3_save_aprs(cpu_if);
> + break;
> + }
> + case KVM_HOST_SMCCC_FUNC(__vgic_v3_restore_aprs): {
> + struct vgic_v3_cpu_if *cpu_if =
> + (struct vgic_v3_cpu_if *)host_ctxt->regs.regs[1];
> +
> + __vgic_v3_restore_aprs(cpu_if);
> + break;
> + }
> + default:
> + /* Invalid host HVC. */
> + host_ctxt->regs.regs[0] = SMCCC_RET_NOT_SUPPORTED;
> + return;
> + }
> +
> +out:
> + host_ctxt->regs.regs[0] = SMCCC_RET_SUCCESS;
> + host_ctxt->regs.regs[1] = ret;
> +}
>
> void handle_trap(struct kvm_cpu_context *host_ctxt) {
> u64 esr = read_sysreg_el2(SYS_ESR);
> - hypcall_fn_t func;
> - unsigned long ret;
> + unsigned long func_id;
>
> if (ESR_ELx_EC(esr) != ESR_ELx_EC_HVC64)
> hyp_panic();
>
> - /*
> - * __kvm_call_hyp takes a pointer in the host address space and
> - * up to three arguments.
> - */
> - func = (hypcall_fn_t)kern_hyp_va(host_ctxt->regs.regs[0]);
> - ret = func(host_ctxt->regs.regs[1],
> - host_ctxt->regs.regs[2],
> - host_ctxt->regs.regs[3]);
> - host_ctxt->regs.regs[0] = ret;
> + func_id = host_ctxt->regs.regs[0];
> + handle_host_hcall(func_id, host_ctxt);
> }
> --
> 2.28.0.402.g5ffc5be6b7-goog
>
>
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-09-07 13:52 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-03 13:52 [PATCH v3 00/18] Introduce separate nVHE hyp context Andrew Scull
2020-09-03 13:52 ` [PATCH v3 01/18] KVM: arm64: Remove __activate_vm wrapper Andrew Scull
2020-09-03 13:52 ` [PATCH v3 02/18] KVM: arm64: Remove hyp_panic arguments Andrew Scull
2020-09-07 10:21 ` Marc Zyngier
2020-09-03 13:52 ` [PATCH v3 03/18] KVM: arm64: Remove kvm_host_data_t typedef Andrew Scull
2020-09-03 13:52 ` [PATCH v3 04/18] KVM: arm64: Restrict symbol aliasing to outside nVHE Andrew Scull
2020-09-07 10:38 ` Marc Zyngier
2020-09-08 10:13 ` Andrew Scull
2020-09-03 13:52 ` [PATCH v3 05/18] KVM: arm64: Save chosen hyp vector to a percpu variable Andrew Scull
2020-09-03 13:52 ` [PATCH v3 06/18] KVM: arm64: nVHE: Use separate vector for the host Andrew Scull
2020-09-07 11:38 ` Marc Zyngier
2020-09-08 10:29 ` Andrew Scull
2020-09-03 13:52 ` [PATCH v3 07/18] KVM: arm64: nVHE: Don't consume host SErrors with ESB Andrew Scull
2020-09-07 11:46 ` Marc Zyngier
2020-09-03 13:52 ` [PATCH v3 08/18] KVM: arm64: Introduce hyp context Andrew Scull
2020-09-07 13:29 ` Marc Zyngier
2020-09-08 10:52 ` Andrew Scull
2020-09-03 13:52 ` [PATCH v3 09/18] KVM: arm64: Update context references from host to hyp Andrew Scull
2020-09-03 13:52 ` [PATCH v3 10/18] KVM: arm64: Restore hyp when panicking in guest context Andrew Scull
2020-09-03 13:53 ` [PATCH v3 11/18] KVM: arm64: Share context save and restore macros Andrew Scull
2020-09-03 13:53 ` [PATCH v3 12/18] KVM: arm64: nVHE: Switch to hyp context for EL2 Andrew Scull
2020-09-07 13:02 ` Marc Zyngier
2020-09-08 10:42 ` Andrew Scull
2020-09-03 13:53 ` [PATCH v3 13/18] KVM: arm64: nVHE: Handle hyp panics Andrew Scull
2020-09-07 13:24 ` Marc Zyngier
2020-09-03 13:53 ` [PATCH v3 14/18] smccc: Cast arguments to unsigned long Andrew Scull
2020-09-07 13:33 ` Marc Zyngier
2020-09-08 10:58 ` Andrew Scull
2020-09-03 13:53 ` [PATCH v3 15/18] KVM: arm64: nVHE: Pass pointers consistently to hyp-init Andrew Scull
2020-09-03 13:53 ` [PATCH v3 16/18] KVM: arm64: nVHE: Migrate hyp interface to SMCCC Andrew Scull
2020-09-07 13:47 ` Marc Zyngier [this message]
2020-09-07 14:20 ` Marc Zyngier
2020-09-08 11:02 ` Andrew Scull
2020-09-09 8:30 ` Andrew Scull
2020-09-03 13:53 ` [PATCH v3 17/18] KVM: arm64: nVHE: Migrate hyp-init " Andrew Scull
2020-09-03 13:53 ` [PATCH v3 18/18] KVM: arm64: nVHE: Fix pointers during SMCCC convertion Andrew Scull
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=87pn6xlmuo.wl-maz@kernel.org \
--to=maz@kernel.org \
--cc=ascull@google.com \
--cc=catalin.marinas@arm.com \
--cc=dbrazdil@google.com \
--cc=james.morse@arm.com \
--cc=julien.thierry.kdev@gmail.com \
--cc=kernel-team@android.com \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=sudeep.holla@arm.com \
--cc=suzuki.poulose@arm.com \
--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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).