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 <address@hidden> wrote:
>
> Am 15.06.2012 05:06, schrieb Li Zhang:
> >
> >
> > On Thu, Jun 14, 2012 at 10:27 PM, Andreas Färber <address@hidden
> > <mailto:address@hidden>> wrote:
> >
> >     Am 14.06.2012 07:17, schrieb address@hidden
> >     <mailto:address@hidden>:
> >     > From: Li Zhang <address@hidden
> >     <mailto:address@hidden>>
> >     >
> >     > 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 <address@hidden
> >     <mailto:address@hidden>>
> >     > reviewed-by:   Anthony Liguori <address@hidden
> >     <mailto:address@hidden>>
> >     > reviewed-by:   Benjamin Herrenschmidt <address@hidden
> >     <mailto:address@hidden>>
> >     > ---
> >     >  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