Re: [RFC PATCH 4/4] arm64/sve: KVM: Basic SVE support

Andrew Jones
2017-11-25T17:45:58Z

On Thu, Nov 23, 2017 at 07:49:29PM +0100, Christoffer Dall wrote:
> On Thu, Nov 23, 2017 at 06:06:34PM +0000, Dave Martin wrote:
> > > > diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> > > > index 12ee62d..d8e8d22 100644
> > > > --- a/arch/arm64/kvm/hyp/entry.S
> > > > +++ b/arch/arm64/kvm/hyp/entry.S
> > > > @@ -162,37 +162,75 @@ ENTRY(__fpsimd_guest_restore)
> > > > stp x2, x3, [sp, #-16]!
> > > > stp x4, lr, [sp, #-16]!
> > > >
> > > > + mrs x0, cptr_el2
> > > > +
> > > > +alternative_if_not ARM64_SVE
> > > > + mov x1, #CPTR_EL2_TFP
> > > > + mov x2, #CPACR_EL1_FPEN
> > > > +alternative_else
> > > > + mov x1, #CPTR_EL2_TFP | CPTR_EL2_TZ
> > > > + mov x2, #CPACR_EL1_FPEN | CPACR_EL1_ZEN
> > > > +alternative_endif
> > > > +
> > > > alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
> > > > - mrs x2, cptr_el2
> > > > - bic x2, x2, #CPTR_EL2_TFP
> > > > - msr cptr_el2, x2
> > > > + bic x0, x0, x1
> > > > alternative_else
> > > > - mrs x2, cpacr_el1
> > > > - orr x2, x2, #CPACR_EL1_FPEN
> > > > - msr cpacr_el1, x2
> > > > + orr x0, x0, x2
> > > > alternative_endif
> > > > +
> > > > + msr cptr_el2, x0
> > > > isb
> > > >
> > > > - mrs x3, tpidr_el2
> > > > + mrs x4, tpidr_el2
> > > >
> > > > - ldr x0, [x3, #VCPU_HOST_CONTEXT]
> > > > + ldr x0, [x4, #VCPU_HOST_CONTEXT]
> > > > kern_hyp_va x0
> > > > add x0, x0, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
> > > > bl __fpsimd_save_state
> > > >
> > > > - add x2, x3, #VCPU_CONTEXT
> > > > + add x2, x4, #VCPU_CONTEXT
> > > > +
> > > > +#ifdef CONFIG_ARM64_SVE
> > > > +alternative_if ARM64_SVE
> > > > + b 2f
> > > > +alternative_else_nop_endif
> > > > +#endif
> > > > +
> > > > add x0, x2, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
> > > > bl __fpsimd_restore_state
> > > >
> > > > +0:
> > > > // Skip restoring fpexc32 for AArch64 guests
> > > > mrs x1, hcr_el2
> > > > tbnz x1, #HCR_RW_SHIFT, 1f
> > > > - ldr x4, [x3, #VCPU_FPEXC32_EL2]
> > > > - msr fpexc32_el2, x4
> > > > + ldr x3, [x4, #VCPU_FPEXC32_EL2]
> > > > + msr fpexc32_el2, x3
> > > > 1:
> > > > ldp x4, lr, [sp], #16
> > > > ldp x2, x3, [sp], #16
> > > > ldp x0, x1, [sp], #16
> > > >
> > > > eret
> > > > +
> > > > +#ifdef CONFIG_ARM64_SVE
> > > > +2:
> > > > +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
> > > > + ldr x0, [x4, #VCPU_ZCR_EL2]
> > > > + ldr x3, [x4, #VCPU_ZCR_EL1]
> > > > + msr_s SYS_ZCR_EL2, x0
> > > > +alternative_else
> > > > + ldr x0, [x4, #VCPU_ZCR_EL1]
> > > > + ldr x3, [x4, #VCPU_ZCR_EL2]
> > > > + msr_s SYS_ZCR_EL12, x0
> > > > +alternative_endif
> > > > +
> > > > + ldr w0, [x4, #VCPU_SVE_FFR_OFFSET]
> > > > + add x0, x4, x0
> > > > + add x1, x2, #VCPU_FPSR - VCPU_CONTEXT
> > > > + mov x2, x3
> > > > + bl __sve_load_state
> > > > +
> > > > + b 0b
> > > > +#endif /* CONFIG_ARM64_SVE */
> > >
> > > I think if we could move all of this out to C code that would be much
> > > nicer. Also, we currently do the short-circuit fault-in-fpsimd state
> > > inside hyp and always save the fpsimd state when returning to the host
> > > kernel, but in my optimization series I change this to leave the state
> > > in hardware until we schedule or return to userspace, so perhaps we can
> > > go all the way back to handle_exit() then without much performance loss,
> > > which may further simplify the SVE support?

Or all the way back to VCPU RUN exiting, if Rik's proposal[*] for x86 FPU
registers makes sense for these registers as well.

[*] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1536992.html

> >
> > This is what I would be aiming for, coupled with awareness of whether
> > the host has any live FPSIMD/SVE state in the first place: for SVE
> > specifically the answer will _usually_ be "no".
> >
> > Ideally, we would give a vcpu its own SVE/FPSIMD context on a par with
> > host tasks and just reuse the fpsimd.c machinery rather than inventing
> > separate machinery for KVM --

That's what I understand Rik is trying to do.

> > the flipside would be that the hyp code
> > may need to mainpulate thread flags etc. -- I didn't want to rush to
> > this destination before learning to walk though.
>
> Fair enough, but then I think the "let's rely on VHE for SVE support"
> comes in, because you can mostly think of hyp code as being the host
> kernel code.
>
> >
> > For the first iteration of KVM SVE support I would rather not do that
> > rafactoring, to reduce the number of things I'm breaking at the same
> > time...
> >
>
> On the other hand, if it makes the SVE support code much simpler, it may
> be easier in the end. It may be worth thinking of a way we can measure
> the two approaches with basic FPSIMD so that we know the consequences of
> refactoring anything.
>
> > >
> > > The hit would obviously be a slightly higher latency on the first fault
> > > in the guest on SVE/fpsimd.
> >
> > For SVE it's probably not the end of the world since that's higher
> > cost to save/restore in the first place.
> >
> > For FPSIMD I've less of a feel for it.
> >
>
> Yeah, me neither. We'd have to measure something.

Thanks,
drew