| Message ID | 20240329-arm64-2023-dpisa-v6-1-ba42db6c27f3@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Support for 2023 dpISA extensions | expand |
On Fri, 29 Mar 2024 00:13:42 +0000, 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. > 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? > > 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(-) > > 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; Well, this is going away, and you know it. M.
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?
On Tue, 02 Apr 2024 17:20:36 +0100, Mark Brown <broonie@kernel.org> wrote: > > 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. Here's a trick: you could ask. Other people do. > The amount of churn in KVM recently and long periods where the relevant > patches are apparently pre accepted but for various not always clear Nothing is "pre accepted". Everything gets discussed and reviewed. Specially when it comes to what you call "churn", which I call "crap removal". > reasons not actually merged is making it quite hard to target, you're Things get merged when they are reviewed and ready. Not before. > 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 This isn't about resolving a simple conflict. This is a fundamental change in the way the state is tracked. We have argued about this for months now, you were Cc'd on the patches addressing this problem, and you even reviewed them. What other hint do you need? > flight serieses your communication style means that it's not been > entirely clear if you're just noting the overlap. Not clear? That's a first. I'm usually seen as "blunt and assertive". But I'll keep that in mind and aspire to greater clarity in the future. > 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? That, and the already merged feature enforcement framework which you keep ignoring. I'll push out a rc3-based branch in to -next shortly so that it is crystal clear what you need to base things on. M.
On Wed, Apr 10, 2024 at 08:27:07AM +0100, Marc Zyngier wrote: > Mark Brown <broonie@kernel.org> wrote: > > On Tue, Apr 02, 2024 at 03:53:33PM +0100, Marc Zyngier wrote: > > > Mark Brown <broonie@kernel.org> wrote: > > > 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. > Here's a trick: you could ask. Other people do. I do ask questions from time to time, and you do respond to some of them, but in particular with "what's the status" type questions you have given the impression that they're not super welcome. TBH my thinking was just the standard submit against whatever is merged, rebase as needed approach - I was of course aware of your series but also aware that it was still in review. > > The amount of churn in KVM recently and long periods where the relevant > > patches are apparently pre accepted but for various not always clear > Nothing is "pre accepted". Everything gets discussed and reviewed. Sure, but then there's some things like your cleanup series which are still in the discussion and review phase (or at least not applied) but where you are saying you've got a strong expectation that other work should be based on them. Most if not all of those are clearly going to go in (as you indicate below with this one) but for whatever reason haven't actually done so yet, it's them that I'm referring to here. > Specially when it comes to what you call "churn", which I call "crap > removal". Indeed, a lot of things get churned for good reasons - the point here is not that there aren't valuable improvements being made (there definitely are here) but rather that it does make for a bit of a moving target. > > 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 > This isn't about resolving a simple conflict. This is a fundamental > change in the way the state is tracked. We have argued about this for I was on balance expecting to need to do the rebase at some point but unclear what approach you and your comaintainers would want. TBH I don't really see it as a huge update, refactoring with the current approach in this series should be a *broadly* mechanical operation. > months now, you were Cc'd on the patches addressing this problem, and > you even reviewed them. What other hint do you need? Honestly, them actually landing in git or a positive statement to the effect of "these will go in after whatever, please base your work on it" or whatever. It's not that I can't tell they're likely to go in at some point, it's just that I'm targetting the current tree. This is a fairly standard approach which many maintainers prefer. > > flight serieses your communication style means that it's not been > > entirely clear if you're just noting the overlap. > Not clear? That's a first. I'm usually seen as "blunt and assertive". > But I'll keep that in mind and aspire to greater clarity in the future. Those aren't quite the words I would use though I do understand the sentiment. One consequence of this style is that while it's obvious that you don't like something it's not always quite so obvious what the practical suggestion is, and since the general affect is so strong any emphasis is not really visible. More generally to this particular issue with rebasing on your series you will have noticed that I have submitted a number of serieses which aimed to address some of your review comments which you've rejected as not in fact being things you were looking for. > > 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? > That, and the already merged feature enforcement framework which you > keep ignoring. I'll push out a rc3-based branch in to -next shortly so > that it is crystal clear what you need to base things on. I really thought when I submitted this that I'd updated everything for the feature enforcement framework now that it has actually landed, and as I said in reply to the other patch apart from the nested idregs (which I had missed) and the exposure of CPA (which did actually introduce traps that I'd missed so I've updated to not expose CPA) I'm still unclear what exactly it is that I have missed there. Prior to that work landing this was all the same thing as I was mentioning above with submitting against what's actually applied, especially for serieses that aren't KVM specific where there's a higher cost for pulling in extra dependencies early. I was aware that there were changes coming but also aware that they were still not there yet.
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(-)