| Message ID | 1332407367-26551-1-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
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?
>> + 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
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
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.
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
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.
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 >
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; }
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(-)