From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58849) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLrB8-0007tU-VK for qemu-devel@nongnu.org; Fri, 07 Mar 2014 04:35:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WLrAz-0007IP-BR for qemu-devel@nongnu.org; Fri, 07 Mar 2014 04:34:54 -0500 Received: from e28smtp07.in.ibm.com ([122.248.162.7]:48310) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLrAy-0007Gb-JM for qemu-devel@nongnu.org; Fri, 07 Mar 2014 04:34:45 -0500 Received: from /spool/local by e28smtp07.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 7 Mar 2014 15:04:40 +0530 Message-ID: <53199289.3010802@linux.vnet.ibm.com> Date: Fri, 07 Mar 2014 17:34:01 +0800 From: Mark Wu MIME-Version: 1.0 References: <1393591540-32435-1-git-send-email-wudxw@linux.vnet.ibm.com> <53108919.10103@redhat.com> <53187A32.5000001@linux.vnet.ibm.com> <5318872E.5040301@redhat.com> <531888A7.2030400@suse.de> <53189DCE.5050101@redhat.com> In-Reply-To: <53189DCE.5050101@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 1/2] Add a generic vga device type for that specified by '-device' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , =?ISO-8859-1?Q?Andreas_F=E4rbe?= =?ISO-8859-1?Q?r?= , qemu-devel@nongnu.org, qemu-ppc@nongnu.org Cc: Alexander Graf , nikunj@linux.vnet.ibm.com, Anthony Liguori On 03/07/2014 12:09 AM, Paolo Bonzini wrote: > 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. Yes, you're correct. I didn't think it clearly. I will update the patch as what you suggested. Thanks a lot for the great help! > > 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); >