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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 54748C6FA82 for ; Tue, 20 Sep 2022 17:54:25 +0000 (UTC) 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:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=YWaekzyysyPsDdHsKtnjioAoGd1zELtMemiZxQPUrzA=; b=miPkjOxLLsBStl 6932W9+xKUD/JEpSUxvVbMjWR+BZLl5TDRe6jrBXvwYtZ5tisMR4sPRNXsHl6bhLLyT3g4YlvJ7vG n8DQPPv2nIQWCitEEaFY3ZfL9VyGzy2FSrTA3Op1O7Q4a0SJHG001Add8xpeTE9ffuNV427U7GLJ5 9CO4sAP7kEKBiFDFj+JVMODSZWJ3AHSltw4HBix+1+PUOUwGLNUB53B7oXJOekYwC321fQdJccGTZ 8OwpTemSFhHSkQ1H1s/S6g1ZUANl5RZ/ChOnfudzks3sUYCgg4E5PFVCzh+0a18c1afmg8DVjpmr7 hROqG+Gsv/Cx+qaW5jNA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oahQj-005Ztg-3N; Tue, 20 Sep 2022 17:53:09 +0000 Received: from ams.source.kernel.org ([2604:1380:4601:e00::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oahQf-005ZsO-2M for linux-arm-kernel@lists.infradead.org; Tue, 20 Sep 2022 17:53:08 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 505F2B82BE2; Tue, 20 Sep 2022 17:53:03 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 11510C433D6; Tue, 20 Sep 2022 17:53:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1663696382; bh=xCgnmcH/2uSfbXCIRvneJfblf9LGApKKzOsPR5tudKQ=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=GsYCJPx2EPsFucIbphD0n1yrj4L5Yp3g/xSLupD3a5ArD7P4kN5nNyYTCZNELJdyA CLS1r7tRo5eS9/v76kv+Vn7GnxtAix5s8nSM1g47gQoQInKPEKnAE+JXyInGROHuDi nQ5PlZoGwbHyiIbfF0/srSg2gJosqXPPer9Aa0Q+wDAEkNZbbGLw/O+fnzADG8vgYB yP4NwXaVENjye+zos2/bP7/+fZx3j2oGfkOc16eebDxQYpQ2iHEEjhX2QD0m983Yhh ljzV/HioroOvfpGa52vF3WTK/+lEdr5OENcZJpBJ/znIB/FVPcvAmt5o/vMuidL9Qq V0LtnulNJkytQ== Received: from 185-176-101-241.host.sccbroadband.ie ([185.176.101.241] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1oahQZ-00BSvr-Ru; Tue, 20 Sep 2022 18:53:00 +0100 Date: Tue, 20 Sep 2022 18:52:59 +0100 Message-ID: <87wn9yj5l0.wl-maz@kernel.org> From: Marc Zyngier To: Mark Brown Cc: Catalin Marinas , Will Deacon , Zhang Lei , James Morse , Alexandru Elisei , Andre Przywara , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 3/7] arm64/fpsimd: Have KVM explicitly say which FP registers to save In-Reply-To: <20220815225529.930315-4-broonie@kernel.org> References: <20220815225529.930315-1-broonie@kernel.org> <20220815225529.930315-4-broonie@kernel.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 185.176.101.241 X-SA-Exim-Rcpt-To: broonie@kernel.org, catalin.marinas@arm.com, will@kernel.org, zhang.lei@jp.fujitsu.com, james.morse@arm.com, alexandru.elisei@arm.com, andre.przywara@arm.com, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220920_105306_401346_3BD545CF X-CRM114-Status: GOOD ( 40.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 Mon, 15 Aug 2022 23:55:25 +0100, Mark Brown wrote: > > In order to avoid needlessly saving and restoring the guest registers KVM > relies on the host FPSMID code to save the guest registers when we context > switch away from the guest. This is done by binding the KVM guest state to > the CPU on top of the task state that was originally there, then carefully > managing the TIF_SVE flag for the task to cause the host to save the full > SVE state when needed regardless of the needs of the host task. This works > well enough but isn't terribly direct about what is going on and makes it > much more complicated to try to optimise what we're doing with the SVE > register state. > > Let's instead have KVM pass in the register state it wants saving when it > binds to the CPU. We introduce a new FP_TYPE_TASK for use during normal > task binding to indicate that we should base our decisions on the current > task. In order to ease any future debugging that might be required this > patch does not actually update any of the decision making about what to > save, it merely starts tracking the new information and warns if the > requested state is not what we would otherwise have decided to save. > > Signed-off-by: Mark Brown > --- > arch/arm64/include/asm/fpsimd.h | 3 ++- > arch/arm64/include/asm/processor.h | 1 + > arch/arm64/kernel/fpsimd.c | 20 +++++++++++++++++++- > arch/arm64/kvm/fpsimd.c | 9 ++++++++- > 4 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h > index b74103a79052..21a1dd320ca5 100644 > --- a/arch/arm64/include/asm/fpsimd.h > +++ b/arch/arm64/include/asm/fpsimd.h > @@ -61,7 +61,8 @@ extern void fpsimd_kvm_prepare(void); > extern void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *state, > void *sve_state, unsigned int sve_vl, > void *za_state, unsigned int sme_vl, > - u64 *svcr, enum fp_state *type); > + u64 *svcr, enum fp_state *type, > + enum fp_state to_save); > > extern void fpsimd_flush_task_state(struct task_struct *target); > extern void fpsimd_save_and_flush_cpu_state(void); > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h > index 4818a6b77f39..89c248b8d4ba 100644 > --- a/arch/arm64/include/asm/processor.h > +++ b/arch/arm64/include/asm/processor.h > @@ -123,6 +123,7 @@ enum vec_type { > }; > > enum fp_state { > + FP_STATE_TASK, /* Save based on current, invalid as fp_type */ How is that related to the FP_TYPE_TASK in the commit message? What does this 'invalid as fp_type' mean? > FP_STATE_FPSIMD, > FP_STATE_SVE, > }; > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 6544ae00230f..7be20ced2c45 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -126,6 +126,7 @@ struct fpsimd_last_state_struct { > unsigned int sve_vl; > unsigned int sme_vl; > enum fp_state *fp_type; > + enum fp_state to_save; > }; > > static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state); > @@ -459,6 +460,21 @@ static void fpsimd_save(void) > vl = last->sve_vl; > } > > + /* > + * For now we're just validating that the requested state is > + * consistent with what we'd otherwise work out. Nit: work out? or worked out? the "we'd" doesn't help disambiguate it for a non-native speaker. > + */ > + switch (last->to_save) { > + case FP_STATE_TASK: > + break; > + case FP_STATE_FPSIMD: > + WARN_ON_ONCE(save_sve_regs); > + break; > + case FP_STATE_SVE: > + WARN_ON_ONCE(!save_sve_regs); > + break; > + } > + > if (system_supports_sme()) { > u64 *svcr = last->svcr; > > @@ -1693,6 +1709,7 @@ static void fpsimd_bind_task_to_cpu(void) > last->sme_vl = task_get_sme_vl(current); > last->svcr = ¤t->thread.svcr; > last->fp_type = ¤t->thread.fp_type; > + last->to_save = FP_STATE_TASK; > current->thread.fpsimd_cpu = smp_processor_id(); > > /* > @@ -1717,7 +1734,7 @@ static void fpsimd_bind_task_to_cpu(void) > void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st, void *sve_state, > unsigned int sve_vl, void *za_state, > unsigned int sme_vl, u64 *svcr, > - enum fp_state *type) > + enum fp_state *type, enum fp_state to_save) OK, how many discrete arguments are we going to pass to this function, which most of them are part the vcpu structure? It really feels like what you want is a getter for the per-cpu structure, and let the KVM code do the actual business. If this function was supposed to provide some level of abstraction, well, it's a fail. > { > struct fpsimd_last_state_struct *last = > this_cpu_ptr(&fpsimd_last_state); > @@ -1732,6 +1749,7 @@ void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st, void *sve_state, > last->sve_vl = sve_vl; > last->sme_vl = sme_vl; > last->fp_type = type; > + last->to_save = to_save; > } > > /* > diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c > index a92977759f8d..db0b2bacaeb8 100644 > --- a/arch/arm64/kvm/fpsimd.c > +++ b/arch/arm64/kvm/fpsimd.c > @@ -130,9 +130,16 @@ void kvm_arch_vcpu_ctxflush_fp(struct kvm_vcpu *vcpu) > */ > void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu) > { > + enum fp_state fp_type; > + > WARN_ON_ONCE(!irqs_disabled()); > > if (vcpu->arch.fp_state == FP_STATE_GUEST_OWNED) { > + if (vcpu_has_sve(vcpu)) > + fp_type = FP_STATE_SVE; Eventually, I'd like to relax this, and start tracking the actual use of the guest rather than assuming that SVE guest use SVE at all times (odds are they won't). I hope this series still leaves us with this option. Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel