[Qemu-devel] [PATCH 1/1] Add usb option in machine options.

[Qemu-devel] [PATCH 1/1] Add usb option in machine options.

[zhlcindy]

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

For pseries machine, it needs to enable usb
to add kbd 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_opt is an global option
which can be checked by machines. For example,
on pseries, it will check if usb_opt is on, if
it is on, it will create one usb ohci controller.
As the following:
if (usb_opts && strcmp(usb_opts, "on") == 0)
     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>
reviewed-by:   Anthony Liguori <aliguori@us.ibm.com>
reviewed-by:   Benjamin Herrenschmidt <benh@au1.ibm.com>
---
 qemu-config.c |    4 ++++
 sysemu.h      |    1 +
 vl.c          |   12 ++++++++++++
 3 files changed, 17 insertions(+)

diff --git a/qemu-config.c b/qemu-config.c
index bb3bff4..258712a 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 */ }
     },
diff --git a/sysemu.h b/sysemu.h
index bc2c788..c5ea10d 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -13,6 +13,7 @@
 /* vl.c */
 
 extern const char *bios_name;
+extern const char *usb_opt;
 
 extern const char *qemu_name;
 extern uint8_t qemu_uuid[];
diff --git a/vl.c b/vl.c
index 204d85b..10f8e4c 100644
--- a/vl.c
+++ b/vl.c
@@ -171,6 +171,7 @@ int main(int argc, char **argv)
 
 static const char *data_dir;
 const char *bios_name = NULL;
+const char *usb_opt = NULL;
 enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
 DisplayType display_type = DT_DEFAULT;
 int display_remote = 0;
@@ -758,6 +759,15 @@ static int bt_parse(const char *opt)
     return 1;
 }
 
+static int default_enable_usb(QemuOpts *opts)
+{
+    if (NULL == qemu_opt_get(opts, "usb")) {
+        qemu_opt_set(opts, "usb", "on");
+    }
+
+    return 0;
+}
+
 /***********************************************************/
 /* QEMU Block devices */
 
@@ -3356,6 +3366,8 @@ 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");
+        default_enable_usb(machine_opts);
+        usb_opt = qemu_opt_get(machine_opts, "usb");
     } else {
         kernel_filename = initrd_filename = kernel_cmdline = NULL;
     }
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 1/1] Add usb option in machine options.

[Andreas Färber]

Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com:
> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> 
> For pseries machine, it needs to enable usb
> to add kbd 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_opt is an global option
> which can be checked by machines. For example,
> on pseries, it will check if usb_opt is on, if
> it is on, it will create one usb ohci controller.
> As the following:
> if (usb_opts && strcmp(usb_opts, "on") == 0)
>      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>
> reviewed-by:   Anthony Liguori <aliguori@us.ibm.com>
> reviewed-by:   Benjamin Herrenschmidt <benh@au1.ibm.com>
> ---
>  qemu-config.c |    4 ++++
>  sysemu.h      |    1 +
>  vl.c          |   12 ++++++++++++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/qemu-config.c b/qemu-config.c
> index bb3bff4..258712a 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 */ }
>      },
> diff --git a/sysemu.h b/sysemu.h
> index bc2c788..c5ea10d 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -13,6 +13,7 @@
>  /* vl.c */
>  
>  extern const char *bios_name;
> +extern const char *usb_opt;
>  
>  extern const char *qemu_name;
>  extern uint8_t qemu_uuid[];
> diff --git a/vl.c b/vl.c
> index 204d85b..10f8e4c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -171,6 +171,7 @@ int main(int argc, char **argv)
>  
>  static const char *data_dir;
>  const char *bios_name = NULL;
> +const char *usb_opt = NULL;
>  enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
>  DisplayType display_type = DT_DEFAULT;
>  int display_remote = 0;

I'd be very surprised if Anthony has actually reviewed this...

The point of using machine options is so that you can use the QemuOpts
infrastructure to inquire this value, not to save more global state.
Especially not a string when all you want is a boolean value.

Further, in this patch it's only being assigned, not used anywhere.

Andreas

> @@ -758,6 +759,15 @@ static int bt_parse(const char *opt)
>      return 1;
>  }
>  
> +static int default_enable_usb(QemuOpts *opts)
> +{
> +    if (NULL == qemu_opt_get(opts, "usb")) {
> +        qemu_opt_set(opts, "usb", "on");
> +    }
> +
> +    return 0;
> +}
> +
>  /***********************************************************/
>  /* QEMU Block devices */
>  
> @@ -3356,6 +3366,8 @@ 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");
> +        default_enable_usb(machine_opts);
> +        usb_opt = qemu_opt_get(machine_opts, "usb");
>      } else {
>          kernel_filename = initrd_filename = kernel_cmdline = NULL;
>      }


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

Re: [Qemu-devel] [PATCH 1/1] Add usb option in machine options.

[Benjamin Herrenschmidt]

On Thu, 2012-06-14 at 16:27 +0200, Andreas Färber wrote:
> Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com:
> > From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> > 
> > For pseries machine, it needs to enable usb
> > to add kbd 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_opt is an global option
> > which can be checked by machines. For example,
> > on pseries, it will check if usb_opt is on, if
> > it is on, it will create one usb ohci controller.
> > As the following:
> > if (usb_opts && strcmp(usb_opts, "on") == 0)
> >      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>
> > reviewed-by:   Anthony Liguori <aliguori@us.ibm.com>
> > reviewed-by:   Benjamin Herrenschmidt <benh@au1.ibm.com>

I have not reviewed that patch. Li, you must -never- make up such
reviewed-by: lines ! You put them in if and only if you've had them
given to you by the reviewer who has actually reviewed the patch in
details.

Ben.
 
> > ---
> >  qemu-config.c |    4 ++++
> >  sysemu.h      |    1 +
> >  vl.c          |   12 ++++++++++++
> >  3 files changed, 17 insertions(+)
> > 
> > diff --git a/qemu-config.c b/qemu-config.c
> > index bb3bff4..258712a 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 */ }
> >      },
> > diff --git a/sysemu.h b/sysemu.h
> > index bc2c788..c5ea10d 100644
> > --- a/sysemu.h
> > +++ b/sysemu.h
> > @@ -13,6 +13,7 @@
> >  /* vl.c */
> >  
> >  extern const char *bios_name;
> > +extern const char *usb_opt;
> >  
> >  extern const char *qemu_name;
> >  extern uint8_t qemu_uuid[];
> > diff --git a/vl.c b/vl.c
> > index 204d85b..10f8e4c 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -171,6 +171,7 @@ int main(int argc, char **argv)
> >  
> >  static const char *data_dir;
> >  const char *bios_name = NULL;
> > +const char *usb_opt = NULL;
> >  enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
> >  DisplayType display_type = DT_DEFAULT;
> >  int display_remote = 0;
> 
> I'd be very surprised if Anthony has actually reviewed this...
> 
> The point of using machine options is so that you can use the QemuOpts
> infrastructure to inquire this value, not to save more global state.
> Especially not a string when all you want is a boolean value.
> 
> Further, in this patch it's only being assigned, not used anywhere.
> 
> Andreas
> 
> > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt)
> >      return 1;
> >  }
> >  
> > +static int default_enable_usb(QemuOpts *opts)
> > +{
> > +    if (NULL == qemu_opt_get(opts, "usb")) {
> > +        qemu_opt_set(opts, "usb", "on");
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  /***********************************************************/
> >  /* QEMU Block devices */
> >  
> > @@ -3356,6 +3366,8 @@ 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");
> > +        default_enable_usb(machine_opts);
> > +        usb_opt = qemu_opt_get(machine_opts, "usb");
> >      } else {
> >          kernel_filename = initrd_filename = kernel_cmdline = NULL;
> >      }
> 
> 

Re: [Qemu-devel] [PATCH 1/1] Add usb option in machine options.

[Li Zhang]

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

OK,sorry for my fault.

best regards

Li Zhang
在 2012-6-15 上午6:05，"Benjamin Herrenschmidt" <benh@au1.ibm.com>写道：
>
> On Thu, 2012-06-14 at 16:27 +0200, Andreas Färber wrote:
> > Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com:
> > > From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> > >
> > > For pseries machine, it needs to enable usb
> > > to add kbd 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_opt is an global option
> > > which can be checked by machines. For example,
> > > on pseries, it will check if usb_opt is on, if
> > > it is on, it will create one usb ohci controller.
> > > As the following:
> > > if (usb_opts && strcmp(usb_opts, "on") == 0)
> > >      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>
> > > reviewed-by:   Anthony Liguori <aliguori@us.ibm.com>
> > > reviewed-by:   Benjamin Herrenschmidt <benh@au1.ibm.com>
>
> I have not reviewed that patch. Li, you must -never- make up such
> reviewed-by: lines ! You put them in if and only if you've had them
> given to you by the reviewer who has actually reviewed the patch in
> details.
>
> Ben.
>
> > > ---
> > >  qemu-config.c |    4 ++++
> > >  sysemu.h      |    1 +
> > >  vl.c          |   12 ++++++++++++
> > >  3 files changed, 17 insertions(+)
> > >
> > > diff --git a/qemu-config.c b/qemu-config.c
> > > index bb3bff4..258712a 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 */ }
> > >      },
> > > diff --git a/sysemu.h b/sysemu.h
> > > index bc2c788..c5ea10d 100644
> > > --- a/sysemu.h
> > > +++ b/sysemu.h
> > > @@ -13,6 +13,7 @@
> > >  /* vl.c */
> > >
> > >  extern const char *bios_name;
> > > +extern const char *usb_opt;
> > >
> > >  extern const char *qemu_name;
> > >  extern uint8_t qemu_uuid[];
> > > diff --git a/vl.c b/vl.c
> > > index 204d85b..10f8e4c 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -171,6 +171,7 @@ int main(int argc, char **argv)
> > >
> > >  static const char *data_dir;
> > >  const char *bios_name = NULL;
> > > +const char *usb_opt = NULL;
> > >  enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
> > >  DisplayType display_type = DT_DEFAULT;
> > >  int display_remote = 0;
> >
> > I'd be very surprised if Anthony has actually reviewed this...
> >
> > The point of using machine options is so that you can use the QemuOpts
> > infrastructure to inquire this value, not to save more global state.
> > Especially not a string when all you want is a boolean value.
> >
> > Further, in this patch it's only being assigned, not used anywhere.
> >
> > Andreas
> >
> > > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt)
> > >      return 1;
> > >  }
> > >
> > > +static int default_enable_usb(QemuOpts *opts)
> > > +{
> > > +    if (NULL == qemu_opt_get(opts, "usb")) {
> > > +        qemu_opt_set(opts, "usb", "on");
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >  /***********************************************************/
> > >  /* QEMU Block devices */
> > >
> > > @@ -3356,6 +3366,8 @@ 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");
> > > +        default_enable_usb(machine_opts);
> > > +        usb_opt = qemu_opt_get(machine_opts, "usb");
> > >      } else {
> > >          kernel_filename = initrd_filename = kernel_cmdline = NULL;
> > >      }
> >
> >
>
>

[-- Attachment #2: Type: text/html, Size: 6596 bytes --]

Re: [Qemu-devel] [PATCH 1/1] Add usb option in machine options.

[Li Zhang]

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

On Thu, Jun 14, 2012 at 10:27 PM, Andreas Färber <afaerber@suse.de> wrote:

> Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com:
> > From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> >
> > For pseries machine, it needs to enable usb
> > to add kbd 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_opt is an global option
> > which can be checked by machines. For example,
> > on pseries, it will check if usb_opt is on, if
> > it is on, it will create one usb ohci controller.
> > As the following:
> > if (usb_opts && strcmp(usb_opts, "on") == 0)
> >      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>
> > reviewed-by:   Anthony Liguori <aliguori@us.ibm.com>
> > reviewed-by:   Benjamin Herrenschmidt <benh@au1.ibm.com>
> > ---
> >  qemu-config.c |    4 ++++
> >  sysemu.h      |    1 +
> >  vl.c          |   12 ++++++++++++
> >  3 files changed, 17 insertions(+)
> >
> > diff --git a/qemu-config.c b/qemu-config.c
> > index bb3bff4..258712a 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 */ }
> >      },
> > diff --git a/sysemu.h b/sysemu.h
> > index bc2c788..c5ea10d 100644
> > --- a/sysemu.h
> > +++ b/sysemu.h
> > @@ -13,6 +13,7 @@
> >  /* vl.c */
> >
> >  extern const char *bios_name;
> > +extern const char *usb_opt;
> >
> >  extern const char *qemu_name;
> >  extern uint8_t qemu_uuid[];
> > diff --git a/vl.c b/vl.c
> > index 204d85b..10f8e4c 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -171,6 +171,7 @@ int main(int argc, char **argv)
> >
> >  static const char *data_dir;
> >  const char *bios_name = NULL;
> > +const char *usb_opt = NULL;
> >  enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
> >  DisplayType display_type = DT_DEFAULT;
> >  int display_remote = 0;
>
> I'd be very surprised if Anthony has actually reviewed this...
>

Sorry, I just think I add him here because he gives some suggestions about
adding usb option to machine options in the mailing list.:)
Maybe it is not appropriate to add here. Sorry for that.

>
> The point of using machine options is so that you can use the QemuOpts
> infrastructure to inquire this value, not to save more global state.
>
OK, I think it still needs one global value because other machines need to
check whether usb is enabled.


> Especially not a string when all you want is a boolean value.
>
> From the qemu, QEMU_OPT_BOOL type  still use a string
"on/off" to enable/disable usb.
Maybe it's better to convert it to boolean value.

Further, in this patch it's only being assigned, not used anywhere.
>
> In vl.c, it seems that it is not used.
I have been thinking to use it in spapr.c.

if (usb_opts && strcmp(usb_opts, "on") == 0)
            pci_create_simple(bus, -1, "pci-ohci");

But I want to see whether this way is accepted.
If it is ok, I will add this option to spapr.c.

Andreas
>
> > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt)
> >      return 1;
> >  }
> >
> > +static int default_enable_usb(QemuOpts *opts)
> > +{
> > +    if (NULL == qemu_opt_get(opts, "usb")) {
> > +        qemu_opt_set(opts, "usb", "on");
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  /***********************************************************/
> >  /* QEMU Block devices */
> >
> > @@ -3356,6 +3366,8 @@ 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");
> > +        default_enable_usb(machine_opts);
> > +        usb_opt = qemu_opt_get(machine_opts, "usb");
> >      } else {
> >          kernel_filename = initrd_filename = kernel_cmdline = NULL;
> >      }
>
>
> --
> 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

[-- Attachment #2: Type: text/html, Size: 6794 bytes --]

Re: [Qemu-devel] [PATCH 1/1] Add usb option in machine options.

[Peter Crosthwaite]

On Fri, Jun 15, 2012 at 1:06 PM, Li Zhang <zhlcindy@gmail.com> wrote:
>
>
> On Thu, Jun 14, 2012 at 10:27 PM, Andreas Färber <afaerber@suse.de> wrote:
>>
>> Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com:
>> > From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>> >
>> > For pseries machine, it needs to enable usb
>> > to add kbd 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_opt is an global option
>> > which can be checked by machines. For example,
>> > on pseries, it will check if usb_opt is on, if
>> > it is on, it will create one usb ohci controller.
>> > As the following:
>> > if (usb_opts && strcmp(usb_opts, "on") == 0)
>> >      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>
>> > reviewed-by:   Anthony Liguori <aliguori@us.ibm.com>
>> > reviewed-by:   Benjamin Herrenschmidt <benh@au1.ibm.com>
>> > ---
>> >  qemu-config.c |    4 ++++
>> >  sysemu.h      |    1 +
>> >  vl.c          |   12 ++++++++++++
>> >  3 files changed, 17 insertions(+)
>> >
>> > diff --git a/qemu-config.c b/qemu-config.c
>> > index bb3bff4..258712a 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 */ }
>> >      },
>> > diff --git a/sysemu.h b/sysemu.h
>> > index bc2c788..c5ea10d 100644
>> > --- a/sysemu.h
>> > +++ b/sysemu.h
>> > @@ -13,6 +13,7 @@
>> >  /* vl.c */
>> >
>> >  extern const char *bios_name;
>> > +extern const char *usb_opt;
>> >
>> >  extern const char *qemu_name;
>> >  extern uint8_t qemu_uuid[];
>> > diff --git a/vl.c b/vl.c
>> > index 204d85b..10f8e4c 100644
>> > --- a/vl.c
>> > +++ b/vl.c
>> > @@ -171,6 +171,7 @@ int main(int argc, char **argv)
>> >
>> >  static const char *data_dir;
>> >  const char *bios_name = NULL;
>> > +const char *usb_opt = NULL;
>> >  enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
>> >  DisplayType display_type = DT_DEFAULT;
>> >  int display_remote = 0;
>>
>> I'd be very surprised if Anthony has actually reviewed this...
>
>
> Sorry, I just think I add him here because he gives some suggestions about
> adding usb option to machine options in the mailing list.:)
> Maybe it is not appropriate to add here. Sorry for that.
>>
>>
>> The point of using machine options is so that you can use the QemuOpts
>> infrastructure to inquire this value, not to save more global state.
>
> OK, I think it still needs one global value because other machines need to
> check whether usb is enabled.
>

Hi Li,

What should really happen here, is if you do change over to a machine
opt, then all clients of the -usb should use the machine opt
infrastructure, then the need for global state is eliminated
completely.

But does this new -usb cmd line option have any connection to the
existing -usb in this series? It looks like your trying to implement a
new option to add/remove your usb controller, in which case, I think
the best option is to do it using either configs or -device arguments.

Regards,
Peter
>>
>> Especially not a string when all you want is a boolean value.
>>
> From the qemu, QEMU_OPT_BOOL type  still use a string
> "on/off" to enable/disable usb.
> Maybe it's better to convert it to boolean value.
>
>> Further, in this patch it's only being assigned, not used anywhere.
>>
> In vl.c, it seems that it is not used.
> I have been thinking to use it in spapr.c.
>
> if (usb_opts && strcmp(usb_opts, "on") == 0)
>             pci_create_simple(bus, -1, "pci-ohci");
>
> But I want to see whether this way is accepted.
> If it is ok, I will add this option to spapr.c.
>

It seems like the purpose of this patch it to get comments? You should
include "RFC" in your patch description as it flags your patches and
series as a request for comments, rather than a merge proposal:

git format-patch --thread --cover-letter --subject-prefix "RFC PATCH v1" ...

>> Andreas
>>
>> > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt)
>> >      return 1;
>> >  }
>> >
>> > +static int default_enable_usb(QemuOpts *opts)
>> > +{
>> > +    if (NULL == qemu_opt_get(opts, "usb")) {
>> > +        qemu_opt_set(opts, "usb", "on");
>> > +    }
>> > +
>> > +    return 0;
>> > +}
>> > +
>> >  /***********************************************************/
>> >  /* QEMU Block devices */
>> >
>> > @@ -3356,6 +3366,8 @@ 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");
>> > +        default_enable_usb(machine_opts);
>> > +        usb_opt = qemu_opt_get(machine_opts, "usb");
>> >      } else {
>> >          kernel_filename = initrd_filename = kernel_cmdline = NULL;
>> >      }
>>
>>
>> --
>> 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] [PATCH 1/1] Add usb option in machine options.

[Benjamin Herrenschmidt]

On Fri, 2012-06-15 at 15:15 +1000, Peter Crosthwaite wrote:
> 
> What should really happen here, is if you do change over to a machine
> opt, then all clients of the -usb should use the machine opt
> infrastructure, then the need for global state is eliminated
> completely.
> 
> But does this new -usb cmd line option have any connection to the
> existing -usb in this series? It looks like your trying to implement a
> new option to add/remove your usb controller, in which case, I think
> the best option is to do it using either configs or -device arguments.

The purpose is to have options for the ppc machines such as pseries to
disable the default usb controller.

IE. Those machines (well some of them at least) have no other input
device (no PS/2, no ADB, ...) and so in order to provide a semi-decent
user interface, we want to create an OHCI with mouse & keyboard
emulation along with the vga display by default, and have options to
disable those defaults for cases like libvirt where it really wants to
create all that stuff itself so it gets the right names.

VGA is already handled by a generic "high level" option (by opposition
to using -device which I qualify of "low level") -vga which takes an
argument and that argument can be "none". However, the generic "high
level"  -usb option doesn't take an argument and is only meant to
enable, not disable (and thus cannot be augmented easily without
breaking things).

So after the discussion on the list, we decided that -machine options is
the right way to handle that. I haven't had a chance to review Li's
patch itself and whether it does that though. Just chiming in on what
the intention is.

Cheers,
Ben.

Re: [Qemu-devel] [PATCH 1/1] Add usb option in machine options.

[Li Zhang]

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

On Fri, Jun 15, 2012 at 1:15 PM, Peter Crosthwaite <
peter.crosthwaite@petalogix.com> wrote:

> On Fri, Jun 15, 2012 at 1:06 PM, Li Zhang <zhlcindy@gmail.com> wrote:
> >
> >
> > On Thu, Jun 14, 2012 at 10:27 PM, Andreas Färber <afaerber@suse.de>
> wrote:
> >>
> >> Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com:
> >> > From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> >> >
> >> > For pseries machine, it needs to enable usb
> >> > to add kbd 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_opt is an global option
> >> > which can be checked by machines. For example,
> >> > on pseries, it will check if usb_opt is on, if
> >> > it is on, it will create one usb ohci controller.
> >> > As the following:
> >> > if (usb_opts && strcmp(usb_opts, "on") == 0)
> >> >      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>
> >> > reviewed-by:   Anthony Liguori <aliguori@us.ibm.com>
> >> > reviewed-by:   Benjamin Herrenschmidt <benh@au1.ibm.com>
> >> > ---
> >> >  qemu-config.c |    4 ++++
> >> >  sysemu.h      |    1 +
> >> >  vl.c          |   12 ++++++++++++
> >> >  3 files changed, 17 insertions(+)
> >> >
> >> > diff --git a/qemu-config.c b/qemu-config.c
> >> > index bb3bff4..258712a 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 */ }
> >> >      },
> >> > diff --git a/sysemu.h b/sysemu.h
> >> > index bc2c788..c5ea10d 100644
> >> > --- a/sysemu.h
> >> > +++ b/sysemu.h
> >> > @@ -13,6 +13,7 @@
> >> >  /* vl.c */
> >> >
> >> >  extern const char *bios_name;
> >> > +extern const char *usb_opt;
> >> >
> >> >  extern const char *qemu_name;
> >> >  extern uint8_t qemu_uuid[];
> >> > diff --git a/vl.c b/vl.c
> >> > index 204d85b..10f8e4c 100644
> >> > --- a/vl.c
> >> > +++ b/vl.c
> >> > @@ -171,6 +171,7 @@ int main(int argc, char **argv)
> >> >
> >> >  static const char *data_dir;
> >> >  const char *bios_name = NULL;
> >> > +const char *usb_opt = NULL;
> >> >  enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
> >> >  DisplayType display_type = DT_DEFAULT;
> >> >  int display_remote = 0;
> >>
> >> I'd be very surprised if Anthony has actually reviewed this...
> >
> >
> > Sorry, I just think I add him here because he gives some suggestions
> about
> > adding usb option to machine options in the mailing list.:)
> > Maybe it is not appropriate to add here. Sorry for that.
> >>
> >>
> >> The point of using machine options is so that you can use the QemuOpts
> >> infrastructure to inquire this value, not to save more global state.
> >
> > OK, I think it still needs one global value because other machines need
> to
> > check whether usb is enabled.
> >
>
> Hi Li,
>
> What should really happen here, is if you do change over to a machine
> opt, then all clients of the -usb should use the machine opt
> infrastructure, then the need for global state is eliminated
> completely.
>
> Yes, right.

But does this new -usb cmd line option have any connection to the
> existing -usb in this series? It looks like your trying to implement a
> new option to add/remove your usb controller, in which case, I think
> the best option is to do it using either configs or -device arguments.
>
> No, it doesn't have any connection to -usb series.
We want to create usb controller by default.
For example,
#qemu-system-ppc64 -machine type=pseries without any -device arguments.

Regards,
> Peter
> >>
> >> Especially not a string when all you want is a boolean value.
> >>
> > From the qemu, QEMU_OPT_BOOL type  still use a string
> > "on/off" to enable/disable usb.
> > Maybe it's better to convert it to boolean value.
> >
> >> Further, in this patch it's only being assigned, not used anywhere.
> >>
> > In vl.c, it seems that it is not used.
> > I have been thinking to use it in spapr.c.
> >
> > if (usb_opts && strcmp(usb_opts, "on") == 0)
> >             pci_create_simple(bus, -1, "pci-ohci");
> >
> > But I want to see whether this way is accepted.
> > If it is ok, I will add this option to spapr.c.
> >
>
> It seems like the purpose of this patch it to get comments? You should
> include "RFC" in your patch description as it flags your patches and
> series as a request for comments, rather than a merge proposal:
>
> git format-patch --thread --cover-letter --subject-prefix "RFC PATCH v1" ..
>
Sorry, I think I should use RFC. Thanks. :)

>
> >> Andreas
> >>
> >> > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt)
> >> >      return 1;
> >> >  }
> >> >
> >> > +static int default_enable_usb(QemuOpts *opts)
> >> > +{
> >> > +    if (NULL == qemu_opt_get(opts, "usb")) {
> >> > +        qemu_opt_set(opts, "usb", "on");
> >> > +    }
> >> > +
> >> > +    return 0;
> >> > +}
> >> > +
> >> >  /***********************************************************/
> >> >  /* QEMU Block devices */
> >> >
> >> > @@ -3356,6 +3366,8 @@ 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");
> >> > +        default_enable_usb(machine_opts);
> >> > +        usb_opt = qemu_opt_get(machine_opts, "usb");
> >> >      } else {
> >> >          kernel_filename = initrd_filename = kernel_cmdline = NULL;
> >> >      }
> >>
> >>
> >> --
> >> 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
> >
>



-- 

Best Regards
-Li

[-- Attachment #2: Type: text/html, Size: 9469 bytes --]

Re: [Qemu-devel] [PATCH 1/1] Add usb option in machine options.

[Andreas Färber]

Am 15.06.2012 05:06, schrieb Li Zhang:
> 
> 
> On Thu, Jun 14, 2012 at 10:27 PM, Andreas Färber <afaerber@suse.de
> <mailto:afaerber@suse.de>> wrote:
> 
>     Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com
>     <mailto:zhlcindy@gmail.com>:
>     > From: Li Zhang <zhlcindy@linux.vnet.ibm.com
>     <mailto:zhlcindy@linux.vnet.ibm.com>>
>     >
>     > For pseries machine, it needs to enable usb
>     > to add kbd 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_opt is an global option
>     > which can be checked by machines. For example,
>     > on pseries, it will check if usb_opt is on, if
>     > it is on, it will create one usb ohci controller.
>     > As the following:
>     > if (usb_opts && strcmp(usb_opts, "on") == 0)
>     >      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
>     <mailto:zhlcindy@linux.vnet.ibm.com>>
>     > reviewed-by:   Anthony Liguori <aliguori@us.ibm.com
>     <mailto:aliguori@us.ibm.com>>
>     > reviewed-by:   Benjamin Herrenschmidt <benh@au1.ibm.com
>     <mailto:benh@au1.ibm.com>>
>     > ---
>     >  qemu-config.c |    4 ++++
>     >  sysemu.h      |    1 +
>     >  vl.c          |   12 ++++++++++++
>     >  3 files changed, 17 insertions(+)
>     >
>     > diff --git a/qemu-config.c b/qemu-config.c
>     > index bb3bff4..258712a 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 */ }
>     >      },
>     > diff --git a/sysemu.h b/sysemu.h
>     > index bc2c788..c5ea10d 100644
>     > --- a/sysemu.h
>     > +++ b/sysemu.h
>     > @@ -13,6 +13,7 @@
>     >  /* vl.c */
>     >
>     >  extern const char *bios_name;
>     > +extern const char *usb_opt;
>     >
>     >  extern const char *qemu_name;
>     >  extern uint8_t qemu_uuid[];
>     > diff --git a/vl.c b/vl.c
>     > index 204d85b..10f8e4c 100644
>     > --- a/vl.c
>     > +++ b/vl.c
>     > @@ -171,6 +171,7 @@ int main(int argc, char **argv)
>     >
>     >  static const char *data_dir;
>     >  const char *bios_name = NULL;
>     > +const char *usb_opt = NULL;
>     >  enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
>     >  DisplayType display_type = DT_DEFAULT;
>     >  int display_remote = 0;

[...]

>     The point of using machine options is so that you can use the QemuOpts
>     infrastructure to inquire this value, not to save more global state.
> 
> OK, I think it still needs one global value because other machines need
> to check whether usb is enabled.
>  
> 
>     Especially not a string when all you want is a boolean value.
> 
> From the qemu, QEMU_OPT_BOOL type  still use a string
> "on/off" to enable/disable usb.
> Maybe it's better to convert it to boolean value. 
> 
>     Further, in this patch it's only being assigned, not used anywhere.
> 
> In vl.c, it seems that it is not used.  
> I have been thinking to use it in spapr.c. 
> 
> if (usb_opts && strcmp(usb_opts, "on") == 0)
>             pci_create_simple(bus, -1, "pci-ohci");
> 
> But I want to see whether this way is accepted. 
> If it is ok, I will add this option to spapr.c.  

First, please don't reply with HTML mails, it breaks the quoting as you
can see.

You don't seem to be getting what I'm saying: As discussed in great
lengths for how to access a passed-in device tree for ARM, the code
checking this option (i.e., in spapr.c) is expected to call
qemu_opt_get() itself, instead of saving its value globally in vl.c. For
example:

machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
usb_opt = qemu_opt_get(machine_opts, "usb");
if (strcmp(usb_opt, "off") != 0) {
    // set up OHCI
}

with appropriate NULL checks left as exercise for the reader.

So you should combine the definition of the option in qemu-config.c
(which looks okay) with its actual use in hw/spapr.c in one patch.

Once introduced, hw/ppc_newworld.c should be updated to check the new
option as well, but that can be a follow-up patch.

Regards,
Andreas

>     > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt)
>     >      return 1;
>     >  }
>     >
>     > +static int default_enable_usb(QemuOpts *opts)
>     > +{
>     > +    if (NULL == qemu_opt_get(opts, "usb")) {
>     > +        qemu_opt_set(opts, "usb", "on");
>     > +    }
>     > +
>     > +    return 0;
>     > +}
>     > +
>     >  /***********************************************************/
>     >  /* QEMU Block devices */
>     >
>     > @@ -3356,6 +3366,8 @@ 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");
>     > +        default_enable_usb(machine_opts);
>     > +        usb_opt = qemu_opt_get(machine_opts, "usb");
>     >      } else {
>     >          kernel_filename = initrd_filename = kernel_cmdline = NULL;
>     >      }

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

Re: [Qemu-devel] [PATCH 1/1] Add usb option in machine options.

[Li Zhang]

On Fri, Jun 15, 2012 at 7:09 PM, Andreas Färber <afaerber@suse.de> wrote:
>
> Am 15.06.2012 05:06, schrieb Li Zhang:
> >
> >
> > On Thu, Jun 14, 2012 at 10:27 PM, Andreas Färber <afaerber@suse.de
> > <mailto:afaerber@suse.de>> wrote:
> >
> >     Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com
> >     <mailto:zhlcindy@gmail.com>:
> >     > From: Li Zhang <zhlcindy@linux.vnet.ibm.com
> >     <mailto:zhlcindy@linux.vnet.ibm.com>>
> >     >
> >     > For pseries machine, it needs to enable usb
> >     > to add kbd 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_opt is an global option
> >     > which can be checked by machines. For example,
> >     > on pseries, it will check if usb_opt is on, if
> >     > it is on, it will create one usb ohci controller.
> >     > As the following:
> >     > if (usb_opts && strcmp(usb_opts, "on") == 0)
> >     >      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
> >     <mailto:zhlcindy@linux.vnet.ibm.com>>
> >     > reviewed-by:   Anthony Liguori <aliguori@us.ibm.com
> >     <mailto:aliguori@us.ibm.com>>
> >     > reviewed-by:   Benjamin Herrenschmidt <benh@au1.ibm.com
> >     <mailto:benh@au1.ibm.com>>
> >     > ---
> >     >  qemu-config.c |    4 ++++
> >     >  sysemu.h      |    1 +
> >     >  vl.c          |   12 ++++++++++++
> >     >  3 files changed, 17 insertions(+)
> >     >
> >     > diff --git a/qemu-config.c b/qemu-config.c
> >     > index bb3bff4..258712a 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 */ }
> >     >      },
> >     > diff --git a/sysemu.h b/sysemu.h
> >     > index bc2c788..c5ea10d 100644
> >     > --- a/sysemu.h
> >     > +++ b/sysemu.h
> >     > @@ -13,6 +13,7 @@
> >     >  /* vl.c */
> >     >
> >     >  extern const char *bios_name;
> >     > +extern const char *usb_opt;
> >     >
> >     >  extern const char *qemu_name;
> >     >  extern uint8_t qemu_uuid[];
> >     > diff --git a/vl.c b/vl.c
> >     > index 204d85b..10f8e4c 100644
> >     > --- a/vl.c
> >     > +++ b/vl.c
> >     > @@ -171,6 +171,7 @@ int main(int argc, char **argv)
> >     >
> >     >  static const char *data_dir;
> >     >  const char *bios_name = NULL;
> >     > +const char *usb_opt = NULL;
> >     >  enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
> >     >  DisplayType display_type = DT_DEFAULT;
> >     >  int display_remote = 0;
>
> [...]
>
> >     The point of using machine options is so that you can use the
> > QemuOpts
> >     infrastructure to inquire this value, not to save more global state.
> >
> > OK, I think it still needs one global value because other machines need
> > to check whether usb is enabled.
> >
> >
> >     Especially not a string when all you want is a boolean value.
> >
> > From the qemu, QEMU_OPT_BOOL type  still use a string
> > "on/off" to enable/disable usb.
> > Maybe it's better to convert it to boolean value.
> >
> >     Further, in this patch it's only being assigned, not used anywhere.
> >
> > In vl.c, it seems that it is not used.
> > I have been thinking to use it in spapr.c.
> >
> > if (usb_opts && strcmp(usb_opts, "on") == 0)
> >             pci_create_simple(bus, -1, "pci-ohci");
> >
> > But I want to see whether this way is accepted.
> > If it is ok, I will add this option to spapr.c.
>
> First, please don't reply with HTML mails, it breaks the quoting as you
> can see.

Thanks for your reminding. :)

> You don't seem to be getting what I'm saying: As discussed in great
> lengths for how to access a passed-in device tree for ARM, the code
> checking this option (i.e., in spapr.c) is expected to call
> qemu_opt_get() itself, instead of saving its value globally in vl.c. For
> example:
>
> machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
> usb_opt = qemu_opt_get(machine_opts, "usb");
> if (strcmp(usb_opt, "off") != 0) {
>    // set up OHCI
> }
>
I see.
I  just thought if every machine has the same code, it can be done in vl.c.
Isn't  the global variable good ?

> with appropriate NULL checks left as exercise for the reader.
>
> So you should combine the definition of the option in qemu-config.c
> (which looks okay) with its actual use in hw/spapr.c in one patch.
>
> Once introduced, hw/ppc_newworld.c should be updated to check the new
> option as well, but that can be a follow-up patch.
>
> Regards,
> Andreas
>
> >     > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt)
> >     >      return 1;
> >     >  }
> >     >
> >     > +static int default_enable_usb(QemuOpts *opts)
> >     > +{
> >     > +    if (NULL == qemu_opt_get(opts, "usb")) {
> >     > +        qemu_opt_set(opts, "usb", "on");
> >     > +    }
> >     > +
> >     > +    return 0;
> >     > +}
> >     > +
> >     >  /***********************************************************/
> >     >  /* QEMU Block devices */
> >     >
> >     > @@ -3356,6 +3366,8 @@ 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");
> >     > +        default_enable_usb(machine_opts);
> >     > +        usb_opt = qemu_opt_get(machine_opts, "usb");
> >     >      } else {
> >     >          kernel_filename = initrd_filename = kernel_cmdline =
> > NULL;
> >     >      }
>
> --
> 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