Date: Wed, 13 Sep 2017 17:06:36 +0200
From: Borislav Petkov <bp@suse.de>
To: Brijesh Singh <brijesh.singh@amd.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org, kvm@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>, Joerg Roedel <joro@8bytes.org>,
        "Michael S . Tsirkin" <mst@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        =?utf-8?B?XCJSYWRpbSBLcsSNbcOhxZlcIg==?= <rkrcmar@redhat.com>,
        Tom Lendacky <thomas.lendacky@amd.com>
Subject: Re: [RFC Part2 PATCH v3 13/26] KVM: SVM: Add KVM_SEV_INIT command
Message-ID: <20170913150636.fcjhbg7wdf2whmy2@pd.tnic>
References: <20170724200303.12197-1-brijesh.singh@amd.com>
 <20170724200303.12197-14-brijesh.singh@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20170724200303.12197-14-brijesh.singh@amd.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6895
Lines: 304

On Mon, Jul 24, 2017 at 03:02:50PM -0500, Brijesh Singh wrote:
> The command initializes the SEV firmware and allocate a new ASID for

					       allocates

> this guest from SEV ASID pool. The firmware must be initialized before

	     from the

> we issue guest launch command to create a new encryption context.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  arch/x86/kvm/svm.c | 188 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 187 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 2a5a03a..e99a572 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -37,6 +37,8 @@
>  #include <linux/amd-iommu.h>
>  #include <linux/hashtable.h>
>  #include <linux/frame.h>
> +#include <linux/psp-sev.h>
> +#include <linux/file.h>
>  
>  #include <asm/apic.h>
>  #include <asm/perf_event.h>
> @@ -321,6 +323,14 @@ enum {
>  
>  /* Secure Encrypted Virtualization */
>  static unsigned int max_sev_asid;
> +static unsigned long *sev_asid_bitmap;
> +static int sev_asid_new(void);

This forward declaration is superfluous.

> +static void sev_asid_free(int asid);

You can move the sev_asid* code up and thus get rid of this forward
declaration too.

And also, svm.c is really huge. We probably should think about splitting
it in logical pieces... But that's for another day.

> +
> +static bool svm_sev_enabled(void)
> +{
> +	return !!max_sev_asid;

You don't need the "!!".

> +}
>  
>  static inline struct kvm_sev_info *to_sev_info(struct kvm *kvm)
>  {
> @@ -1093,6 +1103,12 @@ static __init void sev_hardware_setup(void)
>  	if (!nguests)
>  		return;
>  
> +	/* Initialize SEV ASID bitmap */
> +	sev_asid_bitmap = kcalloc(BITS_TO_LONGS(nguests),
> +				  sizeof(unsigned long), GFP_KERNEL);
> +	if (IS_ERR(sev_asid_bitmap))
> +		return;
> +
>  	max_sev_asid = nguests;
>  }
>  
> @@ -1184,10 +1200,18 @@ static __init int svm_hardware_setup(void)
>  	return r;
>  }
>  
> +static __exit void sev_hardware_unsetup(void)
> +{
> +	kfree(sev_asid_bitmap);
> +}
> +
>  static __exit void svm_hardware_unsetup(void)

"unsetup" - oh my. :)

>  	int cpu;
>  
> +	if (svm_sev_enabled())
> +		sev_hardware_unsetup();
> +
>  	for_each_possible_cpu(cpu)
>  		svm_cpu_uninit(cpu);
>  
> @@ -1373,6 +1397,9 @@ static void init_vmcb(struct vcpu_svm *svm)
>  		svm->vmcb->control.virt_ext |= VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK;
>  	}
>  
> +	if (sev_guest(svm->vcpu.kvm))
> +		svm->vmcb->control.nested_ctl |= SVM_NESTED_CTL_SEV_ENABLE;
> +
>  	mark_all_dirty(svm->vmcb);
>  
>  	enable_gif(svm);
> @@ -1483,6 +1510,51 @@ static inline int avic_free_vm_id(int id)
>  	return 0;
>  }
>  
> +static int sev_platform_get_state(int *state, int *error)
> +{
> +	int ret;
> +	struct sev_data_status *data;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);

It's a bit silly to do the allocation only for the duration of
sev_platform_status() - just allocate "data" on the stack.

> +	if (!data)
> +		return -ENOMEM;
> +
> +	ret = sev_platform_status(data, error);
> +	if (!ret)
> +		*state = data->state;
> +
> +	kfree(data);
> +	return ret;
> +}
> +
> +static void sev_firmware_uninit(void)

I guess sev_firmware_exit() or so.

> +{
> +	int rc, state, error;
> +
> +	rc = sev_platform_get_state(&state, &error);
> +	if (rc) {
> +		pr_err("SEV: failed to get firmware state (%#x)\n",
> +				error);

error fits on the same line.

> +		return;
> +	}
> +
> +	/* If we are in initialized state then uninitialize it */
> +	if (state == SEV_STATE_INIT)
> +		sev_platform_shutdown(&error);
> +
> +}
> +
> +static void sev_vm_destroy(struct kvm *kvm)
> +{
> +	int state, error;

arch/x86/kvm/svm.c: In function ‘sev_vm_destroy’:
arch/x86/kvm/svm.c:1548:13: warning: unused variable ‘error’ [-Wunused-variable]
  int state, error;
             ^~~~~
arch/x86/kvm/svm.c:1548:6: warning: unused variable ‘state’ [-Wunused-variable]
  int state, error;
      ^~~~~

> +
> +	if (!sev_guest(kvm))
> +		return;
> +
> +	sev_asid_free(sev_get_asid(kvm));
> +	sev_firmware_uninit();

I think you want to destroy the firmware context first and then free the
asid.

> +}
> +
>  static void avic_vm_destroy(struct kvm *kvm)
>  {
>  	unsigned long flags;
> @@ -1503,6 +1575,12 @@ static void avic_vm_destroy(struct kvm *kvm)
>  	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
>  }
>  
> +static void svm_vm_destroy(struct kvm *kvm)
> +{
> +	avic_vm_destroy(kvm);
> +	sev_vm_destroy(kvm);
> +}
> +
>  static int avic_vm_init(struct kvm *kvm)
>  {
>  	unsigned long flags;
> @@ -5428,6 +5506,112 @@ static void svm_setup_mce(struct kvm_vcpu *vcpu)
>  	vcpu->arch.mcg_cap &= 0x1ff;
>  }
>  
> +static int sev_asid_new(void)
> +{
> +	int pos;
> +
> +	if (!max_sev_asid)

	if (!svm_sev_enabled())

since you have an accessor.

> +		return -EINVAL;
> +
> +	pos = find_first_zero_bit(sev_asid_bitmap, max_sev_asid);
> +	if (pos >= max_sev_asid)
> +		return -EBUSY;
> +
> +	set_bit(pos, sev_asid_bitmap);
> +	return pos + 1;
> +}
> +
> +static void sev_asid_free(int asid)
> +{
> +	int pos;

	if (!svm_sev_enabled())
		return;
> +
> +	pos = asid - 1;
> +	clear_bit(pos, sev_asid_bitmap);
> +}
> +
> +static int sev_firmware_init(int *error)
> +{
> +	int ret, state;
> +
> +	ret = sev_platform_get_state(&state, error);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * If SEV firmware is in uninitialized state, lets initialize the
> +	 * firmware before issuing guest commands.
> +	 */
> +	if (state == SEV_STATE_UNINIT) {
> +		struct sev_data_init *data;

Same note as above - allocate data on the stack.

> +
> +		data = kzalloc(sizeof(*data), GFP_KERNEL);
> +		if (!data)
> +			return -ENOMEM;
> +
> +		ret = sev_platform_init(data, error);
> +		kfree(data);
> +	}
> +
> +	return ret;
> +}
> +
> +static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +	int asid, ret;
> +	struct fd f;
> +
> +	f = fdget(argp->sev_fd);
> +	if (!f.file)
> +		return -EBADF;
> +
> +	/* initialize the SEV firmware */
> +	ret = sev_firmware_init(&argp->error);
> +	if (ret)
> +		goto e_err;
> +
> +	/* allocate asid from SEV pool */
> +	ret = -ENOTTY;
> +	asid = sev_asid_new();
> +	if (asid < 0) {
> +		pr_err("SEV: failed to get free asid\n");
> +		sev_platform_shutdown(&argp->error);
> +		goto e_err;
> +	}
> +
> +	sev_set_active(kvm);

I think that should be the last thing you execute before returning.

> +	sev_set_asid(kvm, asid);
> +	sev_set_fd(kvm, argp->sev_fd);
> +	ret = 0;
> +e_err:
> +	fdput(f);
> +	return ret;
> +}
> +
> +static int svm_memory_encryption_op(struct kvm *kvm, void __user *argp)
> +{
> +	struct kvm_sev_cmd sev_cmd;
> +	int r = -ENOTTY;
> +
> +	if (copy_from_user(&sev_cmd, argp, sizeof(struct kvm_sev_cmd)))
> +		return -EFAULT;

Sanity-check that sev_cmd.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--