| Message ID | 1339754783-14341-1-git-send-email-zhlcindy@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Li Zhang <zhlcindy@linux.vnet.ibm.com> writes: > For pseries machine, it needs to enable usb to add > keyboard or usb mouse. -usb option won't be used in > the future, and machine options is a better way to > enable usb. > > So this patch is to add usb option to machine options > (-machine type=psereis,usb=on/off)to enable/disable > usb controller. > > In this patch, usb_on is an global option which can > be checked by machines. > For example, on pseries, it will check if usb_on is 1, > if it is 1, it will create one usb ohci controller. > As the following: > if (usb_on == 1) { > pci_create_simple(bus, -1, "pci-ohci"); > } > > In this patch, usb is on by default. So, for -nodefault, > usb should be set off in the command line as the following: > -machine type=pseries,usb=off. > > Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> > > --- > hw/spapr.c | 5 +++++ > sysemu.h | 1 + > vl.c | 17 +++++++++++++++++ > 3 files changed, 23 insertions(+) > > diff --git a/hw/spapr.c b/hw/spapr.c > index d0bddbc..1feb739 100644 > --- a/hw/spapr.c > +++ b/hw/spapr.c > @@ -661,6 +661,11 @@ static void ppc_spapr_init(ram_addr_t ram_size, > spapr_vscsi_create(spapr->vio_bus); > } > > + if (usb_on == 1) { > + 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/sysemu.h b/sysemu.h > index bc2c788..08134ae 100644 > --- a/sysemu.h > +++ b/sysemu.h > @@ -109,6 +109,7 @@ extern int vga_interface_type; > #define vmsvga_enabled (vga_interface_type == VGA_VMWARE) > #define qxl_enabled (vga_interface_type == VGA_QXL) > > +extern int usb_on; > extern int graphic_width; > extern int graphic_height; > extern int graphic_depth; > diff --git a/vl.c b/vl.c > index 204d85b..b200203 100644 > --- a/vl.c > +++ b/vl.c > @@ -202,6 +202,7 @@ int smp_cpus = 1; > int max_cpus = 0; > int smp_cores = 1; > int smp_threads = 1; > +int usb_on = 0; > #ifdef CONFIG_VNC > const char *vnc_display; > #endif > @@ -758,6 +759,21 @@ static int bt_parse(const char *opt) > return 1; > } > > +static int get_usb_opt(QemuOpts *opts) > +{ > + const char *usb_opt = NULL; Useless initializer. > + int usb_on = 0; > + > + if (NULL == qemu_opt_get(opts, "usb")) > + qemu_opt_set(opts, "usb", "on"); Why are you changing opts? > + > + usb_opt = qemu_opt_get(opts, "usb"); > + if (usb_opt && strcmp(usb_opt, "on") == 0) Please don't duplicate parsing of bools; use qemu_opt_get_bool(). > + usb_on = 1; > + > + return usb_on; > +} > + > /***********************************************************/ > /* QEMU Block devices */ > > @@ -3356,6 +3372,7 @@ int main(int argc, char **argv, char **envp) > kernel_filename = qemu_opt_get(machine_opts, "kernel"); > initrd_filename = qemu_opt_get(machine_opts, "initrd"); > kernel_cmdline = qemu_opt_get(machine_opts, "append"); > + usb_on = get_usb_opt(machine_opts); Anything wrong with usb_on = qemu_opt_get_bool(machine_opts, "usb", 0); ? > } else { > kernel_filename = initrd_filename = kernel_cmdline = NULL; > } Your new global usb_on is still unused, and it's not integrated with -usb. I doubt it can be merged that way.
Am 15.06.2012 12:06, schrieb Li Zhang: > For pseries machine, it needs to enable usb to add > keyboard or usb mouse. -usb option won't be used in > the future, and machine options is a better way to > enable usb. > > So this patch is to add usb option to machine options > (-machine type=psereis,usb=on/off)to enable/disable > usb controller. > > In this patch, usb_on is an global option which can > be checked by machines. [snip] ...which is exactly what you've been asked to change in v1. Please do. Also please stick to QEMU Coding Style, which requires braces for if. Use of the bool type is preferred of int for two-state logic. Regards, Andreas
On Fri, Jun 15, 2012 at 8:04 PM, Markus Armbruster <armbru@redhat.com> wrote: > Li Zhang <zhlcindy@linux.vnet.ibm.com> writes: > >> For pseries machine, it needs to enable usb to add >> keyboard or usb mouse. -usb option won't be used in >> the future, and machine options is a better way to >> enable usb. >> >> So this patch is to add usb option to machine options >> (-machine type=psereis,usb=on/off)to enable/disable >> usb controller. >> >> In this patch, usb_on is an global option which can >> be checked by machines. >> For example, on pseries, it will check if usb_on is 1, >> if it is 1, it will create one usb ohci controller. >> As the following: >> if (usb_on == 1) { >> pci_create_simple(bus, -1, "pci-ohci"); >> } >> >> In this patch, usb is on by default. So, for -nodefault, >> usb should be set off in the command line as the following: >> -machine type=pseries,usb=off. >> >> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> >> >> --- >> hw/spapr.c | 5 +++++ >> sysemu.h | 1 + >> vl.c | 17 +++++++++++++++++ >> 3 files changed, 23 insertions(+) >> >> diff --git a/hw/spapr.c b/hw/spapr.c >> index d0bddbc..1feb739 100644 >> --- a/hw/spapr.c >> +++ b/hw/spapr.c >> @@ -661,6 +661,11 @@ static void ppc_spapr_init(ram_addr_t ram_size, >> spapr_vscsi_create(spapr->vio_bus); >> } >> >> + if (usb_on == 1) { >> + 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/sysemu.h b/sysemu.h >> index bc2c788..08134ae 100644 >> --- a/sysemu.h >> +++ b/sysemu.h >> @@ -109,6 +109,7 @@ extern int vga_interface_type; >> #define vmsvga_enabled (vga_interface_type == VGA_VMWARE) >> #define qxl_enabled (vga_interface_type == VGA_QXL) >> >> +extern int usb_on; >> extern int graphic_width; >> extern int graphic_height; >> extern int graphic_depth; >> diff --git a/vl.c b/vl.c >> index 204d85b..b200203 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -202,6 +202,7 @@ int smp_cpus = 1; >> int max_cpus = 0; >> int smp_cores = 1; >> int smp_threads = 1; >> +int usb_on = 0; >> #ifdef CONFIG_VNC >> const char *vnc_display; >> #endif >> @@ -758,6 +759,21 @@ static int bt_parse(const char *opt) >> return 1; >> } >> >> +static int get_usb_opt(QemuOpts *opts) >> +{ >> + const char *usb_opt = NULL; > > Useless initializer. Thanks. I will remove it. > >> + int usb_on = 0; >> + >> + if (NULL == qemu_opt_get(opts, "usb")) >> + qemu_opt_set(opts, "usb", "on"); > > Why are you changing opts? USB is enabled by default when there is no usb option setting. For example, using # qemu-system-ppc64 -machine type=pseries There is no usb option, but usb is set on. > >> + >> + usb_opt = qemu_opt_get(opts, "usb"); >> + if (usb_opt && strcmp(usb_opt, "on") == 0) > > Please don't duplicate parsing of bools; use qemu_opt_get_bool(). OK. > >> + usb_on = 1; >> + >> + return usb_on; >> +} >> + >> /***********************************************************/ >> /* QEMU Block devices */ >> >> @@ -3356,6 +3372,7 @@ int main(int argc, char **argv, char **envp) >> kernel_filename = qemu_opt_get(machine_opts, "kernel"); >> initrd_filename = qemu_opt_get(machine_opts, "initrd"); >> kernel_cmdline = qemu_opt_get(machine_opts, "append"); >> + usb_on = get_usb_opt(machine_opts); > > Anything wrong with > > usb_on = qemu_opt_get_bool(machine_opts, "usb", 0); > > ? > >> } else { >> kernel_filename = initrd_filename = kernel_cmdline = NULL; >> } > > Your new global usb_on is still unused, and it's not integrated with > -usb. I doubt it can be merged that way. > It is used in spapr.c. In the front of this patch, it is used. It seems that this is not good. Maybe it is better to move the options setting to spapr.c.
On Fri, Jun 15, 2012 at 8:30 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 15.06.2012 12:06, schrieb Li Zhang: >> For pseries machine, it needs to enable usb to add >> keyboard or usb mouse. -usb option won't be used in >> the future, and machine options is a better way to >> enable usb. >> >> So this patch is to add usb option to machine options >> (-machine type=psereis,usb=on/off)to enable/disable >> usb controller. >> >> In this patch, usb_on is an global option which can >> be checked by machines. > [snip] > > ...which is exactly what you've been asked to change in v1. Please do. > OK. I will do it. :) > Also please stick to QEMU Coding Style, which requires braces for if. > Use of the bool type is preferred of int for two-state logic. > Got it. I will do it carefully in the future. > Regards, > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >
Li Zhang <zhlcindy@gmail.com> writes: > On Fri, Jun 15, 2012 at 8:04 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Li Zhang <zhlcindy@linux.vnet.ibm.com> writes: >> >>> For pseries machine, it needs to enable usb to add >>> keyboard or usb mouse. -usb option won't be used in >>> the future, and machine options is a better way to >>> enable usb. >>> >>> So this patch is to add usb option to machine options >>> (-machine type=psereis,usb=on/off)to enable/disable >>> usb controller. >>> >>> In this patch, usb_on is an global option which can >>> be checked by machines. >>> For example, on pseries, it will check if usb_on is 1, >>> if it is 1, it will create one usb ohci controller. >>> As the following: >>> if (usb_on == 1) { >>> pci_create_simple(bus, -1, "pci-ohci"); >>> } >>> >>> In this patch, usb is on by default. So, for -nodefault, >>> usb should be set off in the command line as the following: >>> -machine type=pseries,usb=off. >>> >>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> >>> >>> --- >>> hw/spapr.c | 5 +++++ >>> sysemu.h | 1 + >>> vl.c | 17 +++++++++++++++++ >>> 3 files changed, 23 insertions(+) >>> >>> diff --git a/hw/spapr.c b/hw/spapr.c >>> index d0bddbc..1feb739 100644 >>> --- a/hw/spapr.c >>> +++ b/hw/spapr.c >>> @@ -661,6 +661,11 @@ static void ppc_spapr_init(ram_addr_t ram_size, >>> spapr_vscsi_create(spapr->vio_bus); >>> } >>> >>> + if (usb_on == 1) { >>> + 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/sysemu.h b/sysemu.h >>> index bc2c788..08134ae 100644 >>> --- a/sysemu.h >>> +++ b/sysemu.h >>> @@ -109,6 +109,7 @@ extern int vga_interface_type; >>> #define vmsvga_enabled (vga_interface_type == VGA_VMWARE) >>> #define qxl_enabled (vga_interface_type == VGA_QXL) >>> >>> +extern int usb_on; >>> extern int graphic_width; >>> extern int graphic_height; >>> extern int graphic_depth; >>> diff --git a/vl.c b/vl.c >>> index 204d85b..b200203 100644 >>> --- a/vl.c >>> +++ b/vl.c >>> @@ -202,6 +202,7 @@ int smp_cpus = 1; >>> int max_cpus = 0; >>> int smp_cores = 1; >>> int smp_threads = 1; >>> +int usb_on = 0; >>> #ifdef CONFIG_VNC >>> const char *vnc_display; >>> #endif >>> @@ -758,6 +759,21 @@ static int bt_parse(const char *opt) >>> return 1; >>> } >>> >>> +static int get_usb_opt(QemuOpts *opts) >>> +{ >>> + const char *usb_opt = NULL; >> >> Useless initializer. > Thanks. I will remove it. >> >>> + int usb_on = 0; >>> + >>> + if (NULL == qemu_opt_get(opts, "usb")) >>> + qemu_opt_set(opts, "usb", "on"); >> >> Why are you changing opts? > USB is enabled by default when there is no usb option setting. > For example, > using # qemu-system-ppc64 -machine type=pseries > There is no usb option, but usb is set on. Isn't it off by default for at least some machines now? Anyway, I don't see why we need to update opts. Who's using the updated opts? >>> + >>> + usb_opt = qemu_opt_get(opts, "usb"); >>> + if (usb_opt && strcmp(usb_opt, "on") == 0) >> >> Please don't duplicate parsing of bools; use qemu_opt_get_bool(). > OK. >> >>> + usb_on = 1; >>> + >>> + return usb_on; >>> +} >>> + >>> /***********************************************************/ >>> /* QEMU Block devices */ >>> >>> @@ -3356,6 +3372,7 @@ int main(int argc, char **argv, char **envp) >>> kernel_filename = qemu_opt_get(machine_opts, "kernel"); >>> initrd_filename = qemu_opt_get(machine_opts, "initrd"); >>> kernel_cmdline = qemu_opt_get(machine_opts, "append"); >>> + usb_on = get_usb_opt(machine_opts); >> >> Anything wrong with >> >> usb_on = qemu_opt_get_bool(machine_opts, "usb", 0); >> >> ? >> >>> } else { >>> kernel_filename = initrd_filename = kernel_cmdline = NULL; >>> } >> >> Your new global usb_on is still unused, and it's not integrated with >> -usb. I doubt it can be merged that way. >> > It is used in spapr.c. In the front of this patch, it is used. > It seems that this is not good. > Maybe it is better to move the options setting to spapr.c. Dang, I missed that. Sorry for the noise.
On Fri, Jun 15, 2012 at 10:34 PM, Markus Armbruster <armbru@redhat.com> wrote: > Li Zhang <zhlcindy@gmail.com> writes: > >> On Fri, Jun 15, 2012 at 8:04 PM, Markus Armbruster <armbru@redhat.com> wrote: >>> Li Zhang <zhlcindy@linux.vnet.ibm.com> writes: >>> >>>> For pseries machine, it needs to enable usb to add >>>> keyboard or usb mouse. -usb option won't be used in >>>> the future, and machine options is a better way to >>>> enable usb. >>>> >>>> So this patch is to add usb option to machine options >>>> (-machine type=psereis,usb=on/off)to enable/disable >>>> usb controller. >>>> >>>> In this patch, usb_on is an global option which can >>>> be checked by machines. >>>> For example, on pseries, it will check if usb_on is 1, >>>> if it is 1, it will create one usb ohci controller. >>>> As the following: >>>> if (usb_on == 1) { >>>> pci_create_simple(bus, -1, "pci-ohci"); >>>> } >>>> >>>> In this patch, usb is on by default. So, for -nodefault, >>>> usb should be set off in the command line as the following: >>>> -machine type=pseries,usb=off. >>>> >>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> >>>> >>>> --- >>>> hw/spapr.c | 5 +++++ >>>> sysemu.h | 1 + >>>> vl.c | 17 +++++++++++++++++ >>>> 3 files changed, 23 insertions(+) >>>> >>>> diff --git a/hw/spapr.c b/hw/spapr.c >>>> index d0bddbc..1feb739 100644 >>>> --- a/hw/spapr.c >>>> +++ b/hw/spapr.c >>>> @@ -661,6 +661,11 @@ static void ppc_spapr_init(ram_addr_t ram_size, >>>> spapr_vscsi_create(spapr->vio_bus); >>>> } >>>> >>>> + if (usb_on == 1) { >>>> + 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/sysemu.h b/sysemu.h >>>> index bc2c788..08134ae 100644 >>>> --- a/sysemu.h >>>> +++ b/sysemu.h >>>> @@ -109,6 +109,7 @@ extern int vga_interface_type; >>>> #define vmsvga_enabled (vga_interface_type == VGA_VMWARE) >>>> #define qxl_enabled (vga_interface_type == VGA_QXL) >>>> >>>> +extern int usb_on; >>>> extern int graphic_width; >>>> extern int graphic_height; >>>> extern int graphic_depth; >>>> diff --git a/vl.c b/vl.c >>>> index 204d85b..b200203 100644 >>>> --- a/vl.c >>>> +++ b/vl.c >>>> @@ -202,6 +202,7 @@ int smp_cpus = 1; >>>> int max_cpus = 0; >>>> int smp_cores = 1; >>>> int smp_threads = 1; >>>> +int usb_on = 0; >>>> #ifdef CONFIG_VNC >>>> const char *vnc_display; >>>> #endif >>>> @@ -758,6 +759,21 @@ static int bt_parse(const char *opt) >>>> return 1; >>>> } >>>> >>>> +static int get_usb_opt(QemuOpts *opts) >>>> +{ >>>> + const char *usb_opt = NULL; >>> >>> Useless initializer. >> Thanks. I will remove it. >>> >>>> + int usb_on = 0; >>>> + >>>> + if (NULL == qemu_opt_get(opts, "usb")) >>>> + qemu_opt_set(opts, "usb", "on"); >>> >>> Why are you changing opts? >> USB is enabled by default when there is no usb option setting. >> For example, >> using # qemu-system-ppc64 -machine type=pseries >> There is no usb option, but usb is set on. > > Isn't it off by default for at least some machines now? > OK. This default setting is decided by the machine. In the new version, I put this setting in machine. It can be set off or on. For psereis it sets on. > 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. >>>> + >>>> + usb_opt = qemu_opt_get(opts, "usb"); >>>> + if (usb_opt && strcmp(usb_opt, "on") == 0) >>> >>> Please don't duplicate parsing of bools; use qemu_opt_get_bool(). >> OK. >>> >>>> + usb_on = 1; >>>> + >>>> + return usb_on; >>>> +} >>>> + >>>> /***********************************************************/ >>>> /* QEMU Block devices */ >>>> >>>> @@ -3356,6 +3372,7 @@ int main(int argc, char **argv, char **envp) >>>> kernel_filename = qemu_opt_get(machine_opts, "kernel"); >>>> initrd_filename = qemu_opt_get(machine_opts, "initrd"); >>>> kernel_cmdline = qemu_opt_get(machine_opts, "append"); >>>> + usb_on = get_usb_opt(machine_opts); >>> >>> Anything wrong with >>> >>> usb_on = qemu_opt_get_bool(machine_opts, "usb", 0); >>> >>> ? >>> >>>> } else { >>>> kernel_filename = initrd_filename = kernel_cmdline = NULL; >>>> } >>> >>> Your new global usb_on is still unused, and it's not integrated with >>> -usb. I doubt it can be merged that way. >>> >> It is used in spapr.c. In the front of this patch, it is used. >> It seems that this is not good. >> Maybe it is better to move the options setting to spapr.c. > > Dang, I missed that. Sorry for the noise.
Li Zhang <zhlcindy@gmail.com> writes: > On Fri, Jun 15, 2012 at 10:34 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Li Zhang <zhlcindy@gmail.com> writes: >> >>> On Fri, Jun 15, 2012 at 8:04 PM, Markus Armbruster <armbru@redhat.com> wrote: >>>> Li Zhang <zhlcindy@linux.vnet.ibm.com> writes: >>>> >>>>> For pseries machine, it needs to enable usb to add >>>>> keyboard or usb mouse. -usb option won't be used in >>>>> the future, and machine options is a better way to >>>>> enable usb. >>>>> >>>>> So this patch is to add usb option to machine options >>>>> (-machine type=psereis,usb=on/off)to enable/disable >>>>> usb controller. >>>>> >>>>> In this patch, usb_on is an global option which can >>>>> be checked by machines. >>>>> For example, on pseries, it will check if usb_on is 1, >>>>> if it is 1, it will create one usb ohci controller. >>>>> As the following: >>>>> if (usb_on == 1) { >>>>> pci_create_simple(bus, -1, "pci-ohci"); >>>>> } >>>>> >>>>> In this patch, usb is on by default. So, for -nodefault, >>>>> usb should be set off in the command line as the following: >>>>> -machine type=pseries,usb=off. >>>>> >>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> >>>>> >>>>> --- >>>>> hw/spapr.c | 5 +++++ >>>>> sysemu.h | 1 + >>>>> vl.c | 17 +++++++++++++++++ >>>>> 3 files changed, 23 insertions(+) >>>>> >>>>> diff --git a/hw/spapr.c b/hw/spapr.c >>>>> index d0bddbc..1feb739 100644 >>>>> --- a/hw/spapr.c >>>>> +++ b/hw/spapr.c >>>>> @@ -661,6 +661,11 @@ static void ppc_spapr_init(ram_addr_t ram_size, >>>>> spapr_vscsi_create(spapr->vio_bus); >>>>> } >>>>> >>>>> + if (usb_on == 1) { >>>>> + 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/sysemu.h b/sysemu.h >>>>> index bc2c788..08134ae 100644 >>>>> --- a/sysemu.h >>>>> +++ b/sysemu.h >>>>> @@ -109,6 +109,7 @@ extern int vga_interface_type; >>>>> #define vmsvga_enabled (vga_interface_type == VGA_VMWARE) >>>>> #define qxl_enabled (vga_interface_type == VGA_QXL) >>>>> >>>>> +extern int usb_on; >>>>> extern int graphic_width; >>>>> extern int graphic_height; >>>>> extern int graphic_depth; >>>>> diff --git a/vl.c b/vl.c >>>>> index 204d85b..b200203 100644 >>>>> --- a/vl.c >>>>> +++ b/vl.c >>>>> @@ -202,6 +202,7 @@ int smp_cpus = 1; >>>>> int max_cpus = 0; >>>>> int smp_cores = 1; >>>>> int smp_threads = 1; >>>>> +int usb_on = 0; >>>>> #ifdef CONFIG_VNC >>>>> const char *vnc_display; >>>>> #endif >>>>> @@ -758,6 +759,21 @@ static int bt_parse(const char *opt) >>>>> return 1; >>>>> } >>>>> >>>>> +static int get_usb_opt(QemuOpts *opts) >>>>> +{ >>>>> + const char *usb_opt = NULL; >>>> >>>> Useless initializer. >>> Thanks. I will remove it. >>>> >>>>> + int usb_on = 0; >>>>> + >>>>> + if (NULL == qemu_opt_get(opts, "usb")) >>>>> + qemu_opt_set(opts, "usb", "on"); >>>> >>>> Why are you changing opts? >>> USB is enabled by default when there is no usb option setting. >>> For example, >>> using # qemu-system-ppc64 -machine type=pseries >>> There is no usb option, but usb is set on. >> >> Isn't it off by default for at least some machines now? >> > OK. This default setting is decided by the machine. > In the new version, I put this setting in machine. > It can be set off or on. > For psereis it sets on. Makes sense. Perhaps we really have three kinds of machines, not just two: 1. Must have USB: main() sets usb_enabled to true. 2. May have USB: usb_enabled = -usb or -usbdevice given 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). [...]
On Mon, Jun 18, 2012 at 3:29 PM, Markus Armbruster <armbru@redhat.com> wrote: > Li Zhang <zhlcindy@gmail.com> writes: > >> On Fri, Jun 15, 2012 at 10:34 PM, Markus Armbruster <armbru@redhat.com> wrote: >>> Li Zhang <zhlcindy@gmail.com> writes: >>> >>>> On Fri, Jun 15, 2012 at 8:04 PM, Markus Armbruster <armbru@redhat.com> wrote: >>>>> Li Zhang <zhlcindy@linux.vnet.ibm.com> writes: >>>>> >>>>>> For pseries machine, it needs to enable usb to add >>>>>> keyboard or usb mouse. -usb option won't be used in >>>>>> the future, and machine options is a better way to >>>>>> enable usb. >>>>>> >>>>>> So this patch is to add usb option to machine options >>>>>> (-machine type=psereis,usb=on/off)to enable/disable >>>>>> usb controller. >>>>>> >>>>>> In this patch, usb_on is an global option which can >>>>>> be checked by machines. >>>>>> For example, on pseries, it will check if usb_on is 1, >>>>>> if it is 1, it will create one usb ohci controller. >>>>>> As the following: >>>>>> if (usb_on == 1) { >>>>>> pci_create_simple(bus, -1, "pci-ohci"); >>>>>> } >>>>>> >>>>>> In this patch, usb is on by default. So, for -nodefault, >>>>>> usb should be set off in the command line as the following: >>>>>> -machine type=pseries,usb=off. >>>>>> >>>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> >>>>>> >>>>>> --- >>>>>> hw/spapr.c | 5 +++++ >>>>>> sysemu.h | 1 + >>>>>> vl.c | 17 +++++++++++++++++ >>>>>> 3 files changed, 23 insertions(+) >>>>>> >>>>>> diff --git a/hw/spapr.c b/hw/spapr.c >>>>>> index d0bddbc..1feb739 100644 >>>>>> --- a/hw/spapr.c >>>>>> +++ b/hw/spapr.c >>>>>> @@ -661,6 +661,11 @@ static void ppc_spapr_init(ram_addr_t ram_size, >>>>>> spapr_vscsi_create(spapr->vio_bus); >>>>>> } >>>>>> >>>>>> + if (usb_on == 1) { >>>>>> + 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/sysemu.h b/sysemu.h >>>>>> index bc2c788..08134ae 100644 >>>>>> --- a/sysemu.h >>>>>> +++ b/sysemu.h >>>>>> @@ -109,6 +109,7 @@ extern int vga_interface_type; >>>>>> #define vmsvga_enabled (vga_interface_type == VGA_VMWARE) >>>>>> #define qxl_enabled (vga_interface_type == VGA_QXL) >>>>>> >>>>>> +extern int usb_on; >>>>>> extern int graphic_width; >>>>>> extern int graphic_height; >>>>>> extern int graphic_depth; >>>>>> diff --git a/vl.c b/vl.c >>>>>> index 204d85b..b200203 100644 >>>>>> --- a/vl.c >>>>>> +++ b/vl.c >>>>>> @@ -202,6 +202,7 @@ int smp_cpus = 1; >>>>>> int max_cpus = 0; >>>>>> int smp_cores = 1; >>>>>> int smp_threads = 1; >>>>>> +int usb_on = 0; >>>>>> #ifdef CONFIG_VNC >>>>>> const char *vnc_display; >>>>>> #endif >>>>>> @@ -758,6 +759,21 @@ static int bt_parse(const char *opt) >>>>>> return 1; >>>>>> } >>>>>> >>>>>> +static int get_usb_opt(QemuOpts *opts) >>>>>> +{ >>>>>> + const char *usb_opt = NULL; >>>>> >>>>> Useless initializer. >>>> Thanks. I will remove it. >>>>> >>>>>> + int usb_on = 0; >>>>>> + >>>>>> + if (NULL == qemu_opt_get(opts, "usb")) >>>>>> + qemu_opt_set(opts, "usb", "on"); >>>>> >>>>> Why are you changing opts? >>>> USB is enabled by default when there is no usb option setting. >>>> For example, >>>> using # qemu-system-ppc64 -machine type=pseries >>>> There is no usb option, but usb is set on. >>> >>> Isn't it off by default for at least some machines now? >>> >> OK. This default setting is decided by the machine. >> In the new version, I put this setting in machine. >> It can be set off or on. >> For psereis it sets on. > > Makes sense. > > Perhaps we really have three kinds of machines, not just two: > > 1. Must have USB: main() sets usb_enabled to true. We only hope to use usb option of machine options, not use -usb or -usbdevice, which will be removed in the future. But there are still some machines are using it. > > 2. May have USB: usb_enabled = -usb or -usbdevice given > For one machine, if it uses usb option in machine, usb_enabled can't work for this machine. In fact, even if user use -usb, it still doesn't work. > 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); > } > } > > In fact, I really hope to remove usb_enabled. >>> 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). Yes, I have modified this for pseries. It will get usb_on from machine options. But there some other kinds of machines using usb_enabled. I have send out my latest patches, but it seems that there are some issues of my network. I can't see them in the mailing list. I will send out later. > > [...]
diff --git a/hw/spapr.c b/hw/spapr.c index d0bddbc..1feb739 100644 --- a/hw/spapr.c +++ b/hw/spapr.c @@ -661,6 +661,11 @@ static void ppc_spapr_init(ram_addr_t ram_size, spapr_vscsi_create(spapr->vio_bus); } + if (usb_on == 1) { + 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/sysemu.h b/sysemu.h index bc2c788..08134ae 100644 --- a/sysemu.h +++ b/sysemu.h @@ -109,6 +109,7 @@ extern int vga_interface_type; #define vmsvga_enabled (vga_interface_type == VGA_VMWARE) #define qxl_enabled (vga_interface_type == VGA_QXL) +extern int usb_on; extern int graphic_width; extern int graphic_height; extern int graphic_depth; diff --git a/vl.c b/vl.c index 204d85b..b200203 100644 --- a/vl.c +++ b/vl.c @@ -202,6 +202,7 @@ int smp_cpus = 1; int max_cpus = 0; int smp_cores = 1; int smp_threads = 1; +int usb_on = 0; #ifdef CONFIG_VNC const char *vnc_display; #endif @@ -758,6 +759,21 @@ static int bt_parse(const char *opt) return 1; } +static int get_usb_opt(QemuOpts *opts) +{ + const char *usb_opt = NULL; + int usb_on = 0; + + if (NULL == qemu_opt_get(opts, "usb")) + qemu_opt_set(opts, "usb", "on"); + + usb_opt = qemu_opt_get(opts, "usb"); + if (usb_opt && strcmp(usb_opt, "on") == 0) + usb_on = 1; + + return usb_on; +} + /***********************************************************/ /* QEMU Block devices */ @@ -3356,6 +3372,7 @@ int main(int argc, char **argv, char **envp) kernel_filename = qemu_opt_get(machine_opts, "kernel"); initrd_filename = qemu_opt_get(machine_opts, "initrd"); kernel_cmdline = qemu_opt_get(machine_opts, "append"); + usb_on = get_usb_opt(machine_opts); } else { kernel_filename = initrd_filename = kernel_cmdline = NULL; }
For pseries machine, it needs to enable usb to add keyboard or usb mouse. -usb option won't be used in the future, and machine options is a better way to enable usb. So this patch is to add usb option to machine options (-machine type=psereis,usb=on/off)to enable/disable usb controller. In this patch, usb_on is an global option which can be checked by machines. For example, on pseries, it will check if usb_on is 1, if it is 1, it will create one usb ohci controller. As the following: if (usb_on == 1) { pci_create_simple(bus, -1, "pci-ohci"); } In this patch, usb is on by default. So, for -nodefault, usb should be set off in the command line as the following: -machine type=pseries,usb=off. Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- hw/spapr.c | 5 +++++ sysemu.h | 1 + vl.c | 17 +++++++++++++++++ 3 files changed, 23 insertions(+)