* [PATCH] vga: don't abort when adding a duplicate isa-vga device
@ 2021-08-13 23:36 Jose R. Ziviani
2021-08-14 6:52 ` Thomas Huth
0 siblings, 1 reply; 5+ messages in thread
From: Jose R. Ziviani @ 2021-08-13 23:36 UTC (permalink / raw)
To: qemu-devel; +Cc: thuth, kraxel, Jose R. Ziviani
If users try to add an isa-vga device that was already registered,
still in command line, qemu will crash:
$ qemu-system-mips64el -M pica61 -device isa-vga
RAMBlock "vga.vram" already registered, abort!
Aborted (core dumped)
That particular board registers such device automaticaly, so it's
not obvious that a VGA device already exists. This patch changes
this behavior by displaying a message and ignoring that device,
starting qemu normally.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/44
Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
---
hw/display/vga-isa.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 90851e730b..69db502dde 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -61,6 +61,15 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
MemoryRegion *vga_io_memory;
const MemoryRegionPortio *vga_ports, *vbe_ports;
+ /*
+ * some machines register VGA by default, so instead of aborting
+ * it, show a message and ignore this device.
+ */
+ if (qemu_ram_block_by_name("vga.vram")) {
+ error_report("vga.vram is already registered, ignoring this device");
+ return;
+ }
+
s->global_vmstate = true;
vga_common_init(s, OBJECT(dev));
s->legacy_address_space = isa_address_space(isadev);
--
2.32.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] vga: don't abort when adding a duplicate isa-vga device
2021-08-13 23:36 [PATCH] vga: don't abort when adding a duplicate isa-vga device Jose R. Ziviani
@ 2021-08-14 6:52 ` Thomas Huth
2021-08-16 5:05 ` Gerd Hoffmann
2021-08-24 5:12 ` Markus Armbruster
0 siblings, 2 replies; 5+ messages in thread
From: Thomas Huth @ 2021-08-14 6:52 UTC (permalink / raw)
To: Jose R. Ziviani, qemu-devel; +Cc: kraxel
On 14/08/2021 01.36, Jose R. Ziviani wrote:
> If users try to add an isa-vga device that was already registered,
> still in command line, qemu will crash:
>
> $ qemu-system-mips64el -M pica61 -device isa-vga
> RAMBlock "vga.vram" already registered, abort!
> Aborted (core dumped)
>
> That particular board registers such device automaticaly, so it's
> not obvious that a VGA device already exists. This patch changes
> this behavior by displaying a message and ignoring that device,
> starting qemu normally.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/44
> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
> ---
> hw/display/vga-isa.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
> index 90851e730b..69db502dde 100644
> --- a/hw/display/vga-isa.c
> +++ b/hw/display/vga-isa.c
> @@ -61,6 +61,15 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
> MemoryRegion *vga_io_memory;
> const MemoryRegionPortio *vga_ports, *vbe_ports;
>
> + /*
> + * some machines register VGA by default, so instead of aborting
> + * it, show a message and ignore this device.
> + */
> + if (qemu_ram_block_by_name("vga.vram")) {
> + error_report("vga.vram is already registered, ignoring this device");
> + return;
> + }
I think we should not ignore the error, but rather turn this into a proper
error (instead of aborting).
So if you replace error_report(...) with error_setg(errp, ...), the patch
should be fine.
Thomas
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vga: don't abort when adding a duplicate isa-vga device
2021-08-14 6:52 ` Thomas Huth
@ 2021-08-16 5:05 ` Gerd Hoffmann
2021-08-16 12:34 ` Jose Ricardo Ziviani
2021-08-24 5:12 ` Markus Armbruster
1 sibling, 1 reply; 5+ messages in thread
From: Gerd Hoffmann @ 2021-08-16 5:05 UTC (permalink / raw)
To: Thomas Huth; +Cc: qemu-devel, Jose R. Ziviani
Hi,
> > + if (qemu_ram_block_by_name("vga.vram")) {
> > + error_report("vga.vram is already registered, ignoring this device");
> > + return;
> > + }
>
> I think we should not ignore the error, but rather turn this into a proper
> error (instead of aborting).
Yes. Silently fixing up things automatically tends to be worse long-term.
take care,
Gerd
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vga: don't abort when adding a duplicate isa-vga device
2021-08-16 5:05 ` Gerd Hoffmann
@ 2021-08-16 12:34 ` Jose Ricardo Ziviani
0 siblings, 0 replies; 5+ messages in thread
From: Jose Ricardo Ziviani @ 2021-08-16 12:34 UTC (permalink / raw)
To: Gerd Hoffmann, Thomas Huth; +Cc: qemu-devel
Hello Thomas and Gerd,
Thank you for reviewing it. Sending a v2 soon.
Thank you very much!
On 16/08/2021 02:05, Gerd Hoffmann wrote:
> Hi,
>
>>> + if (qemu_ram_block_by_name("vga.vram")) {
>>> + error_report("vga.vram is already registered, ignoring this device");
>>> + return;
>>> + }
>> I think we should not ignore the error, but rather turn this into a proper
>> error (instead of aborting).
> Yes. Silently fixing up things automatically tends to be worse long-term.
>
> take care,
> Gerd
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vga: don't abort when adding a duplicate isa-vga device
2021-08-14 6:52 ` Thomas Huth
2021-08-16 5:05 ` Gerd Hoffmann
@ 2021-08-24 5:12 ` Markus Armbruster
1 sibling, 0 replies; 5+ messages in thread
From: Markus Armbruster @ 2021-08-24 5:12 UTC (permalink / raw)
To: Thomas Huth; +Cc: kraxel, qemu-devel, Jose R. Ziviani
Thomas Huth <thuth@redhat.com> writes:
> On 14/08/2021 01.36, Jose R. Ziviani wrote:
>> If users try to add an isa-vga device that was already registered,
>> still in command line, qemu will crash:
>> $ qemu-system-mips64el -M pica61 -device isa-vga
>> RAMBlock "vga.vram" already registered, abort!
>> Aborted (core dumped)
>> That particular board registers such device automaticaly, so it's
>> not obvious that a VGA device already exists. This patch changes
>> this behavior by displaying a message and ignoring that device,
>> starting qemu normally.
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/44
>> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
>> ---
>> hw/display/vga-isa.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>> diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
>> index 90851e730b..69db502dde 100644
>> --- a/hw/display/vga-isa.c
>> +++ b/hw/display/vga-isa.c
>> @@ -61,6 +61,15 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
>> MemoryRegion *vga_io_memory;
>> const MemoryRegionPortio *vga_ports, *vbe_ports;
>> + /*
>> + * some machines register VGA by default, so instead of aborting
>> + * it, show a message and ignore this device.
>> + */
>> + if (qemu_ram_block_by_name("vga.vram")) {
>> + error_report("vga.vram is already registered, ignoring this device");
>> + return;
>> + }
>
> I think we should not ignore the error, but rather turn this into a
> proper error (instead of aborting).
>
> So if you replace error_report(...) with error_setg(errp, ...), the
> patch should be fine.
Agreed.
error_report() in a function with an Error **errp parameter is almost
always wrong.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-08-24 5:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-13 23:36 [PATCH] vga: don't abort when adding a duplicate isa-vga device Jose R. Ziviani
2021-08-14 6:52 ` Thomas Huth
2021-08-16 5:05 ` Gerd Hoffmann
2021-08-16 12:34 ` Jose Ricardo Ziviani
2021-08-24 5:12 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.