On Thu, 01 Aug 2024 20:07:16 +0100,
Mark Brown <broonie@xxxxxxxxxx> wrote:
>
> [1 <text/plain; us-ascii (7bit)>]
> On Thu, Aug 01, 2024 at 10:19:51AM +0100, Marc Zyngier wrote:
>
> > index 6af179c6356d..2466dd231362 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/switch.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
> > @@ -198,6 +198,15 @@ static void kvm_hyp_save_fpsimd_host(struct kvm_vcpu *vcpu)
> > } else {
> > __fpsimd_save_state(*host_data_ptr(fpsimd_state));
> > }
> > +
> > + if (kvm_has_fpmr(vcpu->kvm)) {
>
> nVHE is faulting for me, apparently on the kvm_has_fpmr() check though I
> ran out of time to actually figure out where exactly it is going wrong.
> I'll have a further poke tomorrow. Backtrace below.
Well, that's actually pretty obvious when you see the crash
(FAR:ffffff880115cd1c spills the beans).
>
> > + u64 fpmr = read_sysreg_s(SYS_FPMR);
> > +
> > + if (unlikely(is_protected_kvm_enabled()))
> > + *host_data_ptr(fpmr) = fpmr;
>
> That looks wrong until you remember what host_data_ptr() does but but
> it's actually fine. host_data_ptr() is looking inside the struct
> kvm_host_data for the CPU rather than referencing the locally defined
> variable fpmr here. I do think it's worth avoiding the name collision
> though, perhaps just avoid the temporary variable?
I'll rename the variable if that avoids people getting their brains in
a twist. Full potential fix below.
Thanks,
M.
diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
index 2466dd231362..c0832ca0285b 100644
--- a/arch/arm64/kvm/hyp/nvhe/switch.c
+++ b/arch/arm64/kvm/hyp/nvhe/switch.c
@@ -199,13 +199,13 @@ static void kvm_hyp_save_fpsimd_host(struct kvm_vcpu *vcpu)
__fpsimd_save_state(*host_data_ptr(fpsimd_state));
}
- if (kvm_has_fpmr(vcpu->kvm)) {
- u64 fpmr = read_sysreg_s(SYS_FPMR);
+ if (kvm_has_fpmr(kern_hyp_va(vcpu->kvm))) {
+ u64 val = read_sysreg_s(SYS_FPMR);
if (unlikely(is_protected_kvm_enabled()))
- *host_data_ptr(fpmr) = fpmr;
+ *host_data_ptr(fpmr) = val;
else
- **host_data_ptr(fpmr_ptr) = fpmr;
+ **host_data_ptr(fpmr_ptr) = val;
}
}
--
Without deviation from the norm, progress is not possible.
