[PULL v2 00/31] Misc patches for 2020-06-24

[PULL v2 00/31] Misc patches for 2020-06-24

[Paolo Bonzini]

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

[PULL v2 09/31] exec: fetch the alignment of Linux devdax pmem character device nodes

[Paolo Bonzini]

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

[PULL v2 25/31] osdep: Make MIN/MAX evaluate arguments only once

[Paolo Bonzini]

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

Re: [PULL v2 00/31] Misc patches for 2020-06-24

[Peter Maydell]

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

Re: [PULL v2 25/31] osdep: Make MIN/MAX evaluate arguments only once

[Peter Maydell]

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

Re: [PULL v2 25/31] osdep: Make MIN/MAX evaluate arguments only once

[Eric Blake]

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

Re: [PULL v2 25/31] osdep: Make MIN/MAX evaluate arguments only once

[Paolo Bonzini]

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

Re: [PULL v2 25/31] osdep: Make MIN/MAX evaluate arguments only once

[Markus Armbruster]

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.