Re: [PULL 00/13] Misc bugfix patches for 2021-06-04

[Peter Maydell]

On Fri, 4 Jun 2021 at 16:19, Paolo Bonzini  wrote:
>
> The following changes since commit 5a95f5ce3cd5842cc8f61a91ecd4fb4e8d10104f:
>
>   Merge remote-tracking branch 'remotes/rth-gitlab/tags/pull-fpu-20210603' 
> into staging (2021-06-04 10:04:11 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/bonzini/qemu.git tags/for-upstream
>
> for you to fetch changes up to 49e987695a1873a769a823604f9065aa88e00c55:
>
>   vl: plug -object back into -readconfig (2021-06-04 13:50:04 +0200)
>
> 
> * OpenBSD cleanup (Brad)
> * fixes for the i386 accel/cpu refactoring (Claudio)
> * unmap test for emulated SCSI (Kit)
> * fix for iscsi module (myself)
> * fix for -readconfig of objects (myself)
> * fixes for x86 16-bit task switching (myself)
> * fix for x86 MOV from/to CR8 (Richard)
>


Applied, thanks.

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

-- PMM

Re: [PATCH] vhost-user-gpu: reorder free calls.

[Li Qiang]

Gerd Hoffmann  于2021年6月4日周五 下午6:37写道：
>
> Free in correct order to avoid use-after-free.
>
> Resolves: CID 1453812
> Signed-off-by: Gerd Hoffmann 


Sorry, my fault.

Reviewed-by: Li Qiang 

> ---
>  contrib/vhost-user-gpu/vhost-user-gpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c 
> b/contrib/vhost-user-gpu/vhost-user-gpu.c
> index 6dc6a44f4e26..611360e6b475 100644
> --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
> +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
> @@ -350,8 +350,8 @@ vg_resource_create_2d(VuGpu *g,
>  if (!res->image) {
>  g_critical("%s: resource creation failed %d %d %d",
> __func__, c2d.resource_id, c2d.width, c2d.height);
> -g_free(res);
>  vugbm_buffer_destroy(&res->buffer);
> +g_free(res);
>  cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
>  return;
>  }
> --
> 2.31.1
>
>

Re: [PATCH v4 0/8] IOMMU: Add support for IOMMU Bypass Feature

[Igor Mammedov]

On Tue, 25 May 2021 03:49:57 +
Wang Xingang  wrote:

> From: Xingang Wang 
> 
> These patches add support for configure bypass_iommu on/off for
> pci root bus, including primary bus and pxb root bus. At present,
> all root bus will go through iommu when iommu is configured,
> which is not flexible, because in many situations the need for using
> iommu and bypass iommu aften exists at the same time.

'many situations' doesn't describe why bypass is needed,
can you provide a use-cases here and what are security implications
when bypass is allowed.
(PS: the later probably should be documented somewhere in the docs/option 
description)
 
> So this add option to enable/disable bypass_iommu for primary bus
> and pxb root bus. The bypass_iommu property is set to false default,
> meaning that devcies will go through iommu if no explicit configuration
> is added. When bypass_iommu is enabled for the root bus, devices
> attached to it will bypass iommu, otherwise devices will go through
> iommu.
> 
> This feature can be used in this manner:
> arm: -machine virt,iommu=smmuv3,bypass_iommu=true
> x86: -machine q35,bypass_iommu=true
> pxb: -device pxb-pcie,bus_nr=0x10,id=pci.10,bus=pcie.0,bypass_iommu=true 
> 
> History:
> 
> v3 -> v4:
> - simplify the logic in building the IORT idmap
> 
> v2 -> v3:
> - rebase on top of v6.0.0-rc4
> - Took into account Eric's comments, replace with a bypass_iommu
>   proerty 
> - When building the IORT idmap, cover the whole RID space
> 
> v1 -> v2:
> - rebase on top of v6.0.0-rc0
> - Fix some issues
> - Took into account Eric's comments, and remove the PCI_BUS_IOMMU flag,
>   replace it with a property in PCIHostState.
> - Add support for x86 iommu option
> 
> Xingang Wang (8):
>   hw/pci/pci_host: Allow bypass iommu for pci host
>   hw/pxb: Add a bypass iommu property
>   hw/arm/virt: Add a machine option to bypass iommu for primary bus
>   hw/i386: Add a pc machine option to bypass iommu for primary bus
>   hw/pci: Add pci_bus_range to get bus number range
>   hw/arm/virt-acpi-build: Add explicit IORT idmap for smmuv3 node
>   hw/i386/acpi-build: Add explicit scope in DMAR table
>   hw/i386/acpi-build: Add bypass_iommu check when building IVRS table
> 
>  hw/arm/virt-acpi-build.c| 135 
>  hw/arm/virt.c   |  26 ++
>  hw/i386/acpi-build.c|  70 ++-
>  hw/i386/pc.c|  18 
>  hw/pci-bridge/pci_expander_bridge.c |   3 +
>  hw/pci-host/q35.c   |   1 +
>  hw/pci/pci.c|  33 ++-
>  hw/pci/pci_host.c   |   2 +
>  include/hw/arm/virt.h   |   1 +
>  include/hw/i386/pc.h|   1 +
>  include/hw/pci/pci.h|   2 +
>  include/hw/pci/pci_host.h   |   1 +
>  12 files changed, 270 insertions(+), 23 deletions(-)
> 

Re: [PATCH v5 09/16] docs/devel/testing: add --gdb option to the debugging section of QEMU iotests

[Vladimir Sementsov-Ogievskiy]

04.06.2021 12:17, Emanuele Giuseppe Esposito wrote:

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
  docs/devel/testing.rst | 11 +++
  1 file changed, 11 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 9d6a8f8636..11a0359218 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -229,6 +229,17 @@ Debugging a test case
  The following options to the ``check`` script can be useful when debugging
  a failing test:
  
+* ``--gdb`` wraps every QEMU invocation in a ``gdbserver``, which waits for a

+  connection from a gdb client.  The options given to ``gdbserver`` (e.g. the
+  address on which to listen for connections) are taken from the 
``$GDB_OPTIONS``
+  environment variable.  By default (if ``$GDB_OPTIONS`` is empty), it listens 
on
+  ``localhost:12345``.
+  It is possible to connect to it for example with
+  ``gdb -iex "target remote $addr"``, where ``$addr`` is the address
+  ``gdbserver`` listens on.
+  If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored,


Then, --gdb here too


+  regardless on whether it is set or not.
+
  * ``-d`` (debug) just increases the logging verbosity, showing
for example the QMP commands and answers.
  




--
Best regards,
Vladimir

Re: [PATCH v5 10/16] qemu-iotests: extend the check script to prepare supporting valgrind for python tests

[Vladimir Sementsov-Ogievskiy]

04.06.2021 12:17, Emanuele Giuseppe Esposito wrote:

Currently, the check script only parses the option and sets the
VALGRIND_QEMU environmental variable to "y".
Add another local python variable that prepares the command line,
identical to the one provided in the test scripts.

Because the python script does not know in advance the valgrind
PID to assign to the log file name, use the "%p" flag in valgrind
log file name that automatically puts the process PID at runtime.

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Max Reitz 
---
  tests/qemu-iotests/check  |  7 ---
  tests/qemu-iotests/iotests.py | 11 +++
  tests/qemu-iotests/testenv.py |  1 +
  3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 1dba4218c0..e6aa110715 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -39,6 +39,10 @@ def make_argparser() -> argparse.ArgumentParser:
  p.add_argument('--gdb', action='store_true',
 help="start gdbserver with $GDB_OPTIONS options \
  ('localhost:12345' if $GDB_OPTIONS is empty)")
+p.add_argument('--valgrind', action='store_true',
+help='use valgrind, sets VALGRIND_QEMU environment '
+'variable')
+
  p.add_argument('--misalign', action='store_true',
 help='misalign memory allocations')
  p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -88,9 +92,6 @@ def make_argparser() -> argparse.ArgumentParser:
  g_bash.add_argument('-o', dest='imgopts',
  help='options to pass to qemu-img create/convert, '
  'sets IMGOPTS environment variable')
-g_bash.add_argument('--valgrind', action='store_true',
-help='use valgrind, sets VALGRIND_QEMU environment '
-'variable')
  
  g_sel = p.add_argument_group('test selecting options',

   'The following options specify test set '
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index c547e8c07b..3fa1bd0ab5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -96,6 +96,17 @@
  sys.stderr.write('Please run this test via the "check" script\n')
  sys.exit(os.EX_USAGE)
  
+qemu_valgrind = []

+if os.environ.get('VALGRIND_QEMU') == "y" and \
+os.environ.get('NO_VALGRIND') != "y":


Hmm, interesting, why do you need additional NO_VALGRIND variable


+valgrind_logfile = "--log-file=" + test_dir
+# %p allows to put the valgrind process PID, since
+# we don't know it a priori (subprocess.Popen is
+# not yet invoked)
+valgrind_logfile += "/%p.valgrind"
+
+qemu_valgrind = ['valgrind', valgrind_logfile, '--error-exitcode=99']
+
  socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
  
  luks_default_secret_object = 'secret,id=keysec0,data=' + \

diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 8501c6caf5..8bf154376f 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -298,6 +298,7 @@ def print_env(self) -> None:
  SOCK_DIR  -- {SOCK_DIR}
  SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
  GDB_OPTIONS   -- {GDB_OPTIONS}
+VALGRIND_QEMU -- {VALGRIND_QEMU}
  """
  
  args = collections.defaultdict(str, self.get_env())






Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v5 02/16] python: Reduce strictness of pylint's duplicate-code check

[Vladimir Sementsov-Ogievskiy]

04.06.2021 12:17, Emanuele Giuseppe Esposito wrote:

From: John Snow

Pylint prior to 2.8.3 (We pin at >= 2.8.0) includes function and method
signatures as part of its duplicate checking algorithm. This check does
not listen to pragmas, so the only way to disable it is to turn it off
completely or increase the minimum duplicate lines so that it doesn't
trigger for functions with long, multi-line signatures.

When we decide to upgrade to pylint 2.8.3 or greater, we will be able to
use 'ignore-signatures = true' to the config instead.

I'd prefer not to keep us on the very bleeding edge of pylint if I can
help it -- 2.8.3 came out only three days ago at time of writing.

See:https://github.com/PyCQA/pylint/pull/4474
Signed-off-by: John Snow


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v5 07/11] block: use int64_t instead of int in driver write_zeroes handlers

[Vladimir Sementsov-Ogievskiy]

04.06.2021 23:09, Eric Blake wrote:

On Wed, May 05, 2021 at 10:49:57AM +0300, Vladimir Sementsov-Ogievskiy wrote:

We are generally moving to int64_t for both offset and bytes parameters
on all io paths.

Main motivation is realization of 64-bit write_zeroes operation for
fast zeroing large disk chunks, up to the whole disk.

We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).

So, convert driver write_zeroes handlers bytes parameter to int64_t.

The only caller of all updated function is bdrv_co_do_pwrite_zeroes().

bdrv_co_do_pwrite_zeroes() itself is of course OK with widening of
callee parameter type. Also, bdrv_co_do_pwrite_zeroes()'s
max_write_zeroes is limited to INT_MAX. So, updated functions all are
safe, the will not get "bytes" larger than before.


they



Still, let's look through all updated functions, and add assertions to
the ones which are actually unprepared to values larger than INT_MAX.
For these drivers also set explicit max_pwrite_zeroes limit.

Let's go:

backup-top: Calls backup_top_cbw() and bdrv_co_pwrite_zeroes, both
   have 64bit argument.


backup_top_cbw has uint64_t argument (at least on current qemu.git; I
didn't spot if it was changed in the meantime earlier in this series
or if I'm missing review of a prerequisite), but we're safe since the
block layer does not pass in negative values.



blkdebug: calculations can't overflow, thanks to
   bdrv_check_qiov_request() in generic layer. rule_check() and
   bdrv_co_pwrite_zeroes() both have 64bit argument.


rule_check() is another function currently using uint64_t.



blklogwrites: pass to blk_log_writes_co_log() with 64bit argument.


That, and struct BlkLogWritesFileReq, still use uint64_t.



blkreply, copy-on-read, filter-compress: pass to bdrv_co_pwrite_zeroes() which 
is OK


blkreplay



file-posix: both handler calls raw_do_pwrite_zeroes, which is updated.
   In raw_do_pwrite_zeroes() calculations are OK due to
   bdrv_check_qiov_request(), bytes go to RawPosixAIOData::aio_nbytes
   which is uint64_t.


We also have to check where that uint64_t gets handed;
handle_aiocb_write_zeroes_block() passes a uint64_t[2] to
ioctl(BLKZEROOUT), handle_aiocb_write_zeroes() calls do_fallocate()
which takes off_t (and we compile to always have 64-bit off_t), as
does handle_aiocb_write_zeroes_unmap.  All look safe.



gluster: bytes go to GlusterAIOCB::size which is int64_t and to
   glfs_zerofill_async works with off_t.

iscsi: Aha, here we deal with iscsi_writesame16_task() that has
   uint32_t num_blocks argument and iscsi_writesame16_task() has


s/16/10/


   uint16_t argument. Make comments, add assertions and clarify
   max_pwrite_zeroes calculation.
   iscsi_allocmap_() functions already has int64_t argument
   is_byte_request_lun_aligned is simple to update, do it.

mirror_top: pass to bdrv_mirror_top_do_write which has uint16_t


s/16/64/


   argument

nbd: Aha, here we have protocol limitation, and NBDRequest::len is
   uint32_t. max_pwrite_zeroes is cleanly set to 32bit value, so we are
   OK for now.

nvme: Again, protocol limitation. And no inherent limit for
   write-zeroes at all. But from code that calculates cdw12 it's obvious
   that we do have limit and alignment. Let's clarify it. Also,
   obviously the code is not prepared to bytes=0. Let's handle this


to handle bytes=0


   case too.
   trace events already 64bit

qcow2: offset + bytes and alignment still works good (thanks to
   bdrv_check_qiov_request()), so tail calculation is OK
   qcow2_subcluster_zeroize() has 64bit argument, should be OK
   trace events updated

qed: qed_co_request wants int nb_sectors. Also in code we have size_t
   used for request length which may be 32bit.. So, let's just keep


Double dot looks odd.


   INT_MAX as a limit (aligning it down to pwrite_zeroes_alignment) and
   don't care.


Yeah, even when size_t is 64-bit, qed is not our high priority so
sticking to 32-bit limit encourages people to switch away from qed ;)



raw-format: Is OK. raw_adjust_offset and bdrv_co_pwrite_zeroes are both
   64bit.


I probably already mentioned it in earlier reviews, but hopefully by
the end of the series, we clean up raw_adjust_offset() to take
int64_t* instead of uint64_t* to get rid of ugly casts.  Doesn't have
to be done in this patch.



throttle: Both throttle_group_co_io_limits_intercept() and
   bdrv_co_pwrite_zeroes() are 64bit.

vmdk: pass to vmdk_pwritev which is 64bit

quorum: pass to quorum_co_pwritev() which is 64bit

preallocated: pass to handle_write() and bdrv_co_pwrite_zeroes(), both


File is named preallocate.c, the 'd' seems odd. Worth sorting this
before qcow2 in the commit message?


I think yes




   64bit.

Hooray!

At this point all block drivers are prepared to 64bit write-zero
requests or has explicitly set max_pwrite_zeroes.


are prepared to support 64bit write-zero requests, or have explicitly set



Signed-off-

Re: [PATCH RFC 0/3] python/iotests: Run iotest linters during Python CI

[Vladimir Sementsov-Ogievskiy]

04.06.2021 19:39, John Snow wrote:

Since iotests are such a heavy and prominent user of the Python qemu.qmp
and qemu.machine packages, it would be convenient if the Python linting
suite also checked this client for any possible regressions introduced
by shifting around signatures, types, or interfaces in these packages.


I think that's a right idea.



(Of course, we'd eventually find those problems when iotest 297 ran, but
with increasing distance between Python development and Block
development, the risk of an accidental breakage in this regard
increases. I, personally, know to run iotests (and especially 297) after
changing Python code, but not everyone in the future might.)

Add the ability for the Python CI to run the iotest linters too, which
means that iotests would be checked against:

- Python 3.6, using a frozen set of packages using 'pipenv'
- Python 3.6 through Python 3.10 inclusive, using 'tox' and the latest
   versions of mypy/pylint that happen to be installed during test
   time. (This CI test is allowed to fail with a warning, and can serve
   as a bellwether for when new incompatible changes may disrupt the
   linters. Testing against old and new Python interpreters alike can
   help surface incompatibility issues we may need to be aware of.)

It also means that you can cd to ./python and:

- "make venv-check", if you have Python 3.6 and pipenv installed. (On
   Fedora: `dnf install python36` or `dnf install python3.6`) This will
   set up a venv with exactly the same versions of all packages and their
   dependencies as the CI test does. After this series, it will run the
   iotest linters, too.

- "make check-tox", if you have Python 3.6 through Python 3.10
   installed. (On Fedora: `dnf install python3-tox python3.10`) This will
   set up five different venvs, one for each Python version, and run all
   of the Python linters against each. After this series, it will also
   include the iotest linters.


So, it doesn't run from "make check"?



"John, that's annoying. None of those invocations are free from some
kind of annoying dependency. Not everyone runs Fedora!"

Yeah, yeah. This series doesn't *remove* iotest 297 either. It continues
to work just fine! There's also a slightly more involved method that
will run on "any version you happen to have", but the setup is more
laborious, and I haven't made a Makefile invocation to canonize it yet:


cd /python
python3 -m venv ~/.cache/qemu-venv/
source ~/.cache/qemu-venv/bin/activate
make develop
make check
deactivate


- This uses whatever version of Python you happen to have, and doesn't
   require pipenv or tox.
- It should work on any distro with any python3 >= 3.6.0
- use 'activate.[fish|csh] as desired to enter the venv. (I use FiSH!)
- This will run the linters with correct versions against the qemu
   packages installed into this venv.

Example outputs from the three different local execution methods, in
order as outlined above:

jsnow@scv ~/s/q/python (python-package-iotest)> make venv-check
make[1]: Entering directory '/home/jsnow/src/qemu/python'
JOB ID : f5f383275da6b9d5eb5fe717e463f47f18980d07
JOB LOG: 
/home/jsnow/avocado/job-results/job-2021-06-04T12.28-f5f3832/job.log
  (1/5) tests/flake8.sh: PASS (0.43 s)
  (2/5) tests/iotests.sh: PASS (9.93 s)
  (3/5) tests/isort.sh: PASS (0.24 s)
  (4/5) tests/mypy.sh: PASS (0.25 s)
  (5/5) tests/pylint.sh: PASS (3.66 s)
RESULTS: PASS 5 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 14.85 s
make[1]: Leaving directory '/home/jsnow/src/qemu/python'

jsnow@scv ~/s/q/python (python-package-iotest)> make check-tox
GLOB sdist-make: /home/jsnow/src/qemu/python/setup.py
py36 inst-nodeps: 
/home/jsnow/src/qemu/python/.tox/.tmp/package/1/qemu-0.6.1.0a1.zip
py36 installed: 
appdirs==1.4.4,astroid==2.5.6,avocado-framework==88.1,distlib==0.3.2,filelock==3.0.12,flake8==3.9.2,fusepy==3.0.1,importlib-metadata==4.5.0,importlib-resources==5.1.4,isort==5.8.0,lazy-object-proxy==1.6.0,mccabe==0.6.1,mypy==0.812,mypy-extensions==0.4.3,packaging==20.9,pluggy==0.13.1,py==1.10.0,pycodestyle==2.7.0,pyflakes==2.3.1,pylint==2.8.3,pyparsing==2.4.7,qemu
 @ 
file:///home/jsnow/src/qemu/python/.tox/.tmp/package/1/qemu-0.6.1.0a1.zip,six==1.16.0,toml==0.10.2,tox==3.23.1,typed-ast==1.4.3,typing-extensions==3.10.0.0,virtualenv==20.4.7,wrapt==1.12.1,zipp==3.4.1
py36 run-test-pre: PYTHONHASHSEED='1077404307'
py36 run-test: commands[0] | make check
JOB ID : 8d6a98b947956794e83943950a66dea2e2ee2f0b
JOB LOG: 
/home/jsnow/avocado/job-results/job-2021-06-04T12.29-8d6a98b/job.log
  (1/5) tests/flake8.sh:  PASS (0.36 s)
  (2/5) tests/iotests.sh:  PASS (9.64 s)
  (3/5) tests/isort.sh:  PASS (0.19 s)
  (4/5) tests/mypy.sh:  PASS (0.24 s)
  (5/5) tests/pylint.sh:  PASS (3.64 s)
RESULTS: PASS 5 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 14.38 s
py37 inst-nodeps: 
/home/jsnow/src/qemu/python/.tox/.tmp/package/1/qemu-0.6.1.0a1.zip
py37 installed: 
appdi

Re: [PATCH RFC 2/3] iotests: split 'linters.py' off from 297

[Vladimir Sementsov-Ogievskiy]

04.06.2021 19:39, John Snow wrote:

Refactor the core function of the linting configuration out of 297 and
into a new file called linters.py.

Now, linters.py represents an invocation of the linting scripts that
more resembles a "normal" execution of pylint/mypy, like you'd expect to
use if 'qemu' was a bona-fide package you obtained from PyPI.

297, by contrast, now represents the iotests-specific configuration bits
you need to get it to function correctly as a part of iotests, and with
'qemu' as a namespace package that isn't "installed" to the current
environment, but just lives elsewhere in our source tree.

By doing this, we will able to run the same linting configuration from
the Python CI tests without calling iotest logging functions or messing
around with PYTHONPATH / MYPYPATH.

iotest 297 continues to operate in a standalone fashion for now --
presumably, it's convenient for block maintainers and contributors to
run in this manner.

See the following commit for how this is used from the Python packaging side.

Signed-off-by: John Snow 

---

- It's a big glob of a patch. Sorry. I can work it into smaller pieces
   if the idea is well received.


If at least split movement from other refactoring, would be good



- I change the invocations of mypy/pylint to "python3 -m pylint" and
   "python3 -m mypy" respectively, which causes these linters to use the
   virtual environment's preferred version. This forces the test to use the
   test environments curated by the CI jobs.

- If you have installed Fedora's pylint package that provides
   "pylint-3", the above trick will still work correctly.

- Checking for "pylint-3" specifically in 297 was left
   alone. Theoretically, this check could be broadened to simply look for
   the presence of a 'pylint' module to allow it to be more permissive.

Signed-off-by: John Snow 
---
  tests/qemu-iotests/297|  88 ---
  tests/qemu-iotests/linters.py | 130 ++
  2 files changed, 143 insertions(+), 75 deletions(-)
  create mode 100644 tests/qemu-iotests/linters.py

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 433b732336..5c753279fc 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -17,98 +17,36 @@
  # along with this program.  If not, see .
  
  import os

-import re
  import shutil


[..]


-# fixed (in tests, at least)
  env = os.environ.copy()
-qemu_module_path = os.path.join(os.path.dirname(__file__),
-'..', '..', 'python')
+qemu_module_path = os.path.join(
+os.path.dirname(__file__),
+'..', '..', 'python'
+)


Hmm, you made 3 lines from 2 :) ... If rename to python_path it will fit into 
one line. I'm not sure that it's better.


+
  try:
  env['PYTHONPATH'] += os.pathsep + qemu_module_path
  except KeyError:
  env['PYTHONPATH'] = qemu_module_path
-subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
-   env=env, check=False)
  
-print('=== mypy ===')

-sys.stdout.flush()
-
-# We have to call mypy separately for each file.  Otherwise, it
-# will interpret all given files as belonging together (i.e., they
-# may not both define the same classes, etc.; most notably, they
-# must not both define the __main__ module).
  env['MYPYPATH'] = env['PYTHONPATH']
-for filename in files:
-p = subprocess.run(('mypy',
-'--warn-unused-configs',
-'--disallow-subclassing-any',
-'--disallow-any-generics',
-'--disallow-incomplete-defs',
-'--disallow-untyped-decorators',
-'--no-implicit-optional',
-'--warn-redundant-casts',
-'--warn-unused-ignores',
-'--no-implicit-reexport',
-'--namespace-packages',
-filename),
-   env=env,
-   check=False,
-   stdout=subprocess.PIPE,
-   stderr=subprocess.STDOUT,
-   universal_newlines=True)
  
-if p.returncode != 0:

-print(p.stdout)
+for linter in ('pylint-3', 'mypy'):
+if shutil.which(linter) is None:
+iotests.notrun(f'{linter} not found')
  
+iotests.script_main(lambda: linters.run_linters(files, env=env))


Why to use lambda, and not just pass main to script_main?

Or, may be, use iotests.script_initialize() at top, and keep the whole script 
at top indentation level?


--
Best regards,
Vladimir

Re: [PATCH v4 5/6] block/blkdebug: remove new_state field and instead use a local variable

[Vladimir Sementsov-Ogievskiy]

04.06.2021 13:07, Emanuele Giuseppe Esposito wrote:

There seems to be no benefit in using a field. Replace it with a local
variable.

Signed-off-by: Emanuele Giuseppe Esposito 
---
  block/blkdebug.c | 13 +++--
  1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index dffd869b32..d597753139 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -40,7 +40,6 @@
  
  typedef struct BDRVBlkdebugState {

  int state;
-int new_state;
  uint64_t align;
  uint64_t max_transfer;
  uint64_t opt_write_zero;
@@ -792,7 +791,7 @@ static void suspend_request(BlockDriverState *bs, 
BlkdebugRule *rule)
  }
  
  static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,

- int *action_count)
+ int *action_count, int *new_state)
  {
  BDRVBlkdebugState *s = bs->opaque;
  
@@ -812,7 +811,8 @@ static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,

  break;
  
  case ACTION_SET_STATE:

-s->new_state = rule->options.set_state.new_state;
+assert(new_state != NULL);


you don't need additional assertion: crash on dereferencing NULL pointer is not 
less clear

anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy 


+*new_state = rule->options.set_state.new_state;
  break;
  
  case ACTION_SUSPEND:

@@ -825,13 +825,14 @@ static void blkdebug_debug_event(BlockDriverState *bs, 
BlkdebugEvent event)
  {
  BDRVBlkdebugState *s = bs->opaque;
  struct BlkdebugRule *rule, *next;
+int new_state;
  int actions_count[ACTION__MAX] = { 0 };
  
  assert((int)event >= 0 && event < BLKDBG__MAX);
  
-s->new_state = s->state;

+new_state = s->state;
  QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) {
-process_rule(bs, rule, actions_count);
+process_rule(bs, rule, actions_count, &new_state);
  }
  
  while (actions_count[ACTION_SUSPEND] > 0) {

@@ -839,7 +840,7 @@ static void blkdebug_debug_event(BlockDriverState *bs, 
BlkdebugEvent event)
  actions_count[ACTION_SUSPEND]--;
  }
  
-s->state = s->new_state;

+s->state = new_state;
  }
  
  static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event,





--
Best regards,
Vladimir

Re: [RFC PATCH 5/9] qapi: Implement 'next-machine-phase' command

[Paolo Bonzini]

Il ven 4 giu 2021, 16:26 Eric Blake  ha scritto:

> As an API, this command seems awkward.  How do I query what phase I'm
> currently in?  How many times do I have to call it?  Do we anticipate
> the number of times I need to call it to change in future qemu
> releases?
>

Indeed this has been changed in the last version of the proposal. There
will be an x-machine-init command and the existing x-exit-preconfig.

Paolo


> Would it be better to require me to pass an enum parameter stating the
> phase I intend to move into (and error out if that is not actually the
> next phase), where the introspection of the enum tells me how many
> times to use this command?  Should this command return a struct
> containing "new-state":"enum-value" telling me what state I moved to
> on success?
>
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>
>

Re: [PULL 00/15] tcg patch queue

[Peter Maydell]

On Fri, 4 Jun 2021 at 21:14, Richard Henderson
 wrote:
>
> The following changes since commit 1cbd2d914939ee6028e9688d4ba859a528c28405:
>
>   Merge remote-tracking branch 'remotes/jasowang/tags/net-pull-request' into 
> staging (2021-06-04 13:38:49 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/rth7680/qemu.git tags/pull-tcg-20210604
>
> for you to fetch changes up to 0006039e29b9e6118beab300146f7c4931f7a217:
>
>   tcg/arm: Implement TCG_TARGET_HAS_rotv_vec (2021-06-04 11:50:11 -0700)
>
> 
> Host vector support for arm neon.
>
> 
> Richard Henderson (15):
>   tcg: Change parameters for tcg_target_const_match
>   tcg/arm: Add host vector framework
>   tcg/arm: Implement tcg_out_ld/st for vector types
>   tcg/arm: Implement tcg_out_mov for vector types
>   tcg/arm: Implement tcg_out_dup*_vec
>   tcg/arm: Implement minimal vector operations
>   tcg/arm: Implement andc, orc, abs, neg, not vector operations
>   tcg/arm: Implement TCG_TARGET_HAS_shi_vec
>   tcg/arm: Implement TCG_TARGET_HAS_mul_vec
>   tcg/arm: Implement TCG_TARGET_HAS_sat_vec
>   tcg/arm: Implement TCG_TARGET_HAS_minmax_vec
>   tcg/arm: Implement TCG_TARGET_HAS_bitsel_vec
>   tcg/arm: Implement TCG_TARGET_HAS_shv_vec
>   tcg/arm: Implement TCG_TARGET_HAS_roti_vec
>   tcg/arm: Implement TCG_TARGET_HAS_rotv_vec


Applied, thanks.

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

-- PMM

Re: [PATCH v4 6/6] blkdebug: protect rules and suspended_reqs with a lock

[Vladimir Sementsov-Ogievskiy]

04.06.2021 13:07, Emanuele Giuseppe Esposito wrote:

First, categorize the structure fields to identify what needs
to be protected and what doesn't.

We essentially need to protect only .state, and the 3 lists in
BDRVBlkdebugState.

Then, add the lock and mark the functions accordingly.

Co-developed-by: Paolo Bonzini 
Signed-off-by: Emanuele Giuseppe Esposito 
---
  block/blkdebug.c | 46 +++---
  1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index d597753139..ac3799f739 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -38,24 +38,27 @@
  #include "qapi/qobject-input-visitor.h"
  #include "sysemu/qtest.h"
  
+/* All APIs are thread-safe */

+
  typedef struct BDRVBlkdebugState {
-int state;
+/* IN: initialized in blkdebug_open() and never changed */
  uint64_t align;
  uint64_t max_transfer;
  uint64_t opt_write_zero;
  uint64_t max_write_zero;
  uint64_t opt_discard;
  uint64_t max_discard;
-
+char *config_file; /* For blkdebug_refresh_filename() */
+/* initialized in blkdebug_parse_perms() */
  uint64_t take_child_perms;
  uint64_t unshare_child_perms;
  
-/* For blkdebug_refresh_filename() */

-char *config_file;
-
+/* State. Protected by lock */
+int state;
  QLIST_HEAD(, BlkdebugRule) rules[BLKDBG__MAX];
  QSIMPLEQ_HEAD(, BlkdebugRule) active_rules;
  QLIST_HEAD(, BlkdebugSuspendedReq) suspended_reqs;
+QemuMutex lock;
  } BDRVBlkdebugState;
  
  typedef struct BlkdebugAIOCB {

@@ -64,6 +67,7 @@ typedef struct BlkdebugAIOCB {
  } BlkdebugAIOCB;
  
  typedef struct BlkdebugSuspendedReq {

+/* IN: initialized in suspend_request() */
  Coroutine *co;
  char *tag;


@next is part of *suspended_reqs list (in a manner), so it should be protected 
by lock


  QLIST_ENTRY(BlkdebugSuspendedReq) next;
@@ -77,6 +81,7 @@ enum {
  };
  
  typedef struct BlkdebugRule {

+/* IN: initialized in add_rule() or blkdebug_debug_breakpoint() */
  BlkdebugEvent event;
  int action;
  int state;


as well as @next and @active_next here.


@@ -244,11 +249,14 @@ static int add_rule(void *opaque, QemuOpts *opts, Error 
**errp)




  };
  
  /* Add the rule */

+qemu_mutex_lock(&s->lock);
  QLIST_INSERT_HEAD(&s->rules[event], rule, next);
+qemu_mutex_unlock(&s->lock);
  


actually, add_rule is called only from .open(), so doesn't need a lock.. But it 
doesn't hurt.


  return 0;
  }
  
+/* Called with lock held or from .bdrv_close */

  static void remove_rule(BlkdebugRule *rule)
  {
  switch (rule->action) {
@@ -467,6 +475,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
*options, int flags,
  int ret;
  uint64_t align;
  
+qemu_mutex_init(&s->lock);

  opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
  if (!qemu_opts_absorb_qdict(opts, options, errp)) {
  ret = -EINVAL;
@@ -567,6 +576,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
*options, int flags,
  ret = 0;
  out:
  if (ret < 0) {
+qemu_mutex_destroy(&s->lock);
  g_free(s->config_file);
  }
  qemu_opts_del(opts);
@@ -581,6 +591,7 @@ static int rule_check(BlockDriverState *bs, uint64_t 
offset, uint64_t bytes,
  int error;
  bool immediately;
  
+qemu_mutex_lock(&s->lock);

  QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
  uint64_t inject_offset = rule->options.inject.offset;
  
@@ -594,6 +605,7 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes,

  }
  
  if (!rule || !rule->options.inject.error) {

+qemu_mutex_unlock(&s->lock);
  return 0;
  }
  
@@ -605,6 +617,7 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes,

  remove_rule(rule);
  }
  
+qemu_mutex_unlock(&s->lock);

  if (!immediately) {
  aio_co_schedule(qemu_get_current_aio_context(), 
qemu_coroutine_self());
  qemu_coroutine_yield();
@@ -770,8 +783,10 @@ static void blkdebug_close(BlockDriverState *bs)
  }
  
  g_free(s->config_file);

+qemu_mutex_destroy(&s->lock);
  }
  
+/* Called with lock held.  */

  static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
  {
  BDRVBlkdebugState *s = bs->opaque;
@@ -790,6 +805,7 @@ static void suspend_request(BlockDriverState *bs, 
BlkdebugRule *rule)
  }
  }
  
+/* Called with lock held.  */

  static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
   int *action_count, int *new_state)
  {
@@ -830,17 +846,18 @@ static void blkdebug_debug_event(BlockDriverState *bs, 
BlkdebugEvent event)
  
  assert((int)event >= 0 && event < BLKDBG__MAX);
  
-new_state = s->state;

-QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) {
-process_rule(bs, rule, actions_count, &new_state);
+WITH_QEMU_LOCK_G

Re: [PATCH 0/2] Improve vmstate_vmbus_dev handling

[Maciej S. Szmigiero]

Hi Thomas,

On 03.06.2021 12:41, Thomas Huth wrote:

I accidentally came accross vmstate_vmbus_dev and noticed that
it is currently not used at all ... wire it up and make it
static, since it is only used in one file.

Thomas Huth (2):
   hw/hyperv/vmbus: Wire up vmstate_vmbus_dev
   hw/hyperv/vmbus: Make vmstate_vmbus_dev static

  hw/hyperv/vmbus.c | 29 +++--
  include/hw/hyperv/vmbus.h |  3 ---
  2 files changed, 15 insertions(+), 17 deletions(-)



I think the idea is to embed vmstate_vmbus_dev into a child device
VMStateDescription using VMSTATE_STRUCT() - since particular VMBus
devices aren't merged yet there are currently no users of it.

This is similar how USB devices VMState is organized - see
vmstate_usb_device and VMSTATE_USB_DEVICE() macro.

Thanks,
Maciej

Re: [PATCH v16 61/99] target/arm: remove broad "else" statements when checking accels

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

From: Claudio Fontana

There might be more than just KVM and TCG in the future,
so where appropriate, replace broad "else" statements
with the appropriate if (accel_enabled()) check.

Also invert some checks for !kvm_enabled() or !tcg_enabled()
where it seems appropriate to do so.

Note that to make qtest happy we need to perform gpio
initialization in the qtest_enabled() case as well.

Hopefully we do not break any Xen stuff.

Signed-off-by: Claudio Fontana
Cc: Julien Grall
Cc: Stefano Stabellini
Cc: Olaf Hering
Cc: Alex Bennée
Signed-off-by: Alex Bennée
---
  target/arm/cpu.c |  9 +
  target/arm/cpu64.c   |  9 +
  target/arm/machine.c | 18 ++
  3 files changed, 16 insertions(+), 20 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v16 63/99] tests/qtest: skip bios-tables-test test_acpi_oem_fields_virt for KVM

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

From: Claudio Fontana

test is TCG-only.

Signed-off-by: Claudio Fontana
Cc: Philippe Mathieu-Daudé
Signed-off-by: Alex Bennée
---
  tests/qtest/bios-tables-test.c | 7 +++
  1 file changed, 7 insertions(+)


The new qtest_has_accel should be used instead of an ifdef.


r~

Re: [PATCH v16 64/99] tests: do not run test-hmp on all machines for ARM KVM-only

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

From: Claudio Fontana

on ARM we currently list and build all machines, even when
building KVM-only, without TCG.

Until we fix this (and we only list and build machines that are
compatible with KVM), only test specifically using the "virt"
machine in this case.

Signed-off-by: Claudio Fontana
Cc: Philippe Mathieu-Daudé
Signed-off-by: Alex Bennée
---
  tests/qtest/test-hmp.c | 20 
  1 file changed, 20 insertions(+)


qtest_has_accel, I assume.

Re: [PATCH v16 65/99] tests: device-introspect-test: cope with ARM TCG-only devices

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

From: Claudio Fontana

Skip the test_device_intro_concrete for now for ARM KVM-only build,
as on ARM we currently build devices for ARM that are not
compatible with a KVM-only build.

We can remove this workaround when we fix this in KConfig etc,
and we only list and build machines that are compatible with KVM
for KVM-only builds.

Alternative implementation provided by Alex.

Suggested-by: Alex Bennée
Signed-off-by: Claudio Fontana
Cc: Philippe Mathieu-Daudé
Signed-off-by: Alex Bennée
---
  tests/qtest/device-introspect-test.c | 32 +++-
  1 file changed, 27 insertions(+), 5 deletions(-)


Similarly, qtest_has_accel.

r~

Re: [PATCH v16 66/99] tests: do not run qom-test on all machines for ARM KVM-only

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

From: Claudio Fontana

on ARM we currently list and build all machines, even when
building KVM-only, without TCG.

Until we fix this (and we only list and build machines that are
compatible with KVM), only test specifically using the "virt"
machine in this case.

Signed-off-by: Claudio Fontana
Cc: Philippe Mathieu-Daudé
Signed-off-by: Alex Bennée
---
  tests/qtest/qom-test.c | 20 
  1 file changed, 20 insertions(+)


Similarly, qtest_has_accel.

r~

Re: [PATCH v16 69/99] target/arm: add tcg cpu accel class

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

From: Claudio Fontana 

move init, realizefn and reset code into it.

Signed-off-by: Claudio Fontana 
Cc: Paolo Bonzini 
Signed-off-by: Alex Bennée 
---
  target/arm/tcg/tcg-cpu.h|  4 ++-
  target/arm/cpu.c| 44 ++
  target/arm/tcg/sysemu/tcg-cpu.c | 27 
  target/arm/tcg/tcg-cpu-models.c | 10 +++---
  target/arm/tcg/tcg-cpu.c| 55 +++--
  5 files changed, 92 insertions(+), 48 deletions(-)

diff --git a/target/arm/tcg/tcg-cpu.h b/target/arm/tcg/tcg-cpu.h
index d93c6a6749..dd08587949 100644
--- a/target/arm/tcg/tcg-cpu.h
+++ b/target/arm/tcg/tcg-cpu.h
@@ -22,15 +22,17 @@
  
  #include "cpu.h"

  #include "hw/core/tcg-cpu-ops.h"
+#include "hw/core/accel-cpu.h"


Ideally we'd have a qemu/typedef.h entry so this include isn't needed...

  
  void arm_cpu_synchronize_from_tb(CPUState *cs,

   const TranslationBlock *tb);
  
-extern struct TCGCPUOps arm_tcg_ops;

+void tcg_arm_init_accel_cpu(AccelCPUClass *accel_cpu, CPUClass *cc);


... simply for this declaration.
Also, can we now remove the tcg-cpu-ops.h include?


@@ -1467,7 +1429,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void 
*data)
  cc->disas_set_info = arm_disas_set_info;
  
  #ifdef CONFIG_TCG

-cc->tcg_ops = &arm_tcg_ops;
+cc->init_accel_cpu = tcg_arm_init_accel_cpu;
  #endif /* CONFIG_TCG */


Is this already inside tcg_enabled()?  Because otherwise it looks as if this is 
overwriting a field also used by kvm.  Whereas the code that's being replaced 
set a field only used by tcg.


KVM sets its hooks differently, via kvm_cpu_accel_register_types, so I don't 
understand this hook at all.  But it seems like there should not be two 
different ways to set acc->cpu_instance_init.



r~

RE: [PATCH 2/2] vhost-vdpa: remove the unused vhost_vdpa_get_acked_features()

[Gautam Dawar]

No user for this helper, let's remove it.

[GD>>]  These patches seem unrelated to me. Do you think they should be part of 
one patch series?

Signed-off-by: Jason Wang 
---
 include/net/vhost-vdpa.h | 1 -
 net/vhost-vdpa.c | 9 -
 2 files changed, 10 deletions(-)

diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h index 
45e34b7cfc..b81f9a6f2a 100644
--- a/include/net/vhost-vdpa.h
+++ b/include/net/vhost-vdpa.h
@@ -15,7 +15,6 @@
 #define TYPE_VHOST_VDPA "vhost-vdpa"
 
 struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc); -uint64_t 
vhost_vdpa_get_acked_features(NetClientState *nc);
 
 extern const int vdpa_feature_bits[];
 
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index fe659ec9e2..8dc86332a6 
100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -66,15 +66,6 @@ VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
 return s->vhost_net;
 }
 
-uint64_t vhost_vdpa_get_acked_features(NetClientState *nc) -{
-VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
-assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
-s->acked_features = vhost_net_get_acked_features(s->vhost_net);
-
-return s->acked_features;
-}
-
 static int vhost_vdpa_net_check_device_id(struct vhost_net *net)  {
 uint32_t device_id;
--
2.25.1

Re: [PATCH v5 10/16] qemu-iotests: extend the check script to prepare supporting valgrind for python tests

[Emanuele Giuseppe Esposito]

On 05/06/2021 15:28, Vladimir Sementsov-Ogievskiy wrote:

04.06.2021 12:17, Emanuele Giuseppe Esposito wrote:

Currently, the check script only parses the option and sets the
VALGRIND_QEMU environmental variable to "y".
Add another local python variable that prepares the command line,
identical to the one provided in the test scripts.

Because the python script does not know in advance the valgrind
PID to assign to the log file name, use the "%p" flag in valgrind
log file name that automatically puts the process PID at runtime.

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Max Reitz 
---
  tests/qemu-iotests/check  |  7 ---
  tests/qemu-iotests/iotests.py | 11 +++
  tests/qemu-iotests/testenv.py |  1 +
  3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 1dba4218c0..e6aa110715 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -39,6 +39,10 @@ def make_argparser() -> argparse.ArgumentParser:
  p.add_argument('--gdb', action='store_true',
 help="start gdbserver with $GDB_OPTIONS options \
  ('localhost:12345' if $GDB_OPTIONS is empty)")
+    p.add_argument('--valgrind', action='store_true',
+    help='use valgrind, sets VALGRIND_QEMU environment '
+    'variable')
+
  p.add_argument('--misalign', action='store_true',
 help='misalign memory allocations')
  p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -88,9 +92,6 @@ def make_argparser() -> argparse.ArgumentParser:
  g_bash.add_argument('-o', dest='imgopts',
  help='options to pass to qemu-img 
create/convert, '

  'sets IMGOPTS environment variable')
-    g_bash.add_argument('--valgrind', action='store_true',
-    help='use valgrind, sets VALGRIND_QEMU 
environment '

-    'variable')
  g_sel = p.add_argument_group('test selecting options',
   'The following options specify test 
set '
diff --git a/tests/qemu-iotests/iotests.py 
b/tests/qemu-iotests/iotests.py

index c547e8c07b..3fa1bd0ab5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -96,6 +96,17 @@
  sys.stderr.write('Please run this test via the "check" script\n')
  sys.exit(os.EX_USAGE)
+qemu_valgrind = []
+if os.environ.get('VALGRIND_QEMU') == "y" and \
+    os.environ.get('NO_VALGRIND') != "y":


Hmm, interesting, why do you need additional NO_VALGRIND variable


To maintain consistency with the bash tests, where we have:

# Valgrind bug #409141 https://bugs.kde.org/show_bug.cgi?id=409141
# Until valgrind 3.16+ is ubiquitous, we must work around a hang in
# valgrind when issuing sigkill. Disable valgrind for this invocation.
_NO_VALGRIND()
{
NO_VALGRIND="y" "$@"
}





+    valgrind_logfile = "--log-file=" + test_dir
+    # %p allows to put the valgrind process PID, since
+    # we don't know it a priori (subprocess.Popen is
+    # not yet invoked)
+    valgrind_logfile += "/%p.valgrind"
+
+    qemu_valgrind = ['valgrind', valgrind_logfile, 
'--error-exitcode=99']

+
  socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 
'socket_scm_helper')

  luks_default_secret_object = 'secret,id=keysec0,data=' + \
diff --git a/tests/qemu-iotests/testenv.py 
b/tests/qemu-iotests/testenv.py

index 8501c6caf5..8bf154376f 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -298,6 +298,7 @@ def print_env(self) -> None:
  SOCK_DIR  -- {SOCK_DIR}
  SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
  GDB_OPTIONS   -- {GDB_OPTIONS}
+VALGRIND_QEMU -- {VALGRIND_QEMU}
  """
  args = collections.defaultdict(str, self.get_env())





Reviewed-by: Vladimir Sementsov-Ogievskiy 

[PATCH 1/2] Remove leading underscores from QEMU defines

[Ahmed Abouzied]

Leading underscores followed by a capital letter or underscore are
reserved by the C standard.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/369

Signed-off-by: Ahmed Abouzied 
---
 include/fpu/softfloat-helpers.h | 4 ++--
 include/hw/usb/dwc2-regs.h  | 4 ++--
 include/hw/usb/xlnx-usb-subsystem.h | 4 ++--
 include/hw/usb/xlnx-versal-usb2-ctrl-regs.h | 4 ++--
 include/qemu/plugin-memory.h| 4 ++--
 include/qemu/selfmap.h  | 4 ++--
 include/user/syscall-trace.h| 4 ++--
 plugins/plugin.h| 4 ++--
 tests/qtest/fuzz/qos_fuzz.h | 4 ++--
 tests/tcg/minilib/minilib.h | 4 ++--
 10 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/include/fpu/softfloat-helpers.h b/include/fpu/softfloat-helpers.h
index 34f4cf92ae..a98d759cd3 100644
--- a/include/fpu/softfloat-helpers.h
+++ b/include/fpu/softfloat-helpers.h
@@ -48,8 +48,8 @@ this code that are retained.
 ===
 */
 
-#ifndef _SOFTFLOAT_HELPERS_H_
-#define _SOFTFLOAT_HELPERS_H_
+#ifndef SOFTFLOAT_HELPERS_H
+#define SOFTFLOAT_HELPERS_H
 
 #include "fpu/softfloat-types.h"
 
diff --git a/include/hw/usb/dwc2-regs.h b/include/hw/usb/dwc2-regs.h
index 40af23a0ba..a7eb531485 100644
--- a/include/hw/usb/dwc2-regs.h
+++ b/include/hw/usb/dwc2-regs.h
@@ -39,8 +39,8 @@
  * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#ifndef __DWC2_HW_H__
-#define __DWC2_HW_H__
+#ifndef DWC2_HW_H
+#define DWC2_HW_H
 
 #define HSOTG_REG(x)   (x)
 
diff --git a/include/hw/usb/xlnx-usb-subsystem.h 
b/include/hw/usb/xlnx-usb-subsystem.h
index 739bef7f45..999e423951 100644
--- a/include/hw/usb/xlnx-usb-subsystem.h
+++ b/include/hw/usb/xlnx-usb-subsystem.h
@@ -22,8 +22,8 @@
  * THE SOFTWARE.
  */
 
-#ifndef _XLNX_VERSAL_USB_SUBSYSTEM_H_
-#define _XLNX_VERSAL_USB_SUBSYSTEM_H_
+#ifndef XLNX_VERSAL_USB_SUBSYSTEM_H
+#define XLNX_VERSAL_USB_SUBSYSTEM_H
 
 #include "hw/usb/xlnx-versal-usb2-ctrl-regs.h"
 #include "hw/usb/hcd-dwc3.h"
diff --git a/include/hw/usb/xlnx-versal-usb2-ctrl-regs.h 
b/include/hw/usb/xlnx-versal-usb2-ctrl-regs.h
index 975a717627..b76dce0419 100644
--- a/include/hw/usb/xlnx-versal-usb2-ctrl-regs.h
+++ b/include/hw/usb/xlnx-versal-usb2-ctrl-regs.h
@@ -23,8 +23,8 @@
  * THE SOFTWARE.
  */
 
-#ifndef _XLNX_USB2_REGS_H_
-#define _XLNX_USB2_REGS_H_
+#ifndef XLNX_USB2_REGS_H
+#define XLNX_USB2_REGS_H
 
 #define TYPE_XILINX_VERSAL_USB2_CTRL_REGS "xlnx.versal-usb2-ctrl-regs"
 
diff --git a/include/qemu/plugin-memory.h b/include/qemu/plugin-memory.h
index fbbe99474b..b36def27d7 100644
--- a/include/qemu/plugin-memory.h
+++ b/include/qemu/plugin-memory.h
@@ -6,8 +6,8 @@
  * SPDX-License-Identifier: GPL-2.0-or-later
  */
 
-#ifndef _PLUGIN_MEMORY_H_
-#define _PLUGIN_MEMORY_H_
+#ifndef PLUGIN_MEMORY_H
+#define PLUGIN_MEMORY_H
 
 struct qemu_plugin_hwaddr {
 bool is_io;
diff --git a/include/qemu/selfmap.h b/include/qemu/selfmap.h
index 8382c4c779..80cf920fba 100644
--- a/include/qemu/selfmap.h
+++ b/include/qemu/selfmap.h
@@ -6,8 +6,8 @@
  * SPDX-License-Identifier: GPL-2.0-or-later
  */
 
-#ifndef _SELFMAP_H_
-#define _SELFMAP_H_
+#ifndef SELFMAP_H
+#define SELFMAP_H
 
 typedef struct {
 unsigned long start;
diff --git a/include/user/syscall-trace.h b/include/user/syscall-trace.h
index 42e3b48b03..614cfacfa5 100644
--- a/include/user/syscall-trace.h
+++ b/include/user/syscall-trace.h
@@ -7,8 +7,8 @@
  * SPDX-License-Identifier: GPL-2.0-or-later
  */
 
-#ifndef _SYSCALL_TRACE_H_
-#define _SYSCALL_TRACE_H_
+#ifndef SYSCALL_TRACE_H
+#define SYSCALL_TRACE_H
 
 #include "trace/trace-root.h"
 
diff --git a/plugins/plugin.h b/plugins/plugin.h
index 55017e3581..b13677d0dc 100644
--- a/plugins/plugin.h
+++ b/plugins/plugin.h
@@ -9,8 +9,8 @@
  * SPDX-License-Identifier: GPL-2.0-or-later
  */
 
-#ifndef _PLUGIN_INTERNAL_H_
-#define _PLUGIN_INTERNAL_H_
+#ifndef PLUGIN_INTERNAL_H
+#define PLUGIN_INTERNAL_H
 
 #include 
 #include "qemu/qht.h"
diff --git a/tests/qtest/fuzz/qos_fuzz.h b/tests/qtest/fuzz/qos_fuzz.h
index 477f11b02b..63d8459b71 100644
--- a/tests/qtest/fuzz/qos_fuzz.h
+++ b/tests/qtest/fuzz/qos_fuzz.h
@@ -10,8 +10,8 @@
  * See the COPYING file in the top-level directory.
  */
 
-#ifndef _QOS_FUZZ_H_
-#define _QOS_FUZZ_H_
+#ifndef QOS_FUZZ_H
+#define QOS_FUZZ_H
 
 #include "tests/qtest/fuzz/fuzz.h"
 #include "tests/qtest/libqos/qgraph.h"
diff --git a/tests/tcg/minilib/minilib.h b/tests/tcg/minilib/minilib.h
index e23361380a..17d0f2f314 100644
--- a/tests/tcg/minilib/minilib.h
+++ b/tests/tcg/minilib/minilib.h
@@ -9,8 +9,8 @@
  * SPDX-License-Identifier: GPL-2.0-only
  */
 
-#ifndef _MINILIB_H_
-#define _MINILIB_H_
+#ifndef MINILIB_H
+#define MINILIB_H
 
 /*
  * Provided by the individual arch
-- 
2.25.1

[PATCH 2/2] Remove leading underscores from Xen defines

[Ahmed Abouzied]

Identifiers with leading underscores followed by capital letters or
underscores are reserved for C standards.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/369

Signed-off-by: Ahmed Abouzied 
---
 include/hw/xen/interface/grant_table.h  | 4 ++--
 include/hw/xen/interface/io/blkif.h | 4 ++--
 include/hw/xen/interface/io/console.h   | 4 ++--
 include/hw/xen/interface/io/fbif.h  | 4 ++--
 include/hw/xen/interface/io/kbdif.h | 4 ++--
 include/hw/xen/interface/io/netif.h | 4 ++--
 include/hw/xen/interface/io/protocols.h | 4 ++--
 include/hw/xen/interface/io/ring.h  | 4 ++--
 include/hw/xen/interface/io/usbif.h | 4 ++--
 9 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/hw/xen/interface/grant_table.h 
b/include/hw/xen/interface/grant_table.h
index 2af0cbdde3..c0a09dadad 100644
--- a/include/hw/xen/interface/grant_table.h
+++ b/include/hw/xen/interface/grant_table.h
@@ -25,8 +25,8 @@
  * Copyright (c) 2004, K A Fraser
  */
 
-#ifndef __XEN_PUBLIC_GRANT_TABLE_H__
-#define __XEN_PUBLIC_GRANT_TABLE_H__
+#ifndef XEN_PUBLIC_GRANT_TABLE_H
+#define XEN_PUBLIC_GRANT_TABLE_H
 
 /*
  * Reference to a grant entry in a specified domain's grant table.
diff --git a/include/hw/xen/interface/io/blkif.h 
b/include/hw/xen/interface/io/blkif.h
index d07fa1e078..680914571f 100644
--- a/include/hw/xen/interface/io/blkif.h
+++ b/include/hw/xen/interface/io/blkif.h
@@ -25,8 +25,8 @@
  * Copyright (c) 2012, Spectra Logic Corporation
  */
 
-#ifndef __XEN_PUBLIC_IO_BLKIF_H__
-#define __XEN_PUBLIC_IO_BLKIF_H__
+#ifndef XEN_PUBLIC_IO_BLKIF_H
+#define XEN_PUBLIC_IO_BLKIF_H
 
 #include "ring.h"
 #include "../grant_table.h"
diff --git a/include/hw/xen/interface/io/console.h 
b/include/hw/xen/interface/io/console.h
index e2155d1cf5..0d4a72456e 100644
--- a/include/hw/xen/interface/io/console.h
+++ b/include/hw/xen/interface/io/console.h
@@ -24,8 +24,8 @@
  * Copyright (c) 2005, Keir Fraser
  */
 
-#ifndef __XEN_PUBLIC_IO_CONSOLE_H__
-#define __XEN_PUBLIC_IO_CONSOLE_H__
+#ifndef XEN_PUBLIC_IO_CONSOLE_H
+#define XEN_PUBLIC_IO_CONSOLE_H
 
 typedef uint32_t XENCONS_RING_IDX;
 
diff --git a/include/hw/xen/interface/io/fbif.h 
b/include/hw/xen/interface/io/fbif.h
index ea87ebec0a..4e25423490 100644
--- a/include/hw/xen/interface/io/fbif.h
+++ b/include/hw/xen/interface/io/fbif.h
@@ -23,8 +23,8 @@
  * Copyright (C) 2006 Red Hat, Inc., Markus Armbruster 
  */
 
-#ifndef __XEN_PUBLIC_IO_FBIF_H__
-#define __XEN_PUBLIC_IO_FBIF_H__
+#ifndef XEN_PUBLIC_IO_FBIF_H
+#define XEN_PUBLIC_IO_FBIF_H
 
 /* Out events (frontend -> backend) */
 
diff --git a/include/hw/xen/interface/io/kbdif.h 
b/include/hw/xen/interface/io/kbdif.h
index 1d68cd458e..a952c77bf2 100644
--- a/include/hw/xen/interface/io/kbdif.h
+++ b/include/hw/xen/interface/io/kbdif.h
@@ -23,8 +23,8 @@
  * Copyright (C) 2006 Red Hat, Inc., Markus Armbruster 
  */
 
-#ifndef __XEN_PUBLIC_IO_KBDIF_H__
-#define __XEN_PUBLIC_IO_KBDIF_H__
+#ifndef XEN_PUBLIC_IO_KBDIF_H
+#define XEN_PUBLIC_IO_KBDIF_H
 
 /*
  *
diff --git a/include/hw/xen/interface/io/netif.h 
b/include/hw/xen/interface/io/netif.h
index 48fa530950..f4a28a43b1 100644
--- a/include/hw/xen/interface/io/netif.h
+++ b/include/hw/xen/interface/io/netif.h
@@ -24,8 +24,8 @@
  * Copyright (c) 2003-2004, Keir Fraser
  */
 
-#ifndef __XEN_PUBLIC_IO_NETIF_H__
-#define __XEN_PUBLIC_IO_NETIF_H__
+#ifndef XEN_PUBLIC_IO_NETIF_H
+#define XEN_PUBLIC_IO_NETIF_H
 
 #include "ring.h"
 #include "../grant_table.h"
diff --git a/include/hw/xen/interface/io/protocols.h 
b/include/hw/xen/interface/io/protocols.h
index 52b4de0f81..3d1cac322b 100644
--- a/include/hw/xen/interface/io/protocols.h
+++ b/include/hw/xen/interface/io/protocols.h
@@ -22,8 +22,8 @@
  * Copyright (c) 2008, Keir Fraser
  */
 
-#ifndef __XEN_PROTOCOLS_H__
-#define __XEN_PROTOCOLS_H__
+#ifndef XEN_PROTOCOLS_H
+#define XEN_PROTOCOLS_H
 
 #define XEN_IO_PROTO_ABI_X86_32 "x86_32-abi"
 #define XEN_IO_PROTO_ABI_X86_64 "x86_64-abi"
diff --git a/include/hw/xen/interface/io/ring.h 
b/include/hw/xen/interface/io/ring.h
index 115705f3f4..ea324c5a62 100644
--- a/include/hw/xen/interface/io/ring.h
+++ b/include/hw/xen/interface/io/ring.h
@@ -24,8 +24,8 @@
  * Tim Deegan and Andrew Warfield November 2004.
  */
 
-#ifndef __XEN_PUBLIC_IO_RING_H__
-#define __XEN_PUBLIC_IO_RING_H__
+#ifndef XEN_PUBLIC_IO_RING_H
+#define XEN_PUBLIC_IO_RING_H
 
 /*
  * When #include'ing this header, you need to provide the following
diff --git a/include/hw/xen/interface/io/usbif.h 
b/include/hw/xen/interface/io/usbif.h
index c6a58639d6..564c0115e8 100644
--- a/include/hw/xen/interface/io/usbif.h
+++ b/include/hw/xen/interface/io/usbif.h
@@ -25,8 +25,8 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
-#ifndef __XEN_PUBLIC_IO_USBIF_H__
-#define __XEN_PUBLIC_IO_USBIF_H__
+#ifndef XEN_PUBLIC_IO_USBIF_H
+#define XEN_PUBLIC_IO_USBIF_H
 
 #include "ring.h"
 #include "../grant_table.h"
-- 
2.25.1

Re: [PATCH v4 6/6] blkdebug: protect rules and suspended_reqs with a lock

[Emanuele Giuseppe Esposito]

On 05/06/2021 17:15, Vladimir Sementsov-Ogievskiy wrote:

04.06.2021 13:07, Emanuele Giuseppe Esposito wrote:

First, categorize the structure fields to identify what needs
to be protected and what doesn't.

We essentially need to protect only .state, and the 3 lists in
BDRVBlkdebugState.

Then, add the lock and mark the functions accordingly.

Co-developed-by: Paolo Bonzini 
Signed-off-by: Emanuele Giuseppe Esposito 
---
  block/blkdebug.c | 46 +++---
  1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index d597753139..ac3799f739 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -38,24 +38,27 @@
  #include "qapi/qobject-input-visitor.h"
  #include "sysemu/qtest.h"
+/* All APIs are thread-safe */
+
  typedef struct BDRVBlkdebugState {
-    int state;
+    /* IN: initialized in blkdebug_open() and never changed */
  uint64_t align;
  uint64_t max_transfer;
  uint64_t opt_write_zero;
  uint64_t max_write_zero;
  uint64_t opt_discard;
  uint64_t max_discard;
-
+    char *config_file; /* For blkdebug_refresh_filename() */
+    /* initialized in blkdebug_parse_perms() */
  uint64_t take_child_perms;
  uint64_t unshare_child_perms;
-    /* For blkdebug_refresh_filename() */
-    char *config_file;
-
+    /* State. Protected by lock */
+    int state;
  QLIST_HEAD(, BlkdebugRule) rules[BLKDBG__MAX];
  QSIMPLEQ_HEAD(, BlkdebugRule) active_rules;
  QLIST_HEAD(, BlkdebugSuspendedReq) suspended_reqs;
+    QemuMutex lock;
  } BDRVBlkdebugState;
  typedef struct BlkdebugAIOCB {
@@ -64,6 +67,7 @@ typedef struct BlkdebugAIOCB {
  } BlkdebugAIOCB;
  typedef struct BlkdebugSuspendedReq {
+    /* IN: initialized in suspend_request() */
  Coroutine *co;
  char *tag;


@next is part of *suspended_reqs list (in a manner), so it should be 
protected by lock



  QLIST_ENTRY(BlkdebugSuspendedReq) next;
@@ -77,6 +81,7 @@ enum {
  };
  typedef struct BlkdebugRule {
+    /* IN: initialized in add_rule() or blkdebug_debug_breakpoint() */
  BlkdebugEvent event;
  int action;
  int state;


as well as @next and @active_next here.


They all are already protected by the lock, I will just update the 
comments in case I need to re-spin.


[...]



Optional suggestion for additional comments and more use of 
QEMU_LOCK_GUARD (it looks large because of indentation change):


Exactly, indentation change. If I recall correctly, you (rightly) 
complained about that in one of my patches (not sure if it was in this 
series).




diff --git a/block/blkdebug.c b/block/blkdebug.c
index ac3799f739..b4f8844e76 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -70,6 +70,8 @@ typedef struct BlkdebugSuspendedReq {
  /* IN: initialized in suspend_request() */
  Coroutine *co;
  char *tag;
+
+    /* List entry protected BDRVBlkdebugState::lock */


Is this C++ style ok to be used here? I don't think I have seen it used 
in QEMU. But I might be wrong, someone with better style taste and 
experience should comment here. Maybe it's better to have


/* List entry protected BDRVBlkdebugState's lock */


  QLIST_ENTRY(BlkdebugSuspendedReq) next;
  } BlkdebugSuspendedReq;

@@ -100,6 +102,8 @@ typedef struct BlkdebugRule {
  char *tag;
  } suspend;
  } options;
+
+    /* List entries protected BDRVBlkdebugState::lock */
  QLIST_ENTRY(BlkdebugRule) next;
  QSIMPLEQ_ENTRY(BlkdebugRule) active_next;
  } BlkdebugRule;
@@ -249,9 +253,9 @@ static int add_rule(void *opaque, QemuOpts *opts, 
Error **errp)

  };

  /* Add the rule */
-    qemu_mutex_lock(&s->lock);
-    QLIST_INSERT_HEAD(&s->rules[event], rule, next);
-    qemu_mutex_unlock(&s->lock);
+    WITH_QEMU_LOCK_GUARD(&s->lock) {
+    QLIST_INSERT_HEAD(&s->rules[event], rule, next);
+    }
Same lines used, just additional indentation added. For one line it 
might be okay? not sure.


  return 0;
  }
@@ -591,33 +595,32 @@ static int rule_check(BlockDriverState *bs, 
uint64_t offset, uint64_t bytes,

  int error;
  bool immediately;

-    qemu_mutex_lock(&s->lock);
-    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
-    uint64_t inject_offset = rule->options.inject.offset;
-
-    if ((inject_offset == -1 ||
- (bytes && inject_offset >= offset &&
-  inject_offset < offset + bytes)) &&
-    (rule->options.inject.iotype_mask & (1ull << iotype)))
-    {
-    break;
+    WITH_QEMU_LOCK_GUARD(&s->lock) {
+    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
+    uint64_t inject_offset = rule->options.inject.offset;
+
+    if ((inject_offset == -1 ||
+ (bytes && inject_offset >= offset &&
+  inject_offset < offset + bytes)) &&
+    (rule->options.inject.iotype_mask & (1ull << iotype)))
+    {
+    break;
+    }
  

Re: [PATCH v16 71/99] target/arm: cpu-sve: new module

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

+#ifndef CPU_SVE_H
+#define CPU_SVE_H
+
+/* note: SVE is an AARCH64-only option, only include this for TARGET_AARCH64 */


This seems unnecessary.


@@ -201,13 +202,9 @@ typedef struct {
  
  #ifdef TARGET_AARCH64

  # define ARM_MAX_VQ16
-void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
-void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);
  #else
  # define ARM_MAX_VQ1
-static inline void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp) { }
-static inline void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp) { }
-#endif
+#endif /* TARGET_AARCH64 */


Hmm.  I'm not sure about removing the stubs, but see below.


diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 2fef8ca471..6db37b42d1 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -23,6 +23,7 @@
  #include "target/arm/idau.h"
  #include "qapi/error.h"
  #include "cpu.h"
+#include "cpu-sve.h"


Not following the advice given above, which I agree with.


+#ifdef TARGET_AARCH64
  if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
  arm_cpu_sve_finalize(cpu, &local_err);
  if (local_err != NULL) {
@@ -838,6 +840,7 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
  }
  }
  }
+#endif /* TARGET_AARCH64 */


New ifdefs, which isn't great.  But I also see that it's more or less a 1-1 
swap.  One ifdef here, or one ifdef in the header to create the stub.  So I 
guess it's a draw.


So, modulo the comment at the top,
Reviewed-by: Richard Henderson 


r~

Re: [PATCH v16 72/99] target/arm: cpu-sve: rename functions according to module prefix

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

@@ -276,8 +276,8 @@ static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, 
const char *name,
   * of the contents of "name" to determine which bit on which
   * to operate.
   */
-static void cpu_arm_get_sve_vq(Object *obj, Visitor *v, const char *name,
-   void *opaque, Error **errp)
+static void get_prop_vq(Object *obj, Visitor *v, const char *name,
+void *opaque, Error **errp)


The comment above this function needs some renames to match.
Otherwise,
Reviewed-by: Richard Henderson 



diff --git a/target/arm/kvm/kvm-cpu.c b/target/arm/kvm/kvm-cpu.c
index a23831e3c6..09aede9319 100644
--- a/target/arm/kvm/kvm-cpu.c
+++ b/target/arm/kvm/kvm-cpu.c
@@ -89,7 +89,7 @@ static void host_cpu_instance_init(Object *obj)
  
  kvm_arm_set_cpu_features_from_host(cpu);

  if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
-aarch64_add_sve_properties(obj);
+cpu_sve_add_props(obj);


As an aside, I think this if is always true.  kvm is 64-bit only now.  One can 
start 32-bit guests, with proper host cpu support, but it's the "aarch64" cpu 
property that controls this.  And all cpu properties are processed all at once.



r~

Re: [PATCH v16 74/99] target/arm: cpu-sve: make cpu_sve_finalize_features return bool

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

From: Claudio Fontana

return false on error, true on success.

Signed-off-by: Claudio Fontana
Signed-off-by: Alex Bennée
---
  target/arm/cpu-sve.h |  2 +-
  target/arm/cpu-sve.c | 17 +
  target/arm/cpu.c |  3 +--
  3 files changed, 11 insertions(+), 11 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v2] target/riscv: Do not include 'pmp.h' in user emulation

[Laurent Vivier]

Le 16/05/2021 à 22:53, Philippe Mathieu-Daudé a écrit :
> Physical Memory Protection is a system feature.
> Avoid polluting the user-mode emulation by its definitions.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/riscv/cpu.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 7e879fb9ca5..0619b491a42 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -97,7 +97,9 @@ enum {
>  
>  typedef struct CPURISCVState CPURISCVState;
>  
> +#if !defined(CONFIG_USER_ONLY)
>  #include "pmp.h"
> +#endif
>  
>  #define RV_VLEN_MAX 256
>  
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH] i386/kvm: The value passed to strerror should be positive

[Laurent Vivier]

Le 19/05/2021 à 13:35, Dmitry Voronetskiy a écrit :
> From: Dmitry Voronetskiy 
> 
> Signed-off-by: Dmitry Voronetskiy 
> 
> ---
>  hw/i386/kvm/apic.c   |  2 +-
>  hw/i386/kvm/clock.c  |  4 ++--
>  hw/i386/kvm/i8254.c  | 10 +-
>  hw/i386/kvm/i8259.c  |  4 ++--
>  hw/i386/kvm/ioapic.c |  4 ++--
>  5 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> index 52ff490910..1e89ca0899 100644
> --- a/hw/i386/kvm/apic.c
> +++ b/hw/i386/kvm/apic.c
> @@ -145,7 +145,7 @@ static void kvm_apic_put(CPUState *cs, run_on_cpu_data 
> data)
>  
>  ret = kvm_vcpu_ioctl(CPU(s->cpu), KVM_SET_LAPIC, &kapic);
>  if (ret < 0) {
> -fprintf(stderr, "KVM_SET_LAPIC failed: %s\n", strerror(ret));
> +fprintf(stderr, "KVM_SET_LAPIC failed: %s\n", strerror(-ret));
>  abort();
>  }
>  }
> diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> index efbc1e0d12..df70b4a033 100644
> --- a/hw/i386/kvm/clock.c
> +++ b/hw/i386/kvm/clock.c
> @@ -105,7 +105,7 @@ static void kvm_update_clock(KVMClockState *s)
>  
>  ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
>  if (ret < 0) {
> -fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
> +fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(-ret));
>  abort();
>  }
>  s->clock = data.clock;
> @@ -189,7 +189,7 @@ static void kvmclock_vm_state_change(void *opaque, bool 
> running,
>  data.clock = s->clock;
>  ret = kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
>  if (ret < 0) {
> -fprintf(stderr, "KVM_SET_CLOCK failed: %s\n", strerror(ret));
> +fprintf(stderr, "KVM_SET_CLOCK failed: %s\n", strerror(-ret));
>  abort();
>  }
>  
> diff --git a/hw/i386/kvm/i8254.c b/hw/i386/kvm/i8254.c
> index c558893961..fa68669e8a 100644
> --- a/hw/i386/kvm/i8254.c
> +++ b/hw/i386/kvm/i8254.c
> @@ -104,7 +104,7 @@ static void kvm_pit_get(PITCommonState *pit)
>  if (kvm_has_pit_state2()) {
>  ret = kvm_vm_ioctl(kvm_state, KVM_GET_PIT2, &kpit);
>  if (ret < 0) {
> -fprintf(stderr, "KVM_GET_PIT2 failed: %s\n", strerror(ret));
> +fprintf(stderr, "KVM_GET_PIT2 failed: %s\n", strerror(-ret));
>  abort();
>  }
>  pit->channels[0].irq_disabled = kpit.flags & 
> KVM_PIT_FLAGS_HPET_LEGACY;
> @@ -115,7 +115,7 @@ static void kvm_pit_get(PITCommonState *pit)
>   */
>  ret = kvm_vm_ioctl(kvm_state, KVM_GET_PIT, &kpit);
>  if (ret < 0) {
> -fprintf(stderr, "KVM_GET_PIT failed: %s\n", strerror(ret));
> +fprintf(stderr, "KVM_GET_PIT failed: %s\n", strerror(-ret));
>  abort();
>  }
>  }
> @@ -180,7 +180,7 @@ static void kvm_pit_put(PITCommonState *pit)
>  if (ret < 0) {
>  fprintf(stderr, "%s failed: %s\n",
>  kvm_has_pit_state2() ? "KVM_SET_PIT2" : "KVM_SET_PIT",
> -strerror(ret));
> +strerror(-ret));
>  abort();
>  }
>  }
> @@ -272,7 +272,7 @@ static void kvm_pit_realizefn(DeviceState *dev, Error 
> **errp)
>  }
>  if (ret < 0) {
>  error_setg(errp, "Create kernel PIC irqchip failed: %s",
> -   strerror(ret));
> +   strerror(-ret));
>  return;
>  }
>  switch (s->lost_tick_policy) {
> @@ -286,7 +286,7 @@ static void kvm_pit_realizefn(DeviceState *dev, Error 
> **errp)
>  if (ret < 0) {
>  error_setg(errp,
> "Can't disable in-kernel PIT reinjection: %s",
> -   strerror(ret));
> +   strerror(-ret));
>  return;
>  }
>  }
> diff --git a/hw/i386/kvm/i8259.c b/hw/i386/kvm/i8259.c
> index 3f8bf69e9c..d61bae4dc3 100644
> --- a/hw/i386/kvm/i8259.c
> +++ b/hw/i386/kvm/i8259.c
> @@ -43,7 +43,7 @@ static void kvm_pic_get(PICCommonState *s)
>  chip.chip_id = s->master ? KVM_IRQCHIP_PIC_MASTER : 
> KVM_IRQCHIP_PIC_SLAVE;
>  ret = kvm_vm_ioctl(kvm_state, KVM_GET_IRQCHIP, &chip);
>  if (ret < 0) {
> -fprintf(stderr, "KVM_GET_IRQCHIP failed: %s\n", strerror(ret));
> +fprintf(stderr, "KVM_GET_IRQCHIP failed: %s\n", strerror(-ret));
>  abort();
>  }
>  
> @@ -96,7 +96,7 @@ static void kvm_pic_put(PICCommonState *s)
>  
>  ret = kvm_vm_ioctl(kvm_state, KVM_SET_IRQCHIP, &chip);
>  if (ret < 0) {
> -fprintf(stderr, "KVM_SET_IRQCHIP failed: %s\n", strerror(ret));
> +fprintf(stderr, "KVM_SET_IRQCHIP failed: %s\n", strerror(-ret));
>  abort();
>  }
>  }
> diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
> index 71a563181e..ee7c8ef68b 100644
> --- a/hw/i386/kvm/ioapic.c
> +++ b/hw/i386/kvm/ioapic.c
> @@ -62,7 +62,7 @@ static void kvm_ioapic_get(IOAPICCommonState *s)
>  chip.chip_id = KVM_IRQCHIP_IOAPIC;
>  ret = kvm_vm_ioctl(k

Re: [PATCH] meson: Fix 'interpretor' typo

[Laurent Vivier]

Le 21/05/2021 à 12:34, Philippe Mathieu-Daudé a écrit :
> Fix a typo from commit fa2f7b0b9b7 ("meson: Warn when TCI is
> selected but TCG backend is available").
> 
> Reported-by: Peter Maydell 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 1559e8d873a..230a0e4b558 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -247,7 +247,7 @@
>error('Unsupported CPU @0@, try --enable-tcg-interpreter'.format(cpu))
>  endif
>elif get_option('tcg_interpreter')
> -warning('Use of the TCG interpretor is not recommended on this host')
> +warning('Use of the TCG interpreter is not recommended on this host')
>  warning('architecture. There is a native TCG execution backend 
> available')
>  warning('which provides substantially better performance and 
> reliability.')
>  warning('It is strongly recommended to remove the 
> --enable-tcg-interpreter')
> 

Reviewed-by: Laurent Vivier 

Re: [PATCH v2] hw/virtio: Document *_should_notify() are called within rcu_read_lock()

[Laurent Vivier]

Le 23/05/2021 à 11:40, Philippe Mathieu-Daudé a écrit :
> Such comments make reviewing this file somehow easier.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> v2: only one space before end of comment (mst)
> ---
>  hw/virtio/virtio.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index e02544b2df7..130e3568409 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2449,6 +2449,7 @@ static void virtio_set_isr(VirtIODevice *vdev, int 
> value)
>  }
>  }
>  
> +/* Called within rcu_read_lock(). */
>  static bool virtio_split_should_notify(VirtIODevice *vdev, VirtQueue *vq)
>  {
>  uint16_t old, new;
> @@ -2485,6 +2486,7 @@ static bool vring_packed_need_event(VirtQueue *vq, bool 
> wrap,
>  return vring_need_event(off, new, old);
>  }
>  
> +/* Called within rcu_read_lock(). */
>  static bool virtio_packed_should_notify(VirtIODevice *vdev, VirtQueue *vq)
>  {
>  VRingPackedDescEvent e;
> 

Reviewed-by: Laurent Vivier 

Re: [PATCH] misc: Correct relative include path

[Laurent Vivier]

Le 16/05/2021 à 22:50, Philippe Mathieu-Daudé a écrit :
> Headers should be included from the 'include/' directory,
> not from the root directory.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/i386/acpi-common.h | 6 +++---
>  include/monitor/monitor.h | 2 +-
>  hw/gpio/aspeed_gpio.c | 2 +-
>  hw/intc/ppc-uic.c | 2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/acpi-common.h b/hw/i386/acpi-common.h
> index b12cd73ea5d..a68825acf50 100644
> --- a/hw/i386/acpi-common.h
> +++ b/hw/i386/acpi-common.h
> @@ -1,9 +1,9 @@
>  #ifndef HW_I386_ACPI_COMMON_H
>  #define HW_I386_ACPI_COMMON_H
> -#include "include/hw/acpi/acpi_dev_interface.h"
>  
> -#include "include/hw/acpi/bios-linker-loader.h"
> -#include "include/hw/i386/x86.h"
> +#include "hw/acpi/acpi_dev_interface.h"
> +#include "hw/acpi/bios-linker-loader.h"
> +#include "hw/i386/x86.h"
>  
>  /* Default IOAPIC ID */
>  #define ACPI_BUILD_IOAPIC_ID 0x0
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index af3887bb71d..1211d6e6d69 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -4,7 +4,7 @@
>  #include "block/block.h"
>  #include "qapi/qapi-types-misc.h"
>  #include "qemu/readline.h"
> -#include "include/exec/hwaddr.h"
> +#include "exec/hwaddr.h"
>  
>  typedef struct MonitorHMP MonitorHMP;
>  typedef struct MonitorOptions MonitorOptions;
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> index 985a259e05b..db7ef88ee56 100644
> --- a/hw/gpio/aspeed_gpio.c
> +++ b/hw/gpio/aspeed_gpio.c
> @@ -10,7 +10,7 @@
>  #include "qemu/host-utils.h"
>  #include "qemu/log.h"
>  #include "hw/gpio/aspeed_gpio.h"
> -#include "include/hw/misc/aspeed_scu.h"
> +#include "hw/misc/aspeed_scu.h"
>  #include "qapi/error.h"
>  #include "qapi/visitor.h"
>  #include "hw/irq.h"
> diff --git a/hw/intc/ppc-uic.c b/hw/intc/ppc-uic.c
> index 7171de7b355..60013f2dde3 100644
> --- a/hw/intc/ppc-uic.c
> +++ b/hw/intc/ppc-uic.c
> @@ -23,7 +23,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> -#include "include/hw/intc/ppc-uic.h"
> +#include "hw/intc/ppc-uic.h"
>  #include "hw/irq.h"
>  #include "cpu.h"
>  #include "hw/ppc/ppc.h"
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH v2] linux-user/syscall: Constify bitmask_transtbl fcntl/mmap flags_tlb[]

[Laurent Vivier]

Le 17/05/2021 à 07:52, Philippe Mathieu-Daudé a écrit :
> Keep bitmask_transtbl in .rodata by marking the arrays const.
> 
> Reviewed-by: Laurent Vivier 
> Reviewed-by: Richard Henderson 
> Reviewed-by: Bin Meng 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  linux-user/syscall.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 95d79ddc437..64f486743a9 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -365,7 +365,7 @@ _syscall5(int, sys_statx, int, dirfd, const char *, 
> pathname, int, flags,
>  _syscall2(int, membarrier, int, cmd, int, flags)
>  #endif
>  
> -static bitmask_transtbl fcntl_flags_tbl[] = {
> +static const bitmask_transtbl fcntl_flags_tbl[] = {
>{ TARGET_O_ACCMODE,   TARGET_O_WRONLY,O_ACCMODE,   O_WRONLY,},
>{ TARGET_O_ACCMODE,   TARGET_O_RDWR,  O_ACCMODE,   O_RDWR,  },
>{ TARGET_O_CREAT, TARGET_O_CREAT, O_CREAT, O_CREAT, },
> @@ -6062,7 +6062,7 @@ static const StructEntry struct_termios_def = {
>  .print = print_termios,
>  };
>  
> -static bitmask_transtbl mmap_flags_tbl[] = {
> +static const bitmask_transtbl mmap_flags_tbl[] = {
>  { TARGET_MAP_SHARED, TARGET_MAP_SHARED, MAP_SHARED, MAP_SHARED },
>  { TARGET_MAP_PRIVATE, TARGET_MAP_PRIVATE, MAP_PRIVATE, MAP_PRIVATE },
>  { TARGET_MAP_FIXED, TARGET_MAP_FIXED, MAP_FIXED, MAP_FIXED },
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH] docs: fix broken reference

[Laurent Vivier]

Le 11/05/2021 à 21:29, John Snow a écrit :
> Long story short, we need a space here for the reference to work
> correctly.
> 
> 
> Longer story:
> 
> Without the space, kerneldoc generates a line like this:
> 
> one of :c:type:`MemoryListener.region_add\(\) 
> `,:c:type:`MemoryListener.region_del\(\)
> 
> Sphinx does not process the role information correctly, so we get this
> (my pseudo-notation) construct:
> 
> ,:c:type:
> MemoryListener.region_del()
> 
> which does not reference the desired entity, and leaves some extra junk
> in the rendered output. See
> https://qemu-project.gitlab.io/qemu/devel/memory.html#c.MemoryListener
> member log_start for an example of the broken output as it looks today.
> 
> Signed-off-by: John Snow 
> ---
>  include/exec/memory.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 5728a681b27..3b9ca696061 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -571,7 +571,7 @@ struct MemoryListener {
>   * @log_start:
>   *
>   * Called during an address space update transaction, after
> - * one of #MemoryListener.region_add(),#MemoryListener.region_del() or
> + * one of #MemoryListener.region_add(), #MemoryListener.region_del() or
>   * #MemoryListener.region_nop(), if dirty memory logging clients have
>   * become active since the last transaction.
>   *
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH] target/nios2: fix page-fit instruction count

[Laurent Vivier]

Le 25/05/2021 à 16:28, Richard Henderson a écrit :
> Laurent, how about through trivial?

Applied to my trivial-patches branch.

Thanks,
Laurent

> 
> r~
> 
> On 5/24/21 11:40 PM, Pavel Dovgalyuk wrote:
>> ping
>>
>> On 11.05.2021 11:40, Pavel Dovgalyuk wrote:
>>> This patch fixes calculation of number of the instructions
>>> that fit the current page. It prevents creation of the translation
>>> blocks that cross the page boundaries. It is required for deterministic
>>> exception generation in icount mode.
>>>
>>> Signed-off-by: Pavel Dovgalyuk 
>>> Reviewed-by: Richard Henderson 
>>> ---
>>>   target/nios2/translate.c |    2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/nios2/translate.c b/target/nios2/translate.c
>>> index 9824544eb3..399f22d938 100644
>>> --- a/target/nios2/translate.c
>>> +++ b/target/nios2/translate.c
>>> @@ -829,7 +829,7 @@ void gen_intermediate_code(CPUState *cs, 
>>> TranslationBlock *tb, int max_insns)
>>>   /* Set up instruction counts */
>>>   num_insns = 0;
>>>   if (max_insns > 1) {
>>> -    int page_insns = (TARGET_PAGE_SIZE - (tb->pc & TARGET_PAGE_MASK)) 
>>> / 4;
>>> +    int page_insns = (TARGET_PAGE_SIZE - (tb->pc & ~TARGET_PAGE_MASK)) 
>>> / 4;
>>>   if (max_insns > page_insns) {
>>>   max_insns = page_insns;
>>>   }
>>>
>>
> 
> 

Re: [PATCH v5 10/16] qemu-iotests: extend the check script to prepare supporting valgrind for python tests

[Vladimir Sementsov-Ogievskiy]

05.06.2021 20:38, Emanuele Giuseppe Esposito wrote:



On 05/06/2021 15:28, Vladimir Sementsov-Ogievskiy wrote:

04.06.2021 12:17, Emanuele Giuseppe Esposito wrote:

Currently, the check script only parses the option and sets the
VALGRIND_QEMU environmental variable to "y".
Add another local python variable that prepares the command line,
identical to the one provided in the test scripts.

Because the python script does not know in advance the valgrind
PID to assign to the log file name, use the "%p" flag in valgrind
log file name that automatically puts the process PID at runtime.

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Max Reitz 
---
  tests/qemu-iotests/check  |  7 ---
  tests/qemu-iotests/iotests.py | 11 +++
  tests/qemu-iotests/testenv.py |  1 +
  3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 1dba4218c0..e6aa110715 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -39,6 +39,10 @@ def make_argparser() -> argparse.ArgumentParser:
  p.add_argument('--gdb', action='store_true',
 help="start gdbserver with $GDB_OPTIONS options \
  ('localhost:12345' if $GDB_OPTIONS is empty)")
+    p.add_argument('--valgrind', action='store_true',
+    help='use valgrind, sets VALGRIND_QEMU environment '
+    'variable')
+
  p.add_argument('--misalign', action='store_true',
 help='misalign memory allocations')
  p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -88,9 +92,6 @@ def make_argparser() -> argparse.ArgumentParser:
  g_bash.add_argument('-o', dest='imgopts',
  help='options to pass to qemu-img create/convert, '
  'sets IMGOPTS environment variable')
-    g_bash.add_argument('--valgrind', action='store_true',
-    help='use valgrind, sets VALGRIND_QEMU environment '
-    'variable')
  g_sel = p.add_argument_group('test selecting options',
   'The following options specify test set '
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index c547e8c07b..3fa1bd0ab5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -96,6 +96,17 @@
  sys.stderr.write('Please run this test via the "check" script\n')
  sys.exit(os.EX_USAGE)
+qemu_valgrind = []
+if os.environ.get('VALGRIND_QEMU') == "y" and \
+    os.environ.get('NO_VALGRIND') != "y":


Hmm, interesting, why do you need additional NO_VALGRIND variable


To maintain consistency with the bash tests, where we have:

# Valgrind bug #409141 https://bugs.kde.org/show_bug.cgi?id=409141
# Until valgrind 3.16+ is ubiquitous, we must work around a hang in
# valgrind when issuing sigkill. Disable valgrind for this invocation.
_NO_VALGRIND()
{
     NO_VALGRIND="y" "$@"
}



A, hm, I see it in bash tests. So, it's intended to not set by user but by 
test.. But I doubt that python test will want to set environment variable to 
disable valgrind. Most probably they will want to set some 
valgrind_supported=False near supported_fmts=['qcow2']. So, we'll need 
valgrind_supported argument for execute_setup_common. But no reason to 
implement it until we don't need.






+    valgrind_logfile = "--log-file=" + test_dir
+    # %p allows to put the valgrind process PID, since
+    # we don't know it a priori (subprocess.Popen is
+    # not yet invoked)
+    valgrind_logfile += "/%p.valgrind"
+
+    qemu_valgrind = ['valgrind', valgrind_logfile, '--error-exitcode=99']
+
  socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
  luks_default_secret_object = 'secret,id=keysec0,data=' + \
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 8501c6caf5..8bf154376f 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -298,6 +298,7 @@ def print_env(self) -> None:
  SOCK_DIR  -- {SOCK_DIR}
  SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
  GDB_OPTIONS   -- {GDB_OPTIONS}
+VALGRIND_QEMU -- {VALGRIND_QEMU}
  """
  args = collections.defaultdict(str, self.get_env())





Reviewed-by: Vladimir Sementsov-Ogievskiy 






--
Best regards,
Vladimir

Re: [PATCH] hw/display/macfb: Classify the "nubus-macfb" as display device

[Laurent Vivier]

Le 31/05/2021 à 09:32, Thomas Huth a écrit :
> The "nubus-macfb" currently shows up as uncategorized device in
> the output of "-device help". Put it into the display category
> to fix this ugliness.
> 
> Signed-off-by: Thomas Huth 
> ---
>  hw/display/macfb.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index ff8bdb846b..d8183b9bbd 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -450,6 +450,7 @@ static void macfb_nubus_class_init(ObjectClass *klass, 
> void *data)
>  dc->desc = "Nubus Macintosh framebuffer";
>  dc->reset = macfb_nubus_reset;
>  dc->vmsd = &vmstate_macfb;
> +set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
>  device_class_set_props(dc, macfb_nubus_properties);
>  }
>  
> 


Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH] target/hppa: Remove unused 'memory.h' header

[Laurent Vivier]

Le 17/05/2021 à 12:15, Philippe Mathieu-Daudé a écrit :
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/hppa/cpu.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h
> index 61178fa6a2a..748270bfa31 100644
> --- a/target/hppa/cpu.h
> +++ b/target/hppa/cpu.h
> @@ -22,7 +22,6 @@
>  
>  #include "cpu-qom.h"
>  #include "exec/cpu-defs.h"
> -#include "exec/memory.h"
>  
>  /* PA-RISC 1.x processors have a strong memory model.  */
>  /* ??? While we do not yet implement PA-RISC 2.0, those processors have
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH] memory: Display MemoryRegion name in read/write ops trace events

[Laurent Vivier]

Le 07/03/2021 à 08:48, Philippe Mathieu-Daudé a écrit :
> MemoryRegion names is cached on first call to memory_region_name(),
> so displaying the name is trace events is cheap. Add it for read /
> write ops.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  softmmu/memory.c | 12 
>  softmmu/trace-events |  4 ++--
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 874a8fccdee..d4d9ab8828e 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -444,7 +444,8 @@ static MemTxResult  
> memory_region_read_accessor(MemoryRegion *mr,
>  trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, 
> size);
>  } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) 
> {
>  hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
> -trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, 
> size);
> +trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, 
> size,
> + memory_region_name(mr));
>  }
>  memory_region_shift_read_access(value, shift, mask, tmp);
>  return MEMTX_OK;
> @@ -466,7 +467,8 @@ static MemTxResult 
> memory_region_read_with_attrs_accessor(MemoryRegion *mr,
>  trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, 
> size);
>  } else if (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_READ)) 
> {
>  hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
> -trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, 
> size);
> +trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, 
> size,
> + memory_region_name(mr));
>  }
>  memory_region_shift_read_access(value, shift, mask, tmp);
>  return r;
> @@ -486,7 +488,8 @@ static MemTxResult 
> memory_region_write_accessor(MemoryRegion *mr,
>  trace_memory_region_subpage_write(get_cpu_index(), mr, addr, tmp, 
> size);
>  } else if 
> (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_WRITE)) {
>  hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
> -trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, 
> size);
> +trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, 
> size,
> +  memory_region_name(mr));
>  }
>  mr->ops->write(mr->opaque, addr, tmp, size);
>  return MEMTX_OK;
> @@ -506,7 +509,8 @@ static MemTxResult 
> memory_region_write_with_attrs_accessor(MemoryRegion *mr,
>  trace_memory_region_subpage_write(get_cpu_index(), mr, addr, tmp, 
> size);
>  } else if 
> (trace_event_get_state_backends(TRACE_MEMORY_REGION_OPS_WRITE)) {
>  hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
> -trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, 
> size);
> +trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, 
> size,
> +  memory_region_name(mr));
>  }
>  return mr->ops->write_with_attrs(mr->opaque, addr, tmp, size, attrs);
>  }
> diff --git a/softmmu/trace-events b/softmmu/trace-events
> index b80ca042e1f..359fb37cc8d 100644
> --- a/softmmu/trace-events
> +++ b/softmmu/trace-events
> @@ -9,8 +9,8 @@ cpu_in(unsigned int addr, char size, unsigned int val) "addr 
> 0x%x(%c) value %u"
>  cpu_out(unsigned int addr, char size, unsigned int val) "addr 0x%x(%c) value 
> %u"
>  
>  # memory.c
> -memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t 
> value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size 
> %u"
> -memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t 
> value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size 
> %u"
> +memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t 
> value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 
> 0x%"PRIx64" size %u name '%s'"
> +memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t 
> value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 
> 0x%"PRIx64" size %u name '%s'"
>  memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, 
> uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 
> 0x%"PRIx64" size %u"
>  memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, 
> uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 
> 0x%"PRIx64" size %u"
>  memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, 
> uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 
> 0x%"PRIx64" size %u"
> 

Reviewed-by: Laurent Vivier 

Re: [PATCH 2/3] target/mips: Fix 'Uncoditional' typo

[Laurent Vivier]

Le 02/06/2021 à 19:07, Philippe Mathieu-Daudé a écrit :
> Fix Uncoditional -> Unconditional typo.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/mips/tcg/translate.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
> index c03a8ae1fed..797eba44347 100644
> --- a/target/mips/tcg/translate.c
> +++ b/target/mips/tcg/translate.c
> @@ -12238,7 +12238,7 @@ static void gen_compute_compact_branch(DisasContext 
> *ctx, uint32_t opc,
>  }
>  
>  if (bcond_compute == 0) {
> -/* Uncoditional compact branch */
> +/* Unconditional compact branch */
>  switch (opc) {
>  case OPC_JIALC:
>  tcg_gen_movi_tl(cpu_gpr[31], ctx->base.pc_next + 4 + m16_lowbit);
> @@ -19092,7 +19092,7 @@ static void gen_compute_imm_branch(DisasContext *ctx, 
> uint32_t opc,
>  ctx->base.is_jmp = DISAS_NORETURN;
>  
>  if (cond == TCG_COND_ALWAYS) {
> -/* Uncoditional compact branch */
> +/* Unconditional compact branch */
>  gen_goto_tb(ctx, 0, ctx->btarget);
>  } else {
>  /* Conditional compact branch */
> @@ -19201,7 +19201,7 @@ static void 
> gen_compute_compact_branch_nm(DisasContext *ctx, uint32_t opc,
>  }
>  
>  if (bcond_compute == 0) {
> -/* Uncoditional compact branch */
> +/* Unconditional compact branch */
>  switch (opc) {
>  case OPC_BC:
>  gen_goto_tb(ctx, 0, ctx->btarget);
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH 3/3] scripts/oss-fuzz: Fix typo in documentation

[Laurent Vivier]

Le 02/06/2021 à 19:07, Philippe Mathieu-Daudé a écrit :
> While we only use stdin, the chardev is named 'stdio'.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py 
> b/scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py
> index 890e1def856..b154a25508f 100755
> --- a/scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py
> +++ b/scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py
> @@ -14,7 +14,7 @@
>  /path/to/crash 2> qtest_log_output
>  scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py qtest_log_output > qtest_trace
>  ./i386-softmmu/qemu-fuzz-i386 -machine q35,accel=qtest \
> --qtest stdin < qtest_trace
> +-qtest stdio < qtest_trace
>  
>  ### Details ###
>  
> 


Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH v16 73/99] target/arm: cpu-sve: split TCG and KVM functionality

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

  DECLARE_BITMAP(kvm_supported, ARM_MAX_VQ);
-DECLARE_BITMAP(tmp, ARM_MAX_VQ);
-uint32_t vq, max_vq = 0;
-
-/* Collect the set of vector lengths supported by KVM. */
-bitmap_zero(kvm_supported, ARM_MAX_VQ);
-if (kvm_enabled() && kvm_arm_sve_supported()) {
-kvm_arm_sve_get_vls(CPU(cpu), kvm_supported);
-} else if (kvm_enabled()) {
-assert(!cpu_isar_feature(aa64_sve, cpu));
-}
+uint32_t max_vq = 0;
  
+if (kvm_enabled()) {

+kvm_sve_get_supported_lens(cpu, kvm_supported);
+}


Previously, kvm_supported was always initialized.

I guess this is sort-of a cleanup.  But we've got 4 different checks for kvm 
and tcg.  I think we can actually tidy this up with a set of callbacks.


static bool do_sve_finalize(ARMCPU *cpu, Error **errp,
void (*get_supported_lens)(ARMCPU *, unsigned long *),
void (*enable_lens)(unsigned long *vq_map,
unsigned long *vq_init,
uint32_t max_vq,
unsigned long *supported),
void (*disable_lens)(unsigned long *vq_map,
 unsigned long *vq_init,
 unsigned long *supported,
 Error **errp),
void (*validate_lens)(unsigned long *vq_map,
  unsigned long *vq_init,
  unsigned long *supported,
  Error **errp, uint32_t max_vq))
{
...
}

bool cpu_sve_finalize_features(ARMCPU *cpu, Error **errp)
{
if (kvm_enabled()) {
return do_sve_finalize(cpu, errp,
   kvm_sve_get_supported_lens,
   kvm_sve_enable_lens,
   kvm_sve_disable_lens,
   kvm_sve_validate_lens);
} else if (tcg_enabled()) {
return do_sve_finalize(cpu, errp,
   tcg_sve_get_supported_lens,
   tcg_sve_enable_lens,
   tcg_sve_disable_lens,
   tcg_sve_validate_lens);
} else {
g_assert_not_reached(); /* ??? */
}
}

with

void tcg_sve_get_supported_lens(ARMCPU *cpu,
unsigned long *supported)
{
bitmap_fill(supported, ARM_MAX_VQ);
}

which we can later adjust for, e.g. -cpu a64fx and neoverse-v1, etc, which 
don't support all vq sizes.



r~

Re: [PATCH v2] vhost-vdpa: Remove redundant declaration of address_space_memory

[Laurent Vivier]

Le 17/05/2021 à 14:32, Xie Yongji a écrit :
> The symbol address_space_memory are already declared in
> include/exec/address-spaces.h. So let's add this header file
> and remove the redundant declaration in include/hw/virtio/vhost-vdpa.h.
> 
> Signed-off-by: Xie Yongji 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  hw/virtio/vhost-vdpa.c | 1 +
>  include/hw/virtio/vhost-vdpa.h | 1 -
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 8f2fb9f10b2a..ee51863d280b 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -18,6 +18,7 @@
>  #include "hw/virtio/vhost-backend.h"
>  #include "hw/virtio/virtio-net.h"
>  #include "hw/virtio/vhost-vdpa.h"
> +#include "exec/address-spaces.h"
>  #include "qemu/main-loop.h"
>  #include "cpu.h"
>  #include "trace.h"
> diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> index 28ca65018ed7..ae9ee7adb2d0 100644
> --- a/include/hw/virtio/vhost-vdpa.h
> +++ b/include/hw/virtio/vhost-vdpa.h
> @@ -21,5 +21,4 @@ typedef struct vhost_vdpa {
>  struct vhost_dev *dev;
>  } VhostVDPA;
>  
> -extern AddressSpace address_space_memory;
>  #endif
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH v4 6/6] blkdebug: protect rules and suspended_reqs with a lock

[Vladimir Sementsov-Ogievskiy]

05.06.2021 20:53, Emanuele Giuseppe Esposito wrote:



On 05/06/2021 17:15, Vladimir Sementsov-Ogievskiy wrote:

04.06.2021 13:07, Emanuele Giuseppe Esposito wrote:

First, categorize the structure fields to identify what needs
to be protected and what doesn't.

We essentially need to protect only .state, and the 3 lists in
BDRVBlkdebugState.

Then, add the lock and mark the functions accordingly.

Co-developed-by: Paolo Bonzini 
Signed-off-by: Emanuele Giuseppe Esposito 
---
  block/blkdebug.c | 46 +++---
  1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index d597753139..ac3799f739 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -38,24 +38,27 @@
  #include "qapi/qobject-input-visitor.h"
  #include "sysemu/qtest.h"
+/* All APIs are thread-safe */
+
  typedef struct BDRVBlkdebugState {
-    int state;
+    /* IN: initialized in blkdebug_open() and never changed */
  uint64_t align;
  uint64_t max_transfer;
  uint64_t opt_write_zero;
  uint64_t max_write_zero;
  uint64_t opt_discard;
  uint64_t max_discard;
-
+    char *config_file; /* For blkdebug_refresh_filename() */
+    /* initialized in blkdebug_parse_perms() */
  uint64_t take_child_perms;
  uint64_t unshare_child_perms;
-    /* For blkdebug_refresh_filename() */
-    char *config_file;
-
+    /* State. Protected by lock */
+    int state;
  QLIST_HEAD(, BlkdebugRule) rules[BLKDBG__MAX];
  QSIMPLEQ_HEAD(, BlkdebugRule) active_rules;
  QLIST_HEAD(, BlkdebugSuspendedReq) suspended_reqs;
+    QemuMutex lock;
  } BDRVBlkdebugState;
  typedef struct BlkdebugAIOCB {
@@ -64,6 +67,7 @@ typedef struct BlkdebugAIOCB {
  } BlkdebugAIOCB;
  typedef struct BlkdebugSuspendedReq {
+    /* IN: initialized in suspend_request() */
  Coroutine *co;
  char *tag;


@next is part of *suspended_reqs list (in a manner), so it should be protected 
by lock


  QLIST_ENTRY(BlkdebugSuspendedReq) next;
@@ -77,6 +81,7 @@ enum {
  };
  typedef struct BlkdebugRule {
+    /* IN: initialized in add_rule() or blkdebug_debug_breakpoint() */
  BlkdebugEvent event;
  int action;
  int state;


as well as @next and @active_next here.


They all are already protected by the lock, I will just update the comments in 
case I need to re-spin.

[...]



Optional suggestion for additional comments and more use of QEMU_LOCK_GUARD (it 
looks large because of indentation change):


Exactly, indentation change. If I recall correctly, you (rightly) complained 
about that in one of my patches (not sure if it was in this series).


Probably here, indentation doesn't become so big :)

Maximum is

WITH_ () {
  FOR {
 if {
***

And this if contains only one simple "break".

Of course, that's a kind of taste. I hope I was not too insistent that past 
time.





diff --git a/block/blkdebug.c b/block/blkdebug.c
index ac3799f739..b4f8844e76 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -70,6 +70,8 @@ typedef struct BlkdebugSuspendedReq {
  /* IN: initialized in suspend_request() */
  Coroutine *co;
  char *tag;
+
+    /* List entry protected BDRVBlkdebugState::lock */


Is this C++ style ok to be used here? I don't think I have seen it used in 
QEMU. But I might be wrong, someone with better style taste and experience 
should comment here. Maybe it's better to have

/* List entry protected BDRVBlkdebugState's lock */


OK for me, I don't care)

Hmm, looking at git log, I see :: syntax in my commits. And not only my.




  QLIST_ENTRY(BlkdebugSuspendedReq) next;
  } BlkdebugSuspendedReq;

@@ -100,6 +102,8 @@ typedef struct BlkdebugRule {
  char *tag;
  } suspend;
  } options;
+
+    /* List entries protected BDRVBlkdebugState::lock */
  QLIST_ENTRY(BlkdebugRule) next;
  QSIMPLEQ_ENTRY(BlkdebugRule) active_next;
  } BlkdebugRule;
@@ -249,9 +253,9 @@ static int add_rule(void *opaque, QemuOpts *opts, Error 
**errp)
  };

  /* Add the rule */
-    qemu_mutex_lock(&s->lock);
-    QLIST_INSERT_HEAD(&s->rules[event], rule, next);
-    qemu_mutex_unlock(&s->lock);
+    WITH_QEMU_LOCK_GUARD(&s->lock) {
+    QLIST_INSERT_HEAD(&s->rules[event], rule, next);
+    }

Same lines used, just additional indentation added. For one line it might be 
okay? not sure.


Same three lines, but don't need to call _unlock()..

I think, for last time I just get used to the thought that 
WITH_QEMU_LOCK_GUARD(){} is a good thing.

Here it's really doesn't make real sense. So, take the suggestion only if you 
like it, my r-b stands without it as well.



  return 0;
  }
@@ -591,33 +595,32 @@ static int rule_check(BlockDriverState *bs, uint64_t 
offset, uint64_t bytes,
  int error;
  bool immediately;

-    qemu_mutex_lock(&s->lock);
-    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
-    uint64_t inject_offset = rule->options.inject.offset;
-
-    if ((inject_offs

Re: [PATCH v16 73/99] target/arm: cpu-sve: split TCG and KVM functionality

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

+void kvm_sve_get_supported_lens(ARMCPU *cpu, unsigned long *kvm_supported)
+{
+/* Collect the set of vector lengths supported by KVM. */
+bitmap_zero(kvm_supported, ARM_MAX_VQ);
+
+if (kvm_arm_sve_supported()) {
+kvm_arm_sve_get_vls(CPU(cpu), kvm_supported);
+} else {
+assert(!cpu_isar_feature(aa64_sve, cpu));
+}
+}
+
+void kvm_sve_enable_lens(unsigned long *sve_vq_map,
+ unsigned long *sve_vq_init, uint32_t max_vq,
+ unsigned long *kvm_supported)
+{
+/*
+ * For KVM we have to automatically enable all supported unitialized
+ * lengths, even when the smaller lengths are not all powers-of-two.
+ */
+DECLARE_BITMAP(tmp, ARM_MAX_VQ);
+
+bitmap_andnot(tmp, kvm_supported, sve_vq_init, max_vq);
+bitmap_or(sve_vq_map, sve_vq_map, tmp, max_vq);
+}
+
+uint32_t kvm_sve_disable_lens(unsigned long *sve_vq_map,
+  unsigned long *sve_vq_init,
+  unsigned long *kvm_supported, Error **errp)
+{
+uint32_t max_vq, vq;
+
+/* Disabling a supported length disables all larger lengths. */
+for (vq = 1; vq <= ARM_MAX_VQ; ++vq) {
+if (test_bit(vq - 1, sve_vq_init) &&
+test_bit(vq - 1, kvm_supported)) {
+break;
+}
+}
+
+max_vq = vq <= ARM_MAX_VQ ? vq - 1 : ARM_MAX_VQ;
+bitmap_andnot(sve_vq_map, kvm_supported, sve_vq_init, max_vq);
+
+if (max_vq == 0 || bitmap_empty(sve_vq_map, max_vq)) {
+error_setg(errp, "cannot disable sve%d", vq * 128);
+error_append_hint(errp, "Disabling sve%d results in all "
+  "vector lengths being disabled.\n",
+  vq * 128);
+error_append_hint(errp, "With SVE enabled, at least one "
+  "vector length must be enabled.\n");
+return 0;
+}
+
+return max_vq;
+}
+
+bool kvm_sve_validate_lens(unsigned long *sve_vq_map, uint32_t max_vq,
+   unsigned long *kvm_supported, Error **errp,
+   uint32_t sve_max_vq)
+{
+/* Ensure the set of lengths matches what KVM supports. */
+DECLARE_BITMAP(tmp, ARM_MAX_VQ);
+uint32_t vq;
+
+bitmap_xor(tmp, sve_vq_map, kvm_supported, max_vq);
+if (bitmap_empty(tmp, max_vq)) {
+return true;
+}
+
+vq = find_last_bit(tmp, max_vq) + 1;
+if (test_bit(vq - 1, sve_vq_map)) {
+if (sve_max_vq) {
+error_setg(errp, "cannot set sve-max-vq=%d", sve_max_vq);
+error_append_hint(errp, "This KVM host does not support "
+  "the vector length %d-bits.\n", vq * 128);
+error_append_hint(errp, "It may not be possible to use "
+  "sve-max-vq with this KVM host. Try "
+  "using only sve properties.\n");
+} else {
+error_setg(errp, "cannot enable sve%d", vq * 128);
+error_append_hint(errp, "This KVM host does not support "
+  "the vector length %d-bits.\n", vq * 128);
+}
+} else {
+error_setg(errp, "cannot disable sve%d", vq * 128);
+error_append_hint(errp, "The KVM host requires all "
+  "supported vector lengths smaller "
+  "than %d bits to also be enabled.\n", max_vq * 128);
+}
+return false;
+}


Also, I think I mentioned this before in review of a previous version, I 
strongly suspect that aside from kvm_sve_get_supported_lens, the other 
functions will be used without change by all other hw virt systems.


So I think we'll do best, short term, to keep all of this in cpu-sve.c for now. 
 The tcg_enabled() test should elide all of the code that's not needed when 
!CONFIG_TCG.



r~

Re: [PATCH v16 75/99] target/arm: make is_aa64 and arm_el_is_aa64 a macro for !TARGET_AARCH64

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

From: Claudio Fontana 

when TARGET_AARCH64 is not defined, it is helpful to make
is_aa64() and arm_el_is_aa64 macros defined to "false".

This way we can make more code TARGET_AARCH64-only.

Signed-off-by: Claudio Fontana 
Signed-off-by: Alex Bennée 
---
  target/arm/cpu.h | 37 -
  1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index b9b9bd8b01..8614948543 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1060,6 +1060,11 @@ void aarch64_sve_narrow_vq(CPUARMState *env, unsigned 
vq);
  void aarch64_sve_change_el(CPUARMState *env, int old_el,
 int new_el, bool el0_a64);
  
+static inline bool is_a64(CPUARMState *env)

+{
+return env->aarch64;
+}
+
  /*
   * SVE registers are encoded in KVM's memory in an endianness-invariant 
format.
   * The byte at offset i from the start of the in-memory representation 
contains
@@ -1089,7 +1094,10 @@ static inline void aarch64_sve_narrow_vq(CPUARMState 
*env, unsigned vq) { }
  static inline void aarch64_sve_change_el(CPUARMState *env, int o,
   int n, bool a)
  { }
-#endif
+
+#define is_a64(env) ((void)env, false)


In theory, parenthesis are required around the env expansion.


+#define arm_el_is_aa64(env, el) ((void)env, (void)el, false)


Likewise.  Otherwise,
Reviewed-by: Richard Henderson 


r~

Re: [PATCH v16 76/99] target/arm: restrict rebuild_hflags_a64 to TARGET_AARCH64

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

--- a/target/arm/tcg/helper.c
+++ b/target/arm/tcg/helper.c
@@ -999,6 +999,8 @@ static CPUARMTBFlags rebuild_hflags_a32(CPUARMState *env, 
int fp_el,
  return rebuild_hflags_common_32(env, fp_el, mmu_idx, flags);
  }
  
+#ifdef TARGET_AARCH64

+
  static CPUARMTBFlags rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
  ARMMMUIdx mmu_idx)
  {
@@ -1122,6 +1124,14 @@ static CPUARMTBFlags rebuild_hflags_a64(CPUARMState 
*env, int el, int fp_el,
  return rebuild_hflags_common(env, fp_el, mmu_idx, flags);
  }
  
+#else

+
+QEMU_ERROR("this should have been optimized away!")
+CPUARMTBFlags rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
+ ARMMMUIdx mmu_idx);
+
+#endif /* TARGET_AARCH64 */
+
  static CPUARMTBFlags rebuild_hflags_internal(CPUARMState *env)
  {
  int el = arm_current_el(env);
@@ -1183,6 +1193,7 @@ void HELPER(rebuild_hflags_a32)(CPUARMState *env, int el)
  env->hflags = rebuild_hflags_a32(env, fp_el, mmu_idx);
  }
  
+#ifdef TARGET_AARCH64

  void HELPER(rebuild_hflags_a64)(CPUARMState *env, int el)
  {
  int fp_el = fp_exception_el(env, el);
@@ -1190,6 +1201,7 @@ void HELPER(rebuild_hflags_a64)(CPUARMState *env, int el)
  
  env->hflags = rebuild_hflags_a64(env, el, fp_el, mmu_idx);

  }
+#endif /* TARGET_AARCH64 */


It would be better to move some of this code to helper-a64.c, above multiple 
ifdefs.



r~

Re: [PATCH v16 77/99] target/arm: arch_dump: restrict ELFCLASS64 to AArch64

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

From: Claudio Fontana 

this will allow us to restrict more code to TARGET_AARCH64

Signed-off-by: Claudio Fontana 
Reviewed-by: Richard Henderson 
Signed-off-by: Alex Bennée 


I don't see this as an improvement, necessarily.  IIRC, arch_dump is sysemu 
only.

I think that for sysemu we should simply work to eliminate qemu-system-arm (or 
conversely, qemu-system-aarch64), running all Arm stuff with one binary.  At 
which point all this ifdeffery must be undone.




--- a/roms/u-boot
+++ b/roms/u-boot
@@ -1 +1 @@
-Subproject commit b46dd116ce03e235f2a7d4843c6278e1da44b5e1
+Subproject commit d3689267f92c5956e09cc7d1baa4700141662bff


Mistake.


r~

Re: [PATCH v16 78/99] target/arm: cpu-exceptions, cpu-exceptions-aa64: new modules

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

diff --git a/target/arm/cpu-exceptions-aa64.h b/target/arm/cpu-exceptions-aa64.h
new file mode 100644
index 00..64f800a15d
--- /dev/null
+++ b/target/arm/cpu-exceptions-aa64.h
@@ -0,0 +1,28 @@
+/*
+ * QEMU AArch64 CPU Exceptions Sysemu code
+ *
+ * Copyright (c) 2012 SUSE LINUX Products GmbH
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see
+ *
+ */
+
+#ifndef CPU_EXCEPTIONS_AA64_H
+#define CPU_EXCEPTIONS_AA64_H
+
+#include "cpu.h"
+
+void arm_cpu_do_interrupt_aarch64(CPUState *cs);
+
+#endif /* CPU_EXCEPTIONS_AA64_H */


I don't see the point in the excessive replication of header files, for exactly 
one declaration.  This is not the first example.


What's wrong with internal.h?  Or some other header that collects sysemu 
specific declarations?


Also, "cpu.h" is not required by this declaration, as I've pointed out before.


r~

Re: [PATCH v16 78/99] target/arm: cpu-exceptions, cpu-exceptions-aa64: new modules

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

From: Claudio Fontana 

after restricting AArch64-specific code to TARGET_AARCH64 builds,
we can now extract the exception handling code from cpu-sysemu,
and split its AArch64-specific part into its own module.

Signed-off-by: Claudio Fontana 
Signed-off-by: Alex Bennée 
---
  target/arm/cpu-exceptions-aa64.h |  28 ++
  target/arm/cpu-exceptions-aa64.c | 276 +
  target/arm/cpu-exceptions.c  | 445 


Oh, splitting stuff out of cpu-sysemu.c is fine, but back to the single binary 
theory, do we gain anything with two files?



diff --git a/target/arm/cpu-user.c b/target/arm/cpu-user.c
index 6a1a1fa273..a8e6f28ec6 100644
--- a/target/arm/cpu-user.c
+++ b/target/arm/cpu-user.c
@@ -12,6 +12,7 @@
  #include "qapi/qapi-commands-machine-target.h"
  #include "qapi/error.h"
  #include "cpu.h"
+#include "cpu-exceptions-aa64.h"


Please point to what requires this include.


r~

Re: [PATCH v16 79/99] target/arm: tcg: restrict ZCR cpregs to TARGET_AARCH64

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

From: Claudio Fontana

restrict zcr_el1, zcr_el2, zcr_no_el2, zcr_el3 reginfo,
and the related SVE functions to TARGET_AARCH64.

Signed-off-by: Claudio Fontana
Reviewed-by: Richard Henderson
Signed-off-by: Alex Bennée
---
  target/arm/tcg/cpregs.c | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/target/arm/tcg/cpregs.c b/target/arm/tcg/cpregs.c
index 8422da4335..56d56f7f81 100644
--- a/target/arm/tcg/cpregs.c
+++ b/target/arm/tcg/cpregs.c
@@ -5791,6 +5791,8 @@ static const ARMCPRegInfo debug_lpae_cp_reginfo[] = {
  REGINFO_SENTINEL
  };
  
+#ifdef TARGET_AARCH64

+
  static void zcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
uint64_t value)
  {
@@ -5843,6 +5845,8 @@ static const ARMCPRegInfo zcr_el3_reginfo = {
  .writefn = zcr_write, .raw_writefn = raw_write
  };
  
+#endif /* TARGET_AARCH64 */

+


Given that tcg/cpregs.c is > 7k lines, I wouldn't mind splitting the file on 
that principal.  But just sprinking ifdefs is not on.



r~

Re: [PATCH v16 80/99] target/arm: tcg-sve: import narrow_vq and change_el functions

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

From: Claudio Fontana 

aarch64_sve_narrow_vq and aarch64_sve_change_el are SVE-related
functions only used for TCG, so we can put them in the
tcg-sve.c module.

Signed-off-by: Claudio Fontana 
Reviewed-by: Richard Henderson 
Signed-off-by: Alex Bennée 
---
  target/arm/cpu.h |  7 ---
  target/arm/tcg/tcg-sve.h |  5 ++
  linux-user/syscall.c |  4 ++
  target/arm/cpu-exceptions-aa64.c |  1 +
  target/arm/tcg/cpregs.c  |  4 ++
  target/arm/tcg/helper-a64.c  |  1 +
  target/arm/tcg/helper.c  | 87 
  target/arm/tcg/tcg-sve.c | 86 +++
  8 files changed, 101 insertions(+), 94 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 8614948543..3edf8bb4ec 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1056,9 +1056,6 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, 
CPUState *cs,
  #ifdef TARGET_AARCH64
  int aarch64_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
  int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
-void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
-void aarch64_sve_change_el(CPUARMState *env, int old_el,
-   int new_el, bool el0_a64);
  
  static inline bool is_a64(CPUARMState *env)

  {
@@ -1090,10 +1087,6 @@ static inline uint64_t *sve_bswap64(uint64_t *dst, 
uint64_t *src, int nr)
  }
  
  #else

-static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { }
-static inline void aarch64_sve_change_el(CPUARMState *env, int o,
- int n, bool a)
-{ }
  
  #define is_a64(env) ((void)env, false)
  
diff --git a/target/arm/tcg/tcg-sve.h b/target/arm/tcg/tcg-sve.h

index 4bed809b9a..5855bb4289 100644
--- a/target/arm/tcg/tcg-sve.h
+++ b/target/arm/tcg/tcg-sve.h
@@ -21,4 +21,9 @@ uint32_t tcg_sve_disable_lens(unsigned long *sve_vq_map,
  bool tcg_sve_validate_lens(unsigned long *sve_vq_map, uint32_t max_vq,
 Error **errp);
  
+void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);

+
+void aarch64_sve_change_el(CPUARMState *env, int old_el,
+   int new_el, bool el0_a64);
+
  #endif /* TCG_SVE_H */
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index c9f812091c..db4b7b1e46 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -134,6 +134,10 @@
  #include "fd-trans.h"
  #include "tcg/tcg.h"
  
+#ifdef TARGET_AARCH64

+#include "tcg/tcg-sve.h"
+#endif /* TARGET_AARCH64 */
+
  #ifndef CLONE_IO
  #define CLONE_IO0x8000  /* Clone io context */
  #endif
diff --git a/target/arm/cpu-exceptions-aa64.c b/target/arm/cpu-exceptions-aa64.c
index 7daaba0426..adaf3bab17 100644
--- a/target/arm/cpu-exceptions-aa64.c
+++ b/target/arm/cpu-exceptions-aa64.c
@@ -21,6 +21,7 @@
  #include "qemu/osdep.h"
  #include "qemu/log.h"
  #include "cpu.h"
+#include "tcg/tcg-sve.h"
  #include "internals.h"
  #include "sysemu/tcg.h"


Ok, sure.

  
diff --git a/target/arm/tcg/cpregs.c b/target/arm/tcg/cpregs.c

index 56d56f7f81..9d3c9ae841 100644
--- a/target/arm/tcg/cpregs.c
+++ b/target/arm/tcg/cpregs.c
@@ -16,6 +16,10 @@
  #include "cpu-mmu.h"
  #include "cpregs.h"
  
+#ifdef TARGET_AARCH64

+#include "tcg/tcg-sve.h"
+#endif /* TARGET_AARCH64 */


Don't like the ifdef here.  If in the previous patch we had moved zcr to 
cpregs-a64.c, it certainly wouldn't be required.



diff --git a/target/arm/tcg/tcg-sve.c b/target/arm/tcg/tcg-sve.c
index 99cfde1f41..908d2c2f2c 100644
--- a/target/arm/tcg/tcg-sve.c
+++ b/target/arm/tcg/tcg-sve.c
@@ -24,6 +24,7 @@
  #include "sysemu/tcg.h"
  #include "cpu-sve.h"
  #include "tcg-sve.h"
+#include "cpu-exceptions-aa64.h"


Unnecessary.


r~

Re: [PATCH v16 82/99] target/arm: move sve_zcr_len_for_el to TARGET_AARCH64-only cpu-sve

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 3edf8bb4ec..e9bfb6f575 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -223,7 +223,8 @@ typedef struct ARMPACKey {
  } ARMPACKey;
  #else
  static inline void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp) { }
-#endif
+
+#endif /* TARGET_AARCH64 */
  
  /* See the commentary above the TBFLAG field definitions.  */

  typedef struct CPUARMTBFlags {


Unrelated change.


diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index b297d0e6aa..0e41854b92 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -23,7 +23,11 @@
  #include "target/arm/idau.h"
  #include "qapi/error.h"
  #include "cpu.h"
+
+#ifdef TARGET_AARCH64
  #include "cpu-sve.h"
+#endif /* TARGET_AARCH64 */
+


Unrelated change and unnecessary.


diff --git a/target/arm/tcg/helper.c b/target/arm/tcg/helper.c
index edc4b4cb4e..984dae7643 100644
--- a/target/arm/tcg/helper.c
+++ b/target/arm/tcg/helper.c
@@ -18,6 +18,10 @@
  #include "cpregs.h"
  #include "tcg-cpu.h"
  
+#ifdef TARGET_AARCH64

+#include "cpu-sve.h"
+#endif /* TARGET_AARCH64 */


Are the ifdefs really necessary?

r~

Re: [PATCH v16 84/99] target/arm: cpu-common: wrap a64-only check with is_a64

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

From: Claudio Fontana

now that is_a64() is just always false when !TARGET_AARCH64,
we can just use that instead of introducing a new ifdef.

Signed-off-by: Claudio Fontana
Signed-off-by: Alex Bennée
---
  target/arm/cpu-common.c | 10 +++---
  1 file changed, 7 insertions(+), 3 deletions(-)


Ok, I guess.
Acked-by: Richard Henderson 

r~

Re: [PATCH v16 85/99] target/arm: cpu-pauth: new module for ARMv8.3 Pointer Authentication

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

From: Claudio Fontana 

Pointer Authentication is an AARCH64-only ARMv8.3 optional
extension, whose cpu properties can be separated out in its own module.

Signed-off-by: Claudio Fontana 
Signed-off-by: Alex Bennée 
---
  target/arm/cpu.h   |  3 --
  target/arm/tcg/cpu-pauth.h | 34 
  target/arm/cpu.c   |  1 +
  target/arm/cpu64.c | 35 ++---
  target/arm/tcg/cpu-pauth.c | 63 ++
  target/arm/tcg/meson.build |  1 +
  6 files changed, 101 insertions(+), 36 deletions(-)
  create mode 100644 target/arm/tcg/cpu-pauth.h
  create mode 100644 target/arm/tcg/cpu-pauth.c

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index e9bfb6f575..02e0fe5dbd 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -216,13 +216,10 @@ typedef struct ARMPredicateReg {
  uint64_t p[DIV_ROUND_UP(2 * ARM_MAX_VQ, 8)] QEMU_ALIGNED(16);
  } ARMPredicateReg;
  
-void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);

  /* In AArch32 mode, PAC keys do not exist at all.  */
  typedef struct ARMPACKey {
  uint64_t lo, hi;
  } ARMPACKey;
-#else
-static inline void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp) { }
  
  #endif /* TARGET_AARCH64 */
  
diff --git a/target/arm/tcg/cpu-pauth.h b/target/arm/tcg/cpu-pauth.h

new file mode 100644
index 00..af127876fe
--- /dev/null
+++ b/target/arm/tcg/cpu-pauth.h
@@ -0,0 +1,34 @@
+/*
+ * QEMU AArch64 Pointer Authentication Extensions
+ *
+ * Copyright (c) 2013 Linaro Ltd
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see
+ * 
+ */
+
+#ifndef CPU_PAUTH_H
+#define CPU_PAUTH_H
+
+/* ARMv8.3 pauth is an AARCH64 option, only include this for TARGET_AARCH64 */
+
+#include "cpu.h"
+
+/* called by arm_cpu_finalize_features in realizefn */
+void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);
+
+/* add the CPU Pointer Authentication properties */
+void cpu_pauth_add_props(Object *obj);
+
+#endif /* CPU_PAUTH_H */


Similar comments re "cpu.h" and the proliferation of headers.
Otherwise,
Reviewed-by: Richard Henderson 

r~

Re: [PATCH v16 86/99] target/arm: cpu-pauth: change arm_cpu_pauth_finalize name and sig

[Richard Henderson]

On 6/4/21 8:52 AM, Alex Bennée wrote:

From: Claudio Fontana

make arm_cpu_pauth_finalize return a bool,
and make the name canonical for the module (cpu_pauth_finalize).

Signed-off-by: Claudio Fontana
Signed-off-by: Alex Bennée
---
  target/arm/tcg/cpu-pauth.h | 2 +-
  target/arm/cpu.c   | 3 +--
  target/arm/tcg/cpu-pauth.c | 5 -
  3 files changed, 6 insertions(+), 4 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v16 87/99] target/arm: move arm_cpu_finalize_features into cpu64

[Richard Henderson]

On 6/4/21 8:53 AM, Alex Bennée wrote:

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index fefb6954fc..c762f3f07a 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -469,6 +469,40 @@ static gchar *aarch64_gdb_arch_name(CPUState *cs)
  return g_strdup("aarch64");
  }
  
+void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)

+{
+Error *local_err = NULL;
+
+#ifdef TARGET_AARCH64
+if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
+if (!cpu_sve_finalize_features(cpu, &local_err)) {
+error_propagate(errp, local_err);
+return;
+}
+
+/*
+ * KVM does not support modifications to this feature.
+ * We have not registered the cpu properties when KVM
+ * is in use, so the user will not be able to set them.
+ */
+if (tcg_enabled()) {
+if (!cpu_pauth_finalize(cpu, &local_err)) {
+error_propagate(errp, local_err);
+return;
+}
+}
+}
+#endif /* TARGET_AARCH64 */


These ifdefs are useless.


diff --git a/target/arm/monitor.c b/target/arm/monitor.c
index 0c72bf7c31..95c1e72cd1 100644
--- a/target/arm/monitor.c
+++ b/target/arm/monitor.c
@@ -184,9 +184,11 @@ CpuModelExpansionInfo 
*qmp_query_cpu_model_expansion(CpuModelExpansionType type,
  if (!err) {
  visit_check_struct(visitor, &err);
  }
+#ifdef TARGET_AARCH64
  if (!err) {
  arm_cpu_finalize_features(ARM_CPU(obj), &err);
  }
+#endif /* TARGET_AARCH64 */
  visit_end_struct(visitor, NULL);
  visit_free(visitor);
  if (err) {
@@ -195,7 +197,9 @@ CpuModelExpansionInfo 
*qmp_query_cpu_model_expansion(CpuModelExpansionType type,
  return NULL;
  }
  } else {
+#ifdef TARGET_AARCH64
  arm_cpu_finalize_features(ARM_CPU(obj), &error_abort);
+#endif /* TARGET_AARCH64 */


Not keen on all of the extra ifdefs here.


r~

Re: [PATCH v16 88/99] target/arm: cpu64: rename arm_cpu_finalize_features

[Richard Henderson]

On 6/4/21 8:53 AM, Alex Bennée wrote:

@@ -469,11 +469,10 @@ static gchar *aarch64_gdb_arch_name(CPUState *cs)
  return g_strdup("aarch64");
  }
  
-void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)

+void aarch64_cpu_finalize_features(ARMCPU *cpu, Error **errp)
  {
  Error *local_err = NULL;
  
-#ifdef TARGET_AARCH64

  if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
  if (!cpu_sve_finalize_features(cpu, &local_err)) {
  error_propagate(errp, local_err);
@@ -492,7 +491,6 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
  }
  }
  }
-#endif /* TARGET_AARCH64 */
  


The ifdef removal shouldn't be buried in a rename.


r~

Re: [PATCH v16 89/99] target/arm: cpu64: some final cleanup on aarch64_cpu_finalize_features

[Richard Henderson]

On 6/4/21 8:53 AM, Alex Bennée wrote:

From: Claudio Fontana 

bail out immediately if ARM_FEATURE_AARCH64 is not set,
and add an else statement when checking for accelerators.

Signed-off-by: Claudio Fontana 
Signed-off-by: Alex Bennée 
---
  target/arm/cpu64.c | 33 -
  1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 3058e2c273..ecce8c4308 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -473,26 +473,25 @@ void aarch64_cpu_finalize_features(ARMCPU *cpu, Error 
**errp)
  {
  Error *local_err = NULL;
  
-if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {

-if (!cpu_sve_finalize_features(cpu, &local_err)) {
+if (!arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
+return;
+}


I'm not sure this is correct, either before or after.
What about nonsensical combinations such as

  -cpu max,aarch64=off,sve-vq-max=4

Don't we want cpu_sve_finalize_features and friends to produce an error about 
enabling sve without aarch64.


r~

Re: [PATCH v16 92/99] target/arm: remove v7m stub function for !CONFIG_TCG

[Richard Henderson]

On 6/4/21 8:53 AM, Alex Bennée wrote:

-if (arm_feature(env, ARM_FEATURE_M)) {
-return arm_v7m_mmu_idx_for_secstate(env, env->v7m.secure);
+if (tcg_enabled()) {
+if (arm_feature(env, ARM_FEATURE_M)) {


Merge the two if with &&?

Otherwise,
Reviewed-by: Richard Henderson 


r~

Re: [PATCH v16 93/99] meson: Introduce target-specific Kconfig

[Richard Henderson]

On 6/4/21 8:53 AM, Alex Bennée wrote:

From: Philippe Mathieu-Daudé 

Add a target-specific Kconfig.

Target foo now has CONFIG_FOO defined.

Two architecture have a particularity, ARM and MIPS:
their 64-bit version include the 32-bit subset.

Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <2021013316.232778-6-f4...@amsat.org>
---
  meson.build   |  3 ++-
  Kconfig   |  1 +
  target/Kconfig| 23 +++
  target/alpha/Kconfig  |  2 ++
  target/arm/Kconfig|  6 ++
  target/avr/Kconfig|  2 ++
  target/cris/Kconfig   |  2 ++
  target/hppa/Kconfig   |  2 ++
  target/i386/Kconfig   |  5 +
  target/lm32/Kconfig   |  2 ++
  target/m68k/Kconfig   |  2 ++
  target/microblaze/Kconfig |  2 ++
  target/mips/Kconfig   |  6 ++
  target/moxie/Kconfig  |  2 ++
  target/nios2/Kconfig  |  2 ++
  target/openrisc/Kconfig   |  2 ++
  target/ppc/Kconfig|  5 +
  target/riscv/Kconfig  |  5 +
  target/rx/Kconfig |  2 ++
  target/s390x/Kconfig  |  2 ++
  target/sh4/Kconfig|  2 ++
  target/sparc/Kconfig  |  5 +
  target/tilegx/Kconfig |  2 ++
  target/tricore/Kconfig|  2 ++
  target/unicore32/Kconfig  |  2 ++
  target/xtensa/Kconfig |  2 ++
  26 files changed, 92 insertions(+), 1 deletion(-)
  create mode 100644 target/Kconfig
  create mode 100644 target/alpha/Kconfig
  create mode 100644 target/arm/Kconfig
  create mode 100644 target/avr/Kconfig
  create mode 100644 target/cris/Kconfig
  create mode 100644 target/hppa/Kconfig
  create mode 100644 target/i386/Kconfig
  create mode 100644 target/lm32/Kconfig
  create mode 100644 target/m68k/Kconfig
  create mode 100644 target/microblaze/Kconfig
  create mode 100644 target/mips/Kconfig
  create mode 100644 target/moxie/Kconfig
  create mode 100644 target/nios2/Kconfig
  create mode 100644 target/openrisc/Kconfig
  create mode 100644 target/ppc/Kconfig
  create mode 100644 target/riscv/Kconfig
  create mode 100644 target/rx/Kconfig
  create mode 100644 target/s390x/Kconfig
  create mode 100644 target/sh4/Kconfig
  create mode 100644 target/sparc/Kconfig
  create mode 100644 target/tilegx/Kconfig
  create mode 100644 target/tricore/Kconfig
  create mode 100644 target/unicore32/Kconfig
  create mode 100644 target/xtensa/Kconfig


I guess you haven't rebased since unicore, moxie et al were removed?


--- a/meson.build
+++ b/meson.build
@@ -1359,7 +1359,8 @@ foreach target : target_dirs
command: [minikconf,
  get_option('default_devices') ? '--defconfig' : 
'--allnoconfig',
  config_devices_mak, '@DEPFILE@', '@INPUT@',
-host_kconfig, accel_kconfig])
+host_kconfig, accel_kconfig,
+'CONFIG_' + config_target['TARGET_ARCH'].to_upper() + '=y'])


I can understand this,

  
  config_devices_data = configuration_data()

  config_devices = keyval.load(config_devices_mak)
diff --git a/Kconfig b/Kconfig
index d52ebd839b..fb6a24a2de 100644
--- a/Kconfig
+++ b/Kconfig
@@ -1,5 +1,6 @@
  source Kconfig.host
  source backends/Kconfig
  source accel/Kconfig
+source target/Kconfig
  source hw/Kconfig
  source semihosting/Kconfig
diff --git a/target/Kconfig b/target/Kconfig
new file mode 100644
index 00..a6f719f223
--- /dev/null
+++ b/target/Kconfig
@@ -0,0 +1,23 @@
+source alpha/Kconfig
+source arm/Kconfig
+source avr/Kconfig
+source cris/Kconfig
+source hppa/Kconfig
+source i386/Kconfig
+source lm32/Kconfig
+source m68k/Kconfig
+source microblaze/Kconfig
+source mips/Kconfig
+source moxie/Kconfig
+source nios2/Kconfig
+source openrisc/Kconfig
+source ppc/Kconfig
+source riscv/Kconfig
+source rx/Kconfig
+source s390x/Kconfig
+source sh4/Kconfig
+source sparc/Kconfig
+source tilegx/Kconfig
+source tricore/Kconfig
+source unicore32/Kconfig
+source xtensa/Kconfig
diff --git a/target/alpha/Kconfig b/target/alpha/Kconfig
new file mode 100644
index 00..267222c05b
--- /dev/null
+++ b/target/alpha/Kconfig
@@ -0,0 +1,2 @@
+config ALPHA
+bool


But not these.  I guess the whole Kconfig thing is processed unconditionally, 
and there must be some definition?



r~

Re: [PATCH v16 94/99] target/arm: move CONFIG_V7M out of default-devices

[Richard Henderson]

On 6/4/21 8:53 AM, Alex Bennée wrote:

  config ARM_V7M
  bool
+# currently v7M must be included in a TCG build due to translate.c
+default y if TCG && (ARM || AARCH64)
  select PTIMER
+select ARM_COMPATIBLE_SEMIHOSTING


I don't really understand "default", because if one were to set ARM_V7M=n in 
the config, things wouldn't work.  But I guess the comment sort-of covers that.


I'll give you an
Acked-by: Richard Henderson 

anyway, because it does seem an improvement for the --disable-tcg case.


r~

Re: [PATCH v16 95/99] hw/arm: add dependency on OR_IRQ for XLNX_VERSAL

[Richard Henderson]

On 6/4/21 8:53 AM, Alex Bennée wrote:

We need this functionality due to:

 /* XRAM IRQs get ORed into a single line.  */
 object_initialize_child(OBJECT(s), "xram-irq-orgate",
 &s->lpd.xram.irq_orgate, TYPE_OR_IRQ);

Signed-off-by: Alex Bennée
---
  hw/arm/Kconfig | 1 +
  1 file changed, 1 insertion(+)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v16 96/99] tests/qtest: split the cdrom-test into arm/aarch64

[Richard Henderson]

On 6/4/21 8:53 AM, Alex Bennée wrote:

The assumption that the qemu-system-aarch64 image can run all 32 bit
machines is about to be broken...


Um, what?


r~



 and besides it's not likely this is

improving out coverage by much. Test the "virt" machine for both arm
and aarch64 as it can be used by either architecture.

Signed-off-by: Alex Bennée 
---
  tests/qtest/cdrom-test.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
index 5af944a5fb..1e74354624 100644
--- a/tests/qtest/cdrom-test.c
+++ b/tests/qtest/cdrom-test.c
@@ -220,13 +220,16 @@ int main(int argc, char **argv)
  "magnum", "malta", "pica61", NULL
  };
  add_cdrom_param_tests(mips64machines);
-} else if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) {
+} else if (g_str_equal(arch, "arm")) {
  const char *armmachines[] = {
  "realview-eb", "realview-eb-mpcore", "realview-pb-a8",
  "realview-pbx-a9", "versatileab", "versatilepb", "vexpress-a15",
  "vexpress-a9", "virt", NULL
  };
  add_cdrom_param_tests(armmachines);
+} else if (g_str_equal(arch, "aarch64")) {
+const char *aarch64machines[] = { "virt", NULL };
+add_cdrom_param_tests(aarch64machines);
  } else {
  const char *nonemachine[] = { "none", NULL };
  add_cdrom_param_tests(nonemachine);

Re: [PATCH v16 97/99] tests/qtest: make xlnx-can-test conditional on being configured

[Richard Henderson]

On 6/4/21 8:53 AM, Alex Bennée wrote:

It will soon be possible to build an qemu-system-aarch64 system that
doesn't have this.

Signed-off-by: Alex Bennée
---
  tests/qtest/meson.build | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v16 98/99] configure: allow the overriding of default-config in the build

[Richard Henderson]

On 6/4/21 8:53 AM, Alex Bennée wrote:

+++ b/configs/aarch64-softmmu/64bit-only.mak
@@ -0,0 +1,10 @@
+#
+# A version of the config that only supports 64bits and their devices.
+# This doesn't quite eliminate all 32 bit devices as some boards like
+# "virt" support both.


It's not really 64bit-only then is it?
What is this attempting to show that virt-only (with sbsa) doesn't?



+++ b/configs/aarch64-softmmu/virt-only.mak
@@ -0,0 +1,8 @@
+#
+# A version of the config that only supports virtual machines. This is
+# intended to be combined with options like --disable-tcg for a
+# minimal build supporting only machines we can virtualise with a
+# hypervisor.
+#
+
+CONFIG_ARM_VIRT=y


I would have included SBSA here.  That's a virtual machine as well, just not 
named "virt".  It would also make a useful build configuration, imo, suitable 
for --disable-tcg --enable-kvm.



r~

Re: [PATCH v16 99/99] gitlab: defend the new stripped down arm64 configs

[Richard Henderson]

On 6/4/21 8:53 AM, Alex Bennée wrote:

We can now build a KVM only aarch64-softmmu image which we need to
cross build. We can also build a version that only supports a limited
set of 64 bit images.

Signed-off-by: Alex Bennée 
---
  .gitlab-ci.d/buildtest.yml   | 10 ++
  .gitlab-ci.d/crossbuilds.yml |  9 +
  2 files changed, 19 insertions(+)

diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index b72c57e4df..a48e723efe 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -645,6 +645,16 @@ build-without-default-features:
  
--target-list-exclude=arm-softmmu,i386-softmmu,mipsel-softmmu,mips64-softmmu,ppc-softmmu
  MAKE_CHECK_ARGS: check-unit
  
+build-64bit-only-aarch64-softmmu:

+  extends: .native_build_job_template
+  needs:
+job: amd64-debian-container
+  variables:
+IMAGE: debian-amd64
+TARGETS: aarch64-softmmu
+CONFIGURE_ARGS: 
--with-devices-aarch64=../configs/aarch64-softmmu/64bit-only.mak
+MAKE_CHECK_ARGS: check
+
  build-libvhost-user:
stage: build
image: $CI_REGISTRY_IMAGE/qemu/fedora:latest
diff --git a/.gitlab-ci.d/crossbuilds.yml b/.gitlab-ci.d/crossbuilds.yml
index 6b3865c9e8..a118aa3052 100644
--- a/.gitlab-ci.d/crossbuilds.yml
+++ b/.gitlab-ci.d/crossbuilds.yml
@@ -36,6 +36,15 @@ cross-arm64-system:
variables:
  IMAGE: debian-arm64-cross
  
+cross-arm64-kvm-only-system:

+  extends: .cross_accel_build_job
+  needs:
+job: arm64-debian-cross-container
+  variables:
+IMAGE: debian-arm64-cross
+ACCEL: kvm
+EXTRA_CONFIGURE_OPTS: --disable-tcg
+


Any reason not to merge these two?  And use virt-only, which as I mentioned is 
a useful kvm-only configuration.



r~

Re: [PATCH 1/8] Make qemu-palcode build environment standalone. NFC.

[Richard Henderson]

On 6/2/21 8:53 PM, Jason Thorpe wrote:

Don't include system headers.  Instead, provide standalone definitions
and declarations of types needed and functions used by the PALcode that
are compatible with the standard Alpha / GCC ABI.

Signed-off-by: Jason Thorpe
---
  init.c   |  2 --
  memcpy.c |  2 +-
  memset.c |  2 +-
  printf.c |  4 +---
  protos.h | 30 +-
  5 files changed, 28 insertions(+), 12 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH 2/8] Fix delivery of unaligned access exceptions.

[Richard Henderson]

On 6/2/21 8:53 PM, Jason Thorpe wrote:

In the unaligned access exception vector, actually pass the return PC
in the exception frame.  This is required in order for unaligned access
fixup handlers in the operating system to work.

Signed-off-by: Jason Thorpe
---
  pal.S | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Ouch.  Good catch.
Reviewed-by: Richard Henderson 

r~

Re: [PATCH 3/8] Fix initialization of the hwrpb.hwrpb.cpuid field.

[Richard Henderson]

On 6/2/21 8:53 PM, Jason Thorpe wrote:

Initialize the hwrpb.hwrpb.cpuid field with the primary CPU ID, not
the processor type, as per the architecture specification.  Some
operating systems check and assert this.

Improve a couple of comments.

Signed-off-by: Jason Thorpe
---
  init.c | 29 ++---
  1 file changed, 22 insertions(+), 7 deletions(-)


Reviewed-by: Richard Henderson 



@@ -257,8 +272,8 @@ init_i8259 (void)
   outb(0x04, PORT_PIC1_DATA);  /* ICW3: slave control INTC2 */
   outb(0x01, PORT_PIC1_DATA);  /* ICW4 */
 
-  /* Initialize level triggers.  The CY82C693UB that's on real alpha

- hardware doesn't have this; this is a PIIX extension.  However,
+  /* Initialize level triggers.  The CY82C693UB that's on some real alpha
+ systems controls these differently; we assume a PIIX here.  However,
  QEMU doesn't implement regular level triggers.  */
   outb(0xff, PORT_PIC2_ELCR);
   outb(0xff, PORT_PIC1_ELCR);


I'll split this out to a separate patch.


r~

Re: [PATCH 4/8] Make some PCI macros available to other files. NFC.

[Richard Henderson]

On 6/2/21 8:53 PM, Jason Thorpe wrote:

Move PCI_DEVFN(), PCI_BUS(), PCI_SLOT(), and PCI_FUNC() to pci.h.

Signed-off-by: Jason Thorpe
---
  pci.c | 4 
  pci.h | 5 +
  2 files changed, 5 insertions(+), 4 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH 5/8] Fix incorrect initialization of PCI BARs.

[Richard Henderson]

On 6/2/21 8:53 PM, Jason Thorpe wrote:

- if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK)
+ if ((old & PCI_BASE_ADDRESS_SPACE_IO) == 0 &&


The correct test is

  (old & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY

Bitwise it's the same thing. I'll fix it up while applying.


r~

Re: [PATCH 6/8] Provide interrupt mapping information in PCI config registers.

[Richard Henderson]

On 6/2/21 8:53 PM, Jason Thorpe wrote:

Use system-specific information to program the interrupt line register
with the interrupt mappings, which is what the SRM console does on real
hardware; some operating systems (e.g. NetBSD) use this information
rather than having interrupt mappings tables for every possible system
variation.

Signed-off-by: Jason Thorpe 


Thanks.


+  /* Map the interrupt and program the IRQ into the line register.
+ Some operating systems rely on the Console providing this information
+ in order to avoid having mapping tables for every possible system
+ variation.  */
+
+  const uint8_t pin = pci_config_readb(bdf, PCI_INTERRUPT_PIN);
+  const uint8_t slot = PCI_SLOT(bdf);
+  const int irq = MAP_PCI_INTERRUPT(slot, pin, class_id);
+
+  if (irq == -1)
+{
+  /* No interrupt mapping.  */
+  pci_config_writeb(bdf, PCI_INTERRUPT_LINE, 0xff);
+}
+  else
+{
+  pci_config_writeb(bdf, PCI_INTERRUPT_LINE, irq);
+}


I've folded this non-distinction into the functional interface.


+#define MAP_PCI_INTERRUPT(SLOT, PIN, CLASS_ID) \


I've turned this into a static inline.


r~