[PATCH 0/2] KVM: x86: Fix incorrect VM-exit profiling

[PATCH 0/2] KVM: x86: Fix incorrect VM-exit profiling

[Wei Zhang]

The profile=kvm boot option has been useful because it provides a
convenient approach to profile VM exits. However, it's problematic because
the profiling buffer is indexed by (pc - _stext), and a guest's pc minus a
host's _stext doesn't make sense in most cases.

When running another linux kernel in the guest, we could work around the
problem by disabling KASLR in both the host and the guest so they have the
same _stext. However, this is inconvenient and not always possible.

We're looking for a solution to this problem. A straightforward idea is to
pass the guest's _stext to the host so the profiling buffer can be indexed
correctly. This approach is quite brute, as you can see in the prototype
patches.

We had some initial discussions and here is a short summary:
1. The VM-exit profiling is already hacky. It's collecting stats about all
   KVM guests bunched together into a single global buffer without any
   separation.
2. Even if we pass _stext from the guest, there are still a lot of
   limitations: There can be only one running guest, and the size of its
   text region shouldn't exceed the size of the profiling buffer,
   which is (_etext - _stext) in the host.
3. There are other methods for profiling VM exits, but it would be really
   convenient if readprofile just works out of box for KVM profiling.

It would be awesome to hear more thoughts on this. Should we try to fix the
existing VM-exit profiling functionility? Or should we avoid adding more
hacks there? If it should be fixed, what's the preferred way? Thanks in
advance for any suggestions.

Wei Zhang (2):
  KVM: x86: allow guest to send its _stext for kvm profiling
  KVM: x86: illustrative example for sending guest _stext with a
    hypercall

 arch/x86/kernel/setup.c       |  6 ++++++
 arch/x86/kvm/x86.c            | 15 +++++++++++++++
 include/linux/kvm_host.h      |  4 ++++
 include/uapi/linux/kvm_para.h |  1 +
 virt/kvm/Kconfig              |  5 +++++
 5 files changed, 31 insertions(+)

base-commit: 42dcbe7d8bac997eef4c379e61d9121a15ed4e36
-- 
2.35.1.1178.g4f1659d476-goog

[PATCH 1/2] KVM: x86: allow guest to send its _stext for kvm profiling

[Wei Zhang]

The profiling buffer is indexed by (pc - _stext) in do_profile_hits(),
which doesn't work for KVM profiling because the pc represents an address
in the guest kernel. readprofile is broken in this case, unless the guest
kernel happens to have the same _stext as the host kernel.

This patch adds a new hypercall so guests could send its _stext to the
host, which will then be used to adjust the calculation for KVM profiling.

Signed-off-by: Wei Zhang <zhanwei@google.com>
---
 arch/x86/kvm/x86.c            | 15 +++++++++++++++
 include/linux/kvm_host.h      |  4 ++++
 include/uapi/linux/kvm_para.h |  1 +
 virt/kvm/Kconfig              |  5 +++++
 4 files changed, 25 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 547ba00ef64f..abeacdd5d362 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9246,6 +9246,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 		vcpu->arch.complete_userspace_io = complete_hypercall_exit;
 		return 0;
 	}
+#ifdef CONFIG_ACCURATE_KVM_PROFILING
+	case KVM_HC_GUEST_STEXT:
+		vcpu->kvm->guest_stext = a0;
+		ret = 0;
+		break;
+#endif
 	default:
 		ret = -KVM_ENOSYS;
 		break;
@@ -10261,6 +10267,15 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	 */
 	if (unlikely(prof_on == KVM_PROFILING)) {
 		unsigned long rip = kvm_rip_read(vcpu);
+#ifdef CONFIG_ACCURATE_KVM_PROFILING
+		/*
+		 * Profiling buffer is indexed by (rip - _stext), but it's
+		 * supposed to be indexed by (rip - guest_stext) instead.
+		 * Therefore apply an offest in advance to get correct results.
+		 */
+		if (vcpu->kvm->guest_stext)
+			rip += (unsigned long)_stext - vcpu->kvm->guest_stext;
+#endif
 		profile_hit(KVM_PROFILING, (void *)rip);
 	}
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 3f9b22c4983a..65caaa4d87c4 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -781,6 +781,10 @@ struct kvm {
 	struct notifier_block pm_notifier;
 #endif
 	char stats_id[KVM_STATS_NAME_SIZE];
+
+#ifdef CONFIG_ACCURATE_KVM_PROFILING
+	unsigned long guest_stext;
+#endif
 };
 
 #define kvm_err(fmt, ...) \
diff --git a/include/uapi/linux/kvm_para.h b/include/uapi/linux/kvm_para.h
index 960c7e93d1a9..dcb4ba1f033c 100644
--- a/include/uapi/linux/kvm_para.h
+++ b/include/uapi/linux/kvm_para.h
@@ -30,6 +30,7 @@
 #define KVM_HC_SEND_IPI		10
 #define KVM_HC_SCHED_YIELD		11
 #define KVM_HC_MAP_GPA_RANGE		12
+#define KVM_HC_GUEST_STEXT		13
 
 /*
  * hypercalls use architecture specific
diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
index a8c5c9f06b3c..8798f75ddade 100644
--- a/virt/kvm/Kconfig
+++ b/virt/kvm/Kconfig
@@ -72,3 +72,8 @@ config KVM_XFER_TO_GUEST_WORK
 
 config HAVE_KVM_PM_NOTIFIER
        bool
+
+# Offer an additional hypercall to a guest so it could pass value of _stext to
+# host, which will be used to adjust the calculation of KVM profiling.
+config ACCURATE_KVM_PROFILING
+       bool
-- 
2.35.1.1178.g4f1659d476-goog

[PATCH 2/2] KVM: x86: illustrative example for sending guest _stext with a hypercall

[Wei Zhang]

A guest could send its _stext with the newly added hypercall. The
statistics collected afterwards will be correct in the kernel.

Signed-off-by: Wei Zhang <zhanwei@google.com>
---
 arch/x86/kernel/setup.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index c95b9ac5a457..ee8c4fd4efe9 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -24,6 +24,7 @@
 #include <linux/static_call.h>
 #include <linux/swiotlb.h>
 
+#include <uapi/linux/kvm_para.h>
 #include <uapi/linux/mount.h>
 
 #include <xen/xen.h>
@@ -1241,6 +1242,11 @@ void __init setup_arch(char **cmdline_p)
 #endif
 
 	unwind_init();
+
+	/*
+	 * Send the _stext to the host kernel.
+	 */
+	kvm_hypercall1(KVM_HC_GUEST_STEXT, (unsigned long)_stext);
 }
 
 #ifdef CONFIG_X86_32
-- 
2.35.1.1178.g4f1659d476-goog

Re: [PATCH 1/2] KVM: x86: allow guest to send its _stext for kvm profiling

[Sean Christopherson]

On Tue, Apr 12, 2022, Wei Zhang wrote:
> The profiling buffer is indexed by (pc - _stext) in do_profile_hits(),
> which doesn't work for KVM profiling because the pc represents an address
> in the guest kernel. readprofile is broken in this case, unless the guest
> kernel happens to have the same _stext as the host kernel.
> 
> This patch adds a new hypercall so guests could send its _stext to the
> host, which will then be used to adjust the calculation for KVM profiling.

Disclaimer, I know nothing about using profiling.

Why not just omit the _stext adjustment and profile the raw guest RIP?  It seems
like userspace needs to know about the guest layout in order to make use of profling
info, so why not report raw info and let host userspace do all adjustments?

> Signed-off-by: Wei Zhang <zhanwei@google.com>
> ---
>  arch/x86/kvm/x86.c            | 15 +++++++++++++++
>  include/linux/kvm_host.h      |  4 ++++
>  include/uapi/linux/kvm_para.h |  1 +
>  virt/kvm/Kconfig              |  5 +++++
>  4 files changed, 25 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 547ba00ef64f..abeacdd5d362 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9246,6 +9246,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>  		vcpu->arch.complete_userspace_io = complete_hypercall_exit;
>  		return 0;
>  	}
> +#ifdef CONFIG_ACCURATE_KVM_PROFILING
> +	case KVM_HC_GUEST_STEXT:
> +		vcpu->kvm->guest_stext = a0;

Rather than snapshot the guest offset, snapshot the delta.  E.g.

		vcpu->kvm->arch.guest_stext_offset = (unsigned long)_stext - a0;

Then the profiling flow can just be

		unsigned long rip;

		rip = kvm_rip_read(vcpu) + vcpu->kvm->arch.guest_text_offset;
		profile_hit(KVM_PROFILING, (void *)rip);


> +		ret = 0;
> +		break;
> +#endif
>  	default:
>  		ret = -KVM_ENOSYS;
>  		break;
> @@ -10261,6 +10267,15 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  	 */
>  	if (unlikely(prof_on == KVM_PROFILING)) {
>  		unsigned long rip = kvm_rip_read(vcpu);
> +#ifdef CONFIG_ACCURATE_KVM_PROFILING

A Kconfig, and really any #define, is completely unnecessary.  This is all x86
code, just throw the offest into struct kvm_arch.

Re: [PATCH 0/2] KVM: x86: Fix incorrect VM-exit profiling

[Sean Christopherson]

On Tue, Apr 12, 2022, Wei Zhang wrote:
> The profile=kvm boot option has been useful because it provides a
> convenient approach to profile VM exits.

What exactly are you profiling?  Where the guest executing at any given exit?  Mostly
out of curiosity, but also in the hope that we might be able to replace profiling with
a dedicated KVM stat(s).

Re: [PATCH 0/2] KVM: x86: Fix incorrect VM-exit profiling

[Wei Zhang]

Yes, the profiling is about finding out which instructions in the
guest trigger VM exits and the corresponding frequencies.

Basically this will give a histogram array in /proc/profile. So if
'array[A] == T', we know that the instruction at (_stext + A) triggers
VM exits T times. readprofile command could read the information and
show a summary.




On Tue, May 10, 2022 at 1:57 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Apr 12, 2022, Wei Zhang wrote:
> > The profile=kvm boot option has been useful because it provides a
> > convenient approach to profile VM exits.
>
> What exactly are you profiling?  Where the guest executing at any given exit?  Mostly
> out of curiosity, but also in the hope that we might be able to replace profiling with
> a dedicated KVM stat(s).

Re: [PATCH 1/2] KVM: x86: allow guest to send its _stext for kvm profiling

[Wei Zhang]

On Tue, May 10, 2022 at 1:55 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Apr 12, 2022, Wei Zhang wrote:
> > The profiling buffer is indexed by (pc - _stext) in do_profile_hits(),
> > which doesn't work for KVM profiling because the pc represents an address
> > in the guest kernel. readprofile is broken in this case, unless the guest
> > kernel happens to have the same _stext as the host kernel.
> >
> > This patch adds a new hypercall so guests could send its _stext to the
> > host, which will then be used to adjust the calculation for KVM profiling.
>
> Disclaimer, I know nothing about using profiling.
>
> Why not just omit the _stext adjustment and profile the raw guest RIP?  It seems
> like userspace needs to know about the guest layout in order to make use of profling
> info, so why not report raw info and let host userspace do all adjustments?

It's hard to store raw IPs if we want to reuse the existing profiling
facility. The profiling function is initially used to store the
current IP at each clock tick for the host kernel.

The original design avoided the trouble of storing raw IPs by creating
a buffer array with a length of (_etext - _stext) and do buffer[IP -
_stext]++ at each clock tick. In the user space, the readprofile
command could read it from /proc/profile and tell us roughly how many
ticks occurred in each kernel function with a map file. (IP - _stext)
has a clear meaning here since it gives us an offset with respect to
the start of the text segment. This gets tricky after the profile=kvm
boot option was introduced
(https://github.com/torvalds/linux/commit/07031e14) because (IP -
_stext) is no longer meaningful.

I think raw guest IPs are easy to consume by userspace tools. But we
probably need to go with a different approach if we want to store raw
guest IPs.

Re: [PATCH 0/2] KVM: x86: Fix incorrect VM-exit profiling

[Sean Christopherson]

+Jing and David

On Wed, May 11, 2022, Wei Zhang wrote:

Please don't top-post.  From https://people.kernel.org/tglx/notes-about-netiquette:

  A: Because it messes up the order in which people normally read text.
  Q: Why is top-posting such a bad thing?

  A: Top-posting.
  Q: What is the most annoying thing in e-mail?

  A: No.
  Q: Should I include quotations after my reply?


> On Tue, May 10, 2022 at 1:57 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Tue, Apr 12, 2022, Wei Zhang wrote:
> > > The profile=kvm boot option has been useful because it provides a
> > > convenient approach to profile VM exits.
> >
> > What exactly are you profiling?  Where the guest executing at any given exit?  Mostly
> > out of curiosity, but also in the hope that we might be able to replace profiling with
> > a dedicated KVM stat(s).
>
> Yes, the profiling is about finding out which instructions in the
> guest trigger VM exits and the corresponding frequencies.

Do you actually what to profile which instructions _trigger_ exits?  Because that's
not what this does.  This profiles every exit, regardless of whether or not the exit
was due to a guest action.  E.g. host IRQs/NMIs, page faults, etc... will all get
included and pollute the profile.  Over time, the signal-to-noise ratio will likely
improve, but there's definitely still going to be noise.

We actually tried to upstream histograms for exit reasons[*] (link is for arm64,
but we want it for x86 too, just can't find a link), but it was deemed too expensive
in terms of memory cost for general use.

An idea that's on our (GCP folks) todo list is to explore adding an eBPF hook into
the exit path that would allow userspace to inspect e.g. struct kvm_run on VM-Exit.
That would allow userspace to collect all kinds of info about VM-Exits without
committing to ABI beyond kvm_run, and without bloating the size of a vCPU for
environments that don't want detailed histograms/profiling.

My preference would be to find a more complete, KVM-specific solution.  The
profiling stuff seems like it's a dead end, i.e. will always be flawed in some
way.  If this cleanup didn't require a new hypercall then I wouldn't care, but
I don't love having to extend KVM's guest/host ABI for something that ideally
will become obsolete sooner than later.

[*] https://lore.kernel.org/all/20210922010851.2312845-3-jingzhangos@google.com

Re: [PATCH 0/2] KVM: x86: Fix incorrect VM-exit profiling

[Wei Zhang]

> Please don't top-post.  From https://people.kernel.org/tglx/notes-about-netiquette:

Ah, I didn't know this should be avoided. Thanks for the info!

> My preference would be to find a more complete, KVM-specific solution.  The
> profiling stuff seems like it's a dead end, i.e. will always be flawed in some
> way.  If this cleanup didn't require a new hypercall then I wouldn't care, but
> I don't love having to extend KVM's guest/host ABI for something that ideally
> will become obsolete sooner than later.

I also feel that adding a new hypercall is too much here. A
KVM-specific solution is definitely better, and the eBPF based
approach you mentioned sounds like the ultimate solution (at least for
inspecting exit reasons).

+Suleiman What do you think? The on-going work Sean described sounds
promising, perhaps we should put this patch aside for the time being.

Re: [PATCH 0/2] KVM: x86: Fix incorrect VM-exit profiling

[Suleiman Souhlal]

On Tue, May 17, 2022 at 4:30 AM Wei Zhang <zhanwei@google.com> wrote:
>
> > Please don't top-post.  From https://people.kernel.org/tglx/notes-about-netiquette:
>
> Ah, I didn't know this should be avoided. Thanks for the info!
>
> > My preference would be to find a more complete, KVM-specific solution.  The
> > profiling stuff seems like it's a dead end, i.e. will always be flawed in some
> > way.  If this cleanup didn't require a new hypercall then I wouldn't care, but
> > I don't love having to extend KVM's guest/host ABI for something that ideally
> > will become obsolete sooner than later.
>
> I also feel that adding a new hypercall is too much here. A
> KVM-specific solution is definitely better, and the eBPF based
> approach you mentioned sounds like the ultimate solution (at least for
> inspecting exit reasons).
>
> +Suleiman What do you think? The on-going work Sean described sounds
> promising, perhaps we should put this patch aside for the time being.

I'm ok with that.
That said, the advantage of the current solution is that it already
exists and is very easy to use, by anyone, without having to write any
code. The proposed solution doesn't sound like it will be as easy.

Regarding the earlier question about wanting to know which
instructions trigger exits, most times I've needed to get exit
profiles, I actually wanted to know where the guest was at the time of
the exit, regardless of who triggered the exit.

-- Suleiman

Re: [PATCH 0/2] KVM: x86: Fix incorrect VM-exit profiling

[Sean Christopherson]

On Wed, May 18, 2022, Suleiman Souhlal wrote:
> On Tue, May 17, 2022 at 4:30 AM Wei Zhang <zhanwei@google.com> wrote:
> >
> > > Please don't top-post.  From https://people.kernel.org/tglx/notes-about-netiquette:
> >
> > Ah, I didn't know this should be avoided. Thanks for the info!
> >
> > > My preference would be to find a more complete, KVM-specific solution.  The
> > > profiling stuff seems like it's a dead end, i.e. will always be flawed in some
> > > way.  If this cleanup didn't require a new hypercall then I wouldn't care, but
> > > I don't love having to extend KVM's guest/host ABI for something that ideally
> > > will become obsolete sooner than later.
> >
> > I also feel that adding a new hypercall is too much here. A
> > KVM-specific solution is definitely better, and the eBPF based
> > approach you mentioned sounds like the ultimate solution (at least for
> > inspecting exit reasons).
> >
> > +Suleiman What do you think? The on-going work Sean described sounds
> > promising, perhaps we should put this patch aside for the time being.
> 
> I'm ok with that.
> That said, the advantage of the current solution is that it already
> exists and is very easy to use, by anyone, without having to write any
> code. The proposed solution doesn't sound like it will be as easy.

My goal/hope is to make the eBPF approach just as easy by providing/building a
library of KVM eBPF programs in tools/ so that doing common things like profiling
VM-Exits doesn't require reinventing the wheel.  And those programs could be used
(and thus implicitly tested) by KVM selftests to verify the kernel functionality.