[Qemu-devel] pvpanic device should not be automatically included as an internal device

[Qemu-devel] pvpanic device should not be automatically included as an internal device

[Marcel Apfelbaum]

Hi,

The problem with pvpanic being an internal device is that VMs running
operating systems without a driver for this device will have problems
when qemu will be upgraded (from qemu without this pvpanic).

The outcome may be, for example: in Windows(let's say XP) the Device manager
will open a "new device" wizard and the device will appear as an unrecognized device.
Now what will happen on a cluster with hundreds of such VMs? If that cluster has a health
monitoring service it may show all the VMs in a "not healthy" state.

My point is that a device that requires a driver that is not "inbox", should not
be present by default.
One possible solution is to add it manually with -device from command line.

Any thoughts?
Marcel

Re: [Qemu-devel] pvpanic device should not be automatically included as an internal device

[Michael S. Tsirkin]

On Thu, Aug 01, 2013 at 04:08:57PM +0300, Marcel Apfelbaum wrote:
> Hi,
> 
> The problem with pvpanic being an internal device is that VMs running
> operating systems without a driver for this device will have problems
> when qemu will be upgraded (from qemu without this pvpanic).
> 
> The outcome may be, for example: in Windows(let's say XP) the Device manager
> will open a "new device" wizard and the device will appear as an unrecognized device.
> Now what will happen on a cluster with hundreds of such VMs? If that cluster has a health
> monitoring service it may show all the VMs in a "not healthy" state.
> 
> My point is that a device that requires a driver that is not "inbox", should not
> be present by default.
> One possible solution is to add it manually with -device from command line.
> 
> Any thoughts?
> Marcel

Interesting. You are basically saying we should have a rule
that no new builtin devices should be added
without an explicit request from management interface?


-- 
MST

Re: [Qemu-devel] pvpanic device should not be automatically included as an internal device

[Marcel Apfelbaum]

On Thu, 2013-08-01 at 16:32 +0300, Michael S. Tsirkin wrote:
> On Thu, Aug 01, 2013 at 04:08:57PM +0300, Marcel Apfelbaum wrote:
> > Hi,
> > 
> > The problem with pvpanic being an internal device is that VMs running
> > operating systems without a driver for this device will have problems
> > when qemu will be upgraded (from qemu without this pvpanic).
> > 
> > The outcome may be, for example: in Windows(let's say XP) the Device manager
> > will open a "new device" wizard and the device will appear as an unrecognized device.
> > Now what will happen on a cluster with hundreds of such VMs? If that cluster has a health
> > monitoring service it may show all the VMs in a "not healthy" state.
> > 
> > My point is that a device that requires a driver that is not "inbox", should not
> > be present by default.
> > One possible solution is to add it manually with -device from command line.
> > 
> > Any thoughts?
> > Marcel
> 
> Interesting. You are basically saying we should have a rule
> that no new builtin devices should be added
> without an explicit request from management interface?

Basically, yes.
The only builtin devices shall be devices that the operating systems
know how to handle with the default drivers.
Marcel

> 
> 

Re: [Qemu-devel] pvpanic device should not be automatically included as an internal device

[Gerd Hoffmann]

On 08/01/13 15:08, Marcel Apfelbaum wrote:
> Hi,
> 
> The problem with pvpanic being an internal device is that VMs running
> operating systems without a driver for this device will have problems
> when qemu will be upgraded (from qemu without this pvpanic).
> 
> The outcome may be, for example: in Windows(let's say XP) the Device manager
> will open a "new device" wizard and the device will appear as an unrecognized device.

Only happens when also changing the machine type on upgrade as it is
turned off on old machine types.

But, yes, pvpanic will show up as "Unknown device" without driver and
with the funky yellow exclamation mark in device manager in windows
guests.  Newer windows versions don't kick the "new device" wizard.  But
still I have my doubts that it is a good idea to add it unconditionally ...

cheers,
  Gerd

Re: [Qemu-devel] pvpanic device should not be automatically included as an internal device

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]

On 08/01/2013 08:18 AM, Gerd Hoffmann wrote:
> On 08/01/13 15:08, Marcel Apfelbaum wrote:
>> Hi,
>>
>> The problem with pvpanic being an internal device is that VMs running
>> operating systems without a driver for this device will have problems
>> when qemu will be upgraded (from qemu without this pvpanic).
>>
>> The outcome may be, for example: in Windows(let's say XP) the Device manager
>> will open a "new device" wizard and the device will appear as an unrecognized device.
> 
> Only happens when also changing the machine type on upgrade as it is
> turned off on old machine types.
> 
> But, yes, pvpanic will show up as "Unknown device" without driver and
> with the funky yellow exclamation mark in device manager in windows
> guests.  Newer windows versions don't kick the "new device" wizard.  But
> still I have my doubts that it is a good idea to add it unconditionally ...

Automatic devices with no command line argument have proven to be a
nightmare for libvirt as well.  Although the just-released libvirt 1.1.1
now supports the <on_crash> element for controlling the command line
parameters of qemu related to how qemu will behave when the pvpanic
device is triggered, I would also welcome having the ability to control
whether the guest even has a pvpanic device exposed, just as we can
control whether a guest has a memballoon device exposed.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] pvpanic device should not be automatically included as an internal device

[Michael S. Tsirkin]

On Thu, Aug 01, 2013 at 10:26:53AM -0600, Eric Blake wrote:
> On 08/01/2013 08:18 AM, Gerd Hoffmann wrote:
> > On 08/01/13 15:08, Marcel Apfelbaum wrote:
> >> Hi,
> >>
> >> The problem with pvpanic being an internal device is that VMs running
> >> operating systems without a driver for this device will have problems
> >> when qemu will be upgraded (from qemu without this pvpanic).
> >>
> >> The outcome may be, for example: in Windows(let's say XP) the Device manager
> >> will open a "new device" wizard and the device will appear as an unrecognized device.
> > 
> > Only happens when also changing the machine type on upgrade as it is
> > turned off on old machine types.
> > 
> > But, yes, pvpanic will show up as "Unknown device" without driver and
> > with the funky yellow exclamation mark in device manager in windows
> > guests.  Newer windows versions don't kick the "new device" wizard.  But
> > still I have my doubts that it is a good idea to add it unconditionally ...
> 
> Automatic devices with no command line argument have proven to be a
> nightmare for libvirt as well.  Although the just-released libvirt 1.1.1
> now supports the <on_crash> element for controlling the command line
> parameters of qemu related to how qemu will behave when the pvpanic
> device is triggered, I would also welcome having the ability to control
> whether the guest even has a pvpanic device exposed, just as we can
> control whether a guest has a memballoon device exposed.


A natural way to do this would be with -device pvpanic.
I'm not sure why it wasn't done like this from the beginning,
but it shouldn't be hard to redo, hopefully we can fix this
bug in time for 1.6.

-- 
MST

Re: [Qemu-devel] pvpanic device should not be automatically included as an internal device

[Marcel Apfelbaum]

On Thu, 2013-08-01 at 19:31 +0300, Michael S. Tsirkin wrote:
> On Thu, Aug 01, 2013 at 10:26:53AM -0600, Eric Blake wrote:
> > On 08/01/2013 08:18 AM, Gerd Hoffmann wrote:
> > > On 08/01/13 15:08, Marcel Apfelbaum wrote:
> > >> Hi,
> > >>
> > >> The problem with pvpanic being an internal device is that VMs running
> > >> operating systems without a driver for this device will have problems
> > >> when qemu will be upgraded (from qemu without this pvpanic).
> > >>
> > >> The outcome may be, for example: in Windows(let's say XP) the Device manager
> > >> will open a "new device" wizard and the device will appear as an unrecognized device.
> > > 
> > > Only happens when also changing the machine type on upgrade as it is
> > > turned off on old machine types.
> > > 
> > > But, yes, pvpanic will show up as "Unknown device" without driver and
> > > with the funky yellow exclamation mark in device manager in windows
> > > guests.  Newer windows versions don't kick the "new device" wizard.  But
> > > still I have my doubts that it is a good idea to add it unconditionally ...
> > 
> > Automatic devices with no command line argument have proven to be a
> > nightmare for libvirt as well.  Although the just-released libvirt 1.1.1
> > now supports the <on_crash> element for controlling the command line
> > parameters of qemu related to how qemu will behave when the pvpanic
> > device is triggered, I would also welcome having the ability to control
> > whether the guest even has a pvpanic device exposed, just as we can
> > control whether a guest has a memballoon device exposed.
> 
> 
> A natural way to do this would be with -device pvpanic.
> I'm not sure why it wasn't done like this from the beginning,
> but it shouldn't be hard to redo, hopefully we can fix this
> bug in time for 1.6.
> 
I'll come up with something, hopefully in time.
Marcel

Re: [Qemu-devel] pvpanic device should not be automatically included as an internal device

[Paolo Bonzini]

On 08/01/2013 06:26 PM, Eric Blake wrote:
> On 08/01/2013 08:18 AM, Gerd Hoffmann wrote:
>> On 08/01/13 15:08, Marcel Apfelbaum wrote:
>>> Hi,
>>>
>>> The problem with pvpanic being an internal device is that VMs running
>>> operating systems without a driver for this device will have problems
>>> when qemu will be upgraded (from qemu without this pvpanic).
>>>
>>> The outcome may be, for example: in Windows(let's say XP) the Device manager
>>> will open a "new device" wizard and the device will appear as an unrecognized device.
>>
>> Only happens when also changing the machine type on upgrade as it is
>> turned off on old machine types.
>>
>> But, yes, pvpanic will show up as "Unknown device" without driver and
>> with the funky yellow exclamation mark in device manager in windows
>> guests.  Newer windows versions don't kick the "new device" wizard.  But
>> still I have my doubts that it is a good idea to add it unconditionally ...
>
> Automatic devices with no command line argument have proven to be a
> nightmare for libvirt as well.  Although the just-released libvirt 1.1.1
> now supports the <on_crash> element for controlling the command line
> parameters of qemu related to how qemu will behave when the pvpanic
> device is triggered, I would also welcome having the ability to control
> whether the guest even has a pvpanic device exposed, just as we can
> control whether a guest has a memballoon device exposed.

This is quite different from memballoon.

pvpanic is a single I/O port, it doesn't use up a PCI slot (thus
causing conflicts with other devices at the same address).

Perhaps this issue is simply fixed by making the _STA method
return 0x0B instead of 0x0F (i.e. turning off the "show in user
interface" bit).

Paolo

Re: [Qemu-devel] pvpanic device should not be automatically included as an internal device

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

On 08/01/2013 04:23 PM, Paolo Bonzini wrote:
>> Automatic devices with no command line argument have proven to be a
>> nightmare for libvirt as well.  Although the just-released libvirt 1.1.1
>> now supports the <on_crash> element for controlling the command line
>> parameters of qemu related to how qemu will behave when the pvpanic
>> device is triggered, I would also welcome having the ability to control
>> whether the guest even has a pvpanic device exposed, just as we can
>> control whether a guest has a memballoon device exposed.
> 
> This is quite different from memballoon.
> 
> pvpanic is a single I/O port, it doesn't use up a PCI slot (thus
> causing conflicts with other devices at the same address).
> 
> Perhaps this issue is simply fixed by making the _STA method
> return 0x0B instead of 0x0F (i.e. turning off the "show in user
> interface" bit).

That may "fix" the issue of a windows guest showing the yellow ! mark,
but what if, down the road, someone writes an actual windows driver that
is aware of that port and how to make a windows BSOD write a panic
notification to the port?  How does a user go about installing such a
driver if the device is not exposed in the user interface list of devices?


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] pvpanic device should not be automatically included as an internal device

[Paolo Bonzini]

On 08/02/2013 12:42 AM, Eric Blake wrote:
> On 08/01/2013 04:23 PM, Paolo Bonzini wrote:
>>> Automatic devices with no command line argument have proven to be a
>>> nightmare for libvirt as well.  Although the just-released libvirt 1.1.1
>>> now supports the <on_crash> element for controlling the command line
>>> parameters of qemu related to how qemu will behave when the pvpanic
>>> device is triggered, I would also welcome having the ability to control
>>> whether the guest even has a pvpanic device exposed, just as we can
>>> control whether a guest has a memballoon device exposed.
>>
>> This is quite different from memballoon.
>>
>> pvpanic is a single I/O port, it doesn't use up a PCI slot (thus
>> causing conflicts with other devices at the same address).
>>
>> Perhaps this issue is simply fixed by making the _STA method
>> return 0x0B instead of 0x0F (i.e. turning off the "show in user
>> interface" bit).
>
> That may "fix" the issue of a windows guest showing the yellow ! mark,
> but what if, down the road, someone writes an actual windows driver that
> is aware of that port and how to make a windows BSOD write a panic
> notification to the port?  How does a user go about installing such a
> driver if the device is not exposed in the user interface list of devices?

The user can still manually install a driver even for a device that is 
not exposed.

Having to manually specify the pvpanic device would be yet another knob 
that nobody uses.  Panic notification is a useful feature that should be 
supported with no particular intervention from the user.

Paolo

Re: [Qemu-devel] pvpanic device should not be automatically included as an internal device

[Daniel P. Berrange]

On Fri, Aug 02, 2013 at 10:31:11AM +0200, Paolo Bonzini wrote:
> On 08/02/2013 12:42 AM, Eric Blake wrote:
> >On 08/01/2013 04:23 PM, Paolo Bonzini wrote:
> >>>Automatic devices with no command line argument have proven to be a
> >>>nightmare for libvirt as well.  Although the just-released libvirt 1.1.1
> >>>now supports the <on_crash> element for controlling the command line
> >>>parameters of qemu related to how qemu will behave when the pvpanic
> >>>device is triggered, I would also welcome having the ability to control
> >>>whether the guest even has a pvpanic device exposed, just as we can
> >>>control whether a guest has a memballoon device exposed.
> >>
> >>This is quite different from memballoon.
> >>
> >>pvpanic is a single I/O port, it doesn't use up a PCI slot (thus
> >>causing conflicts with other devices at the same address).
> >>
> >>Perhaps this issue is simply fixed by making the _STA method
> >>return 0x0B instead of 0x0F (i.e. turning off the "show in user
> >>interface" bit).
> >
> >That may "fix" the issue of a windows guest showing the yellow ! mark,
> >but what if, down the road, someone writes an actual windows driver that
> >is aware of that port and how to make a windows BSOD write a panic
> >notification to the port?  How does a user go about installing such a
> >driver if the device is not exposed in the user interface list of devices?
> 
> The user can still manually install a driver even for a device that
> is not exposed.
> 
> Having to manually specify the pvpanic device would be yet another
> knob that nobody uses.  Panic notification is a useful feature that
> should be supported with no particular intervention from the user.

Yep, that was the big motivation behind doing it as an I/O port that we
could have enabled by default, as opposed to a virtio serial device or
some other paravirt device that required explicit configuration.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

[Qemu-devel] [PATCH for-1.6 1/2] don't create pvpanic device by default.

[Hu Tao]

The problem with pvpanic being an internal device is that VMs running
operating systems without a driver for this device will have problems
when qemu will be upgraded (from qemu without this pvpanic).

The outcome may be, for example: in Windows(let's say XP) the Device
manager will open a "new device" wizard and the device will appear as
an unrecognized device. On a cluster with hundreds of such VMs, If
that cluster has a health monitoring service it may show all the VMs
in a "not healthy" state.

Reported-by: Marcel Apfelbaum <marcel.a@redhat.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/i386/pc_piix.c | 8 --------
 hw/i386/pc_q35.c  | 6 ------
 2 files changed, 14 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index ab25458..3ccf96c 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -56,7 +56,6 @@ static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
 static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
 static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
 
-static bool has_pvpanic = true;
 static bool has_pci_info = true;
 
 /* PC hardware initialisation */
@@ -228,10 +227,6 @@ static void pc_init1(MemoryRegion *system_memory,
     if (pci_enabled) {
         pc_pci_device_init(pci_bus);
     }
-
-    if (has_pvpanic) {
-        pvpanic_init(isa_bus);
-    }
 }
 
 static void pc_init_pci(QEMUMachineInitArgs *args)
@@ -257,7 +252,6 @@ static void pc_init_pci_1_5(QEMUMachineInitArgs *args)
 
 static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
 {
-    has_pvpanic = false;
     x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
     pc_init_pci_1_5(args);
 }
@@ -290,7 +284,6 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
     const char *kernel_cmdline = args->kernel_cmdline;
     const char *initrd_filename = args->initrd_filename;
     const char *boot_device = args->boot_device;
-    has_pvpanic = false;
     has_pci_info = false;
     disable_kvm_pv_eoi();
     enable_compat_apic_id_mode();
@@ -309,7 +302,6 @@ static void pc_init_isa(QEMUMachineInitArgs *args)
     const char *kernel_cmdline = args->kernel_cmdline;
     const char *initrd_filename = args->initrd_filename;
     const char *boot_device = args->boot_device;
-    has_pvpanic = false;
     has_pci_info = false;
     if (cpu_model == NULL)
         cpu_model = "486";
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 2f35d12..c816c2f 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -46,7 +46,6 @@
 /* ICH9 AHCI has 6 ports */
 #define MAX_SATA_PORTS     6
 
-static bool has_pvpanic = true;
 static bool has_pci_info = true;
 
 /* PC hardware initialisation */
@@ -211,10 +210,6 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     if (pci_enabled) {
         pc_pci_device_init(host_bus);
     }
-
-    if (has_pvpanic) {
-        pvpanic_init(isa_bus);
-    }
 }
 
 static void pc_q35_init_1_5(QEMUMachineInitArgs *args)
@@ -225,7 +220,6 @@ static void pc_q35_init_1_5(QEMUMachineInitArgs *args)
 
 static void pc_q35_init_1_4(QEMUMachineInitArgs *args)
 {
-    has_pvpanic = false;
     x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE);
     pc_q35_init_1_5(args);
 }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH for-1.6 1/2] don't create pvpanic device by default.

[Paolo Bonzini]

On 08/02/2013 09:04 AM, Hu Tao wrote:
> The problem with pvpanic being an internal device is that VMs running
> operating systems without a driver for this device will have problems
> when qemu will be upgraded (from qemu without this pvpanic).
>
> The outcome may be, for example: in Windows(let's say XP) the Device
> manager will open a "new device" wizard and the device will appear as
> an unrecognized device. On a cluster with hundreds of such VMs, If
> that cluster has a health monitoring service it may show all the VMs
> in a "not healthy" state.
>
> Reported-by: Marcel Apfelbaum <marcel.a@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>

NACK,

this is premature.  It is fundamentally a firmware problem.

We have time to apply an even smaller patch that doesn't set has_pvpanic 
to true, and delay the whole feature to 1.7, if we do not fix the 
firmware in the next two weeks.

Paolo

Re: [Qemu-devel] [PATCH for-1.6 1/2] don't create pvpanic device by default.

[Michael S. Tsirkin]

On Fri, Aug 02, 2013 at 10:27:31AM +0200, Paolo Bonzini wrote:
> On 08/02/2013 09:04 AM, Hu Tao wrote:
> >The problem with pvpanic being an internal device is that VMs running
> >operating systems without a driver for this device will have problems
> >when qemu will be upgraded (from qemu without this pvpanic).
> >
> >The outcome may be, for example: in Windows(let's say XP) the Device
> >manager will open a "new device" wizard and the device will appear as
> >an unrecognized device. On a cluster with hundreds of such VMs, If
> >that cluster has a health monitoring service it may show all the VMs
> >in a "not healthy" state.
> >
> >Reported-by: Marcel Apfelbaum <marcel.a@redhat.com>
> >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> 
> NACK,
> 
> this is premature.  It is fundamentally a firmware problem.
> 
> We have time to apply an even smaller patch that doesn't set
> has_pvpanic to true, and delay the whole feature to 1.7, if we do
> not fix the firmware in the next two weeks.
> 
> Paolo

I think this is not just a firmware problem.  Adding device by default
was too rush, assumption was risk of guest bugs was 0.

We are now seeing problems with bios guest code and with linux guest
drivers as well.  Yes they all can be fixed, but we simply shouldn't
force this risk of broken guests on everyone.

libvirt is the main user and libvirt people
indicated their preference to creating device with
-device pvpanic rather than a built-in one that
can't be removed.

So please reconsider, and here's an ack from me.

Acked-by: Michael S. Tsirkin <mst@redhat.com>

Re: [Qemu-devel] [PATCH for-1.6 1/2] don't create pvpanic device by default.

[Andreas Färber]

Am 11.08.2013 12:33, schrieb Michael S. Tsirkin:
> On Fri, Aug 02, 2013 at 10:27:31AM +0200, Paolo Bonzini wrote:
>> On 08/02/2013 09:04 AM, Hu Tao wrote:
>>> The problem with pvpanic being an internal device is that VMs running
>>> operating systems without a driver for this device will have problems
>>> when qemu will be upgraded (from qemu without this pvpanic).
>>>
>>> The outcome may be, for example: in Windows(let's say XP) the Device
>>> manager will open a "new device" wizard and the device will appear as
>>> an unrecognized device. On a cluster with hundreds of such VMs, If
>>> that cluster has a health monitoring service it may show all the VMs
>>> in a "not healthy" state.
>>>
>>> Reported-by: Marcel Apfelbaum <marcel.a@redhat.com>
>>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>>
>> NACK,
>>
>> this is premature.  It is fundamentally a firmware problem.
>>
>> We have time to apply an even smaller patch that doesn't set
>> has_pvpanic to true, and delay the whole feature to 1.7, if we do
>> not fix the firmware in the next two weeks.
>>
>> Paolo
> 
> I think this is not just a firmware problem.  Adding device by default
> was too rush, assumption was risk of guest bugs was 0.
> 
> We are now seeing problems with bios guest code and with linux guest
> drivers as well.  Yes they all can be fixed, but we simply shouldn't
> force this risk of broken guests on everyone.
> 
> libvirt is the main user and libvirt people
> indicated their preference to creating device with
> -device pvpanic rather than a built-in one that
> can't be removed.
> 
> So please reconsider, and here's an ack from me.
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

NACK for this v1: As pointed out on the KVM call, we still need to keep
the pvpanic device around by default for pc-*-1.5. Removing has_pvpanic
completely therefore seems wrong. Can you submit a v2 for rc3 tomorrow?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.6 1/2] don't create pvpanic device by default.

[Michael S. Tsirkin]

On Sun, Aug 11, 2013 at 04:45:03PM +0200, Andreas Färber wrote:
> Am 11.08.2013 12:33, schrieb Michael S. Tsirkin:
> > On Fri, Aug 02, 2013 at 10:27:31AM +0200, Paolo Bonzini wrote:
> >> On 08/02/2013 09:04 AM, Hu Tao wrote:
> >>> The problem with pvpanic being an internal device is that VMs running
> >>> operating systems without a driver for this device will have problems
> >>> when qemu will be upgraded (from qemu without this pvpanic).
> >>>
> >>> The outcome may be, for example: in Windows(let's say XP) the Device
> >>> manager will open a "new device" wizard and the device will appear as
> >>> an unrecognized device. On a cluster with hundreds of such VMs, If
> >>> that cluster has a health monitoring service it may show all the VMs
> >>> in a "not healthy" state.
> >>>
> >>> Reported-by: Marcel Apfelbaum <marcel.a@redhat.com>
> >>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >>
> >> NACK,
> >>
> >> this is premature.  It is fundamentally a firmware problem.
> >>
> >> We have time to apply an even smaller patch that doesn't set
> >> has_pvpanic to true, and delay the whole feature to 1.7, if we do
> >> not fix the firmware in the next two weeks.
> >>
> >> Paolo
> > 
> > I think this is not just a firmware problem.  Adding device by default
> > was too rush, assumption was risk of guest bugs was 0.
> > 
> > We are now seeing problems with bios guest code and with linux guest
> > drivers as well.  Yes they all can be fixed, but we simply shouldn't
> > force this risk of broken guests on everyone.
> > 
> > libvirt is the main user and libvirt people
> > indicated their preference to creating device with
> > -device pvpanic rather than a built-in one that
> > can't be removed.
> > 
> > So please reconsider, and here's an ack from me.
> > 
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> NACK for this v1: As pointed out on the KVM call, we still need to keep
> the pvpanic device around by default for pc-*-1.5. Removing has_pvpanic
> completely therefore seems wrong.

We also mentioned an option to patch 1.5 stable to change it there,
but I'm fine with not doing it.

> Can you submit a v2 for rc3 tomorrow?
> 
> Andreas


> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.6 1/2] don't create pvpanic device by default.

[Marcel Apfelbaum]

On Sun, 2013-08-11 at 16:45 +0200, Andreas Färber wrote:
> Am 11.08.2013 12:33, schrieb Michael S. Tsirkin:
> > On Fri, Aug 02, 2013 at 10:27:31AM +0200, Paolo Bonzini wrote:
> >> On 08/02/2013 09:04 AM, Hu Tao wrote:
> >>> The problem with pvpanic being an internal device is that VMs running
> >>> operating systems without a driver for this device will have problems
> >>> when qemu will be upgraded (from qemu without this pvpanic).
> >>>
> >>> The outcome may be, for example: in Windows(let's say XP) the Device
> >>> manager will open a "new device" wizard and the device will appear as
> >>> an unrecognized device. On a cluster with hundreds of such VMs, If
> >>> that cluster has a health monitoring service it may show all the VMs
> >>> in a "not healthy" state.
> >>>
> >>> Reported-by: Marcel Apfelbaum <marcel.a@redhat.com>
> >>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >>
> >> NACK,
> >>
> >> this is premature.  It is fundamentally a firmware problem.
> >>
> >> We have time to apply an even smaller patch that doesn't set
> >> has_pvpanic to true, and delay the whole feature to 1.7, if we do
> >> not fix the firmware in the next two weeks.
> >>
> >> Paolo
> > 
> > I think this is not just a firmware problem.  Adding device by default
> > was too rush, assumption was risk of guest bugs was 0.
> > 
> > We are now seeing problems with bios guest code and with linux guest
> > drivers as well.  Yes they all can be fixed, but we simply shouldn't
> > force this risk of broken guests on everyone.
> > 
> > libvirt is the main user and libvirt people
> > indicated their preference to creating device with
> > -device pvpanic rather than a built-in one that
> > can't be removed.
> > 
> > So please reconsider, and here's an ack from me.
> > 
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> NACK for this v1: As pointed out on the KVM call, we still need to keep
> the pvpanic device around by default for pc-*-1.5. Removing has_pvpanic
> completely therefore seems wrong. Can you submit a v2 for rc3 tomorrow?

I just sent a patchset with V2. Can you please review it?
Thanks,
Marcel

> 
> Andreas
> 

[Qemu-devel] [PATCH for-1.6 2/2] pvpanic: make pvpanic known to user

[Hu Tao]

Thus user can create pvpanic by -device.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/misc/pvpanic.c    | 23 ++++++++---------------
 include/hw/i386/pc.h |  3 ---
 2 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 7bb49a5..6e4c53e 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -93,14 +93,6 @@ static void pvpanic_isa_initfn(Object *obj)
     memory_region_init_io(&s->io, OBJECT(s), &pvpanic_ops, s, "pvpanic", 1);
 }
 
-static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp)
-{
-    ISADevice *d = ISA_DEVICE(dev);
-    PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
-
-    isa_register_ioport(d, &s->io, s->ioport);
-}
-
 static void pvpanic_fw_cfg(ISADevice *dev, FWCfgState *fw_cfg)
 {
     PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
@@ -111,15 +103,16 @@ static void pvpanic_fw_cfg(ISADevice *dev, FWCfgState *fw_cfg)
                     sizeof(*pvpanic_port));
 }
 
-void pvpanic_init(ISABus *bus)
+static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp)
 {
-    ISADevice *dev;
+    ISADevice *d = ISA_DEVICE(dev);
+    PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
     FWCfgState *fw_cfg = fw_cfg_find();
-    if (!fw_cfg) {
-        return;
+
+    isa_register_ioport(d, &s->io, s->ioport);
+    if (fw_cfg) {
+        pvpanic_fw_cfg(d, fw_cfg);
     }
-    dev = isa_create_simple (bus, TYPE_ISA_PVPANIC_DEVICE);
-    pvpanic_fw_cfg(dev, fw_cfg);
 }
 
 static Property pvpanic_isa_properties[] = {
@@ -132,8 +125,8 @@ static void pvpanic_isa_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = pvpanic_isa_realizefn;
-    dc->no_user = 1;
     dc->props = pvpanic_isa_properties;
+    dc->bus_type = TYPE_ISA_BUS;
 }
 
 static TypeInfo pvpanic_isa_info = {
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 3a0c4e3..e54751c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -202,9 +202,6 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
 /* pc_sysfw.c */
 void pc_system_firmware_init(MemoryRegion *rom_memory);
 
-/* pvpanic.c */
-void pvpanic_init(ISABus *bus);
-
 /* e820 types */
 #define E820_RAM        1
 #define E820_RESERVED   2
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH for-1.6 2/2] pvpanic: make pvpanic known to user

[Andreas Färber]

Am 02.08.2013 09:04, schrieb Hu Tao:
> Thus user can create pvpanic by -device.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/misc/pvpanic.c    | 23 ++++++++---------------
>  include/hw/i386/pc.h |  3 ---
>  2 files changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> index 7bb49a5..6e4c53e 100644
> --- a/hw/misc/pvpanic.c
> +++ b/hw/misc/pvpanic.c
> @@ -93,14 +93,6 @@ static void pvpanic_isa_initfn(Object *obj)
>      memory_region_init_io(&s->io, OBJECT(s), &pvpanic_ops, s, "pvpanic", 1);
>  }
>  
> -static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp)
> -{
> -    ISADevice *d = ISA_DEVICE(dev);
> -    PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
> -
> -    isa_register_ioport(d, &s->io, s->ioport);
> -}
> -
>  static void pvpanic_fw_cfg(ISADevice *dev, FWCfgState *fw_cfg)
>  {
>      PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
> @@ -111,15 +103,16 @@ static void pvpanic_fw_cfg(ISADevice *dev, FWCfgState *fw_cfg)
>                      sizeof(*pvpanic_port));
>  }
>  
> -void pvpanic_init(ISABus *bus)
> +static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp)
>  {
> -    ISADevice *dev;
> +    ISADevice *d = ISA_DEVICE(dev);
> +    PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
>      FWCfgState *fw_cfg = fw_cfg_find();
> -    if (!fw_cfg) {
> -        return;
> +
> +    isa_register_ioport(d, &s->io, s->ioport);
> +    if (fw_cfg) {
> +        pvpanic_fw_cfg(d, fw_cfg);
>      }
> -    dev = isa_create_simple (bus, TYPE_ISA_PVPANIC_DEVICE);
> -    pvpanic_fw_cfg(dev, fw_cfg);
>  }

Doing this in-place above might've been a bit easier to read. ;)

The only thing fw_cfg does at realize time is registering its I/O ports,
the /machine/fw_cfg path is set up in fw_cfg_init() helper function
before, so there are no potential realize ordering problems here.

>  
>  static Property pvpanic_isa_properties[] = {
> @@ -132,8 +125,8 @@ static void pvpanic_isa_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = pvpanic_isa_realizefn;
> -    dc->no_user = 1;
>      dc->props = pvpanic_isa_properties;

> +    dc->bus_type = TYPE_ISA_BUS;

This is already done in hw/isa/isa-bus.c:isa_device_class_init(), please
drop if we go with this.

Regards,
Andreas

>  }
>  
>  static TypeInfo pvpanic_isa_info = {
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 3a0c4e3..e54751c 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -202,9 +202,6 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
>  /* pc_sysfw.c */
>  void pc_system_firmware_init(MemoryRegion *rom_memory);
>  
> -/* pvpanic.c */
> -void pvpanic_init(ISABus *bus);
> -
>  /* e820 types */
>  #define E820_RAM        1
>  #define E820_RESERVED   2
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg