On Fri, Jun 15, 2012 at 7:09 PM, Andreas Färber <afaer...@suse.de> wrote:
>
> Am 15.06.2012 05:06, schrieb Li Zhang:
> >
> >
> > On Thu, Jun 14, 2012 at 10:27 PM, Andreas Färber <afaer...@suse.de
> > <mailto:afaer...@suse.de>> wrote:
> >
> > Am 14.06.2012 07:17, schrieb zhlci...@gmail.com
> > <mailto:zhlci...@gmail.com>:
> > > From: Li Zhang <zhlci...@linux.vnet.ibm.com
> > <mailto: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
> > <mailto:zhlci...@linux.vnet.ibm.com>>
> > > reviewed-by: Anthony Liguori <aligu...@us.ibm.com
> > <mailto:aligu...@us.ibm.com>>
> > > reviewed-by: Benjamin Herrenschmidt <b...@au1.ibm.com
> > <mailto: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;
>
> [...]
>
> > 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
