Re: [Qemu-devel] [PATCH 1/2] Add a generic vga device type for that specified by '-device'

[Paolo Bonzini <pbonzini@redhat.com>]

Il 06/03/2014 15:39, Andreas Färber ha scritto:
> Am 06.03.2014 15:33, schrieb Paolo Bonzini:
>> Il 06/03/2014 14:37, Mark Wu ha scritto:
>>> Thanks for your reply!  I need confirm I am understanding your comments
>>> correctly.  I think you're suggesting to traverse the pci devices and
>>> check if it owns the I/O port 0x3d4 to detect if the vga device
>>> is initialized.    But it seems not be able to resolve the bug. Because
>>> the machine initialization code runs before the generic device
>>> initialization, the I/O port 0x3d4 will not be registered at the time
>>> machine initializes.  So it can't change the return value of
>>> pci_vga_init.  The return value is checked in ppc code, which causes the
>>> bug.
>>
>> Right.  What about looking for any PCI device with VGA class?
> 
> Since VGA doesn't need to be PCI (e.g., ISA, SysBus) maybe it would be a
> good idea to add a QOM interface to those devices?

Actually, I now understand the problem better.  All of this happens
before the VGA device is even created, so it cannot be done with QOM.
It's basically a command-line parsing problem.

There are four cases:

1) "-vga none" on the command line

2) "-device VGA" on the command line

3) "-nodefaults" on the command line

4) "-nodefaults -device VGA" on the command line

sPAPR wants to enable graphics in the second and fourth case.  However, 
this is not exactly what the patch does, as I read it, because it actually
enables graphics in the third and fourth cases, and not in the second
case.  Both the third and the fourth case have !has_defaults, and with
has_defaults == 0 you always have default_vga == 0.

The last three cases have "default_vga == 0" in vl.c.  The problem is 
that -nodefaults sets default_vga to 0 too early.  Perhaps something 
like this would work:

diff --git a/vl.c b/vl.c
index 685a7a9..d9afff4 100644
--- a/vl.c
+++ b/vl.c
@@ -213,6 +213,7 @@ ---> add a new variable
 enum xen_mode xen_mode = XEN_EMULATE;
 static int tcg_tb_size;
 
+static int no_defaults = 0;
 static int default_serial = 1;
 static int default_parallel = 1;
 static int default_virtcon = 1;
@@ -2041,7 +2042,7 @@ ---> select_vgahw is never called with VGA_DEVICE
 {
     const char *opts;
 
-    vga_interface_type = VGA_NONE;
+    assert(vga_interface_type == VGA_NONE);
     if (strstart(p, "std", &opts)) {
         if (vga_available()) {
             vga_interface_type = VGA_STD;
@@ -2825,7 +2826,7 @@ ---> need to detect -vga
     const char *loadvm = NULL;
     QEMUMachine *machine;
     const char *cpu_model;
-    const char *vga_model = "none";
+    const char *vga_model = NULL;
     const char *qtest_chrdev = NULL;
     const char *qtest_log = NULL;
     const char *pid_file = NULL;
@@ -3682,16 +3683,7 @@ ---> just set a flag here
                 runstate_set(RUN_STATE_INMIGRATE);
                 break;
             case QEMU_OPTION_nodefaults:
-                default_serial = 0;
-                default_parallel = 0;
-                default_virtcon = 0;
-                default_sclp = 0;
-                default_monitor = 0;
-                default_net = 0;
-                default_floppy = 0;
-                default_cdrom = 0;
-                default_sdcard = 0;
-                default_vga = 0;
+                no_defaults = 1;
                 break;
             case QEMU_OPTION_xen_domid:
                 if (!(xen_available())) {
@@ -3918,27 +3910,35 @@ ---> detect "-device VGA" here, also disable defaults for -nodefaults
     qemu_opts_foreach(qemu_find_opts("device"), default_driver_check, NULL, 0);
     qemu_opts_foreach(qemu_find_opts("global"), default_driver_check, NULL, 0);
 
-    if (machine->no_serial) {
+    if (!vga_model && !default_vga) {
+        vga_interface_type = VGA_DEVICE;
+    }
+    if (no_defaults || machine->no_serial) {
         default_serial = 0;
     }
-    if (machine->no_parallel) {
+    if (no_defaults || machine->no_parallel) {
         default_parallel = 0;
     }
-    if (!machine->use_virtcon) {
+    if (no_defaults || !machine->use_virtcon) {
         default_virtcon = 0;
     }
-    if (!machine->use_sclp) {
+    if (no_defaults || !machine->use_sclp) {
         default_sclp = 0;
     }
-    if (machine->no_floppy) {
+    if (no_defaults || machine->no_floppy) {
         default_floppy = 0;
     }
-    if (machine->no_cdrom) {
+    if (no_defaults || machine->no_cdrom) {
         default_cdrom = 0;
     }
-    if (machine->no_sdcard) {
+    if (no_defaults || machine->no_sdcard) {
         default_sdcard = 0;
     }
+    if (no_defaults) {
+        default_monitor = 0;
+        default_net = 0;
+        default_vga = 0;
+    }
 
     if (is_daemonized()) {
         /* According to documentation and historically, -nographic redirects
@@ -4243,7 +4243,9 @@ ---> only call select_vgahw for "-vga" (or the default VGA, see the if ending here)
             vga_model = "std";
         }
     }
-    select_vgahw(vga_model);
+    if (vga_model) {
+        select_vgahw(vga_model);
+    }
 
     if (watchdog) {
         i = select_watchdog(watchdog);