[Qemu-devel] [PATCHv2 1/1] Add usb option in machine options to enable/disable usb

[Qemu-devel] [PATCHv2 1/1] Add usb option in machine options to enable/disable usb

[Li Zhang]

For pseries machine, it needs to enable usb to add
keyboard or usb mouse. -usb option won't be used in
the future, and machine options is a better way to
enable usb.

So this patch is to add usb option to machine options
(-machine type=psereis,usb=on/off)to enable/disable
usb controller.

In this patch, usb_on is an global option which can
be checked by machines.
For example, on pseries, it will check if usb_on is 1,
if it is 1, it will create one usb ohci controller.
As the following:
if (usb_on == 1) {
     pci_create_simple(bus, -1, "pci-ohci");
}

In this patch, usb is on by default. So, for -nodefault,
usb should be set off in the command line as the following:
 -machine type=pseries,usb=off.

Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
---
 hw/spapr.c |    5 +++++
 sysemu.h   |    1 +
 vl.c       |   17 +++++++++++++++++
 3 files changed, 23 insertions(+)

diff --git a/hw/spapr.c b/hw/spapr.c
index d0bddbc..1feb739 100644
--- a/hw/spapr.c
+++ b/hw/spapr.c
@@ -661,6 +661,11 @@ static void ppc_spapr_init(ram_addr_t ram_size,
         spapr_vscsi_create(spapr->vio_bus);
     }
 
+    if (usb_on == 1) {
+        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
+                          -1, "pci-ohci");
+    }
+
     if (rma_size < (MIN_RMA_SLOF << 20)) {
         fprintf(stderr, "qemu: pSeries SLOF firmware requires >= "
                 "%ldM guest RMA (Real Mode Area memory)\n", MIN_RMA_SLOF);
diff --git a/sysemu.h b/sysemu.h
index bc2c788..08134ae 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -109,6 +109,7 @@ extern int vga_interface_type;
 #define vmsvga_enabled (vga_interface_type == VGA_VMWARE)
 #define qxl_enabled (vga_interface_type == VGA_QXL)
 
+extern int usb_on;
 extern int graphic_width;
 extern int graphic_height;
 extern int graphic_depth;
diff --git a/vl.c b/vl.c
index 204d85b..b200203 100644
--- a/vl.c
+++ b/vl.c
@@ -202,6 +202,7 @@ int smp_cpus = 1;
 int max_cpus = 0;
 int smp_cores = 1;
 int smp_threads = 1;
+int usb_on = 0;
 #ifdef CONFIG_VNC
 const char *vnc_display;
 #endif
@@ -758,6 +759,21 @@ static int bt_parse(const char *opt)
     return 1;
 }
 
+static int get_usb_opt(QemuOpts *opts)
+{
+    const char *usb_opt = NULL;
+    int usb_on = 0;
+
+    if (NULL == qemu_opt_get(opts, "usb"))
+        qemu_opt_set(opts, "usb", "on");
+
+    usb_opt = qemu_opt_get(opts, "usb");
+    if (usb_opt && strcmp(usb_opt, "on") == 0)
+        usb_on = 1;
+
+    return usb_on;
+}
+
 /***********************************************************/
 /* QEMU Block devices */
 
@@ -3356,6 +3372,7 @@ int main(int argc, char **argv, char **envp)
         kernel_filename = qemu_opt_get(machine_opts, "kernel");
         initrd_filename = qemu_opt_get(machine_opts, "initrd");
         kernel_cmdline = qemu_opt_get(machine_opts, "append");
+        usb_on = get_usb_opt(machine_opts);
     } else {
         kernel_filename = initrd_filename = kernel_cmdline = NULL;
     }
-- 
1.7.9.5

Re: [Qemu-devel] [PATCHv2 1/1] Add usb option in machine options to enable/disable usb

[Markus Armbruster]

Li Zhang <zhlcindy@linux.vnet.ibm.com> writes:

> For pseries machine, it needs to enable usb to add
> keyboard or usb mouse. -usb option won't be used in
> the future, and machine options is a better way to
> enable usb.
>
> So this patch is to add usb option to machine options
> (-machine type=psereis,usb=on/off)to enable/disable
> usb controller.
>
> In this patch, usb_on is an global option which can
> be checked by machines.
> For example, on pseries, it will check if usb_on is 1,
> if it is 1, it will create one usb ohci controller.
> As the following:
> if (usb_on == 1) {
>      pci_create_simple(bus, -1, "pci-ohci");
> }
>
> In this patch, usb is on by default. So, for -nodefault,
> usb should be set off in the command line as the following:
>  -machine type=pseries,usb=off.
>
> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>
> ---
>  hw/spapr.c |    5 +++++
>  sysemu.h   |    1 +
>  vl.c       |   17 +++++++++++++++++
>  3 files changed, 23 insertions(+)
>
> diff --git a/hw/spapr.c b/hw/spapr.c
> index d0bddbc..1feb739 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -661,6 +661,11 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>          spapr_vscsi_create(spapr->vio_bus);
>      }
>  
> +    if (usb_on == 1) {
> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
> +                          -1, "pci-ohci");
> +    }
> +
>      if (rma_size < (MIN_RMA_SLOF << 20)) {
>          fprintf(stderr, "qemu: pSeries SLOF firmware requires >= "
>                  "%ldM guest RMA (Real Mode Area memory)\n", MIN_RMA_SLOF);
> diff --git a/sysemu.h b/sysemu.h
> index bc2c788..08134ae 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -109,6 +109,7 @@ extern int vga_interface_type;
>  #define vmsvga_enabled (vga_interface_type == VGA_VMWARE)
>  #define qxl_enabled (vga_interface_type == VGA_QXL)
>  
> +extern int usb_on;
>  extern int graphic_width;
>  extern int graphic_height;
>  extern int graphic_depth;
> diff --git a/vl.c b/vl.c
> index 204d85b..b200203 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -202,6 +202,7 @@ int smp_cpus = 1;
>  int max_cpus = 0;
>  int smp_cores = 1;
>  int smp_threads = 1;
> +int usb_on = 0;
>  #ifdef CONFIG_VNC
>  const char *vnc_display;
>  #endif
> @@ -758,6 +759,21 @@ static int bt_parse(const char *opt)
>      return 1;
>  }
>  
> +static int get_usb_opt(QemuOpts *opts)
> +{
> +    const char *usb_opt = NULL;

Useless initializer.

> +    int usb_on = 0;
> +
> +    if (NULL == qemu_opt_get(opts, "usb"))
> +        qemu_opt_set(opts, "usb", "on");

Why are you changing opts?

> +
> +    usb_opt = qemu_opt_get(opts, "usb");
> +    if (usb_opt && strcmp(usb_opt, "on") == 0)

Please don't duplicate parsing of bools; use qemu_opt_get_bool().

> +        usb_on = 1;
> +
> +    return usb_on;
> +}
> +
>  /***********************************************************/
>  /* QEMU Block devices */
>  
> @@ -3356,6 +3372,7 @@ int main(int argc, char **argv, char **envp)
>          kernel_filename = qemu_opt_get(machine_opts, "kernel");
>          initrd_filename = qemu_opt_get(machine_opts, "initrd");
>          kernel_cmdline = qemu_opt_get(machine_opts, "append");
> +        usb_on = get_usb_opt(machine_opts);

Anything wrong with

         usb_on = qemu_opt_get_bool(machine_opts, "usb", 0);

?

>      } else {
>          kernel_filename = initrd_filename = kernel_cmdline = NULL;
>      }

Your new global usb_on is still unused, and it's not integrated with
-usb.  I doubt it can be merged that way.

Re: [Qemu-devel] [PATCHv2 1/1] Add usb option in machine options to enable/disable usb

[Andreas Färber]

Am 15.06.2012 12:06, schrieb Li Zhang:
> For pseries machine, it needs to enable usb to add
> keyboard or usb mouse. -usb option won't be used in
> the future, and machine options is a better way to
> enable usb.
> 
> So this patch is to add usb option to machine options
> (-machine type=psereis,usb=on/off)to enable/disable
> usb controller.
> 
> In this patch, usb_on is an global option which can
> be checked by machines.
[snip]

...which is exactly what you've been asked to change in v1. Please do.

Also please stick to QEMU Coding Style, which requires braces for if.
Use of the bool type is preferred of int for two-state logic.

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] [PATCHv2 1/1] Add usb option in machine options to enable/disable usb

[Li Zhang]

On Fri, Jun 15, 2012 at 8:04 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Li Zhang <zhlcindy@linux.vnet.ibm.com> writes:
>
>> For pseries machine, it needs to enable usb to add
>> keyboard or usb mouse. -usb option won't be used in
>> the future, and machine options is a better way to
>> enable usb.
>>
>> So this patch is to add usb option to machine options
>> (-machine type=psereis,usb=on/off)to enable/disable
>> usb controller.
>>
>> In this patch, usb_on is an global option which can
>> be checked by machines.
>> For example, on pseries, it will check if usb_on is 1,
>> if it is 1, it will create one usb ohci controller.
>> As the following:
>> if (usb_on == 1) {
>>      pci_create_simple(bus, -1, "pci-ohci");
>> }
>>
>> In this patch, usb is on by default. So, for -nodefault,
>> usb should be set off in the command line as the following:
>>  -machine type=pseries,usb=off.
>>
>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>
>> ---
>>  hw/spapr.c |    5 +++++
>>  sysemu.h   |    1 +
>>  vl.c       |   17 +++++++++++++++++
>>  3 files changed, 23 insertions(+)
>>
>> diff --git a/hw/spapr.c b/hw/spapr.c
>> index d0bddbc..1feb739 100644
>> --- a/hw/spapr.c
>> +++ b/hw/spapr.c
>> @@ -661,6 +661,11 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>          spapr_vscsi_create(spapr->vio_bus);
>>      }
>>
>> +    if (usb_on == 1) {
>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>> +                          -1, "pci-ohci");
>> +    }
>> +
>>      if (rma_size < (MIN_RMA_SLOF << 20)) {
>>          fprintf(stderr, "qemu: pSeries SLOF firmware requires >= "
>>                  "%ldM guest RMA (Real Mode Area memory)\n", MIN_RMA_SLOF);
>> diff --git a/sysemu.h b/sysemu.h
>> index bc2c788..08134ae 100644
>> --- a/sysemu.h
>> +++ b/sysemu.h
>> @@ -109,6 +109,7 @@ extern int vga_interface_type;
>>  #define vmsvga_enabled (vga_interface_type == VGA_VMWARE)
>>  #define qxl_enabled (vga_interface_type == VGA_QXL)
>>
>> +extern int usb_on;
>>  extern int graphic_width;
>>  extern int graphic_height;
>>  extern int graphic_depth;
>> diff --git a/vl.c b/vl.c
>> index 204d85b..b200203 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -202,6 +202,7 @@ int smp_cpus = 1;
>>  int max_cpus = 0;
>>  int smp_cores = 1;
>>  int smp_threads = 1;
>> +int usb_on = 0;
>>  #ifdef CONFIG_VNC
>>  const char *vnc_display;
>>  #endif
>> @@ -758,6 +759,21 @@ static int bt_parse(const char *opt)
>>      return 1;
>>  }
>>
>> +static int get_usb_opt(QemuOpts *opts)
>> +{
>> +    const char *usb_opt = NULL;
>
> Useless initializer.
Thanks. I will remove it.
>
>> +    int usb_on = 0;
>> +
>> +    if (NULL == qemu_opt_get(opts, "usb"))
>> +        qemu_opt_set(opts, "usb", "on");
>
> Why are you changing opts?
USB is enabled by default when there is no usb option setting.
For example,
using  # qemu-system-ppc64 -machine type=pseries
There is no usb option, but usb is set on.
>
>> +
>> +    usb_opt = qemu_opt_get(opts, "usb");
>> +    if (usb_opt && strcmp(usb_opt, "on") == 0)
>
> Please don't duplicate parsing of bools; use qemu_opt_get_bool().
OK.
>
>> +        usb_on = 1;
>> +
>> +    return usb_on;
>> +}
>> +
>>  /***********************************************************/
>>  /* QEMU Block devices */
>>
>> @@ -3356,6 +3372,7 @@ int main(int argc, char **argv, char **envp)
>>          kernel_filename = qemu_opt_get(machine_opts, "kernel");
>>          initrd_filename = qemu_opt_get(machine_opts, "initrd");
>>          kernel_cmdline = qemu_opt_get(machine_opts, "append");
>> +        usb_on = get_usb_opt(machine_opts);
>
> Anything wrong with
>
>         usb_on = qemu_opt_get_bool(machine_opts, "usb", 0);
>
> ?
>
>>      } else {
>>          kernel_filename = initrd_filename = kernel_cmdline = NULL;
>>      }
>
> Your new global usb_on is still unused, and it's not integrated with
> -usb.  I doubt it can be merged that way.
>
It is used in spapr.c. In the front of this patch, it is used.
It seems that this is not good.
Maybe it is better to move the options setting  to spapr.c.



-- 

Best Regards
-Li

Re: [Qemu-devel] [PATCHv2 1/1] Add usb option in machine options to enable/disable usb

[Li Zhang]

On Fri, Jun 15, 2012 at 8:30 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 15.06.2012 12:06, schrieb Li Zhang:
>> For pseries machine, it needs to enable usb to add
>> keyboard or usb mouse. -usb option won't be used in
>> the future, and machine options is a better way to
>> enable usb.
>>
>> So this patch is to add usb option to machine options
>> (-machine type=psereis,usb=on/off)to enable/disable
>> usb controller.
>>
>> In this patch, usb_on is an global option which can
>> be checked by machines.
> [snip]
>
> ...which is exactly what you've been asked to change in v1. Please do.
>
OK. I will do it. :)

> Also please stick to QEMU Coding Style, which requires braces for if.
> Use of the bool type is preferred of int for two-state logic.
>
Got it. I will do it carefully in the future.
> 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
>



-- 

Best Regards
-Li

Re: [Qemu-devel] [PATCHv2 1/1] Add usb option in machine options to enable/disable usb

[Markus Armbruster]

Li Zhang <zhlcindy@gmail.com> writes:

> On Fri, Jun 15, 2012 at 8:04 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Li Zhang <zhlcindy@linux.vnet.ibm.com> writes:
>>
>>> For pseries machine, it needs to enable usb to add
>>> keyboard or usb mouse. -usb option won't be used in
>>> the future, and machine options is a better way to
>>> enable usb.
>>>
>>> So this patch is to add usb option to machine options
>>> (-machine type=psereis,usb=on/off)to enable/disable
>>> usb controller.
>>>
>>> In this patch, usb_on is an global option which can
>>> be checked by machines.
>>> For example, on pseries, it will check if usb_on is 1,
>>> if it is 1, it will create one usb ohci controller.
>>> As the following:
>>> if (usb_on == 1) {
>>>      pci_create_simple(bus, -1, "pci-ohci");
>>> }
>>>
>>> In this patch, usb is on by default. So, for -nodefault,
>>> usb should be set off in the command line as the following:
>>>  -machine type=pseries,usb=off.
>>>
>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>
>>> ---
>>>  hw/spapr.c |    5 +++++
>>>  sysemu.h   |    1 +
>>>  vl.c       |   17 +++++++++++++++++
>>>  3 files changed, 23 insertions(+)
>>>
>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>> index d0bddbc..1feb739 100644
>>> --- a/hw/spapr.c
>>> +++ b/hw/spapr.c
>>> @@ -661,6 +661,11 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>          spapr_vscsi_create(spapr->vio_bus);
>>>      }
>>>
>>> +    if (usb_on == 1) {
>>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>> +                          -1, "pci-ohci");
>>> +    }
>>> +
>>>      if (rma_size < (MIN_RMA_SLOF << 20)) {
>>>          fprintf(stderr, "qemu: pSeries SLOF firmware requires >= "
>>>                  "%ldM guest RMA (Real Mode Area memory)\n", MIN_RMA_SLOF);
>>> diff --git a/sysemu.h b/sysemu.h
>>> index bc2c788..08134ae 100644
>>> --- a/sysemu.h
>>> +++ b/sysemu.h
>>> @@ -109,6 +109,7 @@ extern int vga_interface_type;
>>>  #define vmsvga_enabled (vga_interface_type == VGA_VMWARE)
>>>  #define qxl_enabled (vga_interface_type == VGA_QXL)
>>>
>>> +extern int usb_on;
>>>  extern int graphic_width;
>>>  extern int graphic_height;
>>>  extern int graphic_depth;
>>> diff --git a/vl.c b/vl.c
>>> index 204d85b..b200203 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -202,6 +202,7 @@ int smp_cpus = 1;
>>>  int max_cpus = 0;
>>>  int smp_cores = 1;
>>>  int smp_threads = 1;
>>> +int usb_on = 0;
>>>  #ifdef CONFIG_VNC
>>>  const char *vnc_display;
>>>  #endif
>>> @@ -758,6 +759,21 @@ static int bt_parse(const char *opt)
>>>      return 1;
>>>  }
>>>
>>> +static int get_usb_opt(QemuOpts *opts)
>>> +{
>>> +    const char *usb_opt = NULL;
>>
>> Useless initializer.
> Thanks. I will remove it.
>>
>>> +    int usb_on = 0;
>>> +
>>> +    if (NULL == qemu_opt_get(opts, "usb"))
>>> +        qemu_opt_set(opts, "usb", "on");
>>
>> Why are you changing opts?
> USB is enabled by default when there is no usb option setting.
> For example,
> using  # qemu-system-ppc64 -machine type=pseries
> There is no usb option, but usb is set on.

Isn't it off by default for at least some machines now?

Anyway, I don't see why we need to update opts.  Who's using the updated
opts?

>>> +
>>> +    usb_opt = qemu_opt_get(opts, "usb");
>>> +    if (usb_opt && strcmp(usb_opt, "on") == 0)
>>
>> Please don't duplicate parsing of bools; use qemu_opt_get_bool().
> OK.
>>
>>> +        usb_on = 1;
>>> +
>>> +    return usb_on;
>>> +}
>>> +
>>>  /***********************************************************/
>>>  /* QEMU Block devices */
>>>
>>> @@ -3356,6 +3372,7 @@ int main(int argc, char **argv, char **envp)
>>>          kernel_filename = qemu_opt_get(machine_opts, "kernel");
>>>          initrd_filename = qemu_opt_get(machine_opts, "initrd");
>>>          kernel_cmdline = qemu_opt_get(machine_opts, "append");
>>> +        usb_on = get_usb_opt(machine_opts);
>>
>> Anything wrong with
>>
>>         usb_on = qemu_opt_get_bool(machine_opts, "usb", 0);
>>
>> ?
>>
>>>      } else {
>>>          kernel_filename = initrd_filename = kernel_cmdline = NULL;
>>>      }
>>
>> Your new global usb_on is still unused, and it's not integrated with
>> -usb.  I doubt it can be merged that way.
>>
> It is used in spapr.c. In the front of this patch, it is used.
> It seems that this is not good.
> Maybe it is better to move the options setting  to spapr.c.

Dang, I missed that.  Sorry for the noise.

Re: [Qemu-devel] [PATCHv2 1/1] Add usb option in machine options to enable/disable usb

[Li Zhang]

On Fri, Jun 15, 2012 at 10:34 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Li Zhang <zhlcindy@gmail.com> writes:
>
>> On Fri, Jun 15, 2012 at 8:04 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Li Zhang <zhlcindy@linux.vnet.ibm.com> writes:
>>>
>>>> For pseries machine, it needs to enable usb to add
>>>> keyboard or usb mouse. -usb option won't be used in
>>>> the future, and machine options is a better way to
>>>> enable usb.
>>>>
>>>> So this patch is to add usb option to machine options
>>>> (-machine type=psereis,usb=on/off)to enable/disable
>>>> usb controller.
>>>>
>>>> In this patch, usb_on is an global option which can
>>>> be checked by machines.
>>>> For example, on pseries, it will check if usb_on is 1,
>>>> if it is 1, it will create one usb ohci controller.
>>>> As the following:
>>>> if (usb_on == 1) {
>>>>      pci_create_simple(bus, -1, "pci-ohci");
>>>> }
>>>>
>>>> In this patch, usb is on by default. So, for -nodefault,
>>>> usb should be set off in the command line as the following:
>>>>  -machine type=pseries,usb=off.
>>>>
>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>
>>>> ---
>>>>  hw/spapr.c |    5 +++++
>>>>  sysemu.h   |    1 +
>>>>  vl.c       |   17 +++++++++++++++++
>>>>  3 files changed, 23 insertions(+)
>>>>
>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>> index d0bddbc..1feb739 100644
>>>> --- a/hw/spapr.c
>>>> +++ b/hw/spapr.c
>>>> @@ -661,6 +661,11 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>>          spapr_vscsi_create(spapr->vio_bus);
>>>>      }
>>>>
>>>> +    if (usb_on == 1) {
>>>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>> +                          -1, "pci-ohci");
>>>> +    }
>>>> +
>>>>      if (rma_size < (MIN_RMA_SLOF << 20)) {
>>>>          fprintf(stderr, "qemu: pSeries SLOF firmware requires >= "
>>>>                  "%ldM guest RMA (Real Mode Area memory)\n", MIN_RMA_SLOF);
>>>> diff --git a/sysemu.h b/sysemu.h
>>>> index bc2c788..08134ae 100644
>>>> --- a/sysemu.h
>>>> +++ b/sysemu.h
>>>> @@ -109,6 +109,7 @@ extern int vga_interface_type;
>>>>  #define vmsvga_enabled (vga_interface_type == VGA_VMWARE)
>>>>  #define qxl_enabled (vga_interface_type == VGA_QXL)
>>>>
>>>> +extern int usb_on;
>>>>  extern int graphic_width;
>>>>  extern int graphic_height;
>>>>  extern int graphic_depth;
>>>> diff --git a/vl.c b/vl.c
>>>> index 204d85b..b200203 100644
>>>> --- a/vl.c
>>>> +++ b/vl.c
>>>> @@ -202,6 +202,7 @@ int smp_cpus = 1;
>>>>  int max_cpus = 0;
>>>>  int smp_cores = 1;
>>>>  int smp_threads = 1;
>>>> +int usb_on = 0;
>>>>  #ifdef CONFIG_VNC
>>>>  const char *vnc_display;
>>>>  #endif
>>>> @@ -758,6 +759,21 @@ static int bt_parse(const char *opt)
>>>>      return 1;
>>>>  }
>>>>
>>>> +static int get_usb_opt(QemuOpts *opts)
>>>> +{
>>>> +    const char *usb_opt = NULL;
>>>
>>> Useless initializer.
>> Thanks. I will remove it.
>>>
>>>> +    int usb_on = 0;
>>>> +
>>>> +    if (NULL == qemu_opt_get(opts, "usb"))
>>>> +        qemu_opt_set(opts, "usb", "on");
>>>
>>> Why are you changing opts?
>> USB is enabled by default when there is no usb option setting.
>> For example,
>> using  # qemu-system-ppc64 -machine type=pseries
>> There is no usb option, but usb is set on.
>
> Isn't it off by default for at least some machines now?
>
OK. This default setting is decided by the machine.
In the new version, I put this setting in machine.
It can be set off or on.
For psereis it sets on.
> Anyway, I don't see why we need to update opts.  Who's using the updated
> opts?
>
psereis will use this opts.
usb kbd and mouse will be needed  with vga enabled.
>>>> +
>>>> +    usb_opt = qemu_opt_get(opts, "usb");
>>>> +    if (usb_opt && strcmp(usb_opt, "on") == 0)
>>>
>>> Please don't duplicate parsing of bools; use qemu_opt_get_bool().
>> OK.
>>>
>>>> +        usb_on = 1;
>>>> +
>>>> +    return usb_on;
>>>> +}
>>>> +
>>>>  /***********************************************************/
>>>>  /* QEMU Block devices */
>>>>
>>>> @@ -3356,6 +3372,7 @@ int main(int argc, char **argv, char **envp)
>>>>          kernel_filename = qemu_opt_get(machine_opts, "kernel");
>>>>          initrd_filename = qemu_opt_get(machine_opts, "initrd");
>>>>          kernel_cmdline = qemu_opt_get(machine_opts, "append");
>>>> +        usb_on = get_usb_opt(machine_opts);
>>>
>>> Anything wrong with
>>>
>>>         usb_on = qemu_opt_get_bool(machine_opts, "usb", 0);
>>>
>>> ?
>>>
>>>>      } else {
>>>>          kernel_filename = initrd_filename = kernel_cmdline = NULL;
>>>>      }
>>>
>>> Your new global usb_on is still unused, and it's not integrated with
>>> -usb.  I doubt it can be merged that way.
>>>
>> It is used in spapr.c. In the front of this patch, it is used.
>> It seems that this is not good.
>> Maybe it is better to move the options setting  to spapr.c.
>
> Dang, I missed that.  Sorry for the noise.



-- 

Best Regards
-Li

Re: [Qemu-devel] [PATCHv2 1/1] Add usb option in machine options to enable/disable usb

[Markus Armbruster]

Li Zhang <zhlcindy@gmail.com> writes:

> On Fri, Jun 15, 2012 at 10:34 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Li Zhang <zhlcindy@gmail.com> writes:
>>
>>> On Fri, Jun 15, 2012 at 8:04 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Li Zhang <zhlcindy@linux.vnet.ibm.com> writes:
>>>>
>>>>> For pseries machine, it needs to enable usb to add
>>>>> keyboard or usb mouse. -usb option won't be used in
>>>>> the future, and machine options is a better way to
>>>>> enable usb.
>>>>>
>>>>> So this patch is to add usb option to machine options
>>>>> (-machine type=psereis,usb=on/off)to enable/disable
>>>>> usb controller.
>>>>>
>>>>> In this patch, usb_on is an global option which can
>>>>> be checked by machines.
>>>>> For example, on pseries, it will check if usb_on is 1,
>>>>> if it is 1, it will create one usb ohci controller.
>>>>> As the following:
>>>>> if (usb_on == 1) {
>>>>>      pci_create_simple(bus, -1, "pci-ohci");
>>>>> }
>>>>>
>>>>> In this patch, usb is on by default. So, for -nodefault,
>>>>> usb should be set off in the command line as the following:
>>>>>  -machine type=pseries,usb=off.
>>>>>
>>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>>
>>>>> ---
>>>>>  hw/spapr.c |    5 +++++
>>>>>  sysemu.h   |    1 +
>>>>>  vl.c       |   17 +++++++++++++++++
>>>>>  3 files changed, 23 insertions(+)
>>>>>
>>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>>> index d0bddbc..1feb739 100644
>>>>> --- a/hw/spapr.c
>>>>> +++ b/hw/spapr.c
>>>>> @@ -661,6 +661,11 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>>>          spapr_vscsi_create(spapr->vio_bus);
>>>>>      }
>>>>>
>>>>> +    if (usb_on == 1) {
>>>>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>>> +                          -1, "pci-ohci");
>>>>> +    }
>>>>> +
>>>>>      if (rma_size < (MIN_RMA_SLOF << 20)) {
>>>>>          fprintf(stderr, "qemu: pSeries SLOF firmware requires >= "
>>>>>                  "%ldM guest RMA (Real Mode Area memory)\n", MIN_RMA_SLOF);
>>>>> diff --git a/sysemu.h b/sysemu.h
>>>>> index bc2c788..08134ae 100644
>>>>> --- a/sysemu.h
>>>>> +++ b/sysemu.h
>>>>> @@ -109,6 +109,7 @@ extern int vga_interface_type;
>>>>>  #define vmsvga_enabled (vga_interface_type == VGA_VMWARE)
>>>>>  #define qxl_enabled (vga_interface_type == VGA_QXL)
>>>>>
>>>>> +extern int usb_on;
>>>>>  extern int graphic_width;
>>>>>  extern int graphic_height;
>>>>>  extern int graphic_depth;
>>>>> diff --git a/vl.c b/vl.c
>>>>> index 204d85b..b200203 100644
>>>>> --- a/vl.c
>>>>> +++ b/vl.c
>>>>> @@ -202,6 +202,7 @@ int smp_cpus = 1;
>>>>>  int max_cpus = 0;
>>>>>  int smp_cores = 1;
>>>>>  int smp_threads = 1;
>>>>> +int usb_on = 0;
>>>>>  #ifdef CONFIG_VNC
>>>>>  const char *vnc_display;
>>>>>  #endif
>>>>> @@ -758,6 +759,21 @@ static int bt_parse(const char *opt)
>>>>>      return 1;
>>>>>  }
>>>>>
>>>>> +static int get_usb_opt(QemuOpts *opts)
>>>>> +{
>>>>> +    const char *usb_opt = NULL;
>>>>
>>>> Useless initializer.
>>> Thanks. I will remove it.
>>>>
>>>>> +    int usb_on = 0;
>>>>> +
>>>>> +    if (NULL == qemu_opt_get(opts, "usb"))
>>>>> +        qemu_opt_set(opts, "usb", "on");
>>>>
>>>> Why are you changing opts?
>>> USB is enabled by default when there is no usb option setting.
>>> For example,
>>> using  # qemu-system-ppc64 -machine type=pseries
>>> There is no usb option, but usb is set on.
>>
>> Isn't it off by default for at least some machines now?
>>
> OK. This default setting is decided by the machine.
> In the new version, I put this setting in machine.
> It can be set off or on.
> For psereis it sets on.

Makes sense.

Perhaps we really have three kinds of machines, not just two:

1. Must have USB: main() sets usb_enabled to true.

2. May have USB: usb_enabled = -usb or -usbdevice given

3. Can't have USB: fail if the user tries to enable it.

Code sketch:

    /* init USB devices */
    if (!machine->has_usb) {
        if (usb_enabled)
            [report error; should point to the offending options]
            exit(1);
        }
    } else {
        if (machine->has_usb > 0) {
            usb_enabled = 1;
        }
        if (usb_enabled) {
            if (foreach_device_config(DEV_USB, usb_parse) < 0)
                exit(1);
        }
    }


>> Anyway, I don't see why we need to update opts.  Who's using the updated
>> opts?
>>
> psereis will use this opts.
> usb kbd and mouse will be needed  with vga enabled.

Do they use the updated QemuOpts *opts?  I'd expect them to use usb_on,
or whatever flag variable governs USB (now: usb_enabled).

[...]

Re: [Qemu-devel] [PATCHv2 1/1] Add usb option in machine options to enable/disable usb

[Peter Crosthwaite]

>
> 3. Can't have USB: fail if the user tries to enable it.
>
> Code sketch:
>
>    /* init USB devices */
>    if (!machine->has_usb) {
>        if (usb_enabled)
>            [report error; should point to the offending options]
>            exit(1);
>        }
>    } else {
>        if (machine->has_usb > 0) {
>            usb_enabled = 1;
>        }
>        if (usb_enabled) {
>            if (foreach_device_config(DEV_USB, usb_parse) < 0)
>                exit(1);
>        }
>    }
>
>
>>> Anyway, I don't see why we need to update opts.  Who's using the updated
>>> opts?
>>>
>> psereis will use this opts.
>> usb kbd and mouse will be needed  with vga enabled.
>
> Do they use the updated QemuOpts *opts?  I'd expect them to use usb_on,
> or whatever flag variable governs USB (now: usb_enabled).
>

I think whats going on here is Li is trying to do the right thing by
using QEMU opts for this new machine functionality, however, its
getting tangled with all this global state replication of -usb. Isnt
there predecessor work here in getting rid of usb_enabled first? To
that end, I think what is being proposed here is two (somewhat
independent) patches. One patch for changing usb to QEMU_OPTS that
primarily does this:

diff --git a/sysemu.h b/sysemu.h
index bc2c788..9f5ce2c 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -117,7 +117,6 @@ extern const char *keyboard_layout;
 extern int win2k_install_hack;
 extern int alt_grab;
 extern int ctrl_grab;
-extern int usb_enabled;
 extern int smp_cpus;
 extern int max_cpus;
 extern int cursor_hide;
[6]   Done                    gedit ./sysemu.h

And the second patch which is the pseries machine model stuff.

Which way round probably doesnt matter right? You could make your
machine model use the extern int usb_enabled initially then move it
across to machine opts along with the rest of the usb subsystem. Or
you could fix USB first (globally) then build on top of it. But I
think that this patch as is, is going to do is introduce is a
duplicate -usb implementation which is a little messy (even if it is
only an intermediary state).

Regards,
Peter

> [...]
>

Re: [Qemu-devel] [PATCHv2 1/1] Add usb option in machine options to enable/disable usb

[Li Zhang]

On Mon, Jun 18, 2012 at 3:29 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Li Zhang <zhlcindy@gmail.com> writes:
>
>> On Fri, Jun 15, 2012 at 10:34 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Li Zhang <zhlcindy@gmail.com> writes:
>>>
>>>> On Fri, Jun 15, 2012 at 8:04 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>> Li Zhang <zhlcindy@linux.vnet.ibm.com> writes:
>>>>>
>>>>>> For pseries machine, it needs to enable usb to add
>>>>>> keyboard or usb mouse. -usb option won't be used in
>>>>>> the future, and machine options is a better way to
>>>>>> enable usb.
>>>>>>
>>>>>> So this patch is to add usb option to machine options
>>>>>> (-machine type=psereis,usb=on/off)to enable/disable
>>>>>> usb controller.
>>>>>>
>>>>>> In this patch, usb_on is an global option which can
>>>>>> be checked by machines.
>>>>>> For example, on pseries, it will check if usb_on is 1,
>>>>>> if it is 1, it will create one usb ohci controller.
>>>>>> As the following:
>>>>>> if (usb_on == 1) {
>>>>>>      pci_create_simple(bus, -1, "pci-ohci");
>>>>>> }
>>>>>>
>>>>>> In this patch, usb is on by default. So, for -nodefault,
>>>>>> usb should be set off in the command line as the following:
>>>>>>  -machine type=pseries,usb=off.
>>>>>>
>>>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>>>
>>>>>> ---
>>>>>>  hw/spapr.c |    5 +++++
>>>>>>  sysemu.h   |    1 +
>>>>>>  vl.c       |   17 +++++++++++++++++
>>>>>>  3 files changed, 23 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>>>> index d0bddbc..1feb739 100644
>>>>>> --- a/hw/spapr.c
>>>>>> +++ b/hw/spapr.c
>>>>>> @@ -661,6 +661,11 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>>>>          spapr_vscsi_create(spapr->vio_bus);
>>>>>>      }
>>>>>>
>>>>>> +    if (usb_on == 1) {
>>>>>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>>>> +                          -1, "pci-ohci");
>>>>>> +    }
>>>>>> +
>>>>>>      if (rma_size < (MIN_RMA_SLOF << 20)) {
>>>>>>          fprintf(stderr, "qemu: pSeries SLOF firmware requires >= "
>>>>>>                  "%ldM guest RMA (Real Mode Area memory)\n", MIN_RMA_SLOF);
>>>>>> diff --git a/sysemu.h b/sysemu.h
>>>>>> index bc2c788..08134ae 100644
>>>>>> --- a/sysemu.h
>>>>>> +++ b/sysemu.h
>>>>>> @@ -109,6 +109,7 @@ extern int vga_interface_type;
>>>>>>  #define vmsvga_enabled (vga_interface_type == VGA_VMWARE)
>>>>>>  #define qxl_enabled (vga_interface_type == VGA_QXL)
>>>>>>
>>>>>> +extern int usb_on;
>>>>>>  extern int graphic_width;
>>>>>>  extern int graphic_height;
>>>>>>  extern int graphic_depth;
>>>>>> diff --git a/vl.c b/vl.c
>>>>>> index 204d85b..b200203 100644
>>>>>> --- a/vl.c
>>>>>> +++ b/vl.c
>>>>>> @@ -202,6 +202,7 @@ int smp_cpus = 1;
>>>>>>  int max_cpus = 0;
>>>>>>  int smp_cores = 1;
>>>>>>  int smp_threads = 1;
>>>>>> +int usb_on = 0;
>>>>>>  #ifdef CONFIG_VNC
>>>>>>  const char *vnc_display;
>>>>>>  #endif
>>>>>> @@ -758,6 +759,21 @@ static int bt_parse(const char *opt)
>>>>>>      return 1;
>>>>>>  }
>>>>>>
>>>>>> +static int get_usb_opt(QemuOpts *opts)
>>>>>> +{
>>>>>> +    const char *usb_opt = NULL;
>>>>>
>>>>> Useless initializer.
>>>> Thanks. I will remove it.
>>>>>
>>>>>> +    int usb_on = 0;
>>>>>> +
>>>>>> +    if (NULL == qemu_opt_get(opts, "usb"))
>>>>>> +        qemu_opt_set(opts, "usb", "on");
>>>>>
>>>>> Why are you changing opts?
>>>> USB is enabled by default when there is no usb option setting.
>>>> For example,
>>>> using  # qemu-system-ppc64 -machine type=pseries
>>>> There is no usb option, but usb is set on.
>>>
>>> Isn't it off by default for at least some machines now?
>>>
>> OK. This default setting is decided by the machine.
>> In the new version, I put this setting in machine.
>> It can be set off or on.
>> For psereis it sets on.
>
> Makes sense.
>
> Perhaps we really have three kinds of machines, not just two:
>
> 1. Must have USB: main() sets usb_enabled to true.

We only hope to use usb option of machine options, not use -usb or -usbdevice,
which will be removed in the future.
But there are still some machines are using it.
>
> 2. May have USB: usb_enabled = -usb or -usbdevice given
>
For one machine, if it uses usb option in machine, usb_enabled can't
work for this machine.
In fact, even if user use -usb, it still doesn't work.
> 3. Can't have USB: fail if the user tries to enable it.
>
> Code sketch:
>
>    /* init USB devices */
>    if (!machine->has_usb) {
>        if (usb_enabled)
>            [report error; should point to the offending options]
>            exit(1);
>        }
>    } else {
>        if (machine->has_usb > 0) {
>            usb_enabled = 1;
>        }
>        if (usb_enabled) {
>            if (foreach_device_config(DEV_USB, usb_parse) < 0)
>                exit(1);
>        }
>    }
>
>
In fact, I really hope to remove usb_enabled.

>>> Anyway, I don't see why we need to update opts.  Who's using the updated
>>> opts?
>>>
>> psereis will use this opts.
>> usb kbd and mouse will be needed  with vga enabled.
>
> Do they use the updated QemuOpts *opts?  I'd expect them to use usb_on,
> or whatever flag variable governs USB (now: usb_enabled).

Yes, I have modified this for pseries. It will get usb_on from machine options.
But there some other kinds of machines using usb_enabled.

I have send out my latest patches, but it seems that there are some
issues of my network.
I can't see them in the mailing list. I will send out later.
>
> [...]



-- 

Best Regards
-Li

Re: [Qemu-devel] [PATCHv2 1/1] Add usb option in machine options to enable/disable usb

[Li Zhang]

On Mon, Jun 18, 2012 at 3:54 PM, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
>>
>> 3. Can't have USB: fail if the user tries to enable it.
>>
>> Code sketch:
>>
>>    /* init USB devices */
>>    if (!machine->has_usb) {
>>        if (usb_enabled)
>>            [report error; should point to the offending options]
>>            exit(1);
>>        }
>>    } else {
>>        if (machine->has_usb > 0) {
>>            usb_enabled = 1;
>>        }
>>        if (usb_enabled) {
>>            if (foreach_device_config(DEV_USB, usb_parse) < 0)
>>                exit(1);
>>        }
>>    }
>>
>>
>>>> Anyway, I don't see why we need to update opts.  Who's using the updated
>>>> opts?
>>>>
>>> psereis will use this opts.
>>> usb kbd and mouse will be needed  with vga enabled.
>>
>> Do they use the updated QemuOpts *opts?  I'd expect them to use usb_on,
>> or whatever flag variable governs USB (now: usb_enabled).
>>
>
> I think whats going on here is Li is trying to do the right thing by
> using QEMU opts for this new machine functionality, however, its
> getting tangled with all this global state replication of -usb. Isnt
> there predecessor work here in getting rid of usb_enabled first? To
I won't introduce global state any more in the latest version.
It just gets the usb_on from machine options.
It won't use usb_enabled.

> that end, I think what is being proposed here is two (somewhat
> independent) patches. One patch for changing usb to QEMU_OPTS that
> primarily does this:
>
> diff --git a/sysemu.h b/sysemu.h
> index bc2c788..9f5ce2c 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -117,7 +117,6 @@ extern const char *keyboard_layout;
>  extern int win2k_install_hack;
>  extern int alt_grab;
>  extern int ctrl_grab;
> -extern int usb_enabled;
>  extern int smp_cpus;
>  extern int max_cpus;
>  extern int cursor_hide;
> [6]   Done                    gedit ./sysemu.h
>
> And the second patch which is the pseries machine model stuff.
>
> Which way round probably doesnt matter right? You could make your
Because there are some machines using usb_enabled.
So I'd rather to left it as global state and add another usb option in
machine options.
Then the machine can get usb option from machine options to enable usb.
So the latest patch won't introduce the global state.
I will send out the latest version later.
> machine model use the extern int usb_enabled initially then move it
> across to machine opts along with the rest of the usb subsystem. Or
> you could fix USB first (globally) then build on top of it. But I
> think that this patch as is, is going to do is introduce is a
> duplicate -usb implementation which is a little messy (even if it is
> only an intermediary state).

> Regards,
> Peter
>
>> [...]
>>



-- 

Best Regards
-Li

[Qemu-devel] [PATCH 1/2] Add usb option in machine options to enable/disable usb

[Li Zhang]

For pseries machine, it needs to enable usb to add
keyboard or usb mouse. -usb option won't be used in
the future, and machine options is a better way to
enable usb.

So this patch is to add usb option to machine options
(-machine type=psereis,usb=on/off)to enable/disable
usb controller.

For specific machines, they will get the machine option
and then create usb controller according to usb option.

In this patch, usb is on by default on pseries.
So, for -nodefault,usb should be set off in the command
line as the following:
 -machine type=pseries,usb=off.

Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
---
 hw/spapr.c    |   10 ++++++++++
 qemu-config.c |    4 ++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/hw/spapr.c b/hw/spapr.c
index d0bddbc..8d158d7 100644
--- a/hw/spapr.c
+++ b/hw/spapr.c
@@ -529,6 +529,8 @@ static void ppc_spapr_init(ram_addr_t ram_size,
     long load_limit, rtas_limit, fw_size;
     long pteg_shift = 17;
     char *filename;
+    QemuOpts * machine_opts;
+    bool usb_on = false;

     spapr = g_malloc0(sizeof(*spapr));
     QLIST_INIT(&spapr->phbs);
@@ -661,6 +663,14 @@ static void ppc_spapr_init(ram_addr_t ram_size,
         spapr_vscsi_create(spapr->vio_bus);
     }

+    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
+    if (machine_opts)
+        usb_on = qemu_opt_get_bool(machine_opts, "usb", true);
+
+    if (usb_on) {
+        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
+                          -1, "pci-ohci");
+    }
     if (rma_size < (MIN_RMA_SLOF << 20)) {
         fprintf(stderr, "qemu: pSeries SLOF firmware requires >= "
                 "%ldM guest RMA (Real Mode Area memory)\n", MIN_RMA_SLOF);
diff --git a/qemu-config.c b/qemu-config.c
index bb3bff4..cdab765 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = {
             .name = "dtb",
             .type = QEMU_OPT_STRING,
             .help = "Linux kernel device tree file",
+        },{
+            .name = "usb",
+            .type = QEMU_OPT_BOOL,
+            .help = "Set on/off to enable/disable usb",
         },
         { /* End of list */ }
     },
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 1/2] Add usb option in machine options to enable/disable usb

[陳韋任 (Wei-Ren Chen)]

Hi Li Zhang,

  Perhaps you miss "[PATCH v3 1/2]" in the subject?

Regards,
chenwj

-- 
Wei-Ren Chen (陳韋任)
Computer Systems Lab, Institute of Information Science,
Academia Sinica, Taiwan (R.O.C.)
Tel:886-2-2788-3799 #1667
Homepage: http://people.cs.nctu.edu.tw/~chenwj