Re: [PATCH v3 1/5] KVM: Add arch specific interfaces for sampling guest callchains

[Marc Zyngier]

On Sun, 10 Dec 2023 08:12:18 +0000,
Tianyi Liu <i.pear@xxxxxxxxxxx> wrote:
> 
> This patch adds two architecture specific interfaces used by `perf kvm`:
> 
> - kvm_arch_vcpu_get_unwind_info: Return required data for unwinding
>   at once; including ip address, frame pointer, whether the guest vCPU
>   is running in 32 or 64 bits, and possibly the base addresses of
>   the segments.
> 
> - kvm_arch_vcpu_read_virt: Read data from a virtual address of the
>   guest vm.
> 
> `perf_kvm.h` has been added to the `include/linux/` directory to store
> the interface structures between the perf events subsystem and the KVM
> subsystem.
> 
> Since arm64 hasn't provided some foundational infrastructure, stub the
> arm64 implementation for now because it's a bit complex.

It's not complex. It is *unsafe*. Do you see the difference?

> 
> The above interfaces require architecture support for
> `CONFIG_GUEST_PERF_EVENTS`, which is only implemented by x86 and arm64
> currently. For more architectures, they need to implement these interfaces
> when enabling `CONFIG_GUEST_PERF_EVENTS`.
> 
> In terms of safety, guests are designed to be read-only in this feature,
> and we will never inject page faults into the guests, ensuring that the
> guests are not interfered by profiling. In extremely rare cases, if the
> guest is modifying the page table, there is a possibility of reading
> incorrect data. Additionally, if certain programs running in the guest OS
> do not support frame pointers, it may also result in some erroneous data.
> These erroneous data will eventually appear as `[unknown]` entries in the
> report. It is sufficient as long as most of the records are correct for
> profiling.
> 
> Signed-off-by: Tianyi Liu <i.pear@xxxxxxxxxxx>
> ---
>  MAINTAINERS              |  1 +
>  arch/arm64/kvm/arm.c     | 12 ++++++++++++
>  arch/x86/kvm/x86.c       | 24 ++++++++++++++++++++++++
>  include/linux/kvm_host.h |  5 +++++
>  include/linux/perf_kvm.h | 18 ++++++++++++++++++
>  5 files changed, 60 insertions(+)
>  create mode 100644 include/linux/perf_kvm.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 788be9ab5b73..5ee36b4a9701 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16976,6 +16976,7 @@ F:	arch/*/kernel/*/perf_event*.c
>  F:	arch/*/kernel/perf_callchain.c
>  F:	arch/*/kernel/perf_event*.c
>  F:	include/linux/perf_event.h
> +F:	include/linux/perf_kvm.h
>  F:	include/uapi/linux/perf_event.h
>  F:	kernel/events/*
>  F:	tools/lib/perf/
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index e5f75f1f1085..5ae74b5c263a 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -574,6 +574,18 @@ unsigned long kvm_arch_vcpu_get_ip(struct kvm_vcpu *vcpu)
>  {
>  	return *vcpu_pc(vcpu);
>  }
> +
> +bool kvm_arch_vcpu_get_unwind_info(struct kvm_vcpu *vcpu, struct perf_kvm_guest_unwind_info *info)
> +{
> +	/* TODO: implement */
> +	return false;
> +}
> +
> +bool kvm_arch_vcpu_read_virt(struct kvm_vcpu *vcpu, gva_t addr, void *dest, unsigned int length)
> +{
> +	/* TODO: implement */
> +	return false;
> +}

I don't do it very often, but the only thing I can say about this is
*NAK*.

You have decided to ignore the previous review comments, which is your
prerogative. However, I absolutely refuse to add half baked and
*dangerous* stuff to the arm64's version of KVM.

If you can convince the x86 folks that they absolutely want this, fine
by me. But this need to be a buy-in interface, not something that is
required for each and every architecture to have stubs, wrongly
suggesting that extra work is needed.

For arm64, the way to go is to have this in userspace. Which is both
easy to implement and safe. And until we have such a userspace
implementation as a baseline, I will not consider a kernel
version.

	M.

-- 
Without deviation from the norm, progress is not possible.