Re: [Qemu-devel] [PATCH 2/2] target-i386: Intel MPX support

[Liu, Jinsong]

Paolo Bonzini wrote:
> Il 29/11/2013 14:17, Liu, Jinsong ha scritto:
>> From aac033473bc88befe39a9add99820c0a7118ac90 Mon Sep 17 00:00:00
>> 2001 
>> From: root <address@hidden(none)>
>> Date: Fri, 22 Nov 2013 00:24:35 +0800
>> Subject: [PATCH 2/2] target-i386: Intel MPX support
>> 
>> Expose cpuid leaf (0xd, 3) and (0xd, 4) to guest.
>> Fix ebx and re-calculate ecx of cpuid leaf (0xd, 0).
> 
> There is no reason to get the size and offset from the host.  Peter
> Anvin confirmed that the sizes and offsets will never change (as
> should be the case for migration to work across different CPU
> versions).  In fact, the size and offset is documented for every
> XSAVE feature except MPX in the copy I have of the Intel
> documentation. 

If the sizes and offsets will never change, what's the bad effect of getting 
them from host?

> 
> Please get the size and offset from the documentation, if it has been
> updated, or from a real host, and hardcode them in QEMU.
> 

Hmm, the problem is what I get is not equal to real test :(
For example, I was told XSTATE_BNDCSR_SIZE is 0x40, but real test shows it's 
0x10.

Maybe getting from real h/w is not bad than hardcode it?

Thanks
Jinsong

> 
>> Signed-off-by: Liu Jinsong <address@hidden>
>> ---
>>  target-i386/cpu.c |   34 ++++++++++++++++++++++++++-------- 
>>  target-i386/cpu.h |    1 + 2 files changed, 27 insertions(+), 8
>> deletions(-) 
>> 
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index 544b57f..7d04f28 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -330,12 +330,12 @@ X86RegisterInfo32
>> x86_reg_info_32[CPU_NB_REGS32] = { 
>> 
>>  typedef struct ExtSaveArea {
>>      uint32_t feature, bits;
>> -    uint32_t offset, size;
>>  } ExtSaveArea;
>> 
>>  static const ExtSaveArea ext_save_areas[] = {
>> -    [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX,
>> -            .offset = 0x240, .size = 0x100 },
>> +    [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX },
>> +    [3] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX },
>> +    [4] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX }, 
>> }; 
>> 
>>  const char *get_register_name_32(unsigned int reg)
>> @@ -2204,9 +2204,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t
>>              index, uint32_t count,
>> ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32); 
>> 
>>          if (count == 0) {
>> -            *ecx = 0x240;
>> +            *ebx = *ecx = 0x240;
>>              for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) {
>> +                uint32_t offset, size;
>>                  const ExtSaveArea *esa = &ext_save_areas[i]; +
>>                  if ((env->features[esa->feature] & esa->bits) ==
>>                      esa->bits && (kvm_mask & (1 << i)) != 0) {
>>                      if (i < 32) {
>> @@ -2214,19 +2216,35 @@ void cpu_x86_cpuid(CPUX86State *env,
>>                          uint32_t index, uint32_t count,            
>>                      } else { *edx |= 1 << (i - 32); }
>> -                    *ecx = MAX(*ecx, esa->offset + esa->size); +
>> +                    size = kvm_arch_get_supported_cpuid(s, 0xd, i,
>> R_EAX); +                    offset =
>> kvm_arch_get_supported_cpuid(s, 0xd, i, R_EBX); +                   
>> *ecx = MAX(*ecx, offset + size); + +                    /*
>> +                     * EBX here just in order to
>> +                     * 1. keep compatible with old qemu version,
>> take AVX +                     *    into account;
>> +                     * 2. keep compatible with old kernel version.
>> Currently +                     *    KVM has bug when expose cpuid
>> 0xd to guest (include +                     *    static value when
>> guest booting and dynamic value +                     *    when
>> guest enables XCR0 features. EBX here can +                     *   
>> co-work with old buggy and new updated KVM, keep +                  
>> *    same value independent to CPU and kernel version. +            
>> */ +                    if (i == 2) +                        *ebx =
>>              MAX(*ebx, offset + size);                  } }
>>              *eax |= kvm_mask & (XSTATE_FP | XSTATE_SSE); -         
>>          *ebx = *ecx; } else if (count == 1) {
>>              *eax = kvm_arch_get_supported_cpuid(s, 0xd, 1, R_EAX);
>>          } else if (count < ARRAY_SIZE(ext_save_areas)) {
>>              const ExtSaveArea *esa = &ext_save_areas[count];
>>              if ((env->features[esa->feature] & esa->bits) ==
>>                  esa->bits && (kvm_mask & (1 << count)) != 0) {
>> -                *eax = esa->size;
>> -                *ebx = esa->offset;
>> +                *eax = kvm_arch_get_supported_cpuid(s, 0xd, count,
>> R_EAX); +                *ebx = kvm_arch_get_supported_cpuid(s, 0xd,
>>          count, R_EBX);              } }
>>          break;
>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>> index ea373e8..9a838d1 100644
>> --- a/target-i386/cpu.h
>> +++ b/target-i386/cpu.h
>> @@ -545,6 +545,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>>  #define CPUID_7_0_EBX_ERMS     (1 << 9)
>>  #define CPUID_7_0_EBX_INVPCID  (1 << 10)
>>  #define CPUID_7_0_EBX_RTM      (1 << 11)
>> +#define CPUID_7_0_EBX_MPX      (1 << 14)
>>  #define CPUID_7_0_EBX_RDSEED   (1 << 18)
>>  #define CPUID_7_0_EBX_ADX      (1 << 19)
>>  #define CPUID_7_0_EBX_SMAP     (1 << 20)
>> --
>> 1.7.1