[Qemu-devel] [6322] add a -vga none cli option (Stefano Stabellini)

[Qemu-devel] [6322] add a -vga none cli option (Stefano Stabellini)

[Anthony Liguori]

Revision: 6322
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6322
Author:   aliguori
Date:     2009-01-15 20:37:28 +0000 (Thu, 15 Jan 2009)

Log Message:
-----------
add a -vga none cli option (Stefano Stabellini)

currently there is no way to fully disable any graphic card device for
the PC architecture.
You can have no graphical output, thanks to -nographic, but you would
have the VGA device connected to your PCI bus anyway.
There is already a convenient -vga option to choose between std, cirrus
and vmware; this patch add the new option "none" to select no graphic
card at all.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Modified Paths:
--------------
    trunk/hw/pc.c
    trunk/sysemu.h
    trunk/vl.c

Modified: trunk/hw/pc.c
===================================================================
--- trunk/hw/pc.c	2009-01-15 20:16:51 UTC (rev 6321)
+++ trunk/hw/pc.c	2009-01-15 20:37:28 UTC (rev 6322)
@@ -852,22 +852,24 @@
         exit(1);
     }
 
-    /* VGA BIOS load */
-    if (cirrus_vga_enabled) {
-        snprintf(buf, sizeof(buf), "%s/%s", bios_dir, VGABIOS_CIRRUS_FILENAME);
-    } else {
-        snprintf(buf, sizeof(buf), "%s/%s", bios_dir, VGABIOS_FILENAME);
-    }
-    vga_bios_size = get_image_size(buf);
-    if (vga_bios_size <= 0 || vga_bios_size > 65536)
-        goto vga_bios_error;
-    vga_bios_offset = qemu_ram_alloc(65536);
+    if (cirrus_vga_enabled || std_vga_enabled || vmsvga_enabled) {
+        /* VGA BIOS load */
+        if (cirrus_vga_enabled) {
+            snprintf(buf, sizeof(buf), "%s/%s", bios_dir, VGABIOS_CIRRUS_FILENAME);
+        } else {
+            snprintf(buf, sizeof(buf), "%s/%s", bios_dir, VGABIOS_FILENAME);
+        }
+        vga_bios_size = get_image_size(buf);
+        if (vga_bios_size <= 0 || vga_bios_size > 65536)
+            goto vga_bios_error;
+        vga_bios_offset = qemu_ram_alloc(65536);
 
-    ret = load_image(buf, phys_ram_base + vga_bios_offset);
-    if (ret != vga_bios_size) {
-    vga_bios_error:
-        fprintf(stderr, "qemu: could not load VGA BIOS '%s'\n", buf);
-        exit(1);
+        ret = load_image(buf, phys_ram_base + vga_bios_offset);
+        if (ret != vga_bios_size) {
+vga_bios_error:
+            fprintf(stderr, "qemu: could not load VGA BIOS '%s'\n", buf);
+            exit(1);
+        }
     }
 
     /* setup basic memory access */
@@ -956,7 +958,7 @@
                             vga_ram_addr, vga_ram_size);
         else
             fprintf(stderr, "%s: vmware_vga: no PCI bus\n", __FUNCTION__);
-    } else {
+    } else if (std_vga_enabled) {
         if (pci_enabled) {
             pci_vga_init(pci_bus, ds, phys_ram_base + vga_ram_addr,
                          vga_ram_addr, vga_ram_size, 0, 0);

Modified: trunk/sysemu.h
===================================================================
--- trunk/sysemu.h	2009-01-15 20:16:51 UTC (rev 6321)
+++ trunk/sysemu.h	2009-01-15 20:37:28 UTC (rev 6322)
@@ -83,6 +83,7 @@
 
 extern int bios_size;
 extern int cirrus_vga_enabled;
+extern int std_vga_enabled;
 extern int vmsvga_enabled;
 extern int graphic_width;
 extern int graphic_height;

Modified: trunk/vl.c
===================================================================
--- trunk/vl.c	2009-01-15 20:16:51 UTC (rev 6321)
+++ trunk/vl.c	2009-01-15 20:37:28 UTC (rev 6322)
@@ -192,6 +192,7 @@
 static int rtc_utc = 1;
 static int rtc_date_offset = -1; /* -1 means no change */
 int cirrus_vga_enabled = 1;
+int std_vga_enabled = 0;
 int vmsvga_enabled = 0;
 #ifdef TARGET_SPARC
 int graphic_width = 1024;
@@ -3873,7 +3874,7 @@
            "                use -soundhw ? to get the list of supported cards\n"
            "                use -soundhw all to enable all of them\n"
 #endif
-           "-vga [std|cirrus|vmware]\n"
+           "-vga [std|cirrus|vmware|none]\n"
            "                select video card type\n"
            "-localtime      set the real time clock to local time [default=utc]\n"
            "-full-screen    start in full screen\n"
@@ -4407,14 +4408,21 @@
     const char *opts;
 
     if (strstart(p, "std", &opts)) {
+        std_vga_enabled = 1;
         cirrus_vga_enabled = 0;
         vmsvga_enabled = 0;
     } else if (strstart(p, "cirrus", &opts)) {
         cirrus_vga_enabled = 1;
+        std_vga_enabled = 0;
         vmsvga_enabled = 0;
     } else if (strstart(p, "vmware", &opts)) {
         cirrus_vga_enabled = 0;
+        std_vga_enabled = 0;
         vmsvga_enabled = 1;
+    } else if (strstart(p, "none", &opts)) {
+        cirrus_vga_enabled = 0;
+        std_vga_enabled = 0;
+        vmsvga_enabled = 0;
     } else {
     invalid_vga:
         fprintf(stderr, "Unknown vga type: %s\n", p);

[Qemu-devel] Re: [6322] add a -vga none cli option (Stefano Stabellini)

[Jan Kiszka]

Anthony Liguori wrote:
> Revision: 6322
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6322
> Author:   aliguori
> Date:     2009-01-15 20:37:28 +0000 (Thu, 15 Jan 2009)
> 
> Log Message:
> -----------
> add a -vga none cli option (Stefano Stabellini)
> 
> currently there is no way to fully disable any graphic card device for
> the PC architecture.
> You can have no graphical output, thanks to -nographic, but you would
> have the VGA device connected to your PCI bus anyway.
> There is already a convenient -vga option to choose between std, cirrus
> and vmware; this patch add the new option "none" to select no graphic
> card at all.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> Modified Paths:
> --------------
>     trunk/hw/pc.c
>     trunk/sysemu.h
>     trunk/vl.c
> 
> Modified: trunk/hw/pc.c
> ===================================================================
> --- trunk/hw/pc.c	2009-01-15 20:16:51 UTC (rev 6321)
> +++ trunk/hw/pc.c	2009-01-15 20:37:28 UTC (rev 6322)
> @@ -852,22 +852,24 @@
>          exit(1);
>      }
>  
> -    /* VGA BIOS load */
> -    if (cirrus_vga_enabled) {
> -        snprintf(buf, sizeof(buf), "%s/%s", bios_dir, VGABIOS_CIRRUS_FILENAME);
> -    } else {
> -        snprintf(buf, sizeof(buf), "%s/%s", bios_dir, VGABIOS_FILENAME);
> -    }
> -    vga_bios_size = get_image_size(buf);
> -    if (vga_bios_size <= 0 || vga_bios_size > 65536)
> -        goto vga_bios_error;
> -    vga_bios_offset = qemu_ram_alloc(65536);
> +    if (cirrus_vga_enabled || std_vga_enabled || vmsvga_enabled) {
> +        /* VGA BIOS load */
> +        if (cirrus_vga_enabled) {
> +            snprintf(buf, sizeof(buf), "%s/%s", bios_dir, VGABIOS_CIRRUS_FILENAME);
> +        } else {
> +            snprintf(buf, sizeof(buf), "%s/%s", bios_dir, VGABIOS_FILENAME);
> +        }
> +        vga_bios_size = get_image_size(buf);
> +        if (vga_bios_size <= 0 || vga_bios_size > 65536)
> +            goto vga_bios_error;
> +        vga_bios_offset = qemu_ram_alloc(65536);
>  
> -    ret = load_image(buf, phys_ram_base + vga_bios_offset);
> -    if (ret != vga_bios_size) {
> -    vga_bios_error:
> -        fprintf(stderr, "qemu: could not load VGA BIOS '%s'\n", buf);
> -        exit(1);
> +        ret = load_image(buf, phys_ram_base + vga_bios_offset);
> +        if (ret != vga_bios_size) {
> +vga_bios_error:
> +            fprintf(stderr, "qemu: could not load VGA BIOS '%s'\n", buf);
> +            exit(1);
> +        }
>      }
>  
>      /* setup basic memory access */

This hunk now generates:

qemu/hw/pc.c: In function ‘pc_init1’:
qemu/hw/pc.c:762: warning: ‘vga_bios_offset’ may be used uninitialized in this function                                                                                                       

For obvious reasons. Can we simply make the related
cpu_register_physical_memory conditional as well?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [6322] add a -vga none cli option (Stefano Stabellini)

[Stefano Stabellini]

Jan Kiszka wrote:

> This hunk now generates:
> 
> qemu/hw/pc.c: In function â¬Üpc_init1â¬":
> qemu/hw/pc.c:762: warning: â¬Üvga_bios_offsetâ¬" may be used uninitialized in this function 
> 
> For obvious reasons. Can we simply make the related
> cpu_register_physical_memory conditional as well?
> 
> Jan
> 



You are right that patch caused variable initialization issues, this
patch should fix them.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff --git a/hw/pc.c b/hw/pc.c
index d64e442..ff0f16d 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -759,7 +759,7 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
 {
     char buf[1024];
     int ret, linux_boot, i;
-    ram_addr_t ram_addr, vga_ram_addr, bios_offset, vga_bios_offset;
+    ram_addr_t ram_addr, vga_ram_addr = 0x00, bios_offset, vga_bios_offset;
     ram_addr_t below_4g_mem_size, above_4g_mem_size = 0;
     int bios_size, isa_bios_size, vga_bios_size;
     PCIBus *pci_bus;
@@ -831,10 +831,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
                                      ram_addr);
     }
 
-
-    /* allocate VGA RAM */
-    vga_ram_addr = qemu_ram_alloc(vga_ram_size);
-
     /* BIOS load */
     if (bios_name == NULL)
         bios_name = BIOS_FILENAME;
@@ -853,6 +849,9 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
     }
 
     if (cirrus_vga_enabled || std_vga_enabled || vmsvga_enabled) {
+        /* allocate VGA RAM */
+        vga_ram_addr = qemu_ram_alloc(vga_ram_size);
+
         /* VGA BIOS load */
         if (cirrus_vga_enabled) {
             snprintf(buf, sizeof(buf), "%s/%s", bios_dir, VGABIOS_CIRRUS_FILENAME);
@@ -870,11 +869,11 @@ vga_bios_error:
             fprintf(stderr, "qemu: could not load VGA BIOS '%s'\n", buf);
             exit(1);
         }
-    }
 
-    /* setup basic memory access */
-    cpu_register_physical_memory(0xc0000, 0x10000,
-                                 vga_bios_offset | IO_MEM_ROM);
+        /* setup basic memory access */
+        cpu_register_physical_memory(0xc0000, 0x10000,
+                vga_bios_offset | IO_MEM_ROM);
+    }
 
     /* map the last 128KB of the BIOS in ISA space */
     isa_bios_size = bios_size;

Re: [Qemu-devel] Re: [6322] add a -vga none cli option (Stefano Stabellini)

[Anthony Liguori]

Jan Kiszka wrote:
> This hunk now generates:
>
> qemu/hw/pc.c: In function ‘pc_init1’:
> qemu/hw/pc.c:762: warning: ‘vga_bios_offset’ may be used uninitialized in this function                                                                                                       
>
> For obvious reasons. Can we simply make the related
> cpu_register_physical_memory conditional as well?
>   

Yup, fixed now.  Thanks.

Regards,

Anthony Liguori

> Jan
>
>   

Re: [Qemu-devel] Re: [6322] add a -vga none cli option (Stefano Stabellini)

[Anthony Liguori]

Stefano Stabellini wrote:
> Jan Kiszka wrote:
>
>   
>> This hunk now generates:
>>
>> qemu/hw/pc.c: In function â¬Üpc_init1â¬":
>> qemu/hw/pc.c:762: warning: â¬Üvga_bios_offsetâ¬" may be used uninitialized in this function 
>>
>> For obvious reasons. Can we simply make the related
>> cpu_register_physical_memory conditional as well?
>>
>> Jan
>>
>>     
>
>
>
> You are right that patch caused variable initialization issues, this
> patch should fix them.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> diff --git a/hw/pc.c b/hw/pc.c
> index d64e442..ff0f16d 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -759,7 +759,7 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
>  {
>      char buf[1024];
>      int ret, linux_boot, i;
> -    ram_addr_t ram_addr, vga_ram_addr, bios_offset, vga_bios_offset;
> +    ram_addr_t ram_addr, vga_ram_addr = 0x00, bios_offset, vga_bios_offset;
>      ram_addr_t below_4g_mem_size, above_4g_mem_size = 0;
>      int bios_size, isa_bios_size, vga_bios_size;
>      PCIBus *pci_bus;
> @@ -831,10 +831,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
>                                       ram_addr);
>      }
>  
> -
> -    /* allocate VGA RAM */
> -    vga_ram_addr = qemu_ram_alloc(vga_ram_size);
> -
>   

It's a little safer to leave this allocation in place because it 
maintains the 1-1 mapping of phys_ram_base and low memory.  
Unfortunately, I think there is still code that depends on this.

Regards,

Anthony Liguroi

[Qemu-devel] Re: [6322] add a -vga none cli option (Stefano Stabellini)

[Sebastian Herbszt]

Anthony Liguori wrote:
> Revision: 6322
>          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6322
> Author:   aliguori
> Date:     2009-01-15 20:37:28 +0000 (Thu, 15 Jan 2009)
> 
> Log Message:
> -----------
> add a -vga none cli option (Stefano Stabellini)
> 
> currently there is no way to fully disable any graphic card device for
> the PC architecture.
> You can have no graphical output, thanks to -nographic, but you would
> have the VGA device connected to your PCI bus anyway.
> There is already a convenient -vga option to choose between std, cirrus
> and vmware; this patch add the new option "none" to select no graphic
> card at all.

I just tried this with r6367 on WinXP and SDL support as

qemu.exe -L . -hda dummy.img -vga none

and it made qemu crash. This happens on the call to
"sdl_display_init(ds, full_screen, no_frame);" in vl.c.

Maybe "-vga none" should imply "-nographic" ?

- Sebastian

Re: [Qemu-devel] Re: [6322] add a -vga none cli option (Stefano Stabellini)

[Daniel P. Berrange]

On Sun, Jan 18, 2009 at 10:39:58PM +0100, Sebastian Herbszt wrote:
> Anthony Liguori wrote:
> >Revision: 6322
> >         http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6322
> >Author:   aliguori
> >Date:     2009-01-15 20:37:28 +0000 (Thu, 15 Jan 2009)
> >
> >Log Message:
> >-----------
> >add a -vga none cli option (Stefano Stabellini)
> >
> >currently there is no way to fully disable any graphic card device for
> >the PC architecture.
> >You can have no graphical output, thanks to -nographic, but you would
> >have the VGA device connected to your PCI bus anyway.
> >There is already a convenient -vga option to choose between std, cirrus
> >and vmware; this patch add the new option "none" to select no graphic
> >card at all.
> 
> I just tried this with r6367 on WinXP and SDL support as
> 
> qemu.exe -L . -hda dummy.img -vga none
> 
> and it made qemu crash. This happens on the call to
> "sdl_display_init(ds, full_screen, no_frame);" in vl.c.
> 
> Maybe "-vga none" should imply "-nographic" ?

The VGA adapter isn't the only thing being displayed on the SDL/VNC
display - you potentially have the monitor console, and serial / parallel
devices & anything else that's using one of QEMU's text consoles. So it 
is correct that '-vga none' is independant of '-nographic' & thus it is 
neccessary to fix the crash seen.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] Re: [6322] add a -vga none cli option (Stefano Stabellini)

[Stefano Stabellini]

Daniel P. Berrange wrote:

>> I just tried this with r6367 on WinXP and SDL support as
>>
>> qemu.exe -L . -hda dummy.img -vga none
>>
>> and it made qemu crash. This happens on the call to
>> "sdl_display_init(ds, full_screen, no_frame);" in vl.c.
>>
>> Maybe "-vga none" should imply "-nographic" ?
> 
> The VGA adapter isn't the only thing being displayed on the SDL/VNC
> display - you potentially have the monitor console, and serial / parallel
> devices & anything else that's using one of QEMU's text consoles. So it 
> is correct that '-vga none' is independant of '-nographic' & thus it is 
> neccessary to fix the crash seen.
> 



Yes the two options should be independent but currently they are not
because the qemu console code doesn't allocate and manage its own
DisplayState but uses the one allocated by the graphic card instead.
This is a known problem and when we implement multiple DisplayState
handling, this bug will be fixed properly.

At the moment we either set nographic = 1 when "-vga none" or we go with
the following patch.

Please note that this patch is alternative to the other patch I sent
today as a reply to the thread "Adds null check for DisplayStatus", and
fixes that bug too.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

diff --git a/vl.c b/vl.c
index bfacdcf..b356323 100644
--- a/vl.c
+++ b/vl.c
@@ -2774,26 +2774,17 @@ DisplayState *get_displaystate(void)
 }

 /* dumb display */
-
-static void dumb_update(DisplayState *ds, int x, int y, int w, int h)
+static void dumb_display_init(void)
 {
-}
-
-static void dumb_resize(DisplayState *ds)
-{
-}
+    DisplayState *ds;

-static void dumb_display_init(DisplayState *ds)
-{
-    DisplayChangeListener *dcl =
qemu_mallocz(sizeof(DisplayChangeListener));
-    if (!dcl)
+    ds = (DisplayState *) qemu_mallocz(sizeof(DisplayState));
+    if (ds == NULL) {
+        fprintf(stderr, "dumb_display_init: DisplayState allocation
failed\n");
         exit(1);
-    dcl->dpy_update = dumb_update;
-    dcl->dpy_resize = dumb_resize;
-    dcl->dpy_refresh = NULL;
-    dcl->idle = 1;
-    dcl->gui_timer_interval = 500;
-    register_displaychangelistener(ds, dcl);
+    }
+    ds->surface = qemu_create_displaysurface(640, 480, 32, 640 * 4);
+    register_displaystate(ds);
 }

 /***********************************************************/
@@ -5535,6 +5526,8 @@ int main(int argc, char **argv, char **envp)
         }
     }

+    if (!display_state)
+        dumb_display_init();
     /* just use the first displaystate for the moment */
     ds = display_state;
     /* terminal init */
@@ -5543,8 +5536,6 @@ int main(int argc, char **argv, char **envp)
             fprintf(stderr, "fatal: -nographic can't be used with
-curses\n");
             exit(1);
         }
-        /* nearly nothing to do */
-        dumb_display_init(ds);
     } else {
 #if defined(CONFIG_CURSES)
             if (curses) {
@@ -5563,8 +5554,6 @@ int main(int argc, char **argv, char **envp)
                     sdl_display_init(ds, full_screen, no_frame);
 #elif defined(CONFIG_COCOA)
                     cocoa_display_init(ds, full_screen);
-#else
-                    dumb_display_init(ds);
 #endif
             }
     }