On Mon, 17 Jun 2013, 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 <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.
> >
> > 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.
Yeah, that's pretty bad. Even though at the moment the Xen tools would
always create QEMU with -M xenfv, I would like to be able to use other
QEMUMachines too in the future.
