Re: [PATCH v2 2/8] KVM: arm64: Remove host FPSIMD saving for non-protected KVM

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Feb 10, 2025 at 05:00:04PM +0000, Mark Rutland wrote:
> | static inline bool kvm_hyp_handle_fpsimd(struct kvm_vcpu *vcpu, u64 *exit_code)
> | {
> | 	...
> | 
> | 	/* Valid trap */
> | 
> | 	/*
> | 	 * Enable everything EL2 might need to save/restore state.
> | 	 * Maybe each of the bits should depend on system_has_xxx()
> | 	 */
> | 	cpacr_clear_set(0, CPACR_EL1_FPEN | CPACR_EL1_ZEN | CPACR_EL1_SMEN */
> | 	isb();
> | 
> | 	...
> | 
> | 	/* Write out the host state if it's in the registers */
> | 	if (is_protected_kvm_enabled() && host_owns_fp_regs())
> | 		kvm_hyp_save_fpsimd_host(vcpu);
> | 	
> | 	/* Restore guest state */
> | 
> | 	...
> | 
> | 	/*
> | 	 * Enable traps for the VCPU. The ERET will cause the traps to
> | 	 * take effect in the guest, so no ISB is necessary.
> | 	 */
> | 	cpacr_guest = CPACR_EL1_FPEN;
> | 	if (vcpu_has_sve(vcpu))
> | 		cpacr_guest |= CPACR_EL1_ZEN;
> | 	if (vcpu_has_sme(vcpu))			// whenever we add this
> | 		cpacr_guest |= CPACR_EL1_SMEN;
> | 	cpacr_clear_set(CPACR_EL1_FPEN | CPACR_EL1_ZEN | CPACR_EL1_SMEN,
> | 			cpacr_guest);
> | 
> | 	return true;
> | }
> 
> ... where we'd still have the CPACR write to re-enable traps, but it'd
> be unconditional, and wouldn't need an extra ISB.

I had a quick go at this, and there are a few things that I spotted that
got in the way, so I'm not going to spin that immediately, but I'd be
happy to in a short while. Notes below:

(1) I looked at using __activate_cptr_traps() and
    __deactivate_cptr_traps() rather than poking CPACR/CPTR directly,
    to ensure that this is kept in-sync with the regular guest<->host
    transitions, but that requires a bit of refactoring (e.g. moving
    those *back* into the common header), and potentially requires doing
    a bit of redundant work here, so I'm not sure whether that's
    actually preferable or not.

    If you have a strong preference either way as to doing that or
    poking CPACR/CPTR directly, knowing that would be helfpul.

(2) The cpacr_clear_set() function doesn't behave the same as
    sysreg_clear_set(), and doesn't handle the case where a field is in
    both the 'clr' and 'set' masks as is the case above. For example, if
    one writes the following to clear THEN set the FPEN field, disabling
    traps:

    | cpacr_clear_set(CPACR_EL1_FPEN, CPACR_EL1_FPEN);

    ... the VHE code looks good:

    | mrs     x0, cpacr_el1
    | orr     x1, x0, #0x300000		// set both FPEN bits
    | cmp     x0, x1
    | b.eq    <<skip_write>>
    | msr     cpacr_el1, x1

    ... but the nVHE code does the opposite:

    | mrs     x0, cptr_el2
    | orr     x1, x0, #0x400		// set TFP
    | tbnz    w0, #10, <<skip_write>>
    | msr     cptr_el2, x1

    Luckily this does the right thing for all existing users, but that'd
    need to be cleaned up.

(3) We should be able to get rid of the ISB when writing to FPEXC32_EL2.
    That register has no effect while in AArch64 state, and the ERET
    will synchronize it before AArch32 guest code can be executed.

    That should probably be factored out into save/restore functions
    that are used consistently.

Mark.