| Message ID | 1348171412-23669-3-git-send-email-Don@CloudSwitch.com |
|---|---|
| State | New |
| Headers | show |
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 >
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
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
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[] = {
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(-)