[PULL 00/10] testing and other mix fixes

Alex Bennée posted 10 patches 3 years, 7 months ago
Only 3 patches received!
docs/devel/tcg-plugins.rst                    | 146 ++++++++++++++++++++++++++
docs/system/deprecated.rst                    |   7 ++
CODING_STYLE.rst                              |  37 ++++++-
configure                                     |  48 +++++----
Makefile                                      |  11 ++
{tests/plugin => contrib/plugins}/hotblocks.c |   0
{tests/plugin => contrib/plugins}/hotpages.c  |   0
{tests/plugin => contrib/plugins}/howvec.c    |   0
{tests/plugin => contrib/plugins}/lockstep.c  |   0
hw/i386/pc_piix.c                             |   2 +-
hw/usb/host-libusb.c                          |   2 +-
target/mips/translate.c                       |  12 +--
.travis.yml                                   |   2 +-
MAINTAINERS                                   |   3 +-
contrib/plugins/Makefile                      |  42 ++++++++
tests/Makefile.include                        |   4 +-
tests/acceptance/avocado_qemu/__init__.py     |  12 +++
tests/meson.build                             |   3 +-
tests/plugin/meson.build                      |   4 +-
tests/requirements.txt                        |   2 +-
tests/tcg/Makefile.target                     |   3 +-
21 files changed, 297 insertions(+), 43 deletions(-)
rename {tests/plugin => contrib/plugins}/hotblocks.c (100%)
rename {tests/plugin => contrib/plugins}/hotpages.c (100%)
rename {tests/plugin => contrib/plugins}/howvec.c (100%)
rename {tests/plugin => contrib/plugins}/lockstep.c (100%)
create mode 100644 contrib/plugins/Makefile
[PULL 00/10] testing and other mix fixes
Posted by Alex Bennée 3 years, 7 months ago
The following changes since commit 9435a8b3dd35f1f926f1b9127e8a906217a5518a:

  Merge remote-tracking branch 'remotes/kraxel/tags/sirius/ipxe-20200908-pull-request' into staging (2020-09-08 21:21:13 +0100)

are available in the Git repository at:

  https://github.com/stsquad/qemu.git tags/pull-testing-fixes-100920-1

for you to fetch changes up to c17a386b6afe608086aa4d260e29662865680b7f:

  plugins: move the more involved plugins to contrib (2020-09-10 10:47:03 +0100)

----------------------------------------------------------------
Various misc and testing fixes:

  - Expand CODING_STYLE.rst a little more
  - usb-host build fix
  - allow check-softfloat unit tests without TCG
  - simplify mips imm_branch so compiler isn't confused
  - mark ppc64abi32 for deprecation
  - more compiler soothing in pch_rev_id
  - allow acceptance to skip missing binaries
  - more a bunch of plugins to contrib

----------------------------------------------------------------
Alex Bennée (7):
      CODING_STYLE.rst: flesh out our naming conventions.
      tests/meson.build: fp tests don't need CONFIG_TCG
      target/mips: simplify gen_compute_imm_branch logic
      docs/system/deprecated: mark ppc64abi32-linux-user for deprecation
      configure: don't enable ppc64abi32-linux-user by default
      hw/i386: make explicit clearing of pch_rev_id
      plugins: move the more involved plugins to contrib

Gerd Hoffmann (1):
      usb-host: restrict workaround to new libusb versions

Pavel Dovgaluk (1):
      tests: bump avocado version

Philippe Mathieu-Daudé (1):
      tests/acceptance: Add Test.fetch_asset(cancel_on_missing=True)

 docs/devel/tcg-plugins.rst                    | 146 ++++++++++++++++++++++++++
 docs/system/deprecated.rst                    |   7 ++
 CODING_STYLE.rst                              |  37 ++++++-
 configure                                     |  48 +++++----
 Makefile                                      |  11 ++
 {tests/plugin => contrib/plugins}/hotblocks.c |   0
 {tests/plugin => contrib/plugins}/hotpages.c  |   0
 {tests/plugin => contrib/plugins}/howvec.c    |   0
 {tests/plugin => contrib/plugins}/lockstep.c  |   0
 hw/i386/pc_piix.c                             |   2 +-
 hw/usb/host-libusb.c                          |   2 +-
 target/mips/translate.c                       |  12 +--
 .travis.yml                                   |   2 +-
 MAINTAINERS                                   |   3 +-
 contrib/plugins/Makefile                      |  42 ++++++++
 tests/Makefile.include                        |   4 +-
 tests/acceptance/avocado_qemu/__init__.py     |  12 +++
 tests/meson.build                             |   3 +-
 tests/plugin/meson.build                      |   4 +-
 tests/requirements.txt                        |   2 +-
 tests/tcg/Makefile.target                     |   3 +-
 21 files changed, 297 insertions(+), 43 deletions(-)
 rename {tests/plugin => contrib/plugins}/hotblocks.c (100%)
 rename {tests/plugin => contrib/plugins}/hotpages.c (100%)
 rename {tests/plugin => contrib/plugins}/howvec.c (100%)
 rename {tests/plugin => contrib/plugins}/lockstep.c (100%)
 create mode 100644 contrib/plugins/Makefile

-- 
2.20.1


Re: [PULL 00/10] testing and other mix fixes
Posted by Peter Maydell 3 years, 7 months ago
On Thu, 10 Sep 2020 at 14:15, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> The following changes since commit 9435a8b3dd35f1f926f1b9127e8a906217a5518a:
>
>   Merge remote-tracking branch 'remotes/kraxel/tags/sirius/ipxe-20200908-pull-request' into staging (2020-09-08 21:21:13 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/stsquad/qemu.git tags/pull-testing-fixes-100920-1
>
> for you to fetch changes up to c17a386b6afe608086aa4d260e29662865680b7f:
>
>   plugins: move the more involved plugins to contrib (2020-09-10 10:47:03 +0100)
>
> ----------------------------------------------------------------
> Various misc and testing fixes:
>
>   - Expand CODING_STYLE.rst a little more
>   - usb-host build fix
>   - allow check-softfloat unit tests without TCG
>   - simplify mips imm_branch so compiler isn't confused
>   - mark ppc64abi32 for deprecation
>   - more compiler soothing in pch_rev_id
>   - allow acceptance to skip missing binaries
>   - more a bunch of plugins to contrib

Applied, thanks (we can tweak the config-deprecation code as
a followon patch).

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2
for any user-visible changes.

-- PMM

[PULL 01/10] CODING_STYLE.rst: flesh out our naming conventions.
Posted by Alex Bennée 3 years, 7 months ago
Mention a few of the more common naming conventions we follow in the
code base including common variable names and function prefix and
suffix examples.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20200909112742.25730-2-alex.bennee@linaro.org>

diff --git a/CODING_STYLE.rst b/CODING_STYLE.rst
index 427699e0e42..8b13ef0669e 100644
--- a/CODING_STYLE.rst
+++ b/CODING_STYLE.rst
@@ -109,8 +109,41 @@ names are lower_case_with_underscores_ending_with_a_t, like the POSIX
 uint64_t and family.  Note that this last convention contradicts POSIX
 and is therefore likely to be changed.
 
-When wrapping standard library functions, use the prefix ``qemu_`` to alert
-readers that they are seeing a wrapped version; otherwise avoid this prefix.
+Variable Naming Conventions
+---------------------------
+
+A number of short naming conventions exist for variables that use
+common QEMU types. For example, the architecture independent CPUState
+is often held as a ``cs`` pointer variable, whereas the concrete
+CPUArchState is usually held in a pointer called ``env``.
+
+Likewise, in device emulation code the common DeviceState is usually
+called ``dev``.
+
+Function Naming Conventions
+---------------------------
+
+Wrapped version of standard library or GLib functions use a ``qemu_``
+prefix to alert readers that they are seeing a wrapped version, for
+example ``qemu_strtol`` or ``qemu_mutex_lock``.  Other utility functions
+that are widely called from across the codebase should not have any
+prefix, for example ``pstrcpy`` or bit manipulation functions such as
+``find_first_bit``.
+
+The ``qemu_`` prefix is also used for functions that modify global
+emulator state, for example ``qemu_add_vm_change_state_handler``.
+However, if there is an obvious subsystem-specific prefix it should be
+used instead.
+
+Public functions from a file or subsystem (declared in headers) tend
+to have a consistent prefix to show where they came from. For example,
+``tlb_`` for functions from ``cputlb.c`` or ``cpu_`` for functions
+from cpus.c.
+
+If there are two versions of a function to be called with or without a
+lock held, the function that expects the lock to be already held
+usually uses the suffix ``_locked``.
+
 
 Block structure
 ===============
-- 
2.20.1


[PULL 02/10] usb-host: restrict workaround to new libusb versions
Posted by Alex Bennée 3 years, 7 months ago
From: Gerd Hoffmann <kraxel@redhat.com>

Fixes build failures with old kernels (USBDEVFS_GET_SPEED missing),
on the assumtion that distros with old kernels also have old libusb.

Reported-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200902081445.3291-1-kraxel@redhat.com>
Message-Id: <20200909112742.25730-3-alex.bennee@linaro.org>

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 08604f787fd..c5d38cb09c0 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -942,7 +942,7 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
     usb_host_ep_update(s);
 
     libusb_speed = libusb_get_device_speed(dev);
-#ifdef CONFIG_LINUX
+#if LIBUSB_API_VERSION >= 0x01000107 && defined(CONFIG_LINUX)
     if (hostfd && libusb_speed == 0) {
         /*
          * Workaround libusb bug: libusb_get_device_speed() does not
-- 
2.20.1


Re: [PULL 02/10] usb-host: restrict workaround to new libusb versions
Posted by Igor Mammedov 3 years, 7 months ago
On Thu, 10 Sep 2020 14:14:56 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> From: Gerd Hoffmann <kraxel@redhat.com>
> 
> Fixes build failures with old kernels (USBDEVFS_GET_SPEED missing),
> on the assumtion that distros with old kernels also have old libusb.
> 
> Reported-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Message-Id: <20200902081445.3291-1-kraxel@redhat.com>
> Message-Id: <20200909112742.25730-3-alex.bennee@linaro.org>

Can we get this merged to fix travis-ci builds, please.

> 
> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> index 08604f787fd..c5d38cb09c0 100644
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c
> @@ -942,7 +942,7 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev, int hostfd)
>      usb_host_ep_update(s);
>  
>      libusb_speed = libusb_get_device_speed(dev);
> -#ifdef CONFIG_LINUX
> +#if LIBUSB_API_VERSION >= 0x01000107 && defined(CONFIG_LINUX)
>      if (hostfd && libusb_speed == 0) {
>          /*
>           * Workaround libusb bug: libusb_get_device_speed() does not


[PULL 04/10] target/mips: simplify gen_compute_imm_branch logic
Posted by Alex Bennée 3 years, 7 months ago
One of the Travis builds was complaining about:

  qemu/include/tcg/tcg.h:437:12: error: ‘cond’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
       return (TCGCond)(c ^ 1);
  ../target/mips/translate.c:20031:13: note: ‘cond’ was declared here
       TCGCond cond;

Rather than figure out exactly which one was causing the complaint I
just defaulted to TCG_COND_ALWAYS and allowed that state to double up
for the now defunct bcond_compute variable.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200909112742.25730-5-alex.bennee@linaro.org>

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 899b90ae0ff..398edf72898 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -20028,8 +20028,7 @@ static void gen_pool32axf_nanomips_insn(CPUMIPSState *env, DisasContext *ctx)
 static void gen_compute_imm_branch(DisasContext *ctx, uint32_t opc,
                                    int rt, int32_t imm, int32_t offset)
 {
-    TCGCond cond;
-    int bcond_compute = 0;
+    TCGCond cond = TCG_COND_ALWAYS;
     TCGv t0 = tcg_temp_new();
     TCGv t1 = tcg_temp_new();
 
@@ -20046,7 +20045,6 @@ static void gen_compute_imm_branch(DisasContext *ctx, uint32_t opc,
             /* Treat as NOP */
             goto out;
         } else {
-            bcond_compute = 1;
             cond = TCG_COND_EQ;
         }
         break;
@@ -20065,7 +20063,6 @@ static void gen_compute_imm_branch(DisasContext *ctx, uint32_t opc,
             tcg_gen_shri_tl(t0, t0, imm);
             tcg_gen_andi_tl(t0, t0, 1);
             tcg_gen_movi_tl(t1, 0);
-            bcond_compute = 1;
             if (opc == NM_BBEQZC) {
                 cond = TCG_COND_EQ;
             } else {
@@ -20080,7 +20077,6 @@ static void gen_compute_imm_branch(DisasContext *ctx, uint32_t opc,
         } else if (rt == 0 && imm != 0) {
             /* Unconditional branch */
         } else {
-            bcond_compute = 1;
             cond = TCG_COND_NE;
         }
         break;
@@ -20088,24 +20084,20 @@ static void gen_compute_imm_branch(DisasContext *ctx, uint32_t opc,
         if (rt == 0 && imm == 0) {
             /* Unconditional branch */
         } else  {
-            bcond_compute = 1;
             cond = TCG_COND_GE;
         }
         break;
     case NM_BLTIC:
-        bcond_compute = 1;
         cond = TCG_COND_LT;
         break;
     case NM_BGEIUC:
         if (rt == 0 && imm == 0) {
             /* Unconditional branch */
         } else  {
-            bcond_compute = 1;
             cond = TCG_COND_GEU;
         }
         break;
     case NM_BLTIUC:
-        bcond_compute = 1;
         cond = TCG_COND_LTU;
         break;
     default:
@@ -20118,7 +20110,7 @@ static void gen_compute_imm_branch(DisasContext *ctx, uint32_t opc,
     clear_branch_hflags(ctx);
     ctx->base.is_jmp = DISAS_NORETURN;
 
-    if (bcond_compute == 0) {
+    if (cond == TCG_COND_ALWAYS) {
         /* Uncoditional compact branch */
         gen_goto_tb(ctx, 0, ctx->btarget);
     } else {
-- 
2.20.1


[PULL 05/10] docs/system/deprecated: mark ppc64abi32-linux-user for deprecation
Posted by Alex Bennée 3 years, 7 months ago
It's buggy and we are not sure anyone uses it.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
Message-Id: <20200909112742.25730-6-alex.bennee@linaro.org>

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 851dbdeb8ab..a158e765c33 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -424,6 +424,13 @@ linux-user mode) is deprecated and will be removed in a future version
 of QEMU. Support for this CPU was removed from the upstream Linux
 kernel in 2018, and has also been dropped from glibc.
 
+``ppc64abi32`` CPUs (since 5.2.0)
+'''''''''''''''''''''''''''''''''
+
+The ``ppc64abi32`` architecture has a number of issues which regularly
+trip up our CI testing and is suspected to be quite broken. For that
+reason the maintainers strongly suspect no one actually uses it.
+
 Related binaries
 ----------------
 
-- 
2.20.1

[PULL 06/10] configure: don't enable ppc64abi32-linux-user by default
Posted by Alex Bennée 3 years, 7 months ago
The user can still enable this explicitly but they will get a warning
at the end of configure for their troubles. This also drops any builds
of ppc64abi32 from our CI tests.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20200909112742.25730-7-alex.bennee@linaro.org>

diff --git a/configure b/configure
index 4231d56bcc0..2b5492a0d63 100755
--- a/configure
+++ b/configure
@@ -542,6 +542,8 @@ gettext=""
 bogus_os="no"
 malloc_trim=""
 
+deprecated_features=""
+
 # parse CC options first
 for opt do
   optarg=$(expr "x$opt" : 'x[^=]*=\(.*\)')
@@ -1720,26 +1722,25 @@ if [ "$bsd_user" = "yes" ]; then
     mak_wilds="${mak_wilds} $source_path/default-configs/*-bsd-user.mak"
 fi
 
-if test -z "$target_list_exclude"; then
-    for config in $mak_wilds; do
-        default_target_list="${default_target_list} $(basename "$config" .mak)"
-    done
-else
-    exclude_list=$(echo "$target_list_exclude" | sed -e 's/,/ /g')
-    for config in $mak_wilds; do
-        target="$(basename "$config" .mak)"
-        exclude="no"
-        for excl in $exclude_list; do
-            if test "$excl" = "$target"; then
-                exclude="yes"
-                break;
-            fi
-        done
-        if test "$exclude" = "no"; then
-            default_target_list="${default_target_list} $target"
+if test -z "$target_list_exclude" -a -z "$target_list"; then
+    # if the user doesn't specify anything lets skip deprecating stuff
+    target_list_exclude=ppc64abi32-linux-user
+fi
+
+exclude_list=$(echo "$target_list_exclude" | sed -e 's/,/ /g')
+for config in $mak_wilds; do
+    target="$(basename "$config" .mak)"
+    exclude="no"
+    for excl in $exclude_list; do
+        if test "$excl" = "$target"; then
+            exclude="yes"
+            break;
         fi
     done
-fi
+    if test "$exclude" = "no"; then
+        default_target_list="${default_target_list} $target"
+    fi
+done
 
 # Enumerate public trace backends for --help output
 trace_backend_list=$(echo $(grep -le '^PUBLIC = True$' "$source_path"/scripts/tracetool/backend/*.py | sed -e 's/^.*\/\(.*\)\.py$/\1/'))
@@ -7557,7 +7558,7 @@ TARGET_SYSTBL=""
 case "$target_name" in
   i386)
     mttcg="yes"
-	gdb_xml_files="i386-32bit.xml"
+    gdb_xml_files="i386-32bit.xml"
     TARGET_SYSTBL_ABI=i386
     TARGET_SYSTBL=syscall_32.tbl
   ;;
@@ -7667,6 +7668,7 @@ case "$target_name" in
     TARGET_SYSTBL_ABI=common,nospu,32
     echo "TARGET_ABI32=y" >> $config_target_mak
     gdb_xml_files="power64-core.xml power-fpu.xml power-altivec.xml power-spe.xml power-vsx.xml"
+    deprecated_features="ppc64abi32 ${deprecated_features}"
   ;;
   riscv32)
     TARGET_BASE_ARCH=riscv
@@ -8011,6 +8013,12 @@ fi
 touch ninjatool.stamp
 fi
 
+if test -n "${deprecated_features}"; then
+    echo "Warning, deprecated features enabled."
+    echo "Please see docs/system/deprecated.rst"
+    echo "  features: ${deprecated_features}"
+fi
+
 # Save the configure command line for later reuse.
 cat <<EOD >config.status
 #!/bin/sh
-- 
2.20.1


Re: [PULL 06/10] configure: don't enable ppc64abi32-linux-user by default
Posted by Peter Maydell 3 years, 7 months ago
On Thu, 10 Sep 2020 at 14:15, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> The user can still enable this explicitly but they will get a warning
> at the end of configure for their troubles. This also drops any builds
> of ppc64abi32 from our CI tests.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Message-Id: <20200909112742.25730-7-alex.bennee@linaro.org>

I know this is in a pullreq at this point, but I just got round
to looking at it, and it has some odd logic in it so I figured
I'd give my review comments anyway.

> +if test -z "$target_list_exclude" -a -z "$target_list"; then
> +    # if the user doesn't specify anything lets skip deprecating stuff
> +    target_list_exclude=ppc64abi32-linux-user

Doesn't this have the slightly curious logic
that if we have more than one deprecated target then
configure with --target-list-exclude=something-else
will actually build more targets than configure without that
exclude option (because it will exclude the something-else
but stop excluding the deprecated targets)? I would have
expected the deprecated targets to be not compiled unless
the user explicitly enabled them. I think that would be
something more like

   deprecated_targets_list=ppc64abi32-linux-user
   if test -z "$target_list"; then
       target_list_exclude="$target_list_exclude,$deprecated_targets_list"
   fi

I suppose we would ideally like an "enable all including
the deprecated stuff", and that gets messy because there's
no way to do it except listing everything explicitly I think...

> +fi
> +
> +exclude_list=$(echo "$target_list_exclude" | sed -e 's/,/ /g')
> +for config in $mak_wilds; do
> +    target="$(basename "$config" .mak)"
> +    exclude="no"
> +    for excl in $exclude_list; do
> +        if test "$excl" = "$target"; then
> +            exclude="yes"
> +            break;
>          fi
>      done
> -fi
> +    if test "$exclude" = "no"; then
> +        default_target_list="${default_target_list} $target"
> +    fi
> +done
>
>  # Enumerate public trace backends for --help output
>  trace_backend_list=$(echo $(grep -le '^PUBLIC = True$' "$source_path"/scripts/tracetool/backend/*.py | sed -e 's/^.*\/\(.*\)\.py$/\1/'))
> @@ -7557,7 +7558,7 @@ TARGET_SYSTBL=""
>  case "$target_name" in
>    i386)
>      mttcg="yes"
> -       gdb_xml_files="i386-32bit.xml"
> +    gdb_xml_files="i386-32bit.xml"
>      TARGET_SYSTBL_ABI=i386
>      TARGET_SYSTBL=syscall_32.tbl
>    ;;

(unrelated change ;-))

> @@ -7667,6 +7668,7 @@ case "$target_name" in
>      TARGET_SYSTBL_ABI=common,nospu,32
>      echo "TARGET_ABI32=y" >> $config_target_mak
>      gdb_xml_files="power64-core.xml power-fpu.xml power-altivec.xml power-spe.xml power-vsx.xml"
> +    deprecated_features="ppc64abi32 ${deprecated_features}"

Maybe prefer
    add_to deprecated_features ppc64abi32

?

>    ;;
>    riscv32)
>      TARGET_BASE_ARCH=riscv

If we just made the deprecation warning be printed by
generic logic whenever a deprecated target ended up in
the enabled list then it would be easier to add other deprecated
targets to the list, as you wouldn't thae also have to find some
other part of configure like this to set a deprecated_features variable.

(I'll send a patch to add lm32-softmmu,tilegx-linux-user,unicore32-softmmu
to the deprecated-target list later once we have the mechanism in place.)

> @@ -8011,6 +8013,12 @@ fi
>  touch ninjatool.stamp
>  fi
>
> +if test -n "${deprecated_features}"; then
> +    echo "Warning, deprecated features enabled."
> +    echo "Please see docs/system/deprecated.rst"
> +    echo "  features: ${deprecated_features}"
> +fi
> +
>  # Save the configure command line for later reuse.
>  cat <<EOD >config.status
>  #!/bin/sh
> --

thanks
-- PMM

Re: [PULL 06/10] configure: don't enable ppc64abi32-linux-user by default
Posted by Alex Bennée 3 years, 7 months ago
Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 10 Sep 2020 at 14:15, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> The user can still enable this explicitly but they will get a warning
>> at the end of configure for their troubles. This also drops any builds
>> of ppc64abi32 from our CI tests.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Message-Id: <20200909112742.25730-7-alex.bennee@linaro.org>
>
> I know this is in a pullreq at this point, but I just got round
> to looking at it, and it has some odd logic in it so I figured
> I'd give my review comments anyway.
>
>> +if test -z "$target_list_exclude" -a -z "$target_list"; then
>> +    # if the user doesn't specify anything lets skip deprecating stuff
>> +    target_list_exclude=ppc64abi32-linux-user
>
> Doesn't this have the slightly curious logic
> that if we have more than one deprecated target then
> configure with --target-list-exclude=something-else
> will actually build more targets than configure without that
> exclude option (because it will exclude the something-else
> but stop excluding the deprecated targets)? I would have
> expected the deprecated targets to be not compiled unless
> the user explicitly enabled them. I think that would be
> something more like
>
>    deprecated_targets_list=ppc64abi32-linux-user
>    if test -z "$target_list"; then
>        target_list_exclude="$target_list_exclude,$deprecated_targets_list"
>    fi
>
> I suppose we would ideally like an "enable all including
> the deprecated stuff", and that gets messy because there's
> no way to do it except listing everything explicitly I think...

Yeah - we could make it smoother although I think the only real users of
--target-list-exclude are the CI systems and they all explicitly exclude
ppc64abi32 when they do it.

Do you want me to re-spin with those changes?

>
>> +fi
>> +
>> +exclude_list=$(echo "$target_list_exclude" | sed -e 's/,/ /g')
>> +for config in $mak_wilds; do
>> +    target="$(basename "$config" .mak)"
>> +    exclude="no"
>> +    for excl in $exclude_list; do
>> +        if test "$excl" = "$target"; then
>> +            exclude="yes"
>> +            break;
>>          fi
>>      done
>> -fi
>> +    if test "$exclude" = "no"; then
>> +        default_target_list="${default_target_list} $target"
>> +    fi
>> +done
>>
>>  # Enumerate public trace backends for --help output
>>  trace_backend_list=$(echo $(grep -le '^PUBLIC = True$' "$source_path"/scripts/tracetool/backend/*.py | sed -e 's/^.*\/\(.*\)\.py$/\1/'))
>> @@ -7557,7 +7558,7 @@ TARGET_SYSTBL=""
>>  case "$target_name" in
>>    i386)
>>      mttcg="yes"
>> -       gdb_xml_files="i386-32bit.xml"
>> +    gdb_xml_files="i386-32bit.xml"
>>      TARGET_SYSTBL_ABI=i386
>>      TARGET_SYSTBL=syscall_32.tbl
>>    ;;
>
> (unrelated change ;-))
>
>> @@ -7667,6 +7668,7 @@ case "$target_name" in
>>      TARGET_SYSTBL_ABI=common,nospu,32
>>      echo "TARGET_ABI32=y" >> $config_target_mak
>>      gdb_xml_files="power64-core.xml power-fpu.xml power-altivec.xml power-spe.xml power-vsx.xml"
>> +    deprecated_features="ppc64abi32 ${deprecated_features}"
>
> Maybe prefer
>     add_to deprecated_features ppc64abi32
>
> ?
>
>>    ;;
>>    riscv32)
>>      TARGET_BASE_ARCH=riscv
>
> If we just made the deprecation warning be printed by
> generic logic whenever a deprecated target ended up in
> the enabled list then it would be easier to add other deprecated
> targets to the list, as you wouldn't thae also have to find some
> other part of configure like this to set a deprecated_features variable.
>
> (I'll send a patch to add lm32-softmmu,tilegx-linux-user,unicore32-softmmu
> to the deprecated-target list later once we have the mechanism in place.)
>
>> @@ -8011,6 +8013,12 @@ fi
>>  touch ninjatool.stamp
>>  fi
>>
>> +if test -n "${deprecated_features}"; then
>> +    echo "Warning, deprecated features enabled."
>> +    echo "Please see docs/system/deprecated.rst"
>> +    echo "  features: ${deprecated_features}"
>> +fi
>> +
>>  # Save the configure command line for later reuse.
>>  cat <<EOD >config.status
>>  #!/bin/sh
>> --
>
> thanks
> -- PMM


-- 
Alex Bennée

[PULL 07/10] hw/i386: make explicit clearing of pch_rev_id
Posted by Alex Bennée 3 years, 7 months ago
Some compilers (notably the Xenial gcc in Travis) fail to spot that
this will always be set if pch_dev_id != 0xffff. Given this is setup
code and using _Pragma to override is equally as ugly lets just remove
the doubt from the compilers mind.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200909112742.25730-8-alex.bennee@linaro.org>

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 32b1453e6a8..33fa035fb7f 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -930,7 +930,7 @@ void igd_passthrough_isa_bridge_create(PCIBus *bus, uint16_t gpu_dev_id)
     struct PCIDevice *bridge_dev;
     int i, num;
     uint16_t pch_dev_id = 0xffff;
-    uint8_t pch_rev_id;
+    uint8_t pch_rev_id = 0;
 
     num = ARRAY_SIZE(igd_combo_id_infos);
     for (i = 0; i < num; i++) {
-- 
2.20.1


[PULL 09/10] tests/acceptance: Add Test.fetch_asset(cancel_on_missing=True)
Posted by Alex Bennée 3 years, 7 months ago
From: Philippe Mathieu-Daudé <philmd@redhat.com>

Invert the default of avocado.Test.fetch_asset 'cancel_on_missing'
keyword: accept missing artefacts by default. If a test is certain
an artifact can't be missing, it will set cancel_on_missing=False.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200908202352.298506-3-philmd@redhat.com>
Message-Id: <20200909112742.25730-10-alex.bennee@linaro.org>

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index db9c0f5d79d..4cda0371879 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -192,3 +192,15 @@ class Test(avocado.Test):
     def tearDown(self):
         for vm in self._vms.values():
             vm.shutdown()
+
+    def fetch_asset(self, name,
+                    asset_hash=None, algorithm=None,
+                    locations=None, expire=None,
+                    find_only=False, cancel_on_missing=True):
+        return super(Test, self).fetch_asset(name,
+                        asset_hash=asset_hash,
+                        algorithm=algorithm,
+                        locations=locations,
+                        expire=expire,
+                        find_only=find_only,
+                        cancel_on_missing=cancel_on_missing)
-- 
2.20.1