[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] pvpanic: initialization cleanup
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] pvpanic: initialization cleanup |
Date: |
Mon, 17 Jun 2013 12:07:48 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130513 Thunderbird/17.0.6 |
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.
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>