[Qemu-devel] [PATCH v3 1/2] Fix vga_interface_type for command '-device VGA'

[Qemu-devel] [PATCH v3 1/2] Fix vga_interface_type for command '-device VGA'

[Mark Wu]

Some machine (like ppc) initialization code determines if it has
grahicis 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, which will be initliazed in qom devices
initialization.

Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>
---
Changes:
v2 -> v3:
Change the var name from 'no_defaults' to 'has_defaults' to avoid
initializing statics to 0.


 include/sysemu/sysemu.h |  2 +-
 vl.c                    | 42 ++++++++++++++++++++++--------------------
 2 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b90df9a..c01304d 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -104,7 +104,7 @@ extern int autostart;
 
 typedef enum {
     VGA_NONE, VGA_STD, VGA_CIRRUS, VGA_VMWARE, VGA_XENFB, VGA_QXL,
-    VGA_TCX, VGA_CG3,
+    VGA_TCX, VGA_CG3, VGA_DEVICE
 } VGAInterfaceType;
 
 extern int vga_interface_type;
diff --git a/vl.c b/vl.c
index 50693e6..f8f7c00 100644
--- a/vl.c
+++ b/vl.c
@@ -213,6 +213,7 @@ uint32_t xen_domid;
 enum xen_mode xen_mode = XEN_EMULATE;
 static int tcg_tb_size;
 
+static int has_defaults = 1;
 static int default_serial = 1;
 static int default_parallel = 1;
 static int default_virtcon = 1;
@@ -2047,7 +2048,7 @@ static void select_vgahw (const char *p)
 {
     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;
@@ -2831,7 +2832,7 @@ int main(int argc, char **argv, char **envp)
     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;
@@ -3688,16 +3689,7 @@ int main(int argc, char **argv, char **envp)
                 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;
+                has_defaults = 0;
                 break;
             case QEMU_OPTION_xen_domid:
                 if (!(xen_available())) {
@@ -3924,27 +3916,35 @@ int main(int argc, char **argv, char **envp)
     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 (!has_defaults || machine->no_serial) {
         default_serial = 0;
     }
-    if (machine->no_parallel) {
+    if (!has_defaults || machine->no_parallel) {
         default_parallel = 0;
     }
-    if (!machine->use_virtcon) {
+    if (!has_defaults || !machine->use_virtcon) {
         default_virtcon = 0;
     }
-    if (!machine->use_sclp) {
+    if (!has_defaults || !machine->use_sclp) {
         default_sclp = 0;
     }
-    if (machine->no_floppy) {
+    if (!has_defaults || machine->no_floppy) {
         default_floppy = 0;
     }
-    if (machine->no_cdrom) {
+    if (!has_defaults || machine->no_cdrom) {
         default_cdrom = 0;
     }
-    if (machine->no_sdcard) {
+    if (!has_defaults || machine->no_sdcard) {
         default_sdcard = 0;
     }
+    if (!has_defaults) {
+        default_monitor = 0;
+        default_net = 0;
+        default_vga = 0;
+    }
 
     if (is_daemonized()) {
         /* According to documentation and historically, -nographic redirects
@@ -4249,7 +4249,9 @@ int main(int argc, char **argv, char **envp)
             vga_model = "std";
         }
     }
-    select_vgahw(vga_model);
+    if (vga_model) {
+        select_vgahw(vga_model);
+    }
 
     if (watchdog) {
         i = select_watchdog(watchdog);
-- 
1.8.4.2

[Qemu-devel] [PATCH v3 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.

To keep '-nodefaults' have the semantics of bare minimum, it adds a
check of 'has_defaults' in usb_enabled() to avoid that a USB controller
is added by '-nodefautls, -device VGA' implicitly.

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 +++-
 vl.c           | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bf46c38..5c9a154 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -781,13 +781,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;
     }
 }
 
diff --git a/vl.c b/vl.c
index f8f7c00..e9d8baf 100644
--- a/vl.c
+++ b/vl.c
@@ -932,7 +932,8 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
 
 bool usb_enabled(bool default_usb)
 {
-    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb", default_usb);
+    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
+                             has_defaults && default_usb);
 }
 
 #ifndef _WIN32
-- 
1.8.4.2

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

[Paolo Bonzini]

Il 10/03/2014 15:37, Mark Wu ha scritto:
> 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.
>
> To keep '-nodefaults' have the semantics of bare minimum, it adds a
> check of 'has_defaults' in usb_enabled() to avoid that a USB controller
> is added by '-nodefautls, -device VGA' implicitly.
>
> 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 +++-
>  vl.c           | 3 ++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bf46c38..5c9a154 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -781,13 +781,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;
>      }
>  }
>
> diff --git a/vl.c b/vl.c
> index f8f7c00..e9d8baf 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -932,7 +932,8 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>
>  bool usb_enabled(bool default_usb)
>  {
> -    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb", default_usb);
> +    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
> +                             has_defaults && default_usb);
>  }
>
>  #ifndef _WIN32
>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

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

[Mark Wu]

On 03/10/2014 10:52 PM, Paolo Bonzini wrote:
> Il 10/03/2014 15:37, Mark Wu ha scritto:
>> 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.
>>
>> To keep '-nodefaults' have the semantics of bare minimum, it adds a
>> check of 'has_defaults' in usb_enabled() to avoid that a USB controller
>> is added by '-nodefautls, -device VGA' implicitly.
>>
>> 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 +++-
>>  vl.c           | 3 ++-
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index bf46c38..5c9a154 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -781,13 +781,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;
>>      }
>>  }
>>
>> diff --git a/vl.c b/vl.c
>> index f8f7c00..e9d8baf 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -932,7 +932,8 @@ static int parse_sandbox(QemuOpts *opts, void 
>> *opaque)
>>
>>  bool usb_enabled(bool default_usb)
>>  {
>> -    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb", 
>> default_usb);
>> +    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
>> +                             has_defaults && default_usb);
>>  }
>>
>>  #ifndef _WIN32
>>
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
Thanks a lot for the review.  You contributed a lot on this patch, but I 
forgot to acknowledge it in commit message.
I think re-sending a new patch to include it could cause some noise.  
May I ask the maintainer add the following line
in the commit message when it's picked?  Same on 1/2 in this series Thanks.
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>

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

[Paolo Bonzini]

Il 11/03/2014 10:52, Mark Wu ha scritto:
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>
> Thanks a lot for the review.  You contributed a lot on this patch, but I
> forgot to acknowledge it in commit message.
> I think re-sending a new patch to include it could cause some noise.
> May I ask the maintainer add the following line
> in the commit message when it's picked?  Same on 1/2 in this series Thanks.
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>

No problem from me, I don't really care.  It's kind of you anyway.  :)

Paolo

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

[Andreas Färber]

Am 11.03.2014 10:52, schrieb Mark Wu:
> On 03/10/2014 10:52 PM, Paolo Bonzini wrote:
>> Il 10/03/2014 15:37, Mark Wu ha scritto:
>>> 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.
>>>
>>> To keep '-nodefaults' have the semantics of bare minimum, it adds a
>>> check of 'has_defaults' in usb_enabled() to avoid that a USB controller
>>> is added by '-nodefautls, -device VGA' implicitly.
>>>
>>> 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 +++-
>>>  vl.c           | 3 ++-
>>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index bf46c38..5c9a154 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -781,13 +781,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;
>>>      }
>>>  }
>>>
>>> diff --git a/vl.c b/vl.c
>>> index f8f7c00..e9d8baf 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -932,7 +932,8 @@ static int parse_sandbox(QemuOpts *opts, void
>>> *opaque)
>>>
>>>  bool usb_enabled(bool default_usb)
>>>  {
>>> -    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
>>> default_usb);
>>> +    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
>>> +                             has_defaults && default_usb);
>>>  }
>>>
>>>  #ifndef _WIN32
>>>
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>
> Thanks a lot for the review.  You contributed a lot on this patch, but I
> forgot to acknowledge it in commit message.
> I think re-sending a new patch to include it could cause some noise. 
> May I ask the maintainer add the following line
> in the commit message when it's picked?  Same on 1/2 in this series Thanks.
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>

Sorry, didn't spot this yesterday, applied to ppc-next (with typo fix):
https://github.com/afaerber/qemu-cpu/commits/ppc-next

Thanks,
Andreas

P.S. Mark, please remember to include a cover letter 0/2 next time.

-- 
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 v3 1/2] Fix vga_interface_type for command '-device VGA'

[Paolo Bonzini]

Il 10/03/2014 15:37, Mark Wu ha scritto:
> Some machine (like ppc) initialization code determines if it has
> grahicis 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, which will be initliazed in qom devices
> initialization.
>
> Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>
> ---
> Changes:
> v2 -> v3:
> Change the var name from 'no_defaults' to 'has_defaults' to avoid
> initializing statics to 0.
>
>
>  include/sysemu/sysemu.h |  2 +-
>  vl.c                    | 42 ++++++++++++++++++++++--------------------
>  2 files changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index b90df9a..c01304d 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -104,7 +104,7 @@ extern int autostart;
>
>  typedef enum {
>      VGA_NONE, VGA_STD, VGA_CIRRUS, VGA_VMWARE, VGA_XENFB, VGA_QXL,
> -    VGA_TCX, VGA_CG3,
> +    VGA_TCX, VGA_CG3, VGA_DEVICE
>  } VGAInterfaceType;
>
>  extern int vga_interface_type;
> diff --git a/vl.c b/vl.c
> index 50693e6..f8f7c00 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -213,6 +213,7 @@ uint32_t xen_domid;
>  enum xen_mode xen_mode = XEN_EMULATE;
>  static int tcg_tb_size;
>
> +static int has_defaults = 1;
>  static int default_serial = 1;
>  static int default_parallel = 1;
>  static int default_virtcon = 1;
> @@ -2047,7 +2048,7 @@ static void select_vgahw (const char *p)
>  {
>      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;
> @@ -2831,7 +2832,7 @@ int main(int argc, char **argv, char **envp)
>      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;
> @@ -3688,16 +3689,7 @@ int main(int argc, char **argv, char **envp)
>                  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;
> +                has_defaults = 0;
>                  break;
>              case QEMU_OPTION_xen_domid:
>                  if (!(xen_available())) {
> @@ -3924,27 +3916,35 @@ int main(int argc, char **argv, char **envp)
>      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 (!has_defaults || machine->no_serial) {
>          default_serial = 0;
>      }
> -    if (machine->no_parallel) {
> +    if (!has_defaults || machine->no_parallel) {
>          default_parallel = 0;
>      }
> -    if (!machine->use_virtcon) {
> +    if (!has_defaults || !machine->use_virtcon) {
>          default_virtcon = 0;
>      }
> -    if (!machine->use_sclp) {
> +    if (!has_defaults || !machine->use_sclp) {
>          default_sclp = 0;
>      }
> -    if (machine->no_floppy) {
> +    if (!has_defaults || machine->no_floppy) {
>          default_floppy = 0;
>      }
> -    if (machine->no_cdrom) {
> +    if (!has_defaults || machine->no_cdrom) {
>          default_cdrom = 0;
>      }
> -    if (machine->no_sdcard) {
> +    if (!has_defaults || machine->no_sdcard) {
>          default_sdcard = 0;
>      }
> +    if (!has_defaults) {
> +        default_monitor = 0;
> +        default_net = 0;
> +        default_vga = 0;
> +    }
>
>      if (is_daemonized()) {
>          /* According to documentation and historically, -nographic redirects
> @@ -4249,7 +4249,9 @@ int main(int argc, char **argv, char **envp)
>              vga_model = "std";
>          }
>      }
> -    select_vgahw(vga_model);
> +    if (vga_model) {
> +        select_vgahw(vga_model);
> +    }
>
>      if (watchdog) {
>          i = select_watchdog(watchdog);
>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>