[PATCH v3] Warn user if the vga flag is passed but no vga device is created

[PATCH v3] Warn user if the vga flag is passed but no vga device is created

[Gautam Agrawal]

A global boolean variable "vga_interface_created"(declared in softmmu/globals.c)
has been used to track the creation of vga interface. If the vga flag is passed
in the command line "default_vga"(declared in softmmu/vl.c) variable is set to 0.
To warn user, the condition checks if vga_interface_created is false
and default_vga is equal to 0. If "-vga none" is passed, this patch will not warn the
user regarding the creation of VGA device.

The warning "A -vga option was passed but this
machine type does not use that option; no VGA device has been created"
is logged if vga flag is passed but no vga device is created.

This patch has been tested for x86_64, i386, sparc, sparc64 and arm boards.

Signed-off-by: Gautam Agrawal <gautamnagrawal@gmail.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/581
---
 hw/hppa/machine.c         | 1 +
 hw/isa/isa-bus.c          | 1 +
 hw/mips/fuloong2e.c       | 1 +
 hw/pci/pci.c              | 1 +
 hw/ppc/spapr.c            | 1 +
 hw/sparc/sun4m.c          | 2 ++
 hw/sparc64/sun4u.c        | 1 +
 hw/xenpv/xen_machine_pv.c | 1 +
 include/sysemu/sysemu.h   | 1 +
 softmmu/globals.c         | 1 +
 softmmu/vl.c              | 6 ++++++
 11 files changed, 17 insertions(+)

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index f7595c0857..2e37349347 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -192,6 +192,7 @@ static void machine_hppa_init(MachineState *machine)
 
     /* Graphics setup. */
     if (machine->enable_graphics && vga_interface_type != VGA_NONE) {
+        vga_interface_created = true;
         dev = qdev_new("artist");
         s = SYS_BUS_DEVICE(dev);
         sysbus_realize_and_unref(s, &error_fatal);
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 0ad1c5fd65..cd5ad3687d 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -166,6 +166,7 @@ bool isa_realize_and_unref(ISADevice *dev, ISABus *bus, Error **errp)
 
 ISADevice *isa_vga_init(ISABus *bus)
 {
+    vga_interface_created = true;
     switch (vga_interface_type) {
     case VGA_CIRRUS:
         return isa_create_simple(bus, "isa-cirrus-vga");
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 7b13098f9b..5ee546f5f6 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -320,6 +320,7 @@ static void mips_fuloong2e_init(MachineState *machine)
 
     /* GPU */
     if (vga_interface_type != VGA_NONE) {
+        vga_interface_created = true;
         pci_dev = pci_new(-1, "ati-vga");
         dev = DEVICE(pci_dev);
         qdev_prop_set_uint32(dev, "vgamem_mb", 16);
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e99417e501..9c58f02853 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2037,6 +2037,7 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus,
 
 PCIDevice *pci_vga_init(PCIBus *bus)
 {
+    vga_interface_created = true;
     switch (vga_interface_type) {
     case VGA_CIRRUS:
         return pci_create_simple(bus, -1, "cirrus-vga");
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 22569305d2..9df493cfe2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1742,6 +1742,7 @@ static void spapr_rtc_create(SpaprMachineState *spapr)
 /* Returns whether we want to use VGA or not */
 static bool spapr_vga_init(PCIBus *pci_bus, Error **errp)
 {
+    vga_interface_created = true;
     switch (vga_interface_type) {
     case VGA_NONE:
         return false;
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index fccaed1eb4..b693eea0e0 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -920,6 +920,7 @@ static void sun4m_hw_init(MachineState *machine)
             /* sbus irq 5 */
             cg3_init(hwdef->tcx_base, slavio_irq[11], 0x00100000,
                      graphic_width, graphic_height, graphic_depth);
+            vga_interface_created = true;
         } else {
             /* If no display specified, default to TCX */
             if (graphic_depth != 8 && graphic_depth != 24) {
@@ -935,6 +936,7 @@ static void sun4m_hw_init(MachineState *machine)
 
             tcx_init(hwdef->tcx_base, slavio_irq[11], 0x00100000,
                      graphic_width, graphic_height, graphic_depth);
+            vga_interface_created = true;
         }
     }
 
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 6fd08e2298..7c461d194a 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -632,6 +632,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
     switch (vga_interface_type) {
     case VGA_STD:
         pci_create_simple(pci_busA, PCI_DEVFN(2, 0), "VGA");
+        vga_interface_created = true;
         break;
     case VGA_NONE:
         break;
diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
index 8df575a457..20c9611d71 100644
--- a/hw/xenpv/xen_machine_pv.c
+++ b/hw/xenpv/xen_machine_pv.c
@@ -63,6 +63,7 @@ static void xen_init_pv(MachineState *machine)
     if (vga_interface_type == VGA_XENFB) {
         xen_config_dev_vfb(0, "vnc");
         xen_config_dev_vkbd(0);
+        vga_interface_created = true;
     }
 
     /* configure disks */
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 10e283c170..360a408edf 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -34,6 +34,7 @@ typedef enum {
 } VGAInterfaceType;
 
 extern int vga_interface_type;
+extern bool vga_interface_created;
 
 extern int graphic_width;
 extern int graphic_height;
diff --git a/softmmu/globals.c b/softmmu/globals.c
index 3ebd718e35..98b64e0492 100644
--- a/softmmu/globals.c
+++ b/softmmu/globals.c
@@ -40,6 +40,7 @@ int nb_nics;
 NICInfo nd_table[MAX_NICS];
 int autostart = 1;
 int vga_interface_type = VGA_NONE;
+bool vga_interface_created;
 Chardev *parallel_hds[MAX_PARALLEL_PORTS];
 int win2k_install_hack;
 int singlestep;
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 06a0e342fe..8411cb15af 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2734,6 +2734,12 @@ static void qemu_machine_creation_done(void)
     if (foreach_device_config(DEV_GDB, gdbserver_start) < 0) {
         exit(1);
     }
+    if (!vga_interface_created && !default_vga &&
+        vga_interface_type != VGA_NONE) {
+        warn_report("A -vga option was passed but this machine "
+                    "type does not use that option; "
+                    "No VGA device has been created");
+    }
 }
 
 void qmp_x_exit_preconfig(Error **errp)
-- 
2.34.1

Re: [PATCH v3] Warn user if the vga flag is passed but no vga device is created

[Peter Maydell]

On Sun, 1 May 2022 at 13:25, Gautam Agrawal <gautamnagrawal@gmail.com> wrote:
>
> A global boolean variable "vga_interface_created"(declared in softmmu/globals.c)
> has been used to track the creation of vga interface. If the vga flag is passed
> in the command line "default_vga"(declared in softmmu/vl.c) variable is set to 0.
> To warn user, the condition checks if vga_interface_created is false
> and default_vga is equal to 0. If "-vga none" is passed, this patch will not warn the
> user regarding the creation of VGA device.
>
> The warning "A -vga option was passed but this
> machine type does not use that option; no VGA device has been created"
> is logged if vga flag is passed but no vga device is created.
>
> This patch has been tested for x86_64, i386, sparc, sparc64 and arm boards.
>
> Signed-off-by: Gautam Agrawal <gautamnagrawal@gmail.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/581

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3] Warn user if the vga flag is passed but no vga device is created

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 7017 bytes --]

On Sun, May 01, 2022 at 05:55:05PM +0530, Gautam Agrawal wrote:
> A global boolean variable "vga_interface_created"(declared in softmmu/globals.c)
> has been used to track the creation of vga interface. If the vga flag is passed
> in the command line "default_vga"(declared in softmmu/vl.c) variable is set to 0.
> To warn user, the condition checks if vga_interface_created is false
> and default_vga is equal to 0. If "-vga none" is passed, this patch will not warn the
> user regarding the creation of VGA device.
> 
> The warning "A -vga option was passed but this
> machine type does not use that option; no VGA device has been created"
> is logged if vga flag is passed but no vga device is created.
> 
> This patch has been tested for x86_64, i386, sparc, sparc64 and arm boards.
> 
> Signed-off-by: Gautam Agrawal <gautamnagrawal@gmail.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/581
> ---
>  hw/hppa/machine.c         | 1 +
>  hw/isa/isa-bus.c          | 1 +
>  hw/mips/fuloong2e.c       | 1 +
>  hw/pci/pci.c              | 1 +
>  hw/ppc/spapr.c            | 1 +
>  hw/sparc/sun4m.c          | 2 ++
>  hw/sparc64/sun4u.c        | 1 +
>  hw/xenpv/xen_machine_pv.c | 1 +
>  include/sysemu/sysemu.h   | 1 +
>  softmmu/globals.c         | 1 +
>  softmmu/vl.c              | 6 ++++++
>  11 files changed, 17 insertions(+)

Gerd or Richard: do you want to merge this patch?

Thanks,
Stefan

> 
> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index f7595c0857..2e37349347 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -192,6 +192,7 @@ static void machine_hppa_init(MachineState *machine)
>  
>      /* Graphics setup. */
>      if (machine->enable_graphics && vga_interface_type != VGA_NONE) {
> +        vga_interface_created = true;
>          dev = qdev_new("artist");
>          s = SYS_BUS_DEVICE(dev);
>          sysbus_realize_and_unref(s, &error_fatal);
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index 0ad1c5fd65..cd5ad3687d 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -166,6 +166,7 @@ bool isa_realize_and_unref(ISADevice *dev, ISABus *bus, Error **errp)
>  
>  ISADevice *isa_vga_init(ISABus *bus)
>  {
> +    vga_interface_created = true;
>      switch (vga_interface_type) {
>      case VGA_CIRRUS:
>          return isa_create_simple(bus, "isa-cirrus-vga");
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index 7b13098f9b..5ee546f5f6 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -320,6 +320,7 @@ static void mips_fuloong2e_init(MachineState *machine)
>  
>      /* GPU */
>      if (vga_interface_type != VGA_NONE) {
> +        vga_interface_created = true;
>          pci_dev = pci_new(-1, "ati-vga");
>          dev = DEVICE(pci_dev);
>          qdev_prop_set_uint32(dev, "vgamem_mb", 16);
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index e99417e501..9c58f02853 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2037,6 +2037,7 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus,
>  
>  PCIDevice *pci_vga_init(PCIBus *bus)
>  {
> +    vga_interface_created = true;
>      switch (vga_interface_type) {
>      case VGA_CIRRUS:
>          return pci_create_simple(bus, -1, "cirrus-vga");
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 22569305d2..9df493cfe2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1742,6 +1742,7 @@ static void spapr_rtc_create(SpaprMachineState *spapr)
>  /* Returns whether we want to use VGA or not */
>  static bool spapr_vga_init(PCIBus *pci_bus, Error **errp)
>  {
> +    vga_interface_created = true;
>      switch (vga_interface_type) {
>      case VGA_NONE:
>          return false;
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index fccaed1eb4..b693eea0e0 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -920,6 +920,7 @@ static void sun4m_hw_init(MachineState *machine)
>              /* sbus irq 5 */
>              cg3_init(hwdef->tcx_base, slavio_irq[11], 0x00100000,
>                       graphic_width, graphic_height, graphic_depth);
> +            vga_interface_created = true;
>          } else {
>              /* If no display specified, default to TCX */
>              if (graphic_depth != 8 && graphic_depth != 24) {
> @@ -935,6 +936,7 @@ static void sun4m_hw_init(MachineState *machine)
>  
>              tcx_init(hwdef->tcx_base, slavio_irq[11], 0x00100000,
>                       graphic_width, graphic_height, graphic_depth);
> +            vga_interface_created = true;
>          }
>      }
>  
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index 6fd08e2298..7c461d194a 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -632,6 +632,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>      switch (vga_interface_type) {
>      case VGA_STD:
>          pci_create_simple(pci_busA, PCI_DEVFN(2, 0), "VGA");
> +        vga_interface_created = true;
>          break;
>      case VGA_NONE:
>          break;
> diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
> index 8df575a457..20c9611d71 100644
> --- a/hw/xenpv/xen_machine_pv.c
> +++ b/hw/xenpv/xen_machine_pv.c
> @@ -63,6 +63,7 @@ static void xen_init_pv(MachineState *machine)
>      if (vga_interface_type == VGA_XENFB) {
>          xen_config_dev_vfb(0, "vnc");
>          xen_config_dev_vkbd(0);
> +        vga_interface_created = true;
>      }
>  
>      /* configure disks */
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 10e283c170..360a408edf 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -34,6 +34,7 @@ typedef enum {
>  } VGAInterfaceType;
>  
>  extern int vga_interface_type;
> +extern bool vga_interface_created;
>  
>  extern int graphic_width;
>  extern int graphic_height;
> diff --git a/softmmu/globals.c b/softmmu/globals.c
> index 3ebd718e35..98b64e0492 100644
> --- a/softmmu/globals.c
> +++ b/softmmu/globals.c
> @@ -40,6 +40,7 @@ int nb_nics;
>  NICInfo nd_table[MAX_NICS];
>  int autostart = 1;
>  int vga_interface_type = VGA_NONE;
> +bool vga_interface_created;
>  Chardev *parallel_hds[MAX_PARALLEL_PORTS];
>  int win2k_install_hack;
>  int singlestep;
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 06a0e342fe..8411cb15af 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2734,6 +2734,12 @@ static void qemu_machine_creation_done(void)
>      if (foreach_device_config(DEV_GDB, gdbserver_start) < 0) {
>          exit(1);
>      }
> +    if (!vga_interface_created && !default_vga &&
> +        vga_interface_type != VGA_NONE) {
> +        warn_report("A -vga option was passed but this machine "
> +                    "type does not use that option; "
> +                    "No VGA device has been created");
> +    }
>  }
>  
>  void qmp_x_exit_preconfig(Error **errp)
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3] Warn user if the vga flag is passed but no vga device is created

[Thomas Huth]

On 06/05/2022 16.24, Stefan Hajnoczi wrote:
> On Sun, May 01, 2022 at 05:55:05PM +0530, Gautam Agrawal wrote:
>> A global boolean variable "vga_interface_created"(declared in softmmu/globals.c)
>> has been used to track the creation of vga interface. If the vga flag is passed
>> in the command line "default_vga"(declared in softmmu/vl.c) variable is set to 0.
>> To warn user, the condition checks if vga_interface_created is false
>> and default_vga is equal to 0. If "-vga none" is passed, this patch will not warn the
>> user regarding the creation of VGA device.
>>
>> The warning "A -vga option was passed but this
>> machine type does not use that option; no VGA device has been created"
>> is logged if vga flag is passed but no vga device is created.
>>
>> This patch has been tested for x86_64, i386, sparc, sparc64 and arm boards.
>>
>> Signed-off-by: Gautam Agrawal <gautamnagrawal@gmail.com>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/581
>> ---
>>   hw/hppa/machine.c         | 1 +
>>   hw/isa/isa-bus.c          | 1 +
>>   hw/mips/fuloong2e.c       | 1 +
>>   hw/pci/pci.c              | 1 +
>>   hw/ppc/spapr.c            | 1 +
>>   hw/sparc/sun4m.c          | 2 ++
>>   hw/sparc64/sun4u.c        | 1 +
>>   hw/xenpv/xen_machine_pv.c | 1 +
>>   include/sysemu/sysemu.h   | 1 +
>>   softmmu/globals.c         | 1 +
>>   softmmu/vl.c              | 6 ++++++
>>   11 files changed, 17 insertions(+)
> 
> Gerd or Richard: do you want to merge this patch?

I'm just in progress of preparing a pull request with misc patches, I can 
also throw it in there if nobody minds.

  Thomas

Re: [PATCH v3] Warn user if the vga flag is passed but no vga device is created

[Peter Maydell]

On Fri, 6 May 2022 at 15:32, Thomas Huth <thuth@redhat.com> wrote:
>
> On 06/05/2022 16.24, Stefan Hajnoczi wrote:
> > On Sun, May 01, 2022 at 05:55:05PM +0530, Gautam Agrawal wrote:
> >> A global boolean variable "vga_interface_created"(declared in softmmu/globals.c)
> >> has been used to track the creation of vga interface. If the vga flag is passed
> >> in the command line "default_vga"(declared in softmmu/vl.c) variable is set to 0.
> >> To warn user, the condition checks if vga_interface_created is false
> >> and default_vga is equal to 0. If "-vga none" is passed, this patch will not warn the
> >> user regarding the creation of VGA device.
> >>
> >> The warning "A -vga option was passed but this
> >> machine type does not use that option; no VGA device has been created"
> >> is logged if vga flag is passed but no vga device is created.
> >>
> >> This patch has been tested for x86_64, i386, sparc, sparc64 and arm boards.
> >>
> >> Signed-off-by: Gautam Agrawal <gautamnagrawal@gmail.com>
> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/581
> >> ---
> >>   hw/hppa/machine.c         | 1 +
> >>   hw/isa/isa-bus.c          | 1 +
> >>   hw/mips/fuloong2e.c       | 1 +
> >>   hw/pci/pci.c              | 1 +
> >>   hw/ppc/spapr.c            | 1 +
> >>   hw/sparc/sun4m.c          | 2 ++
> >>   hw/sparc64/sun4u.c        | 1 +
> >>   hw/xenpv/xen_machine_pv.c | 1 +
> >>   include/sysemu/sysemu.h   | 1 +
> >>   softmmu/globals.c         | 1 +
> >>   softmmu/vl.c              | 6 ++++++
> >>   11 files changed, 17 insertions(+)
> >
> > Gerd or Richard: do you want to merge this patch?
>
> I'm just in progress of preparing a pull request with misc patches, I can
> also throw it in there if nobody minds.

Paolo mentioned on IRC yesterday that there was some detail he thought
it wasn't handling right with VGA_DEVICE, but I didn't really understand
the details. Paolo ?

thanks
-- PMM

Re: [PATCH v3] Warn user if the vga flag is passed but no vga device is created

[Paolo Bonzini]

On 5/6/22 16:48, Peter Maydell wrote:
>> I'm just in progress of preparing a pull request with misc patches, I can
>> also throw it in there if nobody minds.
> Paolo mentioned on IRC yesterday that there was some detail he thought
> it wasn't handling right with VGA_DEVICE, but I didn't really understand
> the details. Paolo ?

Yeah, I was wondering if this would warn for "-device VGA".  But if so 
it should be enough to do this to fix it:

diff --git a/softmmu/vl.c b/softmmu/vl.c
index eef1558281..7ff76b795a 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1352,6 +1352,7 @@ static void qemu_disable_default_devices(void)

      if (!vga_model && !default_vga) {
          vga_interface_type = VGA_DEVICE;
+        vga_interface_created = true;
      }
      if (!has_defaults || machine_class->no_serial) {
          default_serial = 0;

Paolo

Re: [PATCH v3] Warn user if the vga flag is passed but no vga device is created

[Peter Maydell]

On Fri, 6 May 2022 at 16:43, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 5/6/22 16:48, Peter Maydell wrote:
> >> I'm just in progress of preparing a pull request with misc patches, I can
> >> also throw it in there if nobody minds.
> > Paolo mentioned on IRC yesterday that there was some detail he thought
> > it wasn't handling right with VGA_DEVICE, but I didn't really understand
> > the details. Paolo ?
>
> Yeah, I was wondering if this would warn for "-device VGA".  But if so
> it should be enough to do this to fix it:
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index eef1558281..7ff76b795a 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1352,6 +1352,7 @@ static void qemu_disable_default_devices(void)
>
>       if (!vga_model && !default_vga) {
>           vga_interface_type = VGA_DEVICE;
> +        vga_interface_created = true;
>       }
>       if (!has_defaults || machine_class->no_serial) {
>           default_serial = 0;

Can you explain why that's right? qemu_disable_default_devices()
isn't creating any devices at all, so it's not clear to me
(a) why it's setting vga_interface_type or (b) why setting
vga_interface_created to true is OK.

What I would have expected would have been some kind
of callback where the device created with -device whatever
arranged to set vga_interface_type to VGA_DEVICE when
it was created; but that's clearly not how the code works,
so I'm confused about how it does work...

thanks
-- PMM

Re: [PATCH v3] Warn user if the vga flag is passed but no vga device is created

[Paolo Bonzini]

On 5/6/22 17:47, Peter Maydell wrote:
>>        if (!vga_model && !default_vga) {
>>            vga_interface_type = VGA_DEVICE;
>> +          vga_interface_created = true;
>>        }
>>        if (!has_defaults || machine_class->no_serial) {
>>            default_serial = 0;
>
> Can you explain why that's right? qemu_disable_default_devices()
> isn't creating any devices at all, so it's not clear to me
> (a) why it's setting vga_interface_type or (b) why setting
> vga_interface_created to true is OK.

VGA_DEVICE means the device has been specified on the command line, but 
the board should otherwise behave as if "-vga something" was there.

While the device has not been created yet, it will be in 
qemu_create_cli_devices(), and that's what !default_vga means at this 
point of the function.

This in fact means that almost all three occurrences of 
"vga_interface_type != VGA_NONE" are wrong. :(

Paolo

Re: [PATCH v3] Warn user if the vga flag is passed but no vga device is created

[Peter Maydell]

On Fri, 6 May 2022 at 17:10, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 5/6/22 17:47, Peter Maydell wrote:
> >>        if (!vga_model && !default_vga) {
> >>            vga_interface_type = VGA_DEVICE;
> >> +          vga_interface_created = true;
> >>        }
> >>        if (!has_defaults || machine_class->no_serial) {
> >>            default_serial = 0;
> >
> > Can you explain why that's right? qemu_disable_default_devices()
> > isn't creating any devices at all, so it's not clear to me
> > (a) why it's setting vga_interface_type or (b) why setting
> > vga_interface_created to true is OK.
>
> VGA_DEVICE means the device has been specified on the command line, but
> the board should otherwise behave as if "-vga something" was there.

Oh, I see now -- qemu_disable_default_devices() does a
preliminary scan through of every supplied -device option,
looking to see if it has a driver=foo that matches some
value in the default_list[] array; if it does then we
set default_vga or whatever to false. (So effectively we
have a hardcoded list of things we consider to be "VGA
devices" for this purpose, which might or might not be the same
as the set of actual VGA devices we support...)

I guess this is all here for backwards compatibility purposes?
I kind of expect that short options like '-vga' might have
magic extra behaviour beyond "create a device", but having
some devices that have magic "make the board behave differently"
behaviour when they're created with '-device' is a bit
unexpected to me.

thanks
-- PMM

Re: [PATCH v3] Warn user if the vga flag is passed but no vga device is created

[Thomas Huth]

On 06/05/2022 17.43, Paolo Bonzini wrote:
> On 5/6/22 16:48, Peter Maydell wrote:
>>> I'm just in progress of preparing a pull request with misc patches, I can
>>> also throw it in there if nobody minds.
>> Paolo mentioned on IRC yesterday that there was some detail he thought
>> it wasn't handling right with VGA_DEVICE, but I didn't really understand
>> the details. Paolo ?
> 
> Yeah, I was wondering if this would warn for "-device VGA".

Yes, it's possible to trigger the warning like this:

$ ./qemu-system-ppc -M ppce500 -device VGA
qemu-system-ppc: warning: A -vga option was passed but this machine type 
does not use that option; No VGA device has been created

> But if so it should be enough to do this to fix it:
> 
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index eef1558281..7ff76b795a 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1352,6 +1352,7 @@ static void qemu_disable_default_devices(void)
> 
>       if (!vga_model && !default_vga) {
>           vga_interface_type = VGA_DEVICE;
> +        vga_interface_created = true;
>       }
>       if (!has_defaults || machine_class->no_serial) {
>           default_serial = 0;

That fixes the warning, indeed. Thanks, I'll fix up the patch with this hunk 
and respin my pull request.

  Thomas

Re: [PATCH v3] Warn user if the vga flag is passed but no vga device is created

[Gerd Hoffmann]

  Hi,

> Oh, I see now -- qemu_disable_default_devices() does a
> preliminary scan through of every supplied -device option,
> looking to see if it has a driver=foo that matches some
> value in the default_list[] array; if it does then we
> set default_vga or whatever to false. (So effectively we
> have a hardcoded list of things we consider to be "VGA
> devices" for this purpose, which might or might not be the same
> as the set of actual VGA devices we support...)
> 
> I guess this is all here for backwards compatibility purposes?

Well, sort of.  This tries to deal with the messy side effects of
the default devices, i.e. avoid users end up with *two* vga devices
in case they use "qemu -device VGA" (same for serial, ...).

take care,
  Gerd