On Fri, Jun 15, 2012 at 1:15 PM, Peter Crosthwaite <
peter.crosthwa...@petalogix.com> wrote:
> On Fri, Jun 15, 2012 at 1:06 PM, Li Zhang <zhlci...@gmail.com> wrote:
> >
> >
> > On Thu, Jun 14, 2012 at 10:27 PM, Andreas Färber <afaer...@suse.de>
> wrote:
> >>
> >> Am 14.06.2012 07:17, schrieb zhlci...@gmail.com:
> >> > From: Li Zhang <zhlci...@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 <zhlci...@linux.vnet.ibm.com>
> >> > reviewed-by: Anthony Liguori <aligu...@us.ibm.com>
> >> > reviewed-by: Benjamin Herrenschmidt <b...@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
