[PATCH-for-6.2 0/2] hw/display: Do not allow multiple (identical) VGA devices

[PATCH-for-6.2 0/2] hw/display: Do not allow multiple (identical) VGA devices

[Philippe Mathieu-Daudé]

Commit 7852a77f598 fixed creating multiple TYPE_ISA_VGA devices,
generalize the fix to all VGA devices.

See https://gitlab.com/qemu-project/qemu/-/issues/44

Philippe Mathieu-Daudé (2):
  hw/display: Add Error* handle to vga_common_init()
  hw/display: Do not allow multiple identical VGA devices

 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            |  4 +++-
 hw/display/vga-isa-mm.c     |  3 ++-
 hw/display/vga-isa.c        | 11 ++---------
 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, 43 insertions(+), 20 deletions(-)

-- 
2.31.1

[PATCH-for-6.2 1/2] hw/display: Add Error* handle to vga_common_init()

[Philippe Mathieu-Daudé]

We want vga_common_init() to fail gracefully,
therefore allow it to take an Error* handle.

Signed-off-by: Philippe Mathieu-Daudé <philmd@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            | 4 +++-
 hw/display/vga-isa-mm.c     | 3 ++-
 hw/display/vga-isa.c        | 4 +++-
 hw/display/vga-pci.c        | 8 ++++++--
 hw/display/vga.c            | 4 +++-
 hw/display/virtio-vga.c     | 4 +++-
 hw/display/vmware_vga.c     | 2 +-
 11 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 847e784ca6a..305e700014d 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 31f22754dce..6e38e005022 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 fdca6ca659f..5a7ddb84ff0 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -2954,7 +2954,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 4f6fb1af3bd..96144bd6906 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 29c80b4289b..16eb98c25d8 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2198,7 +2198,9 @@ 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));
+    if (!vga_common_init(vga, OBJECT(dev), errp)) {
+        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-mm.c b/hw/display/vga-isa-mm.c
index 7321b7a06d5..420da1d7467 100644
--- a/hw/display/vga-isa-mm.c
+++ b/hw/display/vga-isa-mm.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/bitops.h"
 #include "qemu/units.h"
+#include "qapi/error.h"
 #include "migration/vmstate.h"
 #include "hw/display/vga.h"
 #include "vga_int.h"
@@ -101,7 +102,7 @@ int isa_vga_mm_init(hwaddr vram_base,
 
     s->vga.vram_size_mb = VGA_RAM_SIZE / MiB;
     s->vga.global_vmstate = true;
-    vga_common_init(&s->vga, NULL);
+    vga_common_init(&s->vga, NULL, &error_fatal);
     vga_mm_init(s, vram_base, ctrl_base, it_shift, address_space);
 
     s->vga.con = graphic_console_init(NULL, 0, s->vga.hw_ops, s);
diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 8cea84f2bea..2782012196a 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -72,7 +72,9 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
     }
 
     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-pci.c b/hw/display/vga-pci.c
index 62fb5c38c1f..3e5bc259f7a 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 9d1f66af402..a6759c7e934 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2168,7 +2168,7 @@ 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;
 
@@ -2237,6 +2237,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 9e57f61e9ed..d89735d7301 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -119,7 +119,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 e2969a6c81c..d67318e6e46 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1248,7 +1248,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.31.1

[PATCH-for-6.2 2/2] hw/display: Do not allow multiple identical VGA devices

[Philippe Mathieu-Daudé]

vga_common_init() create a MemoryRegion named "vga.vram",
used as a singleton for the device class. When creating
the same device type multiple times, we get:

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

Commit 7852a77f598 ("vga: don't abort when adding a duplicate
isa-vga device") added a check for a single device, generalize
it to all VGA devices which call vga_common_init():

  $ qemu-system-mips64 -M pica61 -device isa-cirrus-vga
  qemu-system-mips64: -device isa-cirrus-vga: at most one isa-cirrus-vga device is permitted

Reported-by: John Snow <jsnow@redhat.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/44
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/display/vga-isa.c |  9 ---------
 hw/display/vga.c     | 13 +++++++++++++
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 2782012196a..b930c8d2667 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -62,15 +62,6 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
     MemoryRegion *vga_io_memory;
     const MemoryRegionPortio *vga_ports, *vbe_ports;
 
-    /*
-     * make sure this device is not being added twice, if so
-     * exit without crashing qemu
-     */
-    if (object_resolve_path_type("", TYPE_ISA_VGA, NULL)) {
-        error_setg(errp, "at most one %s device is permitted", TYPE_ISA_VGA);
-        return;
-    }
-
     s->global_vmstate = true;
     if (!vga_common_init(s, OBJECT(dev), errp)) {
         return;
diff --git a/hw/display/vga.c b/hw/display/vga.c
index a6759c7e934..8f0d5cc9019 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2172,6 +2172,19 @@ bool vga_common_init(VGACommonState *s, Object *obj, Error **errp)
 {
     int i, j, v, b;
 
+    if (obj) {
+        const char *typename = object_get_typename(obj);
+
+        /*
+         * make sure this device is not being added twice,
+         * if so exit without crashing qemu
+         */
+        if (object_resolve_path_type("", typename, NULL)) {
+            error_setg(errp, "at most one %s device is permitted", typename);
+            return false;
+        }
+    }
+
     for(i = 0;i < 256; i++) {
         v = 0;
         for(j = 0; j < 8; j++) {
-- 
2.31.1

Re: [PATCH-for-6.2 2/2] hw/display: Do not allow multiple identical VGA devices

[Paolo Bonzini]

On 11/18/21 20:20, Philippe Mathieu-Daudé wrote:
> +    if (obj) {
> +        const char *typename = object_get_typename(obj);
> +
> +        /*
> +         * make sure this device is not being added twice,
> +         * if so exit without crashing qemu
> +         */
> +        if (object_resolve_path_type("", typename, NULL)) {
> +            error_setg(errp, "at most one %s device is permitted", typename);
> +            return false;
> +        }
> +    }
> +

Wouldn't it give the same error with one ISA and one PCI VGA?

Paolo

Re: [PATCH-for-6.2 2/2] hw/display: Do not allow multiple identical VGA devices

[Philippe Mathieu-Daudé]

On 11/19/21 10:20, Paolo Bonzini wrote:
> On 11/18/21 20:20, Philippe Mathieu-Daudé wrote:
>> +    if (obj) {
>> +        const char *typename = object_get_typename(obj);
>> +
>> +        /*
>> +         * make sure this device is not being added twice,
>> +         * if so exit without crashing qemu
>> +         */
>> +        if (object_resolve_path_type("", typename, NULL)) {
>> +            error_setg(errp, "at most one %s device is permitted",
>> typename);
>> +            return false;
>> +        }
>> +    }
>> +
> 
> Wouldn't it give the same error with one ISA and one PCI VGA?

In that case I'd expect the object path to be different...

Anyhow, the fix from commit 7852a77f598 doesn't seem to work well:

$ qemu-system-x86_64 -M q35 -nodefaults -device isa-vga
qemu-system-x86_64: -device isa-vga: at most one isa-vga device is permitted

Re: [PATCH-for-6.2 2/2] hw/display: Do not allow multiple identical VGA devices

[Daniel P. Berrangé]

On Thu, Nov 18, 2021 at 08:20:20PM +0100, Philippe Mathieu-Daudé wrote:
> vga_common_init() create a MemoryRegion named "vga.vram",
> used as a singleton for the device class. When creating
> the same device type multiple times, we get:
> 
>   $ qemu-system-mips64 -M pica61 -device isa-cirrus-vga
>   RAMBlock "vga.vram" already registered, abort!
>   Aborted (core dumped)

Admittedly that would Callers that aren't ready to 

> 
> Commit 7852a77f598 ("vga: don't abort when adding a duplicate
> isa-vga device") added a check for a single device, generalize
> it to all VGA devices which call vga_common_init():

Not all current VGA devices abort though

 $ qemu-system-x86_64 -device cirrus-vga -device cirrus-vga

runs without aborting.

Your goal was to eliminate the abort() scenario, which
makes sense, but we hit other scenarios too.

How about we instead have vmstate_register_ram() gain
an "Error **errp" so it can propagate the failure into
vga_common_init rather than aborting.  That would
ensure we only affect scenarios that curently abort
and nothing more.

Other callers can pass &error_abort if they dont want
to gracefully handle this scenario.

> 
>   $ qemu-system-mips64 -M pica61 -device isa-cirrus-vga
>   qemu-system-mips64: -device isa-cirrus-vga: at most one isa-cirrus-vga device is permitted
> 
> Reported-by: John Snow <jsnow@redhat.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/44
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/display/vga-isa.c |  9 ---------
>  hw/display/vga.c     | 13 +++++++++++++
>  2 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
> index 2782012196a..b930c8d2667 100644
> --- a/hw/display/vga-isa.c
> +++ b/hw/display/vga-isa.c
> @@ -62,15 +62,6 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
>      MemoryRegion *vga_io_memory;
>      const MemoryRegionPortio *vga_ports, *vbe_ports;
>  
> -    /*
> -     * make sure this device is not being added twice, if so
> -     * exit without crashing qemu
> -     */
> -    if (object_resolve_path_type("", TYPE_ISA_VGA, NULL)) {
> -        error_setg(errp, "at most one %s device is permitted", TYPE_ISA_VGA);
> -        return;
> -    }
> -
>      s->global_vmstate = true;
>      if (!vga_common_init(s, OBJECT(dev), errp)) {
>          return;
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index a6759c7e934..8f0d5cc9019 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -2172,6 +2172,19 @@ bool vga_common_init(VGACommonState *s, Object *obj, Error **errp)
>  {
>      int i, j, v, b;
>  
> +    if (obj) {
> +        const char *typename = object_get_typename(obj);
> +
> +        /*
> +         * make sure this device is not being added twice,
> +         * if so exit without crashing qemu
> +         */
> +        if (object_resolve_path_type("", typename, NULL)) {
> +            error_setg(errp, "at most one %s device is permitted", typename);
> +            return false;
> +        }
> +    }
> +
>      for(i = 0;i < 256; i++) {
>          v = 0;
>          for(j = 0; j < 8; j++) {
> -- 
> 2.31.1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH-for-6.2 0/2] hw/display: Do not allow multiple (identical) VGA devices

[Mark Cave-Ayland]

On 18/11/2021 19:20, Philippe Mathieu-Daudé wrote:

> Commit 7852a77f598 fixed creating multiple TYPE_ISA_VGA devices,
> generalize the fix to all VGA devices.
> 
> See https://gitlab.com/qemu-project/qemu/-/issues/44
> 
> Philippe Mathieu-Daudé (2):
>    hw/display: Add Error* handle to vga_common_init()
>    hw/display: Do not allow multiple identical VGA devices
> 
>   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            |  4 +++-
>   hw/display/vga-isa-mm.c     |  3 ++-
>   hw/display/vga-isa.c        | 11 ++---------
>   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, 43 insertions(+), 20 deletions(-)

Hi Phil,

I don't think this is correct for non-ISA devices: for example years ago I had a PC 
running Windows 98SE with 2 identical PCI graphics cards configured in dual-head mode.

IIRC the BIOS would bring up the first graphics card and configure it to use the 
legacy ISA VGA ioports for compatibility, and then once the main OS drivers loaded 
both cards were switched to PCI mode and configured using the BARs as normal.


ATB,

Mark.

Re: [PATCH-for-6.2 0/2] hw/display: Do not allow multiple (identical) VGA devices

[Philippe Mathieu-Daudé]

On 11/19/21 09:21, Mark Cave-Ayland wrote:
> On 18/11/2021 19:20, Philippe Mathieu-Daudé wrote:
> 
>> Commit 7852a77f598 fixed creating multiple TYPE_ISA_VGA devices,
>> generalize the fix to all VGA devices.
>>
>> See https://gitlab.com/qemu-project/qemu/-/issues/44
>>
>> Philippe Mathieu-Daudé (2):
>>    hw/display: Add Error* handle to vga_common_init()
>>    hw/display: Do not allow multiple identical VGA devices
>>
>>   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            |  4 +++-
>>   hw/display/vga-isa-mm.c     |  3 ++-
>>   hw/display/vga-isa.c        | 11 ++---------
>>   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, 43 insertions(+), 20 deletions(-)
> 
> Hi Phil,
> 
> I don't think this is correct for non-ISA devices: for example years ago
> I had a PC running Windows 98SE with 2 identical PCI graphics cards
> configured in dual-head mode.
> 
> IIRC the BIOS would bring up the first graphics card and configure it to
> use the legacy ISA VGA ioports for compatibility, and then once the main
> OS drivers loaded both cards were switched to PCI mode and configured
> using the BARs as normal.

The problem here is QEMU technical debt, not the hardware.

When vga_common_init() calls memory_region_init_ram_nomigrate()
with obj=NULL, "vga.vram" is registered as a QOM singleton.

Updating it would 1/ require non-QOM devices to be QOM'ified
and 2/ break migration unless using HPFM which I don't master.

Re: [PATCH-for-6.2 0/2] hw/display: Do not allow multiple (identical) VGA devices

[Thomas Huth]

On 19/11/2021 10.49, Philippe Mathieu-Daudé wrote:
> On 11/19/21 09:21, Mark Cave-Ayland wrote:
>> On 18/11/2021 19:20, Philippe Mathieu-Daudé wrote:
>>
>>> Commit 7852a77f598 fixed creating multiple TYPE_ISA_VGA devices,
>>> generalize the fix to all VGA devices.
>>>
>>> See https://gitlab.com/qemu-project/qemu/-/issues/44
>>>
>>> Philippe Mathieu-Daudé (2):
>>>     hw/display: Add Error* handle to vga_common_init()
>>>     hw/display: Do not allow multiple identical VGA devices
>>>
>>>    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            |  4 +++-
>>>    hw/display/vga-isa-mm.c     |  3 ++-
>>>    hw/display/vga-isa.c        | 11 ++---------
>>>    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, 43 insertions(+), 20 deletions(-)
>>
>> Hi Phil,
>>
>> I don't think this is correct for non-ISA devices: for example years ago
>> I had a PC running Windows 98SE with 2 identical PCI graphics cards
>> configured in dual-head mode.
>>
>> IIRC the BIOS would bring up the first graphics card and configure it to
>> use the legacy ISA VGA ioports for compatibility, and then once the main
>> OS drivers loaded both cards were switched to PCI mode and configured
>> using the BARs as normal.
> 
> The problem here is QEMU technical debt, not the hardware.
> 
> When vga_common_init() calls memory_region_init_ram_nomigrate()
> with obj=NULL, "vga.vram" is registered as a QOM singleton.
> 
> Updating it would
 > 1/ require non-QOM devices to be QOM'ified

So sounds like that's the right way to go here.

> and 2/ break migration unless using HPFM which I don't master.

What's HPFM?

  Thomas

Re: [PATCH-for-6.2 0/2] hw/display: Do not allow multiple (identical) VGA devices

[Philippe Mathieu-Daudé]

On 11/19/21 10:58, Thomas Huth wrote:
> On 19/11/2021 10.49, Philippe Mathieu-Daudé wrote:
>> On 11/19/21 09:21, Mark Cave-Ayland wrote:
>>> On 18/11/2021 19:20, Philippe Mathieu-Daudé wrote:
>>>
>>>> Commit 7852a77f598 fixed creating multiple TYPE_ISA_VGA devices,
>>>> generalize the fix to all VGA devices.
>>>>
>>>> See https://gitlab.com/qemu-project/qemu/-/issues/44
>>>>
>>>> Philippe Mathieu-Daudé (2):
>>>>     hw/display: Add Error* handle to vga_common_init()
>>>>     hw/display: Do not allow multiple identical VGA devices
>>>>
>>>>    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            |  4 +++-
>>>>    hw/display/vga-isa-mm.c     |  3 ++-
>>>>    hw/display/vga-isa.c        | 11 ++---------
>>>>    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, 43 insertions(+), 20 deletions(-)
>>>
>>> Hi Phil,
>>>
>>> I don't think this is correct for non-ISA devices: for example years ago
>>> I had a PC running Windows 98SE with 2 identical PCI graphics cards
>>> configured in dual-head mode.
>>>
>>> IIRC the BIOS would bring up the first graphics card and configure it to
>>> use the legacy ISA VGA ioports for compatibility, and then once the main
>>> OS drivers loaded both cards were switched to PCI mode and configured
>>> using the BARs as normal.
>>
>> The problem here is QEMU technical debt, not the hardware.
>>
>> When vga_common_init() calls memory_region_init_ram_nomigrate()
>> with obj=NULL, "vga.vram" is registered as a QOM singleton.
>>
>> Updating it would
>> 1/ require non-QOM devices to be QOM'ified
> 
> So sounds like that's the right way to go here.
> 
>> and 2/ break migration unless using HPFM which I don't master.
> 
> What's HPFM?

Hocus Pocus Freakin' Magic

Re: [PATCH-for-6.2 0/2] hw/display: Do not allow multiple (identical) VGA devices

[Thomas Huth]

On 19/11/2021 11.17, Philippe Mathieu-Daudé wrote:
> On 11/19/21 10:58, Thomas Huth wrote:
>> On 19/11/2021 10.49, Philippe Mathieu-Daudé wrote:
>>> On 11/19/21 09:21, Mark Cave-Ayland wrote:
>>>> On 18/11/2021 19:20, Philippe Mathieu-Daudé wrote:
>>>>
>>>>> Commit 7852a77f598 fixed creating multiple TYPE_ISA_VGA devices,
>>>>> generalize the fix to all VGA devices.
>>>>>
>>>>> See https://gitlab.com/qemu-project/qemu/-/issues/44
>>>>>
>>>>> Philippe Mathieu-Daudé (2):
>>>>>      hw/display: Add Error* handle to vga_common_init()
>>>>>      hw/display: Do not allow multiple identical VGA devices
>>>>>
>>>>>     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            |  4 +++-
>>>>>     hw/display/vga-isa-mm.c     |  3 ++-
>>>>>     hw/display/vga-isa.c        | 11 ++---------
>>>>>     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, 43 insertions(+), 20 deletions(-)
>>>>
>>>> Hi Phil,
>>>>
>>>> I don't think this is correct for non-ISA devices: for example years ago
>>>> I had a PC running Windows 98SE with 2 identical PCI graphics cards
>>>> configured in dual-head mode.
>>>>
>>>> IIRC the BIOS would bring up the first graphics card and configure it to
>>>> use the legacy ISA VGA ioports for compatibility, and then once the main
>>>> OS drivers loaded both cards were switched to PCI mode and configured
>>>> using the BARs as normal.
>>>
>>> The problem here is QEMU technical debt, not the hardware.
>>>
>>> When vga_common_init() calls memory_region_init_ram_nomigrate()
>>> with obj=NULL, "vga.vram" is registered as a QOM singleton.
>>>
>>> Updating it would
>>> 1/ require non-QOM devices to be QOM'ified
>>
>> So sounds like that's the right way to go here.
>>
>>> and 2/ break migration unless using HPFM which I don't master.
>>
>> What's HPFM?
> 
> Hocus Pocus Freakin' Magic

LOL, ok, thanks, TIL.

Anyway, IMHO I'd rather fix this issue by properly QOM'ifying the 
vga-isa-mm.c code and breaking migration here (who's migrating such old ISA 
devices anyway?) instead of introducing more kludges
that might cause other trouble (see also 
https://gitlab.com/qemu-project/qemu/-/issues/733 ).

  Thomas