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

[Peter Crosthwaite]

On Wed, Jun 27, 2012 at 11:13 PM, Li Zhang <address@hidden> wrote:
> 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

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