Re: [PATCH v2 4/8] KVM: arm64: Add save/restore support for FPMR

[Marc Zyngier <maz@kernel.org>]

On Thu, 01 Aug 2024 20:07:16 +0100,
Mark Brown <broonie@kernel.org> 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.