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

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

[Michael S. Tsirkin]

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

[Qemu-devel] [PATCH 2/2] pvpanic: fix fwcfg for big endian hosts

[Michael S. Tsirkin]

Convert port number to little endian when
exposing it in fw cfg.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/misc/pvpanic.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 9ed9897..151eefe 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -106,12 +106,13 @@ int pvpanic_init(ISABus *bus)
     ISADevice *dev = isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE);
     PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
     FWCfgState *fw_cfg = fw_cfg_find();
+    uint16_t *pvpanic_port = g_malloc(sizeof(*pvpanic_port));
+    *pvpanic_port = cpu_to_le16(s->ioport);
 
     assert(fw_cfg);
 
-    fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
-                    g_memdup(&s->ioport, sizeof(s->ioport)),
-                    sizeof(s->ioport));
+    fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", pvpanic_port,
+                    sizeof(*pvpanic_port));
 
     return 0;
 }
-- 
MST

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

[Laszlo Ersek]

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

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

[Michael S. Tsirkin]

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

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

[Laszlo Ersek]

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

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

[Michael S. Tsirkin]

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.

-- 
MST

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

[Laszlo Ersek]

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>

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

[Stefano Stabellini]

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.

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

[Laszlo Ersek]

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

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

[Stefano Stabellini]

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.

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

[Stefano Stabellini]

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