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

[Li Zhang]

On Wed, Jun 27, 2012 at 9:22 PM, Peter Crosthwaite
<address@hidden> wrote:
> 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 ...

Peter, thank you for correcting my English faults.
I will modify it and be more careful next time.  :)

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



-- 

Best Regards
-Li