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

[Stefano Stabellini]

On Mon, 17 Jun 2013, Laszlo Ersek wrote:
> On 06/17/13 11:57, Michael S. Tsirkin wrote:
> > 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.
> 
> Yes, I think that's exactly what happens *if* at domain creation time
> the Xen userspace utilities start qemu with such a machine model that
> sets "has_pvpanic" to false. I'd only like to have confirmation that the
> leading comment on pc_init_pci_no_kvmclock() is up-to-date and we can
> trust this code never to run on Xen.

xenfv now uses pc_xen_hvm_init, that calls directly pc_init_pci, so
has_pvpanic would be true. However we could easily change that if it is
necessary. Even if we fix xenfv, I would like to retain the possibility
to start QEMU on Xen with other QEMUMachine though.


> Actually, we can figure out later, if/when it breaks under Xen. It
> shouldn't be hard to fix.
> 
> series
> Reviewed-by: Laszlo Ersek <address@hidden>

I really appreciate that you involved me in this discussion before
committing the patches and I wouldn't want to be the cause of a delay
in QEMU development. However in general I think it's reasonable to wait
a couple of days for an answer when a clear possibility for breakage
exists.