Re: [PATCH v11 40/40] kselftest/arm64: Add SME support to syscall ABI test

From: Shuah Khan <skhan@linuxfoundation.org>
To: Mark Brown <broonie@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,  Marc Zyngier <maz@kernel.org>,
	Shuah Khan <shuah@kernel.org>,
	Alan Hayward <alan.hayward@arm.com>,
	Luis Machado <luis.machado@arm.com>,
	Salil Akerkar <Salil.Akerkar@arm.com>,
	Basant Kumar Dwivedi <Basant.KumarDwivedi@arm.com>,
	Szabolcs Nagy <szabolcs.nagy@arm.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kselftest@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
	Shuah Khan <skhan@linuxfoundation.org>
Subject: Re: [PATCH v11 40/40] kselftest/arm64: Add SME support to syscall ABI test
Date: Tue, 8 Feb 2022 11:50:01 -0700	[thread overview]
Message-ID: <5ab8ec1b-c978-3eed-9e32-3d99d7cba4e3@linuxfoundation.org> (raw)
In-Reply-To: <YgKzXs2Eti+Z2A1+@sirena.org.uk>

On 2/8/22 11:15 AM, Mark Brown wrote:
> On Mon, Feb 07, 2022 at 06:52:06PM -0700, Shuah Khan wrote:
>> On 2/7/22 8:21 AM, Mark Brown wrote:
> 
>>> +static int check_za(struct syscall_cfg *cfg, int sve_vl, int sme_vl,
>>> +		    uint64_t svcr)
>>> +{
>>> +	size_t reg_size = sme_vl * sme_vl;
> 
>> Is there possibilty of size_t overfolow here?
> 
> No, if the size of a vector were anywhere near to being able to being
> able to do that I think we'd have serious design issues with the ABI -
> the size being calculated here is the size of a single register.  The
> current architectural maximum vector length is 2048 bits, which would
> give a size of 64K for ZA if implemented.
> 
>>> +	if (memcmp(za_in, za_out, reg_size) != 0) {
>>> +		ksft_print_msg("SME VL %d ZA does not match\n", sme_vl);
> 
>> Print the expected value in addition to the sme_val.
> 
> This is not comparing the vector length, this is comparing the contents
> of the ZA register which may be up to 64K in size.  There are serious
> presentational issues with displaying any errors in a useful fashion for
> such a large register which IME needs custom display code adding by
> whoever is debugging the issue that takes account of what the pattern
> being observed is.
> 
>>> @@ -265,8 +357,36 @@ static void test_one_syscall(struct syscall_cfg *cfg)
>>>    		if (sve_vq != sve_vq_from_vl(sve_vl))
>>>    			sve_vq = sve_vq_from_vl(sve_vl);
>>> -		ksft_test_result(do_test(cfg, sve_vl),
>>> +		ksft_test_result(do_test(cfg, sve_vl, default_sme_vl, 0),
>>>    				 "%s SVE VL %d\n", cfg->name, sve_vl);
> 
>> Print default_sme_vl as well.
> 
> default_sme_vl is just being passed in as a dummy value here since the
> function takes a fixed number of arguments, this is testing the case
> where SME is not used or enabled and will be run on systems which do not
> have SME at all so there won't be any defined vector length for SME.  I
> fear that it would cause confusion to display a SME VL here, and
> do_test() won't actually pay any attention to that argument in this
> case.  We will individually step through all possible combinations of
> SVE and SME vector lengths in separate tests.
> 

Sounds good.

thanks,
-- Shuah

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel