Re: [Qemu-devel] [Qemu-ppc][PATCH v5 1/4] Add usb option in machine options

[Li Zhang <zhlcindy@gmail.com>]

On Sun, Jul 8, 2012 at 10:45 PM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 08.07.2012, at 16:21, Li Zhang <zhlcindy@gmail.com> wrote:
>
>> On Fri, Jul 6, 2012 at 9:43 PM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
>>>
>>>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>
>>>> pSeries machine needs to enable USB to add a USB
>>>> keyboard or USB mouse. -usb option won't be used in
>>>> the future, and machine options are a better way to
>>>> enable USB.
>>>>
>>>> So this patch is to add USB option to machine options
>>>> (-machine type=pseries,usb=on/off) to enable/disable
>>>> USB controller. And machines will get the machine option
>>>> and create a USB controller if USB is on.
>>>>
>>>> By the way, USB is on by default on pSeries machine.
>>>> So USB controller should be turned off explicitly through
>>>> the command line for "-nodefault" case as the following:
>>>> -machine type=pseries,usb=off.
>>>>
>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>
>>> Shouldn't you also convert -usb over to the new syntax, so that -usb basically is the same as -machine x,usb=on?
>>>
>>   Assumely, -usb and -machine x, usb=on shouldn't be used at the same time.
>>   When using -usb, and -machine x, usb=off, it's a little like to
>> disable the default USB of machine,
>>   and use another specific USB with -usb option.
>>
>>   That's what my understanding. :)
>
> It's not about passing them in at the same time, but about having a single semantic instance where "Do we expose USB?" gets stored. That makes the code easier.
>
> Also if you change things the way I suggested, every platform will be able to use -usb and -machine usb=x. So things become a lot more logical from the command line perspective as well, since a management app can always pass in -machine usb=x and have the same semantics, regardless of the machine we're running on.
>
Got it. I  will change it.
>
> Alex
>
>>
>>>
>>> Alex
>>>
>>>> ---
>>>> hw/spapr.c    |   11 +++++++++++
>>>> qemu-config.c |    4 ++++
>>>> 2 files changed, 15 insertions(+)
>>>>
>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>> index 81c9343..973de1b 100644
>>>> --- a/hw/spapr.c
>>>> +++ b/hw/spapr.c
>>>> @@ -575,6 +575,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;
>>>> +    bool add_usb = true;
>>>>
>>>>    spapr = g_malloc0(sizeof(*spapr));
>>>>    QLIST_INIT(&spapr->phbs);
>>>> @@ -710,6 +712,15 @@ 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) {
>>>> +        add_usb = qemu_opt_get_bool(machine_opts, "usb", true);
>>>> +    }
>>>> +
>>>> +    if (add_usb) {
>>>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>> +                          -1, "pci-ohci");
>>>> +    }
>>>
>>> Didn't I ask you to extract this out to generic code? I don't want to have a "usb" machine opt that is only available for -M pseries.
>>>
>>  OK. I will do that. :)
>>>
>>> Alex
>>>
>>
>>
>>
>> --
>>
>> Best Regards
>> -Li



-- 

Best Regards
-Li