[v6,11/14] KVM: x86: Introduce KVM_SET_PAGE_ENC_BITMAP ioctl

Commit Message

Kalra, Ashish March 30, 2020, 6:22 a.m. UTC

From: Brijesh Singh <[e-mail]>

The ioctl can be used to set page encryption bitmap for an
incoming guest.

Cc: Thomas Gleixner <[e-mail]>
Cc: Ingo Molnar <[e-mail]>
Cc: "H. Peter Anvin" <[e-mail]>
Cc: Paolo Bonzini <[e-mail]>
Cc: "Radim Krčmář" <[e-mail]>
Cc: Joerg Roedel <[e-mail]>
Cc: Borislav Petkov <[e-mail]>
Cc: Tom Lendacky <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Brijesh Singh <[e-mail]>
Signed-off-by: Ashish Kalra <[e-mail]>
---
 Documentation/virt/kvm/api.rst  | 22 +++++++++++++++++
 arch/x86/include/asm/kvm_host.h |  2 ++
 arch/x86/kvm/svm.c              | 42 +++++++++++++++++++++++++++++++++
 arch/x86/kvm/x86.c              | 12 ++++++++++
 include/uapi/linux/kvm.h        |  1 +
 5 files changed, 79 insertions(+)

Comments

Krish Sadhukhan April 3, 2020, 9:10 p.m. UTC | #1

On 3/29/20 11:22 PM, Ashish Kalra wrote:
> From: Brijesh Singh <[e-mail]>
>
> The ioctl can be used to set page encryption bitmap for an
> incoming guest.
>
> Cc: Thomas Gleixner <[e-mail]>
> Cc: Ingo Molnar <[e-mail]>
> Cc: "H. Peter Anvin" <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: "Radim Krčmář" <[e-mail]>
> Cc: Joerg Roedel <[e-mail]>
> Cc: Borislav Petkov <[e-mail]>
> Cc: Tom Lendacky <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Brijesh Singh <[e-mail]>
> Signed-off-by: Ashish Kalra <[e-mail]>
> ---
>   Documentation/virt/kvm/api.rst  | 22 +++++++++++++++++
>   arch/x86/include/asm/kvm_host.h |  2 ++
>   arch/x86/kvm/svm.c              | 42 +++++++++++++++++++++++++++++++++
>   arch/x86/kvm/x86.c              | 12 ++++++++++
>   include/uapi/linux/kvm.h        |  1 +
>   5 files changed, 79 insertions(+)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 8ad800ebb54f..4d1004a154f6 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -4675,6 +4675,28 @@ or shared. The bitmap can be used during the guest migration, if the page
>   is private then userspace need to use SEV migration commands to transmit
>   the page.
>   
> +4.126 KVM_SET_PAGE_ENC_BITMAP (vm ioctl)
> +---------------------------------------
> +
> +:Capability: basic
> +:Architectures: x86
> +:Type: vm ioctl
> +:Parameters: struct kvm_page_enc_bitmap (in/out)
> +:Returns: 0 on success, -1 on error
> +
> +/* for KVM_SET_PAGE_ENC_BITMAP */
> +struct kvm_page_enc_bitmap {
> +	__u64 start_gfn;
> +	__u64 num_pages;
> +	union {
> +		void __user *enc_bitmap; /* one bit per page */
> +		__u64 padding2;
> +	};
> +};
> +
> +During the guest live migration the outgoing guest exports its page encryption
> +bitmap, the KVM_SET_PAGE_ENC_BITMAP can be used to build the page encryption
> +bitmap for an incoming guest.
>   
>   5. The kvm_run structure
>   ========================
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 27e43e3ec9d8..d30f770aaaea 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1271,6 +1271,8 @@ struct kvm_x86_ops {
>   				  unsigned long sz, unsigned long mode);
>   	int (*get_page_enc_bitmap)(struct kvm *kvm,
>   				struct kvm_page_enc_bitmap *bmap);
> +	int (*set_page_enc_bitmap)(struct kvm *kvm,
> +				struct kvm_page_enc_bitmap *bmap);
>   };
>   
>   struct kvm_arch_async_pf {
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index bae783cd396a..313343a43045 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -7756,6 +7756,47 @@ static int svm_get_page_enc_bitmap(struct kvm *kvm,
>   	return ret;
>   }
>   
> +static int svm_set_page_enc_bitmap(struct kvm *kvm,
> +				   struct kvm_page_enc_bitmap *bmap)
> +{
> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +	unsigned long gfn_start, gfn_end;
> +	unsigned long *bitmap;
> +	unsigned long sz, i;
> +	int ret;
> +
> +	if (!sev_guest(kvm))
> +		return -ENOTTY;
> +
> +	gfn_start = bmap->start_gfn;
> +	gfn_end = gfn_start + bmap->num_pages;


Same comment as the previous one. Do we continue if num_pages is zero ?

> +
> +	sz = ALIGN(bmap->num_pages, BITS_PER_LONG) / 8;
> +	bitmap = kmalloc(sz, GFP_KERNEL);
> +	if (!bitmap)
> +		return -ENOMEM;
> +
> +	ret = -EFAULT;
> +	if (copy_from_user(bitmap, bmap->enc_bitmap, sz))
> +		goto out;
> +
> +	mutex_lock(&kvm->lock);
> +	ret = sev_resize_page_enc_bitmap(kvm, gfn_end);
> +	if (ret)
> +		goto unlock;
> +
> +	i = gfn_start;
> +	for_each_clear_bit_from(i, bitmap, (gfn_end - gfn_start))
> +		clear_bit(i + gfn_start, sev->page_enc_bmap);
> +
> +	ret = 0;
> +unlock:
> +	mutex_unlock(&kvm->lock);
> +out:
> +	kfree(bitmap);
> +	return ret;
> +}
> +
>   static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
>   {
>   	struct kvm_sev_cmd sev_cmd;
> @@ -8161,6 +8202,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
>   
>   	.page_enc_status_hc = svm_page_enc_status_hc,
>   	.get_page_enc_bitmap = svm_get_page_enc_bitmap,
> +	.set_page_enc_bitmap = svm_set_page_enc_bitmap,
>   };
>   
>   static int __init svm_init(void)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3c3fea4e20b5..05e953b2ec61 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5238,6 +5238,18 @@ long kvm_arch_vm_ioctl(struct file *filp,
>   			r = kvm_x86_ops->get_page_enc_bitmap(kvm, &bitmap);
>   		break;
>   	}
> +	case KVM_SET_PAGE_ENC_BITMAP: {
> +		struct kvm_page_enc_bitmap bitmap;
> +
> +		r = -EFAULT;
> +		if (copy_from_user(&bitmap, argp, sizeof(bitmap)))
> +			goto out;
> +
> +		r = -ENOTTY;
> +		if (kvm_x86_ops->set_page_enc_bitmap)
> +			r = kvm_x86_ops->set_page_enc_bitmap(kvm, &bitmap);
> +		break;
> +	}
>   	default:
>   		r = -ENOTTY;
>   	}
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index db1ebf85e177..b4b01d47e568 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1489,6 +1489,7 @@ struct kvm_enc_region {
>   #define KVM_S390_CLEAR_RESET	_IO(KVMIO,   0xc4)
>   
>   #define KVM_GET_PAGE_ENC_BITMAP	_IOW(KVMIO, 0xc5, struct kvm_page_enc_bitmap)
> +#define KVM_SET_PAGE_ENC_BITMAP	_IOW(KVMIO, 0xc6, struct kvm_page_enc_bitmap)
>   
>   /* Secure Encrypted Virtualization command */
>   enum sev_cmd_id {
Reviewed-by: Krish Sadhukhan <[e-mail]>

Venu Busireddy April 3, 2020, 9:46 p.m. UTC | #2

On 2020-03-30 06:22:55 +0000, Ashish Kalra wrote:
> From: Brijesh Singh <[e-mail]>
> 
> The ioctl can be used to set page encryption bitmap for an
> incoming guest.
> 
> Cc: Thomas Gleixner <[e-mail]>
> Cc: Ingo Molnar <[e-mail]>
> Cc: "H. Peter Anvin" <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: "Radim Krčmář" <[e-mail]>
> Cc: Joerg Roedel <[e-mail]>
> Cc: Borislav Petkov <[e-mail]>
> Cc: Tom Lendacky <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Brijesh Singh <[e-mail]>
> Signed-off-by: Ashish Kalra <[e-mail]>

Reviewed-by: Venu Busireddy <[e-mail]>

> ---
>  Documentation/virt/kvm/api.rst  | 22 +++++++++++++++++
>  arch/x86/include/asm/kvm_host.h |  2 ++
>  arch/x86/kvm/svm.c              | 42 +++++++++++++++++++++++++++++++++
>  arch/x86/kvm/x86.c              | 12 ++++++++++
>  include/uapi/linux/kvm.h        |  1 +
>  5 files changed, 79 insertions(+)
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 8ad800ebb54f..4d1004a154f6 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -4675,6 +4675,28 @@ or shared. The bitmap can be used during the guest migration, if the page
>  is private then userspace need to use SEV migration commands to transmit
>  the page.
>  
> +4.126 KVM_SET_PAGE_ENC_BITMAP (vm ioctl)
> +---------------------------------------
> +
> +:Capability: basic
> +:Architectures: x86
> +:Type: vm ioctl
> +:Parameters: struct kvm_page_enc_bitmap (in/out)
> +:Returns: 0 on success, -1 on error
> +
> +/* for KVM_SET_PAGE_ENC_BITMAP */
> +struct kvm_page_enc_bitmap {
> +	__u64 start_gfn;
> +	__u64 num_pages;
> +	union {
> +		void __user *enc_bitmap; /* one bit per page */
> +		__u64 padding2;
> +	};
> +};
> +
> +During the guest live migration the outgoing guest exports its page encryption
> +bitmap, the KVM_SET_PAGE_ENC_BITMAP can be used to build the page encryption
> +bitmap for an incoming guest.
>  
>  5. The kvm_run structure
>  ========================
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 27e43e3ec9d8..d30f770aaaea 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1271,6 +1271,8 @@ struct kvm_x86_ops {
>  				  unsigned long sz, unsigned long mode);
>  	int (*get_page_enc_bitmap)(struct kvm *kvm,
>  				struct kvm_page_enc_bitmap *bmap);
> +	int (*set_page_enc_bitmap)(struct kvm *kvm,
> +				struct kvm_page_enc_bitmap *bmap);
>  };
>  
>  struct kvm_arch_async_pf {
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index bae783cd396a..313343a43045 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -7756,6 +7756,47 @@ static int svm_get_page_enc_bitmap(struct kvm *kvm,
>  	return ret;
>  }
>  
> +static int svm_set_page_enc_bitmap(struct kvm *kvm,
> +				   struct kvm_page_enc_bitmap *bmap)
> +{
> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +	unsigned long gfn_start, gfn_end;
> +	unsigned long *bitmap;
> +	unsigned long sz, i;
> +	int ret;
> +
> +	if (!sev_guest(kvm))
> +		return -ENOTTY;
> +
> +	gfn_start = bmap->start_gfn;
> +	gfn_end = gfn_start + bmap->num_pages;
> +
> +	sz = ALIGN(bmap->num_pages, BITS_PER_LONG) / 8;
> +	bitmap = kmalloc(sz, GFP_KERNEL);
> +	if (!bitmap)
> +		return -ENOMEM;
> +
> +	ret = -EFAULT;
> +	if (copy_from_user(bitmap, bmap->enc_bitmap, sz))
> +		goto out;
> +
> +	mutex_lock(&kvm->lock);
> +	ret = sev_resize_page_enc_bitmap(kvm, gfn_end);
> +	if (ret)
> +		goto unlock;
> +
> +	i = gfn_start;
> +	for_each_clear_bit_from(i, bitmap, (gfn_end - gfn_start))
> +		clear_bit(i + gfn_start, sev->page_enc_bmap);
> +
> +	ret = 0;
> +unlock:
> +	mutex_unlock(&kvm->lock);
> +out:
> +	kfree(bitmap);
> +	return ret;
> +}
> +
>  static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
>  {
>  	struct kvm_sev_cmd sev_cmd;
> @@ -8161,6 +8202,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
>  
>  	.page_enc_status_hc = svm_page_enc_status_hc,
>  	.get_page_enc_bitmap = svm_get_page_enc_bitmap,
> +	.set_page_enc_bitmap = svm_set_page_enc_bitmap,
>  };
>  
>  static int __init svm_init(void)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3c3fea4e20b5..05e953b2ec61 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5238,6 +5238,18 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  			r = kvm_x86_ops->get_page_enc_bitmap(kvm, &bitmap);
>  		break;
>  	}
> +	case KVM_SET_PAGE_ENC_BITMAP: {
> +		struct kvm_page_enc_bitmap bitmap;
> +
> +		r = -EFAULT;
> +		if (copy_from_user(&bitmap, argp, sizeof(bitmap)))
> +			goto out;
> +
> +		r = -ENOTTY;
> +		if (kvm_x86_ops->set_page_enc_bitmap)
> +			r = kvm_x86_ops->set_page_enc_bitmap(kvm, &bitmap);
> +		break;
> +	}
>  	default:
>  		r = -ENOTTY;
>  	}
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index db1ebf85e177..b4b01d47e568 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1489,6 +1489,7 @@ struct kvm_enc_region {
>  #define KVM_S390_CLEAR_RESET	_IO(KVMIO,   0xc4)
>  
>  #define KVM_GET_PAGE_ENC_BITMAP	_IOW(KVMIO, 0xc5, struct kvm_page_enc_bitmap)
> +#define KVM_SET_PAGE_ENC_BITMAP	_IOW(KVMIO, 0xc6, struct kvm_page_enc_bitmap)
>  
>  /* Secure Encrypted Virtualization command */
>  enum sev_cmd_id {
> -- 
> 2.17.1
>

Steve Rutherford April 8, 2020, 12:26 a.m. UTC | #3

On Sun, Mar 29, 2020 at 11:23 PM Ashish Kalra <[e-mail]> wrote:
>
> From: Brijesh Singh <[e-mail]>
>
> The ioctl can be used to set page encryption bitmap for an
> incoming guest.
>
> Cc: Thomas Gleixner <[e-mail]>
> Cc: Ingo Molnar <[e-mail]>
> Cc: "H. Peter Anvin" <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: "Radim Krčmář" <[e-mail]>
> Cc: Joerg Roedel <[e-mail]>
> Cc: Borislav Petkov <[e-mail]>
> Cc: Tom Lendacky <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Brijesh Singh <[e-mail]>
> Signed-off-by: Ashish Kalra <[e-mail]>
> ---
>  Documentation/virt/kvm/api.rst  | 22 +++++++++++++++++
>  arch/x86/include/asm/kvm_host.h |  2 ++
>  arch/x86/kvm/svm.c              | 42 +++++++++++++++++++++++++++++++++
>  arch/x86/kvm/x86.c              | 12 ++++++++++
>  include/uapi/linux/kvm.h        |  1 +
>  5 files changed, 79 insertions(+)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 8ad800ebb54f..4d1004a154f6 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -4675,6 +4675,28 @@ or shared. The bitmap can be used during the guest migration, if the page
>  is private then userspace need to use SEV migration commands to transmit
>  the page.
>
> +4.126 KVM_SET_PAGE_ENC_BITMAP (vm ioctl)
> +---------------------------------------
> +
> +:Capability: basic
> +:Architectures: x86
> +:Type: vm ioctl
> +:Parameters: struct kvm_page_enc_bitmap (in/out)
> +:Returns: 0 on success, -1 on error
> +
> +/* for KVM_SET_PAGE_ENC_BITMAP */
> +struct kvm_page_enc_bitmap {
> +       __u64 start_gfn;
> +       __u64 num_pages;
> +       union {
> +               void __user *enc_bitmap; /* one bit per page */
> +               __u64 padding2;
> +       };
> +};
> +
> +During the guest live migration the outgoing guest exports its page encryption
> +bitmap, the KVM_SET_PAGE_ENC_BITMAP can be used to build the page encryption
> +bitmap for an incoming guest.
>
>  5. The kvm_run structure
>  ========================
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 27e43e3ec9d8..d30f770aaaea 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1271,6 +1271,8 @@ struct kvm_x86_ops {
>                                   unsigned long sz, unsigned long mode);
>         int (*get_page_enc_bitmap)(struct kvm *kvm,
>                                 struct kvm_page_enc_bitmap *bmap);
> +       int (*set_page_enc_bitmap)(struct kvm *kvm,
> +                               struct kvm_page_enc_bitmap *bmap);
>  };
>
>  struct kvm_arch_async_pf {
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index bae783cd396a..313343a43045 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -7756,6 +7756,47 @@ static int svm_get_page_enc_bitmap(struct kvm *kvm,
>         return ret;
>  }
>
> +static int svm_set_page_enc_bitmap(struct kvm *kvm,
> +                                  struct kvm_page_enc_bitmap *bmap)
> +{
> +       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +       unsigned long gfn_start, gfn_end;
> +       unsigned long *bitmap;
> +       unsigned long sz, i;
> +       int ret;
> +
> +       if (!sev_guest(kvm))
> +               return -ENOTTY;
> +
> +       gfn_start = bmap->start_gfn;
> +       gfn_end = gfn_start + bmap->num_pages;
> +
> +       sz = ALIGN(bmap->num_pages, BITS_PER_LONG) / 8;
> +       bitmap = kmalloc(sz, GFP_KERNEL);
> +       if (!bitmap)
> +               return -ENOMEM;
> +
> +       ret = -EFAULT;
> +       if (copy_from_user(bitmap, bmap->enc_bitmap, sz))
> +               goto out;
> +
> +       mutex_lock(&kvm->lock);
> +       ret = sev_resize_page_enc_bitmap(kvm, gfn_end);
I realize now that usermode could use this for initializing the
minimum size of the enc bitmap, which probably solves my issue from
the other thread.
> +       if (ret)
> +               goto unlock;
> +
> +       i = gfn_start;
> +       for_each_clear_bit_from(i, bitmap, (gfn_end - gfn_start))
> +               clear_bit(i + gfn_start, sev->page_enc_bmap);
This API seems a bit strange, since it can only clear bits. I would
expect "set" to force the values to match the values passed down,
instead of only ensuring that cleared bits in the input are also
cleared in the kernel.

This should copy the values from userspace (and fix up the ends since
byte alignment makes that complicated), instead of iterating in this
way.
> +
> +       ret = 0;
> +unlock:
> +       mutex_unlock(&kvm->lock);
> +out:
> +       kfree(bitmap);
> +       return ret;
> +}
> +
>  static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
>  {
>         struct kvm_sev_cmd sev_cmd;
> @@ -8161,6 +8202,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
>
>         .page_enc_status_hc = svm_page_enc_status_hc,
>         .get_page_enc_bitmap = svm_get_page_enc_bitmap,
> +       .set_page_enc_bitmap = svm_set_page_enc_bitmap,
>  };
>
>  static int __init svm_init(void)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3c3fea4e20b5..05e953b2ec61 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5238,6 +5238,18 @@ long kvm_arch_vm_ioctl(struct file *filp,
>                         r = kvm_x86_ops->get_page_enc_bitmap(kvm, &bitmap);
>                 break;
>         }
> +       case KVM_SET_PAGE_ENC_BITMAP: {
> +               struct kvm_page_enc_bitmap bitmap;
> +
> +               r = -EFAULT;
> +               if (copy_from_user(&bitmap, argp, sizeof(bitmap)))
> +                       goto out;
> +
> +               r = -ENOTTY;
> +               if (kvm_x86_ops->set_page_enc_bitmap)
> +                       r = kvm_x86_ops->set_page_enc_bitmap(kvm, &bitmap);
> +               break;
> +       }
>         default:
>                 r = -ENOTTY;
>         }
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index db1ebf85e177..b4b01d47e568 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1489,6 +1489,7 @@ struct kvm_enc_region {
>  #define KVM_S390_CLEAR_RESET   _IO(KVMIO,   0xc4)
>
>  #define KVM_GET_PAGE_ENC_BITMAP        _IOW(KVMIO, 0xc5, struct kvm_page_enc_bitmap)
> +#define KVM_SET_PAGE_ENC_BITMAP        _IOW(KVMIO, 0xc6, struct kvm_page_enc_bitmap)
>
>  /* Secure Encrypted Virtualization command */
>  enum sev_cmd_id {
> --
> 2.17.1
>

Kalra, Ashish April 8, 2020, 1:48 a.m. UTC | #4

Hello Steve,

On Tue, Apr 07, 2020 at 05:26:33PM -0700, Steve Rutherford wrote:
> On Sun, Mar 29, 2020 at 11:23 PM Ashish Kalra <[e-mail]> wrote:
> >
> > From: Brijesh Singh <[e-mail]>
> >
> > The ioctl can be used to set page encryption bitmap for an
> > incoming guest.
> >
> > Cc: Thomas Gleixner <[e-mail]>
> > Cc: Ingo Molnar <[e-mail]>
> > Cc: "H. Peter Anvin" <[e-mail]>
> > Cc: Paolo Bonzini <[e-mail]>
> > Cc: "Radim Krčmář" <[e-mail]>
> > Cc: Joerg Roedel <[e-mail]>
> > Cc: Borislav Petkov <[e-mail]>
> > Cc: Tom Lendacky <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Signed-off-by: Brijesh Singh <[e-mail]>
> > Signed-off-by: Ashish Kalra <[e-mail]>
> > ---
> >  Documentation/virt/kvm/api.rst  | 22 +++++++++++++++++
> >  arch/x86/include/asm/kvm_host.h |  2 ++
> >  arch/x86/kvm/svm.c              | 42 +++++++++++++++++++++++++++++++++
> >  arch/x86/kvm/x86.c              | 12 ++++++++++
> >  include/uapi/linux/kvm.h        |  1 +
> >  5 files changed, 79 insertions(+)
> >
> > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> > index 8ad800ebb54f..4d1004a154f6 100644
> > --- a/Documentation/virt/kvm/api.rst
> > +++ b/Documentation/virt/kvm/api.rst
> > @@ -4675,6 +4675,28 @@ or shared. The bitmap can be used during the guest migration, if the page
> >  is private then userspace need to use SEV migration commands to transmit
> >  the page.
> >
> > +4.126 KVM_SET_PAGE_ENC_BITMAP (vm ioctl)
> > +---------------------------------------
> > +
> > +:Capability: basic
> > +:Architectures: x86
> > +:Type: vm ioctl
> > +:Parameters: struct kvm_page_enc_bitmap (in/out)
> > +:Returns: 0 on success, -1 on error
> > +
> > +/* for KVM_SET_PAGE_ENC_BITMAP */
> > +struct kvm_page_enc_bitmap {
> > +       __u64 start_gfn;
> > +       __u64 num_pages;
> > +       union {
> > +               void __user *enc_bitmap; /* one bit per page */
> > +               __u64 padding2;
> > +       };
> > +};
> > +
> > +During the guest live migration the outgoing guest exports its page encryption
> > +bitmap, the KVM_SET_PAGE_ENC_BITMAP can be used to build the page encryption
> > +bitmap for an incoming guest.
> >
> >  5. The kvm_run structure
> >  ========================
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 27e43e3ec9d8..d30f770aaaea 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1271,6 +1271,8 @@ struct kvm_x86_ops {
> >                                   unsigned long sz, unsigned long mode);
> >         int (*get_page_enc_bitmap)(struct kvm *kvm,
> >                                 struct kvm_page_enc_bitmap *bmap);
> > +       int (*set_page_enc_bitmap)(struct kvm *kvm,
> > +                               struct kvm_page_enc_bitmap *bmap);
> >  };
> >
> >  struct kvm_arch_async_pf {
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index bae783cd396a..313343a43045 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -7756,6 +7756,47 @@ static int svm_get_page_enc_bitmap(struct kvm *kvm,
> >         return ret;
> >  }
> >
> > +static int svm_set_page_enc_bitmap(struct kvm *kvm,
> > +                                  struct kvm_page_enc_bitmap *bmap)
> > +{
> > +       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > +       unsigned long gfn_start, gfn_end;
> > +       unsigned long *bitmap;
> > +       unsigned long sz, i;
> > +       int ret;
> > +
> > +       if (!sev_guest(kvm))
> > +               return -ENOTTY;
> > +
> > +       gfn_start = bmap->start_gfn;
> > +       gfn_end = gfn_start + bmap->num_pages;
> > +
> > +       sz = ALIGN(bmap->num_pages, BITS_PER_LONG) / 8;
> > +       bitmap = kmalloc(sz, GFP_KERNEL);
> > +       if (!bitmap)
> > +               return -ENOMEM;
> > +
> > +       ret = -EFAULT;
> > +       if (copy_from_user(bitmap, bmap->enc_bitmap, sz))
> > +               goto out;
> > +
> > +       mutex_lock(&kvm->lock);
> > +       ret = sev_resize_page_enc_bitmap(kvm, gfn_end);
> I realize now that usermode could use this for initializing the
> minimum size of the enc bitmap, which probably solves my issue from
> the other thread.
> > +       if (ret)
> > +               goto unlock;
> > +
> > +       i = gfn_start;
> > +       for_each_clear_bit_from(i, bitmap, (gfn_end - gfn_start))
> > +               clear_bit(i + gfn_start, sev->page_enc_bmap);
> This API seems a bit strange, since it can only clear bits. I would
> expect "set" to force the values to match the values passed down,
> instead of only ensuring that cleared bits in the input are also
> cleared in the kernel.
>

The sev_resize_page_enc_bitmap() will allocate a new bitmap and
set it to all 0xFF's, therefore, the code here simply clears the bits
in the bitmap as per the cleared bits in the input.

Thanks,
Ashish

> This should copy the values from userspace (and fix up the ends since
> byte alignment makes that complicated), instead of iterating in this
> way.
> > +
> > +       ret = 0;
> > +unlock:
> > +       mutex_unlock(&kvm->lock);
> > +out:
> > +       kfree(bitmap);
> > +       return ret;
> > +}
> > +
> >  static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
> >  {
> >         struct kvm_sev_cmd sev_cmd;
> > @@ -8161,6 +8202,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
> >
> >         .page_enc_status_hc = svm_page_enc_status_hc,
> >         .get_page_enc_bitmap = svm_get_page_enc_bitmap,
> > +       .set_page_enc_bitmap = svm_set_page_enc_bitmap,
> >  };
> >
> >  static int __init svm_init(void)
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 3c3fea4e20b5..05e953b2ec61 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -5238,6 +5238,18 @@ long kvm_arch_vm_ioctl(struct file *filp,
> >                         r = kvm_x86_ops->get_page_enc_bitmap(kvm, &bitmap);
> >                 break;
> >         }
> > +       case KVM_SET_PAGE_ENC_BITMAP: {
> > +               struct kvm_page_enc_bitmap bitmap;
> > +
> > +               r = -EFAULT;
> > +               if (copy_from_user(&bitmap, argp, sizeof(bitmap)))
> > +                       goto out;
> > +
> > +               r = -ENOTTY;
> > +               if (kvm_x86_ops->set_page_enc_bitmap)
> > +                       r = kvm_x86_ops->set_page_enc_bitmap(kvm, &bitmap);
> > +               break;
> > +       }
> >         default:
> >                 r = -ENOTTY;
> >         }
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index db1ebf85e177..b4b01d47e568 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -1489,6 +1489,7 @@ struct kvm_enc_region {
> >  #define KVM_S390_CLEAR_RESET   _IO(KVMIO,   0xc4)
> >
> >  #define KVM_GET_PAGE_ENC_BITMAP        _IOW(KVMIO, 0xc5, struct kvm_page_enc_bitmap)
> > +#define KVM_SET_PAGE_ENC_BITMAP        _IOW(KVMIO, 0xc6, struct kvm_page_enc_bitmap)
> >
> >  /* Secure Encrypted Virtualization command */
> >  enum sev_cmd_id {
> > --
> > 2.17.1
> >

Steve Rutherford April 10, 2020, 12:06 a.m. UTC | #5

On Tue, Apr 7, 2020 at 6:49 PM Ashish Kalra <[e-mail]> wrote:
>
> Hello Steve,
>
> On Tue, Apr 07, 2020 at 05:26:33PM -0700, Steve Rutherford wrote:
> > On Sun, Mar 29, 2020 at 11:23 PM Ashish Kalra <[e-mail]> wrote:
> > >
> > > From: Brijesh Singh <[e-mail]>
> > >
> > > The ioctl can be used to set page encryption bitmap for an
> > > incoming guest.
> > >
> > > Cc: Thomas Gleixner <[e-mail]>
> > > Cc: Ingo Molnar <[e-mail]>
> > > Cc: "H. Peter Anvin" <[e-mail]>
> > > Cc: Paolo Bonzini <[e-mail]>
> > > Cc: "Radim Krčmář" <[e-mail]>
> > > Cc: Joerg Roedel <[e-mail]>
> > > Cc: Borislav Petkov <[e-mail]>
> > > Cc: Tom Lendacky <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > Signed-off-by: Brijesh Singh <[e-mail]>
> > > Signed-off-by: Ashish Kalra <[e-mail]>
> > > ---
> > >  Documentation/virt/kvm/api.rst  | 22 +++++++++++++++++
> > >  arch/x86/include/asm/kvm_host.h |  2 ++
> > >  arch/x86/kvm/svm.c              | 42 +++++++++++++++++++++++++++++++++
> > >  arch/x86/kvm/x86.c              | 12 ++++++++++
> > >  include/uapi/linux/kvm.h        |  1 +
> > >  5 files changed, 79 insertions(+)
> > >
> > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> > > index 8ad800ebb54f..4d1004a154f6 100644
> > > --- a/Documentation/virt/kvm/api.rst
> > > +++ b/Documentation/virt/kvm/api.rst
> > > @@ -4675,6 +4675,28 @@ or shared. The bitmap can be used during the guest migration, if the page
> > >  is private then userspace need to use SEV migration commands to transmit
> > >  the page.
> > >
> > > +4.126 KVM_SET_PAGE_ENC_BITMAP (vm ioctl)
> > > +---------------------------------------
> > > +
> > > +:Capability: basic
> > > +:Architectures: x86
> > > +:Type: vm ioctl
> > > +:Parameters: struct kvm_page_enc_bitmap (in/out)
> > > +:Returns: 0 on success, -1 on error
> > > +
> > > +/* for KVM_SET_PAGE_ENC_BITMAP */
> > > +struct kvm_page_enc_bitmap {
> > > +       __u64 start_gfn;
> > > +       __u64 num_pages;
> > > +       union {
> > > +               void __user *enc_bitmap; /* one bit per page */
> > > +               __u64 padding2;
> > > +       };
> > > +};
> > > +
> > > +During the guest live migration the outgoing guest exports its page encryption
> > > +bitmap, the KVM_SET_PAGE_ENC_BITMAP can be used to build the page encryption
> > > +bitmap for an incoming guest.
> > >
> > >  5. The kvm_run structure
> > >  ========================
> > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > index 27e43e3ec9d8..d30f770aaaea 100644
> > > --- a/arch/x86/include/asm/kvm_host.h
> > > +++ b/arch/x86/include/asm/kvm_host.h
> > > @@ -1271,6 +1271,8 @@ struct kvm_x86_ops {
> > >                                   unsigned long sz, unsigned long mode);
> > >         int (*get_page_enc_bitmap)(struct kvm *kvm,
> > >                                 struct kvm_page_enc_bitmap *bmap);
> > > +       int (*set_page_enc_bitmap)(struct kvm *kvm,
> > > +                               struct kvm_page_enc_bitmap *bmap);
> > >  };
> > >
> > >  struct kvm_arch_async_pf {
> > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > > index bae783cd396a..313343a43045 100644
> > > --- a/arch/x86/kvm/svm.c
> > > +++ b/arch/x86/kvm/svm.c
> > > @@ -7756,6 +7756,47 @@ static int svm_get_page_enc_bitmap(struct kvm *kvm,
> > >         return ret;
> > >  }
> > >
> > > +static int svm_set_page_enc_bitmap(struct kvm *kvm,
> > > +                                  struct kvm_page_enc_bitmap *bmap)
> > > +{
> > > +       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > > +       unsigned long gfn_start, gfn_end;
> > > +       unsigned long *bitmap;
> > > +       unsigned long sz, i;
> > > +       int ret;
> > > +
> > > +       if (!sev_guest(kvm))
> > > +               return -ENOTTY;
> > > +
> > > +       gfn_start = bmap->start_gfn;
> > > +       gfn_end = gfn_start + bmap->num_pages;
> > > +
> > > +       sz = ALIGN(bmap->num_pages, BITS_PER_LONG) / 8;
> > > +       bitmap = kmalloc(sz, GFP_KERNEL);
> > > +       if (!bitmap)
> > > +               return -ENOMEM;
> > > +
> > > +       ret = -EFAULT;
> > > +       if (copy_from_user(bitmap, bmap->enc_bitmap, sz))
> > > +               goto out;
> > > +
> > > +       mutex_lock(&kvm->lock);
> > > +       ret = sev_resize_page_enc_bitmap(kvm, gfn_end);
> > I realize now that usermode could use this for initializing the
> > minimum size of the enc bitmap, which probably solves my issue from
> > the other thread.
> > > +       if (ret)
> > > +               goto unlock;
> > > +
> > > +       i = gfn_start;
> > > +       for_each_clear_bit_from(i, bitmap, (gfn_end - gfn_start))
> > > +               clear_bit(i + gfn_start, sev->page_enc_bmap);
> > This API seems a bit strange, since it can only clear bits. I would
> > expect "set" to force the values to match the values passed down,
> > instead of only ensuring that cleared bits in the input are also
> > cleared in the kernel.
> >
>
> The sev_resize_page_enc_bitmap() will allocate a new bitmap and
> set it to all 0xFF's, therefore, the code here simply clears the bits
> in the bitmap as per the cleared bits in the input.

If I'm not mistaken, resize only reinitializes the newly extended part
of the buffer, and copies the old values for the rest.
With the API you proposed you could probably reimplement a normal set
call by calling get, then reset, and then set, but this feels
cumbersome.

--Steve

Kalra, Ashish April 10, 2020, 1:23 a.m. UTC | #6

Hello Steve,

On Thu, Apr 09, 2020 at 05:06:21PM -0700, Steve Rutherford wrote:
> On Tue, Apr 7, 2020 at 6:49 PM Ashish Kalra <[e-mail]> wrote:
> >
> > Hello Steve,
> >
> > On Tue, Apr 07, 2020 at 05:26:33PM -0700, Steve Rutherford wrote:
> > > On Sun, Mar 29, 2020 at 11:23 PM Ashish Kalra <[e-mail]> wrote:
> > > >
> > > > From: Brijesh Singh <[e-mail]>
> > > >
> > > > The ioctl can be used to set page encryption bitmap for an
> > > > incoming guest.
> > > >
> > > > Cc: Thomas Gleixner <[e-mail]>
> > > > Cc: Ingo Molnar <[e-mail]>
> > > > Cc: "H. Peter Anvin" <[e-mail]>
> > > > Cc: Paolo Bonzini <[e-mail]>
> > > > Cc: "Radim Krčmář" <[e-mail]>
> > > > Cc: Joerg Roedel <[e-mail]>
> > > > Cc: Borislav Petkov <[e-mail]>
> > > > Cc: Tom Lendacky <[e-mail]>
> > > > Cc: [e-mail]
> > > > Cc: [e-mail]
> > > > Cc: [e-mail]
> > > > Signed-off-by: Brijesh Singh <[e-mail]>
> > > > Signed-off-by: Ashish Kalra <[e-mail]>
> > > > ---
> > > >  Documentation/virt/kvm/api.rst  | 22 +++++++++++++++++
> > > >  arch/x86/include/asm/kvm_host.h |  2 ++
> > > >  arch/x86/kvm/svm.c              | 42 +++++++++++++++++++++++++++++++++
> > > >  arch/x86/kvm/x86.c              | 12 ++++++++++
> > > >  include/uapi/linux/kvm.h        |  1 +
> > > >  5 files changed, 79 insertions(+)
> > > >
> > > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> > > > index 8ad800ebb54f..4d1004a154f6 100644
> > > > --- a/Documentation/virt/kvm/api.rst
> > > > +++ b/Documentation/virt/kvm/api.rst
> > > > @@ -4675,6 +4675,28 @@ or shared. The bitmap can be used during the guest migration, if the page
> > > >  is private then userspace need to use SEV migration commands to transmit
> > > >  the page.
> > > >
> > > > +4.126 KVM_SET_PAGE_ENC_BITMAP (vm ioctl)
> > > > +---------------------------------------
> > > > +
> > > > +:Capability: basic
> > > > +:Architectures: x86
> > > > +:Type: vm ioctl
> > > > +:Parameters: struct kvm_page_enc_bitmap (in/out)
> > > > +:Returns: 0 on success, -1 on error
> > > > +
> > > > +/* for KVM_SET_PAGE_ENC_BITMAP */
> > > > +struct kvm_page_enc_bitmap {
> > > > +       __u64 start_gfn;
> > > > +       __u64 num_pages;
> > > > +       union {
> > > > +               void __user *enc_bitmap; /* one bit per page */
> > > > +               __u64 padding2;
> > > > +       };
> > > > +};
> > > > +
> > > > +During the guest live migration the outgoing guest exports its page encryption
> > > > +bitmap, the KVM_SET_PAGE_ENC_BITMAP can be used to build the page encryption
> > > > +bitmap for an incoming guest.
> > > >
> > > >  5. The kvm_run structure
> > > >  ========================
> > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > > index 27e43e3ec9d8..d30f770aaaea 100644
> > > > --- a/arch/x86/include/asm/kvm_host.h
> > > > +++ b/arch/x86/include/asm/kvm_host.h
> > > > @@ -1271,6 +1271,8 @@ struct kvm_x86_ops {
> > > >                                   unsigned long sz, unsigned long mode);
> > > >         int (*get_page_enc_bitmap)(struct kvm *kvm,
> > > >                                 struct kvm_page_enc_bitmap *bmap);
> > > > +       int (*set_page_enc_bitmap)(struct kvm *kvm,
> > > > +                               struct kvm_page_enc_bitmap *bmap);
> > > >  };
> > > >
> > > >  struct kvm_arch_async_pf {
> > > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > > > index bae783cd396a..313343a43045 100644
> > > > --- a/arch/x86/kvm/svm.c
> > > > +++ b/arch/x86/kvm/svm.c
> > > > @@ -7756,6 +7756,47 @@ static int svm_get_page_enc_bitmap(struct kvm *kvm,
> > > >         return ret;
> > > >  }
> > > >
> > > > +static int svm_set_page_enc_bitmap(struct kvm *kvm,
> > > > +                                  struct kvm_page_enc_bitmap *bmap)
> > > > +{
> > > > +       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > > > +       unsigned long gfn_start, gfn_end;
> > > > +       unsigned long *bitmap;
> > > > +       unsigned long sz, i;
> > > > +       int ret;
> > > > +
> > > > +       if (!sev_guest(kvm))
> > > > +               return -ENOTTY;
> > > > +
> > > > +       gfn_start = bmap->start_gfn;
> > > > +       gfn_end = gfn_start + bmap->num_pages;
> > > > +
> > > > +       sz = ALIGN(bmap->num_pages, BITS_PER_LONG) / 8;
> > > > +       bitmap = kmalloc(sz, GFP_KERNEL);
> > > > +       if (!bitmap)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       ret = -EFAULT;
> > > > +       if (copy_from_user(bitmap, bmap->enc_bitmap, sz))
> > > > +               goto out;
> > > > +
> > > > +       mutex_lock(&kvm->lock);
> > > > +       ret = sev_resize_page_enc_bitmap(kvm, gfn_end);
> > > I realize now that usermode could use this for initializing the
> > > minimum size of the enc bitmap, which probably solves my issue from
> > > the other thread.
> > > > +       if (ret)
> > > > +               goto unlock;
> > > > +
> > > > +       i = gfn_start;
> > > > +       for_each_clear_bit_from(i, bitmap, (gfn_end - gfn_start))
> > > > +               clear_bit(i + gfn_start, sev->page_enc_bmap);
> > > This API seems a bit strange, since it can only clear bits. I would
> > > expect "set" to force the values to match the values passed down,
> > > instead of only ensuring that cleared bits in the input are also
> > > cleared in the kernel.
> > >
> >
> > The sev_resize_page_enc_bitmap() will allocate a new bitmap and
> > set it to all 0xFF's, therefore, the code here simply clears the bits
> > in the bitmap as per the cleared bits in the input.
> 
> If I'm not mistaken, resize only reinitializes the newly extended part
> of the buffer, and copies the old values for the rest.
> With the API you proposed you could probably reimplement a normal set
> call by calling get, then reset, and then set, but this feels
> cumbersome.
> 

As i mentioned earlier, the set api is basically meant for the incoming
VM, the resize will initialize the incoming VM's bitmap to all 0xFF's
and as there won't be any bitmap allocated initially on the incoming VM,
therefore, the bitmap copy will not do anything and the clear_bit later
will clear the incoming VM's bits as per the input.

Thanks,
Ashish

Steve Rutherford April 10, 2020, 6:08 p.m. UTC | #7

On Thu, Apr 9, 2020 at 6:23 PM Ashish Kalra <[e-mail]> wrote:
>
> Hello Steve,
>
> On Thu, Apr 09, 2020 at 05:06:21PM -0700, Steve Rutherford wrote:
> > On Tue, Apr 7, 2020 at 6:49 PM Ashish Kalra <[e-mail]> wrote:
> > >
> > > Hello Steve,
> > >
> > > On Tue, Apr 07, 2020 at 05:26:33PM -0700, Steve Rutherford wrote:
> > > > On Sun, Mar 29, 2020 at 11:23 PM Ashish Kalra <[e-mail]> wrote:
> > > > >
> > > > > From: Brijesh Singh <[e-mail]>
> > > > >
> > > > > The ioctl can be used to set page encryption bitmap for an
> > > > > incoming guest.
> > > > >
> > > > > Cc: Thomas Gleixner <[e-mail]>
> > > > > Cc: Ingo Molnar <[e-mail]>
> > > > > Cc: "H. Peter Anvin" <[e-mail]>
> > > > > Cc: Paolo Bonzini <[e-mail]>
> > > > > Cc: "Radim Krčmář" <[e-mail]>
> > > > > Cc: Joerg Roedel <[e-mail]>
> > > > > Cc: Borislav Petkov <[e-mail]>
> > > > > Cc: Tom Lendacky <[e-mail]>
> > > > > Cc: [e-mail]
> > > > > Cc: [e-mail]
> > > > > Cc: [e-mail]
> > > > > Signed-off-by: Brijesh Singh <[e-mail]>
> > > > > Signed-off-by: Ashish Kalra <[e-mail]>
> > > > > ---
> > > > >  Documentation/virt/kvm/api.rst  | 22 +++++++++++++++++
> > > > >  arch/x86/include/asm/kvm_host.h |  2 ++
> > > > >  arch/x86/kvm/svm.c              | 42 +++++++++++++++++++++++++++++++++
> > > > >  arch/x86/kvm/x86.c              | 12 ++++++++++
> > > > >  include/uapi/linux/kvm.h        |  1 +
> > > > >  5 files changed, 79 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> > > > > index 8ad800ebb54f..4d1004a154f6 100644
> > > > > --- a/Documentation/virt/kvm/api.rst
> > > > > +++ b/Documentation/virt/kvm/api.rst
> > > > > @@ -4675,6 +4675,28 @@ or shared. The bitmap can be used during the guest migration, if the page
> > > > >  is private then userspace need to use SEV migration commands to transmit
> > > > >  the page.
> > > > >
> > > > > +4.126 KVM_SET_PAGE_ENC_BITMAP (vm ioctl)
> > > > > +---------------------------------------
> > > > > +
> > > > > +:Capability: basic
> > > > > +:Architectures: x86
> > > > > +:Type: vm ioctl
> > > > > +:Parameters: struct kvm_page_enc_bitmap (in/out)
> > > > > +:Returns: 0 on success, -1 on error
> > > > > +
> > > > > +/* for KVM_SET_PAGE_ENC_BITMAP */
> > > > > +struct kvm_page_enc_bitmap {
> > > > > +       __u64 start_gfn;
> > > > > +       __u64 num_pages;
> > > > > +       union {
> > > > > +               void __user *enc_bitmap; /* one bit per page */
> > > > > +               __u64 padding2;
> > > > > +       };
> > > > > +};
> > > > > +
> > > > > +During the guest live migration the outgoing guest exports its page encryption
> > > > > +bitmap, the KVM_SET_PAGE_ENC_BITMAP can be used to build the page encryption
> > > > > +bitmap for an incoming guest.
> > > > >
> > > > >  5. The kvm_run structure
> > > > >  ========================
> > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > > > index 27e43e3ec9d8..d30f770aaaea 100644
> > > > > --- a/arch/x86/include/asm/kvm_host.h
> > > > > +++ b/arch/x86/include/asm/kvm_host.h
> > > > > @@ -1271,6 +1271,8 @@ struct kvm_x86_ops {
> > > > >                                   unsigned long sz, unsigned long mode);
> > > > >         int (*get_page_enc_bitmap)(struct kvm *kvm,
> > > > >                                 struct kvm_page_enc_bitmap *bmap);
> > > > > +       int (*set_page_enc_bitmap)(struct kvm *kvm,
> > > > > +                               struct kvm_page_enc_bitmap *bmap);
> > > > >  };
> > > > >
> > > > >  struct kvm_arch_async_pf {
> > > > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > > > > index bae783cd396a..313343a43045 100644
> > > > > --- a/arch/x86/kvm/svm.c
> > > > > +++ b/arch/x86/kvm/svm.c
> > > > > @@ -7756,6 +7756,47 @@ static int svm_get_page_enc_bitmap(struct kvm *kvm,
> > > > >         return ret;
> > > > >  }
> > > > >
> > > > > +static int svm_set_page_enc_bitmap(struct kvm *kvm,
> > > > > +                                  struct kvm_page_enc_bitmap *bmap)
> > > > > +{
> > > > > +       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > > > > +       unsigned long gfn_start, gfn_end;
> > > > > +       unsigned long *bitmap;
> > > > > +       unsigned long sz, i;
> > > > > +       int ret;
> > > > > +
> > > > > +       if (!sev_guest(kvm))
> > > > > +               return -ENOTTY;
> > > > > +
> > > > > +       gfn_start = bmap->start_gfn;
> > > > > +       gfn_end = gfn_start + bmap->num_pages;
> > > > > +
> > > > > +       sz = ALIGN(bmap->num_pages, BITS_PER_LONG) / 8;
> > > > > +       bitmap = kmalloc(sz, GFP_KERNEL);
> > > > > +       if (!bitmap)
> > > > > +               return -ENOMEM;
> > > > > +
> > > > > +       ret = -EFAULT;
> > > > > +       if (copy_from_user(bitmap, bmap->enc_bitmap, sz))
> > > > > +               goto out;
> > > > > +
> > > > > +       mutex_lock(&kvm->lock);
> > > > > +       ret = sev_resize_page_enc_bitmap(kvm, gfn_end);
> > > > I realize now that usermode could use this for initializing the
> > > > minimum size of the enc bitmap, which probably solves my issue from
> > > > the other thread.
> > > > > +       if (ret)
> > > > > +               goto unlock;
> > > > > +
> > > > > +       i = gfn_start;
> > > > > +       for_each_clear_bit_from(i, bitmap, (gfn_end - gfn_start))
> > > > > +               clear_bit(i + gfn_start, sev->page_enc_bmap);
> > > > This API seems a bit strange, since it can only clear bits. I would
> > > > expect "set" to force the values to match the values passed down,
> > > > instead of only ensuring that cleared bits in the input are also
> > > > cleared in the kernel.
> > > >
> > >
> > > The sev_resize_page_enc_bitmap() will allocate a new bitmap and
> > > set it to all 0xFF's, therefore, the code here simply clears the bits
> > > in the bitmap as per the cleared bits in the input.
> >
> > If I'm not mistaken, resize only reinitializes the newly extended part
> > of the buffer, and copies the old values for the rest.
> > With the API you proposed you could probably reimplement a normal set
> > call by calling get, then reset, and then set, but this feels
> > cumbersome.
> >
>
> As i mentioned earlier, the set api is basically meant for the incoming
> VM, the resize will initialize the incoming VM's bitmap to all 0xFF's
> and as there won't be any bitmap allocated initially on the incoming VM,
> therefore, the bitmap copy will not do anything and the clear_bit later
> will clear the incoming VM's bits as per the input.

The documentation does not make that super clear. A typical set call
in the KVM API let's you go to any state, not just a subset of states.
Yes, this works in the common case of migrating a VM to a particular
target, once. I find the behavior of the current API surprising. I
prefer APIs that are unsurprising. If I were to not have read the
code, it would be very easy for me to have assumed it worked like a
normal set call. You could rename the ioctl something like
"CLEAR_BITS", but a set based API is more common.

Thanks,
Steve

Patch

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 8ad800ebb54f..4d1004a154f6 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -4675,6 +4675,28 @@  or shared. The bitmap can be used during the guest migration, if the page
 is private then userspace need to use SEV migration commands to transmit
 the page.
 
+4.126 KVM_SET_PAGE_ENC_BITMAP (vm ioctl)
+---------------------------------------
+
+:Capability: basic
+:Architectures: x86
+:Type: vm ioctl
+:Parameters: struct kvm_page_enc_bitmap (in/out)
+:Returns: 0 on success, -1 on error
+
+/* for KVM_SET_PAGE_ENC_BITMAP */
+struct kvm_page_enc_bitmap {
+	__u64 start_gfn;
+	__u64 num_pages;
+	union {
+		void __user *enc_bitmap; /* one bit per page */
+		__u64 padding2;
+	};
+};
+
+During the guest live migration the outgoing guest exports its page encryption
+bitmap, the KVM_SET_PAGE_ENC_BITMAP can be used to build the page encryption
+bitmap for an incoming guest.
 
 5. The kvm_run structure
 ========================
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 27e43e3ec9d8..d30f770aaaea 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1271,6 +1271,8 @@  struct kvm_x86_ops {
 				  unsigned long sz, unsigned long mode);
 	int (*get_page_enc_bitmap)(struct kvm *kvm,
 				struct kvm_page_enc_bitmap *bmap);
+	int (*set_page_enc_bitmap)(struct kvm *kvm,
+				struct kvm_page_enc_bitmap *bmap);
 };
 
 struct kvm_arch_async_pf {
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index bae783cd396a..313343a43045 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -7756,6 +7756,47 @@  static int svm_get_page_enc_bitmap(struct kvm *kvm,
 	return ret;
 }
 
+static int svm_set_page_enc_bitmap(struct kvm *kvm,
+				   struct kvm_page_enc_bitmap *bmap)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	unsigned long gfn_start, gfn_end;
+	unsigned long *bitmap;
+	unsigned long sz, i;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	gfn_start = bmap->start_gfn;
+	gfn_end = gfn_start + bmap->num_pages;
+
+	sz = ALIGN(bmap->num_pages, BITS_PER_LONG) / 8;
+	bitmap = kmalloc(sz, GFP_KERNEL);
+	if (!bitmap)
+		return -ENOMEM;
+
+	ret = -EFAULT;
+	if (copy_from_user(bitmap, bmap->enc_bitmap, sz))
+		goto out;
+
+	mutex_lock(&kvm->lock);
+	ret = sev_resize_page_enc_bitmap(kvm, gfn_end);
+	if (ret)
+		goto unlock;
+
+	i = gfn_start;
+	for_each_clear_bit_from(i, bitmap, (gfn_end - gfn_start))
+		clear_bit(i + gfn_start, sev->page_enc_bmap);
+
+	ret = 0;
+unlock:
+	mutex_unlock(&kvm->lock);
+out:
+	kfree(bitmap);
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;
@@ -8161,6 +8202,7 @@  static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
 
 	.page_enc_status_hc = svm_page_enc_status_hc,
 	.get_page_enc_bitmap = svm_get_page_enc_bitmap,
+	.set_page_enc_bitmap = svm_set_page_enc_bitmap,
 };
 
 static int __init svm_init(void)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3c3fea4e20b5..05e953b2ec61 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5238,6 +5238,18 @@  long kvm_arch_vm_ioctl(struct file *filp,
 			r = kvm_x86_ops->get_page_enc_bitmap(kvm, &bitmap);
 		break;
 	}
+	case KVM_SET_PAGE_ENC_BITMAP: {
+		struct kvm_page_enc_bitmap bitmap;
+
+		r = -EFAULT;
+		if (copy_from_user(&bitmap, argp, sizeof(bitmap)))
+			goto out;
+
+		r = -ENOTTY;
+		if (kvm_x86_ops->set_page_enc_bitmap)
+			r = kvm_x86_ops->set_page_enc_bitmap(kvm, &bitmap);
+		break;
+	}
 	default:
 		r = -ENOTTY;
 	}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index db1ebf85e177..b4b01d47e568 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1489,6 +1489,7 @@  struct kvm_enc_region {
 #define KVM_S390_CLEAR_RESET	_IO(KVMIO,   0xc4)
 
 #define KVM_GET_PAGE_ENC_BITMAP	_IOW(KVMIO, 0xc5, struct kvm_page_enc_bitmap)
+#define KVM_SET_PAGE_ENC_BITMAP	_IOW(KVMIO, 0xc6, struct kvm_page_enc_bitmap)
 
 /* Secure Encrypted Virtualization command */
 enum sev_cmd_id {