| Message ID | 1339651039-15290-1-git-send-email-zhlcindy@gmail.com |
|---|---|
| State | New |
| Headers | show |
Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com: > From: Li Zhang <zhlcindy@linux.vnet.ibm.com> > > For pseries machine, it needs to enable usb > to add kbd 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_opt is an global option > which can be checked by machines. For example, > on pseries, it will check if usb_opt is on, if > it is on, it will create one usb ohci controller. > As the following: > if (usb_opts && strcmp(usb_opts, "on") == 0) > 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> > reviewed-by: Anthony Liguori <aliguori@us.ibm.com> > reviewed-by: Benjamin Herrenschmidt <benh@au1.ibm.com> > --- > qemu-config.c | 4 ++++ > sysemu.h | 1 + > vl.c | 12 ++++++++++++ > 3 files changed, 17 insertions(+) > > diff --git a/qemu-config.c b/qemu-config.c > index bb3bff4..258712a 100644 > --- a/qemu-config.c > +++ b/qemu-config.c > @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = { > .name = "dtb", > .type = QEMU_OPT_STRING, > .help = "Linux kernel device tree file", > + }, { > + .name = "usb", > + .type = QEMU_OPT_BOOL, > + .help = "Set on/off to enable/disable usb", > }, > { /* End of list */ } > }, > diff --git a/sysemu.h b/sysemu.h > index bc2c788..c5ea10d 100644 > --- a/sysemu.h > +++ b/sysemu.h > @@ -13,6 +13,7 @@ > /* vl.c */ > > extern const char *bios_name; > +extern const char *usb_opt; > > extern const char *qemu_name; > extern uint8_t qemu_uuid[]; > diff --git a/vl.c b/vl.c > index 204d85b..10f8e4c 100644 > --- a/vl.c > +++ b/vl.c > @@ -171,6 +171,7 @@ int main(int argc, char **argv) > > static const char *data_dir; > const char *bios_name = NULL; > +const char *usb_opt = NULL; > enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB; > DisplayType display_type = DT_DEFAULT; > int display_remote = 0; I'd be very surprised if Anthony has actually reviewed this... The point of using machine options is so that you can use the QemuOpts infrastructure to inquire this value, not to save more global state. Especially not a string when all you want is a boolean value. Further, in this patch it's only being assigned, not used anywhere. Andreas > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt) > return 1; > } > > +static int default_enable_usb(QemuOpts *opts) > +{ > + if (NULL == qemu_opt_get(opts, "usb")) { > + qemu_opt_set(opts, "usb", "on"); > + } > + > + return 0; > +} > + > /***********************************************************/ > /* QEMU Block devices */ > > @@ -3356,6 +3366,8 @@ 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"); > + default_enable_usb(machine_opts); > + usb_opt = qemu_opt_get(machine_opts, "usb"); > } else { > kernel_filename = initrd_filename = kernel_cmdline = NULL; > }
On Thu, 2012-06-14 at 16:27 +0200, Andreas Färber wrote: > Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com: > > From: Li Zhang <zhlcindy@linux.vnet.ibm.com> > > > > For pseries machine, it needs to enable usb > > to add kbd 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_opt is an global option > > which can be checked by machines. For example, > > on pseries, it will check if usb_opt is on, if > > it is on, it will create one usb ohci controller. > > As the following: > > if (usb_opts && strcmp(usb_opts, "on") == 0) > > 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> > > reviewed-by: Anthony Liguori <aliguori@us.ibm.com> > > reviewed-by: Benjamin Herrenschmidt <benh@au1.ibm.com> I have not reviewed that patch. Li, you must -never- make up such reviewed-by: lines ! You put them in if and only if you've had them given to you by the reviewer who has actually reviewed the patch in details. Ben. > > --- > > qemu-config.c | 4 ++++ > > sysemu.h | 1 + > > vl.c | 12 ++++++++++++ > > 3 files changed, 17 insertions(+) > > > > diff --git a/qemu-config.c b/qemu-config.c > > index bb3bff4..258712a 100644 > > --- a/qemu-config.c > > +++ b/qemu-config.c > > @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = { > > .name = "dtb", > > .type = QEMU_OPT_STRING, > > .help = "Linux kernel device tree file", > > + }, { > > + .name = "usb", > > + .type = QEMU_OPT_BOOL, > > + .help = "Set on/off to enable/disable usb", > > }, > > { /* End of list */ } > > }, > > diff --git a/sysemu.h b/sysemu.h > > index bc2c788..c5ea10d 100644 > > --- a/sysemu.h > > +++ b/sysemu.h > > @@ -13,6 +13,7 @@ > > /* vl.c */ > > > > extern const char *bios_name; > > +extern const char *usb_opt; > > > > extern const char *qemu_name; > > extern uint8_t qemu_uuid[]; > > diff --git a/vl.c b/vl.c > > index 204d85b..10f8e4c 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -171,6 +171,7 @@ int main(int argc, char **argv) > > > > static const char *data_dir; > > const char *bios_name = NULL; > > +const char *usb_opt = NULL; > > enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB; > > DisplayType display_type = DT_DEFAULT; > > int display_remote = 0; > > I'd be very surprised if Anthony has actually reviewed this... > > The point of using machine options is so that you can use the QemuOpts > infrastructure to inquire this value, not to save more global state. > Especially not a string when all you want is a boolean value. > > Further, in this patch it's only being assigned, not used anywhere. > > Andreas > > > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt) > > return 1; > > } > > > > +static int default_enable_usb(QemuOpts *opts) > > +{ > > + if (NULL == qemu_opt_get(opts, "usb")) { > > + qemu_opt_set(opts, "usb", "on"); > > + } > > + > > + return 0; > > +} > > + > > /***********************************************************/ > > /* QEMU Block devices */ > > > > @@ -3356,6 +3366,8 @@ 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"); > > + default_enable_usb(machine_opts); > > + usb_opt = qemu_opt_get(machine_opts, "usb"); > > } else { > > kernel_filename = initrd_filename = kernel_cmdline = NULL; > > } > >
OK,sorry for my fault. best regards Li Zhang 在 2012-6-15 上午6:05，"Benjamin Herrenschmidt" <benh@au1.ibm.com>写道： > > On Thu, 2012-06-14 at 16:27 +0200, Andreas Färber wrote: > > Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com: > > > From: Li Zhang <zhlcindy@linux.vnet.ibm.com> > > > > > > For pseries machine, it needs to enable usb > > > to add kbd 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_opt is an global option > > > which can be checked by machines. For example, > > > on pseries, it will check if usb_opt is on, if > > > it is on, it will create one usb ohci controller. > > > As the following: > > > if (usb_opts && strcmp(usb_opts, "on") == 0) > > > 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> > > > reviewed-by: Anthony Liguori <aliguori@us.ibm.com> > > > reviewed-by: Benjamin Herrenschmidt <benh@au1.ibm.com> > > I have not reviewed that patch. Li, you must -never- make up such > reviewed-by: lines ! You put them in if and only if you've had them > given to you by the reviewer who has actually reviewed the patch in > details. > > Ben. > > > > --- > > > qemu-config.c | 4 ++++ > > > sysemu.h | 1 + > > > vl.c | 12 ++++++++++++ > > > 3 files changed, 17 insertions(+) > > > > > > diff --git a/qemu-config.c b/qemu-config.c > > > index bb3bff4..258712a 100644 > > > --- a/qemu-config.c > > > +++ b/qemu-config.c > > > @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = { > > > .name = "dtb", > > > .type = QEMU_OPT_STRING, > > > .help = "Linux kernel device tree file", > > > + }, { > > > + .name = "usb", > > > + .type = QEMU_OPT_BOOL, > > > + .help = "Set on/off to enable/disable usb", > > > }, > > > { /* End of list */ } > > > }, > > > diff --git a/sysemu.h b/sysemu.h > > > index bc2c788..c5ea10d 100644 > > > --- a/sysemu.h > > > +++ b/sysemu.h > > > @@ -13,6 +13,7 @@ > > > /* vl.c */ > > > > > > extern const char *bios_name; > > > +extern const char *usb_opt; > > > > > > extern const char *qemu_name; > > > extern uint8_t qemu_uuid[]; > > > diff --git a/vl.c b/vl.c > > > index 204d85b..10f8e4c 100644 > > > --- a/vl.c > > > +++ b/vl.c > > > @@ -171,6 +171,7 @@ int main(int argc, char **argv) > > > > > > static const char *data_dir; > > > const char *bios_name = NULL; > > > +const char *usb_opt = NULL; > > > enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB; > > > DisplayType display_type = DT_DEFAULT; > > > int display_remote = 0; > > > > I'd be very surprised if Anthony has actually reviewed this... > > > > The point of using machine options is so that you can use the QemuOpts > > infrastructure to inquire this value, not to save more global state. > > Especially not a string when all you want is a boolean value. > > > > Further, in this patch it's only being assigned, not used anywhere. > > > > Andreas > > > > > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt) > > > return 1; > > > } > > > > > > +static int default_enable_usb(QemuOpts *opts) > > > +{ > > > + if (NULL == qemu_opt_get(opts, "usb")) { > > > + qemu_opt_set(opts, "usb", "on"); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > /***********************************************************/ > > > /* QEMU Block devices */ > > > > > > @@ -3356,6 +3366,8 @@ 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"); > > > + default_enable_usb(machine_opts); > > > + usb_opt = qemu_opt_get(machine_opts, "usb"); > > > } else { > > > kernel_filename = initrd_filename = kernel_cmdline = NULL; > > > } > > > > > >
On Thu, Jun 14, 2012 at 10:27 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com: > > From: Li Zhang <zhlcindy@linux.vnet.ibm.com> > > > > For pseries machine, it needs to enable usb > > to add kbd 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_opt is an global option > > which can be checked by machines. For example, > > on pseries, it will check if usb_opt is on, if > > it is on, it will create one usb ohci controller. > > As the following: > > if (usb_opts && strcmp(usb_opts, "on") == 0) > > 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> > > reviewed-by: Anthony Liguori <aliguori@us.ibm.com> > > reviewed-by: Benjamin Herrenschmidt <benh@au1.ibm.com> > > --- > > qemu-config.c | 4 ++++ > > sysemu.h | 1 + > > vl.c | 12 ++++++++++++ > > 3 files changed, 17 insertions(+) > > > > diff --git a/qemu-config.c b/qemu-config.c > > index bb3bff4..258712a 100644 > > --- a/qemu-config.c > > +++ b/qemu-config.c > > @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = { > > .name = "dtb", > > .type = QEMU_OPT_STRING, > > .help = "Linux kernel device tree file", > > + }, { > > + .name = "usb", > > + .type = QEMU_OPT_BOOL, > > + .help = "Set on/off to enable/disable usb", > > }, > > { /* End of list */ } > > }, > > diff --git a/sysemu.h b/sysemu.h > > index bc2c788..c5ea10d 100644 > > --- a/sysemu.h > > +++ b/sysemu.h > > @@ -13,6 +13,7 @@ > > /* vl.c */ > > > > extern const char *bios_name; > > +extern const char *usb_opt; > > > > extern const char *qemu_name; > > extern uint8_t qemu_uuid[]; > > diff --git a/vl.c b/vl.c > > index 204d85b..10f8e4c 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -171,6 +171,7 @@ int main(int argc, char **argv) > > > > static const char *data_dir; > > const char *bios_name = NULL; > > +const char *usb_opt = NULL; > > enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB; > > DisplayType display_type = DT_DEFAULT; > > int display_remote = 0; > > I'd be very surprised if Anthony has actually reviewed this... > Sorry, I just think I add him here because he gives some suggestions about adding usb option to machine options in the mailing list.:) Maybe it is not appropriate to add here. Sorry for that. > > The point of using machine options is so that you can use the QemuOpts > infrastructure to inquire this value, not to save more global state. > OK, I think it still needs one global value because other machines need to check whether usb is enabled. > Especially not a string when all you want is a boolean value. > > From the qemu, QEMU_OPT_BOOL type still use a string "on/off" to enable/disable usb. Maybe it's better to convert it to boolean value. Further, in this patch it's only being assigned, not used anywhere. > > In vl.c, it seems that it is not used. I have been thinking to use it in spapr.c. if (usb_opts && strcmp(usb_opts, "on") == 0) pci_create_simple(bus, -1, "pci-ohci"); But I want to see whether this way is accepted. If it is ok, I will add this option to spapr.c. Andreas > > > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt) > > return 1; > > } > > > > +static int default_enable_usb(QemuOpts *opts) > > +{ > > + if (NULL == qemu_opt_get(opts, "usb")) { > > + qemu_opt_set(opts, "usb", "on"); > > + } > > + > > + return 0; > > +} > > + > > /***********************************************************/ > > /* QEMU Block devices */ > > > > @@ -3356,6 +3366,8 @@ 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"); > > + default_enable_usb(machine_opts); > > + usb_opt = qemu_opt_get(machine_opts, "usb"); > > } else { > > kernel_filename = initrd_filename = kernel_cmdline = NULL; > > } > > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >
On Fri, Jun 15, 2012 at 1:06 PM, Li Zhang <zhlcindy@gmail.com> wrote: > > > On Thu, Jun 14, 2012 at 10:27 PM, Andreas Färber <afaerber@suse.de> wrote: >> >> Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com: >> > From: Li Zhang <zhlcindy@linux.vnet.ibm.com> >> > >> > For pseries machine, it needs to enable usb >> > to add kbd 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_opt is an global option >> > which can be checked by machines. For example, >> > on pseries, it will check if usb_opt is on, if >> > it is on, it will create one usb ohci controller. >> > As the following: >> > if (usb_opts && strcmp(usb_opts, "on") == 0) >> > 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> >> > reviewed-by: Anthony Liguori <aliguori@us.ibm.com> >> > reviewed-by: Benjamin Herrenschmidt <benh@au1.ibm.com> >> > --- >> > qemu-config.c | 4 ++++ >> > sysemu.h | 1 + >> > vl.c | 12 ++++++++++++ >> > 3 files changed, 17 insertions(+) >> > >> > diff --git a/qemu-config.c b/qemu-config.c >> > index bb3bff4..258712a 100644 >> > --- a/qemu-config.c >> > +++ b/qemu-config.c >> > @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = { >> > .name = "dtb", >> > .type = QEMU_OPT_STRING, >> > .help = "Linux kernel device tree file", >> > + }, { >> > + .name = "usb", >> > + .type = QEMU_OPT_BOOL, >> > + .help = "Set on/off to enable/disable usb", >> > }, >> > { /* End of list */ } >> > }, >> > diff --git a/sysemu.h b/sysemu.h >> > index bc2c788..c5ea10d 100644 >> > --- a/sysemu.h >> > +++ b/sysemu.h >> > @@ -13,6 +13,7 @@ >> > /* vl.c */ >> > >> > extern const char *bios_name; >> > +extern const char *usb_opt; >> > >> > extern const char *qemu_name; >> > extern uint8_t qemu_uuid[]; >> > diff --git a/vl.c b/vl.c >> > index 204d85b..10f8e4c 100644 >> > --- a/vl.c >> > +++ b/vl.c >> > @@ -171,6 +171,7 @@ int main(int argc, char **argv) >> > >> > static const char *data_dir; >> > const char *bios_name = NULL; >> > +const char *usb_opt = NULL; >> > enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB; >> > DisplayType display_type = DT_DEFAULT; >> > int display_remote = 0; >> >> I'd be very surprised if Anthony has actually reviewed this... > > > Sorry, I just think I add him here because he gives some suggestions about > adding usb option to machine options in the mailing list.:) > Maybe it is not appropriate to add here. Sorry for that. >> >> >> The point of using machine options is so that you can use the QemuOpts >> infrastructure to inquire this value, not to save more global state. > > OK, I think it still needs one global value because other machines need to > check whether usb is enabled. > Hi Li, What should really happen here, is if you do change over to a machine opt, then all clients of the -usb should use the machine opt infrastructure, then the need for global state is eliminated completely. But does this new -usb cmd line option have any connection to the existing -usb in this series? It looks like your trying to implement a new option to add/remove your usb controller, in which case, I think the best option is to do it using either configs or -device arguments. Regards, Peter >> >> Especially not a string when all you want is a boolean value. >> > From the qemu, QEMU_OPT_BOOL type still use a string > "on/off" to enable/disable usb. > Maybe it's better to convert it to boolean value. > >> Further, in this patch it's only being assigned, not used anywhere. >> > In vl.c, it seems that it is not used. > I have been thinking to use it in spapr.c. > > if (usb_opts && strcmp(usb_opts, "on") == 0) > pci_create_simple(bus, -1, "pci-ohci"); > > But I want to see whether this way is accepted. > If it is ok, I will add this option to spapr.c. > It seems like the purpose of this patch it to get comments? You should include "RFC" in your patch description as it flags your patches and series as a request for comments, rather than a merge proposal: git format-patch --thread --cover-letter --subject-prefix "RFC PATCH v1" ... >> Andreas >> >> > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt) >> > return 1; >> > } >> > >> > +static int default_enable_usb(QemuOpts *opts) >> > +{ >> > + if (NULL == qemu_opt_get(opts, "usb")) { >> > + qemu_opt_set(opts, "usb", "on"); >> > + } >> > + >> > + return 0; >> > +} >> > + >> > /***********************************************************/ >> > /* QEMU Block devices */ >> > >> > @@ -3356,6 +3366,8 @@ 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"); >> > + default_enable_usb(machine_opts); >> > + usb_opt = qemu_opt_get(machine_opts, "usb"); >> > } else { >> > kernel_filename = initrd_filename = kernel_cmdline = NULL; >> > } >> >> >> -- >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany >> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg > > > > > -- > > Best Regards > -Li >
On Fri, 2012-06-15 at 15:15 +1000, Peter Crosthwaite wrote: > > What should really happen here, is if you do change over to a machine > opt, then all clients of the -usb should use the machine opt > infrastructure, then the need for global state is eliminated > completely. > > But does this new -usb cmd line option have any connection to the > existing -usb in this series? It looks like your trying to implement a > new option to add/remove your usb controller, in which case, I think > the best option is to do it using either configs or -device arguments. The purpose is to have options for the ppc machines such as pseries to disable the default usb controller. IE. Those machines (well some of them at least) have no other input device (no PS/2, no ADB, ...) and so in order to provide a semi-decent user interface, we want to create an OHCI with mouse & keyboard emulation along with the vga display by default, and have options to disable those defaults for cases like libvirt where it really wants to create all that stuff itself so it gets the right names. VGA is already handled by a generic "high level" option (by opposition to using -device which I qualify of "low level") -vga which takes an argument and that argument can be "none". However, the generic "high level" -usb option doesn't take an argument and is only meant to enable, not disable (and thus cannot be augmented easily without breaking things). So after the discussion on the list, we decided that -machine options is the right way to handle that. I haven't had a chance to review Li's patch itself and whether it does that though. Just chiming in on what the intention is. Cheers, Ben.
On Fri, Jun 15, 2012 at 1:15 PM, Peter Crosthwaite < peter.crosthwaite@petalogix.com> wrote: > On Fri, Jun 15, 2012 at 1:06 PM, Li Zhang <zhlcindy@gmail.com> wrote: > > > > > > On Thu, Jun 14, 2012 at 10:27 PM, Andreas Färber <afaerber@suse.de> > wrote: > >> > >> Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com: > >> > From: Li Zhang <zhlcindy@linux.vnet.ibm.com> > >> > > >> > For pseries machine, it needs to enable usb > >> > to add kbd 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_opt is an global option > >> > which can be checked by machines. For example, > >> > on pseries, it will check if usb_opt is on, if > >> > it is on, it will create one usb ohci controller. > >> > As the following: > >> > if (usb_opts && strcmp(usb_opts, "on") == 0) > >> > 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> > >> > reviewed-by: Anthony Liguori <aliguori@us.ibm.com> > >> > reviewed-by: Benjamin Herrenschmidt <benh@au1.ibm.com> > >> > --- > >> > qemu-config.c | 4 ++++ > >> > sysemu.h | 1 + > >> > vl.c | 12 ++++++++++++ > >> > 3 files changed, 17 insertions(+) > >> > > >> > diff --git a/qemu-config.c b/qemu-config.c > >> > index bb3bff4..258712a 100644 > >> > --- a/qemu-config.c > >> > +++ b/qemu-config.c > >> > @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = { > >> > .name = "dtb", > >> > .type = QEMU_OPT_STRING, > >> > .help = "Linux kernel device tree file", > >> > + }, { > >> > + .name = "usb", > >> > + .type = QEMU_OPT_BOOL, > >> > + .help = "Set on/off to enable/disable usb", > >> > }, > >> > { /* End of list */ } > >> > }, > >> > diff --git a/sysemu.h b/sysemu.h > >> > index bc2c788..c5ea10d 100644 > >> > --- a/sysemu.h > >> > +++ b/sysemu.h > >> > @@ -13,6 +13,7 @@ > >> > /* vl.c */ > >> > > >> > extern const char *bios_name; > >> > +extern const char *usb_opt; > >> > > >> > extern const char *qemu_name; > >> > extern uint8_t qemu_uuid[]; > >> > diff --git a/vl.c b/vl.c > >> > index 204d85b..10f8e4c 100644 > >> > --- a/vl.c > >> > +++ b/vl.c > >> > @@ -171,6 +171,7 @@ int main(int argc, char **argv) > >> > > >> > static const char *data_dir; > >> > const char *bios_name = NULL; > >> > +const char *usb_opt = NULL; > >> > enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB; > >> > DisplayType display_type = DT_DEFAULT; > >> > int display_remote = 0; > >> > >> I'd be very surprised if Anthony has actually reviewed this... > > > > > > Sorry, I just think I add him here because he gives some suggestions > about > > adding usb option to machine options in the mailing list.:) > > Maybe it is not appropriate to add here. Sorry for that. > >> > >> > >> The point of using machine options is so that you can use the QemuOpts > >> infrastructure to inquire this value, not to save more global state. > > > > OK, I think it still needs one global value because other machines need > to > > check whether usb is enabled. > > > > Hi Li, > > What should really happen here, is if you do change over to a machine > opt, then all clients of the -usb should use the machine opt > infrastructure, then the need for global state is eliminated > completely. > > Yes, right. But does this new -usb cmd line option have any connection to the > existing -usb in this series? It looks like your trying to implement a > new option to add/remove your usb controller, in which case, I think > the best option is to do it using either configs or -device arguments. > > No, it doesn't have any connection to -usb series. We want to create usb controller by default. For example, #qemu-system-ppc64 -machine type=pseries without any -device arguments. Regards, > Peter > >> > >> Especially not a string when all you want is a boolean value. > >> > > From the qemu, QEMU_OPT_BOOL type still use a string > > "on/off" to enable/disable usb. > > Maybe it's better to convert it to boolean value. > > > >> Further, in this patch it's only being assigned, not used anywhere. > >> > > In vl.c, it seems that it is not used. > > I have been thinking to use it in spapr.c. > > > > if (usb_opts && strcmp(usb_opts, "on") == 0) > > pci_create_simple(bus, -1, "pci-ohci"); > > > > But I want to see whether this way is accepted. > > If it is ok, I will add this option to spapr.c. > > > > It seems like the purpose of this patch it to get comments? You should > include "RFC" in your patch description as it flags your patches and > series as a request for comments, rather than a merge proposal: > > git format-patch --thread --cover-letter --subject-prefix "RFC PATCH v1" .. > Sorry, I think I should use RFC. Thanks. :) > > >> Andreas > >> > >> > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt) > >> > return 1; > >> > } > >> > > >> > +static int default_enable_usb(QemuOpts *opts) > >> > +{ > >> > + if (NULL == qemu_opt_get(opts, "usb")) { > >> > + qemu_opt_set(opts, "usb", "on"); > >> > + } > >> > + > >> > + return 0; > >> > +} > >> > + > >> > /***********************************************************/ > >> > /* QEMU Block devices */ > >> > > >> > @@ -3356,6 +3366,8 @@ 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"); > >> > + default_enable_usb(machine_opts); > >> > + usb_opt = qemu_opt_get(machine_opts, "usb"); > >> > } else { > >> > kernel_filename = initrd_filename = kernel_cmdline = NULL; > >> > } > >> > >> > >> -- > >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > >> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg > > > > > > > > > > -- > > > > Best Regards > > -Li > > >
Am 15.06.2012 05:06, schrieb Li Zhang: > > > On Thu, Jun 14, 2012 at 10:27 PM, Andreas Färber <afaerber@suse.de > <mailto:afaerber@suse.de>> wrote: > > Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com > <mailto:zhlcindy@gmail.com>: > > From: Li Zhang <zhlcindy@linux.vnet.ibm.com > <mailto:zhlcindy@linux.vnet.ibm.com>> > > > > For pseries machine, it needs to enable usb > > to add kbd 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_opt is an global option > > which can be checked by machines. For example, > > on pseries, it will check if usb_opt is on, if > > it is on, it will create one usb ohci controller. > > As the following: > > if (usb_opts && strcmp(usb_opts, "on") == 0) > > 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 > <mailto:zhlcindy@linux.vnet.ibm.com>> > > reviewed-by: Anthony Liguori <aliguori@us.ibm.com > <mailto:aliguori@us.ibm.com>> > > reviewed-by: Benjamin Herrenschmidt <benh@au1.ibm.com > <mailto:benh@au1.ibm.com>> > > --- > > qemu-config.c | 4 ++++ > > sysemu.h | 1 + > > vl.c | 12 ++++++++++++ > > 3 files changed, 17 insertions(+) > > > > diff --git a/qemu-config.c b/qemu-config.c > > index bb3bff4..258712a 100644 > > --- a/qemu-config.c > > +++ b/qemu-config.c > > @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = { > > .name = "dtb", > > .type = QEMU_OPT_STRING, > > .help = "Linux kernel device tree file", > > + }, { > > + .name = "usb", > > + .type = QEMU_OPT_BOOL, > > + .help = "Set on/off to enable/disable usb", > > }, > > { /* End of list */ } > > }, > > diff --git a/sysemu.h b/sysemu.h > > index bc2c788..c5ea10d 100644 > > --- a/sysemu.h > > +++ b/sysemu.h > > @@ -13,6 +13,7 @@ > > /* vl.c */ > > > > extern const char *bios_name; > > +extern const char *usb_opt; > > > > extern const char *qemu_name; > > extern uint8_t qemu_uuid[]; > > diff --git a/vl.c b/vl.c > > index 204d85b..10f8e4c 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -171,6 +171,7 @@ int main(int argc, char **argv) > > > > static const char *data_dir; > > const char *bios_name = NULL; > > +const char *usb_opt = NULL; > > enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB; > > DisplayType display_type = DT_DEFAULT; > > int display_remote = 0; [...] > The point of using machine options is so that you can use the QemuOpts > infrastructure to inquire this value, not to save more global state. > > OK, I think it still needs one global value because other machines need > to check whether usb is enabled. > > > Especially not a string when all you want is a boolean value. > > From the qemu, QEMU_OPT_BOOL type still use a string > "on/off" to enable/disable usb. > Maybe it's better to convert it to boolean value. > > Further, in this patch it's only being assigned, not used anywhere. > > In vl.c, it seems that it is not used. > I have been thinking to use it in spapr.c. > > if (usb_opts && strcmp(usb_opts, "on") == 0) > pci_create_simple(bus, -1, "pci-ohci"); > > But I want to see whether this way is accepted. > If it is ok, I will add this option to spapr.c. First, please don't reply with HTML mails, it breaks the quoting as you can see. You don't seem to be getting what I'm saying: As discussed in great lengths for how to access a passed-in device tree for ARM, the code checking this option (i.e., in spapr.c) is expected to call qemu_opt_get() itself, instead of saving its value globally in vl.c. For example: machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0); usb_opt = qemu_opt_get(machine_opts, "usb"); if (strcmp(usb_opt, "off") != 0) { // set up OHCI } with appropriate NULL checks left as exercise for the reader. So you should combine the definition of the option in qemu-config.c (which looks okay) with its actual use in hw/spapr.c in one patch. Once introduced, hw/ppc_newworld.c should be updated to check the new option as well, but that can be a follow-up patch. Regards, Andreas > > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt) > > return 1; > > } > > > > +static int default_enable_usb(QemuOpts *opts) > > +{ > > + if (NULL == qemu_opt_get(opts, "usb")) { > > + qemu_opt_set(opts, "usb", "on"); > > + } > > + > > + return 0; > > +} > > + > > /***********************************************************/ > > /* QEMU Block devices */ > > > > @@ -3356,6 +3366,8 @@ 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"); > > + default_enable_usb(machine_opts); > > + usb_opt = qemu_opt_get(machine_opts, "usb"); > > } else { > > kernel_filename = initrd_filename = kernel_cmdline = NULL; > > }
On Fri, Jun 15, 2012 at 7:09 PM, Andreas Färber <afaerber@suse.de> wrote: > > Am 15.06.2012 05:06, schrieb Li Zhang: > > > > > > On Thu, Jun 14, 2012 at 10:27 PM, Andreas Färber <afaerber@suse.de > > <mailto:afaerber@suse.de>> wrote: > > > > Am 14.06.2012 07:17, schrieb zhlcindy@gmail.com > > <mailto:zhlcindy@gmail.com>: > > > From: Li Zhang <zhlcindy@linux.vnet.ibm.com > > <mailto:zhlcindy@linux.vnet.ibm.com>> > > > > > > For pseries machine, it needs to enable usb > > > to add kbd 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_opt is an global option > > > which can be checked by machines. For example, > > > on pseries, it will check if usb_opt is on, if > > > it is on, it will create one usb ohci controller. > > > As the following: > > > if (usb_opts && strcmp(usb_opts, "on") == 0) > > > 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 > > <mailto:zhlcindy@linux.vnet.ibm.com>> > > > reviewed-by: Anthony Liguori <aliguori@us.ibm.com > > <mailto:aliguori@us.ibm.com>> > > > reviewed-by: Benjamin Herrenschmidt <benh@au1.ibm.com > > <mailto:benh@au1.ibm.com>> > > > --- > > > qemu-config.c | 4 ++++ > > > sysemu.h | 1 + > > > vl.c | 12 ++++++++++++ > > > 3 files changed, 17 insertions(+) > > > > > > diff --git a/qemu-config.c b/qemu-config.c > > > index bb3bff4..258712a 100644 > > > --- a/qemu-config.c > > > +++ b/qemu-config.c > > > @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = { > > > .name = "dtb", > > > .type = QEMU_OPT_STRING, > > > .help = "Linux kernel device tree file", > > > + }, { > > > + .name = "usb", > > > + .type = QEMU_OPT_BOOL, > > > + .help = "Set on/off to enable/disable usb", > > > }, > > > { /* End of list */ } > > > }, > > > diff --git a/sysemu.h b/sysemu.h > > > index bc2c788..c5ea10d 100644 > > > --- a/sysemu.h > > > +++ b/sysemu.h > > > @@ -13,6 +13,7 @@ > > > /* vl.c */ > > > > > > extern const char *bios_name; > > > +extern const char *usb_opt; > > > > > > extern const char *qemu_name; > > > extern uint8_t qemu_uuid[]; > > > diff --git a/vl.c b/vl.c > > > index 204d85b..10f8e4c 100644 > > > --- a/vl.c > > > +++ b/vl.c > > > @@ -171,6 +171,7 @@ int main(int argc, char **argv) > > > > > > static const char *data_dir; > > > const char *bios_name = NULL; > > > +const char *usb_opt = NULL; > > > enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB; > > > DisplayType display_type = DT_DEFAULT; > > > int display_remote = 0; > > [...] > > > The point of using machine options is so that you can use the > > QemuOpts > > infrastructure to inquire this value, not to save more global state. > > > > OK, I think it still needs one global value because other machines need > > to check whether usb is enabled. > > > > > > Especially not a string when all you want is a boolean value. > > > > From the qemu, QEMU_OPT_BOOL type still use a string > > "on/off" to enable/disable usb. > > Maybe it's better to convert it to boolean value. > > > > Further, in this patch it's only being assigned, not used anywhere. > > > > In vl.c, it seems that it is not used. > > I have been thinking to use it in spapr.c. > > > > if (usb_opts && strcmp(usb_opts, "on") == 0) > > pci_create_simple(bus, -1, "pci-ohci"); > > > > But I want to see whether this way is accepted. > > If it is ok, I will add this option to spapr.c. > > First, please don't reply with HTML mails, it breaks the quoting as you > can see. Thanks for your reminding. :) > You don't seem to be getting what I'm saying: As discussed in great > lengths for how to access a passed-in device tree for ARM, the code > checking this option (i.e., in spapr.c) is expected to call > qemu_opt_get() itself, instead of saving its value globally in vl.c. For > example: > > machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0); > usb_opt = qemu_opt_get(machine_opts, "usb"); > if (strcmp(usb_opt, "off") != 0) { > // set up OHCI > } > I see. I just thought if every machine has the same code, it can be done in vl.c. Isn't the global variable good ? > with appropriate NULL checks left as exercise for the reader. > > So you should combine the definition of the option in qemu-config.c > (which looks okay) with its actual use in hw/spapr.c in one patch. > > Once introduced, hw/ppc_newworld.c should be updated to check the new > option as well, but that can be a follow-up patch. > > Regards, > Andreas > > > > @@ -758,6 +759,15 @@ static int bt_parse(const char *opt) > > > return 1; > > > } > > > > > > +static int default_enable_usb(QemuOpts *opts) > > > +{ > > > + if (NULL == qemu_opt_get(opts, "usb")) { > > > + qemu_opt_set(opts, "usb", "on"); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > /***********************************************************/ > > > /* QEMU Block devices */ > > > > > > @@ -3356,6 +3366,8 @@ 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"); > > > + default_enable_usb(machine_opts); > > > + usb_opt = qemu_opt_get(machine_opts, "usb"); > > > } else { > > > kernel_filename = initrd_filename = kernel_cmdline = > > NULL; > > > } > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -- Best Regards -Li
On 06/15/2012 02:46 PM, Li Zhang wrote: > On Fri, Jun 15, 2012 at 7:09 PM, Andreas Färber<afaerber@suse.de> wrote: >> >> You don't seem to be getting what I'm saying: As discussed in great >> lengths for how to access a passed-in device tree for ARM, the code >> checking this option (i.e., in spapr.c) is expected to call >> qemu_opt_get() itself, instead of saving its value globally in vl.c. For >> example: >> >> machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0); >> usb_opt = qemu_opt_get(machine_opts, "usb"); >> if (strcmp(usb_opt, "off") != 0) { >> // set up OHCI >> } >> > I see. > I just thought if every machine has the same code, it can be done in vl.c. > Isn't the global variable good ? No, global variables are always bad :). If you see code duplication, just throw the interesting bit into a static inline function in a header and have the users call that. Alex
On Thu, Jun 28, 2012 at 9:22 PM, Alexander Graf <agraf@suse.de> wrote: > On 06/15/2012 02:46 PM, Li Zhang wrote: >> >> On Fri, Jun 15, 2012 at 7:09 PM, Andreas Färber<afaerber@suse.de> wrote: >>> >>> >>> You don't seem to be getting what I'm saying: As discussed in great >>> lengths for how to access a passed-in device tree for ARM, the code >>> checking this option (i.e., in spapr.c) is expected to call >>> qemu_opt_get() itself, instead of saving its value globally in vl.c. For >>> example: >>> >>> machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0); >>> usb_opt = qemu_opt_get(machine_opts, "usb"); >>> if (strcmp(usb_opt, "off") != 0) { >>> // set up OHCI >>> } >>> >> I see. >> I just thought if every machine has the same code, it can be done in >> vl.c. >> Isn't the global variable good ? > > > No, global variables are always bad :). If you see code duplication, just > throw the interesting bit into a static inline function in a header and have > the users call that. > Got it. Thanks. :) > > Alex >
diff --git a/qemu-config.c b/qemu-config.c index bb3bff4..258712a 100644 --- a/qemu-config.c +++ b/qemu-config.c @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = { .name = "dtb", .type = QEMU_OPT_STRING, .help = "Linux kernel device tree file", + }, { + .name = "usb", + .type = QEMU_OPT_BOOL, + .help = "Set on/off to enable/disable usb", }, { /* End of list */ } }, diff --git a/sysemu.h b/sysemu.h index bc2c788..c5ea10d 100644 --- a/sysemu.h +++ b/sysemu.h @@ -13,6 +13,7 @@ /* vl.c */ extern const char *bios_name; +extern const char *usb_opt; extern const char *qemu_name; extern uint8_t qemu_uuid[]; diff --git a/vl.c b/vl.c index 204d85b..10f8e4c 100644 --- a/vl.c +++ b/vl.c @@ -171,6 +171,7 @@ int main(int argc, char **argv) static const char *data_dir; const char *bios_name = NULL; +const char *usb_opt = NULL; enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB; DisplayType display_type = DT_DEFAULT; int display_remote = 0; @@ -758,6 +759,15 @@ static int bt_parse(const char *opt) return 1; } +static int default_enable_usb(QemuOpts *opts) +{ + if (NULL == qemu_opt_get(opts, "usb")) { + qemu_opt_set(opts, "usb", "on"); + } + + return 0; +} + /***********************************************************/ /* QEMU Block devices */ @@ -3356,6 +3366,8 @@ 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"); + default_enable_usb(machine_opts); + usb_opt = qemu_opt_get(machine_opts, "usb"); } else { kernel_filename = initrd_filename = kernel_cmdline = NULL; }