[Qemu-devel] [PATCH 0/4] target-i386: Don't try to enable unsupported TCG features by default

[Qemu-devel] [PATCH 0/4] target-i386: Don't try to enable unsupported TCG features by default

[Eduardo Habkost]

This series makes QEMU stop trying to enable an unsupported feature by default
in TCG mode, avoiding the warnings shown by the (now enabled by default)
"check" mode.

To do that, I cchanged the kvm_default_features code to be based on QOM
properties, and added a new "type" field to allow per-CPU-model KVM defaults to
be specified.

Before this series:

  $ qemu-system-x86_64 -S
  warning: TCG doesn't support requested feature: CPUID.01H:EDX.de [bit 2]
  warning: TCG doesn't support requested feature: CPUID.80000001H:EDX [bit 2]

After this series:

  $ ./x86_64-softmmu/qemu-system-x86_64 -S
  [no warning output]

Eduardo Habkost (4):
  target-i386: Expand PPRO_FEATURES on qemu64/qemu32 definitions
  target-i386: Convert kvm_default_*features to property/value pairs
  target-i386: Add optional class name to kvm_default_props
  target-i386: Disable DE on qemu64 & qemu32 on TCG

 hw/i386/pc_piix.c |   8 ++---
 hw/i386/pc_q35.c  |   4 +--
 target-i386/cpu.c | 104 ++++++++++++++++++++++++++++++++++--------------------
 target-i386/cpu.h |  12 +++++--
 4 files changed, 81 insertions(+), 47 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/4] target-i386: Expand PPRO_FEATURES on qemu64/qemu32 definitions

[Eduardo Habkost]

The qemu64 and qemu32 CPU models won't be Pentium Pro supersets anymore
because the DE feature will be removed. Copy PPRO_FEATURES into their
definitions so we can change the CPU models later.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 32fee00..d505743 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -701,7 +701,10 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .model = 6,
         .stepping = 3,
         .features[FEAT_1_EDX] =
-            PPRO_FEATURES |
+            CPUID_FP87 | CPUID_DE | CPUID_PSE | CPUID_TSC |
+            CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_PGE | CPUID_CMOV |
+            CPUID_PAT | CPUID_FXSR | CPUID_MMX | CPUID_SSE | CPUID_SSE2 |
+            CPUID_PAE | CPUID_SEP | CPUID_APIC |
             CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA |
             CPUID_PSE36,
         .features[FEAT_1_ECX] =
@@ -804,7 +807,10 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .model = 6,
         .stepping = 3,
         .features[FEAT_1_EDX] =
-            PPRO_FEATURES,
+            CPUID_FP87 | CPUID_DE | CPUID_PSE | CPUID_TSC |
+            CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_PGE | CPUID_CMOV |
+            CPUID_PAT | CPUID_FXSR | CPUID_MMX | CPUID_SSE | CPUID_SSE2 |
+            CPUID_PAE | CPUID_SEP | CPUID_APIC,
         .features[FEAT_1_ECX] =
             CPUID_EXT_SSE3 | CPUID_EXT_POPCNT,
         .xlevel = 0x80000004,
-- 
2.1.0

[Qemu-devel] [PATCH 2/4] target-i386: Convert kvm_default_*features to property/value pairs

[Eduardo Habkost]

Convert the kvm_default_features and kvm_default_unset_features arrays
into a simple list of property/value pairs that will be applied to
X86CPU objects when using KVM.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc_piix.c |  8 ++---
 hw/i386/pc_q35.c  |  4 +--
 target-i386/cpu.c | 87 ++++++++++++++++++++++++++++++++-----------------------
 target-i386/cpu.h | 11 +++++--
 4 files changed, 65 insertions(+), 45 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 3f925b2..e79ef0d 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -325,7 +325,7 @@ static void pc_compat_2_1(MachineState *machine)
 
     pc_compat_2_2(machine);
     smbios_uuid_encoded = false;
-    x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
+    x86_cpu_change_kvm_default("svm", NULL);
     pcms->enforce_aligned_dimm = false;
 }
 
@@ -361,7 +361,7 @@ static void pc_compat_1_7(MachineState *machine)
     gigabyte_align = false;
     option_rom_has_mr = true;
     legacy_acpi_table_size = 6414;
-    x86_cpu_compat_kvm_no_autoenable(FEAT_1_ECX, CPUID_EXT_X2APIC);
+    x86_cpu_change_kvm_default("x2apic", NULL);
 }
 
 static void pc_compat_1_6(MachineState *machine)
@@ -391,7 +391,7 @@ static void pc_compat_1_3(MachineState *machine)
 static void pc_compat_1_2(MachineState *machine)
 {
     pc_compat_1_3(machine);
-    x86_cpu_compat_kvm_no_autoenable(FEAT_KVM, 1 << KVM_FEATURE_PV_EOI);
+    x86_cpu_change_kvm_default("kvm-pv-eoi", NULL);
 }
 
 /* PC compat function for pc-0.10 to pc-0.13 */
@@ -414,7 +414,7 @@ static void pc_init_isa(MachineState *machine)
     if (!machine->cpu_model) {
         machine->cpu_model = "486";
     }
-    x86_cpu_compat_kvm_no_autoenable(FEAT_KVM, 1 << KVM_FEATURE_PV_EOI);
+    x86_cpu_change_kvm_default("kvm-pv-eoi", NULL);
     enable_compat_apic_id_mode();
     pc_init1(machine, TYPE_I440FX_PCI_HOST_BRIDGE, TYPE_I440FX_PCI_DEVICE);
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 11601ab..c17d9df 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -309,7 +309,7 @@ static void pc_compat_2_1(MachineState *machine)
     pc_compat_2_2(machine);
     pcms->enforce_aligned_dimm = false;
     smbios_uuid_encoded = false;
-    x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
+    x86_cpu_change_kvm_default("svm", NULL);
 }
 
 static void pc_compat_2_0(MachineState *machine)
@@ -326,7 +326,7 @@ static void pc_compat_1_7(MachineState *machine)
     smbios_defaults = false;
     gigabyte_align = false;
     option_rom_has_mr = true;
-    x86_cpu_compat_kvm_no_autoenable(FEAT_1_ECX, CPUID_EXT_X2APIC);
+    x86_cpu_change_kvm_default("x2apic", NULL);
 }
 
 static void pc_compat_1_6(MachineState *machine)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d505743..9a4bfa3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -478,38 +478,6 @@ const char *get_register_name_32(unsigned int reg)
     return x86_reg_info_32[reg].name;
 }
 
-/* KVM-specific features that are automatically added to all CPU models
- * when KVM is enabled.
- */
-static uint32_t kvm_default_features[FEATURE_WORDS] = {
-    [FEAT_KVM] = (1 << KVM_FEATURE_CLOCKSOURCE) |
-        (1 << KVM_FEATURE_NOP_IO_DELAY) |
-        (1 << KVM_FEATURE_CLOCKSOURCE2) |
-        (1 << KVM_FEATURE_ASYNC_PF) |
-        (1 << KVM_FEATURE_STEAL_TIME) |
-        (1 << KVM_FEATURE_PV_EOI) |
-        (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT),
-    [FEAT_1_ECX] = CPUID_EXT_X2APIC,
-};
-
-/* Features that are not added by default to any CPU model when KVM is enabled.
- */
-static uint32_t kvm_default_unset_features[FEATURE_WORDS] = {
-    [FEAT_1_EDX] = CPUID_ACPI,
-    [FEAT_1_ECX] = CPUID_EXT_MONITOR,
-    [FEAT_8000_0001_ECX] = CPUID_EXT3_SVM,
-};
-
-void x86_cpu_compat_kvm_no_autoenable(FeatureWord w, uint32_t features)
-{
-    kvm_default_features[w] &= ~features;
-}
-
-void x86_cpu_compat_kvm_no_autodisable(FeatureWord w, uint32_t features)
-{
-    kvm_default_unset_features[w] &= ~features;
-}
-
 /*
  * Returns the set of feature flags that are supported and migratable by
  * QEMU, for a given FeatureWord.
@@ -1398,6 +1366,43 @@ static X86CPUDefinition builtin_x86_defs[] = {
     },
 };
 
+typedef struct PropValue {
+    const char *prop, *value;
+} PropValue;
+
+/* KVM-specific features that are automatically added/removed
+ * from all CPU models when KVM is enabled.
+ */
+static PropValue kvm_default_props[] = {
+    { "kvmclock", "on" },
+    { "kvm-nopiodelay", "on" },
+    { "kvm-asyncpf", "on" },
+    { "kvm-steal-time", "on" },
+    { "kvm-pv-eoi", "on" },
+    { "kvmclock-stable-bit", "on" },
+    { "x2apic", "on" },
+    { "acpi", "off" },
+    { "monitor", "off" },
+    { "svm", "off" },
+    { NULL, NULL },
+};
+
+void x86_cpu_change_kvm_default(const char *prop, const char *value)
+{
+    PropValue *pv;
+    for (pv = kvm_default_props; pv->prop; pv++) {
+        if (!strcmp(pv->prop, prop)) {
+            pv->value = value;
+            break;
+        }
+    }
+
+    /* It is valid to call this function only for properties that
+     * are already present in the kvm_default_props table.
+     */
+    assert(pv->prop);
+}
+
 static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
                                                    bool migratable_only);
 
@@ -2067,6 +2072,18 @@ static int x86_cpu_filter_features(X86CPU *cpu)
     return rv;
 }
 
+static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
+{
+    PropValue *pv;
+    for (pv = props; pv->prop; pv++) {
+        if (!pv->value) {
+            continue;
+        }
+        object_property_parse(OBJECT(cpu), pv->value, pv->prop,
+                              &error_abort);
+    }
+}
+
 /* Load data from X86CPUDefinition
  */
 static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
@@ -2090,11 +2107,7 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
 
     /* Special cases not set in the X86CPUDefinition structs: */
     if (kvm_enabled()) {
-        FeatureWord w;
-        for (w = 0; w < FEATURE_WORDS; w++) {
-            env->features[w] |= kvm_default_features[w];
-            env->features[w] &= ~kvm_default_unset_features[w];
-        }
+        x86_cpu_apply_props(cpu, kvm_default_props);
     }
 
     env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 74b674d..f7c733b 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1332,8 +1332,15 @@ void cpu_smm_update(X86CPU *cpu);
 
 void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
 
-void x86_cpu_compat_kvm_no_autoenable(FeatureWord w, uint32_t features);
-void x86_cpu_compat_kvm_no_autodisable(FeatureWord w, uint32_t features);
+/* Change the value of a KVM-specific default
+ *
+ * If value is NULL, no default will be set and the original
+ * value from the CPU model table will be kept.
+ *
+ * It is valid to call this funciton only for properties that
+ * are already present in the kvm_default_props table.
+ */
+void x86_cpu_change_kvm_default(const char *prop, const char *value);
 
 
 /* Return name of 32-bit register, from a R_* constant */
-- 
2.1.0

[Qemu-devel] [PATCH 3/4] target-i386: Add optional class name to kvm_default_props

[Eduardo Habkost]

This will allow us to define class-specific KVM defaults, instead of
defaults that apply to all CPU models.

If this table is starting to look like the global properties tables,
that's not a coincidence: in the future, we might convert this to
accelerator-specific code that simply register global properties when
the machine is initialized.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc_piix.c |  8 ++++----
 hw/i386/pc_q35.c  |  4 ++--
 target-i386/cpu.c | 29 +++++++++++++++++------------
 target-i386/cpu.h |  3 ++-
 4 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e79ef0d..ad98735 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -325,7 +325,7 @@ static void pc_compat_2_1(MachineState *machine)
 
     pc_compat_2_2(machine);
     smbios_uuid_encoded = false;
-    x86_cpu_change_kvm_default("svm", NULL);
+    x86_cpu_change_kvm_default(NULL, "svm", NULL);
     pcms->enforce_aligned_dimm = false;
 }
 
@@ -361,7 +361,7 @@ static void pc_compat_1_7(MachineState *machine)
     gigabyte_align = false;
     option_rom_has_mr = true;
     legacy_acpi_table_size = 6414;
-    x86_cpu_change_kvm_default("x2apic", NULL);
+    x86_cpu_change_kvm_default(NULL, "x2apic", NULL);
 }
 
 static void pc_compat_1_6(MachineState *machine)
@@ -391,7 +391,7 @@ static void pc_compat_1_3(MachineState *machine)
 static void pc_compat_1_2(MachineState *machine)
 {
     pc_compat_1_3(machine);
-    x86_cpu_change_kvm_default("kvm-pv-eoi", NULL);
+    x86_cpu_change_kvm_default(NULL, "kvm-pv-eoi", NULL);
 }
 
 /* PC compat function for pc-0.10 to pc-0.13 */
@@ -414,7 +414,7 @@ static void pc_init_isa(MachineState *machine)
     if (!machine->cpu_model) {
         machine->cpu_model = "486";
     }
-    x86_cpu_change_kvm_default("kvm-pv-eoi", NULL);
+    x86_cpu_change_kvm_default(NULL, "kvm-pv-eoi", NULL);
     enable_compat_apic_id_mode();
     pc_init1(machine, TYPE_I440FX_PCI_HOST_BRIDGE, TYPE_I440FX_PCI_DEVICE);
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c17d9df..94d417e 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -309,7 +309,7 @@ static void pc_compat_2_1(MachineState *machine)
     pc_compat_2_2(machine);
     pcms->enforce_aligned_dimm = false;
     smbios_uuid_encoded = false;
-    x86_cpu_change_kvm_default("svm", NULL);
+    x86_cpu_change_kvm_default(NULL, "svm", NULL);
 }
 
 static void pc_compat_2_0(MachineState *machine)
@@ -326,7 +326,7 @@ static void pc_compat_1_7(MachineState *machine)
     smbios_defaults = false;
     gigabyte_align = false;
     option_rom_has_mr = true;
-    x86_cpu_change_kvm_default("x2apic", NULL);
+    x86_cpu_change_kvm_default(NULL, "x2apic", NULL);
 }
 
 static void pc_compat_1_6(MachineState *machine)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 9a4bfa3..ec9eca2 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1367,6 +1367,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
 };
 
 typedef struct PropValue {
+    const char *type;
     const char *prop, *value;
 } PropValue;
 
@@ -1374,24 +1375,25 @@ typedef struct PropValue {
  * from all CPU models when KVM is enabled.
  */
 static PropValue kvm_default_props[] = {
-    { "kvmclock", "on" },
-    { "kvm-nopiodelay", "on" },
-    { "kvm-asyncpf", "on" },
-    { "kvm-steal-time", "on" },
-    { "kvm-pv-eoi", "on" },
-    { "kvmclock-stable-bit", "on" },
-    { "x2apic", "on" },
-    { "acpi", "off" },
-    { "monitor", "off" },
-    { "svm", "off" },
+    { NULL, "kvmclock", "on" },
+    { NULL, "kvm-nopiodelay", "on" },
+    { NULL, "kvm-asyncpf", "on" },
+    { NULL, "kvm-steal-time", "on" },
+    { NULL, "kvm-pv-eoi", "on" },
+    { NULL, "kvmclock-stable-bit", "on" },
+    { NULL, "x2apic", "on" },
+    { NULL, "acpi", "off" },
+    { NULL, "monitor", "off" },
+    { NULL, "svm", "off" },
     { NULL, NULL },
 };
 
-void x86_cpu_change_kvm_default(const char *prop, const char *value)
+void x86_cpu_change_kvm_default(const char *type, const char *prop,
+                                const char *value)
 {
     PropValue *pv;
     for (pv = kvm_default_props; pv->prop; pv++) {
-        if (!strcmp(pv->prop, prop)) {
+        if (!g_strcmp0(pv->type, type) && !strcmp(pv->prop, prop)) {
             pv->value = value;
             break;
         }
@@ -2079,6 +2081,9 @@ static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
         if (!pv->value) {
             continue;
         }
+        if (pv->type && !object_dynamic_cast(OBJECT(cpu), pv->type)) {
+            continue;
+        }
         object_property_parse(OBJECT(cpu), pv->value, pv->prop,
                               &error_abort);
     }
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index f7c733b..c77d455 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1340,7 +1340,8 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
  * It is valid to call this funciton only for properties that
  * are already present in the kvm_default_props table.
  */
-void x86_cpu_change_kvm_default(const char *prop, const char *value);
+void x86_cpu_change_kvm_default(const char *type, const char *prop,
+                                const char *value);
 
 
 /* Return name of 32-bit register, from a R_* constant */
-- 
2.1.0

[Qemu-devel] [PATCH 4/4] target-i386: Disable DE on qemu64 & qemu32 on TCG

[Eduardo Habkost]

We don't want to make QEMU show CPUID feature warnings by default when
running in TCG mode, so disable the flag in the default qemu32 and
qemu64 CPU models, and add it to kvm_default_props so it gets enabled
when running KVM.

We don't need any compat code for this because:
* DE was already enabled in KVM mode, and is kept enabled
* DE was already disabled in TCG mode, and is kept disabled

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ec9eca2..1617e48 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -669,7 +669,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .model = 6,
         .stepping = 3,
         .features[FEAT_1_EDX] =
-            CPUID_FP87 | CPUID_DE | CPUID_PSE | CPUID_TSC |
+            CPUID_FP87 | CPUID_PSE | CPUID_TSC |
             CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_PGE | CPUID_CMOV |
             CPUID_PAT | CPUID_FXSR | CPUID_MMX | CPUID_SSE | CPUID_SSE2 |
             CPUID_PAE | CPUID_SEP | CPUID_APIC |
@@ -775,7 +775,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .model = 6,
         .stepping = 3,
         .features[FEAT_1_EDX] =
-            CPUID_FP87 | CPUID_DE | CPUID_PSE | CPUID_TSC |
+            CPUID_FP87 | CPUID_PSE | CPUID_TSC |
             CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_PGE | CPUID_CMOV |
             CPUID_PAT | CPUID_FXSR | CPUID_MMX | CPUID_SSE | CPUID_SSE2 |
             CPUID_PAE | CPUID_SEP | CPUID_APIC,
@@ -1385,6 +1385,8 @@ static PropValue kvm_default_props[] = {
     { NULL, "acpi", "off" },
     { NULL, "monitor", "off" },
     { NULL, "svm", "off" },
+    { X86_CPU_TYPE_NAME("qemu64"), "de", "on" },
+    { X86_CPU_TYPE_NAME("qemu32"), "de", "on" },
     { NULL, NULL },
 };
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH 0/4] target-i386: Don't try to enable unsupported TCG features by default

[Richard Henderson]

On 09/11/2015 12:25 PM, Eduardo Habkost wrote:
> This series makes QEMU stop trying to enable an unsupported feature by default
> in TCG mode, avoiding the warnings shown by the (now enabled by default)
> "check" mode.
> 
> To do that, I cchanged the kvm_default_features code to be based on QOM
> properties, and added a new "type" field to allow per-CPU-model KVM defaults to
> be specified.
> 
> Before this series:
> 
>   $ qemu-system-x86_64 -S
>   warning: TCG doesn't support requested feature: CPUID.01H:EDX.de [bit 2]
>   warning: TCG doesn't support requested feature: CPUID.80000001H:EDX [bit 2]
> 
> After this series:
> 
>   $ ./x86_64-softmmu/qemu-system-x86_64 -S
>   [no warning output]

Wouldn't it be easier simply to implement this bit?
It seems like we're simply creating a compatibility mess otherwise.


r~

Re: [Qemu-devel] [PATCH 0/4] target-i386: Don't try to enable unsupported TCG features by default

[Eduardo Habkost]

On Fri, Sep 11, 2015 at 12:32:35PM -0700, Richard Henderson wrote:
> On 09/11/2015 12:25 PM, Eduardo Habkost wrote:
> > This series makes QEMU stop trying to enable an unsupported feature by default
> > in TCG mode, avoiding the warnings shown by the (now enabled by default)
> > "check" mode.
> > 
> > To do that, I cchanged the kvm_default_features code to be based on QOM
> > properties, and added a new "type" field to allow per-CPU-model KVM defaults to
> > be specified.
> > 
> > Before this series:
> > 
> >   $ qemu-system-x86_64 -S
> >   warning: TCG doesn't support requested feature: CPUID.01H:EDX.de [bit 2]
> >   warning: TCG doesn't support requested feature: CPUID.80000001H:EDX [bit 2]
> > 
> > After this series:
> > 
> >   $ ./x86_64-softmmu/qemu-system-x86_64 -S
> >   [no warning output]
> 
> Wouldn't it be easier simply to implement this bit?
> It seems like we're simply creating a compatibility mess otherwise.

I don't think the compatibility mess is worse than the one we already
have with x2apic. I even think the new type/property/value system is
cleaner than the existing kvm_default_*features arrays.

About implementing DE in TCG: I really don't think it is easier, but if
somebody wants to implement it, it would be welcome.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/4] target-i386: Don't try to enable unsupported TCG features by default

[Paolo Bonzini]

On 11/09/2015 21:58, Eduardo Habkost wrote:
> About implementing DE in TCG: I really don't think it is easier, but if
> somebody wants to implement it, it would be welcome.

Actually I agree that it's easier, and even a partial implementation
(e.g. no I/O port breakpoints) would be nice to have because recent
Windows IIRC requires DE.

Paolo

Re: [Qemu-devel] [PATCH 2/4] target-i386: Convert kvm_default_*features to property/value pairs

[Paolo Bonzini]

On 11/09/2015 21:25, Eduardo Habkost wrote:
> Convert the kvm_default_features and kvm_default_unset_features arrays
> into a simple list of property/value pairs that will be applied to
> X86CPU objects when using KVM.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/i386/pc_piix.c |  8 ++---
>  hw/i386/pc_q35.c  |  4 +--
>  target-i386/cpu.c | 87 ++++++++++++++++++++++++++++++++-----------------------
>  target-i386/cpu.h | 11 +++++--
>  4 files changed, 65 insertions(+), 45 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 3f925b2..e79ef0d 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -325,7 +325,7 @@ static void pc_compat_2_1(MachineState *machine)
>  
>      pc_compat_2_2(machine);
>      smbios_uuid_encoded = false;
> -    x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
> +    x86_cpu_change_kvm_default("svm", NULL);
>      pcms->enforce_aligned_dimm = false;
>  }
>  
> @@ -361,7 +361,7 @@ static void pc_compat_1_7(MachineState *machine)
>      gigabyte_align = false;
>      option_rom_has_mr = true;
>      legacy_acpi_table_size = 6414;
> -    x86_cpu_compat_kvm_no_autoenable(FEAT_1_ECX, CPUID_EXT_X2APIC);
> +    x86_cpu_change_kvm_default("x2apic", NULL);
>  }
>  
>  static void pc_compat_1_6(MachineState *machine)
> @@ -391,7 +391,7 @@ static void pc_compat_1_3(MachineState *machine)
>  static void pc_compat_1_2(MachineState *machine)
>  {
>      pc_compat_1_3(machine);
> -    x86_cpu_compat_kvm_no_autoenable(FEAT_KVM, 1 << KVM_FEATURE_PV_EOI);
> +    x86_cpu_change_kvm_default("kvm-pv-eoi", NULL);
>  }
>  
>  /* PC compat function for pc-0.10 to pc-0.13 */
> @@ -414,7 +414,7 @@ static void pc_init_isa(MachineState *machine)
>      if (!machine->cpu_model) {
>          machine->cpu_model = "486";
>      }
> -    x86_cpu_compat_kvm_no_autoenable(FEAT_KVM, 1 << KVM_FEATURE_PV_EOI);
> +    x86_cpu_change_kvm_default("kvm-pv-eoi", NULL);
>      enable_compat_apic_id_mode();
>      pc_init1(machine, TYPE_I440FX_PCI_HOST_BRIDGE, TYPE_I440FX_PCI_DEVICE);
>  }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 11601ab..c17d9df 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -309,7 +309,7 @@ static void pc_compat_2_1(MachineState *machine)
>      pc_compat_2_2(machine);
>      pcms->enforce_aligned_dimm = false;
>      smbios_uuid_encoded = false;
> -    x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
> +    x86_cpu_change_kvm_default("svm", NULL);
>  }
>  
>  static void pc_compat_2_0(MachineState *machine)
> @@ -326,7 +326,7 @@ static void pc_compat_1_7(MachineState *machine)
>      smbios_defaults = false;
>      gigabyte_align = false;
>      option_rom_has_mr = true;
> -    x86_cpu_compat_kvm_no_autoenable(FEAT_1_ECX, CPUID_EXT_X2APIC);
> +    x86_cpu_change_kvm_default("x2apic", NULL);
>  }
>  
>  static void pc_compat_1_6(MachineState *machine)
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index d505743..9a4bfa3 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -478,38 +478,6 @@ const char *get_register_name_32(unsigned int reg)
>      return x86_reg_info_32[reg].name;
>  }
>  
> -/* KVM-specific features that are automatically added to all CPU models
> - * when KVM is enabled.
> - */
> -static uint32_t kvm_default_features[FEATURE_WORDS] = {
> -    [FEAT_KVM] = (1 << KVM_FEATURE_CLOCKSOURCE) |
> -        (1 << KVM_FEATURE_NOP_IO_DELAY) |
> -        (1 << KVM_FEATURE_CLOCKSOURCE2) |
> -        (1 << KVM_FEATURE_ASYNC_PF) |
> -        (1 << KVM_FEATURE_STEAL_TIME) |
> -        (1 << KVM_FEATURE_PV_EOI) |
> -        (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT),
> -    [FEAT_1_ECX] = CPUID_EXT_X2APIC,
> -};
> -
> -/* Features that are not added by default to any CPU model when KVM is enabled.
> - */
> -static uint32_t kvm_default_unset_features[FEATURE_WORDS] = {
> -    [FEAT_1_EDX] = CPUID_ACPI,
> -    [FEAT_1_ECX] = CPUID_EXT_MONITOR,
> -    [FEAT_8000_0001_ECX] = CPUID_EXT3_SVM,
> -};
> -
> -void x86_cpu_compat_kvm_no_autoenable(FeatureWord w, uint32_t features)
> -{
> -    kvm_default_features[w] &= ~features;
> -}
> -
> -void x86_cpu_compat_kvm_no_autodisable(FeatureWord w, uint32_t features)
> -{
> -    kvm_default_unset_features[w] &= ~features;
> -}
> -
>  /*
>   * Returns the set of feature flags that are supported and migratable by
>   * QEMU, for a given FeatureWord.
> @@ -1398,6 +1366,43 @@ static X86CPUDefinition builtin_x86_defs[] = {
>      },
>  };
>  
> +typedef struct PropValue {
> +    const char *prop, *value;
> +} PropValue;
> +
> +/* KVM-specific features that are automatically added/removed
> + * from all CPU models when KVM is enabled.
> + */
> +static PropValue kvm_default_props[] = {
> +    { "kvmclock", "on" },
> +    { "kvm-nopiodelay", "on" },
> +    { "kvm-asyncpf", "on" },
> +    { "kvm-steal-time", "on" },
> +    { "kvm-pv-eoi", "on" },
> +    { "kvmclock-stable-bit", "on" },
> +    { "x2apic", "on" },
> +    { "acpi", "off" },
> +    { "monitor", "off" },
> +    { "svm", "off" },
> +    { NULL, NULL },
> +};
> +
> +void x86_cpu_change_kvm_default(const char *prop, const char *value)
> +{
> +    PropValue *pv;
> +    for (pv = kvm_default_props; pv->prop; pv++) {
> +        if (!strcmp(pv->prop, prop)) {
> +            pv->value = value;
> +            break;
> +        }
> +    }
> +
> +    /* It is valid to call this function only for properties that
> +     * are already present in the kvm_default_props table.
> +     */
> +    assert(pv->prop);
> +}
> +
>  static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
>                                                     bool migratable_only);
>  
> @@ -2067,6 +2072,18 @@ static int x86_cpu_filter_features(X86CPU *cpu)
>      return rv;
>  }
>  
> +static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
> +{
> +    PropValue *pv;
> +    for (pv = props; pv->prop; pv++) {
> +        if (!pv->value) {
> +            continue;
> +        }
> +        object_property_parse(OBJECT(cpu), pv->value, pv->prop,
> +                              &error_abort);
> +    }
> +}
> +
>  /* Load data from X86CPUDefinition
>   */
>  static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
> @@ -2090,11 +2107,7 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
>  
>      /* Special cases not set in the X86CPUDefinition structs: */
>      if (kvm_enabled()) {
> -        FeatureWord w;
> -        for (w = 0; w < FEATURE_WORDS; w++) {
> -            env->features[w] |= kvm_default_features[w];
> -            env->features[w] &= ~kvm_default_unset_features[w];
> -        }
> +        x86_cpu_apply_props(cpu, kvm_default_props);
>      }
>  
>      env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 74b674d..f7c733b 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1332,8 +1332,15 @@ void cpu_smm_update(X86CPU *cpu);
>  
>  void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
>  
> -void x86_cpu_compat_kvm_no_autoenable(FeatureWord w, uint32_t features);
> -void x86_cpu_compat_kvm_no_autodisable(FeatureWord w, uint32_t features);
> +/* Change the value of a KVM-specific default
> + *
> + * If value is NULL, no default will be set and the original
> + * value from the CPU model table will be kept.
> + *
> + * It is valid to call this funciton only for properties that
> + * are already present in the kvm_default_props table.
> + */
> +void x86_cpu_change_kvm_default(const char *prop, const char *value);
>  
>  
>  /* Return name of 32-bit register, from a R_* constant */
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 0/4] target-i386: Don't try to enable unsupported TCG features by default

[Eduardo Habkost]

On Sat, Sep 12, 2015 at 11:03:45PM +0200, Paolo Bonzini wrote:
> 
> 
> On 11/09/2015 21:58, Eduardo Habkost wrote:
> > About implementing DE in TCG: I really don't think it is easier, but if
> > somebody wants to implement it, it would be welcome.
> 
> Actually I agree that it's easier, and even a partial implementation
> (e.g. no I/O port breakpoints) would be nice to have because recent
> Windows IIRC requires DE.

So if anybody volunteer to implement DE in TCG before 2.5, we won't need
this series (except for patch 2/4, that I plan to apply anyway).

If nobody implements it before soft freeze, I will resubmit the series.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/4] target-i386: Don't try to enable unsupported TCG features by default

[Richard Henderson]

On 09/12/2015 02:03 PM, Paolo Bonzini wrote:
> 
> 
> On 11/09/2015 21:58, Eduardo Habkost wrote:
>> About implementing DE in TCG: I really don't think it is easier, but if
>> somebody wants to implement it, it would be welcome.
> 
> Actually I agree that it's easier, and even a partial implementation
> (e.g. no I/O port breakpoints) would be nice to have because recent
> Windows IIRC requires DE.

Isn't I/O port breakpoints the only thing we're missing from DE?


r~

Re: [Qemu-devel] [PATCH 0/4] target-i386: Don't try to enable unsupported TCG features by default

[Paolo Bonzini]

On 14/09/2015 19:21, Richard Henderson wrote:
>>> About implementing DE in TCG: I really don't think it is easier, but if
>>> >> somebody wants to implement it, it would be welcome.
>> > 
>> > Actually I agree that it's easier, and even a partial implementation
>> > (e.g. no I/O port breakpoints) would be nice to have because recent
>> > Windows IIRC requires DE.
> Isn't I/O port breakpoints the only thing we're missing from DE?

I was thinking of the CR4 bit, but it looks like we don't raise #GP at
all on writes to unknown CR0 or CR4 bits.

Paolo

Re: [Qemu-devel] [PATCH 0/4] target-i386: Don't try to enable unsupported TCG features by default

[Richard Henderson]

On 09/14/2015 10:23 AM, Paolo Bonzini wrote:
> 
> 
> On 14/09/2015 19:21, Richard Henderson wrote:
>>>> About implementing DE in TCG: I really don't think it is easier, but if
>>>>>> somebody wants to implement it, it would be welcome.
>>>>
>>>> Actually I agree that it's easier, and even a partial implementation
>>>> (e.g. no I/O port breakpoints) would be nice to have because recent
>>>> Windows IIRC requires DE.
>> Isn't I/O port breakpoints the only thing we're missing from DE?
> 
> I was thinking of the CR4 bit, but it looks like we don't raise #GP at
> all on writes to unknown CR0 or CR4 bits.

Nor do we #gp accesses to dr[45] with DE, nor map dr[45] to dr[67] without DE.

Does the i/o breakpoint happen before or after the access?  If before, we could
implement the breakpoint in check_io.  Otherwise we'd need a new helper.


r~

Re: [Qemu-devel] [PATCH 0/4] target-i386: Don't try to enable unsupported TCG features by default

[Paolo Bonzini]

On 14/09/2015 19:31, Richard Henderson wrote:
> On 09/14/2015 10:23 AM, Paolo Bonzini wrote:
>>
>>
>> On 14/09/2015 19:21, Richard Henderson wrote:
>>>>> About implementing DE in TCG: I really don't think it is easier, but if
>>>>>>> somebody wants to implement it, it would be welcome.
>>>>>
>>>>> Actually I agree that it's easier, and even a partial implementation
>>>>> (e.g. no I/O port breakpoints) would be nice to have because recent
>>>>> Windows IIRC requires DE.
>>> Isn't I/O port breakpoints the only thing we're missing from DE?
>>
>> I was thinking of the CR4 bit, but it looks like we don't raise #GP at
>> all on writes to unknown CR0 or CR4 bits.
> 
> Nor do we #gp accesses to dr[45] with DE, nor map dr[45] to dr[67] without DE.
> 
> Does the i/o breakpoint happen before or after the access?  If before, we could
> implement the breakpoint in check_io.  Otherwise we'd need a new helper.

Table 6-3 says it's a trap, so it occurs after.

Paolo

Re: [Qemu-devel] [PATCH 2/4] target-i386: Convert kvm_default_*features to property/value pairs

[Eduardo Habkost]

On Mon, Sep 14, 2015 at 10:32:14AM +0200, Paolo Bonzini wrote:
> On 11/09/2015 21:25, Eduardo Habkost wrote:
> > Convert the kvm_default_features and kvm_default_unset_features arrays
> > into a simple list of property/value pairs that will be applied to
> > X86CPU objects when using KVM.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
[...]
> 
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Tested using the avocado-vt+tp-qemu CPUID test case that compares CPUID
data with existing CPUID dumps, no issues found.

Applied to x86 tree.

-- 
Eduardo