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

[Stefano Stabellini]

On Mon, 17 Jun 2013, 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 <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.)

Thanks for spotting this!


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

Give a look at hw/i386/pc_piix.c: the xenfv machine has pc_xen_hvm_init
as init function; pc_xen_hvm_init calls xen_hvm_init and pc_init_pci.
Unfortunately the comment on top of pc_init_pci_no_kvmclock is not up to
date as of:

commit 9cdf79d068f52f7de347cb45cfd8903519410e4d
Author: Stefano Stabellini <address@hidden>
Date:   Mon May 27 16:46:01 2013 +0000

    xen: use pc_init_pci instead of pc_init_pci_no_kvmclock