On 18.06.2012, at 10:24, Li Zhang wrote:
> On Mon, Jun 18, 2012 at 3:54 PM, Peter Crosthwaite
> <peter.crosthwa...@petalogix.com> 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
