| Message ID | 20180627115836.25092-2-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] vga: disable global_vmstate for 3.0+ machine types | expand |
Hi Gerd, On 06/27/2018 08:58 AM, Gerd Hoffmann wrote: > Two vga cards will listen on the same legacy (isa) ioports. Due to this > conflict only one of the two cards will work correctly in vga mode. The last one registered... > Print a warning message in that case. > > Signed-off-by: Gerd Hoffmann <kraxel@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 c82e6d240a..a6c246f76b 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,6 +2290,11 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space, > 1); > memory_region_set_coalescing(vga_io_memory); > if (init_vga_ports) { > + if (vgaports_registered) { > + warn_report("multiple vga cards may not work as expected"); > + } else { > + vgaports_registered = true; > + } > portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga"); Here we leak s->vga_port_list->regions from the 1st card. Can we call portio_list_destroy() in the if (vgaports_registered) block? > portio_list_set_flush_coalesced(&s->vga_port_list); > portio_list_add(&s->vga_port_list, address_space_io, 0x3b0); > Regards, Phil.
On Wed, Jun 27, 2018 at 10:10:17AM -0300, Philippe Mathieu-Daudé wrote: > Hi Gerd, > > On 06/27/2018 08:58 AM, Gerd Hoffmann wrote: > > Two vga cards will listen on the same legacy (isa) ioports. Due to this > > conflict only one of the two cards will work correctly in vga mode. > > The last one registered... > > > Print a warning message in that case. > > > > Signed-off-by: Gerd Hoffmann <kraxel@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 c82e6d240a..a6c246f76b 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,6 +2290,11 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space, > > 1); > > memory_region_set_coalescing(vga_io_memory); > > if (init_vga_ports) { > > + if (vgaports_registered) { > > + warn_report("multiple vga cards may not work as expected"); > > + } else { > > + vgaports_registered = true; > > + } > > portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga"); > > Here we leak s->vga_port_list->regions from the 1st card. > Can we call portio_list_destroy() in the if (vgaports_registered) block? We don't have a pointer at hand. Easier would be to only register the ports in the else block. Or throw an error in the if (vgaports_registered) block instead of printing a warning only. Hmm ... cheers, Gerd
On 06/27/2018 10:51 AM, Gerd Hoffmann wrote: > On Wed, Jun 27, 2018 at 10:10:17AM -0300, Philippe Mathieu-Daudé wrote: >> Hi Gerd, >> >> On 06/27/2018 08:58 AM, Gerd Hoffmann wrote: >>> Two vga cards will listen on the same legacy (isa) ioports. Due to this >>> conflict only one of the two cards will work correctly in vga mode. >> >> The last one registered... >> >>> Print a warning message in that case. >>> >>> Signed-off-by: Gerd Hoffmann <kraxel@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 c82e6d240a..a6c246f76b 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,6 +2290,11 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space, >>> 1); >>> memory_region_set_coalescing(vga_io_memory); >>> if (init_vga_ports) { >>> + if (vgaports_registered) { >>> + warn_report("multiple vga cards may not work as expected"); >>> + } else { >>> + vgaports_registered = true; >>> + } >>> portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga"); >> >> Here we leak s->vga_port_list->regions from the 1st card. >> Can we call portio_list_destroy() in the if (vgaports_registered) block? > > We don't have a pointer at hand. Easier would be to only register > the ports in the else block. I was thinking of: if (vgaports_registered) { warn_report("multiple vga cards may not work as expected"); portio_list_destroy(&s->vga_port_list); } else { vgaports_registered = true; } But "only register the ports in the else block" is easier indeed. > > Or throw an error in the if (vgaports_registered) block instead of > printing a warning only. > > Hmm ... Let's see if there interested user replying to this thread...
diff --git a/hw/display/vga.c b/hw/display/vga.c index c82e6d240a..a6c246f76b 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,6 +2290,11 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space, 1); memory_region_set_coalescing(vga_io_memory); if (init_vga_ports) { + if (vgaports_registered) { + warn_report("multiple vga cards may not work as expected"); + } else { + vgaports_registered = true; + } 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);
Two vga cards will listen on 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. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/display/vga.c | 6 ++++++ 1 file changed, 6 insertions(+)