From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47867) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SfMrX-00061Q-TT for qemu-devel@nongnu.org; Thu, 14 Jun 2012 23:06:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SfMrU-0007zX-Hd for qemu-devel@nongnu.org; Thu, 14 Jun 2012 23:06:15 -0400 MIME-Version: 1.0 In-Reply-To: <4FD9F4C3.6080401@suse.de> References: <1339651039-15290-1-git-send-email-zhlcindy@gmail.com> <4FD9F4C3.6080401@suse.de> Date: Fri, 15 Jun 2012 11:06:09 +0800 Message-ID: From: Li Zhang Content-Type: multipart/alternative; boundary=f46d04426f9c13154804c27a1986 Subject: Re: [Qemu-devel] [PATCH 1/1] Add usb option in machine options. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-1?Q?Andreas_F=E4rber?= Cc: aliguori@us.ibm.com, benh@au1.ibm.com, qemu-devel@nongnu.org, zhlcindy@linux.vnet.ibm.com, Alexander Graf , qemu-ppc --f46d04426f9c13154804c27a1986 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On Thu, Jun 14, 2012 at 10:27 PM, Andreas F=E4rber wrote= : > Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com: > > From: Li Zhang > > > > 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=3Dpsereis,usb=3Don/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") =3D=3D 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=3Dpseries,usb=3Doff. > > > > Signed-off-by: Li Zhang > > reviewed-by: Anthony Liguori > > reviewed-by: Benjamin Herrenschmidt > > --- > > 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 =3D { > > .name =3D "dtb", > > .type =3D QEMU_OPT_STRING, > > .help =3D "Linux kernel device tree file", > > + }, { > > + .name =3D "usb", > > + .type =3D QEMU_OPT_BOOL, > > + .help =3D "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 =3D NULL; > > +const char *usb_opt =3D NULL; > > enum vga_retrace_method vga_retrace_method =3D VGA_RETRACE_DUMB; > > DisplayType display_type =3D DT_DEFAULT; > > int display_remote =3D 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. > 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") =3D=3D 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. Andreas > > > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt) > > return 1; > > } > > > > +static int default_enable_usb(QemuOpts *opts) > > +{ > > + if (NULL =3D=3D 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 =3D qemu_opt_get(machine_opts, "kernel"); > > initrd_filename =3D qemu_opt_get(machine_opts, "initrd"); > > kernel_cmdline =3D qemu_opt_get(machine_opts, "append"); > > + default_enable_usb(machine_opts); > > + usb_opt =3D qemu_opt_get(machine_opts, "usb"); > > } else { > > kernel_filename =3D initrd_filename =3D kernel_cmdline =3D NUL= L; > > } > > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg > --=20 Best Regards -Li --f46d04426f9c13154804c27a1986 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

On Thu, Jun 14, 2012 at 10:27 PM, Andrea= s F=E4rber <afaerber@suse.de> wrote:
Am 14.06.2012 07:17, schrieb zhlcindy= @gmail.com:
> From: Li Zhang <zhlcindy@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=3Dpsereis,usb=3Don/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") =3D=3D 0)
> =A0 =A0 =A0pci_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:
> =A0-machine type=3Dpseries,usb=3Doff.
>
> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> reviewed-by: =A0 Anthony Liguori <aliguori@us.ibm.com>
> reviewed-by: =A0 Benjamin Herrenschmidt <benh@au1.ibm.com>
> ---
> =A0qemu-config.c | =A0 =A04 ++++
> =A0sysemu.h =A0 =A0 =A0| =A0 =A01 +
> =A0vl.c =A0 =A0 =A0 =A0 =A0| =A0 12 ++++++++++++
> =A03 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 =3D {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0.name =3D "dtb",
> =A0 =A0 =A0 =A0 =A0 =A0 =A0.type =3D QEMU_OPT_STRING,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0.help =3D "Linux kernel device tree fi= le",
> + =A0 =A0 =A0 =A0}, {
> + =A0 =A0 =A0 =A0 =A0 =A0.name =3D "usb",
> + =A0 =A0 =A0 =A0 =A0 =A0.type =3D QEMU_OPT_BOOL,
> + =A0 =A0 =A0 =A0 =A0 =A0.help =3D "Set on/off to enable/disable = usb",
> =A0 =A0 =A0 =A0 =A0},
> =A0 =A0 =A0 =A0 =A0{ /* End of list */ }
> =A0 =A0 =A0},
> diff --git a/sysemu.h b/sysemu.h
> index bc2c788..c5ea10d 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -13,6 +13,7 @@
> =A0/* vl.c */
>
> =A0extern const char *bios_name;
> +extern const char *usb_opt;
>
> =A0extern const char *qemu_name;
> =A0extern 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)
>
> =A0static const char *data_dir;
> =A0const char *bios_name =3D NULL;
> +const char *usb_opt =3D NULL;
> =A0enum vga_retrace_method vga_retrace_method =3D VGA_RETRACE_DUMB; > =A0DisplayType display_type =3D DT_DEFAULT;
> =A0int display_remote =3D 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 option= s in the mailing list.:)
Maybe it is not appropriate to add here. Sorry for that.=A0

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 ma= chines need to check whether usb is enabled.
=A0
Especially not a string when all you want is a boolean value.

From the qemu,=A0QEMU_OPT_BOOL type =A0still use a st= ring "on/off"=A0to=A0enable/disable usb.
Maybe it's= better to convert it to boolean value.=A0

Further, in this patch it's only being assigned, not used anywhere.

In vl.c, it seems that it is not used. =A0
= I have been thinking to use it in spapr.c.=A0

if (usb_opts && strcmp(usb_opts, "on") =3D=3D 0)
=A0 =A0 =A0 =A0 =A0 =A0 pci_create_simple(bus, -1, "pci-ohci");

But I want to see whether this way is accepte= d.=A0
If it is ok, I will add this option to spapr.c.=A0=A0
=

Andreas

> @@ -758,6 +759,15 @@ static int bt_parse(const char *opt)
> =A0 =A0 =A0return 1;
> =A0}
>
> +static int default_enable_usb(QemuOpts *opts)
> +{
> + =A0 =A0if (NULL =3D=3D qemu_opt_get(opts, "usb")) {
> + =A0 =A0 =A0 =A0qemu_opt_set(opts, "usb", "on");<= br> > + =A0 =A0}
> +
> + =A0 =A0return 0;
> +}
> +
> =A0/***********************************************************/
> =A0/* QEMU Block devices */
>
> @@ -3356,6 +3366,8 @@ int main(int argc, char **argv, char **envp)
> =A0 =A0 =A0 =A0 =A0kernel_filename =3D qemu_opt_get(machine_opts, &quo= t;kernel");
> =A0 =A0 =A0 =A0 =A0initrd_filename =3D qemu_opt_get(machine_opts, &quo= t;initrd");
> =A0 =A0 =A0 =A0 =A0kernel_cmdline =3D qemu_opt_get(machine_opts, "= ;append");
> + =A0 =A0 =A0 =A0default_enable_usb(machine_opts);
> + =A0 =A0 =A0 =A0usb_opt =3D qemu_opt_get(machine_opts, "usb"= ;);
> =A0 =A0 =A0} else {
> =A0 =A0 =A0 =A0 =A0kernel_filename =3D initrd_filename =3D kernel_cmdl= ine =3D NULL;
> =A0 =A0 =A0}


--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnberg=



--

Best Regards
-Li

--f46d04426f9c13154804c27a1986--