On 03.07.2013, at 18:09, Caraman Mihai Claudiu-B02008 wrote: >>> + * Simulate AltiVec unavailable fault to load guest state >>> + * from thread to AltiVec unit. >>> + * It requires to be called with preemption disabled. >>> + */ >>> +static inline void kvmppc_load_guest_altivec(struct kvm_vcpu *vcpu) >>> +{ >>> + if (kvmppc_supports_altivec()) { >>> + if (!(current->thread.regs->msr & MSR_VEC)) { >>> + load_up_altivec(NULL); >>> + current->thread.regs->msr |= MSR_VEC; >> >> Does this ensure that the kernel saves / restores all altivec state on >> task switch? Does it load it again when it schedules us in? Would >> definitely be worth a comment. > > These units are _LAZY_ !!! Only SMP kernel save the state when it schedules > out.
Then how do you ensure that altivec state is still consistent after another altivec user got scheduled? Have I missed a vcpu_load hook anywhere? > >> >>> + } >>> + } >>> +} >>> + >>> +/* >>> * Helper function for "full" MSR writes. No need to call this if only >>> * EE/CE/ME/DE/RI are changing. >>> */ >>> @@ -678,6 +706,12 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, >> struct kvm_vcpu *vcpu) >>> u64 fpr[32]; >>> #endif >>> >>> +#ifdef CONFIG_ALTIVEC >>> + vector128 vr[32]; >>> + vector128 vscr; >>> + int used_vr = 0; >> >> bool > > Why don't you ask first to change the type of used_vr member in > /include/asm/processor.h? Ah, it's a copy from thread. Fair enough. > >> >>> +#endif >>> + >>> if (!vcpu->arch.sane) { >>> kvm_run->exit_reason = KVM_EXIT_INTERNAL_ERROR; >>> return -EINVAL; >>> @@ -716,6 +750,22 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, >> struct kvm_vcpu *vcpu) >>> kvmppc_load_guest_fp(vcpu); >>> #endif >>> >>> +#ifdef CONFIG_ALTIVEC >> >> /* Switch from user space Altivec to guest Altivec state */ >> >>> + if (cpu_has_feature(CPU_FTR_ALTIVEC)) { >> >> Why not use your kvmppc_supports_altivec() helper here? > > Give it a try ... because Linus guarded this members with CONFIG_ALTIVEC :) Huh? You already are in an #ifdef CONFIG_ALTIVEC here. I think it's a good idea to be consistent in helper usage. And the name you gave to the helper (kvmppc_supports_altivec) is actually quite nice and tells us exactly what we're asking for. > >> >>> + /* Save userspace VEC state in stack */ >>> + enable_kernel_altivec(); >> >> Can't you hide this in the load function? We only want to enable kernel >> altivec for a short time while we shuffle the registers around. >> >>> + memcpy(vr, current->thread.vr, sizeof(current->thread.vr)); >> >> vr = current->thread.vr; > > Are you kidding, be more careful with arrays :) Bleks :). > >> >>> + vscr = current->thread.vscr; >>> + used_vr = current->thread.used_vr; >>> + >>> + /* Restore guest VEC state to thread */ >>> + memcpy(current->thread.vr, vcpu->arch.vr, sizeof(vcpu- >>> arch.vr)); >> >> current->thread.vr = vcpu->arch.vr; >> >>> + current->thread.vscr = vcpu->arch.vscr; >>> + >>> + kvmppc_load_guest_altivec(vcpu); >>> + } >> >> Do we need to do this even when the guest doesn't use Altivec? Can't we >> just load it on demand then once we fault? This code path really should >> only be a prefetch enable when MSR_VEC is already set in guest context. > > No we can't, read 6/6. So we have to make sure we're completely unlazy when it comes to a KVM guest. Are we? Alex > >> >>> +#endif >>> + >>> ret = __kvmppc_vcpu_run(kvm_run, vcpu); >>> >>> /* No need for kvm_guest_exit. It's done in handle_exit. >>> @@ -736,6 +786,23 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, >> struct kvm_vcpu *vcpu) >>> current->thread.fpexc_mode = fpexc_mode; >>> #endif >>> >>> +#ifdef CONFIG_ALTIVEC >> >> /* Switch from guest Altivec to user space Altivec state */ >> >>> + if (cpu_has_feature(CPU_FTR_ALTIVEC)) { >>> + /* Save AltiVec state to thread */ >>> + if (current->thread.regs->msr & MSR_VEC) >>> + giveup_altivec(current); >>> + >>> + /* Save guest state */ >>> + memcpy(vcpu->arch.vr, current->thread.vr, sizeof(vcpu- >>> arch.vr)); >>> + vcpu->arch.vscr = current->thread.vscr; >>> + >>> + /* Restore userspace state */ >>> + memcpy(current->thread.vr, vr, sizeof(current->thread.vr)); >>> + current->thread.vscr = vscr; >>> + current->thread.used_vr = used_vr; >>> + } >> >> Same comments here. If the guest never touched Altivec state, don't >> bother restoring it, as it's still good. > > LOL, the mighty guest kernel does whatever he wants with AltiVec and > doesn't inform us. > > -Mike > -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html