From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59387) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoWBO-0007G3-Iz for qemu-devel@nongnu.org; Mon, 17 Jun 2013 05:57:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UoWBF-00037Y-0w for qemu-devel@nongnu.org; Mon, 17 Jun 2013 05:57:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57199) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoWBE-00037N-Pa for qemu-devel@nongnu.org; Mon, 17 Jun 2013 05:56:56 -0400 Date: Mon, 17 Jun 2013 12:57:37 +0300 From: "Michael S. Tsirkin" Message-ID: <20130617095737.GA7613@redhat.com> References: <1371416189-29563-1-git-send-email-mst@redhat.com> <51BEC148.50009@redhat.com> <20130617091924.GB7173@redhat.com> <51BED844.5000802@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51BED844.5000802@redhat.com> Subject: Re: [Qemu-devel] [PATCH 1/2] pvpanic: initialization cleanup List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: Hu Tao , Anthony Liguori , stefano.stabellini@eu.citrix.com, qemu-devel@nongnu.org, Markus Armbruster On Mon, Jun 17, 2013 at 11:35:00AM +0200, Laszlo Ersek wrote: > On 06/17/13 11:19, Michael S. Tsirkin wrote: > > On Mon, Jun 17, 2013 at 09:56:56AM +0200, Laszlo Ersek wrote: > >> On 06/16/13 22:59, Michael S. Tsirkin wrote: > >>> Avoid use of static variables: PC systems initialize pvpanic device > >>> through pvpanic_init, so we can simply create the fw_cfg file at that > >>> point. Others don't use fw_cfg at all. This also makes it possible to > >>> assert if fw_cfg is not there rather than skipping the device silently. > >>> > >>> Signed-off-by: Michael S. Tsirkin > >>> --- > >>> hw/misc/pvpanic.c | 23 ++++++++++------------- > >>> 1 file changed, 10 insertions(+), 13 deletions(-) > >>> > >>> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c > >>> index 060099b..9ed9897 100644 > >>> --- a/hw/misc/pvpanic.c > >>> +++ b/hw/misc/pvpanic.c > >>> @@ -97,25 +97,22 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp) > >>> { > >>> ISADevice *d = ISA_DEVICE(dev); > >>> PVPanicState *s = ISA_PVPANIC_DEVICE(dev); > >>> - static bool port_configured; > >>> - FWCfgState *fw_cfg; > >>> > >>> isa_register_ioport(d, &s->io, s->ioport); > >>> - > >>> - if (!port_configured) { > >>> - fw_cfg = fw_cfg_find(); > >>> - if (fw_cfg) { > >>> - fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", > >>> - g_memdup(&s->ioport, sizeof(s->ioport)), > >>> - sizeof(s->ioport)); > >>> - port_configured = true; > >>> - } > >>> - } > >>> } > >>> > >>> int pvpanic_init(ISABus *bus) > >>> { > >>> - isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); > >>> + ISADevice *dev = isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); > >>> + PVPanicState *s = ISA_PVPANIC_DEVICE(dev); > >>> + FWCfgState *fw_cfg = fw_cfg_find(); > >>> + > >>> + assert(fw_cfg); > >> > >> Won't the assert fire if: > >> > >> xen_enabled() && > >> machine != "pc-0.10" && machine != "pc-0.11" && > >> machine != "pc-0.12" && machine != "pc-0.13" && > >> machine != "pc-q35-1.4" > >> > >> Because under the above condition "has_pvpanic" remains "true", but > >> fw_cfg is not initialized. > >> > >> (pc_init_pci_no_kvmclock() in "hw/i386/pc_piix.c" sets "has_pvpanic" to > >> "false", and claims to be "reused by xenfv", so the above condition may > >> be constant false.) > > > > That's what I think - if user wants pvpanic to work, fw cfg is required ATM. > > What I have in mind is the following: suppose xen is enabled and qemu is > started with -M pc-i440fx-1.5. > > Before the patch, the pvpanic device didn't work, but qemu didn't crash > either. After the patch, the assert() is triggered at startup. > > Of course, if starting qemu for xen with "-M pc-i440fx-1.5" is *already* > broken (for other, maybe more serious, reasons), ie. PEBKAC, then the > patch is correct. But I can't evaluate that condition to constant false, > and suppose that it's a possible configuration, under which qemu would > now start with an assertion failure. > > Can someone with Xen knowledge chime in? CC'ing Stefano. > > Laszlo A sane alternative is to avoid creating the pvpanic device. Not as easy to debug as an assert, but at least guest does not get reserved ports which said guest has no way to discover. -- MST