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

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

[Mark Wu]

Some machine (like ppc) initialization code determines if it has vga
configured according to vga_interface_type. In the original code,
vga_interface_type is evaluated to VGA_NONE even if a vga is added
by '-device VGA'. It causes the machine not aware of the graphics
device configured. This patch adds a new vga device type to indicate
that it has a vga device, but the machine code doesn't need to care
about its initialization.

Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>
---
 include/sysemu/sysemu.h | 2 +-
 vl.c                    | 8 +++++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 495dae8..a21205f 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -103,7 +103,7 @@ typedef enum DisplayType
 extern int autostart;
 
 typedef enum {
-    VGA_NONE, VGA_STD, VGA_CIRRUS, VGA_VMWARE, VGA_XENFB, VGA_QXL,
+    VGA_NONE, VGA_STD, VGA_CIRRUS, VGA_VMWARE, VGA_XENFB, VGA_QXL, VGA_DEVICE
 } VGAInterfaceType;
 
 extern int vga_interface_type;
diff --git a/vl.c b/vl.c
index 1d27b34..06a4253 100644
--- a/vl.c
+++ b/vl.c
@@ -226,6 +226,7 @@ static int default_floppy = 1;
 static int default_cdrom = 1;
 static int default_sdcard = 1;
 static int default_vga = 1;
+static int has_defaults = 1;
 
 static struct {
     const char *driver;
@@ -2066,7 +2067,11 @@ static void select_vgahw (const char *p)
             fprintf(stderr, "Error: QXL VGA not available\n");
             exit(0);
         }
-    } else if (!strstart(p, "none", &opts)) {
+    } else if (strstart(p, "none", &opts)) {
+        if (!has_defaults && !default_vga) {
+            vga_interface_type = VGA_DEVICE;
+        }
+    } else {
     invalid_vga:
         fprintf(stderr, "Unknown vga type: %s\n", p);
         exit(1);
@@ -3672,6 +3677,7 @@ int main(int argc, char **argv, char **envp)
                 default_cdrom = 0;
                 default_sdcard = 0;
                 default_vga = 0;
+                has_defaults = 0;
                 break;
             case QEMU_OPTION_xen_domid:
                 if (!(xen_available())) {
-- 
1.8.4.2

[Qemu-devel] [PATCH 2/2] Fix return value of vga initlization on ppc

[Mark Wu]

Before spapr_vga_init will returned false if the vga is specified by
the command '-device VGA' because vga_interface_type was evaluated to
VGA_NONE. With the change in previous patch of this series,
spapr_vga_init should return true if it's told that the vga will be
initialized in flow of the generic devices initialization.

This patch also makes two cleanups:
1. skip initialization for VGA_NONE
2. remove the useless 'break'

Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 93d02c1..4d0ac56 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -765,13 +765,15 @@ static int spapr_vga_init(PCIBus *pci_bus)
 {
     switch (vga_interface_type) {
     case VGA_NONE:
+        return false;
+    case VGA_DEVICE:
+        return true;
     case VGA_STD:
         return pci_vga_init(pci_bus) != NULL;
     default:
         fprintf(stderr, "This vga model is not supported,"
                 "currently it only supports -vga std\n");
         exit(0);
-        break;
     }
 }
 
-- 
1.8.4.2

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

[Paolo Bonzini]

Il 28/02/2014 13:45, Mark Wu ha scritto:
> Some machine (like ppc) initialization code determines if it has vga
> configured according to vga_interface_type. In the original code,
> vga_interface_type is evaluated to VGA_NONE even if a vga is added
> by '-device VGA'. It causes the machine not aware of the graphics
> device configured. This patch adds a new vga device type to indicate
> that it has a vga device, but the machine code doesn't need to care
> about its initialization.

Can you use memory_region_present and look (for example) for a device 
that owns I/O port 0x3d4?

Paolo

> Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>
> ---
>  include/sysemu/sysemu.h | 2 +-
>  vl.c                    | 8 +++++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 495dae8..a21205f 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -103,7 +103,7 @@ typedef enum DisplayType
>  extern int autostart;
>
>  typedef enum {
> -    VGA_NONE, VGA_STD, VGA_CIRRUS, VGA_VMWARE, VGA_XENFB, VGA_QXL,
> +    VGA_NONE, VGA_STD, VGA_CIRRUS, VGA_VMWARE, VGA_XENFB, VGA_QXL, VGA_DEVICE
>  } VGAInterfaceType;
>
>  extern int vga_interface_type;
> diff --git a/vl.c b/vl.c
> index 1d27b34..06a4253 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -226,6 +226,7 @@ static int default_floppy = 1;
>  static int default_cdrom = 1;
>  static int default_sdcard = 1;
>  static int default_vga = 1;
> +static int has_defaults = 1;
>
>  static struct {
>      const char *driver;
> @@ -2066,7 +2067,11 @@ static void select_vgahw (const char *p)
>              fprintf(stderr, "Error: QXL VGA not available\n");
>              exit(0);
>          }
> -    } else if (!strstart(p, "none", &opts)) {
> +    } else if (strstart(p, "none", &opts)) {
> +        if (!has_defaults && !default_vga) {
> +            vga_interface_type = VGA_DEVICE;
> +        }
> +    } else {
>      invalid_vga:
>          fprintf(stderr, "Unknown vga type: %s\n", p);
>          exit(1);
> @@ -3672,6 +3677,7 @@ int main(int argc, char **argv, char **envp)
>                  default_cdrom = 0;
>                  default_sdcard = 0;
>                  default_vga = 0;
> +                has_defaults = 0;
>                  break;
>              case QEMU_OPTION_xen_domid:
>                  if (!(xen_available())) {
>

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

[Mark Wu]

On 02/28/2014 09:03 PM, Paolo Bonzini wrote:
> Il 28/02/2014 13:45, Mark Wu ha scritto:
>> Some machine (like ppc) initialization code determines if it has vga
>> configured according to vga_interface_type. In the original code,
>> vga_interface_type is evaluated to VGA_NONE even if a vga is added
>> by '-device VGA'. It causes the machine not aware of the graphics
>> device configured. This patch adds a new vga device type to indicate
>> that it has a vga device, but the machine code doesn't need to care
>> about its initialization.
>
> Can you use memory_region_present and look (for example) for a device 
> that owns I/O port 0x3d4?
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.

Please correct me if my understanding on your comments is wrong. Thanks!




>
> Paolo
>
>> Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>
>> ---
>>  include/sysemu/sysemu.h | 2 +-
>>  vl.c                    | 8 +++++++-
>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index 495dae8..a21205f 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -103,7 +103,7 @@ typedef enum DisplayType
>>  extern int autostart;
>>
>>  typedef enum {
>> -    VGA_NONE, VGA_STD, VGA_CIRRUS, VGA_VMWARE, VGA_XENFB, VGA_QXL,
>> +    VGA_NONE, VGA_STD, VGA_CIRRUS, VGA_VMWARE, VGA_XENFB, VGA_QXL, 
>> VGA_DEVICE
>>  } VGAInterfaceType;
>>
>>  extern int vga_interface_type;
>> diff --git a/vl.c b/vl.c
>> index 1d27b34..06a4253 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -226,6 +226,7 @@ static int default_floppy = 1;
>>  static int default_cdrom = 1;
>>  static int default_sdcard = 1;
>>  static int default_vga = 1;
>> +static int has_defaults = 1;
>>
>>  static struct {
>>      const char *driver;
>> @@ -2066,7 +2067,11 @@ static void select_vgahw (const char *p)
>>              fprintf(stderr, "Error: QXL VGA not available\n");
>>              exit(0);
>>          }
>> -    } else if (!strstart(p, "none", &opts)) {
>> +    } else if (strstart(p, "none", &opts)) {
>> +        if (!has_defaults && !default_vga) {
>> +            vga_interface_type = VGA_DEVICE;
>> +        }
>> +    } else {
>>      invalid_vga:
>>          fprintf(stderr, "Unknown vga type: %s\n", p);
>>          exit(1);
>> @@ -3672,6 +3677,7 @@ int main(int argc, char **argv, char **envp)
>>                  default_cdrom = 0;
>>                  default_sdcard = 0;
>>                  default_vga = 0;
>> +                has_defaults = 0;
>>                  break;
>>              case QEMU_OPTION_xen_domid:
>>                  if (!(xen_available())) {
>>
>
>

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

[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?

Paolo

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

[Andreas Färber]

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?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Paolo Bonzini]

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

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

[Mark Wu]

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