[PATCH 2/2] hw/virtio: Extract QMP QOM-specific functions to virtio-qmp.c

[Philippe Mathieu-Daudé]

virtio.c is big enough, extract more QMP related code to virtio-qmp.c.
To do so, expose qmp_find_virtio_device() and declar virtio_list in
the internal virtio-qmp.h header.

Note we have to leave qmp_x_query_virtio_queue_status() and
qmp_x_query_virtio_queue_element(), because they access VirtQueue
internal fields, and VirtQueue is only declared within virtio.c.

Suggested-by: Jonah Palmer 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/virtio/virtio-qmp.c | 192 -
 hw/virtio/virtio-qmp.h |   9 ++
 hw/virtio/virtio.c | 191 +---
 3 files changed, 201 insertions(+), 191 deletions(-)

diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index 8e7282658f..e4d4bece2d 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -10,9 +10,14 @@
  */
 
 #include "qemu/osdep.h"
-#include "hw/virtio/virtio.h"
 #include "virtio-qmp.h"
 
+#include "qapi/error.h"
+#include "qapi/qapi-commands-virtio.h"
+#include "qapi/qapi-commands-qom.h"
+#include "qapi/qmp/qobject.h"
+#include "qapi/qmp/qjson.h"
+
 #include "standard-headers/linux/virtio_ids.h"
 #include "standard-headers/linux/vhost_types.h"
 #include "standard-headers/linux/virtio_blk.h"
@@ -657,3 +662,188 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t 
device_id, uint64_t bitmap)
 
 return features;
 }
+
+VirtioInfoList *qmp_x_query_virtio(Error **errp)
+{
+VirtioInfoList *list = NULL;
+VirtioInfoList *node;
+VirtIODevice *vdev;
+
+QTAILQ_FOREACH(vdev, &virtio_list, next) {
+DeviceState *dev = DEVICE(vdev);
+Error *err = NULL;
+QObject *obj = qmp_qom_get(dev->canonical_path, "realized", &err);
+
+if (err == NULL) {
+GString *is_realized = qobject_to_json_pretty(obj, true);
+/* virtio device is NOT realized, remove it from list */
+if (!strncmp(is_realized->str, "false", 4)) {
+QTAILQ_REMOVE(&virtio_list, vdev, next);
+} else {
+node = g_new0(VirtioInfoList, 1);
+node->value = g_new(VirtioInfo, 1);
+node->value->path = g_strdup(dev->canonical_path);
+node->value->name = g_strdup(vdev->name);
+QAPI_LIST_PREPEND(list, node->value);
+}
+   g_string_free(is_realized, true);
+}
+qobject_unref(obj);
+}
+
+return list;
+}
+
+VirtIODevice *qmp_find_virtio_device(const char *path)
+{
+VirtIODevice *vdev;
+
+QTAILQ_FOREACH(vdev, &virtio_list, next) {
+DeviceState *dev = DEVICE(vdev);
+
+if (strcmp(dev->canonical_path, path) != 0) {
+continue;
+}
+
+Error *err = NULL;
+QObject *obj = qmp_qom_get(dev->canonical_path, "realized", &err);
+if (err == NULL) {
+GString *is_realized = qobject_to_json_pretty(obj, true);
+/* virtio device is NOT realized, remove it from list */
+if (!strncmp(is_realized->str, "false", 4)) {
+g_string_free(is_realized, true);
+qobject_unref(obj);
+QTAILQ_REMOVE(&virtio_list, vdev, next);
+return NULL;
+}
+g_string_free(is_realized, true);
+} else {
+/* virtio device doesn't exist in QOM tree */
+QTAILQ_REMOVE(&virtio_list, vdev, next);
+qobject_unref(obj);
+return NULL;
+}
+/* device exists in QOM tree & is realized */
+qobject_unref(obj);
+return vdev;
+}
+return NULL;
+}
+
+VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
+{
+VirtIODevice *vdev;
+VirtioStatus *status;
+
+vdev = qmp_find_virtio_device(path);
+if (vdev == NULL) {
+error_setg(errp, "Path %s is not a VirtIODevice", path);
+return NULL;
+}
+
+status = g_new0(VirtioStatus, 1);
+status->name = g_strdup(vdev->name);
+status->device_id = vdev->device_id;
+status->vhost_started = vdev->vhost_started;
+status->guest_features = qmp_decode_features(vdev->device_id,
+ vdev->guest_features);
+status->host_features = qmp_decode_features(vdev->device_id,
+vdev->host_features);
+status->backend_features = qmp_decode_features(vdev->device_id,
+   vdev->backend_features);
+
+switch (vdev->device_endian) {
+case VIRTIO_DEVICE_ENDIAN_LITTLE:
+status->device_endian = g_strdup("little");
+break;
+case VIRTIO_DEVICE_ENDIAN_BIG:
+status->device_endian = g_strdup("big");
+break;
+default:
+status->device_endian = g_strdup("unknown");
+break;
+}
+
+status->num_vqs = virtio_get_num_queues(vdev);
+status->status = qmp_decode_status(vdev->status);
+status->isr = vdev->isr;
+status->queue_

[PATCH 0/2] hw/virtio: Extract QMP QOM-specific functions to virtio-qmp.c

[Philippe Mathieu-Daudé]

Move more QMP code from virtio.c, as suggested by Jonah while
reviewing "hw/virtio: Split ioconfig / qmp code from virtio.c"
(https://lore.kernel.org/qemu-devel/20221213111707.34921-1-phi...@linaro.org/).

Philippe Mathieu-Daudé (2):
  hw/virtio: Rename virtio_device_find() -> qmp_find_virtio_device()
  hw/virtio: Extract QMP QOM-specific functions to virtio-qmp.c

 hw/virtio/virtio-qmp.c | 192 +++-
 hw/virtio/virtio-qmp.h |   9 ++
 hw/virtio/virtio.c | 195 +
 3 files changed, 203 insertions(+), 193 deletions(-)

-- 
2.38.1

Re: [PATCH v3 4/5] coroutine: Split qemu/coroutine-core.h off qemu/coroutine.h

[Paolo Bonzini]

On 12/21/22 14:14, Markus Armbruster wrote:

+/**
+ * Mark a function that executes in coroutine context
+ *
+ *
+ * Functions that execute in coroutine context cannot be called
+ * directly from normal functions.  Use @coroutine_fn to mark such
+ * functions.  For example:
+ *
+ *   static void coroutine_fn foo(void) {
+ *   
+ *   }
+ *
+ * In the future it would be nice to have the compiler or a static
+ * checker catch misuse of such functions.  This annotation might make
+ * it possible and in the meantime it serves as documentation.
+ */
+


Is it intentional that "#define coroutine_fn" is not here?

Paolo

[PATCH 1/2] hw/virtio: Rename virtio_device_find() -> qmp_find_virtio_device()

[Philippe Mathieu-Daudé]

To emphasize this function is QMP related, rename it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/virtio/virtio.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 289eb71045..a87007d22f 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3826,7 +3826,7 @@ VirtioInfoList *qmp_x_query_virtio(Error **errp)
 return list;
 }
 
-static VirtIODevice *virtio_device_find(const char *path)
+static VirtIODevice *qmp_find_virtio_device(const char *path)
 {
 VirtIODevice *vdev;
 
@@ -3867,7 +3867,7 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, 
Error **errp)
 VirtIODevice *vdev;
 VirtioStatus *status;
 
-vdev = virtio_device_find(path);
+vdev = qmp_find_virtio_device(path);
 if (vdev == NULL) {
 error_setg(errp, "Path %s is not a VirtIODevice", path);
 return NULL;
@@ -3943,7 +3943,7 @@ VirtVhostQueueStatus 
*qmp_x_query_virtio_vhost_queue_status(const char *path,
 VirtIODevice *vdev;
 VirtVhostQueueStatus *status;
 
-vdev = virtio_device_find(path);
+vdev = qmp_find_virtio_device(path);
 if (vdev == NULL) {
 error_setg(errp, "Path %s is not a VirtIODevice", path);
 return NULL;
@@ -3987,7 +3987,7 @@ VirtQueueStatus *qmp_x_query_virtio_queue_status(const 
char *path,
 VirtIODevice *vdev;
 VirtQueueStatus *status;
 
-vdev = virtio_device_find(path);
+vdev = qmp_find_virtio_device(path);
 if (vdev == NULL) {
 error_setg(errp, "Path %s is not a VirtIODevice", path);
 return NULL;
@@ -4080,7 +4080,7 @@ VirtioQueueElement 
*qmp_x_query_virtio_queue_element(const char *path,
 VirtQueue *vq;
 VirtioQueueElement *element = NULL;
 
-vdev = virtio_device_find(path);
+vdev = qmp_find_virtio_device(path);
 if (vdev == NULL) {
 error_setg(errp, "Path %s is not a VirtIO device", path);
 return NULL;
-- 
2.38.1

Re: [PATCH] chardev: clean up chardev-parallel.c

[Philippe Mathieu-Daudé]

On 22/12/22 08:38, Paolo Bonzini wrote:

Replace HAVE_CHARDEV_PARPORT with a Meson conditional, remove unnecessary
defines, and close the file descriptor on FreeBSD/DragonFly.

Signed-off-by: Paolo Bonzini 
---
  chardev/char-parallel.c | 15 ++-
  chardev/meson.build |  5 -
  include/qemu/osdep.h|  5 -
  3 files changed, 6 insertions(+), 19 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2] MIPS: remove support for trap and emulate KVM

[Philippe Mathieu-Daudé]

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

From: Paolo Bonzini 

This support was limited to the Malta board, drop it.
I do not have a machine that can run VZ KVM, so I am assuming
that it works for -M malta as well.

Signed-off-by: Paolo Bonzini 
Signed-off-by: Philippe Mathieu-Daudé 
---
Since Paolo's v1:

- Remove cpu_mips_kvm_um_phys_to_kseg0() declaration in "cpu.h"
- Remove unused KVM_KSEG0_BASE/KVM_KSEG2_BASE definitions
- Use USEG_LIMIT/KSEG0_BASE instead of magic values

/* Check where the kernel has been linked */
   -if (!(kernel_entry & 0x8000ll)) {
   -error_report("CONFIG_KVM_GUEST kernels are not supported");
   +if (kernel_entry <= USEG_LIMIT) {
   +error_report("Trap-and-Emul kernels (Linux CONFIG_KVM_GUEST)"
   + " are not supported");

   -env->CP0_EBase = (cs->cpu_index & 0x3FF) | (int32_t)0x8000;
   +env->CP0_EBase = KSEG0_BASE | (cs->cpu_index & 0x3FF);
---
  docs/about/deprecated.rst   |  9 ---
  docs/about/removed-features.rst |  9 +++
  hw/mips/malta.c | 46 +
  target/mips/cpu.c   |  7 +
  target/mips/cpu.h   |  3 ---
  target/mips/internal.h  |  3 ---
  target/mips/kvm.c   | 11 +---
  target/mips/sysemu/addr.c   | 17 
  target/mips/sysemu/physaddr.c   | 13 --
  9 files changed, 18 insertions(+), 100 deletions(-)


Thanks, queued to mips-next.

Re: [PATCH v3 10/10] libvduse: Fix assignment in vring_set_avail_event

[Paolo Bonzini]

On 12/21/22 14:10, Marcel Holtmann wrote:

  static inline void vring_set_avail_event(VduseVirtq *vq, uint16_t val)
  {
-*((uint16_t *)&vq->vring.used->ring[vq->vring.num]) = htole16(val);
+uint16_t *avail;
+
+avail = (uint16_t *)&vq->vring.used->ring[vq->vring.num];
+*avail = htole16(val);


That this doesn't warn is basically a compiler bug.

Please use memcpy instead, i.e.

  uint16_t val_le = htole16(val);
  memcpy(&vq->vring.used->ring[vq->vring.num]), &val_le, sizeof(uint16_t));

Paolo

Re: [PATCH v3 02/10] libvhost-user: Replace typeof with __typeof__

[Paolo Bonzini]

On 12/21/22 14:10, Marcel Holtmann wrote:

Strictly speaking only -std=gnu99 support the usage of typeof and for
easier inclusion in external projects, it is better to use __typeof__.

   CC   libvhost-user.o
libvhost-user.c: In function ‘vu_log_queue_fill’:
libvhost-user.c:86:13: error: implicit declaration of function ‘typeof’ 
[-Werror=implicit-function-declaration]
86 | typeof(x) _min1 = (x);  \
   | ^~

Changing these two users of typeof makes the compiler happy and no extra
flags or pragmas need to be provided.

Signed-off-by: Marcel Holtmann
Reviewed-by: Philippe Mathieu-Daudé


The build system uses "c_std=gnu99".  If you are extracting 
libvhost-user and not using its build files, you need to add --std=gnu99 
yourself when compiling.


If you really don't want to do that, as long as it's just a couple 
underscores that's fine I guess, but mixed declarations and code are 
going to reappear sooner or later.  Please add a patch like this:


diff --git a/subprojects/libvhost-user/meson.build 
b/subprojects/libvhost-user/meson.build

index 39825d9404ae..5deecbfe377d 100644
--- a/subprojects/libvhost-user/meson.build
+++ b/subprojects/libvhost-user/meson.build
@@ -1,6 +1,13 @@
 project('libvhost-user', 'c',
 license: 'GPL-2.0-or-later',
-default_options: ['c_std=gnu99'])
+default_options: ['warning_level=1', 'c_std=gnu99'])
+
+cc = meson.get_compiler('c')
+add_project_arguments(cc.get_supported_arguments(
+  '-Wsign-compare',
+  '-Wdeclaration-after-statement',
+  '-Wstrict-aliasing'),
+  native: false, language: 'c')

 threads = dependency('threads')
 glib = dependency('glib-2.0')


to avoid regressions, and likewise for libvduse.

Paolo

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

[Paolo Bonzini]

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;
 | ^~~~

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. :)

Paolo

Re: [PATCH 0/2] gitlab: improve debuggability of jobs

[Paolo Bonzini]

Queued patch 1 while the second is figured out, thanks.

Paolo

[PATCH] dockerfiles: update to Fedora 36

[Paolo Bonzini]

lcitool has removed the fedora-35 target, so let's follow
suit.

Signed-off-by: Paolo Bonzini 
---
 tests/docker/dockerfiles/fedora-win32-cross.docker | 4 ++--
 tests/docker/dockerfiles/fedora-win64-cross.docker | 4 ++--
 tests/docker/dockerfiles/fedora.docker | 4 ++--
 tests/lcitool/libvirt-ci   | 2 +-
 tests/lcitool/refresh  | 6 +++---
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tests/docker/dockerfiles/fedora-win32-cross.docker 
b/tests/docker/dockerfiles/fedora-win32-cross.docker
index 75383ba185a4..aece4f55818d 100644
--- a/tests/docker/dockerfiles/fedora-win32-cross.docker
+++ b/tests/docker/dockerfiles/fedora-win32-cross.docker
@@ -1,10 +1,10 @@
 # THIS FILE WAS AUTO-GENERATED
 #
-#  $ lcitool dockerfile --layers all --cross mingw32 fedora-35 qemu
+#  $ lcitool dockerfile --layers all --cross mingw32 fedora-36 qemu
 #
 # https://gitlab.com/libvirt/libvirt-ci
 
-FROM registry.fedoraproject.org/fedora:35
+FROM registry.fedoraproject.org/fedora:36
 
 RUN dnf install -y nosync && \
 echo -e '#!/bin/sh\n\
diff --git a/tests/docker/dockerfiles/fedora-win64-cross.docker 
b/tests/docker/dockerfiles/fedora-win64-cross.docker
index 98c03dc13b9c..2d573e2505f2 100644
--- a/tests/docker/dockerfiles/fedora-win64-cross.docker
+++ b/tests/docker/dockerfiles/fedora-win64-cross.docker
@@ -1,10 +1,10 @@
 # THIS FILE WAS AUTO-GENERATED
 #
-#  $ lcitool dockerfile --layers all --cross mingw64 fedora-35 qemu
+#  $ lcitool dockerfile --layers all --cross mingw64 fedora-36 qemu
 #
 # https://gitlab.com/libvirt/libvirt-ci
 
-FROM registry.fedoraproject.org/fedora:35
+FROM registry.fedoraproject.org/fedora:36
 
 RUN dnf install -y nosync && \
 echo -e '#!/bin/sh\n\
diff --git a/tests/docker/dockerfiles/fedora.docker 
b/tests/docker/dockerfiles/fedora.docker
index d200c7fc1055..901562bb8e5a 100644
--- a/tests/docker/dockerfiles/fedora.docker
+++ b/tests/docker/dockerfiles/fedora.docker
@@ -1,10 +1,10 @@
 # THIS FILE WAS AUTO-GENERATED
 #
-#  $ lcitool dockerfile --layers all fedora-35 qemu
+#  $ lcitool dockerfile --layers all fedora-36 qemu
 #
 # https://gitlab.com/libvirt/libvirt-ci
 
-FROM registry.fedoraproject.org/fedora:35
+FROM registry.fedoraproject.org/fedora:36
 
 RUN dnf install -y nosync && \
 echo -e '#!/bin/sh\n\
diff --git a/tests/lcitool/libvirt-ci b/tests/lcitool/libvirt-ci
index e3eb28cf2e17..a29b1c9925ed 16
--- a/tests/lcitool/libvirt-ci
+++ b/tests/lcitool/libvirt-ci
@@ -1 +1 @@
-Subproject commit e3eb28cf2e17fbcf7fe7e19505ee432b8ec5bbb5
+Subproject commit a29b1c9925ed8549b2a9e83374a8aa5ebe16bc71
diff --git a/tests/lcitool/refresh b/tests/lcitool/refresh
index fa966e4009d5..ee55ee40ee5d 100755
--- a/tests/lcitool/refresh
+++ b/tests/lcitool/refresh
@@ -111,7 +111,7 @@ try:
 generate_dockerfile("centos8", "centos-stream-8")
 generate_dockerfile("debian-amd64", "debian-11",
 trailer="".join(debian11_extras))
-generate_dockerfile("fedora", "fedora-35")
+generate_dockerfile("fedora", "fedora-36")
 generate_dockerfile("opensuse-leap", "opensuse-leap-153")
 generate_dockerfile("ubuntu2004", "ubuntu-2004",
 trailer="".join(ubuntu2004_tsanhack))
@@ -161,12 +161,12 @@ try:
 trailer=cross_build("s390x-linux-gnu-",
 "s390x-softmmu,s390x-linux-user"))
 
-generate_dockerfile("fedora-win32-cross", "fedora-35",
+generate_dockerfile("fedora-win32-cross", "fedora-36",
 cross="mingw32",
 trailer=cross_build("i686-w64-mingw32-",
 "i386-softmmu"))
 
-generate_dockerfile("fedora-win64-cross", "fedora-35",
+generate_dockerfile("fedora-win64-cross", "fedora-36",
 cross="mingw64",
 trailer=cross_build("x86_64-w64-mingw32-",
 "x86_64-softmmu"))
-- 
2.38.1

Re: [PATCH 0/6] enforce use of G_GNUC_PRINTF annotations

[Paolo Bonzini]

Queued, thanks.

Paolo

Re: [PATCH] hw/display: avoid creating empty loadable modules

[Paolo Bonzini]

On 12/19/22 13:58, Daniel P. Berrangé wrote:

When using --disable-virglrenderer, QEMU still creates

   hw-display-virtio-gpu-gl.so
   hw-display-virtio-vga-gl.so
   hw-display-virtio-gpu-pci-gl.so

but when these are loaded, they provide no functionality as the code
which registers types is not compiled in. Funtionally this is
relatively harmless, because QEMU is fine loading a module with no
types.

This is rather confusing for users and OS distro maintainers though,
as they think they have the GL functionality built, but in fact the
module they are looking at provides nothing of value.

The root cause is the use of 'when/if_true' rules when adding sources
to the module source set. If all the rules evaluate to false, then we
have declared the module, but not added anything to it.  We need to
put declaration of the entire module inside a condition based on
existance of the 3rd party library deps that are mandatory.

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1352
Signed-off-by: Daniel P. Berrangé 
---
  hw/display/meson.build | 24 ++--
  1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/hw/display/meson.build b/hw/display/meson.build
index 7a725ed80e..20a53d24a8 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -64,7 +64,7 @@ softmmu_ss.add(when: 'CONFIG_ARTIST', if_true: 
files('artist.c'))
  softmmu_ss.add(when: [pixman, 'CONFIG_ATI_VGA'], if_true: files('ati.c', 
'ati_2d.c', 'ati_dbg.c'))
  
  
-if config_all_devices.has_key('CONFIG_VIRTIO_GPU')

+if config_all_devices.has_key('CONFIG_VIRTIO_GPU') and pixman.found()


pixman is mandatory if have_system.

Can you instead change hw/meson.build to skip system-only directories? 
I think this should do:


subdir('core')
if not have_system
  subdir_done()
endif

# everything else...

The rest is good, so I have applied it.

Paolo


virtio_gpu_ss = ss.source_set()
virtio_gpu_ss.add(when: 'CONFIG_VIRTIO_GPU',
  if_true: [files('virtio-gpu-base.c', 'virtio-gpu.c'), 
pixman])
@@ -73,13 +73,15 @@ if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
virtio_gpu_ss.add(when: 'CONFIG_VHOST_USER_GPU', if_true: 
files('vhost-user-gpu.c'))
hw_display_modules += {'virtio-gpu': virtio_gpu_ss}
  
-  virtio_gpu_gl_ss = ss.source_set()

-  virtio_gpu_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', virgl, opengl],
-   if_true: [files('virtio-gpu-gl.c', 
'virtio-gpu-virgl.c'), pixman, virgl])
-  hw_display_modules += {'virtio-gpu-gl': virtio_gpu_gl_ss}
+  if virgl.found() and opengl.found()
+virtio_gpu_gl_ss = ss.source_set()
+virtio_gpu_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', virgl, opengl],
+ if_true: [files('virtio-gpu-gl.c', 
'virtio-gpu-virgl.c'), pixman, virgl])
+hw_display_modules += {'virtio-gpu-gl': virtio_gpu_gl_ss}
+  endif
  endif
  
-if config_all_devices.has_key('CONFIG_VIRTIO_PCI')

+if config_all_devices.has_key('CONFIG_VIRTIO_PCI') and pixman.found()
virtio_gpu_pci_ss = ss.source_set()
virtio_gpu_pci_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRTIO_PCI'],
  if_true: [files('virtio-gpu-pci.c'), pixman])
@@ -87,10 +89,12 @@ if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
  if_true: files('vhost-user-gpu-pci.c'))
hw_display_modules += {'virtio-gpu-pci': virtio_gpu_pci_ss}
  
-  virtio_gpu_pci_gl_ss = ss.source_set()

-  virtio_gpu_pci_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRTIO_PCI', 
virgl, opengl],
-   if_true: [files('virtio-gpu-pci-gl.c'), pixman])
-  hw_display_modules += {'virtio-gpu-pci-gl': virtio_gpu_pci_gl_ss}
+  if virgl.found() and opengl.found()
+virtio_gpu_pci_gl_ss = ss.source_set()
+virtio_gpu_pci_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRTIO_PCI', 
virgl, opengl],
+ if_true: [files('virtio-gpu-pci-gl.c'), pixman])
+hw_display_modules += {'virtio-gpu-pci-gl': virtio_gpu_pci_gl_ss}
+  endif
  endif
  
  if config_all_devices.has_key('CONFIG_VIRTIO_VGA')

Re: [PATCH v3 4/5] coroutine: Split qemu/coroutine-core.h off qemu/coroutine.h

[Markus Armbruster]

Paolo Bonzini  writes:

> On 12/21/22 14:14, Markus Armbruster wrote:
>> +/**
>> + * Mark a function that executes in coroutine context
>> + *
>> + *
>> + * Functions that execute in coroutine context cannot be called
>> + * directly from normal functions.  Use @coroutine_fn to mark such
>> + * functions.  For example:
>> + *
>> + *   static void coroutine_fn foo(void) {
>> + *   
>> + *   }
>> + *
>> + * In the future it would be nice to have the compiler or a static
>> + * checker catch misuse of such functions.  This annotation might make
>> + * it possible and in the meantime it serves as documentation.
>> + */
>> +
>
> Is it intentional that "#define coroutine_fn" is not here?

Yes: I moved it to qemu/osdep.h in PATCH 2, along with its doc comment.
To avoid compromising coroutine.h as overview documentation, I added
rephrased documentation there.

This patch copies this rephrased documentation to coroutine-core.h.

I'm open to better ideas.

RE: [PATCH] hw/sd/sdhci: Block Size Register bits [14:12] is lost

[Gao, Lu]

Hello,

Just want to check whether we can have it merged? Any other comments?

Thanks a lot!
B.R.

-Original Message-
From: Philippe Mathieu-Daudé [mailto:philippe.mathieu.da...@gmail.com] On 
Behalf Of Philippe Mathieu-Daudé
Sent: Tuesday, May 31, 2022 6:09 PM
To: Gao, Lu; qemu-devel@nongnu.org
Cc: Wen, Jianxian; Bin Meng; open list:SD (Secure Card); Alexander Bulekov; 
Prasad J Pandit
Subject: Re: [PATCH] hw/sd/sdhci: Block Size Register bits [14:12] is lost

On 21/3/22 06:56, Lu Gao wrote:
> Block Size Register bits [14:12] is SDMA Buffer Boundary, it is missed
> in register write, but it is needed in SDMA transfer. e.g. it will be
> used in sdhci_sdma_transfer_multi_blocks to calculate boundary_ variables.
> 
> Missing this field will cause wrong operation for different SDMA Buffer
> Boundary settings.

Fixes: d7dfca0807 ("hw/sdhci: introduce standard SD host controller")
Fixes: dfba99f17f ("hw/sdhci: Fix DMA Transfer Block Size field")

Reviewed-by: Philippe Mathieu-Daudé 

> Signed-off-by: Lu Gao 
> Signed-off-by: Jianxian Wen 
> ---
>   hw/sd/sdhci.c | 15 +++
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index e0bbc90344..350ceb487d 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -321,6 +321,8 @@ static void sdhci_poweron_reset(DeviceState *dev)
>   
>   static void sdhci_data_transfer(void *opaque);
>   
> +#define BLOCK_SIZE_MASK (4 * KiB - 1)
> +
>   static void sdhci_send_command(SDHCIState *s)
>   {
>   SDRequest request;
> @@ -371,7 +373,8 @@ static void sdhci_send_command(SDHCIState *s)
>   
>   sdhci_update_irq(s);
>   
> -if (!timeout && s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
> +if (!timeout && (s->blksize & BLOCK_SIZE_MASK) &&
> +(s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
>   s->data_count = 0;
>   sdhci_data_transfer(s);
>   }
> @@ -406,7 +409,6 @@ static void sdhci_end_transfer(SDHCIState *s)
>   /*
>* Programmed i/o data transfer
>*/
> -#define BLOCK_SIZE_MASK (4 * KiB - 1)
>   
>   /* Fill host controller's read buffer with BLKSIZE bytes of data from card 
> */
>   static void sdhci_read_block_from_card(SDHCIState *s)
> @@ -1137,7 +1139,8 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
> unsigned size)
>   s->sdmasysad = (s->sdmasysad & mask) | value;
>   MASKED_WRITE(s->sdmasysad, mask, value);
>   /* Writing to last byte of sdmasysad might trigger transfer */
> -if (!(mask & 0xFF00) && s->blkcnt && s->blksize &&
> +if (!(mask & 0xFF00) && s->blkcnt &&
> +(s->blksize & BLOCK_SIZE_MASK) &&
>   SDHC_DMA_TYPE(s->hostctl1) == SDHC_CTRL_SDMA) {
>   if (s->trnmod & SDHC_TRNS_MULTI) {
>   sdhci_sdma_transfer_multi_blocks(s);
> @@ -1151,7 +1154,11 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
> unsigned size)
>   if (!TRANSFERRING_DATA(s->prnsts)) {
>   uint16_t blksize = s->blksize;
>   
> -MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
> +/*
> + * [14:12] SDMA Buffer Boundary
> + * [11:00] Transfer Block Size
> + */
> +MASKED_WRITE(s->blksize, mask, extract32(value, 0, 15));
>   MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
>   
>   /* Limit block size to the maximum buffer size */

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

[Philippe Mathieu-Daudé]

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?


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();
 }
 d->Q(0) = r0;
 d->Q(1) = r1;
---

Re: [PATCH 4/5] include/hw/pci: Split pci_device.h off pci.h

[Markus Armbruster]

"Michael S. Tsirkin"  writes:

> On Fri, Dec 09, 2022 at 02:48:01PM +0100, Markus Armbruster wrote:
>> PCIDeviceClass and PCIDevice are defined in pci.h.  Many users of the
>> header don't actually need them.  Similar structs live in their own
>> headers: PCIBusClass and PCIBus in pci_bus.h, PCIBridge in
>> pci_bridge.h, PCIHostBridgeClass and PCIHostState in pci_host.h,
>> PCIExpressHost in pcie_host.h, and PCIERootPortClass, PCIEPort, and
>> PCIESlot in pcie_port.h.
>> 
>> Move PCIDeviceClass and PCIDeviceClass to new pci_device.h, along with
>> the code that needs them.  Adjust include directives.
>> 
>> This also enables the next commit.
>> 
>> Signed-off-by: Markus Armbruster 
>
>
> checkpatch is unhappy:
>
> ./scripts/checkpatch.pl /tmp/patch 
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #707: 
> new file mode 100644

We're good:

$ scripts/get_maintainer.pl -f include/hw/pci/pci_device.h 
"Michael S. Tsirkin"  (supporter:PCI)
Marcel Apfelbaum  (supporter:PCI)
qemu-devel@nongnu.org (open list:All patches CC here)

But checkpatch is too simple-minded to see that.

> ERROR: spaces required around that '*' (ctx:WxV)
> #997: FILE: include/hw/pci/pci_device.h:286:
> +   uint##_bits##_t *val, \
> ^

False positive.

  #define PCI_DMA_DEFINE_LDST(_l, _s, _bits) \
  static inline MemTxResult ld##_l##_pci_dma(PCIDevice *dev, \
 dma_addr_t addr, \
---> uint##_bits##_t *val, \
 MemTxAttrs attrs) \
  { \
  return ld##_l##_dma(pci_get_address_space(dev), addr, val, attrs); \
  } \
  static inline MemTxResult st##_s##_pci_dma(PCIDevice *dev, \
 dma_addr_t addr, \
 uint##_bits##_t val, \
 MemTxAttrs attrs) \
  { \
  return st##_s##_dma(pci_get_address_space(dev), addr, val, attrs); \
  }

The part checkpatch objects to is actually a parameter declaration of
the form

TYPE *NAME

The use of spaces is fine.  Not fine would be

TYPE * NAME

Having a macro expand into a function definition confuses checkpatch.

> It's right - we need a MAINTAINERS entry.
> Not sure how to fix the error - any idea?

There is nothing to fix :)

Re: [PATCH v3 0/5] coroutine: Clean up includes

[Markus Armbruster]

Markus Armbruster  writes:

> Philippe Mathieu-Daudé  writes:
>
>> On 21/12/22 14:14, Markus Armbruster wrote:
>>> v3:
>>> * PATCH 4: Unnecessary hunks dropped
>>> v2:
>>> * Rebased
>>> * PATCH 4: Rewritten [Paolo]
>>> * PATCH 5: New
>>> Markus Armbruster (5):
>>>coroutine: Clean up superfluous inclusion of qemu/coroutine.h
>>>coroutine: Move coroutine_fn to qemu/osdep.h, trim includes
>>>coroutine: Clean up superfluous inclusion of qemu/lockable.h
>>>coroutine: Split qemu/coroutine-core.h off qemu/coroutine.h
>>>coroutine: Use Coroutine typedef name instead of structure tag
>>
>> I had to add:
>>
>> -- >8 --
>> diff --git a/hw/pci/pci-hmp-cmds.c b/hw/pci/pci-hmp-cmds.c
>> index fb7591d6ab..b09fce9377 100644
>> --- a/hw/pci/pci-hmp-cmds.c
>> +++ b/hw/pci/pci-hmp-cmds.c
>> @@ -15,6 +15,7 @@
>>
>>  #include "qemu/osdep.h"
>>  #include "hw/pci/pci.h"
>> +#include "hw/pci/pci_device.h"
>>  #include "monitor/hmp.h"
>>  #include "monitor/monitor.h"
>>  #include "pci-internal.h"
>> diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
>> index 8e7282658f..3d4497da99 100644
>> --- a/hw/virtio/virtio-qmp.c
>> +++ b/hw/virtio/virtio-qmp.c
>> @@ -11,6 +11,7 @@
>>
>>  #include "qemu/osdep.h"
>>  #include "hw/virtio/virtio.h"
>> +#include "hw/virtio/vhost.h"
>>  #include "virtio-qmp.h"
>>
>> ---
>>
>> Otherwise I get:
>>
>> ../hw/pci/pci-hmp-cmds.c: In function ‘pcibus_dev_print’:
>> ../hw/pci/pci-hmp-cmds.c:129:31: error: invalid use of incomplete typedef 
>> ‘PCIDevice’
>>   129 | int class = pci_get_word(d->config + PCI_CLASS_DEVICE);
>>   |   ^~
>>
>> ../hw/virtio/virtio-qmp.c:187:19: error: ‘VHOST_USER_F_PROTOCOL_FEATURES’ 
>> undeclared here (not in a function); 
>> did you mean ‘VHOST_USER_PROTOCOL_F_RARP’?
>>   187 | FEATURE_ENTRY(VHOST_USER_F_PROTOCOL_FEATURES, \
>>   |   ^~
>>
>>
>> Maybe some recently merged change?
>
> Yes.  I'll rebase.

To avoid confusion: this series doesn't need a rebase.  "[PATCH 0/5]
include/hw/pci include/hw/cxl: Clean up includes" does.

[...]

[PATCH v2 1/7] include/hw/pci: Break inclusion loop pci_bridge.h and cxl.h

[Markus Armbruster]

hw/pci/pci_bridge.h and hw/cxl/cxl.h include each other.

Fortunately, breaking the loop is merely a matter of deleting
unnecessary includes from headers, and adding them back in places
where they are now missing.

Signed-off-by: Markus Armbruster 
---
 hw/alpha/alpha_sys.h  | 1 -
 hw/rdma/rdma_utils.h  | 1 -
 hw/rdma/vmw/pvrdma.h  | 1 -
 hw/usb/hcd-ehci.h | 1 -
 hw/xen/xen_pt.h   | 1 -
 include/hw/cxl/cxl.h  | 1 -
 include/hw/cxl/cxl_cdat.h | 1 +
 include/hw/cxl/cxl_device.h   | 1 +
 include/hw/cxl/cxl_pci.h  | 2 --
 include/hw/i386/ich9.h| 4 
 include/hw/i386/x86-iommu.h   | 1 -
 include/hw/isa/vt82c686.h | 1 -
 include/hw/pci-host/designware.h  | 3 ---
 include/hw/pci-host/i440fx.h  | 2 +-
 include/hw/pci-host/ls7a.h| 2 --
 include/hw/pci-host/pnv_phb3.h| 2 --
 include/hw/pci-host/pnv_phb4.h| 3 +--
 include/hw/pci-host/xilinx-pcie.h | 1 -
 include/hw/pci/pcie.h | 1 -
 include/hw/virtio/virtio-scsi.h   | 1 -
 hw/alpha/pci.c| 1 +
 hw/alpha/typhoon.c| 2 +-
 hw/i386/acpi-build.c  | 2 +-
 hw/pci-bridge/i82801b11.c | 2 +-
 hw/rdma/rdma_utils.c  | 1 +
 hw/scsi/virtio-scsi.c | 1 +
 26 files changed, 10 insertions(+), 30 deletions(-)

diff --git a/hw/alpha/alpha_sys.h b/hw/alpha/alpha_sys.h
index 2263e821da..a303c58438 100644
--- a/hw/alpha/alpha_sys.h
+++ b/hw/alpha/alpha_sys.h
@@ -5,7 +5,6 @@
 
 #include "target/alpha/cpu-qom.h"
 #include "hw/pci/pci.h"
-#include "hw/pci/pci_host.h"
 #include "hw/boards.h"
 #include "hw/intc/i8259.h"
 
diff --git a/hw/rdma/rdma_utils.h b/hw/rdma/rdma_utils.h
index 0c6414e7e0..54e4f56edd 100644
--- a/hw/rdma/rdma_utils.h
+++ b/hw/rdma/rdma_utils.h
@@ -18,7 +18,6 @@
 #define RDMA_UTILS_H
 
 #include "qemu/error-report.h"
-#include "hw/pci/pci.h"
 #include "sysemu/dma.h"
 
 #define rdma_error_report(fmt, ...) \
diff --git a/hw/rdma/vmw/pvrdma.h b/hw/rdma/vmw/pvrdma.h
index d08965d3e2..0caf95ede8 100644
--- a/hw/rdma/vmw/pvrdma.h
+++ b/hw/rdma/vmw/pvrdma.h
@@ -18,7 +18,6 @@
 
 #include "qemu/units.h"
 #include "qemu/notify.h"
-#include "hw/pci/pci.h"
 #include "hw/pci/msix.h"
 #include "chardev/char-fe.h"
 #include "hw/net/vmxnet3_defs.h"
diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
index a173707d9b..4d4b2830b7 100644
--- a/hw/usb/hcd-ehci.h
+++ b/hw/usb/hcd-ehci.h
@@ -23,7 +23,6 @@
 #include "sysemu/dma.h"
 #include "hw/pci/pci.h"
 #include "hw/sysbus.h"
-#include "qom/object.h"
 
 #ifndef EHCI_DEBUG
 #define EHCI_DEBUG   0
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index e7c4316a7d..cf10fc7bbf 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -2,7 +2,6 @@
 #define XEN_PT_H
 
 #include "hw/xen/xen_common.h"
-#include "hw/pci/pci.h"
 #include "xen-host-pci-device.h"
 #include "qom/object.h"
 
diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
index 38e0e271d5..5129557bee 100644
--- a/include/hw/cxl/cxl.h
+++ b/include/hw/cxl/cxl.h
@@ -13,7 +13,6 @@
 
 #include "qapi/qapi-types-machine.h"
 #include "qapi/qapi-visit-machine.h"
-#include "hw/pci/pci_bridge.h"
 #include "hw/pci/pci_host.h"
 #include "cxl_pci.h"
 #include "cxl_component.h"
diff --git a/include/hw/cxl/cxl_cdat.h b/include/hw/cxl/cxl_cdat.h
index e9eda00142..7f67638685 100644
--- a/include/hw/cxl/cxl_cdat.h
+++ b/include/hw/cxl/cxl_cdat.h
@@ -11,6 +11,7 @@
 #define CXL_CDAT_H
 
 #include "hw/cxl/cxl_pci.h"
+#include "hw/pci/pcie_doe.h"
 
 /*
  * Reference:
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 449b0edfe9..fd475b947b 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -10,6 +10,7 @@
 #ifndef CXL_DEVICE_H
 #define CXL_DEVICE_H
 
+#include "hw/pci/pci.h"
 #include "hw/register.h"
 
 /*
diff --git a/include/hw/cxl/cxl_pci.h b/include/hw/cxl/cxl_pci.h
index 3cb79eca1e..aca14845ab 100644
--- a/include/hw/cxl/cxl_pci.h
+++ b/include/hw/cxl/cxl_pci.h
@@ -11,8 +11,6 @@
 #define CXL_PCI_H
 
 #include "qemu/compiler.h"
-#include "hw/pci/pci.h"
-#include "hw/pci/pcie.h"
 #include "hw/cxl/cxl_cdat.h"
 
 #define CXL_VENDOR_ID 0x1e98
diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
index 23ee8e371b..222781e8b9 100644
--- a/include/hw/i386/ich9.h
+++ b/include/hw/i386/ich9.h
@@ -5,12 +5,8 @@
 #include "hw/sysbus.h"
 #include "hw/i386/pc.h"
 #include "hw/isa/apm.h"
-#include "hw/pci/pci.h"
-#include "hw/pci/pcie_host.h"
-#include "hw/pci/pci_bridge.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/ich9.h"
-#include "hw/pci/pci_bus.h"
 #include "qom/object.h"
 
 void ich9_lpc_set_irq(void *opaque, int irq_num, int level);
diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h
index 7637edb430..8d8d53b18b 100644
--- a/include/hw/i386/x86-iommu.h
+++ b/include/hw/i386/x86-iommu.h
@@ -21,7 +21,6 @@
 #define HW_I386_X86_IOMMU_H
 
 #include "hw/sysbus.h"
-#include "hw/pci/pci.h"
 #include "hw/pci/m

[PATCH v2 5/7] include/hw/pci: Split pci_device.h off pci.h

[Markus Armbruster]

PCIDeviceClass and PCIDevice are defined in pci.h.  Many users of the
header don't actually need them.  Similar structs live in their own
headers: PCIBusClass and PCIBus in pci_bus.h, PCIBridge in
pci_bridge.h, PCIHostBridgeClass and PCIHostState in pci_host.h,
PCIExpressHost in pcie_host.h, and PCIERootPortClass, PCIEPort, and
PCIESlot in pcie_port.h.

Move PCIDeviceClass and PCIDeviceClass to new pci_device.h, along with
the code that needs them.  Adjust include directives.

This also enables the next commit.

Signed-off-by: Markus Armbruster 
---
 hw/display/ati_int.h |   2 +-
 hw/display/qxl.h |   3 +-
 hw/ide/ahci_internal.h   |   2 +-
 hw/net/vmxnet3_defs.h|   2 +-
 hw/nvme/nvme.h   |   2 +-
 hw/rdma/vmw/pvrdma.h |   1 +
 hw/scsi/mptsas.h |   2 +-
 hw/usb/hcd-ehci.h|   2 +-
 hw/usb/hcd-uhci.h|   2 +-
 hw/usb/hcd-xhci-pci.h|   1 +
 hw/vfio/pci.h|   2 +-
 include/hw/acpi/piix4.h  |   2 +-
 include/hw/arm/allwinner-a10.h   |   1 +
 include/hw/cxl/cxl_device.h  |   2 +-
 include/hw/ide/pci.h |   2 +-
 include/hw/misc/macio/macio.h|   2 +-
 include/hw/pci-host/gpex.h   |   2 +-
 include/hw/pci-host/i440fx.h |   2 +-
 include/hw/pci-host/q35.h|   2 +-
 include/hw/pci-host/sabre.h  |   2 +-
 include/hw/pci/msi.h |   2 +-
 include/hw/pci/pci.h | 344 --
 include/hw/pci/pci_bridge.h  |   2 +-
 include/hw/pci/pci_device.h  | 350 +++
 include/hw/pci/pcie_port.h   |   1 +
 include/hw/pci/shpc.h|   2 +-
 include/hw/remote/iohub.h|   2 +-
 include/hw/remote/proxy.h|   2 +-
 include/hw/sd/sdhci.h|   2 +-
 include/hw/southbridge/piix.h|   3 +-
 include/hw/xen/xen_common.h  |   2 +-
 hw/acpi/erst.c   |   2 +-
 hw/audio/ac97.c  |   2 +-
 hw/audio/es1370.c|   2 +-
 hw/audio/via-ac97.c  |   2 +-
 hw/char/serial-pci-multi.c   |   2 +-
 hw/char/serial-pci.c |   2 +-
 hw/core/qdev-properties-system.c |   1 +
 hw/display/bochs-display.c   |   2 +-
 hw/display/cirrus_vga.c  |   2 +-
 hw/display/sm501.c   |   2 +-
 hw/display/vga-pci.c |   2 +-
 hw/display/vmware_vga.c  |   2 +-
 hw/i386/xen/xen_pvdevice.c   |   2 +-
 hw/ipack/tpci200.c   |   2 +-
 hw/ipmi/pci_ipmi_bt.c|   2 +-
 hw/ipmi/pci_ipmi_kcs.c   |   2 +-
 hw/isa/i82378.c  |   2 +-
 hw/mips/gt64xxx_pci.c|   2 +-
 hw/misc/pci-testdev.c|   2 +-
 hw/misc/pvpanic-pci.c|   2 +-
 hw/net/can/can_kvaser_pci.c  |   2 +-
 hw/net/can/can_mioe3680_pci.c|   2 +-
 hw/net/can/can_pcm3680_pci.c |   2 +-
 hw/net/can/ctucan_pci.c  |   2 +-
 hw/net/e1000.c   |   2 +-
 hw/net/e1000x_common.c   |   2 +-
 hw/net/eepro100.c|   2 +-
 hw/net/ne2000-pci.c  |   2 +-
 hw/net/net_tx_pkt.c  |   2 +-
 hw/net/pcnet-pci.c   |   2 +-
 hw/net/rocker/rocker.c   |   2 +-
 hw/net/rocker/rocker_desc.c  |   2 +-
 hw/net/rtl8139.c |   2 +-
 hw/net/sungem.c  |   2 +-
 hw/net/sunhme.c  |   2 +-
 hw/net/tulip.c   |   2 +-
 hw/net/virtio-net.c  |   2 +-
 hw/pci-host/bonito.c |   2 +-
 hw/pci-host/dino.c   |   2 +-
 hw/pci-host/grackle.c|   2 +-
 hw/pci-host/mv64361.c|   2 +-
 hw/pci-host/ppce500.c|   2 +-
 hw/pci-host/raven.c  |   2 +-
 hw/pci-host/sh_pci.c |   2 +-
 hw/pci-host/uninorth.c   |   2 +-
 hw/pci-host/versatile.c  |   2 +-
 hw/pci/pci-hmp-cmds.c|   1 +
 hw/pci/pcie_host.c   |   2 +-
 hw/pci/pcie_sriov.c  |   2 +-
 hw/pci/slotid_cap.c  |   2 +-
 hw/ppc/ppc440_pcix.c |   2 +-
 hw/ppc/ppc4xx_pci.c  |   2 +-
 hw/ppc/spapr_pci_vfio.c  |   1 +
 hw/rdma/rdma_utils.c |   2 +-
 hw/s390x/s390-pci-inst.c |   1 +
 hw/scsi/esp-pci.c|   2 +-
 hw/scsi/lsi53c895a.c |   2 +-
 hw/smbios/smbios.c   |   1 +
 hw/usb/hcd-ohci-pci.c|   2 +-
 hw/watchdog/wdt_i6300esb.c   |   2 +-
 tests/qtest/fuzz/generic_fuzz.c  |   1 +
 ui/util.c|   2 +-
 93 files changed, 441 insertions(+), 427 deletions(-)
 create mode 100644 include/hw/pci/pci_device.h

diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
index 8acb9c7466..e8d3c7af75 100644
--- a/hw/display/ati_int.h
+++ b/hw/display/ati_int.h
@@ -10,7 +10,7 @@
 #define ATI_INT_H
 
 #include "qemu/timer.h"
-#include "hw/pci/pci.h"
+#include "hw/pci/pci_device.h"
 #include "hw/i2c/bitbang

[PATCH v2 0/7] include/hw/pci include/hw/cxl: Clean up includes

[Markus Armbruster]

Back in 2016, we discussed[1] rules for headers, and these were
generally liked:

1. Have a carefully curated header that's included everywhere first.  We
   got that already thanks to Peter: osdep.h.

2. Headers should normally include everything they need beyond osdep.h.
   If exceptions are needed for some reason, they must be documented in
   the header.  If all that's needed from a header is typedefs, put
   those into qemu/typedefs.h instead of including the header.

3. Cyclic inclusion is forbidden.

After this series, include/hw/pci and include/hw/cxl conform to these
rules.

It is based on

[PATCH v2 0/3] block: Clean up includes
[PATCH v3 0/5] coroutine: Clean up includes

v2:
* Rebased
* PATCH 1: Actually breaks an inclusion loop; commit message rephrased
  accordingly
* PATCH 2: New [Jonathan]
* PATCH 5: tests/qtest/fuzz/generic_fuzz.c fixed [Michael]
* PATCH 6: Inclusion of cxl_pci.h into cxl_cdat_h kept, commit message
  adjusted [Jonathan]

[1] Message-ID: <87h9g8j57d@blackfin.pond.sub.org>
https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg03345.html

Based-on: <20221221133551.3967339-1-arm...@redhat.com>

Markus Armbruster (7):
  include/hw/pci: Break inclusion loop pci_bridge.h and cxl.h
  include/hw/cxl: Move typedef PXBDev to cxl.h, and put it to use
  include/hw/cxl: Include hw/cxl/*.h where needed
  include/hw/pci: Clean up a few things checkpatch.pl would flag
  include/hw/pci: Split pci_device.h off pci.h
  include/hw/pci: Include hw/pci/pci.h where needed
  include/hw/cxl: Break inclusion loop cxl_pci.h and cxl_cdat_h

 hw/alpha/alpha_sys.h|   1 -
 hw/display/ati_int.h|   2 +-
 hw/display/qxl.h|   3 +-
 hw/ide/ahci_internal.h  |   2 +-
 hw/net/vmxnet3_defs.h   |   2 +-
 hw/nvme/nvme.h  |   2 +-
 hw/rdma/rdma_utils.h|   1 -
 hw/rdma/vmw/pvrdma.h|   2 +-
 hw/scsi/mptsas.h|   2 +-
 hw/usb/hcd-ehci.h   |   3 +-
 hw/usb/hcd-uhci.h   |   2 +-
 hw/usb/hcd-xhci-pci.h   |   1 +
 hw/vfio/pci.h   |   2 +-
 hw/xen/xen_pt.h |   1 -
 include/hw/acpi/piix4.h |   2 +-
 include/hw/arm/allwinner-a10.h  |   1 +
 include/hw/cxl/cxl.h|   5 +-
 include/hw/cxl/cxl_cdat.h   |   1 +
 include/hw/cxl/cxl_component.h  |   1 +
 include/hw/cxl/cxl_device.h |   2 +
 include/hw/cxl/cxl_pci.h|   3 -
 include/hw/i386/ich9.h  |   4 -
 include/hw/i386/x86-iommu.h |   1 -
 include/hw/ide/pci.h|   2 +-
 include/hw/isa/vt82c686.h   |   1 -
 include/hw/misc/macio/macio.h   |   2 +-
 include/hw/pci-host/designware.h|   3 -
 include/hw/pci-host/gpex.h  |   2 +-
 include/hw/pci-host/i440fx.h|   2 +-
 include/hw/pci-host/ls7a.h  |   2 -
 include/hw/pci-host/pnv_phb3.h  |   2 -
 include/hw/pci-host/pnv_phb4.h  |   3 +-
 include/hw/pci-host/q35.h   |   2 +-
 include/hw/pci-host/sabre.h |   2 +-
 include/hw/pci-host/xilinx-pcie.h   |   1 -
 include/hw/pci/msi.h|   2 +-
 include/hw/pci/pci.h| 341 ---
 include/hw/pci/pci_bridge.h |   3 +-
 include/hw/pci/pci_device.h | 350 
 include/hw/pci/pcie.h   |   1 -
 include/hw/pci/pcie_port.h  |   1 +
 include/hw/pci/pcie_sriov.h |   2 +
 include/hw/pci/shpc.h   |   2 +-
 include/hw/remote/iohub.h   |   2 +-
 include/hw/remote/proxy.h   |   2 +-
 include/hw/sd/sdhci.h   |   2 +-
 include/hw/southbridge/piix.h   |   3 +-
 include/hw/virtio/virtio-scsi.h |   1 -
 include/hw/xen/xen_common.h |   2 +-
 hw/acpi/erst.c  |   2 +-
 hw/alpha/pci.c  |   1 +
 hw/alpha/typhoon.c  |   2 +-
 hw/audio/ac97.c |   2 +-
 hw/audio/es1370.c   |   2 +-
 hw/audio/via-ac97.c |   2 +-
 hw/char/serial-pci-multi.c  |   2 +-
 hw/char/serial-pci.c|   2 +-
 hw/core/qdev-properties-system.c|   1 +
 hw/display/bochs-display.c  |   2 +-
 hw/display/cirrus_vga.c |   2 +-
 hw/display/sm501.c  |   2 +-
 hw/display/vga-pci.c|   2 +-
 hw/display/vmware_vga.c |   2 +-
 hw/i386/acpi-build.c|   2 +-
 hw/i386/xen/xen_pvdevice.c  |   2 +-
 hw/ipack/tpci200.c  |   2 +-
 hw/ipmi/pci_ipmi_bt.c   |   2 +-
 hw/ipmi/pci_ipmi_kcs.c  |   2 +-
 hw/isa/i82378.c |   2 +-
 hw/mips/gt64xxx_pci.c   |   2 +-
 hw/misc/pci-testdev.c   |   2 +-
 hw/misc/pvpanic-pci.c   |   2 +-
 hw/net/can/can_kvaser_pci.c |   2 +-
 hw/net/can/can_m

[PATCH v2 6/7] include/hw/pci: Include hw/pci/pci.h where needed

[Markus Armbruster]

hw/pci/pcie_sriov.h needs PCI_NUM_REGIONS.  Without the previous
commit, this would close an inclusion loop: hw/pci/pci.h used to
include hw/pci/pcie.h for PCIExpressDevice, which includes
pcie_sriov.h for PCIESriovPF, which now includes hw/pci/pci.h for
PCI_NUM_REGIONS.

Signed-off-by: Markus Armbruster 
Reviewed-by: Philippe Mathieu-Daudé 
---
 include/hw/pci/pcie_sriov.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/hw/pci/pcie_sriov.h b/include/hw/pci/pcie_sriov.h
index 80f5c84e75..96cc743309 100644
--- a/include/hw/pci/pcie_sriov.h
+++ b/include/hw/pci/pcie_sriov.h
@@ -13,6 +13,8 @@
 #ifndef QEMU_PCIE_SRIOV_H
 #define QEMU_PCIE_SRIOV_H
 
+#include "hw/pci/pci.h"
+
 struct PCIESriovPF {
 uint16_t num_vfs;   /* Number of virtual functions created */
 uint8_t vf_bar_type[PCI_NUM_REGIONS];   /* Store type for each VF bar */
-- 
2.38.1

[PATCH v2 4/7] include/hw/pci: Clean up a few things checkpatch.pl would flag

[Markus Armbruster]

Fix a few style violations so that checkpatch.pl won't complain when I
move this code.

Signed-off-by: Markus Armbruster 
Reviewed-by: Philippe Mathieu-Daudé 
---
 include/hw/pci/pci.h | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 954f260f84..5ca2a9df58 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -284,8 +284,10 @@ struct PCIDevice {
 /* PCI config space */
 uint8_t *config;
 
-/* Used to enable config checks on load. Note that writable bits are
- * never checked even if set in cmask. */
+/*
+ * Used to enable config checks on load. Note that writable bits are
+ * never checked even if set in cmask.
+ */
 uint8_t *cmask;
 
 /* Used to implement R/W bytes */
@@ -299,10 +301,11 @@ struct PCIDevice {
 
 /* the following fields are read only */
 int32_t devfn;
-/* Cached device to fetch requester ID from, to avoid the PCI
- * tree walking every time we invoke PCI request (e.g.,
- * MSI). For conventional PCI root complex, this field is
- * meaningless. */
+/*
+ * Cached device to fetch requester ID from, to avoid the PCI tree
+ * walking every time we invoke PCI request (e.g., MSI). For
+ * conventional PCI root complex, this field is meaningless.
+ */
 PCIReqIDCache requester_id_cache;
 char name[64];
 PCIIORegion io_regions[PCI_NUM_REGIONS];
@@ -943,7 +946,7 @@ extern const VMStateDescription vmstate_pci_device;
 .name   = (stringify(_field)),   \
 .size   = sizeof(PCIDevice), \
 .vmsd   = &vmstate_pci_device,   \
-.flags  = VMS_STRUCT|VMS_POINTER,\
+.flags  = VMS_STRUCT | VMS_POINTER,  \
 .offset = vmstate_offset_pointer(_state, _field, PCIDevice), \
 }
 
-- 
2.38.1

[PATCH v2 3/7] include/hw/cxl: Include hw/cxl/*.h where needed

[Markus Armbruster]

hw/cxl/cxl_component.h needs CDATObject from hw/cxl/cxl_cdat.h.

hw/cxl/cxl_device.h needs CXLComponentState from
hw/cxl/cxl_component.h.

Signed-off-by: Markus Armbruster 
Acked-by: Jonathan Cameron 
---
 include/hw/cxl/cxl_component.h | 1 +
 include/hw/cxl/cxl_device.h| 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
index 34075cfb72..5dca21e95b 100644
--- a/include/hw/cxl/cxl_component.h
+++ b/include/hw/cxl/cxl_component.h
@@ -18,6 +18,7 @@
 #include "qemu/compiler.h"
 #include "qemu/range.h"
 #include "qemu/typedefs.h"
+#include "hw/cxl/cxl_cdat.h"
 #include "hw/register.h"
 #include "qapi/error.h"
 
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index fd475b947b..3f91969db0 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -10,6 +10,7 @@
 #ifndef CXL_DEVICE_H
 #define CXL_DEVICE_H
 
+#include "hw/cxl/cxl_component.h"
 #include "hw/pci/pci.h"
 #include "hw/register.h"
 
-- 
2.38.1

[PATCH v2 2/7] include/hw/cxl: Move typedef PXBDev to cxl.h, and put it to use

[Markus Armbruster]

hw/cxl/cxl.h uses the PXBDev structure tag instead of the typedef
name.  The typedef name is defined in hw/pci/pci_bridge.h.  Its
inclusion was dropped in the previous commit to break an inclusion
loop.

Move the typedef to hw/cxl/cxl.h, and use it there.  Delete an extra
typedef in hw/pci-bridge/pci_expander_bridge.c.

Signed-off-by: Markus Armbruster 
---
 include/hw/cxl/cxl.h| 4 +++-
 include/hw/pci/pci_bridge.h | 1 -
 hw/pci-bridge/pci_expander_bridge.c | 1 -
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
index 5129557bee..b161be59b7 100644
--- a/include/hw/cxl/cxl.h
+++ b/include/hw/cxl/cxl.h
@@ -23,10 +23,12 @@
 
 #define CXL_WINDOW_MAX 10
 
+typedef struct PXBDev PXBDev;
+
 typedef struct CXLFixedWindow {
 uint64_t size;
 char **targets;
-struct PXBDev *target_hbs[8];
+PXBDev *target_hbs[8];
 uint8_t num_targets;
 uint8_t enc_int_ways;
 uint8_t enc_int_gran;
diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index ca6caf487e..58a3fb0c2c 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -97,7 +97,6 @@ struct PXBDev {
 } cxl;
 };
 
-typedef struct PXBDev PXBDev;
 #define TYPE_PXB_CXL_DEVICE "pxb-cxl"
 DECLARE_INSTANCE_CHECKER(PXBDev, PXB_CXL_DEV,
  TYPE_PXB_CXL_DEVICE)
diff --git a/hw/pci-bridge/pci_expander_bridge.c 
b/hw/pci-bridge/pci_expander_bridge.c
index c9e817aa58..870d9bab11 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -50,7 +50,6 @@ struct PXBBus {
 };
 
 #define TYPE_PXB_DEVICE "pxb"
-typedef struct PXBDev PXBDev;
 DECLARE_INSTANCE_CHECKER(PXBDev, PXB_DEV,
  TYPE_PXB_DEVICE)
 
-- 
2.38.1

[PATCH v2 7/7] include/hw/cxl: Break inclusion loop cxl_pci.h and cxl_cdat_h

[Markus Armbruster]

hw/cxl/cxl_pci.h and hw/cxl/cxl_cdat.h include each other.  The former
doesn't actually need the latter, so drop that inclusion to break the
loop.

Signed-off-by: Markus Armbruster 
---
 include/hw/cxl/cxl_pci.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/hw/cxl/cxl_pci.h b/include/hw/cxl/cxl_pci.h
index aca14845ab..01e15ed5b4 100644
--- a/include/hw/cxl/cxl_pci.h
+++ b/include/hw/cxl/cxl_pci.h
@@ -11,7 +11,6 @@
 #define CXL_PCI_H
 
 #include "qemu/compiler.h"
-#include "hw/cxl/cxl_cdat.h"
 
 #define CXL_VENDOR_ID 0x1e98
 
-- 
2.38.1

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

[Eric Auger]

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?

Thanks

Eric
>  d->Q(0) = r0;
>  d->Q(1) = r1;
> ---
>

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Bin Meng]

On Thu, Dec 22, 2022 at 2:29 AM Daniel Henrique Barboza
 wrote:
>
> This test is used to do a quick sanity check to ensure that we're able
> to run the existing QEMU FW image.
>
> 'sifive_u', 'spike' and 'virt' riscv64 machines, and 'sifive_u' and
> 'virt' 32 bit machines are able to run the default RISCV64_BIOS_BIN |
> RISCV32_BIOS_BIN firmware with minimal options.
>
> Cc: Cleber Rosa 
> Cc: Philippe Mathieu-Daudé 
> Cc: Wainer dos Santos Moschetta 
> Cc: Beraldo Leal 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  tests/avocado/riscv_opensbi.py | 65 ++
>  1 file changed, 65 insertions(+)
>  create mode 100644 tests/avocado/riscv_opensbi.py
>
> diff --git a/tests/avocado/riscv_opensbi.py b/tests/avocado/riscv_opensbi.py
> new file mode 100644
> index 00..abc99ced30
> --- /dev/null
> +++ b/tests/avocado/riscv_opensbi.py
> @@ -0,0 +1,65 @@
> +# opensbi boot test for RISC-V machines
> +#
> +# Copyright (c) 2022, Ventana Micro
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later.  See the COPYING file in the top-level directory.
> +
> +from avocado_qemu import QemuSystemTest
> +from avocado_qemu import wait_for_console_pattern
> +
> +class RiscvOpensbi(QemuSystemTest):
> +"""
> +:avocado: tags=accel:tcg
> +"""
> +timeout = 5
> +
> +def test_riscv64_virt(self):
> +"""
> +:avocado: tags=arch:riscv64
> +:avocado: tags=machine:virt
> +"""
> +self.vm.set_console()
> +self.vm.launch()
> +wait_for_console_pattern(self, 'Platform Name')
> +wait_for_console_pattern(self, 'Boot HART MEDELEG')
> +
> +def test_riscv64_spike(self):
> +"""
> +:avocado: tags=arch:riscv64
> +:avocado: tags=machine:spike
> +"""
> +self.vm.set_console()
> +self.vm.launch()
> +wait_for_console_pattern(self, 'Platform Name')
> +wait_for_console_pattern(self, 'Boot HART MEDELEG')
> +
> +def test_riscv64_sifive_u(self):
> +"""
> +:avocado: tags=arch:riscv64
> +:avocado: tags=machine:sifive_u
> +"""
> +self.vm.set_console()
> +self.vm.launch()
> +wait_for_console_pattern(self, 'Platform Name')
> +wait_for_console_pattern(self, 'Boot HART MEDELEG')
> +
> +def test_riscv32_virt(self):
> +"""
> +:avocado: tags=arch:riscv32
> +:avocado: tags=machine:virt
> +"""
> +self.vm.set_console()
> +self.vm.launch()
> +wait_for_console_pattern(self, 'Platform Name')
> +wait_for_console_pattern(self, 'Boot HART MEDELEG')

How about testing riscv32_spike too?

> +
> +def test_riscv32_sifive_u(self):
> +"""
> +:avocado: tags=arch:riscv32
> +:avocado: tags=machine:sifive_u
> +"""
> +self.vm.set_console()
> +self.vm.launch()
> +wait_for_console_pattern(self, 'Platform Name')
> +wait_for_console_pattern(self, 'Boot HART MEDELEG')
> --

Regards,
Bin

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

[Philippe Mathieu-Daudé]

On 22/12/22 11:07, 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/


Adding the compiler version in the commit description would help:

--
Using GCC 11.3.1 "cc (GCC) 11.3.1 20220421 (Red Hat 11.3.1-2)" we get:
--


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?


Well my compiler (Apple clang version 14.0.0 (clang-1400.0.29.202))
doesn't display the warning, I don't have yours handy to test it :)

Re: [PATCH v2 0/7] include/hw/pci include/hw/cxl: Clean up includes

[Bernhard Beschow]

Am 22. Dezember 2022 10:03:23 UTC schrieb Markus Armbruster :
>Back in 2016, we discussed[1] rules for headers, and these were
>generally liked:
>
>1. Have a carefully curated header that's included everywhere first.  We
>   got that already thanks to Peter: osdep.h.
>
>2. Headers should normally include everything they need beyond osdep.h.
>   If exceptions are needed for some reason, they must be documented in
>   the header.  If all that's needed from a header is typedefs, put
>   those into qemu/typedefs.h instead of including the header.
>
>3. Cyclic inclusion is forbidden.

Sounds like these -- useful and sane -- rules belong in QEMU's coding style. 
What about putting them there for easy reference?

Best regards,
Bernhard

>After this series, include/hw/pci and include/hw/cxl conform to these
>rules.
>
>It is based on
>
>[PATCH v2 0/3] block: Clean up includes
>[PATCH v3 0/5] coroutine: Clean up includes
>
>v2:
>* Rebased
>* PATCH 1: Actually breaks an inclusion loop; commit message rephrased
>  accordingly
>* PATCH 2: New [Jonathan]
>* PATCH 5: tests/qtest/fuzz/generic_fuzz.c fixed [Michael]
>* PATCH 6: Inclusion of cxl_pci.h into cxl_cdat_h kept, commit message
>  adjusted [Jonathan]
>
>[1] Message-ID: <87h9g8j57d@blackfin.pond.sub.org>
>https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg03345.html
>
>Based-on: <20221221133551.3967339-1-arm...@redhat.com>
>
>Markus Armbruster (7):
>  include/hw/pci: Break inclusion loop pci_bridge.h and cxl.h
>  include/hw/cxl: Move typedef PXBDev to cxl.h, and put it to use
>  include/hw/cxl: Include hw/cxl/*.h where needed
>  include/hw/pci: Clean up a few things checkpatch.pl would flag
>  include/hw/pci: Split pci_device.h off pci.h
>  include/hw/pci: Include hw/pci/pci.h where needed
>  include/hw/cxl: Break inclusion loop cxl_pci.h and cxl_cdat_h
>
> hw/alpha/alpha_sys.h|   1 -
> hw/display/ati_int.h|   2 +-
> hw/display/qxl.h|   3 +-
> hw/ide/ahci_internal.h  |   2 +-
> hw/net/vmxnet3_defs.h   |   2 +-
> hw/nvme/nvme.h  |   2 +-
> hw/rdma/rdma_utils.h|   1 -
> hw/rdma/vmw/pvrdma.h|   2 +-
> hw/scsi/mptsas.h|   2 +-
> hw/usb/hcd-ehci.h   |   3 +-
> hw/usb/hcd-uhci.h   |   2 +-
> hw/usb/hcd-xhci-pci.h   |   1 +
> hw/vfio/pci.h   |   2 +-
> hw/xen/xen_pt.h |   1 -
> include/hw/acpi/piix4.h |   2 +-
> include/hw/arm/allwinner-a10.h  |   1 +
> include/hw/cxl/cxl.h|   5 +-
> include/hw/cxl/cxl_cdat.h   |   1 +
> include/hw/cxl/cxl_component.h  |   1 +
> include/hw/cxl/cxl_device.h |   2 +
> include/hw/cxl/cxl_pci.h|   3 -
> include/hw/i386/ich9.h  |   4 -
> include/hw/i386/x86-iommu.h |   1 -
> include/hw/ide/pci.h|   2 +-
> include/hw/isa/vt82c686.h   |   1 -
> include/hw/misc/macio/macio.h   |   2 +-
> include/hw/pci-host/designware.h|   3 -
> include/hw/pci-host/gpex.h  |   2 +-
> include/hw/pci-host/i440fx.h|   2 +-
> include/hw/pci-host/ls7a.h  |   2 -
> include/hw/pci-host/pnv_phb3.h  |   2 -
> include/hw/pci-host/pnv_phb4.h  |   3 +-
> include/hw/pci-host/q35.h   |   2 +-
> include/hw/pci-host/sabre.h |   2 +-
> include/hw/pci-host/xilinx-pcie.h   |   1 -
> include/hw/pci/msi.h|   2 +-
> include/hw/pci/pci.h| 341 ---
> include/hw/pci/pci_bridge.h |   3 +-
> include/hw/pci/pci_device.h | 350 
> include/hw/pci/pcie.h   |   1 -
> include/hw/pci/pcie_port.h  |   1 +
> include/hw/pci/pcie_sriov.h |   2 +
> include/hw/pci/shpc.h   |   2 +-
> include/hw/remote/iohub.h   |   2 +-
> include/hw/remote/proxy.h   |   2 +-
> include/hw/sd/sdhci.h   |   2 +-
> include/hw/southbridge/piix.h   |   3 +-
> include/hw/virtio/virtio-scsi.h |   1 -
> include/hw/xen/xen_common.h |   2 +-
> hw/acpi/erst.c  |   2 +-
> hw/alpha/pci.c  |   1 +
> hw/alpha/typhoon.c  |   2 +-
> hw/audio/ac97.c |   2 +-
> hw/audio/es1370.c   |   2 +-
> hw/audio/via-ac97.c |   2 +-
> hw/char/serial-pci-multi.c  |   2 +-
> hw/char/serial-pci.c|   2 +-
> hw/core/qdev-properties-system.c|   1 +
> hw/display/bochs-display.c  |   2 +-
> hw/display/cirrus_vga.c |   2 +-
> hw/display/sm501.c  |   2 +-
> hw/display/vga-pci.c|   2 +-
> hw/display/vmware_vga.c |   2 +-
> hw/i386/acpi-build.c|   2 +-
> hw/i386/xen/xen_pvdevice.c  |   2 +-
> hw/ipack/tpci200.c  |   2 +-
> hw/ipmi/pc

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Daniel Henrique Barboza]

On 12/22/22 07:24, Bin Meng wrote:

On Thu, Dec 22, 2022 at 2:29 AM Daniel Henrique Barboza
 wrote:

This test is used to do a quick sanity check to ensure that we're able
to run the existing QEMU FW image.

'sifive_u', 'spike' and 'virt' riscv64 machines, and 'sifive_u' and
'virt' 32 bit machines are able to run the default RISCV64_BIOS_BIN |
RISCV32_BIOS_BIN firmware with minimal options.

Cc: Cleber Rosa 
Cc: Philippe Mathieu-Daudé 
Cc: Wainer dos Santos Moschetta 
Cc: Beraldo Leal 
Signed-off-by: Daniel Henrique Barboza 
---
  tests/avocado/riscv_opensbi.py | 65 ++
  1 file changed, 65 insertions(+)
  create mode 100644 tests/avocado/riscv_opensbi.py

diff --git a/tests/avocado/riscv_opensbi.py b/tests/avocado/riscv_opensbi.py
new file mode 100644
index 00..abc99ced30
--- /dev/null
+++ b/tests/avocado/riscv_opensbi.py
@@ -0,0 +1,65 @@
+# opensbi boot test for RISC-V machines
+#
+# Copyright (c) 2022, Ventana Micro
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later.  See the COPYING file in the top-level directory.
+
+from avocado_qemu import QemuSystemTest
+from avocado_qemu import wait_for_console_pattern
+
+class RiscvOpensbi(QemuSystemTest):
+"""
+:avocado: tags=accel:tcg
+"""
+timeout = 5
+
+def test_riscv64_virt(self):
+"""
+:avocado: tags=arch:riscv64
+:avocado: tags=machine:virt
+"""
+self.vm.set_console()
+self.vm.launch()
+wait_for_console_pattern(self, 'Platform Name')
+wait_for_console_pattern(self, 'Boot HART MEDELEG')
+
+def test_riscv64_spike(self):
+"""
+:avocado: tags=arch:riscv64
+:avocado: tags=machine:spike
+"""
+self.vm.set_console()
+self.vm.launch()
+wait_for_console_pattern(self, 'Platform Name')
+wait_for_console_pattern(self, 'Boot HART MEDELEG')
+
+def test_riscv64_sifive_u(self):
+"""
+:avocado: tags=arch:riscv64
+:avocado: tags=machine:sifive_u
+"""
+self.vm.set_console()
+self.vm.launch()
+wait_for_console_pattern(self, 'Platform Name')
+wait_for_console_pattern(self, 'Boot HART MEDELEG')
+
+def test_riscv32_virt(self):
+"""
+:avocado: tags=arch:riscv32
+:avocado: tags=machine:virt
+"""
+self.vm.set_console()
+self.vm.launch()
+wait_for_console_pattern(self, 'Platform Name')
+wait_for_console_pattern(self, 'Boot HART MEDELEG')

How about testing riscv32_spike too?



I didn't manage to make it work. This riscv64 spark command line boots opensbi:


$ ./qemu-system-riscv64 -nographic -display none -vga none -machine spike

OpenSBI v1.1
       _  _
  / __ \  / |  _ \_   _|
 | |  | |_ __   ___ _ __ | (___ | |_) || |
 | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
 | |__| | |_) |  __/ | | |) | |_) || |_
  \/| .__/ \___|_| |_|_/|/_|
    | |
    |_|

(...)

The same command line doesn't boot riscv32 spark:

./qemu-system-riscv32 -nographic -display none -vga none -machine spike
(--- hangs indefinitely ---)

I debugged it a bit and, as far as boot code goes, it goes all the way and 
loads the
opensbi 32bit binary.

After that I tried to found any command line example that boots spike with 
riscv32
bit and didn't find any.  So I gave up digging it further because I became 
unsure
about whether 32-bit spike works.

If someone can verify that yes, 32-bit spike is supposed to work, then I 
believe it's
worth investigating why it's not the case ATM.


Thanks,


Daniel




+
+def test_riscv32_sifive_u(self):
+"""
+:avocado: tags=arch:riscv32
+:avocado: tags=machine:sifive_u
+"""
+self.vm.set_console()
+self.vm.launch()
+wait_for_console_pattern(self, 'Platform Name')
+wait_for_console_pattern(self, 'Boot HART MEDELEG')
--

Regards,
Bin

[PATCH v2 2/4] include/hw/ppc: Supply a few missing includes

[Markus Armbruster]

A few headers neglect to include headers they need.  They compile only
if something else includes the required header(s) first.  Fix that.

Signed-off-by: Markus Armbruster 
Reviewed-by: Cédric Le Goater 
Reviewed-by: Daniel Henrique Barboza 
---
 include/hw/ppc/pnv_lpc.h| 3 ++-
 include/hw/ppc/pnv_occ.h| 3 ++-
 include/hw/ppc/pnv_pnor.h   | 2 +-
 include/hw/ppc/pnv_sbe.h| 3 ++-
 include/hw/ppc/pnv_xscom.h  | 3 ++-
 include/hw/ppc/xive2.h  | 2 ++
 include/hw/ppc/xive2_regs.h | 2 ++
 7 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
index 8a8d1a3d42..001eee27d7 100644
--- a/include/hw/ppc/pnv_lpc.h
+++ b/include/hw/ppc/pnv_lpc.h
@@ -20,7 +20,8 @@
 #ifndef PPC_PNV_LPC_H
 #define PPC_PNV_LPC_H
 
-#include "qom/object.h"
+#include "exec/memory.h"
+#include "hw/qdev-core.h"
 
 #define TYPE_PNV_LPC "pnv-lpc"
 typedef struct PnvLpcClass PnvLpcClass;
diff --git a/include/hw/ppc/pnv_occ.h b/include/hw/ppc/pnv_occ.h
index 90a81dae2b..df321244e3 100644
--- a/include/hw/ppc/pnv_occ.h
+++ b/include/hw/ppc/pnv_occ.h
@@ -20,7 +20,8 @@
 #ifndef PPC_PNV_OCC_H
 #define PPC_PNV_OCC_H
 
-#include "qom/object.h"
+#include "exec/memory.h"
+#include "hw/qdev-core.h"
 
 #define TYPE_PNV_OCC "pnv-occ"
 OBJECT_DECLARE_TYPE(PnvOCC, PnvOCCClass,
diff --git a/include/hw/ppc/pnv_pnor.h b/include/hw/ppc/pnv_pnor.h
index bab2f79844..2e37ac88bf 100644
--- a/include/hw/ppc/pnv_pnor.h
+++ b/include/hw/ppc/pnv_pnor.h
@@ -10,7 +10,7 @@
 #ifndef PPC_PNV_PNOR_H
 #define PPC_PNV_PNOR_H
 
-#include "qom/object.h"
+#include "hw/sysbus.h"
 
 /*
  * PNOR offset on the LPC FW address space
diff --git a/include/hw/ppc/pnv_sbe.h b/include/hw/ppc/pnv_sbe.h
index f54a3ae9ba..b6b378ad14 100644
--- a/include/hw/ppc/pnv_sbe.h
+++ b/include/hw/ppc/pnv_sbe.h
@@ -20,7 +20,8 @@
 #ifndef PPC_PNV_SBE_H
 #define PPC_PNV_SBE_H
 
-#include "qom/object.h"
+#include "exec/memory.h"
+#include "hw/qdev-core.h"
 
 #define TYPE_PNV_SBE "pnv-sbe"
 OBJECT_DECLARE_TYPE(PnvSBE, PnvSBEClass, PNV_SBE)
diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
index c6e9ef8dd2..cbe848d27b 100644
--- a/include/hw/ppc/pnv_xscom.h
+++ b/include/hw/ppc/pnv_xscom.h
@@ -20,7 +20,8 @@
 #ifndef PPC_PNV_XSCOM_H
 #define PPC_PNV_XSCOM_H
 
-#include "qom/object.h"
+#include "exec/memory.h"
+#include "hw/ppc/pnv.h"
 
 typedef struct PnvXScomInterface PnvXScomInterface;
 
diff --git a/include/hw/ppc/xive2.h b/include/hw/ppc/xive2.h
index e9e3ea135e..ab68f8d157 100644
--- a/include/hw/ppc/xive2.h
+++ b/include/hw/ppc/xive2.h
@@ -11,7 +11,9 @@
 #ifndef PPC_XIVE2_H
 #define PPC_XIVE2_H
 
+#include "hw/ppc/xive.h"
 #include "hw/ppc/xive2_regs.h"
+#include "hw/sysbus.h"
 
 /*
  * XIVE2 Router (POWER10)
diff --git a/include/hw/ppc/xive2_regs.h b/include/hw/ppc/xive2_regs.h
index 14605bd458..b7adbdb7b9 100644
--- a/include/hw/ppc/xive2_regs.h
+++ b/include/hw/ppc/xive2_regs.h
@@ -10,6 +10,8 @@
 #ifndef PPC_XIVE2_REGS_H
 #define PPC_XIVE2_REGS_H
 
+#include "cpu.h"
+
 /*
  * Thread Interrupt Management Area (TIMA)
  *
-- 
2.38.1

[PATCH v2 0/4] hw/ppc: Clean up includes

[Markus Armbruster]

Back in 2016, we discussed[1] rules for headers, and these were
generally liked:

1. Have a carefully curated header that's included everywhere first.  We
   got that already thanks to Peter: osdep.h.

2. Headers should normally include everything they need beyond osdep.h.
   If exceptions are needed for some reason, they must be documented in
   the header.  If all that's needed from a header is typedefs, put
   those into qemu/typedefs.h instead of including the header.

3. Cyclic inclusion is forbidden.

After this series, include/hw/ppc and include/hw/pci-host/pnv* conform
to these rules.

It is based on

[PATCH v2 0/7] include/hw/pci include/hw/cxl: Clean up includes
[PATCH v2 0/3] block: Clean up includes
[PATCH v3 0/5] coroutine: Clean up includes

v2:
* PATCH 3: Drop an #include [Cédric]
* PATCH 4: Add it back, because now we actually need it

[1] Message-ID: <87h9g8j57d@blackfin.pond.sub.org>
https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg03345.html

Based-on: <20221222100330.380143-1-arm...@redhat.com>

Markus Armbruster (4):
  include/hw/ppc: Split pnv_chip.h off pnv.h
  include/hw/ppc: Supply a few missing includes
  include/hw/ppc: Don't include hw/pci-host/pnv_phb.h from pnv.h
  include/hw/ppc include/hw/pci-host: Drop extra typedefs

 hw/pci-host/pnv_phb.h  |   2 +-
 include/hw/pci-host/pnv_phb3.h |   1 -
 include/hw/pci-host/pnv_phb4.h |   5 +-
 include/hw/ppc/pnv.h   | 146 +---
 include/hw/ppc/pnv_chip.h  | 147 +
 include/hw/ppc/pnv_core.h  |   3 +-
 include/hw/ppc/pnv_homer.h |   2 +-
 include/hw/ppc/pnv_lpc.h   |  11 +--
 include/hw/ppc/pnv_occ.h   |   3 +-
 include/hw/ppc/pnv_pnor.h  |   2 +-
 include/hw/ppc/pnv_sbe.h   |   3 +-
 include/hw/ppc/pnv_xive.h  |   7 +-
 include/hw/ppc/pnv_xscom.h |   3 +-
 include/hw/ppc/xive2.h |   2 +
 include/hw/ppc/xive2_regs.h|   2 +
 hw/intc/pnv_xive.c |   1 +
 hw/intc/pnv_xive2.c|   1 +
 hw/pci-host/pnv_phb3.c |   1 +
 hw/pci-host/pnv_phb4_pec.c |   1 +
 hw/ppc/pnv.c   |   3 +
 hw/ppc/pnv_core.c  |   1 +
 hw/ppc/pnv_homer.c |   1 +
 hw/ppc/pnv_lpc.c   |   1 +
 hw/ppc/pnv_psi.c   |   1 +
 hw/ppc/pnv_xscom.c |   1 +
 25 files changed, 186 insertions(+), 165 deletions(-)
 create mode 100644 include/hw/ppc/pnv_chip.h

-- 
2.38.1

Re: [PATCH v2 0/7] include/hw/pci include/hw/cxl: Clean up includes

[Markus Armbruster]

Bernhard Beschow  writes:

> Am 22. Dezember 2022 10:03:23 UTC schrieb Markus Armbruster 
> :
>>Back in 2016, we discussed[1] rules for headers, and these were
>>generally liked:
>>
>>1. Have a carefully curated header that's included everywhere first.  We
>>   got that already thanks to Peter: osdep.h.
>>
>>2. Headers should normally include everything they need beyond osdep.h.
>>   If exceptions are needed for some reason, they must be documented in
>>   the header.  If all that's needed from a header is typedefs, put
>>   those into qemu/typedefs.h instead of including the header.
>>
>>3. Cyclic inclusion is forbidden.
>
> Sounds like these -- useful and sane -- rules belong in QEMU's coding style. 
> What about putting them there for easy reference?

Makes sense.  I'll see what I can do.  Thanks!

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

[Bernhard Beschow]

Am 22. Dezember 2022 09:01:34 UTC schrieb "Philippe Mathieu-Daudé" 
:
>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?
>
>>> 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.

I agree. At least it deserves a comment to preserve the original intention.

Best regards,
Bernhard

>-- >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();
> }
> d->Q(0) = r0;
> d->Q(1) = r1;
>---
>

[PATCH v2 4/4] include/hw/ppc include/hw/pci-host: Drop extra typedefs

[Markus Armbruster]

PnvChip is typedef'ed in five places, and PnvPhb4PecState in two.
Keep one, drop the others.

Signed-off-by: Markus Armbruster 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Cédric Le Goater 
Reviewed-by: Daniel Henrique Barboza 
---
 hw/pci-host/pnv_phb.h  | 2 +-
 include/hw/pci-host/pnv_phb3.h | 1 -
 include/hw/pci-host/pnv_phb4.h | 2 --
 include/hw/ppc/pnv_core.h  | 3 +--
 include/hw/ppc/pnv_homer.h | 2 +-
 include/hw/ppc/pnv_lpc.h   | 8 ++--
 include/hw/ppc/pnv_xive.h  | 7 +++
 7 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/hw/pci-host/pnv_phb.h b/hw/pci-host/pnv_phb.h
index 58ebd6dd0f..eb429d529f 100644
--- a/hw/pci-host/pnv_phb.h
+++ b/hw/pci-host/pnv_phb.h
@@ -12,9 +12,9 @@
 
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pcie_port.h"
+#include "hw/ppc/pnv.h"
 #include "qom/object.h"
 
-typedef struct PnvChip PnvChip;
 typedef struct PnvPhb4PecState PnvPhb4PecState;
 
 struct PnvPHB {
diff --git a/include/hw/pci-host/pnv_phb3.h b/include/hw/pci-host/pnv_phb3.h
index f791ebda9b..d62b3091ac 100644
--- a/include/hw/pci-host/pnv_phb3.h
+++ b/include/hw/pci-host/pnv_phb3.h
@@ -15,7 +15,6 @@
 #include "hw/pci-host/pnv_phb.h"
 
 typedef struct PnvPHB3 PnvPHB3;
-typedef struct PnvChip PnvChip;
 
 /*
  * PHB3 XICS Source for MSIs
diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index b4f2b29fb5..1f3237c9d5 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -16,10 +16,8 @@
 #include "hw/ppc/xive.h"
 #include "qom/object.h"
 
-typedef struct PnvPhb4PecState PnvPhb4PecState;
 typedef struct PnvPhb4PecStack PnvPhb4PecStack;
 typedef struct PnvPHB4 PnvPHB4;
-typedef struct PnvChip PnvChip;
 
 /*
  * We have one such address space wrapper per possible device under
diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
index c22eab2e1f..3d75706e95 100644
--- a/include/hw/ppc/pnv_core.h
+++ b/include/hw/ppc/pnv_core.h
@@ -22,14 +22,13 @@
 
 #include "hw/cpu/core.h"
 #include "target/ppc/cpu.h"
+#include "hw/ppc/pnv.h"
 #include "qom/object.h"
 
 #define TYPE_PNV_CORE "powernv-cpu-core"
 OBJECT_DECLARE_TYPE(PnvCore, PnvCoreClass,
 PNV_CORE)
 
-typedef struct PnvChip PnvChip;
-
 struct PnvCore {
 /*< private >*/
 CPUCore parent_obj;
diff --git a/include/hw/ppc/pnv_homer.h b/include/hw/ppc/pnv_homer.h
index 07e8b19311..b1c5d498dc 100644
--- a/include/hw/ppc/pnv_homer.h
+++ b/include/hw/ppc/pnv_homer.h
@@ -39,7 +39,7 @@ DECLARE_INSTANCE_CHECKER(PnvHomer, PNV10_HOMER,
 struct PnvHomer {
 DeviceState parent;
 
-struct PnvChip *chip;
+PnvChip *chip;
 MemoryRegion pba_regs;
 MemoryRegion regs;
 };
diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
index 001eee27d7..5d22c45570 100644
--- a/include/hw/ppc/pnv_lpc.h
+++ b/include/hw/ppc/pnv_lpc.h
@@ -21,6 +21,7 @@
 #define PPC_PNV_LPC_H
 
 #include "exec/memory.h"
+#include "hw/ppc/pnv.h"
 #include "hw/qdev-core.h"
 
 #define TYPE_PNV_LPC "pnv-lpc"
@@ -93,13 +94,8 @@ struct PnvLpcClass {
 DeviceRealize parent_realize;
 };
 
-/*
- * Old compilers error on typdef forward declarations. Keep them happy.
- */
-struct PnvChip;
-
 ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error **errp);
-int pnv_dt_lpc(struct PnvChip *chip, void *fdt, int root_offset,
+int pnv_dt_lpc(PnvChip *chip, void *fdt, int root_offset,
uint64_t lpcm_addr, uint64_t lpcm_size);
 
 #endif /* PPC_PNV_LPC_H */
diff --git a/include/hw/ppc/pnv_xive.h b/include/hw/ppc/pnv_xive.h
index b5d91505e5..9c48430ee4 100644
--- a/include/hw/ppc/pnv_xive.h
+++ b/include/hw/ppc/pnv_xive.h
@@ -10,12 +10,11 @@
 #ifndef PPC_PNV_XIVE_H
 #define PPC_PNV_XIVE_H
 
+#include "hw/ppc/pnv.h"
 #include "hw/ppc/xive.h"
 #include "qom/object.h"
 #include "hw/ppc/xive2.h"
 
-struct PnvChip;
-
 #define TYPE_PNV_XIVE "pnv-xive"
 OBJECT_DECLARE_TYPE(PnvXive, PnvXiveClass,
 PNV_XIVE)
@@ -31,7 +30,7 @@ struct PnvXive {
 XiveRouterparent_obj;
 
 /* Owning chip */
-struct PnvChip *chip;
+PnvChip *chip;
 
 /* XSCOM addresses giving access to the controller registers */
 MemoryRegion  xscom_regs;
@@ -106,7 +105,7 @@ typedef struct PnvXive2 {
 Xive2Router   parent_obj;
 
 /* Owning chip */
-struct PnvChip *chip;
+PnvChip *chip;
 
 /* XSCOM addresses giving access to the controller registers */
 MemoryRegion  xscom_regs;
-- 
2.38.1

[PATCH v3 4/6] virtio-mem: Fail if a memory backend with "prealloc=on" is specified

[David Hildenbrand]

"prealloc=on" for the memory backend does not work as expected, as
virtio-mem will simply discard all preallocated memory immediately again.
In the best case, it's an expensive NOP. In the worst case, it's an
unexpected allocation error.

Instead, "prealloc=on" should be specified for the virtio-mem device only,
such that virtio-mem will try preallocating memory before plugging
memory dynamically to the guest. Fail if such a memory backend is
provided.

Tested-by: Michal Privoznik 
Signed-off-by: David Hildenbrand 
---
 hw/virtio/virtio-mem.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 5c22c4b876..e0e908d392 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -772,6 +772,12 @@ static void virtio_mem_device_realize(DeviceState *dev, 
Error **errp)
 error_setg(errp, "'%s' property specifies an unsupported memdev",
VIRTIO_MEM_MEMDEV_PROP);
 return;
+} else if (vmem->memdev->prealloc) {
+error_setg(errp, "'%s' property specifies a memdev with preallocation"
+   " enabled: %s. Instead, specify 'prealloc=on' for the"
+   " virtio-mem device. ", VIRTIO_MEM_MEMDEV_PROP,
+   object_get_canonical_path_component(OBJECT(vmem->memdev)));
+return;
 }
 
 if ((nb_numa_nodes && vmem->node >= nb_numa_nodes) ||
-- 
2.38.1

[PATCH v2 1/4] include/hw/ppc: Split pnv_chip.h off pnv.h

[Markus Armbruster]

PnvChipClass, PnvChip, Pnv8Chip, Pnv9Chip, and Pnv10Chip are defined
in pnv.h.  Many users of the header don't actually need them.  One
instance is this inclusion loop: hw/ppc/pnv_homer.h includes
hw/ppc/pnv.h for typedef PnvChip, and vice versa for struct PnvHomer.

Similar structs live in their own headers: PnvHomerClass and PnvHomer
in pnv_homer.h, PnvLpcClass and PnvLpcController in pci_lpc.h,
PnvPsiClass, PnvPsi, Pnv8Psi, Pnv9Psi, Pnv10Psi in pnv_psi.h, ...

Move PnvChipClass, PnvChip, Pnv8Chip, Pnv9Chip, and Pnv10Chip to new
pnv_chip.h, and adjust include directives.  This breaks the inclusion
loop mentioned above.

Signed-off-by: Markus Armbruster 
Reviewed-by: Cédric Le Goater 
Reviewed-by: Daniel Henrique Barboza 
---
 include/hw/ppc/pnv.h   | 143 +---
 include/hw/ppc/pnv_chip.h  | 147 +
 hw/intc/pnv_xive.c |   1 +
 hw/intc/pnv_xive2.c|   1 +
 hw/pci-host/pnv_phb3.c |   1 +
 hw/pci-host/pnv_phb4_pec.c |   1 +
 hw/ppc/pnv.c   |   3 +
 hw/ppc/pnv_core.c  |   1 +
 hw/ppc/pnv_homer.c |   1 +
 hw/ppc/pnv_lpc.c   |   1 +
 hw/ppc/pnv_xscom.c |   1 +
 11 files changed, 160 insertions(+), 141 deletions(-)
 create mode 100644 include/hw/ppc/pnv_chip.h

diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 9ef7e2d0dc..ca49e4281d 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -20,158 +20,19 @@
 #ifndef PPC_PNV_H
 #define PPC_PNV_H
 
+#include "cpu.h"
 #include "hw/boards.h"
 #include "hw/sysbus.h"
 #include "hw/ipmi/ipmi.h"
-#include "hw/ppc/pnv_lpc.h"
 #include "hw/ppc/pnv_pnor.h"
-#include "hw/ppc/pnv_psi.h"
-#include "hw/ppc/pnv_occ.h"
-#include "hw/ppc/pnv_sbe.h"
-#include "hw/ppc/pnv_homer.h"
-#include "hw/ppc/pnv_xive.h"
-#include "hw/ppc/pnv_core.h"
-#include "hw/pci-host/pnv_phb3.h"
-#include "hw/pci-host/pnv_phb4.h"
 #include "hw/pci-host/pnv_phb.h"
-#include "qom/object.h"
 
 #define TYPE_PNV_CHIP "pnv-chip"
-OBJECT_DECLARE_TYPE(PnvChip, PnvChipClass,
-PNV_CHIP)
 
-struct PnvChip {
-/*< private >*/
-SysBusDevice parent_obj;
-
-/*< public >*/
-uint32_t chip_id;
-uint64_t ram_start;
-uint64_t ram_size;
-
-uint32_t nr_cores;
-uint32_t nr_threads;
-uint64_t cores_mask;
-PnvCore  **cores;
-
-uint32_t num_pecs;
-
-MemoryRegion xscom_mmio;
-MemoryRegion xscom;
-AddressSpace xscom_as;
-
-MemoryRegion *fw_mr;
-gchar*dt_isa_nodename;
-};
-
-#define TYPE_PNV8_CHIP "pnv8-chip"
+typedef struct PnvChip PnvChip;
 typedef struct Pnv8Chip Pnv8Chip;
-DECLARE_INSTANCE_CHECKER(Pnv8Chip, PNV8_CHIP,
- TYPE_PNV8_CHIP)
-
-struct Pnv8Chip {
-/*< private >*/
-PnvChip  parent_obj;
-
-/*< public >*/
-MemoryRegion icp_mmio;
-
-PnvLpcController lpc;
-Pnv8Psi  psi;
-PnvOCC   occ;
-PnvHomer homer;
-
-#define PNV8_CHIP_PHB3_MAX 4
-/*
- * The array is used to allow quick access to the phbs by
- * pnv_ics_get_child() and pnv_ics_resend_child().
- */
-PnvPHB   *phbs[PNV8_CHIP_PHB3_MAX];
-uint32_t num_phbs;
-
-XICSFabric*xics;
-};
-
-#define TYPE_PNV9_CHIP "pnv9-chip"
 typedef struct Pnv9Chip Pnv9Chip;
-DECLARE_INSTANCE_CHECKER(Pnv9Chip, PNV9_CHIP,
- TYPE_PNV9_CHIP)
-
-struct Pnv9Chip {
-/*< private >*/
-PnvChip  parent_obj;
-
-/*< public >*/
-PnvXive  xive;
-Pnv9Psi  psi;
-PnvLpcController lpc;
-PnvOCC   occ;
-PnvSBE   sbe;
-PnvHomer homer;
-
-uint32_t nr_quads;
-PnvQuad  *quads;
-
-#define PNV9_CHIP_MAX_PEC 3
-PnvPhb4PecState pecs[PNV9_CHIP_MAX_PEC];
-};
-
-/*
- * A SMT8 fused core is a pair of SMT4 cores.
- */
-#define PNV9_PIR2FUSEDCORE(pir) (((pir) >> 3) & 0xf)
-#define PNV9_PIR2CHIP(pir)  (((pir) >> 8) & 0x7f)
-
-#define TYPE_PNV10_CHIP "pnv10-chip"
 typedef struct Pnv10Chip Pnv10Chip;
-DECLARE_INSTANCE_CHECKER(Pnv10Chip, PNV10_CHIP,
- TYPE_PNV10_CHIP)
-
-struct Pnv10Chip {
-/*< private >*/
-PnvChip  parent_obj;
-
-/*< public >*/
-PnvXive2 xive;
-Pnv9Psi  psi;
-PnvLpcController lpc;
-PnvOCC   occ;
-PnvSBE   sbe;
-PnvHomer homer;
-
-uint32_t nr_quads;
-PnvQuad  *quads;
-
-#define PNV10_CHIP_MAX_PEC 2
-PnvPhb4PecState pecs[PNV10_CHIP_MAX_PEC];
-};
-
-#define PNV10_PIR2FUSEDCORE(pir) (((pir) >> 3) & 0xf)
-#define PNV10_PIR2CHIP(pir)  (((pir) >> 8) & 0x7f)
-
-struct PnvChipClass {
-/*< private >*/
-SysBusDeviceClass parent_class;
-
-/*< public >*/
-uint64_t chip_cfam_id;
-uint64_t cores_mask;
-uint32_t num_pecs;
-uint32_t num_phbs;
-
-DeviceRealize parent_realize;
-
-uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
-void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Err

[PATCH v3 0/6] virtio-mem: Handle preallocation with migration

[David Hildenbrand]

While playing with migration of virtio-mem with an ordinary file backing,
I realized that migration and prealloc doesn't currently work as expected
for virtio-mem. Further, Jing Qi reported that setup issues (insufficient
huge pages on the destination) result in QEMU getting killed with SIGBUS
instead of failing gracefully.

In contrast to ordinary memory backend preallocation, virtio-mem
preallocates memory before plugging blocks to the guest. Consequently,
when migrating we are not actually preallocating on the destination but
"only" migrate pages. Fix that be migrating the bitmap early, before any
RAM content, and use that information to preallocate memory early, before
migrating any RAM.

Postcopy needs some extra care, and I realized that prealloc+postcopy is
shaky in general. Let's at least try to mimic what ordinary
prealloc+postcopy does: temporarily allocate the memory, discard it, and
cross fingers that we'll still have sufficient memory when postcopy
actually tries placing pages.

Cc: Dr. David Alan Gilbert 
Cc: Juan Quintela 
Cc: Peter Xu 
Cc: Michael S. Tsirkin 
Cc: Michal Privoznik 

v2 -> v3:
- New approach/rewrite, drop RB and TB of last patch

v1 -> v2:
- Added RBs and Tested-bys
- "virtio-mem: Fail if a memory backend with "prealloc=on" is specified"
-- Fail instead of warn
-- Adjust subject/description

David Hildenbrand (6):
  migration: Allow immutable device state to be migrated early (i.e.,
before RAM)
  migration/vmstate: Introduce VMSTATE_WITH_TMP_TEST() and
VMSTATE_BITMAP_TEST()
  migration: Factor out checks for advised and listening incomming
postcopy
  virtio-mem: Fail if a memory backend with "prealloc=on" is specified
  virtio-mem: Migrate bitmap, size and sanity checks early
  virtio-mem: Proper support for preallocation with migration

 hw/core/machine.c  |   4 +-
 hw/virtio/virtio-mem.c | 154 -
 include/hw/virtio/virtio-mem.h |   8 ++
 include/migration/misc.h   |   6 +-
 include/migration/vmstate.h|  19 +++-
 migration/migration.c  |  27 ++
 migration/migration.h  |   4 +
 migration/ram.c|  16 +---
 migration/savevm.c | 104 --
 migration/savevm.h |   1 +
 10 files changed, 298 insertions(+), 45 deletions(-)

-- 
2.38.1

[PATCH v3 5/6] virtio-mem: Migrate bitmap, size and sanity checks early

[David Hildenbrand]

The bitmap and the size are immutable while migration is active: see
virtio_mem_is_busy(). We can migrate this information early, before
migrating any actual RAM content.

Having this information in place early will, for example, allow for
properly preallocating memory before touching these memory locations
during RAM migration: this way, we can make sure that all memory was
actually preallocated and that any user errors (e.g., insufficient
hugetlb pages) can be handled gracefully.

In contrast, usable_region_size and requested_size can theoretically
still be modified on the source while the VM is running. Keep migrating
these properties the usual, late, way.

Use a new device property to keep behavior of compat machines
unmodified.

Signed-off-by: David Hildenbrand 
---
 hw/core/machine.c  |  4 ++-
 hw/virtio/virtio-mem.c | 51 --
 include/hw/virtio/virtio-mem.h |  8 ++
 3 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index f589b92909..6532190412 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -40,7 +40,9 @@
 #include "hw/virtio/virtio-pci.h"
 #include "qom/object_interfaces.h"
 
-GlobalProperty hw_compat_7_2[] = {};
+GlobalProperty hw_compat_7_2[] = {
+{ "virtio-mem", "x-early-migration", "false" },
+};
 const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
 
 GlobalProperty hw_compat_7_1[] = {
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index e0e908d392..043b96f509 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -31,6 +31,8 @@
 #include CONFIG_DEVICES
 #include "trace.h"
 
+static const VMStateDescription vmstate_virtio_mem_device_early;
+
 /*
  * We only had legacy x86 guests that did not support
  * VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE. Other targets don't have legacy guests.
@@ -878,6 +880,10 @@ static void virtio_mem_device_realize(DeviceState *dev, 
Error **errp)
 
 host_memory_backend_set_mapped(vmem->memdev, true);
 vmstate_register_ram(&vmem->memdev->mr, DEVICE(vmem));
+if (vmem->early_migration) {
+vmstate_register(VMSTATE_IF(vmem), VMSTATE_INSTANCE_ID_ANY,
+ &vmstate_virtio_mem_device_early, vmem);
+}
 qemu_register_reset(virtio_mem_system_reset, vmem);
 
 /*
@@ -899,6 +905,10 @@ static void virtio_mem_device_unrealize(DeviceState *dev)
  */
 memory_region_set_ram_discard_manager(&vmem->memdev->mr, NULL);
 qemu_unregister_reset(virtio_mem_system_reset, vmem);
+if (vmem->early_migration) {
+vmstate_unregister(VMSTATE_IF(vmem), &vmstate_virtio_mem_device_early,
+   vmem);
+}
 vmstate_unregister_ram(&vmem->memdev->mr, DEVICE(vmem));
 host_memory_backend_set_mapped(vmem->memdev, false);
 virtio_del_queue(vdev, 0);
@@ -1015,18 +1025,53 @@ static const VMStateDescription 
vmstate_virtio_mem_sanity_checks = {
 },
 };
 
+static bool virtio_mem_vmstate_field_exists(void *opaque, int version_id)
+{
+const VirtIOMEM *vmem = VIRTIO_MEM(opaque);
+
+/* With early migration, these fields were already migrated. */
+return !vmem->early_migration;
+}
+
 static const VMStateDescription vmstate_virtio_mem_device = {
 .name = "virtio-mem-device",
 .minimum_version_id = 1,
 .version_id = 1,
 .priority = MIG_PRI_VIRTIO_MEM,
 .post_load = virtio_mem_post_load,
+.fields = (VMStateField[]) {
+VMSTATE_WITH_TMP_TEST(VirtIOMEM, virtio_mem_vmstate_field_exists,
+  VirtIOMEMMigSanityChecks,
+  vmstate_virtio_mem_sanity_checks),
+VMSTATE_UINT64(usable_region_size, VirtIOMEM),
+VMSTATE_UINT64_TEST(size, VirtIOMEM, virtio_mem_vmstate_field_exists),
+VMSTATE_UINT64(requested_size, VirtIOMEM),
+VMSTATE_BITMAP_TEST(bitmap, VirtIOMEM, virtio_mem_vmstate_field_exists,
+0, bitmap_size),
+VMSTATE_END_OF_LIST()
+},
+};
+
+/*
+ * Transfer properties that are immutable while migration is active early,
+ * such that we have have this information around before migrating any RAM
+ * content.
+ *
+ * Note that virtio_mem_is_busy() makes sure these properties can no longer
+ * change on the migration source until migration completed.
+ *
+ * With QEMU compat machines, we transmit these properties later, via
+ * vmstate_virtio_mem_device instead -- see virtio_mem_vmstate_field_exists().
+ */
+static const VMStateDescription vmstate_virtio_mem_device_early = {
+.name = "virtio-mem-device-early",
+.minimum_version_id = 1,
+.version_id = 1,
+.priority = MIG_PRI_POST_SETUP,
 .fields = (VMStateField[]) {
 VMSTATE_WITH_TMP(VirtIOMEM, VirtIOMEMMigSanityChecks,
  vmstate_virtio_mem_sanity_checks),
-VMSTATE_UINT64(usable_region_size, VirtIOMEM),
 VMSTATE_UINT64(size, VirtIOMEM),
-VMSTATE_UINT64(requested_size, VirtIOMEM),

[PATCH v3 3/6] migration: Factor out checks for advised and listening incomming postcopy

[David Hildenbrand]

Let's factor out both checks, to be used in virtio-mem context next.

While at it, fix a spelling error in a related comment.

Signed-off-by: David Hildenbrand 
---
 include/migration/misc.h |  6 +-
 migration/migration.c| 14 ++
 migration/ram.c  | 16 ++--
 3 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 465906710d..c7e67a6804 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -67,8 +67,12 @@ bool migration_has_failed(MigrationState *);
 /* ...and after the device transmission */
 bool migration_in_postcopy_after_devices(MigrationState *);
 void migration_global_dump(Monitor *mon);
-/* True if incomming migration entered POSTCOPY_INCOMING_DISCARD */
+/* True if incoming migration entered POSTCOPY_INCOMING_DISCARD */
 bool migration_in_incoming_postcopy(void);
+/* True if incoming migration entered POSTCOPY_INCOMING_ADVISE */
+bool migration_incoming_postcopy_advised(void);
+/* True if incoming migration entered POSTCOPY_INCOMING_LISTENING */
+bool migration_incoming_postcopy_listening(void);
 /* True if background snapshot is active */
 bool migration_in_bg_snapshot(void);
 
diff --git a/migration/migration.c b/migration/migration.c
index 78b6bb8765..7a69bb93b0 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2094,6 +2094,20 @@ bool migration_in_incoming_postcopy(void)
 return ps >= POSTCOPY_INCOMING_DISCARD && ps < POSTCOPY_INCOMING_END;
 }
 
+bool migration_incoming_postcopy_advised(void)
+{
+PostcopyState ps = postcopy_state_get();
+
+return ps >= POSTCOPY_INCOMING_ADVISE && ps < POSTCOPY_INCOMING_END;
+}
+
+bool migration_incoming_postcopy_listening(void)
+{
+PostcopyState ps = postcopy_state_get();
+
+return ps >= POSTCOPY_INCOMING_LISTENING && ps < POSTCOPY_INCOMING_END;
+}
+
 bool migration_in_bg_snapshot(void)
 {
 MigrationState *s = migrate_get_current();
diff --git a/migration/ram.c b/migration/ram.c
index 334309f1c6..44b063eccd 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4091,18 +4091,6 @@ int ram_load_postcopy(QEMUFile *f, int channel)
 return ret;
 }
 
-static bool postcopy_is_advised(void)
-{
-PostcopyState ps = postcopy_state_get();
-return ps >= POSTCOPY_INCOMING_ADVISE && ps < POSTCOPY_INCOMING_END;
-}
-
-static bool postcopy_is_running(void)
-{
-PostcopyState ps = postcopy_state_get();
-return ps >= POSTCOPY_INCOMING_LISTENING && ps < POSTCOPY_INCOMING_END;
-}
-
 /*
  * Flush content of RAM cache into SVM's memory.
  * Only flush the pages that be dirtied by PVM or SVM or both.
@@ -4167,7 +4155,7 @@ static int ram_load_precopy(QEMUFile *f)
 MigrationIncomingState *mis = migration_incoming_get_current();
 int flags = 0, ret = 0, invalid_flags = 0, len = 0, i = 0;
 /* ADVISE is earlier, it shows the source has the postcopy capability on */
-bool postcopy_advised = postcopy_is_advised();
+bool postcopy_advised = migration_incoming_postcopy_advised();
 if (!migrate_use_compression()) {
 invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE;
 }
@@ -4365,7 +4353,7 @@ static int ram_load(QEMUFile *f, void *opaque, int 
version_id)
  * If system is running in postcopy mode, page inserts to host memory must
  * be atomic
  */
-bool postcopy_running = postcopy_is_running();
+bool postcopy_running = migration_incoming_postcopy_listening();
 
 seq_iter++;
 
-- 
2.38.1

[PATCH v3 1/6] migration: Allow immutable device state to be migrated early (i.e., before RAM)

[David Hildenbrand]

For virtio-mem, we want to have the plugged/unplugged state of memory
blocks available before migrating any actual RAM content. This
information is immutable on the migration source while migration is active,

For example, we want to use this information for proper preallocation
support with migration: currently, we don't preallocate memory on the
migration target, and especially with hugetlb, we can easily run out of
hugetlb pages during RAM migration and will crash (SIGBUS) instead of
catching this gracefully via preallocation.

Migrating device state before we start iterating is currently impossible.
Introduce and use qemu_savevm_state_start_precopy(), and use
a new special migration priority -- MIG_PRI_POST_SETUP -- to decide whether
state will be saved in qemu_savevm_state_start_precopy() or in
qemu_savevm_state_complete_precopy_*().

We have to take care of properly including the early device state in the
vmdesc. Relying on migrate_get_current() to temporarily store the vmdesc is
a bit sub-optimal, but we use that explicitly or implicitly all over the
place already, so this barely matters in practice.

Note that only very selected devices (i.e., ones seriously messing with
RAM setup) are supposed to make use of that.

Signed-off-by: David Hildenbrand 
---
 include/migration/vmstate.h |   7 +++
 migration/migration.c   |  13 +
 migration/migration.h   |   4 ++
 migration/savevm.c  | 104 +++-
 migration/savevm.h  |   1 +
 5 files changed, 104 insertions(+), 25 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index ad24aa1934..79eb2409a2 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -156,6 +156,13 @@ typedef enum {
 MIG_PRI_VIRTIO_MEM, /* Must happen before IOMMU */
 MIG_PRI_GICV3_ITS,  /* Must happen before PCI devices */
 MIG_PRI_GICV3,  /* Must happen before the ITS */
+/*
+ * Must happen before all other devices (iterable and non-iterable),
+ * especiall, before migrating RAM content. Such device state must be
+ * guaranteed to be immutable on the migration source until migration
+ * ends and must not depend on the CPU state to be synchronized.
+ */
+MIG_PRI_POST_SETUP,
 MIG_PRI_MAX,
 } MigrationPriority;
 
diff --git a/migration/migration.c b/migration/migration.c
index 52b5d39244..78b6bb8765 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2170,6 +2170,9 @@ void migrate_init(MigrationState *s)
 s->vm_was_running = false;
 s->iteration_initial_bytes = 0;
 s->threshold_size = 0;
+
+json_writer_free(s->vmdesc);
+s->vmdesc = NULL;
 }
 
 int migrate_add_blocker_internal(Error *reason, Error **errp)
@@ -3997,6 +4000,9 @@ static void *migration_thread(void *opaque)
 
 trace_migration_thread_setup_complete();
 
+/* Process early data that has to get migrated before iterating. */
+qemu_savevm_state_start_precopy(s->to_dst_file);
+
 while (migration_is_active(s)) {
 if (urgent || !qemu_file_rate_limit(s->to_dst_file)) {
 MigIterateState iter_state = migration_iteration_run(s);
@@ -4125,6 +4131,12 @@ static void *bg_migration_thread(void *opaque)
 if (vm_stop_force_state(RUN_STATE_PAUSED)) {
 goto fail;
 }
+
+/* Migrate early data that would usually get migrated before iterating. */
+if (qemu_savevm_state_start_precopy(fb)) {
+goto fail;
+}
+
 /*
  * Put vCPUs in sync with shadow context structures, then
  * save their state to channel-buffer along with devices.
@@ -4445,6 +4457,7 @@ static void migration_instance_finalize(Object *obj)
 qemu_sem_destroy(&ms->rp_state.rp_sem);
 qemu_sem_destroy(&ms->postcopy_qemufile_src_sem);
 error_free(ms->error);
+json_writer_free(ms->vmdesc);
 }
 
 static void migration_instance_init(Object *obj)
diff --git a/migration/migration.h b/migration/migration.h
index ae4ffd3454..66511ce532 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -17,6 +17,7 @@
 #include "exec/cpu-common.h"
 #include "hw/qdev-core.h"
 #include "qapi/qapi-types-migration.h"
+#include "qapi/qmp/json-writer.h"
 #include "qemu/thread.h"
 #include "qemu/coroutine_int.h"
 #include "io/channel.h"
@@ -366,6 +367,9 @@ struct MigrationState {
  * This save hostname when out-going migration starts
  */
 char *hostname;
+
+/* QEMU_VM_VMDESCRIPTION content filled for all non-iterable devices. */
+JSONWriter *vmdesc;
 };
 
 void migrate_set_state(int *state, int old_state, int new_state);
diff --git a/migration/savevm.c b/migration/savevm.c
index a0cdb714f7..b810409574 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -42,7 +42,6 @@
 #include "postcopy-ram.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-migration.h"
-#include "qapi/qmp/json-writer.h"
 #include "qapi/clone-visitor.h"
 #include "qapi/qapi-builtin-visit.h"
 #include

[PATCH v3 6/6] virtio-mem: Proper support for preallocation with migration

[David Hildenbrand]

Ordinary memory preallocation runs when QEMU starts up and creates the
memory backends, before processing the incoming migration stream. With
virtio-mem, we don't know which memory blocks to preallocate before
migration started. Now that we migrate the virtio-mem bitmap early, before
migrating any RAM content, we can safely preallocate memory for all plugged
memory blocks before migrating any RAM content.

This is especially relevant for the following cases:

(1) User errors

With hugetlb/files, if we don't have sufficient backend memory available on
the migration destination, we'll crash QEMU (SIGBUS) during RAM migration
when running out of backend memory. Preallocating memory before actual
RAM migration allows for failing gracefully and informing the user about
the setup problem.

(2) Excluded memory ranges during migration

For example, virtio-balloon free page hinting will exclude some pages
from getting migrated. In that case, we won't crash during RAM
migration, but later, when running the VM on the destination, which is
bad.

To fix this for new QEMU machines that migrate the bitmap early,
preallocate the memory early, before any RAM migration. Warn with old
QEMU machines.

Getting postcopy right is a bit tricky, but we essentially now implement
the same (problematic) preallocation logic as ordinary preallocation:
preallocate memory early and discard it again before precopy starts. During
ordinary preallocation, discarding of RAM happens when postcopy is advised.
As the our state (bitmap) is loaded after postcopy was advised but before
postcopy starts listening, we have to discard memory we preallocated
immediately again ourselves.

Note that nothing (not even hugetlb reservations) guarantees for postcopy
that backend memory (especially, hugetlb pages) are still free after they
were freed ones while discarding RAM. Still, allocating that memory at
least once helps catching some basic setup problems.

Before this change, trying to restore a VM when insufficient hugetlb
pages are around results in the process crashing to to a "Bus error"
(SIGBUS). With this change, QEMU fails gracefully:

  qemu-system-x86_64: qemu_prealloc_mem: preallocating memory failed: Bad 
address
  qemu-system-x86_64: error while loading state for instance 0x0 of device 
':00:03.0/virtio-mem-device-early'
  qemu-system-x86_64: load of migration failed: Cannot allocate memory

Reported-by: Jing Qi 
Signed-off-by: David Hildenbrand 
---
 hw/virtio/virtio-mem.c | 97 ++
 1 file changed, 97 insertions(+)

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 043b96f509..c1cf448046 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -204,6 +204,30 @@ static int virtio_mem_for_each_unplugged_range(const 
VirtIOMEM *vmem, void *arg,
 return ret;
 }
 
+static int virtio_mem_for_each_plugged_range(const VirtIOMEM *vmem, void *arg,
+ virtio_mem_range_cb cb)
+{
+unsigned long first_bit, last_bit;
+uint64_t offset, size;
+int ret = 0;
+
+first_bit = find_first_bit(vmem->bitmap, vmem->bitmap_size);
+while (first_bit < vmem->bitmap_size) {
+offset = first_bit * vmem->block_size;
+last_bit = find_next_zero_bit(vmem->bitmap, vmem->bitmap_size,
+  first_bit + 1) - 1;
+size = (last_bit - first_bit + 1) * vmem->block_size;
+
+ret = cb(vmem, arg, offset, size);
+if (ret) {
+break;
+}
+first_bit = find_next_bit(vmem->bitmap, vmem->bitmap_size,
+  last_bit + 2);
+}
+return ret;
+}
+
 /*
  * Adjust the memory section to cover the intersection with the given range.
  *
@@ -938,6 +962,10 @@ static int virtio_mem_post_load(void *opaque, int 
version_id)
 RamDiscardListener *rdl;
 int ret;
 
+if (vmem->prealloc && !vmem->early_migration) {
+warn_report("Proper preallocation with migration requires a newer QEMU 
machine");
+}
+
 /*
  * We started out with all memory discarded and our memory region is mapped
  * into an address space. Replay, now that we updated the bitmap.
@@ -957,6 +985,74 @@ static int virtio_mem_post_load(void *opaque, int 
version_id)
 return virtio_mem_restore_unplugged(vmem);
 }
 
+static int virtio_mem_prealloc_range_cb(const VirtIOMEM *vmem, void *arg,
+uint64_t offset, uint64_t size)
+{
+void *area = memory_region_get_ram_ptr(&vmem->memdev->mr) + offset;
+int fd = memory_region_get_fd(&vmem->memdev->mr);
+Error *local_err = NULL;
+
+qemu_prealloc_mem(fd, area, size, 1, NULL, &local_err);
+if (local_err) {
+error_report_err(local_err);
+return -ENOMEM;
+}
+return 0;
+}
+
+static int virtio_mem_post_load_early(void *opaque, int version_id)
+{
+VirtIOMEM *vmem = VIRTIO_MEM(opaque);
+RAMBlock *rb = vmem->memdev->mr.ram_block

[PATCH v3 2/6] migration/vmstate: Introduce VMSTATE_WITH_TMP_TEST() and VMSTATE_BITMAP_TEST()

[David Hildenbrand]

We'll make use of both next in the context of virtio-mem.

Signed-off-by: David Hildenbrand 
---
 include/migration/vmstate.h | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 79eb2409a2..73ad1ae0eb 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -712,8 +712,9 @@ extern const VMStateInfo vmstate_info_qlist;
  *'_state' type
  *That the pointer is right at the start of _tmp_type.
  */
-#define VMSTATE_WITH_TMP(_state, _tmp_type, _vmsd) { \
+#define VMSTATE_WITH_TMP_TEST(_state, _test, _tmp_type, _vmsd) { \
 .name = "tmp",   \
+.field_exists = (_test), \
 .size = sizeof(_tmp_type) +  \
 QEMU_BUILD_BUG_ON_ZERO(offsetof(_tmp_type, parent) != 0) + 
\
 type_check_pointer(_state,   \
@@ -722,6 +723,9 @@ extern const VMStateInfo vmstate_info_qlist;
 .info = &vmstate_info_tmp,   \
 }
 
+#define VMSTATE_WITH_TMP(_state, _tmp_type, _vmsd) \
+VMSTATE_WITH_TMP_TEST(_state, NULL, _tmp_type, _vmsd)
+
 #define VMSTATE_UNUSED_BUFFER(_test, _version, _size) {  \
 .name = "unused",\
 .field_exists = (_test), \
@@ -745,8 +749,9 @@ extern const VMStateInfo vmstate_info_qlist;
 /* _field_size should be a int32_t field in the _state struct giving the
  * size of the bitmap _field in bits.
  */
-#define VMSTATE_BITMAP(_field, _state, _version, _field_size) {  \
+#define VMSTATE_BITMAP_TEST(_field, _state, _test, _version, _field_size) { \
 .name = (stringify(_field)), \
+.field_exists = (_test), \
 .version_id   = (_version),  \
 .size_offset  = vmstate_offset_value(_state, _field_size, int32_t),\
 .info = &vmstate_info_bitmap,\
@@ -754,6 +759,9 @@ extern const VMStateInfo vmstate_info_qlist;
 .offset   = offsetof(_state, _field),\
 }
 
+#define VMSTATE_BITMAP(_field, _state, _version, _field_size) \
+VMSTATE_BITMAP_TEST(_field, _state, NULL, _version, _field_size)
+
 /* For migrating a QTAILQ.
  * Target QTAILQ needs be properly initialized.
  * _type: type of QTAILQ element
-- 
2.38.1

[PATCH v2 3/4] include/hw/ppc: Don't include hw/pci-host/pnv_phb.h from pnv.h

[Markus Armbruster]

The next commit needs to include hw/ppc/pnv.h from
hw/pci-host/pnv_phb.h.  Avoid an inclusion loop.

Signed-off-by: Markus Armbruster 
Reviewed-by: Cédric Le Goater 
Reviewed-by: Daniel Henrique Barboza 
---
 include/hw/pci-host/pnv_phb4.h | 3 ++-
 include/hw/ppc/pnv.h   | 3 ++-
 hw/ppc/pnv_psi.c   | 1 +
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
index d9cea3f952..b4f2b29fb5 100644
--- a/include/hw/pci-host/pnv_phb4.h
+++ b/include/hw/pci-host/pnv_phb4.h
@@ -10,14 +10,15 @@
 #ifndef PCI_HOST_PNV_PHB4_H
 #define PCI_HOST_PNV_PHB4_H
 
+#include "hw/pci-host/pnv_phb.h"
 #include "hw/pci/pci_bus.h"
+#include "hw/ppc/pnv.h"
 #include "hw/ppc/xive.h"
 #include "qom/object.h"
 
 typedef struct PnvPhb4PecState PnvPhb4PecState;
 typedef struct PnvPhb4PecStack PnvPhb4PecStack;
 typedef struct PnvPHB4 PnvPHB4;
-typedef struct PnvPHB PnvPHB;
 typedef struct PnvChip PnvChip;
 
 /*
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index ca49e4281d..96fb850419 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -25,7 +25,6 @@
 #include "hw/sysbus.h"
 #include "hw/ipmi/ipmi.h"
 #include "hw/ppc/pnv_pnor.h"
-#include "hw/pci-host/pnv_phb.h"
 
 #define TYPE_PNV_CHIP "pnv-chip"
 
@@ -59,6 +58,8 @@ DECLARE_INSTANCE_CHECKER(PnvChip, PNV_CHIP_POWER10,
 
 PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
 
+typedef struct PnvPHB PnvPHB;
+
 #define TYPE_PNV_MACHINE   MACHINE_TYPE_NAME("powernv")
 typedef struct PnvMachineClass PnvMachineClass;
 typedef struct PnvMachineState PnvMachineState;
diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index 98045ed3d2..8aa09ab26b 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -18,6 +18,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "exec/address-spaces.h"
 #include "hw/irq.h"
 #include "target/ppc/cpu.h"
 #include "qemu/log.h"
-- 
2.38.1

Re: [PATCH] dockerfiles: update to Fedora 36

[Daniel P . Berrangé]

On Thu, Dec 22, 2022 at 09:27:56AM +0100, Paolo Bonzini wrote:
> lcitool has removed the fedora-35 target, so let's follow
> suit.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  tests/docker/dockerfiles/fedora-win32-cross.docker | 4 ++--
>  tests/docker/dockerfiles/fedora-win64-cross.docker | 4 ++--
>  tests/docker/dockerfiles/fedora.docker | 4 ++--
>  tests/lcitool/libvirt-ci   | 2 +-
>  tests/lcitool/refresh  | 6 +++---
>  5 files changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[Daniel P . Berrangé]

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.


With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PULL 0/6] testing updates

[Peter Maydell]

On Wed, 21 Dec 2022 at 14:40, Alex Bennée  wrote:
>
> The following changes since commit 8540a1f69578afb3b37866b1ce5bec46a9f6efbc:
>
>   Merge tag 'hppa-fixes-pull-request' of https://github.com/hdeller/qemu-hppa 
> into staging (2022-12-20 15:32:27 +)
>
> are available in the Git repository at:
>
>   https://gitlab.com/stsquad/qemu.git tags/pull-testing-next-211222-1
>
> for you to fetch changes up to 7a8ec48480c116db93e0d91688be1dcf34147795:
>
>   gitlab-ci: Disable docs and GUIs for the build-tci and build-tcg-disabled 
> jobs (2022-12-21 11:19:05 +)
>
> 
> testing updates:
>
>   - fix minor shell-ism that can break check-tcg
>   - turn off verbose logging on custom runners
>   - make configure echo call in CI
>   - fix unused variable in linux-test
>   - add binary compiler docker image for hexagon
>   - disable doc and gui builds for tci and disable-tcg builds
>

Gitlab's CI marks this with a "yaml invalid" tag:
https://gitlab.com/qemu-project/qemu/-/pipelines/729324088

and the error:
'build-user-hexagon' job needs 'hexagon-cross-container' job, but
'hexagon-cross-container' is not in any previous stage

thanks
-- PMM

Re: [PATCH v3 4/5] coroutine: Split qemu/coroutine-core.h off qemu/coroutine.h

[Paolo Bonzini]

On 12/22/22 09:56, Markus Armbruster wrote:

+/**
+ * Mark a function that executes in coroutine context
+ *
+ *
+ * Functions that execute in coroutine context cannot be called
+ * directly from normal functions.  Use @coroutine_fn to mark such
+ * functions.  For example:
+ *
+ *   static void coroutine_fn foo(void) {
+ *   
+ *   }
+ *
+ * In the future it would be nice to have the compiler or a static
+ * checker catch misuse of such functions.  This annotation might make
+ * it possible and in the meantime it serves as documentation.
+ */
+

Is it intentional that "#define coroutine_fn" is not here?

Yes: I moved it to qemu/osdep.h in PATCH 2, along with its doc comment.
To avoid compromising coroutine.h as overview documentation, I added
rephrased documentation there.


Got it.  coroutine.h is not included in the developer documentation so 
that didn't occur to me but I understand it now.


Paolo

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

[Eric Auger]

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.

Thanks

Eric

>
>
> With regards,
> Daniel

RE: [PULL 0/6] testing updates

[Mukilan Thiyagarajan (QUIC)]

I believe the error is caused by the QEMU_JOB_ONLY_FORKS: 1 line
which needs to be removed from the definition of hexagon-cross-container.

Alex, Peter, 

Shall I raise a patch to remove this line? Should the patch be
created against the testing/next branch?

Thanks,
Mukilan

-Original Message-
From: qemu-devel-bounces+quic_mthiyaga=quicinc@nongnu.org 
 On Behalf Of Peter 
Maydell
Sent: Thursday, December 22, 2022 4:45 PM
To: Alex Bennée 
Cc: qemu-devel@nongnu.org
Subject: Re: [PULL 0/6] testing updates

WARNING: This email originated from outside of Qualcomm. Please be wary of any 
links or attachments, and do not enable macros.

On Wed, 21 Dec 2022 at 14:40, Alex Bennée  wrote:
>
> The following changes since commit 8540a1f69578afb3b37866b1ce5bec46a9f6efbc:
>
>   Merge tag 'hppa-fixes-pull-request' of 
> https://github.com/hdeller/qemu-hppa into staging (2022-12-20 15:32:27 
> +)
>
> are available in the Git repository at:
>
>   https://gitlab.com/stsquad/qemu.git tags/pull-testing-next-211222-1
>
> for you to fetch changes up to 7a8ec48480c116db93e0d91688be1dcf34147795:
>
>   gitlab-ci: Disable docs and GUIs for the build-tci and 
> build-tcg-disabled jobs (2022-12-21 11:19:05 +)
>
> 
> testing updates:
>
>   - fix minor shell-ism that can break check-tcg
>   - turn off verbose logging on custom runners
>   - make configure echo call in CI
>   - fix unused variable in linux-test
>   - add binary compiler docker image for hexagon
>   - disable doc and gui builds for tci and disable-tcg builds
>

Gitlab's CI marks this with a "yaml invalid" tag:
https://gitlab.com/qemu-project/qemu/-/pipelines/729324088

and the error:
'build-user-hexagon' job needs 'hexagon-cross-container' job, but 
'hexagon-cross-container' is not in any previous stage

thanks
-- PMM

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

[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!

[PATCH v2 3/4] include: Don't include qemu/osdep.h

[Markus Armbruster]

docs/devel/style.rst mandates:

The "qemu/osdep.h" header contains preprocessor macros that affect
the behavior of core system headers like .  It must be
the first include so that core system headers included by external
libraries get the preprocessor macros that QEMU depends on.

Do not include "qemu/osdep.h" from header files since the .c file
will have already included it.

A few violations have crept in.  Fix them.

Signed-off-by: Markus Armbruster 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Bin Meng 
Reviewed-by: Taylor Simpson 
Reviewed-by: Alistair Francis 
---
 bsd-user/qemu.h | 1 -
 crypto/block-luks-priv.h| 1 -
 include/hw/cxl/cxl_host.h   | 1 -
 include/hw/input/pl050.h| 1 -
 include/hw/tricore/triboard.h   | 1 -
 include/qemu/userfaultfd.h  | 1 -
 net/vmnet_int.h | 1 -
 qga/cutils.h| 1 -
 target/hexagon/hex_arch_types.h | 1 -
 target/hexagon/mmvec/macros.h   | 1 -
 target/riscv/pmu.h  | 1 -
 qga/cutils.c| 3 ++-
 12 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index be6105385e..0ceecfb6df 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -17,7 +17,6 @@
 #ifndef QEMU_H
 #define QEMU_H
 
-#include "qemu/osdep.h"
 #include "cpu.h"
 #include "qemu/units.h"
 #include "exec/cpu_ldst.h"
diff --git a/crypto/block-luks-priv.h b/crypto/block-luks-priv.h
index dc2dd14e52..8fc967afcb 100644
--- a/crypto/block-luks-priv.h
+++ b/crypto/block-luks-priv.h
@@ -18,7 +18,6 @@
  *
  */
 
-#include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/bswap.h"
 
diff --git a/include/hw/cxl/cxl_host.h b/include/hw/cxl/cxl_host.h
index a1b662ce40..c9bc9c7c50 100644
--- a/include/hw/cxl/cxl_host.h
+++ b/include/hw/cxl/cxl_host.h
@@ -7,7 +7,6 @@
  * COPYING file in the top-level directory.
  */
 
-#include "qemu/osdep.h"
 #include "hw/cxl/cxl.h"
 #include "hw/boards.h"
 
diff --git a/include/hw/input/pl050.h b/include/hw/input/pl050.h
index 89ec4fafc9..4cb8985f31 100644
--- a/include/hw/input/pl050.h
+++ b/include/hw/input/pl050.h
@@ -10,7 +10,6 @@
 #ifndef HW_PL050_H
 #define HW_PL050_H
 
-#include "qemu/osdep.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
 #include "hw/input/ps2.h"
diff --git a/include/hw/tricore/triboard.h b/include/hw/tricore/triboard.h
index 094c8bd563..4fdd2d7d97 100644
--- a/include/hw/tricore/triboard.h
+++ b/include/hw/tricore/triboard.h
@@ -18,7 +18,6 @@
  * License along with this library; if not, see .
  */
 
-#include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "hw/boards.h"
 #include "sysemu/sysemu.h"
diff --git a/include/qemu/userfaultfd.h b/include/qemu/userfaultfd.h
index 6b74f92792..55c95998e8 100644
--- a/include/qemu/userfaultfd.h
+++ b/include/qemu/userfaultfd.h
@@ -13,7 +13,6 @@
 #ifndef USERFAULTFD_H
 #define USERFAULTFD_H
 
-#include "qemu/osdep.h"
 #include "exec/hwaddr.h"
 #include 
 
diff --git a/net/vmnet_int.h b/net/vmnet_int.h
index adf6e8c20d..d0b90594f2 100644
--- a/net/vmnet_int.h
+++ b/net/vmnet_int.h
@@ -10,7 +10,6 @@
 #ifndef VMNET_INT_H
 #define VMNET_INT_H
 
-#include "qemu/osdep.h"
 #include "vmnet_int.h"
 #include "clients.h"
 
diff --git a/qga/cutils.h b/qga/cutils.h
index f0f30a7d28..2bfaf554a8 100644
--- a/qga/cutils.h
+++ b/qga/cutils.h
@@ -1,7 +1,6 @@
 #ifndef CUTILS_H_
 #define CUTILS_H_
 
-#include "qemu/osdep.h"
 
 int qga_open_cloexec(const char *name, int flags, mode_t mode);
 
diff --git a/target/hexagon/hex_arch_types.h b/target/hexagon/hex_arch_types.h
index 885f68f760..52a7f2b2f3 100644
--- a/target/hexagon/hex_arch_types.h
+++ b/target/hexagon/hex_arch_types.h
@@ -18,7 +18,6 @@
 #ifndef HEXAGON_HEX_ARCH_TYPES_H
 #define HEXAGON_HEX_ARCH_TYPES_H
 
-#include "qemu/osdep.h"
 #include "mmvec/mmvec.h"
 #include "qemu/int128.h"
 
diff --git a/target/hexagon/mmvec/macros.h b/target/hexagon/mmvec/macros.h
index 8c864e8c68..1201d778d0 100644
--- a/target/hexagon/mmvec/macros.h
+++ b/target/hexagon/mmvec/macros.h
@@ -18,7 +18,6 @@
 #ifndef HEXAGON_MMVEC_MACROS_H
 #define HEXAGON_MMVEC_MACROS_H
 
-#include "qemu/osdep.h"
 #include "qemu/host-utils.h"
 #include "arch.h"
 #include "mmvec/system_ext_mmvec.h"
diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
index 3004ce37b6..0c819ca983 100644
--- a/target/riscv/pmu.h
+++ b/target/riscv/pmu.h
@@ -16,7 +16,6 @@
  * this program.  If not, see .
  */
 
-#include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "cpu.h"
 #include "qemu/main-loop.h"
diff --git a/qga/cutils.c b/qga/cutils.c
index b8e142ef64..b21bcf3683 100644
--- a/qga/cutils.c
+++ b/qga/cutils.c
@@ -2,8 +2,9 @@
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
  */
-#include "cutils.h"
 
+#include "qemu/osdep.h"
+#include "cutils.h"
 #include "qapi/error.h"
 
 /**
-- 
2.38.1

Re: [PATCH v2 0/4] Clean up includes

[Markus Armbruster]

Michael, I forgot to add your R-bys.  I'll fix that in the next
revision if I need one, else in the pull request.

[PATCH v2 4/4] docs/devel: Rules on #include in headers

[Markus Armbruster]

Rules for headers were proposed a long time ago, and generally liked:

Message-ID: <87h9g8j57d@blackfin.pond.sub.org>
https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg03345.html

Wortk them into docs/devel/style.rst.

Suggested-by: Bernhard Beschow 
Signed-off-by: Markus Armbruster 
---
 docs/devel/style.rst | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/docs/devel/style.rst b/docs/devel/style.rst
index 7ddd42b6c2..68aa776930 100644
--- a/docs/devel/style.rst
+++ b/docs/devel/style.rst
@@ -293,6 +293,13 @@ that QEMU depends on.
 Do not include "qemu/osdep.h" from header files since the .c file will have
 already included it.
 
+Headers should normally include everything they need beyond osdep.h.
+If exceptions are needed for some reason, they must be documented in
+the header.  If all that's needed from a header is typedefs, consider
+putting those into qemu/typedefs.h instead of including the header.
+
+Cyclic inclusion is forbidden.
+
 C types
 ===
 
-- 
2.38.1

[PATCH v2 2/4] include: Include headers where needed

[Markus Armbruster]

A number of headers neglect to include everything they need.  They
compile only if the headers they need are already included from
elsewhere.  Fix that.

Signed-off-by: Markus Armbruster 
Reviewed-by: Alistair Francis 
---
 include/exec/plugin-gen.h   | 1 +
 include/hw/acpi/erst.h  | 3 +++
 include/hw/char/cmsdk-apb-uart.h| 1 +
 include/hw/char/goldfish_tty.h  | 1 +
 include/hw/char/xilinx_uartlite.h   | 1 +
 include/hw/cris/etraxfs.h   | 1 +
 include/hw/display/macfb.h  | 3 ++-
 include/hw/dma/sifive_pdma.h| 2 ++
 include/hw/i386/ioapic_internal.h   | 1 +
 include/hw/i386/sgx-epc.h   | 1 +
 include/hw/intc/goldfish_pic.h  | 2 ++
 include/hw/intc/loongarch_pch_msi.h | 2 ++
 include/hw/intc/loongarch_pch_pic.h | 2 ++
 include/hw/intc/nios2_vic.h | 2 ++
 include/hw/misc/mchp_pfsoc_dmc.h| 2 ++
 include/hw/misc/mchp_pfsoc_ioscb.h  | 2 ++
 include/hw/misc/mchp_pfsoc_sysreg.h | 2 ++
 include/hw/misc/pvpanic.h   | 1 +
 include/hw/misc/sifive_e_prci.h | 3 ++-
 include/hw/misc/sifive_u_otp.h  | 3 ++-
 include/hw/misc/sifive_u_prci.h | 3 ++-
 include/hw/misc/virt_ctrl.h | 2 ++
 include/hw/misc/xlnx-versal-pmc-iou-slcr.h  | 1 +
 include/hw/net/lasi_82596.h | 2 +-
 include/hw/net/xlnx-zynqmp-can.h| 1 +
 include/hw/ppc/pnv_psi.h| 2 +-
 include/hw/riscv/boot_opensbi.h | 2 ++
 include/hw/riscv/microchip_pfsoc.h  | 3 +++
 include/hw/riscv/numa.h | 1 +
 include/hw/riscv/sifive_u.h | 2 ++
 include/hw/riscv/spike.h| 2 +-
 include/hw/riscv/virt.h | 2 +-
 include/hw/ssi/sifive_spi.h | 3 +++
 include/hw/timer/sse-timer.h| 1 +
 include/hw/usb/hcd-dwc3.h   | 1 +
 include/hw/usb/hcd-musb.h   | 2 ++
 include/hw/usb/xlnx-usb-subsystem.h | 2 ++
 include/hw/usb/xlnx-versal-usb2-ctrl-regs.h | 3 +++
 include/hw/virtio/virtio-mmio.h | 2 +-
 include/qemu/plugin-memory.h| 3 +++
 include/sysemu/dirtyrate.h  | 2 ++
 include/sysemu/dump.h   | 1 +
 include/user/syscall-trace.h| 1 +
 43 files changed, 71 insertions(+), 9 deletions(-)

diff --git a/include/exec/plugin-gen.h b/include/exec/plugin-gen.h
index 5004728c61..5f5506f1cc 100644
--- a/include/exec/plugin-gen.h
+++ b/include/exec/plugin-gen.h
@@ -12,6 +12,7 @@
 #ifndef QEMU_PLUGIN_GEN_H
 #define QEMU_PLUGIN_GEN_H
 
+#include "exec/cpu_ldst.h"
 #include "qemu/plugin.h"
 #include "tcg/tcg.h"
 
diff --git a/include/hw/acpi/erst.h b/include/hw/acpi/erst.h
index b747fe7739..b2ff663ddc 100644
--- a/include/hw/acpi/erst.h
+++ b/include/hw/acpi/erst.h
@@ -11,6 +11,9 @@
 #ifndef HW_ACPI_ERST_H
 #define HW_ACPI_ERST_H
 
+#include "hw/acpi/bios-linker-loader.h"
+#include "qom/object.h"
+
 void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
 const char *oem_id, const char *oem_table_id);
 
diff --git a/include/hw/char/cmsdk-apb-uart.h b/include/hw/char/cmsdk-apb-uart.h
index 9daff0..64b0a3d534 100644
--- a/include/hw/char/cmsdk-apb-uart.h
+++ b/include/hw/char/cmsdk-apb-uart.h
@@ -15,6 +15,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/sysbus.h"
 #include "chardev/char-fe.h"
+#include "qapi/error.h"
 #include "qom/object.h"
 
 #define TYPE_CMSDK_APB_UART "cmsdk-apb-uart"
diff --git a/include/hw/char/goldfish_tty.h b/include/hw/char/goldfish_tty.h
index 7503d2fa1e..d59733e5ae 100644
--- a/include/hw/char/goldfish_tty.h
+++ b/include/hw/char/goldfish_tty.h
@@ -12,6 +12,7 @@
 
 #include "qemu/fifo8.h"
 #include "chardev/char-fe.h"
+#include "hw/sysbus.h"
 
 #define TYPE_GOLDFISH_TTY "goldfish_tty"
 OBJECT_DECLARE_SIMPLE_TYPE(GoldfishTTYState, GOLDFISH_TTY)
diff --git a/include/hw/char/xilinx_uartlite.h 
b/include/hw/char/xilinx_uartlite.h
index bb32d0fcb3..dd09c06801 100644
--- a/include/hw/char/xilinx_uartlite.h
+++ b/include/hw/char/xilinx_uartlite.h
@@ -17,6 +17,7 @@
 
 #include "hw/qdev-properties.h"
 #include "hw/sysbus.h"
+#include "qapi/error.h"
 
 static inline DeviceState *xilinx_uartlite_create(hwaddr addr,
 qemu_irq irq,
diff --git a/include/hw/cris/etraxfs.h b/include/hw/cris/etraxfs.h
index 8b01ed67d3..467b529dc0 100644
--- a/include/hw/cris/etraxfs.h
+++ b/include/hw/cris/etraxfs.h
@@ -29,6 +29,7 @@
 #include "hw/cris/etraxfs_dma.h"
 #include "hw/qdev-properties.h"
 #include "hw/sysbus.h"
+#include "qapi/error.h"
 
 DeviceState *etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
   struct etraxfs_dma_client *dma_out,
diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
index 55a50d3fb0..27ce

[PATCH v2 0/4] Clean up includes

[Markus Armbruster]

Back in 2016, we discussed[1] rules for headers, and these were
generally liked:

1. Have a carefully curated header that's included everywhere first.  We
   got that already thanks to Peter: osdep.h.

2. Headers should normally include everything they need beyond osdep.h.
   If exceptions are needed for some reason, they must be documented in
   the header.  If all that's needed from a header is typedefs, put
   those into qemu/typedefs.h instead of including the header.

3. Cyclic inclusion is forbidden.

This series fixes a number of rule violations.

It is based on

[PATCH v2 0/4] hw/ppc: Clean up includes
[PATCH v2 0/7] include/hw/pci include/hw/cxl: Clean up includes
[PATCH v2 0/3] block: Clean up includes
[PATCH v3 0/5] coroutine: Clean up includes

With all of these applied, just three inclusion loops remain reachable
from include/:

target/microblaze/cpu.h target/microblaze/mmu.h

target/nios2/cpu.h target/nios2/mmu.h

target/riscv/cpu.h target/riscv/pmp.h

Breaking them would be nice, but I'm out of steam.

v2:
* Rebased
* PATCH 3: v1 posted separately
* PATCH 4: New

[1] Message-ID: <87h9g8j57d@blackfin.pond.sub.org>
https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg03345.html

Based-on: <20221222104628.659681-1-arm...@redhat.com>

Markus Armbruster (4):
  include/hw/virtio: Break inclusion loop
  include: Include headers where needed
  include: Don't include qemu/osdep.h
  docs/devel: Rules on #include in headers

 docs/devel/style.rst| 7 +++
 bsd-user/qemu.h | 1 -
 crypto/block-luks-priv.h| 1 -
 include/exec/plugin-gen.h   | 1 +
 include/hw/acpi/erst.h  | 3 +++
 include/hw/char/cmsdk-apb-uart.h| 1 +
 include/hw/char/goldfish_tty.h  | 1 +
 include/hw/char/xilinx_uartlite.h   | 1 +
 include/hw/cris/etraxfs.h   | 1 +
 include/hw/cxl/cxl_host.h   | 1 -
 include/hw/display/macfb.h  | 3 ++-
 include/hw/dma/sifive_pdma.h| 2 ++
 include/hw/i386/ioapic_internal.h   | 1 +
 include/hw/i386/sgx-epc.h   | 1 +
 include/hw/input/pl050.h| 1 -
 include/hw/intc/goldfish_pic.h  | 2 ++
 include/hw/intc/loongarch_pch_msi.h | 2 ++
 include/hw/intc/loongarch_pch_pic.h | 2 ++
 include/hw/intc/nios2_vic.h | 2 ++
 include/hw/misc/mchp_pfsoc_dmc.h| 2 ++
 include/hw/misc/mchp_pfsoc_ioscb.h  | 2 ++
 include/hw/misc/mchp_pfsoc_sysreg.h | 2 ++
 include/hw/misc/pvpanic.h   | 1 +
 include/hw/misc/sifive_e_prci.h | 3 ++-
 include/hw/misc/sifive_u_otp.h  | 3 ++-
 include/hw/misc/sifive_u_prci.h | 3 ++-
 include/hw/misc/virt_ctrl.h | 2 ++
 include/hw/misc/xlnx-versal-pmc-iou-slcr.h  | 1 +
 include/hw/net/lasi_82596.h | 2 +-
 include/hw/net/xlnx-zynqmp-can.h| 1 +
 include/hw/ppc/pnv_psi.h| 2 +-
 include/hw/riscv/boot_opensbi.h | 2 ++
 include/hw/riscv/microchip_pfsoc.h  | 3 +++
 include/hw/riscv/numa.h | 1 +
 include/hw/riscv/sifive_u.h | 2 ++
 include/hw/riscv/spike.h| 2 +-
 include/hw/riscv/virt.h | 2 +-
 include/hw/ssi/sifive_spi.h | 3 +++
 include/hw/timer/sse-timer.h| 1 +
 include/hw/tricore/triboard.h   | 1 -
 include/hw/usb/hcd-dwc3.h   | 1 +
 include/hw/usb/hcd-musb.h   | 2 ++
 include/hw/usb/xlnx-usb-subsystem.h | 2 ++
 include/hw/usb/xlnx-versal-usb2-ctrl-regs.h | 3 +++
 include/hw/virtio/virtio-mmio.h | 2 +-
 include/hw/virtio/virtio.h  | 1 -
 include/qemu/plugin-memory.h| 3 +++
 include/qemu/userfaultfd.h  | 1 -
 include/sysemu/dirtyrate.h  | 2 ++
 include/sysemu/dump.h   | 1 +
 include/user/syscall-trace.h| 1 +
 net/vmnet_int.h | 1 -
 qga/cutils.h| 1 -
 target/hexagon/hex_arch_types.h | 1 -
 target/hexagon/mmvec/macros.h   | 1 -
 target/riscv/pmu.h  | 1 -
 hw/virtio/virtio-qmp.c  | 1 +
 hw/virtio/virtio.c  | 1 +
 qga/cutils.c| 3 ++-
 59 files changed, 82 insertions(+), 22 deletions(-)

-- 
2.38.1

[PATCH v2 1/4] include/hw/virtio: Break inclusion loop

[Markus Armbruster]

hw/virtio/virtio.h and hw/virtio/vhost.h include each other.  The
former doesn't actually need the latter, so drop that inclusion to
break the loop.

Signed-off-by: Markus Armbruster 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Alistair Francis 
Reviewed-by: Stefano Garzarella 
---
 include/hw/virtio/virtio.h | 1 -
 hw/virtio/virtio-qmp.c | 1 +
 hw/virtio/virtio.c | 1 +
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 24561e933a..48ab2117b5 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -22,7 +22,6 @@
 #include "standard-headers/linux/virtio_config.h"
 #include "standard-headers/linux/virtio_ring.h"
 #include "qom/object.h"
-#include "hw/virtio/vhost.h"
 
 /*
  * A guest should never accept this. It implies negotiation is broken
diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index 8e7282658f..3d4497da99 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -11,6 +11,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/virtio/virtio.h"
+#include "hw/virtio/vhost.h"
 #include "virtio-qmp.h"
 
 #include "standard-headers/linux/virtio_ids.h"
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 289eb71045..0ec6ff9ae4 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -25,6 +25,7 @@
 #include "qom/object_interfaces.h"
 #include "hw/core/cpu.h"
 #include "hw/virtio/virtio.h"
+#include "hw/virtio/vhost.h"
 #include "migration/qemu-file-types.h"
 #include "qemu/atomic.h"
 #include "hw/virtio/virtio-bus.h"
-- 
2.38.1

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

[Philippe Mathieu-Daudé]

On 22/12/22 13:32, Stefan Weil wrote:

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 */
     ...


I'm fine with that, as long as we don't obfuscate the code for clever
compiler's sake. QEMU code base is already complex enough (the devil 😈
is in the details).

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Bin Meng]

On Thu, Dec 22, 2022 at 6:47 PM Daniel Henrique Barboza
 wrote:
>
>
>
> On 12/22/22 07:24, Bin Meng wrote:
> > On Thu, Dec 22, 2022 at 2:29 AM Daniel Henrique Barboza
> >  wrote:
> >> This test is used to do a quick sanity check to ensure that we're able
> >> to run the existing QEMU FW image.
> >>
> >> 'sifive_u', 'spike' and 'virt' riscv64 machines, and 'sifive_u' and
> >> 'virt' 32 bit machines are able to run the default RISCV64_BIOS_BIN |
> >> RISCV32_BIOS_BIN firmware with minimal options.
> >>
> >> Cc: Cleber Rosa 
> >> Cc: Philippe Mathieu-Daudé 
> >> Cc: Wainer dos Santos Moschetta 
> >> Cc: Beraldo Leal 
> >> Signed-off-by: Daniel Henrique Barboza 
> >> ---
> >>   tests/avocado/riscv_opensbi.py | 65 ++
> >>   1 file changed, 65 insertions(+)
> >>   create mode 100644 tests/avocado/riscv_opensbi.py
> >>
> >> diff --git a/tests/avocado/riscv_opensbi.py 
> >> b/tests/avocado/riscv_opensbi.py
> >> new file mode 100644
> >> index 00..abc99ced30
> >> --- /dev/null
> >> +++ b/tests/avocado/riscv_opensbi.py
> >> @@ -0,0 +1,65 @@
> >> +# opensbi boot test for RISC-V machines
> >> +#
> >> +# Copyright (c) 2022, Ventana Micro
> >> +#
> >> +# This work is licensed under the terms of the GNU GPL, version 2 or
> >> +# later.  See the COPYING file in the top-level directory.
> >> +
> >> +from avocado_qemu import QemuSystemTest
> >> +from avocado_qemu import wait_for_console_pattern
> >> +
> >> +class RiscvOpensbi(QemuSystemTest):
> >> +"""
> >> +:avocado: tags=accel:tcg
> >> +"""
> >> +timeout = 5
> >> +
> >> +def test_riscv64_virt(self):
> >> +"""
> >> +:avocado: tags=arch:riscv64
> >> +:avocado: tags=machine:virt
> >> +"""
> >> +self.vm.set_console()
> >> +self.vm.launch()
> >> +wait_for_console_pattern(self, 'Platform Name')
> >> +wait_for_console_pattern(self, 'Boot HART MEDELEG')
> >> +
> >> +def test_riscv64_spike(self):
> >> +"""
> >> +:avocado: tags=arch:riscv64
> >> +:avocado: tags=machine:spike
> >> +"""
> >> +self.vm.set_console()
> >> +self.vm.launch()
> >> +wait_for_console_pattern(self, 'Platform Name')
> >> +wait_for_console_pattern(self, 'Boot HART MEDELEG')
> >> +
> >> +def test_riscv64_sifive_u(self):
> >> +"""
> >> +:avocado: tags=arch:riscv64
> >> +:avocado: tags=machine:sifive_u
> >> +"""
> >> +self.vm.set_console()
> >> +self.vm.launch()
> >> +wait_for_console_pattern(self, 'Platform Name')
> >> +wait_for_console_pattern(self, 'Boot HART MEDELEG')
> >> +
> >> +def test_riscv32_virt(self):
> >> +"""
> >> +:avocado: tags=arch:riscv32
> >> +:avocado: tags=machine:virt
> >> +"""
> >> +self.vm.set_console()
> >> +self.vm.launch()
> >> +wait_for_console_pattern(self, 'Platform Name')
> >> +wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > How about testing riscv32_spike too?
>
>
> I didn't manage to make it work. This riscv64 spark command line boots 
> opensbi:
>
>
> $ ./qemu-system-riscv64 -nographic -display none -vga none -machine spike
>
> OpenSBI v1.1
> _  _
>/ __ \  / |  _ \_   _|
>   | |  | |_ __   ___ _ __ | (___ | |_) || |
>   | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>   | |__| | |_) |  __/ | | |) | |_) || |_
>\/| .__/ \___|_| |_|_/|/_|
>  | |
>  |_|
>
> (...)
>
> The same command line doesn't boot riscv32 spark:
>
> ./qemu-system-riscv32 -nographic -display none -vga none -machine spike
> (--- hangs indefinitely ---)
>
> I debugged it a bit and, as far as boot code goes, it goes all the way and 
> loads the
> opensbi 32bit binary.
>
> After that I tried to found any command line example that boots spike with 
> riscv32
> bit and didn't find any.  So I gave up digging it further because I became 
> unsure
> about whether 32-bit spike works.
>
> If someone can verify that yes, 32-bit spike is supposed to work, then I 
> believe it's
> worth investigating why it's not the case ATM.
>

+Anup who might know if QEMU spike 32-bit machine works with opensbi
32-bit generic image.

Regards,
Bin

Re: [PATCH] usbredir: Do not detach usb if backend chardev disconnect

[Minglei Liu]

ping !
please review this patch : [PATCH] usbredir: Do not detach usb if backend
chardev disconnect - minglei.liu (kernel.org)


minglei.liu  于2022年11月9日周三 19:56写道：

> If the network between qemu and usbredirserver is temporarily disconnected,
> the USB device in the VM will be unplugged. If the reconnect parameter is
> configured for the backend chardev, the device will be reconnected later.
> But from the inside of the VM, this USB device has experienced unplug and
> re-plug, if the USB storage device has been mounted in the VM before,
> the drive letter will change after the device is re-plugged.
>
> So in this case, we no longer unplug the device, and operations to the USB
> is returned immediately at this point.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1305
> Signed-off-by: minglei.liu 
> ---
>  hw/usb/redirect.c | 37 +++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 1bd30efc3e..73731bcab5 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -379,6 +379,11 @@ static void usbredir_cancel_packet(USBDevice *udev,
> USBPacket *p)
>  USBRedirDevice *dev = USB_REDIRECT(udev);
>  int i = USBEP2I(p->ep);
>
> +if(!dev->parser) {
> +ERROR("usbredir parser is not available, need reconnect.\n");
> +return;
> +}
> +
>  if (p->combined) {
>  usb_combined_packet_cancel(udev, p);
>  return;
> @@ -519,6 +524,11 @@ static void usbredir_handle_reset(USBDevice *udev)
>  {
>  USBRedirDevice *dev = USB_REDIRECT(udev);
>
> +if(!dev->parser) {
> +ERROR("usbredir parser is not available, need reconnect.\n");
> +return;
> +}
> +
>  DPRINTF("reset device\n");
>  usbredirparser_send_reset(dev->parser);
>  usbredirparser_do_write(dev->parser);
> @@ -959,6 +969,11 @@ static void usbredir_handle_data(USBDevice *udev,
> USBPacket *p)
>  USBRedirDevice *dev = USB_REDIRECT(udev);
>  uint8_t ep;
>
> +if(!dev->parser) {
> +ERROR("usbredir parser is not available, need reconnect.\n");
> +return;
> +}
> +
>  ep = p->ep->nr;
>  if (p->pid == USB_TOKEN_IN) {
>  ep |= USB_DIR_IN;
> @@ -1027,6 +1042,11 @@ static void usbredir_ep_stopped(USBDevice *udev,
> USBEndpoint *uep)
>  {
>  USBRedirDevice *dev = USB_REDIRECT(udev);
>
> +if(!dev->parser) {
> +ERROR("usbredir parser is not available, need reconnect.\n");
> +return;
> +}
> +
>  usbredir_stop_ep(dev, USBEP2I(uep));
>  usbredirparser_do_write(dev->parser);
>  }
> @@ -1098,6 +1118,11 @@ static void usbredir_handle_control(USBDevice
> *udev, USBPacket *p,
>  USBRedirDevice *dev = USB_REDIRECT(udev);
>  struct usb_redir_control_packet_header control_packet;
>
> +if(!dev->parser) {
> +ERROR("usbredir parser is not available, need reconnect.\n");
> +return;
> +}
> +
>  if (usbredir_already_in_flight(dev, p->id)) {
>  p->status = USB_RET_ASYNC;
>  return;
> @@ -1155,6 +1180,11 @@ static int usbredir_alloc_streams(USBDevice *udev,
> USBEndpoint **eps,
>  struct usb_redir_alloc_bulk_streams_header alloc_streams;
>  int i;
>
> +if(!dev->parser) {
> +ERROR("usbredir parser is not available, need reconnect.\n");
> +return -1;
> +}
> +
>  if (!usbredirparser_peer_has_cap(dev->parser,
>   usb_redir_cap_bulk_streams)) {
>  ERROR("peer does not support streams\n");
> @@ -1193,6 +1223,11 @@ static void usbredir_free_streams(USBDevice *udev,
> USBEndpoint **eps,
>  struct usb_redir_free_bulk_streams_header free_streams;
>  int i;
>
> +if(!dev->parser) {
> +ERROR("usbredir parser is not available, need reconnect.\n");
> +return;
> +}
> +
>  if (!usbredirparser_peer_has_cap(dev->parser,
>   usb_redir_cap_bulk_streams)) {
>  return;
> @@ -1219,8 +1254,6 @@ static void usbredir_chardev_close_bh(void *opaque)
>  USBRedirDevice *dev = opaque;
>
>  qemu_bh_cancel(dev->device_reject_bh);
> -usbredir_device_disconnect(dev);
> -
>  if (dev->parser) {
>  DPRINTF("destroying usbredirparser\n");
>  usbredirparser_destroy(dev->parser);
> --
> 2.27.0
>
>

-- 
刘明磊 Smartx 软件工程师

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

[Eric Auger]

To avoid compilation errors when -Werror=maybe-uninitialized is used,
add a default case with g_assert_not_reached().

Otherwise with GCC 11.3.1 "cc (GCC) 11.3.1 20220421 (Red Hat 11.3.1-2)"
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 

---
v2 -> v3:
- use g_assert_not_reached() and document the compiler in use
---
 target/i386/ops_sse.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
index 3cbc36a59d..0bd6bfad8a 100644
--- 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: /* default case added to help the compiler to avoid warnings */
+g_assert_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: /* default case added to help the compiler to avoid warnings */
+g_assert_not_reached();
 }
 d->Q(0) = r0;
 d->Q(1) = r1;
-- 
2.37.3

Re: [PATCH 03/15] hw/riscv/sifive_u: use 'fdt' from MachineState

[Philippe Mathieu-Daudé]

On 21/12/22 19:22, Daniel Henrique Barboza wrote:

The MachineState object provides a 'fdt' pointer that is already being
used by other RISC-V machines, and it's also used by the 'dumpdtb' QMP
command.

Remove the 'fdt' pointer from SiFiveUState and use MachineState::fdt
instead.

Cc: Palmer Dabbelt 
Signed-off-by: Daniel Henrique Barboza 
---
  hw/riscv/sifive_u.c | 15 ++-
  include/hw/riscv/sifive_u.h |  3 ---
  2 files changed, 6 insertions(+), 12 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 02/15] hw/riscv/spike: use 'fdt' from MachineState

[Philippe Mathieu-Daudé]

On 21/12/22 19:22, Daniel Henrique Barboza wrote:

The MachineState object provides a 'fdt' pointer that is already being
used by other RISC-V machines, and it's also used by the 'dumpdtb' QMP
command.

Remove the 'fdt' pointer from SpikeState and use MachineState::fdt
instead.

Signed-off-by: Daniel Henrique Barboza 
---
  hw/riscv/spike.c | 12 +---
  include/hw/riscv/spike.h |  2 --
  2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 13946acf0d..d96f013e2e 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -52,6 +52,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry 
*memmap,
 uint64_t mem_size, const char *cmdline, bool is_32_bit)
  {
  void *fdt;
+int fdt_size;
  uint64_t addr, size;
  unsigned long clint_addr;
  int cpu, socket;
@@ -64,7 +65,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry 
*memmap,
  "sifive,clint0", "riscv,clint0"
  };
  
-fdt = s->fdt = create_device_tree(&s->fdt_size);

+fdt = mc->fdt = create_device_tree(&fdt_size);


We use 'ms' for MachineState and 'mc' for MachineClass. I first got
scared while looking at that patch that a class field was used. The
variable is simply badly named. Possible future cleanup: s/mc/ms/.

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 04/15] hw/riscv/boot.c: make riscv_find_firmware() static

[Philippe Mathieu-Daudé]

On 21/12/22 19:22, Daniel Henrique Barboza wrote:

The only caller is riscv_find_and_load_firmware(), which is in the same
file.

Signed-off-by: Daniel Henrique Barboza 
---
  hw/riscv/boot.c | 44 -
  include/hw/riscv/boot.h |  1 -
  2 files changed, 22 insertions(+), 23 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 06/15] hw/riscv/spike.c: load initrd right after riscv_load_kernel()

[Philippe Mathieu-Daudé]

On 21/12/22 19:22, Daniel Henrique Barboza wrote:

This will make the code more in line with what the other boards are
doing. We'll also avoid an extra check to machine->kernel_filename since
we already checked that before executing riscv_load_kernel().

Signed-off-by: Daniel Henrique Barboza 
---
  hw/riscv/spike.c | 31 +++
  1 file changed, 15 insertions(+), 16 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 09/15] hw/riscv/boot.c: use MachineState in riscv_load_initrd()

[Philippe Mathieu-Daudé]

On 21/12/22 19:22, Daniel Henrique Barboza wrote:

'filename', 'mem_size' and 'fdt' from riscv_load_initrd() can all be
retrieved by the MachineState object for all callers.

Cc: Palmer Dabbelt 
Signed-off-by: Daniel Henrique Barboza 
---
  hw/riscv/boot.c| 6 --
  hw/riscv/microchip_pfsoc.c | 3 +--
  hw/riscv/sifive_u.c| 3 +--
  hw/riscv/spike.c   | 3 +--
  hw/riscv/virt.c| 3 +--
  include/hw/riscv/boot.h| 3 +--
  6 files changed, 9 insertions(+), 12 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 10/15] hw/riscv/boot.c: use MachineState in riscv_load_kernel()

[Philippe Mathieu-Daudé]

On 21/12/22 19:22, Daniel Henrique Barboza wrote:

All callers are using kernel_filename as machine->kernel_filename.

This will also simplify the changes in riscv_load_kernel() that we're
going to do next.

Cc: Palmer Dabbelt 
Signed-off-by: Daniel Henrique Barboza 
---
  hw/riscv/boot.c| 3 ++-
  hw/riscv/microchip_pfsoc.c | 3 +--
  hw/riscv/opentitan.c   | 3 +--
  hw/riscv/sifive_e.c| 3 +--
  hw/riscv/sifive_u.c| 3 +--
  hw/riscv/spike.c   | 3 +--
  hw/riscv/virt.c| 3 +--
  include/hw/riscv/boot.h| 2 +-
  8 files changed, 9 insertions(+), 14 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 12/15] hw/riscv/boot.c: make riscv_load_initrd() static

[Philippe Mathieu-Daudé]

On 21/12/22 19:22, Daniel Henrique Barboza wrote:

The only remaining caller is riscv_load_kernel() which belongs to the
same file.

Signed-off-by: Daniel Henrique Barboza 
---
  hw/riscv/boot.c | 76 -
  include/hw/riscv/boot.h |  1 -
  2 files changed, 38 insertions(+), 39 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

[PATCH 5/5] hw/net/igb: build support for igb/igbvf devices

[Sriram Yagnaraman]

Signed-off-by: Sriram Yagnaraman 
---
 hw/i386/Kconfig| 1 +
 hw/net/Kconfig | 5 +
 hw/net/igb_core.c  | 4 +---
 hw/net/meson.build | 2 ++
 4 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index d22ac4a4b9..97a0b08842 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -94,6 +94,7 @@ config Q35
 imply VTD
 imply AMD_IOMMU
 imply E1000E_PCI_EXPRESS
+imply IGB_PCI_EXPRESS
 imply VMPORT
 imply VMMOUSE
 select PC_PCI
diff --git a/hw/net/Kconfig b/hw/net/Kconfig
index 1cc1c5775e..18c7851efe 100644
--- a/hw/net/Kconfig
+++ b/hw/net/Kconfig
@@ -44,6 +44,11 @@ config E1000E_PCI_EXPRESS
 default y if PCI_DEVICES
 depends on PCI_EXPRESS && MSI_NONBROKEN
 
+config IGB_PCI_EXPRESS
+bool
+default y if PCI_DEVICES
+depends on PCI_EXPRESS && MSI_NONBROKEN
+
 config RTL8139_PCI
 bool
 default y if PCI_DEVICES
diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index c6b2ed9d3e..f9745f9b26 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -993,14 +993,12 @@ igb_start_recv(IGBCore *core)
 bool
 igb_can_receive(IGBCore *core)
 {
-int i;
-
 if (!e1000x_rx_ready(core->owner, core->mac)) {
 trace_igb_rx_disabled();
 return false;
 }
 
-for (i = 0; i < IGB_NUM_QUEUES; i++) {
+for (int i = 0; i < IGB_NUM_QUEUES; i++) {
 IGBRingInfo *rxi = &core->rx_ring_info[i];
 if (igb_ring_enabled(core, rxi) &&
 igb_has_rxbufs(core, rxi, 1) &&
diff --git a/hw/net/meson.build b/hw/net/meson.build
index ebac261542..a5b56452f5 100644
--- a/hw/net/meson.build
+++ b/hw/net/meson.build
@@ -10,6 +10,8 @@ softmmu_ss.add(when: 'CONFIG_PCNET_COMMON', if_true: 
files('pcnet.c'))
 softmmu_ss.add(when: 'CONFIG_E1000_PCI', if_true: files('e1000.c', 
'e1000x_common.c'))
 softmmu_ss.add(when: 'CONFIG_E1000E_PCI_EXPRESS', if_true: 
files('net_tx_pkt.c', 'net_rx_pkt.c'))
 softmmu_ss.add(when: 'CONFIG_E1000E_PCI_EXPRESS', if_true: files('e1000e.c', 
'e1000e_core.c', 'e1000x_common.c'))
+softmmu_ss.add(when: 'CONFIG_IGB_PCI_EXPRESS', if_true: files('net_tx_pkt.c', 
'net_rx_pkt.c'))
+softmmu_ss.add(when: 'CONFIG_IGB_PCI_EXPRESS', if_true: files('igb.c', 
'igb_core.c', 'igbvf.c', 'e1000x_common.c'))
 softmmu_ss.add(when: 'CONFIG_RTL8139_PCI', if_true: files('rtl8139.c'))
 softmmu_ss.add(when: 'CONFIG_TULIP', if_true: files('tulip.c'))
 softmmu_ss.add(when: 'CONFIG_VMXNET3_PCI', if_true: files('net_tx_pkt.c', 
'net_rx_pkt.c'))
-- 
2.34.1

[PATCH 1/5] pcie: add helper function to get number of VFs

[Sriram Yagnaraman]

Signed-off-by: Sriram Yagnaraman 
---
 hw/pci/pcie_sriov.c | 6 ++
 include/hw/pci/pcie_sriov.h | 5 +
 2 files changed, 11 insertions(+)

diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index 8e3faf1f59..88ba642a20 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -300,3 +300,9 @@ PCIDevice *pcie_sriov_get_vf_at_index(PCIDevice *dev, int n)
 }
 return NULL;
 }
+
+int pcie_sriov_get_num_vfs(PCIDevice *dev)
+{
+assert(!pci_is_vf(dev));
+return dev->exp.sriov_pf.num_vfs;
+}
diff --git a/include/hw/pci/pcie_sriov.h b/include/hw/pci/pcie_sriov.h
index 80f5c84e75..8e9367a03a 100644
--- a/include/hw/pci/pcie_sriov.h
+++ b/include/hw/pci/pcie_sriov.h
@@ -74,4 +74,9 @@ PCIDevice *pcie_sriov_get_pf(PCIDevice *dev);
  */
 PCIDevice *pcie_sriov_get_vf_at_index(PCIDevice *dev, int n);
 
+/*
+ * Get the number of VFs created for this physical function.
+ */
+int pcie_sriov_get_num_vfs(PCIDevice *dev);
+
 #endif /* QEMU_PCIE_SRIOV_H */
-- 
2.34.1

[PATCH 3/5] hw/net/igb: register definitions

[Sriram Yagnaraman]

Signed-off-by: Sriram Yagnaraman 
---
 hw/net/e1000_regs.h| 363 +
 hw/net/e1000x_common.c |  13 ++
 hw/net/e1000x_common.h |  29 
 3 files changed, 376 insertions(+), 29 deletions(-)

diff --git a/hw/net/e1000_regs.h b/hw/net/e1000_regs.h
index 59e050742b..d28cb322b7 100644
--- a/hw/net/e1000_regs.h
+++ b/hw/net/e1000_regs.h
@@ -85,11 +85,13 @@
 #define E1000_DEV_ID_82573E_IAMT 0x108C
 #define E1000_DEV_ID_82573L  0x109A
 #define E1000_DEV_ID_82574L  0x10D3
+#define E1000_DEV_ID_82576   0x10C9
+#define E1000_DEV_ID_82576_VF0x10CA
 #define E1000_DEV_ID_82546GB_QUAD_COPPER_KSP3 0x10B5
-#define E1000_DEV_ID_80003ES2LAN_COPPER_DPT 0x1096
-#define E1000_DEV_ID_80003ES2LAN_SERDES_DPT 0x1098
-#define E1000_DEV_ID_80003ES2LAN_COPPER_SPT 0x10BA
-#define E1000_DEV_ID_80003ES2LAN_SERDES_SPT 0x10BB
+#define E1000_DEV_ID_80003ES2LAN_COPPER_DPT   0x1096
+#define E1000_DEV_ID_80003ES2LAN_SERDES_DPT   0x1098
+#define E1000_DEV_ID_80003ES2LAN_COPPER_SPT   0x10BA
+#define E1000_DEV_ID_80003ES2LAN_SERDES_SPT   0x10BB
 
 #define E1000_DEV_ID_ICH8_IGP_M_AMT  0x1049
 #define E1000_DEV_ID_ICH8_IGP_AMT0x104A
@@ -105,6 +107,7 @@
 #define E1000_PHY_ID2_8254xx_DEFAULT 0xC20 /* 82540x, 82545x, and 82546x */
 #define E1000_PHY_ID2_82573x 0xCC0
 #define E1000_PHY_ID2_82574x 0xCB1
+#define E1000_PHY_ID2_82576x 0x391
 
 /* Register Set. (82543, 82544)
  *
@@ -142,6 +145,7 @@
 #define E1000_IVAR 0x000E4  /* Interrupt Vector Allocation Register - RW */
 #define E1000_EITR 0x000E8  /* Extended Interrupt Throttling Rate - RW */
 #define E1000_RCTL 0x00100  /* RX Control - RW */
+#define E1000_SRRCTL   0x0280C  /* Split and Replication Receive Control - RW 
*/
 #define E1000_RDTR10x02820  /* RX Delay Timer (1) - RW */
 #define E1000_RDBAL1   0x02900  /* RX Descriptor Base Address Low (1) - RW */
 #define E1000_RDBAH1   0x02904  /* RX Descriptor Base Address High (1) - RW */
@@ -150,6 +154,16 @@
 #define E1000_RDT1 0x02918  /* RX Descriptor Tail (1) - RW */
 #define E1000_FCTTV0x00170  /* Flow Control Transmit Timer Value - RW */
 #define E1000_FCRTV0x05F40  /* Flow Control Refresh Timer Value - RW */
+#define E1000_EICR 0x01580  /* Ext. Interrupt Cause Read - R/clr */
+#define E1000_EITR_IGB 0x01680  /* Extended Interrupt Throttling Rate - RW */
+#define E1000_EICS 0x01520  /* Ext. Interrupt Cause Set - W0 */
+#define E1000_EIMS 0x01524  /* Ext. Interrupt Mask Set/Read - RW */
+#define E1000_EIMC 0x01528  /* Ext. Interrupt Mask Clear - WO */
+#define E1000_EIAC_IGB 0x0152C  /* Ext. Interrupt Auto Clear - RW */
+#define E1000_EIAM 0x01530  /* Ext. Interrupt Ack Auto Clear Mask - RW */
+#define E1000_GPIE 0x01514  /* General Purpose Interrupt Enable - RW */
+#define E1000_IVAR_IGB 0x01700  /* Interrupt Vector Allocation (array) - RW */
+#define E1000_IVAR_MISC_IGB 0x01740 /* IVAR for "other" causes - RW */
 #define E1000_TXCW 0x00178  /* TX Configuration Word - RW */
 #define E1000_RXCW 0x00180  /* RX Configuration Word - RO */
 #define E1000_TCTL 0x00400  /* TX Control - RW */
@@ -225,6 +239,7 @@
 #define E1000_TDFHS0x03420  /* TX Data FIFO Head Saved - RW */
 #define E1000_TDFTS0x03428  /* TX Data FIFO Tail Saved - RW */
 #define E1000_TDFPC0x03430  /* TX Data FIFO Packet Count - RW */
+#define E1000_DTXCTL   0x03590  /* DMA TX Control - RW */
 #define E1000_TDBAL0x03800  /* TX Descriptor Base Address Low - RW */
 #define E1000_TDBAL_A  0x00420  /* Alias to TDBAL */
 #define E1000_TDBAH0x03804  /* TX Descriptor Base Address High - RW */
@@ -316,6 +331,7 @@
 #define E1000_ICRXDMTC 0x04120  /* Interrupt Cause Rx Descriptor Minimum 
Threshold Count */
 #define E1000_ICRXOC   0x04124  /* Interrupt Cause Receiver Overrun Count */
 #define E1000_RXCSUM   0x05000  /* RX Checksum Control - RW */
+#define E1000_RLPML0x05004  /* RX Long Packet Max Length */
 #define E1000_RFCTL0x05008  /* Receive Filter Control*/
 #define E1000_MAVTV0   0x05010  /* Management VLAN TAG Value 0 */
 #define E1000_MAVTV1   0x05014  /* Management VLAN TAG Value 1 */
@@ -324,11 +340,18 @@
 #define E1000_MTA  0x05200  /* Multicast Table Array - RW Array */
 #define E1000_RA   0x05400  /* Receive Address - RW Array */
 #define E1000_RA_A 0x00040  /* Alias to RA */
+#define E1000_RA2  0x054E0  /* 2nd half of Rx address array - RW Array */
+#define E1000_PSRTYPE(_i) (0x05480 + ((_i) * 4))
+#define E1000_RAL(_i)  (((_i) <= 15) ? (0x05400 + ((_i) * 8)) : \
+   (0x054E0 + ((_i - 16) * 8)))
+#define E1000_RAH(_i)  (((_i) <= 15) ? (0x05404 + ((_i) * 8)) : \
+   (0x054E4 + ((_i - 16) * 8)))
 #define E1000_VFTA 0x05600  /* VLAN Filter Table Array - RW Array */
 #define E1000_VFTA_A   0x00600  /* Alias to VFTA */
 #define E1000_WUC  0x05800  /* Wakeup Control - RW */
 #def

[PATCH 0/5] hw/net/igb: emulated network device with SR-IOV

[Sriram Yagnaraman]

A new attempt at adding support for Intel 82576 Gigabit Ethernet adapter
with SR-IOV support.

Start qemu with the following parameters.
   qemu-system-x86_64 -enable-kvm -M q35 \
   ...
   -device pcie-root-port,slot=3,id=pcie_port.3 \
   -netdev tap,id=net3,script=no,downscript=/tmp/rmtap,ifname=xcbr3_t2,queues=1 
\
   -device igb,bus=pcie_port.3,netdev=net3,mac=00:00:00:01:03:02

Load IGB/IGBVF modules if needed.
modprobe igb
modprobe igbvf

Create VFs via /sys 
ls /sys/bus/pci/devices/:01:00.0/
echo 2 > /sys/bus/pci/devices/:01:00.0/sriov_numvfs

Sriram Yagnaraman (5):
  pcie: add helper function to get number of VFs
  hw/net/net_tx_pkt: helper function to get l2 hdr
  hw/net/igb: register definitions
  hw/net/igb: emulated intel IGB (82576EB) network device
  hw/net/igb: build support for igb/igbvf devices

 hw/i386/Kconfig |1 +
 hw/net/Kconfig  |5 +
 hw/net/e1000_regs.h |  357 +++-
 hw/net/e1000x_common.c  |   13 +
 hw/net/e1000x_common.h  |   28 +
 hw/net/igb.c|  627 ++
 hw/net/igb.h|  184 ++
 hw/net/igb_core.c   | 3782 +++
 hw/net/igb_core.h   |  216 ++
 hw/net/igbvf.c  |  262 +++
 hw/net/meson.build  |2 +
 hw/net/net_tx_pkt.c |   17 +-
 hw/net/net_tx_pkt.h |8 +
 hw/net/trace-events |  190 ++
 hw/pci/pcie_sriov.c |6 +
 include/hw/pci/pcie_sriov.h |5 +
 16 files changed, 5671 insertions(+), 32 deletions(-)
 create mode 100644 hw/net/igb.c
 create mode 100644 hw/net/igb.h
 create mode 100644 hw/net/igb_core.c
 create mode 100644 hw/net/igb_core.h
 create mode 100644 hw/net/igbvf.c

-- 
2.34.1

[PATCH 2/5] hw/net/net_tx_pkt: helper function to get l2 hdr

[Sriram Yagnaraman]

Also add return value for to send functions

Signed-off-by: Sriram Yagnaraman 
---
 hw/net/net_tx_pkt.c | 17 +++--
 hw/net/net_tx_pkt.h |  8 
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
index 1cb1125d9f..f2e14008b6 100644
--- a/hw/net/net_tx_pkt.c
+++ b/hw/net/net_tx_pkt.c
@@ -284,6 +284,12 @@ struct virtio_net_hdr *net_tx_pkt_get_vhdr(struct NetTxPkt 
*pkt)
 return &pkt->virt_hdr;
 }
 
+struct eth_header *net_tx_pkt_get_l2hdr(struct NetTxPkt *pkt)
+{
+assert(pkt);
+return PKT_GET_ETH_HDR(&pkt->l2_hdr);
+}
+
 static uint8_t net_tx_pkt_get_gso_type(struct NetTxPkt *pkt,
   bool tso_enable)
 {
@@ -551,13 +557,13 @@ static size_t net_tx_pkt_fetch_fragment(struct NetTxPkt 
*pkt,
 return fetched;
 }
 
-static inline void net_tx_pkt_sendv(struct NetTxPkt *pkt,
+static inline ssize_t net_tx_pkt_sendv(struct NetTxPkt *pkt,
 NetClientState *nc, const struct iovec *iov, int iov_cnt)
 {
 if (pkt->is_loopback) {
-qemu_receive_packet_iov(nc, iov, iov_cnt);
+return qemu_receive_packet_iov(nc, iov, iov_cnt);
 } else {
-qemu_sendv_packet(nc, iov, iov_cnt);
+return qemu_sendv_packet(nc, iov, iov_cnt);
 }
 }
 
@@ -632,9 +638,8 @@ bool net_tx_pkt_send(struct NetTxPkt *pkt, NetClientState 
*nc)
 if (pkt->has_virt_hdr ||
 pkt->virt_hdr.gso_type == VIRTIO_NET_HDR_GSO_NONE) {
 net_tx_pkt_fix_ip6_payload_len(pkt);
-net_tx_pkt_sendv(pkt, nc, pkt->vec,
-pkt->payload_frags + NET_TX_PKT_PL_START_FRAG);
-return true;
+return (net_tx_pkt_sendv(pkt, nc, pkt->vec,
+pkt->payload_frags + NET_TX_PKT_PL_START_FRAG) >= 0);
 }
 
 return net_tx_pkt_do_sw_fragmentation(pkt, nc);
diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
index 4ec8bbe9bd..64fae67c58 100644
--- a/hw/net/net_tx_pkt.h
+++ b/hw/net/net_tx_pkt.h
@@ -52,6 +52,14 @@ void net_tx_pkt_uninit(struct NetTxPkt *pkt);
  */
 struct virtio_net_hdr *net_tx_pkt_get_vhdr(struct NetTxPkt *pkt);
 
+/**
+ * get L2 header
+ *
+ * @pkt:packet
+ * @ret:L2 header
+ */
+struct eth_header *net_tx_pkt_get_l2hdr(struct NetTxPkt *pkt);
+
 /**
  * build virtio header (will be stored in module context)
  *
-- 
2.34.1

Re: [PATCH v9 08/14] hw/nvram: NPCM7xx OTP device model

[Philippe Mathieu-Daudé]

Hi,

(old patch)

On 11/9/20 07:20, Havard Skinnemoen wrote:

This supports reading and writing OTP fuses and keys. Only fuse reading
has been tested. Protection is not implemented.

Reviewed-by: Avi Fishman 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Tested-by: Alexander Bulekov 
Signed-off-by: Havard Skinnemoen 
---
  include/hw/arm/npcm7xx.h   |   3 +
  include/hw/nvram/npcm7xx_otp.h |  79 ++
  hw/arm/npcm7xx.c   |  29 +++
  hw/nvram/npcm7xx_otp.c | 440 +
  hw/nvram/meson.build   |   1 +
  5 files changed, 552 insertions(+)
  create mode 100644 include/hw/nvram/npcm7xx_otp.h
  create mode 100644 hw/nvram/npcm7xx_otp.c



+/**
+ * npcm7xx_otp_array_write - ECC encode and write data to OTP array.
+ * @s: OTP module.
+ * @data: Data to be encoded and written.
+ * @offset: Offset of first byte to be written in the OTP array.
+ * @len: Number of bytes before ECC encoding.
+ *
+ * Each nibble of data is encoded into a byte, so the number of bytes written
+ * to the array will be @len * 2.
+ */
+extern void npcm7xx_otp_array_write(NPCM7xxOTPState *s, const void *data,
+unsigned int offset, unsigned int len);



+static void npcm7xx_init_fuses(NPCM7xxState *s)
+{
+NPCM7xxClass *nc = NPCM7XX_GET_CLASS(s);
+uint32_t value;
+
+/*
+ * The initial mask of disabled modules indicates the chip derivative (e.g.
+ * NPCM750 or NPCM730).
+ */
+value = tswap32(nc->disabled_modules);


In which endianness do you want this 32-bit fuse value to be written?

I suppose you used a little-endian host, so we want it big-endian in
the OTP? In that case it would be better to use cpu_to_be32(), to
be able to use the OTP on a big-endian host such s390x.


+npcm7xx_otp_array_write(&s->fuse_array, &value, NPCM7XX_FUSE_DERIVATIVE,
+sizeof(value));
+}


For completeness:

> +static void npcm730_class_init(ObjectClass *oc, void *data)
> +{
> +NPCM7xxClass *nc = NPCM7XX_CLASS(oc);
> +
> +/* NPCM730 is optimized for data center use, so no graphics, etc. */
> +nc->disabled_modules = 0x00300395;
> +nc->num_cpus = 2;
> +}
> +
> +static void npcm750_class_init(ObjectClass *oc, void *data)
> +{
> +NPCM7xxClass *nc = NPCM7XX_CLASS(oc);
> +
> +/* NPCM750 has 2 cores and a full set of peripherals */
> +nc->disabled_modules = 0x;
> +nc->num_cpus = 2;
> +}

Thanks,

Phil.

[PATCH v1 2/2] s390x/pv: Add support for asynchronous teardown for reboot

[Claudio Imbrenda]

This patch adds support for the asynchronous teardown for reboot for
protected VMs.

When attempting to tear down a protected VM, try to use the new
asynchronous interface first. If that fails, fall back to the classic
synchronous one.

The asynchronous interface involves invoking the new
KVM_PV_ASYNC_DISABLE_PREPARE command for the KVM_S390_PV_COMMAND ioctl.

This will prepare the current protected VM for asynchronous teardown.
Once the protected VM is prepared for teardown, execution can continue
immediately.

Once the protected VM has been prepared, a new thread is started to
actually perform the teardown. The new thread uses the new
KVM_PV_ASYNC_DISABLE command for the KVM_S390_PV_COMMAND ioctl. The
previously prepared protected VM is torn down in the new thread.

Once KVM_PV_ASYNC_DISABLE is invoked, it is possible to use
KVM_PV_ASYNC_DISABLE_PREPARE again. If a protected VM has already been
prepared and its cleanup has not started, it will not be possible to
prepare a new VM. In that case the classic synchronous teardown has to
be performed.

The synchronous teardown will now also clean up any prepared VMs whose
asynchronous teardown has not been initiated yet.

This considerably speeds up the reboot of a protected VM; for large VMs
especially, it could take a long time to perform a reboot with the
traditional synchronous teardown, while with this patch it is almost
immediate.

Signed-off-by: Claudio Imbrenda 
---
 hw/s390x/pv.c  | 28 
 hw/s390x/s390-virtio-ccw.c |  5 -
 include/hw/s390x/pv.h  |  2 ++
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
index 8dfe92d8df..0b8345092b 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -16,6 +16,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "sysemu/kvm.h"
+#include "sysemu/cpus.h"
 #include "qom/object_interfaces.h"
 #include "exec/confidential-guest-support.h"
 #include "hw/s390x/ipl.h"
@@ -107,6 +108,33 @@ void s390_pv_vm_disable(void)
  s390_pv_cmd_exit(KVM_PV_DISABLE, NULL);
 }
 
+static void *s390_pv_do_unprot_async_fn(void *p)
+{
+ s390_pv_cmd_exit(KVM_PV_ASYNC_CLEANUP_PERFORM, NULL);
+ return NULL;
+}
+
+bool s390_pv_vm_try_disable_async(void)
+{
+char tname[VCPU_THREAD_NAME_SIZE];
+QemuThread *t;
+
+if (!kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) 
{
+return false;
+}
+if (s390_pv_cmd(KVM_PV_ASYNC_CLEANUP_PREPARE, NULL) != 0) {
+return false;
+}
+
+t = g_malloc0(sizeof(QemuThread));
+snprintf(tname, VCPU_THREAD_NAME_SIZE, "async_unpr/KVM");
+
+qemu_thread_create(t, tname, s390_pv_do_unprot_async_fn, NULL,
+   QEMU_THREAD_DETACHED);
+
+return true;
+}
+
 int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
 {
 struct kvm_s390_pv_sec_parm args = {
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index f22f61b8b6..503f212a31 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -41,6 +41,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/s390x/tod.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/cpus.h"
 #include "hw/s390x/pv.h"
 #include "migration/blocker.h"
 #include "qapi/visitor.h"
@@ -329,7 +330,9 @@ static inline void s390_do_cpu_ipl(CPUState *cs, 
run_on_cpu_data arg)
 
 static void s390_machine_unprotect(S390CcwMachineState *ms)
 {
-s390_pv_vm_disable();
+if (!s390_pv_vm_try_disable_async()) {
+s390_pv_vm_disable();
+}
 ms->pv = false;
 migrate_del_blocker(pv_mig_blocker);
 error_free_or_abort(&pv_mig_blocker);
diff --git a/include/hw/s390x/pv.h b/include/hw/s390x/pv.h
index 9360aa1091..966306a9db 100644
--- a/include/hw/s390x/pv.h
+++ b/include/hw/s390x/pv.h
@@ -41,6 +41,7 @@ static inline bool s390_is_pv(void)
 int s390_pv_query_info(void);
 int s390_pv_vm_enable(void);
 void s390_pv_vm_disable(void);
+bool s390_pv_vm_try_disable_async(void);
 int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
 int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
 void s390_pv_prep_reset(void);
@@ -60,6 +61,7 @@ static inline bool s390_is_pv(void) { return false; }
 static inline int s390_pv_query_info(void) { return 0; }
 static inline int s390_pv_vm_enable(void) { return 0; }
 static inline void s390_pv_vm_disable(void) {}
+static inline bool s390_pv_vm_try_disable_async(void) { return false; }
 static inline int s390_pv_set_sec_parms(uint64_t origin, uint64_t length) { 
return 0; }
 static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) 
{ return 0; }
 static inline void s390_pv_prep_reset(void) {}
-- 
2.38.1

[PATCH v1 0/2] s390x/pv: Add support for asynchronous teardown for reboot

[Claudio Imbrenda]

The first patch is just a minimal header update to compile the second
patch; it can be safely discarded once the Linux headers are updated to
6.2.

The second patch adds support for asynchronous teardown of protected
guests when rebooting. First the existing guest is prepared for
asynchronous teardown, the rebooted guest will be able to continue
immediately, while a background thread actually performs the necessary
cleanup.

Claudio Imbrenda (2):
  Linux header update
  s390x/pv: Add support for asynchronous teardown for reboot

 hw/s390x/pv.c  | 28 
 hw/s390x/s390-virtio-ccw.c |  5 -
 include/hw/s390x/pv.h  |  2 ++
 linux-headers/linux/kvm.h  |  3 +++
 4 files changed, 37 insertions(+), 1 deletion(-)

-- 
2.38.1

[PATCH v1 1/2] Linux header update

[Claudio Imbrenda]

Signed-off-by: Claudio Imbrenda 
---
 linux-headers/linux/kvm.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index ebdafa576d..122b273433 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1175,6 +1175,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_VM_DISABLE_NX_HUGE_PAGES 220
 #define KVM_CAP_S390_ZPCI_OP 221
 #define KVM_CAP_S390_CPU_TOPOLOGY 222
+#define KVM_CAP_S390_PROTECTED_ASYNC_DISABLE 224
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1737,6 +1738,8 @@ enum pv_cmd_id {
KVM_PV_UNSHARE_ALL,
KVM_PV_INFO,
KVM_PV_DUMP,
+   KVM_PV_ASYNC_CLEANUP_PREPARE,
+   KVM_PV_ASYNC_CLEANUP_PERFORM,
 };
 
 struct kvm_pv_cmd {
-- 
2.38.1

Re: [PATCH 02/15] hw/riscv/spike: use 'fdt' from MachineState

[Daniel Henrique Barboza]

On 12/22/22 11:25, Philippe Mathieu-Daudé wrote:

On 21/12/22 19:22, Daniel Henrique Barboza wrote:

The MachineState object provides a 'fdt' pointer that is already being
used by other RISC-V machines, and it's also used by the 'dumpdtb' QMP
command.

Remove the 'fdt' pointer from SpikeState and use MachineState::fdt
instead.

Signed-off-by: Daniel Henrique Barboza 
---
  hw/riscv/spike.c | 12 +---
  include/hw/riscv/spike.h |  2 --
  2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 13946acf0d..d96f013e2e 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -52,6 +52,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry 
*memmap,
 uint64_t mem_size, const char *cmdline, bool is_32_bit)
  {
  void *fdt;
+    int fdt_size;
  uint64_t addr, size;
  unsigned long clint_addr;
  int cpu, socket;
@@ -64,7 +65,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry 
*memmap,
  "sifive,clint0", "riscv,clint0"
  };
  -    fdt = s->fdt = create_device_tree(&s->fdt_size);
+    fdt = mc->fdt = create_device_tree(&fdt_size);


We use 'ms' for MachineState and 'mc' for MachineClass. I first got
scared while looking at that patch that a class field was used. The
variable is simply badly named. Possible future cleanup: s/mc/ms/.


Thanks for the ack Phil!


And yeah, I think I'll drop a patch with your suggestion later on.


Daniel


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 01/15] tests/avocado: add RISC-V opensbi boot test

[Anup Patel]

On Thu, Dec 22, 2022 at 6:27 PM Bin Meng  wrote:
>
> On Thu, Dec 22, 2022 at 6:47 PM Daniel Henrique Barboza
>  wrote:
> >
> >
> >
> > On 12/22/22 07:24, Bin Meng wrote:
> > > On Thu, Dec 22, 2022 at 2:29 AM Daniel Henrique Barboza
> > >  wrote:
> > >> This test is used to do a quick sanity check to ensure that we're able
> > >> to run the existing QEMU FW image.
> > >>
> > >> 'sifive_u', 'spike' and 'virt' riscv64 machines, and 'sifive_u' and
> > >> 'virt' 32 bit machines are able to run the default RISCV64_BIOS_BIN |
> > >> RISCV32_BIOS_BIN firmware with minimal options.
> > >>
> > >> Cc: Cleber Rosa 
> > >> Cc: Philippe Mathieu-Daudé 
> > >> Cc: Wainer dos Santos Moschetta 
> > >> Cc: Beraldo Leal 
> > >> Signed-off-by: Daniel Henrique Barboza 
> > >> ---
> > >>   tests/avocado/riscv_opensbi.py | 65 ++
> > >>   1 file changed, 65 insertions(+)
> > >>   create mode 100644 tests/avocado/riscv_opensbi.py
> > >>
> > >> diff --git a/tests/avocado/riscv_opensbi.py 
> > >> b/tests/avocado/riscv_opensbi.py
> > >> new file mode 100644
> > >> index 00..abc99ced30
> > >> --- /dev/null
> > >> +++ b/tests/avocado/riscv_opensbi.py
> > >> @@ -0,0 +1,65 @@
> > >> +# opensbi boot test for RISC-V machines
> > >> +#
> > >> +# Copyright (c) 2022, Ventana Micro
> > >> +#
> > >> +# This work is licensed under the terms of the GNU GPL, version 2 or
> > >> +# later.  See the COPYING file in the top-level directory.
> > >> +
> > >> +from avocado_qemu import QemuSystemTest
> > >> +from avocado_qemu import wait_for_console_pattern
> > >> +
> > >> +class RiscvOpensbi(QemuSystemTest):
> > >> +"""
> > >> +:avocado: tags=accel:tcg
> > >> +"""
> > >> +timeout = 5
> > >> +
> > >> +def test_riscv64_virt(self):
> > >> +"""
> > >> +:avocado: tags=arch:riscv64
> > >> +:avocado: tags=machine:virt
> > >> +"""
> > >> +self.vm.set_console()
> > >> +self.vm.launch()
> > >> +wait_for_console_pattern(self, 'Platform Name')
> > >> +wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > >> +
> > >> +def test_riscv64_spike(self):
> > >> +"""
> > >> +:avocado: tags=arch:riscv64
> > >> +:avocado: tags=machine:spike
> > >> +"""
> > >> +self.vm.set_console()
> > >> +self.vm.launch()
> > >> +wait_for_console_pattern(self, 'Platform Name')
> > >> +wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > >> +
> > >> +def test_riscv64_sifive_u(self):
> > >> +"""
> > >> +:avocado: tags=arch:riscv64
> > >> +:avocado: tags=machine:sifive_u
> > >> +"""
> > >> +self.vm.set_console()
> > >> +self.vm.launch()
> > >> +wait_for_console_pattern(self, 'Platform Name')
> > >> +wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > >> +
> > >> +def test_riscv32_virt(self):
> > >> +"""
> > >> +:avocado: tags=arch:riscv32
> > >> +:avocado: tags=machine:virt
> > >> +"""
> > >> +self.vm.set_console()
> > >> +self.vm.launch()
> > >> +wait_for_console_pattern(self, 'Platform Name')
> > >> +wait_for_console_pattern(self, 'Boot HART MEDELEG')
> > > How about testing riscv32_spike too?
> >
> >
> > I didn't manage to make it work. This riscv64 spark command line boots 
> > opensbi:
> >
> >
> > $ ./qemu-system-riscv64 -nographic -display none -vga none -machine spike
> >
> > OpenSBI v1.1
> > _  _
> >/ __ \  / |  _ \_   _|
> >   | |  | |_ __   ___ _ __ | (___ | |_) || |
> >   | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
> >   | |__| | |_) |  __/ | | |) | |_) || |_
> >\/| .__/ \___|_| |_|_/|/_|
> >  | |
> >  |_|
> >
> > (...)
> >
> > The same command line doesn't boot riscv32 spark:
> >
> > ./qemu-system-riscv32 -nographic -display none -vga none -machine spike
> > (--- hangs indefinitely ---)
> >
> > I debugged it a bit and, as far as boot code goes, it goes all the way and 
> > loads the
> > opensbi 32bit binary.
> >
> > After that I tried to found any command line example that boots spike with 
> > riscv32
> > bit and didn't find any.  So I gave up digging it further because I became 
> > unsure
> > about whether 32-bit spike works.
> >
> > If someone can verify that yes, 32-bit spike is supposed to work, then I 
> > believe it's
> > worth investigating why it's not the case ATM.
> >
>
> +Anup who might know if QEMU spike 32-bit machine works with opensbi
> 32-bit generic image.

We never got HTIF putc() working on QEMU RV32 Spike but it works
perfectly fine on QEMU RV64 Spike.

See below log of QEMU RV64 Spike ...

Regards,
Anup

anup@anup-ubuntu-vm:~/Work/riscv-test/opensbi$ qemu-system-riscv64 -M
spike -m 256M -nographic -bios
/home/anup/Work/riscv-test/opensbi/build/platform/generic/firmware/fw_payload.elf

OpenSBI v1.1-124-gb848d87
     

[PATCH] nubus-device: fix memory leak in nubus_device_realize

[Mauro Matteo Cascella]

Local variable "name" is allocated through strdup_printf and should be
freed with g_free() to avoid memory leak.

Fixes: 3616f424 ("nubus-device: add romfile property for loading declaration 
ROMs")
Signed-off-by: Mauro Matteo Cascella 
---
 hw/nubus/nubus-device.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 0f1852f671..49008e4938 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -80,6 +80,7 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
&error_abort);
 ret = load_image_mr(path, &nd->decl_rom);
 g_free(path);
+g_free(name);
 if (ret < 0) {
 error_setg(errp, "could not load romfile \"%s\"", nd->romfile);
 return;
-- 
2.38.1

Re: [PATCH] nubus-device: fix memory leak in nubus_device_realize

[Laurent Vivier]

Le 22/12/2022 à 18:29, Mauro Matteo Cascella a écrit :

Local variable "name" is allocated through strdup_printf and should be
freed with g_free() to avoid memory leak.

Fixes: 3616f424 ("nubus-device: add romfile property for loading declaration 
ROMs")
Signed-off-by: Mauro Matteo Cascella 
---
  hw/nubus/nubus-device.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 0f1852f671..49008e4938 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -80,6 +80,7 @@ static void nubus_device_realize(DeviceState *dev, Error 
**errp)
 &error_abort);
  ret = load_image_mr(path, &nd->decl_rom);
  g_free(path);
+g_free(name);
  if (ret < 0) {
  error_setg(errp, "could not load romfile \"%s\"", nd->romfile);
  return;


Reviewed-by: Laurent Vivier 

Re: [PATCH v10 1/9] mm: Introduce memfd_restricted system call to create restricted user memory

[Sean Christopherson]

On Wed, Dec 21, 2022, Chao Peng wrote:
> On Tue, Dec 20, 2022 at 08:33:05AM +, Huang, Kai wrote:
> > On Tue, 2022-12-20 at 15:22 +0800, Chao Peng wrote:
> > > On Mon, Dec 19, 2022 at 08:48:10AM +, Huang, Kai wrote:
> > > > On Mon, 2022-12-19 at 15:53 +0800, Chao Peng wrote:
> > But for non-restricted-mem case, it is correct for KVM to decrease page's
> > refcount after setting up mapping in the secondary mmu, otherwise the page 
> > will
> > be pinned by KVM for normal VM (since KVM uses GUP to get the page).
> 
> That's true. Actually even true for restrictedmem case, most likely we
> will still need the kvm_release_pfn_clean() for KVM generic code. On one
> side, other restrictedmem users like pKVM may not require page pinning
> at all. On the other side, see below.
> 
> > 
> > So what we are expecting is: for KVM if the page comes from restricted mem, 
> > then
> > KVM cannot decrease the refcount, otherwise for normal page via GUP KVM 
> > should.

No, requiring the user (KVM) to guard against lack of support for page migration
in restricted mem is a terrible API.  It's totally fine for restricted mem to 
not
support page migration until there's a use case, but punting the problem to KVM
is not acceptable.  Restricted mem itself doesn't yet support page migration,
e.g. explosions would occur even if KVM wanted to allow migration since there is
no notification to invalidate existing mappings.

> I argue that this page pinning (or page migration prevention) is not
> tied to where the page comes from, instead related to how the page will
> be used. Whether the page is restrictedmem backed or GUP() backed, once
> it's used by current version of TDX then the page pinning is needed. So
> such page migration prevention is really TDX thing, even not KVM generic
> thing (that's why I think we don't need change the existing logic of
> kvm_release_pfn_clean()). Wouldn't better to let TDX code (or who
> requires that) to increase/decrease the refcount when it populates/drops
> the secure EPT entries? This is exactly what the current TDX code does:

I agree that whether or not migration is supported should be controllable by the
user, but I strongly disagree on punting refcount management to KVM (or TDX).
The whole point of restricted mem is to support technologies like TDX and SNP,
accomodating their special needs for things like page migration should be part 
of
the API, not some footnote in the documenation.

It's not difficult to let the user communicate support for page migration, e.g.
if/when restricted mem gains support, add a hook to restrictedmem_notifier_ops
to signal support (or lack thereof) for page migration.  NULL == no migration,
non-NULL == migration allowed.

We know that supporting page migration in TDX and SNP is possible, and we know
that page migration will require a dedicated API since the backing store can't
memcpy() the page.  I don't see any reason to ignore that eventuality.

But again, unless I'm missing something, that's a future problem because 
restricted
mem doesn't yet support page migration regardless of the downstream user.

[PATCH] linux-user: Add strace for prlimit64() syscall

[Helge Deller]

Add proper prlimit64() strace output.

Signed-off-by: Helge Deller 
---
 linux-user/strace.c| 89 ++
 linux-user/strace.list |  3 +-
 2 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/linux-user/strace.c b/linux-user/strace.c
index 3ea91084fb..82dc1a1e20 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -3774,6 +3774,95 @@ print_futex(CPUArchState *cpu_env, const struct 
syscallname *name,
 }
 #endif

+#ifdef TARGET_NR_prlimit64
+static const char *target_ressource_string(abi_ulong r)
+{
+#define RET_RES_ENTRY(res) case TARGET_##res:  return #res;
+switch (r) {
+RET_RES_ENTRY(RLIMIT_AS);
+RET_RES_ENTRY(RLIMIT_CORE);
+RET_RES_ENTRY(RLIMIT_CPU);
+RET_RES_ENTRY(RLIMIT_DATA);
+RET_RES_ENTRY(RLIMIT_FSIZE);
+RET_RES_ENTRY(RLIMIT_LOCKS);
+RET_RES_ENTRY(RLIMIT_MEMLOCK);
+RET_RES_ENTRY(RLIMIT_MSGQUEUE);
+RET_RES_ENTRY(RLIMIT_NICE);
+RET_RES_ENTRY(RLIMIT_NOFILE);
+RET_RES_ENTRY(RLIMIT_NPROC);
+RET_RES_ENTRY(RLIMIT_RSS);
+RET_RES_ENTRY(RLIMIT_RTPRIO);
+#ifdef RLIMIT_RTTIME
+RET_RES_ENTRY(RLIMIT_RTTIME);
+#endif
+RET_RES_ENTRY(RLIMIT_SIGPENDING);
+RET_RES_ENTRY(RLIMIT_STACK);
+default:
+return NULL;
+}
+#undef RET_RES_ENTRY
+}
+
+static void
+print_rlimit64(abi_ulong rlim_addr, int last)
+{
+if (rlim_addr) {
+struct target_rlimit64 *rl;
+
+rl = lock_user(VERIFY_READ, rlim_addr, sizeof(*rl), 1);
+if (!rl) {
+print_pointer(rlim_addr, last);
+return;
+}
+qemu_log("{rlim_cur = %lld, rlim_max = %lld}%s",
+ (long long)tswap64(rl->rlim_cur),
+ (long long)tswap64(rl->rlim_max),
+ get_comma(last));
+unlock_user(rl, rlim_addr, 0);
+} else {
+qemu_log("NULL%s", get_comma(last));
+}
+}
+
+static void
+print_prlimit64(CPUArchState *cpu_env, const struct syscallname *name,
+   abi_long arg0, abi_long arg1, abi_long arg2,
+   abi_long arg3, abi_long arg4, abi_long arg5)
+{
+const char *rlim_name;
+
+print_syscall_prologue(name);
+print_raw_param("%d", arg0, 0);
+rlim_name = target_ressource_string(arg1);
+if (rlim_name) {
+qemu_log("%s,", rlim_name);
+} else {
+print_raw_param("%d", arg1, 0);
+}
+print_rlimit64(arg2, 0);
+print_pointer(arg3, 1);
+print_syscall_epilogue(name);
+}
+
+static void
+print_syscall_ret_prlimit64(CPUArchState *cpu_env,
+   const struct syscallname *name,
+   abi_long ret, abi_long arg0, abi_long arg1,
+   abi_long arg2, abi_long arg3, abi_long arg4,
+   abi_long arg5)
+{
+if (!print_syscall_err(ret)) {
+qemu_log(TARGET_ABI_FMT_ld, ret);
+if (arg3) {
+qemu_log(" (");
+print_rlimit64(arg3, 1);
+qemu_log(")");
+}
+}
+qemu_log("\n");
+}
+#endif
+
 #ifdef TARGET_NR_kill
 static void
 print_kill(CPUArchState *cpu_env, const struct syscallname *name,
diff --git a/linux-user/strace.list b/linux-user/strace.list
index ac8f872371..f9254725a1 100644
--- a/linux-user/strace.list
+++ b/linux-user/strace.list
@@ -1067,7 +1067,8 @@
 { TARGET_NR_preadv, "preadv" , NULL, NULL, NULL },
 #endif
 #ifdef TARGET_NR_prlimit64
-{ TARGET_NR_prlimit64, "prlimit64" , NULL, NULL, NULL },
+{ TARGET_NR_prlimit64, "prlimit64" , NULL, print_prlimit64,
+print_syscall_ret_prlimit64 },
 #endif
 #ifdef TARGET_NR_process_vm_readv
 { TARGET_NR_process_vm_readv, "process_vm_readv" , NULL, NULL, NULL },
--
2.38.1

Re: [PATCH v2 0/7] include/hw/pci include/hw/cxl: Clean up includes

[Michael S. Tsirkin]

On Thu, Dec 22, 2022 at 11:48:25AM +0100, Markus Armbruster wrote:
> Bernhard Beschow  writes:
> 
> > Am 22. Dezember 2022 10:03:23 UTC schrieb Markus Armbruster 
> > :
> >>Back in 2016, we discussed[1] rules for headers, and these were
> >>generally liked:
> >>
> >>1. Have a carefully curated header that's included everywhere first.  We
> >>   got that already thanks to Peter: osdep.h.
> >>
> >>2. Headers should normally include everything they need beyond osdep.h.
> >>   If exceptions are needed for some reason, they must be documented in
> >>   the header.  If all that's needed from a header is typedefs, put
> >>   those into qemu/typedefs.h instead of including the header.
> >>
> >>3. Cyclic inclusion is forbidden.
> >
> > Sounds like these -- useful and sane -- rules belong in QEMU's coding 
> > style. What about putting them there for easy reference?
> 
> Makes sense.  I'll see what I can do.  Thanks!

It would be even better if there was e.g. a make target
pulling in each header and making sure it's self consistent and
no circularity. We could run it e.g. in CI.

-- 
MST

Re: [PULL 0/6] testing updates

[Alex Bennée]

Peter Maydell  writes:

> On Wed, 21 Dec 2022 at 14:40, Alex Bennée  wrote:
>>
>> The following changes since commit 8540a1f69578afb3b37866b1ce5bec46a9f6efbc:
>>
>>   Merge tag 'hppa-fixes-pull-request' of
>> https://github.com/hdeller/qemu-hppa into staging (2022-12-20
>> 15:32:27 +)
>>
>> are available in the Git repository at:
>>
>>   https://gitlab.com/stsquad/qemu.git tags/pull-testing-next-211222-1
>>
>> for you to fetch changes up to 7a8ec48480c116db93e0d91688be1dcf34147795:
>>
>>   gitlab-ci: Disable docs and GUIs for the build-tci and
>> build-tcg-disabled jobs (2022-12-21 11:19:05 +)
>>
>> 
>> testing updates:
>>
>>   - fix minor shell-ism that can break check-tcg
>>   - turn off verbose logging on custom runners
>>   - make configure echo call in CI
>>   - fix unused variable in linux-test
>>   - add binary compiler docker image for hexagon
>>   - disable doc and gui builds for tci and disable-tcg builds
>>
>
> Gitlab's CI marks this with a "yaml invalid" tag:
> https://gitlab.com/qemu-project/qemu/-/pipelines/729324088
>
> and the error:
> 'build-user-hexagon' job needs 'hexagon-cross-container' job, but
> 'hexagon-cross-container' is not in any previous stage

Ahh and doesn't show up on my run:

  https://gitlab.com/stsquad/qemu/-/pipelines/728840940

I guess because it is qemu-project only.

>
> thanks
> -- PMM


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PULL 0/6] testing updates

[Alex Bennée]

"Mukilan Thiyagarajan (QUIC)"  writes:

> I believe the error is caused by the QEMU_JOB_ONLY_FORKS: 1 line
> which needs to be removed from the definition of hexagon-cross-container.
>
> Alex, Peter, 
>
> Shall I raise a patch to remove this line? Should the patch be
> created against the testing/next branch?

Just show the patch here and I'll manually re-spin.

>
> Thanks,
> Mukilan
>
> -Original Message-
> From: qemu-devel-bounces+quic_mthiyaga=quicinc@nongnu.org
>  On Behalf Of
> Peter Maydell
> Sent: Thursday, December 22, 2022 4:45 PM
> To: Alex Bennée 
> Cc: qemu-devel@nongnu.org
> Subject: Re: [PULL 0/6] testing updates
>
> WARNING: This email originated from outside of Qualcomm. Please be
> wary of any links or attachments, and do not enable macros.
>
> On Wed, 21 Dec 2022 at 14:40, Alex Bennée  wrote:
>>
>> The following changes since commit 8540a1f69578afb3b37866b1ce5bec46a9f6efbc:
>>
>>   Merge tag 'hppa-fixes-pull-request' of 
>> https://github.com/hdeller/qemu-hppa into staging (2022-12-20 15:32:27 
>> +)
>>
>> are available in the Git repository at:
>>
>>   https://gitlab.com/stsquad/qemu.git tags/pull-testing-next-211222-1
>>
>> for you to fetch changes up to 7a8ec48480c116db93e0d91688be1dcf34147795:
>>
>>   gitlab-ci: Disable docs and GUIs for the build-tci and 
>> build-tcg-disabled jobs (2022-12-21 11:19:05 +)
>>
>> 
>> testing updates:
>>
>>   - fix minor shell-ism that can break check-tcg
>>   - turn off verbose logging on custom runners
>>   - make configure echo call in CI
>>   - fix unused variable in linux-test
>>   - add binary compiler docker image for hexagon
>>   - disable doc and gui builds for tci and disable-tcg builds
>>
>
> Gitlab's CI marks this with a "yaml invalid" tag:
> https://gitlab.com/qemu-project/qemu/-/pipelines/729324088
>
> and the error:
> 'build-user-hexagon' job needs 'hexagon-cross-container' job, but 
> 'hexagon-cross-container' is not in any previous stage
>
> thanks
> -- PMM


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v4 06/12] libvhost-user: Change dev->postcopy_ufd assignment to make it C90 compliant

[Marcel Holtmann]

The assignment of dev->postcopy_ufd can be moved into an else clause and
then the code becomes C90 compliant.

  CC   libvhost-user.o
libvhost-user.c: In function ‘vu_set_postcopy_advise’:
libvhost-user.c:1625:5: error: ISO C90 forbids mixed declarations and code 
[-Werror=declaration-after-statement]
 1625 | struct uffdio_api api_struct;
  | ^~

Understandable, it might be desired to avoid else clauses, but in this
case it seems clear enough and frankly the dev->postcopy_ufd is only
assigned once.

Signed-off-by: Marcel Holtmann 
Reviewed-by: Philippe Mathieu-Daudé 
---
 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 bf92cc85c086..b28b66cdb159 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -1599,12 +1599,13 @@ vu_set_config(VuDev *dev, VhostUserMsg *vmsg)
 static bool
 vu_set_postcopy_advise(VuDev *dev, VhostUserMsg *vmsg)
 {
-dev->postcopy_ufd = -1;
 #ifdef UFFDIO_API
 struct uffdio_api api_struct;
 
 dev->postcopy_ufd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
 vmsg->size = 0;
+#else
+dev->postcopy_ufd = -1;
 #endif
 
 if (dev->postcopy_ufd == -1) {
-- 
2.38.1

[PATCH v4 01/12] libvhost-user: Provide _GNU_SOURCE when compiling outside of QEMU

[Marcel Holtmann]

Then the libvhost-user sources are used by another project, it can not
be guaranteed that _GNU_SOURCE is set by the build system. If it is for
example not set, errors like this show up.

  CC   libvhost-user.o
libvhost-user.c: In function ‘vu_panic’:
libvhost-user.c:195:9: error: implicit declaration of function ‘vasprintf’; did 
you mean ‘vsprintf’? [-Werror=implicit-function-declaration]
  195 | if (vasprintf(&buf, msg, ap) < 0) {
  | ^
  | vsprintf

The simplest way to allow external complication of libvhost-user.[ch] is
by setting _GNU_SOURCE if it is not already set by the build system.

Signed-off-by: Marcel Holtmann 
---
 subprojects/libvhost-user/libvhost-user.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c
index d6ee6e7d9168..b55b9e244d9a 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -13,6 +13,10 @@
  * later.  See the COPYING file in the top-level directory.
  */
 
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+
 /* this code avoids GLib dependency */
 #include 
 #include 
-- 
2.38.1

Re: [PATCH v3 02/10] libvhost-user: Replace typeof with __typeof__

[Marcel Holtmann]

Hi Paolo,

>> Strictly speaking only -std=gnu99 support the usage of typeof and for
>> easier inclusion in external projects, it is better to use __typeof__.
>>   CC   libvhost-user.o
>> libvhost-user.c: In function ‘vu_log_queue_fill’:
>> libvhost-user.c:86:13: error: implicit declaration of function ‘typeof’ 
>> [-Werror=implicit-function-declaration]
>>86 | typeof(x) _min1 = (x);  \
>>   | ^~
>> Changing these two users of typeof makes the compiler happy and no extra
>> flags or pragmas need to be provided.
>> Signed-off-by: Marcel Holtmann
>> Reviewed-by: Philippe Mathieu-Daudé
> 
> The build system uses "c_std=gnu99".  If you are extracting libvhost-user and 
> not using its build files, you need to add --std=gnu99 yourself when 
> compiling.
> 
> If you really don't want to do that, as long as it's just a couple 
> underscores that's fine I guess, but mixed declarations and code are going to 
> reappear sooner or later.  Please add a patch like this:
> 
> diff --git a/subprojects/libvhost-user/meson.build 
> b/subprojects/libvhost-user/meson.build
> index 39825d9404ae..5deecbfe377d 100644
> --- a/subprojects/libvhost-user/meson.build
> +++ b/subprojects/libvhost-user/meson.build
> @@ -1,6 +1,13 @@
> project('libvhost-user', 'c',
> license: 'GPL-2.0-or-later',
> -default_options: ['c_std=gnu99'])
> +default_options: ['warning_level=1', 'c_std=gnu99'])
> +
> +cc = meson.get_compiler('c')
> +add_project_arguments(cc.get_supported_arguments(
> +  '-Wsign-compare',
> +  '-Wdeclaration-after-statement',
> +  '-Wstrict-aliasing'),
> +  native: false, language: 'c')

good idea. I included that in v4 patchset.

Regards

Marcel

[PATCH v4 10/12] libvhost-user: Fix assignment in vring_set_avail_event

[Marcel Holtmann]

Since it was proposed to change the code in libvduse.c to use memcpy
instead of an assignment, the code in libvhost-user.c should also be
changed to use memcpy.

Signed-off-by: Marcel Holtmann 
Suggested-by: Paolo Bonzini 
---
 subprojects/libvhost-user/libvhost-user.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c
index b28b66cdb159..fc69783d2bf6 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -2478,14 +2478,13 @@ vring_used_flags_unset_bit(VuVirtq *vq, int mask)
 static inline void
 vring_set_avail_event(VuVirtq *vq, uint16_t val)
 {
-uint16_t *avail;
+uint16_t val_le = htole16(val);
 
 if (!vq->notification) {
 return;
 }
 
-avail = (uint16_t *)&vq->vring.used->ring[vq->vring.num];
-*avail = htole16(val);
+memcpy(&vq->vring.used->ring[vq->vring.num], &val_le, sizeof(uint16_t));
 }
 
 void
-- 
2.38.1

[PATCH v4 11/12] libvhost-user: Add extra compiler warnings

[Marcel Holtmann]

In case libvhost-user is used externally, that projects compiler
warnings might be more strict. Enforce an extra set of compiler warnings
to catch issues early on.

Signed-off-by: Marcel Holtmann 
Suggested-by: Paolo Bonzini 
---
 subprojects/libvhost-user/meson.build | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/meson.build 
b/subprojects/libvhost-user/meson.build
index 39825d9404ae..a18014e7f26f 100644
--- a/subprojects/libvhost-user/meson.build
+++ b/subprojects/libvhost-user/meson.build
@@ -1,6 +1,12 @@
 project('libvhost-user', 'c',
 license: 'GPL-2.0-or-later',
-default_options: ['c_std=gnu99'])
+default_options: ['warning_level=1', 'c_std=gnu99'])
+
+cc = meson.get_compiler('c')
+add_project_arguments(cc.get_supported_arguments('-Wsign-compare',
+ 
'-Wdeclaration-after-statement',
+ '-Wstrict-aliasing'),
+  native: false, language: 'c')
 
 threads = dependency('threads')
 glib = dependency('glib-2.0')
-- 
2.38.1

Re: [PATCH v3 10/10] libvduse: Fix assignment in vring_set_avail_event

[Marcel Holtmann]

Hi Paolo,

>>  static inline void vring_set_avail_event(VduseVirtq *vq, uint16_t val)
>>  {
>> -*((uint16_t *)&vq->vring.used->ring[vq->vring.num]) = htole16(val);
>> +uint16_t *avail;
>> +
>> +avail = (uint16_t *)&vq->vring.used->ring[vq->vring.num];
>> +*avail = htole16(val);
> 
> That this doesn't warn is basically a compiler bug.
> 
> Please use memcpy instead, i.e.
> 
>  uint16_t val_le = htole16(val);
>  memcpy(&vq->vring.used->ring[vq->vring.num]), &val_le, sizeof(uint16_t));

excellent. Thanks for this. I included a version that fixes this for
libvhost-user as well.

Regards

Marcel

[PATCH v4 12/12] libvduse: Add extra compiler warnings

[Marcel Holtmann]

In case libvhost-user is used externally, that projects compiler
warnings might be more strict. Enforce an extra set of compiler warnings
to catch issues early on.

Signed-off-by: Marcel Holtmann 
Suggested-by: Paolo Bonzini 
---
 subprojects/libvduse/meson.build | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/subprojects/libvduse/meson.build b/subprojects/libvduse/meson.build
index ba08f5ee1a03..3e3b53da33ae 100644
--- a/subprojects/libvduse/meson.build
+++ b/subprojects/libvduse/meson.build
@@ -1,6 +1,12 @@
 project('libvduse', 'c',
 license: 'GPL-2.0-or-later',
-default_options: ['c_std=gnu99'])
+default_options: ['warning_level=1', 'c_std=gnu99'])
+
+cc = meson.get_compiler('c')
+add_project_arguments(cc.get_supported_arguments('-Wsign-compare',
+ 
'-Wdeclaration-after-statement',
+ '-Wstrict-aliasing'),
+  native: false, language: 'c')
 
 libvduse = static_library('vduse',
   files('libvduse.c'),
-- 
2.38.1

[PATCH v4 08/12] libvduse: Switch to unsigned int for inuse field in struct VduseVirtq

[Marcel Holtmann]

It seems there is no need to keep the inuse field signed and end up with
compiler warnings for sign-compare.

  CC   libvduse.o
libvduse.c: In function ‘vduse_queue_pop’:
libvduse.c:789:19: error: comparison of integer expressions of different 
signedness: ‘int’ and ‘unsigned int’ [-Werror=sign-compare]
  789 | if (vq->inuse >= vq->vring.num) {
  |   ^~

Instead of casting the comparison to unsigned int, just make the inuse
field unsigned int in the fist place.

Signed-off-by: Marcel Holtmann 
Reviewed-by: Xie Yongji 
---
 subprojects/libvduse/libvduse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c
index c871bd331a6b..338ad5e352e7 100644
--- a/subprojects/libvduse/libvduse.c
+++ b/subprojects/libvduse/libvduse.c
@@ -101,7 +101,7 @@ struct VduseVirtq {
 uint16_t signalled_used;
 bool signalled_used_valid;
 int index;
-int inuse;
+unsigned int inuse;
 bool ready;
 int fd;
 VduseDev *dev;
-- 
2.38.1

[PATCH v4 09/12] libvduse: Fix assignment in vring_set_avail_event

[Marcel Holtmann]

Since the assignment is causing a compiler warning, fix it by using
memcpy instead.

  CC   libvduse.o
libvduse.c: In function ‘vring_set_avail_event’:
libvduse.c:603:7: error: dereferencing type-punned pointer will break 
strict-aliasing rules [-Werror=strict-aliasin]
  603 | *((uint16_t *)&vq->vring.used->ring[vq->vring.num]) = htole16(val);
  |  ~^

Signed-off-by: Marcel Holtmann 
Suggested-by: Xie Yongji 
Suggested-by: Paolo Bonzini 
---
 subprojects/libvduse/libvduse.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c
index 338ad5e352e7..377959a0b4fb 100644
--- a/subprojects/libvduse/libvduse.c
+++ b/subprojects/libvduse/libvduse.c
@@ -582,7 +582,8 @@ void vduse_queue_notify(VduseVirtq *vq)
 
 static inline void vring_set_avail_event(VduseVirtq *vq, uint16_t val)
 {
-*((uint16_t *)&vq->vring.used->ring[vq->vring.num]) = htole16(val);
+uint16_t val_le = htole16(val);
+memcpy(&vq->vring.used->ring[vq->vring.num], &val_le, sizeof(uint16_t));
 }
 
 static bool vduse_queue_map_single_desc(VduseVirtq *vq, unsigned int *p_num_sg,
-- 
2.38.1

[PATCH v4 02/12] libvhost-user: Replace typeof with __typeof__

[Marcel Holtmann]

Strictly speaking only -std=gnu99 support the usage of typeof and for
easier inclusion in external projects, it is better to use __typeof__.

  CC   libvhost-user.o
libvhost-user.c: In function ‘vu_log_queue_fill’:
libvhost-user.c:86:13: error: implicit declaration of function ‘typeof’ 
[-Werror=implicit-function-declaration]
   86 | typeof(x) _min1 = (x);  \
  | ^~

Changing these two users of typeof makes the compiler happy and no extra
flags or pragmas need to be provided.

Signed-off-by: Marcel Holtmann 
Reviewed-by: Philippe Mathieu-Daudé 
---
 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 b55b9e244d9a..67d75ece53b7 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -62,8 +62,8 @@
 #endif  /* !__GNUC__ */
 #ifndef MIN
 #define MIN(x, y) ({\
-typeof(x) _min1 = (x);  \
-typeof(y) _min2 = (y);  \
+__typeof__(x) _min1 = (x);  \
+__typeof__(y) _min2 = (y);  \
 (void) (&_min1 == &_min2);  \
 _min1 < _min2 ? _min1 : _min2; })
 #endif
-- 
2.38.1

[PATCH v4 03/12] libvhost-user: Cast rc variable to avoid compiler warning

[Marcel Holtmann]

The assert from recvmsg() return value against an uint32_t size field
from a protocol struct throws a compiler warning.

  CC   libvhost-user.o
In file included from libvhost-user.c:27:
libvhost-user.c: In function ‘vu_message_read_default’:
libvhost-user.c:363:19: error: comparison of integer expressions of different 
signedness: ‘int’ and ‘uint32_t’ {aka ‘unsigned int’} [-Werror=sign-compare]
  363 | assert(rc == vmsg->size);
  |   ^~

This is not critical, but annoying when the libvhost-user source are
used in an external project that has this compiler warning switched on.

Signed-off-by: Marcel Holtmann 
---
 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 67d75ece53b7..bcdf32a24f60 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -339,7 +339,7 @@ vu_message_read_default(VuDev *dev, int conn_fd, 
VhostUserMsg *vmsg)
 goto fail;
 }
 
-assert(rc == vmsg->size);
+assert((uint32_t)rc == vmsg->size);
 }
 
 return true;
-- 
2.38.1

[PATCH v4 05/12] libvhost-user: Declare uffdio_register early to make it C90 compliant

[Marcel Holtmann]

When using libvhost-user source in an external project that wants to
comply with the C90 standard, it is best to declare variables before
code.

  CC   libvhost-user.o
libvhost-user.c: In function ‘generate_faults’:
libvhost-user.c:683:9: error: ISO C90 forbids mixed declarations and code 
[-Werror=declaration-after-statement]
  683 | struct uffdio_register reg_struct;
  | ^~

In this case, it is also simple enough and doesn't cause any extra
ifdef additions.

Signed-off-by: Marcel Holtmann 
Reviewed-by: Philippe Mathieu-Daudé 
---
 subprojects/libvhost-user/libvhost-user.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c 
b/subprojects/libvhost-user/libvhost-user.c
index 211d31a4cc88..bf92cc85c086 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -626,6 +626,8 @@ generate_faults(VuDev *dev) {
 VuDevRegion *dev_region = &dev->regions[i];
 int ret;
 #ifdef UFFDIO_REGISTER
+struct uffdio_register reg_struct;
+
 /*
  * We should already have an open ufd. Mark each memory
  * range as ufd.
@@ -659,7 +661,7 @@ generate_faults(VuDev *dev) {
 "%s: Failed to madvise(NOHUGEPAGE) region %d: %s\n",
 __func__, i, strerror(errno));
 }
-struct uffdio_register reg_struct;
+
 reg_struct.range.start = (uintptr_t)dev_region->mmap_addr;
 reg_struct.range.len = dev_region->size + dev_region->mmap_offset;
 reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
-- 
2.38.1