From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7F563C4338F for ; Wed, 28 Jul 2021 09:43:13 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5B93A60FC2 for ; Wed, 28 Jul 2021 09:43:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231408AbhG1JnO (ORCPT ); Wed, 28 Jul 2021 05:43:14 -0400 Received: from foss.arm.com ([217.140.110.172]:54056 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231783AbhG1JnN (ORCPT ); Wed, 28 Jul 2021 05:43:13 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 74E871FB; Wed, 28 Jul 2021 02:43:12 -0700 (PDT) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 8E9743F73D; Wed, 28 Jul 2021 02:43:11 -0700 (PDT) Date: Wed, 28 Jul 2021 10:41:51 +0100 From: Dave Martin To: Mark Brown Cc: Catalin Marinas , Will Deacon , Shuah Khan , linux-arm-kernel@lists.infradead.org, linux-kselftest@vger.kernel.org Subject: Re: [PATCH v1 2/3] kselftest/arm64: Validate vector lengths are set in sve-probe-vls Message-ID: <20210728094151.GB1724@arm.com> References: <20210727180649.12943-1-broonie@kernel.org> <20210727180649.12943-3-broonie@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210727180649.12943-3-broonie@kernel.org> User-Agent: Mutt/1.5.23 (2014-03-12) Precedence: bulk List-ID: X-Mailing-List: linux-kselftest@vger.kernel.org On Tue, Jul 27, 2021 at 07:06:48PM +0100, Mark Brown wrote: > Currently sve-probe-vls does not verify that the vector lengths reported > by the prctl() interface are actually what is reported by the architecture, > use the rdvl_sve() helper to validate this. > > Signed-off-by: Mark Brown > --- > tools/testing/selftests/arm64/fp/Makefile | 2 +- > tools/testing/selftests/arm64/fp/sve-probe-vls.c | 5 +++++ > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile > index ed62e7003b96..fa3a0167db2d 100644 > --- a/tools/testing/selftests/arm64/fp/Makefile > +++ b/tools/testing/selftests/arm64/fp/Makefile > @@ -13,7 +13,7 @@ fpsimd-test: fpsimd-test.o > $(CC) -nostdlib $^ -o $@ > rdvl-sve: rdvl-sve.o rdvl.o > sve-ptrace: sve-ptrace.o sve-ptrace-asm.o > -sve-probe-vls: sve-probe-vls.o > +sve-probe-vls: sve-probe-vls.o rdvl.o > sve-test: sve-test.o > $(CC) -nostdlib $^ -o $@ > vlset: vlset.o > diff --git a/tools/testing/selftests/arm64/fp/sve-probe-vls.c b/tools/testing/selftests/arm64/fp/sve-probe-vls.c > index 76e138525d55..a6cd1bd6e515 100644 > --- a/tools/testing/selftests/arm64/fp/sve-probe-vls.c > +++ b/tools/testing/selftests/arm64/fp/sve-probe-vls.c > @@ -13,6 +13,7 @@ > #include > > #include "../../kselftest.h" > +#include "rdvl.h" > > int main(int argc, char **argv) > { This test was originally a diagnostic tool, so the way VLs are probed aims to be efficient, rather than being defensive against the kernel doing weird stuff. If the kernel returns a vl > than the one we tried to set, we'll end up in an infinite loop because of the way the loop deliberately uses the kernel's return value to skip unsupported VLs. So, it might be better to probe every single architecturally possible VL and sanity check everything instead. While taking this opportunity, a check that the resulting vl is <= the requested VL might be good too. Also, if the kernel rounds any given vl down to some vl_actual, then it should also round every possible other_vl down to vl_actual, where vl_actual <= other_vl < vl. It's not OK for different vls to be rounded down to different actual values that have nothing to do with each other. Since this test is our example of how to do this probing, it would be good to preserve the "efficient" version though, and check that the probed set of vls matches what is obtained by exhaustive probing. Using comments and well-chosen function names, we can hopefully steer people to use the efficient version that trusts the kernel in their own code. > @@ -38,6 +39,10 @@ int main(int argc, char **argv) > > vl &= PR_SVE_VL_LEN_MASK; > > + if (rdvl_sve() != vl) > + ksft_exit_fail_msg("Set VL %d, RDVL %d\n", > + vl, rdvl_sve()); > + If this fails, the VL vl wasn't "Set" at all. I found this a bit confusing from just looking at this hunk. Can we write something like "PR_SVE_SET_VL reports VL %d, RDVL %d"? [...] Thinking about it, due to the way these tests do inline vector length changes we should also be making sure that they are not built with SVE code gen by default, since inline VL changes could cause bad things to happen in that case. This isn't a problem for now, but as time goes on the toolchain may start to be configured to compile for SVE by default. I'm not sure of the correct option for forcing SVE off against the compiler default -- check with the tools folks. If there isn't a proper way to do this, it's a toolchain defect so we should flag it up, but -mgeneral-regs-only might work for us even though it's a bit of a sledgehammer. Compiler options aside, I think we _probably_ get away with a native-SVE libc, since if everything in libc is VL-agnostic then it shouldn't care what VL we call in with. If being paranoid, we could always restore the VL before doing anything other then prctl() and RVDL, but we should probably wait for something to break first. If we wanted to be super-robust, we could make the business end of each test a pure-asm binary and control them via ptrace. But that's probably too much pain for now, even if the test harness were reusable. Cheers ---Dave From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E91BBC4338F for ; Wed, 28 Jul 2021 09:45:41 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id AD7CC60F9C for ; Wed, 28 Jul 2021 09:45:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org AD7CC60F9C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=f1xEgDnCSSZw90GLVfhK9jrRfCpsYhQrnIL0YYAr01c=; b=4ngB8t8JnMuqLQ ajBOXZMdp1nG/2oGAHlv9ma6+kWbvxxKeV2A6EhFgJLDDHQsToEnpra40sG4ln1/xzm21u+bVv6/w XJ5Ad01pvO30WsAKe/VNAP2bpIVAMDj+YHfbY1+n/RWI/RnSMK/qXABRM9TjkmrYd7jmbplmJiLtE cu33nkmEDbk23l9C8gz1oMnAfxygKgSIMpMvgcVJuqzUj73F1IFppJBha5lgrL99bUXoLCXf1SuUH C0v1BRvK0mO1bC+J6h6GReubv61I5H7aW5Ds5nE+TBAT7q559Xs/sgl0CzBTArhMang9hfsTpyO+c o+EfgSgxYBPk4/rP7f4A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1m8g62-000Dwr-SD; Wed, 28 Jul 2021 09:43:27 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1m8g5s-000DuH-NO for linux-arm-kernel@lists.infradead.org; Wed, 28 Jul 2021 09:43:18 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 74E871FB; Wed, 28 Jul 2021 02:43:12 -0700 (PDT) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 8E9743F73D; Wed, 28 Jul 2021 02:43:11 -0700 (PDT) Date: Wed, 28 Jul 2021 10:41:51 +0100 From: Dave Martin To: Mark Brown Cc: Catalin Marinas , Will Deacon , Shuah Khan , linux-arm-kernel@lists.infradead.org, linux-kselftest@vger.kernel.org Subject: Re: [PATCH v1 2/3] kselftest/arm64: Validate vector lengths are set in sve-probe-vls Message-ID: <20210728094151.GB1724@arm.com> References: <20210727180649.12943-1-broonie@kernel.org> <20210727180649.12943-3-broonie@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210727180649.12943-3-broonie@kernel.org> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210728_024316_937060_1A088FA3 X-CRM114-Status: GOOD ( 35.60 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Jul 27, 2021 at 07:06:48PM +0100, Mark Brown wrote: > Currently sve-probe-vls does not verify that the vector lengths reported > by the prctl() interface are actually what is reported by the architecture, > use the rdvl_sve() helper to validate this. > > Signed-off-by: Mark Brown > --- > tools/testing/selftests/arm64/fp/Makefile | 2 +- > tools/testing/selftests/arm64/fp/sve-probe-vls.c | 5 +++++ > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/arm64/fp/Makefile b/tools/testing/selftests/arm64/fp/Makefile > index ed62e7003b96..fa3a0167db2d 100644 > --- a/tools/testing/selftests/arm64/fp/Makefile > +++ b/tools/testing/selftests/arm64/fp/Makefile > @@ -13,7 +13,7 @@ fpsimd-test: fpsimd-test.o > $(CC) -nostdlib $^ -o $@ > rdvl-sve: rdvl-sve.o rdvl.o > sve-ptrace: sve-ptrace.o sve-ptrace-asm.o > -sve-probe-vls: sve-probe-vls.o > +sve-probe-vls: sve-probe-vls.o rdvl.o > sve-test: sve-test.o > $(CC) -nostdlib $^ -o $@ > vlset: vlset.o > diff --git a/tools/testing/selftests/arm64/fp/sve-probe-vls.c b/tools/testing/selftests/arm64/fp/sve-probe-vls.c > index 76e138525d55..a6cd1bd6e515 100644 > --- a/tools/testing/selftests/arm64/fp/sve-probe-vls.c > +++ b/tools/testing/selftests/arm64/fp/sve-probe-vls.c > @@ -13,6 +13,7 @@ > #include > > #include "../../kselftest.h" > +#include "rdvl.h" > > int main(int argc, char **argv) > { This test was originally a diagnostic tool, so the way VLs are probed aims to be efficient, rather than being defensive against the kernel doing weird stuff. If the kernel returns a vl > than the one we tried to set, we'll end up in an infinite loop because of the way the loop deliberately uses the kernel's return value to skip unsupported VLs. So, it might be better to probe every single architecturally possible VL and sanity check everything instead. While taking this opportunity, a check that the resulting vl is <= the requested VL might be good too. Also, if the kernel rounds any given vl down to some vl_actual, then it should also round every possible other_vl down to vl_actual, where vl_actual <= other_vl < vl. It's not OK for different vls to be rounded down to different actual values that have nothing to do with each other. Since this test is our example of how to do this probing, it would be good to preserve the "efficient" version though, and check that the probed set of vls matches what is obtained by exhaustive probing. Using comments and well-chosen function names, we can hopefully steer people to use the efficient version that trusts the kernel in their own code. > @@ -38,6 +39,10 @@ int main(int argc, char **argv) > > vl &= PR_SVE_VL_LEN_MASK; > > + if (rdvl_sve() != vl) > + ksft_exit_fail_msg("Set VL %d, RDVL %d\n", > + vl, rdvl_sve()); > + If this fails, the VL vl wasn't "Set" at all. I found this a bit confusing from just looking at this hunk. Can we write something like "PR_SVE_SET_VL reports VL %d, RDVL %d"? [...] Thinking about it, due to the way these tests do inline vector length changes we should also be making sure that they are not built with SVE code gen by default, since inline VL changes could cause bad things to happen in that case. This isn't a problem for now, but as time goes on the toolchain may start to be configured to compile for SVE by default. I'm not sure of the correct option for forcing SVE off against the compiler default -- check with the tools folks. If there isn't a proper way to do this, it's a toolchain defect so we should flag it up, but -mgeneral-regs-only might work for us even though it's a bit of a sledgehammer. Compiler options aside, I think we _probably_ get away with a native-SVE libc, since if everything in libc is VL-agnostic then it shouldn't care what VL we call in with. If being paranoid, we could always restore the VL before doing anything other then prctl() and RVDL, but we should probably wait for something to break first. If we wanted to be super-robust, we could make the business end of each test a pure-asm binary and control them via ptrace. But that's probably too much pain for now, even if the test harness were reusable. Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel