From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52227) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1e2wqw-0005gu-Gm for qemu-devel@nongnu.org; Fri, 13 Oct 2017 06:06:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1e2wqr-0004iI-KK for qemu-devel@nongnu.org; Fri, 13 Oct 2017 06:06:02 -0400 Received: from 7.mo5.mail-out.ovh.net ([178.32.124.100]:35090) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1e2wqr-0004gD-EG for qemu-devel@nongnu.org; Fri, 13 Oct 2017 06:05:57 -0400 Received: from player773.ha.ovh.net (b9.ovh.net [213.186.33.59]) by mo5.mail-out.ovh.net (Postfix) with ESMTP id C959713F9FE for ; Fri, 13 Oct 2017 12:05:55 +0200 (CEST) Date: Fri, 13 Oct 2017 12:05:46 +0200 From: Greg Kurz Message-ID: <20171013120546.7a89eeaf@bahia.lan> In-Reply-To: <20171013112459.0082e823@nial.brq.redhat.com> References: <150788370618.25736.8030708425923435364.stgit@bahia> <150788373123.25736.7359515819699182906.stgit@bahia> <20171013112459.0082e823@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] monitor: add proper reference counting of the current CPU List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, Cornelia Huck , "Dr. David Alan Gilbert" , Markus Armbruster , qemu-ppc@nongnu.org, David Gibson On Fri, 13 Oct 2017 11:24:59 +0200 Igor Mammedov wrote: > On Fri, 13 Oct 2017 10:35:31 +0200 > Greg Kurz wrote: > > > If a CPU selected with the "cpu" command is hot-unplugged then "info cpus" > > causes QEMU to exit: > > > > (qemu) device_del cpu1 > > (qemu) info cpus > > qemu:qemu_cpu_kick_thread: No such process > > > > This happens because "cpu" stores the pointer to the selected CPU into > > the monitor structure. When the CPU is hot-unplugged, we end up with a > > dangling pointer. The "info cpus" command then does: > > > > hmp_info_cpus() > > monitor_get_cpu_index() > > mon_get_cpu() > > cpu_synchronize_state() <--- called with dangling pointer > > > > This could cause a QEMU crash as well. > > > > This patch switches the monitor to use object_ref() to ensure the > > CPU object doesn't vanish unexpectedly. The reference is dropped > > either when "cpu" is used to switch to another CPU, or when the > > selected CPU is unrealized and cpu_list_remove() sets its cpu_index > > back to UNASSIGNED_CPU_INDEX. > I don't really like an idea of leaving dangling cpu around. > Is it possible to store QOM path on set_cpu in monitor and > resolving it each to instance each time it's needed? > It sounds workable. Also it would allow the fix to not depend on patch 1 for ppc. Thanks for the suggestion! > > > Signed-off-by: Greg Kurz > > --- > > monitor.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/monitor.c b/monitor.c > > index fe0d1bdbb461..1c0b9a2c3ad3 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -579,6 +579,9 @@ static void monitor_data_init(Monitor *mon) > > > > static void monitor_data_destroy(Monitor *mon) > > { > > + if (mon->mon_cpu) { > > + object_unref((Object *) mon->mon_cpu); > > + } > > qemu_chr_fe_deinit(&mon->chr, false); > > if (monitor_is_qmp(mon)) { > > json_message_parser_destroy(&mon->qmp.parser); > > @@ -1047,12 +1050,21 @@ int monitor_set_cpu(int cpu_index) > > if (cpu == NULL) { > > return -1; > > } > > + if (cur_mon->mon_cpu) { > > + object_unref((Object *) cur_mon->mon_cpu); > > + } > > cur_mon->mon_cpu = cpu; > > + object_ref((Object *) cpu); > > return 0; > > } > > > > CPUState *mon_get_cpu(void) > > { > > + if (cur_mon->mon_cpu && > > + cur_mon->mon_cpu->cpu_index == UNASSIGNED_CPU_INDEX) { > > + object_unref((Object *) cur_mon->mon_cpu); > > + cur_mon->mon_cpu = NULL; > > + } > > if (!cur_mon->mon_cpu) { > > if (!first_cpu) { > > return NULL; > > > > >