Add AMD SEV guest live migration support

From: Brijesh Singh <[email protected]>

This hypercall is used by the SEV guest to notify a change in the page encryption status to the hypervisor. The hypercall should be invoked only when the encryption attribute is changed from encrypted -> decrypted and vice versa. By default all guest pages are considered encrypted.

The patch introduces a new shared pages list implemented as a sorted linked list to track the shared/unencrypted regions marked by the guest hypercall.

arch/x86/kvm/svm/sev.c +6 -0

+
+	mutex_unlock(&kvm->lock);
+	return ret;
+}
+
  int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
  {
  	struct kvm_sev_cmd sev_cmd;

@@ -1693,6 +1842,7 @@ void sev_vm_destroy(struct kvm *kvm)
 
  	sev_unbind_asid(kvm, sev->handle);
  	sev_asid_free(sev->asid);
+	sev->shared_pages_list_count = 0;
  }
 
  void __init sev_hardware_setup(void)

arch/x86/kvm/svm/svm.c +2 -0

index f923e14e87df..bb249ec625fc 100644
@@ -4536,6 +4536,8 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
  	.complete_emulated_msr = svm_complete_emulated_msr,
 
  	.vcpu_deliver_sipi_vector = svm_vcpu_deliver_sipi_vector,
+
+	.page_enc_status_hc = svm_page_enc_status_hc,
  };
 
  static struct kvm_x86_init_ops svm_init_ops __initdata = {

arch/x86/kvm/svm/svm.h +5 -0

index 0fe874ae5498..6437c1fa1f24 100644
@@ -79,6 +79,9 @@ struct kvm_sev_info {
  	unsigned long pages_locked; /* Number of pages locked */
  	struct list_head regions_list;  /* List of registered regions */
  	u64 ap_jump_table;	/* SEV-ES AP Jump Table address */
+	/* List and count of shared pages */
+	int shared_pages_list_count;
+	struct list_head shared_pages_list;
  };
 
  struct kvm_svm {

@@ -472,6 +475,8 @@ int nested_svm_check_exception(struct vcpu_svm *svm, unsigned nr,
  			       bool has_error_code, u32 error_code);
  int nested_svm_exit_special(struct vcpu_svm *svm);
  void sync_nested_vmcb_control(struct vcpu_svm *svm);
+int svm_page_enc_status_hc(struct kvm *kvm, unsigned long gpa,
+			   unsigned long npages, unsigned long enc);
 
  extern struct kvm_x86_nested_ops svm_nested_ops;
 

arch/x86/kvm/vmx/vmx.c +1 -0

index cc60b1fc3ee7..bcbf53851612 100644
@@ -7705,6 +7705,7 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
  	.can_emulate_instruction = vmx_can_emulate_instruction,
  	.apic_init_signal_blocked = vmx_apic_init_signal_blocked,
  	.migrate_timers = vmx_migrate_timers,
+	.page_enc_status_hc = NULL,
 
  	.msr_filter_changed = vmx_msr_filter_changed,
  	.complete_emulated_msr = kvm_complete_insn_gp,

arch/x86/kvm/x86.c +6 -0

index 76bce832cade..2f17f0f9ace7 100644
@@ -8162,6 +8162,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
  		kvm_sched_yield(vcpu->kvm, a0);
  		ret = 0;
  		break;
+	case KVM_HC_PAGE_ENC_STATUS:
+		ret = -KVM_ENOSYS;
+		if (kvm_x86_ops.page_enc_status_hc)
+			ret = kvm_x86_ops.page_enc_status_hc(vcpu->kvm,
+					a0, a1, a2);
+		break;
  	default:
  		ret = -KVM_ENOSYS;
  		break;

include/uapi/linux/kvm_para.h +1 -0

index 8b86609849b9..847b83b75dc8 100644
@@ -29,6 +29,7 @@
  #define KVM_HC_CLOCK_PAIRING		9
  #define KVM_HC_SEND_IPI		10
  #define KVM_HC_SCHED_YIELD		11
+#define KVM_HC_PAGE_ENC_STATUS		12
 
  /*
   * hypercalls use architecture specific

From: Brijesh Singh <[email protected]>

This hypercall is used by the SEV guest to notify a change in the page encryption status to the hypervisor. The hypercall should be invoked only when the encryption attribute is changed from encrypted -> decrypted and vice versa. By default all guest pages are considered encrypted.

The patch introduces a new shared pages list implemented as a sorted linked list to track the shared/unencrypted regions marked by the guest hypercall.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Brijesh Singh <[email protected]>

Co-developed-by Ashish Kalra <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 Documentation/virt/kvm/hypercalls.rst |  15 +++
 arch/x86/include/asm/kvm_host.h       |   2 +
 arch/x86/kvm/svm/sev.c                | 150 ++++++++++++++++++++++++++
 arch/x86/kvm/svm/svm.c                |   2 +
 arch/x86/kvm/svm/svm.h                |   5 +
 arch/x86/kvm/vmx/vmx.c                |   1 +
 arch/x86/kvm/x86.c                    |   6 ++
 include/uapi/linux/kvm_para.h         |   1 +
 8 files changed, 182 insertions(+)

Documentation/virt/kvm/hypercalls.rst +15 -0

index ed4fddd364ea..7aff0cebab7c 100644
@@ -169,3 +169,18 @@ a0: destination APIC ID
 
 :Usage example: When sending a call-function IPI-many to vCPUs, yield if
                any of the IPI target vCPUs was preempted.
+
+
+8. KVM_HC_PAGE_ENC_STATUS
+-------------------------
+:Architecture: x86
+:Status: active
+:Purpose: Notify the encryption status changes in guest page table (SEV guest)
+
+a0: the guest physical address of the start page
+a1: the number of pages
+a2: encryption attribute
+
+   Where:
+       * 1: Encryption attribute is set
+       * 0: Encryption attribute is cleared

arch/x86/include/asm/kvm_host.h +2 -0

index 3d6616f6f6ef..2da5f5e2a10e 100644
@@ -1301,6 +1301,8 @@ struct kvm_x86_ops {
        int (*complete_emulated_msr)(struct kvm_vcpu *vcpu, int err);
 
        void (*vcpu_deliver_sipi_vector)(struct kvm_vcpu *vcpu, u8 vector);
+       int (*page_enc_status_hc)(struct kvm *kvm, unsigned long gpa,
+                                 unsigned long sz, unsigned long mode);
 };
 
 struct kvm_x86_nested_ops {

arch/x86/kvm/svm/sev.c +31 -0

index 25eaf35ba51d..55c628df5155 100644
@@ -45,6 +45,11 @@ struct enc_region {
        unsigned long size;
 };
 
+struct shared_region {
+       struct list_head list;
+       unsigned long gfn_start, gfn_end;
+};
+
 static int sev_flush_asids(void)
 {
        int ret, error = 0;

@@ -196,6 +201,8 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
        sev->active = true;
        sev->asid = asid;
        INIT_LIST_HEAD(&sev->regions_list);
+       INIT_LIST_HEAD(&sev->shared_pages_list);
+       sev->shared_pages_list_count = 0;
 
        return 0;
 

@@ -1473,6 +1480,148 @@ static int sev_receive_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
        return ret;
 }
 
+static int remove_shared_region(unsigned long start, unsigned long end,
+                               struct list_head *head)
+{
+       struct shared_region *pos;
+
+       list_for_each_entry(pos, head, list) {
+               if (pos->gfn_start == start &&
+                   pos->gfn_end == end) {
+                       list_del(&pos->list);
+                       kfree(pos);
+                       return -1;
+               } else if (start >= pos->gfn_start && end <= pos->gfn_end) {
+                       if (start == pos->gfn_start)
+                               pos->gfn_start = end + 1;
+                       else if (end == pos->gfn_end)
+                               pos->gfn_end = start - 1;
+                       else {
+                               /* Do a de-merge -- split linked list nodes */
+                               unsigned long tmp;
+                               struct shared_region *shrd_region;
+
+                               tmp = pos->gfn_end;
+                               pos->gfn_end = start-1;
+                               shrd_region = kzalloc(sizeof(*shrd_region), GFP_KERNEL_ACCOUNT);

arch/x86/kvm/svm/sev.c +20 -0

+       return 0;
+}
+
+static int add_shared_region(unsigned long start, unsigned long end,
+                            struct list_head *shared_pages_list)
+{
+       struct list_head *head = shared_pages_list;
+       struct shared_region *shrd_region;
+       struct shared_region *pos;
+
+       if (list_empty(head)) {
+               shrd_region = kzalloc(sizeof(*shrd_region), GFP_KERNEL_ACCOUNT);
+               if (!shrd_region)
+                       return -ENOMEM;
+               shrd_region->gfn_start = start;
+               shrd_region->gfn_end = end;
+               list_add_tail(&shrd_region->list, head);
+               return 1;
+       }
+

arch/x86/kvm/svm/sev.c +10 -0

+                       return 0;
+               }
+               break;
+       }
 
+       /*
+        * Add a new node, allocate nodes using GFP_KERNEL_ACCOUNT so that
+        * kernel memory can be tracked/throttled in case a
+        * malicious guest makes infinite number of hypercalls to
+        * exhaust host kernel memory and cause a DOS attack.
+        */

Quoted 126 lines · Ashish Kalra

From: Brijesh Singh <[email protected]>

This hypercall is used by the SEV guest to notify a change in the page encryption status to the hypervisor. The hypercall should be invoked only when the encryption attribute is changed from encrypted -> decrypted and vice versa. By default all guest pages are considered encrypted.

The patch introduces a new shared pages list implemented as a sorted linked list to track the shared/unencrypted regions marked by the guest hypercall.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Brijesh Singh <[email protected]>

Co-developed-by Ashish Kalra <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 Documentation/virt/kvm/hypercalls.rst |  15 +++
 arch/x86/include/asm/kvm_host.h       |   2 +
 arch/x86/kvm/svm/sev.c                | 150 ++++++++++++++++++++++++++
 arch/x86/kvm/svm/svm.c                |   2 +
 arch/x86/kvm/svm/svm.h                |   5 +
 arch/x86/kvm/vmx/vmx.c                |   1 +
 arch/x86/kvm/x86.c                    |   6 ++
 include/uapi/linux/kvm_para.h         |   1 +
 8 files changed, 182 insertions(+)

Documentation/virt/kvm/hypercalls.rst +15 -0

index ed4fddd364ea..7aff0cebab7c 100644
@@ -169,3 +169,18 @@ a0: destination APIC ID
 
 :Usage example: When sending a call-function IPI-many to vCPUs, yield if
                any of the IPI target vCPUs was preempted.
+
+
+8. KVM_HC_PAGE_ENC_STATUS
+-------------------------
+:Architecture: x86
+:Status: active
+:Purpose: Notify the encryption status changes in guest page table (SEV guest)
+
+a0: the guest physical address of the start page
+a1: the number of pages
+a2: encryption attribute
+
+   Where:
+       * 1: Encryption attribute is set
+       * 0: Encryption attribute is cleared

arch/x86/include/asm/kvm_host.h +2 -0

index 3d6616f6f6ef..2da5f5e2a10e 100644
@@ -1301,6 +1301,8 @@ struct kvm_x86_ops {
        int (*complete_emulated_msr)(struct kvm_vcpu *vcpu, int err);
 
        void (*vcpu_deliver_sipi_vector)(struct kvm_vcpu *vcpu, u8 vector);
+       int (*page_enc_status_hc)(struct kvm *kvm, unsigned long gpa,
+                                 unsigned long sz, unsigned long mode);
 };
 
 struct kvm_x86_nested_ops {

arch/x86/kvm/svm/sev.c +31 -0

index 25eaf35ba51d..55c628df5155 100644
@@ -45,6 +45,11 @@ struct enc_region {
        unsigned long size;
 };
 
+struct shared_region {
+       struct list_head list;
+       unsigned long gfn_start, gfn_end;
+};
+
 static int sev_flush_asids(void)
 {
        int ret, error = 0;

@@ -196,6 +201,8 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
        sev->active = true;
        sev->asid = asid;
        INIT_LIST_HEAD(&sev->regions_list);
+       INIT_LIST_HEAD(&sev->shared_pages_list);
+       sev->shared_pages_list_count = 0;
 
        return 0;
 

@@ -1473,6 +1480,148 @@ static int sev_receive_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
        return ret;
 }
 
+static int remove_shared_region(unsigned long start, unsigned long end,
+                               struct list_head *head)
+{
+       struct shared_region *pos;
+
+       list_for_each_entry(pos, head, list) {
+               if (pos->gfn_start == start &&
+                   pos->gfn_end == end) {
+                       list_del(&pos->list);
+                       kfree(pos);
+                       return -1;
+               } else if (start >= pos->gfn_start && end <= pos->gfn_end) {
+                       if (start == pos->gfn_start)
+                               pos->gfn_start = end + 1;
+                       else if (end == pos->gfn_end)
+                               pos->gfn_end = start - 1;
+                       else {
+                               /* Do a de-merge -- split linked list nodes */
+                               unsigned long tmp;
+                               struct shared_region *shrd_region;
+
+                               tmp = pos->gfn_end;
+                               pos->gfn_end = start-1;
+                               shrd_region = kzalloc(sizeof(*shrd_region), GFP_KERNEL_ACCOUNT);

arch/x86/kvm/svm/sev.c static int sev_receive_finish(struct kvm *kvm, struct kvm_sev_cmd *argp) · line 1516

Quoted 10 lines

arch/x86/kvm/svm/sev.c +10 -0

+                               if (!shrd_region)
+                                       return -ENOMEM;
+                               shrd_region->gfn_start = end + 1;
+                               shrd_region->gfn_end = tmp;
+                               list_add(&shrd_region->list, &pos->list);
+                               return 1;
+                       }
+                       return 0;
+               }
+       }

This doesn't handle the case where the region being marked as encrypted is larger than than the unencrypted region under consideration, which (I believe) can happen with the current kexec handling (since it is oblivious to the prior state).
I would probably break this down into the "five cases": no intersection (skip), entry is completely contained (drop), entry completely contains the removed region (split), intersect start (chop), and intersect end (chop).

Quoted 28 lines

arch/x86/kvm/svm/sev.c +27 -0

+       return 0;
+}
+
+static int add_shared_region(unsigned long start, unsigned long end,
+                            struct list_head *shared_pages_list)
+{
+       struct list_head *head = shared_pages_list;
+       struct shared_region *shrd_region;
+       struct shared_region *pos;
+
+       if (list_empty(head)) { > > +               shrd_region = kzalloc(sizeof(*shrd_region), GFP_KERNEL_ACCOUNT);
+               if (!shrd_region)
+                       return -ENOMEM;
+               shrd_region->gfn_start = start;
+               shrd_region->gfn_end = end;
+               list_add_tail(&shrd_region->list, head);
+               return 1;
+       }
+
+       /*
+        * Shared pages list is a sorted list in ascending order of
+        * guest PA's and also merges consecutive range of guest PA's
+        */
+       list_for_each_entry(pos, head, list) {
+               if (pos->gfn_end < start)
+                       continue;
+               /* merge consecutive guest PA(s) */

arch/x86/kvm/svm/sev.c static int sev_receive_finish(struct kvm *kvm, struct kvm_sev_cmd *argp) · line 1546

Quoted 2 lines

arch/x86/kvm/svm/sev.c +2 -0

+               if (pos->gfn_start <= start && pos->gfn_end >= start) {
+                       pos->gfn_end = end;

I'm not sure this correctly avoids having duplicate overlapping elements in the list. It also doesn't merge consecutive contiguous regions. Current guest implementation should never call the hypercall with C=0 for the same region twice, without calling with c=1 in between, but this API should be compatible with that model.

The easiest pattern would probably be to:
1) find (or insert) the node that will contain the added region.

2) remove the contents of the added region from the tail (will typically do nothing).
3) merge the head of the tail into the current node, if the end of the current node matches the start of that head.

Quoted 12 lines

arch/x86/kvm/svm/sev.c +10 -0

+                       return 0;
+               }
+               break;
+       }
 
+       /*
+        * Add a new node, allocate nodes using GFP_KERNEL_ACCOUNT so that
+        * kernel memory can be tracked/throttled in case a
+        * malicious guest makes infinite number of hypercalls to
+        * exhaust host kernel memory and cause a DOS attack.
+        */

arch/x86/kvm/svm/sev.c static int sev_receive_finish(struct kvm *kvm, struct kvm_sev_cmd *argp) · line 1565

Quoted 10 lines

arch/x86/kvm/svm/sev.c +9 -0

+       shrd_region = kzalloc(sizeof(*shrd_region), GFP_KERNEL_ACCOUNT);
+       if (!shrd_region)
+               return -ENOMEM;
+       shrd_region->gfn_start = start;
+       shrd_region->gfn_end = end;
+       list_add_tail(&shrd_region->list, &pos->list);
+       return 1;
 
+}
+

Thanks!
Steve

From: Brijesh Singh <[email protected]>

The ioctl is used to retrieve a guest's shared pages list.

arch/x86/kvm/svm/sev.c +17 -0

+int svm_get_shared_pages_list(struct kvm *kvm,
+			      struct kvm_shared_pages_list *list)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct shared_region_array_entry *array;
+	struct shared_region *pos;
+	int ret, nents = 0;
+	unsigned long sz;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	if (!list->size)
+		return -EINVAL;
+
+	if (!sev->shared_pages_list_count)
+		return put_user(0, list->pnents);

arch/x86/kvm/svm/sev.c +17 -0

+	list_for_each_entry(pos, &sev->shared_pages_list, list) {
+		array[nents].gfn_start = pos->gfn_start;
+		array[nents++].gfn_end = pos->gfn_end;
+	}
+	mutex_unlock(&kvm->lock);
+
+	ret = -EFAULT;
+	if (copy_to_user(list->buffer, array, sz))
+		goto out;
+	if (put_user(nents, list->pnents))
+		goto out;
+	ret = 0;
+out:
+	kfree(array);
+	return ret;
+}
+

Quoted 4 lines · Ashish Kalra

From: Brijesh Singh <[email protected]>

The ioctl is used to retrieve a guest's shared pages list.

...

Quoted 23 lines

arch/x86/kvm/svm/sev.c +23 -0

+int svm_get_shared_pages_list(struct kvm *kvm,
+			      struct kvm_shared_pages_list *list)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct shared_region_array_entry *array;
+	struct shared_region *pos;
+	int ret, nents = 0;
+	unsigned long sz;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	if (!list->size)
+		return -EINVAL;
+
+	if (!sev->shared_pages_list_count)
+		return put_user(0, list->pnents);
+
+	sz = sev->shared_pages_list_count * sizeof(struct shared_region_array_entry);
+	if (sz > list->size)
+		return -E2BIG;
+
+	array = kmalloc(sz, GFP_KERNEL);

arch/x86/kvm/svm/sev.c int svm_page_enc_status_hc(struct kvm *kvm, unsigned long gpa, · line 1656

Quoted 4 lines

arch/x86/kvm/svm/sev.c +4 -0

+	if (!array)
+		return -ENOMEM;
+
+	mutex_lock(&kvm->lock);

I think this lock needs to be taken before the memory size is calculated. If the list is expanded after obtaining the size and before taking the lock, you will run off the end of the array.

Quoted 18 lines

arch/x86/kvm/svm/sev.c +17 -0

+	list_for_each_entry(pos, &sev->shared_pages_list, list) {
+		array[nents].gfn_start = pos->gfn_start;
+		array[nents++].gfn_end = pos->gfn_end;
+	}
+	mutex_unlock(&kvm->lock);
+
+	ret = -EFAULT;
+	if (copy_to_user(list->buffer, array, sz))
+		goto out;
+	if (put_user(nents, list->pnents))
+		goto out;
+	ret = 0;
+out:
+	kfree(array);
+	return ret;
+}
+

From: Brijesh Singh <[email protected]>

The ioctl is used to retrieve a guest's shared pages list.

From: Brijesh Singh <[email protected]>

The ioctl is used to retrieve a guest's shared pages list.

What's the performance hit to boot time if KVM_HC_PAGE_ENC_STATUS is passed through to userspace? That way, userspace could manage the set of pages >in whatever data structure they want, and these get/set ioctls go away.

Also, aren't there plans for an in-guest migration helper? If so, do we have any idea what that interface will look like? E.g. if we're going to end up with a full >fledged driver in the guest, why not bite the bullet now and bypass KVM entirely?

From: Sean Christopherson <[email protected]>

Quoted 7 lines · Ashish Kalra

From: Brijesh Singh <[email protected]>

The ioctl is used to retrieve a guest's shared pages list.

What's the performance hit to boot time if KVM_HC_PAGE_ENC_STATUS is passed through to userspace? That way, userspace could manage the set of pages >in whatever data structure they want, and these get/set ioctls go away.

What is the advantage of passing KVM_HC_PAGE_ENC_STATUS through to user-space ?

As such it is just a simple interface to get the shared page list via the get/set ioctl's. simply an array is passed to these ioctl to get/set the shared pages list.

Quoted 4 lines

Also, aren't there plans for an in-guest migration helper? If so, do we have any idea what that interface will look like? E.g. if we're going to end up with a full >fledged driver in the guest, why not bite the bullet now and bypass KVM entirely?

Even the in-guest migration helper will be using page encryption status hypercalls, so some interface is surely required.

Also the in-guest migration will be mainly an OVMF component, won't really be a full fledged kernel driver in the guest.

Quoted 10 lines

From: Sean Christopherson <[email protected]> On Thu, Feb 04, 2021,

Quoted 7 lines · Ashish Kalra

From: Brijesh Singh <[email protected]>

The ioctl is used to retrieve a guest's shared pages list.

What's the performance hit to boot time if KVM_HC_PAGE_ENC_STATUS is passed through to userspace? That way, userspace could manage the
set of pages >in whatever data structure they want, and these get/set ioctls go away.

Quoted 6 lines

What is the advantage of passing KVM_HC_PAGE_ENC_STATUS through to user-space ?

As such it is just a simple interface to get the shared page list via the get/set ioctl's. simply an array is passed to these ioctl to get/set the shared pages list.

It eliminates any probability of the kernel choosing the wrong data structure, and it's two fewer ioctls to maintain and test.

Quoted 4 lines

Also, aren't there plans for an in-guest migration helper? If so, do we have any idea what that interface will look like? E.g. if we're going to end up with a full >fledged driver in the guest, why not bite the bullet now and bypass KVM entirely?

Even the in-guest migration helper will be using page encryption status hypercalls, so some interface is surely required.

If it's a driver with a more extensive interace, then the hypercalls can be replaced by a driver operation. That's obviously a big if, though.

Also the in-guest migration will be mainly an OVMF component, won't really be a full fledged kernel driver in the guest.

Is there code and/or a description of what the proposed helper would look like?

From: Sean Christopherson <[email protected]>

Quoted 16 lines · Ashish

Quoted 16 lines

From: Sean Christopherson <[email protected]> On Thu, Feb 04, 2021,

Quoted 7 lines · Ashish Kalra

From: Brijesh Singh <[email protected]>

The ioctl is used to retrieve a guest's shared pages list.

What's the performance hit to boot time if KVM_HC_PAGE_ENC_STATUS is passed through to userspace? That way, userspace could manage the
set of pages >in whatever data structure they want, and these get/set ioctls go away.

What is the advantage of passing KVM_HC_PAGE_ENC_STATUS through to user-space ?

As such it is just a simple interface to get the shared page list via the get/set ioctl's. simply an array is passed to these ioctl to get/set the shared pages list.

Quoted 2 lines

It eliminates any probability of the kernel choosing the wrong data structure, and it's two fewer ioctls to maintain and test.

The set shared pages list ioctl cannot be avoided as it needs to be issued to setup the shared pages list on the migrated VM, it cannot be achieved by passing KVM_HC_PAGE_ENC_STATUS through to user-space.

So it makes sense to add both get/set shared pages list ioctl, passing through to user-space is just adding more complexity without any significant gains.

From: Sean Christopherson <[email protected]>

Quoted 17 lines · Ashish

Quoted 16 lines

From: Sean Christopherson <[email protected]> On Thu, Feb 04, 2021,

Quoted 7 lines · Ashish Kalra

From: Brijesh Singh <[email protected]>

The ioctl is used to retrieve a guest's shared pages list.

What's the performance hit to boot time if KVM_HC_PAGE_ENC_STATUS
is passed through to userspace? That way, userspace could manage
the set of pages >in whatever data structure they want, and these get/set ioctls go away.

What is the advantage of passing KVM_HC_PAGE_ENC_STATUS through to user-space ?

As such it is just a simple interface to get the shared page list via the get/set ioctl's. simply an array is passed to these ioctl to get/set the shared pages list.

Quoted 6 lines

Quoted 2 lines

It eliminates any probability of the kernel choosing the wrong data structure, and it's two fewer ioctls to maintain and test.

The set shared pages list ioctl cannot be avoided as it needs to be issued to setup the shared pages list on the migrated VM, it cannot be achieved by passing KVM_HC_PAGE_ENC_STATUS through to user-space.

Why's that? AIUI, KVM doesn't do anything with the list other than pass it back to userspace. Assuming that's the case, userspace can just hold onto the list >for the next migration.

From: Sean Christopherson <[email protected]>

Quoted 26 lines · Ashish

From: Sean Christopherson <[email protected]>

Quoted 23 lines · Ashish

Quoted 13 lines

From: Sean Christopherson <[email protected]> On Thu, Feb 04, 2021,

Quoted 7 lines · Ashish Kalra

From: Brijesh Singh <[email protected]>

The ioctl is used to retrieve a guest's shared pages list.

What's the performance hit to boot time if KVM_HC_PAGE_ENC_STATUS
is passed through to userspace? That way, userspace could manage
the set of pages >in whatever data structure they want, and these get/set ioctls go away.

What is the advantage of passing KVM_HC_PAGE_ENC_STATUS through to user-space ?

Quoted 6 lines

As such it is just a simple interface to get the shared page list via the get/set ioctl's. simply an array is passed to these ioctl to get/set the shared pages list.

It eliminates any probability of the kernel choosing the wrong data structure, and it's two fewer ioctls to maintain and test.

The set shared pages list ioctl cannot be avoided as it needs to be issued to setup the shared pages list on the migrated VM, it cannot be achieved by passing KVM_HC_PAGE_ENC_STATUS through to user-space.

Quoted 3 lines

Why's that? AIUI, KVM doesn't do anything with the list other than pass it back to userspace. Assuming that's the case, userspace can just hold onto the list >for the next migration.

KVM does use it as part of the SEV DBG_DECTYPT API, within sev_dbg_decrypt() to check if the guest page(s) are encrypted or not, and accordingly use it to decide whether to decrypt the guest page(s) and return that back to user-space or just return it as it is.

From: Brijesh Singh <[email protected]>

The ioctl is used to retrieve a guest's shared pages list.

What's the performance hit to boot time if KVM_HC_PAGE_ENC_STATUS is passed through to userspace? That way, userspace could manage the set of pages >in whatever data structure they want, and these get/set ioctls go away.

[AMD Public Use]

-----Original Message-----
From: Sean Christopherson <[email protected]>
Sent: Tuesday, February 16, 2021 7:03 PM
To: Kalra, Ashish <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Lendacky, Thomas <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Singh, Brijesh <[email protected]>
Subject: Re: [PATCH v10 10/16] KVM: x86: Introduce KVM_GET_SHARED_PAGES_LIST ioctl

Quoted 2 lines · Ashish Kalra

From: Brijesh Singh <[email protected]>

Quoted 3 lines

The ioctl is used to retrieve a guest's shared pages list.

What's the performance hit to boot time if KVM_HC_PAGE_ENC_STATUS is passed through to userspace? That way, userspace could manage the set of pages >in whatever data structure they want, and these get/set ioctls go away.

I will be more concerned about performance hit during guest DMA I/O if the page encryption status hypercalls are passed through to user-space, a lot of guest DMA I/O dynamically sets up pages for encryption and then flips them at DMA completion, so guest I/O will surely take a performance hit with this pass-through stuff.

# Samples: 19K of event 'kvm:kvm_hypercall'
# Event count (approx.): 19573
#

# Overhead  Command          Shared Object     Symbol
# ........  ...............  ................  .........................

#
100.00% qemu-system-x86 [kernel.vmlinux] [k] kvm_emulate_hypercall

Out of these 19573 hypercalls, # of page encryption status hcalls are 19479, so almost all hypercalls here are page encryption status hypercalls.

The above data indicates that there will be ~2% more Heavyweight VMEXITs during SEV guest boot if we do page encryption status hypercalls pass-through to host userspace.

But, then Brijesh pointed out to me and highlighted that currently
OVMF is doing lot of VMEXITs because they don't use the DMA pool to minimize the C-bit toggles, in other words, OVMF bounce buffer does page state change on every DMA allocate and free.

So here is the performance analysis after kernel and initrd have been loaded into memory using grub and then starting perf just before booting the kernel.

These are the performance #'s after kernel and initrd have been loaded into memory, then perf is attached and kernel is booted :

# Samples: 1M of event 'kvm:kvm_userspace_exit'
# Event count (approx.): 1081235
#
# Overhead Trace output

# ........  ........................
#
    99.77%  reason KVM_EXIT_IO (2)
     0.23%  reason KVM_EXIT_MMIO (6)

# Samples: 1K of event 'kvm:kvm_hypercall'
# Event count (approx.): 1279
#

So as the above data indicates, Linux is only making ~1K hypercalls, compared to ~18K hypercalls made by OVMF in the above use case.

Does the above adds a prerequisite that OVMF needs to be optimized if and before hypercall pass-through can be done ?

Quoted 10 lines · Ashish Kalra

# Samples: 19K of event 'kvm:kvm_hypercall'
# Event count (approx.): 19573
#

# Overhead  Command          Shared Object     Symbol
# ........  ...............  ................  .........................

#
100.00% qemu-system-x86 [kernel.vmlinux] [k] kvm_emulate_hypercall

Out of these 19573 hypercalls, # of page encryption status hcalls are 19479, so almost all hypercalls here are page encryption status hypercalls.

Oof.

Quoted 27 lines

The above data indicates that there will be ~2% more Heavyweight VMEXITs during SEV guest boot if we do page encryption status hypercalls pass-through to host userspace.

But, then Brijesh pointed out to me and highlighted that currently
OVMF is doing lot of VMEXITs because they don't use the DMA pool to minimize the C-bit toggles, in other words, OVMF bounce buffer does page state change on every DMA allocate and free.

So here is the performance analysis after kernel and initrd have been loaded into memory using grub and then starting perf just before booting the kernel.

These are the performance #'s after kernel and initrd have been loaded into memory, then perf is attached and kernel is booted :

# Samples: 1M of event 'kvm:kvm_userspace_exit'
# Event count (approx.): 1081235
#
# Overhead Trace output

# ........  ........................
#
    99.77%  reason KVM_EXIT_IO (2)
     0.23%  reason KVM_EXIT_MMIO (6)

# Samples: 1K of event 'kvm:kvm_hypercall'
# Event count (approx.): 1279
#

Quoted 5 lines

So as the above data indicates, Linux is only making ~1K hypercalls, compared to ~18K hypercalls made by OVMF in the above use case.

Does the above adds a prerequisite that OVMF needs to be optimized if and before hypercall pass-through can be done ?

Disclaimer: my math could be totally wrong.

I doubt it's a hard requirement. Assuming a conversative roundtrip time of 50k cycles, those 18K hypercalls will add well under a 1/2 a second of boot time. If userspace can push the roundtrip time down to 10k cycles, the overhead is more like 50 milliseconds.

That being said, this does seem like a good OVMF cleanup, irrespective of this new hypercall. I assume it's not cheap to convert a page between encrypted and decrypted.

Thanks much for getting the numbers!

Quoted 54 lines · Sean Christopherson

Quoted 10 lines · Ashish Kalra

# Samples: 19K of event 'kvm:kvm_hypercall'
# Event count (approx.): 19573
#

# Overhead  Command          Shared Object     Symbol
# ........  ...............  ................  .........................

#
100.00% qemu-system-x86 [kernel.vmlinux] [k] kvm_emulate_hypercall

Out of these 19573 hypercalls, # of page encryption status hcalls are 19479, so almost all hypercalls here are page encryption status hypercalls.

Oof.

Quoted 27 lines

The above data indicates that there will be ~2% more Heavyweight VMEXITs during SEV guest boot if we do page encryption status hypercalls pass-through to host userspace.

But, then Brijesh pointed out to me and highlighted that currently
OVMF is doing lot of VMEXITs because they don't use the DMA pool to minimize the C-bit toggles, in other words, OVMF bounce buffer does page state change on every DMA allocate and free.

So here is the performance analysis after kernel and initrd have been loaded into memory using grub and then starting perf just before booting the kernel.

These are the performance #'s after kernel and initrd have been loaded into memory, then perf is attached and kernel is booted :

# Samples: 1M of event 'kvm:kvm_userspace_exit'
# Event count (approx.): 1081235
#
# Overhead Trace output

# ........  ........................
#
    99.77%  reason KVM_EXIT_IO (2)
     0.23%  reason KVM_EXIT_MMIO (6)

# Samples: 1K of event 'kvm:kvm_hypercall'
# Event count (approx.): 1279
#

Quoted 5 lines

So as the above data indicates, Linux is only making ~1K hypercalls, compared to ~18K hypercalls made by OVMF in the above use case.

Does the above adds a prerequisite that OVMF needs to be optimized if and before hypercall pass-through can be done ?

Disclaimer: my math could be totally wrong.

I doubt it's a hard requirement. Assuming a conversative roundtrip time of 50k cycles, those 18K hypercalls will add well under a 1/2 a second of boot time. If userspace can push the roundtrip time down to 10k cycles, the overhead is more like 50 milliseconds.

Quoted 5 lines

That being said, this does seem like a good OVMF cleanup, irrespective of this new hypercall. I assume it's not cheap to convert a page between encrypted and decrypted.

Thanks much for getting the numbers!

Considering the above data and guest boot time latencies (and potential issues with OVMF and optimizations required there), do we have any consensus on whether we want to do page encryption status hypercall passthrough or not ?

Thanks,
Ashish

Quoted 62 lines · Ashish Kalra

Quoted 54 lines · Sean Christopherson

Quoted 10 lines · Ashish Kalra

# Samples: 19K of event 'kvm:kvm_hypercall'
# Event count (approx.): 19573
#

# Overhead  Command          Shared Object     Symbol
# ........  ...............  ................  .........................

#
100.00% qemu-system-x86 [kernel.vmlinux] [k] kvm_emulate_hypercall

Out of these 19573 hypercalls, # of page encryption status hcalls are 19479, so almost all hypercalls here are page encryption status hypercalls.

Oof.

Quoted 27 lines

The above data indicates that there will be ~2% more Heavyweight VMEXITs during SEV guest boot if we do page encryption status hypercalls pass-through to host userspace.

But, then Brijesh pointed out to me and highlighted that currently
OVMF is doing lot of VMEXITs because they don't use the DMA pool to minimize the C-bit toggles, in other words, OVMF bounce buffer does page state change on every DMA allocate and free.

So here is the performance analysis after kernel and initrd have been loaded into memory using grub and then starting perf just before booting the kernel.

These are the performance #'s after kernel and initrd have been loaded into memory, then perf is attached and kernel is booted :

# Samples: 1M of event 'kvm:kvm_userspace_exit'
# Event count (approx.): 1081235
#
# Overhead Trace output

# ........  ........................
#
    99.77%  reason KVM_EXIT_IO (2)
     0.23%  reason KVM_EXIT_MMIO (6)

# Samples: 1K of event 'kvm:kvm_hypercall'
# Event count (approx.): 1279
#

Quoted 5 lines

So as the above data indicates, Linux is only making ~1K hypercalls, compared to ~18K hypercalls made by OVMF in the above use case.

Does the above adds a prerequisite that OVMF needs to be optimized if and before hypercall pass-through can be done ?

Disclaimer: my math could be totally wrong.

I doubt it's a hard requirement. Assuming a conversative roundtrip time of 50k cycles, those 18K hypercalls will add well under a 1/2 a second of boot time. If userspace can push the roundtrip time down to 10k cycles, the overhead is more like 50 milliseconds.

Quoted 5 lines

That being said, this does seem like a good OVMF cleanup, irrespective of this new hypercall. I assume it's not cheap to convert a page between encrypted and decrypted.

Thanks much for getting the numbers!

Quoted 7 lines

Considering the above data and guest boot time latencies (and potential issues with OVMF and optimizations required there), do we have any consensus on whether we want to do page encryption status hypercall passthrough or not ?

Thanks,
Ashish

Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall. Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

Essentially, I think you could drop most of the host kernel work if there were generic support for hypercall exiting. Then userspace would be responsible for all of that. Thoughts on this?

--Steve

Quoted 62 lines · Ashish Kalra

Quoted 54 lines · Sean Christopherson

Quoted 10 lines · Ashish Kalra

# Samples: 19K of event 'kvm:kvm_hypercall'
# Event count (approx.): 19573
#

# Overhead  Command          Shared Object     Symbol
# ........  ...............  ................  .........................

#
100.00% qemu-system-x86 [kernel.vmlinux] [k] kvm_emulate_hypercall

Out of these 19573 hypercalls, # of page encryption status hcalls are 19479, so almost all hypercalls here are page encryption status hypercalls.

Oof.

Quoted 27 lines

The above data indicates that there will be ~2% more Heavyweight VMEXITs during SEV guest boot if we do page encryption status hypercalls pass-through to host userspace.

But, then Brijesh pointed out to me and highlighted that currently
OVMF is doing lot of VMEXITs because they don't use the DMA pool to minimize the C-bit toggles, in other words, OVMF bounce buffer does page state change on every DMA allocate and free.

So here is the performance analysis after kernel and initrd have been loaded into memory using grub and then starting perf just before booting the kernel.

These are the performance #'s after kernel and initrd have been loaded into memory, then perf is attached and kernel is booted :

# Samples: 1M of event 'kvm:kvm_userspace_exit'
# Event count (approx.): 1081235
#
# Overhead Trace output

# ........  ........................
#
    99.77%  reason KVM_EXIT_IO (2)
     0.23%  reason KVM_EXIT_MMIO (6)

# Samples: 1K of event 'kvm:kvm_hypercall'
# Event count (approx.): 1279
#

Quoted 5 lines

So as the above data indicates, Linux is only making ~1K hypercalls, compared to ~18K hypercalls made by OVMF in the above use case.

Does the above adds a prerequisite that OVMF needs to be optimized if and before hypercall pass-through can be done ?

Disclaimer: my math could be totally wrong.

I doubt it's a hard requirement. Assuming a conversative roundtrip time of 50k cycles, those 18K hypercalls will add well under a 1/2 a second of boot time. If userspace can push the roundtrip time down to 10k cycles, the overhead is more like 50 milliseconds.

Quoted 5 lines

That being said, this does seem like a good OVMF cleanup, irrespective of this new hypercall. I assume it's not cheap to convert a page between encrypted and decrypted.

Thanks much for getting the numbers!

Quoted 7 lines

Considering the above data and guest boot time latencies (and potential issues with OVMF and optimizations required there), do we have any consensus on whether we want to do page encryption status hypercall passthrough or not ?

Thanks,
Ashish

Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall. Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

Essentially, I think you could drop most of the host kernel work if there were generic support for hypercall exiting. Then userspace would be responsible for all of that. Thoughts on this?

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

Quoted 3 lines

Essentially, I think you could drop most of the host kernel work if there were generic support for hypercall exiting. Then userspace would be responsible for all of that. Thoughts on this?

So if i understand it correctly, i will submitting v11 of this patch-set with in-kernel support for page encryption status hypercalls and shared pages list and the userspace control of SEV live migration feature support and fixes for MSR handling.

In subsequent follow-up patches we will add generic support for hypercall exiting and then drop kvm side of hypercall and also add userspace support for MSR handling.

Thanks,
Ashish

+Will and Quentin (arm64)

Moving the non-KVM x86 folks to bcc, I don't they care about KVM details at this point.

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

	/* KVM_EXIT_HYPERCALL */
	struct {
		__u64 nr;
		__u64 args[6];
		__u64 ret;
		__u32 longmode;
		__u32 pad;
	} hypercall;

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Quoted 55 lines · Sean Christopherson

+Will and Quentin (arm64)

Moving the non-KVM x86 folks to bcc, I don't they care about KVM details at this point.

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

	/* KVM_EXIT_HYPERCALL */
	struct {
		__u64 nr;
		__u64 args[6];
		__u64 ret;
		__u32 longmode;
		__u32 pad;
	} hypercall;

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

Quoted 5 lines

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Looking at arm64's pKVM work, i see that it is a recently introduced RFC patch-set and probably relevant to arm64 nVHE hypervisor mode/implementation, and potentially makes sense as it adds guest memory protection as both host and guest kernels are running on the same privilege level ?

Though i do see that the pKVM stuff adds two hypercalls, specifically :

pkvm_create_mappings() ( I assume this is for setting shared memory regions between host and guest) & pkvm_create_private_mappings().

And the use-cases are quite similar to memory protection architectues use cases, for example, use with virtio devices, guest DMA I/O, etc.

But, isn't this patch set still RFC, and though i agree that it adds an infrastructure for standardised communication between the host and it's guests for mutually controlled shared memory regions and surely adds some kind of portability between hypervisor implementations, but nothing is standardised still, right ?

Quoted 50 lines · Sean Christopherson

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

	/* KVM_EXIT_HYPERCALL */
	struct {
		__u64 nr;
		__u64 args[6];
		__u64 ret;
		__u32 longmode;
		__u32 pad;
	} hypercall;

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

Quoted 5 lines

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Looking at arm64's pKVM work, i see that it is a recently introduced RFC patch-set and probably relevant to arm64 nVHE hypervisor mode/implementation, and potentially makes sense as it adds guest memory protection as both host and guest kernels are running on the same privilege level ?

Though i do see that the pKVM stuff adds two hypercalls, specifically :

pkvm_create_mappings() ( I assume this is for setting shared memory regions between host and guest) & pkvm_create_private_mappings().

And the use-cases are quite similar to memory protection architectues use cases, for example, use with virtio devices, guest DMA I/O, etc.

But, isn't this patch set still RFC, and though i agree that it adds an infrastructure for standardised communication between the host and it's guests for mutually controlled shared memory regions and surely adds some kind of portability between hypervisor implementations, but nothing is standardised still, right ?

[+Marc]

Quoted 69 lines · Ashish Kalra

Quoted 50 lines · Sean Christopherson

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

	/* KVM_EXIT_HYPERCALL */
	struct {
		__u64 nr;
		__u64 args[6];
		__u64 ret;
		__u32 longmode;
		__u32 pad;
	} hypercall;

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

Quoted 5 lines

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Looking at arm64's pKVM work, i see that it is a recently introduced RFC patch-set and probably relevant to arm64 nVHE hypervisor mode/implementation, and potentially makes sense as it adds guest memory protection as both host and guest kernels are running on the same privilege level ?

Though i do see that the pKVM stuff adds two hypercalls, specifically :

pkvm_create_mappings() ( I assume this is for setting shared memory regions between host and guest) & pkvm_create_private_mappings().

Quoted 2 lines

And the use-cases are quite similar to memory protection architectues use cases, for example, use with virtio devices, guest DMA I/O, etc.

These hypercalls are both private to the host kernel communicating with its hypervisor counterpart, so I don't think they're particularly relevant here.

As far as I can see, the more useful thing is to allow
the guest to communicate back to the host (and the VMM) that it has opened up a memory window, perhaps for virtio rings or some other shared memory.

We hacked this up as a prototype in the past:

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fandroid-kvm.googlesource.com%2Flinux%2F%2B%2Fd12a9e2c12a52cf7140d40cd9fa092dc8a85fac9%255E%2521%2F&amp;data=04%7C01%7Cashish.kalra%40amd.com%7C7ae6bbd9fa6442f9edcc08d8de75d14b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637503944913839841%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=Juon5nJ7BB6moTWYssRXOWrDOrYfZLmA%2BLrz3s12Ook%3D&amp;reserved=0

but that's all arm64-specific and if we're solving the same problem as you, then let's avoid arch-specific stuff if possible. The way in which the guest discovers the interface will be arch-specific (we already have a mechanism for that and some hypercalls are already allocated by specs from Arm), but the interface back to the VMM and some (most?) of the host handling could be shared.

Quoted 5 lines

But, isn't this patch set still RFC, and though i agree that it adds an infrastructure for standardised communication between the host and it's guests for mutually controlled shared memory regions and surely adds some kind of portability between hypervisor implementations, but nothing is standardised still, right ?

No, and it seems that you're further ahead than us in terms of implementation in this area. We're happy to review patches though, to make sure we end up with something that works for us both.

[+Marc]

Quoted 43 lines · Ashish Kalra

Quoted 42 lines · Sean Christopherson

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

	/* KVM_EXIT_HYPERCALL */
	struct {
		__u64 nr;
		__u64 args[6];
		__u64 ret;
		__u32 longmode;
		__u32 pad;
	} hypercall;

Quoted 20 lines

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

Quoted 5 lines

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Looking at arm64's pKVM work, i see that it is a recently introduced RFC patch-set and probably relevant to arm64 nVHE hypervisor mode/implementation, and potentially makes sense as it adds guest memory protection as both host and guest kernels are running on the same privilege level ?

Quoted 8 lines

Though i do see that the pKVM stuff adds two hypercalls, specifically :

pkvm_create_mappings() ( I assume this is for setting shared memory regions between host and guest) & pkvm_create_private_mappings().

And the use-cases are quite similar to memory protection architectues use cases, for example, use with virtio devices, guest DMA I/O, etc.

These hypercalls are both private to the host kernel communicating with its hypervisor counterpart, so I don't think they're particularly relevant here. As far as I can see, the more useful thing is to allow the guest to communicate back to the host (and the VMM) that it has opened up a memory window, perhaps for virtio rings or some other shared memory.

We hacked this up as a prototype in the past:

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fandroid-kvm.googlesource.com%2Flinux%2F%2B%2Fd12a9e2c12a52cf7140d40cd9fa092dc8a85fac9%255E%2521%2F&amp;data=04%7C01%7Cashish.kalra%40amd.com%7C7ae6bbd9fa6442f9edcc08d8de75d14b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637503944913839841%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=Juon5nJ7BB6moTWYssRXOWrDOrYfZLmA%2BLrz3s12Ook%3D&amp;reserved=0

but that's all arm64-specific and if we're solving the same problem as you, then let's avoid arch-specific stuff if possible. The way in which the guest discovers the interface will be arch-specific (we already have a mechanism for that and some hypercalls are already allocated by specs from Arm), but the interface back to the VMM and some (most?) of the host handling could be shared.

Quoted 5 lines

But, isn't this patch set still RFC, and though i agree that it adds an infrastructure for standardised communication between the host and it's guests for mutually controlled shared memory regions and surely adds some kind of portability between hypervisor implementations, but nothing is standardised still, right ?

No, and it seems that you're further ahead than us in terms of implementation in this area. We're happy to review patches though, to make sure we end up with something that works for us both.

Will

[+Marc]

Quoted 63 lines · Ashish Kalra

Quoted 50 lines · Sean Christopherson

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

	/* KVM_EXIT_HYPERCALL */
	struct {
		__u64 nr;
		__u64 args[6];
		__u64 ret;
		__u32 longmode;
		__u32 pad;
	} hypercall;

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

Quoted 5 lines

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Looking at arm64's pKVM work, i see that it is a recently introduced RFC patch-set and probably relevant to arm64 nVHE hypervisor mode/implementation, and potentially makes sense as it adds guest memory protection as both host and guest kernels are running on the same privilege level ?

Quoted 8 lines

Though i do see that the pKVM stuff adds two hypercalls, specifically :

pkvm_create_mappings() ( I assume this is for setting shared memory regions between host and guest) & pkvm_create_private_mappings().

And the use-cases are quite similar to memory protection architectues use cases, for example, use with virtio devices, guest DMA I/O, etc.

These hypercalls are both private to the host kernel communicating with its hypervisor counterpart, so I don't think they're particularly relevant here. As far as I can see, the more useful thing is to allow the guest to communicate back to the host (and the VMM) that it has opened up a memory window, perhaps for virtio rings or some other shared memory.

We hacked this up as a prototype in the past:

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fandroid-kvm.googlesource.com%2Flinux%2F%2B%2Fd12a9e2c12a52cf7140d40cd9fa092dc8a85fac9%255E%2521%2F&amp;data=04%7C01%7Cashish.kalra%40amd.com%7C7ae6bbd9fa6442f9edcc08d8de75d14b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637503944913839841%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=Juon5nJ7BB6moTWYssRXOWrDOrYfZLmA%2BLrz3s12Ook%3D&amp;reserved=0

but that's all arm64-specific and if we're solving the same problem as you, then let's avoid arch-specific stuff if possible. The way in which the guest discovers the interface will be arch-specific (we already have a mechanism for that and some hypercalls are already allocated by specs from Arm), but the interface back to the VMM and some (most?) of the host handling could be shared.

Quoted 5 lines

But, isn't this patch set still RFC, and though i agree that it adds an infrastructure for standardised communication between the host and it's guests for mutually controlled shared memory regions and surely adds some kind of portability between hypervisor implementations, but nothing is standardised still, right ?

No, and it seems that you're further ahead than us in terms of implementation in this area. We're happy to review patches though, to make sure we end up with something that works for us both.

Will

Quoted 83 lines · Will Deacon

[+Marc]

Quoted 63 lines · Ashish Kalra

Quoted 50 lines · Sean Christopherson

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

  /* KVM_EXIT_HYPERCALL */
  struct {
          __u64 nr;
          __u64 args[6];
          __u64 ret;
          __u32 longmode;
          __u32 pad;
  } hypercall;

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

Quoted 5 lines

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Looking at arm64's pKVM work, i see that it is a recently introduced RFC patch-set and probably relevant to arm64 nVHE hypervisor mode/implementation, and potentially makes sense as it adds guest memory protection as both host and guest kernels are running on the same privilege level ?

Quoted 8 lines

Though i do see that the pKVM stuff adds two hypercalls, specifically :

pkvm_create_mappings() ( I assume this is for setting shared memory regions between host and guest) & pkvm_create_private_mappings().

And the use-cases are quite similar to memory protection architectues use cases, for example, use with virtio devices, guest DMA I/O, etc.

These hypercalls are both private to the host kernel communicating with its hypervisor counterpart, so I don't think they're particularly relevant here. As far as I can see, the more useful thing is to allow the guest to communicate back to the host (and the VMM) that it has opened up a memory window, perhaps for virtio rings or some other shared memory.

We hacked this up as a prototype in the past:

Quoted 9 lines

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fandroid-kvm.googlesource.com%2Flinux%2F%2B%2Fd12a9e2c12a52cf7140d40cd9fa092dc8a85fac9%255E%2521%2F&amp;data=04%7C01%7Cashish.kalra%40amd.com%7C7ae6bbd9fa6442f9edcc08d8de75d14b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637503944913839841%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=Juon5nJ7BB6moTWYssRXOWrDOrYfZLmA%2BLrz3s12Ook%3D&amp;reserved=0

but that's all arm64-specific and if we're solving the same problem as you, then let's avoid arch-specific stuff if possible. The way in which the guest discovers the interface will be arch-specific (we already have a mechanism for that and some hypercalls are already allocated by specs from Arm), but the interface back to the VMM and some (most?) of the host handling could be shared.

I have started implementing a similar "hypercall to userspace" functionality for these DMA_SHARE/DMA_UNSHARE type of interfaces corresponding to SEV guest's add/remove shared regions on the x86 platform.

This does not implement a generic hypercall exiting infrastructure, mainly extends the KVM hypercall support to return back to userspace specifically for add/remove shared region hypercalls and then re-uses the complete userspace I/O callback functionality to resume the guest after returning back from userspace handling of the hypercall.

Looking fwd. to any comments/feedback/thoughts on the above.

Quoted 49 lines · Ashish Kalra

Quoted 47 lines · Will Deacon

[+Marc]

Quoted 44 lines · Ashish Kalra

Quoted 43 lines · Sean Christopherson

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

  /* KVM_EXIT_HYPERCALL */
  struct {
          __u64 nr;
          __u64 args[6];
          __u64 ret;
          __u32 longmode;
          __u32 pad;
  } hypercall;

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

Quoted 43 lines

Quoted 29 lines

Quoted 12 lines

Quoted 5 lines

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Looking at arm64's pKVM work, i see that it is a recently introduced RFC patch-set and probably relevant to arm64 nVHE hypervisor mode/implementation, and potentially makes sense as it adds guest memory protection as both host and guest kernels are running on the same privilege level ?

Quoted 8 lines

Though i do see that the pKVM stuff adds two hypercalls, specifically :

pkvm_create_mappings() ( I assume this is for setting shared memory regions between host and guest) & pkvm_create_private_mappings().

And the use-cases are quite similar to memory protection architectues use cases, for example, use with virtio devices, guest DMA I/O, etc.

These hypercalls are both private to the host kernel communicating with its hypervisor counterpart, so I don't think they're particularly relevant here. As far as I can see, the more useful thing is to allow the guest to communicate back to the host (and the VMM) that it has opened up a memory window, perhaps for virtio rings or some other shared memory.

We hacked this up as a prototype in the past:

Quoted 9 lines

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fandroid-kvm.googlesource.com%2Flinux%2F%2B%2Fd12a9e2c12a52cf7140d40cd9fa092dc8a85fac9%255E%2521%2F&amp;data=04%7C01%7Cashish.kalra%40amd.com%7C21099e2dd82c477005a008d8e4cf149e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637510925361923243%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=JputIEFoae3HRO5jvvn%2FTpP8i%2FpMMfWSiZ8a2%2Bna5iQ%3D&amp;reserved=0

but that's all arm64-specific and if we're solving the same problem as you, then let's avoid arch-specific stuff if possible. The way in which the guest discovers the interface will be arch-specific (we already have a mechanism for that and some hypercalls are already allocated by specs from Arm), but the interface back to the VMM and some (most?) of the host handling could be shared.

I have started implementing a similar "hypercall to userspace" functionality for these DMA_SHARE/DMA_UNSHARE type of interfaces corresponding to SEV guest's add/remove shared regions on the x86 platform.

Quoted 7 lines

This does not implement a generic hypercall exiting infrastructure, mainly extends the KVM hypercall support to return back to userspace specifically for add/remove shared region hypercalls and then re-uses the complete userspace I/O callback functionality to resume the guest after returning back from userspace handling of the hypercall.

Looking fwd. to any comments/feedback/thoughts on the above.

Others have mentioned a lack of appetite for generic hypercall intercepts, so this is the right approach.

Thanks,
Steve

Quoted 50 lines · Steve Rutherford

Quoted 49 lines · Ashish Kalra

Quoted 47 lines · Will Deacon

[+Marc]

Quoted 44 lines · Ashish Kalra

Quoted 43 lines · Sean Christopherson

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

  /* KVM_EXIT_HYPERCALL */
  struct {
          __u64 nr;
          __u64 args[6];
          __u64 ret;
          __u32 longmode;
          __u32 pad;
  } hypercall;

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

We may also need to pass-through the MSR to userspace, as it is a part of this complete host (userspace/kernel), OVMF and guest kernel negotiation of the SEV live migration feature.

Host (userspace/kernel) advertises it's support for SEV live migration feature via the CPUID bits, which is queried by OVMF and which in turn adds a new UEFI runtime variable to indicate support for SEV live migration, which is later queried during guest kernel boot and accordingly the guest does a wrmrsl() to custom MSR to complete SEV live migration negotiation and enable it.

Now, the GET_SHARED_REGION_LIST ioctl returns error, until this MSR write enables SEV live migration, hence, preventing userspace to start live migration before the feature support has been negotiated and enabled on all the three components - host, guest OVMF and kernel.

But, now with this ioctl not existing anymore, we will need to pass-through the MSR to userspace too, for it to only initiate live migration once the feature negotiation has been completed.

Quoted 76 lines · Ashish Kalra

Quoted 50 lines · Steve Rutherford

Quoted 49 lines · Ashish Kalra

Quoted 47 lines · Will Deacon

[+Marc]

Quoted 44 lines · Ashish Kalra

Quoted 43 lines · Sean Christopherson

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

  /* KVM_EXIT_HYPERCALL */
  struct {
          __u64 nr;
          __u64 args[6];
          __u64 ret;
          __u32 longmode;
          __u32 pad;
  } hypercall;

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

We may also need to pass-through the MSR to userspace, as it is a part of this complete host (userspace/kernel), OVMF and guest kernel negotiation of the SEV live migration feature.

Host (userspace/kernel) advertises it's support for SEV live migration feature via the CPUID bits, which is queried by OVMF and which in turn adds a new UEFI runtime variable to indicate support for SEV live migration, which is later queried during guest kernel boot and accordingly the guest does a wrmrsl() to custom MSR to complete SEV live migration negotiation and enable it.

Now, the GET_SHARED_REGION_LIST ioctl returns error, until this MSR write enables SEV live migration, hence, preventing userspace to start live migration before the feature support has been negotiated and enabled on all the three components - host, guest OVMF and kernel.

Quoted 3 lines

But, now with this ioctl not existing anymore, we will need to pass-through the MSR to userspace too, for it to only initiate live migration once the feature negotiation has been completed.

I can't tell if you were waiting for feedback on this before posting the follow-up patch series.

Here are a few options:
1) Add the MSR explicitly to the list of custom kvm MSRs, but don't have it hooked up anywhere. The expectation would be for the VMM to use msr intercepts to handle the reads and writes. If that seems weird, have svm_set_msr (or whatever) explicitly ignore it.
2) Add a getter and setter for the MSR. Only allow guests to use it if they are sev_guests with the requisite CPUID bit set.

I think I prefer the former, and it should work fine from my understanding of the msr intercepts implementation. I'm also open to other ideas. You could also have the MSR write trigger a KVM_EXIT of the same type as the hypercall, but have it just say "the msr value changed to XYZ", but that design sounds awkward.

Thanks,
Steve

+Will and Quentin (arm64)

Moving the non-KVM x86 folks to bcc, I don't they care about KVM details at this point.

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

	/* KVM_EXIT_HYPERCALL */
	struct {
		__u64 nr;
		__u64 args[6];
		__u64 ret;
		__u32 longmode;
		__u32 pad;
	} hypercall;

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Quoted 55 lines · Sean Christopherson

+Will and Quentin (arm64)

Moving the non-KVM x86 folks to bcc, I don't they care about KVM details at this point.

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

	/* KVM_EXIT_HYPERCALL */
	struct {
		__u64 nr;
		__u64 args[6];
		__u64 ret;
		__u32 longmode;
		__u32 pad;
	} hypercall;

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

Quoted 6 lines

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Potentially, there is another reason for in-kernel hypercall handling considering SEV-SNP. In case of SEV-SNP the RMP table tracks the state of each guest page, for instance pages in hypervisor state, i.e., pages with C=0 and pages in guest valid state with C=1.

Now, there shouldn't be a need for page encryption status hypercalls on SEV-SNP as KVM can track & reference guest page status directly using the RMP table.

As KVM maintains the RMP table, therefore we will need SET/GET type of interfaces to provide the guest page encryption status to userspace.

For the above reason if we do in-kernel hypercall handling for page encryption status (which we probably won't require for SEV-SNP & correspondingly there will be no hypercall exiting),

then we can implement a standard GET/SET ioctl interface to get/set the guest page encryption status for userspace, which will work across SEV, SEV-ES and SEV-SNP.

Quoted 63 lines · Ashish Kalra

Quoted 55 lines · Sean Christopherson

+Will and Quentin (arm64)

Moving the non-KVM x86 folks to bcc, I don't they care about KVM details at this point.

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

	/* KVM_EXIT_HYPERCALL */
	struct {
		__u64 nr;
		__u64 args[6];
		__u64 ret;
		__u32 longmode;
		__u32 pad;
	} hypercall;

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

Quoted 6 lines

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Quoted 8 lines

Potentially, there is another reason for in-kernel hypercall handling considering SEV-SNP. In case of SEV-SNP the RMP table tracks the state of each guest page, for instance pages in hypervisor state, i.e., pages with C=0 and pages in guest valid state with C=1.

Now, there shouldn't be a need for page encryption status hypercalls on SEV-SNP as KVM can track & reference guest page status directly using the RMP table.

Relying on the RMP table itself would require locking the RMP table for an extended duration, and walking the entire RMP to find shared pages would be very inefficient.

Quoted 2 lines

As KVM maintains the RMP table, therefore we will need SET/GET type of interfaces to provide the guest page encryption status to userspace.

Hrm, somehow I temporarily forgot about SNP and TDX adding their own hypercalls for converting between shared and private. And in the case of TDX, the hypercall can't be trusted, i.e. is just a hint, otherwise the guest could induce a #MC in the host.

Quoted 59 lines · Ashish Kalra

Quoted 52 lines · Sean Christopherson

+Will and Quentin (arm64)

Moving the non-KVM x86 folks to bcc, I don't they care about KVM details at this point.

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

	/* KVM_EXIT_HYPERCALL */
	struct {
		__u64 nr;
		__u64 args[6];
		__u64 ret;
		__u32 longmode;
		__u32 pad;
	} hypercall;

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

Quoted 6 lines

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Quoted 8 lines

Potentially, there is another reason for in-kernel hypercall handling considering SEV-SNP. In case of SEV-SNP the RMP table tracks the state of each guest page, for instance pages in hypervisor state, i.e., pages with C=0 and pages in guest valid state with C=1.

Now, there shouldn't be a need for page encryption status hypercalls on SEV-SNP as KVM can track & reference guest page status directly using the RMP table.

Relying on the RMP table itself would require locking the RMP table for an extended duration, and walking the entire RMP to find shared pages would be very inefficient.

Quoted 2 lines

As KVM maintains the RMP table, therefore we will need SET/GET type of interfaces to provide the guest page encryption status to userspace.

Hrm, somehow I temporarily forgot about SNP and TDX adding their own hypercalls for converting between shared and private. And in the case of TDX, the hypercall can't be trusted, i.e. is just a hint, otherwise the guest could induce a #MC in the host.

But, the different guest behavior doesn't require KVM to maintain a list/tree, e.g. adding a dedicated KVM_EXIT_* for notifying userspace of page encryption status changes would also suffice.

Actually, that made me think of another argument against maintaining a list in
KVM: there's no way to notify userspace that a page's status has changed. Userspace would need to query KVM to do GET_LIST after every GET_DIRTY. Obviously not a huge issue, but it does make migration slightly less efficient.

On a related topic, there are fatal race conditions that will require careful coordination between guest and host, and will effectively be wired into the ABI. SNP and TDX don't suffer these issues because host awareness of status is atomic with respect to the guest actually writing the page with the new encryption status.

For SEV live migration...

If the guest does the hypercall after writing the page, then the guest is hosed if it gets migrated while writing the page (scenario #1):

vCPU Userspace
zero_bytes[0:N]
<transfers written bytes as private instead of shared> <migrates vCPU>
zero_bytes[N+1:4095]
set_shared (dest)
kaboom!

If userspace does GET_DIRTY after GET_LIST, then the host would transfer bad data by consuming a stale list (scenario #2):

vCPU Userspace
get_list (from KVM or internally)
set_shared (src)
zero_page (src)
get_dirty
<transfers private data instead of shared> <migrates vCPU>
kaboom!

If both guest and host order things to avoid #1 and #2, the host can still migrate the wrong data (scenario #3):

vCPU Userspace
set_private
zero_bytes[0:4096]
get_dirty
set_shared (src)
get_list
<transfers as shared instead of private> <migrates vCPU>
set_private (dest)
kaboom!

Scenario #3 is unlikely, but plausible, e.g. if the guest bails from its conversion flow for whatever reason, after making the initial hypercall. Maybe it goes without saying, but to address #3, the guest must consider existing data as lost the instant it tells the host the page has been converted to a different type.

Quoted 3 lines

For the above reason if we do in-kernel hypercall handling for page encryption status (which we probably won't require for SEV-SNP & correspondingly there will be no hypercall exiting),

As above, that doesn't preclude KVM from exiting to userspace on conversion.

Quoted 3 lines

then we can implement a standard GET/SET ioctl interface to get/set the guest page encryption status for userspace, which will work across SEV, SEV-ES and SEV-SNP.

Quoted 99 lines · Sean Christopherson

Quoted 59 lines · Ashish Kalra

Quoted 52 lines · Sean Christopherson

+Will and Quentin (arm64)

Moving the non-KVM x86 folks to bcc, I don't they care about KVM details at this point.

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

	/* KVM_EXIT_HYPERCALL */
	struct {
		__u64 nr;
		__u64 args[6];
		__u64 ret;
		__u32 longmode;
		__u32 pad;
	} hypercall;

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

Quoted 6 lines

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Quoted 8 lines

Potentially, there is another reason for in-kernel hypercall handling considering SEV-SNP. In case of SEV-SNP the RMP table tracks the state of each guest page, for instance pages in hypervisor state, i.e., pages with C=0 and pages in guest valid state with C=1.

Now, there shouldn't be a need for page encryption status hypercalls on SEV-SNP as KVM can track & reference guest page status directly using the RMP table.

Relying on the RMP table itself would require locking the RMP table for an extended duration, and walking the entire RMP to find shared pages would be very inefficient.

Quoted 2 lines

As KVM maintains the RMP table, therefore we will need SET/GET type of interfaces to provide the guest page encryption status to userspace.

Hrm, somehow I temporarily forgot about SNP and TDX adding their own hypercalls for converting between shared and private. And in the case of TDX, the hypercall can't be trusted, i.e. is just a hint, otherwise the guest could induce a #MC in the host.

But, the different guest behavior doesn't require KVM to maintain a list/tree, e.g. adding a dedicated KVM_EXIT_* for notifying userspace of page encryption status changes would also suffice.

Actually, that made me think of another argument against maintaining a list in
KVM: there's no way to notify userspace that a page's status has changed. Userspace would need to query KVM to do GET_LIST after every GET_DIRTY. Obviously not a huge issue, but it does make migration slightly less efficient.

On a related topic, there are fatal race conditions that will require careful coordination between guest and host, and will effectively be wired into the ABI. SNP and TDX don't suffer these issues because host awareness of status is atomic with respect to the guest actually writing the page with the new encryption status.

For SEV live migration...

If the guest does the hypercall after writing the page, then the guest is hosed if it gets migrated while writing the page (scenario #1):

Quoted 7 lines

vCPU Userspace
zero_bytes[0:N]
<transfers written bytes as private instead of shared> <migrates vCPU>
zero_bytes[N+1:4095]
set_shared (dest)
kaboom!

Maybe I am missing something, this is not any different from a normal operation inside a guest. Making a page shared/private in the page table does not update the content of the page itself. In your above case, I assume zero_bytes[N+1:4095] are written by the destination VM. The memory region was private in the source VM page table, so, those writes will be performed encrypted. The destination VM later changed the memory to shared, but nobody wrote to the memory after it has been transitioned to the  shared, so a reader of the memory should get ciphertext and unless there was a write after the set_shared (dest).

Quoted 11 lines

If userspace does GET_DIRTY after GET_LIST, then the host would transfer bad data by consuming a stale list (scenario #2):

vCPU Userspace
get_list (from KVM or internally)
set_shared (src)
zero_page (src)
get_dirty
<transfers private data instead of shared> <migrates vCPU>
kaboom!

I don't remember how things are done in recent Ashish Qemu/KVM patches but in previous series, the get_dirty() happens before the querying the encrypted state. There was some logic in VMM to resync the encrypted bitmap during the final migration stage and perform any additional data transfer since last sync.

Quoted 99 lines · Sean Christopherson

Quoted 59 lines · Ashish Kalra

Quoted 52 lines · Sean Christopherson

+Will and Quentin (arm64)

Moving the non-KVM x86 folks to bcc, I don't they care about KVM details at this point.

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

    /* KVM_EXIT_HYPERCALL */
    struct {
            __u64 nr;
            __u64 args[6];
            __u64 ret;
            __u32 longmode;
            __u32 pad;
    } hypercall;

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

Quoted 6 lines

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Quoted 8 lines

Potentially, there is another reason for in-kernel hypercall handling considering SEV-SNP. In case of SEV-SNP the RMP table tracks the state of each guest page, for instance pages in hypervisor state, i.e., pages with C=0 and pages in guest valid state with C=1.

Now, there shouldn't be a need for page encryption status hypercalls on SEV-SNP as KVM can track & reference guest page status directly using the RMP table.

Relying on the RMP table itself would require locking the RMP table for an extended duration, and walking the entire RMP to find shared pages would be very inefficient.

Quoted 2 lines

As KVM maintains the RMP table, therefore we will need SET/GET type of interfaces to provide the guest page encryption status to userspace.

Hrm, somehow I temporarily forgot about SNP and TDX adding their own hypercalls for converting between shared and private. And in the case of TDX, the hypercall can't be trusted, i.e. is just a hint, otherwise the guest could induce a #MC in the host.

But, the different guest behavior doesn't require KVM to maintain a list/tree, e.g. adding a dedicated KVM_EXIT_* for notifying userspace of page encryption status changes would also suffice.

Actually, that made me think of another argument against maintaining a list in
KVM: there's no way to notify userspace that a page's status has changed. Userspace would need to query KVM to do GET_LIST after every GET_DIRTY. Obviously not a huge issue, but it does make migration slightly less efficient.

On a related topic, there are fatal race conditions that will require careful coordination between guest and host, and will effectively be wired into the ABI. SNP and TDX don't suffer these issues because host awareness of status is atomic with respect to the guest actually writing the page with the new encryption status.

For SEV live migration...

If the guest does the hypercall after writing the page, then the guest is hosed if it gets migrated while writing the page (scenario #1):

Quoted 7 lines

vCPU Userspace
zero_bytes[0:N]
<transfers written bytes as private instead of shared>
<migrates vCPU>
zero_bytes[N+1:4095]
set_shared (dest)
kaboom!

Maybe I am missing something, this is not any different from a normal operation inside a guest. Making a page shared/private in the page table does not update the content of the page itself. In your above case, I assume zero_bytes[N+1:4095] are written by the destination VM. The memory region was private in the source VM page table, so, those writes will be performed encrypted. The destination VM later changed the memory to shared, but nobody wrote to the memory after it has been transitioned to the shared, so a reader of the memory should get ciphertext and unless there was a write after the set_shared (dest).

Quoted 11 lines

If userspace does GET_DIRTY after GET_LIST, then the host would transfer bad data by consuming a stale list (scenario #2):

vCPU Userspace
get_list (from KVM or internally)
set_shared (src)
zero_page (src)
get_dirty
<transfers private data instead of shared> <migrates vCPU>
kaboom!

I don't remember how things are done in recent Ashish Qemu/KVM patches but in previous series, the get_dirty() happens before the querying the encrypted state. There was some logic in VMM to resync the encrypted bitmap during the final migration stage and perform any additional data transfer since last sync.

Quoted 13 lines

If both guest and host order things to avoid #1 and #2, the host can still migrate the wrong data (scenario #3):

vCPU Userspace
set_private
zero_bytes[0:4096]
get_dirty
set_shared (src)
get_list
<transfers as shared instead of private>
<migrates vCPU>
set_private (dest)
kaboom!

Since there was no write to the memory after the set_shared (src), so the content of the page should not have changed. After the set_private (dest), the caller should be seeing the same content written by the zero_bytes[0:4096]

Quoted 16 lines

Scenario #3 is unlikely, but plausible, e.g. if the guest bails from its conversion flow for whatever reason, after making the initial hypercall. Maybe it goes without saying, but to address #3, the guest must consider existing data as lost the instant it tells the host the page has been converted to a different type.

Quoted 3 lines

For the above reason if we do in-kernel hypercall handling for page encryption status (which we probably won't require for SEV-SNP & correspondingly there will be no hypercall exiting),

As above, that doesn't preclude KVM from exiting to userspace on conversion.

Quoted 3 lines

then we can implement a standard GET/SET ioctl interface to get/set the guest page encryption status for userspace, which will work across SEV, SEV-ES and SEV-SNP.

Quoted 13 lines · Brijesh Singh

Quoted 10 lines · Sean Christopherson

If the guest does the hypercall after writing the page, then the guest is hosed if it gets migrated while writing the page (scenario #1):

vCPU Userspace
zero_bytes[0:N]
<transfers written bytes as private instead of shared>
<migrates vCPU>
zero_bytes[N+1:4095]
set_shared (dest)
kaboom!

Quoted 9 lines

Maybe I am missing something, this is not any different from a normal operation inside a guest. Making a page shared/private in the page table does not update the content of the page itself. In your above case, I assume zero_bytes[N+1:4095] are written by the destination VM. The memory region was private in the source VM page table, so, those writes will be performed encrypted. The destination VM later changed the memory to shared, but nobody wrote to the memory after it has been transitioned to the shared, so a reader of the memory should get ciphertext and unless there was a write after the set_shared (dest).

Quoted 13 lines

Quoted 11 lines

If userspace does GET_DIRTY after GET_LIST, then the host would transfer bad data by consuming a stale list (scenario #2):

vCPU Userspace
get_list (from KVM or internally)
set_shared (src)
zero_page (src)
get_dirty
<transfers private data instead of shared> <migrates vCPU>
kaboom!

Quoted 5 lines

I don't remember how things are done in recent Ashish Qemu/KVM patches but in previous series, the get_dirty() happens before the querying the encrypted state. There was some logic in VMM to resync the encrypted bitmap during the final migration stage and perform any additional data transfer since last sync.

Quoted 15 lines

Quoted 13 lines

If both guest and host order things to avoid #1 and #2, the host can still migrate the wrong data (scenario #3):

vCPU Userspace
set_private
zero_bytes[0:4096]
get_dirty
set_shared (src)
get_list
<transfers as shared instead of private>
<migrates vCPU>
set_private (dest)
kaboom!

Quoted 4 lines

Since there was no write to the memory after the set_shared (src), so the content of the page should not have changed. After the set_private (dest), the caller should be seeing the same content written by the zero_bytes[0:4096]

I think Sean was going for the situation where the VM has moved to the destination, which would have changed the VEK. That way the guest would be decrypting the old ciphertext with the new (wrong) key.

Quoted 14 lines

Quoted 14 lines

Scenario #3 is unlikely, but plausible, e.g. if the guest bails from its conversion flow for whatever reason, after making the initial hypercall. Maybe it goes without saying, but to address #3, the guest must consider existing data as lost the instant it tells the host the page has been converted to a different type.

Quoted 3 lines

For the above reason if we do in-kernel hypercall handling for page encryption status (which we probably won't require for SEV-SNP & correspondingly there will be no hypercall exiting),

As above, that doesn't preclude KVM from exiting to userspace on conversion.

Quoted 3 lines

then we can implement a standard GET/SET ioctl interface to get/set the guest page encryption status for userspace, which will work across SEV, SEV-ES and SEV-SNP.

Quoted 157 lines · Brijesh Singh

Quoted 102 lines · Sean Christopherson

Quoted 59 lines · Ashish Kalra

Quoted 52 lines · Sean Christopherson

+Will and Quentin (arm64)

Moving the non-KVM x86 folks to bcc, I don't they care about KVM details at this point.

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

   /* KVM_EXIT_HYPERCALL */
   struct {
           __u64 nr;
           __u64 args[6];
           __u64 ret;
           __u32 longmode;
           __u32 pad;
   } hypercall;

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

Quoted 6 lines

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Quoted 8 lines

Potentially, there is another reason for in-kernel hypercall handling considering SEV-SNP. In case of SEV-SNP the RMP table tracks the state of each guest page, for instance pages in hypervisor state, i.e., pages with C=0 and pages in guest valid state with C=1.

Now, there shouldn't be a need for page encryption status hypercalls on SEV-SNP as KVM can track & reference guest page status directly using the RMP table.

Relying on the RMP table itself would require locking the RMP table for an extended duration, and walking the entire RMP to find shared pages would be very inefficient.

Quoted 2 lines

As KVM maintains the RMP table, therefore we will need SET/GET type of interfaces to provide the guest page encryption status to userspace.

Hrm, somehow I temporarily forgot about SNP and TDX adding their own hypercalls for converting between shared and private. And in the case of TDX, the hypercall can't be trusted, i.e. is just a hint, otherwise the guest could induce a #MC in the host.

But, the different guest behavior doesn't require KVM to maintain a list/tree, e.g. adding a dedicated KVM_EXIT_* for notifying userspace of page encryption status changes would also suffice.

Actually, that made me think of another argument against maintaining a list in
KVM: there's no way to notify userspace that a page's status has changed. Userspace would need to query KVM to do GET_LIST after every GET_DIRTY. Obviously not a huge issue, but it does make migration slightly less efficient.

On a related topic, there are fatal race conditions that will require careful coordination between guest and host, and will effectively be wired into the ABI. SNP and TDX don't suffer these issues because host awareness of status is atomic with respect to the guest actually writing the page with the new encryption status.

For SEV live migration...

If the guest does the hypercall after writing the page, then the guest is hosed if it gets migrated while writing the page (scenario #1):

Quoted 7 lines

vCPU Userspace
zero_bytes[0:N]
<transfers written bytes as private instead of shared>
<migrates vCPU>
zero_bytes[N+1:4095]
set_shared (dest)
kaboom!

Maybe I am missing something, this is not any different from a normal operation inside a guest. Making a page shared/private in the page table does not update the content of the page itself. In your above case, I assume zero_bytes[N+1:4095] are written by the destination VM. The memory region was private in the source VM page table, so, those writes will be performed encrypted. The destination VM later changed the memory to shared, but nobody wrote to the memory after it has been transitioned to the shared, so a reader of the memory should get ciphertext and unless there was a write after the set_shared (dest).

Quoted 11 lines

If userspace does GET_DIRTY after GET_LIST, then the host would transfer bad data by consuming a stale list (scenario #2):

vCPU Userspace
get_list (from KVM or internally)
set_shared (src)
zero_page (src)
get_dirty
<transfers private data instead of shared> <migrates vCPU>
kaboom!

I don't remember how things are done in recent Ashish Qemu/KVM patches but in previous series, the get_dirty() happens before the querying the encrypted state. There was some logic in VMM to resync the encrypted bitmap during the final migration stage and perform any additional data transfer since last sync.

Quoted 13 lines

If both guest and host order things to avoid #1 and #2, the host can still migrate the wrong data (scenario #3):

vCPU Userspace
set_private
zero_bytes[0:4096]
get_dirty
set_shared (src)
get_list
<transfers as shared instead of private>
<migrates vCPU>
set_private (dest)
kaboom!

Quoted 4 lines

Since there was no write to the memory after the set_shared (src), so the content of the page should not have changed. After the set_private (dest), the caller should be seeing the same content written by the zero_bytes[0:4096]

I think Sean was going for the situation where the VM has moved to the destination, which would have changed the VEK. That way the guest would be decrypting the old ciphertext with the new (wrong) key.

Quoted 1 line

Quoted 15 lines

Quoted 15 lines

Scenario #3 is unlikely, but plausible, e.g. if the guest bails from its conversion flow for whatever reason, after making the initial hypercall. Maybe it goes without saying, but to address #3, the guest must consider existing data as lost the instant it tells the host the page has been converted to a different type.

Quoted 3 lines

For the above reason if we do in-kernel hypercall handling for page encryption status (which we probably won't require for SEV-SNP & correspondingly there will be no hypercall exiting),

As above, that doesn't preclude KVM from exiting to userspace on conversion.

Quoted 3 lines

then we can implement a standard GET/SET ioctl interface to get/set the guest page encryption status for userspace, which will work across SEV, SEV-ES and SEV-SNP.

Quoted 167 lines · Steve Rutherford

Quoted 161 lines · Brijesh Singh

Quoted 102 lines · Sean Christopherson

Quoted 59 lines · Ashish Kalra

Quoted 52 lines · Sean Christopherson

+Will and Quentin (arm64)

Moving the non-KVM x86 folks to bcc, I don't they care about KVM details at this point.

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

   /* KVM_EXIT_HYPERCALL */
   struct {
           __u64 nr;
           __u64 args[6];
           __u64 ret;
           __u32 longmode;
           __u32 pad;
   } hypercall;

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

Quoted 6 lines

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Quoted 8 lines

Potentially, there is another reason for in-kernel hypercall handling considering SEV-SNP. In case of SEV-SNP the RMP table tracks the state of each guest page, for instance pages in hypervisor state, i.e., pages with C=0 and pages in guest valid state with C=1.

Now, there shouldn't be a need for page encryption status hypercalls on SEV-SNP as KVM can track & reference guest page status directly using the RMP table.

Relying on the RMP table itself would require locking the RMP table for an extended duration, and walking the entire RMP to find shared pages would be very inefficient.

Quoted 2 lines

As KVM maintains the RMP table, therefore we will need SET/GET type of interfaces to provide the guest page encryption status to userspace.

Hrm, somehow I temporarily forgot about SNP and TDX adding their own hypercalls for converting between shared and private. And in the case of TDX, the hypercall can't be trusted, i.e. is just a hint, otherwise the guest could induce a #MC in the host.

But, the different guest behavior doesn't require KVM to maintain a list/tree, e.g. adding a dedicated KVM_EXIT_* for notifying userspace of page encryption status changes would also suffice.

Actually, that made me think of another argument against maintaining a list in
KVM: there's no way to notify userspace that a page's status has changed. Userspace would need to query KVM to do GET_LIST after every GET_DIRTY. Obviously not a huge issue, but it does make migration slightly less efficient.

On a related topic, there are fatal race conditions that will require careful coordination between guest and host, and will effectively be wired into the ABI. SNP and TDX don't suffer these issues because host awareness of status is atomic with respect to the guest actually writing the page with the new encryption status.

For SEV live migration...

If the guest does the hypercall after writing the page, then the guest is hosed if it gets migrated while writing the page (scenario #1):

Quoted 7 lines

vCPU Userspace
zero_bytes[0:N]
<transfers written bytes as private instead of shared>
<migrates vCPU>
zero_bytes[N+1:4095]
set_shared (dest)
kaboom!

Maybe I am missing something, this is not any different from a normal operation inside a guest. Making a page shared/private in the page table does not update the content of the page itself. In your above case, I assume zero_bytes[N+1:4095] are written by the destination VM. The memory region was private in the source VM page table, so, those writes will be performed encrypted. The destination VM later changed the memory to shared, but nobody wrote to the memory after it has been transitioned to the shared, so a reader of the memory should get ciphertext and unless there was a write after the set_shared (dest).

Quoted 11 lines

If userspace does GET_DIRTY after GET_LIST, then the host would transfer bad data by consuming a stale list (scenario #2):

vCPU Userspace
get_list (from KVM or internally)
set_shared (src)
zero_page (src)
get_dirty
<transfers private data instead of shared> <migrates vCPU>
kaboom!

I don't remember how things are done in recent Ashish Qemu/KVM patches but in previous series, the get_dirty() happens before the querying the encrypted state. There was some logic in VMM to resync the encrypted bitmap during the final migration stage and perform any additional data transfer since last sync.

Quoted 13 lines

If both guest and host order things to avoid #1 and #2, the host can still migrate the wrong data (scenario #3):

vCPU Userspace
set_private
zero_bytes[0:4096]
get_dirty
set_shared (src)
get_list
<transfers as shared instead of private>
<migrates vCPU>
set_private (dest)
kaboom!

Since there was no write to the memory after the set_shared (src), so the content of the page should not have changed. After the set_private (dest), the caller should be seeing the same content written by the zero_bytes[0:4096]

Quoted 4 lines

I think Sean was going for the situation where the VM has moved to the destination, which would have changed the VEK. That way the guest would be decrypting the old ciphertext with the new (wrong) key.

Quoted 1 line

But how can this happen, if a page is migrated as private , when it is received it will be decrypted using the transport key TEK and then re-encrypted using the destination VM’s VEK on the destination VM.

Thanks,
Ashish

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

Scenario #3 is unlikely, but plausible, e.g. if the guest bails from its conversion flow for whatever reason, after making the initial hypercall. Maybe it goes without saying, but to address #3, the guest must consider existing data as lost the instant it tells the host the page has been converted to a different type.

Quoted 3 lines

For the above reason if we do in-kernel hypercall handling for page encryption status (which we probably won't require for SEV-SNP & correspondingly there will be no hypercall exiting),

As above, that doesn't preclude KVM from exiting to userspace on conversion.

Quoted 3 lines

then we can implement a standard GET/SET ioctl interface to get/set the guest page encryption status for userspace, which will work across SEV, SEV-ES and SEV-SNP.

Quoted 63 lines · Ashish Kalra

Quoted 55 lines · Sean Christopherson

+Will and Quentin (arm64)

Moving the non-KVM x86 folks to bcc, I don't they care about KVM details at this point.

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Steve Rutherford

On Thu, Feb 25, 2021 at 12:20 PM Ashish Kalra <[email protected]> wrote:
Thanks for grabbing the data!

I am fine with both paths. Sean has stated an explicit desire for hypercall exiting, so I think that would be the current consensus.

Yep, though it'd be good to get Paolo's input, too.

Quoted 4 lines

Quoted 4 lines

If we want to do hypercall exiting, this should be in a follow-up series where we implement something more generic, e.g. a hypercall exiting bitmap or hypercall exit list. If we are taking the hypercall exit route, we can drop the kvm side of the hypercall.

I don't think this is a good candidate for arbitrary hypercall interception. Or rather, I think hypercall interception should be an orthogonal implementation.

The guest, including guest firmware, needs to be aware that the hypercall is supported, and the ABI needs to be well-defined. Relying on userspace VMMs to implement a common ABI is an unnecessary risk.

We could make KVM's default behavior be a nop, i.e. have KVM enforce the ABI but require further VMM intervention. But, I just don't see the point, it would save only a few lines of code. It would also limit what KVM could do in the future, e.g. if KVM wanted to do its own bookkeeping _and_ exit to userspace, then mandatory interception would essentially make it impossible for KVM to do bookkeeping while still honoring the interception request.

However, I do think it would make sense to have the userspace exit be a generic exit type. But hey, we already have the necessary ABI defined for that! It's just not used anywhere.

    /* KVM_EXIT_HYPERCALL */
    struct {
            __u64 nr;
            __u64 args[6];
            __u64 ret;
            __u32 longmode;
            __u32 pad;
    } hypercall;

Quoted 2 lines

Quoted 2 lines

Userspace could also handle the MSR using MSR filters (would need to confirm that). Then userspace could also be in control of the cpuid bit.

An MSR is not a great fit; it's x86 specific and limited to 64 bits of data. The data limitation could be fudged by shoving data into non-standard GPRs, but that will result in truly heinous guest code, and extensibility issues.

Quoted 6 lines

The data limitation is a moot point, because the x86-only thing is a deal breaker. arm64's pKVM work has a near-identical use case for a guest to share memory with a host. I can't think of a clever way to avoid having to support TDX's and SNP's hypervisor-agnostic variants, but we can at least not have multiple KVM variants.

Quoted 8 lines

Potentially, there is another reason for in-kernel hypercall handling considering SEV-SNP. In case of SEV-SNP the RMP table tracks the state of each guest page, for instance pages in hypervisor state, i.e., pages with C=0 and pages in guest valid state with C=1.

Now, there shouldn't be a need for page encryption status hypercalls on SEV-SNP as KVM can track & reference guest page status directly using the RMP table.

Relying on the RMP table itself would require locking the RMP table for an extended duration, and walking the entire RMP to find shared pages would be very inefficient.

Quoted 2 lines

As KVM maintains the RMP table, therefore we will need SET/GET type of interfaces to provide the guest page encryption status to userspace.

Hrm, somehow I temporarily forgot about SNP and TDX adding their own hypercalls for converting between shared and private. And in the case of TDX, the hypercall can't be trusted, i.e. is just a hint, otherwise the guest could induce a #MC in the host.

But, the different guest behavior doesn't require KVM to maintain a list/tree, e.g. adding a dedicated KVM_EXIT_* for notifying userspace of page encryption status changes would also suffice.

Actually, that made me think of another argument against maintaining a list in
KVM: there's no way to notify userspace that a page's status has changed. Userspace would need to query KVM to do GET_LIST after every GET_DIRTY. Obviously not a huge issue, but it does make migration slightly less efficient.

On a related topic, there are fatal race conditions that will require careful coordination between guest and host, and will effectively be wired into the ABI. SNP and TDX don't suffer these issues because host awareness of status is atomic with respect to the guest actually writing the page with the new encryption status.

For SEV live migration...

If the guest does the hypercall after writing the page, then the guest is hosed if it gets migrated while writing the page (scenario #1):

vCPU Userspace
zero_bytes[0:N]
<transfers written bytes as private instead of shared> <migrates vCPU>
zero_bytes[N+1:4095]
set_shared (dest)
kaboom!

If userspace does GET_DIRTY after GET_LIST, then the host would transfer bad data by consuming a stale list (scenario #2):

vCPU Userspace
get_list (from KVM or internally)
set_shared (src)
zero_page (src)
get_dirty
<transfers private data instead of shared> <migrates vCPU>
kaboom!

If both guest and host order things to avoid #1 and #2, the host can still migrate the wrong data (scenario #3):

vCPU Userspace
set_private
zero_bytes[0:4096]
get_dirty
set_shared (src)
get_list
<transfers as shared instead of private> <migrates vCPU>
set_private (dest)
kaboom!

Scenario #3 is unlikely, but plausible, e.g. if the guest bails from its conversion flow for whatever reason, after making the initial hypercall. Maybe it goes without saying, but to address #3, the guest must consider existing data as lost the instant it tells the host the page has been converted to a different type.

From: Brijesh Singh <[email protected]>

The ioctl is used to retrieve a guest's shared pages list.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

From: Ashish Kalra <[email protected]>

Add new KVM_FEATURE_SEV_LIVE_MIGRATION feature for guest to check for host-side support for SEV live migration. Also add a new custom MSR_KVM_SEV_LIVE_MIGRATION for guest to enable the SEV live migration feature.

Signed-off-by Ashish Kalra <[email protected]>

---
 Documentation/virt/kvm/cpuid.rst     |  5 +++++
 Documentation/virt/kvm/msr.rst       | 12 ++++++++++++
 arch/x86/include/uapi/asm/kvm_para.h |  4 ++++
 arch/x86/kvm/svm/sev.c               | 13 +++++++++++++
 arch/x86/kvm/svm/svm.c               | 16 ++++++++++++++++
 arch/x86/kvm/svm/svm.h               |  2 ++
 6 files changed, 52 insertions(+)

Documentation/virt/kvm/cpuid.rst +5 -0

index cf62162d4be2..0bdb6cdb12d3 100644
@@ -96,6 +96,11 @@ KVM_FEATURE_MSI_EXT_DEST_ID        15          guest checks this feature bit
                                                before using extended destination
                                                ID bits in MSI address bits 11-5.
 
+KVM_FEATURE_SEV_LIVE_MIGRATION     16          guest checks this feature bit before
+                                               using the page encryption state
+                                               hypercall to notify the page state
+                                               change
+
 KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 24          host will warn if no guest-side
                                                per-cpu warps are expected in
                                                kvmclock

Documentation/virt/kvm/msr.rst +12 -0

index e37a14c323d2..020245d16087 100644
@@ -376,3 +376,15 @@ data:
        write '1' to bit 0 of the MSR, this causes the host to re-scan its queue
        and check if there are more notifications pending. The MSR is available
        if KVM_FEATURE_ASYNC_PF_INT is present in CPUID.
+
+MSR_KVM_SEV_LIVE_MIGRATION:
+        0x4b564d08
+
+       Control SEV Live Migration features.
+
+data:
+        Bit 0 enables (1) or disables (0) host-side SEV Live Migration feature,
+        in other words, this is guest->host communication that it's properly
+        handling the shared pages list.
+
+        All other bits are reserved.

arch/x86/include/uapi/asm/kvm_para.h +4 -0

index 950afebfba88..f6bfa138874f 100644
@@ -33,6 +33,7 @@
 #define KVM_FEATURE_PV_SCHED_YIELD     13
 #define KVM_FEATURE_ASYNC_PF_INT       14
 #define KVM_FEATURE_MSI_EXT_DEST_ID    15
+#define KVM_FEATURE_SEV_LIVE_MIGRATION 16
 
 #define KVM_HINTS_REALTIME      0
 

@@ -54,6 +55,7 @@
 #define MSR_KVM_POLL_CONTROL   0x4b564d05
 #define MSR_KVM_ASYNC_PF_INT   0x4b564d06
 #define MSR_KVM_ASYNC_PF_ACK   0x4b564d07
+#define MSR_KVM_SEV_LIVE_MIGRATION     0x4b564d08
 
 struct kvm_steal_time {
        __u64 steal;

@@ -136,4 +138,6 @@ struct kvm_vcpu_pv_apf_data {
 #define KVM_PV_EOI_ENABLED KVM_PV_EOI_MASK
 #define KVM_PV_EOI_DISABLED 0x0
 
+#define KVM_SEV_LIVE_MIGRATION_ENABLED BIT_ULL(0)
+
 #endif /* _UAPI_ASM_X86_KVM_PARA_H */

arch/x86/kvm/svm/sev.c +0 -0

index b0d324aed515..93f42b3d3e33 100644