[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC] i386: expose "TCGTCGTCGTCG" in the 0x400000
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH RFC] i386: expose "TCGTCGTCGTCG" in the 0x40000000 CPUID leaf |
Date: |
Thu, 4 May 2017 15:42:41 -0300 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
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 <address@hidden>
> ---
> 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