| Message ID | 20240329-arm64-2023-dpisa-v6-1-ba42db6c27f3@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: Support for 2023 dpISA extensions | expand |
On Sun, Mar 31, 2024 at 11:00:41AM +0100, Marc Zyngier wrote: > Mark Brown <broonie@kernel.org> wrote: > > As part of the lazy FPSIMD state transitioning done by the hypervisor we > > currently share the userpsace FPSIMD state in thread->uw.fpsimd_state with > > the host. Since this struct is non-extensible userspace ABI we have to keep > Using the same representation is just pure convenience, and nothing > requires us to use the it in the kernel/hypervisor. Indeed, the additional data seemed contained enough that it was a reasonable tradeoff. > > the definition as is but the addition of FPMR in the 2023 dpISA means that > > we will want to share more storage with the host. To facilitate this > > refactor the current code to share the entire thread->uw rather than just > > the one field. > So this increase the required sharing with EL2 from 528 bytes to > 560. Not a huge deal, but definitely moving in the wrong direction. Is > there any plans to add more stuff to this structure that wouldn't be > *directly* relevant to the hypervisor? I'm not aware of any current plans to extend this. > > @@ -640,7 +641,7 @@ struct kvm_vcpu_arch { > > struct kvm_guest_debug_arch vcpu_debug_state; > > struct kvm_guest_debug_arch external_debug_state; > > > > - struct user_fpsimd_state *host_fpsimd_state; /* hyp VA */ > > + struct thread_struct_uw *host_uw; /* hyp VA */ > > struct task_struct *parent_task; > Well, this is going away, and you know it. Sure, those patches are still in flight though. It does seem reasonable to target the current code.
On Tue, 02 Apr 2024 15:34:27 +0100, Mark Brown <broonie@kernel.org> wrote: > > [1 <text/plain; us-ascii (quoted-printable)>] > On Sun, Mar 31, 2024 at 11:00:41AM +0100, Marc Zyngier wrote: > > Mark Brown <broonie@kernel.org> wrote: > > > > As part of the lazy FPSIMD state transitioning done by the hypervisor we > > > currently share the userpsace FPSIMD state in thread->uw.fpsimd_state with > > > the host. Since this struct is non-extensible userspace ABI we have to keep > > > Using the same representation is just pure convenience, and nothing > > requires us to use the it in the kernel/hypervisor. > > Indeed, the additional data seemed contained enough that it was a > reasonable tradeoff. > > > > the definition as is but the addition of FPMR in the 2023 dpISA means that > > > we will want to share more storage with the host. To facilitate this > > > refactor the current code to share the entire thread->uw rather than just > > > the one field. > > > So this increase the required sharing with EL2 from 528 bytes to > > 560. Not a huge deal, but definitely moving in the wrong direction. Is > > there any plans to add more stuff to this structure that wouldn't be > > *directly* relevant to the hypervisor? > > I'm not aware of any current plans to extend this. > > > > @@ -640,7 +641,7 @@ struct kvm_vcpu_arch { > > > struct kvm_guest_debug_arch vcpu_debug_state; > > > struct kvm_guest_debug_arch external_debug_state; > > > > > > - struct user_fpsimd_state *host_fpsimd_state; /* hyp VA */ > > > + struct thread_struct_uw *host_uw; /* hyp VA */ > > > struct task_struct *parent_task; > > > Well, this is going away, and you know it. > > Sure, those patches are still in flight though. It does seem reasonable > to target the current code. Sure, if your intent is for this code not to be merged. Because it means this series assumes a different data life cycle, and the review effort spent on it will be invalidated once you move to the per-CPU state. M.
On Tue, Apr 02, 2024 at 03:53:33PM +0100, Marc Zyngier wrote: > Mark Brown <broonie@kernel.org> wrote: > > Sure, those patches are still in flight though. It does seem reasonable > > to target the current code. > Sure, if your intent is for this code not to be merged. > Because it means this series assumes a different data life cycle, and > the review effort spent on it will be invalidated once you move to the > per-CPU state. I don't have any visibility on when those patches are likely to get merged or the general practices with in flight serieses here, last time around with some of the serieses that were in flight it was quite late which did make it unclear if things would go in during that release cycle at all. The amount of churn in KVM recently and long periods where the relevant patches are apparently pre accepted but for various not always clear reasons not actually merged is making it quite hard to target, you're obviously going to be a lot more in the loop so this is doubtless clearer to you than to me. It's also been a little unclear what the expectations are for basing things on - some people do prefer to do their own merging for example, and while you have mentioned your in flight serieses your communication style means that it's not been entirely clear if you're just noting the overlap. Is it just that refactoring series you want taking into account here or are there other in flight serieses that should be rolled into a base?
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 9e8a496fb284..8a251f0da900 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -27,6 +27,7 @@ #include <asm/fpsimd.h> #include <asm/kvm.h> #include <asm/kvm_asm.h> +#include <asm/processor.h> #include <asm/vncr_mapping.h> #define __KVM_HAVE_ARCH_INTC_INITIALIZED @@ -640,7 +641,7 @@ struct kvm_vcpu_arch { struct kvm_guest_debug_arch vcpu_debug_state; struct kvm_guest_debug_arch external_debug_state; - struct user_fpsimd_state *host_fpsimd_state; /* hyp VA */ + struct thread_struct_uw *host_uw; /* hyp VA */ struct task_struct *parent_task; struct { diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index f77371232d8c..78781333ee26 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -152,7 +152,7 @@ struct thread_struct { * Maintainers must ensure manually that this contains no * implicit padding. */ - struct { + struct thread_struct_uw { unsigned long tp_value; /* TLS register */ unsigned long tp2_value; u64 fpmr; diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c index 826307e19e3a..8a0fedbb6f39 100644 --- a/arch/arm64/kvm/fpsimd.c +++ b/arch/arm64/kvm/fpsimd.c @@ -17,13 +17,13 @@ void kvm_vcpu_unshare_task_fp(struct kvm_vcpu *vcpu) { struct task_struct *p = vcpu->arch.parent_task; - struct user_fpsimd_state *fpsimd; + struct thread_struct_uw *uw; if (!is_protected_kvm_enabled() || !p) return; - fpsimd = &p->thread.uw.fpsimd_state; - kvm_unshare_hyp(fpsimd, fpsimd + 1); + uw = &p->thread.uw; + kvm_unshare_hyp(uw, uw + 1); put_task_struct(p); } @@ -39,17 +39,16 @@ void kvm_vcpu_unshare_task_fp(struct kvm_vcpu *vcpu) int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu) { int ret; - - struct user_fpsimd_state *fpsimd = ¤t->thread.uw.fpsimd_state; + struct thread_struct_uw *uw = ¤t->thread.uw; kvm_vcpu_unshare_task_fp(vcpu); /* Make sure the host task fpsimd state is visible to hyp: */ - ret = kvm_share_hyp(fpsimd, fpsimd + 1); + ret = kvm_share_hyp(uw, uw + 1); if (ret) return ret; - vcpu->arch.host_fpsimd_state = kern_hyp_va(fpsimd); + vcpu->arch.host_uw = kern_hyp_va(uw); /* * We need to keep current's task_struct pinned until its data has been diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h index e3fcf8c4d5b4..a9a11893c191 100644 --- a/arch/arm64/kvm/hyp/include/hyp/switch.h +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h @@ -377,7 +377,7 @@ static bool kvm_hyp_handle_fpsimd(struct kvm_vcpu *vcpu, u64 *exit_code) /* Write out the host state if it's in the registers */ if (vcpu->arch.fp_state == FP_STATE_HOST_OWNED) - __fpsimd_save_state(vcpu->arch.host_fpsimd_state); + __fpsimd_save_state(&(vcpu->arch.host_uw->fpsimd_state)); /* Restore the guest state */ if (sve_guest) diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c index 2385fd03ed87..eb2208009875 100644 --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c @@ -42,7 +42,7 @@ static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu) hyp_vcpu->vcpu.arch.fp_state = host_vcpu->arch.fp_state; hyp_vcpu->vcpu.arch.debug_ptr = kern_hyp_va(host_vcpu->arch.debug_ptr); - hyp_vcpu->vcpu.arch.host_fpsimd_state = host_vcpu->arch.host_fpsimd_state; + hyp_vcpu->vcpu.arch.host_uw = host_vcpu->arch.host_uw; hyp_vcpu->vcpu.arch.vsesr_el2 = host_vcpu->arch.vsesr_el2; @@ -64,7 +64,7 @@ static void sync_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu) host_vcpu->arch.fault = hyp_vcpu->vcpu.arch.fault; host_vcpu->arch.iflags = hyp_vcpu->vcpu.arch.iflags; - host_vcpu->arch.fp_state = hyp_vcpu->vcpu.arch.fp_state; + host_vcpu->arch.host_uw = hyp_vcpu->vcpu.arch.host_uw; host_cpu_if->vgic_hcr = hyp_cpu_if->vgic_hcr; for (i = 0; i < hyp_cpu_if->used_lrs; ++i)
As part of the lazy FPSIMD state transitioning done by the hypervisor we currently share the userpsace FPSIMD state in thread->uw.fpsimd_state with the host. Since this struct is non-extensible userspace ABI we have to keep the definition as is but the addition of FPMR in the 2023 dpISA means that we will want to share more storage with the host. To facilitate this refactor the current code to share the entire thread->uw rather than just the one field. The large number of references to fpsimd_state make it very inconvenient to add an additional wrapper struct. Signed-off-by: Mark Brown <broonie@kernel.org> --- arch/arm64/include/asm/kvm_host.h | 3 ++- arch/arm64/include/asm/processor.h | 2 +- arch/arm64/kvm/fpsimd.c | 13 ++++++------- arch/arm64/kvm/hyp/include/hyp/switch.h | 2 +- arch/arm64/kvm/hyp/nvhe/hyp-main.c | 4 ++-- 5 files changed, 12 insertions(+), 12 deletions(-)