Received-SPF: pass (google.com: domain of linux-kernel+bounces-66073-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240214113947.240957-1-leo.yan@linux.dev>
In-Reply-To: <20240214113947.240957-1-leo.yan@linux.dev>
From: Ian Rogers <irogers@google.com>
Date: Wed, 14 Feb 2024 14:42:55 -0800
Message-ID: <CAP-5=fXqdFrTDY2a2dXK1ehP2AbwPfPF8vYWXgeSMcKh4mS+ow@mail.gmail.com>
Subject: Re: [PATCH v1 0/4] perf parse-regs: Cleanup config and building
To: Leo Yan <leo.yan@linux.dev>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>, Namhyung Kim <namhyung@kernel.org>, Jiri Olsa <jolsa@kernel.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>, John Garry <john.g.garry@oracle.com>, 
	Will Deacon <will@kernel.org>, James Clark <james.clark@arm.com>, 
	Mike Leach <mike.leach@linaro.org>, Guo Ren <guoren@kernel.org>, 
	Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, 
	Albert Ou <aou@eecs.berkeley.edu>, Huacai Chen <chenhuacai@kernel.org>, 
	Ming Wang <wangming01@loongson.cn>, Kan Liang <kan.liang@linux.intel.com>, 
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-csky@vger.kernel.org, linux-riscv@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Feb 14, 2024 at 3:40=E2=80=AFAM Leo Yan <leo.yan@linux.dev> wrote:
>
> Currently, the perf building enables register parsing based on the
> target architecture has supported register feature.
>
> Furthermore, the perf building system needs to maintain a variable
> 'NO_PERF_REGS' and defines macro 'HAVE_PERF_REGS_SUPPORT' for statically
> compiling the tool.
>
> As a result, the perf has no flexibilty for parsing register if an
> architecture doesn't support it. And the source files use the macro
> 'HAVE_PERF_REGS_SUPPORT' to switch on and off the register parsing
> related code, which is not a good practice.
>
> This series is to remove the static building for register parsing. In
> theory, we should can dynamically detect if an arch has support this
> feature and functions can return errors when the feature is not
> supported.
>
> The first patch is to remove unused build configuration
> CONFIG_PERF_REGS.
>
> The second patch is to build perf register functions, without using the
> macro 'HAVE_PERF_REGS_SUPPORT' to statically turn on or off code.
>
> The third patch is to introduce a weak function arch__sample_reg_masks(),
> this function can allow the target arch to return its sample register
> list.  With this change, we can totally remove the macro
> 'HAVE_PERF_REGS_SUPPORT' in the source file.
>
> The forth patch is to clean up the Makefile for removing relevant
> configuration and macro definition, as they are not useful anymore.
>
> I tested this patch set on Arm64 and x86 for building and did a cross
> register parsing ('perf record' on Arm64 and 'perf report' on x86).
>
>
> Leo Yan (4):
>   perf build: Remove unused CONFIG_PERF_REGS
>   perf parse-regs: Always build perf register functions
>   perf parse-regs: Introduce a weak function arch__sample_reg_masks()
>   perf build: Cleanup perf register configuration

Thanks Leo, this is great cleanup! Series:
Reviewed-by: Ian Rogers <irogers@google.com>

Ian

>  tools/perf/Makefile.config                    | 25 --------------
>  tools/perf/arch/arm/util/perf_regs.c          |  7 +++-
>  tools/perf/arch/arm64/util/machine.c          |  2 ++
>  tools/perf/arch/arm64/util/perf_regs.c        |  7 +++-
>  tools/perf/arch/csky/util/perf_regs.c         |  7 +++-
>  tools/perf/arch/loongarch/util/perf_regs.c    |  7 +++-
>  tools/perf/arch/mips/util/perf_regs.c         |  7 +++-
>  tools/perf/arch/powerpc/util/perf_regs.c      |  7 +++-
>  tools/perf/arch/riscv/util/perf_regs.c        |  7 +++-
>  tools/perf/arch/s390/util/perf_regs.c         |  7 +++-
>  tools/perf/arch/x86/util/perf_regs.c          |  7 +++-
>  tools/perf/util/parse-regs-options.c          |  8 ++---
>  .../util/perf-regs-arch/perf_regs_aarch64.c   |  4 ---
>  .../perf/util/perf-regs-arch/perf_regs_arm.c  |  4 ---
>  .../perf/util/perf-regs-arch/perf_regs_csky.c |  4 ---
>  .../util/perf-regs-arch/perf_regs_loongarch.c |  4 ---
>  .../perf/util/perf-regs-arch/perf_regs_mips.c |  4 ---
>  .../util/perf-regs-arch/perf_regs_powerpc.c   |  4 ---
>  .../util/perf-regs-arch/perf_regs_riscv.c     |  4 ---
>  .../perf/util/perf-regs-arch/perf_regs_s390.c |  4 ---
>  .../perf/util/perf-regs-arch/perf_regs_x86.c  |  4 ---
>  tools/perf/util/perf_regs.c                   | 11 ++++--
>  tools/perf/util/perf_regs.h                   | 34 +------------------
>  23 files changed, 67 insertions(+), 112 deletions(-)
>
> --
> 2.34.1
>