On Mon, Oct 12, 2020 at 12:27:43AM -0700, Ram Pai wrote:
> Abstract the secure VM related calls into generic calls.
>
> These generic calls will call the corresponding method of the
> backend that prvoides the implementation to support secure VM.
>
> Currently there is only the ultravisor based implementation.
> Modify that implementation to act as a backed to the generic calls.
>
> This plumbing will provide the flexibility to add more backends
> in the future.
>
> Signed-off-by: Ram Pai <linuxram@xxxxxxxxxx>
> ---
> arch/powerpc/include/asm/kvm_book3s_uvmem.h | 100 -----------
> arch/powerpc/include/asm/kvmppc_svm_backend.h | 250 ++++++++++++++++++++++++++
> arch/powerpc/kvm/book3s_64_mmu_radix.c | 6 +-
> arch/powerpc/kvm/book3s_hv.c | 28 +--
> arch/powerpc/kvm/book3s_hv_uvmem.c | 78 ++++++--
> 5 files changed, 327 insertions(+), 135 deletions(-)
> delete mode 100644 arch/powerpc/include/asm/kvm_book3s_uvmem.h
> create mode 100644 arch/powerpc/include/asm/kvmppc_svm_backend.h
>
> diff --git a/arch/powerpc/include/asm/kvmppc_svm_backend.h b/arch/powerpc/include/asm/kvmppc_svm_backend.h
> new file mode 100644
> index 0000000..be60d80
> --- /dev/null
> +++ b/arch/powerpc/include/asm/kvmppc_svm_backend.h
> @@ -0,0 +1,250 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + *
> + * Copyright IBM Corp. 2020
> + *
> + * Authors: Ram Pai <linuxram@xxxxxxxxxx>
> + */
> +
> +#ifndef __POWERPC_KVMPPC_SVM_BACKEND_H__
> +#define __POWERPC_KVMPPC_SVM_BACKEND_H__
> +
> +#include <linux/mutex.h>
> +#include <linux/timer.h>
> +#include <linux/types.h>
> +#include <linux/kvm_types.h>
> +#include <linux/kvm_host.h>
> +#include <linux/bug.h>
> +#ifdef CONFIG_PPC_BOOK3S
> +#include <asm/kvm_book3s.h>
> +#else
> +#include <asm/kvm_booke.h>
> +#endif
> +#ifdef CONFIG_KVM_BOOK3S_64_HANDLER
> +#include <asm/paca.h>
> +#include <asm/xive.h>
> +#include <asm/cpu_has_feature.h>
> +#endif
> +
> +struct kvmppc_hmm_backend {
Though we started with HMM initially, what we ended up with eventually
has nothing to do with HMM. Please don't introduce hmm again :-)
> + /* initialize */
> + int (*kvmppc_secmem_init)(void);
> +
> + /* cleanup */
> + void (*kvmppc_secmem_free)(void);
> +
> + /* is memory available */
> + bool (*kvmppc_secmem_available)(void);
> +
> + /* allocate a protected/secure page for the secure VM */
> + unsigned long (*kvmppc_svm_page_in)(struct kvm *kvm,
> + unsigned long gra,
> + unsigned long flags,
> + unsigned long page_shift);
> +
> + /* recover the protected/secure page from the secure VM */
> + unsigned long (*kvmppc_svm_page_out)(struct kvm *kvm,
> + unsigned long gra,
> + unsigned long flags,
> + unsigned long page_shift);
> +
> + /* initiate the transition of a VM to secure VM */
> + unsigned long (*kvmppc_svm_init_start)(struct kvm *kvm);
> +
> + /* finalize the transition of a secure VM */
> + unsigned long (*kvmppc_svm_init_done)(struct kvm *kvm);
> +
> + /* share the page on page fault */
> + int (*kvmppc_svm_page_share)(struct kvm *kvm, unsigned long gfn);
> +
> + /* abort the transition to a secure VM */
> + unsigned long (*kvmppc_svm_init_abort)(struct kvm *kvm);
> +
> + /* add a memory slot */
> + int (*kvmppc_svm_memslot_create)(struct kvm *kvm,
> + const struct kvm_memory_slot *new);
> +
> + /* free a memory slot */
> + void (*kvmppc_svm_memslot_delete)(struct kvm *kvm,
> + const struct kvm_memory_slot *old);
> +
> + /* drop pages allocated to the secure VM */
> + void (*kvmppc_svm_drop_pages)(const struct kvm_memory_slot *free,
> + struct kvm *kvm, bool skip_page_out);
> +};
Since the structure has kvmppc_ prefix, may be you can drop
the same from its members to make the fields smaller?
> +
> +extern const struct kvmppc_hmm_backend *kvmppc_svm_backend;
> +
> +static inline int kvmppc_svm_page_share(struct kvm *kvm, unsigned long gfn)
> +{
> + if (!kvmppc_svm_backend)
> + return -ENODEV;
> +
> + return kvmppc_svm_backend->kvmppc_svm_page_share(kvm,
> + gfn);
> +}
> +
> +static inline void kvmppc_svm_drop_pages(const struct kvm_memory_slot *memslot,
> + struct kvm *kvm, bool skip_page_out)
> +{
> + if (!kvmppc_svm_backend)
> + return;
> +
> + kvmppc_svm_backend->kvmppc_svm_drop_pages(memslot,
> + kvm, skip_page_out);
> +}
> +
> +static inline int kvmppc_svm_page_in(struct kvm *kvm,
> + unsigned long gpa,
> + unsigned long flags,
> + unsigned long page_shift)
> +{
> + if (!kvmppc_svm_backend)
> + return -ENODEV;
> +
> + return kvmppc_svm_backend->kvmppc_svm_page_in(kvm,
> + gpa, flags, page_shift);
> +}
> +
> +static inline int kvmppc_svm_page_out(struct kvm *kvm,
> + unsigned long gpa,
> + unsigned long flags,
> + unsigned long page_shift)
> +{
> + if (!kvmppc_svm_backend)
> + return -ENODEV;
> +
> + return kvmppc_svm_backend->kvmppc_svm_page_out(kvm,
> + gpa, flags, page_shift);
> +}
> +
> +static inline int kvmppc_svm_init_start(struct kvm *kvm)
> +{
> + if (!kvmppc_svm_backend)
> + return -ENODEV;
> +
> + return kvmppc_svm_backend->kvmppc_svm_init_start(kvm);
> +}
> +
> +static inline int kvmppc_svm_init_done(struct kvm *kvm)
> +{
> + if (!kvmppc_svm_backend)
> + return -ENODEV;
> +
> + return kvmppc_svm_backend->kvmppc_svm_init_done(kvm);
> +}
> +
> +static inline int kvmppc_svm_init_abort(struct kvm *kvm)
> +{
> + if (!kvmppc_svm_backend)
> + return -ENODEV;
> +
> + return kvmppc_svm_backend->kvmppc_svm_init_abort(kvm);
> +}
> +
> +static inline void kvmppc_svm_memslot_create(struct kvm *kvm,
> + const struct kvm_memory_slot *memslot)
> +{
> + if (!kvmppc_svm_backend)
> + return;
> +
> + kvmppc_svm_backend->kvmppc_svm_memslot_create(kvm,
> + memslot);
> +}
> +
> +static inline void kvmppc_svm_memslot_delete(struct kvm *kvm,
> + const struct kvm_memory_slot *memslot)
> +{
> + if (!kvmppc_svm_backend)
> + return;
> +
> + kvmppc_svm_backend->kvmppc_svm_memslot_delete(kvm,
> + memslot);
> +}
> +
> +static inline int kvmppc_secmem_init(void)
> +{
> +#ifdef CONFIG_PPC_UV
> + extern const struct kvmppc_hmm_backend kvmppc_uvmem_backend;
> +
> + kvmppc_svm_backend = NULL;
> + if (kvmhv_on_pseries()) {
> + /* @TODO add the protected memory backend */
> + return 0;
> + }
> +
> + kvmppc_svm_backend = &kvmppc_uvmem_backend;
> +
> + if (!kvmppc_svm_backend->kvmppc_secmem_init) {
You have a function named kvmppc_secmem_init() and the field
named the same, can be confusing.
> + pr_err("KVM-HV: kvmppc_svm_backend has no %s\n", __func__);
> + goto err;
> + }
> + if (!kvmppc_svm_backend->kvmppc_secmem_free) {
> + pr_err("KVM-HV: kvmppc_svm_backend has no kvmppc_secmem_free()\n");
> + goto err;
> + }
> + if (!kvmppc_svm_backend->kvmppc_secmem_available) {
> + pr_err("KVM-HV: kvmppc_svm_backend has no kvmppc_secmem_available()\n");
> + goto err;
> + }
> + if (!kvmppc_svm_backend->kvmppc_svm_page_in) {
> + pr_err("KVM-HV: kvmppc_svm_backend has no kvmppc_svm_page_in()\n");
> + goto err;
> + }
> + if (!kvmppc_svm_backend->kvmppc_svm_page_out) {
> + pr_err("KVM-HV: kvmppc_svm_backend has no kvmppc_svm_page_out()\n");
> + goto err;
> + }
> + if (!kvmppc_svm_backend->kvmppc_svm_init_start) {
> + pr_err("KVM-HV: kvmppc_svm_backend has no kvmppc_svm_init_start()\n");
> + goto err;
> + }
> + if (!kvmppc_svm_backend->kvmppc_svm_init_done) {
> + pr_err("KVM-HV: kvmppc_svm_backend has no kvmppc_svm_init_done()\n");
> + goto err;
> + }
> + if (!kvmppc_svm_backend->kvmppc_svm_page_share) {
> + pr_err("KVM-HV: kvmppc_svm_backend has no kvmppc_svm_page_share()\n");
> + goto err;
> + }
> + if (!kvmppc_svm_backend->kvmppc_svm_init_abort) {
> + pr_err("KVM-HV: kvmppc_svm_backend has no kvmppc_svm_init_abort()\n");
> + goto err;
> + }
> + if (!kvmppc_svm_backend->kvmppc_svm_memslot_create) {
> + pr_err("KVM-HV: kvmppc_svm_backend has no kvmppc_svm_memslot_create()\n");
> + goto err;
> + }
> + if (!kvmppc_svm_backend->kvmppc_svm_memslot_delete) {
> + pr_err("KVM-HV: kvmppc_svm_backend has no kvmppc_svm_memslot_delete()\n");
> + goto err;
> + }
> + if (!kvmppc_svm_backend->kvmppc_svm_drop_pages) {
> + pr_err("KVM-HV: kvmppc_svm_backend has no kvmppc_svm_drop_pages()\n");
> + goto err;
> + }
Do you really need to check each and every callback like above?
If so, may be the check can be optimized?
Regards,
Bharata.
