Patchwork [v4,02/17] target-i386: Add missing kvm bits.

login
register
mail settings
Submitter Don Slutz
Date Sept. 20, 2012, 8:03 p.m.
Message ID <1348171412-23669-3-git-send-email-Don@CloudSwitch.com>
Download mbox | patch
Permalink /patch/185477/
State New
Headers show

Comments

Don Slutz - Sept. 20, 2012, 8:03 p.m.
Fix duplicate name (kvmclock => kvm_clock2) also.

Signed-off-by: Don Slutz <Don@CloudSwitch.com>
---
 target-i386/cpu.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)
Igor Mammedov - Sept. 21, 2012, 8:39 a.m.
On Thu, 20 Sep 2012 16:03:17 -0400
Don Slutz <Don@cloudswitch.com> wrote:

> Fix duplicate name (kvmclock => kvm_clock2) also.
> 
> Signed-off-by: Don Slutz <Don@CloudSwitch.com>
> ---
>  target-i386/cpu.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 0313cf5..5f9866a 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -87,10 +87,14 @@ static const char *ext3_feature_name[] = {
>  };
>  
>  static const char *kvm_feature_name[] = {
> -    "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock", "kvm_asyncpf", NULL, "kvm_pv_eoi", NULL,
> -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> +    "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvm_clock2",
before patch if "kvmclock" is specified it would set 0 and 3 bits,
after patch only bit 0 is set.
Is it correct/expected behavior? if yes, please add rationale into patch
description.
 
> +    "kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    "kvm_clock_stable", NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
>  };
>  
>  static const char *svm_feature_name[] = {
> -- 
> 1.7.1
>
Eduardo Habkost - Sept. 21, 2012, 12:36 p.m.
On Fri, Sep 21, 2012 at 10:39:52AM +0200, Igor Mammedov wrote:
> On Thu, 20 Sep 2012 16:03:17 -0400
> Don Slutz <Don@cloudswitch.com> wrote:
> 
> > Fix duplicate name (kvmclock => kvm_clock2) also.
> > 
> > Signed-off-by: Don Slutz <Don@CloudSwitch.com>
> > ---
> >  target-i386/cpu.c |   12 ++++++++----
> >  1 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 0313cf5..5f9866a 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -87,10 +87,14 @@ static const char *ext3_feature_name[] = {
> >  };
> >  
> >  static const char *kvm_feature_name[] = {
> > -    "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock", "kvm_asyncpf", NULL, "kvm_pv_eoi", NULL,
> > -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> > -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> > -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> > +    "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvm_clock2",
> before patch if "kvmclock" is specified it would set 0 and 3 bits,
> after patch only bit 0 is set.
> Is it correct/expected behavior? if yes, please add rationale into patch
> description.

The problem here seems to be:
- It would be interesting to make "kvmclock=true" enough to enable the
  optimal behavior, instead of requiring users to use "kvm_clock2=true"
  explicitly
- We need to allow older machine-types to be backwards compatible (not
  enabling the second bit by default), so we need a separate property
  to control the second bit.

I think this is best modelled this way:

- Having two separate properties: kvmclock and kvmclock2 (or kvm_clock2)
- Older machine-types would have kvmclock2 default to false. Newer
  machine-types would kvmclock2 default to true.
- kvmclock=false would disable both bits

Then:

 - kvmclock=false would not set any bit (it would be surprising to have
   kvmclock=false but still have kvmclock enabled)
 - kvmclock=true would keep compatible behavior on older machine-types,
   (only the first bit set), but would get optimal behavior on newer
   machine-types (both bits set)
 - kvmclock=true,kvmclock2=true would set both bits
 - kvmclock=true,kvmclock2=false would set only the first bit

It wouldn't be a direct mapping between properties and CPUID bits, but
that's exactly the point. In this case, exposing individual CPUID bits
directly is a too low-level interface.


>  
> > +    "kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", NULL,
> > +    NULL, NULL, NULL, NULL,
> > +    NULL, NULL, NULL, NULL,
> > +    NULL, NULL, NULL, NULL,
> > +    NULL, NULL, NULL, NULL,
> > +    "kvm_clock_stable", NULL, NULL, NULL,
> > +    NULL, NULL, NULL, NULL,
> >  };
> >  
> >  static const char *svm_feature_name[] = {
> > -- 
> > 1.7.1
> > 
> 
> 
> -- 
> Regards,
>   Igor
Don Slutz - Sept. 21, 2012, 1:17 p.m.
On 09/21/12 08:36, Eduardo Habkost wrote:
> On Fri, Sep 21, 2012 at 10:39:52AM +0200, Igor Mammedov wrote:
>> On Thu, 20 Sep 2012 16:03:17 -0400
>> Don Slutz <Don@cloudswitch.com> wrote:
>>
>>> Fix duplicate name (kvmclock => kvm_clock2) also.
>>>
>>> Signed-off-by: Don Slutz <Don@CloudSwitch.com>
>>> ---
>>>   target-i386/cpu.c |   12 ++++++++----
>>>   1 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>> index 0313cf5..5f9866a 100644
>>> --- a/target-i386/cpu.c
>>> +++ b/target-i386/cpu.c
>>> @@ -87,10 +87,14 @@ static const char *ext3_feature_name[] = {
>>>   };
>>>   
>>>   static const char *kvm_feature_name[] = {
>>> -    "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock", "kvm_asyncpf", NULL, "kvm_pv_eoi", NULL,
>>> -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>>> -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>>> -    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>>> +    "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvm_clock2",
>> before patch if "kvmclock" is specified it would set 0 and 3 bits,
>> after patch only bit 0 is set.
>> Is it correct/expected behavior? if yes, please add rationale into patch
>> description.
This is not what I had intended.
> The problem here seems to be:
> - It would be interesting to make "kvmclock=true" enough to enable the
>    optimal behavior, instead of requiring users to use "kvm_clock2=true"
>    explicitly
> - We need to allow older machine-types to be backwards compatible (not
>    enabling the second bit by default), so we need a separate property
>    to control the second bit.
>
> I think this is best modelled this way:
>
> - Having two separate properties: kvmclock and kvmclock2 (or kvm_clock2)
> - Older machine-types would have kvmclock2 default to false. Newer
>    machine-types would kvmclock2 default to true.
> - kvmclock=false would disable both bits
>
> Then:
>
>   - kvmclock=false would not set any bit (it would be surprising to have
>     kvmclock=false but still have kvmclock enabled)
>   - kvmclock=true would keep compatible behavior on older machine-types,
>     (only the first bit set), but would get optimal behavior on newer
>     machine-types (both bits set)
>   - kvmclock=true,kvmclock2=true would set both bits
>   - kvmclock=true,kvmclock2=false would set only the first bit
>
> It wouldn't be a direct mapping between properties and CPUID bits, but
> that's exactly the point. In this case, exposing individual CPUID bits
> directly is a too low-level interface.
>
This does look much better.  For the sake of simple changes, this patch 
will be changed so that -kvmclock (kvmclock=false) will continue to 
clear both bits.  I will look into the right way to fit this into the 
newer cpu model.
>>   
>>> +    "kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", NULL,
>>> +    NULL, NULL, NULL, NULL,
>>> +    NULL, NULL, NULL, NULL,
>>> +    NULL, NULL, NULL, NULL,
>>> +    NULL, NULL, NULL, NULL,
>>> +    "kvm_clock_stable", NULL, NULL, NULL,
>>> +    NULL, NULL, NULL, NULL,
>>>   };
>>>   
>>>   static const char *svm_feature_name[] = {
>>> -- 
>>> 1.7.1
>>>
>>
>> -- 
>> Regards,
>>    Igor
   -Don Slutz

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0313cf5..5f9866a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -87,10 +87,14 @@  static const char *ext3_feature_name[] = {
 };
 
 static const char *kvm_feature_name[] = {
-    "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock", "kvm_asyncpf", NULL, "kvm_pv_eoi", NULL,
-    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
-    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
-    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
+    "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvm_clock2",
+    "kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    "kvm_clock_stable", NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
 };
 
 static const char *svm_feature_name[] = {