[Qemu-trivial] [PATCH] Remove PCI class code from virtio balloon device

[Qemu-trivial] [PATCH] Remove PCI class code from virtio balloon device

[David Gibson]

Currently the virtio balloon device, when using the virtio-pci interface
advertises itself with PCI class code MEMORY_RAM.  This is wrong; the
balloon is vaguely related to memory, but is nothing like a PCI memory
device in the meaning of the class code, and this code is not required or
suggested by the virtio PCI specification.

Worse, this patch causes problems on the pseries machine, because the
firmware, seeing this class code, advertises the device as memory in the
device tree, and then a guest kernel bug causes it to see this "memory"
before the real system memory, leading to a crash in early boot.

This patch fixes the problem by removing the bogus PCI class code on the
balloon device.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/virtio-pci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index a0fb7c1..da8a382 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -919,7 +919,7 @@ static void virtio_balloon_class_init(ObjectClass *klass, void *data)
     k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
     k->device_id = PCI_DEVICE_ID_VIRTIO_BALLOON;
     k->revision = VIRTIO_PCI_ABI_VERSION;
-    k->class_id = PCI_CLASS_MEMORY_RAM;
+    k->class_id = PCI_CLASS_OTHERS;
     dc->reset = virtio_pci_reset;
     dc->props = virtio_balloon_properties;
 }
-- 
1.7.9.1

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[Stefan Hajnoczi]

On Mon, Mar 19, 2012 at 03:59:23PM +1100, David Gibson wrote:
> Currently the virtio balloon device, when using the virtio-pci interface
> advertises itself with PCI class code MEMORY_RAM.  This is wrong; the
> balloon is vaguely related to memory, but is nothing like a PCI memory
> device in the meaning of the class code, and this code is not required or
> suggested by the virtio PCI specification.
> 
> Worse, this patch causes problems on the pseries machine, because the
> firmware, seeing this class code, advertises the device as memory in the
> device tree, and then a guest kernel bug causes it to see this "memory"
> before the real system memory, leading to a crash in early boot.
> 
> This patch fixes the problem by removing the bogus PCI class code on the
> balloon device.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/virtio-pci.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Since this is a guest-visible change we might need to be careful about
how it's introduced.

Do we need to keep the old class code for existing machine types?  The
new class code could be introduced only for 1.1 and later machine types
if we want to be extra careful about introducing guest-visible changes.

Michael: Do you want to take it through your tree?

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[David Gibson]

On Mon, Mar 19, 2012 at 11:33:10AM +0000, Stefan Hajnoczi wrote:
> On Mon, Mar 19, 2012 at 03:59:23PM +1100, David Gibson wrote:
> > Currently the virtio balloon device, when using the virtio-pci interface
> > advertises itself with PCI class code MEMORY_RAM.  This is wrong; the
> > balloon is vaguely related to memory, but is nothing like a PCI memory
> > device in the meaning of the class code, and this code is not required or
> > suggested by the virtio PCI specification.
> > 
> > Worse, this patch causes problems on the pseries machine, because the
> > firmware, seeing this class code, advertises the device as memory in the
> > device tree, and then a guest kernel bug causes it to see this "memory"
> > before the real system memory, leading to a crash in early boot.
> > 
> > This patch fixes the problem by removing the bogus PCI class code on the
> > balloon device.
> > 
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/virtio-pci.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> Since this is a guest-visible change we might need to be careful about
> how it's introduced.
> 
> Do we need to keep the old class code for existing machine types?  The
> new class code could be introduced only for 1.1 and later machine types
> if we want to be extra careful about introducing guest-visible
> changes.

So as a general rule, I like to be very careful about user-visible
changes.  But in this case, I don't think we want to be too hesitant.
In particular, it's not just a question of the machine type, but also
of how the guest OS will deal with the PCI class code.

The class code we were using was Just Plain Wrong.  It was not
suggetsed by the virtio spec, and it makes no sense.  It happens that
so far this caused problems only for a guest on a particular machine
type, but there's no reason it couldn't cause (different) problems for
guests on any machine type.

More to the point, it seems reasonably unlikely for existing guests to
rely on the broken behaviour: again, there's no reason they'd think
they need to based on the spec, and the usual way of matching drivers
to PCI devices is with the vendor/device IDs which are correct and not
changed by this patch.

So, unless we have a known example of an existing guest that would be
broken by this change, I think we should implement it ASAP for all
machine types.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[Stefan Hajnoczi]

On Tue, Mar 20, 2012 at 12:42 AM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Mon, Mar 19, 2012 at 11:33:10AM +0000, Stefan Hajnoczi wrote:
>> On Mon, Mar 19, 2012 at 03:59:23PM +1100, David Gibson wrote:
>> > Currently the virtio balloon device, when using the virtio-pci interface
>> > advertises itself with PCI class code MEMORY_RAM.  This is wrong; the
>> > balloon is vaguely related to memory, but is nothing like a PCI memory
>> > device in the meaning of the class code, and this code is not required or
>> > suggested by the virtio PCI specification.
>> >
>> > Worse, this patch causes problems on the pseries machine, because the
>> > firmware, seeing this class code, advertises the device as memory in the
>> > device tree, and then a guest kernel bug causes it to see this "memory"
>> > before the real system memory, leading to a crash in early boot.
>> >
>> > This patch fixes the problem by removing the bogus PCI class code on the
>> > balloon device.
>> >
>> > Cc: Michael S. Tsirkin <mst@redhat.com>
>> > Cc: Rusty Russell <rusty@rustcorp.com.au>
>> >
>> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> > ---
>> >  hw/virtio-pci.c |    2 +-
>> >  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> Since this is a guest-visible change we might need to be careful about
>> how it's introduced.
>>
>> Do we need to keep the old class code for existing machine types?  The
>> new class code could be introduced only for 1.1 and later machine types
>> if we want to be extra careful about introducing guest-visible
>> changes.
>
> So as a general rule, I like to be very careful about user-visible
> changes.  But in this case, I don't think we want to be too hesitant.
> In particular, it's not just a question of the machine type, but also
> of how the guest OS will deal with the PCI class code.
>
> The class code we were using was Just Plain Wrong.  It was not
> suggetsed by the virtio spec, and it makes no sense.  It happens that
> so far this caused problems only for a guest on a particular machine
> type, but there's no reason it couldn't cause (different) problems for
> guests on any machine type.
>
> More to the point, it seems reasonably unlikely for existing guests to
> rely on the broken behaviour: again, there's no reason they'd think
> they need to based on the spec, and the usual way of matching drivers
> to PCI devices is with the vendor/device IDs which are correct and not
> changed by this patch.
>
> So, unless we have a known example of an existing guest that would be
> broken by this change, I think we should implement it ASAP for all
> machine types.

I agree that in practice the risk is low because working guests are
probably not using the class code.  On the other hand I don't see a
downside to making this part of the 1.1 machine type, which is what
users will run when they get this code change anyway.  That way we can
tell users that we never change the device model in a release with a
straight face :).

Anthony: I'm not sure how strict we are about a user-visible change like this?

Stefan

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[David Gibson]

On Tue, Mar 20, 2012 at 09:54:20AM +0000, Stefan Hajnoczi wrote:
> On Tue, Mar 20, 2012 at 12:42 AM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > On Mon, Mar 19, 2012 at 11:33:10AM +0000, Stefan Hajnoczi wrote:
> >> On Mon, Mar 19, 2012 at 03:59:23PM +1100, David Gibson wrote:
> >> > Currently the virtio balloon device, when using the virtio-pci interface
> >> > advertises itself with PCI class code MEMORY_RAM.  This is wrong; the
> >> > balloon is vaguely related to memory, but is nothing like a PCI memory
> >> > device in the meaning of the class code, and this code is not required or
> >> > suggested by the virtio PCI specification.
> >> >
> >> > Worse, this patch causes problems on the pseries machine, because the
> >> > firmware, seeing this class code, advertises the device as memory in the
> >> > device tree, and then a guest kernel bug causes it to see this "memory"
> >> > before the real system memory, leading to a crash in early boot.
> >> >
> >> > This patch fixes the problem by removing the bogus PCI class code on the
> >> > balloon device.
> >> >
> >> > Cc: Michael S. Tsirkin <mst@redhat.com>
> >> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> >> >
> >> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >> > ---
> >> >  hw/virtio-pci.c |    2 +-
> >> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> Since this is a guest-visible change we might need to be careful about
> >> how it's introduced.
> >>
> >> Do we need to keep the old class code for existing machine types?  The
> >> new class code could be introduced only for 1.1 and later machine types
> >> if we want to be extra careful about introducing guest-visible
> >> changes.
> >
> > So as a general rule, I like to be very careful about user-visible
> > changes.  But in this case, I don't think we want to be too hesitant.
> > In particular, it's not just a question of the machine type, but also
> > of how the guest OS will deal with the PCI class code.
> >
> > The class code we were using was Just Plain Wrong.  It was not
> > suggetsed by the virtio spec, and it makes no sense.  It happens that
> > so far this caused problems only for a guest on a particular machine
> > type, but there's no reason it couldn't cause (different) problems for
> > guests on any machine type.
> >
> > More to the point, it seems reasonably unlikely for existing guests to
> > rely on the broken behaviour: again, there's no reason they'd think
> > they need to based on the spec, and the usual way of matching drivers
> > to PCI devices is with the vendor/device IDs which are correct and not
> > changed by this patch.
> >
> > So, unless we have a known example of an existing guest that would be
> > broken by this change, I think we should implement it ASAP for all
> > machine types.
> 
> I agree that in practice the risk is low because working guests are
> probably not using the class code.  On the other hand I don't see a
> downside to making this part of the 1.1 machine type,

Well.. there's the fact that I can't what mechanism we would use to
make this per-machine...

> which is what
> users will run when they get this code change anyway.  That way we can
> tell users that we never change the device model in a release with a
> straight face :).
> 
> Anthony: I'm not sure how strict we are about a user-visible change like this?
> 
> Stefan
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[Stefan Hajnoczi]

On Tue, Mar 20, 2012 at 09:19:47PM +1100, David Gibson wrote:
> On Tue, Mar 20, 2012 at 09:54:20AM +0000, Stefan Hajnoczi wrote:
> > On Tue, Mar 20, 2012 at 12:42 AM, David Gibson
> > <david@gibson.dropbear.id.au> wrote:
> > > On Mon, Mar 19, 2012 at 11:33:10AM +0000, Stefan Hajnoczi wrote:
> > >> On Mon, Mar 19, 2012 at 03:59:23PM +1100, David Gibson wrote:
> > >> > Currently the virtio balloon device, when using the virtio-pci interface
> > >> > advertises itself with PCI class code MEMORY_RAM.  This is wrong; the
> > >> > balloon is vaguely related to memory, but is nothing like a PCI memory
> > >> > device in the meaning of the class code, and this code is not required or
> > >> > suggested by the virtio PCI specification.
> > >> >
> > >> > Worse, this patch causes problems on the pseries machine, because the
> > >> > firmware, seeing this class code, advertises the device as memory in the
> > >> > device tree, and then a guest kernel bug causes it to see this "memory"
> > >> > before the real system memory, leading to a crash in early boot.
> > >> >
> > >> > This patch fixes the problem by removing the bogus PCI class code on the
> > >> > balloon device.
> > >> >
> > >> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > >> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > >> >
> > >> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > >> > ---
> > >> >  hw/virtio-pci.c |    2 +-
> > >> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > >>
> > >> Since this is a guest-visible change we might need to be careful about
> > >> how it's introduced.
> > >>
> > >> Do we need to keep the old class code for existing machine types?  The
> > >> new class code could be introduced only for 1.1 and later machine types
> > >> if we want to be extra careful about introducing guest-visible
> > >> changes.
> > >
> > > So as a general rule, I like to be very careful about user-visible
> > > changes.  But in this case, I don't think we want to be too hesitant.
> > > In particular, it's not just a question of the machine type, but also
> > > of how the guest OS will deal with the PCI class code.
> > >
> > > The class code we were using was Just Plain Wrong.  It was not
> > > suggetsed by the virtio spec, and it makes no sense.  It happens that
> > > so far this caused problems only for a guest on a particular machine
> > > type, but there's no reason it couldn't cause (different) problems for
> > > guests on any machine type.
> > >
> > > More to the point, it seems reasonably unlikely for existing guests to
> > > rely on the broken behaviour: again, there's no reason they'd think
> > > they need to based on the spec, and the usual way of matching drivers
> > > to PCI devices is with the vendor/device IDs which are correct and not
> > > changed by this patch.
> > >
> > > So, unless we have a known example of an existing guest that would be
> > > broken by this change, I think we should implement it ASAP for all
> > > machine types.
> > 
> > I agree that in practice the risk is low because working guests are
> > probably not using the class code.  On the other hand I don't see a
> > downside to making this part of the 1.1 machine type,
> 
> Well.. there's the fact that I can't what mechanism we would use to
> make this per-machine...

Not sure I parsed this correctly, but I think you're asking how to do
it.

Looking at hw/pc_piix.c there are QEMUMachine types for each QEMU
release.  Legacy machine types (e.g. pc_machine_v0_14) have a
.compat_props array that can override qdev properties.

Perhaps Michael Tsirkin or someone else can comment on how to wire up
hw/virtio-pci.c so that the class code can be overridden.

Stefan

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[Stefan Hajnoczi]

Hi,
It seems your Mail-Followup-To: header causes my client to drop you
from the To: list.

On Wed, Mar 21, 2012 at 11:26 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Mar 20, 2012 at 09:19:47PM +1100, David Gibson wrote:
>> On Tue, Mar 20, 2012 at 09:54:20AM +0000, Stefan Hajnoczi wrote:
>> > On Tue, Mar 20, 2012 at 12:42 AM, David Gibson
>> > <david@gibson.dropbear.id.au> wrote:
>> > > On Mon, Mar 19, 2012 at 11:33:10AM +0000, Stefan Hajnoczi wrote:
>> > >> On Mon, Mar 19, 2012 at 03:59:23PM +1100, David Gibson wrote:
>> > >> > Currently the virtio balloon device, when using the virtio-pci interface
>> > >> > advertises itself with PCI class code MEMORY_RAM.  This is wrong; the
>> > >> > balloon is vaguely related to memory, but is nothing like a PCI memory
>> > >> > device in the meaning of the class code, and this code is not required or
>> > >> > suggested by the virtio PCI specification.
>> > >> >
>> > >> > Worse, this patch causes problems on the pseries machine, because the
>> > >> > firmware, seeing this class code, advertises the device as memory in the
>> > >> > device tree, and then a guest kernel bug causes it to see this "memory"
>> > >> > before the real system memory, leading to a crash in early boot.
>> > >> >
>> > >> > This patch fixes the problem by removing the bogus PCI class code on the
>> > >> > balloon device.
>> > >> >
>> > >> > Cc: Michael S. Tsirkin <mst@redhat.com>
>> > >> > Cc: Rusty Russell <rusty@rustcorp.com.au>
>> > >> >
>> > >> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> > >> > ---
>> > >> >  hw/virtio-pci.c |    2 +-
>> > >> >  1 files changed, 1 insertions(+), 1 deletions(-)
>> > >>
>> > >> Since this is a guest-visible change we might need to be careful about
>> > >> how it's introduced.
>> > >>
>> > >> Do we need to keep the old class code for existing machine types?  The
>> > >> new class code could be introduced only for 1.1 and later machine types
>> > >> if we want to be extra careful about introducing guest-visible
>> > >> changes.
>> > >
>> > > So as a general rule, I like to be very careful about user-visible
>> > > changes.  But in this case, I don't think we want to be too hesitant.
>> > > In particular, it's not just a question of the machine type, but also
>> > > of how the guest OS will deal with the PCI class code.
>> > >
>> > > The class code we were using was Just Plain Wrong.  It was not
>> > > suggetsed by the virtio spec, and it makes no sense.  It happens that
>> > > so far this caused problems only for a guest on a particular machine
>> > > type, but there's no reason it couldn't cause (different) problems for
>> > > guests on any machine type.
>> > >
>> > > More to the point, it seems reasonably unlikely for existing guests to
>> > > rely on the broken behaviour: again, there's no reason they'd think
>> > > they need to based on the spec, and the usual way of matching drivers
>> > > to PCI devices is with the vendor/device IDs which are correct and not
>> > > changed by this patch.
>> > >
>> > > So, unless we have a known example of an existing guest that would be
>> > > broken by this change, I think we should implement it ASAP for all
>> > > machine types.
>> >
>> > I agree that in practice the risk is low because working guests are
>> > probably not using the class code.  On the other hand I don't see a
>> > downside to making this part of the 1.1 machine type,
>>
>> Well.. there's the fact that I can't what mechanism we would use to
>> make this per-machine...
>
> Not sure I parsed this correctly, but I think you're asking how to do
> it.
>
> Looking at hw/pc_piix.c there are QEMUMachine types for each QEMU
> release.  Legacy machine types (e.g. pc_machine_v0_14) have a
> .compat_props array that can override qdev properties.
>
> Perhaps Michael Tsirkin or someone else can comment on how to wire up
> hw/virtio-pci.c so that the class code can be overridden.
>
> Stefan

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[David Gibson]

On Wed, Mar 21, 2012 at 11:28:47AM +0000, Stefan Hajnoczi wrote:
> Hi,
> It seems your Mail-Followup-To: header causes my client to drop you
> from the To: list.

Not mine, it's added by the list AFAICT.  And it's frickin' annoying.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[Michael S. Tsirkin]

On Wed, Mar 21, 2012 at 11:26:15AM +0000, Stefan Hajnoczi wrote:
> On Tue, Mar 20, 2012 at 09:19:47PM +1100, David Gibson wrote:
> > On Tue, Mar 20, 2012 at 09:54:20AM +0000, Stefan Hajnoczi wrote:
> > > On Tue, Mar 20, 2012 at 12:42 AM, David Gibson
> > > <david@gibson.dropbear.id.au> wrote:
> > > > On Mon, Mar 19, 2012 at 11:33:10AM +0000, Stefan Hajnoczi wrote:
> > > >> On Mon, Mar 19, 2012 at 03:59:23PM +1100, David Gibson wrote:
> > > >> > Currently the virtio balloon device, when using the virtio-pci interface
> > > >> > advertises itself with PCI class code MEMORY_RAM.  This is wrong; the
> > > >> > balloon is vaguely related to memory, but is nothing like a PCI memory
> > > >> > device in the meaning of the class code, and this code is not required or
> > > >> > suggested by the virtio PCI specification.
> > > >> >
> > > >> > Worse, this patch causes problems on the pseries machine, because the
> > > >> > firmware, seeing this class code, advertises the device as memory in the
> > > >> > device tree, and then a guest kernel bug causes it to see this "memory"
> > > >> > before the real system memory, leading to a crash in early boot.
> > > >> >
> > > >> > This patch fixes the problem by removing the bogus PCI class code on the
> > > >> > balloon device.
> > > >> >
> > > >> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > >> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > >> >
> > > >> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > >> > ---
> > > >> >  hw/virtio-pci.c |    2 +-
> > > >> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > >>
> > > >> Since this is a guest-visible change we might need to be careful about
> > > >> how it's introduced.
> > > >>
> > > >> Do we need to keep the old class code for existing machine types?  The
> > > >> new class code could be introduced only for 1.1 and later machine types
> > > >> if we want to be extra careful about introducing guest-visible
> > > >> changes.
> > > >
> > > > So as a general rule, I like to be very careful about user-visible
> > > > changes.  But in this case, I don't think we want to be too hesitant.
> > > > In particular, it's not just a question of the machine type, but also
> > > > of how the guest OS will deal with the PCI class code.
> > > >
> > > > The class code we were using was Just Plain Wrong.  It was not
> > > > suggetsed by the virtio spec, and it makes no sense.  It happens that
> > > > so far this caused problems only for a guest on a particular machine
> > > > type, but there's no reason it couldn't cause (different) problems for
> > > > guests on any machine type.
> > > >
> > > > More to the point, it seems reasonably unlikely for existing guests to
> > > > rely on the broken behaviour: again, there's no reason they'd think
> > > > they need to based on the spec, and the usual way of matching drivers
> > > > to PCI devices is with the vendor/device IDs which are correct and not
> > > > changed by this patch.
> > > >
> > > > So, unless we have a known example of an existing guest that would be
> > > > broken by this change, I think we should implement it ASAP for all
> > > > machine types.
> > > 
> > > I agree that in practice the risk is low because working guests are
> > > probably not using the class code.  On the other hand I don't see a
> > > downside to making this part of the 1.1 machine type,
> > 
> > Well.. there's the fact that I can't what mechanism we would use to
> > make this per-machine...
> 
> Not sure I parsed this correctly, but I think you're asking how to do
> it.
> 
> Looking at hw/pc_piix.c there are QEMUMachine types for each QEMU
> release.  Legacy machine types (e.g. pc_machine_v0_14) have a
> .compat_props array that can override qdev properties.
> 
> Perhaps Michael Tsirkin or someone else can comment on how to wire up
> hw/virtio-pci.c so that the class code can be overridden.
> 
> Stefan

afaik we already let users over-write it for some other pci devices,
look there for examples.

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[Anthony Liguori]

On 03/21/2012 08:08 AM, Michael S. Tsirkin wrote:
> On Wed, Mar 21, 2012 at 11:26:15AM +0000, Stefan Hajnoczi wrote:
>> On Tue, Mar 20, 2012 at 09:19:47PM +1100, David Gibson wrote:
>> Looking at hw/pc_piix.c there are QEMUMachine types for each QEMU
>> release.  Legacy machine types (e.g. pc_machine_v0_14) have a
>> .compat_props array that can override qdev properties.
>>
>> Perhaps Michael Tsirkin or someone else can comment on how to wire up
>> hw/virtio-pci.c so that the class code can be overridden.
>>
>> Stefan
>
> afaik we already let users over-write it for some other pci devices,
> look there for examples.

 From hw/pc_piix.c:

     .name = "pc-0.10",
     .desc = "Standard PC, qemu 0.10",
     .init = pc_init_pci_no_kvmclock,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
         {
             .driver   = "virtio-blk-pci",
             .property = "class",
             .value    = stringify(PCI_CLASS_STORAGE_OTHER),
         },{

And from the earlier part of the thread, yes, it's imperative that we do not 
change anything in the PCI configuration space for older pc versions regardless 
of whether it may or may not work.

Certain guests (like Windows) use a complex fingerprinting algorithm to 
determine when hardware changes.  It can be hard to detect in simple testing 
because it's based on a threshold.

Regards,

Anthony Liguori

>
>

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[Michael S. Tsirkin]

On Wed, Mar 21, 2012 at 09:42:41AM -0500, Anthony Liguori wrote:
> On 03/21/2012 08:08 AM, Michael S. Tsirkin wrote:
> >On Wed, Mar 21, 2012 at 11:26:15AM +0000, Stefan Hajnoczi wrote:
> >>On Tue, Mar 20, 2012 at 09:19:47PM +1100, David Gibson wrote:
> >>Looking at hw/pc_piix.c there are QEMUMachine types for each QEMU
> >>release.  Legacy machine types (e.g. pc_machine_v0_14) have a
> >>.compat_props array that can override qdev properties.
> >>
> >>Perhaps Michael Tsirkin or someone else can comment on how to wire up
> >>hw/virtio-pci.c so that the class code can be overridden.
> >>
> >>Stefan
> >
> >afaik we already let users over-write it for some other pci devices,
> >look there for examples.
> 
> From hw/pc_piix.c:
> 
>     .name = "pc-0.10",
>     .desc = "Standard PC, qemu 0.10",
>     .init = pc_init_pci_no_kvmclock,
>     .max_cpus = 255,
>     .compat_props = (GlobalProperty[]) {
>         {
>             .driver   = "virtio-blk-pci",
>             .property = "class",
>             .value    = stringify(PCI_CLASS_STORAGE_OTHER),
>         },{
> 
> And from the earlier part of the thread, yes, it's imperative that
> we do not change anything in the PCI configuration space for older
> pc versions regardless of whether it may or may not work.
> 
> Certain guests (like Windows) use a complex fingerprinting algorithm
> to determine when hardware changes.  It can be hard to detect in
> simple testing because it's based on a threshold.
> 
> Regards,
> 
> Anthony Liguori

Which reminds me - qemu sticks the release version in
guest visible places like CPU version.
This is wrong and causes windows guests to print messages
about driver updates when you switch.
We should find all these places and stop doing this.

> >
> >

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[Anthony Liguori]

On 03/21/2012 10:10 AM, Michael S. Tsirkin wrote:
> On Wed, Mar 21, 2012 at 09:42:41AM -0500, Anthony Liguori wrote:
>> On 03/21/2012 08:08 AM, Michael S. Tsirkin wrote:
>>> On Wed, Mar 21, 2012 at 11:26:15AM +0000, Stefan Hajnoczi wrote:
>>>> On Tue, Mar 20, 2012 at 09:19:47PM +1100, David Gibson wrote:
>>>> Looking at hw/pc_piix.c there are QEMUMachine types for each QEMU
>>>> release.  Legacy machine types (e.g. pc_machine_v0_14) have a
>>>> .compat_props array that can override qdev properties.
>>>>
>>>> Perhaps Michael Tsirkin or someone else can comment on how to wire up
>>>> hw/virtio-pci.c so that the class code can be overridden.
>>>>
>>>> Stefan
>>>
>>> afaik we already let users over-write it for some other pci devices,
>>> look there for examples.
>>
>>  From hw/pc_piix.c:
>>
>>      .name = "pc-0.10",
>>      .desc = "Standard PC, qemu 0.10",
>>      .init = pc_init_pci_no_kvmclock,
>>      .max_cpus = 255,
>>      .compat_props = (GlobalProperty[]) {
>>          {
>>              .driver   = "virtio-blk-pci",
>>              .property = "class",
>>              .value    = stringify(PCI_CLASS_STORAGE_OTHER),
>>          },{
>>
>> And from the earlier part of the thread, yes, it's imperative that
>> we do not change anything in the PCI configuration space for older
>> pc versions regardless of whether it may or may not work.
>>
>> Certain guests (like Windows) use a complex fingerprinting algorithm
>> to determine when hardware changes.  It can be hard to detect in
>> simple testing because it's based on a threshold.
>>
>> Regards,
>>
>> Anthony Liguori
>
> Which reminds me - qemu sticks the release version in
> guest visible places like CPU version.
> This is wrong and causes windows guests to print messages
> about driver updates when you switch.
> We should find all these places and stop doing this.

We could probably get away with doing a query/replace of QEMU_VERSION with 
qemu_get_version(), make version a static variable that defaults to 
QEMU_VERSION, and then provide a way for machines to override it.

Then pc-0.10 could report a version of 0.10.

Regards,

Anthony Liguori

>>>
>>>

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[Michael S. Tsirkin]

On Wed, Mar 21, 2012 at 10:14:35AM -0500, Anthony Liguori wrote:
> On 03/21/2012 10:10 AM, Michael S. Tsirkin wrote:
> >On Wed, Mar 21, 2012 at 09:42:41AM -0500, Anthony Liguori wrote:
> >>On 03/21/2012 08:08 AM, Michael S. Tsirkin wrote:
> >>>On Wed, Mar 21, 2012 at 11:26:15AM +0000, Stefan Hajnoczi wrote:
> >>>>On Tue, Mar 20, 2012 at 09:19:47PM +1100, David Gibson wrote:
> >>>>Looking at hw/pc_piix.c there are QEMUMachine types for each QEMU
> >>>>release.  Legacy machine types (e.g. pc_machine_v0_14) have a
> >>>>.compat_props array that can override qdev properties.
> >>>>
> >>>>Perhaps Michael Tsirkin or someone else can comment on how to wire up
> >>>>hw/virtio-pci.c so that the class code can be overridden.
> >>>>
> >>>>Stefan
> >>>
> >>>afaik we already let users over-write it for some other pci devices,
> >>>look there for examples.
> >>
> >> From hw/pc_piix.c:
> >>
> >>     .name = "pc-0.10",
> >>     .desc = "Standard PC, qemu 0.10",
> >>     .init = pc_init_pci_no_kvmclock,
> >>     .max_cpus = 255,
> >>     .compat_props = (GlobalProperty[]) {
> >>         {
> >>             .driver   = "virtio-blk-pci",
> >>             .property = "class",
> >>             .value    = stringify(PCI_CLASS_STORAGE_OTHER),
> >>         },{
> >>
> >>And from the earlier part of the thread, yes, it's imperative that
> >>we do not change anything in the PCI configuration space for older
> >>pc versions regardless of whether it may or may not work.
> >>
> >>Certain guests (like Windows) use a complex fingerprinting algorithm
> >>to determine when hardware changes.  It can be hard to detect in
> >>simple testing because it's based on a threshold.
> >>
> >>Regards,
> >>
> >>Anthony Liguori
> >
> >Which reminds me - qemu sticks the release version in
> >guest visible places like CPU version.
> >This is wrong and causes windows guests to print messages
> >about driver updates when you switch.
> >We should find all these places and stop doing this.
> 
> We could probably get away with doing a query/replace of
> QEMU_VERSION with qemu_get_version(), make version a static variable
> that defaults to QEMU_VERSION, and then provide a way for machines
> to override it.
> 
> Then pc-0.10 could report a version of 0.10.
> 
> Regards,
> 
> Anthony Liguori

Frankly I don't see value in making it visible to the user,
at all. We are just triggering windows reactivations
without any user benefit. Why not return a fixed value there
to avoid that?

> >>>
> >>>

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[Anthony Liguori]

On 03/21/2012 11:11 AM, Michael S. Tsirkin wrote:
> On Wed, Mar 21, 2012 at 10:14:35AM -0500, Anthony Liguori wrote:
>> On 03/21/2012 10:10 AM, Michael S. Tsirkin wrote:
>>> On Wed, Mar 21, 2012 at 09:42:41AM -0500, Anthony Liguori wrote:
>>>> On 03/21/2012 08:08 AM, Michael S. Tsirkin wrote:
>>>>> On Wed, Mar 21, 2012 at 11:26:15AM +0000, Stefan Hajnoczi wrote:
>>>>>> On Tue, Mar 20, 2012 at 09:19:47PM +1100, David Gibson wrote:
>>>>>> Looking at hw/pc_piix.c there are QEMUMachine types for each QEMU
>>>>>> release.  Legacy machine types (e.g. pc_machine_v0_14) have a
>>>>>> .compat_props array that can override qdev properties.
>>>>>>
>>>>>> Perhaps Michael Tsirkin or someone else can comment on how to wire up
>>>>>> hw/virtio-pci.c so that the class code can be overridden.
>>>>>>
>>>>>> Stefan
>>>>>
>>>>> afaik we already let users over-write it for some other pci devices,
>>>>> look there for examples.
>>>>
>>>>  From hw/pc_piix.c:
>>>>
>>>>      .name = "pc-0.10",
>>>>      .desc = "Standard PC, qemu 0.10",
>>>>      .init = pc_init_pci_no_kvmclock,
>>>>      .max_cpus = 255,
>>>>      .compat_props = (GlobalProperty[]) {
>>>>          {
>>>>              .driver   = "virtio-blk-pci",
>>>>              .property = "class",
>>>>              .value    = stringify(PCI_CLASS_STORAGE_OTHER),
>>>>          },{
>>>>
>>>> And from the earlier part of the thread, yes, it's imperative that
>>>> we do not change anything in the PCI configuration space for older
>>>> pc versions regardless of whether it may or may not work.
>>>>
>>>> Certain guests (like Windows) use a complex fingerprinting algorithm
>>>> to determine when hardware changes.  It can be hard to detect in
>>>> simple testing because it's based on a threshold.
>>>>
>>>> Regards,
>>>>
>>>> Anthony Liguori
>>>
>>> Which reminds me - qemu sticks the release version in
>>> guest visible places like CPU version.
>>> This is wrong and causes windows guests to print messages
>>> about driver updates when you switch.
>>> We should find all these places and stop doing this.
>>
>> We could probably get away with doing a query/replace of
>> QEMU_VERSION with qemu_get_version(), make version a static variable
>> that defaults to QEMU_VERSION, and then provide a way for machines
>> to override it.
>>
>> Then pc-0.10 could report a version of 0.10.
>>
>> Regards,
>>
>> Anthony Liguori
>
> Frankly I don't see value in making it visible to the user,
> at all. We are just triggering windows reactivations
> without any user benefit. Why not return a fixed value there
> to avoid that?

I don't see a problem making it fixed for 1.1, but for 1.0 and older, we should 
expose what we were supposed to expose.

We need to fix the bug first, then we can change the behavior.

Regards,

Anthony Liguori

>
>>>>>
>>>>>

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[Anthony Liguori]

On 03/21/2012 11:26 AM, Anthony Liguori wrote:
> On 03/21/2012 11:11 AM, Michael S. Tsirkin wrote:
>> Frankly I don't see value in making it visible to the user,
>> at all. We are just triggering windows reactivations
>> without any user benefit. Why not return a fixed value there
>> to avoid that?
>
> I don't see a problem making it fixed for 1.1, but for 1.0 and older, we should
> expose what we were supposed to expose.
>
> We need to fix the bug first, then we can change the behavior.

In some cases, like USB, we really do want to expose a version, but we should 
probably only expose the major version, for instance, QEMU 1.x or 2.x.  This 
would only be exposed by the appropriate machine types.

Regards,

Anthony Liguori

>
> Regards,
>
> Anthony Liguori
>
>>
>>>>>>
>>>>>>
>
>

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[Michael S. Tsirkin]

On Wed, Mar 21, 2012 at 11:33:21AM -0500, Anthony Liguori wrote:
> On 03/21/2012 11:26 AM, Anthony Liguori wrote:
> >On 03/21/2012 11:11 AM, Michael S. Tsirkin wrote:
> >>Frankly I don't see value in making it visible to the user,
> >>at all. We are just triggering windows reactivations
> >>without any user benefit. Why not return a fixed value there
> >>to avoid that?
> >
> >I don't see a problem making it fixed for 1.1, but for 1.0 and older, we should
> >expose what we were supposed to expose.
> >
> >We need to fix the bug first, then we can change the behavior.
> 
> In some cases, like USB, we really do want to expose a version,

why, exactly?

> but
> we should probably only expose the major version, for instance, QEMU
> 1.x or 2.x.  This would only be exposed by the appropriate machine
> types.
> 
> Regards,
> 
> Anthony Liguori
> 
> >
> >Regards,
> >
> >Anthony Liguori
> >
> >>
> >>>>>>
> >>>>>>
> >
> >

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Remove PCI class code from virtio balloon device

[Michael S. Tsirkin]

On Wed, Mar 21, 2012 at 11:26:50AM -0500, Anthony Liguori wrote:
> On 03/21/2012 11:11 AM, Michael S. Tsirkin wrote:
> >On Wed, Mar 21, 2012 at 10:14:35AM -0500, Anthony Liguori wrote:
> >>On 03/21/2012 10:10 AM, Michael S. Tsirkin wrote:
> >>>On Wed, Mar 21, 2012 at 09:42:41AM -0500, Anthony Liguori wrote:
> >>>>On 03/21/2012 08:08 AM, Michael S. Tsirkin wrote:
> >>>>>On Wed, Mar 21, 2012 at 11:26:15AM +0000, Stefan Hajnoczi wrote:
> >>>>>>On Tue, Mar 20, 2012 at 09:19:47PM +1100, David Gibson wrote:
> >>>>>>Looking at hw/pc_piix.c there are QEMUMachine types for each QEMU
> >>>>>>release.  Legacy machine types (e.g. pc_machine_v0_14) have a
> >>>>>>.compat_props array that can override qdev properties.
> >>>>>>
> >>>>>>Perhaps Michael Tsirkin or someone else can comment on how to wire up
> >>>>>>hw/virtio-pci.c so that the class code can be overridden.
> >>>>>>
> >>>>>>Stefan
> >>>>>
> >>>>>afaik we already let users over-write it for some other pci devices,
> >>>>>look there for examples.
> >>>>
> >>>> From hw/pc_piix.c:
> >>>>
> >>>>     .name = "pc-0.10",
> >>>>     .desc = "Standard PC, qemu 0.10",
> >>>>     .init = pc_init_pci_no_kvmclock,
> >>>>     .max_cpus = 255,
> >>>>     .compat_props = (GlobalProperty[]) {
> >>>>         {
> >>>>             .driver   = "virtio-blk-pci",
> >>>>             .property = "class",
> >>>>             .value    = stringify(PCI_CLASS_STORAGE_OTHER),
> >>>>         },{
> >>>>
> >>>>And from the earlier part of the thread, yes, it's imperative that
> >>>>we do not change anything in the PCI configuration space for older
> >>>>pc versions regardless of whether it may or may not work.
> >>>>
> >>>>Certain guests (like Windows) use a complex fingerprinting algorithm
> >>>>to determine when hardware changes.  It can be hard to detect in
> >>>>simple testing because it's based on a threshold.
> >>>>
> >>>>Regards,
> >>>>
> >>>>Anthony Liguori
> >>>
> >>>Which reminds me - qemu sticks the release version in
> >>>guest visible places like CPU version.
> >>>This is wrong and causes windows guests to print messages
> >>>about driver updates when you switch.
> >>>We should find all these places and stop doing this.
> >>
> >>We could probably get away with doing a query/replace of
> >>QEMU_VERSION with qemu_get_version(), make version a static variable
> >>that defaults to QEMU_VERSION, and then provide a way for machines
> >>to override it.
> >>
> >>Then pc-0.10 could report a version of 0.10.
> >>
> >>Regards,
> >>
> >>Anthony Liguori
> >
> >Frankly I don't see value in making it visible to the user,
> >at all. We are just triggering windows reactivations
> >without any user benefit. Why not return a fixed value there
> >to avoid that?
> 
> I don't see a problem making it fixed for 1.1, but for 1.0 and
> older, we should expose what we were supposed to expose.
> We need to fix the bug first, then we can change the behavior.
> 
> Regards,
> 
> Anthony Liguori
> 
> >
> >>>>>
> >>>>>

Makes sense to me.