Date: Tue, 12 Sep 2017 22:29:40 +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 10/26] KVM: Introduce
 KVM_MEMORY_ENCRYPT_REGISTER/UNREGISTER_RAM ioctl
Message-ID: <20170912202940.qmq5oq2cx2cmhwqg@pd.tnic>
References: <20170724200303.12197-1-brijesh.singh@amd.com>
 <20170724200303.12197-11-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-11-brijesh.singh@amd.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3876
Lines: 111

On Mon, Jul 24, 2017 at 03:02:47PM -0500, Brijesh Singh wrote:
> If hardware support memory encryption then KVM_MEMORY_REGISTER_RAM and

	      supports

> KVM_MEMORY_UNREGISTER_RAM ioctl's can be used by userspace to register/
> unregister the guest memory regions which may contains the encrypted

"... which may contain encrypted data ... "

> data (e.g guest RAM, PCI BAR, SMRAM etc).
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  4 ++++
>  arch/x86/kvm/x86.c              | 36 ++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/kvm.h        |  9 +++++++++
>  3 files changed, 49 insertions(+)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 99a0e11..4295f82 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1059,6 +1059,10 @@ struct kvm_x86_ops {
>  	void (*setup_mce)(struct kvm_vcpu *vcpu);
>  
>  	int (*memory_encryption_op)(struct kvm *kvm, void __user *argp);
> +	int (*memory_encryption_register_ram)(struct kvm *kvm,
> +					struct kvm_memory_encrypt_ram *ram);
> +	int (*memory_encryption_unregister_ram)(struct kvm *kvm,
> +					struct kvm_memory_encrypt_ram *ram);
>  };

You can shorten those prefixes to "mem_enc" or so and struct
kvm_memory_encrypt_ram to struct enc_region - which is exactly what it
is - an encrypted memory region descriptor - and then fit each function
on a single line.

>  
>  struct kvm_arch_async_pf {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c9d3ff5..8febdb5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3982,6 +3982,24 @@ static int kvm_vm_ioctl_memory_encryption_op(struct kvm *kvm, void __user *argp)
>  	return -ENOTTY;
>  }
>  
> +static int kvm_vm_ioctl_mem_encrypt_register_ram(struct kvm *kvm,
> +					struct kvm_memory_encrypt_ram *ram)
> +{
> +	if (kvm_x86_ops->memory_encryption_register_ram)
> +		return kvm_x86_ops->memory_encryption_register_ram(kvm, ram);
> +
> +	return -ENOTTY;
> +}
> +
> +static int kvm_vm_ioctl_mem_encrypt_unregister_ram(struct kvm *kvm,
> +					struct kvm_memory_encrypt_ram *ram)
> +{
> +	if (kvm_x86_ops->memory_encryption_unregister_ram)
> +		return kvm_x86_ops->memory_encryption_unregister_ram(kvm, ram);
> +
> +	return -ENOTTY;
> +}
> +
>  long kvm_arch_vm_ioctl(struct file *filp,
>  		       unsigned int ioctl, unsigned long arg)
>  {
> @@ -4246,6 +4264,24 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  		r = kvm_vm_ioctl_memory_encryption_op(kvm, argp);
>  		break;
>  	}
> +	case KVM_MEMORY_ENCRYPT_REGISTER_RAM: {
> +		struct kvm_memory_encrypt_ram ram;
> +
> +		r = -EFAULT;
> +		if (copy_from_user(&ram, argp, sizeof(ram)))
> +			goto out;
> +		r = kvm_vm_ioctl_mem_encrypt_register_ram(kvm, &ram);
> +		break;
> +	}
> +	case KVM_MEMORY_ENCRYPT_UNREGISTER_RAM: {
> +		struct kvm_memory_encrypt_ram ram;
> +
> +		r = -EFAULT;
> +		if (copy_from_user(&ram, argp, sizeof(ram)))

As earlier, those ioctl functions need to check the user-supplied values.

> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index ab3b711..6074065 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1357,6 +1357,15 @@ struct kvm_s390_ucas_mapping {
>  #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
>  /* Memory Encryption Commands */
>  #define KVM_MEMORY_ENCRYPT_OP	  _IOWR(KVMIO, 0xba, unsigned long)
> +#define KVM_MEMORY_ENCRYPT_REGISTER_RAM	   _IOR(KVMIO, 0xbb, \
> +						struct kvm_memory_encrypt_ram)
> +#define KVM_MEMORY_ENCRYPT_UNREGISTER_RAM  _IOR(KVMIO, 0xbc, \
> +						struct kvm_memory_encrypt_ram)

As with KVM_MEMORY_ENCRYPT_OP, those two need to be in the KVM API document.

-- 
Regards/Gruss,
    Boris.

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