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 >> 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=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