| Message ID | 20211118192020.61245-3-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/display: Do not allow multiple (identical) VGA devices | expand |
On 11/18/21 20:20, Philippe Mathieu-Daudé wrote: > + 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; > + } > + } > + Wouldn't it give the same error with one ISA and one PCI VGA? Paolo
On 11/19/21 10:20, Paolo Bonzini wrote: > On 11/18/21 20:20, Philippe Mathieu-Daudé wrote: >> + 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; >> + } >> + } >> + > > Wouldn't it give the same error with one ISA and one PCI VGA? In that case I'd expect the object path to be different... Anyhow, the fix from commit 7852a77f598 doesn't seem to work well: $ qemu-system-x86_64 -M q35 -nodefaults -device isa-vga qemu-system-x86_64: -device isa-vga: at most one isa-vga device is permitted
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
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++) {
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) 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(): $ 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(-)