[PATCH 4/6] KVM: PPC: Book3E: Add AltiVec support

[Alexander Graf]

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
>