Re: [PATCH v4 12/14] KVM: arm64: Support FEAT_FPMR for guests
From: Marc Zyngier
Date: Fri Feb 23 2024 - 06:19:02 EST
On Mon, 22 Jan 2024 16:28:15 +0000,
Mark Brown <broonie@xxxxxxxxxx> wrote:
>
> FEAT_FPMR introduces a new system register FPMR which allows configuration
> of floating point behaviour, currently for FP8 specific features. Allow use
> of this in guests, disabling the trap while guests are running and saving
> and restoring the value along with the rest of the floating point state.
> Since FPMR is stored immediately after the main floating point state we
> share it with the hypervisor by adjusting the size of the shared region.
>
> Access to FPMR is covered by both a register specific trap HCRX_EL2.EnFPM
> and the overall floating point access trap so we just unconditionally
> enable the FPMR specific trap and rely on the floating point access trap to
> detect guest floating point usage.
>
> Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>
> ---
> arch/arm64/include/asm/kvm_arm.h | 2 +-
> arch/arm64/include/asm/kvm_host.h | 3 ++-
> arch/arm64/kvm/emulate-nested.c | 8 ++++++++
> arch/arm64/kvm/fpsimd.c | 2 +-
> arch/arm64/kvm/hyp/include/hyp/switch.h | 7 ++++++-
> arch/arm64/kvm/sys_regs.c | 11 +++++++++++
> 6 files changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index 7f45ce9170bb..b2ddb6165953 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -103,7 +103,7 @@
> #define HCR_HOST_VHE_FLAGS (HCR_RW | HCR_TGE | HCR_E2H)
>
> #define HCRX_GUEST_FLAGS \
> - (HCRX_EL2_SMPME | HCRX_EL2_TCR2En | \
> + (HCRX_EL2_SMPME | HCRX_EL2_TCR2En | HCRX_EL2_EnFPM | \
No. We don't do that anymore. This can only be enabled if the guest
has it advertised via ID_AA64PFR2_EL1.FPMR.
> (cpus_have_final_cap(ARM64_HAS_MOPS) ? (HCRX_EL2_MSCEn | HCRX_EL2_MCE2) : 0))
> #define HCRX_HOST_FLAGS (HCRX_EL2_MSCEn | HCRX_EL2_TCR2En | HCRX_EL2_EnFPM)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index c4fdcc94d733..99c0f8944f04 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -384,6 +384,8 @@ enum vcpu_sysreg {
> APGAKEYLO_EL1,
> APGAKEYHI_EL1,
>
> + FPMR,
> +
> /* Memory Tagging Extension registers */
> RGSR_EL1, /* Random Allocation Tag Seed Register */
> GCR_EL1, /* Tag Control Register */
> @@ -544,7 +546,6 @@ struct kvm_vcpu_arch {
> enum fp_type fp_type;
> unsigned int sve_max_vl;
> u64 svcr;
> - unsigned long fpmr;
>
> /* Stage 2 paging state used by the hardware on next switch */
> struct kvm_s2_mmu *hw_mmu;
> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
> index 431fd429932d..3af5fd0e28dc 100644
> --- a/arch/arm64/kvm/emulate-nested.c
> +++ b/arch/arm64/kvm/emulate-nested.c
> @@ -67,6 +67,8 @@ enum cgt_group_id {
> CGT_HCR_TTLBIS,
> CGT_HCR_TTLBOS,
>
> + CGT_HCRX_EnFPM,
> +
> CGT_MDCR_TPMCR,
> CGT_MDCR_TPM,
> CGT_MDCR_TDE,
> @@ -279,6 +281,11 @@ static const struct trap_bits coarse_trap_bits[] = {
> .mask = HCR_TTLBOS,
> .behaviour = BEHAVE_FORWARD_ANY,
> },
> + [CGT_HCRX_EnFPM] = {
> + .index = HCRX_EL2,
> + .mask = HCRX_EL2_EnFPM,
> + .behaviour = BEHAVE_FORWARD_ANY,
This is obviously incorrect.
> + },
> [CGT_MDCR_TPMCR] = {
> .index = MDCR_EL2,
> .value = MDCR_EL2_TPMCR,
> @@ -478,6 +485,7 @@ static const struct encoding_to_trap_config encoding_to_cgt[] __initconst = {
> SR_TRAP(SYS_AIDR_EL1, CGT_HCR_TID1),
> SR_TRAP(SYS_SMIDR_EL1, CGT_HCR_TID1),
> SR_TRAP(SYS_CTR_EL0, CGT_HCR_TID2),
> + SR_TRAP(SYS_FPMR, CGT_HCRX_EnFPM),
> SR_TRAP(SYS_CCSIDR_EL1, CGT_HCR_TID2_TID4),
> SR_TRAP(SYS_CCSIDR2_EL1, CGT_HCR_TID2_TID4),
> SR_TRAP(SYS_CLIDR_EL1, CGT_HCR_TID2_TID4),
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index 6cf22cd8f020..9e002489c843 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -152,7 +152,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->arch.svcr;
> - fp_state.fpmr = &vcpu->arch.fpmr;
> + fp_state.fpmr = (unsigned long *)&__vcpu_sys_reg(vcpu, FPMR);
> fp_state.fp_type = &vcpu->arch.fp_type;
>
> if (vcpu_has_sve(vcpu))
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 27fcdfd432b9..abf785c473d0 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -370,10 +370,15 @@ static bool kvm_hyp_handle_fpsimd(struct kvm_vcpu *vcpu, u64 *exit_code)
> isb();
>
> /* Write out the host state if it's in the registers */
> - if (vcpu->arch.fp_state == FP_STATE_HOST_OWNED)
> + if (vcpu->arch.fp_state == FP_STATE_HOST_OWNED) {
> __fpsimd_save_state(&(vcpu->arch.host_uw->fpsimd_state));
> + if (cpus_have_final_cap(ARM64_HAS_FPMR))
Same thing as above: this cannot be unconditional.
> + vcpu->arch.host_uw->fpmr = read_sysreg_s(SYS_FPMR);
> + }
>
> /* Restore the guest state */
> + if (cpus_have_final_cap(ARM64_HAS_FPMR))
> + write_sysreg_s(__vcpu_sys_reg(vcpu, FPMR), SYS_FPMR);
> if (sve_guest)
> __hyp_sve_restore_guest(vcpu);
> else
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 38503b1cd2eb..216eac44c124 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2067,6 +2067,15 @@ static unsigned int hidden_user_visibility(const struct kvm_vcpu *vcpu,
> .visibility = hidden_user_visibility, \
> }
>
> +static unsigned int fpmr_visibility(const struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *rd)
> +{
> + if (cpus_have_final_cap(ARM64_HAS_FPMR))
> + return 0;
Same thing.
> +
> + return REG_HIDDEN;
> +}
> +
> /*
> * Since reset() callback and field val are not used for idregs, they will be
> * used for specific purposes for idregs.
> @@ -2463,6 +2472,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> { SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
> { SYS_DESC(SYS_CTR_EL0), access_ctr },
> { SYS_DESC(SYS_SVCR), undef_access },
> + { SYS_DESC(SYS_FPMR), access_rw, reset_unknown, FPMR,
> + .visibility = fpmr_visibility },
>
> { PMU_SYS_REG(PMCR_EL0), .access = access_pmcr, .reset = reset_pmcr,
> .reg = PMCR_EL0, .get_user = get_pmcr, .set_user = set_pmcr },
>
Thanks,
M.
--
Without deviation from the norm, progress is not possible.