| Message ID | CAEgOgz7HaHkKJZeh9Mq2_CqLTnEiB4_ojpxKOpPdQBCAi1Q_bw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 18, 2012 at 3:54 PM, Peter Crosthwaite <peter.crosthwaite@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. > machine model use the extern int usb_enabled initially then move it > across to machine opts along with the rest of the usb subsystem. Or > you could fix USB first (globally) then build on top of it. But I > think that this patch as is, is going to do is introduce is a > duplicate -usb implementation which is a little messy (even if it is > only an intermediary state). > Regards, > Peter > >> [...] >>
On 18.06.2012, at 10:24, Li Zhang wrote: > On Mon, Jun 18, 2012 at 3:54 PM, Peter Crosthwaite > <peter.crosthwaite@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
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;