Re: [PATCH v14 16/39] arm64/sme: Implement traps and syscall handling for SME

From: Zenghui Yu <yuzenghui@huawei.com>
To: Mark Brown <broonie@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>,
	Basant Kumar Dwivedi <Basant.KumarDwivedi@arm.com>,
	Will Deacon <will@kernel.org>,
	Luis Machado <luis.machado@arm.com>,
	Szabolcs Nagy <szabolcs.nagy@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Alan Hayward <alan.hayward@arm.com>,
	Salil Akerkar <Salil.Akerkar@arm.com>,
	linux-kselftest@vger.kernel.org,
	Shuah Khan <skhan@linuxfoundation.org>,
	Shuah Khan <shuah@kernel.org>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org,
	Luca Salabrino <luca.scalabrino@arm.com>
Subject: Re: [PATCH v14 16/39] arm64/sme: Implement traps and syscall handling for SME
Date: Thu, 8 Dec 2022 10:15:26 +0800	[thread overview]
Message-ID: <dfcbafc2-17e1-94e8-b890-8ced1c0bcdc8@huawei.com> (raw)
In-Reply-To: <Y5CgRPbjOChpHQEJ@sirena.org.uk>

On 2022/12/7 22:16, Mark Brown wrote:
> On Wed, Dec 07, 2022 at 10:00:17PM +0800, Zenghui Yu wrote:
>> On 2022/4/19 19:22, Mark Brown wrote:
> 
>>> +	/*
>>> +	 * If SME is active then exit streaming mode.  If ZA is active
>>> +	 * then flush the SVE registers but leave userspace access to
>>> +	 * both SVE and SME enabled, otherwise disable SME for the
>>> +	 * task and fall through to disabling SVE too.  This means
> 
>> It looks a bit confusing to me that in the current implementation, if
>> ZA is *not* active, we don't actually go to disable SME for the task
>> (which IMHO can be disabled as user-space is not actively using it now).
> 
> Unlike SVE there's no overhead from leaving SME enabled, the enable bits
> for SM and ZA tell us if there is extra register state to be saved so we
> don't have to worry about the costs there like we do with SVE.  The only
> reason for not just unconditionally enabling SME is the potentially
> large backing storage required for the registers, if a task isn't using
> SME there's no need to impose that overhead.  If we disable SME for
> userspace after the storage has been allocated then we just require an
> additional trip through EL1 to reenable it for any future usage which
> would hurt performance but not really gain us anything otherwise.  We
> don't want to discurage applications from disabling ZA when not in use
> given that there's likely to be physical overheads from keeping it
> enabled.

Ah, thanks a lot for the explanations. The comment itself can be
improved though (I think).

>>> +		if (svcr & SYS_SVCR_EL0_SM_MASK)
>>> +			sme_smstop_sm();
> 
>> As per the SME syscall ABI
> 
>> | On syscall PSTATE.SM will be cleared and the SVE registers will be
>> | handled as per the standard SVE ABI.
> 
>> and the SVE syscall ABI
> 
>> | On syscall, V0..V31 are preserved (as without SVE).  Thus, bits
>> | [127:0] of Z0..Z31 are preserved.  All other bits of Z0..Z31, and all
>> | of P0..P15 and FFR become zero on return from a syscall.
> 
>> Can we infer from the documentation that V0-V31 should be preserved on
>> return from a syscall? But with sme_smstop_sm(), all implemented bits of
>> Z0-Z31 are set to zero by hardware. Is this intentional?
> 
>> Please fix me up if I've mis-read things here.
> 
> No, the intention is to say that we exit streaming mode and then handle
> things as per the non-streaming ABI.  Exiting streaming mode has the
> effect of clearing the values as you say.

Thanks,
Zenghui
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm