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

["Michael S. Tsirkin" <mst@redhat.com>]

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