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 <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.
>
> 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 <ler...@redhat.com>
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.
