Re: [PATCH 2/4] Fix FPU interface changes

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 4320 bytes --]

Zachary Amsden wrote:
> On 05/25/2010 09:33 PM, Jan Kiszka wrote:
>> Zachary Amsden wrote:
>>   
>>> The FPU interface changed to dynamic allocation; redefine the missing
>>> struct fpu to be a static structure and add back the old macros.
>>>
>>> The FPU interface changed to dynamic allocation; redefine the missing
>>> struct fpu to be a static structure and add back the old macros.
>>>
>>>
>>>
>>>  From 95be2b1ac41a40f2f3d581f0674b25ca4b70a025 Mon Sep 17 00:00:00 2001
>>> From: Zachary Amsden<zamsden@redhat.com>
>>> Date: Tue, 25 May 2010 16:32:14 -1000
>>> Subject: [PATCH 2/5] Extend module compatability to FPU code
>>>
>>> Signed-off-by: Zachary Amsden<zamsden@redhat.com>
>>> ---
>>>   x86/external-module-compat.h |   84
>>> ++++++++++++++++++++++++++++++++++++++++++
>>>   1 files changed, 84 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/x86/external-module-compat.h b/x86/external-module-compat.h
>>> index 09bf232..316f7ff 100644
>>> --- a/x86/external-module-compat.h
>>> +++ b/x86/external-module-compat.h
>>> @@ -788,3 +788,87 @@ struct kvm_pvclock_vcpu_time_info {
>>>   #else
>>>   #define kvm_pvclock_vcpu_time_info    pvclock_vcpu_time_info
>>>   #endif
>>> +
>>> +#if LINUX_VERSION_CODE<  KERNEL_VERSION(2,6,34)
>>>      
>> The FPU changes look like 2.6.36 material.
>>    
> 
> It's quite likely I got the version wrong.
> 
> 
>>> +#include<asm/processor.h>
>>> +#include<asm/i387.h>
>>> +struct _fpu {
>>> +    struct i387_fxsave_struct fxsave;
>>> +    struct i387_fxsave_struct host_fx_image;
>>>      
>> The original code before 9144b51caf only used a single state - for the
>> guest.
>>
>>   
>>> +};
>>> +
>>> +struct fpu {
>>> +    struct _fpu *state;
>>> +    struct _fpu state_static;
>>> +};
>>> +
>>> +static inline bool fpu_allocated(struct fpu *fpu)
>>> +{
>>> +    fpu->state =&fpu->state_static;
>>> +        return true;
>>> +}
>>>      
>> Unused.
>>
>>   
>>> +
>>> +static inline int fpu_alloc(struct fpu *fpu)
>>> +{
>>> +    fpu->state =&fpu->state_static;
>>> +    return 0;
>>> +}
>>>      
>> That's smarter than the dumb copy of upstream fpu_alloc that I hacked up.
>>
>>   
>>> +
>>> +static inline void fpu_free(struct fpu *fpu)
>>> +{
>>> +}
>>> +
>>> +static inline void kvm_fx_finit(void)
>>> +{
>>> +        asm("finit");
>>> +}
>>> +
>>> +static inline void kvm_fx_save(struct i387_fxsave_struct *image)
>>> +{
>>> +        asm("fxsave (%0)":: "r" (image));
>>> +}
>>> +
>>> +static inline void kvm_fx_restore(struct i387_fxsave_struct *image)
>>> +{
>>> +        asm("fxrstor (%0)":: "r" (image));
>>> +}
>>> +
>>> +static inline void fpu_finit(struct fpu *fpu)
>>> +{
>>> +    unsigned after_mxcsr_mask;
>>> +
>>> +    /*
>>> +       * Touch the fpu the first time in non atomic context as if
>>> +        * this is the first fpu instruction the exception handler
>>> +        * will fire before the instruction returns and it'll have to
>>> +       * allocate ram with GFP_KERNEL.
>>> +       */
>>> +    if (!used_math())
>>> +        kvm_fx_save(&fpu->state->host_fx_image);
>>> +
>>> +    /* Initialize guest FPU by resetting ours and saving into
>>> guest's */
>>> +    preempt_disable();
>>> +    kvm_fx_save(&fpu->state->host_fx_image);
>>> +    kvm_fx_finit();
>>> +    kvm_fx_save(&fpu->state->fxsave);
>>> +    kvm_fx_restore(&fpu->state->host_fx_image);
>>> +    preempt_enable();
>>>      
>> The original KVM code before 9144b51caf just did finit+fxsave. Why this
>> saving/restoring of the host state?
>>    
> 
> It looks like the new FPU contract allows one to "allocate" an FPU
> context for the thread.  Older kernels might have some other thread's
> FPU state lazily lying around?  It wasn't clear, I didn't care to check
> all kernel, and this code is verbatim from 2.6.33 asm/i387.h

I think it's safe to assume that previous KVM code, including that which
is delivered with 2.6.33 or .34, is sufficient for our purpose. So I
basically implanted it into the wrappers you can find in my next branch.
Builds are fine for all versions (I'm now just waiting for the final
warnings fix being merged). And it works fine for me, but I only tested
with a 2.6.27 host kernel. Please give it a try.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]