[PATCH] Add kvm_arch helper functions for guests' callchains

[PATCH] Add kvm_arch helper functions for guests' callchains

[Tianyi Liu]

Hi Sean and Paolo,

This patch serves as the foundation for enabling callchains for guests,
(used by `perf kvm`). This functionality is useful for me, and I
noticed it holds the top spot on the perf wiki TODO list [1], so I'd like
to implement it. This patch introduces several arch-related kvm helper
functions, which will be later used for guest stack frame profiling.
This also contains the implementation for x86 platform, while arm64 will
be implemented later.

This is part of a series of patches. Since these patches are spread across
various modules like perf, kvm, arch/*, I plan to first submit some
foundational patches and gradually submit the complete implementation.
The full implementation can be found at [2], and it has been tested on
an x86_64 machine.

Specifically, performing stack frame sampling for the guest OS requires
the following helper functions:

* `kvm_arch_vcpu_get_fp` for getting the frame pointer from guest,
such as `rbp` in x86_64 or `x29` in aarch64.

* `kvm_arch_vcpu_read_virt` for reading virtual memory address from guest,
used for unwinding guest's stack.

* `kvm_arch_vcpu_is_64bit` for checking if the guest is running in 64-bit
mode. Stack unwinding needs this to know the size of a stack frame. This
will be used by the `kvm_guest_state()` interface.

After the arm64 implementation be ready, I will use these in
`virt/kvm/kvm_main.c`, and add two callbacks `.get_fp` and `.read_virt`
into `struct perf_guest_info_callbacks`, which will be used in perf.
Sean, I noticed you've previously done some refactoring on this code [3],
do you think there are any issues with the way it was done?

This is my first time submitting a patch for a new feature in the kernel,
and I would greatly appreciate any suggestions.

[1] https://perf.wiki.kernel.org/index.php/Todo
[2] https://github.com/i-Pear/linux/pull/2/files
[3] https://lore.kernel.org/all/20211111020738.2512932-13-seanjc@google.com/

Signed-off-by: Tianyi Liu <i.pear@outlook.com>
---
 arch/x86/kvm/kvm_cache_regs.h |  5 +++++
 arch/x86/kvm/x86.c            | 18 ++++++++++++++++++
 include/linux/kvm_host.h      |  3 +++
 3 files changed, 26 insertions(+)

diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
index 75eae9c4998a..c73acecc7ef9 100644
--- a/arch/x86/kvm/kvm_cache_regs.h
+++ b/arch/x86/kvm/kvm_cache_regs.h
@@ -133,6 +133,11 @@ static inline void kvm_rsp_write(struct kvm_vcpu *vcpu, unsigned long val)
 	kvm_register_write_raw(vcpu, VCPU_REGS_RSP, val);
 }
 
+static inline unsigned long kvm_fp_read(struct kvm_vcpu *vcpu)
+{
+	return kvm_register_read_raw(vcpu, VCPU_REGS_RBP);
+}
+
 static inline u64 kvm_pdptr_read(struct kvm_vcpu *vcpu, int index)
 {
 	might_sleep();  /* on svm */
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c381770bcbf1..2fd3850b1673 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12902,6 +12902,24 @@ unsigned long kvm_arch_vcpu_get_ip(struct kvm_vcpu *vcpu)
 	return kvm_rip_read(vcpu);
 }
 
+unsigned long kvm_arch_vcpu_get_fp(struct kvm_vcpu *vcpu)
+{
+	return kvm_fp_read(vcpu);
+}
+
+bool kvm_arch_vcpu_read_virt(struct kvm_vcpu *vcpu, void *addr, void *dest, unsigned int length)
+{
+	struct x86_exception e;
+
+	/* Return true on success */
+	return kvm_read_guest_virt(vcpu, addr, dest, length, &e) == X86EMUL_CONTINUE;
+}
+
+bool kvm_arch_vcpu_is_64bit(struct kvm_vcpu *vcpu)
+{
+	return is_64_bit_mode(vcpu);
+}
+
 int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
 {
 	return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 9d3ac7720da9..5874710f3691 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1562,6 +1562,9 @@ static inline bool kvm_arch_intc_initialized(struct kvm *kvm)
 
 #ifdef CONFIG_GUEST_PERF_EVENTS
 unsigned long kvm_arch_vcpu_get_ip(struct kvm_vcpu *vcpu);
+unsigned long kvm_arch_vcpu_get_fp(struct kvm_vcpu *vcpu);
+bool kvm_arch_vcpu_read_virt(struct kvm_vcpu *vcpu, void *addr, void *dest, unsigned int length);
+bool kvm_arch_vcpu_is_64bit(struct kvm_vcpu *vcpu);
 
 void kvm_register_perf_callbacks(unsigned int (*pt_intr_handler)(void));
 void kvm_unregister_perf_callbacks(void);
-- 
2.34.1

Re: [PATCH] Add kvm_arch helper functions for guests' callchains

[Sean Christopherson]

Apologies for the belated feedback, I remember seeing this fly by and could have
sworn I responded, but obviously did not.

On Thu, Aug 31, 2023, Tianyi Liu wrote:
> Hi Sean and Paolo,
> 
> This patch serves as the foundation for enabling callchains for guests,
> (used by `perf kvm`). This functionality is useful for me, and I
> noticed it holds the top spot on the perf wiki TODO list [1], so I'd like
> to implement it. This patch introduces several arch-related kvm helper
> functions, which will be later used for guest stack frame profiling.
> This also contains the implementation for x86 platform, while arm64 will
> be implemented later.
> 
> This is part of a series of patches. Since these patches are spread across
> various modules like perf, kvm, arch/*, I plan to first submit some
> foundational patches and gradually submit the complete implementation.
> The full implementation can be found at [2], and it has been tested on
> an x86_64 machine.

Please post the whole thing, or at least enough to actually show the end-to-end
usage.  I suspect the main reason you heard crickets for almost two months is
that's there's nothing actionable to do with this.  This certainly can't be
applied as-is since there is no usage, and it's almost impossible to review
something when only a small sliver is visibible.

> Sean, I noticed you've previously done some refactoring on this code [3],
> do you think there are any issues with the way it was done?

I'd have to look at the whole thing, and to be honest I don't want to pull down
from github to do that.

> diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
> index 75eae9c4998a..c73acecc7ef9 100644
> --- a/arch/x86/kvm/kvm_cache_regs.h
> +++ b/arch/x86/kvm/kvm_cache_regs.h
> @@ -133,6 +133,11 @@ static inline void kvm_rsp_write(struct kvm_vcpu *vcpu, unsigned long val)
>  	kvm_register_write_raw(vcpu, VCPU_REGS_RSP, val);
>  }
>  
> +static inline unsigned long kvm_fp_read(struct kvm_vcpu *vcpu)
> +{
> +	return kvm_register_read_raw(vcpu, VCPU_REGS_RBP);
> +}
> +
>  static inline u64 kvm_pdptr_read(struct kvm_vcpu *vcpu, int index)
>  {
>  	might_sleep();  /* on svm */
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c381770bcbf1..2fd3850b1673 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -12902,6 +12902,24 @@ unsigned long kvm_arch_vcpu_get_ip(struct kvm_vcpu *vcpu)
>  	return kvm_rip_read(vcpu);
>  }
>  
> +unsigned long kvm_arch_vcpu_get_fp(struct kvm_vcpu *vcpu)

Take this with a grain of salt this I can't see the big picture, but I recommend
spelling out frame_pointer.  At first glance I read this as "kvm_arch_vcpu_get_fpu()"
and was all kinds of confused.

This and kvm_arch_vcpu_is_64bit() can also be inlined functions.

> +{
> +	return kvm_fp_read(vcpu);

No need for a dedicated kvm_fp_read(), just open code the read of RBP here.

> +}
> +
> +bool kvm_arch_vcpu_read_virt(struct kvm_vcpu *vcpu, void *addr, void *dest, unsigned int length)

This should needn't an arch hook, though again I can't see the big picture.

> +{
> +	struct x86_exception e;
> +
> +	/* Return true on success */
> +	return kvm_read_guest_virt(vcpu, addr, dest, length, &e) == X86EMUL_CONTINUE;
> +}
> +
> +bool kvm_arch_vcpu_is_64bit(struct kvm_vcpu *vcpu)
> +{
> +	return is_64_bit_mode(vcpu);
> +}
> +
>  int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>  {
>  	return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE;