Re: [PATCH 2/2] i387: split up <asm/i387.h> into exported andinternal interfaces

[Linus Torvalds]

On Tue, Feb 28, 2012 at 3:21 AM, Avi Kivity <avi@xxxxxxxxxx> wrote:
>
> Can you elaborate on what you don't like in the kvm code (apart from "it
> does virtualiztion")?

It doesn't fit any of the patterns of the x87 save/restore code, and I
don't know what it does.

It does clts on its own, in random places without actually restoring
the FPU state. Why is that ok? I don't know. And I don't think it is,
but I didn't change any of it. Why doesn't that thing corrupt the lazy
state save of some other process, for example?

Doing a "clts()" without restoring the FPU state immediately
afterwards is fundamentally *wrong*. It's crazy. Insane. You can now
use the FPU, but with whatever random state that is in it that caused
TS to be set to begin with.

And if you don't have any FPU state to restore, because you want to
use your own kernel state, you should use the
"kernel_fpu_begin()/end()" things that we have had forever.

Here's an example of the kind of UTTER AND ABSOLUTE SHIT that kvm FPU
state restore is:

  static void emulator_get_fpu(struct x86_emulate_ctxt *ctxt)
  {
        preempt_disable();
        kvm_load_guest_fpu(emul_to_vcpu(ctxt));
        /*
         * CR0.TS may reference the host fpu state, not the guest fpu state,
         * so it may be clear at this point.
         */
        clts();
  }

that whole "comment" says nothing at all. And clearing CR0.TS *after*
loading the FPU state is a f*cking joke, since you need it clear to
load the FPU state to begin with. So as far as I can tell,
kvm_load_guest_fpu() will have cleared the FPU state already, but *it*
did it by:

        unlazy_fpu(current);
        fpu_restore_checking(&vcpu->arch.guest_fpu);

where "unlazy_fpu()" will have *set* TS if it wasn't set before, so
fpu_restore_checking() will now TAKE A FAULT, and in that fault
handler it will clear TS so that it can reload the state we just saved
(yes, really), only to then return to fpu_restore_checking() and
reload yet *another* state.

The code is crap. It's insane. It may work, but if it does, it does so
by pure chance and happenstance. The code is CLEARLY INSANE.

I wasn't going to touch it. It had been written by a
random-code-generator that had strung the various FPU accessor
functions up in random order until it compiled.

                  Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/