From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37145) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SfbIe-00076Y-Mg for qemu-devel@nongnu.org; Fri, 15 Jun 2012 14:31:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SfbIc-0002Ig-NA for qemu-devel@nongnu.org; Fri, 15 Jun 2012 14:31:12 -0400 From: Markus Armbruster References: <1339754783-14341-1-git-send-email-zhlcindy@linux.vnet.ibm.com> Date: Fri, 15 Jun 2012 16:34:01 +0200 In-Reply-To: (Li Zhang's message of "Fri, 15 Jun 2012 21:08:56 +0800") Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCHv2 1/1] Add usb option in machine options to enable/disable usb 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-ppc@nongnu.org, qemu-devel@nongnu.org, Li Zhang Li Zhang writes: > On Fri, Jun 15, 2012 at 8:04 PM, Markus Armbruster wr= ote: >> Li Zhang writes: >> >>> For pseries machine, it needs to enable usb to add >>> keyboard 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_on is an global option which can >>> be checked by machines. >>> For example, on pseries, it will check if usb_on is 1, >>> if it is 1, it will create one usb ohci controller. >>> As the following: >>> if (usb_on =3D=3D 1) { >>> =C2=A0 =C2=A0 =C2=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: >>> =C2=A0-machine type=3Dpseries,usb=3Doff. >>> >>> Signed-off-by: Li Zhang >>> >>> --- >>> =C2=A0hw/spapr.c | =C2=A0 =C2=A05 +++++ >>> =C2=A0sysemu.h =C2=A0 | =C2=A0 =C2=A01 + >>> =C2=A0vl.c =C2=A0 =C2=A0 =C2=A0 | =C2=A0 17 +++++++++++++++++ >>> =C2=A03 files changed, 23 insertions(+) >>> >>> diff --git a/hw/spapr.c b/hw/spapr.c >>> index d0bddbc..1feb739 100644 >>> --- a/hw/spapr.c >>> +++ b/hw/spapr.c >>> @@ -661,6 +661,11 @@ static void ppc_spapr_init(ram_addr_t ram_size, >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0spapr_vscsi_create(spapr->vio_bus); >>> =C2=A0 =C2=A0 =C2=A0} >>> >>> + =C2=A0 =C2=A0if (usb_on =3D=3D 1) { >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0pci_create_simple(QLIST_FIRST(&spapr->phbs= )->host_state.bus, >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0-1, "pci-ohci"); >>> + =C2=A0 =C2=A0} >>> + >>> =C2=A0 =C2=A0 =C2=A0if (rma_size < (MIN_RMA_SLOF << 20)) { >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fprintf(stderr, "qemu: pSeries SLOF f= irmware requires >=3D " >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"%ldM gue= st RMA (Real Mode Area memory)\n", MIN_RMA_SLOF); >>> diff --git a/sysemu.h b/sysemu.h >>> index bc2c788..08134ae 100644 >>> --- a/sysemu.h >>> +++ b/sysemu.h >>> @@ -109,6 +109,7 @@ extern int vga_interface_type; >>> =C2=A0#define vmsvga_enabled (vga_interface_type =3D=3D VGA_VMWARE) >>> =C2=A0#define qxl_enabled (vga_interface_type =3D=3D VGA_QXL) >>> >>> +extern int usb_on; >>> =C2=A0extern int graphic_width; >>> =C2=A0extern int graphic_height; >>> =C2=A0extern int graphic_depth; >>> diff --git a/vl.c b/vl.c >>> index 204d85b..b200203 100644 >>> --- a/vl.c >>> +++ b/vl.c >>> @@ -202,6 +202,7 @@ int smp_cpus =3D 1; >>> =C2=A0int max_cpus =3D 0; >>> =C2=A0int smp_cores =3D 1; >>> =C2=A0int smp_threads =3D 1; >>> +int usb_on =3D 0; >>> =C2=A0#ifdef CONFIG_VNC >>> =C2=A0const char *vnc_display; >>> =C2=A0#endif >>> @@ -758,6 +759,21 @@ static int bt_parse(const char *opt) >>> =C2=A0 =C2=A0 =C2=A0return 1; >>> =C2=A0} >>> >>> +static int get_usb_opt(QemuOpts *opts) >>> +{ >>> + =C2=A0 =C2=A0const char *usb_opt =3D NULL; >> >> Useless initializer. > Thanks. I will remove it. >> >>> + =C2=A0 =C2=A0int usb_on =3D 0; >>> + >>> + =C2=A0 =C2=A0if (NULL =3D=3D qemu_opt_get(opts, "usb")) >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0qemu_opt_set(opts, "usb", "on"); >> >> Why are you changing opts? > USB is enabled by default when there is no usb option setting. > For example, > using # qemu-system-ppc64 -machine type=3Dpseries > There is no usb option, but usb is set on. Isn't it off by default for at least some machines now? Anyway, I don't see why we need to update opts. Who's using the updated opts? >>> + >>> + =C2=A0 =C2=A0usb_opt =3D qemu_opt_get(opts, "usb"); >>> + =C2=A0 =C2=A0if (usb_opt && strcmp(usb_opt, "on") =3D=3D 0) >> >> Please don't duplicate parsing of bools; use qemu_opt_get_bool(). > OK. >> >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0usb_on =3D 1; >>> + >>> + =C2=A0 =C2=A0return usb_on; >>> +} >>> + >>> =C2=A0/***********************************************************/ >>> =C2=A0/* QEMU Block devices */ >>> >>> @@ -3356,6 +3372,7 @@ int main(int argc, char **argv, char **envp) >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0kernel_filename =3D qemu_opt_get(mach= ine_opts, "kernel"); >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0initrd_filename =3D qemu_opt_get(mach= ine_opts, "initrd"); >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0kernel_cmdline =3D qemu_opt_get(machi= ne_opts, "append"); >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0usb_on =3D get_usb_opt(machine_opts); >> >> Anything wrong with >> >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 usb_on =3D qemu_opt_get_bool(machine_opts, "= usb", 0); >> >> ? >> >>> =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} >> >> Your new global usb_on is still unused, and it's not integrated with >> -usb. =C2=A0I doubt it can be merged that way. >> > It is used in spapr.c. In the front of this patch, it is used. > It seems that this is not good. > Maybe it is better to move the options setting to spapr.c. Dang, I missed that. Sorry for the noise.