[Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Nikunj A Dadhania]

The following commit caused the regression in qemu-system-ppc64

7effdaa3: spapr: Fix return value of vga initialization
d44229c5: Fix vga_interface_type for command line argument '-device VGA'

Even when -nodefaults was provided, USB Keyboard and Mouse was added
to the machine. This breaks libvirt which uses -nodefaults and adds
the keyboard and mouse separately. The machine got 2 USB Keyboards
and 2 USB Mouses.

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Mark Wu <wudxw@linux.vnet.ibm.com>
CC: Andreas Färber <afaerber@suse.de>
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c          |  6 +++++-
 include/sysemu/sysemu.h |  1 +
 vl.c                    | 10 ++++++++--
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a11e121..3095626 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1328,7 +1328,11 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
 
     if (usb_enabled(spapr->has_graphics)) {
         pci_create_simple(phb->bus, -1, "pci-ohci");
-        if (spapr->has_graphics) {
+        /*
+         * For VGA/VNC, by default add usb keyboard/mouse, if -nodefaults
+         * provided skip adding usb keyboard/mouse
+         */
+        if (spapr->has_graphics && qemu_has_defaults()) {
             usbdevice_create("keyboard");
             usbdevice_create("mouse");
         }
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index ba5c7f8..8e90ad0 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -200,6 +200,7 @@ DeviceState *get_boot_device(uint32_t position);
 QemuOpts *qemu_get_machine_opts(void);
 
 bool usb_enabled(bool default_usb);
+bool qemu_has_defaults(void);
 
 extern QemuOptsList qemu_legacy_drive_opts;
 extern QemuOptsList qemu_common_drive_opts;
diff --git a/vl.c b/vl.c
index 9975e5a..6bf37a2 100644
--- a/vl.c
+++ b/vl.c
@@ -977,8 +977,14 @@ static void parse_name(QemuOpts *opts)
 
 bool usb_enabled(bool default_usb)
 {
-    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
-                             has_defaults && default_usb);
+  return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
+			   has_defaults && default_usb);
+}
+
+bool qemu_has_defaults(void)
+{
+  fprintf(stderr, "has_d %d\n", has_defaults);
+  return has_defaults;
 }
 
 #ifndef _WIN32
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Andreas Färber]

Am 03.04.2014 18:56, schrieb Nikunj A Dadhania:
> The following commit caused the regression in qemu-system-ppc64
> 
> 7effdaa3: spapr: Fix return value of vga initialization
> d44229c5: Fix vga_interface_type for command line argument '-device VGA'
> 
> Even when -nodefaults was provided, USB Keyboard and Mouse was added
> to the machine. This breaks libvirt which uses -nodefaults and adds
> the keyboard and mouse separately. The machine got 2 USB Keyboards
> and 2 USB Mouses.
> 
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Mark Wu <wudxw@linux.vnet.ibm.com>
> CC: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c          |  6 +++++-
>  include/sysemu/sysemu.h |  1 +
>  vl.c                    | 10 ++++++++--
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a11e121..3095626 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1328,7 +1328,11 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>  
>      if (usb_enabled(spapr->has_graphics)) {
>          pci_create_simple(phb->bus, -1, "pci-ohci");
> -        if (spapr->has_graphics) {
> +        /*
> +         * For VGA/VNC, by default add usb keyboard/mouse, if -nodefaults
> +         * provided skip adding usb keyboard/mouse
> +         */
> +        if (spapr->has_graphics && qemu_has_defaults()) {
>              usbdevice_create("keyboard");
>              usbdevice_create("mouse");
>          }
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index ba5c7f8..8e90ad0 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -200,6 +200,7 @@ DeviceState *get_boot_device(uint32_t position);
>  QemuOpts *qemu_get_machine_opts(void);
>  
>  bool usb_enabled(bool default_usb);
> +bool qemu_has_defaults(void);
>  
>  extern QemuOptsList qemu_legacy_drive_opts;
>  extern QemuOptsList qemu_common_drive_opts;
> diff --git a/vl.c b/vl.c
> index 9975e5a..6bf37a2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -977,8 +977,14 @@ static void parse_name(QemuOpts *opts)
>  
>  bool usb_enabled(bool default_usb)
>  {
> -    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
> -                             has_defaults && default_usb);
> +  return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
> +			   has_defaults && default_usb);
> +}
> +
> +bool qemu_has_defaults(void)
> +{
> +  fprintf(stderr, "has_d %d\n", has_defaults);

Debugging leftover surely?

Cheers,
Andreas

> +  return has_defaults;
>  }
>  
>  #ifndef _WIN32
> 


-- 
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] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Nikunj A Dadhania]

Andreas Färber <afaerber@suse.de> writes:

> Am 03.04.2014 18:56, schrieb Nikunj A Dadhania:
>> The following commit caused the regression in qemu-system-ppc64
>> 
>> 7effdaa3: spapr: Fix return value of vga initialization
>> d44229c5: Fix vga_interface_type for command line argument '-device VGA'
>> 
>> Even when -nodefaults was provided, USB Keyboard and Mouse was added
>> to the machine. This breaks libvirt which uses -nodefaults and adds
>> the keyboard and mouse separately. The machine got 2 USB Keyboards
>> and 2 USB Mouses.
>> 
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Mark Wu <wudxw@linux.vnet.ibm.com>
>> CC: Andreas Färber <afaerber@suse.de>
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/spapr.c          |  6 +++++-
>>  include/sysemu/sysemu.h |  1 +
>>  vl.c                    | 10 ++++++++--
>>  3 files changed, 14 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index a11e121..3095626 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1328,7 +1328,11 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>>  
>>      if (usb_enabled(spapr->has_graphics)) {
>>          pci_create_simple(phb->bus, -1, "pci-ohci");
>> -        if (spapr->has_graphics) {
>> +        /*
>> +         * For VGA/VNC, by default add usb keyboard/mouse, if -nodefaults
>> +         * provided skip adding usb keyboard/mouse
>> +         */
>> +        if (spapr->has_graphics && qemu_has_defaults()) {
>>              usbdevice_create("keyboard");
>>              usbdevice_create("mouse");
>>          }
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index ba5c7f8..8e90ad0 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -200,6 +200,7 @@ DeviceState *get_boot_device(uint32_t position);
>>  QemuOpts *qemu_get_machine_opts(void);
>>  
>>  bool usb_enabled(bool default_usb);
>> +bool qemu_has_defaults(void);
>>  
>>  extern QemuOptsList qemu_legacy_drive_opts;
>>  extern QemuOptsList qemu_common_drive_opts;
>> diff --git a/vl.c b/vl.c
>> index 9975e5a..6bf37a2 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -977,8 +977,14 @@ static void parse_name(QemuOpts *opts)
>>  
>>  bool usb_enabled(bool default_usb)
>>  {
>> -    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
>> -                             has_defaults && default_usb);
>> +  return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
>> +			   has_defaults && default_usb);
>> +}
>> +
>> +bool qemu_has_defaults(void)
>> +{
>> +  fprintf(stderr, "has_d %d\n", has_defaults);
>
> Debugging leftover surely?

Yes, you were too fast, i have sent updated mail just now.

Thanks
Nikunj

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Nikunj A Dadhania]

The following commit caused the regression in qemu-system-ppc64

7effdaa3: spapr: Fix return value of vga initialization
d44229c5: Fix vga_interface_type for command line argument '-device VGA'

Even when -nodefaults was provided, USB Keyboard and Mouse was added
to the machine. This breaks libvirt which uses -nodefaults and adds
the keyboard and mouse separately. The machine got 2 USB Keyboards
and 2 USB Mouses.

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Mark Wu <wudxw@linux.vnet.ibm.com>
CC: Andreas Färber <afaerber@suse.de>
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---

Removed the debug statement and fixed indentation breakage:


 hw/ppc/spapr.c          | 6 +++++-
 include/sysemu/sysemu.h | 1 +
 vl.c                    | 5 +++++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a11e121..3095626 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1328,7 +1328,11 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
 
     if (usb_enabled(spapr->has_graphics)) {
         pci_create_simple(phb->bus, -1, "pci-ohci");
-        if (spapr->has_graphics) {
+        /*
+         * For VGA/VNC, by default add usb keyboard/mouse, if -nodefaults
+         * provided skip adding usb keyboard/mouse
+         */
+        if (spapr->has_graphics && qemu_has_defaults()) {
             usbdevice_create("keyboard");
             usbdevice_create("mouse");
         }
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index ba5c7f8..8e90ad0 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -200,6 +200,7 @@ DeviceState *get_boot_device(uint32_t position);
 QemuOpts *qemu_get_machine_opts(void);
 
 bool usb_enabled(bool default_usb);
+bool qemu_has_defaults(void);
 
 extern QemuOptsList qemu_legacy_drive_opts;
 extern QemuOptsList qemu_common_drive_opts;
diff --git a/vl.c b/vl.c
index 9975e5a..4d5832c 100644
--- a/vl.c
+++ b/vl.c
@@ -981,6 +981,11 @@ bool usb_enabled(bool default_usb)
                              has_defaults && default_usb);
 }
 
+bool qemu_has_defaults(void)
+{
+    return has_defaults;
+}
+
 #ifndef _WIN32
 static int parse_add_fd(QemuOpts *opts, void *opaque)
 {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Andreas Färber]

Am 03.04.2014 19:06, schrieb Nikunj A Dadhania:
> 
> The following commit caused the regression in qemu-system-ppc64
> 
> 7effdaa3: spapr: Fix return value of vga initialization
> d44229c5: Fix vga_interface_type for command line argument '-device VGA'
> 
> Even when -nodefaults was provided, USB Keyboard and Mouse was added
> to the machine. This breaks libvirt which uses -nodefaults and adds
> the keyboard and mouse separately. The machine got 2 USB Keyboards
> and 2 USB Mouses.
> 
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Mark Wu <wudxw@linux.vnet.ibm.com>
> CC: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

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] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Markus Armbruster]

Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:

> The following commit caused the regression in qemu-system-ppc64
>
> 7effdaa3: spapr: Fix return value of vga initialization
> d44229c5: Fix vga_interface_type for command line argument '-device VGA'
>
> Even when -nodefaults was provided, USB Keyboard and Mouse was added
> to the machine. This breaks libvirt which uses -nodefaults and adds
> the keyboard and mouse separately. The machine got 2 USB Keyboards
> and 2 USB Mouses.
>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Mark Wu <wudxw@linux.vnet.ibm.com>
> CC: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>
> Removed the debug statement and fixed indentation breakage:
>
>
>  hw/ppc/spapr.c          | 6 +++++-
>  include/sysemu/sysemu.h | 1 +
>  vl.c                    | 5 +++++
>  3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a11e121..3095626 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1328,7 +1328,11 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>  
>      if (usb_enabled(spapr->has_graphics)) {
>          pci_create_simple(phb->bus, -1, "pci-ohci");
> -        if (spapr->has_graphics) {
> +        /*
> +         * For VGA/VNC, by default add usb keyboard/mouse, if -nodefaults
> +         * provided skip adding usb keyboard/mouse
> +         */
> +        if (spapr->has_graphics && qemu_has_defaults()) {
>              usbdevice_create("keyboard");
>              usbdevice_create("mouse");
>          }
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index ba5c7f8..8e90ad0 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -200,6 +200,7 @@ DeviceState *get_boot_device(uint32_t position);
>  QemuOpts *qemu_get_machine_opts(void);
>  
>  bool usb_enabled(bool default_usb);
> +bool qemu_has_defaults(void);
>  
>  extern QemuOptsList qemu_legacy_drive_opts;
>  extern QemuOptsList qemu_common_drive_opts;
> diff --git a/vl.c b/vl.c
> index 9975e5a..4d5832c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -981,6 +981,11 @@ bool usb_enabled(bool default_usb)
>                               has_defaults && default_usb);
>  }
>  
> +bool qemu_has_defaults(void)
> +{
> +    return has_defaults;
> +}
> +
>  #ifndef _WIN32
>  static int parse_add_fd(QemuOpts *opts, void *opaque)
>  {

Have you considered extending QEMUMachineInitArgs instead of adding this
function?

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Nikunj A Dadhania]

Markus Armbruster <armbru@redhat.com> writes:
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
>
> Have you considered extending QEMUMachineInitArgs instead of adding this
> function?

Did not think of this option earlier. You mean doing something like
this?

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3a13231..936a17f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1410,6 +1410,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
     const char *kernel_cmdline = args->kernel_cmdline;
     const char *initrd_filename = args->initrd_filename;
     const char *boot_device = args->boot_device;
+    unsigned int has_defaults = args->has_defaults;
     PowerPCCPU *cpu;
     CPUPPCState *env;
     PCIHostState *phb;
@@ -1605,7 +1606,11 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
 
     if (usb_enabled(spapr->has_graphics)) {
         pci_create_simple(phb->bus, -1, "pci-ohci");
-        if (spapr->has_graphics) {
+        /*
+         * For VGA/VNC, by default add usb keyboard/mouse, if -nodefaults
+         * provided skip adding usb keyboard/mouse
+         */
+        if (spapr->has_graphics && has_defaults) {
             usbdevice_create("keyboard");
             usbdevice_create("mouse");
         }
diff --git a/include/hw/boards.h b/include/hw/boards.h
index b1d4e9f..ee81ddf 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -16,6 +16,7 @@ typedef struct QEMUMachineInitArgs {
     const char *kernel_cmdline;
     const char *initrd_filename;
     const char *cpu_model;
+    unsigned int has_defaults;
 } QEMUMachineInitArgs;
 
 typedef void QEMUMachineInitFunc(QEMUMachineInitArgs *args);
diff --git a/vl.c b/vl.c
index 017f92d..0d6c36c 100644
--- a/vl.c
+++ b/vl.c
@@ -4348,7 +4348,8 @@ int main(int argc, char **argv, char **envp)
                                  .kernel_filename = kernel_filename,
                                  .kernel_cmdline = kernel_cmdline,
                                  .initrd_filename = initrd_filename,
-                                 .cpu_model = cpu_model };
+                                 .cpu_model = cpu_model,
+                                 .has_defaults = has_defaults, };
     machine->init(&args);
 
     audio_init();

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Markus Armbruster]

Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
>>
>> Have you considered extending QEMUMachineInitArgs instead of adding this
>> function?
>
> Did not think of this option earlier. You mean doing something like
> this?

Yes.  Looks nicer, doesn't it?

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Paolo Bonzini]

Il 04/04/2014 12:58, Markus Armbruster ha scritto:
>>> >>
>>> >> Have you considered extending QEMUMachineInitArgs instead of adding this
>>> >> function?
>> >
>> > Did not think of this option earlier. You mean doing something like
>> > this?
> Yes.  Looks nicer, doesn't it?

I still think it's a libvirt bug.  Mixing -nodefaults and -usb and 
-device is looking for trouble I think.  "-usb" is a do-what-I-mean kind 
of option and it makes sense for it to add a keyboard and mouse, even 
with -nodefaults.

Paolo

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 04/04/2014 12:58, Markus Armbruster ha scritto:
>>>> >>
>>>> >> Have you considered extending QEMUMachineInitArgs instead of adding this
>>>> >> function?
>>> >
>>> > Did not think of this option earlier. You mean doing something like
>>> > this?
>> Yes.  Looks nicer, doesn't it?
>
> I still think it's a libvirt bug.  Mixing -nodefaults and -usb and
> -device is looking for trouble I think.  "-usb" is a do-what-I-mean
> kind of option and it makes sense for it to add a keyboard and mouse,
> even with -nodefaults.

I agree that management tools should not use -usb, except when they want
to manage ancient versions of QEMU for some reason.

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

[adding libvir-list]

On 04/04/2014 05:23 AM, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 04/04/2014 12:58, Markus Armbruster ha scritto:
>>>>>>>
>>>>>>> Have you considered extending QEMUMachineInitArgs instead of adding this
>>>>>>> function?
>>>>>
>>>>> Did not think of this option earlier. You mean doing something like
>>>>> this?
>>> Yes.  Looks nicer, doesn't it?
>>
>> I still think it's a libvirt bug.  Mixing -nodefaults and -usb and
>> -device is looking for trouble I think.  "-usb" is a do-what-I-mean
>> kind of option and it makes sense for it to add a keyboard and mouse,
>> even with -nodefaults.
> 
> I agree that management tools should not use -usb, except when they want
> to manage ancient versions of QEMU for some reason.

We've tried to make libvirt avoid -usb when it knows it is targetting a
new enough qemu.  But I'm not the one most familiar with that code.  At
this point, I'm just trying to facilitate discussions, and make sure the
right people are looking at this - what versions of qemu vs. libvirt
have you tested, and what does libvirt still need to patch to avoid
-usb, and/or qemu patch so that libvirt can correctly probe that -usb is
not needed?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Andreas Färber]

Am 04.04.2014 10:28, schrieb Nikunj A Dadhania:
> diff --git a/vl.c b/vl.c
> index 017f92d..0d6c36c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4348,7 +4348,8 @@ int main(int argc, char **argv, char **envp)
>                                   .kernel_filename = kernel_filename,
>                                   .kernel_cmdline = kernel_cmdline,
>                                   .initrd_filename = initrd_filename,
> -                                 .cpu_model = cpu_model };
> +                                 .cpu_model = cpu_model,
> +                                 .has_defaults = has_defaults, };

If we do this, please put
       };
on the next line so that the next person appending something doesn't
need to touch it again. :)

I do agree with the others that libvirt shouldn't be using the legacy
-usb option.

Cheers,
Andreas

>      machine->init(&args);
>  
>      audio_init();

-- 
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] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Paolo Bonzini]

Il 03/04/2014 18:56, Nikunj A Dadhania ha scritto:
> The following commit caused the regression in qemu-system-ppc64
>
> 7effdaa3: spapr: Fix return value of vga initialization
> d44229c5: Fix vga_interface_type for command line argument '-device VGA'
>
> Even when -nodefaults was provided, USB Keyboard and Mouse was added
> to the machine. This breaks libvirt which uses -nodefaults and adds
> the keyboard and mouse separately. The machine got 2 USB Keyboards
> and 2 USB Mouses.

Does libvirt use "-nodefaults -machine usb=true"?  It should create the 
OHCI controller separately instead of using "-machine".

Paolo

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

On 04/03/2014 12:01 PM, Paolo Bonzini wrote:
> Il 03/04/2014 18:56, Nikunj A Dadhania ha scritto:
>> The following commit caused the regression in qemu-system-ppc64
>>
>> 7effdaa3: spapr: Fix return value of vga initialization
>> d44229c5: Fix vga_interface_type for command line argument '-device VGA'
>>
>> Even when -nodefaults was provided, USB Keyboard and Mouse was added
>> to the machine. This breaks libvirt which uses -nodefaults and adds
>> the keyboard and mouse separately. The machine got 2 USB Keyboards
>> and 2 USB Mouses.

s/Mouses/Mice/

> 
> Does libvirt use "-nodefaults -machine usb=true"?  It should create the
> OHCI controller separately instead of using "-machine".

At least with x86 emulation, libvirt prefers '-nodefaults -machine
usb=off -device piix3-usb-uhci'; so I'm assuming libvirt knows how to
directly create the USB devices it needs.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Nikunj A Dadhania]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 03/04/2014 18:56, Nikunj A Dadhania ha scritto:
>> The following commit caused the regression in qemu-system-ppc64
>>
>> 7effdaa3: spapr: Fix return value of vga initialization
>> d44229c5: Fix vga_interface_type for command line argument '-device VGA'
>>
>> Even when -nodefaults was provided, USB Keyboard and Mouse was added
>> to the machine. This breaks libvirt which uses -nodefaults and adds
>> the keyboard and mouse separately. The machine got 2 USB Keyboards
>> and 2 USB Mouses.
>
> Does libvirt use "-nodefaults -machine usb=true"?  It should create the 
> OHCI controller separately instead of using "-machine".

I see it creating:

-nodefaults -usb -device usb-kbd,id=input0 -device usb-mouse,id=input1

And -usb is translated to adding "pci-ohci" controller for spapr

Regards,
Nikunj

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Paolo Bonzini]

Il 03/04/2014 21:24, Nikunj A Dadhania ha scritto:
>> > Does libvirt use "-nodefaults -machine usb=true"?  It should create the
>> > OHCI controller separately instead of using "-machine".
> I see it creating:
>
> -nodefaults -usb -device usb-kbd,id=input0 -device usb-mouse,id=input1
>
> And -usb is translated to adding "pci-ohci" controller for spapr

Yeah, but with -nodefaults it's better to use -device directly.

Paolo

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Nikunj A Dadhania]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 03/04/2014 21:24, Nikunj A Dadhania ha scritto:
>>> > Does libvirt use "-nodefaults -machine usb=true"?  It should create the
>>> > OHCI controller separately instead of using "-machine".
>> I see it creating:
>>
>> -nodefaults -usb -device usb-kbd,id=input0 -device usb-mouse,id=input1
>>
>> And -usb is translated to adding "pci-ohci" controller for spapr
>
> Yeah, but with -nodefaults it's better to use -device directly.

I think there is special handling for this in vl.c

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

And spapr.c uses this:

    if (usb_enabled(spapr->has_graphics)) {
        pci_create_simple(phb->bus, -1, "pci-ohci");

Regards
Nikunj

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Paolo Bonzini]

Il 04/04/2014 07:28, Nikunj A Dadhania ha scritto:
>>> >>
>>> >> And -usb is translated to adding "pci-ohci" controller for spapr
>> >
>> > Yeah, but with -nodefaults it's better to use -device directly.
> I think there is special handling for this in vl.c
>
> bool usb_enabled(bool default_usb)
> {
>     return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
>                              has_defaults && default_usb);
> }
>
> And spapr.c uses this:
>
>     if (usb_enabled(spapr->has_graphics)) {
>         pci_create_simple(phb->bus, -1, "pci-ohci");

Sure.  However, I'm saying that it's fine for spapr to make -usb mean 
"OHCI, and also keyboard & mouse if there is a VGA card in the system".

If libvirt used "-device pci-ohci" unconditionally, it would fix the bug 
*and* it would ensure that the PCI slot of pci-ohci does not change due 
to some other unrelated reason.  So I would rather have the fix in libvirt.

Paolo

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Nikunj A Dadhania]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 04/04/2014 07:28, Nikunj A Dadhania ha scritto:
>>>> >>
>>>> >> And -usb is translated to adding "pci-ohci" controller for spapr
>>> >
>>> > Yeah, but with -nodefaults it's better to use -device directly.
>> I think there is special handling for this in vl.c
>>
>> bool usb_enabled(bool default_usb)
>> {
>>     return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
>>                              has_defaults && default_usb);
>> }
>>
>> And spapr.c uses this:
>>
>>     if (usb_enabled(spapr->has_graphics)) {
>>         pci_create_simple(phb->bus, -1, "pci-ohci");
>
> Sure.  However, I'm saying that it's fine for spapr to make -usb mean 
> "OHCI, and also keyboard & mouse if there is a VGA card in the system".
>
> If libvirt used "-device pci-ohci" unconditionally, it would fix the bug 
> *and* it would ensure that the PCI slot of pci-ohci does not change due 
> to some other unrelated reason.  So I would rather have the fix in libvirt.

Sure, let me check this.

Thanks,
Nikunj

Re: [Qemu-devel] [PATCH] spapr: skip adding usb keyboard/mouse in case of -nodefaults

[Paolo Bonzini]

Il 04/04/2014 13:40, Nikunj A Dadhania ha scritto:
>> > Sure.  However, I'm saying that it's fine for spapr to make -usb mean
>> > "OHCI, and also keyboard & mouse if there is a VGA card in the system".
>> >
>> > If libvirt used "-device pci-ohci" unconditionally, it would fix the bug
>> > *and* it would ensure that the PCI slot of pci-ohci does not change due
>> > to some other unrelated reason.  So I would rather have the fix in libvirt.
> Sure, let me check this.

It is probably not that easy---libvirt needs to have an implicit USB 
<controller> element in the domain XML or something like that, and let 
the user's XML override the default USB controller.  But it would be 
much better and much more consistent with x86.

Paolo