Re: Timeouts in CI jobs

[Stefan Weil via]

Am 24.04.24 um 19:09 schrieb Daniel P. Berrangé:


On Wed, Apr 24, 2024 at 06:27:58PM +0200, Stefan Weil via wrote:

I think the timeouts are caused by running too many parallel processes
during testing.

The CI uses parallel builds:

make -j$(expr $(nproc) + 1) all check-build $MAKE_CHECK_ARGS

Note that command is running both the compile and test phases of
the job. Overcommitting CPUs for the compile phase is a good
idea to keep CPUs busy while another process is waiting on
I/O, and is almost always safe todo.



Thank you for your answer.

Overcommitting for the build is safe, but in my experience the positive 
effect is typically very small on modern hosts with fast disk I/O and 
large buffer caches.


And there is also a negative impact because this requires scheduling 
with process switches.


Therefore I am not so sure that overcommitting is a good idea, 
especially not on cloud servers where the jobs are running in VMs.



Overcommitting CPUs for the test phase is less helpful and
can cause a variety of problems as you say.


It looks like `nproc` returns 8, and make runs with 9 threads.
`meson test` uses the same value to run 9 test processes in parallel:

/builds/qemu-project/qemu/build/pyvenv/bin/meson test  --no-rebuild -t 1
--num-processes 9 --print-errorlogs

Since the host can only handle 8 parallel threads, 9 threads might already
cause some tests to run non-deterministically.

In contributor forks, gitlab CI will be using the public shared
runners. These are Google Cloud VMs, which only have 2 vCPUs.

In the primary QEMU repo, we have a customer runner registered
that uses Azure based VMs. Not sure on the resources we have
configured for them offhand.


I was talking about the primary QEMU.


The important thing there is that what you see for CI speed in
your fork repo is not neccessarily a match for CI speed in QEMU
upstream repo.


I did not run tests in my GitLab fork because I still have to figure out 
how to do that.


In my initial answer to Peter's mail I had described my tests and the 
test environment in detail.


My test environment was an older (= slow) VM with 4 cores. I tested with 
different values for --num-processes. As expected higher values raised 
the number of timeouts. And the most interesting result was that 
`--num-processes 1` avoided timeouts, used less CPU time and did not 
increase the duration.



In my tests setting --num-processes to a lower value not only avoided
timeouts but also reduced the processing overhead without increasing the
runtime.

Could we run all tests with `--num-processes 1`?

The question is what impact that has on the overall job execution
time. A lot of our jobs are already quite long, which is bad for
the turnaround time of CI testing.  Reliable CI though is arguably
the #1 priority though, otherwise developers cease trusting it.
We need to find the balance between avoiding timeouts, while having
the shortest practical job time.  The TCI job you show about came
out at 22 minutes, which is not our worst job, so there is some
scope for allowing it to run longer with less parallelism.


The TCI job terminates after less than 7 minutes in my test runs with 
less parallelism.


Obviously there are tests which already do their own multithreading, and 
maybe other tests run single threaded. So maybe we need different values 
for `--num-processes` depending on the number of threads which the 
single tests use?


Regards,

Stefan

Timeouts in CI jobs (was: cross-i686-tci CI job is flaky again (timeouts): can somebody who cares about TCI investigate?)

[Stefan Weil via]

Am 20.04.24 um 22:25 schrieb Stefan Weil:

Am 16.04.24 um 14:17 schrieb Stefan Weil:

Am 16.04.24 um 14:10 schrieb Peter Maydell:


The cross-i686-tci job is flaky again, with persistent intermittent
failures due to jobs timing out.

[...]

Some of these timeouts are very high -- no test should be taking
10 minutes, even given TCI and a slowish CI runner -- which suggests
to me that there's some kind of intermittent deadlock going on.

Can somebody who cares about TCI investigate, please, and track
down whatever this is?


I'll have a look.


Short summary:

The "persistent intermittent failures due to jobs timing out" are not 
related to TCI: they also occur if the same tests are run with the 
normal TCG. I suggest that the CI tests should run single threaded.


Hi Paolo,

I need help from someone who knows the CI and the build and test 
framework better.


Peter reported intermittent timeouts for the cross-i686-tci job, causing 
it to fail. I can reproduce such timeouts locally, but noticed that they 
are not limited to TCI. The GitLab CI also shows other examples, such as 
this job:


https://gitlab.com/qemu-project/qemu/-/jobs/6700955287

I think the timeouts are caused by running too many parallel processes 
during testing.


The CI uses parallel builds:

make -j$(expr $(nproc) + 1) all check-build $MAKE_CHECK_ARGS

It looks like `nproc` returns 8, and make runs with 9 threads.
`meson test` uses the same value to run 9 test processes in parallel:

/builds/qemu-project/qemu/build/pyvenv/bin/meson test  --no-rebuild -t 1 
 --num-processes 9 --print-errorlogs


Since the host can only handle 8 parallel threads, 9 threads might 
already cause some tests to run non-deterministically.


But if some of the individual tests also use multithreading (according 
to my tests they do so with at least 3 or 4 threads), things get even 
worse. Then there are up to 4 * 9 = 36 threads competing to run on the 
available 8 cores.


In this scenario timeouts are expected and can occur randomly.

In my tests setting --num-processes to a lower value not only avoided 
timeouts but also reduced the processing overhead without increasing the 
runtime.


Could we run all tests with `--num-processes 1`?

Thanks,
Stefan

Re: cross-i686-tci CI job is flaky again (timeouts): can somebody who cares about TCI investigate?

[Stefan Weil via]

Am 16.04.24 um 14:17 schrieb Stefan Weil:

Am 16.04.24 um 14:10 schrieb Peter Maydell:


The cross-i686-tci job is flaky again, with persistent intermittent
failures due to jobs timing out.

[...]

Some of these timeouts are very high -- no test should be taking
10 minutes, even given TCI and a slowish CI runner -- which suggests
to me that there's some kind of intermittent deadlock going on.

Can somebody who cares about TCI investigate, please, and track
down whatever this is?


I'll have a look.


Short summary:

The "persistent intermittent failures due to jobs timing out" are not 
related to TCI: they also occur if the same tests are run with the 
normal TCG. I suggest that the CI tests should run single threaded.


But let's have a look on details of my results.

I have run `time make test` using different scenarios on the rather old 
and not so performant VM which I typically use for QEMU builds. I did 
not restrict the tests to selected architectures like it is done in the 
QEMU CI tests. Therefore I had more tests which all ended successfully:


Ok: 848
Expected Fail:  0
Fail:   0
Unexpected Pass:0
Skipped:68
Timeout:0

---

1st test with normal TCG

`nohup time ../configure --enable-modules --disable-werror && nohup time 
make -j4 && nohup time make test`

[...]
Full log written to 
/home/stefan/src/gitlab/qemu-project/qemu/bin/ndebug/x86_64-linux-gnu/meson-logs/testlog.txt
2296.08user 1525.02system 21:49.78elapsed 291%CPU (0avgtext+0avgdata 
633476maxresident)k

1730448inputs+14140528outputs (11668major+56827263minor)pagefaults 0swaps

---

2nd test with TCI

`nohup time ../configure --enable-modules --disable-werror 
--enable-tcg-interpreter && nohup time make -j4 && nohup time make test`

[...]
Full log written to 
/home/stefan/src/gitlab/qemu-project/qemu/bin/ndebug/x86_64-linux-gnu/meson-logs/testlog.txt
3766.74user 1521.38system 26:50.51elapsed 328%CPU (0avgtext+0avgdata 
633012maxresident)k

32768inputs+14145080outputs (3033major+56121586minor)pagefaults 0swaps

---

So the total test time with TCI was 26:50.51 minutes while for the 
normal test it was 21:49.78 minutes.


These 10 single tests had the longest duration:

1st test with normal TCG

  94/916 qtest-arm / qtest-arm/qom-test 373.41s
  99/916 qtest-aarch64 / qtest-aarch64/qom-test 398.43s
100/916 qtest-i386 / qtest-i386/bios-tables-test   188.06s
103/916 qtest-x86_64 / qtest-x86_64/bios-tables-test   228.33s
106/916 qtest-aarch64 / qtest-aarch64/migration-test   201.15s
119/916 qtest-i386 / qtest-i386/migration-test 253.58s
126/916 qtest-x86_64 / qtest-x86_64/migration-test 266.66s
143/916 qtest-arm / qtest-arm/test-hmp 101.72s
144/916 qtest-aarch64 / qtest-aarch64/test-hmp 113.10s
163/916 qtest-arm / qtest-arm/aspeed_smc-test  256.92s

2nd test with TCI

  68/916 qtest-arm / qtest-arm/qom-test 375.35s
  82/916 qtest-aarch64 / qtest-aarch64/qom-test 403.50s
  93/916 qtest-i386 / qtest-i386/bios-tables-test   192.22s
  99/916 qtest-aarch64 / qtest-aarch64/bios-tables-test 379.92s
100/916 qtest-x86_64 / qtest-x86_64/bios-tables-test   240.19s
103/916 qtest-aarch64 / qtest-aarch64/migration-test   223.49s
106/916 qtest-ppc64 / qtest-ppc64/pxe-test 418.42s
113/916 qtest-i386 / qtest-i386/migration-test 284.96s
118/916 qtest-arm / qtest-arm/aspeed_smc-test  271.10s
119/916 qtest-x86_64 / qtest-x86_64/migration-test 287.36s

---

Summary:

TCI is not much slower than the normal TCG. Surprisingly it was even 
faster for the tests 99 and 103. For other tests like test 106 TCI was 
about half as fast as normal TCG, but in summary it is not "factors" 
slower. A total test time of 26:50 minutes is also not so bad compared 
with the 21:49 minutes of the normal TCG.


No single test (including subtests) with TCI exceeded 10 minutes, the 
longest one was well below that margin with 418 seconds.


---

The tests above were running with x86_64, and I could not reproduce 
timeouts. The Gitlab CI tests were running with i686 and used different 
configure options. Therefore I made additional tests with 32 bit builds 
in a chroot environment (Debian GNU Linux bullseye i386) with the 
original configure options. As expected that reduced the number of tests 
to 250. All tests passed with `make test`:


3rd test with normal TCG

Ok: 250
Expected Fail:  0
Fail:   0
Unexpected Pass:0
Skipped:8
Timeout:0

Full log written to 
/root/qemu/bin/ndebug/i586-linux-gnu/meson-logs/testlog.txt
855.30user 450.53system 6:45.57elapsed 321%CPU (0avgtext+0avgdata 
609180maxresident)k

28232inputs+4772968outputs (64944major+8328814minor)pagefaults 0swaps


4th test with TCI

Ok: 250
Expected Fail:  0
Fail:   0
Unexpected Pass:0
S

Re: cross-i686-tci CI job is flaky again (timeouts): can somebody who cares about TCI investigate?

[Stefan Weil via]

Am 16.04.24 um 14:10 schrieb Peter Maydell:


The cross-i686-tci job is flaky again, with persistent intermittent
failures due to jobs timing out.

[...]

Some of these timeouts are very high -- no test should be taking
10 minutes, even given TCI and a slowish CI runner -- which suggests
to me that there's some kind of intermittent deadlock going on.

Can somebody who cares about TCI investigate, please, and track
down whatever this is?



I'll have a look.

Regards

Stefan

[PATCH for-9.0] Fix some typos in documentation (found by codespell)

[Stefan Weil via]

Signed-off-by: Stefan Weil 
---
 docs/devel/atomics.rst | 2 +-
 docs/devel/ci-jobs.rst.inc | 2 +-
 docs/devel/clocks.rst  | 2 +-
 docs/system/i386/sgx.rst   | 2 +-
 qapi/qom.json  | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/docs/devel/atomics.rst b/docs/devel/atomics.rst
index ff9b5ee30c..b77c6e13e1 100644
--- a/docs/devel/atomics.rst
+++ b/docs/devel/atomics.rst
@@ -119,7 +119,7 @@ The only guarantees that you can rely upon in this case are:
   ordinary accesses instead cause data races if they are concurrent with
   other accesses of which at least one is a write.  In order to ensure this,
   the compiler will not optimize accesses out of existence, create unsolicited
-  accesses, or perform other similar optimzations.
+  accesses, or perform other similar optimizations.
 
 - acquire operations will appear to happen, with respect to the other
   components of the system, before all the LOAD or STORE operations
diff --git a/docs/devel/ci-jobs.rst.inc b/docs/devel/ci-jobs.rst.inc
index ec33e6ee2b..be06322279 100644
--- a/docs/devel/ci-jobs.rst.inc
+++ b/docs/devel/ci-jobs.rst.inc
@@ -115,7 +115,7 @@ CI pipeline.
 QEMU_JOB_SKIPPED
 
 
-The job is not reliably successsful in general, so is not
+The job is not reliably successful in general, so is not
 currently suitable to be run by default. Ideally this should
 be a temporary marker until the problems can be addressed, or
 the job permanently removed.
diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
index b2d1148cdb..177ee1c90d 100644
--- a/docs/devel/clocks.rst
+++ b/docs/devel/clocks.rst
@@ -279,7 +279,7 @@ You can change the multiplier and divider of a clock at 
runtime,
 so you can use this to model clock controller devices which
 have guest-programmable frequency multipliers or dividers.
 
-Similary to ``clock_set()``, ``clock_set_mul_div()`` returns ``true`` if
+Similarly to ``clock_set()``, ``clock_set_mul_div()`` returns ``true`` if
 the clock state was modified; that is, if the multiplier or the diviser
 or both were changed by the call.
 
diff --git a/docs/system/i386/sgx.rst b/docs/system/i386/sgx.rst
index 0f0a73f758..c293f7f44e 100644
--- a/docs/system/i386/sgx.rst
+++ b/docs/system/i386/sgx.rst
@@ -6,7 +6,7 @@ Overview
 
 Intel Software Guard eXtensions (SGX) is a set of instructions and mechanisms
 for memory accesses in order to provide security accesses for sensitive
-applications and data. SGX allows an application to use it's pariticular
+applications and data. SGX allows an application to use its particular
 address space as an *enclave*, which is a protected area provides 
confidentiality
 and integrity even in the presence of privileged malware. Accesses to the
 enclave memory area from any software not resident in the enclave are 
prevented,
diff --git a/qapi/qom.json b/qapi/qom.json
index 8d4ca8ed92..85e6b4f84a 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -802,7 +802,7 @@
 #
 # @fd: file descriptor name previously passed via 'getfd' command,
 # which represents a pre-opened /dev/iommu.  This allows the
-# iommufd object to be shared accross several subsystems (VFIO,
+# iommufd object to be shared across several subsystems (VFIO,
 # VDPA, ...), and the file descriptor to be shared with other
 # process, e.g. DPDK.  (default: QEMU opens /dev/iommu by itself)
 #
-- 
2.39.2

Re: [PATCH 3/7] scripts/nsis.py: Automatically package required DLLs of QEMU executables

[Stefan Weil via]

Am 26.02.24 um 05:35 schrieb Bin Meng:


On Mon, Feb 26, 2024 at 1:37 AM Stefan Weil  wrote:

Am 10.09.22 um 02:37 schrieb Bin Meng:

On Sat, Sep 10, 2022 at 12:49 AM Mark Cave-Ayland
  wrote:

On 08/09/2022 14:28, Bin Meng wrote:


From: Bin Meng

At present packaging the required DLLs of QEMU executables is a
manual process, and error prone.

Actually build/config-host.mak contains a GLIB_BINDIR variable
which is the directory where glib and other DLLs reside. This
works for both Windows native build and cross-build on Linux.
We can use it as the search directory for DLLs and automate
the whole DLL packaging process.

Signed-off-by: Bin Meng
---

meson.build |  1 +
scripts/nsis.py | 46 ++
2 files changed, 43 insertions(+), 4 deletions(-)


[...]>>> diff --git a/scripts/nsis.py b/scripts/nsis.py

index baa6ef9594..03ed7608a2 100644
--- a/scripts/nsis.py
+++ b/scripts/nsis.py
@@ -18,12 +18,36 @@ def signcode(path):
return
subprocess.run([cmd, path])

+def find_deps(exe_or_dll, search_path, analyzed_deps):
+deps = [exe_or_dll]
+output = subprocess.check_output(["objdump", "-p", exe_or_dll], text=True)

This fails on non x86 hosts where objdump does not know how to handle a
Windows x86_64 exe file.

Does this command work in the MSYS2 environment on Windows Arm?



I don't know and cannot test that, because I don't run Windows on ARM.


+output = output.split("\n")
+for line in output:
+if not line.startswith("\tDLL Name: "):
+continue
+
+dep = line.split("DLL Name: ")[1].strip()
+if dep in analyzed_deps:
+continue
+
+dll = os.path.join(search_path, dep)
+if not os.path.exists(dll):
+# assume it's a Windows provided dll, skip it
+continue
+
+analyzed_deps.add(dep)
+# locate the dll dependencies recursively
+rdeps = find_deps(dll, search_path, analyzed_deps)
+deps.extend(rdeps)
+
+return deps

[...]

FWIW I wrote a similar script a while back to help package a custom Windows 
build for
a client, however I used ldd instead of objdump since it provided the full 
paths for
DLLs installed in the msys2/mingw-w64 environment via pacman which were outside 
the
QEMU build tree.


Yep, ldd also works, but only on Windows native build. objdump can
work on both Windows native and Linux cross builds.


objdump fails on Linux cross builds on any non x86 host, because objdump
typically only supports the native host architecture.

Therefore I get an error on an ARM64 host (podman on Mac M1):

  objdump: /tmp/tmpvae5u0qm/qemu/qemu-system-aarch64.exe: file
format not recognized

I could use x86_64-w64-mingw32-objdump to fix the cross builds, but then
native builds might fail because they don't have x86_64-w64-mingw32-objdump.

Is there a simple way how we can get the information here whether
objdump requires a cross build prefix?

For QEMU Windows, I believe the only supported architecture is x86_64,
correct? Do we want to support (cross) building QEMU for Windows Arm?



Yes, I think we only support QEMU on Windows x86_64. I also don't know 
anyone who has tried building for Windows ARM. And up to now I also was 
never asked for that, so obviously there is still no need for it.




Based on your observation, objdump on Linux cross builds on any x86
host works, but not on non x86 host.
Maybe it's a hint to ask for binutils guys to include the PE format
support for objdump Arm by default.



I am afraid that we'll have to find a solution on the QEMU side, not 
wait until all binutils support the PE format for x86_64 (which would 
make the binaries or the library much larger).


Stefan

Re: [PATCH 3/7] scripts/nsis.py: Automatically package required DLLs of QEMU executables

[Stefan Weil via]

Am 10.09.22 um 02:37 schrieb Bin Meng:

On Sat, Sep 10, 2022 at 12:49 AM Mark Cave-Ayland
 wrote:


On 08/09/2022 14:28, Bin Meng wrote:


From: Bin Meng 

At present packaging the required DLLs of QEMU executables is a
manual process, and error prone.

Actually build/config-host.mak contains a GLIB_BINDIR variable
which is the directory where glib and other DLLs reside. This
works for both Windows native build and cross-build on Linux.
We can use it as the search directory for DLLs and automate
the whole DLL packaging process.

Signed-off-by: Bin Meng 
---

   meson.build |  1 +
   scripts/nsis.py | 46 ++
   2 files changed, 43 insertions(+), 4 deletions(-)


[...]>>> diff --git a/scripts/nsis.py b/scripts/nsis.py

index baa6ef9594..03ed7608a2 100644
--- a/scripts/nsis.py
+++ b/scripts/nsis.py
@@ -18,12 +18,36 @@ def signcode(path):
   return
   subprocess.run([cmd, path])

+def find_deps(exe_or_dll, search_path, analyzed_deps):
+deps = [exe_or_dll]
+output = subprocess.check_output(["objdump", "-p", exe_or_dll], text=True)


This fails on non x86 hosts where objdump does not know how to handle a 
Windows x86_64 exe file.



+output = output.split("\n")
+for line in output:
+if not line.startswith("\tDLL Name: "):
+continue
+
+dep = line.split("DLL Name: ")[1].strip()
+if dep in analyzed_deps:
+continue
+
+dll = os.path.join(search_path, dep)
+if not os.path.exists(dll):
+# assume it's a Windows provided dll, skip it
+continue
+
+analyzed_deps.add(dep)
+# locate the dll dependencies recursively
+rdeps = find_deps(dll, search_path, analyzed_deps)
+deps.extend(rdeps)
+
+return deps

[...]


FWIW I wrote a similar script a while back to help package a custom Windows 
build for
a client, however I used ldd instead of objdump since it provided the full 
paths for
DLLs installed in the msys2/mingw-w64 environment via pacman which were outside 
the
QEMU build tree.



Yep, ldd also works, but only on Windows native build. objdump can
work on both Windows native and Linux cross builds.



objdump fails on Linux cross builds on any non x86 host, because objdump 
typically only supports the native host architecture.


Therefore I get an error on an ARM64 host (podman on Mac M1):

objdump: /tmp/tmpvae5u0qm/qemu/qemu-system-aarch64.exe: file 
format not recognized


I could use x86_64-w64-mingw32-objdump to fix the cross builds, but then 
native builds might fail because they don't have x86_64-w64-mingw32-objdump.


Is there a simple way how we can get the information here whether 
objdump requires a cross build prefix?


Stefan

Files without license statement

[Stefan Weil via]

Hi Paolo,

I just noticed that scripts/fix-multiline-comments.sh has a copyright 
statement, but no license statement. Should that be fixed?


It looks like there exist more files with the same problem (if it is a 
problem), for example docs/devel/loads-stores.rst (written by Peter).


LICENSE says that "Source files with no licensing information are 
released under the GNU General Public License [...]". Does that include 
shell and Python scripts, documentation and other files which might not 
be regarded as "source files"? Or should that be updated to "Any files 
with no licensing information [...]" which would also include files 
which have neither a copyright nor a license statement?


Kind regards
Stefan W.

[Bug 2054889] Re: pcap streams are text files which insert 0xD in Windows version

[Stefan Weil]

** Changed in: qemu
   Status: New => Confirmed

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/2054889

Title:
  pcap streams are text files which insert 0xD in Windows version

Status in QEMU:
  Confirmed

Bug description:
  Since Windows text files use CRLFs for all \n, the Windows version of
  QEMU inserts a CR in the PCAP stream when a LF is encountered when
  using USB PCAP files.

  Starting at line 275 in hw/usb/bus (https://gitlab.com/qemu-
  project/qemu/-/blob/master/hw/usb/bus.c?ref_type=heads#L275), the file
  is opened as text instead of binary.

  I think the following patch would fix the issue:
  if (dev->pcap_filename) {
  -   int fd = qemu_open_old(dev->pcap_filename, O_CREAT | O_WRONLY | 
O_TRUNC, 0666);
  +   int fd = qemu_open_old(dev->pcap_filename, O_CREAT | O_WRONLY | 
O_TRUNC | O_BINARY, 0666);
  if (fd < 0) {
  error_setg(errp, "open %s failed", dev->pcap_filename);
  usb_qdev_unrealize(qdev);
  return;
  }
  -   dev->pcap = fdopen(fd, "w");
  +   dev->pcap = fdopen(fd, "wb");
  usb_pcap_init(dev->pcap);
  }

  To show an example, when using a very common protocol to USB disks,
  the BBB protocol uses a 10-byte command packet. For example, the
  READ_CAPACITY(10) command (implemented at https://gitlab.com/qemu-
  project/qemu/-/blob/master/hw/scsi/scsi-disk.c#L2068) will have a
  command block length of 10 (0xA). When this 10-byte command (part of
  the 31-byte CBW) is placed into the PCAP file, the Windows file
  manager inserts a 0xD before the 0xA, turning the 31-byte CBW into a
  32-byte CBW.

  Actual CBW:
0040   55 53 42 43 01 00 00 00 08 00 00 00 80 00 0a 25   USBC
0050   00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  %..

  PCAP CBW
0040   55 53 42 43 01 00 00 00 08 00 00 00 80 00 0d 0a   USBC
0050   25 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00   %..

  I believe simply opening the PCAP file as BINARY instead of TEXT will
  fix this issue.

  Thank you.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/2054889/+subscriptions

Re: dropping 32-bit Windows host support

[Stefan Weil via]

Am 19.02.24 um 17:26 schrieb Thomas Huth:


On 19/02/2024 16.53, Daniel P. Berrangé wrote:

On Mon, Feb 19, 2024 at 03:37:31PM +, Peter Maydell wrote:

Our msys2 32-bit Windows host CI job has been failing recently
because upstream MSYS2 are starting to phase out 32-bit windows
host support and are steadily removing i686 versions of packages.
The latest is dtc:
https://gitlab.com/qemu-project/qemu/-/issues/2177
[...]



I agree with all your comments and also think that support for 32-bit 
Windows hosts can be dropped.


As Peter noted, I have been building 64-bit installers only since QEMU 
8.0.0. I have not received any complaints about this.


Best regards

Stefan

GPL 3.0 in TCG test code

[Stefan Weil via]

Dear all,

some QEMU code under tests/tcg uses GPL 3.0 or later:

tests/tcg/aarch64/semicall.h: * SPDX-License-Identifier: GPL-3.0-or-later
tests/tcg/arm/semicall.h: * SPDX-License-Identifier: GPL-3.0-or-later
tests/tcg/i386/system/boot.S: * SPDX-License-Identifier: GPL-3.0-or-later
tests/tcg/multiarch/arm-compat-semi/semiconsole.c: * 
SPDX-License-Identifier: GPL-3.0-or-later
tests/tcg/multiarch/arm-compat-semi/semihosting.c: * 
SPDX-License-Identifier: GPL-3.0-or-later
tests/tcg/multiarch/float_convd.c: * SPDX-License-Identifier: 
GPL-3.0-or-later
tests/tcg/multiarch/float_convs.c: * SPDX-License-Identifier: 
GPL-3.0-or-later
tests/tcg/multiarch/float_helpers.h: * SPDX-License-Identifier: 
GPL-3.0-or-later
tests/tcg/multiarch/float_madds.c: * SPDX-License-Identifier: 
GPL-3.0-or-later
tests/tcg/multiarch/libs/float_helpers.c: * SPDX-License-Identifier: 
GPL-3.0-or-later

tests/tcg/riscv64/semicall.h: * SPDX-License-Identifier: GPL-3.0-or-later
tests/tcg/x86_64/system/boot.S: * SPDX-License-Identifier: GPL-3.0-or-later

I don't think that there is a conflict with the QEMU license (GPL 2.0 or 
later) because that code is only used in tests.


But maybe it should be mentioned in LICENSE?

Regards,
Stefan

Re: [PATCH v2 1/4] util/uri: Remove uri_string_unescape()

[Stefan Weil via]

Am 23.01.24 um 19:22 schrieb Thomas Huth:


uri_string_unescape() basically does the same as the glib function
g_uri_unescape_segment(). So we can get rid of our implementation
completely by simply using the glib function instead.

Suggested-by: Stefan Weil  [g_uri_unescape_string()]
Suggested-by: Paolo Bonzini  [g_uri_unescape_segment()]
Signed-off-by: Thomas Huth 
---
  include/qemu/uri.h |  1 -
  util/uri.c | 97 ++
  2 files changed, 11 insertions(+), 87 deletions(-)



Reviewed-by: Stefan Weil

Thank you, Thomas and Paolo.

Re: [PATCH 2/5] util/uri: Simplify uri_string_unescape()

[Stefan Weil via]

Am 22.01.24 um 20:17 schrieb Thomas Huth:


uri_string_unescape() basically does the same as the glib function
g_uri_unescape_string(), with just an additional length parameter.
So we can simplify this function a lot by limiting the length with
g_strndup() first and then by calling g_uri_unescape_string() instead
of walking through the string manually.

Suggested-by: Stefan Weil


Can my e-mail address be replaced by another one (s...@weilnetz.de)?


Signed-off-by: Thomas Huth
---
  util/uri.c | 49 +++--
  1 file changed, 3 insertions(+), 46 deletions(-)

diff --git a/util/uri.c b/util/uri.c
index 33b6c7214e..2a75f535ba 100644
--- a/util/uri.c
+++ b/util/uri.c
@@ -1561,15 +1561,6 @@ done_cd:
  return 0;
  }
  
-static int is_hex(char c)

-{
-if (((c >= '0') && (c <= '9')) || ((c >= 'a') && (c <= 'f')) ||
-((c >= 'A') && (c <= 'F'))) {
-return 1;
-}
-return 0;
-}
-
  /**
   * uri_string_unescape:
   * @str:  the string to unescape
@@ -1585,8 +1576,7 @@ static int is_hex(char c)
   */
  char *uri_string_unescape(const char *str, int len)
  {
-char *ret, *out;
-const char *in;
+g_autofree char *lstr = NULL;



Is it necessary to assign NULL? It does not look so.


  
  if (str == NULL) {

  return NULL;
@@ -1594,42 +1584,9 @@ char *uri_string_unescape(const char *str, int len)
  if (len <= 0) {
  len = strlen(str);
  }
-if (len < 0) {
-return NULL;
-}
-
-ret = g_malloc(len + 1);
+lstr = g_strndup(str, len);
  
-in = str;

-out = ret;
-while (len > 0) {
-if ((len > 2) && (*in == '%') && (is_hex(in[1])) && (is_hex(in[2]))) {
-in++;
-if ((*in >= '0') && (*in <= '9')) {
-*out = (*in - '0');
-} else if ((*in >= 'a') && (*in <= 'f')) {
-*out = (*in - 'a') + 10;
-} else if ((*in >= 'A') && (*in <= 'F')) {
-*out = (*in - 'A') + 10;
-}
-in++;
-if ((*in >= '0') && (*in <= '9')) {
-*out = *out * 16 + (*in - '0');
-} else if ((*in >= 'a') && (*in <= 'f')) {
-*out = *out * 16 + (*in - 'a') + 10;
-} else if ((*in >= 'A') && (*in <= 'F')) {
-*out = *out * 16 + (*in - 'A') + 10;
-}
-in++;
-len -= 3;
-out++;
-} else {
-*out++ = *in++;
-    len--;
-}
-}
-*out = 0;
-return ret;
+return g_uri_unescape_string(lstr, NULL);
  }
  
  /**



Thank you.

Reviewed-by: Stefan Weil 

Re: [PATCH 5/5] util/uri: Remove unused macros ISA_RESERVED() and ISA_GEN_DELIM()

[Stefan Weil via]

Am 22.01.24 um 20:17 schrieb Thomas Huth:


They are not used anywhere, so there's no need to keep them around.

Signed-off-by: Thomas Huth 
---
  util/uri.c | 13 -
  1 file changed, 13 deletions(-)



Reviewed-by: Stefan Weil 

Re: [PATCH 4/5] util/uri: Remove unused functions uri_resolve() and uri_resolve_relative()

[Stefan Weil via]

Am 22.01.24 um 20:17 schrieb Thomas Huth:


These rather complex functions have never been used since they've been
introduced in 2012, so looks like they are not really useful for QEMU.
And since the static normalize_uri_path() function is also only used by
uri_resolve(), we can remove that function now, too.

Signed-off-by: Thomas Huth 
---
  include/qemu/uri.h |   2 -
  util/uri.c | 689 -
  2 files changed, 691 deletions(-)



This patch should be applied before patch 3 (so switch the order of the 
patches 3 and 4). With this change:


Reviewed-by: Stefan Weil 

Re: [PATCH 3/5] util/uri: Remove the uri_string_escape() function

[Stefan Weil via]

Am 22.01.24 um 20:17 schrieb Thomas Huth:


It is not used in QEMU - and if somebody needs this functionality,
they can simply use g_uri_escape_string() from the glib instead.

Signed-off-by: Thomas Huth 
---
  include/qemu/uri.h |  1 -
  util/uri.c | 64 --
  2 files changed, 65 deletions(-)



The removed function is used in util/uri.c, so this patch breaks the 
compilation.


That can be fixed by applying patch 4 before this one.

With that re-ordering you may add my signature:

Reviewed-by: Stefan Weil 

Re: [PATCH 1/5] util/uri: Remove the unused "target" argument from uri_string_unescape()

[Stefan Weil via]

Am 22.01.24 um 20:17 schrieb Thomas Huth:


All callers pass NULL as target, so we can simplify the code by
dropping this parameter.

Signed-off-by: Thomas Huth 
---
  include/qemu/uri.h |  2 +-
  util/uri.c | 32 ++--
  2 files changed, 15 insertions(+), 19 deletions(-)



Reviewed-by: Stefan Weil 

Re: [PATCH] util/uri: Remove is_hex() function

[Stefan Weil via]

Am 12.01.24 um 07:35 schrieb Markus Armbruster:

Thomas Huth  writes:


We can simply use the g_ascii_isxdigit() from the glib instead.


... or even use unescape_string() from the glib?

https://docs.gtk.org/glib/type_func.Uri.unescape_string.html

Regards,
Stefan

Re: [PATCH] mailmap: Fix Stefan Weil author email again

[Stefan Weil via]

Am 27.12.23 um 10:12 schrieb Philippe Mathieu-Daudé:


On 27/12/23 10:09, Michael Tokarev wrote:

27.12.2023 11:59, Philippe Mathieu-Daudé:

Commit 5204b499a6 ("mailmap: Fix Stefan Weil author email")
corrected authorship for patch received at qemu-devel@nongnu.org,
correct now for patch received at qemu-triv...@nongnu.org.

Fixes: d819fc9516 ("virtio-blk: Fix potential nullptr read access")


Do you think a single commit warrants an entry in mailmap?


If I cared enough to write and post a patch, I suppose so...

In the past the only limitation was whether someone was willing
to do the work and send a patch, not the size of the .mailmap
file.


Thank you Philippe and Michael.

It looks like I have some more identities. :-)

   1 Author: Stefan Weil 
   1 Author: Stefan Weil 
 697 Author: Stefan Weil 
 361 Author: Stefan Weil 
   1 Author: Stefan Weil via 

My very old address w...@mail.berlios.de might be more interesting for 
mailmap.


Regards,

Stefan

Re: [PATCH] target/hexagon/idef-parser/prepare: use env to invoke bash

[Stefan Weil via]

Am 25.12.23 um 09:05 schrieb Michael Tokarev:

23.11.2023 23:57, Samuel Tardieu :

This file is the only one involved in the compilation process which
still uses the /bin/bash path.

Signed-off-by: Samuel Tardieu 
---
  target/hexagon/idef-parser/prepare | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/hexagon/idef-parser/prepare 
b/target/hexagon/idef-parser/prepare

index 72d6fcbd21..cb3622d4f8 100755
--- a/target/hexagon/idef-parser/prepare
+++ b/target/hexagon/idef-parser/prepare
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/usr/bin/env bash


What's the reason for this indirection?  bash has been /bin/bash for 
decades,
it is used this way in many other places in qemu code and in other 
projects.

Yes I know about current move /bin => /usr/bin etc, but the thing is that
traditional paths like this one (or like /bin/sh) is not going away any 
time

soon.  What's the matter here?

/mjt



On MacOS /bin/bash is the pre-installed bash:

% /bin/bash --version
GNU bash, version 3.2.57(1)-release (arm64-apple-darwin23)
Copyright (C) 2007 Free Software Foundation, Inc.

With /usr/bin/env I get a recent one from Homebrew:

% /usr/bin/env bash --version
GNU bash, version 5.2.21(1)-release (aarch64-apple-darwin23.0.0)
Copyright (C) 2022 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 



This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

So if a bash script uses bash syntax which was added after 2007, that 
would not work with /bin/bash. That's why I had to use the indirection 
for another open source project. I don't know whether the QEMU bash 
scripts require a newer version of bash.


Regards
Stefan

[PATCH] virtio-blk: Fix potential nullpointer read access in virtio_blk_data_plane_destroy

[Stefan Weil via]

Fixes: CID 1532828
Fixes: b6948ab01d ("virtio-blk: add iothread-vq-mapping parameter")
Signed-off-by: Stefan Weil 
---
 hw/block/dataplane/virtio-blk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 6debd4401e..97a302cf49 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -152,7 +152,7 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, 
VirtIOBlkConf *conf,
 void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
 {
 VirtIOBlock *vblk;
-VirtIOBlkConf *conf = s->conf;
+VirtIOBlkConf *conf;
 
 if (!s) {
 return;
@@ -160,6 +160,7 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
 
 vblk = VIRTIO_BLK(s->vdev);
 assert(!vblk->dataplane_started);
+conf = s->conf;
 
 if (conf->iothread_vq_mapping_list) {
 IOThreadVirtQueueMappingList *node;
-- 
2.39.2

[PATCH for-8.2] Fix broken build for QEMU guest agent

[Stefan Weil via]

Meson setup failed:

qga/meson.build:148:4: ERROR: Unknown variable "project".

Fixes commit e20d68aa ("configure, meson: use command line options [...]").

Signed-off-by: Stefan Weil 
---
 qga/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/meson.build b/qga/meson.build
index 940a51d55d..ff7a8496e4 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -146,7 +146,7 @@ if targetos == 'windows'
   libpcre = 'libpcre2'
 endif
 qga_msi_version = get_option('qemu_ga_version') == '' \
-  ? project.version() \
+  ? meson.project_version() \
   : get_option('qemu_ga_version')
 qga_msi = custom_target('QGA MSI',
 input: files('installer/qemu-ga.wxs'),
-- 
2.39.2

Re: [PATCH] spelling: hw/audio/virtio-snd.c: initalize

[Stefan Weil via]

Am 13.11.23 um 22:20 schrieb Michael Tokarev:

Fixes: eb9ad377bb94 "virtio-sound: handle control messages and streams"
Signed-off-by: Michael Tokarev 
---
  hw/audio/virtio-snd.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index a18a9949a7..2fe966e311 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1126,7 +1126,7 @@ static void virtio_snd_realize(DeviceState *dev, Error 
**errp)
  status = virtio_snd_set_pcm_params(vsnd, i, _params);
  if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
  error_setg(errp,
-   "Can't initalize stream params, device responded with 
%s.",
+   "Can't initialize stream params, device responded with 
%s.",
 print_code(status));
  return;
      }


Reviewed-by: Stefan Weil 

Thanks,
Stefan

[PATCH] Fix some typos in documentation and comments

[Stefan Weil via]

Signed-off-by: Stefan Weil 
---

This patch was triggered by a spelling check for the generated
QEMU documentation using codespell. It does not try to fix all
typos which still exist in the QEMU code, but has a focus on
those required to fix the documentation. Nevertheless some code
comments with the same typos were fixed, too.

I think the patch is trivial, so maybe it can still be included
in the upcoming release, but that's not strictly necessary.

Stefan

 docs/about/deprecated.rst| 2 +-
 docs/devel/qom.rst   | 2 +-
 docs/system/devices/nvme.rst | 2 +-
 hw/core/loader.c | 4 ++--
 include/exec/memory.h| 2 +-
 ui/vnc-enc-tight.c   | 2 +-
 6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 1c35f55666..92a2bafd2b 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -369,7 +369,7 @@ mapping permissions et al by using its 'mapped' security 
model option.
 Nowadays it would make sense to reimplement the ``proxy`` backend by using
 QEMU's ``vhost`` feature, which would eliminate the high latency costs under
 which the 9p ``proxy`` backend currently suffers. However as of to date nobody
-has indicated plans for such kind of reimplemention unfortunately.
+has indicated plans for such kind of reimplementation unfortunately.
 
 
 Block device options
diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst
index 0b506426d7..9918fac7f2 100644
--- a/docs/devel/qom.rst
+++ b/docs/devel/qom.rst
@@ -30,7 +30,7 @@ user configuration.
 Creating a QOM class
 
 
-A simple minimal device implementation may look something like bellow:
+A simple minimal device implementation may look something like below:
 
 .. code-block:: c
:caption: Creating a minimal type
diff --git a/docs/system/devices/nvme.rst b/docs/system/devices/nvme.rst
index a8bb8d729c..2a3af268f7 100644
--- a/docs/system/devices/nvme.rst
+++ b/docs/system/devices/nvme.rst
@@ -232,7 +232,7 @@ parameters:
   Set the number of Reclaim Groups.
 
 ``fdp.nruh`` (default: ``0``)
-  Set the number of Reclaim Unit Handles. This is a mandatory paramater and
+  Set the number of Reclaim Unit Handles. This is a mandatory parameter and
   must be non-zero.
 
 ``fdp.runs`` (default: ``96M``)
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 8b7fd9e9e5..4dd5a71fb7 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -863,7 +863,7 @@ ssize_t load_image_gzipped(const char *filename, hwaddr 
addr, uint64_t max_sz)
 
 /*
  * The Linux header magic number for a EFI PE/COFF
- * image targetting an unspecified architecture.
+ * image targeting an unspecified architecture.
  */
 #define EFI_PE_LINUX_MAGIC"\xcd\x23\x82\x81"
 
@@ -1492,7 +1492,7 @@ RomGap rom_find_largest_gap_between(hwaddr base, size_t 
size)
 if (rom->mr || rom->fw_file) {
 continue;
 }
-/* ignore anything finishing bellow base */
+/* ignore anything finishing below base */
 if (rom->addr + rom->romsize <= base) {
 continue;
 }
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 7f5c11a0cc..68284428f8 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -942,7 +942,7 @@ struct MemoryListener {
  *
  * @listener: The #MemoryListener.
  * @last_stage: The last stage to synchronize the log during migration.
- * The caller should gurantee that the synchronization with true for
+ * The caller should guarantee that the synchronization with true for
  * @last_stage is triggered for once after all VCPUs have been stopped.
  */
 void (*log_sync_global)(MemoryListener *listener, bool last_stage);
diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index 09200d71b8..ee853dcfcb 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -77,7 +77,7 @@ static int tight_send_framebuffer_update(VncState *vs, int x, 
int y,
 
 #ifdef CONFIG_VNC_JPEG
 static const struct {
-double jpeg_freq_min;   /* Don't send JPEG if the freq is bellow */
+double jpeg_freq_min;   /* Don't send JPEG if the freq is below */
 double jpeg_freq_threshold; /* Always send JPEG if the freq is above */
 int jpeg_idx;   /* Allow indexed JPEG */
 int jpeg_full;  /* Allow full color JPEG */
-- 
2.39.2

Re: [PATCH v2 1/1] ui/sdl2: disable SDL_HINT_GRAB_KEYBOARD on Windows

[Stefan Weil via]

Am 18.04.23 um 08:56 schrieb Volker Rümelin:


Windows sends an extra left control key up/down input event for
every right alt key up/down input event for keyboards with
international layout. Since commit 830473455f ("ui/sdl2: fix
handling of AltGr key on Windows") QEMU uses a Windows low level
keyboard hook procedure to reliably filter out the special left
control key and to grab the keyboard on Windows.

The SDL2 version 2.0.16 introduced its own Windows low level
keyboard hook procedure to grab the keyboard. Windows calls this
callback before the QEMU keyboard hook procedure. This disables
the special left control key filter when the keyboard is grabbed.

To fix the problem, disable the SDL2 Windows low level keyboard
hook procedure.

Reported-by: Bernhard Beschow 
Signed-off-by: Volker Rümelin 
---
  ui/sdl2.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/ui/sdl2.c b/ui/sdl2.c
index 00aadfae37..9d703200bf 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -855,7 +855,10 @@ static void sdl2_display_init(DisplayState *ds, 
DisplayOptions *o)
  #ifdef SDL_HINT_VIDEO_X11_NET_WM_BYPASS_COMPOSITOR /* only available since 
SDL 2.0.8 */
  SDL_SetHint(SDL_HINT_VIDEO_X11_NET_WM_BYPASS_COMPOSITOR, "0");
  #endif
+#ifndef CONFIG_WIN32
+/* QEMU uses its own low level keyboard hook procecure on Windows */



s/procecure/procedure/



  SDL_SetHint(SDL_HINT_GRAB_KEYBOARD, "1");
+#endif
  #ifdef SDL_HINT_ALLOW_ALT_TAB_WHILE_GRABBED
  SDL_SetHint(SDL_HINT_ALLOW_ALT_TAB_WHILE_GRABBED, "0");
  #endif



The typo fix for the comment does not require a v3 and can be applied in 
the pull request.


Reviewed-by: Stefan Weil 

Re: source fails to compile on msys2

[Stefan Weil via]

Am 12.04.23 um 15:13 schrieb Howard Spoelstra:

Hello Peter,

My source was cloned today. I just cloned again and I still see the 
tokens reversed:
git clone https://www.gitlab.com/qemu/qemu 
 qemu-master-clean


The official URL is https://gitlab.com/qemu-project/qemu/.

Regards,
Stefan

Re: source fails to compile on msys2

[Stefan Weil via]

Am 12.04.23 um 14:12 schrieb BALATON Zoltan:


On Wed, 12 Apr 2023, Howard Spoelstra wrote:

It seems the current source fails to compile with up to date msys2.


See here: https://qemu.weilnetz.de/
I think there are some patches there that aren't upstream. I don't 
know why and also don't know why those binaries are not built from 
QEMU master.



My related Git repository is https://repo.or.cz/w/qemu/ar7.git/. I 
merged v8.0.0-rc3 two days ago, and that code builds with no problems 
for Windows.


And yes, my code includes commits which are (still) missing upstream. 
Some of them are for special hardware which was either removed from QEMU 
master or which I think is not interesting for upstream. Others were 
already sent to qemu-devel, so might become part of QEMU master later. 
In addition there are commits for my Windows build environment which 
also uses a higher warning level than QEMU master. But usually the 
differences to the latest tagged QEMU release are small.


Stefan

[PATCH for-8.0] docs/cxl: Fix sentence

[Stefan Weil via]

Signed-off-by: Stefan Weil 
---

If my change is okay I suggest to apply the patch for 8.0
because it fixes documentation.

Regards,
Stefan W.

 docs/system/devices/cxl.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/system/devices/cxl.rst b/docs/system/devices/cxl.rst
index f25783a4ec..4c38223069 100644
--- a/docs/system/devices/cxl.rst
+++ b/docs/system/devices/cxl.rst
@@ -111,7 +111,7 @@ Interfaces provided include:
 
 CXL Root Ports (CXL RP)
 ~~~
-A CXL Root Port servers te same purpose as a PCIe Root Port.
+A CXL Root Port serves the same purpose as a PCIe Root Port.
 There are a number of CXL specific Designated Vendor Specific
 Extended Capabilities (DVSEC) in PCIe Configuration Space
 and associated component register access via PCI bars.
-- 
2.39.2

[PATCH for-8.0] docs: Fix typo (wphx => whpx)

[Stefan Weil via]

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1529
Signed-off-by: Stefan Weil 
---

I suggest to apply the patch for 8.0 because it fixes documentation.

Regards
Stefan W.

 docs/system/introduction.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/system/introduction.rst b/docs/system/introduction.rst
index c8a9fe6c1d..3e256f8326 100644
--- a/docs/system/introduction.rst
+++ b/docs/system/introduction.rst
@@ -27,7 +27,7 @@ Tiny Code Generator (TCG) capable of emulating many CPUs.
   * - Hypervisor Framework (hvf)
 - MacOS
 - x86 (64 bit only), Arm (64 bit only)
-  * - Windows Hypervisor Platform (wphx)
+  * - Windows Hypervisor Platform (whpx)
 - Windows
 - x86
   * - NetBSD Virtual Machine Monitor (nvmm)
-- 
2.39.2

[PATCH] hw/arm: Fix some typos in comments (most found by codespell)

[Stefan Weil via]

Signed-off-by: Stefan Weil 
---

The patch does not change code and could also be applied for 8.0.

 hw/arm/Kconfig| 2 +-
 hw/arm/exynos4210.c   | 4 ++--
 hw/arm/musicpal.c | 2 +-
 hw/arm/omap1.c| 2 +-
 hw/arm/omap2.c| 2 +-
 hw/arm/virt-acpi-build.c  | 2 +-
 hw/arm/virt.c | 2 +-
 hw/arm/xlnx-versal-virt.c | 2 +-
 8 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index b5aed4aff5..db1105c717 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -126,7 +126,7 @@ config OLIMEX_STM32_H405
 config NSERIES
 bool
 select OMAP
-select TMP105   # tempature sensor
+select TMP105   # temperature sensor
 select BLIZZARD # LCD/TV controller
 select ONENAND
 select TSC210X  # touchscreen/sensors/audio
diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 6f2dda13f6..de39fb0ece 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -326,7 +326,7 @@ static int mapline_size(const int *mapline)
 
 /*
  * Initialize board IRQs.
- * These IRQs contain splitted Int/External Combiner and External Gic IRQs.
+ * These IRQs contain split Int/External Combiner and External Gic IRQs.
  */
 static void exynos4210_init_board_irqs(Exynos4210State *s)
 {
@@ -744,7 +744,7 @@ static void exynos4210_realize(DeviceState *socdev, Error 
**errp)
  * - SDMA
  * - ADMA2
  *
- * As this part of the Exynos4210 is not publically available,
+ * As this part of the Exynos4210 is not publicly available,
  * we used the "HS-MMC Controller S3C2416X RISC Microprocessor"
  * public datasheet which is very similar (implementing
  * MMC Specification Version 4.0 being the only difference noted)
diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index c9010b2ffb..58f3d30c9b 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -100,7 +100,7 @@
 #define MP_LCD_SPI_CMD  0x00104011
 #define MP_LCD_SPI_INVALID  0x
 
-/* Commmands */
+/* Commands */
 #define MP_LCD_INST_SETPAGE00xB0
 /* ... */
 #define MP_LCD_INST_SETPAGE70xB7
diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
index 559c066ce9..d5438156ee 100644
--- a/hw/arm/omap1.c
+++ b/hw/arm/omap1.c
@@ -4057,7 +4057,7 @@ struct omap_mpu_state_s *omap310_mpu_init(MemoryRegion 
*dram,
 s->led[1] = omap_lpg_init(system_memory,
   0xfffbd800, omap_findclk(s, "clk32-kHz"));
 
-/* Register mappings not currenlty implemented:
+/* Register mappings not currently implemented:
  * MCSI2 Comm  fffb2000 - fffb27ff (not mapped on OMAP310)
  * MCSI1 Bluetooth fffb2800 - fffb2fff (not mapped on OMAP310)
  * USB W2FCfffb4000 - fffb47ff
diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c
index 366d6af1b6..d5a2ae7af6 100644
--- a/hw/arm/omap2.c
+++ b/hw/arm/omap2.c
@@ -2523,7 +2523,7 @@ struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion 
*sdram,
 omap_findclk(s, "func_96m_clk"),
 omap_findclk(s, "core_l4_iclk"));
 
-/* All register mappings (includin those not currenlty implemented):
+/* All register mappings (including those not currently implemented):
  * SystemControlMod4800 - 48000fff
  * SystemControlL4 48001000 - 48001fff
  * 32kHz Timer Mod 48004000 - 48004fff
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 4156111d49..4af0de8b24 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -694,7 +694,7 @@ static void build_append_gicr(GArray *table_data, uint64_t 
base, uint32_t size)
 build_append_int_noprefix(table_data, 0xE, 1);  /* Type */
 build_append_int_noprefix(table_data, 16, 1);   /* Length */
 build_append_int_noprefix(table_data, 0, 2);/* Reserved */
-/* Discovery Range Base Addres */
+/* Discovery Range Base Address */
 build_append_int_noprefix(table_data, base, 8);
 build_append_int_noprefix(table_data, size, 4); /* Discovery Range Length 
*/
 }
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index ac626b3bef..4983f5fc93 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2052,7 +2052,7 @@ static void machvirt_init(MachineState *machine)
 int pa_bits;
 
 /*
- * Instanciate a temporary CPU object to find out about what
+ * Instantiate a temporary CPU object to find out about what
  * we are about to deal with. Once this is done, get rid of
  * the object.
  */
diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
index 37fc9b919c..668a9d65a4 100644
--- a/hw/arm/xlnx-versal-virt.c
+++ b/hw/arm/xlnx-versal-virt.c
@@ -659,7 +659,7 @@ static void versal_virt_init(MachineState *machine)
 fdt_add_clk_node(s, "/clk25", 2500, s->phandle.clk_25Mhz);
 
 /* Make the APU cpu address space visible to virtio and other
- * modules unaware of muliple

Re: [PATCH 19/41] compiler.h: replace QEMU_NORETURN with G_NORETURN

[Stefan Weil via]

Am 07.04.23 um 19:01 schrieb Stefan Weil:
Please excuse the late report, but this old patch causes a build failure 
for me:


I just noticed that this is already fixed in latest code (I tested the 
build with v8.0.0-rc0). So nothing to do. Sorry for the noise.


Stefan

Re: [PATCH 19/41] compiler.h: replace QEMU_NORETURN with G_NORETURN

[Stefan Weil via]

Please excuse the late report, but this old patch causes a build failure 
for me:


Am 20.04.22 um 15:26 schrieb marcandre.lur...@redhat.com:

From: Marc-André Lureau 

G_NORETURN was introduced in glib 2.68, fallback to G_GNUC_NORETURN in
glib-compat.

Note that this attribute must be placed before the function declaration
(bringing a bit of consistency in qemu codebase usage).

Signed-off-by: Marc-André Lureau 
---

[...]

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 848916f5165c..14b6b65a5fa9 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -177,7 +177,8 @@ extern "C" {
   * supports QEMU_ERROR, this will be reported at compile time; otherwise
   * this will be reported at link time due to the missing symbol.
   */
-extern void QEMU_NORETURN QEMU_ERROR("code path is reachable")
+extern G_NORETURN
+void QEMU_ERROR("code path is reachable")
  qemu_build_not_reached_always(void);
  #if defined(__OPTIMIZE__) && !defined(__NO_INLINE__)
  #define qemu_build_not_reached()  qemu_build_not_reached_always()


The placement of G_NORETURN causes a compiler error for C++ code in 
cross builds for Windows (see below). C++ expects the attribute 
[[noreturn]] before the extern statement.


I updated my Debian build environment to Debian bookworm and a recent 
cross glib, so maybe the problem was hidden in previous builds because I 
used a rather old glib or an older g++ cross compiler.


Regards,
Stefan


In file included from /mingw64/lib/glib-2.0/include/glibconfig.h:9,
 from /mingw64/include/glib-2.0/glib/gtypes.h:34,
 from /mingw64/include/glib-2.0/glib/galloca.h:34,
 from /mingw64/include/glib-2.0/glib.h:32,
 from 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/include/glib-compat.h:32,
 from 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/include/qemu/osdep.h:144,

 from ../../../qga/vss-win32/requester.cpp:13:
/mingw64/include/glib-2.0/glib/gmacros.h:1076:21: error: standard 
attributes in middle of decl-specifiers

 1076 | # define G_NORETURN [[noreturn]]
  | ^
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/include/qemu/osdep.h:240:8: 
note: in expansion of macro ‘G_NORETURN’

  240 | extern G_NORETURN
  |^~
/mingw64/include/glib-2.0/glib/gmacros.h:1076:21: note: standard 
attributes must precede the decl-specifiers to apply to the declaration, 
or follow them to apply to the type

 1076 | # define G_NORETURN [[noreturn]]
  | ^
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/include/qemu/osdep.h:240:8: 
note: in expansion of macro ‘G_NORETURN’

  240 | extern G_NORETURN
  |^~
/mingw64/include/glib-2.0/glib/gmacros.h:1076:21: warning: attribute 
ignored [-Wattributes]

 1076 | # define G_NORETURN [[noreturn]]
  | ^
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/include/qemu/osdep.h:240:8: 
note: in expansion of macro ‘G_NORETURN’

  240 | extern G_NORETURN
  |^~
/mingw64/include/glib-2.0/glib/gmacros.h:1076:21: note: an attribute 
that appertains to a type-specifier is ignored

 1076 | # define G_NORETURN [[noreturn]]
  | ^
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/include/qemu/osdep.h:240:8: 
note: in expansion of macro ‘G_NORETURN’

  240 | extern G_NORETURN
  |^~
../../../qga/vss-win32/requester.cpp: In function ‘HRESULT 
requester_init()’:
../../../qga/vss-win32/requester.cpp:72:34: warning: cast between 
incompatible function types from ‘FARPROC’ {aka ‘long long int (*)()’} 
to ‘t_CreateVssBackupComponents’ {aka ‘long int 
(*)(IVssBackupComponents**)’} [-Wcast-function-type]

   72 | pCreateVssBackupComponents = (t_CreateVssBackupComponents)
  |  ^
   73 | GetProcAddress(hLib,
  | 
   74 | #ifdef _WIN64 /* 64bit environment */
  | ~
   75 | 
"?CreateVssBackupComponents@@YAJPEAPEAVIVssBackupComponents@@@Z"
  | 


   76 | #else /* 32bit environment */
  | ~
   77 | 
"?CreateVssBackupComponents@@YGJPAPAVIVssBackupComponents@@@Z"
  | 
~~

   78 | #endif
  | ~~
   79 | );
  | ~
../../../qga/vss-win32/requester.cpp:85:34: warning: cast between 
incompatible function types from ‘FARPROC’ {aka ‘long long int (*)()’} 
to ‘t_VssFreeSnapshotProperties’ {aka ‘void (*)(VSS_SNAPSHOT_PROP*)’} 
[-Wcast-function-type]

   85 | pVssFreeSnapshotProperties = (t_VssFreeSnapshotProperties)
  |  ^
   86 | GetProcAddress(hLib, "VssFreeSnapshotProperties");
  | ~
ninja: build stopped: 

Re: [PATCH-for-8.0] block/dmg: Ignore C99 prototype declaration mismatch from

[Stefan Weil via]

Am 27.03.23 um 23:09 schrieb Paolo Bonzini:

Il lun 27 mar 2023, 20:58 Philippe Mathieu-Daudé  
ha scritto:


> The warning can also be suppressed if the build uses `-isystem
> /opt/homebrew/include` instead of `-I/opt/homebrew/include` as I
just
> have tested.

Is that option added by QEMU's configure or meson.build? Or is it 
added by homebrew? The fact that /opt/homebrew/include it isn't 
considered a system seems to be a homebrew decision.


IIUC by design meson only allows including *relative* directories,
and manage the system ones:
https://mesonbuild.com/Include-directories.html

That's for includes that are part of QEMU.

Meson has as_system for dependency objects 
(https://mesonbuild.com/Reference-manual_returned_dep.html) but lzfse 
doesn't have a .pc file, its detection has to be done by hand.


Paolo

> If we can find a solution how to implement that I thing it would
look
> nicer. Technically the patch looks good.
>
    > Reviewed-by: Stefan Weil 

Thanks!



Typically I configure the build on macOS with `./configure 
--extra-cflags=-I/opt/homebrew/include 
--extra-ldflags=-L/opt/homebrew/lib --disable-werror`. With that 
configuration I get the two warnings for lzfse.h.


When I use `./configure '--extra-cflags=-isystem /opt/homebrew/include' 
--extra-ldflags=-L/opt/homebrew/lib --disable-werror` instead, I get no 
compiler warnings (and `--disable-werror` could be ommitted).


So at least for macOS with Homebrew in /opt/homebrew (M1 / M2 Macs) the 
patch is not needed when the right configure options (`--extra-cflags`) 
were used.


Stefan

Re: [PATCH-for-8.0] block/dmg: Ignore C99 prototype declaration mismatch from

[Stefan Weil via]

Am 27.03.23 um 17:13 schrieb Philippe Mathieu-Daudé:


When liblzfe (Apple LZFSE compression library) is present
(for example installed via 'brew') on Darwin, QEMU build
fails as:

   Has header "lzfse.h" : YES
   Library lzfse found: YES

 Dependencies
   lzo support  : NO
   snappy support   : NO
   bzip2 support: YES
   lzfse support: YES
   zstd support : YES 1.5.2

 User defined options
   dmg  : enabled
   lzfse: enabled

   [221/903] Compiling C object libblock.fa.p/block_dmg-lzfse.c.o
   FAILED: libblock.fa.p/block_dmg-lzfse.c.o
   /opt/homebrew/Cellar/lzfse/1.0/include/lzfse.h:56:43: error: this function 
declaration is not a prototype [-Werror,-Wstrict-prototypes]
   LZFSE_API size_t lzfse_encode_scratch_size();
 ^
  void
   /opt/homebrew/Cellar/lzfse/1.0/include/lzfse.h:94:43: error: this function 
declaration is not a prototype [-Werror,-Wstrict-prototypes]
   LZFSE_API size_t lzfse_decode_scratch_size();
 ^
  void
   2 errors generated.
   ninja: build stopped: subcommand failed.

This issue has been reported in the lzfse project in 2016:
https://github.com/lzfse/lzfse/issues/3#issuecomment-226574719

Since the project seems unmaintained, simply ignore the
strict-prototypes warning check for the  header,
similarly to how we deal with the GtkItemFactoryCallback
prototype from , indirectly included
by .

Cc: Julio Faracco 
Signed-off-by: Philippe Mathieu-Daudé 
---
  block/dmg-lzfse.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/block/dmg-lzfse.c b/block/dmg-lzfse.c
index 6798cf4fbf..0abc970bf6 100644
--- a/block/dmg-lzfse.c
+++ b/block/dmg-lzfse.c
@@ -23,7 +23,12 @@
   */
  #include "qemu/osdep.h"
  #include "dmg.h"
+
+/* Work around an -Wstrict-prototypes warning in LZFSE headers */



"Work around a -Wstrict-prototypes" ("a" instead of "an")?



+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wstrict-prototypes"
  #include 
+#pragma GCC diagnostic pop
  
  static int dmg_uncompress_lzfse_do(char *next_in, unsigned int avail_in,

 char *next_out, unsigned int avail_out)



The warning can also be suppressed if the build uses `-isystem 
/opt/homebrew/include` instead of `-I/opt/homebrew/include` as I just 
have tested.


If we can find a solution how to implement that I thing it would look 
nicer. Technically the patch looks good.


Reviewed-by: Stefan Weil 

Thanks,

Stefan

Re: [PATCH] Updated the FSF address to

[Stefan Weil via]

Am 20.02.23 um 08:01 schrieb Khadija Kamran:

From: Khadija Kamran 

The Free Software Foundation moved to a new address and some sources in QEMU 
referred to their old location.
The address should be updated and replaced to a pointer to 
<https://www.gnu.org/licenses/>


... replaced by a pointer ... ?


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

Signed-off-by: Khadija Kamran 
---
  contrib/gitdm/filetypes.txt | 3 +--
  hw/scsi/viosrp.h| 3 +--
  hw/sh4/sh7750_regs.h| 3 +--
  include/hw/arm/raspi_platform.h | 3 +--
  include/qemu/uri.h  | 3 +--
  tests/qemu-iotests/022  | 4 +---
  tests/unit/rcutorture.c | 3 +--
  tests/unit/test-rcu-list.c  | 3 +--
  util/uri.c  | 3 +--
  9 files changed, 9 insertions(+), 19 deletions(-)



Reviewed-by: Stefan Weil 

Re: [RFC PATCH] docs/about/deprecated: Deprecate 32-bit host systems

[Stefan Weil via]

Am 19.02.23 um 12:27 schrieb Reinoud Zandijk:


On Fri, Feb 17, 2023 at 12:05:46PM +0100, Stefan Weil wrote:

So there still seems to be a certain small need for QEMU installers for
32-bit Windows: 170 users für 32 bit only, 339 users for both 32 and 64 bit,
5132 users for 64 bit only.

As you seem to have access to download stats could you check generic download
stats too i.e. not only for Windows installers?



Sorry, but I only have statistics for my own server 
https://qemu.weilnetz.de/ which provides Windows installers.


Stefan

Re: [RFC PATCH] docs/about/deprecated: Deprecate 32-bit host systems

[Stefan Weil via]

On Fri, Feb 17, 2023 at 11:36:41AM +0100, Markus Armbruster wrote:


Which 32-bit hosts are still useful, and why?



Citing my previous mail:

   I now checked all downloads of the latests installers since 2022-12-30.

   qemu-w32-setup-20221230.exe – 509 different IP addresses
   qemu-w64-setup-20221230.exe - 5471 different IP addresses

   339 unique IP addresses are common for 32- and 64-bit, either
   crawlers or people who simply got both variants. So there remain 170
   IP addresses which only downloaded the 32-bit variant in the last week.

   I see 437 different strings for the browser type, but surprisingly
   none of them looks like a crawler.

So there still seems to be a certain small need for QEMU installers for 
32-bit Windows: 170 users für 32 bit only, 339 users for both 32 and 64 
bit, 5132 users for 64 bit only.


Stefan

Re: [PATCH v2 1/2] qga-win: add logging to Windows event log

[Stefan Weil via]

Am 23.01.23 um 20:38 schrieb Andrey Drobyshev:


Hi Stefan,

On 1/23/23 19:28, Stefan Weil wrote:

Hi,

cross builds fail with this code. Please see details below.

Am 29.11.22 um 18:38 schrieb Andrey Drobyshev via:

This commit allows QGA to write to Windows event log using Win32 API's
ReportEvent() [1], much like syslog() under *nix guests.

In order to generate log message definitions we use a very basic message
text file [2], so that every QGA's message gets ID 1.  The tools
"windmc" and "windres" respectively are used to generate ".rc" file and
COFF object file, and then the COFF file is linked into qemu-ga.exe.

[1]
https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-reporteventa
[2]
https://learn.microsoft.com/en-us/windows/win32/eventlog/message-text-files

Originally-by: Yuri Pudgorodskiy 
Signed-off-by: Andrey Drobyshev 
---
   configure |  3 +++
   qga/installer/qemu-ga.wxs |  5 +
   qga/main.c    | 16 +---
   qga/meson.build   | 19 ++-
   qga/messages-win32.mc |  9 +
   5 files changed, 48 insertions(+), 4 deletions(-)
   create mode 100644 qga/messages-win32.mc

diff --git a/configure b/configure
index 26c7bc5154..789a4f6cc9 100755
--- a/configure
+++ b/configure
@@ -372,6 +372,7 @@ smbd="$SMBD"
   strip="${STRIP-${cross_prefix}strip}"
   widl="${WIDL-${cross_prefix}widl}"
   windres="${WINDRES-${cross_prefix}windres}"
+windmc="${WINDMC-${cross_prefix}windmc}"

Here the needed cross prefix is added ...


   pkg_config_exe="${PKG_CONFIG-${cross_prefix}pkg-config}"
   query_pkg_config() {
   "${pkg_config_exe}" ${QEMU_PKG_CONFIG_FLAGS} "$@"

[...]

diff --git a/qga/meson.build b/qga/meson.build
index 3cfb9166e5..1ff159edc1 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -98,7 +98,24 @@ if targetos == 'windows'
     endif
   endif
   -qga = executable('qemu-ga', qga_ss.sources(),
+qga_objs = []
+if targetos == 'windows'
+  windmc = find_program('windmc', required: true)

... but here the cross prefix is missing and the cross build aborts
because windmc does not exist.

There's no need for the cross prefix here.  After you've run ./configure
with --cross-prefix, argument, you'll see the following in
build/config-meson.cross file:

[binaries]

widl = ['x86_64-w64-mingw32-widl']
windres = ['x86_64-w64-mingw32-windres']
windmc = ['x86_64-w64-mingw32-windmc']

And these are the actual values meson's find_program() is going to be
looking for.  So it doesn't seem like there's anything broken here, it's
a matter of build requirements.



My configure terminates with an error because of the missing windmc 
before it has written config-meson.cross. I have run an incremental build:


Program windmc found: NO

../../../qga/meson.build:103:2: ERROR: Program 'windmc' not found or not 
executable


A full log can be found at 
/qemu/bin/debug/x86_64-w64-mingw32/meson-logs/meson-log.txt

ninja: error: rebuilding 'build.ninja': subcommand failed
FAILED: build.ninja
/usr/bin/python3 /qemu/meson/meson.py --internal regenerate /qemu 
/home/stefan/src/gitlab/qemu-project/qemu/bin/debug/x86_64-w64-mingw32 
--backend ninja

make: *** [Makefile:165: run-ninja] Fehler 1
make: Verzeichnis „/qemu/bin/debug/x86_64-w64-mingw32“ wird verlassen

A clean fresh build works indeed fine.

Stefan

Re: [PATCH v2 1/2] qga-win: add logging to Windows event log

[Stefan Weil via]

Hi,

cross builds fail with this code. Please see details below.

Am 29.11.22 um 18:38 schrieb Andrey Drobyshev via:

This commit allows QGA to write to Windows event log using Win32 API's
ReportEvent() [1], much like syslog() under *nix guests.

In order to generate log message definitions we use a very basic message
text file [2], so that every QGA's message gets ID 1.  The tools
"windmc" and "windres" respectively are used to generate ".rc" file and
COFF object file, and then the COFF file is linked into qemu-ga.exe.

[1] 
https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-reporteventa
[2] https://learn.microsoft.com/en-us/windows/win32/eventlog/message-text-files

Originally-by: Yuri Pudgorodskiy 
Signed-off-by: Andrey Drobyshev 
---
  configure |  3 +++
  qga/installer/qemu-ga.wxs |  5 +
  qga/main.c| 16 +---
  qga/meson.build   | 19 ++-
  qga/messages-win32.mc |  9 +
  5 files changed, 48 insertions(+), 4 deletions(-)
  create mode 100644 qga/messages-win32.mc

diff --git a/configure b/configure
index 26c7bc5154..789a4f6cc9 100755
--- a/configure
+++ b/configure
@@ -372,6 +372,7 @@ smbd="$SMBD"
  strip="${STRIP-${cross_prefix}strip}"
  widl="${WIDL-${cross_prefix}widl}"
  windres="${WINDRES-${cross_prefix}windres}"
+windmc="${WINDMC-${cross_prefix}windmc}"


Here the needed cross prefix is added ...


  pkg_config_exe="${PKG_CONFIG-${cross_prefix}pkg-config}"
  query_pkg_config() {
  "${pkg_config_exe}" ${QEMU_PKG_CONFIG_FLAGS} "$@"

[...]

diff --git a/qga/meson.build b/qga/meson.build
index 3cfb9166e5..1ff159edc1 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -98,7 +98,24 @@ if targetos == 'windows'
endif
  endif
  
-qga = executable('qemu-ga', qga_ss.sources(),

+qga_objs = []
+if targetos == 'windows'
+  windmc = find_program('windmc', required: true)


... but here the cross prefix is missing and the cross build aborts 
because windmc does not exist.


Regards
Stefan

Re: Announcement of aborting HAXM maintenance

[Stefan Weil via]

Am 19.01.23 um 11:12 schrieb Daniel P. Berrangé:

On Thu, Jan 19, 2023 at 03:56:04AM +, Wang, Wenchao wrote:

Hi, Philippe,

Intel decided to abort the development of HAXM and the maintenance
of its QEMU part. Should we submit a patch to mark the Guest CPU
Cores (HAXM) status as Orphan and remove the maintainers from the
corresponding list? Meanwhile, should the code enabling HAX in QEMU
once committed by the community be retained?


If you no longer intend to work on QEMU bits related to HAXM, then
yes, you should send a patch for the MAINTAINERS file to remove you
name and mark it as "Orphan" status.

We would not normally delete code from QEMU, merely because it has
been orphaned. If it is still known to work then we would retain
it indefinitely, unless some compelling reason arises to drop it.
This gives time for any potential users to adjust their plans,
and/or opportunity for other interested people to take over the
maintenance role.


HAXM will not only be no longer maintained in QEMU, but also the 
necessary framework for macOS and Windows will be retired. See 
https://github.com/intel/haxm#status on their GitHub page. As stated 
there, macOS provides HVF which can be used instead of HAXM, and Windows 
users can use WHPX. Both HVF and WHPX are supported by QEMU. As far as I 
know HAXM could only provide a limited RAM size (2 GiB?). Maybe it still 
has more deficits. And unmaintained HAXM drivers for macOS and Windows 
might be a security risk, too. It is also not clear whether the last 
downloads of those drivers will be available in the future.


Therefore I'd prefer to remove the whole HAXM code in QEMU soon, even in 
a minor update for this special case.


Stefan

Re: MSYS2 and libfdt

[Stefan Weil via]

Am 19.01.23 um 09:14 schrieb Thomas Huth:



 Hi all,

in some spare minutes, I started playing with a patch to try to remove 
the dtc submodule from the QEMU git repository - according to 
https://repology.org/project/dtc/versions our supported build 
platforms should now all provide the minimum required version.


However, I'm hitting a problem with Windows / MSYS2 in the CI jobs: 
The libfdt is packaged as part of the dtc package there:


 https://packages.msys2.org/package/dtc

... meaning that it is added with a usr/include and usr/lib path 
prefix instead of mingw64/include and mingw64/lib like other packages 
are using (see e.g. 
https://packages.msys2.org/package/mingw-w64-x86_64-zlib?repo=mingw64). 
Thus the compiler does not find the library there. Also there does not 
seem to be a difference between a i686 (32-bit) and x86_64 (64-bit) 
variant available here? Does anybody know how libfdt is supposed to be 
used with MSYS2 ?


 Thomas



Hi Thomas,

"dtc" is not the right package for cross builds. We'd require 
mingw-w64-i686-dtc and mingw-w64-x86_64-dtc packages for the QEMU build, 
but those packages are currently not provided by MSYS2.


See https://packages.msys2.org/search?t=binpkg=zlib for the zlib 
packages and related cross build packages.


Stefan

Re: building qemu on windows 11

[Stefan Weil via]

Am 12.01.23 um 16:16 schrieb Neal Elliott:

Hello,
              is it possible, or has anyone built qemu from the master 
branch using visual studio? I attempted to
build the code using mingw64, but it failed to build. is there a current 
build document for windows?


Building with Visual Studio is not supported.

Building on Windows with Mingw64 might fail (see 
https://gitlab.com/qemu-project/qemu/-/issues/1386), and the current 
documentation requires an update.


I suggest to run a cross build on Linux which works and is also much 
faster than building on Windows. Here is an example of such a build 
running as a GitHub action:

https://github.com/stweil/qemu/actions/runs/3903880614/jobs/6668872989

The related files are win.yml, build.sh and pacman.sh
from https://github.com/stweil/qemu/tree/master/.github/workflows.
The two shell scripts should also work on a typical Debian / Ubuntu or 
similar Linux host.


Regards
Stefan Weil

Re: [PATCH] .gitlab-ci.d/windows: Do not run the qtests in the msys2-32bit job

[Stefan Weil via]

Am 06.01.23 um 09:19 schrieb Thomas Huth:


On 06/01/2023 09.15, Stefan Weil wrote:


Download numbers from yesterday for my latest Windows installers:

qemu-w32-setup-20221230.exe - 243

qemu-w64-setup-20221230.exe - 6540

On Wednesday the ratio was 288 : 3516.

As expected the 64-bit variant is used much more often, but it looks 
like there is still a certain desire for the 32-bit variant.


OK, thanks. Could you maybe also check the browser types in the logs? 
... I'm wondering whether a big part of those w32 downloads were just 
automatic web crawlers?


 Thomas



I now checked all downloads of the latests installers since 2022-12-30.

qemu-w32-setup-20221230.exe – 509 different IP addresses
qemu-w64-setup-20221230.exe - 5471 different IP addresses

339 unique IP addresses are common for 32- and 64-bit, either crawlers 
or people who simply got both variants. So there remain 170 IP addresses 
which only downloaded the 32-bit variant in the last week.


I see 437 different strings for the browser type, but surprisingly none 
of them looks like a crawler.


Stefan

Re: [PATCH] .gitlab-ci.d/windows: Do not run the qtests in the msys2-32bit job

[Stefan Weil via]

Am 06.01.23 um 08:49 schrieb Thomas Huth:


On 05/01/2023 22.42, Philippe Mathieu-Daudé wrote:

> That said, maybe it is time to deprecate the 32-bit
> hosts?

Certainly fine for me, but that's up to the Windows folks to decide. 
Maybe you could just suggest a patch to start the discussion?


 Thomas



Download numbers from yesterday for my latest Windows installers:

qemu-w32-setup-20221230.exe - 243

qemu-w64-setup-20221230.exe - 6540

On Wednesday the ratio was 288 : 3516.

As expected the 64-bit variant is used much more often, but it looks 
like there is still a certain desire for the 32-bit variant.


Stefan

Re: [PATCH v3 04/26] configure: don't enable cross compilers unless in target_list

[Stefan Weil via]

Am 21.10.22 um 00:10 schrieb Richard Henderson:

On 10/20/22 21:51, Alex Bennée wrote:

This avoids the unfortunate effect of always builds the pc-bios blobs
for targets the user isn't interested in.

Suggested-by: Paolo Bonzini 
Signed-off-by: Alex Bennée 
---
  configure | 9 +
  1 file changed, 9 insertions(+)

diff --git a/configure b/configure
index 81561be7c1..dd6f58dcde 100755
--- a/configure
+++ b/configure
@@ -1877,6 +1877,15 @@ probe_target_compiler() {
    container_cross_ranlib=
    container_cross_strip=
+  # We shall skip configuring the target compiler if the user didn't
+  # bother enabling an appropriate guest. This avoids building
+  # extraneous firmware images and tests.
+  if test "${target_list#*$1}" != "$1"; then
+  break;



Isn't break limited for exiting from for, while, or until loop? (*)
If yes, it's wrongly used here. sh does not complain, but other
shells do.

Stefan

*) https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html

Re: [PATCH v2] target/i386: Remove compilation errors when -Werror=maybe-uninitialized

[Stefan Weil via]

Am 22.12.22 um 12:51 schrieb Philippe Mathieu-Daudé:


On 22/12/22 12:18, Eric Auger wrote:

Hi All,

On 12/22/22 12:09, Daniel P. Berrangé wrote:

On Thu, Dec 22, 2022 at 11:07:31AM +0100, Eric Auger wrote:

Hi Philippe,

On 12/22/22 10:01, Philippe Mathieu-Daudé wrote:

On 22/12/22 09:18, Paolo Bonzini wrote:

On 12/21/22 17:36, Eric Auger wrote:
To avoid compilation errors when -Werror=maybe-uninitialized is 
used,

replace 'case 3' by 'default'.

Otherwise we get:

../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
 2495 | d->Q(3) = r3;
  | ^~~~
../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
 2494 | d->Q(2) = r2;
  | ^~~~
../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
 2493 | d->Q(1) = r1;
  | ^~~~
../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
 2492 | d->Q(0) = r0;
  | ^~~~

With what compiler? Is that a supported one?
https://lore.kernel.org/qemu-devel/3aab489e-9d90-c1ad-0b6b-b2b5d80db...@redhat.com/ 


Signed-off-by: Eric Auger 
Suggested-by: Stefan Weil 
Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")
---
   target/i386/ops_sse.h | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
index 3cbc36a59d..c442c8c10c 100644
--- a/target/i386/ops_sse.h
+++ b/target/i386/ops_sse.h
@@ -2466,7 +2466,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
*s, uint32_t order)
   r0 = s->Q(0);
   r1 = s->Q(1);
   break;
-    case 3:
+    default:
   r0 = s->Q(2);
   r1 = s->Q(3);
   break;
@@ -2484,7 +2484,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
*s, uint32_t order)
   r2 = s->Q(0);
   r3 = s->Q(1);
   break;
-    case 3:
+    default:
   r2 = s->Q(2);
   r3 = s->Q(3);
   break;

Queued, but this compiler sucks. :)

Can't we simply add a dumb 'default' case? So when reviewing we don't
have to evaluate 'default' means 3 here.

-- >8 --
--- a/target/i386/ops_sse.h
+++ b/target/i386/ops_sse.h
@@ -2470,6 +2470,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
uint32_t order)
  r0 = s->Q(2);
  r1 = s->Q(3);
  break;
+    default:
+    qemu_build_not_reached();
  }
  switch ((order >> 4) & 3) {
  case 0:
@@ -2488,6 +2490,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
uint32_t order)
  r2 = s->Q(2);
  r3 = s->Q(3);
  break;
+    default:
+    qemu_build_not_reached();
  }
I guess this won't fix the fact r0, r1, r2, r3 are not initialized, 
will it?

This ultimately expands to assert() and the compiler should see that it
terminates the control flow at this point, so shouldn't have a reason
to warn.


OK so with qemu_build_not_reached(); I get

/home/augere/UPSTREAM/qemu/include/qemu/osdep.h:184:35: error: call to
‘qemu_build_not_reached_always’ declared with attribute error: code path
is reachable
   184 | #define qemu_build_not_reached() 
qemu_build_not_reached_always()

   | ^~~


However with g_assert_not_reached(), it does not complain and errors are
removed. So I will respin with g_assert_not_reached() if nobody advises
me against that.


Thank you!



As noted by Paolo a better compiler could know that 0, 1, 2 and 3 are 
the only possible cases. Such a better compiler might complain that an 
additional default case is never reached. Therefore the proposed code 
might cause future compiler warnings.


But we could use this code pattern to make the intention of the code 
clearer:


case 3:
default: /* default case added to help the compiler to avoid warnings */
    ...

Stefan

Re: [PATCH v2] target/i386: Remove compilation errors when -Werror=maybe-uninitialized

[Stefan Weil via]

Am 21.12.22 um 17:36 schrieb Eric Auger:


To avoid compilation errors when -Werror=maybe-uninitialized is used,
replace 'case 3' by 'default'.

Otherwise we get:

../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
2495 | d->Q(3) = r3;
 | ^~~~
../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
2494 | d->Q(2) = r2;
 | ^~~~
../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
2493 | d->Q(1) = r1;
 | ^~~~
../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
2492 | d->Q(0) = r0;
 | ^~~~

Signed-off-by: Eric Auger 
Suggested-by: Stefan Weil 
Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")
---
  target/i386/ops_sse.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
index 3cbc36a59d..c442c8c10c 100644
--- a/target/i386/ops_sse.h
+++ b/target/i386/ops_sse.h
@@ -2466,7 +2466,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t 
order)
  r0 = s->Q(0);
  r1 = s->Q(1);
  break;
-case 3:
+default:
  r0 = s->Q(2);
  r1 = s->Q(3);
  break;
@@ -2484,7 +2484,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t 
order)
  r2 = s->Q(0);
  r3 = s->Q(1);
  break;
-case 3:
+default:
  r2 = s->Q(2);
  r3 = s->Q(3);
  break;



Reviewed-by: Stefan Weil 

Thank you and merry Christmas!




OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH 1/6] disas: add G_GNUC_PRINTF to gstring_printf

[Stefan Weil via]

Am 19.12.22 um 14:02 schrieb Daniel P. Berrangé:

Signed-off-by: Daniel P. Berrangé 
---
  disas.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/disas.c b/disas.c
index 94d3b45042..31df8f5b89 100644
--- a/disas.c
+++ b/disas.c
@@ -239,6 +239,7 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong 
code,
  }
  }
  
+G_GNUC_PRINTF(2, 3)

  static int gstring_printf(FILE *stream, const char *fmt, ...)
  {
  /* We abuse the FILE parameter to pass a GString. */


The current code uses a different pattern:

static RETURN_TYPE G_GNUC_PRINTF(2, 3) function(argument list)

So I would have expected

static int G_GNUC_PRINTF(2, 3)
gstring_printf(FILE *stream, const char *fmt, ...)

Does that matter? Do we care for different patterns?

Stefan


OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH] mailmap: Fix Stefan Weil author email

[Stefan Weil via]

Am 08.12.22 um 16:55 schrieb Philippe Mathieu-Daudé:


Fix authorship of commits 266aaedc37~..ac14949821. See commit
3bd2608db7 ("maint: Add .mailmap entries for patches claiming
list authorship") for rationale.

Signed-off-by: Philippe Mathieu-Daudé 
---
  .mailmap | 1 +
  1 file changed, 1 insertion(+)

diff --git a/.mailmap b/.mailmap
index 35dddbe27b..fad2aff5aa 100644
--- a/.mailmap
+++ b/.mailmap
@@ -45,6 +45,7 @@ Ed Swierk  Ed Swierk via Qemu-devel 
 Ian McKellar via Qemu-devel 

  Julia Suvorova  Julia Suvorova via Qemu-devel 

  Justin Terry (VM)  Justin Terry (VM) via Qemu-devel 

+Stefan Weil  Stefan Weil via 
  
  # Next, replace old addresses by a more recent one.

  Aleksandar Markovic  




Signed-off-by: Stefan Weil 

Thanks!




OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PULL 11/21] docs: Build and install all the docs in a single manual

[Stefan Weil via]

Am 12.01.21 um 17:57 schrieb Peter Maydell:
[...]

diff --git a/docs/meson.build b/docs/meson.build
index fae9849b79b..bb14eaebd3b 100644
--- a/docs/meson.build
+++ b/docs/meson.build
@@ -46,19 +46,11 @@ if build_docs
meson.source_root() / 'docs/sphinx/qmp_lexer.py',
qapi_gen_depends ]
  
-  configure_file(output: 'index.html',

- input: files('index.html.in'),
- configuration: {'VERSION': meson.project_version()},
- install_dir: qemu_docdir)
-  manuals = [ 'devel', 'interop', 'tools', 'specs', 'system', 'user' ]
man_pages = {
-'interop' : {
  'qemu-ga.8': (have_tools ? 'man8' : ''),
  'qemu-ga-ref.7': 'man7',
  'qemu-qmp-ref.7': 'man7',
  'qemu-storage-daemon-qmp-ref.7': (have_tools ? 'man7' : ''),
-},
-'tools': {
  'qemu-img.1': (have_tools ? 'man1' : ''),
  'qemu-nbd.8': (have_tools ? 'man8' : ''),
  'qemu-pr-helper.8': (have_tools ? 'man8' : ''),
@@ -66,53 +58,47 @@ if build_docs
  'qemu-trace-stap.1': (config_host.has_key('CONFIG_TRACE_SYSTEMTAP') ? 
'man1' : ''),
  'virtfs-proxy-helper.1': (have_virtfs_proxy_helper ? 'man1' : ''),
  'virtiofsd.1': (have_virtiofsd ? 'man1' : ''),
-},
-'system': {
  'qemu.1': 'man1',
  'qemu-block-drivers.7': 'man7',
  'qemu-cpu-models.7': 'man7'
-},
}
  
sphinxdocs = []

sphinxmans = []
-  foreach manual : manuals
-private_dir = meson.current_build_dir() / (manual + '.p')
-output_dir = meson.current_build_dir() / manual
-input_dir = meson.current_source_dir() / manual
  
-this_manual = custom_target(manual + ' manual',

+  private_dir = meson.current_build_dir() / 'manual.p'
+  output_dir = meson.current_build_dir() / 'manual'
+  input_dir = meson.current_source_dir()
+
+  this_manual = custom_target('QEMU manual',
  build_by_default: build_docs,
-output: [manual + '.stamp'],
-input: [files('conf.py'), files(manual / 'conf.py')],
-depfile: manual + '.d',
+output: 'docs.stamp',
+input: files('conf.py'),
+depfile: 'docs.d',
  depend_files: sphinx_extn_depends,
  command: [SPHINX_ARGS, '-Ddepfile=@DEPFILE@',
'-Ddepfile_stamp=@OUTPUT0@',
'-b', 'html', '-d', private_dir,
input_dir, output_dir])
-sphinxdocs += this_manual
-if build_docs and manual != 'devel'
-  install_subdir(output_dir, install_dir: qemu_docdir)
-endif
+  sphinxdocs += this_manual
+  install_subdir(output_dir, install_dir: qemu_docdir, strip_directory: true)


This line causes a build warning with the latest code:

../../../docs/meson.build:74: WARNING: Project targets '>=0.61.3' but 
uses feature deprecated since '0.60.0': install_subdir with empty 
directory. It worked by accident and is buggy. Use install_emptydir instead.


It looks like `qemu_docdir` is no longer defined anywhere.

I still did not find out whether this is an issue which needs a fix for 7.2.

Stefan

  
-these_man_pages = []

-install_dirs = []

[...]

Compiler warnings with maximum warning level (was: Re: [PATCH for 7.2?] target/i386: Remove compilation errors when -Werror=maybe-uninitialized)

[Stefan Weil via]

Am 07.12.22 um 20:11 schrieb Stefan Weil:

On 12/7/22 14:24, Eric Auger wrote:

Initialize r0-3 to avoid compilation errors when
-Werror=maybe-uninitialized is used

../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
   2495 | d->Q(3) = r3;
    | ^~~~
../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
   2494 | d->Q(2) = r2;
    | ^~~~
../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
   2493 | d->Q(1) = r1;
    | ^~~~
../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
   2492 | d->Q(0) = r0;
    | ^~~~

Signed-off-by: Eric Auger 
Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")

---

Am I the only one getting this? Or anything wrong in my setup.


Hi Eric,

no, you are not the only one. I regularly build with higher warning 
levels, for example with -Weverything on macOS, and get a much longer 
list which includes the mentioned warnings (see below).


The latest QEMU code produces 6780505 compiler warnings and a build log 
file with 2.7 GB (!) with compiler option `-Weverything` on macOS.


Many warnings occur more than once, but there remain 193313 unique 
warnings for the QEMU code (see 
https://qemu.weilnetz.de/test/warnings-20221207.txt). Here is a list of 
all kinds of warnings sorted by frequency:


   1 -Wkeyword-macro
   1 -Wundeclared-selector
   1 -Wunreachable-code-loop-increment
   1 -Wunused-but-set-parameter
   2 -Wgnu-union-cast
   2 -Woverlength-strings
   3 -Walloca
   5 -Wflexible-array-extensions
   5 -Wstrict-selector-match
   5 -Wstring-conversion
   5 -Wtautological-value-range-compare
   6 -Wcstring-format-directive
   8 -Wstatic-in-inline
  13 -Wobjc-messaging-id
  13 -Wvla
  14 -Wobjc-interface-ivars
  16 -Wimplicit-float-conversion
  17 -Wformat-nonliteral
  24 -Wredundant-parens
  39 -Wfloat-equal
  44 -Wc++-compat
  47 -Wzero-length-array
  53 -Wdouble-promotion
  53 -Wvariadic-macros
  65 -Wpacked
  74 -Wcomma
  82 -Wunreachable-code-return
  90 -Wformat-pedantic
  90 -Wmissing-noreturn
  94 -Wgnu-flexible-array-initializer
 120 -Wcovered-switch-default
 132 -Wdirect-ivar-access
 136 -Wconditional-uninitialized
 144 -Wgnu-designator
 147 -Wdisabled-macro-expansion
 150 -Wgnu-conditional-omitted-operand
 161 -Wunreachable-code-break
 184 -Wcompound-token-split-by-space
 228 -Wfloat-conversion
 248 -Wunreachable-code
 348 -Wgnu-binary-literal
 443 -Wshadow
 534 -Wmissing-variable-declarations
 563 -Wshift-sign-overflow
 613 -Wembedded-directive
 620 -Wgnu-zero-variadic-macro-arguments
 742 -Wswitch-enum
 843 -Wdocumentation
 897 -Wgnu-case-range
1292 -Wassign-enum
1621 -Wgnu-empty-struct
1700 -Wextra-semi
1779 -Wpointer-arith
1847 -Wbad-function-cast
2176 -Wdocumentation-unknown-command
2221 -Wmissing-field-initializers
3101 -Wsign-compare
3238 -Wunused-macros
3559 -Wcast-align
4528 -Wcast-qual
7066 -Wgnu-statement-expression
7651 -Wnull-pointer-subtraction
7995 -Wimplicit-int-conversion
8854 -Wpadded
9737 -Wshorten-64-to-32
10596 -Wgnu-empty-initializer
13274 -Wlanguage-extension-token
13899 -Wunused-parameter
15642 -Wused-but-marked-unused
18669 -Wpedantic
44737 -Wsign-conversion

Re: [PATCH for 7.2?] target/i386: Remove compilation errors when -Werror=maybe-uninitialized

[Stefan Weil via]

Am 07.12.22 um 19:22 schrieb Eric Auger:


On 12/7/22 17:55, Philippe Mathieu-Daudé wrote:

On 7/12/22 15:33, Eric Auger wrote:

On 12/7/22 15:09, Stefan Hajnoczi wrote:

On Wed, 7 Dec 2022 at 08:31, Eric Auger  wrote:

On 12/7/22 14:24, Eric Auger wrote:

Initialize r0-3 to avoid compilation errors when
-Werror=maybe-uninitialized is used

../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
   2495 | d->Q(3) = r3;
    | ^~~~
../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
   2494 | d->Q(2) = r2;
    | ^~~~
../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
   2493 | d->Q(1) = r1;
    | ^~~~
../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
   2492 | d->Q(0) = r0;
    | ^~~~

Signed-off-by: Eric Auger 
Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")

---

Am I the only one getting this? Or anything wrong in my setup.


Hi Eric,

no, you are not the only one. I regularly build with higher warning 
levels, for example with -Weverything on macOS, and get a much longer 
list which includes the mentioned warnings (see below).


The warnings for ops_sse.h are false positives, so I think no fix is 
needed for 7.2. The compiler is not clever enough to see that the switch 
statements handle all possible cases. It should be sufficient to replace 
`case 3` by `default` to help the compiler and fix the warning. Your fix 
might produce new compiler warnings because setting the variables to 0 
has no effect.


Cheers
Stefan

../block/mirror.c:1024:13: warning: variable 'iostatus' may be 
uninitialized when used here [-Wconditional-uninitialized]
../block/mirror.c:1498:20: warning: variable 'bounce_buf' may be 
uninitialized when used here [-Wconditional-uninitialized]
../block/nbd.c:1208:24: warning: variable 'request_ret' may be 
uninitialized when used here [-Wconditional-uninitialized]
../block/nbd.c:1266:24: warning: variable 'request_ret' may be 
uninitialized when used here [-Wconditional-uninitialized]
../block/nbd.c:1424:20: warning: variable 'request_ret' may be 
uninitialized when used here [-Wconditional-uninitialized]
../block/qcow2-snapshot.c:423:51: warning: variable 'snapshots_size' may 
be uninitialized when used here [-Wconditional-uninitialized]
../block/qcow2.c:3236:23: warning: variable 'cur_bytes' may be 
uninitialized when used here [-Wconditional-uninitialized]
../block/ssh.c:306:52: warning: variable 'server_hash_len' may be 
uninitialized when used here [-Wconditional-uninitialized]
../block/ssh.c:313:45: warning: variable 'pubkey_type' may be 
uninitialized when used here [-Wconditional-uninitialized]
../contrib/elf2dmp/main.c:138:17: warning: variable 'kwn' may be 
uninitialized when used here [-Wconditional-uninitialized]
../contrib/elf2dmp/main.c:138:22: warning: variable 'kwa' may be 
uninitialized when used here [-Wconditional-uninitialized]
../contrib/elf2dmp/main.c:138:27: warning: variable 
'KdpDataBlockEncoded' may be uninitialized when used here 
[-Wconditional-uninitialized]
../crypto/block-luks.c:844:29: warning: variable 'splitkeylen' may be 
uninitialized when used here [-Wconditional-uninitialized]
../disas/m68k.c:1513:47: warning: variable 'flval' may be uninitialized 
when used here [-Wconditional-uninitialized]
../dump/win_dump.c:105:18: warning: variable 'ptr64' may be 
uninitialized when used here [-Wconditional-uninitialized]
../dump/win_dump.c:105:26: warning: variable 'ptr32' may be 
uninitialized when used here [-Wconditional-uninitialized]
../gdbstub/gdbstub.c:1191:39: warning: variable 'pid' may be 
uninitialized when used here [-Wconditional-uninitialized]
../gdbstub/gdbstub.c:1209:36: warning: variable 'tid' may be 
uninitialized when used here [-Wconditional-uninitialized]
../hw/9pfs/9p.c:1911:13: warning: variable 'fidst' may be uninitialized 
when used here [-Wconditional-uninitialized]
../hw/block/block.c:110:33: warning: variable 'bs' may be uninitialized 
when used here [-Wconditional-uninitialized]
../hw/core/generic-loader.c:160:23: warning: variable 'entry' may be 
uninitialized when used here [-Wconditional-uninitialized]
../hw/i386/intel_iommu.c:323:12: warning: variable 'entry' may be 
uninitialized when used here [-Wconditional-uninitialized]
../hw/ide/ahci.c:968:60: warning: variable 'tbl_entry_size' may be 
uninitialized when used here [-Wconditional-uninitialized]
../hw/microblaze/boot.c:107:42: warning: variable 'fdt_size' may be 
uninitialized when used here [-Wconditional-uninitialized]
../hw/net/rtl8139.c:1801:20: warning: variable 'buf2' may be 
uninitialized when used here [-Wconditional-uninitialized]

Re: [PATCH v3 for-7.2 0/6] Add format attributes and fix format strings

[Stefan Weil via]

Am 27.11.22 um 19:23 schrieb Stefan Hajnoczi:


We need to wait for Michael to agree to maintainership in patch 5. If
we run out of time I suggest splitting out patch 5.

Reviewed-by: Stefan Hajnoczi



Citing Michael from a v2 email: "pls do".

Stefan



Hello Michael,

I just noticed that MAINTAINERS has no entry for the files in
subprojects/libvhost-user, so I did not cc you in my previous e-mails.
Should that directory be added to the "vhost" section"?

Stefan


   pls do

Re: [PATCH v3 for-7.2 4/6] libvhost-user: Add format attribute to local function vu_panic

[Stefan Weil via]

Am 27.11.22 um 19:14 schrieb Stefan Hajnoczi:


On Sat, 26 Nov 2022 at 10:25, Stefan Weil  wrote:

Signed-off-by: Stefan Weil 
Reviewed-by: Marc-André Lureau 
Message-Id: <20220422070144.1043697-4...@weilnetz.de>
Signed-off-by: Laurent Vivier 
---
  subprojects/libvhost-user/libvhost-user.c | 13 -
  1 file changed, 12 insertions(+), 1 deletion(-)

I would rather not merge something that can cause new build failures
this late in the release cycle.

If you respin, please make this a separate 8.0 patch.



It would only cause a build failure if there remained more format bugs, 
but the last ones were fixed in patch 3. I tested a build for 32 bit ARM 
(which previously failed without patch 3), and it works now.


But you are right, patch 4 is not release critical as it is not a bug 
fix (like the other ones), so not including it in 7.2 might be an option.


Stefan

[PATCH v3 for-7.2 4/6] libvhost-user: Add format attribute to local function vu_panic

[Stefan Weil via]

Signed-off-by: Stefan Weil 
Reviewed-by: Marc-André Lureau 
Message-Id: <20220422070144.1043697-4...@weilnetz.de>
Signed-off-by: Laurent Vivier 
---
 subprojects/libvhost-user/libvhost-user.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c
index 80f9952e71..d6ee6e7d91 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -45,6 +45,17 @@
 #include "libvhost-user.h"
 
 /* usually provided by GLib */
+#if __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 4)
+#if !defined(__clang__) && (__GNUC__ == 4 && __GNUC_MINOR__ == 4)
+#define G_GNUC_PRINTF(format_idx, arg_idx) \
+  __attribute__((__format__(gnu_printf, format_idx, arg_idx)))
+#else
+#define G_GNUC_PRINTF(format_idx, arg_idx) \
+  __attribute__((__format__(__printf__, format_idx, arg_idx)))
+#endif
+#else   /* !__GNUC__ */
+#define G_GNUC_PRINTF(format_idx, arg_idx)
+#endif  /* !__GNUC__ */
 #ifndef MIN
 #define MIN(x, y) ({\
 typeof(x) _min1 = (x);  \
@@ -151,7 +162,7 @@ vu_request_to_string(unsigned int req)
 }
 }
 
-static void
+static void G_GNUC_PRINTF(2, 3)
 vu_panic(VuDev *dev, const char *msg, ...)
 {
 char *buf = NULL;
-- 
2.35.1

[PATCH v3 for-7.2 5/6] MAINTAINERS: Add subprojects/libvhost-user to section "vhost"

[Stefan Weil via]

Signed-off-by: Stefan Weil 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index cf24910249..6966490c94 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2005,6 +2005,7 @@ F: docs/interop/vhost-user.rst
 F: contrib/vhost-user-*/
 F: backends/vhost-user.c
 F: include/sysemu/vhost-user-backend.h
+F: subprojects/libvhost-user/
 
 virtio
 M: Michael S. Tsirkin 
-- 
2.35.1

[PATCH v3 for-7.2 6/6] Add G_GNUC_PRINTF to function qemu_set_info_str and fix related issues

[Stefan Weil via]

With the G_GNUC_PRINTF function attribute the compiler detects
two potential insecure format strings:

../../../net/stream.c:248:31: warning: format string is not a string literal 
(potentially insecure) [-Wformat-security]
qemu_set_info_str(>nc, uri);
  ^~~
../../../net/stream.c:322:31: warning: format string is not a string literal 
(potentially insecure) [-Wformat-security]
qemu_set_info_str(>nc, uri);
  ^~~

There are also two other warnings:

../../../net/socket.c:182:35: warning: zero-length gnu_printf format string 
[-Wformat-zero-length]
  182 | qemu_set_info_str(>nc, "");
  |   ^~
../../../net/stream.c:170:35: warning: zero-length gnu_printf format string 
[-Wformat-zero-length]
  170 | qemu_set_info_str(>nc, "");

Reviewed-by: Peter Maydell 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Stefan Weil 
---
 include/net/net.h | 3 ++-
 net/socket.c  | 2 +-
 net/stream.c  | 6 +++---
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/net/net.h b/include/net/net.h
index 3db75ff841..dc20b31e9f 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -177,7 +177,8 @@ ssize_t qemu_send_packet_async(NetClientState *nc, const 
uint8_t *buf,
 void qemu_purge_queued_packets(NetClientState *nc);
 void qemu_flush_queued_packets(NetClientState *nc);
 void qemu_flush_or_purge_queued_packets(NetClientState *nc, bool purge);
-void qemu_set_info_str(NetClientState *nc, const char *fmt, ...);
+void qemu_set_info_str(NetClientState *nc,
+   const char *fmt, ...) G_GNUC_PRINTF(2, 3);
 void qemu_format_nic_info_str(NetClientState *nc, uint8_t macaddr[6]);
 bool qemu_has_ufo(NetClientState *nc);
 bool qemu_has_vnet_hdr(NetClientState *nc);
diff --git a/net/socket.c b/net/socket.c
index 4944bb70d5..e62137c839 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -179,7 +179,7 @@ static void net_socket_send(void *opaque)
 s->fd = -1;
 net_socket_rs_init(>rs, net_socket_rs_finalize, false);
 s->nc.link_down = true;
-qemu_set_info_str(>nc, "");
+qemu_set_info_str(>nc, "%s", "");
 
 return;
 }
diff --git a/net/stream.c b/net/stream.c
index 53b7040cc4..37ff727e0c 100644
--- a/net/stream.c
+++ b/net/stream.c
@@ -167,7 +167,7 @@ static gboolean net_stream_send(QIOChannel *ioc,
 
 net_socket_rs_init(>rs, net_stream_rs_finalize, false);
 s->nc.link_down = true;
-qemu_set_info_str(>nc, "");
+qemu_set_info_str(>nc, "%s", "");
 
 qapi_event_send_netdev_stream_disconnected(s->nc.name);
 
@@ -245,7 +245,7 @@ static void net_stream_listen(QIONetListener *listener,
 }
 g_assert(addr != NULL);
 uri = socket_uri(addr);
-qemu_set_info_str(>nc, uri);
+qemu_set_info_str(>nc, "%s", uri);
 g_free(uri);
 qapi_event_send_netdev_stream_connected(s->nc.name, addr);
 qapi_free_SocketAddress(addr);
@@ -319,7 +319,7 @@ static void net_stream_client_connected(QIOTask *task, 
gpointer opaque)
 addr = qio_channel_socket_get_remote_address(sioc, NULL);
 g_assert(addr != NULL);
 uri = socket_uri(addr);
-qemu_set_info_str(>nc, uri);
+qemu_set_info_str(>nc, "%s", uri);
 g_free(uri);
 
 ret = qemu_socket_try_set_nonblock(sioc->fd);
-- 
2.35.1

[PATCH v3 for-7.2 3/6] libvhost-user: Fix two more format strings

[Stefan Weil via]

This fix is required for 32 bit hosts. The bug was detected by CI
for arm-linux, but is also relevant for i386-linux.

Reported-by: Stefan Hajnoczi 
Signed-off-by: Stefan Weil 
---
 subprojects/libvhost-user/libvhost-user.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c
index d67953a1c3..80f9952e71 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -651,7 +651,8 @@ generate_faults(VuDev *dev) {
 
 if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, _struct)) {
 vu_panic(dev, "%s: Failed to userfault region %d "
-  "@%" PRIx64 " + size:%zx offset: %zx: (ufd=%d)%s\n",
+  "@%" PRIx64 " + size:%" PRIx64 " offset: %" PRIx64
+  ": (ufd=%d)%s\n",
  __func__, i,
  dev_region->mmap_addr,
  dev_region->size, dev_region->mmap_offset,
-- 
2.35.1

[PATCH v3 for-7.2 0/6] Add format attributes and fix format strings

[Stefan Weil via]

v3:
- Fix description for patch 3
- Add patches 5 and 6

The patches 3 and 5 still need reviews!

[PATCH v3 for-7.2 1/6] libvhost-user: Fix wrong type of argument to
[PATCH v3 for-7.2 2/6] libvhost-user: Fix format strings
[PATCH v3 for-7.2 3/6] libvhost-user: Fix two more format strings
[PATCH v3 for-7.2 4/6] libvhost-user: Add format attribute to local
[PATCH v3 for-7.2 5/6] MAINTAINERS: Add subprojects/libvhost-user to
[PATCH v3 for-7.2 6/6] Add G_GNUC_PRINTF to function

[PATCH v3 for-7.2 2/6] libvhost-user: Fix format strings

[Stefan Weil via]

Signed-off-by: Stefan Weil 
Reviewed-by: Marc-André Lureau 
Message-Id: <20220422070144.1043697-3...@weilnetz.de>
Signed-off-by: Laurent Vivier 
---
 subprojects/libvhost-user/libvhost-user.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c
index d9a6e3e556..d67953a1c3 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -700,7 +700,7 @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
 if (vmsg->size < VHOST_USER_MEM_REG_SIZE) {
 close(vmsg->fds[0]);
 vu_panic(dev, "VHOST_USER_ADD_MEM_REG requires a message size of at "
-  "least %d bytes and only %d bytes were received",
+  "least %zu bytes and only %d bytes were received",
   VHOST_USER_MEM_REG_SIZE, vmsg->size);
 return false;
 }
@@ -826,7 +826,7 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
 if (vmsg->size < VHOST_USER_MEM_REG_SIZE) {
 vmsg_close_fds(vmsg);
 vu_panic(dev, "VHOST_USER_REM_MEM_REG requires a message size of at "
-  "least %d bytes and only %d bytes were received",
+  "least %zu bytes and only %d bytes were received",
   VHOST_USER_MEM_REG_SIZE, vmsg->size);
 return false;
 }
-- 
2.35.1

[PATCH v3 for-7.2 1/6] libvhost-user: Fix wrong type of argument to formatting function (reported by LGTM)

[Stefan Weil via]

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Stefan Weil 
Message-Id: <20220422070144.1043697-2...@weilnetz.de>
Signed-off-by: Laurent Vivier 
---
 subprojects/libvhost-user/libvhost-user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c
index ffed4729a3..d9a6e3e556 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -651,7 +651,7 @@ generate_faults(VuDev *dev) {
 
 if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, _struct)) {
 vu_panic(dev, "%s: Failed to userfault region %d "
-  "@%p + size:%zx offset: %zx: (ufd=%d)%s\n",
+  "@%" PRIx64 " + size:%zx offset: %zx: (ufd=%d)%s\n",
  __func__, i,
  dev_region->mmap_addr,
  dev_region->size, dev_region->mmap_offset,
-- 
2.35.1

Re: [PATCH for 7.2-rc? v2 0/5] continuing efforts to fix vhost-user issues

[Stefan Weil via]

Am 25.11.22 um 18:30 schrieb Alex Bennée:

Hi,

This is continuing to attempt to fix the various vhost-user issues
that are currently plaguing the release. One concrete bug I've come
across is that all qtest MMIO devices where being treated as legacy
which caused the VIRTIO_F_VERSION_1 flag to get missed causing s390x
to fall back to trying to set the endian value for the virt-queues.


Do you want to add my 4 commits which fix format strings for 
libvhost-user to your series, or should they be handled separately?


Regards
Stefan

Re: [PATCH for-7.2 v2 3/4] libvhost-user: Fix two more format strings

[Stefan Weil via]

Am 23.11.22 um 07:35 schrieb Stefan Weil:

Am 15.11.22 um 08:25 schrieb Stefan Weil:

Am 05.11.22 um 11:24 schrieb Stefan Weil:


This fix is required for 32 bit host. The bug was detected by CI
for arm-linux, but is also relevant for i386-linux.

Reported-by: Stefan Hajnoczi 
Signed-off-by: Stefan Weil 
---
  subprojects/libvhost-user/libvhost-user.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c

index d67953a1c3..80f9952e71 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -651,7 +651,8 @@ generate_faults(VuDev *dev) {
  if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, _struct)) {
  vu_panic(dev, "%s: Failed to userfault region %d "
-  "@%" PRIx64 " + size:%zx offset: %zx: 
(ufd=%d)%s\n",
+  "@%" PRIx64 " + size:%" PRIx64 " offset: 
%" PRIx64

+  ": (ufd=%d)%s\n",
   __func__, i,
   dev_region->mmap_addr,
   dev_region->size, dev_region->mmap_offset,



There is still no review for this patch.

I added "for-7.2" to the subject here in my answer. How can we get all 
4 patches of this series into the new release?


Stefan


Ping? Those bug fixes are still missing in -rc2.

Stefan


Hello Michael,

I just noticed that MAINTAINERS has no entry for the files in 
subprojects/libvhost-user, so I did not cc you in my previous e-mails. 
Should that directory be added to the "vhost" section"?


Stefan

Re: [PATCH for-7.2 v2 3/4] libvhost-user: Fix two more format strings

[Stefan Weil via]

Am 15.11.22 um 08:25 schrieb Stefan Weil:

Am 05.11.22 um 11:24 schrieb Stefan Weil:


This fix is required for 32 bit host. The bug was detected by CI
for arm-linux, but is also relevant for i386-linux.

Reported-by: Stefan Hajnoczi 
Signed-off-by: Stefan Weil 
---
  subprojects/libvhost-user/libvhost-user.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c

index d67953a1c3..80f9952e71 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -651,7 +651,8 @@ generate_faults(VuDev *dev) {
  if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, _struct)) {
  vu_panic(dev, "%s: Failed to userfault region %d "
-  "@%" PRIx64 " + size:%zx offset: %zx: 
(ufd=%d)%s\n",
+  "@%" PRIx64 " + size:%" PRIx64 " offset: %" 
PRIx64

+  ": (ufd=%d)%s\n",
   __func__, i,
   dev_region->mmap_addr,
   dev_region->size, dev_region->mmap_offset,



There is still no review for this patch.

I added "for-7.2" to the subject here in my answer. How can we get all 4 
patches of this series into the new release?


Stefan


Ping? Those bug fixes are still missing in -rc2.

Stefan


OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH for-7.2 v2 3/4] libvhost-user: Fix two more format strings

[Stefan Weil via]

Am 05.11.22 um 11:24 schrieb Stefan Weil:


This fix is required for 32 bit host. The bug was detected by CI
for arm-linux, but is also relevant for i386-linux.

Reported-by: Stefan Hajnoczi 
Signed-off-by: Stefan Weil 
---
  subprojects/libvhost-user/libvhost-user.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c
index d67953a1c3..80f9952e71 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -651,7 +651,8 @@ generate_faults(VuDev *dev) {
  
  if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, _struct)) {

  vu_panic(dev, "%s: Failed to userfault region %d "
-  "@%" PRIx64 " + size:%zx offset: %zx: (ufd=%d)%s\n",
+  "@%" PRIx64 " + size:%" PRIx64 " offset: %" PRIx64
+  ": (ufd=%d)%s\n",
   __func__, i,
   dev_region->mmap_addr,
   dev_region->size, dev_region->mmap_offset,



There is still no review for this patch.

I added "for-7.2" to the subject here in my answer. How can we get all 4 
patches of this series into the new release?


Stefan

[PATCH for-7.2] Add G_GNUC_PRINTF to function qemu_set_info_str and fix related issues

[Stefan Weil via]

With the G_GNUC_PRINTF function attribute the compiler detects
two potential insecure format strings:

../../../net/stream.c:248:31: warning: format string is not a string literal 
(potentially insecure) [-Wformat-security]
qemu_set_info_str(>nc, uri);
  ^~~
../../../net/stream.c:322:31: warning: format string is not a string literal 
(potentially insecure) [-Wformat-security]
qemu_set_info_str(>nc, uri);
  ^~~

There are also two other warnings:

../../../net/socket.c:182:35: warning: zero-length gnu_printf format string 
[-Wformat-zero-length]
  182 | qemu_set_info_str(>nc, "");
  |   ^~
../../../net/stream.c:170:35: warning: zero-length gnu_printf format string 
[-Wformat-zero-length]
  170 | qemu_set_info_str(>nc, "");

Signed-off-by: Stefan Weil 
---
 include/net/net.h | 3 ++-
 net/socket.c  | 2 +-
 net/stream.c  | 6 +++---
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/net/net.h b/include/net/net.h
index 3db75ff841..dc20b31e9f 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -177,7 +177,8 @@ ssize_t qemu_send_packet_async(NetClientState *nc, const 
uint8_t *buf,
 void qemu_purge_queued_packets(NetClientState *nc);
 void qemu_flush_queued_packets(NetClientState *nc);
 void qemu_flush_or_purge_queued_packets(NetClientState *nc, bool purge);
-void qemu_set_info_str(NetClientState *nc, const char *fmt, ...);
+void qemu_set_info_str(NetClientState *nc,
+   const char *fmt, ...) G_GNUC_PRINTF(2, 3);
 void qemu_format_nic_info_str(NetClientState *nc, uint8_t macaddr[6]);
 bool qemu_has_ufo(NetClientState *nc);
 bool qemu_has_vnet_hdr(NetClientState *nc);
diff --git a/net/socket.c b/net/socket.c
index 4944bb70d5..e62137c839 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -179,7 +179,7 @@ static void net_socket_send(void *opaque)
 s->fd = -1;
 net_socket_rs_init(>rs, net_socket_rs_finalize, false);
 s->nc.link_down = true;
-qemu_set_info_str(>nc, "");
+qemu_set_info_str(>nc, "%s", "");
 
 return;
 }
diff --git a/net/stream.c b/net/stream.c
index 53b7040cc4..37ff727e0c 100644
--- a/net/stream.c
+++ b/net/stream.c
@@ -167,7 +167,7 @@ static gboolean net_stream_send(QIOChannel *ioc,
 
 net_socket_rs_init(>rs, net_stream_rs_finalize, false);
 s->nc.link_down = true;
-qemu_set_info_str(>nc, "");
+qemu_set_info_str(>nc, "%s", "");
 
 qapi_event_send_netdev_stream_disconnected(s->nc.name);
 
@@ -245,7 +245,7 @@ static void net_stream_listen(QIONetListener *listener,
 }
 g_assert(addr != NULL);
 uri = socket_uri(addr);
-qemu_set_info_str(>nc, uri);
+qemu_set_info_str(>nc, "%s", uri);
 g_free(uri);
 qapi_event_send_netdev_stream_connected(s->nc.name, addr);
 qapi_free_SocketAddress(addr);
@@ -319,7 +319,7 @@ static void net_stream_client_connected(QIOTask *task, 
gpointer opaque)
 addr = qio_channel_socket_get_remote_address(sioc, NULL);
 g_assert(addr != NULL);
 uri = socket_uri(addr);
-qemu_set_info_str(>nc, uri);
+qemu_set_info_str(>nc, "%s", uri);
 g_free(uri);
 
 ret = qemu_socket_try_set_nonblock(sioc->fd);
-- 
2.30.2

Re: [PATCH] s390x: Fix spelling errors

[Stefan Weil via]

_i64();
  
-/* Multipy both even elements from v2 and v3 */

+/* Multiply both even elements from v2 and v3 */
  read_vec_element_i64(l1, get_field(s, v2), 0, ES_64);
  read_vec_element_i64(h1, get_field(s, v3), 0, ES_64);
  tcg_gen_mulu2_i64(l1, h1, l1, h1);
@@ -2084,7 +2084,7 @@ static DisasJumpType op_vmsl(DisasContext *s, DisasOps *o)
  tcg_gen_add2_i64(l1, h1, l1, h1, l1, h1);
  }
  
-/* Multipy both odd elements from v2 and v3 */

+/* Multiply both odd elements from v2 and v3 */
  read_vec_element_i64(l2, get_field(s, v2), 1, ES_64);
  read_vec_element_i64(h2, get_field(s, v3), 1, ES_64);
  tcg_gen_mulu2_i64(l2, h2, l2, h2);
diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index 4d5ad21653..6072906df4 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -19,7 +19,7 @@ _start:
larl %r2, __bss_start
larl %r3, _end
slgr %r3, %r2   /* get sizeof bss */
-   ltgr%r3,%r3 /* bss emtpy? */
+   ltgr%r3,%r3 /* bss empty? */
jz  done
    aghi%r3,-1
srlg%r4,%r3,8   /* how many 256 byte chunks? */



Thanks.

Reviewed-by: Stefan Weil 




OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

[RFC: PATCH v2] Add new build targets 'check-spelling' and 'check-spelling-docs'

[Stefan Weil via]

`make check-spelling` can now be used to get a list of spelling errors.
It uses the latest version of codespell, a spell checker implemented in Python.

'make check-spelling-docs' checks the generated documentation.

Signed-off-by: Stefan Weil 
---

v2: Additional target check-spelling-docs, updated ignore-words

This RFC can already be used for manual tests, but still reports false
positives, mostly because some variable names are interpreted as words.
These words can either be ignored in the check, or in some cases the code
might be changed to use different variable names.

The check currently only skips a few directories and files, so for example
checked out submodules are also checked.

The rule can be extended to allow user provided ignore and skip lists,
for example by introducing Makefile variables CODESPELL_SKIP=userfile
or CODESPELL_IGNORE=userfile. A limited check could be implemented by
providing a base directory CODESPELL_START=basedirectory, for example
CODESPELL_START=docs.

After fixing some typos (patch was just sent to the list), these
"typos" remain in the generated documentation:

SUMMARY:
crypted   3
ede   4
inflight 32
informations  3
mor   1
vas   1

Regards,
Stefan

 tests/Makefile.include   | 19 +++
 tests/codespell/README.rst   | 18 ++
 tests/codespell/exclude-file |  3 +++
 tests/codespell/ignore-words | 21 +
 tests/requirements.txt   |  1 +
 5 files changed, 62 insertions(+)
 create mode 100644 tests/codespell/README.rst
 create mode 100644 tests/codespell/exclude-file
 create mode 100644 tests/codespell/ignore-words

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 9422ddaece..66424c2eac 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -155,6 +155,25 @@ check-acceptance-deprecated-warning:
 
 check-acceptance: check-acceptance-deprecated-warning | check-avocado
 
+CODESPELL_DIR=$(SRC_PATH)/tests/codespell
+
+.PHONY: check-spelling
+check-spelling: check-venv
+   source $(TESTS_VENV_DIR)/bin/activate && \
+   cd "$(SRC_PATH)" && \
+   codespell -s . \
+ --exclude-file=$(CODESPELL_DIR)/exclude-file \
+ --ignore-words=$(CODESPELL_DIR)/ignore-words \
+ --skip="./.git,./bin,./build,./linux-headers,./roms,*.patch,nohup.out"
+
+.PHONY: check-spelling-docs
+check-spelling-docs: check-venv
+   source $(TESTS_VENV_DIR)/bin/activate && \
+   codespell -s docs \
+ --exclude-file=$(CODESPELL_DIR)/exclude-file \
+ --ignore-words=$(CODESPELL_DIR)/ignore-words \
+ --skip="docs/manual/_static,docs/manual/searchindex.js,nohup.out"
+
 # Consolidated targets
 
 .PHONY: check check-clean get-vm-images
diff --git a/tests/codespell/README.rst b/tests/codespell/README.rst
new file mode 100644
index 00..67e070d631
--- /dev/null
+++ b/tests/codespell/README.rst
@@ -0,0 +1,18 @@
+=
+Check spelling with codespell
+=
+
+`make check-spelling` can be used to get a list of spelling errors.
+It reports files with spelling errors and a summary of all misspelled words.
+The report is generated by the latest version of codespell, a spell checker
+implemented in Python.
+
+See https://github.com/codespell-project/codespell for more information.
+
+Some file patterns are excluded from the check.
+
+In addition tests/codespell includes several files which are used to
+suppress certain false positives in the codespell report.
+
+exclude-file - complete lines which should be ignored
+ignore-words - list of words which should be ignored
diff --git a/tests/codespell/exclude-file b/tests/codespell/exclude-file
new file mode 100644
index 00..57de81a4eb
--- /dev/null
+++ b/tests/codespell/exclude-file
@@ -0,0 +1,3 @@
+ * VAS controller.
+number generator daemon such as the one found in the vhost-device crate of
+introspection.  The latter can conceivably confuse clients, so tread
diff --git a/tests/codespell/ignore-words b/tests/codespell/ignore-words
new file mode 100644
index 00..0deb4cc65f
--- /dev/null
+++ b/tests/codespell/ignore-words
@@ -0,0 +1,21 @@
+asign
+bu
+buid
+busses
+conectix
+dout
+falt
+fpr
+hace
+hax
+hda
+nd
+ot
+pard
+parm
+ptd
+ser
+som
+synopsys
+te
+ue
diff --git a/tests/requirements.txt b/tests/requirements.txt
index 0ba561b6bd..dd44e6768f 100644
--- a/tests/requirements.txt
+++ b/tests/requirements.txt
@@ -4,3 +4,4 @@
 # Note that qemu.git/python/ is always implicitly installed.
 avocado-framework==88.1
 pycdlib==1.11.0
+codespell
-- 
2.30.2

[PATCH for-7.2] Fix several typos in documentation (found by codespell)

[Stefan Weil via]

Those typos are in files which are used to generate the QEMU manual.

Signed-off-by: Stefan Weil 
---

I did not fix memory_region_init_resizeable_ram. That might be done after 7.2.

Stefan

 docs/devel/acpi-bits.rst   | 2 +-
 docs/system/devices/can.rst| 2 +-
 hw/scsi/esp.c  | 6 +++---
 include/exec/memory.h  | 6 +++---
 qapi/virtio.json   | 4 ++--
 qemu-options.hx| 6 +++---
 tests/qtest/libqos/qgraph.h| 2 +-
 tests/qtest/libqos/virtio-9p.c | 2 +-
 8 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/docs/devel/acpi-bits.rst b/docs/devel/acpi-bits.rst
index c9564d871a..5e22be8ef6 100644
--- a/docs/devel/acpi-bits.rst
+++ b/docs/devel/acpi-bits.rst
@@ -132,7 +132,7 @@ Under ``tests/avocado/`` as the root we have:
 
(a) They are python2.7 based scripts and not python 3 scripts.
(b) They are run from within the bios bits VM and is not subjected to QEMU
-   build/test python script maintainance and dependency resolutions.
+   build/test python script maintenance and dependency resolutions.
(c) They need not be loaded by avocado framework when running tests.
 
 
diff --git a/docs/system/devices/can.rst b/docs/system/devices/can.rst
index fe37af8223..24b0d4cf41 100644
--- a/docs/system/devices/can.rst
+++ b/docs/system/devices/can.rst
@@ -169,7 +169,7 @@ and with bitrate switch::
 
   cangen can0 -b
 
-The test can be run viceversa, generate messages in the guest system and 
capture them
+The test can be run vice-versa, generate messages in the guest system and 
capture them
 in the host one and much more combinations.
 
 Links to other resources
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index e5b281e836..e52188d022 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -515,7 +515,7 @@ static void do_dma_pdma_cb(ESPState *s)
 } else {
 /*
  * Extra message out bytes received: update cmdfifo_cdb_offset
- * and then switch to commmand phase
+ * and then switch to command phase
  */
 s->cmdfifo_cdb_offset = fifo8_num_used(>cmdfifo);
 s->rregs[ESP_RSTAT] = STAT_TC | STAT_CD;
@@ -627,7 +627,7 @@ static void esp_do_dma(ESPState *s)
 } else {
 /*
  * Extra message out bytes received: update cmdfifo_cdb_offset
- * and then switch to commmand phase
+ * and then switch to command phase
  */
 s->cmdfifo_cdb_offset = fifo8_num_used(>cmdfifo);
 s->rregs[ESP_RSTAT] = STAT_TC | STAT_CD;
@@ -738,7 +738,7 @@ static void esp_do_nodma(ESPState *s)
 } else {
 /*
  * Extra message out bytes received: update cmdfifo_cdb_offset
- * and then switch to commmand phase
+ * and then switch to command phase
  */
 s->cmdfifo_cdb_offset = fifo8_num_used(>cmdfifo);
 s->rregs[ESP_RSTAT] = STAT_TC | STAT_CD;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 80fa75baa1..91f8a2395a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -561,7 +561,7 @@ typedef void (*ReplayRamDiscard)(MemoryRegionSection 
*section, void *opaque);
  * A #RamDiscardManager coordinates which parts of specific RAM #MemoryRegion
  * regions are currently populated to be used/accessed by the VM, notifying
  * after parts were discarded (freeing up memory) and before parts will be
- * populated (consuming memory), to be used/acessed by the VM.
+ * populated (consuming memory), to be used/accessed by the VM.
  *
  * A #RamDiscardManager can only be set for a RAM #MemoryRegion while the
  * #MemoryRegion isn't mapped yet; it cannot change while the #MemoryRegion is
@@ -585,7 +585,7 @@ typedef void (*ReplayRamDiscard)(MemoryRegionSection 
*section, void *opaque);
  * Listeners are called in multiples of the minimum granularity (unless it
  * would exceed the registered range) and changes are aligned to the minimum
  * granularity within the #MemoryRegion. Listeners have to prepare for memory
- * becomming discarded in a different granularity than it was populated and the
+ * becoming discarded in a different granularity than it was populated and the
  * other way around.
  */
 struct RamDiscardManagerClass {
@@ -1247,7 +1247,7 @@ void memory_region_init_ram_flags_nomigrate(MemoryRegion 
*mr,
 Error **errp);
 
 /**
- * memory_region_init_resizeable_ram:  Initialize memory region with resizeable
+ * memory_region_init_resizeable_ram:  Initialize memory region with resizable
  * RAM.  Accesses into the region will
  * modify memory directly.  Only an initial
  * portion of this RAM is actually used.
diff --git a/qapi/virtio.json b/qapi/virtio.json
index 872c7e3623..019d2d1987 100644
--- a/qapi/virtio.json
+++ b

Re: [PATCH trivial for 7.2 1/2] hw/usb/hcd-xhci.c: spelling: tranfer

[Stefan Weil via]

Am 05.11.22 um 22:24 schrieb Michael Tokarev:


05.11.2022 15:23, Stefan Weil via wrote:
..

All typos from this series were also found by codespell.

See https://qemu.weilnetz.de/test/typos7 for many more.
That list was produced with `make check-spelling` from
my previous patch).


Yeah, codespell is a good thing. But qemu has just TOO MANY typos, and
non-typos too (eg addd). I only patched a few places which are visible
in the binaries.

/mjt



More typos which are visible in binaries like firwmare, Changeing, 
Unknow, accomodate, migrateable, facilties, ... can be found with `git 
grep '"' | codespell -s -.


Stefan

Re: [PATCH trivial for 7.2 1/2] hw/usb/hcd-xhci.c: spelling: tranfer

[Stefan Weil via]

Am 05.11.22 um 12:57 schrieb Stefan Weil via:

Am 05.11.22 um 12:48 schrieb Michael Tokarev:

Fixes: effaf5a240e03020f4ae953e10b764622c3e87cc
Signed-off-by: Michael Tokarev 
---
  hw/usb/hcd-xhci.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


All typos from this series were also found by codespell.

See https://qemu.weilnetz.de/test/typos7 for many more.
That list was produced with `make check-spelling` from
my previous patch).

Stefan



OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH trivial for 7.2] hw/ssi/sifive_spi.c: spelling: reigster

[Stefan Weil via]

Am 05.11.22 um 12:53 schrieb Michael Tokarev:

Fixes: 0694dabe9763847f3010b54ab3ec7d367d2f0ff0
Signed-off-by: Michael Tokarev 
---
  hw/ssi/sifive_spi.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ssi/sifive_spi.c b/hw/ssi/sifive_spi.c
index 03540cf5ca..1b4a401ca1 100644
--- a/hw/ssi/sifive_spi.c
+++ b/hw/ssi/sifive_spi.c
@@ -267,7 +267,7 @@ static void sifive_spi_write(void *opaque, hwaddr addr,
  case R_RXDATA:
  case R_IP:
  qemu_log_mask(LOG_GUEST_ERROR,
-  "%s: invalid write to read-only reigster 0x%"
+  "%s: invalid write to read-only register 0x%"
HWADDR_PRIx " with 0x%x\n", __func__, addr << 2, value);
  break;
  


Reviewed-by: Stefan Weil 


OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH trivial for 7.2 2/2] hw/virtio/virtio.c: spelling: suppoted

[Stefan Weil via]

Am 05.11.22 um 12:48 schrieb Michael Tokarev:

Fixes: f3034ad71fcd0a6a58bc37830f182b307f089159
Signed-off-by: Michael Tokarev 
---
  hw/virtio/virtio.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 808446b4c9..e76218bdd5 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -340,7 +340,7 @@ qmp_virtio_feature_map_t virtio_net_feature_map[] = {
  qmp_virtio_feature_map_t virtio_scsi_feature_map[] = {
  FEATURE_ENTRY(VIRTIO_SCSI_F_INOUT, \
  "VIRTIO_SCSI_F_INOUT: Requests including read and writable data "
-"buffers suppoted"),
+"buffers supported"),
  FEATURE_ENTRY(VIRTIO_SCSI_F_HOTPLUG, \
  "VIRTIO_SCSI_F_HOTPLUG: Reporting and handling hot-plug events "
      "supported"),


Reviewed-by: Stefan Weil 


OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH trivial for 7.2 1/2] hw/usb/hcd-xhci.c: spelling: tranfer

[Stefan Weil via]

Am 05.11.22 um 12:48 schrieb Michael Tokarev:

Fixes: effaf5a240e03020f4ae953e10b764622c3e87cc
Signed-off-by: Michael Tokarev 
---
  hw/usb/hcd-xhci.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 8299f35e66..b89b618ec2 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -796,7 +796,7 @@ static int xhci_ring_chain_length(XHCIState *xhci, const 
XHCIRing *ring)
   */
  } while (length < TRB_LINK_LIMIT * 65536 / TRB_SIZE);
  
-qemu_log_mask(LOG_GUEST_ERROR, "%s: exceeded maximum tranfer ring size!\n",

+qemu_log_mask(LOG_GUEST_ERROR, "%s: exceeded maximum transfer ring 
size!\n",
__func__);
  
  return -1;


Reviewed-by: Stefan Weil 


OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

[PATCH for-7.2] tests/qtest: Fix two format strings

[Stefan Weil via]

Signed-off-by: Stefan Weil 
---
 tests/qtest/migration-test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index d2eb107f0c..f574331b7b 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2188,7 +2188,7 @@ static void calc_dirty_rate(QTestState *who, uint64_t 
calc_time)
 qobject_unref(qmp_command(who,
   "{ 'execute': 'calc-dirty-rate',"
   "'arguments': { "
-  "'calc-time': %ld,"
+  "'calc-time': %" PRIu64 ","
   "'mode': 'dirty-ring' }}",
   calc_time));
 }
@@ -2203,7 +2203,7 @@ static void dirtylimit_set_all(QTestState *who, uint64_t 
dirtyrate)
 qobject_unref(qmp_command(who,
   "{ 'execute': 'set-vcpu-dirty-limit',"
   "'arguments': { "
-  "'dirty-rate': %ld } }",
+  "'dirty-rate': %" PRIu64 " } }",
   dirtyrate));
 }
 
-- 
2.30.2

[PATCH for-7.2] accel/tcg: Suppress compiler warning with flag -Wclobbered

[Stefan Weil via]

At least some versions of gcc show a warning when compiler flag -Wclobbered
is used (tested with gcc on Debian bookworm i386 and with cross gcc for
Windows on Debian bullseye).

Signed-off-by: Stefan Weil 
---
 accel/tcg/translate-all.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 921944a5ab..90191d97ec 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -743,6 +743,8 @@ void page_collection_unlock(struct page_collection *set)
 #endif /* !CONFIG_USER_ONLY */
 
 /* Called with mmap_lock held for user mode emulation.  */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wclobbered"
 TranslationBlock *tb_gen_code(CPUState *cpu,
   target_ulong pc, target_ulong cs_base,
   uint32_t flags, int cflags)
@@ -1020,6 +1022,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
 }
 return tb;
 }
+#pragma GCC diagnostic pop
 
 /* user-mode: call with mmap_lock held */
 void tb_check_watchpoint(CPUState *cpu, uintptr_t retaddr)
-- 
2.30.2

Re: [PATCH v2 3/4] libvhost-user: Fix two more format strings

[Stefan Weil via]

Am 05.11.22 um 11:24 schrieb Stefan Weil via:


This fix is required for 32 bit host. The bug was detected by CI
for arm-linux, but is also relevant for i386-linux.



s/host/hosts/

I won't send a v3 for that. Maybe it can be fixed when merging this patch.

Stefan

[PATCH v2 for-7.2 0/4] libvhost-user: Add format attribute and fix format strings

[Stefan Weil via]

v2: Add patch 3 to fix two more format strings before applying patch 4

[PATCH v2 1/4] libvhost-user: Fix wrong type of argument to
[PATCH v2 2/4] libvhost-user: Fix format strings
[PATCH v2 3/4] libvhost-user: Fix two more format strings
[PATCH v2 4/4] libvhost-user: Add format attribute to local function

[PATCH v2 1/4] libvhost-user: Fix wrong type of argument to formatting function (reported by LGTM)

[Stefan Weil via]

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Stefan Weil 
Message-Id: <20220422070144.1043697-2...@weilnetz.de>
Signed-off-by: Laurent Vivier 
---
 subprojects/libvhost-user/libvhost-user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c
index ffed4729a3..d9a6e3e556 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -651,7 +651,7 @@ generate_faults(VuDev *dev) {
 
 if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, _struct)) {
 vu_panic(dev, "%s: Failed to userfault region %d "
-  "@%p + size:%zx offset: %zx: (ufd=%d)%s\n",
+  "@%" PRIx64 " + size:%zx offset: %zx: (ufd=%d)%s\n",
  __func__, i,
  dev_region->mmap_addr,
  dev_region->size, dev_region->mmap_offset,
-- 
2.30.2

[PATCH v2 4/4] libvhost-user: Add format attribute to local function vu_panic

[Stefan Weil via]

Signed-off-by: Stefan Weil 
Reviewed-by: Marc-André Lureau 
Message-Id: <20220422070144.1043697-4...@weilnetz.de>
Signed-off-by: Laurent Vivier 
---
 subprojects/libvhost-user/libvhost-user.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c
index 80f9952e71..d6ee6e7d91 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -45,6 +45,17 @@
 #include "libvhost-user.h"
 
 /* usually provided by GLib */
+#if __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 4)
+#if !defined(__clang__) && (__GNUC__ == 4 && __GNUC_MINOR__ == 4)
+#define G_GNUC_PRINTF(format_idx, arg_idx) \
+  __attribute__((__format__(gnu_printf, format_idx, arg_idx)))
+#else
+#define G_GNUC_PRINTF(format_idx, arg_idx) \
+  __attribute__((__format__(__printf__, format_idx, arg_idx)))
+#endif
+#else   /* !__GNUC__ */
+#define G_GNUC_PRINTF(format_idx, arg_idx)
+#endif  /* !__GNUC__ */
 #ifndef MIN
 #define MIN(x, y) ({\
 typeof(x) _min1 = (x);  \
@@ -151,7 +162,7 @@ vu_request_to_string(unsigned int req)
 }
 }
 
-static void
+static void G_GNUC_PRINTF(2, 3)
 vu_panic(VuDev *dev, const char *msg, ...)
 {
 char *buf = NULL;
-- 
2.30.2

[PATCH v2 2/4] libvhost-user: Fix format strings

[Stefan Weil via]

Signed-off-by: Stefan Weil 
Reviewed-by: Marc-André Lureau 
Message-Id: <20220422070144.1043697-3...@weilnetz.de>
Signed-off-by: Laurent Vivier 
---
 subprojects/libvhost-user/libvhost-user.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c
index d9a6e3e556..d67953a1c3 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -700,7 +700,7 @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
 if (vmsg->size < VHOST_USER_MEM_REG_SIZE) {
 close(vmsg->fds[0]);
 vu_panic(dev, "VHOST_USER_ADD_MEM_REG requires a message size of at "
-  "least %d bytes and only %d bytes were received",
+  "least %zu bytes and only %d bytes were received",
   VHOST_USER_MEM_REG_SIZE, vmsg->size);
 return false;
 }
@@ -826,7 +826,7 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
 if (vmsg->size < VHOST_USER_MEM_REG_SIZE) {
 vmsg_close_fds(vmsg);
 vu_panic(dev, "VHOST_USER_REM_MEM_REG requires a message size of at "
-  "least %d bytes and only %d bytes were received",
+  "least %zu bytes and only %d bytes were received",
   VHOST_USER_MEM_REG_SIZE, vmsg->size);
 return false;
 }
-- 
2.30.2

[PATCH v2 3/4] libvhost-user: Fix two more format strings

[Stefan Weil via]

This fix is required for 32 bit host. The bug was detected by CI
for arm-linux, but is also relevant for i386-linux.

Reported-by: Stefan Hajnoczi 
Signed-off-by: Stefan Weil 
---
 subprojects/libvhost-user/libvhost-user.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c
index d67953a1c3..80f9952e71 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -651,7 +651,8 @@ generate_faults(VuDev *dev) {
 
 if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, _struct)) {
 vu_panic(dev, "%s: Failed to userfault region %d "
-  "@%" PRIx64 " + size:%zx offset: %zx: (ufd=%d)%s\n",
+  "@%" PRIx64 " + size:%" PRIx64 " offset: %" PRIx64
+  ": (ufd=%d)%s\n",
  __func__, i,
  dev_region->mmap_addr,
  dev_region->size, dev_region->mmap_offset,
-- 
2.30.2

Re: [PULL 04/10] libvhost-user: Fix wrong type of argument to formatting function (reported by LGTM)

[Stefan Weil via]

Am 04.11.22 um 17:16 schrieb Laurent Vivier:


Hi Stefan,

Le 03/11/2022 à 17:17, Laurent Vivier a écrit :

From: Stefan Weil 

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Stefan Weil 
Message-Id: <20220422070144.1043697-2...@weilnetz.de>
Signed-off-by: Laurent Vivier 
---
  subprojects/libvhost-user/libvhost-user.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c

index ffed4729a3dc..d9a6e3e5560f 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -651,7 +651,7 @@ generate_faults(VuDev *dev) {
    if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, 
_struct)) {

  vu_panic(dev, "%s: Failed to userfault region %d "
-  "@%p + size:%zx offset: %zx: (ufd=%d)%s\n",
+  "@%" PRIx64 " + size:%zx offset: %zx: 
(ufd=%d)%s\n",

   __func__, i,
   dev_region->mmap_addr,
   dev_region->size, dev_region->mmap_offset,


They all need PRIx64:

typedef struct VuDevRegion {
    /* Guest Physical address. */
    uint64_t gpa;
    /* Memory region size. */
    uint64_t size;
    /* QEMU virtual address (userspace). */
    uint64_t qva;
    /* Starting offset in our mmaped space. */
    uint64_t mmap_offset;
    /* Start address of mmaped space. */
    uint64_t mmap_addr;
} VuDevRegion;

Could you fix your patch?



The patch fixes one error ("%p"). The two "%zx" are old errors which I 
did not notice because they are only relevant for platforms with 
sizeof(void *) != sizeof(uint64_t), and 32 bit Windows builds don't 
compile libvhost-user. So we need an additional patch which fixes the 
"%zx" before patch 06/10 which adds the format attribute is applied.


Stefan, I suggest to merge the trivial branch without patch 06/10. That 
should fix the build failure and fixes at least some of the format 
errors. Then a patch which fixes the remaining format errors can be 
applied later together with the omitted patch 06/10.


Regards,

Stefan

Re: [PATCH] meson: avoid unused arguments of main() in compiler tests

[Stefan Weil via]

Am 03.11.22 um 18:21 schrieb Paolo Bonzini:


meson.build has one test where "main" is declared unnecessarily
with argc and argv arguments, but does not use them.  Because
the test needs -Werror too, HAVE_BROKEN_SIZE_MAX is defined
incorrectly.

Fix the test and, for consistency, remove argc and argv whenever
they are not needed.

Signed-off-by: Paolo Bonzini 
---



Reviewed-by: Stefan Weil 

Thanks, Stefan



  meson.build | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/meson.build b/meson.build
index 17834b3c3def..7beeac6b5194 100644
--- a/meson.build
+++ b/meson.build
@@ -2143,7 +2143,7 @@ config_host_data.set('CONFIG_SPLICE', 
cc.links(gnu_source_prefix + '''
  
  config_host_data.set('HAVE_MLOCKALL', cc.links(gnu_source_prefix + '''

#include 
-  int main(int argc, char *argv[]) {
+  int main(void) {
  return mlockall(MCL_FUTURE);
}'''))
  
@@ -2188,7 +2188,7 @@ config_host_data.set('HAVE_FSXATTR', cc.links('''

  config_host_data.set('HAVE_BROKEN_SIZE_MAX', not cc.compiles('''
  #include 
  #include 
-int main(int argc, char *argv[]) {
+int main(void) {
  return printf("%zu", SIZE_MAX);
  }''', args: ['-Werror']))
  
@@ -2305,7 +2305,7 @@ config_host_data.set('CONFIG_AVX2_OPT', get_option('avx2') \

__m256i x = *(__m256i *)a;
return _mm256_testz_si256(x, x);
  }
-int main(int argc, char *argv[]) { return bar(argv[0]); }
+int main(int argc, char *argv[]) { return bar(argv[argc - 1]); }
'''), error_message: 'AVX2 not available').allowed())
  
  config_host_data.set('CONFIG_AVX512F_OPT', get_option('avx512f') \

@@ -2319,7 +2319,7 @@ config_host_data.set('CONFIG_AVX512F_OPT', 
get_option('avx512f') \
__m512i x = *(__m512i *)a;
return _mm512_test_epi64_mask(x, x);
  }
-int main(int argc, char *argv[]) { return bar(argv[0]); }
+int main(int argc, char *argv[]) { return bar(argv[argc - 1]); }
'''), error_message: 'AVX512F not available').allowed())
  
  have_pvrdma = get_option('pvrdma') \


OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH for 7.2] Fix broken configure with -Wunused-parameter

[Stefan Weil via]

Am 03.11.22 um 12:48 schrieb Peter Maydell:


On Wed, 2 Nov 2022 at 20:24, Stefan Weil via  wrote:

The configure script fails because it tries to compile small C programs
with a main function which is declared with arguments argc and argv
although those arguments are unused.

Running `configure -extra-cflags=-Wunused-parameter` triggers the problem.
configure for a native build does abort but shows the error in config.log.
A cross build configure for Windows with Debian stable aborts with an
error.

Avoiding unused arguments fixes this.

Signed-off-by: Stefan Weil 
---

See https://gitlab.com/qemu-project/qemu/-/issues/1295.

I noticed the problem because I often compile with -Wextra.

Stefan

  configure | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 4275f5419f..1106c04fea 100755
--- a/configure
+++ b/configure
@@ -1258,6 +1258,7 @@ if test "$stack_protector" != "no"; then
cat > $TMPC << EOF
  int main(int argc, char *argv[])
  {
+(void)argc;

I'm not a huge fan of this syntax, and it doesn't match the way
we deal with "argument is unused" elsewhere in the codebase
(where we either don't care about it or else use the GCC 'unused'
attribute hidden behind the glib G_GNUC_UNUSED macro).



Any other variant is also fine for me, for example "using" argc by a 
"return argc == 0;" instead of "return 0;". Would that be better? If 
there is an accepted variant, I can either send a v2 patch, or maybe 
such a trivial change can be applied when merging.




I am surprised that this didn't get caught by the check in
do_compiler_werror(), which is supposed to report "this
configure test passed without -Werror but failed with
-Werror, so configure is probably buggy.". That's what's
supposed to catch "your compiler warns on stuff our doesn't
in the test case programs".

If you're building with --disable-werror then configure
should be OK anyway. This is probably a good idea if you want
to build with extra warning arguments in --extra-cflags.
If it doesn't work right even with --disable-werror that's
also something we should investigate.



Cross builds for Windows fail with and without --disable-werror. See 
also my bug report https://gitlab.com/qemu-project/qemu/-/issues/1295.


You are right that this is strange and should be investigated, 
especially because native builds don't fail like that.




  char arr[64], *p = arr, *c = argv[0];
  while (*c) {
  *p++ = *c++;
@@ -1607,7 +1608,7 @@ fi

  if test "$safe_stack" = "yes"; then
  cat > $TMPC << EOF
-int main(int argc, char *argv[])
+int main(void)
  {
  #if ! __has_feature(safe_stack)
  #error SafeStack Disabled
@@ -1629,7 +1630,7 @@ EOF
fi
  else
  cat > $TMPC << EOF
-int main(int argc, char *argv[])
+int main(void)
  {
  #if defined(__has_feature)
  #if __has_feature(safe_stack)
@@ -1675,7 +1676,7 @@ static const int Z = 1;
  #define TAUT(X) ((X) == Z)
  #define PAREN(X, Y) (X == Y)
  #define ID(X) (X)
-int main(int argc, char *argv[])
+int main(void)
  {
  int x = 0, y = 0;
  x = ID(x);

No objection to the cases where we can pass "void", that's
a neater way to write the test anyway.

thanks
-- PMM



OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH for 7.2] Fix broken configure with -Wunused-parameter

[Stefan Weil via]

Am 03.11.22 um 09:58 schrieb Daniel P. Berrangé:


On Wed, Nov 02, 2022 at 09:22:58PM +0100, Stefan Weil via wrote:

The configure script fails because it tries to compile small C programs
with a main function which is declared with arguments argc and argv
although those arguments are unused.

Running `configure -extra-cflags=-Wunused-parameter` triggers the problem.
configure for a native build does abort but shows the error in config.log.
A cross build configure for Windows with Debian stable aborts with an
error.

Avoiding unused arguments fixes this.

I'm not convinced that we should allow -extra-cflags to influence
the configure compile checks at all, as there are likely more cases
where arbitrary -W$warn flag will impact the checks, potentially
causing configure to silently take the wrong action.



I partially agree, but configure should fail if invalid -extra-cflags 
are specified, and the checks must also respect additional include paths 
given by -extra-cflags of course.


And I think that the changes in my patch are an improvement in any case.

Stefan



OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

[PATCH] Add missing include statement for global xml_builtin

[Stefan Weil via]

This fixes some compiler warnings with compiler flag
-Wmissing-variable-declarations (tested with clang):

aarch64_be-linux-user-gdbstub-xml.c:564:19: warning: no previous extern 
declaration for non-static variable 'xml_builtin' 
[-Wmissing-variable-declarations]
aarch64-linux-user-gdbstub-xml.c:564:19: warning: no previous extern 
declaration for non-static variable 'xml_builtin' 
[-Wmissing-variable-declarations]
aarch64-softmmu-gdbstub-xml.c:1763:19: warning: no previous extern 
declaration for non-static variable 'xml_builtin' 
[-Wmissing-variable-declarations]

Signed-off-by: Stefan Weil 
---
 scripts/feature_to_c.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/feature_to_c.sh b/scripts/feature_to_c.sh
index b1169899c1..c1f67c8f6a 100644
--- a/scripts/feature_to_c.sh
+++ b/scripts/feature_to_c.sh
@@ -56,6 +56,7 @@ for input; do
 done
 
 echo
+echo '#include "exec/gdbstub.h"'
 echo "const char *const xml_builtin[][2] = {"
 
 for input; do
-- 
2.30.2

Re: [PATCH] Run docker probe only if docker or podman are available

[Stefan Weil via]

Am 30.10.22 um 09:35 schrieb Stefan Weil:


The docker probe uses "sudo -n" which can cause an e-mail with a security 
warning
each time when configure is run. Therefore run docker probe only if either 
docker
or podman are available.

That avoids the problematic "sudo -n" on build environments which have neither
docker nor podman installed.

Fixes: c4575b59155e2e00 ("configure: store container engine in config-host.mak")
Signed-off-by: Stefan Weil 
---
  configure | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 81561be7c1..3af99282b7 100755
--- a/configure
+++ b/configure
@@ -1779,7 +1779,7 @@ fi
  # functions to probe cross compilers
  
  container="no"

-if test $use_containers = "yes"; then
+if test $use_containers = "yes" && (has "docker" || has "podman"); then
  case $($python "$source_path"/tests/docker/docker.py probe) in
  *docker) container=docker ;;
  podman) container=podman ;;



Can this patch be applied by qemu-trivial? For me those security e-mails 
are a bug and should be at least avoided as far as possible in the new 
QEMU release.


Thanks, Stefan

[PATCH for 7.2] Fix broken configure with -Wunused-parameter

[Stefan Weil via]

The configure script fails because it tries to compile small C programs
with a main function which is declared with arguments argc and argv
although those arguments are unused.

Running `configure -extra-cflags=-Wunused-parameter` triggers the problem.
configure for a native build does abort but shows the error in config.log.
A cross build configure for Windows with Debian stable aborts with an
error.

Avoiding unused arguments fixes this.

Signed-off-by: Stefan Weil 
---

See https://gitlab.com/qemu-project/qemu/-/issues/1295.

I noticed the problem because I often compile with -Wextra.

Stefan

 configure | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 4275f5419f..1106c04fea 100755
--- a/configure
+++ b/configure
@@ -1258,6 +1258,7 @@ if test "$stack_protector" != "no"; then
   cat > $TMPC << EOF
 int main(int argc, char *argv[])
 {
+(void)argc;
 char arr[64], *p = arr, *c = argv[0];
 while (*c) {
 *p++ = *c++;
@@ -1607,7 +1608,7 @@ fi
 
 if test "$safe_stack" = "yes"; then
 cat > $TMPC << EOF
-int main(int argc, char *argv[])
+int main(void)
 {
 #if ! __has_feature(safe_stack)
 #error SafeStack Disabled
@@ -1629,7 +1630,7 @@ EOF
   fi
 else
 cat > $TMPC << EOF
-int main(int argc, char *argv[])
+int main(void)
 {
 #if defined(__has_feature)
 #if __has_feature(safe_stack)
@@ -1675,7 +1676,7 @@ static const int Z = 1;
 #define TAUT(X) ((X) == Z)
 #define PAREN(X, Y) (X == Y)
 #define ID(X) (X)
-int main(int argc, char *argv[])
+int main(void)
 {
 int x = 0, y = 0;
 x = ID(x);
-- 
2.30.2

Re: [PATCH 4/5] disas/nanomips: Remove headers already included by "qemu/osdep.h"

[Stefan Weil via]

Am 01.11.22 um 12:44 schrieb Philippe Mathieu-Daudé:

Signed-off-by: Philippe Mathieu-Daudé 
---
  disas/nanomips.c | 4 
  1 file changed, 4 deletions(-)

diff --git a/disas/nanomips.c b/disas/nanomips.c
index 3f45447292..821d4f8832 100644
--- a/disas/nanomips.c
+++ b/disas/nanomips.c
@@ -30,10 +30,6 @@
  #include "qemu/osdep.h"
  #include "disas/dis-asm.h"
  
-#include 

-#include 
-#include 
-
  typedef int64_t int64;
  typedef uint64_t uint64;
  typedef uint32_t uint32;


Removing those three typedefs and replacing the related types would also 
be good (in another patch).


Reviewed-by: Stefan Weil 



OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH 3/5] disas/nanomips: Use G_GNUC_PRINTF to avoid invalid string formats

[Stefan Weil via]

Am 01.11.22 um 12:44 schrieb Philippe Mathieu-Daudé:


Suggested-by: Stefan Weil 
Signed-off-by: Philippe Mathieu-Daudé 
---
  disas/nanomips.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/disas/nanomips.c b/disas/nanomips.c
index e4b21e7c45..3f45447292 100644
--- a/disas/nanomips.c
+++ b/disas/nanomips.c
@@ -95,7 +95,7 @@ typedef struct Pool {
  #define IMGASSERTONCE(test)
  
  
-static char *img_format(const char *format, ...)

+static char * G_GNUC_PRINTF(1, 2) img_format(const char *format, ...)
  {
  char *buffer;
  va_list args;



Reviewed-by: Stefan Weil 



OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH 2/5] disas/nanomips: Fix invalid PRIx64 format calling img_format()

[Stefan Weil via]

Am 01.11.22 um 12:44 schrieb Philippe Mathieu-Daudé:


Fix:

   disas/nanomips.c:12231:62: warning: format specifies type 'char *' but the 
argument has type 'uint64' (aka 'unsigned long long') [-Wformat]
 return img_format("RESTOREF 0x%" PRIx64 ", %s", u_value, count_value);
~~^~~
%llu

Fixes: 4066c152b3 ("disas/nanomips: Remove IMMEDIATE functions")
Reported-by: Stefan Weil 
Signed-off-by: Philippe Mathieu-Daudé 
---
  disas/nanomips.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/disas/nanomips.c b/disas/nanomips.c
index 6466c80dc5..e4b21e7c45 100644
--- a/disas/nanomips.c
+++ b/disas/nanomips.c
@@ -12235,7 +12235,8 @@ static char *RESTOREF(uint64 instruction, Dis_info 
*info)
  uint64 u_value = extract_u_11_10_9_8_7_6_5_4_3__s3(instruction);
  
  
-return img_format("RESTOREF 0x%" PRIx64 ", %s", u_value, count_value);

+return img_format("RESTOREF 0x%" PRIx64 ", 0x%" PRIx64,
+  u_value, count_value);
  }



Reviewed-by: Stefan Weil 

Re: [PATCH 1/5] disas/nanomips: Fix invalid PRId64 format calling img_format()

[Stefan Weil via]

Am 01.11.22 um 12:44 schrieb Philippe Mathieu-Daudé:


Fix warnings such:

   disas/nanomips.c:3251:64: warning: format specifies type 'char *' but the 
argument has type 'int64' (aka 'long long') [-Wformat]
 return img_format("CACHE 0x%" PRIx64 ", %s(%s)", op_value, s_value, rs);
 ~~ ^~~
 %lld

Fixes: 4066c152b3 ("disas/nanomips: Remove IMMEDIATE functions")
Reported-by: Stefan Weil 
Signed-off-by: Philippe Mathieu-Daudé 
---



Reviewed-by: Stefan Weil 



  disas/nanomips.c | 35 ---
  1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/disas/nanomips.c b/disas/nanomips.c
index 9647f1a8e3..6466c80dc5 100644
--- a/disas/nanomips.c
+++ b/disas/nanomips.c
@@ -3252,7 +3252,8 @@ static char *CACHE(uint64 instruction, Dis_info *info)
  
  const char *rs = GPR(rs_value, info);
  
-return img_format("CACHE 0x%" PRIx64 ", %s(%s)", op_value, s_value, rs);

+return img_format("CACHE 0x%" PRIx64 ", %" PRId64 "(%s)",
+  op_value, s_value, rs);
  }
  
  
@@ -3274,7 +3275,8 @@ static char *CACHEE(uint64 instruction, Dis_info *info)
  
  const char *rs = GPR(rs_value, info);
  
-return img_format("CACHEE 0x%" PRIx64 ", %s(%s)", op_value, s_value, rs);

+return img_format("CACHEE 0x%" PRIx64 ", %" PRId64 "(%s)",
+  op_value, s_value, rs);
  }
  
  
@@ -5173,7 +5175,7 @@ static char *DADDIU_48_(uint64 instruction, Dis_info *info)
  
  const char *rt = GPR(rt_value, info);
  
-return img_format("DADDIU %s, %s", rt, s_value);

+return img_format("DADDIU %s, %" PRId64, rt, s_value);
  }
  
  
@@ -11859,7 +11861,7 @@ static char *PREF_S9_(uint64 instruction, Dis_info *info)
  
  const char *rs = GPR(rs_value, info);
  
-return img_format("PREF 0x%" PRIx64 ", %s(%s)",

+return img_format("PREF 0x%" PRIx64 ", %" PRId64 "(%s)",
hint_value, s_value, rs);
  }
  
@@ -11905,7 +11907,8 @@ static char *PREFE(uint64 instruction, Dis_info *info)
  
  const char *rs = GPR(rs_value, info);
  
-return img_format("PREFE 0x%" PRIx64 ", %s(%s)", hint_value, s_value, rs);

+return img_format("PREFE 0x%" PRIx64 ", %" PRId64 "(%s)",
+  hint_value, s_value, rs);
  }
  
  
@@ -12079,7 +12082,7 @@ static char *REPL_PH(uint64 instruction, Dis_info *info)
  
  const char *rt = GPR(rt_value, info);
  
-return img_format("REPL.PH %s, %s", rt, s_value);

+return img_format("REPL.PH %s, %" PRId64, rt, s_value);
  }
  
  
@@ -12613,7 +12616,7 @@ static char *SB_S9_(uint64 instruction, Dis_info *info)

  const char *rt = GPR(rt_value, info);
  const char *rs = GPR(rs_value, info);
  
-return img_format("SB %s, %s(%s)", rt, s_value, rs);

+return img_format("SB %s, %" PRId64 "(%s)", rt, s_value, rs);
  }
  
  
@@ -12659,7 +12662,7 @@ static char *SBE(uint64 instruction, Dis_info *info)

  const char *rt = GPR(rt_value, info);
  const char *rs = GPR(rs_value, info);
  
-return img_format("SBE %s, %s(%s)", rt, s_value, rs);

+return img_format("SBE %s, %" PRId64 "(%s)", rt, s_value, rs);
  }
  
  
@@ -12706,7 +12709,7 @@ static char *SC(uint64 instruction, Dis_info *info)

  const char *rt = GPR(rt_value, info);
  const char *rs = GPR(rs_value, info);
  
-return img_format("SC %s, %s(%s)", rt, s_value, rs);

+return img_format("SC %s, %" PRId64 "(%s)", rt, s_value, rs);
  }
  
  
@@ -12729,7 +12732,7 @@ static char *SCD(uint64 instruction, Dis_info *info)

  const char *rt = GPR(rt_value, info);
  const char *rs = GPR(rs_value, info);
  
-return img_format("SCD %s, %s(%s)", rt, s_value, rs);

+return img_format("SCD %s, %" PRId64 "(%s)", rt, s_value, rs);
  }
  
  
@@ -12776,7 +12779,7 @@ static char *SCE(uint64 instruction, Dis_info *info)

  const char *rt = GPR(rt_value, info);
  const char *rs = GPR(rs_value, info);
  
-return img_format("SCE %s, %s(%s)", rt, s_value, rs);

+return img_format("SCE %s, %" PRId64 "(%s)", rt, s_value, rs);
  }
  
  
@@ -12868,7 +12871,7 @@ static char *SD_S9_(uint64 instruction, Dis_info *info)

  const char *rt = GPR(rt_value, info);
  const char *rs = GPR(rs_value, info);
  
-return img_format("SD %s, %s(%s)", rt, s_value, rs);

+return img_format("SD %s, %" PRId64 "(%s)", rt, s_value, rs);
  }
  
  
@@ -12973,7 +12976,7 @@ static char *SDC1_S9_(uint64 instruction, 

Re: [PATCH v3 16/24] disas/nanomips: Remove IMMEDIATE functions

[Stefan Weil via]

Am 01.11.22 um 10:27 schrieb Philippe Mathieu-Daudé:


On 1/11/22 09:28, Stefan Weil via wrote:

Am 12.09.22 um 14:26 schrieb Milica Lazarevic:

Both versions of IMMEDIATE functions have been removed.

Before this patch, we'd been calling img_format twice, the first time
through the IMMEDIATE to get an appropriate string and the second time
to print that string. There's no more need for that. Therefore, 
calls to

IMMEDIATE are removed, and now we're directly printing the integer
values instead.

Signed-off-by: Milica Lazarevic 
---
  disas/nanomips.cpp | 756 
-

  1 file changed, 265 insertions(+), 491 deletions(-)

diff --git a/disas/nanomips.cpp b/disas/nanomips.cpp
index 816155527d..441204bb84 100644
--- a/disas/nanomips.cpp
+++ b/disas/nanomips.cpp

[...]
@@ -3305,11 +3271,9 @@ static char *CACHE(uint64 instruction, 
Dis_info *info)

  uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
  int64 s_value = extract_s__se8_15_7_6_5_4_3_2_1_0(instruction);
-    char *op = IMMEDIATE(op_value);
-    char *s = IMMEDIATE(s_value);
  const char *rs = GPR(rs_value);
-    return img_format("CACHE %s, %s(%s)", op, s, rs);
+    return img_format("CACHE 0x%" PRIx64 ", %s(%s)", op_value, 
s_value, rs);

  }
@@ -3329,11 +3293,9 @@ static char *CACHEE(uint64 instruction, 
Dis_info *info)

  uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
  int64 s_value = extract_s__se8_15_7_6_5_4_3_2_1_0(instruction);
-    char *op = IMMEDIATE(op_value);
-    char *s = IMMEDIATE(s_value);
  const char *rs = GPR(rs_value);
-    return img_format("CACHEE %s, %s(%s)", op, s, rs);
+    return img_format("CACHEE 0x%" PRIx64 ", %s(%s)", op_value, 
s_value, rs);

  }


Do we really want to format "int64 s_value" as a string? The code now 
has lots of wrong format strings. Add the patch below to get the 
compiler report.


We once had a discussion about using G_GNUC_PRINTF for local 
functions or not. I think that this example clearly shows that it 
should be mandatory.


Yes. The problem here is nobody wants to maintain this code, but we
inherited it. IIUC this series doesn't make it worst, it just remove
the C++ dependency on UNIX-based hosts.



I expect that "%s" with an int64 s_value will cause a crash while the 
old code worked, so things are worse now and should be fixed for the 
release. If nobody maintains that code, I can try to prepare a patch.


Stefan

Re: [PATCH v3 16/24] disas/nanomips: Remove IMMEDIATE functions

[Stefan Weil via]

Am 12.09.22 um 14:26 schrieb Milica Lazarevic:

Both versions of IMMEDIATE functions have been removed.

Before this patch, we'd been calling img_format twice, the first time
through the IMMEDIATE to get an appropriate string and the second time
to print that string. There's no more need for that. Therefore, calls to
IMMEDIATE are removed, and now we're directly printing the integer
values instead.

Signed-off-by: Milica Lazarevic 
---
  disas/nanomips.cpp | 756 -
  1 file changed, 265 insertions(+), 491 deletions(-)

diff --git a/disas/nanomips.cpp b/disas/nanomips.cpp
index 816155527d..441204bb84 100644
--- a/disas/nanomips.cpp
+++ b/disas/nanomips.cpp

[...]

@@ -3305,11 +3271,9 @@ static char *CACHE(uint64 instruction, Dis_info *info)
  uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
  int64 s_value = extract_s__se8_15_7_6_5_4_3_2_1_0(instruction);
  
-char *op = IMMEDIATE(op_value);

-char *s = IMMEDIATE(s_value);
  const char *rs = GPR(rs_value);
  
-return img_format("CACHE %s, %s(%s)", op, s, rs);

+return img_format("CACHE 0x%" PRIx64 ", %s(%s)", op_value, s_value, rs);
  }
  
  
@@ -3329,11 +3293,9 @@ static char *CACHEE(uint64 instruction, Dis_info *info)

  uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
  int64 s_value = extract_s__se8_15_7_6_5_4_3_2_1_0(instruction);
  
-char *op = IMMEDIATE(op_value);

-char *s = IMMEDIATE(s_value);
  const char *rs = GPR(rs_value);
  
-return img_format("CACHEE %s, %s(%s)", op, s, rs);

+return img_format("CACHEE 0x%" PRIx64 ", %s(%s)", op_value, s_value, rs);
  }


Do we really want to format "int64 s_value" as a string? The code now 
has lots of wrong format strings. Add the patch below to get the 
compiler report.


We once had a discussion about using G_GNUC_PRINTF for local functions 
or not. I think that this example clearly shows that it should be mandatory.


Regards,
Stefan

diff --git a/disas/nanomips.c b/disas/nanomips.c
index 9647f1a8e3..c875818cb9 100644
--- a/disas/nanomips.c
+++ b/disas/nanomips.c
@@ -95,7 +95,7 @@ typedef struct Pool {
 #define IMGASSERTONCE(test)


-static char *img_format(const char *format, ...)
+static char * G_GNUC_PRINTF(1, 2) img_format(const char *format, ...)
 {
 char *buffer;
 va_list args;



OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [RFC PATCH] Add new build target 'check-spelling'

[Stefan Weil via]

Below I added some examples for the words which are currently ignored by 
codespell.


Am 31.10.22 um 16:40 schrieb Philippe Mathieu-Daudé:


On 31/10/22 08:43, Stefan Weil via wrote:

`make check-spelling` can now be used to get a list of spelling errors.
It uses the latest version of codespell, a spell checker implemented 
in Python.


Signed-off-by: Stefan Weil 
---

This RFC can already be used for manual tests, but still reports false
positives, mostly because some variable names are interpreted as words.
These words can either be ignored in the check, or in some cases the 
code

might be changed to use different variable names.

The check currently only skips a few directories and files, so for 
example

checked out submodules are also checked.

The rule can be extended to allow user provided ignore and skip lists,
for example by introducing Makefile variables CODESPELL_SKIP=userfile
or CODESPELL_IGNORE=userfile. A limited check could be implemented by
providing a base directory CODESPELL_START=basedirectory, for example
CODESPELL_START=docs.

Regards,
Stefan

  tests/Makefile.include   | 10 ++
  tests/codespell/README.rst   | 18 ++
  tests/codespell/exclude-file |  3 +++
  tests/codespell/ignore-words | 19 +++
  tests/requirements.txt   |  1 +
  5 files changed, 51 insertions(+)
  create mode 100644 tests/codespell/README.rst
  create mode 100644 tests/codespell/exclude-file
  create mode 100644 tests/codespell/ignore-words


Just wondering about this list...


+++ b/tests/codespell/ignore-words
@@ -0,0 +1,19 @@
+buid


What is 'buid'? PPC-specific apparently.


hw/ppc/spapr_pci.c:SpaprPhbState *spapr_pci_find_phb(SpaprMachineState 
*spapr, uint64_t buid)
include/hw/ppc/xics.h: * We currently only support one BUID which is our 
interrupt base

[...]



+busses
+dout
+falt
+fpr
+hace
+hax
+hda
+nd


Apparently 'NIC info'...

hw/arm/aspeed.c:    NICInfo *nd = _table[0];
hw/display/macfb.c:    NubusDevice *nd = NUBUS_DEVICE(s);
[...]



+ot


Is 'ot' MemOp?


target/i386/tcg/decode-new.c.inc:static bool decode_op_size(DisasContext 
*s, X86OpEntry *e, X86OpSize size, MemOp *ot)

[...]



+pard
+parm
+ptd
+ser
+som
+synopsys
+te


Is that 'target endianness'?


accel/tcg/cputlb.c: * @te: pointer to CPUTLBEntry
hw/audio/cs4231a.c:#define TE (1 << 6)
[...]



+toke


Where is 'toke'?


This one is no longer needed. It was used in the old capstone code which 
I still had in my local sources.




+ue

Where is 'ue'?

tests/tcg/i386/test-i386-fp-exceptions.c:#define UE (1 << 4)
tests/unit/test-keyval.c:    qdict = keyval_parse("val,,ue", "implied", 
NULL, );

[...]

I simply had added some examples of "words" which occurred often and 
which were reported by codespell as typos. These "typos" occur at least 
10 times (list produced with `grep "^[a-z]" codespell.log | sort -n +1`):


statics  10
regiser  11
usig 11
inh  12
tne  12
overriden    13
inactivate   15
upto 15
hsa  16
useable  17
daa  18
crate    21
endianess    22
olt  22
sring    23
vill 25
keypairs 35
gir  46
sav  47
asign   120
inflight    191

Some of them are real typos, others like aSign or statics are variable 
names and should be ignored, too.


Stefan

Re: [PATCH v2] Fix some typos in documentation and comments

[Stefan Weil via]

Am 31.10.22 um 08:35 schrieb Thomas Huth:


On 30/10/2022 11.59, Stefan Weil wrote:

Most of them were found and fixed using codespell.

Signed-off-by: Stefan Weil 
---

v2: Fixes from Peter Maydell's comments

My focus was fixing typos which are relevant for the generated 
documentation.


codespell finds many more typos in source code, and adding it to the 
continuous

integration checks looks more and more like a good idea.


... at least for the docs/ folder, this might indeed be a good idea.

Reviewed-by: Thomas Huth 



See also "Reviewed-by: Stefan Hajnoczi " for the 
first version of this patch.


Maybe the pull request can be made by qemu-trivial?

Thanks,

Stefan




OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH] Add nsis.py to W32/W64 section in MAINTAINERS

[Stefan Weil via]

Am 31.10.22 um 11:28 schrieb Philippe Mathieu-Daudé:


On 31/10/22 10:57, Stefan Weil via wrote:

Signed-off-by: Stefan Weil 
---
  MAINTAINERS | 1 +
  1 file changed, 1 insertion(+)


Reviewed-by: Philippe Mathieu-Daudé 



Cc qemu-trivial

Re: [RFC PATCH] Add new build target 'check-spelling'

[Stefan Weil via]

Am 31.10.22 um 08:52 schrieb Thomas Huth:


On 31/10/2022 08.43, Stefan Weil wrote:

`make check-spelling` can now be used to get a list of spelling errors.
It uses the latest version of codespell, a spell checker implemented 
in Python.


Signed-off-by: Stefan Weil 
---

This RFC can already be used for manual tests, but still reports false
positives, mostly because some variable names are interpreted as words.
These words can either be ignored in the check, or in some cases the 
code

might be changed to use different variable names.

The check currently only skips a few directories and files, so for 
example

checked out submodules are also checked.

The rule can be extended to allow user provided ignore and skip lists,
for example by introducing Makefile variables CODESPELL_SKIP=userfile
or CODESPELL_IGNORE=userfile. A limited check could be implemented by
providing a base directory CODESPELL_START=basedirectory, for example
CODESPELL_START=docs.

Regards,
Stefan

[...]
I like the idea, but I think it's unlikely that we can make this work 
for the whole source tree any time soon. So maybe it makes more sense 
to start with some few directories first (e.g. docs/ ) and then the 
maintainers can opt-in by cleaning up their directories first and 
then by adding their directories to this target here?


 Thomas



Even without implementing CODESPELL_START as described above, the script 
can already be used and integrated into CI scripts.


It takes about 60 seconds to check the whole source tree including 
submodules on my (slow) virtual machine.


The resulting output has about 2 lines or 1272 KiB. It can be 
filtered for relevant parts of the source tree or used for a summary.


Sample script: grep "^[.]" spellcheck.log | sed s/^..// | sed 's/\/.*//' 
| sed s/:.*// | sort | uniq -c


This produces a summary for the top level hierarchy of files and 
directories:


  3 accel
  1 audio
  1 backends
 77 block
  7 block.c
 20 bsd-user
    386 capstone
 12 chardev
  1 configure
  8 contrib
  6 crypto
 64 disas
 32 docs
 31 dtc
  8 fpu
  1 gdbstub
  1 gdb-xml
  1 .github
    537 hw
  7 inc
    114 include
  1 libdecnumber
 33 linux-user
  1 MAINTAINERS
    150 meson
  6 meson.build
 16 migration
  1 nbd
  5 net
 12 pc-bios
  7 python
  3 qapi
  2 qemu
  5 qemu-options.hx
 22 qga
  14175 roms
 43 scripts
  3 semihosting
 18 slirp
  2 softmmu
 59 subprojects
    504 target
  6 tcg
  3 test.rb
    175 tests
  6 tools
 20 ui
  8 util

It shows that "roms" contributes by far the most typos. Omitting it 
would reduce the required time to 22 seconds and the number of typos 
found (2947 lines in output) very much.


"capstone" (which has no entry in MAINTAINERS), "target" and "hw" also 
contribute more than 300 hits each, therefore cc'ing Richard.


Stefan



OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

[PATCH] Add nsis.py to W32/W64 section in MAINTAINERS

[Stefan Weil via]

Signed-off-by: Stefan Weil 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 64893e36bc..534b1b8a63 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -548,6 +548,7 @@ F: */*win32*
 F: include/*/*win32*
 X: qga/*win32*
 F: qemu.nsi
+F: scripts/nsis.py
 
 Darwin (macOS, iOS)
 M: Philippe Mathieu-Daudé 
-- 
2.30.2