From fb00aa61267c8b9c57a2d1a1fa1e336d02e3bcd1 Mon Sep 17 00:00:00 2001 From: Maksim Davydov Date: Thu, 25 May 2023 00:37:48 +0300 Subject: [PATCH 01/17] target/i386: EPYC-Rome model without XSAVES 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 Message-Id: <20230524213748.8918-1-davydov-max@yandex-team.ru> Signed-off-by: Paolo Bonzini --- target/i386/cpu.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/target/i386/cpu.c b/target/i386/cpu.c index a61cd6d99d..1242bd541a 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 */ } } }, From f49d883d4de4011365ca3644fcd1914df5193227 Mon Sep 17 00:00:00 2001 From: Nicolas Saenz Julienne Date: Wed, 24 May 2023 17:31:23 +0000 Subject: [PATCH 02/17] meson.build: Fix glib -Wno-unused-function workaround MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20230524173123.66483-1-nsaenz@amazon.com> Signed-off-by: Paolo Bonzini --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meson.build b/meson.build index 0a5cdefd4d..448b71ad5b 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], From 91a2e6882a788f270b6d9bc168128cd252812808 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 22 May 2023 09:19:03 +0200 Subject: [PATCH 03/17] meson: fix rule for qemu-ga installer 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 --- qga/meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qga/meson.build b/qga/meson.build index 622b5f94a2..d3291b4376 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'], From b03fcd6818f690d168860f895bc9e8eab971d6de Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 22 May 2023 10:05:33 +0200 Subject: [PATCH 04/17] meson: move -no-pie from linker to compiler 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 --- meson.build | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/meson.build b/meson.build index 448b71ad5b..2e47608353 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() From 6301460ce9f59885e8feb65185bcfb6b128c8eff Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 23 May 2023 17:58:40 +0200 Subject: [PATCH 05/17] usb/ohci: Set pad to 0 after frame update 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 Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1048 Signed-off-by: Paolo Bonzini --- 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 88d2b4b13c..cc5cde6983 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) { From d2f07b75aea5f49533c169592f951fd09f77037b Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 19 Apr 2023 16:16:50 +0100 Subject: [PATCH 06/17] softmmu/ioport.c: allocate MemoryRegionPortioList ports on the heap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20230419151652.362717-2-mark.cave-ayland@ilande.co.uk> Signed-off-by: Paolo Bonzini --- softmmu/ioport.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/softmmu/ioport.c b/softmmu/ioport.c index cb8adb0b93..d0d5b0bcaa 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)); From 28770689c5ff53195410fec407a0af7a2d4ac03a Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 19 Apr 2023 16:16:51 +0100 Subject: [PATCH 07/17] softmmu/ioport.c: QOMify MemoryRegionPortioList MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20230419151652.362717-3-mark.cave-ayland@ilande.co.uk> Signed-off-by: Paolo Bonzini --- softmmu/ioport.c | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/softmmu/ioport.c b/softmmu/ioport.c index d0d5b0bcaa..33720fe566 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) From 690705ca0b0f1ed24a34ccd14c9866fbe47c69a6 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 19 Apr 2023 16:16:52 +0100 Subject: [PATCH 08/17] softmmu/ioport.c: make MemoryRegionPortioList owner of portio_list MemoryRegions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20230419151652.362717-4-mark.cave-ayland@ilande.co.uk> Signed-off-by: Paolo Bonzini --- softmmu/ioport.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/softmmu/ioport.c b/softmmu/ioport.c index 33720fe566..b66e0a5a8e 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); } From e37548ef13dcbe158662c8dc9797c15c052c3a81 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 17 May 2023 14:47:55 +0200 Subject: [PATCH 09/17] monitor: use QEMU_LOCK_GUARD a bit more Reviewed-by: Richard Henderson Signed-off-by: Paolo Bonzini --- monitor/monitor.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/monitor/monitor.c b/monitor/monitor.c index 602535696c..4b11bca2a2 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 */ From c5d0c55f1ac402327235e4046f3921d16bc7b529 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 17 May 2023 17:19:03 +0200 Subject: [PATCH 10/17] monitor: allow calling monitor_resume under mon_lock 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 --- monitor/monitor.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/monitor/monitor.c b/monitor/monitor.c index 4b11bca2a2..7080d2da8e 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); } From 4cb96b974265f97a9902b4458e50d01082572a16 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 17 May 2023 14:46:49 +0200 Subject: [PATCH 11/17] monitor: add more *_locked() functions 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 Signed-off-by: Paolo Bonzini --- 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 033390f699..965f5d5450 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 7080d2da8e..20e33e28d2 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; From 6ee7c82d0df9bb6e972a8ea689b935df3ba37486 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 3 Mar 2023 13:32:13 +0100 Subject: [PATCH 12/17] monitor: do not use mb_read/mb_set for suspend_cnt 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 --- 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 5cab56d355..69c1b7e98a 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 53e3808054..61c9b6916d 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 20e33e28d2..15f97538ef 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 e5ddb03bda..d462156405 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 bade1ff3b9..4d4af5a486 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 From 3e6bed619a1d13858e540e01aae275abdf9146ae Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 3 Mar 2023 12:45:29 +0100 Subject: [PATCH 13/17] monitor: cleanup detection of qmp_dispatcher_co shutting down 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 --- 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 15f97538ef..c4ed2547c2 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 092c527b6f..f0cc6dc886 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; } } From 0ff25537018c0939919a35886265c38db28b2a8a Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 3 Mar 2023 12:51:33 +0100 Subject: [PATCH 14/17] monitor: cleanup fetching of QMP requests 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 --- 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 c4ed2547c2..042a1ab918 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 f0cc6dc886..dfc2156328 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) From 9f2d58546e8cc83b1b033c4f89dcb188e7b05c0c Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 3 Mar 2023 13:51:44 +0100 Subject: [PATCH 15/17] monitor: introduce qmp_dispatcher_co_wake This makes it possible to turn qmp_dispatcher_co_busy into a static variable. Signed-off-by: Paolo Bonzini --- 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 61c9b6916d..252de85681 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 042a1ab918..dc352f9e9d 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 dfc2156328..613b74ec74 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) From 60f4f62efeb174fe7433ce9ebc37836e70ec9b75 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 3 Mar 2023 13:51:58 +0100 Subject: [PATCH 16/17] monitor: extract request dequeuing to a new function Signed-off-by: Paolo Bonzini --- monitor/qmp.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/monitor/qmp.c b/monitor/qmp.c index 613b74ec74..e6b1043c9f 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); From eea7cd3fc5139d7523f3c7a67d9c864b944dfacd Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 15 Mar 2023 12:34:01 +0100 Subject: [PATCH 17/17] monitor: do not use mb_read/mb_set 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 --- monitor/qmp.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/monitor/qmp.c b/monitor/qmp.c index e6b1043c9f..c8e0156974 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); }