[PULL 00/20] Misc patches for 2023-05-25

[PULL 00/20] Misc patches for 2023-05-25

[Paolo Bonzini]

The following changes since commit 886c0453cbf10eebd42a9ccf89c3e46eb389c357:

  Merge tag 'pull-qapi-2023-05-17-v2' of https://repo.or.cz/qemu/armbru into staging (2023-05-22 15:54:21 -0700)

are available in the Git repository at:

  https://gitlab.com/bonzini/qemu.git tags/for-upstream

for you to fetch changes up to cd9b15bb62e4c422c3ed5fd69c613ecd5409838e:

  monitor: do not use mb_read/mb_set (2023-05-25 15:40:13 +0200)

----------------------------------------------------------------
* hot-unplug fixes for ioport
* purge qatomic_mb_read/set from monitor
* build system fixes and cleanups
* OHCI fix from gitlab
* provide EPYC-Rome CPU model not susceptible to XSAVES erratum

----------------------------------------------------------------
Maksim Davydov (1):
      target/i386: EPYC-Rome model without XSAVES

Mark Cave-Ayland (3):
      softmmu/ioport.c: allocate MemoryRegionPortioList ports on the heap
      softmmu/ioport.c: QOMify MemoryRegionPortioList
      softmmu/ioport.c: make MemoryRegionPortioList owner of portio_list MemoryRegions

Nicolas Saenz Julienne (1):
      meson.build: Fix glib -Wno-unused-function workaround

Paolo Bonzini (15):
      meson: fix rule for qemu-ga installer
      meson: move -no-pie from linker to compiler
      tests/docker: simplify HOST_ARCH definition
      tests/vm: fix and simplify HOST_ARCH definition
      Makefile: remove $(TESTS_PYTHON)
      usb/ohci: Set pad to 0 after frame update
      monitor: use QEMU_LOCK_GUARD a bit more
      monitor: allow calling monitor_resume under mon_lock
      monitor: add more *_locked() functions
      monitor: do not use mb_read/mb_set for suspend_cnt
      monitor: cleanup detection of qmp_dispatcher_co shutting down
      monitor: cleanup fetching of QMP requests
      monitor: introduce qmp_dispatcher_co_wake
      monitor: extract request dequeuing to a new function
      monitor: do not use mb_read/mb_set

 hw/usb/hcd-ohci.c             |   2 +
 include/monitor/monitor.h     |   3 ++
 meson.build                   |  21 +++++---
 monitor/hmp.c                 |  41 ++++++++--------
 monitor/monitor-internal.h    |   5 +-
 monitor/monitor.c             |  72 ++++++++++++----------------
 monitor/qmp.c                 | 108 +++++++++++++++++++++++++++---------------
 qga/meson.build               |   2 +-
 softmmu/ioport.c              |  61 +++++++++++++++++++++---
 target/i386/cpu.c             |  10 ++++
 tests/Makefile.include        |   8 ++--
 tests/docker/Makefile.include |   2 +-
 tests/qemu-iotests/051.out    |   4 +-
 tests/qemu-iotests/051.pc.out |  20 ++++----
 tests/vm/Makefile.include     |   7 ++-
 15 files changed, 225 insertions(+), 141 deletions(-)
-- 
2.40.1

[PULL 01/20] target/i386: EPYC-Rome model without XSAVES

[Paolo Bonzini]

From: Maksim Davydov <davydov-max@yandex-team.ru>

Based on the kernel commit "b0563468ee x86/CPU/AMD: Disable XSAVES on
AMD family 0x17", host system with EPYC-Rome can clear XSAVES capability
bit. In another words, EPYC-Rome host without XSAVES can occur. Thus, we
need an EPYC-Rome cpu model (without this feature) that matches the
solution of fixing this erratum

Signed-off-by: Maksim Davydov <davydov-max@yandex-team.ru>
Message-Id: <20230524213748.8918-1-davydov-max@yandex-team.ru>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/cpu.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a61cd6d99d1f..1242bd541a53 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4466,6 +4466,16 @@ static const X86CPUDefinition builtin_x86_defs[] = {
                 },
                 .cache_info = &epyc_rome_v3_cache_info
             },
+            {
+                .version = 4,
+                .props = (PropValue[]) {
+                    /* Erratum 1386 */
+                    { "model-id",
+                      "AMD EPYC-Rome-v4 Processor (no XSAVES)" },
+                    { "xsaves", "off" },
+                    { /* end of list */ }
+                },
+            },
             { /* end of list */ }
         }
     },
-- 
2.40.1

[PULL 02/20] meson.build: Fix glib -Wno-unused-function workaround

[Paolo Bonzini]

From: Nicolas Saenz Julienne <nsaenz@amazon.com>

We want to only enable '-Wno-unused-function' if glib's version is
smaller than '2.57.2' and has a G_DEFINE_AUTOPTR_CLEANUP_FUNC()
implementation that doesn't take into account unused functions. But the
compilation test isn't working as intended as '-Wunused-function' isn't
enabled while running it.

Let's enable it.

Fixes: fc9a809e0d28 ("build: move glib detection and workarounds to meson")
Signed-off-by: Nicolas Saenz Julienne <nsaenz@amazon.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230524173123.66483-1-nsaenz@amazon.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 0a5cdefd4d3d..448b71ad5b5c 100644
--- a/meson.build
+++ b/meson.build
@@ -770,7 +770,7 @@ if not cc.compiles('''
     g_free(f);
   }
   G_DEFINE_AUTOPTR_CLEANUP_FUNC(Foo, foo_free)
-  int main(void) { return 0; }''', dependencies: glib_pc, args: ['-Werror'])
+  int main(void) { return 0; }''', dependencies: glib_pc, args: ['-Wunused-function', '-Werror'])
   glib_cflags += cc.get_supported_arguments('-Wno-unused-function')
 endif
 glib = declare_dependency(dependencies: [glib_pc, gmodule],
-- 
2.40.1

[PULL 03/20] meson: fix rule for qemu-ga installer

[Paolo Bonzini]

The bindir variable is not available in the "glib" variable, which is an internal
dependency (created with "declare_dependency").  Use glib_pc instead, which contains
the variable as it is instantiated from glib-2.0.pc.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qga/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/meson.build b/qga/meson.build
index 622b5f94a232..d3291b4376cb 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -154,7 +154,7 @@ if targetos == 'windows'
                               qemu_ga_msi_arch[cpu],
                               qemu_ga_msi_vss,
                               '-D', 'BUILD_DIR=' + meson.project_build_root(),
-                              '-D', 'BIN_DIR=' + glib.get_variable('bindir'),
+                              '-D', 'BIN_DIR=' + glib_pc.get_variable('bindir'),
                               '-D', 'QEMU_GA_VERSION=' + config_host['QEMU_GA_VERSION'],
                               '-D', 'QEMU_GA_MANUFACTURER=' + config_host['QEMU_GA_MANUFACTURER'],
                               '-D', 'QEMU_GA_DISTRO=' + config_host['QEMU_GA_DISTRO'],
-- 
2.40.1

[PULL 04/20] meson: move -no-pie from linker to compiler

[Paolo Bonzini]

The large comment in the patch says it all; the -no-pie flag is broken and
this is why it was not included in QEMU_LDFLAGS before commit a988b4c5614
("build: move remaining compiler flag tests to meson", 2023-05-18).  And
some distros made things even worse, so we have to add it to the compiler
command line.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1664
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/meson.build b/meson.build
index 448b71ad5b5c..2e47608353d1 100644
--- a/meson.build
+++ b/meson.build
@@ -265,12 +265,21 @@ endif
 
 # Meson currently only handles pie as a boolean for now, so if the user
 # has explicitly disabled PIE we need to extend our cflags.
+#
+# -no-pie is supposedly a linker flag that has no effect on the compiler
+# command line, but some distros, that didn't quite know what they were
+# doing, made local changes to gcc's specs file that turned it into
+# a compiler command-line flag.
+#
+# What about linker flags?  For a static build, no PIE is implied by -static
+# which we added above (and if it's not because of the same specs patching,
+# there's nothing we can do: compilation will fail, report a bug to your
+# distro and do not use --disable-pie in the meanwhile).  For dynamic linking,
+# instead, we can't add -no-pie because it overrides -shared: the linker then
+# tries to build an executable instead of a shared library and fails.  So
+# don't add -no-pie anywhere and cross fingers. :(
 if not get_option('b_pie')
-  qemu_common_flags += cc.get_supported_arguments('-fno-pie')
-  if not get_option('prefer_static')
-    # No PIE is implied by -static which we added above.
-    qemu_ldflags += cc.get_supported_link_arguments('-no-pie')
-  endif
+  qemu_common_flags += cc.get_supported_arguments('-fno-pie', '-no-pie')
 endif
 
 if not get_option('stack_protector').disabled()
-- 
2.40.1

[PULL 05/20] tests/docker: simplify HOST_ARCH definition

[Paolo Bonzini]

ARCH is always empty, so just define HOST_ARCH as the result of uname.

Acked-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/docker/Makefile.include | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 94015253254c..142e8605eee9 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -6,7 +6,7 @@ NULL :=
 SPACE := $(NULL) #
 COMMA := ,
 
-HOST_ARCH = $(if $(ARCH),$(ARCH),$(shell uname -m))
+HOST_ARCH = $(shell uname -m)
 USER = $(if $(NOUSER),,$(shell id -un))
 UID = $(if $(NOUSER),,$(shell id -u))
 
-- 
2.40.1

[PULL 06/20] tests/vm: fix and simplify HOST_ARCH definition

[Paolo Bonzini]

ARCH is always empty, so just define HOST_ARCH as the result of uname.
The incorrect definition was not being used because the "ifeq" statement
is wrong; replace it with the same idiom based on $(realpath) that the
main Makefile uses.

With this change, vm-build-netbsd in a configured tree will not use
the PYTHONPATH hack.

Reported-by: John Snow <jsnow@redhat.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/vm/Makefile.include | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
index 2cc2203d0916..c2a8ca1c175a 100644
--- a/tests/vm/Makefile.include
+++ b/tests/vm/Makefile.include
@@ -1,14 +1,12 @@
 # Makefile for VM tests
 
 # Hack to allow running in an unconfigured build tree
-ifeq ($(wildcard $(SRC_PATH)/config-host.mak),)
+ifeq ($(realpath $(SRC_PATH)),$(realpath .))
 VM_PYTHON = PYTHONPATH=$(SRC_PATH)/python /usr/bin/env python3
 VM_VENV =
-HOST_ARCH := $(shell uname -m)
 else
 VM_PYTHON = $(TESTS_PYTHON)
 VM_VENV = check-venv
-HOST_ARCH = $(ARCH)
 endif
 
 .PHONY: vm-build-all vm-clean-all
@@ -23,6 +21,7 @@ ARM64_IMAGES += ubuntu.aarch64 centos.aarch64
 endif
 endif
 
+HOST_ARCH = $(shell uname -m)
 ifeq ($(HOST_ARCH),x86_64)
 IMAGES=$(X86_IMAGES) $(if $(USE_TCG),$(ARM64_IMAGES))
 else ifeq ($(HOST_ARCH),aarch64)
-- 
2.40.1

[PULL 07/20] Makefile: remove $(TESTS_PYTHON)

[Paolo Bonzini]

It is now the same as $(PYTHON), since the latter always points at pyvenv/bin/python3.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/Makefile.include    | 8 +++-----
 tests/vm/Makefile.include | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 5b838ec438b0..0184ef223737 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -89,11 +89,9 @@ distclean-tcg: $(DISTCLEAN_TCG_TARGET_RULES)
 # Build up our target list from the filtered list of ninja targets
 TARGETS=$(patsubst libqemu-%.fa, %, $(filter libqemu-%.fa, $(ninja-targets)))
 
-TESTS_VENV_DIR=$(BUILD_DIR)/pyvenv
 TESTS_VENV_TOKEN=$(BUILD_DIR)/pyvenv/tests.group
 TESTS_VENV_REQ=$(SRC_PATH)/tests/requirements.txt
 TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results
-TESTS_PYTHON=$(TESTS_VENV_DIR)/bin/python3
 ifndef AVOCADO_TESTS
 	AVOCADO_TESTS=tests/avocado
 endif
@@ -109,7 +107,7 @@ else
 endif
 
 quiet-venv-pip = $(quiet-@)$(call quiet-command-run, \
-    $(TESTS_PYTHON) -m pip -q --disable-pip-version-check $1, \
+    $(PYTHON) -m pip -q --disable-pip-version-check $1, \
     "VENVPIP","$1")
 
 $(TESTS_VENV_TOKEN): $(TESTS_VENV_REQ)
@@ -131,7 +129,7 @@ FEDORA_31_DOWNLOAD=$(filter $(FEDORA_31_ARCHES),$(FEDORA_31_ARCHES_CANDIDATES))
 # download one specific Fedora 31 image
 get-vm-image-fedora-31-%: check-venv
 	$(call quiet-command, \
-             $(TESTS_PYTHON) -m avocado vmimage get \
+             $(PYTHON) -m avocado vmimage get \
              --distro=fedora --distro-version=31 --arch=$*, \
 	"AVOCADO", "Downloading avocado tests VM image for $*")
 
@@ -142,7 +140,7 @@ JOBS_OPTION=$(lastword -j1 $(filter-out -j, $(filter -j%,$(MAKEFLAGS))))
 
 check-avocado: check-venv $(TESTS_RESULTS_DIR) get-vm-images
 	$(call quiet-command, 							\
-            $(TESTS_PYTHON) -m avocado 						\
+            $(PYTHON) -m avocado 						\
             --show=$(AVOCADO_SHOW) run --job-results-dir=$(TESTS_RESULTS_DIR) 	\
             $(if $(AVOCADO_TAGS),, 						\
 			--filter-by-tags-include-empty 				\
diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
index c2a8ca1c175a..f0f5d32fb0f0 100644
--- a/tests/vm/Makefile.include
+++ b/tests/vm/Makefile.include
@@ -5,7 +5,7 @@ ifeq ($(realpath $(SRC_PATH)),$(realpath .))
 VM_PYTHON = PYTHONPATH=$(SRC_PATH)/python /usr/bin/env python3
 VM_VENV =
 else
-VM_PYTHON = $(TESTS_PYTHON)
+VM_PYTHON = $(PYTHON)
 VM_VENV = check-venv
 endif
 
-- 
2.40.1

[PULL 08/20] usb/ohci: Set pad to 0 after frame update

[Paolo Bonzini]

When the OHCI controller's framenumber is incremented, HccaPad1 register
should be set to zero (Ref OHCI Spec 4.4)

ReactOS uses hccaPad1 to determine if the OHCI hardware is running,
consequently it fails this check in current qemu master.

Signed-off-by: Ryan Wendland <wendland@live.com.au>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1048
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/usb/hcd-ohci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 88d2b4b13c1d..cc5cde698328 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -1239,6 +1239,8 @@ static void ohci_frame_boundary(void *opaque)
     /* Increment frame number and take care of endianness. */
     ohci->frame_number = (ohci->frame_number + 1) & 0xffff;
     hcca.frame = cpu_to_le16(ohci->frame_number);
+    /* When the HC updates frame number, set pad to 0. Ref OHCI Spec 4.4.1*/
+    hcca.pad = 0;
 
     if (ohci->done_count == 0 && !(ohci->intr_status & OHCI_INTR_WD)) {
         if (!ohci->done) {
-- 
2.40.1

[PULL 09/20] softmmu/ioport.c: allocate MemoryRegionPortioList ports on the heap

[Paolo Bonzini]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

In order to facilitate a conversion of MemoryRegionPortioList to a QOM object
move the allocation of MemoryRegionPortioList ports to the heap instead of
using a variable-length member at the end of the MemoryRegionPortioList
structure.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230419151652.362717-2-mark.cave-ayland@ilande.co.uk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 softmmu/ioport.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/softmmu/ioport.c b/softmmu/ioport.c
index cb8adb0b9363..d0d5b0bcaa2d 100644
--- a/softmmu/ioport.c
+++ b/softmmu/ioport.c
@@ -35,7 +35,7 @@
 typedef struct MemoryRegionPortioList {
     MemoryRegion mr;
     void *portio_opaque;
-    MemoryRegionPortio ports[];
+    MemoryRegionPortio *ports;
 } MemoryRegionPortioList;
 
 static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
@@ -147,6 +147,7 @@ void portio_list_destroy(PortioList *piolist)
     for (i = 0; i < piolist->nr; ++i) {
         mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
         object_unparent(OBJECT(&mrpio->mr));
+        g_free(mrpio->ports);
         g_free(mrpio);
     }
     g_free(piolist->regions);
@@ -227,9 +228,9 @@ static void portio_list_add_1(PortioList *piolist,
     unsigned i;
 
     /* Copy the sub-list and null-terminate it.  */
-    mrpio = g_malloc0(sizeof(MemoryRegionPortioList) +
-                      sizeof(MemoryRegionPortio) * (count + 1));
+    mrpio = g_malloc0(sizeof(MemoryRegionPortioList));
     mrpio->portio_opaque = piolist->opaque;
+    mrpio->ports = g_malloc0(sizeof(MemoryRegionPortio) * (count + 1));
     memcpy(mrpio->ports, pio_init, sizeof(MemoryRegionPortio) * count);
     memset(mrpio->ports + count, 0, sizeof(MemoryRegionPortio));
 
-- 
2.40.1

[PULL 10/20] softmmu/ioport.c: QOMify MemoryRegionPortioList

[Paolo Bonzini]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

The aim of QOMification is so that the lifetime of the MemoryRegionPortioList
structure can be managed using QOM's in-built refcounting instead of having to
handle this manually.

Due to the use of an opaque pointer it isn't possible to model the new
TYPE_MEMORY_REGION_PORTIO_LIST directly using QOM properties, however since
use of the new object is restricted to the portio API we can simply set the
opaque pointer (and the heap-allocated port list) internally.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230419151652.362717-3-mark.cave-ayland@ilande.co.uk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 softmmu/ioport.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/softmmu/ioport.c b/softmmu/ioport.c
index d0d5b0bcaa2d..33720fe5664a 100644
--- a/softmmu/ioport.c
+++ b/softmmu/ioport.c
@@ -32,11 +32,16 @@
 #include "exec/address-spaces.h"
 #include "trace.h"
 
-typedef struct MemoryRegionPortioList {
+struct MemoryRegionPortioList {
+    Object obj;
+
     MemoryRegion mr;
     void *portio_opaque;
     MemoryRegionPortio *ports;
-} MemoryRegionPortioList;
+};
+
+#define TYPE_MEMORY_REGION_PORTIO_LIST "memory-region-portio-list"
+OBJECT_DECLARE_SIMPLE_TYPE(MemoryRegionPortioList, MEMORY_REGION_PORTIO_LIST)
 
 static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
 {
@@ -147,8 +152,7 @@ void portio_list_destroy(PortioList *piolist)
     for (i = 0; i < piolist->nr; ++i) {
         mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
         object_unparent(OBJECT(&mrpio->mr));
-        g_free(mrpio->ports);
-        g_free(mrpio);
+        object_unref(mrpio);
     }
     g_free(piolist->regions);
 }
@@ -228,7 +232,8 @@ static void portio_list_add_1(PortioList *piolist,
     unsigned i;
 
     /* Copy the sub-list and null-terminate it.  */
-    mrpio = g_malloc0(sizeof(MemoryRegionPortioList));
+    mrpio = MEMORY_REGION_PORTIO_LIST(
+                object_new(TYPE_MEMORY_REGION_PORTIO_LIST));
     mrpio->portio_opaque = piolist->opaque;
     mrpio->ports = g_malloc0(sizeof(MemoryRegionPortio) * (count + 1));
     memcpy(mrpio->ports, pio_init, sizeof(MemoryRegionPortio) * count);
@@ -298,3 +303,24 @@ void portio_list_del(PortioList *piolist)
         memory_region_del_subregion(piolist->address_space, &mrpio->mr);
     }
 }
+
+static void memory_region_portio_list_finalize(Object *obj)
+{
+    MemoryRegionPortioList *mrpio = MEMORY_REGION_PORTIO_LIST(obj);
+
+    g_free(mrpio->ports);
+}
+
+static const TypeInfo memory_region_portio_list_info = {
+    .parent             = TYPE_OBJECT,
+    .name               = TYPE_MEMORY_REGION_PORTIO_LIST,
+    .instance_size      = sizeof(MemoryRegionPortioList),
+    .instance_finalize  = memory_region_portio_list_finalize,
+};
+
+static void ioport_register_types(void)
+{
+    type_register_static(&memory_region_portio_list_info);
+}
+
+type_init(ioport_register_types)
-- 
2.40.1

[PULL 11/20] softmmu/ioport.c: make MemoryRegionPortioList owner of portio_list MemoryRegions

[Paolo Bonzini]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Currently when portio_list MemoryRegions are freed using portio_list_destroy() the RCU
thread segfaults generating a backtrace similar to that below:

    #0 0x5555599a34b6 in phys_section_destroy ../softmmu/physmem.c:996
    #1 0x5555599a37a3 in phys_sections_free ../softmmu/physmem.c:1011
    #2 0x5555599b24aa in address_space_dispatch_free ../softmmu/physmem.c:2430
    #3 0x55555996a283 in flatview_destroy ../softmmu/memory.c:292
    #4 0x55555a2cb9fb in call_rcu_thread ../util/rcu.c:284
    #5 0x55555a29b71d in qemu_thread_start ../util/qemu-thread-posix.c:541
    #6 0x7ffff4a0cea6 in start_thread nptl/pthread_create.c:477
    #7 0x7ffff492ca2e in __clone (/lib/x86_64-linux-gnu/libc.so.6+0xfca2e)

The problem here is that portio_list_destroy() unparents the portio_list
MemoryRegions causing them to be freed immediately, however the flatview
still has a reference to the MemoryRegion and so causes a use-after-free
segfault when the RCU thread next updates the flatview.

Solve the lifetime issue by making MemoryRegionPortioList the owner of the
portio_list MemoryRegions, and then reparenting them to the portio_list
owner. This ensures that they can be accessed as QOM children via the
portio_list owner, yet the MemoryRegionPortioList owns the refcount.

Update portio_list_destroy() to unparent the MemoryRegion from the
portio_list owner (while keeping mrpio->mr live until finalization of the
MemoryRegionPortioList), so that the portio_list MemoryRegions remain
allocated until flatview_destroy() removes the final refcount upon the
next flatview update.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230419151652.362717-4-mark.cave-ayland@ilande.co.uk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 softmmu/ioport.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/softmmu/ioport.c b/softmmu/ioport.c
index 33720fe5664a..b66e0a5a8ebc 100644
--- a/softmmu/ioport.c
+++ b/softmmu/ioport.c
@@ -229,6 +229,8 @@ static void portio_list_add_1(PortioList *piolist,
                               unsigned off_low, unsigned off_high)
 {
     MemoryRegionPortioList *mrpio;
+    Object *owner;
+    char *name;
     unsigned i;
 
     /* Copy the sub-list and null-terminate it.  */
@@ -245,8 +247,25 @@ static void portio_list_add_1(PortioList *piolist,
         mrpio->ports[i].base = start + off_low;
     }
 
-    memory_region_init_io(&mrpio->mr, piolist->owner, &portio_ops, mrpio,
+    /*
+     * The MemoryRegion owner is the MemoryRegionPortioList since that manages
+     * the lifecycle via the refcount
+     */
+    memory_region_init_io(&mrpio->mr, OBJECT(mrpio), &portio_ops, mrpio,
                           piolist->name, off_high - off_low);
+
+    /* Reparent the MemoryRegion to the piolist owner */
+    object_ref(&mrpio->mr);
+    object_unparent(OBJECT(&mrpio->mr));
+    if (!piolist->owner) {
+        owner = container_get(qdev_get_machine(), "/unattached");
+    } else {
+        owner = piolist->owner;
+    }
+    name = g_strdup_printf("%s[*]", piolist->name);
+    object_property_add_child(owner, name, OBJECT(&mrpio->mr));
+    g_free(name);
+
     if (piolist->flush_coalesced_mmio) {
         memory_region_set_flush_coalesced(&mrpio->mr);
     }
@@ -308,6 +327,7 @@ static void memory_region_portio_list_finalize(Object *obj)
 {
     MemoryRegionPortioList *mrpio = MEMORY_REGION_PORTIO_LIST(obj);
 
+    object_unref(&mrpio->mr);
     g_free(mrpio->ports);
 }
 
-- 
2.40.1

[PULL 12/20] monitor: use QEMU_LOCK_GUARD a bit more

[Paolo Bonzini]

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor/monitor.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 602535696c59..4b11bca2a21d 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -161,10 +161,9 @@ static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond,
 {
     Monitor *mon = opaque;
 
-    qemu_mutex_lock(&mon->mon_lock);
+    QEMU_LOCK_GUARD(&mon->mon_lock);
     mon->out_watch = 0;
     monitor_flush_locked(mon);
-    qemu_mutex_unlock(&mon->mon_lock);
     return FALSE;
 }
 
@@ -203,9 +202,8 @@ static void monitor_flush_locked(Monitor *mon)
 
 void monitor_flush(Monitor *mon)
 {
-    qemu_mutex_lock(&mon->mon_lock);
+    QEMU_LOCK_GUARD(&mon->mon_lock);
     monitor_flush_locked(mon);
-    qemu_mutex_unlock(&mon->mon_lock);
 }
 
 /* flush at every end of line */
-- 
2.40.1

[PULL 13/20] monitor: allow calling monitor_resume under mon_lock

[Paolo Bonzini]

Move monitor_resume()'s call to readline_show_prompt() outside the
potentially locked section.  Reuse the existing monitor_accept_input()
bottom half for this purpose.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor/monitor.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 4b11bca2a21d..7080d2da8ec6 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -567,6 +567,12 @@ static void monitor_accept_input(void *opaque)
 {
     Monitor *mon = opaque;
 
+    if (!monitor_is_qmp(mon)) {
+        MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
+        assert(hmp_mon->rs);
+        readline_show_prompt(hmp_mon->rs);
+    }
+
     qemu_chr_fe_accept_input(&mon->chr);
 }
 
@@ -585,12 +591,6 @@ void monitor_resume(Monitor *mon)
             ctx = qemu_get_aio_context();
         }
 
-        if (!monitor_is_qmp(mon)) {
-            MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
-            assert(hmp_mon->rs);
-            readline_show_prompt(hmp_mon->rs);
-        }
-
         aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
     }
 
-- 
2.40.1

[PULL 14/20] monitor: add more *_locked() functions

[Paolo Bonzini]

Allow flushing and printing to the monitor while mon->mon_lock is
held.  This will help cleaning up the locking of mon->mux_out and
mon->suspend_cnt.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/monitor/monitor.h |  3 +++
 monitor/monitor.c         | 14 ++++++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 033390f69917..965f5d545003 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -40,6 +40,9 @@ void monitor_flush(Monitor *mon);
 int monitor_set_cpu(Monitor *mon, int cpu_index);
 int monitor_get_cpu_index(Monitor *mon);
 
+int monitor_puts_locked(Monitor *mon, const char *str);
+void monitor_flush_locked(Monitor *mon);
+
 void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error **errp);
 
 void monitor_read_command(MonitorHMP *mon, int show_prompt);
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 7080d2da8ec6..20e33e28d20d 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -154,8 +154,6 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
     return !monitor_uses_readline(container_of(mon, MonitorHMP, common));
 }
 
-static void monitor_flush_locked(Monitor *mon);
-
 static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond,
                                   void *opaque)
 {
@@ -168,7 +166,7 @@ static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond,
 }
 
 /* Caller must hold mon->mon_lock */
-static void monitor_flush_locked(Monitor *mon)
+void monitor_flush_locked(Monitor *mon)
 {
     int rc;
     size_t len;
@@ -207,12 +205,11 @@ void monitor_flush(Monitor *mon)
 }
 
 /* flush at every end of line */
-int monitor_puts(Monitor *mon, const char *str)
+int monitor_puts_locked(Monitor *mon, const char *str)
 {
     int i;
     char c;
 
-    qemu_mutex_lock(&mon->mon_lock);
     for (i = 0; str[i]; i++) {
         c = str[i];
         if (c == '\n') {
@@ -223,11 +220,16 @@ int monitor_puts(Monitor *mon, const char *str)
             monitor_flush_locked(mon);
         }
     }
-    qemu_mutex_unlock(&mon->mon_lock);
 
     return i;
 }
 
+int monitor_puts(Monitor *mon, const char *str)
+{
+    QEMU_LOCK_GUARD(&mon->mon_lock);
+    return monitor_puts_locked(mon, str);
+}
+
 int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
 {
     char *buf;
-- 
2.40.1

[PULL 15/20] monitor: do not use mb_read/mb_set for suspend_cnt

[Paolo Bonzini]

Clean up monitor_event to just use monitor_suspend/monitor_resume,
using mon->mux_out to protect against incorrect nesting (especially
on startup).

The only remaining case of reading suspend_cnt is in the can_read
callback, which is just advisory and can use qatomic_read.

As an extra benefit, mux_out is now simply protected by mon_lock.
Also, moving the prompt to the beginning of the main loop removes
it from the output in some error cases where QEMU does not actually
start successfully.  It is not a full fix and it would be nice to
also remove the monitor heading, but this is already a small (though
unintentional) improvement.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor/hmp.c                 | 41 ++++++++++++++++-------------------
 monitor/monitor-internal.h    |  3 +--
 monitor/monitor.c             |  9 ++++++--
 tests/qemu-iotests/051.out    |  4 ++--
 tests/qemu-iotests/051.pc.out | 20 ++++++++---------
 5 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/monitor/hmp.c b/monitor/hmp.c
index 5cab56d355c8..69c1b7e98abb 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1401,45 +1401,42 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size)
 static void monitor_event(void *opaque, QEMUChrEvent event)
 {
     Monitor *mon = opaque;
-    MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
 
     switch (event) {
     case CHR_EVENT_MUX_IN:
         qemu_mutex_lock(&mon->mon_lock);
-        mon->mux_out = 0;
-        qemu_mutex_unlock(&mon->mon_lock);
-        if (mon->reset_seen) {
-            readline_restart(hmp_mon->rs);
+        if (mon->mux_out) {
+            mon->mux_out = 0;
             monitor_resume(mon);
-            monitor_flush(mon);
-        } else {
-            qatomic_mb_set(&mon->suspend_cnt, 0);
         }
+        qemu_mutex_unlock(&mon->mon_lock);
         break;
 
     case CHR_EVENT_MUX_OUT:
-        if (mon->reset_seen) {
-            if (qatomic_mb_read(&mon->suspend_cnt) == 0) {
-                monitor_printf(mon, "\n");
-            }
-            monitor_flush(mon);
-            monitor_suspend(mon);
-        } else {
-            qatomic_inc(&mon->suspend_cnt);
-        }
         qemu_mutex_lock(&mon->mon_lock);
-        mon->mux_out = 1;
+        if (!mon->mux_out) {
+            if (mon->reset_seen && !mon->suspend_cnt) {
+                monitor_puts_locked(mon, "\n");
+            } else {
+                monitor_flush_locked(mon);
+            }
+            monitor_suspend(mon);
+            mon->mux_out = 1;
+        }
         qemu_mutex_unlock(&mon->mon_lock);
         break;
 
     case CHR_EVENT_OPENED:
         monitor_printf(mon, "QEMU %s monitor - type 'help' for more "
                        "information\n", QEMU_VERSION);
-        if (!mon->mux_out) {
-            readline_restart(hmp_mon->rs);
-            readline_show_prompt(hmp_mon->rs);
-        }
+        qemu_mutex_lock(&mon->mon_lock);
         mon->reset_seen = 1;
+        if (!mon->mux_out) {
+            /* Suspend-resume forces the prompt to be printed.  */
+            monitor_suspend(mon);
+            monitor_resume(mon);
+        }
+        qemu_mutex_unlock(&mon->mon_lock);
         mon_refcount++;
         break;
 
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 53e3808054c7..61c9b6916db3 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -94,7 +94,6 @@ typedef struct HMPCommand {
 
 struct Monitor {
     CharBackend chr;
-    int reset_seen;
     int suspend_cnt;            /* Needs to be accessed atomically */
     bool is_qmp;
     bool skip_flush;
@@ -115,8 +114,8 @@ struct Monitor {
     QLIST_HEAD(, mon_fd_t) fds;
     GString *outbuf;
     guint out_watch;
-    /* Read under either BQL or mon_lock, written with BQL+mon_lock.  */
     int mux_out;
+    int reset_seen;
 };
 
 struct MonitorHMP {
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 20e33e28d20d..15f97538ef2b 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -569,10 +569,15 @@ static void monitor_accept_input(void *opaque)
 {
     Monitor *mon = opaque;
 
-    if (!monitor_is_qmp(mon)) {
+    qemu_mutex_lock(&mon->mon_lock);
+    if (!monitor_is_qmp(mon) && mon->reset_seen) {
         MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
         assert(hmp_mon->rs);
+        readline_restart(hmp_mon->rs);
+        qemu_mutex_unlock(&mon->mon_lock);
         readline_show_prompt(hmp_mon->rs);
+    } else {
+        qemu_mutex_unlock(&mon->mon_lock);
     }
 
     qemu_chr_fe_accept_input(&mon->chr);
@@ -603,7 +608,7 @@ int monitor_can_read(void *opaque)
 {
     Monitor *mon = opaque;
 
-    return !qatomic_mb_read(&mon->suspend_cnt);
+    return !qatomic_read(&mon->suspend_cnt);
 }
 
 void monitor_list_append(Monitor *mon)
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index e5ddb03bda1d..d46215640506 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -74,7 +74,7 @@ QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=foo#12: Invalid node-name: 'fo
 
 Testing: -device virtio-scsi -device scsi-hd
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device scsi-hd: drive property not set
+QEMU_PROG: -device scsi-hd: drive property not set
 
 
 === Overriding backing file ===
@@ -134,7 +134,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive if=virtio
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -drive if=virtio: Device needs media, but drive is empty
+QEMU_PROG: -drive if=virtio: Device needs media, but drive is empty
 
 
 === Attach to node in non-default iothread ===
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index bade1ff3b929..4d4af5a486df 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -74,7 +74,7 @@ QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=foo#12: Invalid node-name: 'fo
 
 Testing: -device virtio-scsi -device scsi-hd
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device scsi-hd: drive property not set
+QEMU_PROG: -device scsi-hd: drive property not set
 
 
 === Overriding backing file ===
@@ -142,11 +142,11 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive if=ide
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: Device needs media, but drive is empty
+QEMU_PROG: Device needs media, but drive is empty
 
 Testing: -drive if=virtio
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -drive if=virtio: Device needs media, but drive is empty
+QEMU_PROG: -drive if=virtio: Device needs media, but drive is empty
 
 Testing: -drive if=none,id=disk -device ide-cd,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
@@ -158,22 +158,22 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive if=none,id=disk -device ide-hd,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device ide-hd,drive=disk: Device needs media, but drive is empty
+QEMU_PROG: -device ide-hd,drive=disk: Device needs media, but drive is empty
 
 Testing: -drive if=none,id=disk -device lsi53c895a -device scsi-hd,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device scsi-hd,drive=disk: Device needs media, but drive is empty
+QEMU_PROG: -device scsi-hd,drive=disk: Device needs media, but drive is empty
 
 
 === Attach to node in non-default iothread ===
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device ide-hd,drive=disk,share-rw=on
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device ide-hd,drive=disk,share-rw=on: Cannot change iothread of active block backend
+QEMU_PROG: -device ide-hd,drive=disk,share-rw=on: Cannot change iothread of active block backend
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,share-rw=on
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,share-rw=on: Cannot change iothread of active block backend
+QEMU_PROG: -device virtio-blk-pci,drive=disk,share-rw=on: Cannot change iothread of active block backend
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device lsi53c895a,id=lsi0 -device scsi-hd,bus=lsi0.0,drive=disk,share-rw=on
 QEMU X.Y.Z monitor - type 'help' for more information
@@ -185,7 +185,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on: Cannot change iothread of active block backend
+QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on: Cannot change iothread of active block backend
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-scsi,id=virtio-scsi1,iothread=thread0 -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on
 QEMU X.Y.Z monitor - type 'help' for more information
@@ -204,7 +204,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=ide,readonly=on
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: Block node is read-only
+QEMU_PROG: Block node is read-only
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=virtio,readonly=on
 QEMU X.Y.Z monitor - type 'help' for more information
@@ -220,7 +220,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,id=disk,readonly=on -device ide-hd,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device ide-hd,drive=disk: Block node is read-only
+QEMU_PROG: -device ide-hd,drive=disk: Block node is read-only
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,id=disk,readonly=on -device lsi53c895a -device scsi-hd,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
-- 
2.40.1

[PULL 16/20] monitor: cleanup detection of qmp_dispatcher_co shutting down

[Paolo Bonzini]

Instead of overloading qmp_dispatcher_co_busy, make the coroutine
pointer NULL.  This will make things break spectacularly if somebody
tries to start a request after monitor_cleanup().

AIO_WAIT_WHILE_UNLOCKED() does not need qatomic_mb_read(), because
the macro contains all the necessary memory barriers.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor/monitor.c | 2 +-
 monitor/qmp.c     | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 15f97538ef2b..c4ed2547c25f 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -686,7 +686,7 @@ void monitor_cleanup(void)
 
     AIO_WAIT_WHILE_UNLOCKED(NULL,
                    (aio_poll(iohandler_get_aio_context(), false),
-                    qatomic_mb_read(&qmp_dispatcher_co_busy)));
+                    qatomic_read(&qmp_dispatcher_co)));
 
     /*
      * We need to explicitly stop the I/O thread (but not destroy it),
diff --git a/monitor/qmp.c b/monitor/qmp.c
index 092c527b6fc9..f0cc6dc886f8 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -226,6 +226,7 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
 
         /* On shutdown, don't take any more requests from the queue */
         if (qmp_dispatcher_co_shutdown) {
+            qatomic_set(&qmp_dispatcher_co, NULL);
             return;
         }
 
@@ -250,6 +251,7 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
              * yielded and were reentered from monitor_cleanup()
              */
             if (qmp_dispatcher_co_shutdown) {
+                qatomic_set(&qmp_dispatcher_co, NULL);
                 return;
             }
         }
-- 
2.40.1

[PULL 17/20] monitor: cleanup fetching of QMP requests

[Paolo Bonzini]

Use a continue statement so that "after going to sleep" is treated the same
way as "after processing a request".  Pull the monitor_lock critical
section out of monitor_qmp_requests_pop_any_with_lock() and protect
qmp_dispatcher_co_shutdown with the monitor_lock.

The two changes are complex to separate because monitor_qmp_dispatcher_co()
previously had a complicated logic to check for shutdown both before
and after going to sleep.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor/monitor.c |  9 +++++++--
 monitor/qmp.c     | 40 +++++++++++++++-------------------------
 2 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index c4ed2547c25f..042a1ab918f9 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -56,7 +56,10 @@ IOThread *mon_iothread;
 /* Coroutine to dispatch the requests received from I/O thread */
 Coroutine *qmp_dispatcher_co;
 
-/* Set to true when the dispatcher coroutine should terminate */
+/*
+ * Set to true when the dispatcher coroutine should terminate.  Protected
+ * by monitor_lock.
+ */
 bool qmp_dispatcher_co_shutdown;
 
 /*
@@ -679,7 +682,9 @@ void monitor_cleanup(void)
      * we'll just leave them in the queue without sending a response
      * and monitor_data_destroy() will free them.
      */
-    qmp_dispatcher_co_shutdown = true;
+    WITH_QEMU_LOCK_GUARD(&monitor_lock) {
+        qmp_dispatcher_co_shutdown = true;
+    }
     if (!qatomic_xchg(&qmp_dispatcher_co_busy, true)) {
         aio_co_wake(qmp_dispatcher_co);
     }
diff --git a/monitor/qmp.c b/monitor/qmp.c
index f0cc6dc886f8..dfc215632865 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -178,8 +178,6 @@ static QMPRequest *monitor_qmp_requests_pop_any_with_lock(void)
     Monitor *mon;
     MonitorQMP *qmp_mon;
 
-    QEMU_LOCK_GUARD(&monitor_lock);
-
     QTAILQ_FOREACH(mon, &mon_list, entry) {
         if (!monitor_is_qmp(mon)) {
             continue;
@@ -215,6 +213,10 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
     MonitorQMP *mon;
 
     while (true) {
+        /*
+         * busy must be set to true again by whoever
+         * rescheduled us to avoid double scheduling
+         */
         assert(qatomic_mb_read(&qmp_dispatcher_co_busy) == true);
 
         /*
@@ -224,36 +226,23 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
          */
         qatomic_mb_set(&qmp_dispatcher_co_busy, false);
 
-        /* On shutdown, don't take any more requests from the queue */
-        if (qmp_dispatcher_co_shutdown) {
-            qatomic_set(&qmp_dispatcher_co, NULL);
-            return;
+        WITH_QEMU_LOCK_GUARD(&monitor_lock) {
+            /* On shutdown, don't take any more requests from the queue */
+            if (qmp_dispatcher_co_shutdown) {
+                return NULL;
+            }
+
+            req_obj = monitor_qmp_requests_pop_any_with_lock();
         }
 
-        while (!(req_obj = monitor_qmp_requests_pop_any_with_lock())) {
+        if (!req_obj) {
             /*
              * No more requests to process.  Wait to be reentered from
              * handle_qmp_command() when it pushes more requests, or
              * from monitor_cleanup() when it requests shutdown.
              */
-            if (!qmp_dispatcher_co_shutdown) {
-                qemu_coroutine_yield();
-
-                /*
-                 * busy must be set to true again by whoever
-                 * rescheduled us to avoid double scheduling
-                 */
-                assert(qatomic_xchg(&qmp_dispatcher_co_busy, false) == true);
-            }
-
-            /*
-             * qmp_dispatcher_co_shutdown may have changed if we
-             * yielded and were reentered from monitor_cleanup()
-             */
-            if (qmp_dispatcher_co_shutdown) {
-                qatomic_set(&qmp_dispatcher_co, NULL);
-                return;
-            }
+            qemu_coroutine_yield();
+            continue;
         }
 
         trace_monitor_qmp_in_band_dequeue(req_obj,
@@ -342,6 +331,7 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
         aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
         qemu_coroutine_yield();
     }
+    qatomic_set(&qmp_dispatcher_co, NULL);
 }
 
 static void handle_qmp_command(void *opaque, QObject *req, Error *err)
-- 
2.40.1

[PULL 18/20] monitor: introduce qmp_dispatcher_co_wake

[Paolo Bonzini]

This makes it possible to turn qmp_dispatcher_co_busy into a static
variable.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor/monitor-internal.h |  2 +-
 monitor/monitor.c          | 26 +-------------------------
 monitor/qmp.c              | 32 +++++++++++++++++++++++++++++---
 3 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 61c9b6916db3..252de856812f 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -165,7 +165,6 @@ typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
 extern IOThread *mon_iothread;
 extern Coroutine *qmp_dispatcher_co;
 extern bool qmp_dispatcher_co_shutdown;
-extern bool qmp_dispatcher_co_busy;
 extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
 extern QemuMutex monitor_lock;
 extern MonitorList mon_list;
@@ -183,6 +182,7 @@ void monitor_fdsets_cleanup(void);
 void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
 void monitor_data_destroy_qmp(MonitorQMP *mon);
 void coroutine_fn monitor_qmp_dispatcher_co(void *data);
+void qmp_dispatcher_co_wake(void);
 
 int get_monitor_def(Monitor *mon, int64_t *pval, const char *name);
 void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 042a1ab918f9..dc352f9e9d95 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -62,27 +62,6 @@ Coroutine *qmp_dispatcher_co;
  */
 bool qmp_dispatcher_co_shutdown;
 
-/*
- * qmp_dispatcher_co_busy is used for synchronisation between the
- * monitor thread and the main thread to ensure that the dispatcher
- * coroutine never gets scheduled a second time when it's already
- * scheduled (scheduling the same coroutine twice is forbidden).
- *
- * It is true if the coroutine is active and processing requests.
- * Additional requests may then be pushed onto mon->qmp_requests,
- * and @qmp_dispatcher_co_shutdown may be set without further ado.
- * @qmp_dispatcher_co_busy must not be woken up in this case.
- *
- * If false, you also have to set @qmp_dispatcher_co_busy to true and
- * wake up @qmp_dispatcher_co after pushing the new requests.
- *
- * The coroutine will automatically change this variable back to false
- * before it yields.  Nobody else may set the variable to false.
- *
- * Access must be atomic for thread safety.
- */
-bool qmp_dispatcher_co_busy;
-
 /*
  * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
  * monitor_destroyed.
@@ -685,9 +664,7 @@ void monitor_cleanup(void)
     WITH_QEMU_LOCK_GUARD(&monitor_lock) {
         qmp_dispatcher_co_shutdown = true;
     }
-    if (!qatomic_xchg(&qmp_dispatcher_co_busy, true)) {
-        aio_co_wake(qmp_dispatcher_co);
-    }
+    qmp_dispatcher_co_wake();
 
     AIO_WAIT_WHILE_UNLOCKED(NULL,
                    (aio_poll(iohandler_get_aio_context(), false),
@@ -742,7 +719,6 @@ void monitor_init_globals(void)
      * rid of those assumptions.
      */
     qmp_dispatcher_co = qemu_coroutine_create(monitor_qmp_dispatcher_co, NULL);
-    qatomic_mb_set(&qmp_dispatcher_co_busy, true);
     aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
 }
 
diff --git a/monitor/qmp.c b/monitor/qmp.c
index dfc215632865..613b74ec74a7 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -33,6 +33,27 @@
 #include "qapi/qmp/qlist.h"
 #include "trace.h"
 
+/*
+ * qmp_dispatcher_co_busy is used for synchronisation between the
+ * monitor thread and the main thread to ensure that the dispatcher
+ * coroutine never gets scheduled a second time when it's already
+ * scheduled (scheduling the same coroutine twice is forbidden).
+ *
+ * It is true if the coroutine is active and processing requests.
+ * Additional requests may then be pushed onto mon->qmp_requests,
+ * and @qmp_dispatcher_co_shutdown may be set without further ado.
+ * @qmp_dispatcher_co_busy must not be woken up in this case.
+ *
+ * If false, you also have to set @qmp_dispatcher_co_busy to true and
+ * wake up @qmp_dispatcher_co after pushing the new requests.
+ *
+ * The coroutine will automatically change this variable back to false
+ * before it yields.  Nobody else may set the variable to false.
+ *
+ * Access must be atomic for thread safety.
+ */
+static bool qmp_dispatcher_co_busy = true;
+
 struct QMPRequest {
     /* Owner of the request */
     MonitorQMP *mon;
@@ -334,6 +355,13 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
     qatomic_set(&qmp_dispatcher_co, NULL);
 }
 
+void qmp_dispatcher_co_wake(void)
+{
+    if (!qatomic_xchg(&qmp_dispatcher_co_busy, true)) {
+        aio_co_wake(qmp_dispatcher_co);
+    }
+}
+
 static void handle_qmp_command(void *opaque, QObject *req, Error *err)
 {
     MonitorQMP *mon = opaque;
@@ -395,9 +423,7 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
     }
 
     /* Kick the dispatcher routine */
-    if (!qatomic_xchg(&qmp_dispatcher_co_busy, true)) {
-        aio_co_wake(qmp_dispatcher_co);
-    }
+    qmp_dispatcher_co_wake();
 }
 
 static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
-- 
2.40.1

[PULL 19/20] monitor: extract request dequeuing to a new function

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor/qmp.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/monitor/qmp.c b/monitor/qmp.c
index 613b74ec74a7..e6b1043c9f7b 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -226,13 +226,8 @@ static QMPRequest *monitor_qmp_requests_pop_any_with_lock(void)
     return req_obj;
 }
 
-void coroutine_fn monitor_qmp_dispatcher_co(void *data)
+static QMPRequest *monitor_qmp_dispatcher_pop_any(void)
 {
-    QMPRequest *req_obj = NULL;
-    QDict *rsp;
-    bool oob_enabled;
-    MonitorQMP *mon;
-
     while (true) {
         /*
          * busy must be set to true again by whoever
@@ -248,24 +243,36 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
         qatomic_mb_set(&qmp_dispatcher_co_busy, false);
 
         WITH_QEMU_LOCK_GUARD(&monitor_lock) {
+            QMPRequest *req_obj;
+
             /* On shutdown, don't take any more requests from the queue */
             if (qmp_dispatcher_co_shutdown) {
                 return NULL;
             }
 
             req_obj = monitor_qmp_requests_pop_any_with_lock();
+            if (req_obj) {
+                return req_obj;
+            }
         }
 
-        if (!req_obj) {
-            /*
-             * No more requests to process.  Wait to be reentered from
-             * handle_qmp_command() when it pushes more requests, or
-             * from monitor_cleanup() when it requests shutdown.
-             */
-            qemu_coroutine_yield();
-            continue;
-        }
+        /*
+         * No more requests to process.  Wait to be reentered from
+         * handle_qmp_command() when it pushes more requests, or
+         * from monitor_cleanup() when it requests shutdown.
+         */
+        qemu_coroutine_yield();
+    }
+}
 
+void coroutine_fn monitor_qmp_dispatcher_co(void *data)
+{
+    QMPRequest *req_obj;
+    QDict *rsp;
+    bool oob_enabled;
+    MonitorQMP *mon;
+
+    while ((req_obj = monitor_qmp_dispatcher_pop_any()) != NULL) {
         trace_monitor_qmp_in_band_dequeue(req_obj,
                                           req_obj->mon->qmp_requests->length);
 
-- 
2.40.1

[PULL 20/20] monitor: do not use mb_read/mb_set

[Paolo Bonzini]

Instead of relying on magic memory barriers, document the pattern that
is being used.  It is the one based on Dekker's algorithm, and in this
case it is embodied as follows:

    enqueue request;              sleeping = true;
    smp_mb();                     smp_mb();
    if (sleeping) kick();         if (!have a request) yield();

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor/qmp.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/monitor/qmp.c b/monitor/qmp.c
index e6b1043c9f7b..c8e0156974d9 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -39,13 +39,16 @@
  * coroutine never gets scheduled a second time when it's already
  * scheduled (scheduling the same coroutine twice is forbidden).
  *
- * It is true if the coroutine is active and processing requests.
- * Additional requests may then be pushed onto mon->qmp_requests,
- * and @qmp_dispatcher_co_shutdown may be set without further ado.
- * @qmp_dispatcher_co_busy must not be woken up in this case.
+ * It is true if the coroutine will process at least one more request
+ * before going to sleep.  Either it has been kicked already, or it
+ * is active and processing requests.  Additional requests may therefore
+ * be pushed onto mon->qmp_requests, and @qmp_dispatcher_co_shutdown may
+ * be set without further ado.  @qmp_dispatcher_co must not be woken up
+ * in this case.
  *
- * If false, you also have to set @qmp_dispatcher_co_busy to true and
- * wake up @qmp_dispatcher_co after pushing the new requests.
+ * If false, you have to wake up @qmp_dispatcher_co after pushing new
+ * requests. You also have to set @qmp_dispatcher_co_busy to true
+ * before waking up the coroutine.
  *
  * The coroutine will automatically change this variable back to false
  * before it yields.  Nobody else may set the variable to false.
@@ -230,15 +233,18 @@ static QMPRequest *monitor_qmp_dispatcher_pop_any(void)
 {
     while (true) {
         /*
-         * busy must be set to true again by whoever
-         * rescheduled us to avoid double scheduling
+         * To avoid double scheduling, busy is true on entry to
+         * monitor_qmp_dispatcher_co(), and must be set again before
+         * aio_co_wake()-ing it.
          */
-        assert(qatomic_mb_read(&qmp_dispatcher_co_busy) == true);
+        assert(qatomic_read(&qmp_dispatcher_co_busy) == true);
 
         /*
          * Mark the dispatcher as not busy already here so that we
          * don't miss any new requests coming in the middle of our
          * processing.
+         *
+         * Clear qmp_dispatcher_co_busy before reading request.
          */
         qatomic_mb_set(&qmp_dispatcher_co_busy, false);
 
@@ -364,6 +370,9 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
 
 void qmp_dispatcher_co_wake(void)
 {
+    /* Write request before reading qmp_dispatcher_co_busy.  */
+    smp_mb__before_rmw();
+
     if (!qatomic_xchg(&qmp_dispatcher_co_busy, true)) {
         aio_co_wake(qmp_dispatcher_co);
     }
-- 
2.40.1

Re: [PULL 00/20] Misc patches for 2023-05-25

[Richard Henderson]

On 5/25/23 07:15, Paolo Bonzini wrote:
> The following changes since commit 886c0453cbf10eebd42a9ccf89c3e46eb389c357:
> 
>    Merge tag 'pull-qapi-2023-05-17-v2' ofhttps://repo.or.cz/qemu/armbru  into staging (2023-05-22 15:54:21 -0700)
> 
> are available in the Git repository at:
> 
>    https://gitlab.com/bonzini/qemu.git  tags/for-upstream
> 
> for you to fetch changes up to cd9b15bb62e4c422c3ed5fd69c613ecd5409838e:
> 
>    monitor: do not use mb_read/mb_set (2023-05-25 15:40:13 +0200)
> 
> ----------------------------------------------------------------
> * hot-unplug fixes for ioport
> * purge qatomic_mb_read/set from monitor
> * build system fixes and cleanups
> * OHCI fix from gitlab
> * provide EPYC-Rome CPU model not susceptible to XSAVES erratum

Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/8.1 as appropriate.


r~