[RFC PATCH 15/16] KVM: PPC: booke: standard PPC floating point support

[Alexander Graf]

On 09.01.2012, at 22:48, Scott Wood wrote:

> On 01/09/2012 11:48 AM, Alexander Graf wrote:
>> 
>> On 21.12.2011, at 02:34, Scott Wood wrote:
>>> +#ifdef CONFIG_PPC_FPU
>>> +	/* Save userspace FPU state in stack */
>>> +	enable_kernel_fp();
>>> +	memcpy(fpr, current->thread.fpr, sizeof(current->thread.fpr));
>>> +	fpscr = current->thread.fpscr.val;
>>> +	fpexc_mode = current->thread.fpexc_mode;
>>> +
>>> +	/* Restore guest FPU state to thread */
>>> +	memcpy(current->thread.fpr, vcpu->arch.fpr, sizeof(vcpu->arch.fpr));
>>> +	current->thread.fpscr.val = vcpu->arch.fpscr;
>>> +
>>> +	/*
>>> +	 * Since we can't trap on MSR_FP in GS-mode, we consider the guest
>>> +	 * as always using the FPU.  Kernel usage of FP (via
>>> +	 * enable_kernel_fp()) in this thread must not occur while
>>> +	 * vcpu->fpu_active is set.
>>> +	 */
>>> +	vcpu->fpu_active = 1;
>>> +
>>> +	kvmppc_load_guest_fp(vcpu);
>>> +#endif
>> 
>> Do you think it's possible to combine this with the book3s_pr code, so we don't duplicate too much here?
> 
> book3s_pr is a bit different in that it can trap when the guest sets
> MSR[FP].

Ah, there's no doorbell? So you always have to swap fpu registers? You still have to enable it manually when preempting in, right? IIRC ppc32 does lazy fpu activation.

> Maybe a few lines could be factored out (the first memcpy, fpscr,
> fpexc_mode).  I'm not sure that it makes sense given the lack of
> isolation between what it's doing and what the rest of the code is doing.

Yeah, looking at the code it does look pretty different. Too bad - I would've hoped to throw the vmx code in as well so we could get vmx/vsx/whatever for free later.

> 
>>> +/*
>>> + * Load up guest vcpu FP state if it's needed.
>>> + * It also set the MSR_FP in thread so that host know
>>> + * we're holding FPU, and then host can help to save
>>> + * guest vcpu FP state if other threads require to use FPU.
>>> + * This simulates an FP unavailable fault.
>>> + *
>>> + * It requires to be called with preemption disabled.
>>> + */
>>> +static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu)
>>> +{
>>> +#ifdef CONFIG_PPC_FPU
>>> +	if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) {
>>> +		load_up_fpu();
>>> +		current->thread.regs->msr |= MSR_FP;
>> 
>> I'm having a hard time to grasp when shared->msr, shadow_msr and regs->msr is used in your code :).
> 
> shadow_msr is the real MSR.
> 
> shared->msr is the guest's view of MSR.
> 
> current->thread.regs->msr is nominally userspace's MSR.  In this case we
> use it to tell host Linux that FP is in use and must be saved on context
> switch.  The actual userspace MSR_FP is known to be clear at this point
> because we called enable_kernel_fp().  It will be clear again when we
> return to userspace because we'll call giveup_fpu().

Ah, this is thread.regs, not vcpu.regs. Sorry, I misread that part. This way it obviously makes a lot more sense.


Alex