From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33000) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoWJf-0000r2-7M for qemu-devel@nongnu.org; Mon, 17 Jun 2013 06:05:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UoWJY-0005z6-D5 for qemu-devel@nongnu.org; Mon, 17 Jun 2013 06:05:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:15068) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoWJX-0005yf-RE for qemu-devel@nongnu.org; Mon, 17 Jun 2013 06:05:32 -0400 Message-ID: <51BEDFF4.1010904@redhat.com> Date: Mon, 17 Jun 2013 12:07:48 +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> In-Reply-To: <20130617095737.GA7613@redhat.com> 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: "Michael S. Tsirkin" Cc: Hu Tao , Anthony Liguori , stefano.stabellini@eu.citrix.com, qemu-devel@nongnu.org, Markus Armbruster 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. Actually, we can figure out later, if/when it breaks under Xen. It shouldn't be hard to fix. series Reviewed-by: Laszlo Ersek