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

[Alexander Graf]

On 18.06.2012, at 10:24, Li Zhang wrote:

> On Mon, Jun 18, 2012 at 3:54 PM, Peter Crosthwaite
> <address@hidden> wrote:
>>> 
>>> 3. Can't have USB: fail if the user tries to enable it.
>>> 
>>> Code sketch:
>>> 
>>>    /* init USB devices */
>>>    if (!machine->has_usb) {
>>>        if (usb_enabled)
>>>            [report error; should point to the offending options]
>>>            exit(1);
>>>        }
>>>    } else {
>>>        if (machine->has_usb > 0) {
>>>            usb_enabled = 1;
>>>        }
>>>        if (usb_enabled) {
>>>            if (foreach_device_config(DEV_USB, usb_parse) < 0)
>>>                exit(1);
>>>        }
>>>    }
>>> 
>>> 
>>>>> Anyway, I don't see why we need to update opts.  Who's using the updated
>>>>> opts?
>>>>> 
>>>> psereis will use this opts.
>>>> usb kbd and mouse will be needed  with vga enabled.
>>> 
>>> Do they use the updated QemuOpts *opts?  I'd expect them to use usb_on,
>>> or whatever flag variable governs USB (now: usb_enabled).
>>> 
>> 
>> I think whats going on here is Li is trying to do the right thing by
>> using QEMU opts for this new machine functionality, however, its
>> getting tangled with all this global state replication of -usb. Isnt
>> there predecessor work here in getting rid of usb_enabled first? To
> I won't introduce global state any more in the latest version.
> It just gets the usb_on from machine options.
> It won't use usb_enabled.
> 
>> that end, I think what is being proposed here is two (somewhat
>> independent) patches. One patch for changing usb to QEMU_OPTS that
>> primarily does this:
>> 
>> diff --git a/sysemu.h b/sysemu.h
>> index bc2c788..9f5ce2c 100644
>> --- a/sysemu.h
>> +++ b/sysemu.h
>> @@ -117,7 +117,6 @@ extern const char *keyboard_layout;
>>  extern int win2k_install_hack;
>>  extern int alt_grab;
>>  extern int ctrl_grab;
>> -extern int usb_enabled;
>>  extern int smp_cpus;
>>  extern int max_cpus;
>>  extern int cursor_hide;
>> [6]   Done                    gedit ./sysemu.h
>> 
>> And the second patch which is the pseries machine model stuff.
>> 
>> Which way round probably doesnt matter right? You could make your
> Because there are some machines using usb_enabled.
> So I'd rather to left it as global state and add another usb option in
> machine options.
> Then the machine can get usb option from machine options to enable usb.
> So the latest patch won't introduce the global state.
> I will send out the latest version later.

Right, and the point was to make -usb create a machine option as well. Then 
replace all users of usb_enabled with a function call like

  bool usb_enabled(bool default_on);

which could for current machines default to off if usb= is not set on the 
machine opts. But for pseries we could pass true as a parameter, defaulting to 
on. And along the way, we have merged everyone onto the same syntax and 
-machine usb=x would even work outside of spapr.c ;).


Alex