[RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Dave Martin]

Currently FPEXC32_EL2 is handled specially when context-switching.
This made sense for arm, where FPEXC affects host execution
(including VFP/SIMD register save/restore at Hyp).

However, FPEXC has no architectural effect when running in AArch64.
The only case where an arm64 host may execute in AArch32 is when
running compat user code at EL0: the architecture explicitly
documents FPEXC as having no effect in this case either when the
kernel (EL2 in the VHE case; otherwise EL1) is AArch64.

So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for
this register) as a regular AArch32-only system register and switch
it without any special handling or synchronisation.

This allows some gnarly special-case code to be eliminated from
hyp.  The extra isb() in __activate_traps_fpsimd32() is dropped
too: for the reasons explained above arm64 never required this
anyway.

Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Christoffer Dall <christoffer.dall@arm.com>
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---

I could do with some confirmation from someone that my interpretation of
the architecture is in fact correct here.  The CheckFPAdvSIMDEnabled64()
and CheckAdvSIMDEnabled() pseudocode functions may be a reasonable
starting point (this was my reference where the wordy text is unclear).

If Alexander could try this out, that would be good.

This cleanup applies on top of the following previously pubished fix:
[PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-September/599537.html


 arch/arm64/kvm/hyp/switch.c    | 45 ++----------------------------------------
 arch/arm64/kvm/hyp/sysreg-sr.c |  2 ++
 2 files changed, 4 insertions(+), 43 deletions(-)

diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index ca46153..0450b85 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -43,32 +43,6 @@ static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
 	return !!(vcpu->arch.flags & KVM_ARM64_FP_ENABLED);
 }
 
-/* Save the 32-bit only FPSIMD system register state */
-static void __hyp_text __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu)
-{
-	if (!vcpu_el1_is_32bit(vcpu))
-		return;
-
-	vcpu->arch.ctxt.sys_regs[FPEXC32_EL2] = read_sysreg(fpexc32_el2);
-}
-
-static void __hyp_text __activate_traps_fpsimd32(struct kvm_vcpu *vcpu)
-{
-	/*
-	 * We are about to set CPTR_EL2.TFP to trap all floating point
-	 * register accesses to EL2, however, the ARM ARM clearly states that
-	 * traps are only taken to EL2 if the operation would not otherwise
-	 * trap to EL1.  Therefore, always make sure that for 32-bit guests,
-	 * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
-	 * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to
-	 * it will cause an exception.
-	 */
-	if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) {
-		write_sysreg(1 << 30, fpexc32_el2);
-		isb();
-	}
-}
-
 static void __hyp_text __activate_traps_common(struct kvm_vcpu *vcpu)
 {
 	/* Trap on AArch32 cp15 c15 (impdef sysregs) accesses (EL1 or EL0) */
@@ -98,10 +72,8 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
 	val = read_sysreg(cpacr_el1);
 	val |= CPACR_EL1_TTA;
 	val &= ~CPACR_EL1_ZEN;
-	if (!update_fp_enabled(vcpu)) {
+	if (!update_fp_enabled(vcpu))
 		val &= ~CPACR_EL1_FPEN;
-		__activate_traps_fpsimd32(vcpu);
-	}
 
 	write_sysreg(val, cpacr_el1);
 
@@ -116,10 +88,8 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
 
 	val = CPTR_EL2_DEFAULT;
 	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
-	if (!update_fp_enabled(vcpu)) {
+	if (!update_fp_enabled(vcpu))
 		val |= CPTR_EL2_TFP;
-		__activate_traps_fpsimd32(vcpu);
-	}
 
 	write_sysreg(val, cptr_el2);
 }
@@ -366,11 +336,6 @@ static bool __hyp_text __hyp_switch_fpsimd(struct kvm_vcpu *vcpu)
 
 	__fpsimd_restore_state(&vcpu->arch.ctxt.gp_regs.fp_regs);
 
-	/* Skip restoring fpexc32 for AArch64 guests */
-	if (!(read_sysreg(hcr_el2) & HCR_RW))
-		write_sysreg(vcpu->arch.ctxt.sys_regs[FPEXC32_EL2],
-			     fpexc32_el2);
-
 	vcpu->arch.flags |= KVM_ARM64_FP_ENABLED;
 
 	return true;
@@ -522,9 +487,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 
 	sysreg_restore_host_state_vhe(host_ctxt);
 
-	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED)
-		__fpsimd_save_fpexc32(vcpu);
-
 	__debug_switch_to_host(vcpu);
 
 	return exit_code;
@@ -580,9 +542,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 
 	__sysreg_restore_state_nvhe(host_ctxt);
 
-	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED)
-		__fpsimd_save_fpexc32(vcpu);
-
 	/*
 	 * This must come after restoring the host sysregs, since a non-VHE
 	 * system may enable SPE here and make use of the TTBRs.
diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
index 9ce2239..12994a9 100644
--- a/arch/arm64/kvm/hyp/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/sysreg-sr.c
@@ -195,6 +195,7 @@ void __hyp_text __sysreg32_save_state(struct kvm_vcpu *vcpu)
 
 	sysreg[DACR32_EL2] = read_sysreg(dacr32_el2);
 	sysreg[IFSR32_EL2] = read_sysreg(ifsr32_el2);
+	sysreg[FPEXC32_EL2] = read_sysreg(fpexc32_el2);
 
 	if (has_vhe() || vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)
 		sysreg[DBGVCR32_EL2] = read_sysreg(dbgvcr32_el2);
@@ -217,6 +218,7 @@ void __hyp_text __sysreg32_restore_state(struct kvm_vcpu *vcpu)
 
 	write_sysreg(sysreg[DACR32_EL2], dacr32_el2);
 	write_sysreg(sysreg[IFSR32_EL2], ifsr32_el2);
+	write_sysreg(sysreg[FPEXC32_EL2], fpexc32_el2);
 
 	if (has_vhe() || vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)
 		write_sysreg(sysreg[DBGVCR32_EL2], dbgvcr32_el2);
-- 
2.1.4

[RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Dave Martin]

Currently FPEXC32_EL2 is handled specially when context-switching.
This made sense for arm, where FPEXC affects host execution
(including VFP/SIMD register save/restore at Hyp).

However, FPEXC has no architectural effect when running in AArch64.
The only case where an arm64 host may execute in AArch32 is when
running compat user code at EL0: the architecture explicitly
documents FPEXC as having no effect in this case either when the
kernel (EL2 in the VHE case; otherwise EL1) is AArch64.

So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for
this register) as a regular AArch32-only system register and switch
it without any special handling or synchronisation.

This allows some gnarly special-case code to be eliminated from
hyp.  The extra isb() in __activate_traps_fpsimd32() is dropped
too: for the reasons explained above arm64 never required this
anyway.

Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Christoffer Dall <christoffer.dall@arm.com>
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---

I could do with some confirmation from someone that my interpretation of
the architecture is in fact correct here.  The CheckFPAdvSIMDEnabled64()
and CheckAdvSIMDEnabled() pseudocode functions may be a reasonable
starting point (this was my reference where the wordy text is unclear).

If Alexander could try this out, that would be good.

This cleanup applies on top of the following previously pubished fix:
[PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-September/599537.html


 arch/arm64/kvm/hyp/switch.c    | 45 ++----------------------------------------
 arch/arm64/kvm/hyp/sysreg-sr.c |  2 ++
 2 files changed, 4 insertions(+), 43 deletions(-)

diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index ca46153..0450b85 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -43,32 +43,6 @@ static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
 	return !!(vcpu->arch.flags & KVM_ARM64_FP_ENABLED);
 }
 
-/* Save the 32-bit only FPSIMD system register state */
-static void __hyp_text __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu)
-{
-	if (!vcpu_el1_is_32bit(vcpu))
-		return;
-
-	vcpu->arch.ctxt.sys_regs[FPEXC32_EL2] = read_sysreg(fpexc32_el2);
-}
-
-static void __hyp_text __activate_traps_fpsimd32(struct kvm_vcpu *vcpu)
-{
-	/*
-	 * We are about to set CPTR_EL2.TFP to trap all floating point
-	 * register accesses to EL2, however, the ARM ARM clearly states that
-	 * traps are only taken to EL2 if the operation would not otherwise
-	 * trap to EL1.  Therefore, always make sure that for 32-bit guests,
-	 * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
-	 * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to
-	 * it will cause an exception.
-	 */
-	if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) {
-		write_sysreg(1 << 30, fpexc32_el2);
-		isb();
-	}
-}
-
 static void __hyp_text __activate_traps_common(struct kvm_vcpu *vcpu)
 {
 	/* Trap on AArch32 cp15 c15 (impdef sysregs) accesses (EL1 or EL0) */
@@ -98,10 +72,8 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
 	val = read_sysreg(cpacr_el1);
 	val |= CPACR_EL1_TTA;
 	val &= ~CPACR_EL1_ZEN;
-	if (!update_fp_enabled(vcpu)) {
+	if (!update_fp_enabled(vcpu))
 		val &= ~CPACR_EL1_FPEN;
-		__activate_traps_fpsimd32(vcpu);
-	}
 
 	write_sysreg(val, cpacr_el1);
 
@@ -116,10 +88,8 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
 
 	val = CPTR_EL2_DEFAULT;
 	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
-	if (!update_fp_enabled(vcpu)) {
+	if (!update_fp_enabled(vcpu))
 		val |= CPTR_EL2_TFP;
-		__activate_traps_fpsimd32(vcpu);
-	}
 
 	write_sysreg(val, cptr_el2);
 }
@@ -366,11 +336,6 @@ static bool __hyp_text __hyp_switch_fpsimd(struct kvm_vcpu *vcpu)
 
 	__fpsimd_restore_state(&vcpu->arch.ctxt.gp_regs.fp_regs);
 
-	/* Skip restoring fpexc32 for AArch64 guests */
-	if (!(read_sysreg(hcr_el2) & HCR_RW))
-		write_sysreg(vcpu->arch.ctxt.sys_regs[FPEXC32_EL2],
-			     fpexc32_el2);
-
 	vcpu->arch.flags |= KVM_ARM64_FP_ENABLED;
 
 	return true;
@@ -522,9 +487,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 
 	sysreg_restore_host_state_vhe(host_ctxt);
 
-	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED)
-		__fpsimd_save_fpexc32(vcpu);
-
 	__debug_switch_to_host(vcpu);
 
 	return exit_code;
@@ -580,9 +542,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 
 	__sysreg_restore_state_nvhe(host_ctxt);
 
-	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED)
-		__fpsimd_save_fpexc32(vcpu);
-
 	/*
 	 * This must come after restoring the host sysregs, since a non-VHE
 	 * system may enable SPE here and make use of the TTBRs.
diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
index 9ce2239..12994a9 100644
--- a/arch/arm64/kvm/hyp/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/sysreg-sr.c
@@ -195,6 +195,7 @@ void __hyp_text __sysreg32_save_state(struct kvm_vcpu *vcpu)
 
 	sysreg[DACR32_EL2] = read_sysreg(dacr32_el2);
 	sysreg[IFSR32_EL2] = read_sysreg(ifsr32_el2);
+	sysreg[FPEXC32_EL2] = read_sysreg(fpexc32_el2);
 
 	if (has_vhe() || vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)
 		sysreg[DBGVCR32_EL2] = read_sysreg(dbgvcr32_el2);
@@ -217,6 +218,7 @@ void __hyp_text __sysreg32_restore_state(struct kvm_vcpu *vcpu)
 
 	write_sysreg(sysreg[DACR32_EL2], dacr32_el2);
 	write_sysreg(sysreg[IFSR32_EL2], ifsr32_el2);
+	write_sysreg(sysreg[FPEXC32_EL2], fpexc32_el2);
 
 	if (has_vhe() || vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)
 		write_sysreg(sysreg[DBGVCR32_EL2], dbgvcr32_el2);
-- 
2.1.4

Re: [RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Christoffer Dall]

On Wed, Sep 05, 2018 at 02:19:13PM +0100, Dave Martin wrote:
> Currently FPEXC32_EL2 is handled specially when context-switching.
> This made sense for arm, where FPEXC affects host execution
> (including VFP/SIMD register save/restore at Hyp).
> 
> However, FPEXC has no architectural effect when running in AArch64.
> The only case where an arm64 host may execute in AArch32 is when
> running compat user code at EL0: the architecture explicitly
> documents FPEXC as having no effect in this case either when the
> kernel (EL2 in the VHE case; otherwise EL1) is AArch64.
> 
> So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for
> this register) as a regular AArch32-only system register and switch
> it without any special handling or synchronisation.
> 
> This allows some gnarly special-case code to be eliminated from
> hyp.  The extra isb() in __activate_traps_fpsimd32() is dropped
> too: for the reasons explained above arm64 never required this
> anyway.

This looks like a reasonable cleanup, but I don't understand how this
works.  The reason we had the special casing of FPXEC_EL2 was to ensure
that we trap to EL2 when VFP should be trapped for a 32-bit guest
instead of going to EL1.  How does that work with this patch if we set
CPTR_EL2.TFP and the guest has set FPXEX32_EL2.EN ?

Thanks,

    Christoffer

> 
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Christoffer Dall <christoffer.dall@arm.com>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
> 
> I could do with some confirmation from someone that my interpretation of
> the architecture is in fact correct here.  The CheckFPAdvSIMDEnabled64()
> and CheckAdvSIMDEnabled() pseudocode functions may be a reasonable
> starting point (this was my reference where the wordy text is unclear).
> 
> If Alexander could try this out, that would be good.
> 
> This cleanup applies on top of the following previously pubished fix:
> [PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD
> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-September/599537.html
> 
> 
>  arch/arm64/kvm/hyp/switch.c    | 45 ++----------------------------------------
>  arch/arm64/kvm/hyp/sysreg-sr.c |  2 ++
>  2 files changed, 4 insertions(+), 43 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index ca46153..0450b85 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -43,32 +43,6 @@ static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>  	return !!(vcpu->arch.flags & KVM_ARM64_FP_ENABLED);
>  }
>  
> -/* Save the 32-bit only FPSIMD system register state */
> -static void __hyp_text __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu)
> -{
> -	if (!vcpu_el1_is_32bit(vcpu))
> -		return;
> -
> -	vcpu->arch.ctxt.sys_regs[FPEXC32_EL2] = read_sysreg(fpexc32_el2);
> -}
> -
> -static void __hyp_text __activate_traps_fpsimd32(struct kvm_vcpu *vcpu)
> -{
> -	/*
> -	 * We are about to set CPTR_EL2.TFP to trap all floating point
> -	 * register accesses to EL2, however, the ARM ARM clearly states that
> -	 * traps are only taken to EL2 if the operation would not otherwise
> -	 * trap to EL1.  Therefore, always make sure that for 32-bit guests,
> -	 * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
> -	 * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to
> -	 * it will cause an exception.
> -	 */
> -	if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) {
> -		write_sysreg(1 << 30, fpexc32_el2);
> -		isb();
> -	}
> -}
> -
>  static void __hyp_text __activate_traps_common(struct kvm_vcpu *vcpu)
>  {
>  	/* Trap on AArch32 cp15 c15 (impdef sysregs) accesses (EL1 or EL0) */
> @@ -98,10 +72,8 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
>  	val = read_sysreg(cpacr_el1);
>  	val |= CPACR_EL1_TTA;
>  	val &= ~CPACR_EL1_ZEN;
> -	if (!update_fp_enabled(vcpu)) {
> +	if (!update_fp_enabled(vcpu))
>  		val &= ~CPACR_EL1_FPEN;
> -		__activate_traps_fpsimd32(vcpu);
> -	}
>  
>  	write_sysreg(val, cpacr_el1);
>  
> @@ -116,10 +88,8 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
>  
>  	val = CPTR_EL2_DEFAULT;
>  	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
> -	if (!update_fp_enabled(vcpu)) {
> +	if (!update_fp_enabled(vcpu))
>  		val |= CPTR_EL2_TFP;
> -		__activate_traps_fpsimd32(vcpu);
> -	}
>  
>  	write_sysreg(val, cptr_el2);
>  }
> @@ -366,11 +336,6 @@ static bool __hyp_text __hyp_switch_fpsimd(struct kvm_vcpu *vcpu)
>  
>  	__fpsimd_restore_state(&vcpu->arch.ctxt.gp_regs.fp_regs);
>  
> -	/* Skip restoring fpexc32 for AArch64 guests */
> -	if (!(read_sysreg(hcr_el2) & HCR_RW))
> -		write_sysreg(vcpu->arch.ctxt.sys_regs[FPEXC32_EL2],
> -			     fpexc32_el2);
> -
>  	vcpu->arch.flags |= KVM_ARM64_FP_ENABLED;
>  
>  	return true;
> @@ -522,9 +487,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>  
>  	sysreg_restore_host_state_vhe(host_ctxt);
>  
> -	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED)
> -		__fpsimd_save_fpexc32(vcpu);
> -
>  	__debug_switch_to_host(vcpu);
>  
>  	return exit_code;
> @@ -580,9 +542,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
>  
>  	__sysreg_restore_state_nvhe(host_ctxt);
>  
> -	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED)
> -		__fpsimd_save_fpexc32(vcpu);
> -
>  	/*
>  	 * This must come after restoring the host sysregs, since a non-VHE
>  	 * system may enable SPE here and make use of the TTBRs.
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> index 9ce2239..12994a9 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> @@ -195,6 +195,7 @@ void __hyp_text __sysreg32_save_state(struct kvm_vcpu *vcpu)
>  
>  	sysreg[DACR32_EL2] = read_sysreg(dacr32_el2);
>  	sysreg[IFSR32_EL2] = read_sysreg(ifsr32_el2);
> +	sysreg[FPEXC32_EL2] = read_sysreg(fpexc32_el2);
>  
>  	if (has_vhe() || vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)
>  		sysreg[DBGVCR32_EL2] = read_sysreg(dbgvcr32_el2);
> @@ -217,6 +218,7 @@ void __hyp_text __sysreg32_restore_state(struct kvm_vcpu *vcpu)
>  
>  	write_sysreg(sysreg[DACR32_EL2], dacr32_el2);
>  	write_sysreg(sysreg[IFSR32_EL2], ifsr32_el2);
> +	write_sysreg(sysreg[FPEXC32_EL2], fpexc32_el2);
>  
>  	if (has_vhe() || vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)
>  		write_sysreg(sysreg[DBGVCR32_EL2], dbgvcr32_el2);
> -- 
> 2.1.4
> 

[RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Christoffer Dall]

On Wed, Sep 05, 2018 at 02:19:13PM +0100, Dave Martin wrote:
> Currently FPEXC32_EL2 is handled specially when context-switching.
> This made sense for arm, where FPEXC affects host execution
> (including VFP/SIMD register save/restore at Hyp).
> 
> However, FPEXC has no architectural effect when running in AArch64.
> The only case where an arm64 host may execute in AArch32 is when
> running compat user code at EL0: the architecture explicitly
> documents FPEXC as having no effect in this case either when the
> kernel (EL2 in the VHE case; otherwise EL1) is AArch64.
> 
> So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for
> this register) as a regular AArch32-only system register and switch
> it without any special handling or synchronisation.
> 
> This allows some gnarly special-case code to be eliminated from
> hyp.  The extra isb() in __activate_traps_fpsimd32() is dropped
> too: for the reasons explained above arm64 never required this
> anyway.

This looks like a reasonable cleanup, but I don't understand how this
works.  The reason we had the special casing of FPXEC_EL2 was to ensure
that we trap to EL2 when VFP should be trapped for a 32-bit guest
instead of going to EL1.  How does that work with this patch if we set
CPTR_EL2.TFP and the guest has set FPXEX32_EL2.EN ?

Thanks,

    Christoffer

> 
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Christoffer Dall <christoffer.dall@arm.com>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
> 
> I could do with some confirmation from someone that my interpretation of
> the architecture is in fact correct here.  The CheckFPAdvSIMDEnabled64()
> and CheckAdvSIMDEnabled() pseudocode functions may be a reasonable
> starting point (this was my reference where the wordy text is unclear).
> 
> If Alexander could try this out, that would be good.
> 
> This cleanup applies on top of the following previously pubished fix:
> [PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD
> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-September/599537.html
> 
> 
>  arch/arm64/kvm/hyp/switch.c    | 45 ++----------------------------------------
>  arch/arm64/kvm/hyp/sysreg-sr.c |  2 ++
>  2 files changed, 4 insertions(+), 43 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index ca46153..0450b85 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -43,32 +43,6 @@ static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>  	return !!(vcpu->arch.flags & KVM_ARM64_FP_ENABLED);
>  }
>  
> -/* Save the 32-bit only FPSIMD system register state */
> -static void __hyp_text __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu)
> -{
> -	if (!vcpu_el1_is_32bit(vcpu))
> -		return;
> -
> -	vcpu->arch.ctxt.sys_regs[FPEXC32_EL2] = read_sysreg(fpexc32_el2);
> -}
> -
> -static void __hyp_text __activate_traps_fpsimd32(struct kvm_vcpu *vcpu)
> -{
> -	/*
> -	 * We are about to set CPTR_EL2.TFP to trap all floating point
> -	 * register accesses to EL2, however, the ARM ARM clearly states that
> -	 * traps are only taken to EL2 if the operation would not otherwise
> -	 * trap to EL1.  Therefore, always make sure that for 32-bit guests,
> -	 * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
> -	 * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to
> -	 * it will cause an exception.
> -	 */
> -	if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) {
> -		write_sysreg(1 << 30, fpexc32_el2);
> -		isb();
> -	}
> -}
> -
>  static void __hyp_text __activate_traps_common(struct kvm_vcpu *vcpu)
>  {
>  	/* Trap on AArch32 cp15 c15 (impdef sysregs) accesses (EL1 or EL0) */
> @@ -98,10 +72,8 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
>  	val = read_sysreg(cpacr_el1);
>  	val |= CPACR_EL1_TTA;
>  	val &= ~CPACR_EL1_ZEN;
> -	if (!update_fp_enabled(vcpu)) {
> +	if (!update_fp_enabled(vcpu))
>  		val &= ~CPACR_EL1_FPEN;
> -		__activate_traps_fpsimd32(vcpu);
> -	}
>  
>  	write_sysreg(val, cpacr_el1);
>  
> @@ -116,10 +88,8 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
>  
>  	val = CPTR_EL2_DEFAULT;
>  	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
> -	if (!update_fp_enabled(vcpu)) {
> +	if (!update_fp_enabled(vcpu))
>  		val |= CPTR_EL2_TFP;
> -		__activate_traps_fpsimd32(vcpu);
> -	}
>  
>  	write_sysreg(val, cptr_el2);
>  }
> @@ -366,11 +336,6 @@ static bool __hyp_text __hyp_switch_fpsimd(struct kvm_vcpu *vcpu)
>  
>  	__fpsimd_restore_state(&vcpu->arch.ctxt.gp_regs.fp_regs);
>  
> -	/* Skip restoring fpexc32 for AArch64 guests */
> -	if (!(read_sysreg(hcr_el2) & HCR_RW))
> -		write_sysreg(vcpu->arch.ctxt.sys_regs[FPEXC32_EL2],
> -			     fpexc32_el2);
> -
>  	vcpu->arch.flags |= KVM_ARM64_FP_ENABLED;
>  
>  	return true;
> @@ -522,9 +487,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>  
>  	sysreg_restore_host_state_vhe(host_ctxt);
>  
> -	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED)
> -		__fpsimd_save_fpexc32(vcpu);
> -
>  	__debug_switch_to_host(vcpu);
>  
>  	return exit_code;
> @@ -580,9 +542,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
>  
>  	__sysreg_restore_state_nvhe(host_ctxt);
>  
> -	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED)
> -		__fpsimd_save_fpexc32(vcpu);
> -
>  	/*
>  	 * This must come after restoring the host sysregs, since a non-VHE
>  	 * system may enable SPE here and make use of the TTBRs.
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> index 9ce2239..12994a9 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> @@ -195,6 +195,7 @@ void __hyp_text __sysreg32_save_state(struct kvm_vcpu *vcpu)
>  
>  	sysreg[DACR32_EL2] = read_sysreg(dacr32_el2);
>  	sysreg[IFSR32_EL2] = read_sysreg(ifsr32_el2);
> +	sysreg[FPEXC32_EL2] = read_sysreg(fpexc32_el2);
>  
>  	if (has_vhe() || vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)
>  		sysreg[DBGVCR32_EL2] = read_sysreg(dbgvcr32_el2);
> @@ -217,6 +218,7 @@ void __hyp_text __sysreg32_restore_state(struct kvm_vcpu *vcpu)
>  
>  	write_sysreg(sysreg[DACR32_EL2], dacr32_el2);
>  	write_sysreg(sysreg[IFSR32_EL2], ifsr32_el2);
> +	write_sysreg(sysreg[FPEXC32_EL2], fpexc32_el2);
>  
>  	if (has_vhe() || vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)
>  		write_sysreg(sysreg[DBGVCR32_EL2], dbgvcr32_el2);
> -- 
> 2.1.4
> 

Re: [RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Dave Martin]

On Wed, Sep 05, 2018 at 05:08:38PM +0200, Christoffer Dall wrote:
> On Wed, Sep 05, 2018 at 02:19:13PM +0100, Dave Martin wrote:
> > Currently FPEXC32_EL2 is handled specially when context-switching.
> > This made sense for arm, where FPEXC affects host execution
> > (including VFP/SIMD register save/restore at Hyp).
> > 
> > However, FPEXC has no architectural effect when running in AArch64.
> > The only case where an arm64 host may execute in AArch32 is when
> > running compat user code at EL0: the architecture explicitly
> > documents FPEXC as having no effect in this case either when the
> > kernel (EL2 in the VHE case; otherwise EL1) is AArch64.
> > 
> > So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for
> > this register) as a regular AArch32-only system register and switch
> > it without any special handling or synchronisation.
> > 
> > This allows some gnarly special-case code to be eliminated from
> > hyp.  The extra isb() in __activate_traps_fpsimd32() is dropped
> > too: for the reasons explained above arm64 never required this
> > anyway.
> 
> This looks like a reasonable cleanup, but I don't understand how this
> works.  The reason we had the special casing of FPXEC_EL2 was to ensure
> that we trap to EL2 when VFP should be trapped for a 32-bit guest
> instead of going to EL1.  How does that work with this patch if we set

Actually, I think the problem was to ensure that the guest never sees
a spurious trap to EL1 when from its point of view FPEXC.EN is set.
Such a trap shouldn't architecturally happen, but if we execute the
guest with FPEXC.EN=0 when the guest thinks it should be 1 then a trap
to EL1 can occur irrespective of CPTR_EL2, because CPTR_EL2 only traps
things that wouldn't otherwise trap to EL1.

> CPTR_EL2.TFP and the guest has set FPXEX32_EL2.EN ?

I'm not sure whether that's the question you intended to ask:
with EN set and TFP set, everything gets trapped to EL2, giving
us a chance to switch in the FP regs, which sounds like the
intended behaviour.

I think the full set of relevant possibilities looks like this:

	TFP	EN	FPEXC	fpregs
	0	0	 -	EL1
	0	1	 -	 -
	1	0	EL2	EL1
	1	1	EL2	EL2

Here, "TFP" is CPTR_EL2.TFP (or VHE equivalent) and EN is FPEXC (or
FPEXC32_EL2 depending on your point of view) . EN.

The FPEXC and fpregs columns show what happens when attempting to access
the FPEXC or other fp regs respectively from the guest.  - indicates no
trap taken, otherwise I list the destination EL for the trap.

(assuming I've interpreted the rules correctly of course).

Cheers
---Dave

[RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Dave Martin]

On Wed, Sep 05, 2018 at 05:08:38PM +0200, Christoffer Dall wrote:
> On Wed, Sep 05, 2018 at 02:19:13PM +0100, Dave Martin wrote:
> > Currently FPEXC32_EL2 is handled specially when context-switching.
> > This made sense for arm, where FPEXC affects host execution
> > (including VFP/SIMD register save/restore at Hyp).
> > 
> > However, FPEXC has no architectural effect when running in AArch64.
> > The only case where an arm64 host may execute in AArch32 is when
> > running compat user code at EL0: the architecture explicitly
> > documents FPEXC as having no effect in this case either when the
> > kernel (EL2 in the VHE case; otherwise EL1) is AArch64.
> > 
> > So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for
> > this register) as a regular AArch32-only system register and switch
> > it without any special handling or synchronisation.
> > 
> > This allows some gnarly special-case code to be eliminated from
> > hyp.  The extra isb() in __activate_traps_fpsimd32() is dropped
> > too: for the reasons explained above arm64 never required this
> > anyway.
> 
> This looks like a reasonable cleanup, but I don't understand how this
> works.  The reason we had the special casing of FPXEC_EL2 was to ensure
> that we trap to EL2 when VFP should be trapped for a 32-bit guest
> instead of going to EL1.  How does that work with this patch if we set

Actually, I think the problem was to ensure that the guest never sees
a spurious trap to EL1 when from its point of view FPEXC.EN is set.
Such a trap shouldn't architecturally happen, but if we execute the
guest with FPEXC.EN=0 when the guest thinks it should be 1 then a trap
to EL1 can occur irrespective of CPTR_EL2, because CPTR_EL2 only traps
things that wouldn't otherwise trap to EL1.

> CPTR_EL2.TFP and the guest has set FPXEX32_EL2.EN ?

I'm not sure whether that's the question you intended to ask:
with EN set and TFP set, everything gets trapped to EL2, giving
us a chance to switch in the FP regs, which sounds like the
intended behaviour.

I think the full set of relevant possibilities looks like this:

	TFP	EN	FPEXC	fpregs
	0	0	 -	EL1
	0	1	 -	 -
	1	0	EL2	EL1
	1	1	EL2	EL2

Here, "TFP" is CPTR_EL2.TFP (or VHE equivalent) and EN is FPEXC (or
FPEXC32_EL2 depending on your point of view) . EN.

The FPEXC and fpregs columns show what happens when attempting to access
the FPEXC or other fp regs respectively from the guest.  - indicates no
trap taken, otherwise I list the destination EL for the trap.

(assuming I've interpreted the rules correctly of course).

Cheers
---Dave

Re: [RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Christoffer Dall]

On Thu, Sep 06, 2018 at 01:11:17PM +0100, Dave Martin wrote:
> On Wed, Sep 05, 2018 at 05:08:38PM +0200, Christoffer Dall wrote:
> > On Wed, Sep 05, 2018 at 02:19:13PM +0100, Dave Martin wrote:
> > > Currently FPEXC32_EL2 is handled specially when context-switching.
> > > This made sense for arm, where FPEXC affects host execution
> > > (including VFP/SIMD register save/restore at Hyp).
> > > 
> > > However, FPEXC has no architectural effect when running in AArch64.
> > > The only case where an arm64 host may execute in AArch32 is when
> > > running compat user code at EL0: the architecture explicitly
> > > documents FPEXC as having no effect in this case either when the
> > > kernel (EL2 in the VHE case; otherwise EL1) is AArch64.
> > > 
> > > So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for
> > > this register) as a regular AArch32-only system register and switch
> > > it without any special handling or synchronisation.
> > > 
> > > This allows some gnarly special-case code to be eliminated from
> > > hyp.  The extra isb() in __activate_traps_fpsimd32() is dropped
> > > too: for the reasons explained above arm64 never required this
> > > anyway.
> > 
> > This looks like a reasonable cleanup, but I don't understand how this
> > works.  The reason we had the special casing of FPXEC_EL2 was to ensure
> > that we trap to EL2 when VFP should be trapped for a 32-bit guest
> > instead of going to EL1.  How does that work with this patch if we set
> 
> Actually, I think the problem was to ensure that the guest never sees
> a spurious trap to EL1 when from its point of view FPEXC.EN is set.
> Such a trap shouldn't architecturally happen, but if we execute the
> guest with FPEXC.EN=0 when the guest thinks it should be 1 then a trap
> to EL1 can occur irrespective of CPTR_EL2, because CPTR_EL2 only traps
> things that wouldn't otherwise trap to EL1.
> 

Yes, you're right.  I had this nasty feeling that we originally fixed
a bug where we were saving/restoring FPEXC and yet relying on
CPTR_EL2.TFP to always trap, but I've gone back and looked at the
history, and that code was indeed not saving/restoring FPEXC32_EL2
unless the guest was using VFP.


> > CPTR_EL2.TFP and the guest has set FPXEX32_EL2.EN ?
> 
> I'm not sure whether that's the question you intended to ask:

I obviously meant the guest had *cleared* FPXEX32_EL2.EN, but it's clear
to me now.

Thanks,

    Christoffer

[RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Christoffer Dall]

On Thu, Sep 06, 2018 at 01:11:17PM +0100, Dave Martin wrote:
> On Wed, Sep 05, 2018 at 05:08:38PM +0200, Christoffer Dall wrote:
> > On Wed, Sep 05, 2018 at 02:19:13PM +0100, Dave Martin wrote:
> > > Currently FPEXC32_EL2 is handled specially when context-switching.
> > > This made sense for arm, where FPEXC affects host execution
> > > (including VFP/SIMD register save/restore at Hyp).
> > > 
> > > However, FPEXC has no architectural effect when running in AArch64.
> > > The only case where an arm64 host may execute in AArch32 is when
> > > running compat user code at EL0: the architecture explicitly
> > > documents FPEXC as having no effect in this case either when the
> > > kernel (EL2 in the VHE case; otherwise EL1) is AArch64.
> > > 
> > > So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for
> > > this register) as a regular AArch32-only system register and switch
> > > it without any special handling or synchronisation.
> > > 
> > > This allows some gnarly special-case code to be eliminated from
> > > hyp.  The extra isb() in __activate_traps_fpsimd32() is dropped
> > > too: for the reasons explained above arm64 never required this
> > > anyway.
> > 
> > This looks like a reasonable cleanup, but I don't understand how this
> > works.  The reason we had the special casing of FPXEC_EL2 was to ensure
> > that we trap to EL2 when VFP should be trapped for a 32-bit guest
> > instead of going to EL1.  How does that work with this patch if we set
> 
> Actually, I think the problem was to ensure that the guest never sees
> a spurious trap to EL1 when from its point of view FPEXC.EN is set.
> Such a trap shouldn't architecturally happen, but if we execute the
> guest with FPEXC.EN=0 when the guest thinks it should be 1 then a trap
> to EL1 can occur irrespective of CPTR_EL2, because CPTR_EL2 only traps
> things that wouldn't otherwise trap to EL1.
> 

Yes, you're right.  I had this nasty feeling that we originally fixed
a bug where we were saving/restoring FPEXC and yet relying on
CPTR_EL2.TFP to always trap, but I've gone back and looked at the
history, and that code was indeed not saving/restoring FPEXC32_EL2
unless the guest was using VFP.


> > CPTR_EL2.TFP and the guest has set FPXEX32_EL2.EN ?
> 
> I'm not sure whether that's the question you intended to ask:

I obviously meant the guest had *cleared* FPXEX32_EL2.EN, but it's clear
to me now.

Thanks,

    Christoffer

Re: [RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Christoffer Dall]

On Wed, Sep 05, 2018 at 02:19:13PM +0100, Dave Martin wrote:
> Currently FPEXC32_EL2 is handled specially when context-switching.
> This made sense for arm, where FPEXC affects host execution
> (including VFP/SIMD register save/restore at Hyp).

I think the point was actually to avoid saving/restoring FPEXC32_EL2
when VFP was being trapped to EL2

> 
> However, FPEXC has no architectural effect when running in AArch64.
> The only case where an arm64 host may execute in AArch32 is when
> running compat user code at EL0: the architecture explicitly
> documents FPEXC as having no effect in this case either when the
> kernel (EL2 in the VHE case; otherwise EL1) is AArch64.
> 
> So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for
> this register) as a regular AArch32-only system register and switch
> it without any special handling or synchronisation.
> 
> This allows some gnarly special-case code to be eliminated from
> hyp.  The extra isb() in __activate_traps_fpsimd32() is dropped
> too: for the reasons explained above arm64 never required this
> anyway.
> 
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Christoffer Dall <christoffer.dall@arm.com>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
> 
> I could do with some confirmation from someone that my interpretation of
> the architecture is in fact correct here.  The CheckFPAdvSIMDEnabled64()
> and CheckAdvSIMDEnabled() pseudocode functions may be a reasonable
> starting point (this was my reference where the wordy text is unclear).

Which aspect do you want confirmed here?

> 
> If Alexander could try this out, that would be good.
> 
> This cleanup applies on top of the following previously pubished fix:
> [PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD
> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-September/599537.html
> 

We can queue this for the next merge window.

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>

> 
>  arch/arm64/kvm/hyp/switch.c    | 45 ++----------------------------------------
>  arch/arm64/kvm/hyp/sysreg-sr.c |  2 ++
>  2 files changed, 4 insertions(+), 43 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index ca46153..0450b85 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -43,32 +43,6 @@ static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>  	return !!(vcpu->arch.flags & KVM_ARM64_FP_ENABLED);
>  }
>  
> -/* Save the 32-bit only FPSIMD system register state */
> -static void __hyp_text __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu)
> -{
> -	if (!vcpu_el1_is_32bit(vcpu))
> -		return;
> -
> -	vcpu->arch.ctxt.sys_regs[FPEXC32_EL2] = read_sysreg(fpexc32_el2);
> -}
> -
> -static void __hyp_text __activate_traps_fpsimd32(struct kvm_vcpu *vcpu)
> -{
> -	/*
> -	 * We are about to set CPTR_EL2.TFP to trap all floating point
> -	 * register accesses to EL2, however, the ARM ARM clearly states that
> -	 * traps are only taken to EL2 if the operation would not otherwise
> -	 * trap to EL1.  Therefore, always make sure that for 32-bit guests,
> -	 * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
> -	 * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to
> -	 * it will cause an exception.
> -	 */
> -	if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) {
> -		write_sysreg(1 << 30, fpexc32_el2);
> -		isb();
> -	}
> -}
> -
>  static void __hyp_text __activate_traps_common(struct kvm_vcpu *vcpu)
>  {
>  	/* Trap on AArch32 cp15 c15 (impdef sysregs) accesses (EL1 or EL0) */
> @@ -98,10 +72,8 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
>  	val = read_sysreg(cpacr_el1);
>  	val |= CPACR_EL1_TTA;
>  	val &= ~CPACR_EL1_ZEN;
> -	if (!update_fp_enabled(vcpu)) {
> +	if (!update_fp_enabled(vcpu))
>  		val &= ~CPACR_EL1_FPEN;
> -		__activate_traps_fpsimd32(vcpu);
> -	}
>  
>  	write_sysreg(val, cpacr_el1);
>  
> @@ -116,10 +88,8 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
>  
>  	val = CPTR_EL2_DEFAULT;
>  	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
> -	if (!update_fp_enabled(vcpu)) {
> +	if (!update_fp_enabled(vcpu))
>  		val |= CPTR_EL2_TFP;
> -		__activate_traps_fpsimd32(vcpu);
> -	}
>  
>  	write_sysreg(val, cptr_el2);
>  }
> @@ -366,11 +336,6 @@ static bool __hyp_text __hyp_switch_fpsimd(struct kvm_vcpu *vcpu)
>  
>  	__fpsimd_restore_state(&vcpu->arch.ctxt.gp_regs.fp_regs);
>  
> -	/* Skip restoring fpexc32 for AArch64 guests */
> -	if (!(read_sysreg(hcr_el2) & HCR_RW))
> -		write_sysreg(vcpu->arch.ctxt.sys_regs[FPEXC32_EL2],
> -			     fpexc32_el2);
> -
>  	vcpu->arch.flags |= KVM_ARM64_FP_ENABLED;
>  
>  	return true;
> @@ -522,9 +487,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>  
>  	sysreg_restore_host_state_vhe(host_ctxt);
>  
> -	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED)
> -		__fpsimd_save_fpexc32(vcpu);
> -
>  	__debug_switch_to_host(vcpu);
>  
>  	return exit_code;
> @@ -580,9 +542,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
>  
>  	__sysreg_restore_state_nvhe(host_ctxt);
>  
> -	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED)
> -		__fpsimd_save_fpexc32(vcpu);
> -
>  	/*
>  	 * This must come after restoring the host sysregs, since a non-VHE
>  	 * system may enable SPE here and make use of the TTBRs.
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> index 9ce2239..12994a9 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> @@ -195,6 +195,7 @@ void __hyp_text __sysreg32_save_state(struct kvm_vcpu *vcpu)
>  
>  	sysreg[DACR32_EL2] = read_sysreg(dacr32_el2);
>  	sysreg[IFSR32_EL2] = read_sysreg(ifsr32_el2);
> +	sysreg[FPEXC32_EL2] = read_sysreg(fpexc32_el2);
>  
>  	if (has_vhe() || vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)
>  		sysreg[DBGVCR32_EL2] = read_sysreg(dbgvcr32_el2);
> @@ -217,6 +218,7 @@ void __hyp_text __sysreg32_restore_state(struct kvm_vcpu *vcpu)
>  
>  	write_sysreg(sysreg[DACR32_EL2], dacr32_el2);
>  	write_sysreg(sysreg[IFSR32_EL2], ifsr32_el2);
> +	write_sysreg(sysreg[FPEXC32_EL2], fpexc32_el2);
>  
>  	if (has_vhe() || vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)
>  		write_sysreg(sysreg[DBGVCR32_EL2], dbgvcr32_el2);
> -- 
> 2.1.4
> 

[RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Christoffer Dall]

On Wed, Sep 05, 2018 at 02:19:13PM +0100, Dave Martin wrote:
> Currently FPEXC32_EL2 is handled specially when context-switching.
> This made sense for arm, where FPEXC affects host execution
> (including VFP/SIMD register save/restore at Hyp).

I think the point was actually to avoid saving/restoring FPEXC32_EL2
when VFP was being trapped to EL2

> 
> However, FPEXC has no architectural effect when running in AArch64.
> The only case where an arm64 host may execute in AArch32 is when
> running compat user code at EL0: the architecture explicitly
> documents FPEXC as having no effect in this case either when the
> kernel (EL2 in the VHE case; otherwise EL1) is AArch64.
> 
> So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for
> this register) as a regular AArch32-only system register and switch
> it without any special handling or synchronisation.
> 
> This allows some gnarly special-case code to be eliminated from
> hyp.  The extra isb() in __activate_traps_fpsimd32() is dropped
> too: for the reasons explained above arm64 never required this
> anyway.
> 
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Christoffer Dall <christoffer.dall@arm.com>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
> 
> I could do with some confirmation from someone that my interpretation of
> the architecture is in fact correct here.  The CheckFPAdvSIMDEnabled64()
> and CheckAdvSIMDEnabled() pseudocode functions may be a reasonable
> starting point (this was my reference where the wordy text is unclear).

Which aspect do you want confirmed here?

> 
> If Alexander could try this out, that would be good.
> 
> This cleanup applies on top of the following previously pubished fix:
> [PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD
> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-September/599537.html
> 

We can queue this for the next merge window.

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>

> 
>  arch/arm64/kvm/hyp/switch.c    | 45 ++----------------------------------------
>  arch/arm64/kvm/hyp/sysreg-sr.c |  2 ++
>  2 files changed, 4 insertions(+), 43 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index ca46153..0450b85 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -43,32 +43,6 @@ static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
>  	return !!(vcpu->arch.flags & KVM_ARM64_FP_ENABLED);
>  }
>  
> -/* Save the 32-bit only FPSIMD system register state */
> -static void __hyp_text __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu)
> -{
> -	if (!vcpu_el1_is_32bit(vcpu))
> -		return;
> -
> -	vcpu->arch.ctxt.sys_regs[FPEXC32_EL2] = read_sysreg(fpexc32_el2);
> -}
> -
> -static void __hyp_text __activate_traps_fpsimd32(struct kvm_vcpu *vcpu)
> -{
> -	/*
> -	 * We are about to set CPTR_EL2.TFP to trap all floating point
> -	 * register accesses to EL2, however, the ARM ARM clearly states that
> -	 * traps are only taken to EL2 if the operation would not otherwise
> -	 * trap to EL1.  Therefore, always make sure that for 32-bit guests,
> -	 * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
> -	 * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to
> -	 * it will cause an exception.
> -	 */
> -	if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) {
> -		write_sysreg(1 << 30, fpexc32_el2);
> -		isb();
> -	}
> -}
> -
>  static void __hyp_text __activate_traps_common(struct kvm_vcpu *vcpu)
>  {
>  	/* Trap on AArch32 cp15 c15 (impdef sysregs) accesses (EL1 or EL0) */
> @@ -98,10 +72,8 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
>  	val = read_sysreg(cpacr_el1);
>  	val |= CPACR_EL1_TTA;
>  	val &= ~CPACR_EL1_ZEN;
> -	if (!update_fp_enabled(vcpu)) {
> +	if (!update_fp_enabled(vcpu))
>  		val &= ~CPACR_EL1_FPEN;
> -		__activate_traps_fpsimd32(vcpu);
> -	}
>  
>  	write_sysreg(val, cpacr_el1);
>  
> @@ -116,10 +88,8 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
>  
>  	val = CPTR_EL2_DEFAULT;
>  	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
> -	if (!update_fp_enabled(vcpu)) {
> +	if (!update_fp_enabled(vcpu))
>  		val |= CPTR_EL2_TFP;
> -		__activate_traps_fpsimd32(vcpu);
> -	}
>  
>  	write_sysreg(val, cptr_el2);
>  }
> @@ -366,11 +336,6 @@ static bool __hyp_text __hyp_switch_fpsimd(struct kvm_vcpu *vcpu)
>  
>  	__fpsimd_restore_state(&vcpu->arch.ctxt.gp_regs.fp_regs);
>  
> -	/* Skip restoring fpexc32 for AArch64 guests */
> -	if (!(read_sysreg(hcr_el2) & HCR_RW))
> -		write_sysreg(vcpu->arch.ctxt.sys_regs[FPEXC32_EL2],
> -			     fpexc32_el2);
> -
>  	vcpu->arch.flags |= KVM_ARM64_FP_ENABLED;
>  
>  	return true;
> @@ -522,9 +487,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>  
>  	sysreg_restore_host_state_vhe(host_ctxt);
>  
> -	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED)
> -		__fpsimd_save_fpexc32(vcpu);
> -
>  	__debug_switch_to_host(vcpu);
>  
>  	return exit_code;
> @@ -580,9 +542,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
>  
>  	__sysreg_restore_state_nvhe(host_ctxt);
>  
> -	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED)
> -		__fpsimd_save_fpexc32(vcpu);
> -
>  	/*
>  	 * This must come after restoring the host sysregs, since a non-VHE
>  	 * system may enable SPE here and make use of the TTBRs.
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> index 9ce2239..12994a9 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> @@ -195,6 +195,7 @@ void __hyp_text __sysreg32_save_state(struct kvm_vcpu *vcpu)
>  
>  	sysreg[DACR32_EL2] = read_sysreg(dacr32_el2);
>  	sysreg[IFSR32_EL2] = read_sysreg(ifsr32_el2);
> +	sysreg[FPEXC32_EL2] = read_sysreg(fpexc32_el2);
>  
>  	if (has_vhe() || vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)
>  		sysreg[DBGVCR32_EL2] = read_sysreg(dbgvcr32_el2);
> @@ -217,6 +218,7 @@ void __hyp_text __sysreg32_restore_state(struct kvm_vcpu *vcpu)
>  
>  	write_sysreg(sysreg[DACR32_EL2], dacr32_el2);
>  	write_sysreg(sysreg[IFSR32_EL2], ifsr32_el2);
> +	write_sysreg(sysreg[FPEXC32_EL2], fpexc32_el2);
>  
>  	if (has_vhe() || vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)
>  		write_sysreg(sysreg[DBGVCR32_EL2], dbgvcr32_el2);
> -- 
> 2.1.4
> 

Re: [RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Dave Martin]

On Wed, Sep 12, 2018 at 12:17:04PM +0200, Christoffer Dall wrote:
> On Wed, Sep 05, 2018 at 02:19:13PM +0100, Dave Martin wrote:
> > Currently FPEXC32_EL2 is handled specially when context-switching.
> > This made sense for arm, where FPEXC affects host execution
> > (including VFP/SIMD register save/restore at Hyp).
> 
> I think the point was actually to avoid saving/restoring FPEXC32_EL2
> when VFP was being trapped to EL2

With what purpose in mind?

We saved neither the trap (since the VFP registers needed switching
anyway) nor the FPEXC32_EL2 switch (since we had to do that at guest
entry).

I may have misunderstood something.


I was somewhat guessing likely rationale, so if you can suggest a
way to reword this that reflects the situation better I'd be happy
to change it.

Thinking about it, I don't see why FPEXC32_EL2 can't be handled via
vcpu_load()/_put().  This register doesn't matter for the host, and
I can't see why we would need to switch it when going through hyp.

> > 
> > However, FPEXC has no architectural effect when running in AArch64.
> > The only case where an arm64 host may execute in AArch32 is when
> > running compat user code at EL0: the architecture explicitly
> > documents FPEXC as having no effect in this case either when the
> > kernel (EL2 in the VHE case; otherwise EL1) is AArch64.
> > 
> > So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for
> > this register) as a regular AArch32-only system register and switch
> > it without any special handling or synchronisation.
> > 
> > This allows some gnarly special-case code to be eliminated from
> > hyp.  The extra isb() in __activate_traps_fpsimd32() is dropped
> > too: for the reasons explained above arm64 never required this
> > anyway.
> > 
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > Cc: Christoffer Dall <christoffer.dall@arm.com>
> > Cc: Alexander Graf <agraf@suse.de>
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> > 
> > I could do with some confirmation from someone that my interpretation of
> > the architecture is in fact correct here.  The CheckFPAdvSIMDEnabled64()
> > and CheckAdvSIMDEnabled() pseudocode functions may be a reasonable
> > starting point (this was my reference where the wordy text is unclear).
> 
> Which aspect do you want confirmed here?

I confess to a certain amount of "according to the spirit of the
architecture it obviously ought to work this way" reasoning.

Following the trail of documentation isn't trivial, so it would
be good if someone could check that they can reach the same conclusion
about FPEXC32_EL2 having no effect on the host.


> > If Alexander could try this out, that would be good.
> > 
> > This cleanup applies on top of the following previously pubished fix:
> > [PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2018-September/599537.html
> > 
> 
> We can queue this for the next merge window.
> 
> Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>

Thanks, modulo the question of maybe moving this to the vcpu_load()/
_put() path.

Cheers
---Dave

[RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Dave Martin]

On Wed, Sep 12, 2018 at 12:17:04PM +0200, Christoffer Dall wrote:
> On Wed, Sep 05, 2018 at 02:19:13PM +0100, Dave Martin wrote:
> > Currently FPEXC32_EL2 is handled specially when context-switching.
> > This made sense for arm, where FPEXC affects host execution
> > (including VFP/SIMD register save/restore at Hyp).
> 
> I think the point was actually to avoid saving/restoring FPEXC32_EL2
> when VFP was being trapped to EL2

With what purpose in mind?

We saved neither the trap (since the VFP registers needed switching
anyway) nor the FPEXC32_EL2 switch (since we had to do that at guest
entry).

I may have misunderstood something.


I was somewhat guessing likely rationale, so if you can suggest a
way to reword this that reflects the situation better I'd be happy
to change it.

Thinking about it, I don't see why FPEXC32_EL2 can't be handled via
vcpu_load()/_put().  This register doesn't matter for the host, and
I can't see why we would need to switch it when going through hyp.

> > 
> > However, FPEXC has no architectural effect when running in AArch64.
> > The only case where an arm64 host may execute in AArch32 is when
> > running compat user code at EL0: the architecture explicitly
> > documents FPEXC as having no effect in this case either when the
> > kernel (EL2 in the VHE case; otherwise EL1) is AArch64.
> > 
> > So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for
> > this register) as a regular AArch32-only system register and switch
> > it without any special handling or synchronisation.
> > 
> > This allows some gnarly special-case code to be eliminated from
> > hyp.  The extra isb() in __activate_traps_fpsimd32() is dropped
> > too: for the reasons explained above arm64 never required this
> > anyway.
> > 
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > Cc: Christoffer Dall <christoffer.dall@arm.com>
> > Cc: Alexander Graf <agraf@suse.de>
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> > 
> > I could do with some confirmation from someone that my interpretation of
> > the architecture is in fact correct here.  The CheckFPAdvSIMDEnabled64()
> > and CheckAdvSIMDEnabled() pseudocode functions may be a reasonable
> > starting point (this was my reference where the wordy text is unclear).
> 
> Which aspect do you want confirmed here?

I confess to a certain amount of "according to the spirit of the
architecture it obviously ought to work this way" reasoning.

Following the trail of documentation isn't trivial, so it would
be good if someone could check that they can reach the same conclusion
about FPEXC32_EL2 having no effect on the host.


> > If Alexander could try this out, that would be good.
> > 
> > This cleanup applies on top of the following previously pubished fix:
> > [PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2018-September/599537.html
> > 
> 
> We can queue this for the next merge window.
> 
> Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>

Thanks, modulo the question of maybe moving this to the vcpu_load()/
_put() path.

Cheers
---Dave

Re: [RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Christoffer Dall]

On Wed, Sep 12, 2018 at 12:16:26PM +0100, Dave Martin wrote:
> On Wed, Sep 12, 2018 at 12:17:04PM +0200, Christoffer Dall wrote:
> > On Wed, Sep 05, 2018 at 02:19:13PM +0100, Dave Martin wrote:
> > > Currently FPEXC32_EL2 is handled specially when context-switching.
> > > This made sense for arm, where FPEXC affects host execution
> > > (including VFP/SIMD register save/restore at Hyp).
> > 
> > I think the point was actually to avoid saving/restoring FPEXC32_EL2
> > when VFP was being trapped to EL2
> 
> With what purpose in mind?

I don't think we thought it through very carefully but just considered
FPEXC32_EL2 as part of the VFP state, but as you say, that doesn't
really make sense.

I was just objecting to the statement that this is a bring-over from the
32-bit side, which confused me, and I don't think that's true.

> 
> We saved neither the trap (since the VFP registers needed switching
> anyway) nor the FPEXC32_EL2 switch (since we had to do that at guest
> entry).
> 
> I may have misunderstood something.
> 

No, we just didn't look at it that carefully back then.

> 
> I was somewhat guessing likely rationale, so if you can suggest a
> way to reword this that reflects the situation better I'd be happy
> to change it.

"Christoffer was being stupid..." ;)  It was just the way
it was, I don't think there was a good rationale, but I don't think it
helps to understand the change to provide guesswork either, so just
remove it.

> 
> Thinking about it, I don't see why FPEXC32_EL2 can't be handled via
> vcpu_load()/_put().  This register doesn't matter for the host, and
> I can't see why we would need to switch it when going through hyp.
> 

That's another good point, should be able to do that.

> > > 
> > > However, FPEXC has no architectural effect when running in AArch64.
> > > The only case where an arm64 host may execute in AArch32 is when
> > > running compat user code at EL0: the architecture explicitly
> > > documents FPEXC as having no effect in this case either when the
> > > kernel (EL2 in the VHE case; otherwise EL1) is AArch64.
> > > 
> > > So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for
> > > this register) as a regular AArch32-only system register and switch
> > > it without any special handling or synchronisation.
> > > 
> > > This allows some gnarly special-case code to be eliminated from
> > > hyp.  The extra isb() in __activate_traps_fpsimd32() is dropped
> > > too: for the reasons explained above arm64 never required this
> > > anyway.
> > > 
> > > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > > Cc: Christoffer Dall <christoffer.dall@arm.com>
> > > Cc: Alexander Graf <agraf@suse.de>
> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > ---
> > > 
> > > I could do with some confirmation from someone that my interpretation of
> > > the architecture is in fact correct here.  The CheckFPAdvSIMDEnabled64()
> > > and CheckAdvSIMDEnabled() pseudocode functions may be a reasonable
> > > starting point (this was my reference where the wordy text is unclear).
> > 
> > Which aspect do you want confirmed here?
> 
> I confess to a certain amount of "according to the spirit of the
> architecture it obviously ought to work this way" reasoning.
> 
> Following the trail of documentation isn't trivial, so it would
> be good if someone could check that they can reach the same conclusion
> about FPEXC32_EL2 having no effect on the host.
> 

That was definitely my conclusion as well.

> 
> > > If Alexander could try this out, that would be good.
> > > 
> > > This cleanup applies on top of the following previously pubished fix:
> > > [PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD
> > > http://lists.infradead.org/pipermail/linux-arm-kernel/2018-September/599537.html
> > > 
> > 
> > We can queue this for the next merge window.
> > 
> > Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
> 
> Thanks, modulo the question of maybe moving this to the vcpu_load()/
> _put() path.
> 

Sure, I'll look at the next version as well if you send a new version.


Thanks,

        Christoffer

[RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Christoffer Dall]

On Wed, Sep 12, 2018 at 12:16:26PM +0100, Dave Martin wrote:
> On Wed, Sep 12, 2018 at 12:17:04PM +0200, Christoffer Dall wrote:
> > On Wed, Sep 05, 2018 at 02:19:13PM +0100, Dave Martin wrote:
> > > Currently FPEXC32_EL2 is handled specially when context-switching.
> > > This made sense for arm, where FPEXC affects host execution
> > > (including VFP/SIMD register save/restore at Hyp).
> > 
> > I think the point was actually to avoid saving/restoring FPEXC32_EL2
> > when VFP was being trapped to EL2
> 
> With what purpose in mind?

I don't think we thought it through very carefully but just considered
FPEXC32_EL2 as part of the VFP state, but as you say, that doesn't
really make sense.

I was just objecting to the statement that this is a bring-over from the
32-bit side, which confused me, and I don't think that's true.

> 
> We saved neither the trap (since the VFP registers needed switching
> anyway) nor the FPEXC32_EL2 switch (since we had to do that at guest
> entry).
> 
> I may have misunderstood something.
> 

No, we just didn't look at it that carefully back then.

> 
> I was somewhat guessing likely rationale, so if you can suggest a
> way to reword this that reflects the situation better I'd be happy
> to change it.

"Christoffer was being stupid..." ;)  It was just the way
it was, I don't think there was a good rationale, but I don't think it
helps to understand the change to provide guesswork either, so just
remove it.

> 
> Thinking about it, I don't see why FPEXC32_EL2 can't be handled via
> vcpu_load()/_put().  This register doesn't matter for the host, and
> I can't see why we would need to switch it when going through hyp.
> 

That's another good point, should be able to do that.

> > > 
> > > However, FPEXC has no architectural effect when running in AArch64.
> > > The only case where an arm64 host may execute in AArch32 is when
> > > running compat user code at EL0: the architecture explicitly
> > > documents FPEXC as having no effect in this case either when the
> > > kernel (EL2 in the VHE case; otherwise EL1) is AArch64.
> > > 
> > > So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for
> > > this register) as a regular AArch32-only system register and switch
> > > it without any special handling or synchronisation.
> > > 
> > > This allows some gnarly special-case code to be eliminated from
> > > hyp.  The extra isb() in __activate_traps_fpsimd32() is dropped
> > > too: for the reasons explained above arm64 never required this
> > > anyway.
> > > 
> > > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > > Cc: Christoffer Dall <christoffer.dall@arm.com>
> > > Cc: Alexander Graf <agraf@suse.de>
> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > ---
> > > 
> > > I could do with some confirmation from someone that my interpretation of
> > > the architecture is in fact correct here.  The CheckFPAdvSIMDEnabled64()
> > > and CheckAdvSIMDEnabled() pseudocode functions may be a reasonable
> > > starting point (this was my reference where the wordy text is unclear).
> > 
> > Which aspect do you want confirmed here?
> 
> I confess to a certain amount of "according to the spirit of the
> architecture it obviously ought to work this way" reasoning.
> 
> Following the trail of documentation isn't trivial, so it would
> be good if someone could check that they can reach the same conclusion
> about FPEXC32_EL2 having no effect on the host.
> 

That was definitely my conclusion as well.

> 
> > > If Alexander could try this out, that would be good.
> > > 
> > > This cleanup applies on top of the following previously pubished fix:
> > > [PULL 2/4] arm64: KVM: Only force FPEXC32_EL2.EN if trapping FPSIMD
> > > http://lists.infradead.org/pipermail/linux-arm-kernel/2018-September/599537.html
> > > 
> > 
> > We can queue this for the next merge window.
> > 
> > Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
> 
> Thanks, modulo the question of maybe moving this to the vcpu_load()/
> _put() path.
> 

Sure, I'll look at the next version as well if you send a new version.


Thanks,

        Christoffer

Re: [RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Dave Martin]

On Wed, Sep 12, 2018 at 01:21:57PM +0200, Christoffer Dall wrote:
> On Wed, Sep 12, 2018 at 12:16:26PM +0100, Dave Martin wrote:
> > On Wed, Sep 12, 2018 at 12:17:04PM +0200, Christoffer Dall wrote:

[...]

> > > I think the point was actually to avoid saving/restoring FPEXC32_EL2
> > > when VFP was being trapped to EL2
> > 
> > With what purpose in mind?
> 
> I don't think we thought it through very carefully but just considered
> FPEXC32_EL2 as part of the VFP state, but as you say, that doesn't
> really make sense.
> 
> I was just objecting to the statement that this is a bring-over from the
> 32-bit side, which confused me, and I don't think that's true.

[...]

> > We saved neither the trap (since the VFP registers needed switching
> > anyway) nor the FPEXC32_EL2 switch (since we had to do that at guest
> > entry).
> > 
> > I may have misunderstood something.
> > 
> 
> No, we just didn't look at it that carefully back then.
> 
> > 
> > I was somewhat guessing likely rationale, so if you can suggest a
> > way to reword this that reflects the situation better I'd be happy
> > to change it.
> 
> "Christoffer was being stupid..." ;)  It was just the way
> it was, I don't think there was a good rationale, but I don't think it
> helps to understand the change to provide guesswork either, so just
> remove it.

OK, I'll drop the bogus history lesson from this patch.  It doesn't
really add anything here, especially if it's wrong.

> > Thinking about it, I don't see why FPEXC32_EL2 can't be handled via
> > vcpu_load()/_put().  This register doesn't matter for the host, and
> > I can't see why we would need to switch it when going through hyp.
> > 
> 
> That's another good point, should be able to do that.

[...]

> > I confess to a certain amount of "according to the spirit of the
> > architecture it obviously ought to work this way" reasoning.
> > 
> > Following the trail of documentation isn't trivial, so it would
> > be good if someone could check that they can reach the same conclusion
> > about FPEXC32_EL2 having no effect on the host.
> > 
> 
> That was definitely my conclusion as well.

OK, sounds good.

[...]

> > > We can queue this for the next merge window.
> > > 
> > > Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
> > 
> > Thanks, modulo the question of maybe moving this to the vcpu_load()/
> > _put() path.
> > 
> 
> Sure, I'll look at the next version as well if you send a new version.

Will do.  Can't say exactly when.

Cheers
---Dave

[RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

[Dave Martin]

On Wed, Sep 12, 2018 at 01:21:57PM +0200, Christoffer Dall wrote:
> On Wed, Sep 12, 2018 at 12:16:26PM +0100, Dave Martin wrote:
> > On Wed, Sep 12, 2018 at 12:17:04PM +0200, Christoffer Dall wrote:

[...]

> > > I think the point was actually to avoid saving/restoring FPEXC32_EL2
> > > when VFP was being trapped to EL2
> > 
> > With what purpose in mind?
> 
> I don't think we thought it through very carefully but just considered
> FPEXC32_EL2 as part of the VFP state, but as you say, that doesn't
> really make sense.
> 
> I was just objecting to the statement that this is a bring-over from the
> 32-bit side, which confused me, and I don't think that's true.

[...]

> > We saved neither the trap (since the VFP registers needed switching
> > anyway) nor the FPEXC32_EL2 switch (since we had to do that at guest
> > entry).
> > 
> > I may have misunderstood something.
> > 
> 
> No, we just didn't look at it that carefully back then.
> 
> > 
> > I was somewhat guessing likely rationale, so if you can suggest a
> > way to reword this that reflects the situation better I'd be happy
> > to change it.
> 
> "Christoffer was being stupid..." ;)  It was just the way
> it was, I don't think there was a good rationale, but I don't think it
> helps to understand the change to provide guesswork either, so just
> remove it.

OK, I'll drop the bogus history lesson from this patch.  It doesn't
really add anything here, especially if it's wrong.

> > Thinking about it, I don't see why FPEXC32_EL2 can't be handled via
> > vcpu_load()/_put().  This register doesn't matter for the host, and
> > I can't see why we would need to switch it when going through hyp.
> > 
> 
> That's another good point, should be able to do that.

[...]

> > I confess to a certain amount of "according to the spirit of the
> > architecture it obviously ought to work this way" reasoning.
> > 
> > Following the trail of documentation isn't trivial, so it would
> > be good if someone could check that they can reach the same conclusion
> > about FPEXC32_EL2 having no effect on the host.
> > 
> 
> That was definitely my conclusion as well.

OK, sounds good.

[...]

> > > We can queue this for the next merge window.
> > > 
> > > Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
> > 
> > Thanks, modulo the question of maybe moving this to the vcpu_load()/
> > _put() path.
> > 
> 
> Sure, I'll look at the next version as well if you send a new version.

Will do.  Can't say exactly when.

Cheers
---Dave