[Qemu-devel] [PATCH v2 1/2] vga: disable global_vmstate for 3.0+ machine types

[Qemu-devel] [PATCH v2 1/2] vga: disable global_vmstate for 3.0+ machine types

[Gerd Hoffmann]

Move global_vmstate from vga_common_init() parameter to VGACommonState
field.  Set global_vmstate to true for isa vga devices, so nothing
changes here.  virtio-vga and secondary-vga already set global_vmstate
to false so no change here either.  All other pci vga devices get a new
global-vmstate property, defaulting to false.  A compat property flips
it to true for older machine types.

With this in place you don't get a vmstate section naming conflict any
more when adding multiple pci vga devices to your vm.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga_int.h    |  3 ++-
 include/hw/compat.h     | 16 ++++++++++++++++
 hw/display/cirrus_vga.c |  9 ++++++---
 hw/display/qxl.c        |  3 ++-
 hw/display/vga-isa-mm.c |  3 ++-
 hw/display/vga-isa.c    |  3 ++-
 hw/display/vga-pci.c    |  5 +++--
 hw/display/vga.c        |  4 ++--
 hw/display/virtio-vga.c |  2 +-
 hw/display/vmware_vga.c |  4 +++-
 10 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index f8fcf62a56..339661bc01 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -133,6 +133,7 @@ typedef struct VGACommonState {
     bool full_update_gfx;
     bool big_endian_fb;
     bool default_endian_fb;
+    bool global_vmstate;
     /* hardware mouse cursor support */
     uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
     uint32_t hw_cursor_x;
@@ -157,7 +158,7 @@ static inline int c6_to_8(int v)
     return (v << 2) | (b << 1) | b;
 }
 
-void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate);
+void vga_common_init(VGACommonState *s, Object *obj);
 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/include/hw/compat.h b/include/hw/compat.h
index 44d5964060..c08f4040bb 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -10,6 +10,22 @@
         .driver   = "hda-audio",\
         .property = "use-timer",\
         .value    = "false",\
+    },{\
+        .driver   = "cirrus-vga",\
+        .property = "global-vmstate",\
+        .value    = "true",\
+    },{\
+        .driver   = "VGA",\
+        .property = "global-vmstate",\
+        .value    = "true",\
+    },{\
+        .driver   = "vmware-svga",\
+        .property = "global-vmstate",\
+        .value    = "true",\
+    },{\
+        .driver   = "qxl-vga",\
+        .property = "global-vmstate",\
+        .value    = "true",\
     },
 
 #define HW_COMPAT_2_11 \
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 138ae961b9..1d050621f1 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -3048,7 +3048,8 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev, Error **errp)
                    s->vram_size_mb);
         return;
     }
-    vga_common_init(s, OBJECT(dev), true);
+    s->global_vmstate = true;
+    vga_common_init(s, OBJECT(dev));
     cirrus_init_common(&d->cirrus_vga, OBJECT(dev), CIRRUS_ID_CLGD5430, 0,
                        isa_address_space(isadev),
                        isa_address_space_io(isadev));
@@ -3062,7 +3063,7 @@ static Property isa_cirrus_vga_properties[] = {
     DEFINE_PROP_UINT32("vgamem_mb", struct ISACirrusVGAState,
                        cirrus_vga.vga.vram_size_mb, 4),
     DEFINE_PROP_BOOL("blitter", struct ISACirrusVGAState,
-                       cirrus_vga.enable_blitter, true),
+                     cirrus_vga.enable_blitter, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -3105,7 +3106,7 @@ static void pci_cirrus_vga_realize(PCIDevice *dev, Error **errp)
          return;
      }
      /* setup VGA */
-     vga_common_init(&s->vga, OBJECT(dev), true);
+     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);
@@ -3134,6 +3135,8 @@ static Property pci_vga_cirrus_properties[] = {
                        cirrus_vga.vga.vram_size_mb, 4),
     DEFINE_PROP_BOOL("blitter", struct PCICirrusVGAState,
                      cirrus_vga.enable_blitter, true),
+    DEFINE_PROP_BOOL("global-vmstate", struct PCICirrusVGAState,
+                     cirrus_vga.vga.global_vmstate, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index a71714ccb4..3f740d7aa3 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2168,7 +2168,7 @@ 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 >> 20;
-    vga_common_init(vga, OBJECT(dev), true);
+    vga_common_init(vga, OBJECT(dev));
     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,
@@ -2410,6 +2410,7 @@ static Property qxl_properties[] = {
 #endif
         DEFINE_PROP_UINT32("xres", PCIQXLDevice, xres, 0),
         DEFINE_PROP_UINT32("yres", PCIQXLDevice, yres, 0),
+        DEFINE_PROP_BOOL("global-vmstate", PCIQXLDevice, vga.global_vmstate, false),
         DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/display/vga-isa-mm.c b/hw/display/vga-isa-mm.c
index e887b45651..d2d6b32abf 100644
--- a/hw/display/vga-isa-mm.c
+++ b/hw/display/vga-isa-mm.c
@@ -131,7 +131,8 @@ int isa_vga_mm_init(hwaddr vram_base,
     s = g_malloc0(sizeof(*s));
 
     s->vga.vram_size_mb = VGA_RAM_SIZE >> 20;
-    vga_common_init(&s->vga, NULL, true);
+    s->vga.global_vmstate = true;
+    vga_common_init(&s->vga, NULL);
     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 469834add5..fa44242e0d 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -58,7 +58,8 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
     MemoryRegion *vga_io_memory;
     const MemoryRegionPortio *vga_ports, *vbe_ports;
 
-    vga_common_init(s, OBJECT(dev), true);
+    s->global_vmstate = true;
+    vga_common_init(s, OBJECT(dev));
     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 1ea559762a..e9e62eac70 100644
--- a/hw/display/vga-pci.c
+++ b/hw/display/vga-pci.c
@@ -222,7 +222,7 @@ static void pci_std_vga_realize(PCIDevice *dev, Error **errp)
     bool qext = false;
 
     /* vga + console init */
-    vga_common_init(s, OBJECT(dev), true);
+    vga_common_init(s, OBJECT(dev));
     vga_init(s, OBJECT(dev), pci_address_space(dev), pci_address_space_io(dev),
              true);
 
@@ -265,7 +265,7 @@ static void pci_secondary_vga_realize(PCIDevice *dev, Error **errp)
     bool qext = false;
 
     /* vga + console init */
-    vga_common_init(s, OBJECT(dev), false);
+    vga_common_init(s, OBJECT(dev));
     s->con = graphic_console_init(DEVICE(dev), 0, s->hw_ops, s);
 
     /* mmio bar */
@@ -308,6 +308,7 @@ static Property vga_pci_properties[] = {
     DEFINE_PROP_BIT("mmio", PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_MMIO, true),
     DEFINE_PROP_BIT("qemu-extended-regs",
                     PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_QEXT, true),
+    DEFINE_PROP_BOOL("global-vmstate", PCIVGAState, vga.global_vmstate, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/display/vga.c b/hw/display/vga.c
index ed476e4e80..c82e6d240a 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2163,7 +2163,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 global_vmstate)
+void vga_common_init(VGACommonState *s, Object *obj)
 {
     int i, j, v, b;
 
@@ -2202,7 +2202,7 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
     s->is_vbe_vmstate = 1;
     memory_region_init_ram_nomigrate(&s->vram, obj, "vga.vram", s->vram_size,
                            &error_fatal);
-    vmstate_register_ram(&s->vram, global_vmstate ? NULL : DEVICE(obj));
+    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);
     s->get_bpp = vga_get_bpp;
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index 97db6c3372..8d3d9e14a7 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -106,7 +106,7 @@ static void virtio_vga_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
 
     /* init vga compat bits */
     vga->vram_size_mb = 8;
-    vga_common_init(vga, OBJECT(vpci_dev), false);
+    vga_common_init(vga, OBJECT(vpci_dev));
     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 bd3e8b3586..39f4d40801 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1241,7 +1241,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), true);
+    vga_common_init(&s->vga, OBJECT(dev));
     vga_init(&s->vga, OBJECT(dev), address_space, io, true);
     vmstate_register(NULL, 0, &vmstate_vga_common, &s->vga);
     s->new_depth = 32;
@@ -1321,6 +1321,8 @@ static void pci_vmsvga_realize(PCIDevice *dev, Error **errp)
 static Property vga_vmware_properties[] = {
     DEFINE_PROP_UINT32("vgamem_mb", struct pci_vmsvga_state_s,
                        chip.vga.vram_size_mb, 16),
+    DEFINE_PROP_BOOL("global-vmstate", struct pci_vmsvga_state_s,
+                     chip.vga.global_vmstate, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.9.3

[Qemu-devel] [PATCH v2 2/2] warn about two vga cards

[Gerd Hoffmann]

Two vga cards will try to register for the same legacy (isa) ioports.
Due to this conflict only one of the two cards will work correctly in
vga mode.

Print a warning message in that case.  Also try registering the ports
only once, so the first vga added to the system will work properly.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index c82e6d240a..e811571624 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2273,6 +2273,7 @@ MemoryRegion *vga_init_io(VGACommonState *s, Object *obj,
 void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
               MemoryRegion *address_space_io, bool init_vga_ports)
 {
+    static bool vgaports_registered;
     MemoryRegion *vga_io_memory;
     const MemoryRegionPortio *vga_ports, *vbe_ports;
 
@@ -2289,9 +2290,14 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
                                         1);
     memory_region_set_coalescing(vga_io_memory);
     if (init_vga_ports) {
-        portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga");
-        portio_list_set_flush_coalesced(&s->vga_port_list);
-        portio_list_add(&s->vga_port_list, address_space_io, 0x3b0);
+        if (vgaports_registered) {
+            warn_report("multiple vga cards may not work as expected");
+        } else {
+            portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga");
+            portio_list_set_flush_coalesced(&s->vga_port_list);
+            portio_list_add(&s->vga_port_list, address_space_io, 0x3b0);
+            vgaports_registered = true;
+        }
     }
     if (vbe_ports) {
         portio_list_init(&s->vbe_port_list, obj, vbe_ports, s, "vbe");
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 2/2] warn about two vga cards

[Gerd Hoffmann]

On Mon, Jul 02, 2018 at 06:33:45PM +0200, Gerd Hoffmann wrote:
> Two vga cards will try to register for the same legacy (isa) ioports.
> Due to this conflict only one of the two cards will work correctly in
> vga mode.
> 
> Print a warning message in that case.  Also try registering the ports
> only once, so the first vga added to the system will work properly.

Scratch that, not working as expected.
Looked like a simple change but appearently isn't.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 2/2] warn about two vga cards

[Philippe Mathieu-Daudé]

On 07/02/2018 01:33 PM, Gerd Hoffmann wrote:
> Two vga cards will try to register for the same legacy (isa) ioports.
> Due to this conflict only one of the two cards will work correctly in
> vga mode.
> 
> Print a warning message in that case.  Also try registering the ports
> only once, so the first vga added to the system will work properly.

And you also fixed a memory leak.

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

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

> ---
>  hw/display/vga.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index c82e6d240a..e811571624 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -2273,6 +2273,7 @@ MemoryRegion *vga_init_io(VGACommonState *s, Object *obj,
>  void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
>                MemoryRegion *address_space_io, bool init_vga_ports)
>  {
> +    static bool vgaports_registered;
>      MemoryRegion *vga_io_memory;
>      const MemoryRegionPortio *vga_ports, *vbe_ports;
>  
> @@ -2289,9 +2290,14 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
>                                          1);
>      memory_region_set_coalescing(vga_io_memory);
>      if (init_vga_ports) {
> -        portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga");
> -        portio_list_set_flush_coalesced(&s->vga_port_list);
> -        portio_list_add(&s->vga_port_list, address_space_io, 0x3b0);
> +        if (vgaports_registered) {
> +            warn_report("multiple vga cards may not work as expected");
> +        } else {
> +            portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga");
> +            portio_list_set_flush_coalesced(&s->vga_port_list);
> +            portio_list_add(&s->vga_port_list, address_space_io, 0x3b0);
> +            vgaports_registered = true;
> +        }
>      }
>      if (vbe_ports) {
>          portio_list_init(&s->vbe_port_list, obj, vbe_ports, s, "vbe");
> 

Re: [Qemu-devel] [PATCH v2 2/2] warn about two vga cards

[Philippe Mathieu-Daudé]

On 07/02/2018 01:43 PM, Gerd Hoffmann wrote:
> On Mon, Jul 02, 2018 at 06:33:45PM +0200, Gerd Hoffmann wrote:
>> Two vga cards will try to register for the same legacy (isa) ioports.
>> Due to this conflict only one of the two cards will work correctly in
>> vga mode.
>>
>> Print a warning message in that case.  Also try registering the ports
>> only once, so the first vga added to the system will work properly.
> 
> Scratch that, not working as expected.

Uh what's wrong?

/me missed it.

> Looked like a simple change but appearently isn't.
> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH v2 2/2] warn about two vga cards

[Gerd Hoffmann]

On Mon, Jul 02, 2018 at 02:03:58PM -0300, Philippe Mathieu-Daudé wrote:
> On 07/02/2018 01:43 PM, Gerd Hoffmann wrote:
> > On Mon, Jul 02, 2018 at 06:33:45PM +0200, Gerd Hoffmann wrote:
> >> Two vga cards will try to register for the same legacy (isa) ioports.
> >> Due to this conflict only one of the two cards will work correctly in
> >> vga mode.
> >>
> >> Print a warning message in that case.  Also try registering the ports
> >> only once, so the first vga added to the system will work properly.
> > 
> > Scratch that, not working as expected.
> 
> Uh what's wrong?

Well, current behavior (with patch 1/2 applied, or with virtio-vga
which doesn't use global_vmstate) is "last vga card wins", and that card
actually works.  This patch breaks the vga display, even when tweaked
that both vga and vbe ports are handled (so they don't end up being
assigned to two different cards).  Didn't figure yet why.

Beside that I've noticed cirrus-vga doesn't use that function, so even
the warning message doesn't work as intended in case cirrus is involved.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 2/2] warn about two vga cards

[Philippe Mathieu-Daudé]

On 07/03/2018 03:25 AM, Gerd Hoffmann wrote:
> On Mon, Jul 02, 2018 at 02:03:58PM -0300, Philippe Mathieu-Daudé wrote:
>> On 07/02/2018 01:43 PM, Gerd Hoffmann wrote:
>>> On Mon, Jul 02, 2018 at 06:33:45PM +0200, Gerd Hoffmann wrote:
>>>> Two vga cards will try to register for the same legacy (isa) ioports.
>>>> Due to this conflict only one of the two cards will work correctly in
>>>> vga mode.
>>>>
>>>> Print a warning message in that case.  Also try registering the ports
>>>> only once, so the first vga added to the system will work properly.
>>>
>>> Scratch that, not working as expected.
>>
>> Uh what's wrong?
> 
> Well, current behavior (with patch 1/2 applied, or with virtio-vga
> which doesn't use global_vmstate) is "last vga card wins", and that card
> actually works.  This patch breaks the vga display, even when tweaked
> that both vga and vbe ports are handled (so they don't end up being
> assigned to two different cards).  Didn't figure yet why.

With this patch the I/O registered is "first vga card wins", maybe there
is now a discrepancy...

> 
> Beside that I've noticed cirrus-vga doesn't use that function, so even
> the warning message doesn't work as intended in case cirrus is involved.

Oh... weird, OK.

Regards,

Phil.