Re: [Qemu-devel] [PATCH v4 12/18] target-i386: Support check/enforce flags in TCG mode, too

[Andreas Färber]

Am 15.05.2014 21:12, schrieb Eduardo Habkost:
> On Thu, May 15, 2014 at 08:54:15PM +0200, Andreas Färber wrote:
>> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
>>> If enforce/check is specified in TCG mode, QEMU will ensure all CPU
>>> features are supported by TCG, so no CPU feature is silently disabled.
>>>
>>> Reviewed-by: Richard Henderson <address@hidden>
>>> Signed-off-by: Eduardo Habkost <address@hidden>
>>> ---
>>> Changes v1 -> v2:
>>>  * Trivial rebase to latest qom-cpu (commit 90c5d39c)
>>>    (Reviewed-by line kept)
>>> Changes v2 -> v3:
>>>  * Trivial rebase after QEMU 2.0 (onto commit 2d03b49)
>>>    (Reviewed-by line kept)
>>> ---
>>>  target-i386/cpu.c | 34 ++++++++++++++++------------------
>>>  1 file changed, 16 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>> index b2e30ca..53b5038 100644
>>> --- a/target-i386/cpu.c
>>> +++ b/target-i386/cpu.c
>>> @@ -1265,8 +1265,9 @@ static int report_unavailable_features(FeatureWord w, 
>>> uint32_t mask)
>>>          if (1 << i & mask) {
>>>              const char *reg = get_register_name_32(f->cpuid_reg);
>>>              assert(reg);
>>> -            fprintf(stderr, "warning: host doesn't support requested 
>>> feature: "
>>> +            fprintf(stderr, "warning: %s doesn't support requested 
>>> feature: "
>>>                  "CPUID.%02XH:%s%s%s [bit %d]\n",
>>> +                kvm_enabled() ? "host" : "TCG",
>>>                  f->cpuid_eax, reg,
>>>                  f->feat_names[i] ? "." : "",
>>>                  f->feat_names[i] ? f->feat_names[i] : "", i);
>>> @@ -1826,17 +1827,18 @@ CpuDefinitionInfoList 
>>> *arch_query_cpu_definitions(Error **errp)
>>>  static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w)
>>>  {
>>>      FeatureWordInfo *wi = &feature_word_info[w];
>>> -    assert(kvm_enabled());
>>> -    return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
>>> -                                                   wi->cpuid_ecx,
>>> -                                                   wi->cpuid_reg);
>>> +    if (kvm_enabled()) {
>>> +        return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
>>> +                                                       wi->cpuid_ecx,
>>> +                                                       wi->cpuid_reg);
>>> +    } else {
>>> +        return wi->tcg_features;
>>> +    }
>>>  }
>>
>> This function is called unconditionally now, so apply the following?
>>
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index 48ba1d8..112b437 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -1839,8 +1839,10 @@ static uint32_t
>> x86_cpu_get_supported_feature_word(FeatureWord w)
>>          return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
>>                                                         wi->cpuid_ecx,
>>                                                         wi->cpuid_reg);
>> -    } else {
>> +    } else if (tcg_enabled()) {
>>          return wi->tcg_features;
>> +    } else {
>> +        return UINT32_MAX;
> 
> Agreed, but I would prefer writing it as ~0 instead of UINT32_MAX.

FTR done as ~0u to avoid any signedness issues.

> 
>>      }
>>  }
>>
>>
>> Not sure what to do about the warning message. It wouldn't occur though
>> due to the suggested mask, so we could just ignore it for now.
> 
> One day we may be able to simply ask the machine object for the current
> accelerator name. In the meantime, we could use:
> 
>   "warning: host (%s) doesn't support requested feature [...]",
>   kvm_enabled() ? "KVM" : (tcg_enabled() ? "TCG" : "QEMU")
> 
> (But I won't object if you prefer to keep the warning message I
> originally sent.)

I think I did the latter, yes...

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg