[PATCH 0/2] KVM: xen: update shared_info when long_mode is set

[PATCH 0/2] KVM: xen: update shared_info when long_mode is set

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

This series is based on my v9 of my "update shared_info and vcpu_info
handling" series [1] and fixes an issue that was latent before the
"allow shared_info to be mapped by fixed HVA" patch of that series allowed
a VMM to set up shared_info before the VM booted and then leave it alone.

The problem was noticed when the guest wallclock apparently reverted to
the Unix epoch. This was because, when the shared_info was set up the
guest's long_mode flag was unset and hence the wallclock was intialized
in the place where a 32-bit guest would expect to find it. The 64-bit
guest being tested instead found zero-ed out memory.

Fix the the issue by first separating the initialization of the
shared_info content from setting its location (by HVA or GPA) and then
(re-)initializing the content any time the long_mode flag is changed.

[1] https://lore.kernel.org/kvm/20231122121822.1042-1-paul@xen.org/

Paul Durrant (2):
  KVM: xen: separate initialization of shared_info cache and content
  KVM: xen: (re-)initialize shared_info if guest (32/64-bit) mode is set

 arch/x86/kvm/xen.c | 84 ++++++++++++++++++++++++++++------------------
 1 file changed, 52 insertions(+), 32 deletions(-)


base-commit: 369e9826edfd346f259471e521c03e12bb0ab476
-- 
2.39.2

[PATCH 1/2] KVM: xen: separate initialization of shared_info cache and content

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

The shared_info cache should only need to be set up once by the VMM, but
the content of the shared_info page may need changed if the mode of guest
changes from 32-bit to 64-bit or vice versa. This re-initialization of
the content will be handles in a subsequent patch.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
 arch/x86/kvm/xen.c | 70 ++++++++++++++++++++++++++--------------------
 1 file changed, 40 insertions(+), 30 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index cfd5051e0800..7bead3f65e55 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -34,7 +34,7 @@ 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, u64 addr, bool addr_is_gfn)
+static int kvm_xen_shared_info_init(struct kvm *kvm)
 {
 	struct gfn_to_pfn_cache *gpc = &kvm->arch.xen.shinfo_cache;
 	struct pvclock_wall_clock *wc;
@@ -44,34 +44,22 @@ static int kvm_xen_shared_info_init(struct kvm *kvm, u64 addr, bool addr_is_gfn)
 	int ret = 0;
 	int idx = srcu_read_lock(&kvm->srcu);
 
-	if ((addr_is_gfn && addr == KVM_XEN_INVALID_GFN) ||
-	    (!addr_is_gfn && addr == 0)) {
-		kvm_gpc_deactivate(gpc);
-		goto out;
-	}
+	read_lock_irq(&gpc->lock);
+	while (!kvm_gpc_check(gpc, PAGE_SIZE)) {
+		read_unlock_irq(&gpc->lock);
 
-	do {
-		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);
+		ret = kvm_gpc_refresh(gpc, PAGE_SIZE);
 		if (ret)
 			goto out;
 
-		/*
-		 * This code mirrors kvm_write_wall_clock() except that it writes
-		 * directly through the pfn cache and doesn't mark the page dirty.
-		 */
-		wall_nsec = kvm_get_wall_clock_epoch(kvm);
-
-		/* It could be invalid again already, so we need to check */
 		read_lock_irq(&gpc->lock);
+	}
 
-		if (gpc->valid)
-			break;
-
-		read_unlock_irq(&gpc->lock);
-	} while (1);
+	/*
+	 * This code mirrors kvm_write_wall_clock() except that it writes
+	 * directly through the pfn cache and doesn't mark the page dirty.
+	 */
+	wall_nsec = ktime_get_real_ns() - get_kvmclock_ns(kvm);
 
 	/* Paranoia checks on the 32-bit struct layout */
 	BUILD_BUG_ON(offsetof(struct compat_shared_info, wc) != 0x900);
@@ -642,17 +630,39 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
 		break;
 
 	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, true);
-		mutex_unlock(&kvm->arch.xen.xen_lock);
-		break;
+	case KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA: {
+		int idx;
 
-	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);
+
+		idx = srcu_read_lock(&kvm->srcu);
+		if (data->type == KVM_XEN_ATTR_TYPE_SHARED_INFO) {
+			if (data->u.shared_info.gfn == KVM_XEN_INVALID_GFN) {
+				kvm_gpc_deactivate(&kvm->arch.xen.shinfo_cache);
+				r = 0;
+			} else {
+				r = kvm_gpc_activate(&kvm->arch.xen.shinfo_cache,
+						     gfn_to_gpa(data->u.shared_info.gfn),
+						     PAGE_SIZE);
+			}
+		} else {
+			if (data->u.shared_info.hva == 0) {
+				kvm_gpc_deactivate(&kvm->arch.xen.shinfo_cache);
+				r = 0;
+			} else {
+				r = kvm_gpc_activate_hva(&kvm->arch.xen.shinfo_cache,
+							 data->u.shared_info.hva,
+							 PAGE_SIZE);
+			}
+		}
+		srcu_read_unlock(&kvm->srcu, idx);
+
+		if (!r && kvm->arch.xen.shinfo_cache.active)
+			r = kvm_xen_shared_info_init(kvm);
+
 		mutex_unlock(&kvm->arch.xen.xen_lock);
 		break;
-
+	}
 	case KVM_XEN_ATTR_TYPE_UPCALL_VECTOR:
 		if (data->u.vector && data->u.vector < 0x10)
 			r = -EINVAL;
-- 
2.39.2

[PATCH 2/2] KVM: xen: (re-)initialize shared_info if guest (32/64-bit) mode is set

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

If the shared_info PFN cache has already been initialized then the content
of the shared_info page needs to be (re-)initialized if the guest mode is
set. It is no lnger done when the PFN cache is activated.
Setting the guest mode is either done explicitly by the VMM via the
KVM_XEN_ATTR_TYPE_LONG_MODE attribute, or implicitly when the guest writes
the MSR to set up the hypercall page.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
 arch/x86/kvm/xen.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 7bead3f65e55..bfc8f6698cbc 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -624,8 +624,15 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
 		} else {
 			mutex_lock(&kvm->arch.xen.xen_lock);
 			kvm->arch.xen.long_mode = !!data->u.long_mode;
+
+			/*
+			 * If shared_info has already been initialized
+			 * then re-initialize it with the new width.
+			 */
+			r = kvm->arch.xen.shinfo_cache.active ?
+				kvm_xen_shared_info_init(kvm) : 0;
+
 			mutex_unlock(&kvm->arch.xen.xen_lock);
-			r = 0;
 		}
 		break;
 
@@ -657,9 +664,6 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
 		}
 		srcu_read_unlock(&kvm->srcu, idx);
 
-		if (!r && kvm->arch.xen.shinfo_cache.active)
-			r = kvm_xen_shared_info_init(kvm);
-
 		mutex_unlock(&kvm->arch.xen.xen_lock);
 		break;
 	}
@@ -1144,7 +1148,13 @@ int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
 	bool lm = is_long_mode(vcpu);
 
 	/* Latch long_mode for shared_info pages etc. */
-	vcpu->kvm->arch.xen.long_mode = lm;
+	kvm->arch.xen.long_mode = lm;
+
+	if (kvm->arch.xen.shinfo_cache.active &&
+	    kvm_xen_shared_info_init(kvm)) {
+		mutex_unlock(&kvm->arch.xen.xen_lock);
+		return 1;
+	}
 
 	/*
 	 * If Xen hypercall intercept is enabled, fill the hypercall
-- 
2.39.2

Re: [PATCH 2/2] KVM: xen: (re-)initialize shared_info if guest (32/64-bit) mode is set

[Paul Durrant]

On 01/12/2023 10:45, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> If the shared_info PFN cache has already been initialized then the content
> of the shared_info page needs to be (re-)initialized if the guest mode is
> set. It is no lnger done when the PFN cache is activated.
> Setting the guest mode is either done explicitly by the VMM via the
> KVM_XEN_ATTR_TYPE_LONG_MODE attribute, or implicitly when the guest writes
> the MSR to set up the hypercall page.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
>   arch/x86/kvm/xen.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 7bead3f65e55..bfc8f6698cbc 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -624,8 +624,15 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
>   		} else {
>   			mutex_lock(&kvm->arch.xen.xen_lock);
>   			kvm->arch.xen.long_mode = !!data->u.long_mode;
> +
> +			/*
> +			 * If shared_info has already been initialized
> +			 * then re-initialize it with the new width.
> +			 */
> +			r = kvm->arch.xen.shinfo_cache.active ?
> +				kvm_xen_shared_info_init(kvm) : 0;
> +
>   			mutex_unlock(&kvm->arch.xen.xen_lock);
> -			r = 0;
>   		}
>   		break;
>   
> @@ -657,9 +664,6 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
>   		}
>   		srcu_read_unlock(&kvm->srcu, idx);
>   
> -		if (!r && kvm->arch.xen.shinfo_cache.active)
> -			r = kvm_xen_shared_info_init(kvm);
> -
>   		mutex_unlock(&kvm->arch.xen.xen_lock);
>   		break;
>   	}
> @@ -1144,7 +1148,13 @@ int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
>   	bool lm = is_long_mode(vcpu);
>   
>   	/* Latch long_mode for shared_info pages etc. */
> -	vcpu->kvm->arch.xen.long_mode = lm;
> +	kvm->arch.xen.long_mode = lm;
> +
> +	if (kvm->arch.xen.shinfo_cache.active &&
> +	    kvm_xen_shared_info_init(kvm)) {
> +		mutex_unlock(&kvm->arch.xen.xen_lock);

This unlock is bogus; it should have been removed. I'll send a v2.

   Paul

> +		return 1;
> +	}
>   
>   	/*
>   	 * If Xen hypercall intercept is enabled, fill the hypercall

Re: [PATCH 0/2] KVM: xen: update shared_info when long_mode is set

[Sean Christopherson]

On Fri, Dec 01, 2023, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> This series is based on my v9 of my "update shared_info and vcpu_info
> handling" series [1] and fixes an issue that was latent before the
> "allow shared_info to be mapped by fixed HVA" patch of that series allowed
> a VMM to set up shared_info before the VM booted and then leave it alone.

Uh, what?   If this is fixing an existing bug then it really shouldn't take a
dependency on a rather large and non-trivial series.  If the bug can only manifest
as a result of said series, then the fix absolutely belongs in that series.

This change from patch 1 in particular:

 -static int kvm_xen_shared_info_init(struct kvm *kvm, u64 addr, bool addr_is_gfn)
 +static int kvm_xen_shared_info_init(struct kvm *kvm)

practically screams for inclusion in that series which does:

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

Why not get the code right the first time instead of fixing it up in a completely
different series?

RE: [PATCH 0/2] KVM: xen: update shared_info when long_mode is set

[Durrant, Paul]

> -----Original Message-----
> From: Sean Christopherson <seanjc@google.com>
> Sent: 01 December 2023 16:46
> To: Paul Durrant <paul@xen.org>
> Cc: David Woodhouse <dwmw2@infradead.org>; Paolo Bonzini <pbonzini@redhat.com>; Thomas Gleixner
> <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; Borislav Petkov <bp@alien8.de>; Dave Hansen
> <dave.hansen@linux.intel.com>; x86@kernel.org; H. Peter Anvin <hpa@zytor.com>; kvm@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: RE: [EXTERNAL] [PATCH 0/2] KVM: xen: update shared_info when long_mode is set
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Fri, Dec 01, 2023, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > This series is based on my v9 of my "update shared_info and vcpu_info
> > handling" series [1] and fixes an issue that was latent before the
> > "allow shared_info to be mapped by fixed HVA" patch of that series allowed
> > a VMM to set up shared_info before the VM booted and then leave it alone.
> 
> Uh, what?   If this is fixing an existing bug then it really shouldn't take a
> dependency on a rather large and non-trivial series.  If the bug can only manifest
> as a result of said series, then the fix absolutely belongs in that series.
> 

There's been radio silence on that series for a while so I was unsure of the status.

> This change from patch 1 in particular:
> 
>  -static int kvm_xen_shared_info_init(struct kvm *kvm, u64 addr, bool addr_is_gfn)
>  +static int kvm_xen_shared_info_init(struct kvm *kvm)
> 
> practically screams for inclusion in that series which does:
> 
>  -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)
> 
> Why not get the code right the first time instead of fixing it up in a completely
> different series?

Sure, I can fold it in.

  Paul

Re: [PATCH 0/2] KVM: xen: update shared_info when long_mode is set

[Sean Christopherson]

On Fri, Dec 01, 2023, Paul Durrant wrote:
> > On Fri, Dec 01, 2023, Paul Durrant wrote:
> > > From: Paul Durrant <pdurrant@amazon.com>
> > >
> > > This series is based on my v9 of my "update shared_info and vcpu_info
> > > handling" series [1] and fixes an issue that was latent before the
> > > "allow shared_info to be mapped by fixed HVA" patch of that series allowed
> > > a VMM to set up shared_info before the VM booted and then leave it alone.
> > 
> > Uh, what?   If this is fixing an existing bug then it really shouldn't take a
> > dependency on a rather large and non-trivial series.  If the bug can only manifest
> > as a result of said series, then the fix absolutely belongs in that series.
> > 
> 
> There's been radio silence on that series for a while so I was unsure of the status.

v9 was posted the day before Thanksgiving, the week after plumbers, and a few
weeks after the merge window closed.  And it's an invasive series to some of KVM's
gnarliest code, i.e. it's not something that can be reviewed in passing.  We're
also entering both the holiday season and the end of the year when people get
sucked into annual reviews and whatnot.

I totally understand that it can be frustrating when upstream moves at a glacial
pace, but deviating from the established best practices is never going to speed
things up, and is almost always going to do the exact oppositie.