Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC V1 08/11] arm64/perf: Enable branch stack sampling
To:     Anshuman Khandual <anshuman.khandual@arm.com>,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Cc:     Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        linux-perf-users@vger.kernel.org
References: <1642998653-21377-1-git-send-email-anshuman.khandual@arm.com>
 <1642998653-21377-9-git-send-email-anshuman.khandual@arm.com>
From:   James Clark <james.clark@arm.com>
Message-ID: <741d159d-fce4-377e-8817-f8a6462f3a71@arm.com>
Date:   Mon, 24 Jan 2022 18:02:51 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <1642998653-21377-9-git-send-email-anshuman.khandual@arm.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 24/01/2022 04:30, Anshuman Khandual wrote:
> Now that all the required pieces are already in place, just enable the perf
> branch stack sampling support on arm64 platform, by removing the gate which
> blocks it in armpmu_event_init().
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  drivers/perf/arm_pmu.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index 0800c8858ed8..2117bf5d3232 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -537,9 +537,28 @@ static int armpmu_event_init(struct perf_event *event)
>  		!cpumask_test_cpu(event->cpu, &armpmu->supported_cpus))
>  		return -ENOENT;
>  
> -	/* does not support taken branch sampling */
> -	if (has_branch_stack(event))
> -		return -EOPNOTSUPP;
> +	if (has_branch_stack(event)) {
> +		/*
> +		 * BRBE support is absent. Select CONFIG_ARM_BRBE_PMU
> +		 * in the config, before branch stack sampling events
> +		 * can be requested.
> +		 */
> +		if (!IS_ENABLED(CONFIG_ARM_BRBE_PMU)) {
> +			pr_warn_once("BRBE is disabled, select CONFIG_ARM_BRBE_PMU\n");
> +			return -EOPNOTSUPP;
> +		}
> +
> +		/*
> +		 * Branch stack sampling event can not be supported in
> +		 * case either the required driver itself is absent or
> +		 * BRBE buffer, is not supported. Besides checking for
> +		 * the callback prevents a crash in case it's absent.
> +		 */
> +		if (!armpmu->brbe_supported || !armpmu->brbe_supported(event)) {
> +			pr_warn_once("BRBE is not supported\n");
> +			return -EOPNOTSUPP;

brbe_supported() returns false for one permission case, rather than a "not supported" case
so EOPNOTSUPP is the wrong thing to return here for this case otherwise it makes perf
print confusing error messages. 

The brbe_supported() function needs to be split into two, one that handles support and one
that handles permissions and different errors need to be reported. Here is the permission
bit from that function:

  +	if (event->attr.branch_sample_type & PERF_SAMPLE_BRANCH_KERNEL) {
  +		if (!perfmon_capable()) {
  +			pr_warn_once("does not have permission for kernel branch filter\n");
  +			return false;
  +		}
  +	}

> +		}
> +	}
>  
>  	if (armpmu->map_event(event) == -ENOENT)
>  		return -ENOENT;
>