On Mon, 15 Aug 2022 23:55:25 +0100,
Mark Brown <broonie@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
