[Qemu-devel] [PATCH RFC] i386: expose "TCGTCGTCGTCG" in the 0x40000000 CPUID leaf

[Qemu-devel] [PATCH RFC] i386: expose "TCGTCGTCGTCG" in the 0x40000000 CPUID leaf

[Daniel P. Berrange]

Currently when running KVM, we expose "KVMKVMKVM\0\0\0" in
the 0x40000000 CPUID leaf. Other hypervisors (VMWare,
HyperV, Xen, BHyve) all do the same thing, which leaves
TCG as the odd one out.

The CPUID is used by software to detect when running in a
virtual environment and change behaviour in certain ways.
For example, systemd supports a ConditionVirtualization=
setting in unit files. Currently they have to resort to
custom hacks like looking for 'fw-cfg' entry in the
/proc/device-tree file. The virt-what command has the
same hacks & needs.

This change thus proposes a signature TCGTCGTCGTCG to be
reported when running under TCG.

NB1, for reasons I don't undersatnd 'cpu_x86_cpuid' function
clamps the requested CPUID leaf based on env->cpuid_level.
The latter comes from the CPU model definitions, and is
lower than 0x40000000, so the CPUID signature request just
gets turned into a completely different request. eg when
using '-cpu qemu64', the 0x40000000 request from the guest
gets clamped to 0xD and thus returns totally bogus data.
I just removed the clamping code, but someone who understands
this might have a better suggestion.

NB2, for KVM, we added a flag for '-cpu kvm=off' to let you
hide the KVMKVMKVM signature from guests. Presumably we should
add a 'tcg=off' flag for the same reason ?

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 target/i386/cpu.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 13c0985..ac2776e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2626,6 +2626,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     X86CPU *cpu = x86_env_get_cpu(env);
     CPUState *cs = CPU(cpu);
     uint32_t pkg_offset;
+    uint32_t signature[3];
 
     /* test if maximum index reached */
     if (index & 0x80000000) {
@@ -2646,8 +2647,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             }
         }
     } else {
-        if (index > env->cpuid_level)
+        /* XXX this just breaks CPUID turning guest requests
+         * into something totally different, thus returning
+         * garbage data
+         */
+        if (0 && index > env->cpuid_level) {
             index = env->cpuid_level;
+        }
     }
 
     switch(index) {
@@ -2872,6 +2878,14 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         }
         break;
     }
+    case 0x40000000:
+        /* XXX add flag to let us hide this */
+        memcpy(signature, "TCGTCGTCGTCG", 12);
+        *eax = 0x40000001;
+        *ebx = signature[0];
+        *ecx = signature[1];
+        *edx = signature[2];
+        break;
     case 0x80000000:
         *eax = env->cpuid_xlevel;
         *ebx = env->cpuid_vendor1;
-- 
2.9.3

Re: [Qemu-devel] [PATCH RFC] i386: expose "TCGTCGTCGTCG" in the 0x40000000 CPUID leaf

[Eduardo Habkost]

On Thu, May 04, 2017 at 03:56:58PM +0100, Daniel P. Berrange wrote:
> Currently when running KVM, we expose "KVMKVMKVM\0\0\0" in
> the 0x40000000 CPUID leaf. Other hypervisors (VMWare,
> HyperV, Xen, BHyve) all do the same thing, which leaves
> TCG as the odd one out.
> 
> The CPUID is used by software to detect when running in a
> virtual environment and change behaviour in certain ways.
> For example, systemd supports a ConditionVirtualization=
> setting in unit files. Currently they have to resort to
> custom hacks like looking for 'fw-cfg' entry in the
> /proc/device-tree file. The virt-what command has the
> same hacks & needs.
> 
> This change thus proposes a signature TCGTCGTCGTCG to be
> reported when running under TCG.
> 
> NB1, for reasons I don't undersatnd 'cpu_x86_cpuid' function
> clamps the requested CPUID leaf based on env->cpuid_level.

That's how CPUID is specified: Intel SDM says: "If a value
entered for CPUID.EAX is higher than the maximum input value for
basic or extended function for that processor then the data for
the highest basic information leaf is returned."

> The latter comes from the CPU model definitions, and is
> lower than 0x40000000, so the CPUID signature request just
> gets turned into a completely different request. eg when
> using '-cpu qemu64', the 0x40000000 request from the guest
> gets clamped to 0xD and thus returns totally bogus data.
> I just removed the clamping code, but someone who understands
> this might have a better suggestion.
> 

I would rewrite that code as:

    switch (index & 0xF0000000) {
    case 0:
        [...] // check cpuid_level
    case 0x40000000:
        if (index > 0x40000001) {
            /* Not sure what we should do here. Intel and KVM
             * documentation is not explicit about it, but it
             * looks like KVM will return the highest _basic_
             * leaf (env->cpuid_level) on that case.
             */
            index = env->cpuid_level;
        }
        [...]
    case 0x80000000:
        [...] // check cpuid_xlevel
    case 0xC0000000:
        [...] // check cpuid_xlevel2
    default:
       /* Intel documentation states that invalid EAX input will
        * return the same information as EAX=cpuid_level
        * (Intel SDM Vol. 2A - Instruction Set Reference - CPUID)
        */
        index = env->cpuid_level;
    }

> NB2, for KVM, we added a flag for '-cpu kvm=off' to let you
> hide the KVMKVMKVM signature from guests. Presumably we should
> add a 'tcg=off' flag for the same reason ?

I don't like "kvm=off" because it sounds like it disables KVM
completely. "tcg=off" would be misleading as well.

What about a generic "hypervisor-cpuid=off" property?

> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  target/i386/cpu.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 13c0985..ac2776e 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -2626,6 +2626,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>      X86CPU *cpu = x86_env_get_cpu(env);
>      CPUState *cs = CPU(cpu);
>      uint32_t pkg_offset;
> +    uint32_t signature[3];
>  
>      /* test if maximum index reached */
>      if (index & 0x80000000) {
> @@ -2646,8 +2647,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>              }
>          }
>      } else {
> -        if (index > env->cpuid_level)
> +        /* XXX this just breaks CPUID turning guest requests
> +         * into something totally different, thus returning
> +         * garbage data
> +         */
> +        if (0 && index > env->cpuid_level) {
>              index = env->cpuid_level;
> +        }
>      }
>  
>      switch(index) {
> @@ -2872,6 +2878,14 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          }
>          break;
>      }
> +    case 0x40000000:
> +        /* XXX add flag to let us hide this */
> +        memcpy(signature, "TCGTCGTCGTCG", 12);

This shouldn't be seen by any guests if not using TCG. Probably
that will never happen with the current code, but I would make
this conditional on tcg_enabled() just in case.

Note that the CPUID code at kvm_arch_init_vcpu() will ignore the
values here. Probably worth mentioning that in a comment so
people don't get confused.

> +        *eax = 0x40000001;

Should we really return 0x40000001 here, if we still don't have
anything being returned on CPUID[0x40000001]?

If we really want to return 0x40000001 here, an explicit
"case 0x40000001:" line returning all-zeroes would make that
intention clearer.

> +        *ebx = signature[0];
> +        *ecx = signature[1];
> +        *edx = signature[2];
> +        break;
>      case 0x80000000:
>          *eax = env->cpuid_xlevel;
>          *ebx = env->cpuid_vendor1;
> -- 
> 2.9.3
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC] i386: expose "TCGTCGTCGTCG" in the 0x40000000 CPUID leaf

[Paolo Bonzini]

On 04/05/2017 20:42, Eduardo Habkost wrote:
> 
>> NB2, for KVM, we added a flag for '-cpu kvm=off' to let you
>> hide the KVMKVMKVM signature from guests. Presumably we should
>> add a 'tcg=off' flag for the same reason ?
> I don't like "kvm=off" because it sounds like it disables KVM
> completely. "tcg=off" would be misleading as well.
> 
> What about a generic "hypervisor-cpuid=off" property?

Yeah, it should also disable the CPUID_EXT_HYPERVISOR feature, in fact.

Paolo

Re: [Qemu-devel] [PATCH RFC] i386: expose "TCGTCGTCGTCG" in the 0x40000000 CPUID leaf

[Daniel P. Berrange]

On Thu, May 04, 2017 at 03:42:41PM -0300, Eduardo Habkost wrote:
> On Thu, May 04, 2017 at 03:56:58PM +0100, Daniel P. Berrange wrote:
> > Currently when running KVM, we expose "KVMKVMKVM\0\0\0" in
> > the 0x40000000 CPUID leaf. Other hypervisors (VMWare,
> > HyperV, Xen, BHyve) all do the same thing, which leaves
> > TCG as the odd one out.
> > 
> > The CPUID is used by software to detect when running in a
> > virtual environment and change behaviour in certain ways.
> > For example, systemd supports a ConditionVirtualization=
> > setting in unit files. Currently they have to resort to
> > custom hacks like looking for 'fw-cfg' entry in the
> > /proc/device-tree file. The virt-what command has the
> > same hacks & needs.
> > 
> > This change thus proposes a signature TCGTCGTCGTCG to be
> > reported when running under TCG.
> > 
> > NB1, for reasons I don't undersatnd 'cpu_x86_cpuid' function
> > clamps the requested CPUID leaf based on env->cpuid_level.

[snip]

> > NB2, for KVM, we added a flag for '-cpu kvm=off' to let you
> > hide the KVMKVMKVM signature from guests. Presumably we should
> > add a 'tcg=off' flag for the same reason ?
> 
> I don't like "kvm=off" because it sounds like it disables KVM
> completely. "tcg=off" would be misleading as well.
> 
> What about a generic "hypervisor-cpuid=off" property?

Assuming that is intended to obsolete the existing 'kvm' parameter too,
I'm not seeing a way to introduce that in a backwards compatible safe
manner.

eg we add

    DEFINE_PROP_BOOL("hypervisor-cpuid", X86CPU, expose_hypervisor, true),

and in legacy machine types we need to set

    {\
        .driver   = TYPE_X86_CPU,\
        .property = "hypervisor-cpuid",\
        .value    = "off",\
    },\

to prevent TCGTCGTCGTCG appearing.


Now in current KVM code we have

    if (cpu->expose_kvm) {
        memcpy(signature, "KVMKVMKVM\0\0\0", 12);
        ....
    }

if we add 'expose_hypervisor' as a generic variable and thus change
existing KVM code to

    if (cpu->expose_kvm || cpu->expose_hypervisor)

then to disable the KVMKVMKVMKVM signature, we now need to set *two*
flags - kvm=off, and hypervisor-cpuid=off and this will break existing
apps that are only setting kvm=off to disable it.

Conversely if we do

    if (cpu->expose_kvm && cpu->expose_hypervisor)

we're going to break KVMKVMKVM signature reporting by default, since
we need to set hypervisor-cpuid=off in back compat machine types
to prevent TCGTCGTCGTCG being exposed.

So it seems we're doomed either way, as we can't distinguish between
a boolean value that is at its default, vs a boolean value that has
been explicitly set to a value that matches the default.

Having a separate variable just for TCG seems the only viable option
to me - eg

    DEFINE_PROP_BOOL("tcg-cpuid", X86CPU, expose_tcg, true),


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH RFC] i386: expose "TCGTCGTCGTCG" in the 0x40000000 CPUID leaf

[Eduardo Habkost]

On Fri, May 05, 2017 at 12:46:21PM +0100, Daniel P. Berrange wrote:
> On Thu, May 04, 2017 at 03:42:41PM -0300, Eduardo Habkost wrote:
> > On Thu, May 04, 2017 at 03:56:58PM +0100, Daniel P. Berrange wrote:
> > > Currently when running KVM, we expose "KVMKVMKVM\0\0\0" in
> > > the 0x40000000 CPUID leaf. Other hypervisors (VMWare,
> > > HyperV, Xen, BHyve) all do the same thing, which leaves
> > > TCG as the odd one out.
> > > 
> > > The CPUID is used by software to detect when running in a
> > > virtual environment and change behaviour in certain ways.
> > > For example, systemd supports a ConditionVirtualization=
> > > setting in unit files. Currently they have to resort to
> > > custom hacks like looking for 'fw-cfg' entry in the
> > > /proc/device-tree file. The virt-what command has the
> > > same hacks & needs.
> > > 
> > > This change thus proposes a signature TCGTCGTCGTCG to be
> > > reported when running under TCG.
> > > 
> > > NB1, for reasons I don't undersatnd 'cpu_x86_cpuid' function
> > > clamps the requested CPUID leaf based on env->cpuid_level.
> 
> [snip]
> 
> > > NB2, for KVM, we added a flag for '-cpu kvm=off' to let you
> > > hide the KVMKVMKVM signature from guests. Presumably we should
> > > add a 'tcg=off' flag for the same reason ?
> > 
> > I don't like "kvm=off" because it sounds like it disables KVM
> > completely. "tcg=off" would be misleading as well.
> > 
> > What about a generic "hypervisor-cpuid=off" property?
> 
> Assuming that is intended to obsolete the existing 'kvm' parameter too,
> I'm not seeing a way to introduce that in a backwards compatible safe
> manner.
> 
> eg we add
> 
>     DEFINE_PROP_BOOL("hypervisor-cpuid", X86CPU, expose_hypervisor, true),
> 
> and in legacy machine types we need to set
> 
>     {\
>         .driver   = TYPE_X86_CPU,\
>         .property = "hypervisor-cpuid",\
>         .value    = "off",\
>     },\
> 
> to prevent TCGTCGTCGTCG appearing.
> 
> 
> Now in current KVM code we have
> 
>     if (cpu->expose_kvm) {
>         memcpy(signature, "KVMKVMKVM\0\0\0", 12);
>         ....
>     }
> 
> if we add 'expose_hypervisor' as a generic variable and thus change
> existing KVM code to
> 
>     if (cpu->expose_kvm || cpu->expose_hypervisor)
> 
> then to disable the KVMKVMKVMKVM signature, we now need to set *two*
> flags - kvm=off, and hypervisor-cpuid=off and this will break existing
> apps that are only setting kvm=off to disable it.
> 
> Conversely if we do
> 
>     if (cpu->expose_kvm && cpu->expose_hypervisor)
> 
> we're going to break KVMKVMKVM signature reporting by default, since
> we need to set hypervisor-cpuid=off in back compat machine types
> to prevent TCGTCGTCGTCG being exposed.
> 
> So it seems we're doomed either way, as we can't distinguish between
> a boolean value that is at its default, vs a boolean value that has
> been explicitly set to a value that matches the default.

That's true, even if we add a hypervisor-cpuid property, we need
a way to represent the behavior of QEMU 2.9 (disabling hypervisor
CPUID only for TCG).

> 
> Having a separate variable just for TCG seems the only viable option
> to me - eg
> 
>     DEFINE_PROP_BOOL("tcg-cpuid", X86CPU, expose_tcg, true),

Sounds good to me.

-- 
Eduardo