Patchwork [1/2] pvpanic: initialization cleanup

login
register
mail settings
Submitter Michael S. Tsirkin
Date June 16, 2013, 8:59 p.m.
Message ID <1371416189-29563-1-git-send-email-mst@redhat.com>
Download mbox | patch
Permalink /patch/251741/
State New
Headers show

Comments

Michael S. Tsirkin - June 16, 2013, 8:59 p.m.
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(-)
Laszlo Ersek - June 17, 2013, 7:56 a.m.
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.)

> +
> +    fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
> +                    g_memdup(&s->ioport, sizeof(s->ioport)),
> +                    sizeof(s->ioport));
> +
>      return 0;
>  }
>  
> 

Series looks good to me otherwise.

Thanks
Laszlo
Michael S. Tsirkin - June 17, 2013, 9:19 a.m.
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.

> > +
> > +    fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
> > +                    g_memdup(&s->ioport, sizeof(s->ioport)),
> > +                    sizeof(s->ioport));
> > +
> >      return 0;
> >  }
> >  
> > 
> 
> Series looks good to me otherwise.
> 
> Thanks
> Laszlo
Laszlo Ersek - June 17, 2013, 9:35 a.m.
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
Michael S. Tsirkin - June 17, 2013, 9:57 a.m.
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.
Laszlo Ersek - June 17, 2013, 10:07 a.m.
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.

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>
Stefano Stabellini - June 19, 2013, 1:29 p.m.
On Mon, 17 Jun 2013, 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.)

Thanks for spotting this!


> > 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.

Give a look at hw/i386/pc_piix.c: the xenfv machine has pc_xen_hvm_init
as init function; pc_xen_hvm_init calls xen_hvm_init and pc_init_pci.
Unfortunately the comment on top of pc_init_pci_no_kvmclock is not up to
date as of:

commit 9cdf79d068f52f7de347cb45cfd8903519410e4d
Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Date:   Mon May 27 16:46:01 2013 +0000

    xen: use pc_init_pci instead of pc_init_pci_no_kvmclock
Stefano Stabellini - June 19, 2013, 1:30 p.m.
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 <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.

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.
Stefano Stabellini - June 19, 2013, 1:39 p.m.
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.
Laszlo Ersek - June 19, 2013, 1:58 p.m.
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

Patch

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);
+
+    fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
+                    g_memdup(&s->ioport, sizeof(s->ioport)),
+                    sizeof(s->ioport));
+
     return 0;
 }