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