On Wed, Jun 27, 2012 at 11:13 PM, Li Zhang <zhlci...@gmail.com> wrote: > On Wed, Jun 27, 2012 at 8:00 PM, Andreas Färber <afaer...@suse.de> wrote: >> Am 18.06.2012 11:22, schrieb Li Zhang: >>> For pseries machine, it needs to enable usb to add >>> keyboard or usb mouse. -usb option won't be used in
Grammar: The pseries machine needs to enable USB to add a keyboard or USB mouse. The -usb option ... >>> the future, and machine options is a better way to "are" a better way >>> enable usb. >>> >>> So this patch is to add usb option to machine options So this patch adds a USB option ... >>> (-machine type=psereis,usb=on/off)to enable/disable >> >> "pseries" >> >> Space after ) please, Western fonts are probably more narrow. ;) >> > OK, I will correct it. >>> usb controller. >>> >>> For specific machines, they will get the machine option >>> and then create usb controller according to usb option. >>> >>> In this patch, usb is on by default on pseries. >>> So, for -nodefault,usb should be set off in the command >> >> Space before "usb" please. >> >>> line as the following: >>> -machine type=pseries,usb=off. >>> >>> Signed-off-by: Li Zhang <zhlci...@linux.vnet.ibm.com> >>> --- >>> hw/spapr.c | 10 ++++++++++ >>> qemu-config.c | 4 ++++ >>> 2 files changed, 14 insertions(+), 0 deletions(-) >>> >>> diff --git a/hw/spapr.c b/hw/spapr.c >>> index d0bddbc..8d158d7 100644 >>> --- a/hw/spapr.c >>> +++ b/hw/spapr.c >>> @@ -529,6 +529,8 @@ static void ppc_spapr_init(ram_addr_t ram_size, >>> long load_limit, rtas_limit, fw_size; >>> long pteg_shift = 17; >>> char *filename; >>> + QemuOpts * machine_opts; >> >> QemuOpts *machine_opts; >> >> checkpatch.pl sometimes emits bogus warnings about this. >> > OK. I will use it to do that. :) > >>> + bool usb_on = false; >> >> Didn't you want this to be true for sPAPR in absence of -machine? >> > It is set in the following: > > if (machine_opts) > usb_on = qemu_opt_get_bool(machine_opts, "usb", true); > > It means that when using "-machine" option, usb_on is set as true if > usb option is not specified. > >> Maybe use a more functional naming? It's either added or not, so add_usb >> perhaps or provide_usb? Purely stylistic of course. >> >>> >>> spapr = g_malloc0(sizeof(*spapr)); >>> QLIST_INIT(&spapr->phbs); >>> @@ -661,6 +663,14 @@ static void ppc_spapr_init(ram_addr_t ram_size, >>> spapr_vscsi_create(spapr->vio_bus); >>> } >>> >>> + machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0); >>> + if (machine_opts) >>> + usb_on = qemu_opt_get_bool(machine_opts, "usb", true); >> >> Still missing braces for if. >> >> Other than that looking good to me now. Where's 2/2? > 2/2's title is as the following: > [Qemu-devel][PATCH 2/2] spapr: Add support for -vga option. :) > >> >> Please post the fixed version as a top-level [PATCH v4] along with a >> small change log after --- or in a cover letter. >> > OK. I will do that. :) > >> Andreas >> >>> + >>> + if (usb_on) { >>> + pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus, >>> + -1, "pci-ohci"); >>> + } >>> if (rma_size < (MIN_RMA_SLOF << 20)) { >>> fprintf(stderr, "qemu: pSeries SLOF firmware requires >= " >>> "%ldM guest RMA (Real Mode Area memory)\n", MIN_RMA_SLOF); >>> diff --git a/qemu-config.c b/qemu-config.c >>> index bb3bff4..cdab765 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 */ } >>> }, >> >> >> -- >> 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 >