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.
> > +
> > + 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
