| Message ID | 20180507101013.32736-1-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 7 May 2018 at 11:10, Gerd Hoffmann <kraxel@redhat.com> wrote: > ... to a virtual machine. Without that we fail a ramblock register > sanity check, leading to a abort(), which isn't exactly user friendly. > > https://bugzilla.redhat.com/show_bug.cgi?id=1206037 > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/display/vga.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/display/vga.c b/hw/display/vga.c > index 72181330b8..328b6413ad 100644 > --- a/hw/display/vga.c > +++ b/hw/display/vga.c > @@ -2184,6 +2184,14 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate) > } > s->vbe_size_mask = s->vbe_size - 1; > > + if (global_vmstate) { > + static int have_vga; > + if (have_vga) { > + error_report("only one vga device is supported"); > + exit(1); > + } > + have_vga = true; > + } > s->is_vbe_vmstate = 1; > memory_region_init_ram_nomigrate(&s->vram, obj, "vga.vram", s->vram_size, > &error_fatal); > -- > 2.9.3 > This seems like a bit of an irritating reason to refuse to allow multiple VGA devices though, since we could make them work by having the vmstate be not-global. Which is exactly what it ought to be -- we're only defaulting to "use global" here for migration compatibility, I think. Since multiple VGA devices didn't work before, there's no old configs we need to migrate from, and so I think we could just say "the second and subsequent VGA devices get non-global ram vmstate" ? thanks -- PMM
> > + if (global_vmstate) { > > + static int have_vga; > > + if (have_vga) { > > + error_report("only one vga device is supported"); > > + exit(1); > > + } > > + have_vga = true; > > + } > > s->is_vbe_vmstate = 1; > > memory_region_init_ram_nomigrate(&s->vram, obj, "vga.vram", s->vram_size, > > &error_fatal); > > -- > > 2.9.3 > > > > This seems like a bit of an irritating reason to refuse to > allow multiple VGA devices though, since we could make > them work by having the vmstate be not-global. Multiple vga devices will also try to use the same isa vga ports. Seems that doesn't make qemu abort any more, which used to happen in older versions. > Which is > exactly what it ought to be -- we're only defaulting to > "use global" here for migration compatibility, I think. Yes. > Since multiple VGA devices didn't work before, there's > no old configs we need to migrate from, and so I think > we could just say "the second and subsequent VGA devices > get non-global ram vmstate" ? Yes, we could allow multiple vga devices that way. But due to the ioport resource conflict only one of the two (or more) vga devices will work properly. So I'm not sure allowing that configuration is actually an improvement ... I surely can update the commit message to also mention the ioport conflict though. cheers, Gerd
On 7 May 2018 at 13:11, Gerd Hoffmann <kraxel@redhat.com> wrote: >> This seems like a bit of an irritating reason to refuse to >> allow multiple VGA devices though, since we could make >> them work by having the vmstate be not-global. > > Multiple vga devices will also try to use the same isa vga > ports. Seems that doesn't make qemu abort any more, which > used to happen in older versions. Oh, good point. Shouldn't we be checking for that on some other thing than whether global_vmstate is set here, though? Otherwise it won't work consistently for the vga devices which pass 'false'. thanks -- PMM
On Mon, May 07, 2018 at 02:36:30PM +0100, Peter Maydell wrote: > On 7 May 2018 at 13:11, Gerd Hoffmann <kraxel@redhat.com> wrote: > >> This seems like a bit of an irritating reason to refuse to > >> allow multiple VGA devices though, since we could make > >> them work by having the vmstate be not-global. > > > > Multiple vga devices will also try to use the same isa vga > > ports. Seems that doesn't make qemu abort any more, which > > used to happen in older versions. > > Oh, good point. Shouldn't we be checking for that on > some other thing than whether global_vmstate is set here, > though? Otherwise it won't work consistently for the > vga devices which pass 'false'. The only one which passes global_vmstate == false is "-device secondary-vga", and that does *not* register vga ioports (the registers can be accessed via mmio pci bar though). So, right now global_vmstate == "registers vga ioports", and I don't expect that to change. cheers, Gerd
On 7 May 2018 at 15:30, Gerd Hoffmann <kraxel@redhat.com> wrote: > The only one which passes global_vmstate == false is "-device > secondary-vga", and that does *not* register vga ioports (the > registers can be accessed via mmio pci bar though). Looks like virtio-vga does as well. > So, right now global_vmstate == "registers vga ioports", and > I don't expect that to change. If we want that to remain true I think it's more likely to continue to hold if we (a) document it and (b) give the parameter a name that closer matches its function. But since vga_common_init() doesn't actually do anything related to registering the ioports, it's kind of odd. My assumption reading the code was "this is purely for migration back compat and if we wrote a new VGA device model we should have it pass 'false', whether it has IO ports or not. thanks -- PMM
On 05/07/2018 05:10 AM, Gerd Hoffmann wrote: > ... to a virtual machine. Without that we fail a ramblock register > sanity check, leading to a abort(), which isn't exactly user friendly. > > https://bugzilla.redhat.com/show_bug.cgi?id=1206037 > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/display/vga.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > > + if (global_vmstate) { > + static int have_vga; It looks funny to declare this as int, > + if (have_vga) { > + error_report("only one vga device is supported"); > + exit(1); > + } > + have_vga = true; but to treat it as bool. Why not just declare it as bool to begin with?
On Mon, May 07, 2018 at 06:00:56PM +0100, Peter Maydell wrote: > On 7 May 2018 at 15:30, Gerd Hoffmann <kraxel@redhat.com> wrote: > > The only one which passes global_vmstate == false is "-device > > secondary-vga", and that does *not* register vga ioports (the > > registers can be accessed via mmio pci bar though). > > Looks like virtio-vga does as well. Oh, ok. Didn't remember that one. Probably because there is no reason to worry about live migration backward compatibility for this one. Guess I have to rethink the logic then. cheers, Gerd
diff --git a/hw/display/vga.c b/hw/display/vga.c index 72181330b8..328b6413ad 100644 --- a/hw/display/vga.c +++ b/hw/display/vga.c @@ -2184,6 +2184,14 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate) } s->vbe_size_mask = s->vbe_size - 1; + if (global_vmstate) { + static int have_vga; + if (have_vga) { + error_report("only one vga device is supported"); + exit(1); + } + have_vga = true; + } s->is_vbe_vmstate = 1; memory_region_init_ram_nomigrate(&s->vram, obj, "vga.vram", s->vram_size, &error_fatal);
... to a virtual machine. Without that we fail a ramblock register sanity check, leading to a abort(), which isn't exactly user friendly. https://bugzilla.redhat.com/show_bug.cgi?id=1206037 Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/display/vga.c | 8 ++++++++ 1 file changed, 8 insertions(+)