From: Marc Zyngier <maz@kernel.org>
To: Mark Brown <broonie@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
Zhang Lei <zhang.lei@jp.fujitsu.com>,
Andre Przywara <andre.przywara@arm.com>,
Will Deacon <will@kernel.org>,
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
Date: Tue, 20 Sep 2022 18:52:59 +0100 [thread overview]
Message-ID: <87wn9yj5l0.wl-maz@kernel.org> (raw)
In-Reply-To: <20220815225529.930315-4-broonie@kernel.org>
On Mon, 15 Aug 2022 23:55:25 +0100,
Mark Brown <broonie@kernel.org> 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 <broonie@kernel.org>
> ---
> 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.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
next prev parent reply other threads:[~2022-09-20 17:53 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-15 22:55 [PATCH v3 0/7] arm64/sve: Clean up KVM integration and optimise syscalls Mark Brown
2022-08-15 22:55 ` [PATCH v3 1/7] KVM: arm64: Discard any SVE state when entering KVM guests Mark Brown
2022-09-20 16:44 ` Marc Zyngier
2022-09-20 20:21 ` Mark Brown
2022-09-21 17:31 ` Marc Zyngier
2022-09-22 11:44 ` Mark Brown
2022-08-15 22:55 ` [PATCH v3 2/7] arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE Mark Brown
2022-09-20 17:14 ` Marc Zyngier
2022-09-20 18:09 ` Mark Brown
2022-09-20 18:30 ` Marc Zyngier
2022-08-15 22:55 ` [PATCH v3 3/7] arm64/fpsimd: Have KVM explicitly say which FP registers to save Mark Brown
2022-09-20 17:52 ` Marc Zyngier [this message]
2022-09-20 18:32 ` Mark Brown
2022-09-21 17:47 ` Marc Zyngier
2022-09-22 12:18 ` Mark Brown
2022-08-15 22:55 ` [PATCH v3 4/7] arm64/fpsimd: Stop using TIF_SVE to manage register saving in KVM Mark Brown
2022-09-20 18:04 ` Marc Zyngier
2022-09-20 18:53 ` Mark Brown
2022-08-15 22:55 ` [PATCH v3 5/7] arm64/fpsimd: Load FP state based on recorded data type Mark Brown
2022-09-20 18:19 ` Marc Zyngier
2022-09-20 19:02 ` Mark Brown
2022-08-15 22:55 ` [PATCH v3 6/7] arm64/fpsimd: SME no longer requires SVE register state Mark Brown
2022-08-15 22:55 ` [PATCH v3 7/7] arm64/sve: Leave SVE enabled on syscall if we don't context switch Mark Brown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87wn9yj5l0.wl-maz@kernel.org \
--to=maz@kernel.org \
--cc=andre.przywara@arm.com \
--cc=broonie@kernel.org \
--cc=catalin.marinas@arm.com \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=will@kernel.org \
--cc=zhang.lei@jp.fujitsu.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).