[Qemu-devel] [PATCH] vga: print friendly error message in case multiple vga devices are added

[Qemu-devel] [PATCH] vga: print friendly error message in case multiple vga devices are added

[Gerd Hoffmann]

... 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

Re: [Qemu-devel] [PATCH] vga: print friendly error message in case multiple vga devices are added

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] vga: print friendly error message in case multiple vga devices are added

[Gerd Hoffmann]

> > +    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

Re: [Qemu-devel] [PATCH] vga: print friendly error message in case multiple vga devices are added

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] vga: print friendly error message in case multiple vga devices are added

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH] vga: print friendly error message in case multiple vga devices are added

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] vga: print friendly error message in case multiple vga devices are added

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH] vga: print friendly error message in case multiple vga devices are added

[Eric Blake]

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?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org