> On Nov 5, 2014, at 14:04, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>
>
>
> On 02/11/2014 10:54, Nadav Amit wrote:
>> When resetting the VCPU, the FPU should be reset as well (e.g., XCR0 state).
>> Call fx_init during reset as well.
>
> Actually it shouldn't be after INIT. XCR0 is not mentioned explicitly
> in Table 9-1 of the SDM (IA-32 Processor States Following Power-up,
> Reset, or INIT), but since MSR_IA32_XSS is not specified, I think XCR0
> should fall under "All other MSRs”.
I should have given a reference, since Intel SDM is a wild place - see section 2.6 “EXTENDED CONTROL REGISTERS (INCLUDING XCR0)” : "After reset, all bits (except bit 0) in XCR0 are cleared to zero, XCR0[0] is set to 1."
>
>> Signed-off-by: Nadav Amit <namit@xxxxxxxxxxxxxxxxx>
>> ---
>> arch/x86/kvm/x86.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 773c17e..9b90ea7 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -7020,6 +7020,9 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu)
>> vcpu->arch.regs_avail = ~0;
>> vcpu->arch.regs_dirty = ~0;
>>
>> + /* should never fail, since fpu_alloc already done */
>> + fx_init(vcpu);
>> +
>> kvm_x86_ops->vcpu_reset(vcpu);
>> }
>>
>>
>
> Even then, I think this patch is not really nice... The call sequence
> leading to kvm_vcpu_reset is:
I agree. I did a lousy job this time… (made a hack, and forgot to unhack it).
>
> kvm_vm_ioctl_create_vcpu
> kvm_arch_vcpu_create
> kvm_vcpu_init
> kvm_arch_vcpu_init
> fx_init (does fpu_alloc)
> kvm_arch_vcpu_setup
> kvm_vcpu_reset
> fx_init (no fpu_alloc)
>
> The FPU state is not needed between kvm_arch_vcpu_init and
> kvm_arch_vcpu_setup. So we could simply move the reset from
> kvm_vcpu_init to kvm_vcpu_reset, like this:
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 904535fe825e..eaa3be26dfdc 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -914,8 +914,6 @@ void kvm_pic_clear_all(struct kvm_pic *pic, int irq_source_id);
>
> void kvm_inject_nmi(struct kvm_vcpu *vcpu);
>
> -int fx_init(struct kvm_vcpu *vcpu);
> -
> void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
> const u8 *new, int bytes);
> int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 773c17ec42dd..a0566efbb77f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6863,26 +6863,10 @@ int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
> return 0;
> }
>
> -int fx_init(struct kvm_vcpu *vcpu)
> +static int fx_init(struct kvm_vcpu *vcpu)
> {
> - int err;
> -
> - err = fpu_alloc(&vcpu->arch.guest_fpu);
> - if (err)
> - return err;
> -
> - fpu_finit(&vcpu->arch.guest_fpu);
> -
> - /*
> - * Ensure guest xcr0 is valid for loading
> - */
> - vcpu->arch.xcr0 = XSTATE_FP;
> -
> - vcpu->arch.cr0 |= X86_CR0_ET;
> -
> - return 0;
> + return fpu_alloc(&vcpu->arch.guest_fpu);
> }
> -EXPORT_SYMBOL_GPL(fx_init);
>
> static void fx_free(struct kvm_vcpu *vcpu)
> {
> @@ -7020,6 +7004,15 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu)
> vcpu->arch.regs_avail = ~0;
> vcpu->arch.regs_dirty = ~0;
>
> + fpu_finit(&vcpu->arch.guest_fpu);
> +
> + /*
> + * Ensure guest xcr0 is valid for loading
> + */
> + vcpu->arch.xcr0 = XSTATE_FP;
> +
> + vcpu->arch.cr0 |= X86_CR0_ET;
> +
> kvm_x86_ops->vcpu_reset(vcpu);
> }
>
>
> However, as said above I'm not applying either patch, at least for now.
Ok. I hope you will apply it in the future, since we need to workaround it in our tests.
( I run some tests that stress the emulator, but this bug causes problems even without this stress ).
Thanks,
Nadav--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
