Re: [PATCH v4 03/14] arm64/fpsimd: Support FEAT_FPMR

[Mark Brown]

On Fri, Feb 23, 2024 at 11:07:02AM +0000, Marc Zyngier wrote:
> Mark Brown <broonie@xxxxxxxxxx> wrote:

> > --- a/arch/arm64/kvm/fpsimd.c
> > +++ b/arch/arm64/kvm/fpsimd.c
> > @@ -153,6 +153,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.fp_type = &vcpu->arch.fp_type;

> Given the number of fields you keep track of, it would make a lot more
> sense if these FP-related fields were in their own little structure
> and tracked by a single pointer (I don't think there is a case where
> we track them independently).

I sent a patch series that attempted to address this somewhat by
refactoring things so that we initialise the struct we bind during vCPU
setup rather than each time we prepare to switch away from the guest:

    https://lore.kernel.org/linux-arm-kernel/20240226-kvm-arm64-group-fp-data-v1-0-07d13759517e@xxxxxxxxxx/

which you weren't terribly enthusiastic about:

    https://lore.kernel.org/all/86y1b027mc.wl-maz@xxxxxxxxxx/

As covered in the changelog for patch 2 of that series the FP state
includes both system registers which KVM wants to expose via it's system
register ABI and FPSIMD state which the host kernel wants to flag as
suitable for hardended usercopy by embedding in thread_struct.uw.  This
means that pulling everything into a single structure which is used
throughout the kernel would require a bunch of surgery which I'm not
sure is worth the effort or ongoing maintainance cost. 

The current approach is verbose but not complicated and localised to the
code managing the FP state, as I said in the discussion on that thread I
think it represents a reasonable tradeoff.

Attachment: signature.asc
Description: PGP signature