[Qemu-devel] [PATCH] target-i386: Enhance the stub for kvm_arch_get_supported_cpuid()

[Qemu-devel] [PATCH] target-i386: Enhance the stub for kvm_arch_get_supported_cpuid()

[Kamil Rytarowski]

This improves the commit:
"target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid()"
r. 2140cfa51d59177815f5b82e94ac48fb24909aba

Clang/LLVM on NetBSD with enabled optimization cannot link
correct qemu program because of a missing symbol of
kvm_arch_get_supported_cpuid() in kvm-stubs.o used by executables.

There are more than a single one kvm-stub.c and several types
of possible programs such as bsd-user ones. the previous workaround
does not work reliably for all use-cases. Instead of reworking
the stubs and linking rules, move the workaround from a code that
depends on the __OPTIMIZE__ builtin compiler flag, build option (KVM),
compiler and arrangement of linking rules to a simple macro in a
shared header with all the users that defines fallback dummy
implementation, ignoring whether it is optimized out or not.

Signed-off-by: Kamil Rytarowski <n54@gmx.com>
---
 include/sysemu/kvm.h   | 13 +++++++++++++
 target/i386/kvm-stub.c | 10 ----------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index a6d1cd190f..93d3c0f0b3 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -459,8 +459,21 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
         kvm_vcpu_ioctl(cpu, KVM_ENABLE_CAP, &cap);                   \
     })
 
+#ifdef CONFIG_KVM
 uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
                                       uint32_t index, int reg);
+#else
+/*
+ * This function is only called inside conditionals which we
+ * rely on the compiler to optimize out when CONFIG_KVM is not
+ * defined.
+ */
+#define kvm_arch_get_supported_cpuid(a, b, c, d)                     \
+    ({                                                               \
+        abort();                                                     \
+        0;                                                           \
+    })
+#endif
 uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index);
 
 
diff --git a/target/i386/kvm-stub.c b/target/i386/kvm-stub.c
index e7a673e5db..9ce8566700 100644
--- a/target/i386/kvm-stub.c
+++ b/target/i386/kvm-stub.c
@@ -29,16 +29,6 @@ bool kvm_enable_x2apic(void)
 {
     return false;
 }
-
-/* This function is only called inside conditionals which we
- * rely on the compiler to optimize out when CONFIG_KVM is not
- * defined.
- */
-uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
-                                      uint32_t index, int reg)
-{
-    abort();
-}
 #endif
 
 bool kvm_hv_vpindex_settable(void)
-- 
2.20.1

Re: [Qemu-devel] [PATCH] target-i386: Enhance the stub for kvm_arch_get_supported_cpuid()

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190202144531.5772-1-n54@gmx.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14
=== TEST SCRIPT END ===


Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu --prefix=/tmp/qemu-test/install --python=/usr/bin/python3 --cross-prefix=x86_64-w64-mingw32- --enable-trace-backends=simple --enable-gnutls --enable-nettle --enable-curl --enable-vnc --enable-bzip2 --enable-guest-agent --with-sdlabi=2.0
ERROR: unknown option --with-sdlabi=2.0
Try '/tmp/qemu-test/src/configure --help' for more information
# QEMU configure log Sun Feb  3 17:31:44 UTC 2019
# Configured with: '/tmp/qemu-test/src/configure' '--enable-werror' '--target-list=x86_64-softmmu,aarch64-softmmu' '--prefix=/tmp/qemu-test/install' '--python=/usr/bin/python3' '--cross-prefix=x86_64-w64-mingw32-' '--enable-trace-backends=simple' '--enable-gnutls' '--enable-nettle' '--enable-curl' '--enable-vnc' '--enable-bzip2' '--enable-guest-agent' '--with-sdlabi=2.0'
---
funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 617 634 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __linux__ not defined
 #error __linux__ not defined
  ^~~~~

---
funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 617 686 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __i386__ not defined
 #error __i386__ not defined
  ^~~~~

---
funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 617 689 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __ILP32__ not defined
 #error __ILP32__ not defined
  ^~~~~

---
lines: 92 128 920 0
x86_64-w64-mingw32-gcc -mthreads -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -o config-temp/qemu-conf.exe config-temp/qemu-conf.c -g -liberty
/usr/lib/gcc/x86_64-w64-mingw32/8.2.0/../../../../x86_64-w64-mingw32/bin/ld: cannot find -liberty
collect2: error: ld returned 1 exit status
Failed to run 'configure'
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 563, in <module>


The full log is available at
http://patchew.org/logs/20190202144531.5772-1-n54@gmx.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] target-i386: Enhance the stub for kvm_arch_get_supported_cpuid()

[Kamil Rytarowski]

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

Ping?

On 02.02.2019 15:45, Kamil Rytarowski wrote:
> This improves the commit:
> "target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid()"
> r. 2140cfa51d59177815f5b82e94ac48fb24909aba
> 
> Clang/LLVM on NetBSD with enabled optimization cannot link
> correct qemu program because of a missing symbol of
> kvm_arch_get_supported_cpuid() in kvm-stubs.o used by executables.
> 
> There are more than a single one kvm-stub.c and several types
> of possible programs such as bsd-user ones. the previous workaround
> does not work reliably for all use-cases. Instead of reworking
> the stubs and linking rules, move the workaround from a code that
> depends on the __OPTIMIZE__ builtin compiler flag, build option (KVM),
> compiler and arrangement of linking rules to a simple macro in a
> shared header with all the users that defines fallback dummy
> implementation, ignoring whether it is optimized out or not.
> 
> Signed-off-by: Kamil Rytarowski <n54@gmx.com>
> ---
>  include/sysemu/kvm.h   | 13 +++++++++++++
>  target/i386/kvm-stub.c | 10 ----------
>  2 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index a6d1cd190f..93d3c0f0b3 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -459,8 +459,21 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
>          kvm_vcpu_ioctl(cpu, KVM_ENABLE_CAP, &cap);                   \
>      })
>  
> +#ifdef CONFIG_KVM
>  uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
>                                        uint32_t index, int reg);
> +#else
> +/*
> + * This function is only called inside conditionals which we
> + * rely on the compiler to optimize out when CONFIG_KVM is not
> + * defined.
> + */
> +#define kvm_arch_get_supported_cpuid(a, b, c, d)                     \
> +    ({                                                               \
> +        abort();                                                     \
> +        0;                                                           \
> +    })
> +#endif
>  uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index);
>  
>  
> diff --git a/target/i386/kvm-stub.c b/target/i386/kvm-stub.c
> index e7a673e5db..9ce8566700 100644
> --- a/target/i386/kvm-stub.c
> +++ b/target/i386/kvm-stub.c
> @@ -29,16 +29,6 @@ bool kvm_enable_x2apic(void)
>  {
>      return false;
>  }
> -
> -/* This function is only called inside conditionals which we
> - * rely on the compiler to optimize out when CONFIG_KVM is not
> - * defined.
> - */
> -uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
> -                                      uint32_t index, int reg)
> -{
> -    abort();
> -}
>  #endif
>  
>  bool kvm_hv_vpindex_settable(void)
> 



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

Re: [Qemu-devel] [PATCH] target-i386: Enhance the stub for kvm_arch_get_supported_cpuid()

[Paolo Bonzini]

On 02/02/19 15:45, Kamil Rytarowski wrote:
> 
> Clang/LLVM on NetBSD with enabled optimization cannot link
> correct qemu program because of a missing symbol of
> kvm_arch_get_supported_cpuid() in kvm-stubs.o used by executables.

Can you please include the full error message?  Usually these things are
a sign of a bug elsewhere.

Paolo

Re: [Qemu-devel] [PATCH] target-i386: Enhance the stub for kvm_arch_get_supported_cpuid()

[Kamil Rytarowski]

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

On 14.02.2019 19:44, Paolo Bonzini wrote:
> On 02/02/19 15:45, Kamil Rytarowski wrote:
>>
>> Clang/LLVM on NetBSD with enabled optimization cannot link
>> correct qemu program because of a missing symbol of
>> kvm_arch_get_supported_cpuid() in kvm-stubs.o used by executables.
> 
> Can you please include the full error message?  Usually these things are
> a sign of a bug elsewhere.
> 
> Paolo
> 

Please do replace the current kludge that is sensitive to:
 - compiler behavior that can change with new versions
 - compiler gcc/clang
 - optimization options
 - linux(KVM) - non-linux (no-KVM) build
 - community not actively testing non-linux no-kvm build with
optimization on clang


My patch replaced it makes it work.

Build error:

  LINK    i386-bsd-user/qemu-i386
/usr/bin/ld: /usr/lib/libc.so and /usr/lib/crt0.o: warning: multiple
common of `environ'
/usr/bin/ld: target/i386/cpu.o: in function `x86_cpu_filter_features':
/tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5047:
undefined reference to `kvm_arch_get_supported_cpuid'
/usr/bin/ld:
/tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5048:
undefined reference to `kvm_arch_get_supported_cpuid'
/usr/bin/ld:
/tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5049:
undefined reference to `kvm_arch_get_supported_cpuid'
/usr/bin/ld:
/tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5050:
undefined reference to `kvm_arch_get_supported_cpuid'
/usr/bin/ld:
/tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5051:
undefined reference to `kvm_arch_get_supported_cpuid'
clang-9: error: linker command failed with exit code 1 (use -v to see
invocation)
make[1]: *** [Makefile:199: qemu-i386] Error 1
gmake: *** [Makefile:483: subdir-i386-bsd-user] Error 2
gmake: *** Waiting for unfinished jobs....
  LINK    x86_64-bsd-user/qemu-x86_64
/usr/bin/ld: /usr/lib/libc.so and /usr/lib/crt0.o: warning: multiple
common of `environ'
/usr/bin/ld: target/i386/cpu.o: in function `x86_cpu_filter_features':
/tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5047:
undefined reference to `kvm_arch_get_supported_cpuid'
/usr/bin/ld:
/tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5048:
undefined reference to `kvm_arch_get_supported_cpuid'
/usr/bin/ld:
/tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5049:
undefined reference to `kvm_arch_get_supported_cpuid'
/usr/bin/ld:
/tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5050:
undefined reference to `kvm_arch_get_supported_cpuid'
/usr/bin/ld:
/tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5051:
undefined reference to `kvm_arch_get_supported_cpuid'
clang-9: error: linker command failed with exit code 1 (use -v to see
invocation)
make[1]: *** [Makefile:199: qemu-x86_64] Error 1
gmake: *** [Makefile:483: subdir-x86_64-bsd-user] Error 2
*** Error code 2


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

Re: [Qemu-devel] [PATCH] target-i386: Enhance the stub for kvm_arch_get_supported_cpuid()

[Paolo Bonzini]

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

On 14/02/19 20:41, Kamil Rytarowski wrote:
> Please do replace the current kludge that is sensitive to:
>  - compiler behavior that can change with new versions
>  - compiler gcc/clang
>  - optimization options

Not really, any half-decent compiler will optimize away "if (0)" and
QEMU is far from being the only software that relies on that.

GCC has been doing that even at -O0 for like 15 years, at some point it
was basically the only optimization it did.  Just try it for yourself:

	int f(void);

	int main()
	{
	        if (0)
	                return f();
	        else
	                return 0;
	}

Throw it at all compilers and optimization levels, and it *will* work.
If it doesn't then I'll consider again your patch.

>  - linux(KVM) - non-linux (no-KVM) build

That's the point.  We want your non-Linux non-KVM build to be as lean as
possible and not cause possible run-time failures due to people
forgetting about them.

>  - community not actively testing non-linux no-kvm build with
> optimization on clang

False, we test OS X and there are VM builds for the BSDs.
> My patch replaced it makes it work.
> 
> Build error:
> 
>   LINK    i386-bsd-user/qemu-i386

Ok, please use "make -C i386-bsd-user target/i386/cpu.o V=1" to get the
command line, invoke it again with "-save-temps" at the end, and send me
both the command line and the resulting "cpu.i" file.

Paolo

> /usr/bin/ld: /usr/lib/libc.so and /usr/lib/crt0.o: warning: multiple
> common of `environ'
> /usr/bin/ld: target/i386/cpu.o: in function `x86_cpu_filter_features':
> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5047:
> undefined reference to `kvm_arch_get_supported_cpuid'
> /usr/bin/ld:
> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5048:
> undefined reference to `kvm_arch_get_supported_cpuid'
> /usr/bin/ld:
> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5049:
> undefined reference to `kvm_arch_get_supported_cpuid'
> /usr/bin/ld:
> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5050:
> undefined reference to `kvm_arch_get_supported_cpuid'
> /usr/bin/ld:
> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5051:
> undefined reference to `kvm_arch_get_supported_cpuid'
> clang-9: error: linker command failed with exit code 1 (use -v to see
> invocation)
> make[1]: *** [Makefile:199: qemu-i386] Error 1
> gmake: *** [Makefile:483: subdir-i386-bsd-user] Error 2
> gmake: *** Waiting for unfinished jobs....
>   LINK    x86_64-bsd-user/qemu-x86_64
> /usr/bin/ld: /usr/lib/libc.so and /usr/lib/crt0.o: warning: multiple
> common of `environ'
> /usr/bin/ld: target/i386/cpu.o: in function `x86_cpu_filter_features':
> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5047:
> undefined reference to `kvm_arch_get_supported_cpuid'
> /usr/bin/ld:
> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5048:
> undefined reference to `kvm_arch_get_supported_cpuid'
> /usr/bin/ld:
> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5049:
> undefined reference to `kvm_arch_get_supported_cpuid'
> /usr/bin/ld:
> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5050:
> undefined reference to `kvm_arch_get_supported_cpuid'
> /usr/bin/ld:
> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5051:
> undefined reference to `kvm_arch_get_supported_cpuid'
> clang-9: error: linker command failed with exit code 1 (use -v to see
> invocation)
> make[1]: *** [Makefile:199: qemu-x86_64] Error 1
> gmake: *** [Makefile:483: subdir-x86_64-bsd-user] Error 2
> *** Error code 2
> 



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

Re: [Qemu-devel] [PATCH] target-i386: Enhance the stub for kvm_arch_get_supported_cpuid()

[Kamil Rytarowski]

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

On 14.02.2019 21:51, Paolo Bonzini wrote:
> On 14/02/19 20:41, Kamil Rytarowski wrote:
>> Please do replace the current kludge that is sensitive to:
>>  - compiler behavior that can change with new versions
>>  - compiler gcc/clang
>>  - optimization options
> 
> Not really, any half-decent compiler will optimize away "if (0)" and
> QEMU is far from being the only software that relies on that.
> 
> GCC has been doing that even at -O0 for like 15 years, at some point it
> was basically the only optimization it did.  Just try it for yourself:
> 
> 	int f(void);
> 
> 	int main()
> 	{
> 	        if (0)
> 	                return f();
> 	        else
> 	                return 0;
> 	}
> 
> Throw it at all compilers and optimization levels, and it *will* work.
> If it doesn't then I'll consider again your patch.
> 

I consider it as fragile hack and certainly not something to depend on.
Also in some circumstances of such code, especially "if (zero0)" we want
to enable disabled code under a debugger.

There were also kernel backdoors due to this optimization.

>>  - linux(KVM) - non-linux (no-KVM) build
> 
> That's the point.  We want your non-Linux non-KVM build to be as lean as
> possible and not cause possible run-time failures due to people
> forgetting about them.
> 
>>  - community not actively testing non-linux no-kvm build with
>> optimization on clang
> 
> False, we test OS X and there are VM builds for the BSDs.

Unfortunately not in the same combination of options as nobody caught it
in years. (Probably not many people actually develop it on these OSes
with debug flags). I was keeping this patch locally for some time now.
This hack was introduced several years ago.

>> My patch replaced it makes it work.
>>
>> Build error:
>>
>>   LINK    i386-bsd-user/qemu-i386
> 
> Ok, please use "make -C i386-bsd-user target/i386/cpu.o V=1" to get the
> command line, invoke it again with "-save-temps" at the end, and send me
> both the command line and the resulting "cpu.i" file.
> 

I'm building qemu with pkgsrc that provides all the dependencies and
compiler settings. It also uses wrappers to translate original compiler
options with transformed ones.

Log from pkgsrc with command lines:

http://netbsd.org/~kamil/qemu/qemu-build-2019-02-14.txt.bz2

Requested cpu.i (hopefully correctly generated)

http://netbsd.org/~kamil/qemu/cpu.i.bz2

I've generated it manually with this command.

/usr/local/bin/clang -iquote
/tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/target/i386 -iquote
target/i386 -iquote /tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/tcg
-iquote /tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/tcg/i386 -iquote .
-iquote /tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0 -iquote
/tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/accel/tcg -iquote
/tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/include
-I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.x11-buildlink/include/pixman-1
-I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/dtc/libfdt -pthread
-I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.buildlink/include/glib/glib-2.0
-I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.buildlink/lib/glib-2.0/include
-I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.buildlink/include -m64 -mcx16
-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
-Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings
-Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv
-Wno-error=address-of-packed-member -Wno-string-plus-int
-Wno-initializer-overrides -Wexpansion-to-defined -Wendif-labels
-Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body
-Wnested-externs -Wformat-security -Wformat-y2k -Winit-self
-Wignored-qualifiers -Wold-style-definition -Wtype-limits
-fstack-protector-strong
-I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.buildlink/include/libpng16
-I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/capstone/include -iquote
.. -iquote /tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/target/i386
-DNEED_CPU_H -iquote
/tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/include -MMD -MP -MT
target/i386/cpu.o -MF target/i386/cpu.d -O2 -g -O2 -O0 -g -ggdb
-I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.buildlink/include/SDL2
-I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.x11-buildlink/include
-I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.x11-buildlink/include/libdrm
-I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.buildlink/include/glib/gio-unix-2.0
-I/usr/include/krb5 -c -o target/i386/cpu.o
/tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/target/i386/cpu.c
-Qunused-arguments -fstack-protector -save-temps

> Paolo
> 
>> /usr/bin/ld: /usr/lib/libc.so and /usr/lib/crt0.o: warning: multiple
>> common of `environ'
>> /usr/bin/ld: target/i386/cpu.o: in function `x86_cpu_filter_features':
>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5047:
>> undefined reference to `kvm_arch_get_supported_cpuid'
>> /usr/bin/ld:
>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5048:
>> undefined reference to `kvm_arch_get_supported_cpuid'
>> /usr/bin/ld:
>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5049:
>> undefined reference to `kvm_arch_get_supported_cpuid'
>> /usr/bin/ld:
>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5050:
>> undefined reference to `kvm_arch_get_supported_cpuid'
>> /usr/bin/ld:
>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5051:
>> undefined reference to `kvm_arch_get_supported_cpuid'
>> clang-9: error: linker command failed with exit code 1 (use -v to see
>> invocation)
>> make[1]: *** [Makefile:199: qemu-i386] Error 1
>> gmake: *** [Makefile:483: subdir-i386-bsd-user] Error 2
>> gmake: *** Waiting for unfinished jobs....
>>   LINK    x86_64-bsd-user/qemu-x86_64
>> /usr/bin/ld: /usr/lib/libc.so and /usr/lib/crt0.o: warning: multiple
>> common of `environ'
>> /usr/bin/ld: target/i386/cpu.o: in function `x86_cpu_filter_features':
>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5047:
>> undefined reference to `kvm_arch_get_supported_cpuid'
>> /usr/bin/ld:
>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5048:
>> undefined reference to `kvm_arch_get_supported_cpuid'
>> /usr/bin/ld:
>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5049:
>> undefined reference to `kvm_arch_get_supported_cpuid'
>> /usr/bin/ld:
>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5050:
>> undefined reference to `kvm_arch_get_supported_cpuid'
>> /usr/bin/ld:
>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5051:
>> undefined reference to `kvm_arch_get_supported_cpuid'
>> clang-9: error: linker command failed with exit code 1 (use -v to see
>> invocation)
>> make[1]: *** [Makefile:199: qemu-x86_64] Error 1
>> gmake: *** [Makefile:483: subdir-x86_64-bsd-user] Error 2
>> *** Error code 2
>>
> 
> 



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

Re: [Qemu-devel] [PATCH] target-i386: Enhance the stub for kvm_arch_get_supported_cpuid()

[Kamil Rytarowski]

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

Ping, still valid.

On 15.02.2019 00:38, Kamil Rytarowski wrote:
> On 14.02.2019 21:51, Paolo Bonzini wrote:
>> On 14/02/19 20:41, Kamil Rytarowski wrote:
>>> Please do replace the current kludge that is sensitive to:
>>>  - compiler behavior that can change with new versions
>>>  - compiler gcc/clang
>>>  - optimization options
>>
>> Not really, any half-decent compiler will optimize away "if (0)" and
>> QEMU is far from being the only software that relies on that.
>>
>> GCC has been doing that even at -O0 for like 15 years, at some point it
>> was basically the only optimization it did.  Just try it for yourself:
>>
>> 	int f(void);
>>
>> 	int main()
>> 	{
>> 	        if (0)
>> 	                return f();
>> 	        else
>> 	                return 0;
>> 	}
>>
>> Throw it at all compilers and optimization levels, and it *will* work.
>> If it doesn't then I'll consider again your patch.
>>
> 
> I consider it as fragile hack and certainly not something to depend on.
> Also in some circumstances of such code, especially "if (zero0)" we want
> to enable disabled code under a debugger.
> 
> There were also kernel backdoors due to this optimization.
> 
>>>  - linux(KVM) - non-linux (no-KVM) build
>>
>> That's the point.  We want your non-Linux non-KVM build to be as lean as
>> possible and not cause possible run-time failures due to people
>> forgetting about them.
>>
>>>  - community not actively testing non-linux no-kvm build with
>>> optimization on clang
>>
>> False, we test OS X and there are VM builds for the BSDs.
> 
> Unfortunately not in the same combination of options as nobody caught it
> in years. (Probably not many people actually develop it on these OSes
> with debug flags). I was keeping this patch locally for some time now.
> This hack was introduced several years ago.
> 
>>> My patch replaced it makes it work.
>>>
>>> Build error:
>>>
>>>   LINK    i386-bsd-user/qemu-i386
>>
>> Ok, please use "make -C i386-bsd-user target/i386/cpu.o V=1" to get the
>> command line, invoke it again with "-save-temps" at the end, and send me
>> both the command line and the resulting "cpu.i" file.
>>
> 
> I'm building qemu with pkgsrc that provides all the dependencies and
> compiler settings. It also uses wrappers to translate original compiler
> options with transformed ones.
> 
> Log from pkgsrc with command lines:
> 
> http://netbsd.org/~kamil/qemu/qemu-build-2019-02-14.txt.bz2
> 
> Requested cpu.i (hopefully correctly generated)
> 
> http://netbsd.org/~kamil/qemu/cpu.i.bz2
> 
> I've generated it manually with this command.
> 
> /usr/local/bin/clang -iquote
> /tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/target/i386 -iquote
> target/i386 -iquote /tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/tcg
> -iquote /tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/tcg/i386 -iquote .
> -iquote /tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0 -iquote
> /tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/accel/tcg -iquote
> /tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/include
> -I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.x11-buildlink/include/pixman-1
> -I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/dtc/libfdt -pthread
> -I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.buildlink/include/glib/glib-2.0
> -I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.buildlink/lib/glib-2.0/include
> -I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.buildlink/include -m64 -mcx16
> -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
> -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings
> -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv
> -Wno-error=address-of-packed-member -Wno-string-plus-int
> -Wno-initializer-overrides -Wexpansion-to-defined -Wendif-labels
> -Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body
> -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self
> -Wignored-qualifiers -Wold-style-definition -Wtype-limits
> -fstack-protector-strong
> -I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.buildlink/include/libpng16
> -I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/capstone/include -iquote
> .. -iquote /tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/target/i386
> -DNEED_CPU_H -iquote
> /tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/include -MMD -MP -MT
> target/i386/cpu.o -MF target/i386/cpu.d -O2 -g -O2 -O0 -g -ggdb
> -I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.buildlink/include/SDL2
> -I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.x11-buildlink/include
> -I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.x11-buildlink/include/libdrm
> -I/tmp/pkgsrc-tmp/wip/qemu-haxm/work/.buildlink/include/glib/gio-unix-2.0
> -I/usr/include/krb5 -c -o target/i386/cpu.o
> /tmp/pkgsrc-tmp/wip/qemu-haxm/work/qemu-3.0.0/target/i386/cpu.c
> -Qunused-arguments -fstack-protector -save-temps
> 
>> Paolo
>>
>>> /usr/bin/ld: /usr/lib/libc.so and /usr/lib/crt0.o: warning: multiple
>>> common of `environ'
>>> /usr/bin/ld: target/i386/cpu.o: in function `x86_cpu_filter_features':
>>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5047:
>>> undefined reference to `kvm_arch_get_supported_cpuid'
>>> /usr/bin/ld:
>>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5048:
>>> undefined reference to `kvm_arch_get_supported_cpuid'
>>> /usr/bin/ld:
>>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5049:
>>> undefined reference to `kvm_arch_get_supported_cpuid'
>>> /usr/bin/ld:
>>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5050:
>>> undefined reference to `kvm_arch_get_supported_cpuid'
>>> /usr/bin/ld:
>>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5051:
>>> undefined reference to `kvm_arch_get_supported_cpuid'
>>> clang-9: error: linker command failed with exit code 1 (use -v to see
>>> invocation)
>>> make[1]: *** [Makefile:199: qemu-i386] Error 1
>>> gmake: *** [Makefile:483: subdir-i386-bsd-user] Error 2
>>> gmake: *** Waiting for unfinished jobs....
>>>   LINK    x86_64-bsd-user/qemu-x86_64
>>> /usr/bin/ld: /usr/lib/libc.so and /usr/lib/crt0.o: warning: multiple
>>> common of `environ'
>>> /usr/bin/ld: target/i386/cpu.o: in function `x86_cpu_filter_features':
>>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5047:
>>> undefined reference to `kvm_arch_get_supported_cpuid'
>>> /usr/bin/ld:
>>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5048:
>>> undefined reference to `kvm_arch_get_supported_cpuid'
>>> /usr/bin/ld:
>>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5049:
>>> undefined reference to `kvm_arch_get_supported_cpuid'
>>> /usr/bin/ld:
>>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5050:
>>> undefined reference to `kvm_arch_get_supported_cpuid'
>>> /usr/bin/ld:
>>> /tmp/pkgsrc-tmp/emulators/qemu/work/qemu-3.1.0/target/i386/cpu.c:5051:
>>> undefined reference to `kvm_arch_get_supported_cpuid'
>>> clang-9: error: linker command failed with exit code 1 (use -v to see
>>> invocation)
>>> make[1]: *** [Makefile:199: qemu-x86_64] Error 1
>>> gmake: *** [Makefile:483: subdir-x86_64-bsd-user] Error 2
>>> *** Error code 2
>>>
>>
>>
> 
> 



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

Re: [Qemu-devel] [PATCH] target-i386: Enhance the stub for kvm_arch_get_supported_cpuid()

[Paolo Bonzini]

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

On 20/02/19 12:59, Kamil Rytarowski wrote:
> Ping, still valid.

Sorry, I missed your email.

> On 15.02.2019 00:38, Kamil Rytarowski wrote:
>> I consider it as fragile hack and certainly not something to depend on.
>> Also in some circumstances of such code, especially "if (zero0)" we want
>> to enable disabled code under a debugger.

That's a good objection, but certainly does not apply to KVM on NetBSD.

>> There were also kernel backdoors due to this optimization.

Citation please?

>> Requested cpu.i (hopefully correctly generated)
>>
>> http://netbsd.org/~kamil/qemu/cpu.i.bz2

So, first thing first I can reproduce clang's behavior with this .i file
and also with this reduced test case.

    extern void f(void);
    int i, j;
    int main()
    {
        if (0  && i) f();
        if (j  && 0) f();
   }

The first is eliminated but the second is not, just like in QEMU where
this works:

        if (kvm_enabled() && cpu->enable_pmu) {
            KVMState *s = cs->kvm_state;

            *eax = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EAX);
            *ebx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EBX);
            *ecx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_ECX);
            *edx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EDX);
        } else if (hvf_enabled() && cpu->enable_pmu) {
            *eax = hvf_get_supported_cpuid(0xA, count, R_EAX);
            *ebx = hvf_get_supported_cpuid(0xA, count, R_EBX);
            *ecx = hvf_get_supported_cpuid(0xA, count, R_ECX);
            *edx = hvf_get_supported_cpuid(0xA, count, R_EDX);

while this doesn't:

    if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
        kvm_enabled()) {
        KVMState *s = CPU(cpu)->kvm_state;
        uint32_t eax_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EAX);
        uint32_t ebx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EBX);
        uint32_t ecx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_ECX);
        uint32_t eax_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EAX);
        uint32_t ebx_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EBX);

But, that's okay, it's -O0 so we give clang a pass for that  Note that
clang does do the optimization even in more complex cases like

    extern _Bool f(void);
    int main()
    {
        if (!0) return 0;
        if (!f()) return 0;
    }

The problem is that there is a kvm-stub.c entry for that, and in fact
my compilation passes and the symbol is resolved correctly:

$ nm target/i386/cpu.o |grep kvm_.*get_sup
                 U kvm_arch_get_supported_cpuid
$ nm target/i386/kvm-stub.o|grep kvm_.*get_sup
0000000000000030 T kvm_arch_get_supported_cpuid
$ nm qemu-system-x86_64 |grep kvm_.*get_sup
000000000046eab0 T kvm_arch_get_supported_cpuid

As expected, something much less obvious is going on for you, in
particular __OPTIMIZE__seems not to be working properly.  However,
that would also be very surprising.

Please:

1) run the last two "nm" commands on your build (wthout grep).

2) do the same exercise to get a .i for target/i386/kvm-stub.c

3) try removing the "#ifndef __OPTIMIZE__" and leave everything else as is,
see if it works.  No need to play with macros, which also goes to show that
you didn't really understand what's going on---that's fine, but then please
refrain from making summary judgments which only lengthen the discussion.

Thanks,

Paolo


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

Re: [Qemu-devel] [PATCH] target-i386: Enhance the stub for kvm_arch_get_supported_cpuid()

[Kamil Rytarowski]

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

On 20.02.2019 18:29, Paolo Bonzini wrote:
> On 20/02/19 12:59, Kamil Rytarowski wrote:
>> Ping, still valid.
> 
> Sorry, I missed your email.
> 
>> On 15.02.2019 00:38, Kamil Rytarowski wrote:
>>> I consider it as fragile hack and certainly not something to depend on.
>>> Also in some circumstances of such code, especially "if (zero0)" we want
>>> to enable disabled code under a debugger.
> 
> That's a good objection, but certainly does not apply to KVM on NetBSD.
>

There is KVM for Darwin (experimental and rather toy project) and it
might be ported to NetBSD (I have actually forked it on GitHub
recently), but I doubt that someone would enable KVM on any platform
under a debugger this way and expect something to work.

>>> There were also kernel backdoors due to this optimization.
> 
> Citation please?
> 

I saw an exploit for such case with a .txt writeup on ftp of grsecurity
but that service seems to be gone (probably long time ago), so please
defer discussion on it. If someone is interested to find it out, there
are enough pointers to dig it (assuming that this is still possible).

>>> Requested cpu.i (hopefully correctly generated)
>>>
>>> http://netbsd.org/~kamil/qemu/cpu.i.bz2
> 
> So, first thing first I can reproduce clang's behavior with this .i file
> and also with this reduced test case.
> 
>     extern void f(void);
>     int i, j;
>     int main()
>     {
>         if (0  && i) f();
>         if (j  && 0) f();
>    }
> 
> The first is eliminated but the second is not, just like in QEMU where
> this works:
> 
>         if (kvm_enabled() && cpu->enable_pmu) {
>             KVMState *s = cs->kvm_state;
> 
>             *eax = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EAX);
>             *ebx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EBX);
>             *ecx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_ECX);
>             *edx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EDX);
>         } else if (hvf_enabled() && cpu->enable_pmu) {
>             *eax = hvf_get_supported_cpuid(0xA, count, R_EAX);
>             *ebx = hvf_get_supported_cpuid(0xA, count, R_EBX);
>             *ecx = hvf_get_supported_cpuid(0xA, count, R_ECX);
>             *edx = hvf_get_supported_cpuid(0xA, count, R_EDX);
> 
> while this doesn't:
> 
>     if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
>         kvm_enabled()) {
>         KVMState *s = CPU(cpu)->kvm_state;
>         uint32_t eax_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EAX);
>         uint32_t ebx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EBX);
>         uint32_t ecx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_ECX);
>         uint32_t eax_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EAX);
>         uint32_t ebx_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EBX);
> 
> But, that's okay, it's -O0 so we give clang a pass for that  Note that
> clang does do the optimization even in more complex cases like
> 
>     extern _Bool f(void);
>     int main()
>     {
>         if (!0) return 0;
>         if (!f()) return 0;
>     }
> 
> The problem is that there is a kvm-stub.c entry for that, and in fact
> my compilation passes and the symbol is resolved correctly:
> 
> $ nm target/i386/cpu.o |grep kvm_.*get_sup
>                  U kvm_arch_get_supported_cpuid
> $ nm target/i386/kvm-stub.o|grep kvm_.*get_sup
> 0000000000000030 T kvm_arch_get_supported_cpuid
> $ nm qemu-system-x86_64 |grep kvm_.*get_sup
> 000000000046eab0 T kvm_arch_get_supported_cpuid
> 
> As expected, something much less obvious is going on for you, in
> particular __OPTIMIZE__seems not to be working properly.  However,
> that would also be very surprising.
> 
> Please:
> 
> 1) run the last two "nm" commands on your build (wthout grep).
> 

I cannot run nm(1) on qemu-system-x86_64 as it's not linkable.

I'm getting the same result for target/i386/cpu.o and
target/i386/kvm-stub.o.

$ nm ./i386-softmmu/target/i386/kvm-stub.o
                 U abort
0000000000000000 T kvm_allows_irq0_override
0000000000000030 T kvm_arch_get_supported_cpuid
0000000000000020 T kvm_enable_x2apic
0000000000000010 T kvm_has_smm
0000000000000050 T kvm_hv_vpindex_settable

grep(1) used, but otherwise I would need to upload results somewhere else.

$ nm ./i386-bsd-user/target/i386/cpu.o |grep kvm
                 U kvm_arch_get_supported_cpuid
0000000000001290 d kvm_default_props
                 U kvm_state
0000000000000240 T x86_cpu_change_kvm_default

Please note that there are 4 types of x86 build: i386, x86_64 and two
bsd-user (32-bit and 64-bit).

According to my observations of repeated attempts both builds of
bsd-user are affected.

There is also a difference that kvm-stub is twice in !bsd-user
directories and once in bsd-user ones.

$ find . -name kvm-stub.o|grep x86_64
./x86_64-softmmu/accel/stubs/kvm-stub.o
./x86_64-softmmu/target/i386/kvm-stub.o
./x86_64-bsd-user/accel/stubs/kvm-stub.o

> 2) do the same exercise to get a .i for target/i386/kvm-stub.c
> 
> 3) try removing the "#ifndef __OPTIMIZE__" and leave everything else as is,
> see if it works.

Same result as above, it seems that bsd-user ones are affected in the
same way.

>  No need to play with macros, which also goes to show that
> you didn't really understand what's going on---that's fine, but then please
> refrain from making summary judgments which only lengthen the discussion.
> 

I stand with my expression that I find playing with optimizer as too
fragile to depend on it. Relying on a preprocessor to disable unused
code is predictable. But if that is the style in qemu, I can just
acknowledge it.

> Thanks,
> 
> Paolo
> 



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

Re: [Qemu-devel] [PATCH] target-i386: Enhance the stub for kvm_arch_get_supported_cpuid()

[Paolo Bonzini]

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

On 21/02/19 03:08, Kamil Rytarowski wrote:
> $ find . -name kvm-stub.o|grep x86_64
> ./x86_64-softmmu/accel/stubs/kvm-stub.o
> ./x86_64-softmmu/target/i386/kvm-stub.o
> ./x86_64-bsd-user/accel/stubs/kvm-stub.o
> 

So here's the bug.  The fix should be as simple as

diff --git a/target/i386/Makefile.objs b/target/i386/Makefile.objs
index cb9c265525..48e0c28434 100644
--- a/target/i386/Makefile.objs
+++ b/target/i386/Makefile.objs
@@ -3,10 +3,10 @@ obj-$(CONFIG_TCG) += translate.o
 obj-$(CONFIG_TCG) += bpt_helper.o cc_helper.o excp_helper.o fpu_helper.o
 obj-$(CONFIG_TCG) += int_helper.o mem_helper.o misc_helper.o mpx_helper.o
 obj-$(CONFIG_TCG) += seg_helper.o smm_helper.o svm_helper.o
+obj-$(call lnot,$(CONFIG_KVM)) += kvm-stub.o
 ifeq ($(CONFIG_SOFTMMU),y)
 obj-y += machine.o arch_memory_mapping.o arch_dump.o monitor.o
 obj-$(CONFIG_KVM) += kvm.o
-obj-$(call lnot,$(CONFIG_KVM)) += kvm-stub.o
 obj-$(CONFIG_HYPERV) += hyperv.o
 obj-$(call lnot,$(CONFIG_HYPERV)) += hyperv-stub.o
 ifeq ($(CONFIG_WIN32),y)

Thanks,

Paolo


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

Re: [Qemu-devel] [PATCH] target-i386: Enhance the stub for kvm_arch_get_supported_cpuid()

[Kamil Rytarowski]

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

Ping.

On 21.02.2019 03:08, Kamil Rytarowski wrote:
> On 20.02.2019 18:29, Paolo Bonzini wrote:
>> On 20/02/19 12:59, Kamil Rytarowski wrote:
>>> Ping, still valid.
>>
>> Sorry, I missed your email.
>>
>>> On 15.02.2019 00:38, Kamil Rytarowski wrote:
>>>> I consider it as fragile hack and certainly not something to depend on.
>>>> Also in some circumstances of such code, especially "if (zero0)" we want
>>>> to enable disabled code under a debugger.
>>
>> That's a good objection, but certainly does not apply to KVM on NetBSD.
>>
> 
> There is KVM for Darwin (experimental and rather toy project) and it
> might be ported to NetBSD (I have actually forked it on GitHub
> recently), but I doubt that someone would enable KVM on any platform
> under a debugger this way and expect something to work.
> 
>>>> There were also kernel backdoors due to this optimization.
>>
>> Citation please?
>>
> 
> I saw an exploit for such case with a .txt writeup on ftp of grsecurity
> but that service seems to be gone (probably long time ago), so please
> defer discussion on it. If someone is interested to find it out, there
> are enough pointers to dig it (assuming that this is still possible).
> 
>>>> Requested cpu.i (hopefully correctly generated)
>>>>
>>>> http://netbsd.org/~kamil/qemu/cpu.i.bz2
>>
>> So, first thing first I can reproduce clang's behavior with this .i file
>> and also with this reduced test case.
>>
>>     extern void f(void);
>>     int i, j;
>>     int main()
>>     {
>>         if (0  && i) f();
>>         if (j  && 0) f();
>>    }
>>
>> The first is eliminated but the second is not, just like in QEMU where
>> this works:
>>
>>         if (kvm_enabled() && cpu->enable_pmu) {
>>             KVMState *s = cs->kvm_state;
>>
>>             *eax = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EAX);
>>             *ebx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EBX);
>>             *ecx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_ECX);
>>             *edx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EDX);
>>         } else if (hvf_enabled() && cpu->enable_pmu) {
>>             *eax = hvf_get_supported_cpuid(0xA, count, R_EAX);
>>             *ebx = hvf_get_supported_cpuid(0xA, count, R_EBX);
>>             *ecx = hvf_get_supported_cpuid(0xA, count, R_ECX);
>>             *edx = hvf_get_supported_cpuid(0xA, count, R_EDX);
>>
>> while this doesn't:
>>
>>     if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
>>         kvm_enabled()) {
>>         KVMState *s = CPU(cpu)->kvm_state;
>>         uint32_t eax_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EAX);
>>         uint32_t ebx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EBX);
>>         uint32_t ecx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_ECX);
>>         uint32_t eax_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EAX);
>>         uint32_t ebx_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EBX);
>>
>> But, that's okay, it's -O0 so we give clang a pass for that  Note that
>> clang does do the optimization even in more complex cases like
>>
>>     extern _Bool f(void);
>>     int main()
>>     {
>>         if (!0) return 0;
>>         if (!f()) return 0;
>>     }
>>
>> The problem is that there is a kvm-stub.c entry for that, and in fact
>> my compilation passes and the symbol is resolved correctly:
>>
>> $ nm target/i386/cpu.o |grep kvm_.*get_sup
>>                  U kvm_arch_get_supported_cpuid
>> $ nm target/i386/kvm-stub.o|grep kvm_.*get_sup
>> 0000000000000030 T kvm_arch_get_supported_cpuid
>> $ nm qemu-system-x86_64 |grep kvm_.*get_sup
>> 000000000046eab0 T kvm_arch_get_supported_cpuid
>>
>> As expected, something much less obvious is going on for you, in
>> particular __OPTIMIZE__seems not to be working properly.  However,
>> that would also be very surprising.
>>
>> Please:
>>
>> 1) run the last two "nm" commands on your build (wthout grep).
>>
> 
> I cannot run nm(1) on qemu-system-x86_64 as it's not linkable.
> 
> I'm getting the same result for target/i386/cpu.o and
> target/i386/kvm-stub.o.
> 
> $ nm ./i386-softmmu/target/i386/kvm-stub.o
>                  U abort
> 0000000000000000 T kvm_allows_irq0_override
> 0000000000000030 T kvm_arch_get_supported_cpuid
> 0000000000000020 T kvm_enable_x2apic
> 0000000000000010 T kvm_has_smm
> 0000000000000050 T kvm_hv_vpindex_settable
> 
> grep(1) used, but otherwise I would need to upload results somewhere else.
> 
> $ nm ./i386-bsd-user/target/i386/cpu.o |grep kvm
>                  U kvm_arch_get_supported_cpuid
> 0000000000001290 d kvm_default_props
>                  U kvm_state
> 0000000000000240 T x86_cpu_change_kvm_default
> 
> Please note that there are 4 types of x86 build: i386, x86_64 and two
> bsd-user (32-bit and 64-bit).
> 
> According to my observations of repeated attempts both builds of
> bsd-user are affected.
> 
> There is also a difference that kvm-stub is twice in !bsd-user
> directories and once in bsd-user ones.
> 
> $ find . -name kvm-stub.o|grep x86_64
> ./x86_64-softmmu/accel/stubs/kvm-stub.o
> ./x86_64-softmmu/target/i386/kvm-stub.o
> ./x86_64-bsd-user/accel/stubs/kvm-stub.o
> 
>> 2) do the same exercise to get a .i for target/i386/kvm-stub.c
>>
>> 3) try removing the "#ifndef __OPTIMIZE__" and leave everything else as is,
>> see if it works.
> 
> Same result as above, it seems that bsd-user ones are affected in the
> same way.
> 
>>  No need to play with macros, which also goes to show that
>> you didn't really understand what's going on---that's fine, but then please
>> refrain from making summary judgments which only lengthen the discussion.
>>
> 
> I stand with my expression that I find playing with optimizer as too
> fragile to depend on it. Relying on a preprocessor to disable unused
> code is predictable. But if that is the style in qemu, I can just
> acknowledge it.
> 
>> Thanks,
>>
>> Paolo
>>
> 
> 



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

Re: [Qemu-devel] [PATCH] target-i386: Enhance the stub for kvm_arch_get_supported_cpuid()

[Kamil Rytarowski]

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

On 25.02.2019 08:10, Paolo Bonzini wrote:
> I have replied already.
> 



Sorry, I don't have this mail in my mailbox for some reason.

The issue is gone with the following patch:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg598417.html

Tested-by: Kamil Rytarowski <n54@gmx.com>

Please merge with qemu. Thank you in advance!



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