[PATCH 0/8] KVM: xen: update shared_info and vcpu_info handling

[PATCH 0/8] 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 (8):
  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

 arch/x86/include/asm/kvm_host.h |   4 +
 arch/x86/kvm/x86.c              |  18 ++---
 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        |   7 +-
 virt/kvm/pfncache.c             | 129 +++++++++++++++++++++++---------
 8 files changed, 251 insertions(+), 80 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 1/8] 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.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 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

Re: [PATCH 1/8] KVM: pfncache: add a map helper function

[David Woodhouse]

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

On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
> 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.
> 
> 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 --]

[PATCH 2/8] 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 need to know
about this detail.

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
---
 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

Re: [PATCH 2/8] KVM: pfncache: add a mark-dirty helper

[David Woodhouse]

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

On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
> --- 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)

ISTR there was a reason why the mark_page_dirty_in_slot() was called
*after* unlocking. Although now I say it, that seems wrong... is that
because the spinlock is only protecting the uHVA→kHVA mapping, while
the memslot/gpa are going to remain valid even after unlock, because
those are protected by sRCU?

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

Re: [PATCH 2/8] KVM: pfncache: add a mark-dirty helper

[Paul Durrant]

On 14/09/2023 10:21, David Woodhouse wrote:
> On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
>> --- 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)
> 
> ISTR there was a reason why the mark_page_dirty_in_slot() was called
> *after* unlocking. Although now I say it, that seems wrong... is that
> because the spinlock is only protecting the uHVA→kHVA mapping, while
> the memslot/gpa are going to remain valid even after unlock, because
> those are protected by sRCU?

Without the lock you could see an inconsistent GPA and memslot so I 
think you could theoretically calculate a bogus rel_gfn and walk off the 
end of the dirty bitmap. Hence moving the call inside the lock while I 
was in the neighbourhood seemed like a good idea. I could call it out in 
the commit comment if you'd like.

   Paul

Re: [PATCH 2/8] KVM: pfncache: add a mark-dirty helper

[David Woodhouse]

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

On Thu, 2023-09-14 at 11:34 +0200, Paul Durrant wrote:
> On 14/09/2023 10:21, David Woodhouse wrote:
> > On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
> > > --- 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)
> > 
> > ISTR there was a reason why the mark_page_dirty_in_slot() was called
> > *after* unlocking. Although now I say it, that seems wrong... is that
> > because the spinlock is only protecting the uHVA→kHVA mapping, while
> > the memslot/gpa are going to remain valid even after unlock, because
> > those are protected by sRCU?
> 
> Without the lock you could see an inconsistent GPA and memslot so I 
> think you could theoretically calculate a bogus rel_gfn and walk off the 
> end of the dirty bitmap. Hence moving the call inside the lock while I 
> was in the neighbourhood seemed like a good idea. I could call it out in 
> the commit comment if you'd like.

Yeah, I can't see a reason why it needs to be outside the lock, and as
you note, there really is a reason why it should be *inside*. Whatever
reason there was, it either disappeared in the revisions of the gpc
patch set or it was stupidity on my part in the first place.

So yeah, let it move inside the lock, call that out in the commit
message (I did note some of the other commits could have used a 'No
functional change intended' too, FWIW), and

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

Thanks.

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

Re: [PATCH 2/8] KVM: pfncache: add a mark-dirty helper

[Paul Durrant]

On 14/09/2023 13:39, David Woodhouse wrote:
> On Thu, 2023-09-14 at 11:34 +0200, Paul Durrant wrote:
>> On 14/09/2023 10:21, David Woodhouse wrote:
>>> On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
>>>> --- 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)
>>>
>>> ISTR there was a reason why the mark_page_dirty_in_slot() was called
>>> *after* unlocking. Although now I say it, that seems wrong... is that
>>> because the spinlock is only protecting the uHVA→kHVA mapping, while
>>> the memslot/gpa are going to remain valid even after unlock, because
>>> those are protected by sRCU?
>>
>> Without the lock you could see an inconsistent GPA and memslot so I
>> think you could theoretically calculate a bogus rel_gfn and walk off the
>> end of the dirty bitmap. Hence moving the call inside the lock while I
>> was in the neighbourhood seemed like a good idea. I could call it out in
>> the commit comment if you'd like.
> 
> Yeah, I can't see a reason why it needs to be outside the lock, and as
> you note, there really is a reason why it should be *inside*. Whatever
> reason there was, it either disappeared in the revisions of the gpc
> patch set or it was stupidity on my part in the first place.
> 
> So yeah, let it move inside the lock, call that out in the commit
> message (I did note some of the other commits could have used a 'No
> functional change intended' too, FWIW), and

Ack. Will do.

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

Thanks.

   Paul

[PATCH 3/8] 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.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: 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

Re: [PATCH 3/8] KVM: pfncache: add a helper to get the gpa

[David Woodhouse]

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

On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
> 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 <dwmw@amazon.co.uk>


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

[PATCH 4/8] 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).

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 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

Re: [PATCH 4/8] KVM: pfncache: base offset check on khva rather than gpa

[David Woodhouse]

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

On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
> 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 <dwmw@amazon.co.uk>


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

[PATCH 5/8] 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..ae97fb19c0d5 100644
--- a/include/linux/kvm_types.h
+++ b/include/linux/kvm_types.h
@@ -64,7 +64,8 @@ struct gfn_to_hva_cache {
 
 struct gfn_to_pfn_cache {
 	u64 generation;
-	gpa_t gpa;
+	unsigned long addr;
+	bool addr_is_gpa;
 	unsigned long uhva;
 	struct kvm_memory_slot *memslot;
 	struct kvm *kvm;
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 37bcb4399780..a86dc6cf49c6 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, unsigned long 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 5/8] KVM: pfncache: allow a cache to be activated with a fixed (userspace) HVA

[David Woodhouse]

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

On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
> 
>  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);

I think I have a slight preference for leaving kvm_gpc_refresh()
working on a GPA unconditionally, thus calling __kvm_gpc_refresh() with
the final argument set to true.

Introduce another one-line wrapper kvm_gpc_refresh_hva() for the false
case. And perhaps BUG_ON() calling the 'wrong' refresh function?

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

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

[Paul Durrant]

On 14/09/2023 10:29, David Woodhouse wrote:
> On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
>>
>>   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);
> 
> I think I have a slight preference for leaving kvm_gpc_refresh()
> working on a GPA unconditionally, thus calling __kvm_gpc_refresh() with
> the final argument set to true.
> 
> Introduce another one-line wrapper kvm_gpc_refresh_hva() for the false
> case. And perhaps BUG_ON() calling the 'wrong' refresh function?

Hmm. That makes life harder for the code messing with the vcpu_info. I 
would need to know which cache it was looking at, because it could be 
the vcpu_info cache or shinfo cache, and if it's the shinfo cache it 
would need to know how it was activated.

   Paul

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

[David Woodhouse]

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

On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
> --- a/include/linux/kvm_types.h
> +++ b/include/linux/kvm_types.h
> @@ -64,7 +64,8 @@ struct gfn_to_hva_cache {
>  
>  struct gfn_to_pfn_cache {
>         u64 generation;
> -       gpa_t gpa;
> +       unsigned long addr;

On 32-bit hosts gpa_t is 64 bits and doesn't fit in an 'unsigned long'

> +       bool addr_is_gpa;

Don't put that there. There are already bools at the end of the struct
which wouldn't leave 63 bits of padding.


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

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

[Paul Durrant]

On 14/09/2023 14:51, David Woodhouse wrote:
> On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
>> --- a/include/linux/kvm_types.h
>> +++ b/include/linux/kvm_types.h
>> @@ -64,7 +64,8 @@ struct gfn_to_hva_cache {
>>   
>>   struct gfn_to_pfn_cache {
>>          u64 generation;
>> -       gpa_t gpa;
>> +       unsigned long addr;
> 
> On 32-bit hosts gpa_t is 64 bits and doesn't fit in an 'unsigned long'
> 

Damn. So used to the host only ever being 64-bit. A u64 it is then.

>> +       bool addr_is_gpa;
> 
> Don't put that there. There are already bools at the end of the struct
> which wouldn't leave 63 bits of padding.
> 

True. Will move.

   Paul

[PATCH 6/8] 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.

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
---
 arch/x86/kvm/x86.c       |  3 ++-
 arch/x86/kvm/xen.c       | 28 ++++++++++++++++++++++------
 include/uapi/linux/kvm.h |  6 +++++-
 3 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d669a8801265..0df06f47801c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4531,7 +4531,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;
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 f089ab290978..d3f371bafad8 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;
@@ -1786,9 +1787,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;
@@ -1830,6 +1832,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

Re: [PATCH 6/8] KVM: xen: allow shared_info to be mapped by fixed HVA

[David Woodhouse]

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

On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
> @@ -1786,9 +1787,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;

Hm, do we consider that to be an acceptable ABI change? I suppose
arguably it's compatible with any existing source or binary code, and
that's the criterion that really matters?

Worth drawing attention to it in case anyone strongly objects.

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

As discussed though, I'd be inclined not to *advertise* the new cap
yet; roll the auto-vcpu-info in with it and only set it in the final
patch where you add the test cases.

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

[PATCH 7/8] 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 less 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.

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>
---
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
---
 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 0df06f47801c..4cd577d01bc4 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..892563fea40f 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));
 }
 
+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

Re: [PATCH 7/8] KVM: xen: prepare for using 'default' vcpu_info

[David Woodhouse]

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

On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
> 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 less than 32 vCPUs) if they don't
                                     ^ fewer

> 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.
> 
> 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.

No functional change intended...?


> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

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

> 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
> ---
>  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 0df06f47801c..4cd577d01bc4 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..892563fea40f 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));
>  }
>  
> +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;


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

Re: [PATCH 7/8] KVM: xen: prepare for using 'default' vcpu_info

[kernel test robot]

Hi Paul,

kernel test robot noticed the following build warnings:

[auto build test WARNING on kvm/queue]
[also build test WARNING on mst-vhost/linux-next]
[cannot apply to kvm/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Paul-Durrant/KVM-pfncache-add-a-map-helper-function/20230914-171017
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
patch link:    https://lore.kernel.org/r/20230914084946.200043-8-paul%40xen.org
patch subject: [PATCH 7/8] KVM: xen: prepare for using 'default' vcpu_info
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230915/202309150313.w8vtnM1C-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230915/202309150313.w8vtnM1C-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309150313.w8vtnM1C-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> arch/x86/kvm/xen.c:492:26: warning: no previous prototype for 'get_vcpu_info_cache' [-Wmissing-prototypes]
     492 | struct gfn_to_pfn_cache *get_vcpu_info_cache(struct kvm_vcpu *v, unsigned long *offset)
         |                          ^~~~~~~~~~~~~~~~~~~


vim +/get_vcpu_info_cache +492 arch/x86/kvm/xen.c

   491	
 > 492	struct gfn_to_pfn_cache *get_vcpu_info_cache(struct kvm_vcpu *v, unsigned long *offset)
   493	{
   494		if (offset)
   495			*offset = 0;
   496	
   497		return &v->arch.xen.vcpu_info_cache;
   498	}
   499	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 7/8] KVM: xen: prepare for using 'default' vcpu_info

[kernel test robot]

Hi Paul,

kernel test robot noticed the following build warnings:

[auto build test WARNING on kvm/queue]
[also build test WARNING on mst-vhost/linux-next linus/master v6.6-rc2 next-20230918]
[cannot apply to kvm/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Paul-Durrant/KVM-pfncache-add-a-map-helper-function/20230914-171017
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
patch link:    https://lore.kernel.org/r/20230914084946.200043-8-paul%40xen.org
patch subject: [PATCH 7/8] KVM: xen: prepare for using 'default' vcpu_info
config: i386-randconfig-063-20230918 (https://download.01.org/0day-ci/archive/20230918/202309181725.cpTHcjo6-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230918/202309181725.cpTHcjo6-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309181725.cpTHcjo6-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> arch/x86/kvm/xen.c:492:25: sparse: sparse: symbol 'get_vcpu_info_cache' was not declared. Should it be static?
   arch/x86/kvm/xen.c:441:9: sparse: sparse: context imbalance in 'kvm_xen_update_runstate_guest' - unexpected unlock

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 8/8] KVM: xen: automatically use the vcpu_info embedded in shared_info

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

Add a KVM_XEN_HVM_CONFIG_DEFAULT_VCPU_INFO flag so that the VMM knows it
need only set the KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO attribute in response to
a VCPUOP_register_vcpu_info hypercall, and 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.

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
---
 arch/x86/kvm/x86.c       |  3 ++-
 arch/x86/kvm/xen.c       | 15 +++++++++++++++
 include/uapi/linux/kvm.h |  1 +
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4cd577d01bc4..cdce8c463a1b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4528,7 +4528,8 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 		    KVM_XEN_HVM_CONFIG_SHARED_INFO |
 		    KVM_XEN_HVM_CONFIG_EVTCHN_2LEVEL |
 		    KVM_XEN_HVM_CONFIG_EVTCHN_SEND |
-		    KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA;
+		    KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA |
+		    KVM_XEN_HVM_CONFIG_DEFAULT_VCPU_INFO;
 		if (sched_info_on())
 			r |= KVM_XEN_HVM_CONFIG_RUNSTATE |
 			     KVM_XEN_HVM_CONFIG_RUNSTATE_UPDATE_FLAG;
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 892563fea40f..66050a96ba25 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)
 
 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;
 
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index d3f371bafad8..480612f73fc2 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1283,6 +1283,7 @@ struct kvm_x86_mce {
 #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)
+#define KVM_XEN_HVM_CONFIG_DEFAULT_VCPU_INFO	(1 << 8)
 
 struct kvm_xen_hvm_config {
 	__u32 flags;
-- 
2.39.2

Re: [PATCH 8/8] KVM: xen: automatically use the vcpu_info embedded in shared_info

[David Woodhouse]

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

On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> Add a KVM_XEN_HVM_CONFIG_DEFAULT_VCPU_INFO flag so that the VMM knows it
> need only set the KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO attribute in response to
> a VCPUOP_register_vcpu_info hypercall, and 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.

Might be simpler just to link this behaviour to the
KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA support? If userspace sets the
shared_info as an HVA, then the default vcpu_info will be used therein.

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

Re: [PATCH 8/8] KVM: xen: automatically use the vcpu_info embedded in shared_info

[Paul Durrant]

On 14/09/2023 10:09, David Woodhouse wrote:
> On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
>> From: Paul Durrant <pdurrant@amazon.com>
>>
>> Add a KVM_XEN_HVM_CONFIG_DEFAULT_VCPU_INFO flag so that the VMM knows it
>> need only set the KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO attribute in response to
>> a VCPUOP_register_vcpu_info hypercall, and 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.
> 
> Might be simpler just to link this behaviour to the
> KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA support? If userspace sets the
> shared_info as an HVA, then the default vcpu_info will be used therein.

Well there's no problem in using the default embedded vcpu_info even if 
the guests sets the shared_info via GPA... it still saves the VMM making 
a few ioctls. So do we really want to link the two things?

   Paul

Re: [PATCH 8/8] KVM: xen: automatically use the vcpu_info embedded in shared_info

[David Woodhouse]

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

On Thu, 2023-09-14 at 11:17 +0200, Paul Durrant wrote:
> On 14/09/2023 10:09, David Woodhouse wrote:
> > On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
> > > From: Paul Durrant <pdurrant@amazon.com>
> > > 
> > > Add a KVM_XEN_HVM_CONFIG_DEFAULT_VCPU_INFO flag so that the VMM knows it
> > > need only set the KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO attribute in response to
> > > a VCPUOP_register_vcpu_info hypercall, and 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.
> > 
> > Might be simpler just to link this behaviour to the
> > KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA support? If userspace sets the
> > shared_info as an HVA, then the default vcpu_info will be used therein.
> 
> Well there's no problem in using the default embedded vcpu_info even if 
> the guests sets the shared_info via GPA... it still saves the VMM making 
> a few ioctls. So do we really want to link the two things?

I'd prefer to avoid the combinatorial explosion in the advertised features.

And ideally we need to allow the VMM to opt *in* to the new behaviour,
although I suppose you could argue that it doesn't make much difference
in this case.

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

Re: [PATCH 0/8] KVM: xen: update shared_info and vcpu_info handling

[David Woodhouse]

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

On Thu, 2023-09-14 at 08:49 +0000, Paul Durrant wrote:
> 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.

Looks sane to me in general; thanks.

The changes to the pfncache ended up being a little bit more than I
originally anticipated when I said you just needed to disable the
GPA→uHVA lookup via memslots which was about 10 lines... but I think
it's still true that it's worth using the pfncache to avoid reproducing
the intricate mmu_notifier invalidations.


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