* [PULL v2 00/31] Misc patches for 2020-06-24
@ 2020-06-26 13:56 Paolo Bonzini
2020-06-26 13:56 ` [PULL v2 09/31] exec: fetch the alignment of Linux devdax pmem character device nodes Paolo Bonzini
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Paolo Bonzini @ 2020-06-26 13:56 UTC (permalink / raw)
To: qemu-devel
The following changes since commit 5acc270a355120ce967ca1f1eeca0abbdb9303c8:
Merge remote-tracking branch 'remotes/xtensa/tags/20200625-xtensa' into staging (2020-06-25 21:20:45 +0100)
are available in the Git repository at:
git://github.com/bonzini/qemu.git tags/for-upstream
for you to fetch changes up to 730319aef0fcb94f11a4a2d32656437fdde7efdd:
i386: Mask SVM features if nested SVM is disabled (2020-06-26 09:39:40 -0400)
----------------------------------------------------------------
* Various fixes
* libdaxctl support to correctly align devdax character devices (Jingqi)
* initial-all-set support for live migration (Jay)
* forbid '-numa node, mem' for 5.1 and newer machine types (Igor)
* x87 fixes (Joseph)
* Tighten memory_region_access_valid (Michael) and fix fallout (myself)
* Replay fixes (Pavel)
----------------------------------------------------------------
v1->v2: update MIN/MAX patch, fix 32-bit compilation
Anthony PERARD (1):
xen: Actually fix build without passthrough
David CARLIER (1):
util/getauxval: Porting to FreeBSD getauxval feature
Eduardo Habkost (1):
i386: Mask SVM features if nested SVM is disabled
Eric Blake (1):
osdep: Make MIN/MAX evaluate arguments only once
Igor Mammedov (1):
numa: forbid '-numa node, mem' for 5.1 and newer machine types
Jay Zhou (1):
kvm: support to get/set dirty log initial-all-set capability
Jingqi Liu (3):
configure: add libdaxctl support
exec: fetch the alignment of Linux devdax pmem character device nodes
docs/nvdimm: add description of alignment requirement of device dax
Jon Doron (1):
hyperv: vmbus: Remove the 2nd IRQ
Joseph Myers (10):
target/i386: reimplement f2xm1 using floatx80 operations
softfloat: merge floatx80_mod and floatx80_rem
softfloat: fix floatx80 remainder pseudo-denormal check for zero
softfloat: do not return pseudo-denormal from floatx80 remainder
softfloat: do not set denominator high bit for floatx80 remainder
softfloat: return low bits of quotient from floatx80_modrem
target/i386: reimplement fprem, fprem1 using floatx80 operations
target/i386: reimplement fyl2xp1 using floatx80 operations
target/i386: reimplement fyl2x using floatx80 operations
target/i386: reimplement fpatan using floatx80 operations
Liao Pingfang (1):
Makefile: Install qemu-[qmp/ga]-ref.* into the directory "interop"
Marcelo Tosatti (1):
kvm: i386: allow TSC to differ by NTP correction bounds without TSC scaling
Michael S. Tsirkin (1):
memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"
Paolo Bonzini (4):
libqos: usb-hcd-ehci: use 32-bit write for config register
libqos: pci-pc: use 32-bit write for EJ register
vmport: move compat properties to hw_compat_5_0
ibex_uart: fix XOR-as-pow
Pavel Dovgaluk (2):
replay: notify the main loop when there are no instructions
replay: synchronize on every virtual timer callback
Tao Xu (1):
target/i386: Add notes for versioned CPU models
Thomas Huth (1):
hw/scsi/megasas: Fix possible out-of-bounds array access in tracepoints
Makefile | 10 +-
accel/kvm/kvm-all.c | 21 +-
accel/tcg/translate-all.c | 6 +-
configure | 29 +
cpus.c | 15 +-
docs/index.html.in | 4 +-
docs/nvdimm.txt | 10 +
docs/system/deprecated.rst | 37 +-
exec.c | 54 +-
fpu/softfloat.c | 87 ++-
hw/arm/virt.c | 2 +-
hw/char/ibex_uart.c | 2 +-
hw/core/machine.c | 8 +-
hw/core/numa.c | 7 +
hw/hyperv/vmbus.c | 3 +-
hw/i386/acpi-build.c | 4 +-
hw/i386/pc.c | 1 -
hw/i386/pc_piix.c | 1 +
hw/i386/pc_q35.c | 1 +
hw/ppc/spapr.c | 2 +-
hw/scsi/megasas.c | 36 +-
hw/usb/hcd-xhci.h | 2 +-
hw/xen/Makefile.objs | 2 +-
include/block/block.h | 4 +-
include/exec/cpu-all.h | 8 +-
include/exec/cpu-defs.h | 7 +-
include/fpu/softfloat.h | 3 +
include/hw/hyperv/vmbus-bridge.h | 3 +-
include/qemu/osdep.h | 57 +-
memory.c | 29 +-
migration/qemu-file.c | 2 +-
qemu-options.hx | 9 +-
replay/replay.c | 2 +-
target/i386/cpu.c | 9 +
target/i386/fpu_helper.c | 1396 +++++++++++++++++++++++++++++++----
target/i386/kvm.c | 46 +-
target/m68k/softfloat.c | 83 ---
target/m68k/softfloat.h | 1 -
tests/qtest/libqos/pci-pc.c | 2 +-
tests/qtest/test-x86-cpuid-compat.c | 4 +-
tests/qtest/usb-hcd-ehci-test.c | 2 +-
tests/tcg/i386/test-i386-f2xm1.c | 1140 ++++++++++++++++++++++++++++
tests/tcg/i386/test-i386-fpatan.c | 1071 +++++++++++++++++++++++++++
tests/tcg/i386/test-i386-fyl2x.c | 1161 +++++++++++++++++++++++++++++
tests/tcg/i386/test-i386-fyl2xp1.c | 1156 +++++++++++++++++++++++++++++
util/getauxval.c | 10 +
util/qemu-timer.c | 32 +-
47 files changed, 6211 insertions(+), 370 deletions(-)
create mode 100644 tests/tcg/i386/test-i386-f2xm1.c
create mode 100644 tests/tcg/i386/test-i386-fpatan.c
create mode 100644 tests/tcg/i386/test-i386-fyl2x.c
create mode 100644 tests/tcg/i386/test-i386-fyl2xp1.c
--
2.26.2
^ permalink raw reply [flat|nested] 8+ messages in thread* [PULL v2 09/31] exec: fetch the alignment of Linux devdax pmem character device nodes 2020-06-26 13:56 [PULL v2 00/31] Misc patches for 2020-06-24 Paolo Bonzini @ 2020-06-26 13:56 ` Paolo Bonzini 2020-06-26 13:56 ` [PULL v2 25/31] osdep: Make MIN/MAX evaluate arguments only once Paolo Bonzini 2020-06-26 17:22 ` [PULL v2 00/31] Misc patches for 2020-06-24 Peter Maydell 2 siblings, 0 replies; 8+ messages in thread From: Paolo Bonzini @ 2020-06-26 13:56 UTC (permalink / raw) To: qemu-devel; +Cc: Jingqi Liu, Dan Williams, Joao Martins From: Jingqi Liu <jingqi.liu@intel.com> If the backend file is devdax pmem character device, the alignment specified by the option 'align=NUM' in the '-object memory-backend-file' needs to match the alignment requirement of the devdax pmem character device. This patch uses the interfaces of libdaxctl to fetch the devdax pmem file 'align', so that we can compare it with the NUM of 'align=NUM'. The NUM needs to be larger than or equal to the devdax pmem file 'align'. It also fixes the problem that mmap() returns failure in qemu_ram_mmap() when the NUM of 'align=NUM' is less than the devdax pmem file 'align'. Suggested-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Joao Martins <joao.m.martins@oracle.com> Signed-off-by: Jingqi Liu <jingqi.liu@intel.com> Message-Id: <20200429085011.63752-2-jingqi.liu@intel.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- exec.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/exec.c b/exec.c index d6712fba7e..21926dc9c7 100644 --- a/exec.c +++ b/exec.c @@ -77,6 +77,10 @@ #include "monitor/monitor.h" +#ifdef CONFIG_LIBDAXCTL +#include <daxctl/libdaxctl.h> +#endif + //#define DEBUG_SUBPAGE #if !defined(CONFIG_USER_ONLY) @@ -1745,6 +1749,46 @@ static int64_t get_file_size(int fd) return size; } +static int64_t get_file_align(int fd) +{ + int64_t align = -1; +#if defined(__linux__) && defined(CONFIG_LIBDAXCTL) + struct stat st; + + if (fstat(fd, &st) < 0) { + return -errno; + } + + /* Special handling for devdax character devices */ + if (S_ISCHR(st.st_mode)) { + g_autofree char *path = NULL; + g_autofree char *rpath = NULL; + struct daxctl_ctx *ctx; + struct daxctl_region *region; + int rc = 0; + + path = g_strdup_printf("/sys/dev/char/%d:%d", + major(st.st_rdev), minor(st.st_rdev)); + rpath = realpath(path, NULL); + + rc = daxctl_new(&ctx); + if (rc) { + return -1; + } + + daxctl_region_foreach(ctx, region) { + if (strstr(rpath, daxctl_region_get_path(region))) { + align = daxctl_region_get_align(region); + break; + } + } + daxctl_unref(ctx); + } +#endif /* defined(__linux__) && defined(CONFIG_LIBDAXCTL) */ + + return align; +} + static int file_ram_open(const char *path, const char *region_name, bool *created, @@ -2296,7 +2340,7 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr, { RAMBlock *new_block; Error *local_err = NULL; - int64_t file_size; + int64_t file_size, file_align; /* Just support these ram flags by now. */ assert((ram_flags & ~(RAM_SHARED | RAM_PMEM)) == 0); @@ -2332,6 +2376,14 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr, return NULL; } + file_align = get_file_align(fd); + if (file_align > 0 && mr && file_align > mr->align) { + error_setg(errp, "backing store align 0x%" PRIx64 + " is larger than 'align' option 0x%" PRIx64, + file_align, mr->align); + return NULL; + } + new_block = g_malloc0(sizeof(*new_block)); new_block->mr = mr; new_block->used_length = size; -- 2.26.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PULL v2 25/31] osdep: Make MIN/MAX evaluate arguments only once 2020-06-26 13:56 [PULL v2 00/31] Misc patches for 2020-06-24 Paolo Bonzini 2020-06-26 13:56 ` [PULL v2 09/31] exec: fetch the alignment of Linux devdax pmem character device nodes Paolo Bonzini @ 2020-06-26 13:56 ` Paolo Bonzini 2020-06-27 21:35 ` Peter Maydell 2020-06-26 17:22 ` [PULL v2 00/31] Misc patches for 2020-06-24 Peter Maydell 2 siblings, 1 reply; 8+ messages in thread From: Paolo Bonzini @ 2020-06-26 13:56 UTC (permalink / raw) To: qemu-devel; +Cc: Philippe Mathieu-Daudé From: Eric Blake <eblake@redhat.com> I'm not aware of any immediate bugs in qemu where a second runtime evaluation of the arguments to MIN() or MAX() causes a problem, but proactively preventing such abuse is easier than falling prey to an unintended case down the road. At any rate, here's the conversation that sparked the current patch: https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05718.html Update the MIN/MAX macros to only evaluate their argument once at runtime; this uses typeof(1 ? (a) : (b)) to ensure that we are promoting the temporaries to the same type as the final comparison (we have to trigger type promotion, as typeof(bitfield) won't compile; and we can't use typeof((a) + (b)) or even typeof((a) + 0), as some of our uses of MAX are on void* pointers where such addition is undefined). However, we are unable to work around gcc refusing to compile ({}) in a constant context (such as the array length of a static variable), even when only used in the dead branch of a __builtin_choose_expr(), so we have to provide a second macro pair MIN_CONST and MAX_CONST for use when both arguments are known to be compile-time constants and where the result must also be usable as a constant; this second form evaluates arguments multiple times but that doesn't matter for constants. By using a void expression as the expansion if a non-constant is presented to this second form, we can enlist the compiler to ensure the double evaluation is not attempted on non-constants. Alas, as both macros now rely on compiler intrinsics, they are no longer usable in preprocessor #if conditions; those will just have to be open-coded or the logic rewritten into #define or runtime 'if' conditions (but where the compiler dead-code-elimination will probably still apply). I tested that both gcc 10.1.1 and clang 10.0.0 produce errors for all forms of macro mis-use. As the errors can sometimes be cryptic, I'm demonstrating the gcc output: Use of MIN when MIN_CONST is needed: In file included from /home/eblake/qemu/qemu-img.c:25: /home/eblake/qemu/include/qemu/osdep.h:249:5: error: braced-group within expression allowed only inside a function 249 | ({ \ | ^ /home/eblake/qemu/qemu-img.c:92:12: note: in expansion of macro ‘MIN’ 92 | char array[MIN(1, 2)] = ""; | ^~~ Use of MIN_CONST when MIN is needed: /home/eblake/qemu/qemu-img.c: In function ‘is_allocated_sectors’: /home/eblake/qemu/qemu-img.c:1225:15: error: void value not ignored as it ought to be 1225 | i = MIN_CONST(i, n); | ^ Use of MIN in the preprocessor: In file included from /home/eblake/qemu/accel/tcg/translate-all.c:20: /home/eblake/qemu/accel/tcg/translate-all.c: In function ‘page_check_range’: /home/eblake/qemu/include/qemu/osdep.h:249:6: error: token "{" is not valid in preprocessor expressions 249 | ({ \ | ^ Fix the resulting callsites that used #if or computed a compile-time constant min or max to use the new macros. cpu-defs.h is interesting, as CPU_TLB_DYN_MAX_BITS is sometimes used as a constant and sometimes dynamic. It may be worth improving glib's MIN/MAX definitions to be saner, but that is a task for another day. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> Message-Id: <20200625162602.700741-1-eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- accel/tcg/translate-all.c | 6 ++--- hw/usb/hcd-xhci.h | 2 +- include/block/block.h | 4 +-- include/exec/cpu-all.h | 8 +++--- include/exec/cpu-defs.h | 7 ++++- include/qemu/osdep.h | 57 ++++++++++++++++++++++++++++++++------- migration/qemu-file.c | 2 +- 7 files changed, 63 insertions(+), 23 deletions(-) diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index c3d37058a1..2afa46bd2b 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -2582,9 +2582,9 @@ int page_check_range(target_ulong start, target_ulong len, int flags) /* This function should never be called with addresses outside the guest address space. If this assert fires, it probably indicates a missing call to h2g_valid. */ -#if TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS - assert(start < ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS)); -#endif + if (TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS) { + assert(start < ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS)); + } if (len == 0) { return 0; diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h index 2fad4df2a7..946af51fc2 100644 --- a/hw/usb/hcd-xhci.h +++ b/hw/usb/hcd-xhci.h @@ -214,7 +214,7 @@ struct XHCIState { uint32_t dcbaap_high; uint32_t config; - USBPort uports[MAX(MAXPORTS_2, MAXPORTS_3)]; + USBPort uports[MAX_CONST(MAXPORTS_2, MAXPORTS_3)]; XHCIPort ports[MAXPORTS]; XHCISlot slots[MAXSLOTS]; uint32_t numports; diff --git a/include/block/block.h b/include/block/block.h index 25e299605e..e8fc814996 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -133,8 +133,8 @@ typedef struct HDGeometry { #define BDRV_SECTOR_BITS 9 #define BDRV_SECTOR_SIZE (1ULL << BDRV_SECTOR_BITS) -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \ - INT_MAX >> BDRV_SECTOR_BITS) +#define BDRV_REQUEST_MAX_SECTORS MIN_CONST(SIZE_MAX >> BDRV_SECTOR_BITS, \ + INT_MAX >> BDRV_SECTOR_BITS) #define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) /* diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h index fb4e8a8e29..fc403d456b 100644 --- a/include/exec/cpu-all.h +++ b/include/exec/cpu-all.h @@ -176,11 +176,9 @@ extern unsigned long reserved_va; * avoid setting bits at the top of guest addresses that might need * to be used for tags. */ -#if MIN(TARGET_VIRT_ADDR_SPACE_BITS, TARGET_ABI_BITS) <= 32 -# define GUEST_ADDR_MAX_ UINT32_MAX -#else -# define GUEST_ADDR_MAX_ (~0ul) -#endif +#define GUEST_ADDR_MAX_ \ + ((MIN_CONST(TARGET_VIRT_ADDR_SPACE_BITS, TARGET_ABI_BITS) <= 32) ? \ + UINT32_MAX : ~0ul) #define GUEST_ADDR_MAX (reserved_va ? reserved_va - 1 : GUEST_ADDR_MAX_) #else diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h index 8c44abefa2..9185632337 100644 --- a/include/exec/cpu-defs.h +++ b/include/exec/cpu-defs.h @@ -102,8 +102,13 @@ typedef uint64_t target_ulong; * Skylake's Level-2 STLB has 16 1G entries. * Also, make sure we do not size the TLB past the guest's address space. */ -# define CPU_TLB_DYN_MAX_BITS \ +# ifdef TARGET_PAGE_BITS_VARY +# define CPU_TLB_DYN_MAX_BITS \ MIN(22, TARGET_VIRT_ADDR_SPACE_BITS - TARGET_PAGE_BITS) +# else +# define CPU_TLB_DYN_MAX_BITS \ + MIN_CONST(22, TARGET_VIRT_ADDR_SPACE_BITS - TARGET_PAGE_BITS) +# endif # endif typedef struct CPUTLBEntry { diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index ff7c17b857..0d26a1b9bd 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -236,18 +236,55 @@ extern int daemon(int, int); #define SIZE_MAX ((size_t)-1) #endif -#ifndef MIN -#define MIN(a, b) (((a) < (b)) ? (a) : (b)) -#endif -#ifndef MAX -#define MAX(a, b) (((a) > (b)) ? (a) : (b)) -#endif +/* + * Two variations of MIN/MAX macros. The first is for runtime use, and + * evaluates arguments only once (so it is safe even with side + * effects), but will not work in constant contexts (such as array + * size declarations) because of the '{}'. The second is for constant + * expression use, where evaluating arguments twice is safe because + * the result is going to be constant anyway, but will not work in a + * runtime context because of a void expression where a value is + * expected. Thus, both gcc and clang will fail to compile if you use + * the wrong macro (even if the error may seem a bit cryptic). + * + * Note that neither form is usable as an #if condition; if you truly + * need to write conditional code that depends on a minimum or maximum + * determined by the pre-processor instead of the compiler, you'll + * have to open-code it. + */ +#undef MIN +#define MIN(a, b) \ + ({ \ + typeof(1 ? (a) : (b)) _a = (a), _b = (b); \ + _a < _b ? _a : _b; \ + }) +#define MIN_CONST(a, b) \ + __builtin_choose_expr( \ + __builtin_constant_p(a) && __builtin_constant_p(b), \ + (a) < (b) ? (a) : (b), \ + ((void)0)) +#undef MAX +#define MAX(a, b) \ + ({ \ + typeof(1 ? (a) : (b)) _a = (a), _b = (b); \ + _a > _b ? _a : _b; \ + }) +#define MAX_CONST(a, b) \ + __builtin_choose_expr( \ + __builtin_constant_p(a) && __builtin_constant_p(b), \ + (a) > (b) ? (a) : (b), \ + ((void)0)) -/* Minimum function that returns zero only iff both values are zero. - * Intended for use with unsigned values only. */ +/* + * Minimum function that returns zero only if both values are zero. + * Intended for use with unsigned values only. + */ #ifndef MIN_NON_ZERO -#define MIN_NON_ZERO(a, b) ((a) == 0 ? (b) : \ - ((b) == 0 ? (a) : (MIN(a, b)))) +#define MIN_NON_ZERO(a, b) \ + ({ \ + typeof(1 ? (a) : (b)) _a = (a), _b = (b); \ + _a == 0 ? _b : (_b == 0 || _b > _a) ? _a : _b; \ + }) #endif /* Round number down to multiple */ diff --git a/migration/qemu-file.c b/migration/qemu-file.c index 1c3a358a14..be21518c57 100644 --- a/migration/qemu-file.c +++ b/migration/qemu-file.c @@ -31,7 +31,7 @@ #include "qapi/error.h" #define IO_BUF_SIZE 32768 -#define MAX_IOV_SIZE MIN(IOV_MAX, 64) +#define MAX_IOV_SIZE MIN_CONST(IOV_MAX, 64) struct QEMUFile { const QEMUFileOps *ops; -- 2.26.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PULL v2 25/31] osdep: Make MIN/MAX evaluate arguments only once 2020-06-26 13:56 ` [PULL v2 25/31] osdep: Make MIN/MAX evaluate arguments only once Paolo Bonzini @ 2020-06-27 21:35 ` Peter Maydell 2020-06-29 15:23 ` Eric Blake 2020-06-29 16:01 ` Markus Armbruster 0 siblings, 2 replies; 8+ messages in thread From: Peter Maydell @ 2020-06-27 21:35 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Philippe Mathieu-Daudé, QEMU Developers On Fri, 26 Jun 2020 at 14:57, Paolo Bonzini <pbonzini@redhat.com> wrote: > > From: Eric Blake <eblake@redhat.com> > > I'm not aware of any immediate bugs in qemu where a second runtime > evaluation of the arguments to MIN() or MAX() causes a problem, but > proactively preventing such abuse is easier than falling prey to an > unintended case down the road. At any rate, here's the conversation > that sparked the current patch: > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05718.html Hi; the changes in this patch seem to confuse Coverity. > +#undef MIN > +#define MIN(a, b) \ > + ({ \ > + typeof(1 ? (a) : (b)) _a = (a), _b = (b); \ > + _a < _b ? _a : _b; \ > + }) > +#define MIN_CONST(a, b) \ > + __builtin_choose_expr( \ > + __builtin_constant_p(a) && __builtin_constant_p(b), \ > + (a) < (b) ? (a) : (b), \ > + ((void)0)) > +#undef MAX > +#define MAX(a, b) \ > + ({ \ > + typeof(1 ? (a) : (b)) _a = (a), _b = (b); \ > + _a > _b ? _a : _b; \ > + }) > +#define MAX_CONST(a, b) \ > + __builtin_choose_expr( \ > + __builtin_constant_p(a) && __builtin_constant_p(b), \ > + (a) > (b) ? (a) : (b), \ > + ((void)0)) In particular, where MIN_CONST or MAX_CONST are used to define values that must be const, eg in qemu-file.c: 50 DECLARE_BITMAP(may_free, MAX_IOV_SIZE); or in hcd-xhci.h: 217 USBPort uports[MAX_CONST(MAXPORTS_2, MAXPORTS_3)]; Coverity reports: CID 1429992 (#1 of 1): Unrecoverable parse warning (PARSE_ERROR)1. expr_not_constant: expression must have a constant value Can we do something (eg providing fallback less-intelligent versions of the macro ifdef __COVERITY__) to help it? (This is the cause of CID 1429992, 1429995, 1429997, 1429999. Parse errors are unfortunate because Coverity abandons analysis of the affected function entirely, and analysis of its callers is also limited.) thanks -- PMM ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PULL v2 25/31] osdep: Make MIN/MAX evaluate arguments only once 2020-06-27 21:35 ` Peter Maydell @ 2020-06-29 15:23 ` Eric Blake 2020-06-29 15:28 ` Paolo Bonzini 2020-06-29 16:01 ` Markus Armbruster 1 sibling, 1 reply; 8+ messages in thread From: Eric Blake @ 2020-06-29 15:23 UTC (permalink / raw) To: Peter Maydell, Paolo Bonzini; +Cc: Philippe Mathieu-Daudé, QEMU Developers On 6/27/20 4:35 PM, Peter Maydell wrote: > On Fri, 26 Jun 2020 at 14:57, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> From: Eric Blake <eblake@redhat.com> >> >> I'm not aware of any immediate bugs in qemu where a second runtime >> evaluation of the arguments to MIN() or MAX() causes a problem, but >> proactively preventing such abuse is easier than falling prey to an >> unintended case down the road. At any rate, here's the conversation >> that sparked the current patch: >> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05718.html > > Hi; the changes in this patch seem to confuse Coverity. Oh dear. > In particular, where MIN_CONST or MAX_CONST are used to > define values that must be const, eg in qemu-file.c: > 50 DECLARE_BITMAP(may_free, MAX_IOV_SIZE); > or in hcd-xhci.h: > 217 USBPort uports[MAX_CONST(MAXPORTS_2, MAXPORTS_3)]; > > Coverity reports: > > CID 1429992 (#1 of 1): Unrecoverable parse warning (PARSE_ERROR)1. > expr_not_constant: expression must have a constant value > > Can we do something (eg providing fallback less-intelligent > versions of the macro ifdef __COVERITY__) to help it? Absolutely; I see we've done similar in include/qemu/thread.h. I'll post something later today. > > (This is the cause of CID 1429992, 1429995, 1429997, > 1429999. Parse errors are unfortunate because Coverity > abandons analysis of the affected function entirely, > and analysis of its callers is also limited.) > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PULL v2 25/31] osdep: Make MIN/MAX evaluate arguments only once 2020-06-29 15:23 ` Eric Blake @ 2020-06-29 15:28 ` Paolo Bonzini 0 siblings, 0 replies; 8+ messages in thread From: Paolo Bonzini @ 2020-06-29 15:28 UTC (permalink / raw) To: Eric Blake, Peter Maydell; +Cc: Philippe Mathieu-Daudé, QEMU Developers On 29/06/20 17:23, Eric Blake wrote: >> >> >> Can we do something (eg providing fallback less-intelligent >> versions of the macro ifdef __COVERITY__) to help it? > > Absolutely; I see we've done similar in include/qemu/thread.h. I'll > post something later today. Done already, you're in Cc. :) Paolo ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PULL v2 25/31] osdep: Make MIN/MAX evaluate arguments only once 2020-06-27 21:35 ` Peter Maydell 2020-06-29 15:23 ` Eric Blake @ 2020-06-29 16:01 ` Markus Armbruster 1 sibling, 0 replies; 8+ messages in thread From: Markus Armbruster @ 2020-06-29 16:01 UTC (permalink / raw) To: Peter Maydell; +Cc: Paolo Bonzini, Philippe Mathieu-Daudé, QEMU Developers Peter Maydell <peter.maydell@linaro.org> writes: > On Fri, 26 Jun 2020 at 14:57, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> From: Eric Blake <eblake@redhat.com> >> >> I'm not aware of any immediate bugs in qemu where a second runtime >> evaluation of the arguments to MIN() or MAX() causes a problem, but >> proactively preventing such abuse is easier than falling prey to an >> unintended case down the road. At any rate, here's the conversation >> that sparked the current patch: >> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05718.html > > Hi; the changes in this patch seem to confuse Coverity. > >> +#undef MIN >> +#define MIN(a, b) \ >> + ({ \ >> + typeof(1 ? (a) : (b)) _a = (a), _b = (b); \ >> + _a < _b ? _a : _b; \ >> + }) >> +#define MIN_CONST(a, b) \ >> + __builtin_choose_expr( \ >> + __builtin_constant_p(a) && __builtin_constant_p(b), \ >> + (a) < (b) ? (a) : (b), \ >> + ((void)0)) >> +#undef MAX >> +#define MAX(a, b) \ >> + ({ \ >> + typeof(1 ? (a) : (b)) _a = (a), _b = (b); \ >> + _a > _b ? _a : _b; \ >> + }) >> +#define MAX_CONST(a, b) \ >> + __builtin_choose_expr( \ >> + __builtin_constant_p(a) && __builtin_constant_p(b), \ >> + (a) > (b) ? (a) : (b), \ >> + ((void)0)) > > In particular, where MIN_CONST or MAX_CONST are used to > define values that must be const, eg in qemu-file.c: > 50 DECLARE_BITMAP(may_free, MAX_IOV_SIZE); > or in hcd-xhci.h: > 217 USBPort uports[MAX_CONST(MAXPORTS_2, MAXPORTS_3)]; > > Coverity reports: > > CID 1429992 (#1 of 1): Unrecoverable parse warning (PARSE_ERROR)1. > expr_not_constant: expression must have a constant value > > Can we do something (eg providing fallback less-intelligent > versions of the macro ifdef __COVERITY__) to help it? Perhaps we can solve the issue with scripts/coverity-model.c. Unfortunately, I can't spare the time to try right now. > (This is the cause of CID 1429992, 1429995, 1429997, > 1429999. Parse errors are unfortunate because Coverity > abandons analysis of the affected function entirely, > and analysis of its callers is also limited.) Bummer. I recommend to revert until we figure out how not to break Coverity. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PULL v2 00/31] Misc patches for 2020-06-24 2020-06-26 13:56 [PULL v2 00/31] Misc patches for 2020-06-24 Paolo Bonzini 2020-06-26 13:56 ` [PULL v2 09/31] exec: fetch the alignment of Linux devdax pmem character device nodes Paolo Bonzini 2020-06-26 13:56 ` [PULL v2 25/31] osdep: Make MIN/MAX evaluate arguments only once Paolo Bonzini @ 2020-06-26 17:22 ` Peter Maydell 2 siblings, 0 replies; 8+ messages in thread From: Peter Maydell @ 2020-06-26 17:22 UTC (permalink / raw) To: Paolo Bonzini; +Cc: QEMU Developers On Fri, 26 Jun 2020 at 14:57, Paolo Bonzini <pbonzini@redhat.com> wrote: > > The following changes since commit 5acc270a355120ce967ca1f1eeca0abbdb9303c8: > > Merge remote-tracking branch 'remotes/xtensa/tags/20200625-xtensa' into staging (2020-06-25 21:20:45 +0100) > > are available in the Git repository at: > > git://github.com/bonzini/qemu.git tags/for-upstream > > for you to fetch changes up to 730319aef0fcb94f11a4a2d32656437fdde7efdd: > > i386: Mask SVM features if nested SVM is disabled (2020-06-26 09:39:40 -0400) > > ---------------------------------------------------------------- > * Various fixes > * libdaxctl support to correctly align devdax character devices (Jingqi) > * initial-all-set support for live migration (Jay) > * forbid '-numa node, mem' for 5.1 and newer machine types (Igor) > * x87 fixes (Joseph) > * Tighten memory_region_access_valid (Michael) and fix fallout (myself) > * Replay fixes (Pavel) > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-06-29 16:05 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-06-26 13:56 [PULL v2 00/31] Misc patches for 2020-06-24 Paolo Bonzini 2020-06-26 13:56 ` [PULL v2 09/31] exec: fetch the alignment of Linux devdax pmem character device nodes Paolo Bonzini 2020-06-26 13:56 ` [PULL v2 25/31] osdep: Make MIN/MAX evaluate arguments only once Paolo Bonzini 2020-06-27 21:35 ` Peter Maydell 2020-06-29 15:23 ` Eric Blake 2020-06-29 15:28 ` Paolo Bonzini 2020-06-29 16:01 ` Markus Armbruster 2020-06-26 17:22 ` [PULL v2 00/31] Misc patches for 2020-06-24 Peter Maydell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).