From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37726) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SfMN5-0002ed-Iu for qemu-devel@nongnu.org; Thu, 14 Jun 2012 22:34:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SfMN3-0000ia-Ai for qemu-devel@nongnu.org; Thu, 14 Jun 2012 22:34:47 -0400 MIME-Version: 1.0 In-Reply-To: <1339711491.9220.168.camel@pasglop> References: <1339651039-15290-1-git-send-email-zhlcindy@gmail.com> <4FD9F4C3.6080401@suse.de> <1339711491.9220.168.camel@pasglop> Date: Fri, 15 Jun 2012 10:34:37 +0800 Message-ID: From: Li Zhang Content-Type: multipart/alternative; boundary=0016e6dbe9325468c404c279a8b3 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: Benjamin Herrenschmidt Cc: aliguori@us.ibm.com, Alexander Graf , zhlcindy@linux.vnet.ibm.com, qemu-devel@nongnu.org, qemu-ppc , =?ISO-8859-1?Q?Andreas_F=E4rber?= --0016e6dbe9325468c404c279a8b3 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable OK,sorry for my fault. best regards Li Zhang =E5=9C=A8 2012-6-15 =E4=B8=8A=E5=8D=886:05=EF=BC=8C"Benjamin Herrenschmidt"= =E5=86=99=E9=81=93=EF=BC=9A > > On Thu, 2012-06-14 at 16:27 +0200, Andreas F=C3=A4rber 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 > > I have not reviewed that patch. Li, you must -never- make up such > reviewed-by: lines ! You put them in if and only if you've had them > given to you by the reviewer who has actually reviewed the patch in > details. > > Ben. > > > > --- > > > 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... > > > > 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. > > Especially not a string when all you want is a boolean value. > > > > Further, in this patch it's only being assigned, not used anywhere. > > > > 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 N= ULL; > > > } > > > > > > --0016e6dbe9325468c404c279a8b3 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

OK,sorry for my fault.

best regards

Li Zhang
=E5=9C=A8 2012-6-15 =E4=B8=8A=E5=8D=886:05=EF=BC=8C"Benjamin Herrensch= midt" <benh@au1.ibm.com>= =E5=86=99=E9=81=93=EF=BC=9A
>
> On Thu, 2012-06-14 at 16:27 +0200, Andreas F=C3=A4rber 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)
> > > =C2=A0 =C2=A0 =C2=A0pci_create_simple(bus, -1, "pci-ohc= i");
> > >
> > > In this patch, usb is on by default.
> > > So, for -nodefault, usb should be set off in the
> > > command line as the following:
> > > =C2=A0-machine type=3Dpseries,usb=3Doff.
> > >
> > > Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> > > reviewed-by: =C2=A0 Anthony Liguori <aliguori@us.ibm.com>
> > > reviewed-by: =C2=A0 Benjamin Herrenschmidt <benh@au1.ibm.com>
>
> I have not reviewed that patch. Li, you must -never- make up such
> reviewed-by: lines ! You put them in if and only if you've had the= m
> given to you by the reviewer who has actually reviewed the patch in > details.
>
> Ben.
>
> > > ---
> > > =C2=A0qemu-config.c | =C2=A0 =C2=A04 ++++
> > > =C2=A0sysemu.h =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A01 +
> > > =C2=A0vl.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 12 +++= +++++++++
> > > =C2=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 {
> > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.name =3D &q= uot;dtb",
> > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.type =3D QE= MU_OPT_STRING,
> > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.help =3D &q= uot;Linux kernel device tree file",
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0}, {
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.name =3D "u= sb",
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.type =3D QEMU_OP= T_BOOL,
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.help =3D "S= et on/off to enable/disable usb",
> > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0},
> > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{ /* End of list */ }
> > > =C2=A0 =C2=A0 =C2=A0},
> > > diff --git a/sysemu.h b/sysemu.h
> > > index bc2c788..c5ea10d 100644
> > > --- a/sysemu.h
> > > +++ b/sysemu.h
> > > @@ -13,6 +13,7 @@
> > > =C2=A0/* vl.c */
> > >
> > > =C2=A0extern const char *bios_name;
> > > +extern const char *usb_opt;
> > >
> > > =C2=A0extern const char *qemu_name;
> > > =C2=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)
> > >
> > > =C2=A0static const char *data_dir;
> > > =C2=A0const char *bios_name =3D NULL;
> > > +const char *usb_opt =3D NULL;
> > > =C2=A0enum vga_retrace_method vga_retrace_method =3D VGA_RET= RACE_DUMB;
> > > =C2=A0DisplayType display_type =3D DT_DEFAULT;
> > > =C2=A0int display_remote =3D 0;
> >
> > I'd be very surprised if Anthony has actually reviewed this..= .
> >
> > The point of using machine options is so that you can use the Qem= uOpts
> > infrastructure to inquire this value, not to save more global sta= te.
> > Especially not a string when all you want is a boolean value.
> >
> > Further, in this patch it's only being assigned, not used any= where.
> >
> > Andreas
> >
> > > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt) > > > =C2=A0 =C2=A0 =C2=A0return 1;
> > > =C2=A0}
> > >
> > > +static int default_enable_usb(QemuOpts *opts)
> > > +{
> > > + =C2=A0 =C2=A0if (NULL =3D=3D qemu_opt_get(opts, "usb&= quot;)) {
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0qemu_opt_set(opts, "usb&qu= ot;, "on");
> > > + =C2=A0 =C2=A0}
> > > +
> > > + =C2=A0 =C2=A0return 0;
> > > +}
> > > +
> > > =C2=A0/*****************************************************= ******/
> > > =C2=A0/* QEMU Block devices */
> > >
> > > @@ -3356,6 +3366,8 @@ int main(int argc, char **argv, char *= *envp)
> > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0kernel_filename =3D qemu_o= pt_get(machine_opts, "kernel");
> > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0initrd_filename =3D qemu_o= pt_get(machine_opts, "initrd");
> > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0kernel_cmdline =3D qemu_op= t_get(machine_opts, "append");
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0default_enable_usb(machine_opts= );
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0usb_opt =3D qemu_opt_get(machin= e_opts, "usb");
> > > =C2=A0 =C2=A0 =C2=A0} else {
> > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0kernel_filename =3D initrd= _filename =3D kernel_cmdline =3D NULL;
> > > =C2=A0 =C2=A0 =C2=A0}
> >
> >
>
>

--0016e6dbe9325468c404c279a8b3--