Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
From:   Tianyi Liu <i.pear@outlook.com>
To:     mlevitsk@redhat.com
Cc:     acme@kernel.org, adrian.hunter@intel.com,
        alexander.shishkin@linux.intel.com, i.pear@outlook.com,
        irogers@google.com, jolsa@kernel.org, kvm@vger.kernel.org,
        kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        mark.rutland@arm.com, mingo@redhat.com, namhyung@kernel.org,
        pbonzini@redhat.com, peterz@infradead.org, seanjc@google.com,
        x86@kernel.org
Subject: Re: [PATCH v2 4/5] perf kvm: Support sampling guest callchains
Date:   Wed, 11 Oct 2023 22:44:55 +0800
Message-ID: <SY4P282MB1084293EB22AB6FBF72DA0599DCCA@SY4P282MB1084.AUSP282.PROD.OUTLOOK.COM>
In-Reply-To: <ce964b43f926708f30c85640591b2fc62397b719.camel@redhat.com>
References: <ce964b43f926708f30c85640591b2fc62397b719.camel@redhat.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: SY4P282MB1084:EE_|MEYP282MB4270:EE_
X-MS-Office365-Filtering-Correlation-Id: bf82b402-d673-4f3c-346a-08dbca68cedd
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: 8Mtiy/4+8nxePELceqHkyV2VIKRZZZKlw/IBJ+7YKhNdB7K65FDm8KBbFYevgSST3Pu1PHitECUtxlso2wTFJdPSx7Wke2JF1mg+z49/ABxF3uADOpQdBBfQU96GibrPw1wT+A1HgcrI1uTizxU0eKeXliWQ0DNsRIwKVMtcf1e+KSpj9C/stqubi0L1NetIV7yWbJdH/LQfw++7Q05ZK1Tthfubwo6k22OsXrsM0adlfqo0sc1Urwiu4YeLdDUL+0mVHCClAHHTdfgxlv1Nutp8KNsOVJggvkiY4cN3+Vdk31hodMjoRmQfYms+XZ8vbCJ08ZVPe/W1Dm7qpanFLJZTjZXh9jkoxJGI0oX09lASCJ+C+M70dGi/MA5s6bKptQmGV8KGR9rCL0JEPqnksqi7J8e5gRczLBmJ50FB12+qW73rho788qn53C98djt2c8af0CKxvlfyPwK8ul0ZbhOAE6QncJ2aUVTTJ9oa99hUGJ6So65r9LjTgd52+DcHUE6ieF/CzLCQOcHkUAhM2BpH3MgMebwzeiixV6Q5PLhyyiTG+SsoBPfhEs7tYKks8dnZlTCQwPhcIJusaR54lG3TFXhpF4rWEr8EITwe1pAcB10pTiSaNCmMpeGRCMmK6ohkZPJRsi450MnlUcUvScbu0mSCEKNbSeWXmLHH6L8=
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?0klE0HF+fBCtKN1M3+nOJGxQzgBDvzHl60CUdkNNZksOHCHoFqNr6MkJ/Tzi?=
 =?us-ascii?Q?6GJ9lsgnuV0ek2feMmbBahwvTbYm0HxcBI3izVNugdMyDnHQLqtj4Niq444i?=
 =?us-ascii?Q?LNVkqIc8HF/jGUVEjB/ufAS0+IPxeQdoUcQnA4lG/+Voj8QO5gjUa0Cxt2Ib?=
 =?us-ascii?Q?x/2YAZ/JVu2MWGJ+S9NxDBZmSqfX/InSLfCDEtHSNhINO1j94X6xZ0xQFAhK?=
 =?us-ascii?Q?kV8yJG+2wM/+3vwGl1oxCkHc3vvFQS0uJV0082JmcXkfy4PISf/gJL37cRqS?=
 =?us-ascii?Q?DfVwG4gsD78W51IgY/KP6ev8wmCZM0v/HUnO8HRHs+6IgIgnzXtPs9RlPRSC?=
 =?us-ascii?Q?daDhz9WK2k+0IScEPP3XZ5jK8V51KUmkzWRAV3I1M6r58KTPZOOR32qOGFFu?=
 =?us-ascii?Q?2A8IHHltNqFQW/ESEFXmxU20csrjpDa2g7uMfzSs+148vwg7bA+9j4msQAnx?=
 =?us-ascii?Q?+35zlgz0YwGQqcKSzL/1AXhqMoc34QLwvrv64wlFBTSpuycZ3RXyhNBNJijo?=
 =?us-ascii?Q?PVpEq8RqxLgRi6LkX0EDAclKmJzu3RZIdo3/cuLrujeTuFvdyDoza2rjlHch?=
 =?us-ascii?Q?oBDohmVGB/y9lwFOUad1HKMXlN3TWcsYp7CENb8bzBR8ruh6/tp7aDYRJDdA?=
 =?us-ascii?Q?fJ+7v57hS75lXzDF4/65oE7b2BK7fcduIzs41uOo/DxSE5Jc3T+8Bh/WXL2W?=
 =?us-ascii?Q?kM3ZKMvjiyQ+Jhf0fSpKpLDhD1yk/5NBZ6nRGgEneViIFcM5zcMsZwfOdeyk?=
 =?us-ascii?Q?sGB/Tx6rloCJ+jmPrYOkkZ5fwGRlaIQsDheJPupQVlXZfdD384CIuVdq0UV4?=
 =?us-ascii?Q?i2q4LT6VfmypJjcGVPnBRPYlR8Y9XLav85Y3ByfdZj/DrOFR4lqAqwy7VFo8?=
 =?us-ascii?Q?2Y0croLwd+XKpeI26Pu+AxXKhTLYvn0WXIWLAR8Jre+fNt+3Q+jZkMweJD1W?=
 =?us-ascii?Q?Peen+5xw/r5ATA+D6oDRmf2G+r2Y3nBkLr1byRTGEwwQKF5bPuRzpS0fmg/U?=
 =?us-ascii?Q?DN3g9V0KXfj+iQfPmY5xQ6w0U7W0L5IV4IwVIUaVhXASy2bPR9aMrjnU0yCA?=
 =?us-ascii?Q?dTYXxXkHo2ewQTPVm3dAV8LaNsN5ZtLvD9clrIp8JeGXxhOV/kAkHtn/ZZiI?=
 =?us-ascii?Q?EJbNNjDSOlL6IwDQ0Z2aWHNxANs9UEnGkj6m2/E9jRIC+6tE5L3BlYwE3DTy?=
 =?us-ascii?Q?yqOOpSMcz//080LyM949VkKbj+5Cz5nGnAwjrnFXWyrPP917YnAfmrZLepOZ?=
 =?us-ascii?Q?uzd/TD836E1YcAvWD5U6?=
X-OriginatorOrg: outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bf82b402-d673-4f3c-346a-08dbca68cedd
X-MS-Exchange-CrossTenant-AuthSource: SY4P282MB1084.AUSP282.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Oct 2023 14:46:11.3399
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MEYP282MB4270
X-Spam-Status: No, score=3.0 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,
	HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_SBL_CSS,
	SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Wed, 11 Oct 2023 07:46:37 -0700 (PDT)
X-Spam-Level: **

Hi Maxim,

At 2023-10-10 16:12 +0000, Maxim Levitsky wrote:
> > +static inline void
> > +perf_callchain_guest32(struct perf_callchain_entry_ctx *entry)
> > +{
> > +	struct stack_frame_ia32 frame;
> > +	const struct stack_frame_ia32 *fp;
> > +
> > +	fp = (void *)perf_guest_get_frame_pointer();
> > +	while (fp && entry->nr < entry->max_stack) {
> > +		if (!perf_guest_read_virt(&fp->next_frame, &frame.next_frame,
> This should be fp->next_frame.
> > +			sizeof(frame.next_frame)))
> > +			break;
> > +		if (!perf_guest_read_virt(&fp->return_address, &frame.return_address,
> Same here.
> > +			sizeof(frame.return_address)))
> > +			break;
> > +		perf_callchain_store(entry, frame.return_address);
> > +		fp = (void *)frame.next_frame;
> > +	}
> > +}
> > +

The address space where `fp` resides here is in the guest memory, not in
the directly accessible kernel address space. `&fp->next_frame` and
`&fp->return_address` are simply calculating address offsets in a more
readable manner, much like `fp + 0` and `fp + 4`.

The original implementation of `perf_callchain_user` and
`perf_callchain_user32` also use this approach [1].

>
> For symmetry, maybe it makes sense to have perf_callchain_guest32 and perf_callchain_guest64
> and then make perf_callchain_guest call each? No strong opinion on this of course.
>

The `perf_callchain_guest` and `perf_callchain_guest32` here are simply
designed to mimic `perf_callchain_user` and `perf_callchain_user32` [2].
I'm also open to make the logic fully separate, if this doesn't seem
elegant enough.

[1] https://github.com/torvalds/linux/blob/master/arch/x86/events/core.c#L2890
[2] https://github.com/torvalds/linux/blob/master/arch/x86/events/core.c#L2820


Best regards,
Tianyi Liu