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

[Mark Wu <wudxw@linux.vnet.ibm.com>]

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);
>