From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55391) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SfUPg-0001Jw-Fl for qemu-devel@nongnu.org; Fri, 15 Jun 2012 07:10:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SfUPY-0003oH-O0 for qemu-devel@nongnu.org; Fri, 15 Jun 2012 07:10:00 -0400 Message-ID: <4FDB17FA.10702@suse.de> Date: Fri, 15 Jun 2012 13:09:46 +0200 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1339651039-15290-1-git-send-email-zhlcindy@gmail.com> <4FD9F4C3.6080401@suse.de> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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: Li Zhang Cc: aliguori@us.ibm.com, benh@au1.ibm.com, qemu-devel@nongnu.org, zhlcindy@linux.vnet.ibm.com, Alexander Graf , qemu-ppc Am 15.06.2012 05:06, schrieb Li Zhang: >=20 >=20 > On Thu, Jun 14, 2012 at 10:27 PM, Andreas F=E4rber > wrote: >=20 > 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; [...] > The point of using machine options is so that you can use the QemuO= pts > infrastructure to inquire this value, not to save more global state= . >=20 > OK, I think it still needs one global value because other machines need > to check whether usb is enabled. > =20 >=20 > Especially not a string when all you want is a boolean value. >=20 > 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.=20 >=20 > Further, in this patch it's only being assigned, not used anywhere. >=20 > In vl.c, it seems that it is not used. =20 > I have been thinking to use it in spapr.c.=20 >=20 > if (usb_opts && strcmp(usb_opts, "on") =3D=3D 0) > pci_create_simple(bus, -1, "pci-ohci"); >=20 > But I want to see whether this way is accepted.=20 > If it is ok, I will add this option to spapr.c. =20 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 =3D qemu_opts_find(qemu_find_opts("machine"), 0); usb_opt =3D qemu_opt_get(machine_opts, "usb"); if (strcmp(usb_opt, "off") !=3D 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 =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= NULL; > > } --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg