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

[Qemu-devel] [PATCH v2 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>
---
 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 685a7a9..d9afff4 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 no_defaults = 0;
 static int default_serial = 1;
 static int default_parallel = 1;
 static int default_virtcon = 1;
@@ -2041,7 +2042,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;
@@ -2825,7 +2826,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;
@@ -3682,16 +3683,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;
+                no_defaults = 1;
                 break;
             case QEMU_OPTION_xen_domid:
                 if (!(xen_available())) {
@@ -3918,27 +3910,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 (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 @@ 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 v2 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 v2 2/2] Fix return value of vga initlization on ppc

[Paolo Bonzini]

Il 07/03/2014 10: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.
> 
> This patch also makes two cleanups:
> 1. skip initialization for VGA_NONE
> 2. remove the useless 'break'

I think that after this patch, "-nodefaults -device VGA" will get a USB 
controller that it didn't get before.

Perhaps this in vl.c:

bool usb_enabled(bool default_usb)
{
    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb", default_usb);
}

should be

bool usb_enabled(bool default_usb)
{
    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
                             !no_defaults && default_usb);
}

?

Thanks,

Paolo

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

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

[Alexey Kardashevskiy]

On 03/07/2014 08:43 PM, Paolo Bonzini wrote:
> Il 07/03/2014 10: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.
>>
>> This patch also makes two cleanups:
>> 1. skip initialization for VGA_NONE
>> 2. remove the useless 'break'
> 
> I think that after this patch, "-nodefaults -device VGA" will get a USB 
> controller that it didn't get before.


I suspect what was meant by "the machine not aware of the graphics device"
 is that the guest won't work with VGA and without keyboard (default
console will be vga + keyboard and not serial) which is USB and this is why
the patch is trying to add USB.


> 
> Perhaps this in vl.c:
> 
> bool usb_enabled(bool default_usb)
> {
>     return qemu_opt_get_bool(qemu_get_machine_opts(), "usb", default_usb);
> }
> 
> should be
> 
> bool usb_enabled(bool default_usb)
> {
>     return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
>                              !no_defaults && default_usb);
> }
> 
> ?
> 
> Thanks,
> 
> Paolo
> 
>> 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;
>>      }
>>  }
>>  
>>
> 
> 


-- 
Alexey

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

[Paolo Bonzini]

Il 08/03/2014 14:26, Alexey Kardashevskiy ha scritto:
>> > I think that after this patch, "-nodefaults -device VGA" will get a USB
>> > controller that it didn't get before.
>
> I suspect what was meant by "the machine not aware of the graphics device"
>  is that the guest won't work with VGA and without keyboard (default
> console will be vga + keyboard and not serial) which is USB and this is why
> the patch is trying to add USB.

But with -nodefaults QEMU should never be adding USB.

Paolo

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

[Alexey Kardashevskiy]

On 03/10/2014 05:07 AM, Paolo Bonzini wrote:
> Il 08/03/2014 14:26, Alexey Kardashevskiy ha scritto:
>>> > I think that after this patch, "-nodefaults -device VGA" will get a USB
>>> > controller that it didn't get before.
>>
>> I suspect what was meant by "the machine not aware of the graphics device"
>>  is that the guest won't work with VGA and without keyboard (default
>> console will be vga + keyboard and not serial) which is USB and this is why
>> the patch is trying to add USB.
> 
> But with -nodefaults QEMU should never be adding USB.


As I was told in this list before, even with -nodefaults, QEMU should not
create a machine which is known for not working or not being supported.
Having VGA and not having any input device is kind of such a config, no?


-- 
Alexey

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

[Paolo Bonzini]

Il 10/03/2014 13:23, Alexey Kardashevskiy ha scritto:
> On 03/10/2014 05:07 AM, Paolo Bonzini wrote:
>> Il 08/03/2014 14:26, Alexey Kardashevskiy ha scritto:
>>>>> I think that after this patch, "-nodefaults -device VGA" will get a USB
>>>>> controller that it didn't get before.
>>>
>>> I suspect what was meant by "the machine not aware of the graphics device"
>>>  is that the guest won't work with VGA and without keyboard (default
>>> console will be vga + keyboard and not serial) which is USB and this is why
>>> the patch is trying to add USB.
>>
>> But with -nodefaults QEMU should never be adding USB.
>
> As I was told in this list before, even with -nodefaults, QEMU should not
> create a machine which is known for not working or not being supported.
> Having VGA and not having any input device is kind of such a config, no?

-nodefaults is exactly the opposite of that: no magic whatsoever.  No 
VGA, no serial, nothing.

And especially, with -nodefaults adding a VGA means just that: adding a VGA.

Paolo

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

[Alexey Kardashevskiy]

On 03/10/2014 11:26 PM, Paolo Bonzini wrote:
> Il 10/03/2014 13:23, Alexey Kardashevskiy ha scritto:
>> On 03/10/2014 05:07 AM, Paolo Bonzini wrote:
>>> Il 08/03/2014 14:26, Alexey Kardashevskiy ha scritto:
>>>>>> I think that after this patch, "-nodefaults -device VGA" will get a USB
>>>>>> controller that it didn't get before.
>>>>
>>>> I suspect what was meant by "the machine not aware of the graphics device"
>>>>  is that the guest won't work with VGA and without keyboard (default
>>>> console will be vga + keyboard and not serial) which is USB and this is
>>>> why
>>>> the patch is trying to add USB.
>>>
>>> But with -nodefaults QEMU should never be adding USB.
>>
>> As I was told in this list before, even with -nodefaults, QEMU should not
>> create a machine which is known for not working or not being supported.
>> Having VGA and not having any input device is kind of such a config, no?
> 
> -nodefaults is exactly the opposite of that: no magic whatsoever.  No VGA,
> no serial, nothing.


qemu-system-x86_64 -enable-kvm -nographic -nodefaults -monitor stdio

"info qtree" shows a whole bunch of devices like "i440FX-pcihost",
"isa-fdc", "piix3-ide", "vmmouse", "vmport" (what are the last two?). I was
told here that 8042 is to emulate A20, ok, but others - I do not really
understand. "q35" is bit different than "pc" but not smaller. The point was
made that there is no point in emulating a machine which does not exist in
the real world. Has it changed recently?


> And especially, with -nodefaults adding a VGA means just that: adding a VGA.

Usual issue - libvirt expects keyboard with VGA and x86 provides this as it
always has keyboard. PPC does not have such default.


-- 
Alexey

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

[Paolo Bonzini]

Il 10/03/2014 13:45, Alexey Kardashevskiy ha scritto:
>>> As I was told in this list before, even with -nodefaults, QEMU should not
>>> create a machine which is known for not working or not being supported.
>>> Having VGA and not having any input device is kind of such a config, no?
>>
>> -nodefaults is exactly the opposite of that: no magic whatsoever.  No VGA,
>> no serial, nothing.
>
> qemu-system-x86_64 -enable-kvm -nographic -nodefaults -monitor stdio
>
> "info qtree" shows a whole bunch of devices like "i440FX-pcihost",
> "isa-fdc", "piix3-ide", "vmmouse", "vmport" (what are the last two?). I was
> told here that 8042 is to emulate A20, ok, but others - I do not really
> understand. "q35" is bit different than "pc" but not smaller. The point was
> made that there is no point in emulating a machine which does not exist in
> the real world. Has it changed recently?

-nodefaults should be the bare minimum that is necessary for the VM 
firmware to work.  On x86, isa-fdc/vmmouse/vmport are there only for 
backwards compatibility reasons.

>> And especially, with -nodefaults adding a VGA means just that: adding a VGA.
>
> Usual issue - libvirt expects keyboard with VGA and x86 provides this as it
> always has keyboard. PPC does not have such default.

I don't think it's even a libvirt bug.  Then in the libvirt XML you 
should have:

    <devices>
      <input type='keyboard' bus='usb'/>
    </devices>

explicitly.  If libvirt uses -nodefaults, it's up to virt-manager or 
oVirt or whatever management layer you're using to add all the necessary 
controllers (USB in this case) and devices (keyboard) for the machine to 
be usable.

Paolo

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

[Mark Wu]

On 03/07/2014 05:43 PM, Paolo Bonzini wrote:
> Il 07/03/2014 10: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.
>>
>> This patch also makes two cleanups:
>> 1. skip initialization for VGA_NONE
>> 2. remove the useless 'break'
> I think that after this patch, "-nodefaults -device VGA" will get a USB
> controller that it didn't get before.
>
> Perhaps this in vl.c:
>
> bool usb_enabled(bool default_usb)
> {
>      return qemu_opt_get_bool(qemu_get_machine_opts(), "usb", default_usb);
> }
>
> should be
>
> bool usb_enabled(bool default_usb)
> {
>      return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
>                               !no_defaults && default_usb);
> }
Yes, it changes the semantics of 'nodefaults'.  I will fix it in v3 and 
let the management app add usb controller.
> ?
>
> Thanks,
>
> Paolo
>
>> 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;
>>       }
>>   }
>>   
>>