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

[Peter Crosthwaite]

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