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
