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

[Michael S. Tsirkin]

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

-- 
MST