[RFC v1 2/2] KVM: PPC: Book3S HV: abstract secure VM related calls.

[Ram Pai]

On Mon, Oct 12, 2020 at 08:58:36PM +0530, Bharata B Rao wrote:
> 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 at us.ibm.com>
> > ---
> >  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 at us.ibm.com>
> > + */
> > +
> > +#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 :-)

Hmm.. its still a extension to HMM to help manage device memory.
right?  What am i missing?


> 
> > +	/* 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?

ok.


> 
> > +
> > +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.

ok. anyway the 'kvmppc' of the field will go away as per your comment
above. So the confusion will also go away :)

> 
> > +		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?

It gets checked only the first time, when the backend is introduced.
If we dont check it during initialization, then we will have to check
everytime the method is called. So it is optimized in that sense.

Do you see a better way to optimize it?

Thanks for your comments,
RP