Re: [PATCH v4 02/17] target-i386: Add missing kvm bits.

Don Slutz <Don@xxxxxxxxxxxxxxx> · Fri, 21 Sep 2012 09:17:19 -0400

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@xxxxxxxxxxxxxxx> wrote:

Fix duplicate name (kvmclock => kvm_clock2) also.

Signed-off-by: Don Slutz <Don@xxxxxxxxxxxxxxx>
---
  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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html





