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.
