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

[Qemu-devel] [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.  The backwards compatibility PC machines get new compat
properties so that they don't change.

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/pc_piix.c    |   28 ++++++++++++++++++++++++++++
 hw/virtio-pci.c |    8 +++++++-
 2 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 3f99f9a..55dcd2e 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -386,6 +386,10 @@ static QEMUMachine pc_machine_v1_0 = {
             .driver   = "isa-fdc",
             .property = "check_media_rate",
             .value    = "off",
+        }, {
+            .driver   = "virtio-balloon-pci",
+            .property = "class",
+            .value    = stringify(PCI_CLASS_MEMORY_RAM),
         },
         { /* end of list */ }
     },
@@ -405,6 +409,10 @@ static QEMUMachine pc_machine_v0_15 = {
             .driver   = "isa-fdc",
             .property = "check_media_rate",
             .value    = "off",
+        }, {
+            .driver   = "virtio-balloon-pci",
+            .property = "class",
+            .value    = stringify(PCI_CLASS_MEMORY_RAM),
         },
         { /* end of list */ }
     },
@@ -449,6 +457,10 @@ static QEMUMachine pc_machine_v0_14 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        }, {
+            .driver   = "virtio-balloon-pci",
+            .property = "class",
+            .value    = stringify(PCI_CLASS_MEMORY_RAM),
         },
         { /* end of list */ }
     },
@@ -505,6 +517,10 @@ static QEMUMachine pc_machine_v0_13 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        }, {
+            .driver   = "virtio-balloon-pci",
+            .property = "class",
+            .value    = stringify(PCI_CLASS_MEMORY_RAM),
         },
         { /* end of list */ }
     },
@@ -565,6 +581,10 @@ static QEMUMachine pc_machine_v0_12 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        }, {
+            .driver   = "virtio-balloon-pci",
+            .property = "class",
+            .value    = stringify(PCI_CLASS_MEMORY_RAM),
         },
         { /* end of list */ }
     }
@@ -633,6 +653,10 @@ static QEMUMachine pc_machine_v0_11 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        }, {
+            .driver   = "virtio-balloon-pci",
+            .property = "class",
+            .value    = stringify(PCI_CLASS_MEMORY_RAM),
         },
         { /* end of list */ }
     }
@@ -713,6 +737,10 @@ static QEMUMachine pc_machine_v0_10 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        }, {
+            .driver   = "virtio-balloon-pci",
+            .property = "class",
+            .value    = stringify(PCI_CLASS_MEMORY_RAM),
         },
         { /* end of list */ }
     },
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index a0fb7c1..a1fa656 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -790,6 +790,11 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
     VirtIODevice *vdev;
 
+    if (proxy->class_code != PCI_CLASS_OTHERS &&
+        proxy->class_code != PCI_CLASS_MEMORY_RAM) { /* qemu < 1.1 */
+        proxy->class_code = PCI_CLASS_OTHERS;
+    }
+
     vdev = virtio_balloon_init(&pci_dev->qdev);
     if (!vdev) {
         return -1;
@@ -905,6 +910,7 @@ static TypeInfo virtio_serial_info = {
 };
 
 static Property virtio_balloon_properties[] = {
+    DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, PCI_CLASS_OTHERS),
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -919,7 +925,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-devel] [PATCH] Remove PCI class code from virtio balloon device

[David Gibson]

Anthony..

please apply?

On Mon, Mar 26, 2012 at 12:19:40PM +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.  The backwards compatibility PC machines get new compat
> properties so that they don't change.
> 
> 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/pc_piix.c    |   28 ++++++++++++++++++++++++++++
>  hw/virtio-pci.c |    8 +++++++-
>  2 files changed, 35 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 3f99f9a..55dcd2e 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -386,6 +386,10 @@ static QEMUMachine pc_machine_v1_0 = {
>              .driver   = "isa-fdc",
>              .property = "check_media_rate",
>              .value    = "off",
> +        }, {
> +            .driver   = "virtio-balloon-pci",
> +            .property = "class",
> +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
>          },
>          { /* end of list */ }
>      },
> @@ -405,6 +409,10 @@ static QEMUMachine pc_machine_v0_15 = {
>              .driver   = "isa-fdc",
>              .property = "check_media_rate",
>              .value    = "off",
> +        }, {
> +            .driver   = "virtio-balloon-pci",
> +            .property = "class",
> +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
>          },
>          { /* end of list */ }
>      },
> @@ -449,6 +457,10 @@ static QEMUMachine pc_machine_v0_14 = {
>              .driver   = "pc-sysfw",
>              .property = "rom_only",
>              .value    = stringify(1),
> +        }, {
> +            .driver   = "virtio-balloon-pci",
> +            .property = "class",
> +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
>          },
>          { /* end of list */ }
>      },
> @@ -505,6 +517,10 @@ static QEMUMachine pc_machine_v0_13 = {
>              .driver   = "pc-sysfw",
>              .property = "rom_only",
>              .value    = stringify(1),
> +        }, {
> +            .driver   = "virtio-balloon-pci",
> +            .property = "class",
> +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
>          },
>          { /* end of list */ }
>      },
> @@ -565,6 +581,10 @@ static QEMUMachine pc_machine_v0_12 = {
>              .driver   = "pc-sysfw",
>              .property = "rom_only",
>              .value    = stringify(1),
> +        }, {
> +            .driver   = "virtio-balloon-pci",
> +            .property = "class",
> +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
>          },
>          { /* end of list */ }
>      }
> @@ -633,6 +653,10 @@ static QEMUMachine pc_machine_v0_11 = {
>              .driver   = "pc-sysfw",
>              .property = "rom_only",
>              .value    = stringify(1),
> +        }, {
> +            .driver   = "virtio-balloon-pci",
> +            .property = "class",
> +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
>          },
>          { /* end of list */ }
>      }
> @@ -713,6 +737,10 @@ static QEMUMachine pc_machine_v0_10 = {
>              .driver   = "pc-sysfw",
>              .property = "rom_only",
>              .value    = stringify(1),
> +        }, {
> +            .driver   = "virtio-balloon-pci",
> +            .property = "class",
> +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
>          },
>          { /* end of list */ }
>      },
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index a0fb7c1..a1fa656 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -790,6 +790,11 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
>      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
>      VirtIODevice *vdev;
>  
> +    if (proxy->class_code != PCI_CLASS_OTHERS &&
> +        proxy->class_code != PCI_CLASS_MEMORY_RAM) { /* qemu < 1.1 */
> +        proxy->class_code = PCI_CLASS_OTHERS;
> +    }
> +
>      vdev = virtio_balloon_init(&pci_dev->qdev);
>      if (!vdev) {
>          return -1;
> @@ -905,6 +910,7 @@ static TypeInfo virtio_serial_info = {
>  };
>  
>  static Property virtio_balloon_properties[] = {
> +    DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, PCI_CLASS_OTHERS),
>      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
>      DEFINE_PROP_END_OF_LIST(),
>  };
> @@ -919,7 +925,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;
>  }

-- 
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-devel] [PATCH] Remove PCI class code from virtio balloon device

[Michael S. Tsirkin]

This conflicts with deduplication of properties work.
I'll apply on top of that, so don't worry.

On Mon, Apr 02, 2012 at 12:43:06PM +1000, David Gibson wrote:
> Anthony..
> 
> please apply?
> 
> On Mon, Mar 26, 2012 at 12:19:40PM +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.  The backwards compatibility PC machines get new compat
> > properties so that they don't change.
> > 
> > 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/pc_piix.c    |   28 ++++++++++++++++++++++++++++
> >  hw/virtio-pci.c |    8 +++++++-
> >  2 files changed, 35 insertions(+), 1 deletions(-)
> > 
> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > index 3f99f9a..55dcd2e 100644
> > --- a/hw/pc_piix.c
> > +++ b/hw/pc_piix.c
> > @@ -386,6 +386,10 @@ static QEMUMachine pc_machine_v1_0 = {
> >              .driver   = "isa-fdc",
> >              .property = "check_media_rate",
> >              .value    = "off",
> > +        }, {
> > +            .driver   = "virtio-balloon-pci",
> > +            .property = "class",
> > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> >          },
> >          { /* end of list */ }
> >      },
> > @@ -405,6 +409,10 @@ static QEMUMachine pc_machine_v0_15 = {
> >              .driver   = "isa-fdc",
> >              .property = "check_media_rate",
> >              .value    = "off",
> > +        }, {
> > +            .driver   = "virtio-balloon-pci",
> > +            .property = "class",
> > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> >          },
> >          { /* end of list */ }
> >      },
> > @@ -449,6 +457,10 @@ static QEMUMachine pc_machine_v0_14 = {
> >              .driver   = "pc-sysfw",
> >              .property = "rom_only",
> >              .value    = stringify(1),
> > +        }, {
> > +            .driver   = "virtio-balloon-pci",
> > +            .property = "class",
> > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> >          },
> >          { /* end of list */ }
> >      },
> > @@ -505,6 +517,10 @@ static QEMUMachine pc_machine_v0_13 = {
> >              .driver   = "pc-sysfw",
> >              .property = "rom_only",
> >              .value    = stringify(1),
> > +        }, {
> > +            .driver   = "virtio-balloon-pci",
> > +            .property = "class",
> > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> >          },
> >          { /* end of list */ }
> >      },
> > @@ -565,6 +581,10 @@ static QEMUMachine pc_machine_v0_12 = {
> >              .driver   = "pc-sysfw",
> >              .property = "rom_only",
> >              .value    = stringify(1),
> > +        }, {
> > +            .driver   = "virtio-balloon-pci",
> > +            .property = "class",
> > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> >          },
> >          { /* end of list */ }
> >      }
> > @@ -633,6 +653,10 @@ static QEMUMachine pc_machine_v0_11 = {
> >              .driver   = "pc-sysfw",
> >              .property = "rom_only",
> >              .value    = stringify(1),
> > +        }, {
> > +            .driver   = "virtio-balloon-pci",
> > +            .property = "class",
> > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> >          },
> >          { /* end of list */ }
> >      }
> > @@ -713,6 +737,10 @@ static QEMUMachine pc_machine_v0_10 = {
> >              .driver   = "pc-sysfw",
> >              .property = "rom_only",
> >              .value    = stringify(1),
> > +        }, {
> > +            .driver   = "virtio-balloon-pci",
> > +            .property = "class",
> > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> >          },
> >          { /* end of list */ }
> >      },
> > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> > index a0fb7c1..a1fa656 100644
> > --- a/hw/virtio-pci.c
> > +++ b/hw/virtio-pci.c
> > @@ -790,6 +790,11 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
> >      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> >      VirtIODevice *vdev;
> >  
> > +    if (proxy->class_code != PCI_CLASS_OTHERS &&
> > +        proxy->class_code != PCI_CLASS_MEMORY_RAM) { /* qemu < 1.1 */
> > +        proxy->class_code = PCI_CLASS_OTHERS;
> > +    }
> > +
> >      vdev = virtio_balloon_init(&pci_dev->qdev);
> >      if (!vdev) {
> >          return -1;
> > @@ -905,6 +910,7 @@ static TypeInfo virtio_serial_info = {
> >  };
> >  
> >  static Property virtio_balloon_properties[] = {
> > +    DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, PCI_CLASS_OTHERS),
> >      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> > @@ -919,7 +925,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;
> >  }
> 
> -- 
> 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-devel] [PATCH] Remove PCI class code from virtio balloon device

[David Gibson]

On Mon, Apr 02, 2012 at 09:46:07AM +0300, Michael S. Tsirkin wrote:
> This conflicts with deduplication of properties work.
> I'll apply on top of that, so don't worry.

Alrighty.  Any ETA?

> 
> On Mon, Apr 02, 2012 at 12:43:06PM +1000, David Gibson wrote:
> > Anthony..
> > 
> > please apply?
> > 
> > On Mon, Mar 26, 2012 at 12:19:40PM +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.  The backwards compatibility PC machines get new compat
> > > properties so that they don't change.
> > > 
> > > 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/pc_piix.c    |   28 ++++++++++++++++++++++++++++
> > >  hw/virtio-pci.c |    8 +++++++-
> > >  2 files changed, 35 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > > index 3f99f9a..55dcd2e 100644
> > > --- a/hw/pc_piix.c
> > > +++ b/hw/pc_piix.c
> > > @@ -386,6 +386,10 @@ static QEMUMachine pc_machine_v1_0 = {
> > >              .driver   = "isa-fdc",
> > >              .property = "check_media_rate",
> > >              .value    = "off",
> > > +        }, {
> > > +            .driver   = "virtio-balloon-pci",
> > > +            .property = "class",
> > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > >          },
> > >          { /* end of list */ }
> > >      },
> > > @@ -405,6 +409,10 @@ static QEMUMachine pc_machine_v0_15 = {
> > >              .driver   = "isa-fdc",
> > >              .property = "check_media_rate",
> > >              .value    = "off",
> > > +        }, {
> > > +            .driver   = "virtio-balloon-pci",
> > > +            .property = "class",
> > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > >          },
> > >          { /* end of list */ }
> > >      },
> > > @@ -449,6 +457,10 @@ static QEMUMachine pc_machine_v0_14 = {
> > >              .driver   = "pc-sysfw",
> > >              .property = "rom_only",
> > >              .value    = stringify(1),
> > > +        }, {
> > > +            .driver   = "virtio-balloon-pci",
> > > +            .property = "class",
> > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > >          },
> > >          { /* end of list */ }
> > >      },
> > > @@ -505,6 +517,10 @@ static QEMUMachine pc_machine_v0_13 = {
> > >              .driver   = "pc-sysfw",
> > >              .property = "rom_only",
> > >              .value    = stringify(1),
> > > +        }, {
> > > +            .driver   = "virtio-balloon-pci",
> > > +            .property = "class",
> > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > >          },
> > >          { /* end of list */ }
> > >      },
> > > @@ -565,6 +581,10 @@ static QEMUMachine pc_machine_v0_12 = {
> > >              .driver   = "pc-sysfw",
> > >              .property = "rom_only",
> > >              .value    = stringify(1),
> > > +        }, {
> > > +            .driver   = "virtio-balloon-pci",
> > > +            .property = "class",
> > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > >          },
> > >          { /* end of list */ }
> > >      }
> > > @@ -633,6 +653,10 @@ static QEMUMachine pc_machine_v0_11 = {
> > >              .driver   = "pc-sysfw",
> > >              .property = "rom_only",
> > >              .value    = stringify(1),
> > > +        }, {
> > > +            .driver   = "virtio-balloon-pci",
> > > +            .property = "class",
> > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > >          },
> > >          { /* end of list */ }
> > >      }
> > > @@ -713,6 +737,10 @@ static QEMUMachine pc_machine_v0_10 = {
> > >              .driver   = "pc-sysfw",
> > >              .property = "rom_only",
> > >              .value    = stringify(1),
> > > +        }, {
> > > +            .driver   = "virtio-balloon-pci",
> > > +            .property = "class",
> > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > >          },
> > >          { /* end of list */ }
> > >      },
> > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> > > index a0fb7c1..a1fa656 100644
> > > --- a/hw/virtio-pci.c
> > > +++ b/hw/virtio-pci.c
> > > @@ -790,6 +790,11 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
> > >      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> > >      VirtIODevice *vdev;
> > >  
> > > +    if (proxy->class_code != PCI_CLASS_OTHERS &&
> > > +        proxy->class_code != PCI_CLASS_MEMORY_RAM) { /* qemu < 1.1 */
> > > +        proxy->class_code = PCI_CLASS_OTHERS;
> > > +    }
> > > +
> > >      vdev = virtio_balloon_init(&pci_dev->qdev);
> > >      if (!vdev) {
> > >          return -1;
> > > @@ -905,6 +910,7 @@ static TypeInfo virtio_serial_info = {
> > >  };
> > >  
> > >  static Property virtio_balloon_properties[] = {
> > > +    DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, PCI_CLASS_OTHERS),
> > >      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > > @@ -919,7 +925,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;
> > >  }
> > 
> 

-- 
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-devel] [PATCH] Remove PCI class code from virtio balloon device

[Michael S. Tsirkin]

On Mon, Apr 02, 2012 at 04:49:58PM +1000, David Gibson wrote:
> On Mon, Apr 02, 2012 at 09:46:07AM +0300, Michael S. Tsirkin wrote:
> > This conflicts with deduplication of properties work.
> > I'll apply on top of that, so don't worry.
> 
> Alrighty.  Any ETA?

I expect to send a pull request Tuesday.

> > 
> > On Mon, Apr 02, 2012 at 12:43:06PM +1000, David Gibson wrote:
> > > Anthony..
> > > 
> > > please apply?
> > > 
> > > On Mon, Mar 26, 2012 at 12:19:40PM +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.  The backwards compatibility PC machines get new compat
> > > > properties so that they don't change.
> > > > 
> > > > 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/pc_piix.c    |   28 ++++++++++++++++++++++++++++
> > > >  hw/virtio-pci.c |    8 +++++++-
> > > >  2 files changed, 35 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > > > index 3f99f9a..55dcd2e 100644
> > > > --- a/hw/pc_piix.c
> > > > +++ b/hw/pc_piix.c
> > > > @@ -386,6 +386,10 @@ static QEMUMachine pc_machine_v1_0 = {
> > > >              .driver   = "isa-fdc",
> > > >              .property = "check_media_rate",
> > > >              .value    = "off",
> > > > +        }, {
> > > > +            .driver   = "virtio-balloon-pci",
> > > > +            .property = "class",
> > > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > > >          },
> > > >          { /* end of list */ }
> > > >      },
> > > > @@ -405,6 +409,10 @@ static QEMUMachine pc_machine_v0_15 = {
> > > >              .driver   = "isa-fdc",
> > > >              .property = "check_media_rate",
> > > >              .value    = "off",
> > > > +        }, {
> > > > +            .driver   = "virtio-balloon-pci",
> > > > +            .property = "class",
> > > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > > >          },
> > > >          { /* end of list */ }
> > > >      },
> > > > @@ -449,6 +457,10 @@ static QEMUMachine pc_machine_v0_14 = {
> > > >              .driver   = "pc-sysfw",
> > > >              .property = "rom_only",
> > > >              .value    = stringify(1),
> > > > +        }, {
> > > > +            .driver   = "virtio-balloon-pci",
> > > > +            .property = "class",
> > > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > > >          },
> > > >          { /* end of list */ }
> > > >      },
> > > > @@ -505,6 +517,10 @@ static QEMUMachine pc_machine_v0_13 = {
> > > >              .driver   = "pc-sysfw",
> > > >              .property = "rom_only",
> > > >              .value    = stringify(1),
> > > > +        }, {
> > > > +            .driver   = "virtio-balloon-pci",
> > > > +            .property = "class",
> > > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > > >          },
> > > >          { /* end of list */ }
> > > >      },
> > > > @@ -565,6 +581,10 @@ static QEMUMachine pc_machine_v0_12 = {
> > > >              .driver   = "pc-sysfw",
> > > >              .property = "rom_only",
> > > >              .value    = stringify(1),
> > > > +        }, {
> > > > +            .driver   = "virtio-balloon-pci",
> > > > +            .property = "class",
> > > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > > >          },
> > > >          { /* end of list */ }
> > > >      }
> > > > @@ -633,6 +653,10 @@ static QEMUMachine pc_machine_v0_11 = {
> > > >              .driver   = "pc-sysfw",
> > > >              .property = "rom_only",
> > > >              .value    = stringify(1),
> > > > +        }, {
> > > > +            .driver   = "virtio-balloon-pci",
> > > > +            .property = "class",
> > > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > > >          },
> > > >          { /* end of list */ }
> > > >      }
> > > > @@ -713,6 +737,10 @@ static QEMUMachine pc_machine_v0_10 = {
> > > >              .driver   = "pc-sysfw",
> > > >              .property = "rom_only",
> > > >              .value    = stringify(1),
> > > > +        }, {
> > > > +            .driver   = "virtio-balloon-pci",
> > > > +            .property = "class",
> > > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > > >          },
> > > >          { /* end of list */ }
> > > >      },
> > > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> > > > index a0fb7c1..a1fa656 100644
> > > > --- a/hw/virtio-pci.c
> > > > +++ b/hw/virtio-pci.c
> > > > @@ -790,6 +790,11 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
> > > >      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> > > >      VirtIODevice *vdev;
> > > >  
> > > > +    if (proxy->class_code != PCI_CLASS_OTHERS &&
> > > > +        proxy->class_code != PCI_CLASS_MEMORY_RAM) { /* qemu < 1.1 */
> > > > +        proxy->class_code = PCI_CLASS_OTHERS;
> > > > +    }
> > > > +
> > > >      vdev = virtio_balloon_init(&pci_dev->qdev);
> > > >      if (!vdev) {
> > > >          return -1;
> > > > @@ -905,6 +910,7 @@ static TypeInfo virtio_serial_info = {
> > > >  };
> > > >  
> > > >  static Property virtio_balloon_properties[] = {
> > > > +    DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, PCI_CLASS_OTHERS),
> > > >      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
> > > >      DEFINE_PROP_END_OF_LIST(),
> > > >  };
> > > > @@ -919,7 +925,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;
> > > >  }
> > > 
> > 
> 
> -- 
> 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-devel] [PATCH] Remove PCI class code from virtio balloon device

[Michael S. Tsirkin]

On Mon, Apr 02, 2012 at 10:11:31AM +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 02, 2012 at 04:49:58PM +1000, David Gibson wrote:
> > On Mon, Apr 02, 2012 at 09:46:07AM +0300, Michael S. Tsirkin wrote:
> > > This conflicts with deduplication of properties work.
> > > I'll apply on top of that, so don't worry.
> > 
> > Alrighty.  Any ETA?
> 
> I expect to send a pull request Tuesday.

It's on my branch but Anthony did not yet push the
previous bits so sit tight a bit longer pls.

> > > 
> > > On Mon, Apr 02, 2012 at 12:43:06PM +1000, David Gibson wrote:
> > > > Anthony..
> > > > 
> > > > please apply?
> > > > 
> > > > On Mon, Mar 26, 2012 at 12:19:40PM +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.  The backwards compatibility PC machines get new compat
> > > > > properties so that they don't change.
> > > > > 
> > > > > 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/pc_piix.c    |   28 ++++++++++++++++++++++++++++
> > > > >  hw/virtio-pci.c |    8 +++++++-
> > > > >  2 files changed, 35 insertions(+), 1 deletions(-)
> > > > > 
> > > > > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > > > > index 3f99f9a..55dcd2e 100644
> > > > > --- a/hw/pc_piix.c
> > > > > +++ b/hw/pc_piix.c
> > > > > @@ -386,6 +386,10 @@ static QEMUMachine pc_machine_v1_0 = {
> > > > >              .driver   = "isa-fdc",
> > > > >              .property = "check_media_rate",
> > > > >              .value    = "off",
> > > > > +        }, {
> > > > > +            .driver   = "virtio-balloon-pci",
> > > > > +            .property = "class",
> > > > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > > > >          },
> > > > >          { /* end of list */ }
> > > > >      },
> > > > > @@ -405,6 +409,10 @@ static QEMUMachine pc_machine_v0_15 = {
> > > > >              .driver   = "isa-fdc",
> > > > >              .property = "check_media_rate",
> > > > >              .value    = "off",
> > > > > +        }, {
> > > > > +            .driver   = "virtio-balloon-pci",
> > > > > +            .property = "class",
> > > > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > > > >          },
> > > > >          { /* end of list */ }
> > > > >      },
> > > > > @@ -449,6 +457,10 @@ static QEMUMachine pc_machine_v0_14 = {
> > > > >              .driver   = "pc-sysfw",
> > > > >              .property = "rom_only",
> > > > >              .value    = stringify(1),
> > > > > +        }, {
> > > > > +            .driver   = "virtio-balloon-pci",
> > > > > +            .property = "class",
> > > > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > > > >          },
> > > > >          { /* end of list */ }
> > > > >      },
> > > > > @@ -505,6 +517,10 @@ static QEMUMachine pc_machine_v0_13 = {
> > > > >              .driver   = "pc-sysfw",
> > > > >              .property = "rom_only",
> > > > >              .value    = stringify(1),
> > > > > +        }, {
> > > > > +            .driver   = "virtio-balloon-pci",
> > > > > +            .property = "class",
> > > > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > > > >          },
> > > > >          { /* end of list */ }
> > > > >      },
> > > > > @@ -565,6 +581,10 @@ static QEMUMachine pc_machine_v0_12 = {
> > > > >              .driver   = "pc-sysfw",
> > > > >              .property = "rom_only",
> > > > >              .value    = stringify(1),
> > > > > +        }, {
> > > > > +            .driver   = "virtio-balloon-pci",
> > > > > +            .property = "class",
> > > > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > > > >          },
> > > > >          { /* end of list */ }
> > > > >      }
> > > > > @@ -633,6 +653,10 @@ static QEMUMachine pc_machine_v0_11 = {
> > > > >              .driver   = "pc-sysfw",
> > > > >              .property = "rom_only",
> > > > >              .value    = stringify(1),
> > > > > +        }, {
> > > > > +            .driver   = "virtio-balloon-pci",
> > > > > +            .property = "class",
> > > > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > > > >          },
> > > > >          { /* end of list */ }
> > > > >      }
> > > > > @@ -713,6 +737,10 @@ static QEMUMachine pc_machine_v0_10 = {
> > > > >              .driver   = "pc-sysfw",
> > > > >              .property = "rom_only",
> > > > >              .value    = stringify(1),
> > > > > +        }, {
> > > > > +            .driver   = "virtio-balloon-pci",
> > > > > +            .property = "class",
> > > > > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> > > > >          },
> > > > >          { /* end of list */ }
> > > > >      },
> > > > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> > > > > index a0fb7c1..a1fa656 100644
> > > > > --- a/hw/virtio-pci.c
> > > > > +++ b/hw/virtio-pci.c
> > > > > @@ -790,6 +790,11 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
> > > > >      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> > > > >      VirtIODevice *vdev;
> > > > >  
> > > > > +    if (proxy->class_code != PCI_CLASS_OTHERS &&
> > > > > +        proxy->class_code != PCI_CLASS_MEMORY_RAM) { /* qemu < 1.1 */
> > > > > +        proxy->class_code = PCI_CLASS_OTHERS;
> > > > > +    }
> > > > > +
> > > > >      vdev = virtio_balloon_init(&pci_dev->qdev);
> > > > >      if (!vdev) {
> > > > >          return -1;
> > > > > @@ -905,6 +910,7 @@ static TypeInfo virtio_serial_info = {
> > > > >  };
> > > > >  
> > > > >  static Property virtio_balloon_properties[] = {
> > > > > +    DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, PCI_CLASS_OTHERS),
> > > > >      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
> > > > >      DEFINE_PROP_END_OF_LIST(),
> > > > >  };
> > > > > @@ -919,7 +925,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;
> > > > >  }
> > > > 
> > > 
> > 
> > -- 
> > 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

[Qemu-devel] [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.  The backwards compatibility PC machines get new compat
properties so that they don't change.

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/pc_piix.c    |   24 ++++++++++++++++++++++++
 hw/virtio-pci.c |    7 ++++++-
 2 files changed, 30 insertions(+), 1 deletions(-)


diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 3f99f9a..72a4250 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -386,6 +386,10 @@ static QEMUMachine pc_machine_v1_0 = {
             .driver   = "isa-fdc",
             .property = "check_media_rate",
             .value    = "off",
+        }, {
+            .driver   = "virtio-balloon-pci",
+            .property = "class",
+            .value    = stringify(PCI_CLASS_MEMORY_RAM),
         },
         { /* end of list */ }
     },
@@ -449,6 +453,10 @@ static QEMUMachine pc_machine_v0_14 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        }, {
+            .driver   = "virtio-balloon-pci",
+            .property = "class",
+            .value    = stringify(PCI_CLASS_MEMORY_RAM),
         },
         { /* end of list */ }
     },
@@ -505,6 +513,10 @@ static QEMUMachine pc_machine_v0_13 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        }, {
+            .driver   = "virtio-balloon-pci",
+            .property = "class",
+            .value    = stringify(PCI_CLASS_MEMORY_RAM),
         },
         { /* end of list */ }
     },
@@ -565,6 +577,10 @@ static QEMUMachine pc_machine_v0_12 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        }, {
+            .driver   = "virtio-balloon-pci",
+            .property = "class",
+            .value    = stringify(PCI_CLASS_MEMORY_RAM),
         },
         { /* end of list */ }
     }
@@ -633,6 +649,10 @@ static QEMUMachine pc_machine_v0_11 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        }, {
+            .driver   = "virtio-balloon-pci",
+            .property = "class",
+            .value    = stringify(PCI_CLASS_MEMORY_RAM),
         },
         { /* end of list */ }
     }
@@ -713,6 +733,10 @@ static QEMUMachine pc_machine_v0_10 = {
             .driver   = "pc-sysfw",
             .property = "rom_only",
             .value    = stringify(1),
+        }, {
+            .driver   = "virtio-balloon-pci",
+            .property = "class",
+            .value    = stringify(PCI_CLASS_MEMORY_RAM),
         },
         { /* end of list */ }
     },
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index a0fb7c1..1fd5768 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -790,6 +790,10 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
     VirtIODevice *vdev;
 
+    if (proxy->class_code != PCI_CLASS_OTHERS &&
+        proxy->class_code != PCI_CLASS_MEMORY_RAM) /* qemu < 1.1 */
+        proxy->class_code = PCI_CLASS_OTHERS;
+
     vdev = virtio_balloon_init(&pci_dev->qdev);
     if (!vdev) {
         return -1;
@@ -905,6 +909,7 @@ static TypeInfo virtio_serial_info = {
 };
 
 static Property virtio_balloon_properties[] = {
+    DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, PCI_CLASS_OTHERS),
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -919,7 +924,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-devel] [PATCH] Remove PCI class code from virtio balloon device

[Stefan Hajnoczi]

On Thu, Mar 22, 2012 at 9:09 AM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 3f99f9a..72a4250 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -386,6 +386,10 @@ static QEMUMachine pc_machine_v1_0 = {
>             .driver   = "isa-fdc",
>             .property = "check_media_rate",
>             .value    = "off",
> +        }, {
> +            .driver   = "virtio-balloon-pci",
> +            .property = "class",
> +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
>         },
>         { /* end of list */ }
>     },
> @@ -449,6 +453,10 @@ static QEMUMachine pc_machine_v0_14 = {

pc_machine_v0_15 should also use PCI_CLASS_MEMORY_RAM?  Did you skip
it on purpose?

> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index a0fb7c1..1fd5768 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -790,6 +790,10 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
>     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
>     VirtIODevice *vdev;
>
> +    if (proxy->class_code != PCI_CLASS_OTHERS &&
> +        proxy->class_code != PCI_CLASS_MEMORY_RAM) /* qemu < 1.1 */
> +        proxy->class_code = PCI_CLASS_OTHERS;
> +

Why is this hunk is needed?

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

[Gerd Hoffmann]

>> +    if (proxy->class_code != PCI_CLASS_OTHERS &&
>> +        proxy->class_code != PCI_CLASS_MEMORY_RAM) /* qemu < 1.1 */
>> +        proxy->class_code = PCI_CLASS_OTHERS;
>> +
> 
> Why is this hunk is needed?

Catch users doing -device virtio-balloon,class=42

cheers,
  Gerd

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

[Stefan Hajnoczi]

On Thu, Mar 22, 2012 at 10:27 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>> +    if (proxy->class_code != PCI_CLASS_OTHERS &&
>>> +        proxy->class_code != PCI_CLASS_MEMORY_RAM) /* qemu < 1.1 */
>>> +        proxy->class_code = PCI_CLASS_OTHERS;
>>> +
>>
>> Why is this hunk is needed?
>
> Catch users doing -device virtio-balloon,class=42

I see that the other virtio devices that have a class property also do
this.  Sorry, my bad.

Stefan

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

[David Gibson]

On Thu, Mar 22, 2012 at 10:01:46AM +0000, Stefan Hajnoczi wrote:
> On Thu, Mar 22, 2012 at 9:09 AM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > index 3f99f9a..72a4250 100644
> > --- a/hw/pc_piix.c
> > +++ b/hw/pc_piix.c
> > @@ -386,6 +386,10 @@ static QEMUMachine pc_machine_v1_0 = {
> >             .driver   = "isa-fdc",
> >             .property = "check_media_rate",
> >             .value    = "off",
> > +        }, {
> > +            .driver   = "virtio-balloon-pci",
> > +            .property = "class",
> > +            .value    = stringify(PCI_CLASS_MEMORY_RAM),
> >         },
> >         { /* end of list */ }
> >     },
> > @@ -449,6 +453,10 @@ static QEMUMachine pc_machine_v0_14 = {
> 
> pc_machine_v0_15 should also use PCI_CLASS_MEMORY_RAM?  Did you skip
> it on purpose?

Nope, just missed it.  I'll respin.

-- 
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-devel] [PATCH] Remove PCI class code from virtio balloon device

[Michael S. Tsirkin]

On Thu, Mar 22, 2012 at 08:09:27PM +1100, David Gibson wrote:
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index a0fb7c1..1fd5768 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -790,6 +790,10 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
>      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
>      VirtIODevice *vdev;
>  
> +    if (proxy->class_code != PCI_CLASS_OTHERS &&
> +        proxy->class_code != PCI_CLASS_MEMORY_RAM) /* qemu < 1.1 */
> +        proxy->class_code = PCI_CLASS_OTHERS;
> +

{} around if.

>      vdev = virtio_balloon_init(&pci_dev->qdev);
>      if (!vdev) {
>          return -1;
> @@ -905,6 +909,7 @@ static TypeInfo virtio_serial_info = {
>  };
>  
>  static Property virtio_balloon_properties[] = {
> +    DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, PCI_CLASS_OTHERS),
>      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
>      DEFINE_PROP_END_OF_LIST(),
>  };

Sorry to bug you - why not set
 +  DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),

and then

 +    if (!proxy->class_code) {
 +        proxy->class_code = PCI_CLASS_OTHERS;
 +    }

This is what other devices do.

> @@ -919,7 +924,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-devel] [PATCH] Remove PCI class code from virtio balloon device

[David Gibson]

On Thu, Mar 22, 2012 at 01:53:33PM +0200, Michael S. Tsirkin wrote:
> On Thu, Mar 22, 2012 at 08:09:27PM +1100, David Gibson wrote:
> > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> > index a0fb7c1..1fd5768 100644
> > --- a/hw/virtio-pci.c
> > +++ b/hw/virtio-pci.c
> > @@ -790,6 +790,10 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
> >      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> >      VirtIODevice *vdev;
> >  
> > +    if (proxy->class_code != PCI_CLASS_OTHERS &&
> > +        proxy->class_code != PCI_CLASS_MEMORY_RAM) /* qemu < 1.1 */
> > +        proxy->class_code = PCI_CLASS_OTHERS;
> > +
> 
> {} around if.

Fixed.  I was copying the bad example from the other virtio pci
devices.


> >      vdev = virtio_balloon_init(&pci_dev->qdev);
> >      if (!vdev) {
> >          return -1;
> > @@ -905,6 +909,7 @@ static TypeInfo virtio_serial_info = {
> >  };
> >  
> >  static Property virtio_balloon_properties[] = {
> > +    DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, PCI_CLASS_OTHERS),
> >      DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> 
> Sorry to bug you - why not set
>  +  DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
> 
> and then
> 
>  +    if (!proxy->class_code) {
>  +        proxy->class_code = PCI_CLASS_OTHERS;
>  +    }
> 
> This is what other devices do.

There is no fragment of code quite like the one you quote, only the
check for valid class values, which will accomplish the same thing.
It seemed clearer to have the default class value in the property
definition be, well, the default class value, rather than setting it
to 0 and having it overwritten.

-- 
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-devel] [PATCH] Remove PCI class code from virtio balloon device

[Anthony Liguori]

On 03/22/2012 08:52 PM, David Gibson wrote:
> There is no fragment of code quite like the one you quote, only the
> check for valid class values, which will accomplish the same thing.
> It seemed clearer to have the default class value in the property
> definition be, well, the default class value, rather than setting it
> to 0 and having it overwritten.

Agreed.

Regards,

Anthony Liguori

>

[Qemu-devel] [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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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.

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

[Michael S. Tsirkin]

On Tue, Mar 20, 2012 at 11:42:06AM +1100, David Gibson 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.

Talked with windows driver guys. It seems their driver does not
check the class code, but one thing that will happen
if this is changed across migration, is that
on bus rescan, windows will detect a change and driver will
get re-installed. For the baloon, this will give all memory
back to guest, which seems undesirable.

So I think keeping the old class for old machine types is
the prudent thing to do.

I also removed Cc qemu-trivial - it's a short but non-trivial patch.

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

[Qemu-devel] [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 |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index a0fb7c1..3c3907a 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -919,7 +919,6 @@ 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;
     dc->reset = virtio_pci_reset;
     dc->props = virtio_balloon_properties;
 }
-- 
1.7.9.1

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

[Michael S. Tsirkin]

On Fri, Mar 16, 2012 at 12:03:08PM +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>

Well, this gives the class a legacy value 00,
and the PCI spec says we should not use this:

D.1.
Base Class 00h
This base class is defined to provide backward compatibility for devices
that were built
before the Class Code field was defined. No new devices should use this
value and existing
devices should switch to a more appropriate value if possible.
For class codes with this base class value, there are two defined values
for the remaining
fields as shown in the table below. All other values are reserved.
Base Class
00h
Sub-Class
Interface
00h
01h
VGA-compatible device
00h
00h
All currently implemented devices
except VGA-compatible devices

You probably want this instead:

#define PCI_CLASS_OTHERS		0xff


> ---
>  hw/virtio-pci.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index a0fb7c1..3c3907a 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -919,7 +919,6 @@ 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;
>      dc->reset = virtio_pci_reset;
>      dc->props = virtio_balloon_properties;
>  }
> -- 
> 1.7.9.1

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

[David Gibson]

On Sun, Mar 18, 2012 at 02:38:42PM +0200, Michael S. Tsirkin wrote:
> On Fri, Mar 16, 2012 at 12:03:08PM +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>
> 
> Well, this gives the class a legacy value 00,
> and the PCI spec says we should not use this:
> 
> D.1.
> Base Class 00h
> This base class is defined to provide backward compatibility for devices
> that were built
> before the Class Code field was defined. No new devices should use this
> value and existing
> devices should switch to a more appropriate value if possible.
> For class codes with this base class value, there are two defined values
> for the remaining
> fields as shown in the table below. All other values are reserved.
> Base Class
> 00h
> Sub-Class
> Interface
> 00h
> 01h
> VGA-compatible device
> 00h
> 00h
> All currently implemented devices
> except VGA-compatible devices
> 
> You probably want this instead:
> 
> #define PCI_CLASS_OTHERS		0xff

Ah, thanks.  I had mistakenly assumed that 0 meant "no class
specified".  I'll respin changing to PCI_CLASS_OTHERS.

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