Re: [PATCH v2 4/4] perf tools: arm64: Add support for VG register

From: Leo Yan <leo.yan@linaro.org>
To: James Clark <james.clark@arm.com>
Cc: linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
	broonie@kernel.org, acme@kernel.org, german.gomez@arm.com,
	mathieu.poirier@linaro.org, john.garry@huawei.com,
	Will Deacon <will@kernel.org>, Mike Leach <mike.leach@linaro.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 4/4] perf tools: arm64: Add support for VG register
Date: Tue, 17 May 2022 21:19:52 +0800	[thread overview]
Message-ID: <20220517131952.GE153558@leoy-ThinkPad-X240s> (raw)
In-Reply-To: <20220517102005.3022017-5-james.clark@arm.com>

On Tue, May 17, 2022 at 11:20:05AM +0100, James Clark wrote:
> Add the name of the VG register so it can be used in --user-regs
> 
> The event will fail to open if the register is requested but not
> available so only add it to the mask if the kernel supports sve and also
> if it supports that specific register.
> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/arch/arm64/util/perf_regs.c | 34 ++++++++++++++++++++++++++
>  tools/perf/util/perf_regs.c            |  2 ++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/tools/perf/arch/arm64/util/perf_regs.c b/tools/perf/arch/arm64/util/perf_regs.c
> index 476b037eea1c..c0a921512a90 100644
> --- a/tools/perf/arch/arm64/util/perf_regs.c
> +++ b/tools/perf/arch/arm64/util/perf_regs.c
> @@ -2,9 +2,11 @@
>  #include <errno.h>
>  #include <regex.h>
>  #include <string.h>
> +#include <sys/auxv.h>
>  #include <linux/kernel.h>
>  #include <linux/zalloc.h>
>  
> +#include "../../../perf-sys.h"
>  #include "../../../util/debug.h"
>  #include "../../../util/event.h"
>  #include "../../../util/perf_regs.h"
> @@ -43,6 +45,7 @@ const struct sample_reg sample_reg_masks[] = {
>  	SMPL_REG(lr, PERF_REG_ARM64_LR),
>  	SMPL_REG(sp, PERF_REG_ARM64_SP),
>  	SMPL_REG(pc, PERF_REG_ARM64_PC),
> +	SMPL_REG(vg, PERF_REG_ARM64_VG),
>  	SMPL_REG_END
>  };
>  
> @@ -131,3 +134,34 @@ int arch_sdt_arg_parse_op(char *old_op, char **new_op)
>  
>  	return SDT_ARG_VALID;
>  }
> +
> +uint64_t arch__user_reg_mask(void)
> +{
> +	struct perf_event_attr attr = {
> +		.type                   = PERF_TYPE_HARDWARE,
> +		.config                 = PERF_COUNT_HW_CPU_CYCLES,
> +		.sample_type            = PERF_SAMPLE_REGS_USER,
> +		.disabled               = 1,
> +		.exclude_kernel         = 1,
> +		.sample_period		= 1,
> +		.sample_regs_user	= PERF_REGS_MASK
> +	};
> +	int fd;
> +
> +	if (getauxval(AT_HWCAP) & HWCAP_SVE)
> +		attr.sample_regs_user |= SMPL_REG_MASK(PERF_REG_ARM64_VG);
> +
> +	/*
> +	 * Check if the pmu supports perf extended regs, before
> +	 * returning the register mask to sample.
> +	 */
> +	if (attr.sample_regs_user != PERF_REGS_MASK) {
> +		event_attr_init(&attr);
> +		fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
> +		if (fd != -1) {
> +			close(fd);
> +			return attr.sample_regs_user;
> +		}
> +	}

Just curious, since we can know SVE is supported from reading
auxiliary value, can we directly return the register mask as below?

  PERF_REGS_MASK | SMPL_REG_MASK(PERF_REG_ARM64_VG);

Except this question, this patch looks good to me.

Thanks,
Leo

> +	return PERF_REGS_MASK;
> +}
> diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c
> index a982e40ee5a9..872dd3d38782 100644
> --- a/tools/perf/util/perf_regs.c
> +++ b/tools/perf/util/perf_regs.c
> @@ -103,6 +103,8 @@ static const char *__perf_reg_name_arm64(int id)
>  		return "lr";
>  	case PERF_REG_ARM64_PC:
>  		return "pc";
> +	case PERF_REG_ARM64_VG:
> +		return "vg";
>  	default:
>  		return NULL;
>  	}
> -- 
> 2.28.0
> 