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 <js...@redhat.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/44
> Signed-off-by: Philippe Mathieu-Daudé <phi...@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 :|
