[2/2] Expose tsc deadline timer cpuid to guest

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



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.

Andreas, do you expect CPU devices to be ready for qemu 1.1? We would
need them to pass a feature exclusion mask from machine.compat_props to
the (x86) CPU init code.

Well, given that introducing some intermediate solution for this would
be complex and hacky and that there is a way to configure tsc_deadline
for old machines away, though only an explicit one, I could live with
postponing the feature mask after the CPU device conversion. But the
last word will have the maintainers.

Jan

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 09.03.2012 21:52, schrieb Jan Kiszka:
> Andreas, do you expect CPU devices to be ready for qemu 1.1? We 
> would need them to pass a feature exclusion mask from 
> machine.compat_props to the (x86) CPU init code.

I was sure hoping to!

Marcelo and Avi both going into vacation have let me down with the
kvmclock patch that you wanted to go through uq/master.

http://patchwork.ozlabs.org/patch/141721/

After it didn't make it into Avi's next PULL, I resubmitted it as part
of the qom-user series. That's still on the list. Review and tags
appreciated.

http://patchwork.ozlabs.org/patch/144529/

Also on the list is a patch for a new object_class_get_list() function
for sensible -cpu ? handling.

http://patchwork.ozlabs.org/patch/143076/

I have been waiting for progress on these prerequisites before posting
huge follow-on batches, but I can spam the list right away...
The remaining issue was how to solve the struct CPU vs. CPU() issue -
my solution on qom-cpu branch is to free the identifier CPUState in a
quite invasive but half-scripted renaming. Based on struct CPUState,
all targets are long successfully converted and keep needing to be
rebased whenever someone moves code to a new microblaze file, adds a
new arm board or in-kernel device using "CPUState", etc.
Since there's a configure change involved in *-user QOM support,
virtually all files get rebuilt on each rebase and that takes quite
some time, so getting that in soonish would be appreciated.

http://repo.or.cz/w/qemu/afaerber.git/shortlog/refs/heads/qom-cpu

Andreas

- -- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.18 (GNU/Linux)

iQIcBAEBAgAGBQJPWqljAAoJEPou0S0+fgE/u+kQAInqv/OYlL2zU+hyeKXdJcZ4
RJ9Ne7J1+MOYcOI0p2T4lTImawHmZbF1HQyM9gTYv3EcGoSLm/SgYnHhFOulvPaE
dbpuwk3KPHU+1ekvMVhEX89F62rXvMTz8JxO7hSmNAwIU2fIeKKWEAo0tED2tY16
OC2ExnE46+xLpandGJmfnx8vUJ1/PaO6fpd/FLOZbACT75zmdDxjDpF85e6NNsct
ysMQ3kNMWcc0RfTU02dPgrhCPc5irEbalaX6BtOnquVQZDH4ypwzKHOZpMfoTF4u
QWyxy+NaOVMVBHgHJeGek0wQ4nbihIhrQAn+Hg/h0P+NPuxQ8s7zX4xwqlYwhAHn
cdDfN1sVbIvRWkGCXSRJR5eIkPKV+81Afm7/pL26eNlVtSrVV9UnsOJyfyDrtFs/
tSE90n8ZecjARGXUaACboZRalt7Pxl4EGWJwBhsNd5+3xaYvc9/WuTYP1FK1nltk
KiBETQXnSY5aTh13erKLjbtYGR5YexXiLIOXvnBD0qQ/LDryHNjslgM8HcShrx4S
U9LFAqEq78fgpCXZlzwtnzed341Wuerw3z4jELrSW8GH+R21jMfT9L8lwaHexabh
7YlulgdSpsmCxzhHOUCMeUTFFY6TlPxVk2NhEhQxlzDXdDmm6nd67R9qJIf7d42z
CPzkIX+OpKBmorMQREIg
=a+ZQ
-----END PGP SIGNATURE-----

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.
> 
> Andreas, do you expect CPU devices to be ready for qemu 1.1? We would
> need them to pass a feature exclusion mask from machine.compat_props
> to 
> the (x86) CPU init code.

cpu devices is just another format of current cpu_model. It helps nothing to our problem.
Again, the point is, by what method the feature exclusion mask can be generated, if user not give hint manually?

Thanks,
Jinsong

> 
> Well, given that introducing some intermediate solution for this would
> be complex and hacky and that there is a way to configure tsc_deadline
> for old machines away, though only an explicit one, I could live with
> postponing the feature mask after the CPU device conversion. But the
> last word will have the maintainers.
> 
> Jan

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"},
    [...]
  }

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