From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 66DB9290C; Sun, 10 Dec 2023 12:16:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="XNGPxpqM" Received: by smtp.kernel.org (Postfix) with ESMTPSA id AF365C433C9; Sun, 10 Dec 2023 12:16:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1702210596; bh=zI/jeFRyjHplsqmull7ZiNDfoFDcHnPQwoXdDnLIe58=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=XNGPxpqMpKbxiZDIIwqaAP4vnfVrNqeUImkpUgaTvVibWyF+2Td6NYdBTgGCG8ACy eXA05rq0mJvnlQ7bT6gFshJKSrDZ4rRxoaTWphSdn0dyipzHpkw3agwiO4oRHtBnD7 XdrNR6lyCoQZdaOWT6KN330vSSuhzLMDIw7VHQVZNG1PW9RPfHNniNnkap6e+aI6M5 sUdbMPxuS88tY+Ql086/6F63E7KcTUMRyDs0Kxi7qFZyt46iJ4X5z5zftq1SAiXQyX CwA9Nh7Xs5MLIRJvzRv+zGsx2zYl5k+gNoRbrnoZgDDskHAaZyCWViXyAjstMkwJJ4 2EEHczNzYYg+g== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1rCIja-002tai-3P; Sun, 10 Dec 2023 12:16:34 +0000 Date: Sun, 10 Dec 2023 12:16:32 +0000 Message-ID: <865y16b6cf.wl-maz@kernel.org> From: Marc Zyngier To: Tianyi Liu Cc: seanjc@google.com, pbonzini@redhat.com, peterz@infradead.org, mingo@redhat.com, acme@kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, kvm@vger.kernel.org, x86@kernel.org, mark.rutland@arm.com, mlevitsk@redhat.com, alexander.shishkin@linux.intel.com, jolsa@kernel.org, namhyung@kernel.org, irogers@google.com, adrian.hunter@intel.com Subject: Re: [PATCH v3 1/5] KVM: Add arch specific interfaces for sampling guest callchains In-Reply-To: References: User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Precedence: bulk X-Mailing-List: linux-perf-users@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: i.pear@outlook.com, seanjc@google.com, pbonzini@redhat.com, peterz@infradead.org, mingo@redhat.com, acme@kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, kvm@vger.kernel.org, x86@kernel.org, mark.rutland@arm.com, mlevitsk@redhat.com, alexander.shishkin@linux.intel.com, jolsa@kernel.org, namhyung@kernel.org, irogers@google.com, adrian.hunter@intel.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false On Sun, 10 Dec 2023 08:12:18 +0000, Tianyi Liu 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 > --- > 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.