From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46330) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoVaX-0004XI-N3 for qemu-devel@nongnu.org; Mon, 17 Jun 2013 05:19:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UoVaT-00054a-Hp for qemu-devel@nongnu.org; Mon, 17 Jun 2013 05:19:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48743) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoVaT-00054Q-9W for qemu-devel@nongnu.org; Mon, 17 Jun 2013 05:18:57 -0400 Date: Mon, 17 Jun 2013 12:19:24 +0300 From: "Michael S. Tsirkin" Message-ID: <20130617091924.GB7173@redhat.com> References: <1371416189-29563-1-git-send-email-mst@redhat.com> <51BEC148.50009@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51BEC148.50009@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 , qemu-devel@nongnu.org, Markus Armbruster 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. > > + > > + fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", > > + g_memdup(&s->ioport, sizeof(s->ioport)), > > + sizeof(s->ioport)); > > + > > return 0; > > } > > > > > > Series looks good to me otherwise. > > Thanks > Laszlo