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;
