[PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness

[Alexander Graf]

On 03.07.2013, at 19:17, Scott Wood wrote:

> On 07/03/2013 11:59:45 AM, Alexander Graf wrote:
>> On 03.07.2013, at 17:41, Caraman Mihai Claudiu-B02008 wrote:
>> >>>>> Increase FPU laziness by calling kvmppc_load_guest_fp() just before
>> >>>>> returning to guest instead of each sched in. Without this improvement
>> >>>>> an interrupt may also claim floting point corrupting guest state.
>> >>>>
>> >>>> Not sure I follow. Could you please describe exactly what's happening?
>> >>>
>> >>> This was already discussed on the list, I will forward you the thread.
>> >>
>> >> The only thing I've seen in that thread was some pathetic theoretical
>> >> case where an interrupt handler would enable fp and clobber state
>> >> carelessly. That's not something I'm worried about.
>> >
>> > Neither me though I don't find it pathetic. Please refer it to Scott.
>> If from Linux's point of view we look like a user space program with active floating point registers, we don't have to worry about this case. Kernel code that would clobber that fp state would clobber random user space's fp state too.
> 
> This patch makes it closer to how it works with a user space program.  Or rather, it reduces the time window when we don't (and can't) act like a normal userspace program -- and ensures that we have interrupts disabled during that window.  An interrupt can't randomly clobber FP state; it has to call enable_kernel_fp() just like KVM does.  enable_kernel_fp() clears the userspace MSR_FP to ensure that the state it saves gets restored before userspace uses it again, but that won't have any effect on guest execution (especially in HV-mode).  Thus kvmppc_load_guest_fp() needs to be atomic with guest entry.  Conceptually it's like taking an automatic FP unavailable trap when we enter the guest, since we can't be lazy in HV-mode.

Yep. Once I understood that point things became clear to me :).

> 
>> >> I really don't see where this patch improves anything tbh. It certainly
>> >> makes the code flow more awkward.
>> >
>> > I was pointing you to this: The idea of FPU/AltiVec laziness that the kernel
>> > is struggling to achieve is to reduce the number of store/restore operations.
>> > Without this improvement we restore the unit each time we are sched it. If an
>> > other process take the ownership of the unit (on SMP it's even worse but don't
>> > bother with this) the kernel store the unit state to qemu task. This can happen
>> > multiple times during handle_exit().
>> >
>> > Do you see it now?
>> Yup. Looks good. The code flow is very hard to follow though - there are a lot of implicit assumptions that don't get documented anywhere. For example the fact that we rely on giveup_fpu() to remove MSR_FP from our thread.
> 
> That's not new to this patch...

Would be nice to fix nevertheless. I'm probably not going to be the last person forgetting how this works.


Alex