Re: [Qemu-devel] [PATCH 3/4] KVM/X86: Enable Intel MPX for guest

[Liu, Jinsong]

Paolo Bonzini wrote:
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index a8ce117..e30d4ce 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -75,7 +75,7 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
>>                      (best->eax | ((u64)best->edx << 32)) &
>>                      host_xcr0 & KVM_SUPPORTED_XCR0;
>>              vcpu->arch.guest_xstate_size = best->ebx =
>> -                    xstate_required_size(vcpu->arch.guest_supported_xcr0);
>> +                    xstate_required_size(vcpu->arch.xcr0);
>>      }
>> 
>>      kvm_pmu_cpuid_update(vcpu);
>> ...
>>      kvm_put_guest_xcr0(vcpu);
>>      vcpu->arch.xcr0 = xcr0;
>> +
>> +    if ((xcr0 ^ old_xcr0) & XSTATE_EXTEND_MASK)
>> +            kvm_update_cpuid(vcpu);
>> +
>>      return 0;
>>  }
> 
> These hunks should be part of the previous patch.
> 
>> @@ -5960,6 +5967,9 @@ static int vcpu_enter_guest(struct kvm_vcpu
>> *vcpu)       preempt_disable(); 
>> 
>>      kvm_x86_ops->prepare_guest_switch(vcpu);
>> +    if (kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE) &&
> 
> Shouldn't be necessary, setting xcr0 fails unless OSXSAVE=1.
> 
>> +            (vcpu->arch.xcr0 & (u64)(XSTATE_BNDREGS | XSTATE_BNDCSR)))
>> +            kvm_x86_ops->fpu_activate(vcpu);
> 
> Can you explain this?

No, in fact I'm also some wondering about it, but per it has been tested, I 
didn't update this code.
I will double check and drop it if need (or, maybe Xudong can elaborate more?)

Thanks,
Jinsong

> 
>>      if (vcpu->fpu_active)
>>              kvm_load_guest_fpu(vcpu);
>>      kvm_load_guest_xcr0(vcpu);
>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>> index 587fb9e..985e40e 100644
>> --- a/arch/x86/kvm/x86.h
>> +++ b/arch/x86/kvm/x86.h
>> @@ -122,7 +122,8 @@ int kvm_write_guest_virt_system(struct
>>      x86_emulate_ctxt *ctxt, gva_t addr, void *val, unsigned int bytes,
>>      struct x86_exception *exception);
>> 
>> -#define KVM_SUPPORTED_XCR0  (XSTATE_FP | XSTATE_SSE | XSTATE_YMM)
>> +#define KVM_SUPPORTED_XCR0  (XSTATE_FP | XSTATE_SSE | XSTATE_YMM \
>> +                            | XSTATE_BNDREGS | XSTATE_BNDCSR)
>>  extern u64 host_xcr0;
>> 
>>  extern struct static_key kvm_no_apic_vcpu;
>> 
> 
> Otherwise looks straightforward.

Thanks, will update per your comments.