Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest

[Liu, Jinsong]

Eduardo Habkost wrote:
> On Fri, Mar 09, 2012 at 09:52:29PM +0100, Jan Kiszka wrote:
>> On 2012-03-09 20:09, Liu, Jinsong wrote:
>>> Jan Kiszka wrote:
>>>> On 2012-03-09 19:27, Liu, Jinsong wrote:
>>>>> Jan Kiszka wrote:
>>>>>> On 2012-03-06 08:49, Liu, Jinsong wrote:
>>>>>>> Jan,
>>>>>>> 
>>>>>>> Any comments? I feel some confused about your point 'disable
>>>>>>> cpuid feature for older machine types by default': are you
>>>>>>> planning a common approach for this common issue, or, you just
>>>>>>> ask me a specific solution for the tsc deadline timer case?
>>>>>> 
>>>>>> I think a generic solution for this can be as simple as passing a
>>>>>> feature exclusion mask to cpu_init. You could simple append a
>>>>>> string of "-feature1,-feature2" to the cpu model that is
>>>>>> specified on creation. And that string could be defined in the
>>>>>> compat machine descriptions. Does this make sense?
>>>>>> 
>>>>> 
>>>>> Jan, to prevent misunderstanding, I elaborate my understanding of
>>>>> your points below (if any misunderstanding please point out to
>>>>> me): ===================== Your target is, to migrate from A(old
>>>>> qemu) to B(new qemu) by 
>>>>> 1. at A: qemu-version-A [-cpu whatever]      // currently the
>>>>> default machine type is pc-A 
>>>>> 2. at B: qemu-version-B -machine pc-A [-cpu whatever] -feature1
>>>>> -feature2 
>>>>> 
>>>>> B run new qemu-version-B (w/ new features 'feature1' and
>>>>> 'feature2'), but when B runs w/ compat '-machine pc-A', vm should
>>>>> not see 'feature1' and 'feature2', so commandline append string to
>>>>> cpu model '-cpu whatever -feature1 -feature2' to hidden new
>>>>> feature1 and feature2 to vm, hence vm can see same cpuid features
>>>>> (at B) as those at A (which means, no feature1, no feature2)
>>>>> =====================
>>>>> 
>>>>> If my understanding of your thoughts is right, I think currently
>>>>>      qemu has satisfied your target, code refer to
>>>>>      pc_cpus_init(cpu_model) ...... cpu_init(cpu_model)
>>>>> --> cpu_x86_register(*env, cpu_model)
>>>>> --> cpu_x86_find_by_name(*def, cpu_model)     // parse '+/-
>>>>> 
>>>>> features', generate feature masks plus_features... // and
>>>>> minus_features...(this is feature exclusion masks you want)
>>>>> I think your point 'define in the compat machine description' is
>>>>> unnecessary.
>>>> 
>>>> The user would have to specify the new feature as exclusions
>>>> *manually* on the command line if -machine pc-A doesn't inject them
>>>> *automatically*. So it is necessary to enhance qemu in this regard.
>>>> 
>>> 
>>> ... You suggest 'append a string of "-feature1,-feature2" to the
>>> cpu model that is specified on creation' at your last email. Could
>>> you tell me other way user exclude features? I only know qemu
>>> command line :-(  
>> 
>> I was thinking of something like
>> 
>> diff --git a/hw/boards.h b/hw/boards.h
>> index 667177d..2bae071 100644
>> --- a/hw/boards.h
>> +++ b/hw/boards.h
>> @@ -28,6 +28,7 @@ typedef struct QEMUMachine {
>>      int is_default;
>>      const char *default_machine_opts;
>>      GlobalProperty *compat_props;
>> +    const char *compat_cpu_features;
>>      struct QEMUMachine *next;
>>  } QEMUMachine;
>> 
>> diff --git a/hw/pc.c b/hw/pc.c
>> index bb9867b..4d11559 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -949,8 +949,9 @@ static CPUState *pc_new_cpu(const char
>>  *cpu_model)      return env; }
>> 
>> -void pc_cpus_init(const char *cpu_model)
>> +void pc_cpus_init(const char *cpu_model, const char
>> *append_features)  { +    char *model_and_features;
>>      int i;
>> 
>>      /* init CPUs */
>> @@ -961,10 +962,13 @@ void pc_cpus_init(const char *cpu_model)
>>          cpu_model = "qemu32";
>>  #endif
>>      }
>> +    model_and_features = g_strconcat(cpu_model, ",",
>> append_features, NULL); 
>> 
>>      for(i = 0; i < smp_cpus; i++) {
>> -        pc_new_cpu(cpu_model);
>> +        pc_new_cpu(model_and_features);
>>      }
>> +
>> +    g_free(model_and_features);
>>  }
>> 
>>  void pc_memory_init(MemoryRegion *system_memory,
>> 
>> 
>> However, getting machine.compat_cpu_features to pc_cpus_init is
>> rather 
>> ugly. And we will have CPU devices with real properties soon. Then
>> the compat feature string could be passed that way, without changing
>> any 
>> machine init function.
> 
> What if one cpudef had the wrong flags set but another cpudef was
> correct, and we had to fix it on Qemu 1.1 for only one model? What if
> the user _really_ wanted to edit the config file to add or remove a
> given flag?
> 
> I think the best approach would be:
> - Having versioned CPU model names;
> - Specifying per-machine-type aliases.
> 
> See also the "[libvirt] Modern CPU models cannot be used with libvirt"
> for related discussion.
> 
> The config file would look like:
> 
> [cpudef]
> name = "Westmere-1.0"
> features = "[...]" # no tsc-deadline
> [...]
> 
> [cpudef]
> name = "Westmere-1.1"
> # so we don't have to copy everything from Westmere-1.0 manually:
> base_cpudef = "Westemere-1.0"
> # we could simply copy & extend:
> features = "[...] tsc-deadline"
> # or, even better, if we had a "append" mechanism. e.g.:
> #features_append = "tsc-deadline"
> 
> 
> Then, on the machine-type table:
> 
> - Machine-types "pc-1.0" and older would have:
>   .cpudef_aliases = {
>     {"Westmere", "Westmere-1.0"},
>     [...]
>   }
> 
> - Machine-type "pc-1.1" would have:
>   .cpudef_aliases = {
>     {"Westmere", "Westmere-1.1"},
>     [...]
>   }

Yes, it's reasonable in this way (or something like this). Considering 
currently qemu1.1 is changing cpu_device/per-machine-type, we'd better wait 
until it done and then update tsc deadline timer accordingly.

Thanks,
Jinsong