From 7f2569246c81d5f88e74c142b8fbdc0ee601bffe Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 5 Aug 2016 10:51:37 +0200 Subject: [PATCH 01/14] linuxboot_dma: avoid guest ABI breakage on gcc vs. clang compilation Recent GCC compiles linuxboot_dma.c to 921 bytes, while CentOS 6 needs 1029 and clang needs 1527. Because the size of the ROM, rounded to the next 512 bytes, must match, this causes the API to break between a <1K ROM and one that is bigger. We want to make the ROM 1.5 KB in size, but it's better to make clang produce leaner ROMs, because currently it is worryingly close to the limit. To fix this prevent clang's happy inlining (which -Os cannot prevent). This only requires adding a noinline attribute. Second, the patch makes sure that the ROM has enough padding to prevent ABI breakage on different compilers. The size is now hardcoded in the file that is passed to signrom.py, as was the case before commit 6f71b77 ("scripts/signrom.py: Allow option ROM checksum script to write the size header.", 2016-05-23); signrom.py however will still pad the input to the requested size. This ensures that the padding goes beyond the next multiple of 512 if necessary, and also avoids the need for -fno-toplevel-reorder which clang doesn't support. signrom.py can then error out if the requested size is too small for the actual size of the compiled ROM. Signed-off-by: Paolo Bonzini --- pc-bios/linuxboot_dma.bin | Bin 1024 -> 1536 bytes pc-bios/optionrom/linuxboot_dma.c | 8 ++++++-- scripts/signrom.py | 27 +++++++++++---------------- 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/pc-bios/linuxboot_dma.bin b/pc-bios/linuxboot_dma.bin index e1f623a1245efff0d46ea5b685e0f18e4ef98d74..238a195d3869995067f158d243d852778d38a736 100644 GIT binary patch literal 1536 zcmeHFO=}ZT6n#mjn#9N?ZK{K0k;PaL-DD+#E`)TKK>SF%@QZ?;pp*(SMO{f8vv`CI z_!9&-x)zKBLZ%f_LNuh%S`|r(v{S59T@^nv-g)U07ybYjeTz4D&OPs(`|i7ihXW1v z&y{3)emWlr%H&aYT!!qlh)#^<3M_khdgexI>gwdhOV?7FoX`3Gb8V4T4OV^17z*G` zOn^I3n-~Xwj#DUg8S@xQE3`AD-$A|K(7+ertc1X4;Vf$ie}Jj?IftcP?P z=6ZtIvqam3<7z0a3aQvV%fdk;)OF zd{0P_V?yG)1kU@?8C~hi6fr;?lBo<)AHR4WO3UI?t{W35w~doH(lT=XJJbR$+43du zx6Kjoh43q=nR%#VPPwh#*%zRfiBT!H$tO_6LEl;^Qi;M&6PFP6(lryXhx%9clkV4F zTOA6?dUuL?mn5!9JS1>O;H0C$j!T^o+y{~$mHc7Bzbo;i#1jI)+1JVFQP?AWovfaO zJK4t*I(6nOC3y=6=4q%Dn4e?=0EZ1s*zc=ft{1q{%>6iWxrfC#EH)uF?F&uL= cH&7qfG012%;B+Eu-5wcjuk8-}=N-898@_u-5C8xG literal 1024 zcmd6mziX307{@Q^tG-DiZ>p(g$dKVIh%T9=qM-1)wGcJY!KxJW`(h~-Bt;!#jBtbm z9lAI>If`HmD0vg9wINblk!l@67gN$=2eDW*ug_gPT>JyP;dt+J&wZbtZ~C#n!Tz~o zj3=j(KEJ*^#!l)_mQr7*PmQM8$hE2ITk*;3<5#ZUh})ymX8Y&b7go%$;tR&$u7;7u zd7Q&ph$V;`5*|d8xQX0)4B`i}tBiRJVivhZ(1A#CL)y8mD6B{CjnC`{bTm=MXKl`$#}Z zhxPS-dEM@GB`+A9wh{9%R=26W}#UCn8I#H!Vog7g7j= 65536: - sys.exit("%s: option ROM size too large" % sys.argv[1]) +size = size_byte * 512 +if len(data) > size: + sys.stderr.write('error: ROM is too large (%d > %d)\n' % (len(data), size)) + sys.exit(1) +elif len(data) < size: + # Add padding if necessary, rounding the whole input to a multiple of + # 512 bytes according to the third byte of the input. # size-1 because a final byte is added below to store the checksum. data = data.ljust(size-1, '\0') - data = data[:2] + chr(size/512) + data[3:] else: - # Otherwise the input file specifies the size so use it. - # -1 because we overwrite the last byte of the file with the checksum. - size = size_byte * 512 - 1 - data = fin.read(size) + if ord(data[-1:]) != 0: + sys.stderr.write('WARNING: ROM includes nonzero checksum\n') + data = data[:size-1] fout.write(data) From a9c87304b76d1d61687d585516abb4c6e0ae809e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Fri, 5 Aug 2016 12:23:46 +0400 Subject: [PATCH 02/14] build-sys: fix building with make CFLAGS=.. argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When calling make with a CFLAGS=.. argument, the -g/-O filter is not applied, which may result with build failure with ASAN for example. It could be solved with an 'override' directive on CFLAGS, but that would actually prevent setting different CFLAGS manually. Instead, filter the CFLAGS argument from the top-level Makefile (so you could still call make with a different CFLAGS argument on a rom/Makefile manually) Signed-off-by: Marc-André Lureau Reviewed-by: Paolo Bonzini Message-Id: <20160805082421.21994-2-marcandre.lureau@redhat.com> Signed-off-by: Paolo Bonzini --- Makefile | 3 ++- pc-bios/optionrom/Makefile | 2 -- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 0d7647f796..50b4b3afb9 100644 --- a/Makefile +++ b/Makefile @@ -225,8 +225,9 @@ dtc/%: $(SUBDIR_RULES): libqemuutil.a libqemustub.a $(common-obj-y) $(qom-obj-y) $(crypto-aes-obj-$(CONFIG_USER_ONLY)) ROMSUBDIR_RULES=$(patsubst %,romsubdir-%, $(ROMS)) +# Only keep -O and -g cflags romsubdir-%: - $(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C pc-bios/$* V="$(V)" TARGET_DIR="$*/",) + $(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C pc-bios/$* V="$(V)" TARGET_DIR="$*/" CFLAGS="$(filter -O% -g%,$(CFLAGS))",) ALL_SUBDIRS=$(TARGET_DIRS) $(patsubst %,pc-bios/%, $(ROMS)) diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile index 24e175e0eb..6bab490073 100644 --- a/pc-bios/optionrom/Makefile +++ b/pc-bios/optionrom/Makefile @@ -24,8 +24,6 @@ QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS), -no-integrated-as) QEMU_CFLAGS += -m32 -include $(SRC_PATH)/pc-bios/optionrom/code16gcc.h endif -# Drop gcov and glib flags -CFLAGS := $(filter -O% -g%, $(CFLAGS)) QEMU_INCLUDES += -I$(SRC_PATH) Wa = -Wa, From b0e8f5cadcce7c1e2047e1e2c96f827a26171f58 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 8 Aug 2016 10:57:35 +0200 Subject: [PATCH 03/14] optionrom: add -fno-stack-protector This is required by OpenBSD. Signed-off-by: Paolo Bonzini --- pc-bios/optionrom/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile index 6bab490073..9c018ea48a 100644 --- a/pc-bios/optionrom/Makefile +++ b/pc-bios/optionrom/Makefile @@ -11,6 +11,7 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/optionrom) # Drop -fstack-protector and the like QEMU_CFLAGS := $(filter -W%, $(QEMU_CFLAGS)) $(CFLAGS_NOPIE) -ffreestanding +QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS), -fno-stack-protector) QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS), -m16) ifeq ($(filter -m16, $(QEMU_CFLAGS)),) # Attempt to work around compilers that lack -m16 (GCC <= 4.8, clang <= ??) From 9d4cd7b4ed413a1371997ce74da39900b2a8473b Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 9 Aug 2016 22:50:40 +0200 Subject: [PATCH 04/14] optionrom: fix compilation with mingw docker target Two fixes are needed. First, mingw does not have -D_FORTIFY_SOURCE, hence --enable-debug disables optimization. This is not acceptable for ROMs, which should override CFLAGS to force inclusion of -O2. Second, PE stores global constructors and destructors using the following linker script snippet: ___CTOR_LIST__ = .; __CTOR_LIST__ = . ; LONG (-1);*(.ctors); *(.ctor); *(SORT(.ctors.*)); LONG (0); ___DTOR_LIST__ = .; __DTOR_LIST__ = . ; LONG (-1); *(.dtors); *(.dtor); *(SORT(.dtors.*)); LONG (0); The LONG directives cause the .img files to be 16 bytes too large; the recently added check to signrom.py catches this. To fix this, replace -T and -e options with a linker script. Signed-off-by: Paolo Bonzini --- pc-bios/optionrom/Makefile | 10 +++++++++- pc-bios/optionrom/flat.lds | 6 ++++++ 2 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 pc-bios/optionrom/flat.lds diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile index 9c018ea48a..8aef152262 100644 --- a/pc-bios/optionrom/Makefile +++ b/pc-bios/optionrom/Makefile @@ -9,6 +9,14 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/optionrom) .PHONY : all clean build-all +# Compiling with no optimization creates ROMs that are too large +ifeq ($(filter -O%, $(CFLAGS)),) +override CFLAGS += -O2 +endif +ifeq ($(filter -O%, $(CFLAGS)),-O0) +override CFLAGS += -O2 +endif + # Drop -fstack-protector and the like QEMU_CFLAGS := $(filter -W%, $(QEMU_CFLAGS)) $(CFLAGS_NOPIE) -ffreestanding QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS), -fno-stack-protector) @@ -51,7 +59,7 @@ endif endif %.img: %.o - $(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m $(LD_EMULATION) -Ttext 0 -e _start -s -o $@ $<," Building $(TARGET_DIR)$@") + $(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m $(LD_EMULATION) -T $(SRC_PATH)/pc-bios/optionrom/flat.lds -s -o $@ $<," Building $(TARGET_DIR)$@") %.raw: %.img $(call quiet-command,$(OBJCOPY) -O binary -j .text $< $@," Building $(TARGET_DIR)$@") diff --git a/pc-bios/optionrom/flat.lds b/pc-bios/optionrom/flat.lds new file mode 100644 index 0000000000..cee2eda195 --- /dev/null +++ b/pc-bios/optionrom/flat.lds @@ -0,0 +1,6 @@ +SECTIONS +{ + . = 0; + .text : { *(.text) *(.text.$) } +} +ENTRY(_start) From 5927ed846ad17e1cc8e9f60f50486ec418829776 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 9 Aug 2016 15:02:24 -0400 Subject: [PATCH 05/14] atomic: strip "const" from variables declared with typeof With the latest clang, we have the following warning: /home/pranith/devops/code/qemu/include/qemu/seqlock.h:62:21: warning: passing 'typeof (*&sl->sequence) *' (aka 'const unsigned int *') to parameter of type 'unsigned int *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers] return unlikely(atomic_read(&sl->sequence) != start); ^~~~~~~~~~~~~~~~~~~~~~~~~~ /home/pranith/devops/code/qemu/include/qemu/atomic.h:58:25: note: expanded from macro 'atomic_read' __atomic_load(ptr, &_val, __ATOMIC_RELAXED); \ ^~~~~ Stripping const is a bit tricky due to promotions, but it is doable with either C11 _Generic or GCC extensions. Use the latter. Reported-by: Pranith Kumar Signed-off-by: Paolo Bonzini [pranith: Add conversion for bool type] Signed-off-by: Pranith Kumar Signed-off-by: Paolo Bonzini --- include/qemu/atomic.h | 54 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 48 insertions(+), 6 deletions(-) diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h index 7e13fca351..43b06458f1 100644 --- a/include/qemu/atomic.h +++ b/include/qemu/atomic.h @@ -18,6 +18,48 @@ /* Compiler barrier */ #define barrier() ({ asm volatile("" ::: "memory"); (void)0; }) +/* The variable that receives the old value of an atomically-accessed + * variable must be non-qualified, because atomic builtins return values + * through a pointer-type argument as in __atomic_load(&var, &old, MODEL). + * + * This macro has to handle types smaller than int manually, because of + * implicit promotion. int and larger types, as well as pointers, can be + * converted to a non-qualified type just by applying a binary operator. + */ +#define typeof_strip_qual(expr) \ + typeof( \ + __builtin_choose_expr( \ + __builtin_types_compatible_p(typeof(expr), bool) || \ + __builtin_types_compatible_p(typeof(expr), const bool) || \ + __builtin_types_compatible_p(typeof(expr), volatile bool) || \ + __builtin_types_compatible_p(typeof(expr), const volatile bool), \ + (bool)1, \ + __builtin_choose_expr( \ + __builtin_types_compatible_p(typeof(expr), signed char) || \ + __builtin_types_compatible_p(typeof(expr), const signed char) || \ + __builtin_types_compatible_p(typeof(expr), volatile signed char) || \ + __builtin_types_compatible_p(typeof(expr), const volatile signed char), \ + (signed char)1, \ + __builtin_choose_expr( \ + __builtin_types_compatible_p(typeof(expr), unsigned char) || \ + __builtin_types_compatible_p(typeof(expr), const unsigned char) || \ + __builtin_types_compatible_p(typeof(expr), volatile unsigned char) || \ + __builtin_types_compatible_p(typeof(expr), const volatile unsigned char), \ + (unsigned char)1, \ + __builtin_choose_expr( \ + __builtin_types_compatible_p(typeof(expr), signed short) || \ + __builtin_types_compatible_p(typeof(expr), const signed short) || \ + __builtin_types_compatible_p(typeof(expr), volatile signed short) || \ + __builtin_types_compatible_p(typeof(expr), const volatile signed short), \ + (signed short)1, \ + __builtin_choose_expr( \ + __builtin_types_compatible_p(typeof(expr), unsigned short) || \ + __builtin_types_compatible_p(typeof(expr), const unsigned short) || \ + __builtin_types_compatible_p(typeof(expr), volatile unsigned short) || \ + __builtin_types_compatible_p(typeof(expr), const volatile unsigned short), \ + (unsigned short)1, \ + (expr)+0)))))) + #ifdef __ATOMIC_RELAXED /* For C11 atomic ops */ @@ -54,7 +96,7 @@ #define atomic_read(ptr) \ ({ \ QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ - typeof(*ptr) _val; \ + typeof_strip_qual(*ptr) _val; \ __atomic_load(ptr, &_val, __ATOMIC_RELAXED); \ _val; \ }) @@ -80,7 +122,7 @@ #define atomic_rcu_read(ptr) \ ({ \ QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ - typeof(*ptr) _val; \ + typeof_strip_qual(*ptr) _val; \ atomic_rcu_read__nocheck(ptr, &_val); \ _val; \ }) @@ -103,7 +145,7 @@ #define atomic_mb_read(ptr) \ ({ \ QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ - typeof(*ptr) _val; \ + typeof_strip_qual(*ptr) _val; \ __atomic_load(ptr, &_val, __ATOMIC_RELAXED); \ smp_rmb(); \ _val; \ @@ -120,7 +162,7 @@ #define atomic_mb_read(ptr) \ ({ \ QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ - typeof(*ptr) _val; \ + typeof_strip_qual(*ptr) _val; \ __atomic_load(ptr, &_val, __ATOMIC_SEQ_CST); \ _val; \ }) @@ -137,7 +179,7 @@ #define atomic_xchg(ptr, i) ({ \ QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ - typeof(*ptr) _new = (i), _old; \ + typeof_strip_qual(*ptr) _new = (i), _old; \ __atomic_exchange(ptr, &_new, &_old, __ATOMIC_SEQ_CST); \ _old; \ }) @@ -146,7 +188,7 @@ #define atomic_cmpxchg(ptr, old, new) \ ({ \ QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ - typeof(*ptr) _old = (old), _new = (new); \ + typeof_strip_qual(*ptr) _old = (old), _new = (new); \ __atomic_compare_exchange(ptr, &_old, &_new, false, \ __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); \ _old; \ From 435405ac59b9334b06285e1192c693b497282a31 Mon Sep 17 00:00:00 2001 From: Pranith Kumar Date: Tue, 9 Aug 2016 15:02:26 -0400 Subject: [PATCH 06/14] Disable warn about left shifts of negative values It seems like there's no good reason for the compiler to exploit the undefinedness of left shifts. GCC explicitly documents that they do not use at all this possibility and, while they also say this is subject to change, they have been saying this for 10 years (since the wording appeared in the GCC 4.0 manual). Disable these warnings by passing in -Wno-shift-negative-value. Cc: Peter Maydell Cc: Markus Armbruster Cc: Laszlo Ersek Signed-off-by: Paolo Bonzini [pranith: forward-port part of patch to 2.7] Signed-off-by: Pranith Kumar --- HACKING | 4 ++++ configure | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/HACKING b/HACKING index 058aa8fd49..20a910168d 100644 --- a/HACKING +++ b/HACKING @@ -158,6 +158,10 @@ painful. These are: * you may assume that right shift of a signed integer duplicates the sign bit (ie it is an arithmetic shift, not a logical shift) +In addition, QEMU assumes that the compiler does not use the latitude +given in C99 and C11 to treat aspects of signed '<<' as undefined, as +documented in the GNU Compiler Collection manual starting at version 4.0. + 7. Error handling and reporting 7.1 Reporting errors to the human user diff --git a/configure b/configure index f57fcc689d..8d849191db 100755 --- a/configure +++ b/configure @@ -1452,7 +1452,7 @@ fi gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits" gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags" gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs $gcc_flags" -gcc_flags="-Wendif-labels $gcc_flags" +gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags" gcc_flags="-Wno-initializer-overrides $gcc_flags" gcc_flags="-Wno-string-plus-int $gcc_flags" # Note that we do not add -Werror to gcc_flags here, because that would From 2368635d3943294c672a62abd60a233aca708982 Mon Sep 17 00:00:00 2001 From: Pranith Kumar Date: Tue, 9 Aug 2016 15:02:27 -0400 Subject: [PATCH 07/14] clang: Fix warning reg. expansion to 'defined' Clang produces the following warning. The warning is detailed here: https://reviews.llvm.org/D15866. Fix the warning. /home/pranith/devops/code/qemu/hw/display/qxl.c:507:5: warning: macro expansion producing 'defined' has undefined behavior [-Wexpansion-to-defined] ^ /home/pranith/devops/code/qemu/include/ui/qemu-spice.h:46:5: note: expanded from macro 'SPICE_NEEDS_SET_MM_TIME' (!defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06)) ^ /home/pranith/devops/code/qemu/hw/display/qxl.c:1074:5: warning: macro expansion producing 'defined' has undefined behavior [-Wexpansion-to-defined] ^ /home/pranith/devops/code/qemu/include/ui/qemu-spice.h:46:5: note: expanded from macro 'SPICE_NEEDS_SET_MM_TIME' (!defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06)) Suggested-by: Peter Maydell Signed-off-by: Pranith Kumar Signed-off-by: Paolo Bonzini --- include/ui/qemu-spice.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h index edad5e7bbf..75e12396bb 100644 --- a/include/ui/qemu-spice.h +++ b/include/ui/qemu-spice.h @@ -42,8 +42,11 @@ int qemu_spice_set_pw_expire(time_t expires); int qemu_spice_migrate_info(const char *hostname, int port, int tls_port, const char *subject); -#define SPICE_NEEDS_SET_MM_TIME \ - (!defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06)) +#if !defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06) +#define SPICE_NEEDS_SET_MM_TIME 1 +#else +#define SPICE_NEEDS_SET_MM_TIME 0 +#endif #if SPICE_SERVER_VERSION >= 0x000c02 void qemu_spice_register_ports(void); From 93bf13c6df4d6fd1a889cae890a9570eb00eb8e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radim=20Kr=C4=8Dm=C3=A1=C5=99?= Date: Tue, 9 Aug 2016 19:38:41 +0200 Subject: [PATCH 08/14] checkpatch: ignore automatically imported Linux headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Linux uses tabs for indentation and checkpatch always complained about automatically imported headers. update-linux-headers.sh could be modified to expand tabs, but there is no real reason to complain about any ugly code in Linux headers, so skip all hunk-related checks. Signed-off-by: Radim Krčmář Signed-off-by: Paolo Bonzini --- scripts/checkpatch.pl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 9297087212..8d1813eef6 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1319,6 +1319,9 @@ sub process { # ignore non-hunk lines and lines being removed next if (!$hunk_line || $line =~ /^-/); +# ignore files that are being periodically imported from Linux + next if ($realfile =~ /^(linux-headers|include\/standard-headers)\//); + #trailing whitespace if ($line =~ /^\+.*\015/) { my $herevet = "$here\n" . cat_vet($rawline) . "\n"; From 3fdd0ee393e26178a4892e101e60b011bbfaa9ea Mon Sep 17 00:00:00 2001 From: Gonglei Date: Tue, 9 Aug 2016 15:49:15 +0800 Subject: [PATCH 09/14] timer: set vm_clock disabled default (commit 80dcfb8532ae76343109a48f12ba8ca1c505c179) Upon migration, the code use a timer based on vm_clock for 1ns in the future from post_load to do the event send in case host_connected differs between migration source and target. However, it's not guaranteed that the apic is ready to inject irqs into the guest, and the irq line remained high, resulting in any future interrupts going unnoticed by the guest as well. That's because 1) the migration coroutine is not blocked when it get EAGAIN while reading QEMUFile. 2) The vm_clock is enabled default currently, it doesn't rely on the calling of vm_start(), that means vm_clock timers can run before VCPUs are running. So, let's set the vm_clock disabled default, keep the initial intention of design for vm_clock timers. Meanwhile, change the test-aio usecase, using QEMU_CLOCK_REALTIME instead of QEMU_CLOCK_VIRTUAL as the block code does. CC: Paolo Bonzini CC: Dr. David Alan Gilbert CC: qemu-stable@nongnu.org Signed-off-by: Gonglei Message-Id: <1470728955-90600-1-git-send-email-arei.gonglei@huawei.com> Signed-off-by: Paolo Bonzini --- qemu-timer.c | 2 +- tests/test-aio.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/qemu-timer.c b/qemu-timer.c index eb22e9218b..9299cdc5fb 100644 --- a/qemu-timer.c +++ b/qemu-timer.c @@ -129,7 +129,7 @@ static void qemu_clock_init(QEMUClockType type) assert(main_loop_tlg.tl[type] == NULL); clock->type = type; - clock->enabled = true; + clock->enabled = (type == QEMU_CLOCK_VIRTUAL ? false : true); clock->last = INT64_MIN; QLIST_INIT(&clock->timerlists); notifier_list_init(&clock->reset_notifiers); diff --git a/tests/test-aio.c b/tests/test-aio.c index 982339c801..03aa846970 100644 --- a/tests/test-aio.c +++ b/tests/test-aio.c @@ -452,7 +452,7 @@ static void test_timer_schedule(void) { TimerTestData data = { .n = 0, .ctx = ctx, .ns = SCALE_MS * 750LL, .max = 2, - .clock_type = QEMU_CLOCK_VIRTUAL }; + .clock_type = QEMU_CLOCK_REALTIME }; EventNotifier e; /* aio_poll will not block to wait for timers to complete unless it has @@ -782,7 +782,7 @@ static void test_source_timer_schedule(void) { TimerTestData data = { .n = 0, .ctx = ctx, .ns = SCALE_MS * 750LL, .max = 2, - .clock_type = QEMU_CLOCK_VIRTUAL }; + .clock_type = QEMU_CLOCK_REALTIME }; EventNotifier e; int64_t expiry; From 906fb135e4b875465c424cb9b2b47d90265f7897 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 9 Aug 2016 17:47:42 +0200 Subject: [PATCH 10/14] checkpatch: tweak the files in which TABs are checked Include Python and shell scripts, and make an exception for Perl scripts we imported from Linux or elsewhere. Acked-by: Cornelia Huck Signed-off-by: Paolo Bonzini --- scripts/checkpatch.pl | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 8d1813eef6..082c4cecc9 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1334,7 +1334,7 @@ sub process { } # check we are in a valid source file if not then ignore this hunk - next if ($realfile !~ /\.(h|c|cpp|s|S|pl|sh)$/); + next if ($realfile !~ /\.(h|c|cpp|s|S|pl|py|sh)$/); #80 column limit if ($line =~ /^\+/ && @@ -1354,10 +1354,11 @@ sub process { WARN("adding a line without newline at end of file\n" . $herecurr); } -# check we are in a valid source file C or perl if not then ignore this hunk - next if ($realfile !~ /\.(h|c|cpp|pl)$/); +# tabs are only allowed in assembly source code, and in +# some scripts we imported from other projects. + next if ($realfile =~ /\.(s|S)$/); + next if ($realfile =~ /(checkpatch|get_maintainer|texi2pod)\.pl$/); -# in QEMU, no tabs are allowed if ($rawline =~ /^\+.*\t/) { my $herevet = "$here\n" . cat_vet($rawline) . "\n"; ERROR("code indent should never use tabs\n" . $herevet); From 93eb8e31f38b3eb612e522c62e8932d7fd576ff9 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 10 Aug 2016 10:05:03 +0200 Subject: [PATCH 11/14] checkpatch: check for CVS keywords on all sources These should apply to all files, not just C/C++. Tweak the regular expression to check for whole words, to avoid false positives on Perl variables starting with "Id". Signed-off-by: Paolo Bonzini --- scripts/checkpatch.pl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 082c4cecc9..fea576d1b4 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1354,6 +1354,11 @@ sub process { WARN("adding a line without newline at end of file\n" . $herecurr); } +# check for RCS/CVS revision markers + if ($rawline =~ /^\+.*\$(Revision|Log|Id)(?:\$|\b)/) { + WARN("CVS style keyword markers, these will _not_ be updated\n". $herecurr); + } + # tabs are only allowed in assembly source code, and in # some scripts we imported from other projects. next if ($realfile =~ /\.(s|S)$/); @@ -1368,11 +1373,6 @@ sub process { # check we are in a valid C source file if not then ignore this hunk next if ($realfile !~ /\.(h|c|cpp)$/); -# check for RCS/CVS revision markers - if ($rawline =~ /^\+.*\$(Revision|Log|Id)(?:\$|)/) { - WARN("CVS style keyword markers, these will _not_ be updated\n". $herecurr); - } - # Check for potential 'bare' types my ($stat, $cond, $line_nr_next, $remain_next, $off_next, $realline_next); From 8fbe3d1fcfa16c543f49f24e7cdfbf0024459341 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 7 Sep 2015 11:53:02 +0200 Subject: [PATCH 12/14] CODING_STYLE, checkpatch: update line length rules Line lengths above 80 characters do exist. They are rare, but they happen from time to time. An ignored rule is worse than an exception to the rule, so do the latter. Some on the list expressed their preference for a soft limit that is slightly lower than 80 characters, to account for extra characters in unified diffs (including three-way diffs) and for email quoting. However, there was no consensus on this so keep the 80-character soft limit and add a hard limit at 90. Acked-by: Cornelia Huck Reviewed-by: Thomas Huth Signed-off-by: Paolo Bonzini --- CODING_STYLE | 8 +++++++- scripts/checkpatch.pl | 8 ++++++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/CODING_STYLE b/CODING_STYLE index 3c6978f836..e7fde15003 100644 --- a/CODING_STYLE +++ b/CODING_STYLE @@ -31,7 +31,11 @@ Do not leave whitespace dangling off the ends of lines. 2. Line width -Lines are 80 characters; not longer. +Lines should be 80 characters; try not to make them longer. + +Sometimes it is hard to do, especially when dealing with QEMU subsystems +that use long function or symbol names. Even in that case, do not make +lines much longer than 80 characters. Rationale: - Some people like to tile their 24" screens with a 6x4 matrix of 80x24 @@ -39,6 +43,8 @@ Rationale: let them keep doing it. - Code and especially patches is much more readable if limited to a sane line length. Eighty is traditional. + - The four-space indentation makes the most common excuse ("But look + at all that white space on the left!") moot. - It is the QEMU coding style. 3. Naming diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index fea576d1b4..ba6760d4b3 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1336,12 +1336,16 @@ sub process { # check we are in a valid source file if not then ignore this hunk next if ($realfile !~ /\.(h|c|cpp|s|S|pl|py|sh)$/); -#80 column limit +#90 column limit if ($line =~ /^\+/ && !($line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) && $length > 80) { - WARN("line over 80 characters\n" . $herecurr); + if ($length > 90) { + ERROR("line over 90 characters\n" . $herecurr); + } else { + WARN("line over 80 characters\n" . $herecurr); + } } # check for spaces before a quoted newline From c2df8783251e16279229520f7cf5cb81c188a934 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 9 Aug 2016 17:47:43 +0200 Subject: [PATCH 13/14] checkpatch: bump most warnings to errors This only leaves a warning-level message for the extra-long lines soft limit. Everything else is bumped up. In the future warnings can be added for checks that can have false positives. Reviewed-by: Markus Armbruster Signed-off-by: Paolo Bonzini --- scripts/checkpatch.pl | 66 +++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index ba6760d4b3..714dbbe9c1 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1289,11 +1289,11 @@ sub process { # This is a signoff, if ugly, so do not double report. $signoff++; if (!($line =~ /^\s*Signed-off-by:/)) { - WARN("Signed-off-by: is the preferred form\n" . + ERROR("The correct form is \"Signed-off-by\"\n" . $herecurr); } if ($line =~ /^\s*signed-off-by:\S/i) { - WARN("space required after Signed-off-by:\n" . + ERROR("space required after Signed-off-by:\n" . $herecurr); } } @@ -1350,17 +1350,17 @@ sub process { # check for spaces before a quoted newline if ($rawline =~ /^.*\".*\s\\n/) { - WARN("unnecessary whitespace before a quoted newline\n" . $herecurr); + ERROR("unnecessary whitespace before a quoted newline\n" . $herecurr); } # check for adding lines without a newline. if ($line =~ /^\+/ && defined $lines[$linenr] && $lines[$linenr] =~ /^\\ No newline at end of file/) { - WARN("adding a line without newline at end of file\n" . $herecurr); + ERROR("adding a line without newline at end of file\n" . $herecurr); } # check for RCS/CVS revision markers if ($rawline =~ /^\+.*\$(Revision|Log|Id)(?:\$|\b)/) { - WARN("CVS style keyword markers, these will _not_ be updated\n". $herecurr); + ERROR("CVS style keyword markers, these will _not_ be updated\n". $herecurr); } # tabs are only allowed in assembly source code, and in @@ -1506,7 +1506,7 @@ sub process { { my ($nlength, $nindent) = line_stats($lines[$ctx_ln - 1]); if ($nindent > $indent) { - WARN("trailing semicolon indicates no statements, indent implies otherwise\n" . + ERROR("trailing semicolon indicates no statements, indent implies otherwise\n" . "$here\n$ctx\n$rawlines[$ctx_ln - 1]\n"); } } @@ -1594,7 +1594,7 @@ sub process { if ($check && (($sindent % 4) != 0 || ($sindent <= $indent && $s ne ''))) { - WARN("suspect code indent for conditional statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n"); + ERROR("suspect code indent for conditional statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n"); } } @@ -1772,7 +1772,7 @@ sub process { } elsif ($ctx =~ /$Type$/) { } else { - WARN("space prohibited between function name and open parenthesis '('\n" . $herecurr); + ERROR("space prohibited between function name and open parenthesis '('\n" . $herecurr); } } # Check operator spacing. @@ -2011,7 +2011,7 @@ sub process { if ($line =~ /^.\s*return\s*(E[A-Z]*)\s*;/) { my $name = $1; if ($name ne 'EOF' && $name ne 'ERROR') { - WARN("return of an errno should typically be -ve (return -$1)\n" . $herecurr); + ERROR("return of an errno should typically be -ve (return -$1)\n" . $herecurr); } } @@ -2083,7 +2083,7 @@ sub process { (?:\&\&|\|\||\)|\]) )/x) { - WARN("boolean test with hexadecimal, perhaps just 1 \& or \|?\n" . $herecurr); + ERROR("boolean test with hexadecimal, perhaps just 1 \& or \|?\n" . $herecurr); } # if and else should not have general statements after it @@ -2139,7 +2139,7 @@ sub process { #no spaces allowed after \ in define if ($line=~/\#\s*define.*\\\s$/) { - WARN("Whitepspace after \\ makes next lines useless\n" . $herecurr); + ERROR("Whitespace after \\ makes next lines useless\n" . $herecurr); } # multi-statement macros should be enclosed in a do while loop, grab the @@ -2291,7 +2291,7 @@ sub process { } } if ($seen != ($#chunks + 1)) { - WARN("braces {} are necessary for all arms of this statement\n" . $herectx); + ERROR("braces {} are necessary for all arms of this statement\n" . $herectx); } } } @@ -2359,19 +2359,19 @@ sub process { $herectx .= raw_line($linenr, $n) . "\n";; } - WARN("braces {} are necessary even for single statement blocks\n" . $herectx); + ERROR("braces {} are necessary even for single statement blocks\n" . $herectx); } } # no volatiles please my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b}; if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) { - WARN("Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr); + ERROR("Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr); } # warn about #if 0 if ($line =~ /^.\s*\#\s*if\s+0\b/) { - WARN("if this code is redundant consider removing it\n" . + ERROR("if this code is redundant consider removing it\n" . $herecurr); } @@ -2379,7 +2379,7 @@ sub process { if ($prevline =~ /\bif\s*\(([^\)]*)\)/) { my $expr = $1; if ($line =~ /\bg_free\(\Q$expr\E\);/) { - WARN("g_free(NULL) is safe this check is probably not required\n" . $hereprev); + ERROR("g_free(NULL) is safe this check is probably not required\n" . $hereprev); } } @@ -2397,19 +2397,19 @@ sub process { # check for memory barriers without a comment. if ($line =~ /\b(smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) { if (!ctx_has_comment($first_line, $linenr)) { - WARN("memory barrier without comment\n" . $herecurr); + ERROR("memory barrier without comment\n" . $herecurr); } } # check of hardware specific defines # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases # where they might be necessary. if ($line =~ m@^.\s*\#\s*if.*\b__@) { - WARN("architecture specific defines should be avoided\n" . $herecurr); + ERROR("architecture specific defines should be avoided\n" . $herecurr); } # Check that the storage class is at the beginning of a declaration if ($line =~ /\b$Storage\b/ && $line !~ /^.\s*$Storage\b/) { - WARN("storage class should be at the beginning of the declaration\n" . $herecurr) + ERROR("storage class should be at the beginning of the declaration\n" . $herecurr) } # check the location of the inline attribute, that it is between @@ -2421,7 +2421,7 @@ sub process { # check for sizeof(&) if ($line =~ /\bsizeof\s*\(\s*\&/) { - WARN("sizeof(& should be avoided\n" . $herecurr); + ERROR("sizeof(& should be avoided\n" . $herecurr); } # check for new externs in .c files. @@ -2438,40 +2438,40 @@ sub process { if ($s =~ /^\s*;/ && $function_name ne 'uninitialized_var') { - WARN("externs should be avoided in .c files\n" . $herecurr); + ERROR("externs should be avoided in .c files\n" . $herecurr); } if ($paren_space =~ /\n/) { - WARN("arguments for function declarations should follow identifier\n" . $herecurr); + ERROR("arguments for function declarations should follow identifier\n" . $herecurr); } } elsif ($realfile =~ /\.c$/ && defined $stat && $stat =~ /^.\s*extern\s+/) { - WARN("externs should be avoided in .c files\n" . $herecurr); + ERROR("externs should be avoided in .c files\n" . $herecurr); } # check for pointless casting of g_malloc return if ($line =~ /\*\s*\)\s*g_(try)?(m|re)alloc(0?)(_n)?\b/) { if ($2 == 'm') { - WARN("unnecessary cast may hide bugs, use g_$1new$3 instead\n" . $herecurr); + ERROR("unnecessary cast may hide bugs, use g_$1new$3 instead\n" . $herecurr); } else { - WARN("unnecessary cast may hide bugs, use g_$1renew$3 instead\n" . $herecurr); + ERROR("unnecessary cast may hide bugs, use g_$1renew$3 instead\n" . $herecurr); } } # check for gcc specific __FUNCTION__ if ($line =~ /__FUNCTION__/) { - WARN("__func__ should be used instead of gcc specific __FUNCTION__\n" . $herecurr); + ERROR("__func__ should be used instead of gcc specific __FUNCTION__\n" . $herecurr); } # recommend qemu_strto* over strto* for numeric conversions if ($line =~ /\b(strto[^kd].*?)\s*\(/) { - WARN("consider using qemu_$1 in preference to $1\n" . $herecurr); + ERROR("consider using qemu_$1 in preference to $1\n" . $herecurr); } # check for module_init(), use category-specific init macros explicitly please if ($line =~ /^module_init\s*\(/) { - WARN("please use block_init(), type_init() etc. instead of module_init()\n" . $herecurr); + ERROR("please use block_init(), type_init() etc. instead of module_init()\n" . $herecurr); } # check for various ops structs, ensure they are const. my $struct_ops = qr{AIOCBInfo| @@ -2496,7 +2496,7 @@ sub process { VMStateInfo}x; if ($line !~ /\bconst\b/ && $line =~ /\b($struct_ops)\b/) { - WARN("struct $1 should normally be const\n" . + ERROR("struct $1 should normally be const\n" . $herecurr); } @@ -2506,14 +2506,14 @@ sub process { $string = substr($rawline, $-[1], $+[1] - $-[1]); $string =~ s/%%/__/g; if ($string =~ /(? Date: Tue, 9 Aug 2016 17:47:44 +0200 Subject: [PATCH 14/14] checkpatch: default to success if only warnings CHK-level checks have been removed from checkpatch or bumped to errors, so there is no effect anymore for --strict/--subjective. Furthermore, even most WARNs have been bumped to errors, with WARN only reserved to things that patchew probably ought not to complain about (and that maintainers probably will notice anyway during review if they are extreme). Default to exiting with success even if there are WARN-level failures, and cause --strict to fail for warnings. Maintainers that want to have a strict 80-character limit for their subsystem can add it to a commit hook for example. The --subjective synonym is removed. Reviewed-by: Markus Armbruster Signed-off-by: Paolo Bonzini --- scripts/checkpatch.pl | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 714dbbe9c1..b0096a4460 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -22,7 +22,7 @@ my $tst_only; my $emacs = 0; my $terse = 0; my $file = 0; -my $check = 0; +my $no_warnings = 0; my $summary = 1; my $mailback = 0; my $summary_file = 0; @@ -45,7 +45,7 @@ Options: --emacs emacs compile window format --terse one line per report -f, --file treat FILE as regular source file - --subjective, --strict enable more subjective tests + --strict fail if only warnings are found --root=PATH PATH to the kernel tree root --no-summary suppress the per-file summary --mailback only produce a report in case of warnings/errors @@ -71,8 +71,7 @@ GetOptions( 'emacs!' => \$emacs, 'terse!' => \$terse, 'f|file!' => \$file, - 'subjective!' => \$check, - 'strict!' => \$check, + 'strict!' => \$no_warnings, 'root=s' => \$root, 'summary!' => \$summary, 'mailback!' => \$mailback, @@ -1072,12 +1071,6 @@ sub WARN { our $cnt_warn++; } } -sub CHK { - if ($check && report("CHECK: $_[0]\n")) { - our $clean = 0; - our $cnt_chk++; - } -} sub process { my $filename = shift; @@ -2599,7 +2592,6 @@ sub process { if ($summary && !($clean == 1 && $quiet == 1)) { print "$filename " if ($summary_file); print "total: $cnt_error errors, $cnt_warn warnings, " . - (($check)? "$cnt_chk checks, " : "") . "$cnt_lines lines checked\n"; print "\n" if ($quiet == 0); } @@ -2622,5 +2614,5 @@ sub process { print "CHECKPATCH in MAINTAINERS.\n"; } - return $clean; + return ($no_warnings ? $clean : $cnt_error == 0); }