[PATCH v2 0/3] Fix crash when adding a second ISA VGA device

[PATCH v2 0/3] Fix crash when adding a second ISA VGA device

[Thomas Huth]

QEMU currently abort()s if the user tries to add a second ISA VGA
device, for example:

$ ./qemu-system-x86_64 -device isa-vga -device isa-vga
RAMBlock "vga.vram" already registered, abort!
Aborted (core dumped)
$ ./qemu-system-x86_64 -device isa-cirrus-vga -device isa-cirrus-vga
RAMBlock "vga.vram" already registered, abort!
Aborted (core dumped)

Such a crash should never happen just because of giving bad parameters
at the command line, we should give a proper error message instead
and exit gracefully.

Note: There have been previous attempts to fix this problem, but the
first committed solution had bad side effects and got reverted
(https://gitlab.com/qemu-project/qemu/-/issues/733). There was another
idea to fix it by QOM'ifying the related devices (see the commits around
23f6e3b11be74abae), but after having another close look at the problem,
I think this doesn't work either: For getting unique names in the
vmstate_register_ram() function, the devices need to return unique names
from the qdev_get_dev_path() function, and those ISA VGA devices don't
support that there (unlike PCI, ISA devices don't have a slot id ...
they could be distinguished by their I/O port base address, but all the
ISA VGA cards currently use the same address there, so that doesn't
work either). ==> So the very original idea of checking for the availability
of the "vga.vram" memory region still seems the only usable approach to
me right now. While the original patch by Jose R. Ziviani only fixed the
issue for the isa-vga device, I'm taking a more general approach now by
adding the fix in the vga_common_init() function, so that it works for
the isa-cirrus-vga device, too.

v2: Rework error handling in 2nd patch, vga_common_init() returns bool now

Thomas Huth (3):
  hw/display/cirrus_vga: Clean up indentation in
    pci_cirrus_vga_realize()
  hw/display: Allow vga_common_init() to return errors
  hw/display/vga: Report a proper error when adding a 2nd ISA VGA

 hw/display/vga_int.h        |  2 +-
 hw/display/ati.c            |  4 ++-
 hw/display/cirrus_vga.c     | 59 +++++++++++++++++++------------------
 hw/display/cirrus_vga_isa.c |  4 ++-
 hw/display/qxl.c            |  6 +++-
 hw/display/vga-isa.c        |  5 +++-
 hw/display/vga-mmio.c       |  5 +++-
 hw/display/vga-pci.c        |  8 +++--
 hw/display/vga.c            | 17 +++++++++--
 hw/display/virtio-vga.c     |  4 ++-
 hw/display/vmware_vga.c     |  2 +-
 11 files changed, 76 insertions(+), 40 deletions(-)

-- 
2.27.0

[PATCH v2 1/3] hw/display/cirrus_vga: Clean up indentation in pci_cirrus_vga_realize()

[Thomas Huth]

Most of the code in this function had been indented with 5 spaces instead
of 4. Since 4 is our preferred style, remove one space in the bad lines here.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/display/cirrus_vga.c | 57 +++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index fdca6ca659..7da1be3e12 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -2940,27 +2940,28 @@ void cirrus_init_common(CirrusVGAState *s, Object *owner,
 
 static void pci_cirrus_vga_realize(PCIDevice *dev, Error **errp)
 {
-     PCICirrusVGAState *d = PCI_CIRRUS_VGA(dev);
-     CirrusVGAState *s = &d->cirrus_vga;
-     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
-     int16_t device_id = pc->device_id;
-
-     /* follow real hardware, cirrus card emulated has 4 MB video memory.
-       Also accept 8 MB/16 MB for backward compatibility. */
-     if (s->vga.vram_size_mb != 4 && s->vga.vram_size_mb != 8 &&
-         s->vga.vram_size_mb != 16) {
-         error_setg(errp, "Invalid cirrus_vga ram size '%u'",
-                    s->vga.vram_size_mb);
-         return;
-     }
-     /* setup VGA */
-     vga_common_init(&s->vga, OBJECT(dev));
-     cirrus_init_common(s, OBJECT(dev), device_id, 1, pci_address_space(dev),
-                        pci_address_space_io(dev));
-     s->vga.con = graphic_console_init(DEVICE(dev), 0, s->vga.hw_ops, &s->vga);
-
-     /* setup PCI */
+    PCICirrusVGAState *d = PCI_CIRRUS_VGA(dev);
+    CirrusVGAState *s = &d->cirrus_vga;
+    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
+    int16_t device_id = pc->device_id;
+
+    /*
+     * Follow real hardware, cirrus card emulated has 4 MB video memory.
+     * Also accept 8 MB/16 MB for backward compatibility.
+     */
+    if (s->vga.vram_size_mb != 4 && s->vga.vram_size_mb != 8 &&
+        s->vga.vram_size_mb != 16) {
+        error_setg(errp, "Invalid cirrus_vga ram size '%u'",
+                   s->vga.vram_size_mb);
+        return;
+    }
+    /* setup VGA */
+    vga_common_init(&s->vga, OBJECT(dev));
+    cirrus_init_common(s, OBJECT(dev), device_id, 1, pci_address_space(dev),
+                       pci_address_space_io(dev));
+    s->vga.con = graphic_console_init(DEVICE(dev), 0, s->vga.hw_ops, &s->vga);
 
+    /* setup PCI */
     memory_region_init(&s->pci_bar, OBJECT(dev), "cirrus-pci-bar0", 0x2000000);
 
     /* XXX: add byte swapping apertures */
@@ -2968,14 +2969,14 @@ static void pci_cirrus_vga_realize(PCIDevice *dev, Error **errp)
     memory_region_add_subregion(&s->pci_bar, 0x1000000,
                                 &s->cirrus_linear_bitblt_io);
 
-     /* setup memory space */
-     /* memory #0 LFB */
-     /* memory #1 memory-mapped I/O */
-     /* XXX: s->vga.vram_size must be a power of two */
-     pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->pci_bar);
-     if (device_id == CIRRUS_ID_CLGD5446) {
-         pci_register_bar(&d->dev, 1, 0, &s->cirrus_mmio_io);
-     }
+    /* setup memory space */
+    /* memory #0 LFB */
+    /* memory #1 memory-mapped I/O */
+    /* XXX: s->vga.vram_size must be a power of two */
+    pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->pci_bar);
+    if (device_id == CIRRUS_ID_CLGD5446) {
+        pci_register_bar(&d->dev, 1, 0, &s->cirrus_mmio_io);
+    }
 }
 
 static Property pci_vga_cirrus_properties[] = {
-- 
2.27.0

[PATCH v2 2/3] hw/display: Allow vga_common_init() to return errors

[Thomas Huth]

The vga_common_init() function currently cannot report errors to its
caller. But in the following patch, we'd need this possibility, so
let's change it to take an "Error **" as parameter for this.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/display/vga_int.h        |  2 +-
 hw/display/ati.c            |  4 +++-
 hw/display/cirrus_vga.c     |  4 +++-
 hw/display/cirrus_vga_isa.c |  4 +++-
 hw/display/qxl.c            |  6 +++++-
 hw/display/vga-isa.c        |  5 ++++-
 hw/display/vga-mmio.c       |  5 ++++-
 hw/display/vga-pci.c        |  8 ++++++--
 hw/display/vga.c            | 11 +++++++++--
 hw/display/virtio-vga.c     |  4 +++-
 hw/display/vmware_vga.c     |  2 +-
 11 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 847e784ca6..305e700014 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -156,7 +156,7 @@ static inline int c6_to_8(int v)
     return (v << 2) | (b << 1) | b;
 }
 
-void vga_common_init(VGACommonState *s, Object *obj);
+bool vga_common_init(VGACommonState *s, Object *obj, Error **errp);
 void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
               MemoryRegion *address_space_io, bool init_vga_ports);
 MemoryRegion *vga_init_io(VGACommonState *s, Object *obj,
diff --git a/hw/display/ati.c b/hw/display/ati.c
index 31f22754dc..6e38e00502 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -955,7 +955,9 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
     }
 
     /* init vga bits */
-    vga_common_init(vga, OBJECT(s));
+    if (!vga_common_init(vga, OBJECT(s), errp)) {
+        return;
+    }
     vga_init(vga, OBJECT(s), pci_address_space(dev),
              pci_address_space_io(dev), true);
     vga->con = graphic_console_init(DEVICE(s), 0, s->vga.hw_ops, &s->vga);
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 7da1be3e12..3bb6a58698 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -2956,7 +2956,9 @@ static void pci_cirrus_vga_realize(PCIDevice *dev, Error **errp)
         return;
     }
     /* setup VGA */
-    vga_common_init(&s->vga, OBJECT(dev));
+    if (!vga_common_init(&s->vga, OBJECT(dev), errp)) {
+        return;
+    }
     cirrus_init_common(s, OBJECT(dev), device_id, 1, pci_address_space(dev),
                        pci_address_space_io(dev));
     s->vga.con = graphic_console_init(DEVICE(dev), 0, s->vga.hw_ops, &s->vga);
diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
index 4f6fb1af3b..96144bd690 100644
--- a/hw/display/cirrus_vga_isa.c
+++ b/hw/display/cirrus_vga_isa.c
@@ -56,7 +56,9 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev, Error **errp)
         return;
     }
     s->global_vmstate = true;
-    vga_common_init(s, OBJECT(dev));
+    if (!vga_common_init(s, OBJECT(dev), errp)) {
+        return;
+    }
     cirrus_init_common(&d->cirrus_vga, OBJECT(dev), CIRRUS_ID_CLGD5430, 0,
                        isa_address_space(isadev),
                        isa_address_space_io(isadev));
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 1f9ad31943..adbdbcaeb6 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2215,7 +2215,11 @@ static void qxl_realize_primary(PCIDevice *dev, Error **errp)
     qxl_init_ramsize(qxl);
     vga->vbe_size = qxl->vgamem_size;
     vga->vram_size_mb = qxl->vga.vram_size / MiB;
-    vga_common_init(vga, OBJECT(dev));
+    vga_common_init(vga, OBJECT(dev), &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
     vga_init(vga, OBJECT(dev),
              pci_address_space(dev), pci_address_space_io(dev), false);
     portio_list_init(&qxl->vga_port_list, OBJECT(dev), qxl_vga_portio_list,
diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 90851e730b..46abbc5653 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -62,7 +62,10 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
     const MemoryRegionPortio *vga_ports, *vbe_ports;
 
     s->global_vmstate = true;
-    vga_common_init(s, OBJECT(dev));
+    if (!vga_common_init(s, OBJECT(dev), errp)) {
+        return;
+    }
+
     s->legacy_address_space = isa_address_space(isadev);
     vga_io_memory = vga_init_io(s, OBJECT(dev), &vga_ports, &vbe_ports);
     isa_register_portio_list(isadev, &d->portio_vga,
diff --git a/hw/display/vga-mmio.c b/hw/display/vga-mmio.c
index 4969368081..75dfcedea5 100644
--- a/hw/display/vga-mmio.c
+++ b/hw/display/vga-mmio.c
@@ -102,7 +102,10 @@ static void vga_mmio_realizefn(DeviceState *dev, Error **errp)
 
     s->vga.bank_offset = 0;
     s->vga.global_vmstate = true;
-    vga_common_init(&s->vga, OBJECT(dev));
+    if (!vga_common_init(&s->vga, OBJECT(dev), errp)) {
+        return;
+    }
+
     sysbus_init_mmio(sbd, &s->vga.vram);
     s->vga.con = graphic_console_init(dev, 0, s->vga.hw_ops, &s->vga);
 }
diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
index 62fb5c38c1..3e5bc259f7 100644
--- a/hw/display/vga-pci.c
+++ b/hw/display/vga-pci.c
@@ -239,7 +239,9 @@ static void pci_std_vga_realize(PCIDevice *dev, Error **errp)
     bool edid = false;
 
     /* vga + console init */
-    vga_common_init(s, OBJECT(dev));
+    if (!vga_common_init(s, OBJECT(dev), errp)) {
+        return;
+    }
     vga_init(s, OBJECT(dev), pci_address_space(dev), pci_address_space_io(dev),
              true);
 
@@ -275,7 +277,9 @@ static void pci_secondary_vga_realize(PCIDevice *dev, Error **errp)
     bool edid = false;
 
     /* vga + console init */
-    vga_common_init(s, OBJECT(dev));
+    if (!vga_common_init(s, OBJECT(dev), errp)) {
+        return;
+    }
     s->con = graphic_console_init(DEVICE(dev), 0, s->hw_ops, s);
 
     /* mmio bar */
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 9d1f66af40..ae96023596 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2168,9 +2168,10 @@ static inline uint32_t uint_clamp(uint32_t val, uint32_t vmin, uint32_t vmax)
     return val;
 }
 
-void vga_common_init(VGACommonState *s, Object *obj)
+bool vga_common_init(VGACommonState *s, Object *obj, Error **errp)
 {
     int i, j, v, b;
+    Error *local_err = NULL;
 
     for(i = 0;i < 256; i++) {
         v = 0;
@@ -2206,7 +2207,11 @@ void vga_common_init(VGACommonState *s, Object *obj)
 
     s->is_vbe_vmstate = 1;
     memory_region_init_ram_nomigrate(&s->vram, obj, "vga.vram", s->vram_size,
-                           &error_fatal);
+                                     &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return false;
+    }
     vmstate_register_ram(&s->vram, s->global_vmstate ? NULL : DEVICE(obj));
     xen_register_framebuffer(&s->vram);
     s->vram_ptr = memory_region_get_ram_ptr(&s->vram);
@@ -2237,6 +2242,8 @@ void vga_common_init(VGACommonState *s, Object *obj)
     s->default_endian_fb = false;
 #endif
     vga_dirty_log_start(s);
+
+    return true;
 }
 
 static const MemoryRegionPortio vga_portio_list[] = {
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index 5a2f7a4540..7b55c8d0e7 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -108,7 +108,9 @@ static void virtio_vga_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
 
     /* init vga compat bits */
     vga->vram_size_mb = 8;
-    vga_common_init(vga, OBJECT(vpci_dev));
+    if (!vga_common_init(vga, OBJECT(vpci_dev), errp)) {
+        return;
+    }
     vga_init(vga, OBJECT(vpci_dev), pci_address_space(&vpci_dev->pci_dev),
              pci_address_space_io(&vpci_dev->pci_dev), true);
     pci_register_bar(&vpci_dev->pci_dev, 0,
diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 0cc43a1f15..98c83474ad 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1254,7 +1254,7 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s,
                            &error_fatal);
     s->fifo_ptr = memory_region_get_ram_ptr(&s->fifo_ram);
 
-    vga_common_init(&s->vga, OBJECT(dev));
+    vga_common_init(&s->vga, OBJECT(dev), &error_fatal);
     vga_init(&s->vga, OBJECT(dev), address_space, io, true);
     vmstate_register(NULL, 0, &vmstate_vga_common, &s->vga);
     s->new_depth = 32;
-- 
2.27.0

[PATCH v2 3/3] hw/display/vga: Report a proper error when adding a 2nd ISA VGA

[Thomas Huth]

QEMU currently abort()s if the user tries to add a second ISA VGA
device, for example:

$ ./qemu-system-x86_64 -device isa-vga -device isa-vga
RAMBlock "vga.vram" already registered, abort!
Aborted (core dumped)
$ ./qemu-system-x86_64 -device isa-cirrus-vga -device isa-cirrus-vga
RAMBlock "vga.vram" already registered, abort!
Aborted (core dumped)
$ ./qemu-system-mips64el -M pica61 -device isa-vga
RAMBlock "vga.vram" already registered, abort!
Aborted (core dumped)

Such a crash should never happen just because of giving bad parameters
at the command line. Let's return a proper error message instead.
(The idea is based on an original patch by Jose R. Ziviani for the
isa-vga device, but this now fixes it for the isa-cirrus-vga device, too)

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/44
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/display/vga.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index ae96023596..a7a291fa20 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2206,6 +2206,12 @@ bool vga_common_init(VGACommonState *s, Object *obj, Error **errp)
     s->vbe_size_mask = s->vbe_size - 1;
 
     s->is_vbe_vmstate = 1;
+
+    if (s->global_vmstate && qemu_ram_block_by_name("vga.vram")) {
+        error_setg(errp, "Only one global VGA device can be used at a time");
+        return false;
+    }
+
     memory_region_init_ram_nomigrate(&s->vram, obj, "vga.vram", s->vram_size,
                                      &local_err);
     if (local_err) {
-- 
2.27.0

Re: [PATCH v2 2/3] hw/display: Allow vga_common_init() to return errors

[Philippe Mathieu-Daudé]

On 17/3/22 09:30, Thomas Huth wrote:
> The vga_common_init() function currently cannot report errors to its
> caller. But in the following patch, we'd need this possibility, so
> let's change it to take an "Error **" as parameter for this.

Thanks for updating to return a boolean.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/display/vga_int.h        |  2 +-
>   hw/display/ati.c            |  4 +++-
>   hw/display/cirrus_vga.c     |  4 +++-
>   hw/display/cirrus_vga_isa.c |  4 +++-
>   hw/display/qxl.c            |  6 +++++-
>   hw/display/vga-isa.c        |  5 ++++-
>   hw/display/vga-mmio.c       |  5 ++++-
>   hw/display/vga-pci.c        |  8 ++++++--
>   hw/display/vga.c            | 11 +++++++++--
>   hw/display/virtio-vga.c     |  4 +++-
>   hw/display/vmware_vga.c     |  2 +-
>   11 files changed, 42 insertions(+), 13 deletions(-)