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

[Thomas Huth <thuth@redhat.com>]

  Hi,

thanks for your patch, looks pretty good already, but there is a small 
issue: Try for example:

  ./qemu-system-s390x -vga none

... and it will print the warning "qemu-system-s390x: warning: No vga device 
is created", though the user only asked for no VGA device. This seems to 
happen if a machine does not have any VGA device by default, but still 
requests "-vga none" on the command line.

Some more comments below...

On 08/04/2022 12.45, Gautam Agrawal wrote:
> This patch is in regards to this issue:https://gitlab.com/qemu-project/qemu/-/issues/581#.

Better write this right in front of your Signed-off-by line:

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/581

... then the ticket will be automatically be closed once your patch gets merged.

> 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.
> 
> The warning "No vga device is 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>
> ---
>   hw/isa/isa-bus.c        | 1 +
>   hw/pci/pci.c            | 1 +
>   hw/sparc/sun4m.c        | 2 ++
>   hw/sparc64/sun4u.c      | 1 +
>   include/sysemu/sysemu.h | 1 +
>   softmmu/globals.c       | 1 +
>   softmmu/vl.c            | 3 +++
>   7 files changed, 10 insertions(+)

vga_interface_type is also used in hw/mips/fuloong2e.c and 
hw/xenpv/xen_machine_pv.c ... do they need a change, too?

> 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/pci/pci.c b/hw/pci/pci.c
> index dae9119bfe..fab9c80f8d 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2038,6 +2038,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/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 7f3a7c0027..f45e29acc8 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -921,6 +921,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) {
> @@ -936,6 +937,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 cda7df36e3..75334dba71 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -633,6 +633,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/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index b9421e03ff..a558b895e4 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -32,6 +32,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..1a5f8d42ad 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 = false;

This will trigger a warning from the scripts/checkpatch.pl script:

ERROR: do not initialise globals to 0 or NULL
#238: FILE: softmmu/globals.c:43:
+bool vga_interface_created = false;

>   Chardev *parallel_hds[MAX_PARALLEL_PORTS];
>   int win2k_install_hack;
>   int singlestep;
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 6f646531a0..cb79fa1f42 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2734,6 +2734,9 @@ static void qemu_machine_creation_done(void)
>       if (foreach_device_config(DEV_GDB, gdbserver_start) < 0) {
>           exit(1);
>       }
> +    if (!vga_interface_created && !default_vga) {
> +        warn_report("No vga device is created");

I'm not a native speaker, and maybe it's just a matter of taste, but I'd 
rather say it in past tense: "No VGA device has been created"

> +    }
>   }

  Regards,
   Thomas