[Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Eduardo Habkost]

This implements GET_SUPPORTED_CPUID support using an explicit option for it:
"allow-emulation". We don't want any emulated feature to be enabled by accident,
so they will be enabled only if the user explicitly wants to allow them.

References to previous patch and discussions:
    Message-Id: <1379861095-628-7-git-send-email-bp@alien8.de>
    http://marc.info/?l=kvm&m=137986116331560&w=2

    Message-ID: <20140604204448.GG4105@pd.tnic>
    http://marc.info/?l=kvm&m=140191471903819

This requires a few fixes to the qom-cpu tree which haven't been merged yet.
Git tree:

    git://github.com/ehabkost/qemu-hacks.git work/get-emulated-cpuid

Cc: Borislav Petkov <bp@alien8.de>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: kvm@vger.kernel.org
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>
Cc: Andreas Färber <afaerber@suse.de>
Cc: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>

Borislav Petkov (1):
  kvm: Implement kvm_arch_get_emulated_cpuid()

Eduardo Habkost (1):
  target-i386: Add "allow-emulation" X86CPU property

 include/sysemu/kvm.h  |  3 +++
 target-i386/cpu-qom.h |  3 +++
 target-i386/cpu.c     | 18 ++++++++++++++----
 target-i386/kvm.c     | 38 ++++++++++++++++++++++++++++++++++----
 4 files changed, 54 insertions(+), 8 deletions(-)

-- 
1.9.0

[Qemu-devel] [RFC 1/2] kvm: Implement kvm_arch_get_emulated_cpuid()

[Eduardo Habkost]

From: Borislav Petkov <bp@suse.de>

Add support for the KVM_GET_EMULATED_CPUID ioctl and leave feature bits
enabled, when requested by userspace, if kvm emulates them.

Signed-off-by: Borislav Petkov <bp@suse.de>
[ehabkost: removed target-i386/cpu.c code, changed patch description]
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/sysemu/kvm.h |  3 +++
 target-i386/kvm.c    | 38 ++++++++++++++++++++++++++++++++++----
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index e7ad9d1..ab1ffdb 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -324,6 +324,9 @@ int kvm_check_extension(KVMState *s, unsigned int extension);
 
 uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
                                       uint32_t index, int reg);
+uint32_t kvm_arch_get_emulated_cpuid(KVMState *env, uint32_t function,
+                                     uint32_t index, int reg);
+
 
 #if !defined(CONFIG_USER_ONLY)
 int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index f9ffa4b..242df34 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -88,7 +88,7 @@ bool kvm_allows_irq0_override(void)
     return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
 }
 
-static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max)
+static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int ioctl, int max)
 {
     struct kvm_cpuid2 *cpuid;
     int r, size;
@@ -96,7 +96,7 @@ static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max)
     size = sizeof(*cpuid) + max * sizeof(*cpuid->entries);
     cpuid = (struct kvm_cpuid2 *)g_malloc0(size);
     cpuid->nent = max;
-    r = kvm_ioctl(s, KVM_GET_SUPPORTED_CPUID, cpuid);
+    r = kvm_ioctl(s, ioctl, cpuid);
     if (r == 0 && cpuid->nent >= max) {
         r = -E2BIG;
     }
@@ -105,7 +105,10 @@ static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max)
             g_free(cpuid);
             return NULL;
         } else {
-            fprintf(stderr, "KVM_GET_SUPPORTED_CPUID failed: %s\n",
+            fprintf(stderr, "%s failed: %s\n",
+                    (ioctl == (int)KVM_GET_SUPPORTED_CPUID
+                     ? "KVM_GET_SUPPORTED_CPUID"
+                     : "KVM_GET_EMULATED_CPUID"),
                     strerror(-r));
             exit(1);
         }
@@ -120,7 +123,17 @@ static struct kvm_cpuid2 *get_supported_cpuid(KVMState *s)
 {
     struct kvm_cpuid2 *cpuid;
     int max = 1;
-    while ((cpuid = try_get_cpuid(s, max)) == NULL) {
+    while ((cpuid = try_get_cpuid(s, KVM_GET_SUPPORTED_CPUID, max)) == NULL) {
+        max *= 2;
+    }
+    return cpuid;
+}
+
+static struct kvm_cpuid2 *get_emulated_cpuid(KVMState *s)
+{
+    struct kvm_cpuid2 *cpuid;
+    int max = 1;
+    while ((cpuid = try_get_cpuid(s, KVM_GET_EMULATED_CPUID, max)) == NULL) {
         max *= 2;
     }
     return cpuid;
@@ -248,6 +261,23 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
     return ret;
 }
 
+uint32_t kvm_arch_get_emulated_cpuid(KVMState *s, uint32_t function,
+                                     uint32_t index, int reg)
+{
+    struct kvm_cpuid2 *cpuid __attribute__((unused));
+
+    if (!kvm_check_extension(s, KVM_CAP_EXT_EMUL_CPUID))
+        return 0;
+
+    cpuid = get_emulated_cpuid(s);
+
+    struct kvm_cpuid_entry2 *entry = cpuid_find_entry(cpuid, function, index);
+    if (entry)
+        return cpuid_entry_get_reg(entry, reg);
+
+    return 0;
+}
+
 typedef struct HWPoisonPage {
     ram_addr_t ram_addr;
     QLIST_ENTRY(HWPoisonPage) list;
-- 
1.9.0

[Qemu-devel] [RFC 2/2] target-i386: Add "allow-emulation" X86CPU property

[Eduardo Habkost]

The new option will allow slow emulated features (the ones returned by
GET_EMULATED_CPUID) to be enabled. We don't want to allow them to be
enabled by accident, so they will be enabled only if emulation is
explicitly allowed by the user.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu-qom.h |  3 +++
 target-i386/cpu.c     | 18 ++++++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 385b81f..831f7bf 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -74,6 +74,8 @@ typedef struct X86CPUClass {
  * @migratable: If set, only migratable flags will be accepted when "enforce"
  * mode is used, and only migratable flags will be included in the "host"
  * CPU model.
+ * @allow_emulation: If set, accelerator-specific code will allow emulated
+ * features to be enabled.
  *
  * An x86 CPU.
  */
@@ -91,6 +93,7 @@ typedef struct X86CPU {
     bool check_cpuid;
     bool enforce_cpuid;
     bool migratable;
+    bool allow_emulation;
 
     /* if true the CPUID code directly forward host cache leaves to the guest */
     bool cache_info_passthrough;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1395473..568d82a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1280,7 +1280,8 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void *data)
 }
 
 static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
-                                                   bool migratable_only);
+                                                   bool migratable_only,
+                                                   bool allow_emulation);
 
 static void host_x86_cpu_initfn(Object *obj)
 {
@@ -1297,7 +1298,8 @@ static void host_x86_cpu_initfn(Object *obj)
 
     for (w = 0; w < FEATURE_WORDS; w++) {
         env->features[w] =
-            x86_cpu_get_supported_feature_word(w, cpu->migratable);
+            x86_cpu_get_supported_feature_word(w, cpu->migratable,
+                                                  cpu->allow_emulation);
     }
     object_property_set_bool(OBJECT(cpu), true, "pmu", &error_abort);
 }
@@ -1887,7 +1889,8 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
 }
 
 static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
-                                                   bool migratable_only)
+                                                   bool migratable_only,
+                                                   bool allow_emulation)
 {
     FeatureWordInfo *wi = &feature_word_info[w];
     uint32_t r;
@@ -1896,6 +1899,11 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
         r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
                                                     wi->cpuid_ecx,
                                                     wi->cpuid_reg);
+        if (allow_emulation) {
+            r |= kvm_arch_get_emulated_cpuid(kvm_state,  wi->cpuid_eax,
+                                                         wi->cpuid_ecx,
+                                                         wi->cpuid_reg);
+        }
     } else if (tcg_enabled()) {
         r = wi->tcg_features;
     } else {
@@ -1920,7 +1928,8 @@ static int x86_cpu_filter_features(X86CPU *cpu)
 
     for (w = 0; w < FEATURE_WORDS; w++) {
         uint32_t host_feat =
-            x86_cpu_get_supported_feature_word(w, cpu->migratable);
+            x86_cpu_get_supported_feature_word(w, cpu->migratable,
+                                                  cpu->allow_emulation);
         uint32_t requested_features = env->features[w];
         env->features[w] &= host_feat;
         cpu->filtered_features[w] = requested_features & ~env->features[w];
@@ -2854,6 +2863,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("hv-time", X86CPU, hyperv_time, false),
     DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, false),
     DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
+    DEFINE_PROP_BOOL("allow-emulation", X86CPU, allow_emulation, true),
     DEFINE_PROP_END_OF_LIST()
 };
 
-- 
1.9.0

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Alexander Graf]

On 05.06.14 18:12, Eduardo Habkost wrote:
> This implements GET_SUPPORTED_CPUID support using an explicit option for it:
> "allow-emulation". We don't want any emulated feature to be enabled by accident,
> so they will be enabled only if the user explicitly wants to allow them.

So is this an all-or-nothing approach? I would really prefer to override 
individual bits.

Also, I don't think the line "emulated" is the right one to draw. We 
"emulate" SVM or VMX too, but still enable them by default as soon as we 
think they're ready enough.

So could we add a new flag specifier instead? Today we have -flag and 
+flag. How about *flag to "force enable if the kernel can handle it, but 
doesn't do so by default"?


Alex

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Paolo Bonzini]

Il 05/06/2014 18:24, Alexander Graf ha scritto:
>
> On 05.06.14 18:12, Eduardo Habkost wrote:
>> This implements GET_SUPPORTED_CPUID support using an explicit option
>> for it:
>> "allow-emulation". We don't want any emulated feature to be enabled by
>> accident,
>> so they will be enabled only if the user explicitly wants to allow them.
>
> So is this an all-or-nothing approach? I would really prefer to override
> individual bits.

You can still disable them with "cpu foo,-movbe,allow-emulation".

> Also, I don't think the line "emulated" is the right one to draw. We
> "emulate" SVM or VMX too, but still enable them by default as soon as we
> think they're ready enough.

Well, I disagreed with the whole KVM_GET_EMULATED_CPUID concept for 
MOVBE too for example.  It seemed overengineered to me, sooner or later 
we might graduate MOVBE out of KVM_GET_EMULATED_CPUID as well.

However, for MONITOR/MWAIT it makes some sense.

Paolo

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Alexander Graf]

On 05.06.14 18:26, Paolo Bonzini wrote:
> Il 05/06/2014 18:24, Alexander Graf ha scritto:
>>
>> On 05.06.14 18:12, Eduardo Habkost wrote:
>>> This implements GET_SUPPORTED_CPUID support using an explicit option
>>> for it:
>>> "allow-emulation". We don't want any emulated feature to be enabled by
>>> accident,
>>> so they will be enabled only if the user explicitly wants to allow 
>>> them.
>>
>> So is this an all-or-nothing approach? I would really prefer to override
>> individual bits.
>
> You can still disable them with "cpu foo,-movbe,allow-emulation".
>
>> Also, I don't think the line "emulated" is the right one to draw. We
>> "emulate" SVM or VMX too, but still enable them by default as soon as we
>> think they're ready enough.
>
> Well, I disagreed with the whole KVM_GET_EMULATED_CPUID concept for 
> MOVBE too for example.  It seemed overengineered to me, sooner or 
> later we might graduate MOVBE out of KVM_GET_EMULATED_CPUID as well.
>
> However, for MONITOR/MWAIT it makes some sense.

I honestly think what we want for MONITOR/MWAIT is a cpuid-override bit.

   cpuid = user_specified_cpuids();
   cpuid &= kvm_capable_cpuids()
   cpuid |= user_override_cpuids();

   kvm_set_cpuid(cpuid);

If the user knows what he's doing, he can set the force bit. If the 
kernel happens to emulate that instruction, he's happy. If the kernel 
doesn't emulate it, it will fail and he will realize that he did 
something stupid.

But ok, we do have this awesome GET_EMULATE_CPUID ioctl now, so we can 
as well use it - even though I consider it superfluous:

   cpuid = user_specified_cpuids();
   cpuid &= kvm_capable_cpuids()
   cpuid |= user_override_cpuids() & kvm_emulated_cpuid();

   kvm_set_cpuid(cpuid);

but enabling all experimental features inside KVM just because we want 
one or two of them is very counter-intuitive. Imagine we'd introduce 
emulation support for AVX. Suddenly allow-emulation (which I'd need for 
Mac OS X 10.5) would enable AVX as well which I really don't want enabled.

Also, while we can't change the ioctl name anymore, please let's use 
"experimental" rather than "emulated" as the name everywhere. Maybe 
we'll never bump individual features from experimental to fully 
supported, but there's no reason we wouldn't have emulated features that 
are not part of this bitmap and there's no reason we wouldn't have real 
hardware features that are not ready yet and part of this bitmap.


Alex

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Paolo Bonzini]

Il 05/06/2014 18:40, Alexander Graf ha scritto:
>
>
>   kvm_set_cpuid(cpuid);
>
> but enabling all experimental features inside KVM just because we want
> one or two of them is very counter-intuitive. Imagine we'd introduce
> emulation support for AVX. Suddenly allow-emulation (which I'd need for
> Mac OS X 10.5) would enable AVX as well which I really don't want enabled.

Only if you were using "-cpu somethingThatHasAVX", though, no?

Paolo

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Alexander Graf]

On 05.06.14 18:44, Paolo Bonzini wrote:
> Il 05/06/2014 18:40, Alexander Graf ha scritto:
>>
>>
>>   kvm_set_cpuid(cpuid);
>>
>> but enabling all experimental features inside KVM just because we want
>> one or two of them is very counter-intuitive. Imagine we'd introduce
>> emulation support for AVX. Suddenly allow-emulation (which I'd need for
>> Mac OS X 10.5) would enable AVX as well which I really don't want 
>> enabled.
>
> Only if you were using "-cpu somethingThatHasAVX", though, no?

Yes. The same argument goes the other way around. I want to use AVX 
emulation, do "allow-emulation" and suddenly I get MONITOR/MWAIT emulation.


Alex

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Paolo Bonzini]

Il 05/06/2014 18:45, Alexander Graf ha scritto:
>>>
>>
>> Only if you were using "-cpu somethingThatHasAVX", though, no?
>
> Yes. The same argument goes the other way around. I want to use AVX
> emulation, do "allow-emulation" and suddenly I get MONITOR/MWAIT emulation.

What about:

- letting "-cpu foo,+emulatedfeature" just work

- adding emulated=yes that blindly enables all emulated features

- making "-cpu ...,check" prints a warning for emulated features unless 
emulated=yes

Paolo

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Alexander Graf]

On 05.06.14 18:52, Paolo Bonzini wrote:
> Il 05/06/2014 18:45, Alexander Graf ha scritto:
>>>>
>>>
>>> Only if you were using "-cpu somethingThatHasAVX", though, no?
>>
>> Yes. The same argument goes the other way around. I want to use AVX
>> emulation, do "allow-emulation" and suddenly I get MONITOR/MWAIT 
>> emulation.
>
> What about:
>
> - letting "-cpu foo,+emulatedfeature" just work
>
> - adding emulated=yes that blindly enables all emulated features
>
> - making "-cpu ...,check" prints a warning for emulated features 
> unless emulated=yes

How about we remove the emulated=yes from this list? Then I'm happy :).


Alex

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Paolo Bonzini]

Il 05/06/2014 18:54, Alexander Graf ha scritto:
>>
>> What about:
>>
>> - letting "-cpu foo,+emulatedfeature" just work
>>
>> - adding emulated=yes that blindly enables all emulated features
>>
>> - making "-cpu ...,check" prints a warning for emulated features
>> unless emulated=yes
>
> How about we remove the emulated=yes from this list? Then I'm happy :).

So:

- "-cpu foo" doesn't enable any emulated feature

- "-cpu foo,+movbe" does

- "-cpu foo,check" and "-cpu foo,enforce" print a nice and descriptive 
message if the feature is not available but could be enabled as emulated

Ok?

Paolo

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Alexander Graf]

On 05.06.14 18:52, Paolo Bonzini wrote:
> Il 05/06/2014 18:45, Alexander Graf ha scritto:
>>>>
>>>
>>> Only if you were using "-cpu somethingThatHasAVX", though, no?
>>
>> Yes. The same argument goes the other way around. I want to use AVX
>> emulation, do "allow-emulation" and suddenly I get MONITOR/MWAIT 
>> emulation.
>
> What about:
>
> - letting "-cpu foo,+emulatedfeature" just work
>
> - adding emulated=yes that blindly enables all emulated features
>
> - making "-cpu ...,check" prints a warning for emulated features 
> unless emulated=yes

So:

   -cpu foo,+emulatedFeature just works

   -cpu foo,+notEmulatedFeature still sets the CPUID bit for that feature

   -cpu foo,check prints warnings for all cpuid bits not in the 
"allowed" bitmap. It prints different warnings depending on whether the 
bit is in "emulated" or not


Alex

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Eduardo Habkost]

On Thu, Jun 05, 2014 at 06:45:16PM +0200, Alexander Graf wrote:
> 
> On 05.06.14 18:44, Paolo Bonzini wrote:
> >Il 05/06/2014 18:40, Alexander Graf ha scritto:
> >>
> >>
> >>  kvm_set_cpuid(cpuid);
> >>
> >>but enabling all experimental features inside KVM just because we want
> >>one or two of them is very counter-intuitive. Imagine we'd introduce
> >>emulation support for AVX. Suddenly allow-emulation (which I'd need for
> >>Mac OS X 10.5) would enable AVX as well which I really don't want
> >>enabled.
> >
> >Only if you were using "-cpu somethingThatHasAVX", though, no?
> 
> Yes. The same argument goes the other way around. I want to use AVX
> emulation, do "allow-emulation" and suddenly I get MONITOR/MWAIT
> emulation.

This patch is just changing the set of _allowed_ bits, it is _not_
changing the actual semantics of a given "-cpu" option combination.
Unless you are not using the "enforce" flag, but you should be using it
if you care about not having CPUID data changing under your feet.

If you don't want MONITOR/MWAIT you shouldn't be using a CPU model
containing MONITOR/MWAIT in the first place. If you use "-cpu
somethingWithMONITOR", that means you are already asking QEMU for a CPU
with MONITOR. If you were not getting MONITOR before using
allow-emulation, that means you were not using the "enforce" flag, which
you should be using, already.

-- 
Eduardo

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Eduardo Habkost]

On Thu, Jun 05, 2014 at 06:57:57PM +0200, Paolo Bonzini wrote:
> Il 05/06/2014 18:54, Alexander Graf ha scritto:
> >>
> >>What about:
> >>
> >>- letting "-cpu foo,+emulatedfeature" just work
> >>
> >>- adding emulated=yes that blindly enables all emulated features
> >>
> >>- making "-cpu ...,check" prints a warning for emulated features
> >>unless emulated=yes
> >
> >How about we remove the emulated=yes from this list? Then I'm happy :).
> 
> So:
> 
> - "-cpu foo" doesn't enable any emulated feature

What if "foo" already has movbe in the CPU model definition?

> 
> - "-cpu foo,+movbe" does

What if I want movbe enabled if and only if it is _not_ emulated?

The whole point here is to never ever ever enable an emulated feature
unless it was explicitly what the user wanted.

> 
> - "-cpu foo,check" and "-cpu foo,enforce" print a nice and
> descriptive message if the feature is not available but could be
> enabled as emulated

"nice and descriptive message" needs to be better specified. Messages on
stderr are useless for management software.

Maybe a "emulated-features" property in addition to the existing
"filtered-features" would be useful.

But any of the above changes the fact that this series does not change
the semantics of any "-cpu" option combination, except when not using
the "enforce" flag (which everybody who cares about CPUID data stability
should be already using).

-- 
Eduardo

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Eduardo Habkost]

On Thu, Jun 05, 2014 at 06:40:25PM +0200, Alexander Graf wrote:
> 
> On 05.06.14 18:26, Paolo Bonzini wrote:
> >Il 05/06/2014 18:24, Alexander Graf ha scritto:
> >>
> >>On 05.06.14 18:12, Eduardo Habkost wrote:
> >>>This implements GET_SUPPORTED_CPUID support using an explicit option
> >>>for it:
> >>>"allow-emulation". We don't want any emulated feature to be enabled by
> >>>accident,
> >>>so they will be enabled only if the user explicitly wants to
> >>>allow them.
> >>
> >>So is this an all-or-nothing approach? I would really prefer to override
> >>individual bits.
> >
> >You can still disable them with "cpu foo,-movbe,allow-emulation".
> >
> >>Also, I don't think the line "emulated" is the right one to draw. We
> >>"emulate" SVM or VMX too, but still enable them by default as soon as we
> >>think they're ready enough.
> >
> >Well, I disagreed with the whole KVM_GET_EMULATED_CPUID concept for
> >MOVBE too for example.  It seemed overengineered to me, sooner or
> >later we might graduate MOVBE out of KVM_GET_EMULATED_CPUID as
> >well.
> >
> >However, for MONITOR/MWAIT it makes some sense.
> 
> I honestly think what we want for MONITOR/MWAIT is a cpuid-override bit.
> 
>   cpuid = user_specified_cpuids();
>   cpuid &= kvm_capable_cpuids()
>   cpuid |= user_override_cpuids();
> 
>   kvm_set_cpuid(cpuid);

Note that the above is not how it works today. It works this way:

    requested_cpuid = cpu_model_cpuids(cpu_model);
    requested_cpuid |= user_enabled_flags();
    requested_cpuid &= ~user_disabled_flags();
    possible_cpuid = cpuid & kvm_capable_cpuids();
    if (possible_cpuid != requested_cpuid) {
          if (check || enforce) fprintf(stderr, "some features are not available. help!");
          if (enforce) exit(1);
          cpu.filtered_features = cpuid & ~possible_cpuid;
    }
    cpu.cpuids = possible_cpuid;

This patch only affects the result of kvm_capable_cpuids(), only.

The semantics of a "-cpu" option is completely defined by
requested_cpuid. And this is not changing. The only way you can have a
different result depending on GET_SUPPORTED_CPUID or GET_EMULATE_CPUID,
is if you are _not_ using the "enforce" flag. But you should be using
it, if you care about stable CPUID data.

> 
> If the user knows what he's doing, he can set the force bit. If the
> kernel happens to emulate that instruction, he's happy. If the kernel
> doesn't emulate it, it will fail and he will realize that he did
> something stupid.

If the user knows what he's doing, he simply sets allow-emulation, which
will _not_ affect requested_cpuid.

> 
> But ok, we do have this awesome GET_EMULATE_CPUID ioctl now, so we
> can as well use it - even though I consider it superfluous:
> 
>   cpuid = user_specified_cpuids();
>   cpuid &= kvm_capable_cpuids()
>   cpuid |= user_override_cpuids() & kvm_emulated_cpuid();
> 
>   kvm_set_cpuid(cpuid);
> 
> but enabling all experimental features inside KVM just because we
> want one or two of them is very counter-intuitive. Imagine we'd
> introduce emulation support for AVX. Suddenly allow-emulation (which
> I'd need for Mac OS X 10.5) would enable AVX as well which I really
> don't want enabled.

If allow-emulation can suddenly enable a feature you don't want, your
command-line is already broken because changes to GET_SUPPORTED_CPUID
could also break your setup.

> 
> Also, while we can't change the ioctl name anymore, please let's use
> "experimental" rather than "emulated" as the name everywhere. Maybe
> we'll never bump individual features from experimental to fully
> supported, but there's no reason we wouldn't have emulated features
> that are not part of this bitmap and there's no reason we wouldn't
> have real hardware features that are not ready yet and part of this
> bitmap.

Agreed. The main point of GET_EMULATED_CPUID is to report features we
will never want to enable accidentally. Calling them "experimental"
makes sense to me.

-- 
Eduardo

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Paolo Bonzini]

Il 05/06/2014 19:17, Eduardo Habkost ha scritto:
> 
> If you don't want MONITOR/MWAIT you shouldn't be using a CPU model
> containing MONITOR/MWAIT in the first place. If you use "-cpu
> somethingWithMONITOR", that means you are already asking QEMU for a CPU
> with MONITOR. If you were not getting MONITOR before using
> allow-emulation, that means you were not using the "enforce" flag, which
> you should be using, already.

Except that many cpus don't work with enforce:

$ /usr/libexec/qemu-kvm -cpu core2duo,enforce
warning: host doesn't support requested feature: CPUID.01H:EDX.ds [bit 21]
warning: host doesn't support requested feature: CPUID.01H:EDX.acpi [bit 22]
warning: host doesn't support requested feature: CPUID.01H:EDX.ht [bit 28]
warning: host doesn't support requested feature: CPUID.01H:EDX.tm [bit 29]
warning: host doesn't support requested feature: CPUID.01H:EDX.pbe [bit 31]
warning: host doesn't support requested feature: CPUID.01H:ECX.dtes64 [bit 2]
warning: host doesn't support requested feature: CPUID.01H:ECX.monitor [bit 3]
warning: host doesn't support requested feature: CPUID.01H:ECX.ds_cpl [bit 4]
warning: host doesn't support requested feature: CPUID.01H:ECX.vmx [bit 5]
warning: host doesn't support requested feature: CPUID.01H:ECX.est [bit 7]
warning: host doesn't support requested feature: CPUID.01H:ECX.tm2 [bit 8]
warning: host doesn't support requested feature: CPUID.01H:ECX.xtpr [bit 14]
warning: host doesn't support requested feature: CPUID.01H:ECX.pdcm [bit 15]

Like VMX above, some AMD models require nested SVM to be enabled:

$ /usr/libexec/qemu-kvm -cpu phenom,enforce
warning: host doesn't support requested feature: CPUID.01H:EDX.ht [bit 28]
warning: host doesn't support requested feature: CPUID.01H:ECX.monitor [bit 3]
warning: host doesn't support requested feature: CPUID.80000001H:ECX.svm [bit 2]
warning: host doesn't support requested feature: CPUID.8000000AH:EDX.npt [bit 0]
warning: host doesn't support requested feature: CPUID.8000000AH:EDX.lbrv [bit 1]

(actually, even with nested SVM, LBR virtualization is not supported in L2 guests)

Testing some of the bits does not make sense, for example HT.  Some others will
never be supported (TM/TM2, ACPI, PBE, xTPR, ...).

The set of CPUs that work with enforce is pretty much what you tested (the
Opteron_G* models and some of the code-named Intel chips).

Paolo

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Paolo Bonzini]

Il 05/06/2014 19:19, Eduardo Habkost ha scritto:
> On Thu, Jun 05, 2014 at 06:57:57PM +0200, Paolo Bonzini wrote:
>> Il 05/06/2014 18:54, Alexander Graf ha scritto:
>>>>
>>>> What about:
>>>>
>>>> - letting "-cpu foo,+emulatedfeature" just work
>>>>
>>>> - adding emulated=yes that blindly enables all emulated features
>>>>
>>>> - making "-cpu ...,check" prints a warning for emulated features
>>>> unless emulated=yes
>>>
>>> How about we remove the emulated=yes from this list? Then I'm happy :).
>>
>> So:
>>
>> - "-cpu foo" doesn't enable any emulated feature
>
> What if "foo" already has movbe in the CPU model definition?

It will be disabled.

>>
>> - "-cpu foo,+movbe" does
>
> What if I want movbe enabled if and only if it is _not_ emulated?

Pick a CPU model that has it.

> The whole point here is to never ever ever enable an emulated feature
> unless it was explicitly what the user wanted.

"+foo" could be enough.

> "nice and descriptive message" needs to be better specified. Messages on
> stderr are useless for management software.

I'm not sure this feature is for management software users.

Paolo

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Eduardo Habkost]

On Thu, Jun 05, 2014 at 06:58:17PM +0200, Alexander Graf wrote:
> 
> On 05.06.14 18:52, Paolo Bonzini wrote:
> >Il 05/06/2014 18:45, Alexander Graf ha scritto:
> >>>>
> >>>
> >>>Only if you were using "-cpu somethingThatHasAVX", though, no?
> >>
> >>Yes. The same argument goes the other way around. I want to use AVX
> >>emulation, do "allow-emulation" and suddenly I get MONITOR/MWAIT
> >>emulation.
> >
> >What about:
> >
> >- letting "-cpu foo,+emulatedfeature" just work
> >
> >- adding emulated=yes that blindly enables all emulated features
> >
> >- making "-cpu ...,check" prints a warning for emulated features
> >unless emulated=yes
> 
> So:
> 
>   -cpu foo,+emulatedFeature just works

The problem with this is:

 * -cpu foo,+emulatedFeature,enforce MUST fail. We don't want to enable
   emulated/experimental features by accident, even if they appear in
   the command-line expliclty.
 * -cpu foo,+emulatedFeature (no "enforce" flag) MUST remove remove the
   feature and report it on the "filtered-features" property, because
   that's the only API available for management to check if a feature is
   not available on GET_SUPPORTED_CPUID.

That's why I suggest that the only way to enable emulatedFeature be
using "allow-emulation=yes" explicitly on the command-line IN ADDITION
to already having the feature included in the CPU model definition or in
explicitly in the command-line.

(or "allow-experimental-features", or whatever name we choose)

> 
>   -cpu foo,+notEmulatedFeature still sets the CPUID bit for that feature

That's OK, but (I assume) you mean notEmulatedFeature is on
GET_SUPPORTED_CPUID.

Detailing the requirements:

 * "-cpu foo,+SomeFeature" MUST NOT enable SomeFeature unless it is
   present on GET_SUPPORTED_CPUID.
 * "-cpu foo,+SomeFeature,enforce" MUST abort if the feature is not
   present on GET_SUPPORTED_CPUID.
 * "-cpu foo,+SomeFeature" MUST report SomeFeature on
   "filtered-features", so management knows the feature is not set on
   GET_SUPPORTED_CPUID.

The items above are already part of the semantics of "-cpu" and if we
change it, we risk defeating the whole purpose of GET_EMULATED_CPUID in
the first place.

> 
>   -cpu foo,check prints warnings for all cpuid bits not in the
> "allowed" bitmap. It prints different warnings depending on whether
> the bit is in "emulated" or not

That part makes sense. And we may also report GET_EMULATED_CPUID
features on an "emulated-features" property, so management can get that
information in a machine-friendly way.

-- 
Eduardo

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Eduardo Habkost]

On Thu, Jun 05, 2014 at 07:38:49PM +0200, Paolo Bonzini wrote:
> Il 05/06/2014 19:17, Eduardo Habkost ha scritto:
> > 
> > If you don't want MONITOR/MWAIT you shouldn't be using a CPU model
> > containing MONITOR/MWAIT in the first place. If you use "-cpu
> > somethingWithMONITOR", that means you are already asking QEMU for a CPU
> > with MONITOR. If you were not getting MONITOR before using
> > allow-emulation, that means you were not using the "enforce" flag, which
> > you should be using, already.
> 
> Except that many cpus don't work with enforce:
> 
> $ /usr/libexec/qemu-kvm -cpu core2duo,enforce
> warning: host doesn't support requested feature: CPUID.01H:EDX.ds [bit 21]
> warning: host doesn't support requested feature: CPUID.01H:EDX.acpi [bit 22]
> warning: host doesn't support requested feature: CPUID.01H:EDX.ht [bit 28]
> warning: host doesn't support requested feature: CPUID.01H:EDX.tm [bit 29]
> warning: host doesn't support requested feature: CPUID.01H:EDX.pbe [bit 31]
> warning: host doesn't support requested feature: CPUID.01H:ECX.dtes64 [bit 2]
> warning: host doesn't support requested feature: CPUID.01H:ECX.monitor [bit 3]
> warning: host doesn't support requested feature: CPUID.01H:ECX.ds_cpl [bit 4]
> warning: host doesn't support requested feature: CPUID.01H:ECX.vmx [bit 5]
> warning: host doesn't support requested feature: CPUID.01H:ECX.est [bit 7]
> warning: host doesn't support requested feature: CPUID.01H:ECX.tm2 [bit 8]
> warning: host doesn't support requested feature: CPUID.01H:ECX.xtpr [bit 14]
> warning: host doesn't support requested feature: CPUID.01H:ECX.pdcm [bit 15]

Probably this means the CPU model definition have AMD alises that
shouldn't be there (as they are added automatically when CPU vendor is
AMD). A bug in the CPU model. You still need "enforce" if you care about
stable CPUID data.

> 
> Like VMX above, some AMD models require nested SVM to be enabled:
> 
> $ /usr/libexec/qemu-kvm -cpu phenom,enforce
> warning: host doesn't support requested feature: CPUID.01H:EDX.ht [bit 28]
> warning: host doesn't support requested feature: CPUID.01H:ECX.monitor [bit 3]
> warning: host doesn't support requested feature: CPUID.80000001H:ECX.svm [bit 2]
> warning: host doesn't support requested feature: CPUID.8000000AH:EDX.npt [bit 0]
> warning: host doesn't support requested feature: CPUID.8000000AH:EDX.lbrv [bit 1]
> 
> (actually, even with nested SVM, LBR virtualization is not supported in L2 guests)
> 
> Testing some of the bits does not make sense, for example HT.  Some others will
> never be supported (TM/TM2, ACPI, PBE, xTPR, ...).

If a CPU model doesn't work with enforce, that means that _without_
enforce, you already risk getting CPUID data suddenly change on
migration (if GET_SUPPORTED_CPUID changes). Isn't that exactly the
problem Alex was complaining about?

If existing CPU models have not very useful defaults that allow them to
be useful with "enforce", we may change them. But is not a reason to
drop "enforce", or you risk getting even worse problems.

> 
> The set of CPUs that work with enforce is pretty much what you tested (the
> Opteron_G* models and some of the code-named Intel chips).

So we need to fix this. Just like we fixed the MONITOR flag issue on KVM
mode recently.

-- 
Eduardo

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Eduardo Habkost]

On Thu, Jun 05, 2014 at 07:39:42PM +0200, Paolo Bonzini wrote:
> Il 05/06/2014 19:19, Eduardo Habkost ha scritto:
> >On Thu, Jun 05, 2014 at 06:57:57PM +0200, Paolo Bonzini wrote:
> >>Il 05/06/2014 18:54, Alexander Graf ha scritto:
> >>>>
> >>>>What about:
> >>>>
> >>>>- letting "-cpu foo,+emulatedfeature" just work
> >>>>
> >>>>- adding emulated=yes that blindly enables all emulated features
> >>>>
> >>>>- making "-cpu ...,check" prints a warning for emulated features
> >>>>unless emulated=yes
> >>>
> >>>How about we remove the emulated=yes from this list? Then I'm happy :).
> >>
> >>So:
> >>
> >>- "-cpu foo" doesn't enable any emulated feature
> >
> >What if "foo" already has movbe in the CPU model definition?
> 
> It will be disabled.

I don't disagree with that. But not that if it will be disabled, that
means "-cpu foo,enforce" will abort.

But note that if you use "-cpu foo" without enforce, that means you can
suddenly get movbe enabled once it gets included on GET_SUPPORTED_CPUID.

So, if you care about predictable CPUID results and you want to enable
an emulated/experimental feature, you have to do two things:

 1) Make sure your setup works with "enforce", so you know you will
    never get any feature suddenly and silently enabled.
 2) Add "+feature,allow-emulation=yes" to the command-line, keep
    "enforce" and you will _not_ get any other experimental feature
    suddenly enabled (because now you are using "enforce").

> 
> >>
> >>- "-cpu foo,+movbe" does
> >
> >What if I want movbe enabled if and only if it is _not_ emulated?
> 
> Pick a CPU model that has it.
> 
> >The whole point here is to never ever ever enable an emulated feature
> >unless it was explicitly what the user wanted.
> 
> "+foo" could be enough.

We shouldn't do that, or we risk enabling features by accident and
defeating the whole purpose of GET_EMULATED_CPUID.

The current semantics of "+foo" is "I want FOO, but only if it's on
GET_SUPPORTED_CPUID". We shouldn't break it.

> 
> >"nice and descriptive message" needs to be better specified. Messages on
> >stderr are useless for management software.
> 
> I'm not sure this feature is for management software users.

True. I am just worried about breaking current semantics that is used by
management software.

-- 
Eduardo

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Eduardo Habkost]

Sorry for following the discussion backwards, but I see now that you
started with a proposal that would cover both cases (the one you care
about, and the one I care about), make both of us happy, but it was lost
in favour of other suggestions I disagreed with:

On Thu, Jun 05, 2014 at 06:24:22PM +0200, Alexander Graf wrote:
> 
> On 05.06.14 18:12, Eduardo Habkost wrote:
> >This implements GET_SUPPORTED_CPUID support using an explicit option for it:
> >"allow-emulation". We don't want any emulated feature to be enabled by accident,
> >so they will be enabled only if the user explicitly wants to allow them.
> 
> So is this an all-or-nothing approach? I would really prefer to
> override individual bits.

It is not an all-or-nothing approach if you use "enforce", but now I see
that you care about use cases of people that are _not_ using "enforce"
(which I strongly recommend against, but we can try to cover those use
cases, as well).

> 
> Also, I don't think the line "emulated" is the right one to draw. We
> "emulate" SVM or VMX too, but still enable them by default as soon as
> we think they're ready enough.

Agreed. Let's call it "experimental".

> 
> So could we add a new flag specifier instead? Today we have -flag and
> +flag. How about *flag to "force enable if the kernel can handle it,
> but doesn't do so by default"?

Having an _additional_ way to allow emulation of specific flags, without
changing the semantics of "+flag" would make both of us happy.

But I believe we should look for a syntax that won't require special
symbols, but just plain option names. Because the "-cpu" command-line
options are simply translated to object_property_set() calls. "+flag" is
legacy format, and we should move to use "flag=on" instead.

So, what about, instead of "*foo", having "experimental-foo=on" or
"force-foo=on"?

I was going to suggest "foo=force", but that would require changing the
data type of the properties from bool to string.

-- 
Eduardo

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Eduardo Habkost]

Sorry for replying to my own message, but I believe we can now summarize
a possible solution that makes everybody happy, and the plans for it:

On Thu, Jun 05, 2014 at 03:02:53PM -0300, Eduardo Habkost wrote:
> On Thu, Jun 05, 2014 at 07:39:42PM +0200, Paolo Bonzini wrote:
> > Il 05/06/2014 19:19, Eduardo Habkost ha scritto:
> > >On Thu, Jun 05, 2014 at 06:57:57PM +0200, Paolo Bonzini wrote:
> > >>Il 05/06/2014 18:54, Alexander Graf ha scritto:
> > >>>>
> > >>>>What about:
> > >>>>
> > >>>>- letting "-cpu foo,+emulatedfeature" just work
> > >>>>
> > >>>>- adding emulated=yes that blindly enables all emulated features
> > >>>>
> > >>>>- making "-cpu ...,check" prints a warning for emulated features
> > >>>>unless emulated=yes
> > >>>
> > >>>How about we remove the emulated=yes from this list? Then I'm happy :).
> > >>
> > >>So:
> > >>
> > >>- "-cpu foo" doesn't enable any emulated feature
> > >
> > >What if "foo" already has movbe in the CPU model definition?
> > 
> > It will be disabled.
> 
> I don't disagree with that. But not that if it will be disabled, that
> means "-cpu foo,enforce" will abort.

Typo. I meant "note that".

> 
> But note that if you use "-cpu foo" without enforce, that means you can
> suddenly get movbe enabled once it gets included on GET_SUPPORTED_CPUID.
> 
> So, if you care about predictable CPUID results and you want to enable
> an emulated/experimental feature, you have to do two things:
> 
>  1) Make sure your setup works with "enforce", so you know you will
>     never get any feature suddenly and silently enabled.
>  2) Add "+feature,allow-emulation=yes" to the command-line, keep
>     "enforce" and you will _not_ get any other experimental feature
>     suddenly enabled (because now you are using "enforce").

So, the above would cover the use cases I was thinking about. But I
understand you have a different use case and you want to avoid
GET_EMULATED_CPUID-related surprises even if not using "enforce". For
that, you need a more fine-tuned solution using "*feature" or
"feature=force". (Personally, I prefer "feature=force" instead of
"*feature").

Implementing "feature=force" would be more cleanly implemented after we
introduce the CPU feature properties, which we have been trying to
include for 4 or 5 QEMU releases, and it was never merged.

In the meantime, we could:

 * Include the less fine-tuned "allow-emulation" (or
   "allow-experimental-features") option, which is implemented by this
   series, for people who use "enforce" and/or don't care too much about
   getting other experimental features enabled.
 * Wait until somebody implements "feature=force".

Personally, I don't care which plan we follow, as I am not an user of
GET_EMULATED_CPUID. I will leave that decision for the QEMU maintainers
and other developers.

-- 
Eduardo

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Borislav Petkov]

On Thu, Jun 05, 2014 at 04:12:08PM -0300, Eduardo Habkost wrote:
> In the meantime, we could:
> 
>  * Include the less fine-tuned "allow-emulation" (or
>    "allow-experimental-features") option, which is implemented by this
>    series, for people who use "enforce" and/or don't care too much about
>    getting other experimental features enabled.
>  * Wait until somebody implements "feature=force".

What are you going to do with "allow-emulation" after this? It will
become an API and you'll have to support it.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

On 06/05/2014 01:24 PM, Borislav Petkov wrote:
> On Thu, Jun 05, 2014 at 04:12:08PM -0300, Eduardo Habkost wrote:
>> In the meantime, we could:
>>
>>  * Include the less fine-tuned "allow-emulation" (or
>>    "allow-experimental-features") option, which is implemented by this
>>    series, for people who use "enforce" and/or don't care too much about
>>    getting other experimental features enabled.
>>  * Wait until somebody implements "feature=force".
> 
> What are you going to do with "allow-emulation" after this? It will
> become an API and you'll have to support it.

If you're worried about not needing the option forever, name it
x-allow-emulation; we've already documented that anything with x- prefix
is fair game for subsequent removal.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Eduardo Habkost]

On Thu, Jun 05, 2014 at 01:45:06PM -0600, Eric Blake wrote:
> On 06/05/2014 01:24 PM, Borislav Petkov wrote:
> > On Thu, Jun 05, 2014 at 04:12:08PM -0300, Eduardo Habkost wrote:
> >> In the meantime, we could:
> >>
> >>  * Include the less fine-tuned "allow-emulation" (or
> >>    "allow-experimental-features") option, which is implemented by this
> >>    series, for people who use "enforce" and/or don't care too much about
> >>    getting other experimental features enabled.
> >>  * Wait until somebody implements "feature=force".
> > 
> > What are you going to do with "allow-emulation" after this? It will
> > become an API and you'll have to support it.
> 
> If you're worried about not needing the option forever, name it
> x-allow-emulation; we've already documented that anything with x- prefix
> is fair game for subsequent removal.

Personally, I wouldn't mind about supporting it forever, as its
implementation is very simple. But "x-allow-emulation" would be a good
way to make GET_EMULATED_CPUID available for developers who want to test
it, without any commitment to a specific API.

-- 
Eduardo

[Qemu-devel] [RFC 2/2 v2] target-i386: Add "x-allow-emulation" X86CPU property

[Eduardo Habkost]

The new option will allow slow emulated features (the ones returned by
GET_EMULATED_CPUID) to be enabled. We don't want to allow them to be
enabled by accident, so they will be enabled only if emulation is
explicitly allowed by the user.

Use "x-" prefix on the property name, to document that it is not
supposed to be supported forever, and intended for developers who want
to test GET_EMULATED_CPUID.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
 * Rename property to x-allow-emulation
---
 target-i386/cpu-qom.h |  3 +++
 target-i386/cpu.c     | 18 ++++++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 385b81f..831f7bf 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -74,6 +74,8 @@ typedef struct X86CPUClass {
  * @migratable: If set, only migratable flags will be accepted when "enforce"
  * mode is used, and only migratable flags will be included in the "host"
  * CPU model.
+ * @allow_emulation: If set, accelerator-specific code will allow emulated
+ * features to be enabled.
  *
  * An x86 CPU.
  */
@@ -91,6 +93,7 @@ typedef struct X86CPU {
     bool check_cpuid;
     bool enforce_cpuid;
     bool migratable;
+    bool allow_emulation;
 
     /* if true the CPUID code directly forward host cache leaves to the guest */
     bool cache_info_passthrough;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1395473..cf6ab59 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1280,7 +1280,8 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void *data)
 }
 
 static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
-                                                   bool migratable_only);
+                                                   bool migratable_only,
+                                                   bool allow_emulation);
 
 static void host_x86_cpu_initfn(Object *obj)
 {
@@ -1297,7 +1298,8 @@ static void host_x86_cpu_initfn(Object *obj)
 
     for (w = 0; w < FEATURE_WORDS; w++) {
         env->features[w] =
-            x86_cpu_get_supported_feature_word(w, cpu->migratable);
+            x86_cpu_get_supported_feature_word(w, cpu->migratable,
+                                                  cpu->allow_emulation);
     }
     object_property_set_bool(OBJECT(cpu), true, "pmu", &error_abort);
 }
@@ -1887,7 +1889,8 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
 }
 
 static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
-                                                   bool migratable_only)
+                                                   bool migratable_only,
+                                                   bool allow_emulation)
 {
     FeatureWordInfo *wi = &feature_word_info[w];
     uint32_t r;
@@ -1896,6 +1899,11 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
         r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
                                                     wi->cpuid_ecx,
                                                     wi->cpuid_reg);
+        if (allow_emulation) {
+            r |= kvm_arch_get_emulated_cpuid(kvm_state,  wi->cpuid_eax,
+                                                         wi->cpuid_ecx,
+                                                         wi->cpuid_reg);
+        }
     } else if (tcg_enabled()) {
         r = wi->tcg_features;
     } else {
@@ -1920,7 +1928,8 @@ static int x86_cpu_filter_features(X86CPU *cpu)
 
     for (w = 0; w < FEATURE_WORDS; w++) {
         uint32_t host_feat =
-            x86_cpu_get_supported_feature_word(w, cpu->migratable);
+            x86_cpu_get_supported_feature_word(w, cpu->migratable,
+                                                  cpu->allow_emulation);
         uint32_t requested_features = env->features[w];
         env->features[w] &= host_feat;
         cpu->filtered_features[w] = requested_features & ~env->features[w];
@@ -2854,6 +2863,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("hv-time", X86CPU, hyperv_time, false),
     DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, false),
     DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
+    DEFINE_PROP_BOOL("x-allow-emulation", X86CPU, allow_emulation, true),
     DEFINE_PROP_END_OF_LIST()
 };
 
-- 
1.9.0

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Alexander Graf]

On 05.06.14 19:48, Eduardo Habkost wrote:
> On Thu, Jun 05, 2014 at 06:58:17PM +0200, Alexander Graf wrote:
>> On 05.06.14 18:52, Paolo Bonzini wrote:
>>> Il 05/06/2014 18:45, Alexander Graf ha scritto:
>>>>> Only if you were using "-cpu somethingThatHasAVX", though, no?
>>>> Yes. The same argument goes the other way around. I want to use AVX
>>>> emulation, do "allow-emulation" and suddenly I get MONITOR/MWAIT
>>>> emulation.
>>> What about:
>>>
>>> - letting "-cpu foo,+emulatedfeature" just work
>>>
>>> - adding emulated=yes that blindly enables all emulated features
>>>
>>> - making "-cpu ...,check" prints a warning for emulated features
>>> unless emulated=yes
>> So:
>>
>>    -cpu foo,+emulatedFeature just works
> The problem with this is:
>
>   * -cpu foo,+emulatedFeature,enforce MUST fail. We don't want to enable
>     emulated/experimental features by accident, even if they appear in
>     the command-line expliclty.
>   * -cpu foo,+emulatedFeature (no "enforce" flag) MUST remove remove the
>     feature and report it on the "filtered-features" property, because
>     that's the only API available for management to check if a feature is
>     not available on GET_SUPPORTED_CPUID.
>
> That's why I suggest that the only way to enable emulatedFeature be
> using "allow-emulation=yes" explicitly on the command-line IN ADDITION
> to already having the feature included in the CPU model definition or in
> explicitly in the command-line.
>
> (or "allow-experimental-features", or whatever name we choose)
>
>>    -cpu foo,+notEmulatedFeature still sets the CPUID bit for that feature
> That's OK, but (I assume) you mean notEmulatedFeature is on
> GET_SUPPORTED_CPUID.
>
> Detailing the requirements:
>
>   * "-cpu foo,+SomeFeature" MUST NOT enable SomeFeature unless it is
>     present on GET_SUPPORTED_CPUID.
>   * "-cpu foo,+SomeFeature,enforce" MUST abort if the feature is not
>     present on GET_SUPPORTED_CPUID.
>   * "-cpu foo,+SomeFeature" MUST report SomeFeature on
>     "filtered-features", so management knows the feature is not set on
>     GET_SUPPORTED_CPUID.
>
> The items above are already part of the semantics of "-cpu" and if we
> change it, we risk defeating the whole purpose of GET_EMULATED_CPUID in
> the first place.
>
>>    -cpu foo,check prints warnings for all cpuid bits not in the
>> "allowed" bitmap. It prints different warnings depending on whether
>> the bit is in "emulated" or not
> That part makes sense. And we may also report GET_EMULATED_CPUID
> features on an "emulated-features" property, so management can get that
> information in a machine-friendly way.

I personally like the feature=force way of specifying forcefully enabled 
cpuid bits. Whether it's in GET_EMULATED_CPUID doesn't matter really. 
Only "check" should ever care about that bitmap.

But can we drop the EMULATED name somehow? Can we rename [1] the ioctl 
to say GET_UNSUPPORTED_CPUID or something along those lines? The name is 
just a really really bad pick.


Alex

[1] rename obviously means "introduce a new name with the same ioctl 
number and keep the old name around for legacy compatibility reasons"

Re: [Qemu-devel] [RFC 2/2 v2] target-i386: Add "x-allow-emulation" X86CPU property

[Alexander Graf]

On 05.06.14 21:57, Eduardo Habkost wrote:
> The new option will allow slow emulated features (the ones returned by
> GET_EMULATED_CPUID) to be enabled. We don't want to allow them to be
> enabled by accident, so they will be enabled only if emulation is
> explicitly allowed by the user.
>
> Use "x-" prefix on the property name, to document that it is not
> supposed to be supported forever, and intended for developers who want
> to test GET_EMULATED_CPUID.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

ack if you do s/emulation/experimental/g throughout the patch :).


Alex

> ---
> Changes v1 -> v2:
>   * Rename property to x-allow-emulation
> ---
>   target-i386/cpu-qom.h |  3 +++
>   target-i386/cpu.c     | 18 ++++++++++++++----
>   2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> index 385b81f..831f7bf 100644
> --- a/target-i386/cpu-qom.h
> +++ b/target-i386/cpu-qom.h
> @@ -74,6 +74,8 @@ typedef struct X86CPUClass {
>    * @migratable: If set, only migratable flags will be accepted when "enforce"
>    * mode is used, and only migratable flags will be included in the "host"
>    * CPU model.
> + * @allow_emulation: If set, accelerator-specific code will allow emulated
> + * features to be enabled.
>    *
>    * An x86 CPU.
>    */
> @@ -91,6 +93,7 @@ typedef struct X86CPU {
>       bool check_cpuid;
>       bool enforce_cpuid;
>       bool migratable;
> +    bool allow_emulation;
>   
>       /* if true the CPUID code directly forward host cache leaves to the guest */
>       bool cache_info_passthrough;
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 1395473..cf6ab59 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1280,7 +1280,8 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void *data)
>   }
>   
>   static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
> -                                                   bool migratable_only);
> +                                                   bool migratable_only,
> +                                                   bool allow_emulation);
>   
>   static void host_x86_cpu_initfn(Object *obj)
>   {
> @@ -1297,7 +1298,8 @@ static void host_x86_cpu_initfn(Object *obj)
>   
>       for (w = 0; w < FEATURE_WORDS; w++) {
>           env->features[w] =
> -            x86_cpu_get_supported_feature_word(w, cpu->migratable);
> +            x86_cpu_get_supported_feature_word(w, cpu->migratable,
> +                                                  cpu->allow_emulation);
>       }
>       object_property_set_bool(OBJECT(cpu), true, "pmu", &error_abort);
>   }
> @@ -1887,7 +1889,8 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>   }
>   
>   static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
> -                                                   bool migratable_only)
> +                                                   bool migratable_only,
> +                                                   bool allow_emulation)
>   {
>       FeatureWordInfo *wi = &feature_word_info[w];
>       uint32_t r;
> @@ -1896,6 +1899,11 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
>           r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
>                                                       wi->cpuid_ecx,
>                                                       wi->cpuid_reg);
> +        if (allow_emulation) {
> +            r |= kvm_arch_get_emulated_cpuid(kvm_state,  wi->cpuid_eax,
> +                                                         wi->cpuid_ecx,
> +                                                         wi->cpuid_reg);
> +        }
>       } else if (tcg_enabled()) {
>           r = wi->tcg_features;
>       } else {
> @@ -1920,7 +1928,8 @@ static int x86_cpu_filter_features(X86CPU *cpu)
>   
>       for (w = 0; w < FEATURE_WORDS; w++) {
>           uint32_t host_feat =
> -            x86_cpu_get_supported_feature_word(w, cpu->migratable);
> +            x86_cpu_get_supported_feature_word(w, cpu->migratable,
> +                                                  cpu->allow_emulation);
>           uint32_t requested_features = env->features[w];
>           env->features[w] &= host_feat;
>           cpu->filtered_features[w] = requested_features & ~env->features[w];
> @@ -2854,6 +2863,7 @@ static Property x86_cpu_properties[] = {
>       DEFINE_PROP_BOOL("hv-time", X86CPU, hyperv_time, false),
>       DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, false),
>       DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
> +    DEFINE_PROP_BOOL("x-allow-emulation", X86CPU, allow_emulation, true),
>       DEFINE_PROP_END_OF_LIST()
>   };
>   

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Borislav Petkov]

On Fri, Jun 06, 2014 at 12:24:26AM +0200, Alexander Graf wrote:
> But can we drop the EMULATED name somehow? Can we rename [1] the ioctl
> to say GET_UNSUPPORTED_CPUID or something along those lines? The name
> is just a really really bad pick.

What do you mean, a "bad pick" :-P? I added extra care in naming that
functionality what it is - bitfield in CPUID format of *emulated*
features. Unsupported is wrong too - we do support them if we enable
them explicitly. :-)

How about GET_NOT_REALLY_FAST_BUT_STILL_NOT_FAST_ENOUGH_AS_IN_HW_FAST_CPUID?

:-P

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Eduardo Habkost]

On Fri, Jun 06, 2014 at 03:21:04AM +0200, Borislav Petkov wrote:
> On Fri, Jun 06, 2014 at 12:24:26AM +0200, Alexander Graf wrote:
> > But can we drop the EMULATED name somehow? Can we rename [1] the ioctl
> > to say GET_UNSUPPORTED_CPUID or something along those lines? The name
> > is just a really really bad pick.
> 
> What do you mean, a "bad pick" :-P? I added extra care in naming that
> functionality what it is - bitfield in CPUID format of *emulated*
> features. Unsupported is wrong too - we do support them if we enable
> them explicitly. :-)
> 
> How about GET_NOT_REALLY_FAST_BUT_STILL_NOT_FAST_ENOUGH_AS_IN_HW_FAST_CPUID?

IMO, "emulated" on the kernel interface is good, because it describe
what it is. Deciding which emulated features are "experimental" or "good
enough to be enabled implicitly even if emulated" is policy for
userspace to decide.

"allow-experimental" is being mapped to GET_EMULATED_CPUID initially
only because _by default_ the GET_EMULATED_CPUID-only features won't be
enabled implicitly unless forced. But if one day we decide that
emulation is good enough for a specific feature, we can make
kvm_arch_get_supported_cpuid() return it even if it is present only on
the GET_EMULATED_CPUID bitmap. Even if that decision depends on a
specific implementation of that feature, the kernel can report that
using KVM capabilities (to be checked by kvm_arch_get_supported_cpuid(),
like we already do for tsc-deadline).

-- 
Eduardo

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Alexander Graf]

On 06.06.14 04:37, Eduardo Habkost wrote:
> On Fri, Jun 06, 2014 at 03:21:04AM +0200, Borislav Petkov wrote:
>> On Fri, Jun 06, 2014 at 12:24:26AM +0200, Alexander Graf wrote:
>>> But can we drop the EMULATED name somehow? Can we rename [1] the ioctl
>>> to say GET_UNSUPPORTED_CPUID or something along those lines? The name
>>> is just a really really bad pick.
>> What do you mean, a "bad pick" :-P? I added extra care in naming that
>> functionality what it is - bitfield in CPUID format of *emulated*
>> features. Unsupported is wrong too - we do support them if we enable
>> them explicitly. :-)
>>
>> How about GET_NOT_REALLY_FAST_BUT_STILL_NOT_FAST_ENOUGH_AS_IN_HW_FAST_CPUID?
> IMO, "emulated" on the kernel interface is good, because it describe
> what it is. Deciding which emulated features are "experimental" or "good
> enough to be enabled implicitly even if emulated" is policy for
> userspace to decide.
>
> "allow-experimental" is being mapped to GET_EMULATED_CPUID initially
> only because _by default_ the GET_EMULATED_CPUID-only features won't be
> enabled implicitly unless forced. But if one day we decide that
> emulation is good enough for a specific feature, we can make
> kvm_arch_get_supported_cpuid() return it even if it is present only on
> the GET_EMULATED_CPUID bitmap. Even if that decision depends on a
> specific implementation of that feature, the kernel can report that
> using KVM capabilities (to be checked by kvm_arch_get_supported_cpuid(),
> like we already do for tsc-deadline).

Ok, works for me. I still don't see the point in having the bitmap at 
all then when we need to carry separate CAPs for individual features' 
"working" status, but if it makes people happy I'm ok with it.


Alex

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[gleb]

On Thu, Jun 05, 2014 at 06:26:41PM +0200, Paolo Bonzini wrote:
> Il 05/06/2014 18:24, Alexander Graf ha scritto:
> >
> >On 05.06.14 18:12, Eduardo Habkost wrote:
> >>This implements GET_SUPPORTED_CPUID support using an explicit option
> >>for it:
> >>"allow-emulation". We don't want any emulated feature to be enabled by
> >>accident,
> >>so they will be enabled only if the user explicitly wants to allow them.
> >
> >So is this an all-or-nothing approach? I would really prefer to override
> >individual bits.
> 
> You can still disable them with "cpu foo,-movbe,allow-emulation".
> 
> >Also, I don't think the line "emulated" is the right one to draw. We
> >"emulate" SVM or VMX too, but still enable them by default as soon as we
> >think they're ready enough.
> 
> Well, I disagreed with the whole KVM_GET_EMULATED_CPUID concept for MOVBE
> too for example.  It seemed overengineered to me, sooner or later we might
> graduate MOVBE out of KVM_GET_EMULATED_CPUID as well.
Can you remind me what was your argument against
KVM_GET_EMULATED_CPUID? Where do you want to move MOVBE to? Supported
cpuid? That will make some CPU models unreasonably slow on hosts
that do not support MOVBE natively. KVM is not in a busyness of
instruction emulation, yes sometimes there is no choice (IO/real mode),
sometimes emulating a feature is beneficial (x2apic) and sometimes
guest cannot workaround missing feature, so by emulating it you allow
guest functionality that is impossible otherwise (SVM/VMX). MOVBE (and
MONITOR/MWAIT) is not in any of those categories. By forcing emulation
upon unsuspecting guest you force it to use much slower alternative for
what it can do by other means.

> 
> However, for MONITOR/MWAIT it makes some sense.
> 
> Paolo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [Qemu-devel] [RFC 0/2] GET_EMULATED_CPUID support with "allow-emulation" option

[Eduardo Habkost]

On Fri, Jun 06, 2014 at 01:16:00PM +0200, Alexander Graf wrote:
> 
> On 06.06.14 04:37, Eduardo Habkost wrote:
> >On Fri, Jun 06, 2014 at 03:21:04AM +0200, Borislav Petkov wrote:
> >>On Fri, Jun 06, 2014 at 12:24:26AM +0200, Alexander Graf wrote:
> >>>But can we drop the EMULATED name somehow? Can we rename [1] the ioctl
> >>>to say GET_UNSUPPORTED_CPUID or something along those lines? The name
> >>>is just a really really bad pick.
> >>What do you mean, a "bad pick" :-P? I added extra care in naming that
> >>functionality what it is - bitfield in CPUID format of *emulated*
> >>features. Unsupported is wrong too - we do support them if we enable
> >>them explicitly. :-)
> >>
> >>How about GET_NOT_REALLY_FAST_BUT_STILL_NOT_FAST_ENOUGH_AS_IN_HW_FAST_CPUID?
> >IMO, "emulated" on the kernel interface is good, because it describe
> >what it is. Deciding which emulated features are "experimental" or "good
> >enough to be enabled implicitly even if emulated" is policy for
> >userspace to decide.
> >
> >"allow-experimental" is being mapped to GET_EMULATED_CPUID initially
> >only because _by default_ the GET_EMULATED_CPUID-only features won't be
> >enabled implicitly unless forced. But if one day we decide that
> >emulation is good enough for a specific feature, we can make
> >kvm_arch_get_supported_cpuid() return it even if it is present only on
> >the GET_EMULATED_CPUID bitmap. Even if that decision depends on a
> >specific implementation of that feature, the kernel can report that
> >using KVM capabilities (to be checked by kvm_arch_get_supported_cpuid(),
> >like we already do for tsc-deadline).
> 
> Ok, works for me. I still don't see the point in having the bitmap at
> all then when we need to carry separate CAPs for individual features'
> "working" status, but if it makes people happy I'm ok with it.

We won't necessarily need it, it is just an alternative we will always
have.

GET_*_CPUID is just an alternative capability system, which allow CPUID
features to be automatically handled by generic QEMU code on many cases
(instead of always requiring QEMU code changes for new features). But we
can always get back to plain old KVM capabilities on more complex cases
if necessary.

I don't expect features to get promoted from GET_EMULATED_CPUID to
GET_SUPPORTED_CPUID very often, because that's policy that can be
handled by userspace (but that doesn't mean kernel developers can't do
it if they think it is a good idea).

-- 
Eduardo