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 <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.

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