Re: [PATCH 3/7] KVM: arm64: Add save/restore support for FPMR

[Marc Zyngier <maz@kernel.org>]

On Mon, 08 Jul 2024 18:53:39 +0100,
Marc Zyngier <maz@kernel.org> wrote:
> 
> On Mon, 08 Jul 2024 18:47:45 +0100,
> Marc Zyngier <maz@kernel.org> wrote:
> > 
> > On Mon, 08 Jul 2024 18:34:36 +0100,
> > Mark Brown <broonie@kernel.org> wrote:
> > > 
> > > [1  <text/plain; us-ascii (7bit)>]
> > > On Mon, Jul 08, 2024 at 04:44:34PM +0100, Marc Zyngier wrote:
> > > > Just like the rest of the FP/SIMD state, FPMR needs to be context
> > > > switched.
> > > 
> > > > The only interesting thing here is that we need to treat the pKVM
> > > > part a bit differently, as the host FP state is never written back
> > > > to the vcpu thread, but instead stored locally and eagerly restored.
> > > 
> > > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > > ---
> > > >  arch/arm64/include/asm/kvm_host.h  | 10 ++++++++++
> > > >  arch/arm64/kvm/fpsimd.c            |  1 +
> > > >  arch/arm64/kvm/hyp/nvhe/hyp-main.c |  4 ++++
> > > >  arch/arm64/kvm/hyp/nvhe/switch.c   | 10 ++++++++++
> > > >  arch/arm64/kvm/hyp/vhe/switch.c    |  4 ++++
> > > >  5 files changed, 29 insertions(+)
> > > 
> > > I'm possibly missing something here but I'm not seeing where we load the
> > > state for the guest, especially in the VHE case.  I would expect to see
> > > a change in kvm_hyp_handle_fpsimd() to load FPMR for guests with the
> > > feature (it needs to be in there to keep in sync with the ownership
> > > tracking for the rest of the FP state, and to avoid loading needlessly
> > > in cases where the guest never touches FP).
> > > 
> > > Saving for the guest was handled in the previous patch.
> > > 
> > > > diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> > > > index 77010b76c150f..a307c1d5ac874 100644
> > > > --- a/arch/arm64/kvm/hyp/vhe/switch.c
> > > > +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> > > > @@ -312,6 +312,10 @@ static bool kvm_hyp_handle_eret(struct kvm_vcpu *vcpu, u64 *exit_code)
> > > >  static void kvm_hyp_save_fpsimd_host(struct kvm_vcpu *vcpu)
> > > >  {
> > > >  	__fpsimd_save_state(*host_data_ptr(fpsimd_state));
> > > > +
> > > > +	if (system_supports_fpmr() &&
> > > > +	    kvm_has_feat(vcpu->kvm, ID_AA64PFR2_EL1, FPMR, IMP))
> > > > +		**host_data_ptr(fpmr_ptr) = read_sysreg_s(SYS_FPMR);
> > > >  }
> > > 
> > > That's only saving the host state, it doesn't load the guest
> > > state.

So after cherry-picking my own fixes and realising that I had left
pKVM in the lurch, this is what I have added to this patch, which
hopefully does the right thing.

diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index f59ccfe11ab9a..52c7dc8446f16 100644
--- a/arch/arm64/kvm/hyp/include/hyp/switch.h
+++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
@@ -404,6 +404,10 @@ static bool kvm_hyp_handle_fpsimd(struct kvm_vcpu *vcpu, u64 *exit_code)
 	else
 		__fpsimd_restore_state(&vcpu->arch.ctxt.fp_regs);
 
+	if (system_supports_fpmr() &&
+	    kvm_has_feat(kern_hyp_va(vcpu->kvm), ID_AA64PFR2_EL1, FPMR, IMP))
+		write_sysreg_s(__vcpu_sys_reg(vcpu, FPMR), SYS_FPMR);
+
 	/* Skip restoring fpexc32 for AArch64 guests */
 	if (!(read_sysreg(hcr_el2) & HCR_RW))
 		write_sysreg(__vcpu_sys_reg(vcpu, FPEXC32_EL2), fpexc32_el2);
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index 6b14a2c13e287..97fc6ae123a03 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -62,6 +62,8 @@ static void fpsimd_sve_flush(void)
 
 static void fpsimd_sve_sync(struct kvm_vcpu *vcpu)
 {
+	bool has_fpmr;
+
 	if (!guest_owns_fp_regs())
 		return;
 
@@ -73,13 +75,17 @@ static void fpsimd_sve_sync(struct kvm_vcpu *vcpu)
 	else
 		__fpsimd_save_state(&vcpu->arch.ctxt.fp_regs);
 
+	has_fpmr = (system_supports_fpmr() &&
+		    kvm_has_feat(kern_hyp_va(vcpu->kvm), ID_AA64PFR2_EL1, FPMR, IMP));
+	if (has_fpmr)
+		__vcpu_sys_reg(vcpu, FPMR) = read_sysreg_s(SYS_FPMR);
+
 	if (system_supports_sve())
 		__hyp_sve_restore_host();
 	else
 		__fpsimd_restore_state(*host_data_ptr(fpsimd_state));
 
-	if (system_supports_fpmr() &&
-	    kvm_has_feat(kern_hyp_va(vcpu->kvm), ID_AA64PFR2_EL1, FPMR, IMP))
+	if (has_fpmr)
 		write_sysreg_s(*host_data_ptr(fpmr), SYS_FPMR);
 
 	*host_data_ptr(fp_owner) = FP_STATE_HOST_OWNED;

I'll repost the series after -rc1.

	M.

-- 
Without deviation from the norm, progress is not possible.