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

[Laszlo Ersek <lersek@redhat.com>]

On 06/19/13 15:39, Stefano Stabellini wrote:
> 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 <mst@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 <lersek@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.

It's become clear to me that one can't review stuff without irritating
at least one party. I chose to irritate xen developers / users rather
than annoying Michael by stalling his patch. Sorry. There's no good
solution for the messenger here.

BTW I also asked Paul Durrant about this in the meantime [1]. He
confirmed my worries and Michael posted an updated version [2].

[1] http://thread.gmane.org/gmane.comp.emulators.qemu/217364/focus=217393
[2] http://thread.gmane.org/gmane.comp.emulators.qemu/217408

Laszlo