Re: [Qemu-devel] [PATCH 1/2] pvpanic: initialization cleanup

[Michael S. Tsirkin]

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 <address@hidden>
> > ---
> >  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