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

[Sean Christopherson <seanjc@google.com>]

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;