[PATCH v2 00/12] KVM: xen: update shared_info and vcpu_info handling

[PATCH v2 00/12] KVM: xen: update shared_info and vcpu_info handling

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

Currently we treat the shared_info page as guest memory and the VMM informs
KVM of its location using a GFN. However it is not guest memory as such;
it's an overlay page. So we pointlessly invalidate and re-cache a mapping
to the *same page* of memory every time the guest requests that shared_info
be mapped into its address space. Let's avoid doing that by modifying the
pfncache code to allow activation using a fixed userspace HVA as well as
a GPA.

Also, if the guest does not hypercall to explicitly set a pointer to a
vcpu_info in its own memory, the default vcpu_info embedded in the
shared_info page should be used. At the moment the VMM has to set up a
pointer to the structure explicitly (again treating it like it's in
guest memory, despite being in an overlay page). Let's also avoid the
need for that. We already have a cached mapping for the shared_info
page so just use that directly by default.

Paul Durrant (12):
  KVM: pfncache: add a map helper function
  KVM: pfncache: add a mark-dirty helper
  KVM: pfncache: add a helper to get the gpa
  KVM: pfncache: base offset check on khva rather than gpa
  KVM: pfncache: allow a cache to be activated with a fixed (userspace)
    HVA
  KVM: xen: allow shared_info to be mapped by fixed HVA
  KVM: xen: prepare for using 'default' vcpu_info
  KVM: xen: automatically use the vcpu_info embedded in shared_info
  KVM: selftests / xen: set KVM_XEN_VCPU_ATTR_TYPE_VCPU_ID
  KVM: selftests / xen: map shared_info using HVA rather than GFN
  KVM: selftests / xen: don't explicitly set the vcpu_info address
  KVM: xen: advertize the KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA capability

 Documentation/virt/kvm/api.rst                |  43 ++++--
 arch/x86/include/asm/kvm_host.h               |   4 +
 arch/x86/kvm/x86.c                            |  17 +--
 arch/x86/kvm/xen.c                            | 121 ++++++++++++----
 arch/x86/kvm/xen.h                            |   6 +-
 include/linux/kvm_host.h                      |  43 ++++++
 include/linux/kvm_types.h                     |   3 +-
 include/uapi/linux/kvm.h                      |   6 +-
 .../selftests/kvm/x86_64/xen_shinfo_test.c    |  79 +++++++++--
 virt/kvm/pfncache.c                           | 129 +++++++++++++-----
 10 files changed, 342 insertions(+), 109 deletions(-)
---
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: x86@kernel.org
-- 
2.39.2

[PATCH v2 01/12] KVM: pfncache: add a map helper function

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

We have an unmap helper but mapping is open-coded. Arguably this is fine
because mapping is done in only one place, hva_to_pfn_retry(), but adding
the helper does make that function more readable.

No functional change intended.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: David Woodhouse <dwmw2@infradead.org>
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 virt/kvm/pfncache.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 2d6aba677830..0f36acdf577f 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -96,17 +96,28 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
 }
 EXPORT_SYMBOL_GPL(kvm_gpc_check);
 
-static void gpc_unmap_khva(kvm_pfn_t pfn, void *khva)
+static void *gpc_map(kvm_pfn_t pfn)
+{
+	if (pfn_valid(pfn))
+		return kmap(pfn_to_page(pfn));
+#ifdef CONFIG_HAS_IOMEM
+	else
+		return memremap(pfn_to_hpa(pfn), PAGE_SIZE, MEMREMAP_WB);
+#endif
+}
+
+static void gpc_unmap(kvm_pfn_t pfn, void *khva)
 {
 	/* Unmap the old pfn/page if it was mapped before. */
-	if (!is_error_noslot_pfn(pfn) && khva) {
-		if (pfn_valid(pfn))
-			kunmap(pfn_to_page(pfn));
+	if (is_error_noslot_pfn(pfn) || !khva)
+		return;
+
+	if (pfn_valid(pfn))
+		kunmap(pfn_to_page(pfn));
 #ifdef CONFIG_HAS_IOMEM
-		else
-			memunmap(khva);
+	else
+		memunmap(khva);
 #endif
-	}
 }
 
 static inline bool mmu_notifier_retry_cache(struct kvm *kvm, unsigned long mmu_seq)
@@ -175,7 +186,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
 			 * the existing mapping and didn't create a new one.
 			 */
 			if (new_khva != old_khva)
-				gpc_unmap_khva(new_pfn, new_khva);
+				gpc_unmap(new_pfn, new_khva);
 
 			kvm_release_pfn_clean(new_pfn);
 
@@ -193,15 +204,11 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
 		 * too must be done outside of gpc->lock!
 		 */
 		if (gpc->usage & KVM_HOST_USES_PFN) {
-			if (new_pfn == gpc->pfn) {
+			if (new_pfn == gpc->pfn)
 				new_khva = old_khva;
-			} else if (pfn_valid(new_pfn)) {
-				new_khva = kmap(pfn_to_page(new_pfn));
-#ifdef CONFIG_HAS_IOMEM
-			} else {
-				new_khva = memremap(pfn_to_hpa(new_pfn), PAGE_SIZE, MEMREMAP_WB);
-#endif
-			}
+			else
+				new_khva = gpc_map(new_pfn);
+
 			if (!new_khva) {
 				kvm_release_pfn_clean(new_pfn);
 				goto out_error;
@@ -326,7 +333,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
 	mutex_unlock(&gpc->refresh_lock);
 
 	if (unmap_old)
-		gpc_unmap_khva(old_pfn, old_khva);
+		gpc_unmap(old_pfn, old_khva);
 
 	return ret;
 }
@@ -412,7 +419,7 @@ void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc)
 		list_del(&gpc->list);
 		spin_unlock(&kvm->gpc_lock);
 
-		gpc_unmap_khva(old_pfn, old_khva);
+		gpc_unmap(old_pfn, old_khva);
 	}
 }
 EXPORT_SYMBOL_GPL(kvm_gpc_deactivate);
-- 
2.39.2

[PATCH v2 02/12] KVM: pfncache: add a mark-dirty helper

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

At the moment pages are marked dirty by open-coded calls to
mark_page_dirty_in_slot(), directly deferefencing the gpa and memslot
from the cache. After a subsequent patch these may not always be set
so add a helper now so that caller will protected from the need to know
about this detail.

NOTE: Pages are now marked dirty while the cache lock is held. This is
      to ensure that gpa and memslot are mutually consistent.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: David Woodhouse <dwmw2@infradead.org>
---
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
---
 arch/x86/kvm/x86.c       |  2 +-
 arch/x86/kvm/xen.c       | 13 ++++++-------
 include/linux/kvm_host.h |  7 +++++++
 virt/kvm/pfncache.c      |  6 ++++++
 4 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6c9c81e82e65..d669a8801265 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3137,7 +3137,7 @@ static void kvm_setup_guest_pvclock(struct kvm_vcpu *v,
 
 	guest_hv_clock->version = ++vcpu->hv_clock.version;
 
-	mark_page_dirty_in_slot(v->kvm, gpc->memslot, gpc->gpa >> PAGE_SHIFT);
+	kvm_gpc_mark_dirty(gpc);
 	read_unlock_irqrestore(&gpc->lock, flags);
 
 	trace_kvm_pvclock_update(v->vcpu_id, &vcpu->hv_clock);
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 40edf4d1974c..33fddd29824b 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -430,14 +430,13 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
 		smp_wmb();
 	}
 
-	if (user_len2)
+	if (user_len2) {
+		kvm_gpc_mark_dirty(gpc2);
 		read_unlock(&gpc2->lock);
+	}
 
+	kvm_gpc_mark_dirty(gpc1);
 	read_unlock_irqrestore(&gpc1->lock, flags);
-
-	mark_page_dirty_in_slot(v->kvm, gpc1->memslot, gpc1->gpa >> PAGE_SHIFT);
-	if (user_len2)
-		mark_page_dirty_in_slot(v->kvm, gpc2->memslot, gpc2->gpa >> PAGE_SHIFT);
 }
 
 void kvm_xen_update_runstate(struct kvm_vcpu *v, int state)
@@ -543,13 +542,13 @@ void kvm_xen_inject_pending_events(struct kvm_vcpu *v)
 			     : "0" (evtchn_pending_sel32));
 		WRITE_ONCE(vi->evtchn_upcall_pending, 1);
 	}
+
+	kvm_gpc_mark_dirty(gpc);
 	read_unlock_irqrestore(&gpc->lock, flags);
 
 	/* For the per-vCPU lapic vector, deliver it as MSI. */
 	if (v->arch.xen.upcall_vector)
 		kvm_xen_inject_vcpu_vector(v);
-
-	mark_page_dirty_in_slot(v->kvm, gpc->memslot, gpc->gpa >> PAGE_SHIFT);
 }
 
 int __kvm_xen_has_interrupt(struct kvm_vcpu *v)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index fb6c6109fdca..c71e8fbccaaf 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1367,6 +1367,13 @@ int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len);
  */
 void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc);
 
+/**
+ * kvm_gpc_mark_dirty - mark a cached page as dirty.
+ *
+ * @gpc:	   struct gfn_to_pfn_cache object.
+ */
+void kvm_gpc_mark_dirty(struct gfn_to_pfn_cache *gpc);
+
 void kvm_sigset_activate(struct kvm_vcpu *vcpu);
 void kvm_sigset_deactivate(struct kvm_vcpu *vcpu);
 
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 0f36acdf577f..b68ed7fa56a2 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -386,6 +386,12 @@ int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len)
 }
 EXPORT_SYMBOL_GPL(kvm_gpc_activate);
 
+void kvm_gpc_mark_dirty(struct gfn_to_pfn_cache *gpc)
+{
+	mark_page_dirty_in_slot(gpc->kvm, gpc->memslot, gpc->gpa >> PAGE_SHIFT);
+}
+EXPORT_SYMBOL_GPL(kvm_gpc_mark_dirty);
+
 void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc)
 {
 	struct kvm *kvm = gpc->kvm;
-- 
2.39.2

[PATCH v2 03/12] KVM: pfncache: add a helper to get the gpa

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

A subsequent patch will rename this field since it will become overloaded.
To avoid churn in places that currently retrieve the gpa, add a helper for
that purpose now.

No functional change intended.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: David Woodhouse <dwmw2@infradead.org>
---
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
---
 arch/x86/kvm/xen.c       | 15 ++++++++-------
 include/linux/kvm_host.h |  7 +++++++
 virt/kvm/pfncache.c      |  6 ++++++
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 33fddd29824b..8e6fdcd7bb6e 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -261,8 +261,8 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
 	 * alignment (and the 32-bit ABI doesn't align the 64-bit integers
 	 * anyway, even if the overall struct had been 64-bit aligned).
 	 */
-	if ((gpc1->gpa & ~PAGE_MASK) + user_len >= PAGE_SIZE) {
-		user_len1 = PAGE_SIZE - (gpc1->gpa & ~PAGE_MASK);
+	if ((kvm_gpc_gpa(gpc1) & ~PAGE_MASK) + user_len >= PAGE_SIZE) {
+		user_len1 = PAGE_SIZE - (kvm_gpc_gpa(gpc1) & ~PAGE_MASK);
 		user_len2 = user_len - user_len1;
 	} else {
 		user_len1 = user_len;
@@ -343,7 +343,7 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
 			 * to the second page now because the guest changed to
 			 * 64-bit mode, the second GPC won't have been set up.
 			 */
-			if (kvm_gpc_activate(gpc2, gpc1->gpa + user_len1,
+			if (kvm_gpc_activate(gpc2, kvm_gpc_gpa(gpc1) + user_len1,
 					     user_len2))
 				return;
 
@@ -677,7 +677,8 @@ int kvm_xen_hvm_get_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
 
 	case KVM_XEN_ATTR_TYPE_SHARED_INFO:
 		if (kvm->arch.xen.shinfo_cache.active)
-			data->u.shared_info.gfn = gpa_to_gfn(kvm->arch.xen.shinfo_cache.gpa);
+			data->u.shared_info.gfn =
+				gpa_to_gfn(kvm_gpc_gpa(&kvm->arch.xen.shinfo_cache));
 		else
 			data->u.shared_info.gfn = KVM_XEN_INVALID_GFN;
 		r = 0;
@@ -955,7 +956,7 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
 	switch (data->type) {
 	case KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO:
 		if (vcpu->arch.xen.vcpu_info_cache.active)
-			data->u.gpa = vcpu->arch.xen.vcpu_info_cache.gpa;
+			data->u.gpa = kvm_gpc_gpa(&vcpu->arch.xen.vcpu_info_cache);
 		else
 			data->u.gpa = KVM_XEN_INVALID_GPA;
 		r = 0;
@@ -963,7 +964,7 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
 
 	case KVM_XEN_VCPU_ATTR_TYPE_VCPU_TIME_INFO:
 		if (vcpu->arch.xen.vcpu_time_info_cache.active)
-			data->u.gpa = vcpu->arch.xen.vcpu_time_info_cache.gpa;
+			data->u.gpa = kvm_gpc_gpa(&vcpu->arch.xen.vcpu_time_info_cache);
 		else
 			data->u.gpa = KVM_XEN_INVALID_GPA;
 		r = 0;
@@ -975,7 +976,7 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
 			break;
 		}
 		if (vcpu->arch.xen.runstate_cache.active) {
-			data->u.gpa = vcpu->arch.xen.runstate_cache.gpa;
+			data->u.gpa = kvm_gpc_gpa(&vcpu->arch.xen.runstate_cache);
 			r = 0;
 		}
 		break;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index c71e8fbccaaf..4d8027fe9928 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1374,6 +1374,13 @@ void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc);
  */
 void kvm_gpc_mark_dirty(struct gfn_to_pfn_cache *gpc);
 
+/**
+ * kvm_gpc_gpa - retrieve the guest physical address of a cached mapping
+ *
+ * @gpc:	   struct gfn_to_pfn_cache object.
+ */
+gpa_t kvm_gpc_gpa(struct gfn_to_pfn_cache *gpc);
+
 void kvm_sigset_activate(struct kvm_vcpu *vcpu);
 void kvm_sigset_deactivate(struct kvm_vcpu *vcpu);
 
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index b68ed7fa56a2..17afbb464a70 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -386,6 +386,12 @@ int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len)
 }
 EXPORT_SYMBOL_GPL(kvm_gpc_activate);
 
+gpa_t kvm_gpc_gpa(struct gfn_to_pfn_cache *gpc)
+{
+	return gpc->gpa;
+}
+EXPORT_SYMBOL_GPL(kvm_gpc_gpa);
+
 void kvm_gpc_mark_dirty(struct gfn_to_pfn_cache *gpc)
 {
 	mark_page_dirty_in_slot(gpc->kvm, gpc->memslot, gpc->gpa >> PAGE_SHIFT);
-- 
2.39.2

[PATCH v2 04/12] KVM: pfncache: base offset check on khva rather than gpa

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

After a subsequent patch, the gpa may not always be set whereas khva will
(as long as the cache valid flag is also set).

No functional change intended.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: David Woodhouse <dwmw2@infradead.org>
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 virt/kvm/pfncache.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 17afbb464a70..37bcb4399780 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -83,15 +83,18 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
 	if (!gpc->active)
 		return false;
 
-	if ((gpc->gpa & ~PAGE_MASK) + len > PAGE_SIZE)
+	if (gpc->generation != slots->generation)
 		return false;
 
-	if (gpc->generation != slots->generation || kvm_is_error_hva(gpc->uhva))
+	if (kvm_is_error_hva(gpc->uhva))
 		return false;
 
 	if (!gpc->valid)
 		return false;
 
+	if (offset_in_page(gpc->khva) + len > PAGE_SIZE)
+		return false;
+
 	return true;
 }
 EXPORT_SYMBOL_GPL(kvm_gpc_check);
-- 
2.39.2

[PATCH v2 05/12] KVM: pfncache: allow a cache to be activated with a fixed (userspace) HVA

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

Some cached pages may actually be overlays on guest memory that have a
fixed HVA within the VMM. It's pointless to invalidate such cached
mappings if the overlay is moved so allow a cache to be activated directly
with the HVA to cater for such cases. A subsequent patch will make use
of this facility.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 include/linux/kvm_host.h  | 29 ++++++++++++++++
 include/linux/kvm_types.h |  3 +-
 virt/kvm/pfncache.c       | 73 ++++++++++++++++++++++++++++-----------
 3 files changed, 84 insertions(+), 21 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 4d8027fe9928..6823bae5c66c 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1321,6 +1321,22 @@ void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm,
  */
 int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len);
 
+/**
+ * kvm_gpc_activate_hva - prepare a cached kernel mapping and HPA for a given HVA.
+ *
+ * @gpc:	   struct gfn_to_pfn_cache object.
+ * @hva:	   userspace virtual address to map.
+ * @len:	   sanity check; the range being access must fit a single page.
+ *
+ * @return:	   0 for success.
+ *		   -EINVAL for a mapping which would cross a page boundary.
+ *		   -EFAULT for an untranslatable guest physical address.
+ *
+ * The semantics of this function are the same as those of kvm_gpc_activate(). It
+ * merely bypasses a layer of address translation.
+ */
+int kvm_gpc_activate_hva(struct gfn_to_pfn_cache *gpc, unsigned long hva, unsigned long len);
+
 /**
  * kvm_gpc_check - check validity of a gfn_to_pfn_cache.
  *
@@ -1378,9 +1394,22 @@ void kvm_gpc_mark_dirty(struct gfn_to_pfn_cache *gpc);
  * kvm_gpc_gpa - retrieve the guest physical address of a cached mapping
  *
  * @gpc:	   struct gfn_to_pfn_cache object.
+ *
+ * @return:	   If the cache was activated with a fixed HVA then INVALID_GPA
+ *		   will be returned.
  */
 gpa_t kvm_gpc_gpa(struct gfn_to_pfn_cache *gpc);
 
+/**
+ * kvm_gpc_hva - retrieve the fixed host physical address of a cached mapping
+ *
+ * @gpc:	   struct gfn_to_pfn_cache object.
+ *
+ * @return:	   If the cache was activated with a guest physical address then
+ *		   0 will be returned.
+ */
+unsigned long kvm_gpc_hva(struct gfn_to_pfn_cache *gpc);
+
 void kvm_sigset_activate(struct kvm_vcpu *vcpu);
 void kvm_sigset_deactivate(struct kvm_vcpu *vcpu);
 
diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
index 6f4737d5046a..d49946ee7ae3 100644
--- a/include/linux/kvm_types.h
+++ b/include/linux/kvm_types.h
@@ -64,7 +64,7 @@ struct gfn_to_hva_cache {
 
 struct gfn_to_pfn_cache {
 	u64 generation;
-	gpa_t gpa;
+	u64 addr;
 	unsigned long uhva;
 	struct kvm_memory_slot *memslot;
 	struct kvm *kvm;
@@ -77,6 +77,7 @@ struct gfn_to_pfn_cache {
 	enum pfn_cache_usage usage;
 	bool active;
 	bool valid;
+	bool addr_is_gpa;
 };
 
 #ifdef KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 37bcb4399780..b3e3f7e38410 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -83,7 +83,7 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
 	if (!gpc->active)
 		return false;
 
-	if (gpc->generation != slots->generation)
+	if (gpc->addr_is_gpa && gpc->generation != slots->generation)
 		return false;
 
 	if (kvm_is_error_hva(gpc->uhva))
@@ -229,7 +229,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
 
 	gpc->valid = true;
 	gpc->pfn = new_pfn;
-	gpc->khva = new_khva + (gpc->gpa & ~PAGE_MASK);
+	gpc->khva = new_khva + (gpc->addr & ~PAGE_MASK);
 
 	/*
 	 * Put the reference to the _new_ pfn.  The pfn is now tracked by the
@@ -246,11 +246,11 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc)
 	return -EFAULT;
 }
 
-static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
-			     unsigned long len)
+static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, u64 addr,
+			     unsigned long len, bool addr_is_gpa)
 {
 	struct kvm_memslots *slots = kvm_memslots(gpc->kvm);
-	unsigned long page_offset = gpa & ~PAGE_MASK;
+	unsigned long page_offset = addr & ~PAGE_MASK;
 	bool unmap_old = false;
 	unsigned long old_uhva;
 	kvm_pfn_t old_pfn;
@@ -282,22 +282,34 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
 	old_khva = gpc->khva - offset_in_page(gpc->khva);
 	old_uhva = gpc->uhva;
 
-	/* If the userspace HVA is invalid, refresh that first */
-	if (gpc->gpa != gpa || gpc->generation != slots->generation ||
+	/*
+	 * If the address has changed, switched from guest to host (or vice
+	 * versa), or it's a guest address and the memory slots have been
+	 * updated, we need to refresh the userspace HVA.
+	 */
+	if (gpc->addr != addr ||
+	    gpc->addr_is_gpa != addr_is_gpa ||
+	    (addr_is_gpa && gpc->generation != slots->generation) ||
 	    kvm_is_error_hva(gpc->uhva)) {
-		gfn_t gfn = gpa_to_gfn(gpa);
+		gpc->addr = addr;
+		gpc->addr_is_gpa = addr_is_gpa;
 
-		gpc->gpa = gpa;
-		gpc->generation = slots->generation;
-		gpc->memslot = __gfn_to_memslot(slots, gfn);
-		gpc->uhva = gfn_to_hva_memslot(gpc->memslot, gfn);
+		if (addr_is_gpa) {
+			gfn_t gfn = gpa_to_gfn(addr);
 
-		if (kvm_is_error_hva(gpc->uhva)) {
-			ret = -EFAULT;
-			goto out;
+			gpc->generation = slots->generation;
+			gpc->memslot = __gfn_to_memslot(slots, gfn);
+			gpc->uhva = gfn_to_hva_memslot(gpc->memslot, gfn);
+		} else {
+			gpc->uhva = addr & PAGE_MASK;
 		}
 	}
 
+	if (kvm_is_error_hva(gpc->uhva)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
 	/*
 	 * If the userspace HVA changed or the PFN was already invalid,
 	 * drop the lock and do the HVA to PFN lookup again.
@@ -343,7 +355,7 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa,
 
 int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len)
 {
-	return __kvm_gpc_refresh(gpc, gpc->gpa, len);
+	return __kvm_gpc_refresh(gpc, gpc->addr, len, gpc->addr_is_gpa);
 }
 EXPORT_SYMBOL_GPL(kvm_gpc_refresh);
 
@@ -364,7 +376,8 @@ void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm,
 }
 EXPORT_SYMBOL_GPL(kvm_gpc_init);
 
-int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len)
+static int __kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, u64 addr, unsigned long len,
+			      bool addr_is_gpa)
 {
 	struct kvm *kvm = gpc->kvm;
 
@@ -385,19 +398,39 @@ int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len)
 		gpc->active = true;
 		write_unlock_irq(&gpc->lock);
 	}
-	return __kvm_gpc_refresh(gpc, gpa, len);
+	return __kvm_gpc_refresh(gpc, addr, len, addr_is_gpa);
+}
+
+int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len)
+{
+	return __kvm_gpc_activate(gpc, gpa, len, true);
 }
 EXPORT_SYMBOL_GPL(kvm_gpc_activate);
 
 gpa_t kvm_gpc_gpa(struct gfn_to_pfn_cache *gpc)
 {
-	return gpc->gpa;
+	return gpc->addr_is_gpa ? gpc->addr : INVALID_GPA;
 }
 EXPORT_SYMBOL_GPL(kvm_gpc_gpa);
 
+int kvm_gpc_activate_hva(struct gfn_to_pfn_cache *gpc, unsigned long hva, unsigned long len)
+{
+	return __kvm_gpc_activate(gpc, hva, len, false);
+}
+EXPORT_SYMBOL_GPL(kvm_gpc_activate_hva);
+
+unsigned long kvm_gpc_hva(struct gfn_to_pfn_cache *gpc)
+{
+	return !gpc->addr_is_gpa ? gpc->addr : 0;
+}
+EXPORT_SYMBOL_GPL(kvm_gpc_hva);
+
 void kvm_gpc_mark_dirty(struct gfn_to_pfn_cache *gpc)
 {
-	mark_page_dirty_in_slot(gpc->kvm, gpc->memslot, gpc->gpa >> PAGE_SHIFT);
+	if (!gpc->addr_is_gpa)
+		return;
+
+	mark_page_dirty_in_slot(gpc->kvm, gpc->memslot, gpc->addr >> PAGE_SHIFT);
 }
 EXPORT_SYMBOL_GPL(kvm_gpc_mark_dirty);
 
-- 
2.39.2

Re: [PATCH v2 05/12] KVM: pfncache: allow a cache to be activated with a fixed (userspace) HVA

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 762 bytes --]

On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> Some cached pages may actually be overlays on guest memory that have a
> fixed HVA within the VMM. It's pointless to invalidate such cached
> mappings if the overlay is moved so allow a cache to be activated directly
> with the HVA to cater for such cases. A subsequent patch will make use
> of this facility.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>

Btw, I think you have falsified some Reviewed-by: tags on the rest of
the series. Remember, if they aren't literally cut and pasted, the
magic gets lost. Just the same as Signed-off-by: tags. Never type them
for someone else.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v2 05/12] KVM: pfncache: allow a cache to be activated with a fixed (userspace) HVA

[Paul Durrant]

On 18/09/2023 12:34, David Woodhouse wrote:
> On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
>> From: Paul Durrant <pdurrant@amazon.com>
>>
>> Some cached pages may actually be overlays on guest memory that have a
>> fixed HVA within the VMM. It's pointless to invalidate such cached
>> mappings if the overlay is moved so allow a cache to be activated directly
>> with the HVA to cater for such cases. A subsequent patch will make use
>> of this facility.
>>
>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> 
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> 
> Btw, I think you have falsified some Reviewed-by: tags on the rest of
> the series. Remember, if they aren't literally cut and pasted, the
> magic gets lost. Just the same as Signed-off-by: tags. Never type them
> for someone else.

Indeed. They were all copied and pasted.

   Paul

Re: [PATCH v2 05/12] KVM: pfncache: allow a cache to be activated with a fixed (userspace) HVA

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

On Mon, 2023-09-18 at 13:11 +0100, Paul Durrant wrote:
> On 18/09/2023 12:34, David Woodhouse wrote:
> > On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
> > > From: Paul Durrant <pdurrant@amazon.com>
> > > 
> > > Some cached pages may actually be overlays on guest memory that have a
> > > fixed HVA within the VMM. It's pointless to invalidate such cached
> > > mappings if the overlay is moved so allow a cache to be activated directly
> > > with the HVA to cater for such cases. A subsequent patch will make use
> > > of this facility.
> > > 
> > > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > 
> > Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > Btw, I think you have falsified some Reviewed-by: tags on the rest of
> > the series. Remember, if they aren't literally cut and pasted, the
> > magic gets lost. Just the same as Signed-off-by: tags. Never type them
> > for someone else.
> 
> Indeed. They were all copied and pasted.

I'm prepared to believe my fingers betrayed me and autocompleted
@infradead.org for one or two of them when I meant to use the @amazon
address, but surely not *all* of them that I reviewed last time round?

:)

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v2 05/12] KVM: pfncache: allow a cache to be activated with a fixed (userspace) HVA

[Paul Durrant]

On 18/09/2023 13:59, David Woodhouse wrote:
> On Mon, 2023-09-18 at 13:11 +0100, Paul Durrant wrote:
>> On 18/09/2023 12:34, David Woodhouse wrote:
>>> On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
>>>> From: Paul Durrant <pdurrant@amazon.com>
>>>>
>>>> Some cached pages may actually be overlays on guest memory that have a
>>>> fixed HVA within the VMM. It's pointless to invalidate such cached
>>>> mappings if the overlay is moved so allow a cache to be activated directly
>>>> with the HVA to cater for such cases. A subsequent patch will make use
>>>> of this facility.
>>>>
>>>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>>>
>>> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
>>>
>>> Btw, I think you have falsified some Reviewed-by: tags on the rest of
>>> the series. Remember, if they aren't literally cut and pasted, the
>>> magic gets lost. Just the same as Signed-off-by: tags. Never type them
>>> for someone else.
>>
>> Indeed. They were all copied and pasted.
> 
> I'm prepared to believe my fingers betrayed me and autocompleted
> @infradead.org for one or two of them when I meant to use the @amazon
> address, but surely not *all* of them that I reviewed last time round?
> 

Hmm. I guess I must have cut'n'pasted from the wrong place then. I'll be 
more careful.

   Paul

[PATCH v2 06/12] KVM: xen: allow shared_info to be mapped by fixed HVA

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

The shared_info page is not guest memory as such. It is a dedicated page
allocated by the VMM and overlaid onto guest memory in a GFN chosen by the
guest. The guest may even request that shared_info be moved from one GFN
to another, but the HVA is never going to change. Thus it makes much more
sense to map the shared_info page in kernel once using this fixed HVA.
Hence add a new KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA attribute type for this
purpose and a KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA flag to advertize its
availability. Don't actually advertize it yet though. That will be done in
a subsequent patch, which will also add tests for the new attribute type.

Also update the KVM API documentation with the new attribute and also fix
it up to consistently refer to 'shared_info' (with the underscore).

NOTE: The change of the kvm_xen_hvm_attr shared_info from struct to union
      is technically an ABI change but it's entirely compatible with
      existing users.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: David Woodhouse <dwmw2@infradead.org>
---
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org

v2:
 - Define the new attribute and capability but don't advertize the
   capability yet.
 - Add API documentation.
---
 Documentation/virt/kvm/api.rst | 25 +++++++++++++++++++------
 arch/x86/kvm/xen.c             | 28 ++++++++++++++++++++++------
 include/uapi/linux/kvm.h       |  6 +++++-
 3 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 21a7578142a1..e9df4df6fe48 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -353,7 +353,7 @@ The bits in the dirty bitmap are cleared before the ioctl returns, unless
 KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 is enabled.  For more information,
 see the description of the capability.
 
-Note that the Xen shared info page, if configured, shall always be assumed
+Note that the Xen shared_info page, if configured, shall always be assumed
 to be dirty. KVM will not explicitly mark it such.
 
 
@@ -5408,8 +5408,9 @@ KVM_PV_ASYNC_CLEANUP_PERFORM
 		__u8 long_mode;
 		__u8 vector;
 		__u8 runstate_update_flag;
-		struct {
+		union {
 			__u64 gfn;
+			__u64 hva;
 		} shared_info;
 		struct {
 			__u32 send_port;
@@ -5437,10 +5438,10 @@ type values:
 
 KVM_XEN_ATTR_TYPE_LONG_MODE
   Sets the ABI mode of the VM to 32-bit or 64-bit (long mode). This
-  determines the layout of the shared info pages exposed to the VM.
+  determines the layout of the shared_info page exposed to the VM.
 
 KVM_XEN_ATTR_TYPE_SHARED_INFO
-  Sets the guest physical frame number at which the Xen "shared info"
+  Sets the guest physical frame number at which the Xen shared_info
   page resides. Note that although Xen places vcpu_info for the first
   32 vCPUs in the shared_info page, KVM does not automatically do so
   and instead requires that KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO be used
@@ -5449,7 +5450,7 @@ KVM_XEN_ATTR_TYPE_SHARED_INFO
   not be aware of the Xen CPU id which is used as the index into the
   vcpu_info[] array, so may know the correct default location.
 
-  Note that the shared info page may be constantly written to by KVM;
+  Note that the shared_info page may be constantly written to by KVM;
   it contains the event channel bitmap used to deliver interrupts to
   a Xen guest, amongst other things. It is exempt from dirty tracking
   mechanisms — KVM will not explicitly mark the page as dirty each
@@ -5458,9 +5459,21 @@ KVM_XEN_ATTR_TYPE_SHARED_INFO
   any vCPU has been running or any event channel interrupts can be
   routed to the guest.
 
-  Setting the gfn to KVM_XEN_INVALID_GFN will disable the shared info
+  Setting the gfn to KVM_XEN_INVALID_GFN will disable the shared_info
   page.
 
+KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA
+  If the KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA flag is also set in the
+  Xen capabilities, then this attribute may be used to set the
+  userspace address at which the shared_info page resides, which
+  will always be fixed in the VMM regardless of where it is mapped
+  in guest physical address space. This attribute should be used in
+  preference to KVM_XEN_ATTR_TYPE_SHARED_INFO as it avoids
+  unnecessary invalidation of an internal cache when the page is
+  re-mapped in guest physcial address space.
+
+  Setting the hva to zero will disable the shared_info page.
+
 KVM_XEN_ATTR_TYPE_UPCALL_VECTOR
   Sets the exception vector used to deliver Xen event channel upcalls.
   This is the HVM-wide vector injected directly by the hypervisor
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 8e6fdcd7bb6e..1abb4547642a 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -34,24 +34,27 @@ static bool kvm_xen_hcall_evtchn_send(struct kvm_vcpu *vcpu, u64 param, u64 *r);
 
 DEFINE_STATIC_KEY_DEFERRED_FALSE(kvm_xen_enabled, HZ);
 
-static int kvm_xen_shared_info_init(struct kvm *kvm, gfn_t gfn)
+static int kvm_xen_shared_info_init(struct kvm *kvm, u64 addr, bool addr_is_gfn)
 {
 	struct gfn_to_pfn_cache *gpc = &kvm->arch.xen.shinfo_cache;
 	struct pvclock_wall_clock *wc;
-	gpa_t gpa = gfn_to_gpa(gfn);
 	u32 *wc_sec_hi;
 	u32 wc_version;
 	u64 wall_nsec;
 	int ret = 0;
 	int idx = srcu_read_lock(&kvm->srcu);
 
-	if (gfn == KVM_XEN_INVALID_GFN) {
+	if ((addr_is_gfn && addr == KVM_XEN_INVALID_GFN) ||
+	    (!addr_is_gfn && addr == 0)) {
 		kvm_gpc_deactivate(gpc);
 		goto out;
 	}
 
 	do {
-		ret = kvm_gpc_activate(gpc, gpa, PAGE_SIZE);
+		if (addr_is_gfn)
+			ret = kvm_gpc_activate(gpc, gfn_to_gpa(addr), PAGE_SIZE);
+		else
+			ret = kvm_gpc_activate_hva(gpc, addr, PAGE_SIZE);
 		if (ret)
 			goto out;
 
@@ -604,7 +607,6 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
 {
 	int r = -ENOENT;
 
-
 	switch (data->type) {
 	case KVM_XEN_ATTR_TYPE_LONG_MODE:
 		if (!IS_ENABLED(CONFIG_64BIT) && data->u.long_mode) {
@@ -619,7 +621,13 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
 
 	case KVM_XEN_ATTR_TYPE_SHARED_INFO:
 		mutex_lock(&kvm->arch.xen.xen_lock);
-		r = kvm_xen_shared_info_init(kvm, data->u.shared_info.gfn);
+		r = kvm_xen_shared_info_init(kvm, data->u.shared_info.gfn, true);
+		mutex_unlock(&kvm->arch.xen.xen_lock);
+		break;
+
+	case KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA:
+		mutex_lock(&kvm->arch.xen.xen_lock);
+		r = kvm_xen_shared_info_init(kvm, data->u.shared_info.hva, false);
 		mutex_unlock(&kvm->arch.xen.xen_lock);
 		break;
 
@@ -684,6 +692,14 @@ int kvm_xen_hvm_get_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
 		r = 0;
 		break;
 
+	case KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA:
+		if (kvm->arch.xen.shinfo_cache.active)
+			data->u.shared_info.hva = kvm_gpc_hva(&kvm->arch.xen.shinfo_cache);
+		else
+			data->u.shared_info.hva = 0;
+		r = 0;
+		break;
+
 	case KVM_XEN_ATTR_TYPE_UPCALL_VECTOR:
 		data->u.vector = kvm->arch.xen.upcall_vector;
 		r = 0;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 13065dd96132..062bfa14b4d9 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1282,6 +1282,7 @@ struct kvm_x86_mce {
 #define KVM_XEN_HVM_CONFIG_EVTCHN_2LEVEL	(1 << 4)
 #define KVM_XEN_HVM_CONFIG_EVTCHN_SEND		(1 << 5)
 #define KVM_XEN_HVM_CONFIG_RUNSTATE_UPDATE_FLAG	(1 << 6)
+#define KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA	(1 << 7)
 
 struct kvm_xen_hvm_config {
 	__u32 flags;
@@ -1793,9 +1794,10 @@ struct kvm_xen_hvm_attr {
 		__u8 long_mode;
 		__u8 vector;
 		__u8 runstate_update_flag;
-		struct {
+		union {
 			__u64 gfn;
 #define KVM_XEN_INVALID_GFN ((__u64)-1)
+			__u64 hva;
 		} shared_info;
 		struct {
 			__u32 send_port;
@@ -1837,6 +1839,8 @@ struct kvm_xen_hvm_attr {
 #define KVM_XEN_ATTR_TYPE_XEN_VERSION		0x4
 /* Available with KVM_CAP_XEN_HVM / KVM_XEN_HVM_CONFIG_RUNSTATE_UPDATE_FLAG */
 #define KVM_XEN_ATTR_TYPE_RUNSTATE_UPDATE_FLAG	0x5
+/* Available with KVM_CAP_XEN_HVM / KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA */
+#define KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA	0x6
 
 /* Per-vCPU Xen attributes */
 #define KVM_XEN_VCPU_GET_ATTR	_IOWR(KVMIO, 0xca, struct kvm_xen_vcpu_attr)
-- 
2.39.2

[PATCH v2 07/12] KVM: xen: prepare for using 'default' vcpu_info

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

The shared_info page contains an array of 32 vcpu_info structures
which may be used by guests (with fewer than 32 vCPUs) if they don't
explicitly register vcpu_info structures in their own memory, using
a VCPUOP_register_vcpu_info hypercall.
Currently we rely on the VMM always registering vcpu_info structures,
even if the guest doesn't make that hypercall, which is somewhat
bogus as (as has been stated in the comment of a previous commit)
the shared_info page is not guest memory.
Prepare to automatically use the vcpu_info info embedded in shared_info
by default, by adding a get_vcpu_info_cache() helper function. This
function also passes back an offset to be added to the cached khva.
This is currently always zero since we're still relying on the
current VMM behaviour. A subsequent patch will make proper use of
it.

No functional change intended.

NOTE: To avoid leaking detail of the vcpu_info duality into the main
      x86 code, a kvm_xen_guest_time_update() has also been added
      and use of this requires that kvm_setup_guest_pvclock() ceases
      to be a static function.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: David Woodhouse <dwmw2@infradead.org>
---
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org

v2:
 - Fix build warning.
---
 arch/x86/include/asm/kvm_host.h |  4 +++
 arch/x86/kvm/x86.c              | 12 +++-----
 arch/x86/kvm/xen.c              | 50 ++++++++++++++++++++++++++-------
 arch/x86/kvm/xen.h              |  6 +++-
 4 files changed, 53 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 1a4def36d5bb..6d896f9161c2 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2238,4 +2238,8 @@ int memslot_rmap_alloc(struct kvm_memory_slot *slot, unsigned long npages);
  */
 #define KVM_EXIT_HYPERCALL_MBZ		GENMASK_ULL(31, 1)
 
+void kvm_setup_guest_pvclock(struct kvm_vcpu *v,
+			     struct gfn_to_pfn_cache *gpc,
+			     unsigned int offset);
+
 #endif /* _ASM_X86_KVM_HOST_H */
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d669a8801265..2d7d0ae18820 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3094,9 +3094,9 @@ u64 get_kvmclock_ns(struct kvm *kvm)
 	return data.clock;
 }
 
-static void kvm_setup_guest_pvclock(struct kvm_vcpu *v,
-				    struct gfn_to_pfn_cache *gpc,
-				    unsigned int offset)
+void kvm_setup_guest_pvclock(struct kvm_vcpu *v,
+			     struct gfn_to_pfn_cache *gpc,
+			     unsigned int offset)
 {
 	struct kvm_vcpu_arch *vcpu = &v->arch;
 	struct pvclock_vcpu_time_info *guest_hv_clock;
@@ -3232,11 +3232,7 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
 
 	if (vcpu->pv_time.active)
 		kvm_setup_guest_pvclock(v, &vcpu->pv_time, 0);
-	if (vcpu->xen.vcpu_info_cache.active)
-		kvm_setup_guest_pvclock(v, &vcpu->xen.vcpu_info_cache,
-					offsetof(struct compat_vcpu_info, time));
-	if (vcpu->xen.vcpu_time_info_cache.active)
-		kvm_setup_guest_pvclock(v, &vcpu->xen.vcpu_time_info_cache, 0);
+	kvm_xen_guest_time_update(v);
 	kvm_hv_setup_tsc_page(v->kvm, &vcpu->hv_clock);
 	return 0;
 }
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 1abb4547642a..7fc4fc2e54d8 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -489,6 +489,29 @@ static void kvm_xen_inject_vcpu_vector(struct kvm_vcpu *v)
 	WARN_ON_ONCE(!kvm_irq_delivery_to_apic_fast(v->kvm, NULL, &irq, &r, NULL));
 }
 
+static struct gfn_to_pfn_cache *get_vcpu_info_cache(struct kvm_vcpu *v, unsigned long *offset)
+{
+	if (offset)
+		*offset = 0;
+
+	return &v->arch.xen.vcpu_info_cache;
+}
+
+void kvm_xen_guest_time_update(struct kvm_vcpu *v)
+{
+	unsigned long offset;
+	struct gfn_to_pfn_cache *gpc = get_vcpu_info_cache(v, &offset);
+
+	BUILD_BUG_ON(offsetof(struct vcpu_info, time) !=
+		     offsetof(struct compat_vcpu_info, time));
+
+	if (gpc->active)
+		kvm_setup_guest_pvclock(v, gpc, offset + offsetof(struct compat_vcpu_info, time));
+
+	if (v->arch.xen.vcpu_time_info_cache.active)
+		kvm_setup_guest_pvclock(v, &v->arch.xen.vcpu_time_info_cache, 0);
+}
+
 /*
  * On event channel delivery, the vcpu_info may not have been accessible.
  * In that case, there are bits in vcpu->arch.xen.evtchn_pending_sel which
@@ -499,7 +522,8 @@ static void kvm_xen_inject_vcpu_vector(struct kvm_vcpu *v)
 void kvm_xen_inject_pending_events(struct kvm_vcpu *v)
 {
 	unsigned long evtchn_pending_sel = READ_ONCE(v->arch.xen.evtchn_pending_sel);
-	struct gfn_to_pfn_cache *gpc = &v->arch.xen.vcpu_info_cache;
+	unsigned long offset;
+	struct gfn_to_pfn_cache *gpc = get_vcpu_info_cache(v, &offset);
 	unsigned long flags;
 
 	if (!evtchn_pending_sel)
@@ -522,7 +546,7 @@ void kvm_xen_inject_pending_events(struct kvm_vcpu *v)
 
 	/* Now gpc->khva is a valid kernel address for the vcpu_info */
 	if (IS_ENABLED(CONFIG_64BIT) && v->kvm->arch.xen.long_mode) {
-		struct vcpu_info *vi = gpc->khva;
+		struct vcpu_info *vi = gpc->khva + offset;
 
 		asm volatile(LOCK_PREFIX "orq %0, %1\n"
 			     "notq %0\n"
@@ -534,7 +558,7 @@ void kvm_xen_inject_pending_events(struct kvm_vcpu *v)
 		WRITE_ONCE(vi->evtchn_upcall_pending, 1);
 	} else {
 		u32 evtchn_pending_sel32 = evtchn_pending_sel;
-		struct compat_vcpu_info *vi = gpc->khva;
+		struct compat_vcpu_info *vi = gpc->khva + offset;
 
 		asm volatile(LOCK_PREFIX "orl %0, %1\n"
 			     "notl %0\n"
@@ -556,7 +580,8 @@ void kvm_xen_inject_pending_events(struct kvm_vcpu *v)
 
 int __kvm_xen_has_interrupt(struct kvm_vcpu *v)
 {
-	struct gfn_to_pfn_cache *gpc = &v->arch.xen.vcpu_info_cache;
+	unsigned long offset;
+	struct gfn_to_pfn_cache *gpc = get_vcpu_info_cache(v, &offset);
 	unsigned long flags;
 	u8 rc = 0;
 
@@ -598,7 +623,7 @@ int __kvm_xen_has_interrupt(struct kvm_vcpu *v)
 		read_lock_irqsave(&gpc->lock, flags);
 	}
 
-	rc = ((struct vcpu_info *)gpc->khva)->evtchn_upcall_pending;
+	rc = ((struct vcpu_info *)(gpc->khva + offset))->evtchn_upcall_pending;
 	read_unlock_irqrestore(&gpc->lock, flags);
 	return rc;
 }
@@ -1567,7 +1592,7 @@ static void kvm_xen_check_poller(struct kvm_vcpu *vcpu, int port)
  */
 int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm)
 {
-	struct gfn_to_pfn_cache *gpc = &kvm->arch.xen.shinfo_cache;
+	struct gfn_to_pfn_cache *gpc;
 	struct kvm_vcpu *vcpu;
 	unsigned long *pending_bits, *mask_bits;
 	unsigned long flags;
@@ -1585,7 +1610,8 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm)
 		WRITE_ONCE(xe->vcpu_idx, vcpu->vcpu_idx);
 	}
 
-	if (!vcpu->arch.xen.vcpu_info_cache.active)
+	gpc = get_vcpu_info_cache(vcpu, NULL);
+	if (!gpc->active)
 		return -EINVAL;
 
 	if (xe->port >= max_evtchn_port(kvm))
@@ -1594,6 +1620,7 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm)
 	rc = -EWOULDBLOCK;
 
 	idx = srcu_read_lock(&kvm->srcu);
+	gpc = &kvm->arch.xen.shinfo_cache;
 
 	read_lock_irqsave(&gpc->lock, flags);
 	if (!kvm_gpc_check(gpc, PAGE_SIZE))
@@ -1624,10 +1651,13 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm)
 		rc = -ENOTCONN; /* Masked */
 		kvm_xen_check_poller(vcpu, xe->port);
 	} else {
+		unsigned long offset;
+
 		rc = 1; /* Delivered to the bitmap in shared_info. */
+
 		/* Now switch to the vCPU's vcpu_info to set the index and pending_sel */
 		read_unlock_irqrestore(&gpc->lock, flags);
-		gpc = &vcpu->arch.xen.vcpu_info_cache;
+		gpc = get_vcpu_info_cache(vcpu, &offset);
 
 		read_lock_irqsave(&gpc->lock, flags);
 		if (!kvm_gpc_check(gpc, sizeof(struct vcpu_info))) {
@@ -1641,13 +1671,13 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe, struct kvm *kvm)
 		}
 
 		if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode) {
-			struct vcpu_info *vcpu_info = gpc->khva;
+			struct vcpu_info *vcpu_info = gpc->khva + offset;
 			if (!test_and_set_bit(port_word_bit, &vcpu_info->evtchn_pending_sel)) {
 				WRITE_ONCE(vcpu_info->evtchn_upcall_pending, 1);
 				kick_vcpu = true;
 			}
 		} else {
-			struct compat_vcpu_info *vcpu_info = gpc->khva;
+			struct compat_vcpu_info *vcpu_info = gpc->khva + offset;
 			if (!test_and_set_bit(port_word_bit,
 					      (unsigned long *)&vcpu_info->evtchn_pending_sel)) {
 				WRITE_ONCE(vcpu_info->evtchn_upcall_pending, 1);
diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h
index f8f1fe22d090..c4d29ccbc3ab 100644
--- a/arch/x86/kvm/xen.h
+++ b/arch/x86/kvm/xen.h
@@ -16,6 +16,7 @@
 
 extern struct static_key_false_deferred kvm_xen_enabled;
 
+void kvm_xen_guest_time_update(struct kvm_vcpu *vcpu);
 int __kvm_xen_has_interrupt(struct kvm_vcpu *vcpu);
 void kvm_xen_inject_pending_events(struct kvm_vcpu *vcpu);
 int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data);
@@ -52,7 +53,6 @@ static inline bool kvm_xen_hypercall_enabled(struct kvm *kvm)
 static inline int kvm_xen_has_interrupt(struct kvm_vcpu *vcpu)
 {
 	if (static_branch_unlikely(&kvm_xen_enabled.key) &&
-	    vcpu->arch.xen.vcpu_info_cache.active &&
 	    vcpu->kvm->arch.xen.upcall_vector)
 		return __kvm_xen_has_interrupt(vcpu);
 
@@ -80,6 +80,10 @@ static inline int kvm_xen_has_pending_timer(struct kvm_vcpu *vcpu)
 
 void kvm_xen_inject_timer_irqs(struct kvm_vcpu *vcpu);
 #else
+static inline void kvm_xen_guest_time_update(struct kvm_vcpu *vcpu)
+{
+}
+
 static inline int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
 {
 	return 1;
-- 
2.39.2

[PATCH v2 08/12] KVM: xen: automatically use the vcpu_info embedded in shared_info

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

The VMM should only need to set the KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO
attribute in response to a VCPUOP_register_vcpu_info hypercall. We can
handle the default case internally since we already know where the
shared_info page is. Modify get_vcpu_info_cache() to pass back a pointer
to the shared info pfn cache (and appropriate offset) for any of the
first 32 vCPUs if the attribute has not been set.

A VMM will be able to determine whether it needs to set up default
vcpu_info using the previously defined KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA
Xen capability flag, which will be advertized in a subsequent patch.

Also update the KVM API documentation to describe the new behaviour.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: x86@kernel.org

v2:
 - Dispense with the KVM_XEN_HVM_CONFIG_DEFAULT_VCPU_INFO capability.
 - Add API documentation.
---
 Documentation/virt/kvm/api.rst | 16 +++++++++-------
 arch/x86/kvm/xen.c             | 15 +++++++++++++++
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index e9df4df6fe48..54f5d7392fe8 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -5442,13 +5442,7 @@ KVM_XEN_ATTR_TYPE_LONG_MODE
 
 KVM_XEN_ATTR_TYPE_SHARED_INFO
   Sets the guest physical frame number at which the Xen shared_info
-  page resides. Note that although Xen places vcpu_info for the first
-  32 vCPUs in the shared_info page, KVM does not automatically do so
-  and instead requires that KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO be used
-  explicitly even when the vcpu_info for a given vCPU resides at the
-  "default" location in the shared_info page. This is because KVM may
-  not be aware of the Xen CPU id which is used as the index into the
-  vcpu_info[] array, so may know the correct default location.
+  page resides.
 
   Note that the shared_info page may be constantly written to by KVM;
   it contains the event channel bitmap used to deliver interrupts to
@@ -5564,6 +5558,14 @@ type values:
 
 KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO
   Sets the guest physical address of the vcpu_info for a given vCPU.
+  The vcpu_info for the first 32 vCPUs defaults to the structures
+  embedded in the shared_info page.
+
+  If the KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA flag is also set in the
+  Xen capabilities then the VMM is not required to set this default
+  location; KVM will handle that internally. Otherwise this attribute
+  must be set for all vCPUs.
+
   As with the shared_info page for the VM, the corresponding page may be
   dirtied at any time if event channel interrupt delivery is enabled, so
   userspace should always assume that the page is dirty without relying
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 7fc4fc2e54d8..7cb5f8b55205 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -491,6 +491,21 @@ static void kvm_xen_inject_vcpu_vector(struct kvm_vcpu *v)
 
 static struct gfn_to_pfn_cache *get_vcpu_info_cache(struct kvm_vcpu *v, unsigned long *offset)
 {
+	if (!v->arch.xen.vcpu_info_cache.active && v->arch.xen.vcpu_id < MAX_VIRT_CPUS) {
+		struct kvm *kvm = v->kvm;
+
+		if (offset) {
+			if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode)
+				*offset = offsetof(struct shared_info,
+						   vcpu_info[v->arch.xen.vcpu_id]);
+			else
+				*offset = offsetof(struct compat_shared_info,
+						   vcpu_info[v->arch.xen.vcpu_id]);
+		}
+
+		return &kvm->arch.xen.shinfo_cache;
+	}
+
 	if (offset)
 		*offset = 0;
 
-- 
2.39.2

Re: [PATCH v2 08/12] KVM: xen: automatically use the vcpu_info embedded in shared_info

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]

On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
> -                                              This is because KVM may
> -  not be aware of the Xen CPU id which is used as the index into the
> -  vcpu_info[] array, so may know the correct default location.

Hm, that *was* true at the time of writing, but we did end up teaching
KVM about the Xen vcpu_id so that we can handle timers. (We required
userspace to provide the APIC ID for all the other event channel stuff,
but timer hypercalls come straight from the guest).

But I think the *only* thing we use vcpu->arch.xen.vcpu_id for right
now is acceleration of the timer hypercalls that are restricted to the
vCPU that they're called from, e.g.:

	case VCPUOP_set_singleshot_timer:
		if (vcpu->arch.xen.vcpu_id != vcpu_id) {
			*r = -EINVAL;

So it's never mattered much before now if the Xen vcpu_id changes at
runtime.

Maybe you now want to invalidate the vcpu_info pfncache for a vCPU if
its Xen vcpu_id changes? Or just *forbid* such a change after the
shinfo page has been set up? What locking prevents the xen.vcpu_id
changing in the middle of your new get_vcpu_info_cache() function?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v2 08/12] KVM: xen: automatically use the vcpu_info embedded in shared_info

[Paul Durrant]

On 18/09/2023 12:49, David Woodhouse wrote:
> On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
>> -                                              This is because KVM may
>> -  not be aware of the Xen CPU id which is used as the index into the
>> -  vcpu_info[] array, so may know the correct default location.
> 
> Hm, that *was* true at the time of writing, but we did end up teaching
> KVM about the Xen vcpu_id so that we can handle timers. (We required
> userspace to provide the APIC ID for all the other event channel stuff,
> but timer hypercalls come straight from the guest).
> 
> But I think the *only* thing we use vcpu->arch.xen.vcpu_id for right
> now is acceleration of the timer hypercalls that are restricted to the
> vCPU that they're called from, e.g.:
> 
> 	case VCPUOP_set_singleshot_timer:
> 		if (vcpu->arch.xen.vcpu_id != vcpu_id) {
> 			*r = -EINVAL;
> 
> So it's never mattered much before now if the Xen vcpu_id changes at
> runtime.
> 
> Maybe you now want to invalidate the vcpu_info pfncache for a vCPU if
> its Xen vcpu_id changes? Or just *forbid* such a change after the
> shinfo page has been set up? What locking prevents the xen.vcpu_id
> changing in the middle of your new get_vcpu_info_cache() function?

That's a good point; the vcpu lock itself won't be enough. I think 
forbidding a change of id while a shared_info page is in place is 
probably the most sensible semantic.

   Paul

[PATCH v2 09/12] KVM: selftests / xen: set KVM_XEN_VCPU_ATTR_TYPE_VCPU_ID

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

If the capability (KVM_XEN_HVM_CONFIG_EVTCHN_SEND) is present then set
the guest's vCPU id to match the chosen vcpu_info offset.

Also make some cosmetic fixes to the code for clarity.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>

v2:
 - New in this version.
---
 .../selftests/kvm/x86_64/xen_shinfo_test.c    | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
index 05898ad9f4d9..49d0c91ee078 100644
--- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
+++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
@@ -38,6 +38,8 @@
 #define VCPU_INFO_VADDR	(SHINFO_REGION_GVA + 0x40)
 #define RUNSTATE_VADDR	(SHINFO_REGION_GVA + PAGE_SIZE + PAGE_SIZE - 15)
 
+#define VCPU_ID		1 /* Must correspond to offset of VCPU_INFO_[V]ADDR */
+
 #define EVTCHN_VECTOR	0x10
 
 #define EVTCHN_TEST1 15
@@ -410,7 +412,7 @@ static void *juggle_shinfo_state(void *arg)
 
 	struct kvm_xen_hvm_attr cache_activate = {
 		.type = KVM_XEN_ATTR_TYPE_SHARED_INFO,
-		.u.shared_info.gfn = SHINFO_REGION_GPA / PAGE_SIZE
+		.u.shared_info.gfn = SHINFO_ADDR / PAGE_SIZE
 	};
 
 	struct kvm_xen_hvm_attr cache_deactivate = {
@@ -446,6 +448,7 @@ int main(int argc, char *argv[])
 	bool do_runstate_flag = !!(xen_caps & KVM_XEN_HVM_CONFIG_RUNSTATE_UPDATE_FLAG);
 	bool do_eventfd_tests = !!(xen_caps & KVM_XEN_HVM_CONFIG_EVTCHN_2LEVEL);
 	bool do_evtchn_tests = do_eventfd_tests && !!(xen_caps & KVM_XEN_HVM_CONFIG_EVTCHN_SEND);
+	bool has_vcpu_id = !!(xen_caps & KVM_XEN_HVM_CONFIG_EVTCHN_SEND);
 
 	clock_gettime(CLOCK_REALTIME, &min_ts);
 
@@ -494,7 +497,7 @@ int main(int argc, char *argv[])
 
 	struct kvm_xen_hvm_attr ha = {
 		.type = KVM_XEN_ATTR_TYPE_SHARED_INFO,
-		.u.shared_info.gfn = SHINFO_REGION_GPA / PAGE_SIZE,
+		.u.shared_info.gfn = SHINFO_ADDR / PAGE_SIZE,
 	};
 	vm_ioctl(vm, KVM_XEN_HVM_SET_ATTR, &ha);
 
@@ -508,6 +511,14 @@ int main(int argc, char *argv[])
 	TEST_ASSERT(m == shinfo, "Failed to map /dev/zero over shared info");
 	shinfo->wc = wc_copy;
 
+	if (has_vcpu_id) {
+		struct kvm_xen_vcpu_attr vid = {
+			.type = KVM_XEN_VCPU_ATTR_TYPE_VCPU_ID,
+			.u.vcpu_id = VCPU_ID,
+		};
+		vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &vid);
+	}
+
 	struct kvm_xen_vcpu_attr vi = {
 		.type = KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO,
 		.u.gpa = VCPU_INFO_ADDR,
@@ -983,8 +994,8 @@ int main(int argc, char *argv[])
 	struct pvclock_wall_clock *wc;
 	struct pvclock_vcpu_time_info *ti, *ti2;
 
-	wc = addr_gpa2hva(vm, SHINFO_REGION_GPA + 0xc00);
-	ti = addr_gpa2hva(vm, SHINFO_REGION_GPA + 0x40 + 0x20);
+	wc = addr_gpa2hva(vm, SHINFO_ADDR + 0xc00);
+	ti = addr_gpa2hva(vm, VCPU_INFO_ADDR + 0x20);
 	ti2 = addr_gpa2hva(vm, PVTIME_ADDR);
 
 	if (verbose) {
-- 
2.39.2

Re: [PATCH v2 09/12] KVM: selftests / xen: set KVM_XEN_VCPU_ATTR_TYPE_VCPU_ID

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1827 bytes --]

On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> If the capability (KVM_XEN_HVM_CONFIG_EVTCHN_SEND) is present then set
> the guest's vCPU id to match the chosen vcpu_info offset.

I think from KVM's point of view, the vcpu_id is still zero. As is the
vcpu_idx. What you're setting is the *Xen* vcpu_id.

I like that it's *different* to the vcpu_id; we should definitely be
testing that case. I don't quite know why the code was using
vcpu_info[1] in the shinfo before when we were explicitly setting the
address from userspace; I suppose it didn't matter.

> Also make some cosmetic fixes to the code for clarity.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> 
> v2:
>  - New in this version.
> ---
>  .../selftests/kvm/x86_64/xen_shinfo_test.c    | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
> index 05898ad9f4d9..49d0c91ee078 100644
> --- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
> @@ -38,6 +38,8 @@
>  #define VCPU_INFO_VADDR        (SHINFO_REGION_GVA + 0x40)
>  #define RUNSTATE_VADDR (SHINFO_REGION_GVA + PAGE_SIZE + PAGE_SIZE - 15)
>  
> +#define VCPU_ID                1 /* Must correspond to offset of VCPU_INFO_[V]ADDR */
> 

As well as being a bit clearer in the commit comment as noted above,
let's call this XEN_VCPU_ID ? 

With that cleaned up,

Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v2 09/12] KVM: selftests / xen: set KVM_XEN_VCPU_ATTR_TYPE_VCPU_ID

[Paul Durrant]

On 18/09/2023 12:59, David Woodhouse wrote:
> On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
>> From: Paul Durrant <pdurrant@amazon.com>
>>
>> If the capability (KVM_XEN_HVM_CONFIG_EVTCHN_SEND) is present then set
>> the guest's vCPU id to match the chosen vcpu_info offset.
> 
> I think from KVM's point of view, the vcpu_id is still zero. As is the
> vcpu_idx. What you're setting is the *Xen* vcpu_id.

Ok. I'll clarify the terminology; and I'll need to set it before the 
shinfo as noted on another thread.

> 
> I like that it's *different* to the vcpu_id; we should definitely be
> testing that case.

How so?

> I don't quite know why the code was using
> vcpu_info[1] in the shinfo before when we were explicitly setting the
> address from userspace; I suppose it didn't matter.
> 

Yes, I think it was entirely arbitrary.

>> Also make some cosmetic fixes to the code for clarity.
>>
>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>> ---
>> Cc: Sean Christopherson <seanjc@google.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: David Woodhouse <dwmw2@infradead.org>
>>
>> v2:
>>   - New in this version.
>> ---
>>   .../selftests/kvm/x86_64/xen_shinfo_test.c    | 19 +++++++++++++++----
>>   1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
>> index 05898ad9f4d9..49d0c91ee078 100644
>> --- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
>> +++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
>> @@ -38,6 +38,8 @@
>>   #define VCPU_INFO_VADDR        (SHINFO_REGION_GVA + 0x40)
>>   #define RUNSTATE_VADDR (SHINFO_REGION_GVA + PAGE_SIZE + PAGE_SIZE - 15)
>>   
>> +#define VCPU_ID                1 /* Must correspond to offset of VCPU_INFO_[V]ADDR */
>>
> 
> As well as being a bit clearer in the commit comment as noted above,
> let's call this XEN_VCPU_ID ?
>

Ok.


> With that cleaned up,
> 
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> 

Thanks,

   Paul

Re: [PATCH v2 09/12] KVM: selftests / xen: set KVM_XEN_VCPU_ATTR_TYPE_VCPU_ID

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

On Mon, 2023-09-18 at 13:18 +0100, Paul Durrant wrote:
> On 18/09/2023 12:59, David Woodhouse wrote:
> > On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
> > > From: Paul Durrant <pdurrant@amazon.com>
> > > 
> > > If the capability (KVM_XEN_HVM_CONFIG_EVTCHN_SEND) is present then set
> > > the guest's vCPU id to match the chosen vcpu_info offset.
> > 
> > I think from KVM's point of view, the vcpu_id is still zero. As is the
> > vcpu_idx. What you're setting is the *Xen* vcpu_id.
> 
> Ok. I'll clarify the terminology; and I'll need to set it before the 
> shinfo as noted on another thread.
> 
> > 
> > I like that it's *different* to the vcpu_id; we should definitely be
> > testing that case.
> 
> How so?
> 

Well, imagine you'd not got the kernel's get_vcpu_info_cache() right,
and you accidentally used v->vcpu_id instead of v->arch.xen.vcpu_id. An
easy mistake to make, right?

If you had made that mistake (you didn't; I checked), and if those
numbers had happened to be equal in this test... then this test
wouldn't have shown it.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

[PATCH v2 10/12] KVM: selftests / xen: map shared_info using HVA rather than GFN

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

Using the HVA of the shared_info page is more efficient, so if the
capability (KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA) is present use that method
to do the mapping.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>

v2:
 - New in this version.
---
 .../selftests/kvm/x86_64/xen_shinfo_test.c    | 50 ++++++++++++++++---
 1 file changed, 43 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
index 49d0c91ee078..fa829d6e0848 100644
--- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
+++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
@@ -395,6 +395,7 @@ static int cmp_timespec(struct timespec *a, struct timespec *b)
 		return 0;
 }
 
+static struct shared_info *shinfo;
 static struct vcpu_info *vinfo;
 static struct kvm_vcpu *vcpu;
 
@@ -406,7 +407,7 @@ static void handle_alrm(int sig)
 	TEST_FAIL("IRQ delivery timed out");
 }
 
-static void *juggle_shinfo_state(void *arg)
+static void *juggle_shinfo_state_gfn(void *arg)
 {
 	struct kvm_vm *vm = (struct kvm_vm *)arg;
 
@@ -429,6 +430,29 @@ static void *juggle_shinfo_state(void *arg)
 	return NULL;
 }
 
+static void *juggle_shinfo_state_hva(void *arg)
+{
+	struct kvm_vm *vm = (struct kvm_vm *)arg;
+
+	struct kvm_xen_hvm_attr cache_activate = {
+		.type = KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA,
+		.u.shared_info.hva = (unsigned long)shinfo
+	};
+
+	struct kvm_xen_hvm_attr cache_deactivate = {
+		.type = KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA,
+		.u.shared_info.hva = 0
+	};
+
+	for (;;) {
+		__vm_ioctl(vm, KVM_XEN_HVM_SET_ATTR, &cache_activate);
+		__vm_ioctl(vm, KVM_XEN_HVM_SET_ATTR, &cache_deactivate);
+		pthread_testcancel();
+	}
+
+	return NULL;
+}
+
 int main(int argc, char *argv[])
 {
 	struct timespec min_ts, max_ts, vm_ts;
@@ -449,6 +473,7 @@ int main(int argc, char *argv[])
 	bool do_eventfd_tests = !!(xen_caps & KVM_XEN_HVM_CONFIG_EVTCHN_2LEVEL);
 	bool do_evtchn_tests = do_eventfd_tests && !!(xen_caps & KVM_XEN_HVM_CONFIG_EVTCHN_SEND);
 	bool has_vcpu_id = !!(xen_caps & KVM_XEN_HVM_CONFIG_EVTCHN_SEND);
+	bool has_shinfo_hva = !!(xen_caps & KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA);
 
 	clock_gettime(CLOCK_REALTIME, &min_ts);
 
@@ -459,7 +484,7 @@ int main(int argc, char *argv[])
 				    SHINFO_REGION_GPA, SHINFO_REGION_SLOT, 3, 0);
 	virt_map(vm, SHINFO_REGION_GVA, SHINFO_REGION_GPA, 3);
 
-	struct shared_info *shinfo = addr_gpa2hva(vm, SHINFO_VADDR);
+	shinfo = addr_gpa2hva(vm, SHINFO_VADDR);
 
 	int zero_fd = open("/dev/zero", O_RDONLY);
 	TEST_ASSERT(zero_fd != -1, "Failed to open /dev/zero");
@@ -495,10 +520,16 @@ int main(int argc, char *argv[])
 			    "Failed to read back RUNSTATE_UPDATE_FLAG attr");
 	}
 
-	struct kvm_xen_hvm_attr ha = {
-		.type = KVM_XEN_ATTR_TYPE_SHARED_INFO,
-		.u.shared_info.gfn = SHINFO_ADDR / PAGE_SIZE,
-	};
+	struct kvm_xen_hvm_attr ha = {};
+
+	if (has_shinfo_hva) {
+		ha.type = KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA;
+		ha.u.shared_info.hva = (unsigned long)shinfo;
+	} else {
+		ha.type = KVM_XEN_ATTR_TYPE_SHARED_INFO;
+		ha.u.shared_info.gfn = SHINFO_ADDR / PAGE_SIZE;
+	}
+
 	vm_ioctl(vm, KVM_XEN_HVM_SET_ATTR, &ha);
 
 	/*
@@ -902,7 +933,12 @@ int main(int argc, char *argv[])
 				if (verbose)
 					printf("Testing shinfo lock corruption (KVM_XEN_HVM_EVTCHN_SEND)\n");
 
-				ret = pthread_create(&thread, NULL, &juggle_shinfo_state, (void *)vm);
+				if (has_shinfo_hva)
+					ret = pthread_create(&thread, NULL,
+							     &juggle_shinfo_state_hva, (void *)vm);
+				else
+					ret = pthread_create(&thread, NULL,
+							     &juggle_shinfo_state_gfn, (void *)vm);
 				TEST_ASSERT(ret == 0, "pthread_create() failed: %s", strerror(ret));
 
 				struct kvm_irq_routing_xen_evtchn uxe = {
-- 
2.39.2

Re: [PATCH v2 10/12] KVM: selftests / xen: map shared_info using HVA rather than GFN

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
> 
> -                               ret = pthread_create(&thread, NULL, &juggle_shinfo_state, (void *)vm);
> +                               if (has_shinfo_hva)
> +                                       ret = pthread_create(&thread, NULL,
> +                                                            &juggle_shinfo_state_hva, (void *)vm);
> +                               else
> +                                       ret = pthread_create(&thread, NULL,
> +                                                            &juggle_shinfo_state_gfn, (void *)vm);
>                                 TEST_ASSERT(ret == 0, "pthread_create() failed: %s", strerror(ret));
>  


This means you don't exercise the GFN-based path (which all current VMM
implementations use) on newer kernels. Could you have a single
juggle_shinfo_state() function as before, but make it repeatedly set
and clear the shinfo using *both* HVA and GFN (if the former is
available, of course).

While you're at it, it looks like the thread leaves the shinfo
*deactivated*, which might come as a surprise to anyone who adds tests
at the end near the existing TEST_DONE. Can we make it leave the shinfo
*active* instead?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v2 10/12] KVM: selftests / xen: map shared_info using HVA rather than GFN

[Paul Durrant]

On 18/09/2023 14:19, David Woodhouse wrote:
> On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
>>
>> -                               ret = pthread_create(&thread, NULL, &juggle_shinfo_state, (void *)vm);
>> +                               if (has_shinfo_hva)
>> +                                       ret = pthread_create(&thread, NULL,
>> +                                                            &juggle_shinfo_state_hva, (void *)vm);
>> +                               else
>> +                                       ret = pthread_create(&thread, NULL,
>> +                                                            &juggle_shinfo_state_gfn, (void *)vm);
>>                                  TEST_ASSERT(ret == 0, "pthread_create() failed: %s", strerror(ret));
>>   
> 
> 
> This means you don't exercise the GFN-based path (which all current VMM
> implementations use) on newer kernels. Could you have a single
> juggle_shinfo_state() function as before, but make it repeatedly set
> and clear the shinfo using *both* HVA and GFN (if the former is
> available, of course).

The guidance is to use HVA if the feature is available; a VMM should not 
really be mixing and matching. That said, setting it either way should 
be equivalent.

> 
> While you're at it, it looks like the thread leaves the shinfo
> *deactivated*, which might come as a surprise to anyone who adds tests
> at the end near the existing TEST_DONE. Can we make it leave the shinfo
> *active* instead?

Ok.

   Paul

Re: [PATCH v2 10/12] KVM: selftests / xen: map shared_info using HVA rather than GFN

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1946 bytes --]

On Mon, 2023-09-18 at 14:24 +0100, Paul Durrant wrote:
> On 18/09/2023 14:19, David Woodhouse wrote:
> > On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
> > > 
> > > -                               ret = pthread_create(&thread, NULL, &juggle_shinfo_state, (void *)vm);
> > > +                               if (has_shinfo_hva)
> > > +                                       ret = pthread_create(&thread, NULL,
> > > +                                                            &juggle_shinfo_state_hva, (void *)vm);
> > > +                               else
> > > +                                       ret = pthread_create(&thread, NULL,
> > > +                                                            &juggle_shinfo_state_gfn, (void *)vm);
> > >                                  TEST_ASSERT(ret == 0, "pthread_create() failed: %s", strerror(ret));
> > >   
> > 
> > 
> > This means you don't exercise the GFN-based path (which all current VMM
> > implementations use) on newer kernels. Could you have a single
> > juggle_shinfo_state() function as before, but make it repeatedly set
> > and clear the shinfo using *both* HVA and GFN (if the former is
> > available, of course).
> 
> The guidance is to use HVA if the feature is available; a VMM should not 
> really be mixing and matching. That said, setting it either way should 
> be equivalent.

Sure, but this isn't really a VMM; it's a test harness to exercise a
bunch of ioctls. One of which it no longer bothers to test now, if the
new variant exists.

If we're going to keep the old API we should also continue to *test*
the old API. (And we should).


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

[PATCH v2 11/12] KVM: selftests / xen: don't explicitly set the vcpu_info address

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

If the vCPU id is set and the shared_info is mapped using HVA then we can
infer that KVM has the ability to use a default vcpu_info mapping. Hence
we can stop setting the address of the vcpu_info structure.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>

v2:
 - New in this version.
---
 tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
index fa829d6e0848..d1c88deec0b2 100644
--- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
+++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
@@ -550,11 +550,13 @@ int main(int argc, char *argv[])
 		vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &vid);
 	}
 
-	struct kvm_xen_vcpu_attr vi = {
-		.type = KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO,
-		.u.gpa = VCPU_INFO_ADDR,
-	};
-	vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &vi);
+	if (!has_vcpu_id || !has_shinfo_hva) {
+		struct kvm_xen_vcpu_attr vi = {
+			.type = KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO,
+			.u.gpa = VCPU_INFO_ADDR,
+		};
+		vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &vi);
+	}
 
 	struct kvm_xen_vcpu_attr pvclock = {
 		.type = KVM_XEN_VCPU_ATTR_TYPE_VCPU_TIME_INFO,
-- 
2.39.2

Re: [PATCH v2 11/12] KVM: selftests / xen: don't explicitly set the vcpu_info address

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> If the vCPU id is set and the shared_info is mapped using HVA then we can
> infer that KVM has the ability to use a default vcpu_info mapping. Hence
> we can stop setting the address of the vcpu_info structure.

Again that means we're not *testing* it any more when the test is run
on newer kernels. Can we perhaps set it explicitly, after *half* the
tests are done? Maybe to a *different* address than the default which
is derived from the Xen vcpu_id? And check that the memcpy works right
when we do?


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v2 11/12] KVM: selftests / xen: don't explicitly set the vcpu_info address

[Paul Durrant]

On 18/09/2023 14:21, David Woodhouse wrote:
> On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
>> From: Paul Durrant <pdurrant@amazon.com>
>>
>> If the vCPU id is set and the shared_info is mapped using HVA then we can
>> infer that KVM has the ability to use a default vcpu_info mapping. Hence
>> we can stop setting the address of the vcpu_info structure.
> 
> Again that means we're not *testing* it any more when the test is run
> on newer kernels. Can we perhaps set it explicitly, after *half* the
> tests are done? Maybe to a *different* address than the default which
> is derived from the Xen vcpu_id? And check that the memcpy works right
> when we do?
> 

Ok. The VMM is currently responsible for that memcpy. Are you suggesting 
we push that into KVM too?

   Paul

Re: [PATCH v2 11/12] KVM: selftests / xen: don't explicitly set the vcpu_info address

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2104 bytes --]

On Mon, 2023-09-18 at 14:26 +0100, Paul Durrant wrote:
> On 18/09/2023 14:21, David Woodhouse wrote:
> > On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
> > > From: Paul Durrant <pdurrant@amazon.com>
> > > 
> > > If the vCPU id is set and the shared_info is mapped using HVA then we can
> > > infer that KVM has the ability to use a default vcpu_info mapping. Hence
> > > we can stop setting the address of the vcpu_info structure.
> > 
> > Again that means we're not *testing* it any more when the test is run
> > on newer kernels. Can we perhaps set it explicitly, after *half* the
> > tests are done? Maybe to a *different* address than the default which
> > is derived from the Xen vcpu_id? And check that the memcpy works right
> > when we do?
> > 
> 
> Ok. The VMM is currently responsible for that memcpy. Are you suggesting 
> we push that into KVM too?

Ah OK.

Hm, maybe we should?

What happened before in the case where interrupts were being delivered,
and the vcpu_info address was changed. 

In Xen, I guess it's effectively atomic? Some locking will mean that
the event channel is delivered to the vcpu_info either *before* the
memcpy, or *after* it, but never to the old address after the copy has
been done, so that the event (well the index of it) gets lost?

In KVM before we did the automatic placement, it was the VMM's problem.

If there are any interrupts set up for direct delivery, I suppose the
VMM should have *removed* the vcpu_info mapping before doing the
memcpy, then restored it at the new address? I may have to check qemu
gets that right.

Then again, it's a very hard race to trigger, given that a guest can
only set the vcpu_info once. So it can move it from the shinfo to a
separate address and attempt to trigger this race just that one time.

But in the case where auto-placement has happened, and then the guest
sets an explicit vcpu_info location... are we saying that the VMM must
explicitly *unmap* the vcpu_info first, then memcpy, then set it to the
new location? Or will we handle the memcpy in-kernel?


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v2 11/12] KVM: selftests / xen: don't explicitly set the vcpu_info address

[Paul Durrant]

On 18/09/2023 14:36, David Woodhouse wrote:
> On Mon, 2023-09-18 at 14:26 +0100, Paul Durrant wrote:
>> On 18/09/2023 14:21, David Woodhouse wrote:
>>> On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
>>>> From: Paul Durrant <pdurrant@amazon.com>
>>>>
>>>> If the vCPU id is set and the shared_info is mapped using HVA then we can
>>>> infer that KVM has the ability to use a default vcpu_info mapping. Hence
>>>> we can stop setting the address of the vcpu_info structure.
>>>
>>> Again that means we're not *testing* it any more when the test is run
>>> on newer kernels. Can we perhaps set it explicitly, after *half* the
>>> tests are done? Maybe to a *different* address than the default which
>>> is derived from the Xen vcpu_id? And check that the memcpy works right
>>> when we do?
>>>
>>
>> Ok. The VMM is currently responsible for that memcpy. Are you suggesting
>> we push that into KVM too?
> 
> Ah OK.
> 
> Hm, maybe we should?
> 
> What happened before in the case where interrupts were being delivered,
> and the vcpu_info address was changed.
> 
> In Xen, I guess it's effectively atomic? Some locking will mean that
> the event channel is delivered to the vcpu_info either *before* the
> memcpy, or *after* it, but never to the old address after the copy has
> been done, so that the event (well the index of it) gets lost?
> 
> In KVM before we did the automatic placement, it was the VMM's problem.
> 
> If there are any interrupts set up for direct delivery, I suppose the
> VMM should have *removed* the vcpu_info mapping before doing the
> memcpy, then restored it at the new address? I may have to check qemu
> gets that right.
> 
> Then again, it's a very hard race to trigger, given that a guest can
> only set the vcpu_info once. So it can move it from the shinfo to a
> separate address and attempt to trigger this race just that one time.
> 
> But in the case where auto-placement has happened, and then the guest
> sets an explicit vcpu_info location... are we saying that the VMM must
> explicitly *unmap* the vcpu_info first, then memcpy, then set it to the
> new location? Or will we handle the memcpy in-kernel?
> 

Well, if the VMM is using the default then it can't unmap it. But 
setting a vcpu_info *after* enabling any event channels would be a very 
odd thing for a guest to do and IMO it gets to keep the pieces if it 
does so.

   Paul

Re: [PATCH v2 11/12] KVM: selftests / xen: don't explicitly set the vcpu_info address

[David Woodhouse]

On 18 September 2023 14:41:08 BST, Paul Durrant <xadimgnik@gmail.com> wrote:
>Well, if the VMM is using the default then it can't unmap it. But setting a vcpu_info *after* enabling any event channels would be a very odd thing for a guest to do and IMO it gets to keep the pieces if it does so.


Hm, I suppose I'm OK with that approach. The fact that both VMM implementations using this KVM/Xen support let the guest keep precisely those pieces is a testament to that :)

But now we're hard-coding the behaviour in the kernel and declaring that no VMM will be *able* to "fix" that case even if it does want to. So perhaps it wants a modicum more thought and at least some explicit documentation to that effect?

And a hand-wavy plan at least for what we'd do if we suddenly did find a reason to care?

Re: [PATCH v2 11/12] KVM: selftests / xen: don't explicitly set the vcpu_info address

[Paul Durrant]

On 18/09/2023 15:05, David Woodhouse wrote:
> 
> 
> On 18 September 2023 14:41:08 BST, Paul Durrant <xadimgnik@gmail.com> wrote:
>> Well, if the VMM is using the default then it can't unmap it. But setting a vcpu_info *after* enabling any event channels would be a very odd thing for a guest to do and IMO it gets to keep the pieces if it does so.
> 
> 
> Hm, I suppose I'm OK with that approach. The fact that both VMM implementations using this KVM/Xen support let the guest keep precisely those pieces is a testament to that :)
> 

I can have the selftest explicitly set the vcpu_info to point at the one 
that's already in use, I suppose... so the would at least make sure the 
attribute is functioning.

> But now we're hard-coding the behaviour in the kernel and declaring that no VMM will be *able* to "fix" that case even if it does want to. So perhaps it wants a modicum more thought and at least some explicit documentation to that effect?
> 
> And a hand-wavy plan at least for what we'd do if we suddenly did find a reason to care?

Handwavy plan would be for the VMM to:

a) Mask all open event channels targetting the vcpu
b) Copy vcpu_info content to the new location
c) Tell KVM where it is
d) Unmask the masked event channels

Does that sound ok? If so I can stick it in the API documentation.

[PATCH v2 12/12] KVM: xen: advertize the KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA capability

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

Now that all relevant kernel changes and selftests are in place, enable the
new capability.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: x86@kernel.org

v2:
 - New in this version.
---
 arch/x86/kvm/x86.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2d7d0ae18820..4cd577d01bc4 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4527,7 +4527,8 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 		    KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL |
 		    KVM_XEN_HVM_CONFIG_SHARED_INFO |
 		    KVM_XEN_HVM_CONFIG_EVTCHN_2LEVEL |
-		    KVM_XEN_HVM_CONFIG_EVTCHN_SEND;
+		    KVM_XEN_HVM_CONFIG_EVTCHN_SEND |
+		    KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA;
 		if (sched_info_on())
 			r |= KVM_XEN_HVM_CONFIG_RUNSTATE |
 			     KVM_XEN_HVM_CONFIG_RUNSTATE_UPDATE_FLAG;
-- 
2.39.2

Re: [PATCH v2 12/12] KVM: xen: advertize the KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA capability

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 313 bytes --]

On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> Now that all relevant kernel changes and selftests are in place, enable the
> new capability.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]