On Wed, May 09, 2018 at 10:46:11AM +0100, Marc Zyngier wrote:
> On 09/05/18 10:17, Dave Martin wrote:
> > On Wed, May 09, 2018 at 09:24:36AM +0100, Marc Zyngier wrote:
> >> On 08/05/18 17:44, Dave Martin wrote:
> >>> This patch refactors KVM to align the host and guest FPSIMD
> >>> save/restore logic with each other for arm64. This reduces the
> >
> > [...]
> >
> >>> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> >>> index 9699c13..6429eb6 100644
> >>> --- a/arch/arm64/include/asm/kvm_asm.h
> >>> +++ b/arch/arm64/include/asm/kvm_asm.h
> >>> @@ -33,6 +33,8 @@
> >>> /* vcpu_arch flags field values: */
> >>> #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0
> >>> #define KVM_ARM64_DEBUG_DIRTY (1 << KVM_ARM64_DEBUG_DIRTY_SHIFT)
> >>
> >> At some point, we could remove KVM_ARM64_DEBUG_DIRTY_SHIFT as it is not
> >> used anymore (since 1ea66d27e7b0). Do not respin on this account though.
> >
> > I wondered about this... I presume this was used from assembly once
> > upon a time.
> >
> > Since I need to repost for something else anyway, shall I delete that
> > and move the defines to kvm_host.h? There's no longer an obvious reason
> > for them to be in a different header.
>
> Yes, please.
OK, will do.
> >>> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
[...]
> >>> +void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
> >>> +{
> >>> + BUG_ON(system_supports_sve());
> >>> + BUG_ON(!current->mm);
> >>> +
> >>> + vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED | KVM_ARM64_HOST_SVE_IN_USE);
> >>> + if (test_thread_flag(TIF_SVE))
> >>> + vcpu->arch.flags |= KVM_ARM64_HOST_SVE_IN_USE;
> >>> +
> >>> + vcpu->arch.host_fpsimd_state =
> >>> + kern_hyp_va(¤t->thread.uw.fpsimd_state);
> >>
> >> ... what is the purpose of this assignment? I don't think it is harmful
> >> in any way, but I'm just wondering if I'm missing in the actual flow.
> >
> > The pointer will be NULLed by __hyp_switch_fpsimd() when loading the
> > guest state in, to indicate that there is no longer any host state to
> > save. Perhaps it would have been clearer to have a separate flag to
> > represent this change of state.
> >
> > As things stand I think you're right: the assignments are redundant.
> > We certainly need the pointer to reflect the current task, which is
> > the motivation for doing it here -- but in fact we need to do it
> > more often, at every vcpu_load().
> >
> > I think the most appropriate thing is to delete the assignment from
> > _map_fp().
> >
> > The alternative would be to add a flag for the "no host state to save"
> > case, and fpsimd_host_state only in the pid_change hook.
>
> I think I'd prefer the latter. It makes the whole pointer stuff a lot
> easier to understand (the linkages are basically static), and using
> flags is consistent with the way the rest of the kernel works. I'm not
> sure whether we need to add a new flag or simply rely on the existing
> thread flag, but that for you to decide.
OK, agreed. It should definitely help clarity.
[...]
> > Happy to keep your Reviewed-by with the discussed changes?
> Yes, no problem. I'll eyeball the changes and let you know if I spot
> anything.
OK, thanks.
I'll note the changes under the tearoff.
Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
