From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (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 E7C7B3F077C for ; Wed, 1 Jul 2026 08:55:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782896152; cv=none; b=UtnFDXVtFUQs4HScvThxtLzWBOxzZX6GdrcnYA1X4sBZDCfNCvcLU22rGjmisNmABgruzS0h8uP44THdsmmj5+yeao8qExBWWkk2ICUioWMtcSxgB/XVyCCzvMQM9LLiwUlzobyJjlQYbIaBBf05wCv+YrUs5aD0oMAjIysDIvg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782896152; c=relaxed/simple; bh=4kNWGC/CQkMBf5uas5uoaqDR61Vhp04gK3hMhwr0Oq4=; h=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date: Message-Id; b=Kc6ltiIJzmeK2NIOH3v2bC+0qw1z3Rvm7eLos9UuiaP5crZeZa3F6csbvGZ9FFRuPltZ/bTy595+J5OZZ5K4I9Fp2Mcwl71koF5pvEBQJ3+6XJ36JH7uVkxFJsrB1oirKo4uLEhgrHUrUU2DhYPWP67/VBmEfsxcfygOkyUZbS8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=GMlIzxlS; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="GMlIzxlS" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4A3F01F00A3A; Wed, 1 Jul 2026 08:55:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1782896150; bh=Vinpt7MeoNdX7tgMcW75Vc8X+KPmVlWYqfHgUh9nSro=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=GMlIzxlSXx6SkJmaUkdMiGBBY1haNebW3KaVjiqMZ1Rebs6VvIF77bCtRn+WciQXv JJvpk9vZFH6bIlsws335RMJfNZDbyzHBEnTnzJoiIOEnJJEZCHF8eEBzZFWTWOFyKA 88ZCFE46baQNM9BNS6jYV9aPjHcJP/JcW9h2/aYv3skDyg8mt6b3iFpoojMEYQX+PH jv5ZRyN0CdJXtvjCItlIQ5hSNkhiF4oAVqHL9J+L1PXBOMCillztNsu08GXYRcTg/9 bktFor+BLbzi47XGCFQ3X0VjxUKtypxKTddJQD7J7b+VhoBRLxiwxw9Xq8u3GDlxPo 9KvlMOgKizU/w== From: sashiko-bot@kernel.org Subject: Re: [PATCH 2/9] tools/perf: Add CONFIG_AUXTRACE support for HTM pmu on powerpc Reply-To: sashiko-reviews@lists.linux.dev To: "Athira Rajeev" Cc: linux-perf-users@vger.kernel.org In-Reply-To: <20260701084115.80383-3-atrajeev@linux.ibm.com> References: <20260701084115.80383-1-atrajeev@linux.ibm.com> <20260701084115.80383-3-atrajeev@linux.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Wed, 01 Jul 2026 08:55:48 +0000 Message-Id: <20260701085549.4A3F01F00A3A@smtp.kernel.org> Precedence: bulk X-Mailing-List: linux-perf-users@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Thank you for your contribution! Sashiko AI review found 3 potential issue(= s) to consider: New issues: - [Medium] Missing explicit inclusions of POSIX standard headers `` and `` for standard libc symbols. - [Medium] Memory allocation failure during HTM auxtrace initialization is = silently dropped, causing a broken recording state. Pre-existing issues: - [High] Directly accessing `pos->name` with `strstarts()` causes a segment= ation fault when the event name is NULL. -- commit d4675d2351298802a38f0a4941c6bb4d86a4ed1f Author: Athira Rajeev tools/perf: Add CONFIG_AUXTRACE support for HTM pmu on powerpc The powerpc PMU collecting Hardware Trace Macro (HTM) entries makes use of AUX support in perf infrastructure. The PMU driver has the functionality to collect trace entries in the aux buffer. On the tools side, this data is made available as PERF_RECORD_AUXTRACE records. This record is generated by "perf record" command. To enable the creation of PERF_RECORD_AUXTRACE, add functions to initialize auxtrace records ie "htm_recording_init()". Fill in fields for other callbacks like info_priv_size, info_fill, free, recording options etc. Add header file to define htm pmu specific details. > diff --git a/tools/perf/arch/powerpc/util/auxtrace.c b/tools/perf/arch/po= werpc/util/auxtrace.c > index 0053526329e0e..ec84f8876a4ad 100644 > --- a/tools/perf/arch/powerpc/util/auxtrace.c > +++ b/tools/perf/arch/powerpc/util/auxtrace.c > @@ -31,13 +33,19 @@ struct auxtrace_record *auxtrace_record__init(struct = evlist *evlist, > found_vpa_dtl =3D 1; > pos->needs_auxtrace_mmap =3D true; > break; > + } else if (strstarts(pos->name, "htm")) { ^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] This isn't a bug introduced by this patch, but does accessing pos->name directly cause a segmentation fault if the event name is NULL? If a standard hardware event is parsed without an explicit name (like "perf record -e cycles"), the name remains NULL until it is lazily generated via evsel__name(). Evaluating strncmp(NULL, "htm", 3) inside strstarts() would crash. Since this patch duplicates this pattern here and in htm_recording_options(= ), should we use evsel__name() or check for NULL first? > + found_htm =3D 1; > + pos->needs_auxtrace_mmap =3D true; > + break; > } > } > =20 > if (found_vpa_dtl) > return vpa_dtl_recording_init(pos); > + else if (found_htm) > + return htm_recording_init(pos); [Severity: Medium] If htm_recording_init() fails to allocate memory, it returns NULL. Since we don't update the *err parameter before returning, does this propagate a fal= se success to the caller? If *err remains 0, builtin-record.c might assume auxtrace is simply not required and continue recording, silently dropping the HTM trace data inste= ad of aborting with -ENOMEM. > else { > - *err =3D -EINVAL; > + *err =3D 0; > return NULL; > } > } > diff --git a/tools/perf/arch/powerpc/util/htm.c b/tools/perf/arch/powerpc= /util/htm.c > new file mode 100644 > index 0000000000000..cc733f45ac9b0 > --- /dev/null > +++ b/tools/perf/arch/powerpc/util/htm.c > @@ -0,0 +1,113 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * HTM support > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "../../util/cpumap.h" > +#include "../../util/evsel.h" > +#include "../../util/evlist.h" > +#include "../../util/session.h" > +#include "../../util/util.h" > +#include "../../util/pmu.h" > +#include "../../util/debug.h" > +#include "../../util/auxtrace.h" > +#include "../../util/powerpc-htm.h" > +#include "../../util/record.h" > +#include // page_size > +#include [Severity: Medium] Are we missing the explicit inclusions of and here? This file uses UINT_MAX in htm_recording_options() and free() in htm_free(). Without directly including and , building the perf too= ls with musl libc might fail due to missing declarations. [ ... ] > + /* > + * Set auxtrace_mmap_pages to minimum > + * two pages > + */ > + if (!opts->auxtrace_mmap_pages) { > + opts->auxtrace_mmap_pages =3D KiB(128) / page_size; > + if (opts->mmap_pages =3D=3D UINT_MAX) > + opts->mmap_pages =3D KiB(256) / page_size; > + } > + > + evlist__for_each_entry(evlist, pos) { > + if (strstarts(pos->name, "htm")) { [Severity: High] This is a pre-existing issue, but as mentioned above, this could crash if pos->name is NULL. > + pos->needs_auxtrace_mmap =3D true; > + pos->core.attr.aux_watermark =3D opts->auxtrace_mmap_pages * (size_t)= page_size; > + break; > + } > + } > + > + return 0; > +} --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260701084115.8038= 3-1-atrajeev@linux.ibm.com?part=3D2