From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39673) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UpIrw-0005NJ-Cs for qemu-devel@nongnu.org; Wed, 19 Jun 2013 09:56:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UpIrt-0004H8-DZ for qemu-devel@nongnu.org; Wed, 19 Jun 2013 09:56:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:2965) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UpIrt-0004Gw-6P for qemu-devel@nongnu.org; Wed, 19 Jun 2013 09:56:13 -0400 Message-ID: <51C1B8FE.70400@redhat.com> Date: Wed, 19 Jun 2013 15:58:22 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1371416189-29563-1-git-send-email-mst@redhat.com> <51BEC148.50009@redhat.com> <20130617091924.GB7173@redhat.com> <51BED844.5000802@redhat.com> <20130617095737.GA7613@redhat.com> <51BEDFF4.1010904@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] pvpanic: initialization cleanup List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: Hu Tao , Anthony Liguori , qemu-devel@nongnu.org, Markus Armbruster , "Michael S. Tsirkin" On 06/19/13 15:39, Stefano Stabellini wrote: > On Mon, 17 Jun 2013, Laszlo Ersek wrote: >> On 06/17/13 11:57, Michael S. Tsirkin wrote: >>> 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. >> >> Yes, I think that's exactly what happens *if* at domain creation time >> the Xen userspace utilities start qemu with such a machine model that >> sets "has_pvpanic" to false. I'd only like to have confirmation that the >> leading comment on pc_init_pci_no_kvmclock() is up-to-date and we can >> trust this code never to run on Xen. > > xenfv now uses pc_xen_hvm_init, that calls directly pc_init_pci, so > has_pvpanic would be true. However we could easily change that if it is > necessary. Even if we fix xenfv, I would like to retain the possibility > to start QEMU on Xen with other QEMUMachine though. > > >> Actually, we can figure out later, if/when it breaks under Xen. It >> shouldn't be hard to fix. >> >> series >> Reviewed-by: Laszlo Ersek > > I really appreciate that you involved me in this discussion before > committing the patches and I wouldn't want to be the cause of a delay > in QEMU development. However in general I think it's reasonable to wait > a couple of days for an answer when a clear possibility for breakage > exists. It's become clear to me that one can't review stuff without irritating at least one party. I chose to irritate xen developers / users rather than annoying Michael by stalling his patch. Sorry. There's no good solution for the messenger here. BTW I also asked Paul Durrant about this in the meantime [1]. He confirmed my worries and Michael posted an updated version [2]. [1] http://thread.gmane.org/gmane.comp.emulators.qemu/217364/focus=217393 [2] http://thread.gmane.org/gmane.comp.emulators.qemu/217408 Laszlo