Re: [Qemu-ppc] [Qemu-devel] [PATCH 1/2] Add usb option in machine options to enable/disable usb

[Li Zhang]

On Wed, Jun 27, 2012 at 8:00 PM, Andreas Färber <address@hidden> 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 <address@hidden>
>> ---
>>  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