[PATCH 0/7] KVM: arm64: Add support for FP8

[PATCH 0/7] KVM: arm64: Add support for FP8

[Marc Zyngier]

Although FP8 support was merged in 6.9, the KVM side was dropped, with
no sign of it being picked up again. Given that its absence is getting
in the way of NV upstreaming (HCRX_EL2 needs fleshing out), here's a
small series addressing it.

The support is following the save/restore model established for the
rest of the FP code, with FPMR being tied to it. The sole additions
are the handling of traps in a nested context, and the corresponding
ID registers being made writable. As an extra cleanup, SVCR and FPMR
are moved into the sysreg array.

Patches are on top of kvmarm/next, as this would otherwise majorly
conflict. Note that this is compile-tested only, as I have no access
to FP8 HW or model (and running NV in a model is not something I wish
to entertain ever again).

Also, this is *not* 6.11 material. 6.12 if we're lucky.

Marc Zyngier (7):
  KVM: arm64: Move SVCR into the sysreg array
  KVM: arm64: Move FPMR into the sysreg array
  KVM: arm64: Add save/restore support for FPMR
  KVM: arm64: Honor trap routing for FPMR
  KVM: arm64: Expose ID_AA64FPFR0_EL1 as a writable ID reg
  KVM: arm64: Enable FP8 support when available and configured
  KVM: arm64: Expose ID_AA64PFR2_EL1 to userspace and guests

 arch/arm64/include/asm/kvm_host.h  | 16 ++++++++++--
 arch/arm64/kvm/emulate-nested.c    |  8 ++++++
 arch/arm64/kvm/fpsimd.c            |  5 ++--
 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 +++
 arch/arm64/kvm/sys_regs.c          | 42 +++++++++++++++++++++++++++---
 7 files changed, 82 insertions(+), 7 deletions(-)

-- 
2.39.2

[PATCH 1/7] KVM: arm64: Move SVCR into the sysreg array

[Marc Zyngier]

SVCR is just a system register, and has no purpose being outside
of the sysreg array. If anything, it only makes it more difficult
to eventually support SME one day. If ever.

Move it into the array with its little friends, and associate it
with a visibility predicate.

Although this is dead code, it at least paves the way for the
next set of FP-related extensions.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h |  4 +++-
 arch/arm64/kvm/fpsimd.c           |  2 +-
 arch/arm64/kvm/sys_regs.c         | 11 ++++++++++-
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 25a3b72fbacf2..0b3d7697ca0a7 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -446,6 +446,9 @@ enum vcpu_sysreg {
 	GCR_EL1,	/* Tag Control Register */
 	TFSRE0_EL1,	/* Tag Fault Status Register (EL0) */
 
+	/* FP/SIMD/SVE */
+	SVCR,
+
 	/* 32bit specific registers. */
 	DACR32_EL2,	/* Domain Access Control Register */
 	IFSR32_EL2,	/* Instruction Fault Status Register */
@@ -664,7 +667,6 @@ struct kvm_vcpu_arch {
 	void *sve_state;
 	enum fp_type fp_type;
 	unsigned int sve_max_vl;
-	u64 svcr;
 	u64 fpmr;
 
 	/* Stage 2 paging state used by the hardware on next switch */
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index c53e5b14038dc..e6425414d301f 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -134,7 +134,7 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
 		fp_state.sve_state = vcpu->arch.sve_state;
 		fp_state.sve_vl = vcpu->arch.sve_max_vl;
 		fp_state.sme_state = NULL;
-		fp_state.svcr = &vcpu->arch.svcr;
+		fp_state.svcr = &__vcpu_sys_reg(vcpu, SVCR);
 		fp_state.fpmr = &vcpu->arch.fpmr;
 		fp_state.fp_type = &vcpu->arch.fp_type;
 
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index c90324060436b..2dc6cab43b2f8 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1669,6 +1669,15 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
 	return REG_HIDDEN;
 }
 
+static unsigned int sme_visibility(const struct kvm_vcpu *vcpu,
+				   const struct sys_reg_desc *rd)
+{
+	if (kvm_has_feat(vcpu->kvm, ID_AA64PFR1_EL1, SME, IMP))
+		return 0;
+
+	return REG_HIDDEN;
+}
+
 static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
 					  const struct sys_reg_desc *rd)
 {
@@ -2535,7 +2544,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 			     CTR_EL0_IDC_MASK |
 			     CTR_EL0_DminLine_MASK |
 			     CTR_EL0_IminLine_MASK),
-	{ SYS_DESC(SYS_SVCR), undef_access },
+	{ SYS_DESC(SYS_SVCR), undef_access, reset_val, SVCR, 0, .visibility = sme_visibility  },
 
 	{ PMU_SYS_REG(PMCR_EL0), .access = access_pmcr, .reset = reset_pmcr,
 	  .reg = PMCR_EL0, .get_user = get_pmcr, .set_user = set_pmcr },
-- 
2.39.2

[PATCH 2/7] KVM: arm64: Move FPMR into the sysreg array

[Marc Zyngier]

Just like SVCR, FPMR is currently stored at the wrong location.

Let's move it where it belongs.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h |  2 +-
 arch/arm64/kvm/fpsimd.c           |  2 +-
 arch/arm64/kvm/sys_regs.c         | 10 ++++++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 0b3d7697ca0a7..a14c18e8b173a 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -448,6 +448,7 @@ enum vcpu_sysreg {
 
 	/* FP/SIMD/SVE */
 	SVCR,
+	FPMR,
 
 	/* 32bit specific registers. */
 	DACR32_EL2,	/* Domain Access Control Register */
@@ -667,7 +668,6 @@ struct kvm_vcpu_arch {
 	void *sve_state;
 	enum fp_type fp_type;
 	unsigned int sve_max_vl;
-	u64 fpmr;
 
 	/* Stage 2 paging state used by the hardware on next switch */
 	struct kvm_s2_mmu *hw_mmu;
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index e6425414d301f..4cb8ad5d69a80 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -135,7 +135,7 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
 		fp_state.sve_vl = vcpu->arch.sve_max_vl;
 		fp_state.sme_state = NULL;
 		fp_state.svcr = &__vcpu_sys_reg(vcpu, SVCR);
-		fp_state.fpmr = &vcpu->arch.fpmr;
+		fp_state.fpmr = &__vcpu_sys_reg(vcpu, FPMR);
 		fp_state.fp_type = &vcpu->arch.fp_type;
 
 		if (vcpu_has_sve(vcpu))
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 2dc6cab43b2f8..326262abc2ff4 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1678,6 +1678,15 @@ static unsigned int sme_visibility(const struct kvm_vcpu *vcpu,
 	return REG_HIDDEN;
 }
 
+static unsigned int fp8_visibility(const struct kvm_vcpu *vcpu,
+				   const struct sys_reg_desc *rd)
+{
+	if (kvm_has_feat(vcpu->kvm, ID_AA64PFR2_EL1, FPMR, IMP))
+		return 0;
+
+	return REG_HIDDEN;
+}
+
 static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
 					  const struct sys_reg_desc *rd)
 {
@@ -2545,6 +2554,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 			     CTR_EL0_DminLine_MASK |
 			     CTR_EL0_IminLine_MASK),
 	{ SYS_DESC(SYS_SVCR), undef_access, reset_val, SVCR, 0, .visibility = sme_visibility  },
+	{ SYS_DESC(SYS_FPMR), undef_access, reset_val, FPMR, 0, .visibility = fp8_visibility },
 
 	{ PMU_SYS_REG(PMCR_EL0), .access = access_pmcr, .reset = reset_pmcr,
 	  .reg = PMCR_EL0, .get_user = get_pmcr, .set_user = set_pmcr },
-- 
2.39.2

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

[Marc Zyngier]

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(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index a14c18e8b173a..764d23082eb91 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -599,6 +599,16 @@ struct kvm_host_data {
 		struct cpu_sve_state *sve_state;
 	};
 
+	union {
+		/* HYP VA pointer to the host storage for FPMR */
+		u64	*fpmr_ptr;
+		/*
+		 * Used by pKVM only, as it needs to provide storage
+		 * for the host
+		 */
+		u64	fpmr;
+	};
+
 	/* Ownership of the FP regs */
 	enum {
 		FP_STATE_FREE,
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index 4cb8ad5d69a80..ea5484ce1f3ba 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -63,6 +63,7 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
 	 */
 	*host_data_ptr(fp_owner) = FP_STATE_HOST_OWNED;
 	*host_data_ptr(fpsimd_state) = kern_hyp_va(&current->thread.uw.fpsimd_state);
+	*host_data_ptr(fpmr_ptr) = kern_hyp_va(&current->thread.uw.fpmr);
 
 	vcpu_clear_flag(vcpu, HOST_SVE_ENABLED);
 	if (read_sysreg(cpacr_el1) & CPACR_EL1_ZEN_EL0EN)
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index f43d845f3c4ec..6b14a2c13e287 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -78,6 +78,10 @@ static void fpsimd_sve_sync(struct kvm_vcpu *vcpu)
 	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))
+		write_sysreg_s(*host_data_ptr(fpmr), SYS_FPMR);
+
 	*host_data_ptr(fp_owner) = FP_STATE_HOST_OWNED;
 }
 
diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
index 6af179c6356d6..47d24ecd68fec 100644
--- a/arch/arm64/kvm/hyp/nvhe/switch.c
+++ b/arch/arm64/kvm/hyp/nvhe/switch.c
@@ -198,6 +198,16 @@ static void kvm_hyp_save_fpsimd_host(struct kvm_vcpu *vcpu)
 	} else {
 		__fpsimd_save_state(*host_data_ptr(fpsimd_state));
 	}
+
+	if (system_supports_fpmr() &&
+	    kvm_has_feat(kern_hyp_va(vcpu->kvm), ID_AA64PFR2_EL1, FPMR, IMP)) {
+		u64 fpmr = read_sysreg_s(SYS_FPMR);
+
+		if (unlikely(is_protected_kvm_enabled()))
+			*host_data_ptr(fpmr) = fpmr;
+		else
+			**host_data_ptr(fpmr_ptr) = fpmr;
+	}
 }
 
 static const exit_handler_fn hyp_exit_handlers[] = {
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);
 }
 
 static bool kvm_hyp_handle_tlbi_el2(struct kvm_vcpu *vcpu, u64 *exit_code)
-- 
2.39.2

[PATCH 4/7] KVM: arm64: Honor trap routing for FPMR

[Marc Zyngier]

HCRX_EL2.EnFPM controls the trapping of FPMR (as well as the validity
of any FP8 instruction, but we don't really care about this last part).

Describe the trap bit so that the exception can be reinjected in a
NV guest.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/emulate-nested.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
index 05166eccea0a6..ee280239f14f4 100644
--- a/arch/arm64/kvm/emulate-nested.c
+++ b/arch/arm64/kvm/emulate-nested.c
@@ -83,6 +83,7 @@ enum cgt_group_id {
 	CGT_CPTR_TAM,
 	CGT_CPTR_TCPAC,
 
+	CGT_HCRX_EnFPM,
 	CGT_HCRX_TCR2En,
 
 	/*
@@ -372,6 +373,12 @@ static const struct trap_bits coarse_trap_bits[] = {
 		.mask		= CPTR_EL2_TCPAC,
 		.behaviour	= BEHAVE_FORWARD_ANY,
 	},
+	[CGT_HCRX_EnFPM] = {
+		.index		= HCRX_EL2,
+		.value 		= 0,
+		.mask		= HCRX_EL2_EnFPM,
+		.behaviour	= BEHAVE_FORWARD_ANY,
+	},
 	[CGT_HCRX_TCR2En] = {
 		.index		= HCRX_EL2,
 		.value 		= 0,
@@ -1108,6 +1115,7 @@ static const struct encoding_to_trap_config encoding_to_cgt[] __initconst = {
 	SR_TRAP(SYS_CNTP_CTL_EL0,	CGT_CNTHCTL_EL1PTEN),
 	SR_TRAP(SYS_CNTPCT_EL0,		CGT_CNTHCTL_EL1PCTEN),
 	SR_TRAP(SYS_CNTPCTSS_EL0,	CGT_CNTHCTL_EL1PCTEN),
+	SR_TRAP(SYS_FPMR,		CGT_HCRX_EnFPM),
 };
 
 static DEFINE_XARRAY(sr_forward_xa);
-- 
2.39.2

[PATCH 5/7] KVM: arm64: Expose ID_AA64FPFR0_EL1 as a writable ID reg

[Marc Zyngier]

ID_AA64FPFR0_EL1 contains all sort of bits that contain a description
of which FP8 subfeatures are implemented.

We don't really care about them, so let's just expose that register
and allow userspace to disable subfeatures at will.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 326262abc2ff4..1157c38568e22 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2386,7 +2386,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
 	ID_HIDDEN(ID_AA64SMFR0_EL1),
 	ID_UNALLOCATED(4,6),
-	ID_UNALLOCATED(4,7),
+	ID_WRITABLE(ID_AA64FPFR0_EL1, ~ID_AA64FPFR0_EL1_RES0),
 
 	/* CRm=5 */
 	{ SYS_DESC(SYS_ID_AA64DFR0_EL1),
-- 
2.39.2

[PATCH 6/7] KVM: arm64: Enable FP8 support when available and configured

[Marc Zyngier]

If userspace has enabled FP8 support (by setting ID_AA64PFR2_EL1.FPMR
to 1), let's enable the feature by setting HCRX_EL2.EnFPM for the vcpu.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 1157c38568e22..8b5caad651512 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -4579,6 +4579,9 @@ void kvm_calculate_traps(struct kvm_vcpu *vcpu)
 
 		if (kvm_has_feat(kvm, ID_AA64MMFR3_EL1, TCRX, IMP))
 			vcpu->arch.hcrx_el2 |= HCRX_EL2_TCR2En;
+
+		if (kvm_has_feat(kvm, ID_AA64PFR2_EL1, FPMR, IMP))
+			vcpu->arch.hcrx_el2 |= HCRX_EL2_EnFPM;
 	}
 
 	if (test_bit(KVM_ARCH_FLAG_FGU_INITIALIZED, &kvm->arch.flags))
-- 
2.39.2

[PATCH 7/7] KVM: arm64: Expose ID_AA64PFR2_EL1 to userspace and guests

[Marc Zyngier]

Everything is now in place for a guest to "enjoy" FP8 support.
Expose ID_AA64PFR2_EL1 to both userspace and guests, with the
explicit restriction of only being able to clear FPMR.

All other features (MTE* at the time of writing) are hidden
and not writable.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 8b5caad651512..e6f9e380283ea 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1722,6 +1722,15 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
 	return val;
 }
 
+static u64 read_sanitised_id_aa64pfr2_el1(struct kvm_vcpu *vcpu,
+					  const struct sys_reg_desc *rd)
+{
+	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64PFR2_EL1);
+
+	/* We only expose FPMR */
+	return val & ID_AA64PFR2_EL1_FPMR;
+}
+
 #define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit)			       \
 ({									       \
 	u64 __f_val = FIELD_GET(reg##_##field##_MASK, val);		       \
@@ -2381,7 +2390,12 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 		   ID_AA64PFR0_EL1_AdvSIMD |
 		   ID_AA64PFR0_EL1_FP), },
 	ID_SANITISED(ID_AA64PFR1_EL1),
-	ID_UNALLOCATED(4,2),
+	{ SYS_DESC(SYS_ID_AA64PFR2_EL1),
+	  .access	= access_id_reg,
+	  .get_user	= get_id_reg,
+	  .set_user	= set_id_reg,
+	  .reset	= read_sanitised_id_aa64pfr2_el1,
+	  .val		= ID_AA64PFR2_EL1_FPMR, },
 	ID_UNALLOCATED(4,3),
 	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
 	ID_HIDDEN(ID_AA64SMFR0_EL1),
-- 
2.39.2

Re: [PATCH 1/7] KVM: arm64: Move SVCR into the sysreg array

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

On Mon, Jul 08, 2024 at 04:44:32PM +0100, Marc Zyngier wrote:
> SVCR is just a system register, and has no purpose being outside
> of the sysreg array. If anything, it only makes it more difficult
> to eventually support SME one day. If ever.

Right, that's why the SME serie has a patch adding it to sysreg -
it was kept out due to feature detection.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1800 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marc Zyngier]

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.

Ah, I forgot to cherry-pick the fixes. Fsck knows what else I forgot.
Thanks for reminding me.

	M.

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);

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

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

[Marc Zyngier]

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.
> 
> Ah, I forgot to cherry-pick the fixes. Fsck knows what else I forgot.
> Thanks for reminding me.
> 
> 	M.
> 
> 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);
> 

And thinking of it, that's not enough. I'm missing the pKVM angle that
needs to be special cased. Drat.

	M.

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

Re: [PATCH 0/7] KVM: arm64: Add support for FP8

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

On Mon, Jul 08, 2024 at 04:44:31PM +0100, Marc Zyngier wrote:

> Although FP8 support was merged in 6.9, the KVM side was dropped, with
> no sign of it being picked up again. Given that its absence is getting
> in the way of NV upstreaming (HCRX_EL2 needs fleshing out), here's a
> small series addressing it.

Thanks, I've been prioritising SME since this was going to conflict with
it and some of your comments on the prior version sounded like you would
block things on a bigger refectoring of the interface with the host
kernel which definitely needs to wait for after SME (I do want to redo
the whole way the host stores FP data for threads which would be a good
time for such a refactoring).  Hopefully there'll be a version of the
SME patches suitable for ABI review this week.

Other than the issue with restoring FPMR for the guest these look good
to inspection.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marc Zyngier]

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.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]

On Tue, Jul 09, 2024 at 10:06:06AM +0100, Marc Zyngier wrote:

> 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.

This looks good to me from review, unfortunately I've got IT issues
at the moment which mean I can't test anything with a model with FPMR
support.  Whenever those get resolved I'll try to take this for a spin.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 433 bytes --]

On Tue, Jul 09, 2024 at 10:06:06AM +0100, Marc Zyngier wrote:

> 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.

My IT issues were resolved so I've kicked the tires on this and it all
seems to be working, I'll go through again a bit more thorougly when you
repost but I'd be surprised if there were any issues.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]