From patchwork Thu Nov 1 10:22:10 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alon Levy X-Patchwork-Id: 196142 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 54AE62C0339 for ; Thu, 1 Nov 2012 21:22:36 +1100 (EST) Received: from localhost ([::1]:50790 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTruz-0001IA-N2 for incoming@patchwork.ozlabs.org; Thu, 01 Nov 2012 06:22:33 -0400 Received: from eggs.gnu.org ([208.118.235.92]:50656) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTruk-0001Ah-G8 for qemu-devel@nongnu.org; Thu, 01 Nov 2012 06:22:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TTrue-0002jJ-CR for qemu-devel@nongnu.org; Thu, 01 Nov 2012 06:22:18 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:40119) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTrue-0002jC-3q for qemu-devel@nongnu.org; Thu, 01 Nov 2012 06:22:12 -0400 Received: from zmail15.collab.prod.int.phx2.redhat.com (zmail15.collab.prod.int.phx2.redhat.com [10.5.83.17]) by mx4-phx2.redhat.com (8.13.8/8.13.8) with ESMTP id qA1AMATs022901; Thu, 1 Nov 2012 06:22:10 -0400 Date: Thu, 1 Nov 2012 06:22:10 -0400 (EDT) From: Alon Levy To: Gerd Hoffmann Message-ID: <833806772.25295125.1351765330902.JavaMail.root@redhat.com> In-Reply-To: <50924729.3090101@redhat.com> MIME-Version: 1.0 X-Originating-IP: [10.35.4.71] X-Mailer: Zimbra 7.2.0_GA_2669 (ZimbraWebClient - FF3.0 (Linux)/7.2.0_GA_2669) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6.x X-Received-From: 209.132.183.25 Cc: qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [RFC] hw/qxl: inject interrupts in any state X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org > Hi, > > >> IMO spice-server must not call interface_client_set_capabilities > >> when the vm is not running. After all we notify spice-server > >> about > >> the vm stop/start events for a reason ... > > > > OK, I agree that should be fixed, we can queue this until the vm > > starts running in spice-server. But having an assert on notify in > > qemu is also wrong - and the only way to fix it like you pointed > > out > > without dropping the event is to queue it as well. > > > > So which will it be, queue in spice or in qemu? qemu seems a > > simpler > > place to catch everything. > > When queuing in qemu you are facing the migration issue again in a > different way: Just this time it isn't guest state, but a qxl > register. > Not guest visible, but still state which must be migrated over ... OK, good point. So the assert still bothers me - it should be logged but not asserted. I'm talking about interface_set_client_capabilities and anywhere else that qxl_send_events can be called. i.e.: commit 6614a4db6b88887dd29bfd5365f38ba0fcc264ed Author: Alon Levy Date: Tue Oct 30 18:00:33 2012 +0200 hw/qxl: warn on qxl_send_events attempt if stopped This prevents a known abort on set_client_capabilities, that should be fixed in upstream, but it should also be checked against in qxl. Checks every other location that qxl_send_events is eventually possibly called (everywhere is direct except for interface_release_resource which calls qxl_push_free_res which may call qxl_send_event). RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=870972 Signed-off-by: Alon Levy > > cheers, > Gerd > > > > diff --git a/hw/qxl.c b/hw/qxl.c index 7b88a1e..946f5a2 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -136,6 +136,24 @@ static void qxl_reset_memslots(PCIQXLDevice *d); static void qxl_reset_surfaces(PCIQXLDevice *d); static void qxl_ring_set_dirty(PCIQXLDevice *qxl); +static void spice_server_bug(PCIQXLDevice *qxl, const char *msg, ...) +{ + va_list ap; + va_start(ap, msg); + fprintf(stderr, "qxl-%d: spice-server bug: ", qxl->id); + vfprintf(stderr, msg, ap); + fprintf(stderr, "\n"); + va_end(ap); +} + +#define SPICE_SERVER_BUG_ONCE(qxl, msg, ...) { \ + static int called; \ + if (!called) { \ + called = 1; \ + spice_server_bug(qxl, msg, __VA_ARGS__); \ + } \ +} + void qxl_set_guest_bug(PCIQXLDevice *qxl, const char *msg, ...) { trace_qxl_set_guest_bug(qxl->id); @@ -600,6 +618,10 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext) int notify, ret; trace_qxl_ring_command_check(qxl->id, qxl_mode_to_string(qxl->mode)); + if (!qemu_spice_display_is_running(&qxl->ssd)) { + SPICE_SERVER_BUG_ONCE(qxl, "%s: guest stopped", __func__); + return false; + } switch (qxl->mode) { case QXL_MODE_VGA: @@ -722,6 +744,11 @@ static void interface_release_resource(QXLInstance *sin, return; } + if (!qemu_spice_display_is_running(&qxl->ssd)) { + SPICE_SERVER_BUG_ONCE(qxl, "%s: guest stopped", __func__); + return; + } + /* * ext->info points into guest-visible memory * pci bar 0, $command.release_info @@ -761,6 +788,10 @@ static int interface_get_cursor_command(QXLInstance *sin, struct QXLCommandExt * trace_qxl_ring_cursor_check(qxl->id, qxl_mode_to_string(qxl->mode)); + if (!qemu_spice_display_is_running(&qxl->ssd)) { + SPICE_SERVER_BUG_ONCE(qxl, "%s: guest stopped", __func__); + return false; + } switch (qxl->mode) { case QXL_MODE_COMPAT: case QXL_MODE_NATIVE: @@ -862,6 +893,11 @@ static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie) "qxl: %s: error: current_async = %d != %" PRId64 " = cookie->io\n", __func__, current_async, cookie->io); } + if (!qemu_spice_display_is_running(&qxl->ssd)) { + SPICE_SERVER_BUG_ONCE(qxl, "%s: guest stopped", __func__); + return; + } + switch (current_async) { case QXL_IO_MEMSLOT_ADD_ASYNC: case QXL_IO_DESTROY_PRIMARY_ASYNC: @@ -963,6 +999,10 @@ static void interface_set_client_capabilities(QXLInstance *sin, runstate_check(RUN_STATE_POSTMIGRATE)) { return; } + if (!qemu_spice_display_is_running(&qxl->ssd)) { + SPICE_SERVER_BUG_ONCE(qxl, "%s: guest stopped", __func__); + return; + } qxl->shadow_rom.client_present = client_present; memcpy(qxl->shadow_rom.client_capabilities, caps, sizeof(caps));