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.8 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=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 F19BDC433E0 for ; Tue, 9 Feb 2021 18:01:49 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 8AF6F64DBA for ; Tue, 9 Feb 2021 18:01:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8AF6F64DBA Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject: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=dtivNa6fPlTmQ3c7bZVR1WdeR72lZWe0alqbCaPfIig=; b=WutBIWlwglefYjKWwWvyzxStS J0hGPJ7z8uA3zx2Ms0FOdYgErFSXNecS03NY/3Ra8y3nPRqDB/TypuBfyDe6HX6GUVq7dZWK2vwS7 qPYvi/yVvGHtf1dEpnUSTTJSv6EQCktttImtka/PyydxGC3HebXqaen0tECWVv+UbFiV0VklNyAnk 3SVAfGplkqSJRsiQjP/+KddRgZ39VhjqbmAb40htM8Qq97qu9xYU9OsjJNQnRj6iON3D1+Ohjv4QI MR/wmifk0NRAFXZdeeDRJvunc6TIjUhgj6zRHy6kglD5P+S7Lhf7Vf4dnAfMCB2eA2jMqmbdArRX1 yqsXfVvLg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l9XJH-0002wz-9W; Tue, 09 Feb 2021 18:00:23 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l9XJA-0002tk-Tf for linux-arm-kernel@lists.infradead.org; Tue, 09 Feb 2021 18:00: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 54BD1101E; Tue, 9 Feb 2021 10:00:09 -0800 (PST) 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 1CEF13F73B; Tue, 9 Feb 2021 10:00:08 -0800 (PST) Date: Tue, 9 Feb 2021 17:59:46 +0000 From: Dave Martin To: Mark Brown Subject: Re: [PATCH v7 1/2] arm64/sve: Split TIF_SVE into separate execute and register state flags Message-ID: <20210209175943.GH21837@arm.com> References: <20210201122901.11331-1-broonie@kernel.org> <20210201122901.11331-2-broonie@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210201122901.11331-2-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-20210209_130017_193513_136DD8DD X-CRM114-Status: GOOD ( 54.56 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Julien Grall , Julien Grall , Catalin Marinas , Zhang Lei , Will Deacon , linux-arm-kernel@lists.infradead.org, Daniel Kiss 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 Mon, Feb 01, 2021 at 12:29:00PM +0000, Mark Brown wrote: > Currently we have a single flag TIF_SVE which says that a task is > allowed to execute SVE instructions without trapping and also that full > SVE register state is stored for the task. This results in us doing > extra work storing and restoring the full SVE register state even in > those cases where the ABI is that only the first 128 bits of the Z0-V31 > registers which are shared with the FPSIMD V0-V31 are valid. > > In order to allow us to avoid these overheads split TIF_SVE up so that > we have two separate flags, TIF_SVE_EXEC which allows execution of SVE > instructions without trapping and TIF_SVE_FULL_REGS which indicates that > the full SVE register state is stored. If both are set the behaviour is > as currently, if TIF_SVE_EXEC is set without TIF_SVE_FULL_REGS then we > save and restore only the FPSIMD registers until we return to userspace > with TIF_SVE_EXEC enabled at which point we convert the FPSIMD registers > to SVE. It is not meaningful to have TIF_SVE_FULL_REGS set without > TIF_SVE_EXEC. > > This patch is intended only to split the flags, it does not take > avantage of the ability to set the flags independently and the new > state with TIF_SVE_EXEC only should not be observed. > > This is based on earlier work by Julien Gral implementing a slightly > different approach. General thoughts: I'm wondering if TIF_SVE_FULL_REGS is actually conflating two things here: a) whether the SVE regs can be discarded b) how the FPSIMD/SVE regs are encoded in thread_struct. When implementing a trivial policy for discarding the SVE regs, we discard the regs at the earliest opportunity, so (a) and (b) are not very distinct. But if we try to be cleverer later on then this would break down. If we separate the two meanings from the outset, would it help to steer any future policy stuff in a more maintainable direction. This might mean having two flags instead of one. > Signed-off-by: Mark Brown > --- > arch/arm64/include/asm/thread_info.h | 3 +- > arch/arm64/kernel/fpsimd.c | 174 +++++++++++++++++++-------- > arch/arm64/kernel/process.c | 7 +- > arch/arm64/kernel/ptrace.c | 8 +- > arch/arm64/kernel/signal.c | 15 ++- > arch/arm64/kernel/syscall.c | 3 +- > arch/arm64/kvm/fpsimd.c | 6 +- > 7 files changed, 152 insertions(+), 64 deletions(-) > > diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h > index 9f4e3b266f21..c856159e071c 100644 > --- a/arch/arm64/include/asm/thread_info.h > +++ b/arch/arm64/include/asm/thread_info.h > @@ -65,6 +65,7 @@ void arch_release_task_struct(struct task_struct *tsk); > #define TIF_UPROBE 4 /* uprobe breakpoint or singlestep */ > #define TIF_MTE_ASYNC_FAULT 5 /* MTE Asynchronous Tag Check Fault */ > #define TIF_NOTIFY_SIGNAL 6 /* signal notifications exist */ > +#define TIF_SVE_EXEC 7 /* SVE instructions don't trap */ > #define TIF_SYSCALL_TRACE 8 /* syscall trace active */ > #define TIF_SYSCALL_AUDIT 9 /* syscall auditing */ > #define TIF_SYSCALL_TRACEPOINT 10 /* syscall tracepoint for ftrace */ > @@ -75,7 +76,7 @@ void arch_release_task_struct(struct task_struct *tsk); > #define TIF_RESTORE_SIGMASK 20 > #define TIF_SINGLESTEP 21 > #define TIF_32BIT 22 /* 32bit process */ > -#define TIF_SVE 23 /* Scalable Vector Extension in use */ > +#define TIF_SVE_FULL_REGS 23 /* Full SVE register set stored */ > #define TIF_SVE_VL_INHERIT 24 /* Inherit sve_vl_onexec across exec */ > #define TIF_SSBD 25 /* Wants SSB mitigation */ > #define TIF_TAGGED_ADDR 26 /* Allow tagged user addresses */ > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 062b21f30f94..58c749ef04c4 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -215,48 +215,70 @@ static bool have_cpu_fpsimd_context(void) > */ > static void __sve_free(struct task_struct *task) > { > + /* SVE context will be zeroed when allocated. */ > + clear_tsk_thread_flag(task, TIF_SVE_FULL_REGS); Can you elaborate on why this is here? I'm not sure this is the right place to clear flags. This code is really just for freeing the buffer, along with whatever checks are necessary to confirm that this is safe / sane to do. (i.e., TIF_SVE(_EXEC) set.). > kfree(task->thread.sve_state); > task->thread.sve_state = NULL; > } > > static void sve_free(struct task_struct *task) > { > - WARN_ON(test_tsk_thread_flag(task, TIF_SVE)); > + WARN_ON(test_tsk_thread_flag(task, TIF_SVE_EXEC)); > > __sve_free(task); > } > > /* > - * TIF_SVE controls whether a task can use SVE without trapping while > - * in userspace, and also the way a task's FPSIMD/SVE state is stored > - * in thread_struct. > + * In order to avoid the expense of storing the SVE registers when not > + * in active use by tasks we keep track of the task's SVE usage and > + * only allocate space for SVE registers for tasks that need it. In > + * addition since on first use and after every syscall only the portion > + * of the SVE registers shared with FPSIMD are used we separately track > + * if we need to actually save all that state. > * > - * The kernel uses this flag to track whether a user task is actively > - * using SVE, and therefore whether full SVE register state needs to > - * be tracked. If not, the cheaper FPSIMD context handling code can > - * be used instead of the more costly SVE equivalents. > + * TIF_SVE_EXEC controls whether a task can use SVE without trapping > + * while in userspace. TIF_SVE_FULL_REGS controls the way a task's > + * FPSIMD/SVE state is stored in thread_struct. The kernel uses this > + * flag to track whether a user task has active SVE state, and > + * therefore whether full SVE register state needs to be tracked. If > + * not, the cheaper FPSIMD context handling code can be used instead > + * of the more costly SVE equivalents. > * > - * * TIF_SVE set: > + * * FPSR and FPCR are always stored in task->thread.uw.fpsimd_state > + * irrespective of any flags, since these are not vector length > + * dependent. > * > - * The task can execute SVE instructions while in userspace without > - * trapping to the kernel. > + * * TIF_SVE_EXEC is not set: > * > - * When stored, Z0-Z31 (incorporating Vn in bits[127:0] or the > - * corresponding Zn), P0-P15 and FFR are encoded in in > - * task->thread.sve_state, formatted appropriately for vector > - * length task->thread.sve_vl. > + * An attempt by the user task to execute an SVE instruction causes > + * do_sve_acc() to be called, which does some preparation and sets > + * TIF_SVE_EXEC. > + * > + * When stored, FPSIMD registers V0-V31 are encoded in > + * task->thread.uw.fpsimd_state; bits [max : 128] for each of Z0-Z31 are > + * logically zero but not stored anywhere; P0-P15 and FFR are not > + * stored and have unspecified values from userspace's point of > + * view. For hygiene purposes, the kernel zeroes them on next use, > + * but userspace is discouraged from relying on this. > + * > + * task->thread.sve_state does not need to be non-NULL, valid or any > + * particular size: it must not be dereferenced. TIF_SVE_FULL_REGS > + * will have no effect and should never be set. > + * > + * * TIF_SVE_EXEC set: > + * > + * The task can execute SVE instructions while in userspace without > + * trapping to the kernel. Storage of Z0-Z31 (incorporating Vn in > + * bits[0-127]) is determined by TIF_SVE_FULL_REGS. > * > * task->thread.sve_state must point to a valid buffer at least > * sve_state_size(task) bytes in size. > * > - * During any syscall, the kernel may optionally clear TIF_SVE and > - * discard the vector state except for the FPSIMD subset. > - * > - * * TIF_SVE clear: > + * During any syscall the ABI allows the kernel to discard the > + * vector state other than the FPSIMD subset. When this is done > + * both TIF_SVE_EXEC and TIF_SVE_FULL_REGS will be cleared. Can this occur if TIF_SVE_FULL_REGS is initially set? I thought !TIF_SVE_FULL_REGS our way of telling ourselves that we can discard the SVE state at all... > * > - * An attempt by the user task to execute an SVE instruction causes > - * do_sve_acc() to be called, which does some preparation and then > - * sets TIF_SVE. > + * * TIF_SVE_FULL_REGS is not set: > * > * When stored, FPSIMD registers V0-V31 are encoded in > * task->thread.uw.fpsimd_state; bits [max : 128] for each of Z0-Z31 are > @@ -265,12 +287,38 @@ static void sve_free(struct task_struct *task) > * view. For hygiene purposes, the kernel zeroes them on next use, > * but userspace is discouraged from relying on this. > * > - * task->thread.sve_state does not need to be non-NULL, valid or any > - * particular size: it must not be dereferenced. > + * On entry to the kernel other than from a syscall the kernel must > + * set TIF_SVE_FULL_REGS and save the full register state. Only if TIF_SVE_EXEC is set though? I had thought that it is more natural to set TIF_SVE_FULL_REGS in preparation for entering userspace. This does mean that if we are preempted between fpsimd_restore_current_state() and exception return to userspace, then we would have to save the full regs unnecessarily. This can only happen during a small window though, so it should be rare very unless the system is already thrashing. This is the situation prior to your series IIUC. Alternatively, we set TIF_SVE_FULL_REGS when entering the kernel on a non-syscall path with TIF_SVE_EXEC set - but that adds a small overhead to every kernel entry and feels slightly messier. > - * * FPSR and FPCR are always stored in task->thread.uw.fpsimd_state > - * irrespective of whether TIF_SVE is clear or set, since these are > - * not vector length dependent. > + * * TIF_SVE_FULL_REGS is set: > + * > + * This flag is only valid when TIF_SVE_EXEC is set. > + * > + * When stored, Z0-Z31 (incorporating Vn in bits[127:0] or the > + * corresponding Zn), P0-P15 and FFR are encoded in in > + * task->thread.sve_state, formatted appropriately for vector > + * length task->thread.sve_vl. > + * > + * On entry to the kernel from a syscall this flag and TIF_SVE_EXEC > + * are cleared and only the FPSIMD subset of the register state is > + * stored. > + * > + * In summary, combined with TIF_FOREIGN_FPSTATE: > + * > + * !SVE _EXEC _EXEC+_FULL_REGS > + * +---------------+---------------+---------------+ > + * | Valid: FPSIMD | Valid: FPSIMD | Valid: SVE | > + * !FFP | Trap: Yes | Trap: No | Trap: No | > + * | Where: regs | Where: regs | Where: regs | > + * +---------------+---------------+---------------+ > + * | Valid: FPSIMD | Valid: FPSIMD | Valid: SVE | > + * FFP | Trap: Yes | Trap: No | Trap: No | > + * | Where: memory | Where: memory | Where: memory | > + * +---------------+---------------+---------------+ > + * > + * Where valid indicates what state is valid, trap indicates if we > + * should trap on executing a SVE instruction and where indicates > + * where the current copy of the register state is. I wonder if it would help to describe TIF_SVE_FULL_REGS orthogonally to TIF_SVE_EXEC. Although we don't intend to implement the combination !TIF_SVE_EXEC && TIF_SVE_FULL_REGS, it does have a logically consistent meaning (SVE disabled for userspace, but Vn nonetheless stored in sve_state, laid out according to thread->sve_vl). When flipping the flags, there may be a hazard where this combination temporarily appears, but we could hide that in a helper that runs under get_cpu_fpsimd_context() -- just like we already do for various other things. (We could avoid this by adding atomic thread_flags manipulators that flip multiple flags, but that's overkill just for this.) This makes no functional difference, but it might help simplify the description a bit. > */ > > /* > @@ -279,18 +327,37 @@ static void sve_free(struct task_struct *task) > * This function should be called only when the FPSIMD/SVE state in > * thread_struct is known to be up to date, when preparing to enter > * userspace. > + * > + * When TIF_SVE_EXEC is set but TIF_SVE_FULL_REGS is not set the SVE > + * state will be restored from the FPSIMD state. > */ > static void task_fpsimd_load(void) > { > + unsigned int vl; > + > WARN_ON(!system_supports_fpsimd()); > WARN_ON(!have_cpu_fpsimd_context()); > > - if (system_supports_sve() && test_thread_flag(TIF_SVE)) > - sve_load_state(sve_pffr(¤t->thread), > - ¤t->thread.uw.fpsimd_state.fpsr, > - sve_vq_from_vl(current->thread.sve_vl) - 1); > - else > - fpsimd_load_state(¤t->thread.uw.fpsimd_state); > + if (test_thread_flag(TIF_SVE_EXEC)) { Was system_supports_sve() dropped on purpose? And why? > + vl = sve_vq_from_vl(current->thread.sve_vl) - 1; > + > + /* > + * We always return with the full register state, if . If (Otherwise this misparses as "We always return with the full register state [] if there is no explicit SVE state [] .) > + * there is no explicit SVE state load from the FPSIMD > + * state instead. > + */ To keep line length down with less impact to readability, can we take a pointer to current->thread.uw.fpsimd_state in advance here? We use it multiple times anyway. > + if (test_and_set_thread_flag(TIF_SVE_FULL_REGS)) > + sve_load_state(sve_pffr(¤t->thread), > + ¤t->thread.uw.fpsimd_state.fpsr, > + vl); > + else > + sve_load_from_fpsimd_state(¤t->thread.uw.fpsimd_state, > + vl); As observed above, there could be an argument for setting TIF_SVE_FULL_REGS here, since the only reason to actually load the regs is in preparation for entering userspace, which can freely dirty all the SVE bits undetected since trapping for EL0 will be disabled due to TIF_SVE_EXEC being est. > + > + return; > + } > + > + fpsimd_load_state(¤t->thread.uw.fpsimd_state); Maybe this could be more readable, as well as reducing indentation on the more complex branch of the if, as: if (!system_supports_sve || !test_thread_flag(TIF_SVE_EXEC)) { fpsimd_load_state(¤t->thread.uw.fpsimd_state); return; } /* handle TIF_SVE_EXEC case */ > } > > /* > @@ -307,7 +374,7 @@ static void fpsimd_save(void) > WARN_ON(!have_cpu_fpsimd_context()); > > if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) { > - if (system_supports_sve() && test_thread_flag(TIF_SVE)) { > + if (system_supports_sve() && test_thread_flag(TIF_SVE_EXEC)) { > if (WARN_ON(sve_get_vl() != last->sve_vl)) { > /* > * Can't save the user regs, so current would > @@ -318,6 +385,7 @@ static void fpsimd_save(void) > return; > } > > + set_thread_flag(TIF_SVE_FULL_REGS); Why do this here? What if we're in a syscall? > sve_save_state((char *)last->sve_state + > sve_ffr_offset(last->sve_vl), > &last->st->fpsr); > @@ -536,7 +604,7 @@ void sve_alloc(struct task_struct *task) > */ > void fpsimd_sync_to_sve(struct task_struct *task) > { > - if (!test_tsk_thread_flag(task, TIF_SVE)) > + if (!test_tsk_thread_flag(task, TIF_SVE_FULL_REGS)) > fpsimd_to_sve(task); > } > > @@ -550,7 +618,7 @@ void fpsimd_sync_to_sve(struct task_struct *task) > */ > void sve_sync_to_fpsimd(struct task_struct *task) > { > - if (test_tsk_thread_flag(task, TIF_SVE)) > + if (test_tsk_thread_flag(task, TIF_SVE_FULL_REGS)) > sve_to_fpsimd(task); > } > > @@ -572,7 +640,7 @@ void sve_sync_from_fpsimd_zeropad(struct task_struct *task) > void *sst = task->thread.sve_state; > struct user_fpsimd_state const *fst = &task->thread.uw.fpsimd_state; > > - if (!test_tsk_thread_flag(task, TIF_SVE)) > + if (!test_tsk_thread_flag(task, TIF_SVE_EXEC)) > return; > > vq = sve_vq_from_vl(task->thread.sve_vl); > @@ -627,8 +695,9 @@ int sve_set_vector_length(struct task_struct *task, > } > > fpsimd_flush_task_state(task); > - if (test_and_clear_tsk_thread_flag(task, TIF_SVE)) > + if (test_and_clear_tsk_thread_flag(task, TIF_SVE_FULL_REGS)) > sve_to_fpsimd(task); Won't this mean that we dereference thread->sve_state even if TIF_SVE_EXEC was clear? For the ptrace case, I think we can probably can hit this, IIUC. It might not apply to the prctl() case if TIF_SVE_EXEC was already cleared during syscall entry (?), but as observed above this still assumes that the SVE regs are discarded at the earliest opportinuty, which might not be true in future. > + clear_thread_flag(TIF_SVE_EXEC); > > if (task == current) > put_cpu_fpsimd_context(); > @@ -926,13 +995,14 @@ void fpsimd_release_task(struct task_struct *dead_task) > * Trapped SVE access > * > * Storage is allocated for the full SVE state, the current FPSIMD > - * register contents are migrated across, and TIF_SVE is set so that > + * register contents are migrated across, and TIF_SVE_EXEC is set so that > * the SVE access trap will be disabled the next time this task > * reaches ret_to_user. > * > - * TIF_SVE should be clear on entry: otherwise, fpsimd_restore_current_state() > - * would have disabled the SVE access trap for userspace during > - * ret_to_user, making an SVE access trap impossible in that case. > + * TIF_SVE_EXEC should be clear on entry: otherwise, > + * fpsimd_restore_current_state() would have disabled the SVE access > + * trap for userspace during ret_to_user, making an SVE access trap > + * impossible in that case. > */ > void do_sve_acc(unsigned int esr, struct pt_regs *regs) > { > @@ -952,8 +1022,9 @@ void do_sve_acc(unsigned int esr, struct pt_regs *regs) > fpsimd_flush_task_state(current); > > fpsimd_to_sve(current); Hmmm, there's a latent bug upstream here: if the WARN() fires then sve_state is not safe to dereference. But we already did. So perhaps this should have been something like: if (!WARN_ON(test_and_set_thread_flag(TIF_SVE))) fpsimd_to_sve(); This might make sense as a separate Fixes patch to precede the series. > - if (test_and_set_thread_flag(TIF_SVE)) > + if (test_and_set_thread_flag(TIF_SVE_EXEC)) > WARN_ON(1); /* SVE access shouldn't have trapped */ > + set_thread_flag(TIF_SVE_FULL_REGS); > > put_cpu_fpsimd_context(); > } > @@ -1033,7 +1104,8 @@ void fpsimd_flush_thread(void) > sizeof(current->thread.uw.fpsimd_state)); > > if (system_supports_sve()) { > - clear_thread_flag(TIF_SVE); > + clear_thread_flag(TIF_SVE_EXEC); > + clear_thread_flag(TIF_SVE_FULL_REGS); > sve_free(current); > > /* > @@ -1092,7 +1164,7 @@ void fpsimd_preserve_current_state(void) > void fpsimd_signal_preserve_current_state(void) > { > fpsimd_preserve_current_state(); > - if (system_supports_sve() && test_thread_flag(TIF_SVE)) > + if (system_supports_sve() && test_thread_flag(TIF_SVE_FULL_REGS)) So, here TIF_SVE_FULL_REGS is assumed to be meaningful even in the absence of TIF_SVE_EXEC. While that's not necessarily wrong, it's a bit hard to prove it. If we go down this route, then the confition for sve_state validity should probably be TIF_SVE_FULL_REGS and not TIF_SVE_EXEC, etc. In the comments, you still say that if TIF_SVE_EXEC is set, then sve_state must be valid. If TIF_SVE_FULL_REGS is never set when TIF_SVE_EXEC is clear that we get away with it, but I wonder whether this would bitrot and break under future maintenance. > sve_to_fpsimd(current); > } > > @@ -1114,7 +1186,7 @@ void fpsimd_bind_task_to_cpu(void) > > if (system_supports_sve()) { > /* Toggle SVE trapping for userspace if needed */ > - if (test_thread_flag(TIF_SVE)) > + if (test_thread_flag(TIF_SVE_EXEC)) > sve_user_enable(); > else > sve_user_disable(); > @@ -1163,6 +1235,14 @@ void fpsimd_restore_current_state(void) > if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { > task_fpsimd_load(); > fpsimd_bind_task_to_cpu(); > + } else { > + /* > + * Convert FPSIMD state to SVE if userspace can execute SVE > + * but we have no explicit SVE state. > + */ > + if (test_thread_flag(TIF_SVE_EXEC) && > + !test_and_set_thread_flag(TIF_SVE_FULL_REGS)) > + sve_flush_live(); > } > > put_cpu_fpsimd_context(); > @@ -1181,7 +1261,7 @@ void fpsimd_update_current_state(struct user_fpsimd_state const *state) > get_cpu_fpsimd_context(); > > current->thread.uw.fpsimd_state = *state; > - if (system_supports_sve() && test_thread_flag(TIF_SVE)) > + if (test_thread_flag(TIF_SVE_FULL_REGS)) Unintentionally dropped system_supports_sve()? > fpsimd_to_sve(current); > > task_fpsimd_load(); > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 6616486a58fe..71c8265b9139 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -364,13 +364,14 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) > * Detach src's sve_state (if any) from dst so that it does not > * get erroneously used or freed prematurely. dst's sve_state > * will be allocated on demand later on if dst uses SVE. > - * For consistency, also clear TIF_SVE here: this could be done > + * For consistency, also clear TIF_SVE_* here: this could be done > * later in copy_process(), but to avoid tripping up future > - * maintainers it is best not to leave TIF_SVE and sve_state in > + * maintainers it is best not to leave TIF_SVE_* and sve_state in > * an inconsistent state, even temporarily. > */ > dst->thread.sve_state = NULL; > - clear_tsk_thread_flag(dst, TIF_SVE); > + clear_tsk_thread_flag(dst, TIF_SVE_EXEC); > + clear_tsk_thread_flag(dst, TIF_SVE_FULL_REGS); > > /* clear any pending asynchronous tag fault raised by the parent */ > clear_tsk_thread_flag(dst, TIF_MTE_ASYNC_FAULT); > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c > index 8ac487c84e37..f0406b3dc389 100644 > --- a/arch/arm64/kernel/ptrace.c > +++ b/arch/arm64/kernel/ptrace.c > @@ -719,7 +719,7 @@ static void sve_init_header_from_task(struct user_sve_header *header, > > memset(header, 0, sizeof(*header)); > > - header->flags = test_tsk_thread_flag(target, TIF_SVE) ? > + header->flags = test_tsk_thread_flag(target, TIF_SVE_FULL_REGS) ? Same observations as for fpsimd_signal_preserve_current_state(). > SVE_PT_REGS_SVE : SVE_PT_REGS_FPSIMD; > if (test_tsk_thread_flag(target, TIF_SVE_VL_INHERIT)) > header->flags |= SVE_PT_VL_INHERIT; > @@ -827,7 +827,8 @@ static int sve_set(struct task_struct *target, > if ((header.flags & SVE_PT_REGS_MASK) == SVE_PT_REGS_FPSIMD) { > ret = __fpr_set(target, regset, pos, count, kbuf, ubuf, > SVE_PT_FPSIMD_OFFSET); > - clear_tsk_thread_flag(target, TIF_SVE); > + clear_tsk_thread_flag(target, TIF_SVE_EXEC); > + clear_tsk_thread_flag(target, TIF_SVE_FULL_REGS); > goto out; > } > > @@ -851,7 +852,8 @@ static int sve_set(struct task_struct *target, > * unmodified. > */ > fpsimd_sync_to_sve(target); > - set_tsk_thread_flag(target, TIF_SVE); > + set_tsk_thread_flag(target, TIF_SVE_EXEC); > + set_tsk_thread_flag(target, TIF_SVE_FULL_REGS); > > BUILD_BUG_ON(SVE_PT_SVE_OFFSET != sizeof(header)); > start = SVE_PT_SVE_OFFSET; > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c > index f71d6ce4673f..5d5610af7ea3 100644 > --- a/arch/arm64/kernel/signal.c > +++ b/arch/arm64/kernel/signal.c > @@ -205,7 +205,8 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx) > __get_user_error(fpsimd.fpsr, &ctx->fpsr, err); > __get_user_error(fpsimd.fpcr, &ctx->fpcr, err); > > - clear_thread_flag(TIF_SVE); > + clear_thread_flag(TIF_SVE_EXEC); > + clear_thread_flag(TIF_SVE_FULL_REGS); > > /* load the hardware registers from the fpsimd_state structure */ > if (!err) > @@ -229,7 +230,7 @@ static int preserve_sve_context(struct sve_context __user *ctx) > unsigned int vl = current->thread.sve_vl; > unsigned int vq = 0; > > - if (test_thread_flag(TIF_SVE)) > + if (test_thread_flag(TIF_SVE_EXEC)) > vq = sve_vq_from_vl(vl); > > memset(reserved, 0, sizeof(reserved)); > @@ -241,7 +242,7 @@ static int preserve_sve_context(struct sve_context __user *ctx) > BUILD_BUG_ON(sizeof(ctx->__reserved) != sizeof(reserved)); > err |= __copy_to_user(&ctx->__reserved, reserved, sizeof(reserved)); > > - if (vq) { > + if (vq && test_thread_flag(TIF_SVE_FULL_REGS)) { Hmm, in theory we should be able to mark the regs as discardable once they have been saved in the frame, though we never did that in the past. Since we never zeroed the extra bits on signal handler entry anyway, we could actually skip the zeroing for this specific case. Any change of this sort should go in a separate patch though. (Strictly speaking it could be an ABI break, though it would be pretty insane -- and hard work -- for userspace to rely somehow on the full SVE regs showing through to the signal handler.) > /* > * This assumes that the SVE state has already been saved to > * the task struct by calling the function > @@ -269,7 +270,8 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user) > return -EINVAL; > > if (sve.head.size <= sizeof(*user->sve)) { > - clear_thread_flag(TIF_SVE); > + clear_thread_flag(TIF_SVE_EXEC); > + clear_thread_flag(TIF_SVE_FULL_REGS); > goto fpsimd_only; > } > > @@ -296,7 +298,8 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user) > if (err) > return -EFAULT; > > - set_thread_flag(TIF_SVE); > + set_thread_flag(TIF_SVE_EXEC); > + set_thread_flag(TIF_SVE_FULL_REGS); > > fpsimd_only: > /* copy the FP and status/control registers */ > @@ -587,7 +590,7 @@ static int setup_sigframe_layout(struct rt_sigframe_user_layout *user, > if (system_supports_sve()) { > unsigned int vq = 0; > > - if (add_all || test_thread_flag(TIF_SVE)) { > + if (add_all || test_thread_flag(TIF_SVE_EXEC)) { Doesn't this need to match the SVE reg dumping condition in preserve_sve_context()? > int vl = sve_max_vl; > > if (!add_all) > diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c > index f61e9d8cc55a..f8a2598730c2 100644 > --- a/arch/arm64/kernel/syscall.c > +++ b/arch/arm64/kernel/syscall.c > @@ -186,7 +186,8 @@ static inline void sve_user_discard(void) > if (!system_supports_sve()) > return; > > - clear_thread_flag(TIF_SVE); > + clear_thread_flag(TIF_SVE_EXEC); > + clear_thread_flag(TIF_SVE_FULL_REGS); (Assuming this will be refined in the next patch.) > /* > * task_fpsimd_load() won't be called to update CPACR_EL1 in > diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c > index 3e081d556e81..1b7c0d03581b 100644 > --- a/arch/arm64/kvm/fpsimd.c > +++ b/arch/arm64/kvm/fpsimd.c > @@ -67,7 +67,7 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu) > KVM_ARM64_HOST_SVE_ENABLED); > vcpu->arch.flags |= KVM_ARM64_FP_HOST; > > - if (test_thread_flag(TIF_SVE)) > + if (test_thread_flag(TIF_SVE_EXEC)) > vcpu->arch.flags |= KVM_ARM64_HOST_SVE_IN_USE; Here we're setting TIF_SVE(_EXEC) to describe the guest context, while stashing off the host's TIF_SVE(_EXEC) flag so we can get it back later. Don't we need to do a similar things for TIF_SVE_FULL_REGS though? For the vcpu context, TIF_SVE_FULL_REGS needs to be set for SVE-enabled vcpus (where we don't have a !FULL_REGS case). For the host, this flag might always be clear if we have to be in the KVM_RUN ioctl to get here, but if we're relying on that we should at least stick a WARN_ON() here. > > if (read_sysreg(cpacr_el1) & CPACR_EL1_ZEN_EL0EN) > @@ -90,7 +90,7 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu) > vcpu->arch.sve_max_vl); > > clear_thread_flag(TIF_FOREIGN_FPSTATE); > - update_thread_flag(TIF_SVE, vcpu_has_sve(vcpu)); > + update_thread_flag(TIF_SVE_EXEC, vcpu_has_sve(vcpu)); Does TIF_SVE_FULL_REGS need to be set similarly? > } > } > > @@ -127,7 +127,7 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu) > sysreg_clear_set(CPACR_EL1, CPACR_EL1_ZEN_EL0EN, 0); > } > > - update_thread_flag(TIF_SVE, > + update_thread_flag(TIF_SVE_EXEC, > vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE); And here does TIF_SVE_FULL_REGS need to be restored (or perhaps unconditionally cleared if we believe we can't get here while the host task has full regs?) [...] Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel