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 <m...@redhat.com> >>> --- >>> 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