Re: [PATCH 03/18] KVM: arm64: Drop FP_FOREIGN_STATE from the hypervisor code

[Reiji Watanabe <reijiw@google.com>]

On Sat, Jun 4, 2022 at 1:10 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Fri, 03 Jun 2022 06:23:25 +0100,
> Reiji Watanabe <reijiw@google.com> wrote:
> >
> > Hi Marc,
> >
> > On Sat, May 28, 2022 at 4:38 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > The vcpu KVM_ARM64_FP_FOREIGN_FPSTATE flag tracks the thread's own
> > > TIF_FOREIGN_FPSTATE so that we can evaluate just before running
> > > the vcpu whether it the FP regs contain something that is owned
> > > by the vcpu or not by updating the rest of the FP flags.
> > >
> > > We do this in the hypervisor code in order to make sure we're
> > > in a context where we are not interruptible. But we already
> > > have a hook in the run loop to generate this flag. We may as
> > > well update the FP flags directly and save the pointless flag
> > > tracking.
> > >
> > > Whilst we're at it, rename update_fp_enabled() to guest_owns_fp_regs()
> > > to indicate what the leftover of this helper actually do.
> > >
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> >
> > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> >
> >
> > > --- a/arch/arm64/kvm/fpsimd.c
> > > +++ b/arch/arm64/kvm/fpsimd.c
> > > @@ -107,16 +107,19 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
> > >  }
> > >
> > >  /*
> > > - * Called just before entering the guest once we are no longer
> > > - * preemptable. Syncs the host's TIF_FOREIGN_FPSTATE with the KVM
> > > - * mirror of the flag used by the hypervisor.
> > > + * Called just before entering the guest once we are no longer preemptable
> > > + * and interrupts are disabled. If we have managed to run anything using
> > > + * FP while we were preemptible (such as off the back of an interrupt),
> > > + * then neither the host nor the guest own the FP hardware (and it was the
> > > + * responsibility of the code that used FP to save the existing state).
> > > + *
> > > + * Note that not supporting FP is basically the same thing as far as the
> > > + * hypervisor is concerned (nothing to save).
> > >   */
> > >  void kvm_arch_vcpu_ctxflush_fp(struct kvm_vcpu *vcpu)
> > >  {
> > > -       if (test_thread_flag(TIF_FOREIGN_FPSTATE))
> > > -               vcpu->arch.flags |= KVM_ARM64_FP_FOREIGN_FPSTATE;
> > > -       else
> > > -               vcpu->arch.flags &= ~KVM_ARM64_FP_FOREIGN_FPSTATE;
> > > +       if (!system_supports_fpsimd() || test_thread_flag(TIF_FOREIGN_FPSTATE))
> > > +               vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED | KVM_ARM64_FP_HOST);
> > >  }
> >
> > Although kvm_arch_vcpu_load_fp() unconditionally sets KVM_ARM64_FP_HOST,
> > perhaps having kvm_arch_vcpu_load_fp() set KVM_ARM64_FP_HOST only when
> > FP is supported might be more consistent?
> > Then, checking system_supports_fpsimd() is unnecessary here.
> > (KVM_ARM64_FP_ENABLED is not set when FP is not supported)
>
> That's indeed a possibility. But I'm trying not to change the logic
> here, only to move it to a place that provides the same semantic
> without the need for an extra flag.
>
> I'm happy to stack an extra patch on top of this series though.

Thank you for your reply. I would prefer that.

Thanks,
Reiji



>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel