| Message ID | 1287682587-18642-5-git-send-email-anthony.perard@citrix.com |
|---|---|
| State | New |
| Headers | show |
On 21.10.2010, at 19:36, Anthony.Perard@citrix.com wrote: > From: Anthony PERARD <anthony.perard@citrix.com> > > This option gives the ability to switch one "accelerator" like kvm, xen > or the default one tcg. We can specify more than one accelerator by > separate them by a comma. QEMU will try each one and use the first whose > works. > > So, > > -accel xen,kvm,tcg > > which would try Xen support first, then KVM and finaly tcg if none of > the other works. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> This patch is completely independent of the other patches. Anthony, can we pull it in separately before the others? Acked-by: Alexander Graf <agraf@suse.de>
On 21.10.2010, at 19:36, Anthony.Perard@citrix.com wrote: > From: Anthony PERARD <anthony.perard@citrix.com> > > This option gives the ability to switch one "accelerator" like kvm, xen > or the default one tcg. We can specify more than one accelerator by > separate them by a comma. QEMU will try each one and use the first whose > works. > > So, > > -accel xen,kvm,tcg > > which would try Xen support first, then KVM and finaly tcg if none of > the other works. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > qemu-options.hx | 10 ++++++ > vl.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++------- > 2 files changed, 85 insertions(+), 11 deletions(-) > > diff --git a/qemu-options.hx b/qemu-options.hx > index 718d47a..ba8385b 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -1925,6 +1925,16 @@ Enable KVM full virtualization support. This option is only available > if KVM support is enabled when compiling. > ETEXI > > +DEF("accel", HAS_ARG, QEMU_OPTION_accel, \ > + "-accel accel use an accelerator (kvm,xen,tcg), default is tcg\n", QEMU_ARCH_ALL) > +STEXI > +@item -accel @var{accel}[,@var{accel}[,...]] > +@findex -accel > +This is use to enable an accelerator, in kvm,xen,tcg. > +By default, it use only tcg. If there a more than one accelerator > +specified, the next one is used if the first don't work. > +ETEXI > + > DEF("xen-domid", HAS_ARG, QEMU_OPTION_xen_domid, > "-xen-domid id specify xen guest domain id\n", QEMU_ARCH_ALL) > DEF("xen-create", 0, QEMU_OPTION_xen_create, > diff --git a/vl.c b/vl.c > index df414ef..40a26ee 100644 > --- a/vl.c > +++ b/vl.c > @@ -1750,6 +1750,74 @@ static int debugcon_parse(const char *devname) > return 0; > } > > +static struct { > + const char *opt_name; > + const char *name; > + int (*available)(void); > + int (*init)(int smp_cpus); > + int *allowed; > +} accel_list[] = { > + { "tcg", "tcg", NULL, NULL, NULL }, > + { "kvm", "KVM", kvm_available, kvm_init, &kvm_allowed }, Thinking about this a bit more ... kvm_available is a function pointer that gets #defined to (0) when we don't have KVM available. I can imagine some compiler might throw a warning on us for this one day. Is there a valid reason not to do static inline int kvm_enabled() { #ifdef CONFIG_KVM return kvm_allowed; #else return 0; #endif } That should compile into the exact same code but be valid for function pointers. > +}; > + > +static int accel_parse_init(const char *opts) > +{ > + const char *p = opts; > + char buf[10]; > + int i, ret; > + bool accel_initalised = 0; > + bool init_failed = 0; > + > + while (!accel_initalised && *p != '\0') { > + if (*p == ',') { > + p++; > + } > + p = get_opt_name(buf, sizeof (buf), p, ','); > + for (i = 0; i < ARRAY_SIZE(accel_list); i++) { > + if (strcmp(accel_list[i].opt_name, buf) == 0) { > + if (accel_list[i].init) { If you'd make the function pointers mandatory, you could also get rid of a lot of checks here. Just introduce a new small stub helper for tcg_init and tcg_allowed and always call the pointers. I take back my Acked-by. Sorry, I guess I should have read things in more detail first O_o. I still believe that this patch can go in before the others, but I'd at least like to see some comments on the (0) pointer thing :). Alex
On 11/15/2010 11:46 AM, Alexander Graf wrote: > I take back my Acked-by. Sorry, I guess I should have read things in > more detail first O_o. I still believe that this patch can go in > before the others, but I'd at least like to see some comments on the > (0) pointer thing:). I agree, it kind of works by chance, (0) works but (1) wouldn't. Better to define the function always. Paolo
On Mon, 15 Nov 2010, Alexander Graf wrote: > > On 21.10.2010, at 19:36, Anthony.Perard@citrix.com wrote: > > > From: Anthony PERARD <anthony.perard@citrix.com> > > > > This option gives the ability to switch one "accelerator" like kvm, xen > > or the default one tcg. We can specify more than one accelerator by > > separate them by a comma. QEMU will try each one and use the first whose > > works. > > > > So, > > > > -accel xen,kvm,tcg > > > > which would try Xen support first, then KVM and finaly tcg if none of > > the other works. > > > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > --- > > qemu-options.hx | 10 ++++++ > > vl.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++------- > > 2 files changed, 85 insertions(+), 11 deletions(-) > > > > diff --git a/qemu-options.hx b/qemu-options.hx > > index 718d47a..ba8385b 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -1925,6 +1925,16 @@ Enable KVM full virtualization support. This option is only available > > if KVM support is enabled when compiling. > > ETEXI > > > > +DEF("accel", HAS_ARG, QEMU_OPTION_accel, \ > > + "-accel accel use an accelerator (kvm,xen,tcg), default is tcg\n", QEMU_ARCH_ALL) > > +STEXI > > +@item -accel @var{accel}[,@var{accel}[,...]] > > +@findex -accel > > +This is use to enable an accelerator, in kvm,xen,tcg. > > +By default, it use only tcg. If there a more than one accelerator > > +specified, the next one is used if the first don't work. > > +ETEXI > > + > > DEF("xen-domid", HAS_ARG, QEMU_OPTION_xen_domid, > > "-xen-domid id specify xen guest domain id\n", QEMU_ARCH_ALL) > > DEF("xen-create", 0, QEMU_OPTION_xen_create, > > diff --git a/vl.c b/vl.c > > index df414ef..40a26ee 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -1750,6 +1750,74 @@ static int debugcon_parse(const char *devname) > > return 0; > > } > > > > +static struct { > > + const char *opt_name; > > + const char *name; > > + int (*available)(void); > > + int (*init)(int smp_cpus); > > + int *allowed; > > +} accel_list[] = { > > + { "tcg", "tcg", NULL, NULL, NULL }, > > + { "kvm", "KVM", kvm_available, kvm_init, &kvm_allowed }, > > Thinking about this a bit more ... > > kvm_available is a function pointer that gets #defined to (0) when we don't have KVM available. I can imagine some compiler might throw a warning on us for this one day. > > Is there a valid reason not to do > > static inline int kvm_enabled() > { > #ifdef CONFIG_KVM > return kvm_allowed; > #else > return 0; > #endif > } > > That should compile into the exact same code but be valid for function pointers. I will do this change, as well as for the two others patches. > > +}; > > + > > +static int accel_parse_init(const char *opts) > > +{ > > + const char *p = opts; > > + char buf[10]; > > + int i, ret; > > + bool accel_initalised = 0; > > + bool init_failed = 0; > > + > > + while (!accel_initalised && *p != '\0') { > > + if (*p == ',') { > > + p++; > > + } > > + p = get_opt_name(buf, sizeof (buf), p, ','); > > + for (i = 0; i < ARRAY_SIZE(accel_list); i++) { > > + if (strcmp(accel_list[i].opt_name, buf) == 0) { > > + if (accel_list[i].init) { > > If you'd make the function pointers mandatory, you could also get rid of a lot of checks here. Just introduce a new small stub helper for tcg_init and tcg_allowed and always call the pointers. Yes, this will make the code more readable. > I take back my Acked-by. Sorry, I guess I should have read things in more detail first O_o. I still believe that this patch can go in before the others, but I'd at least like to see some comments on the (0) pointer thing :). And I will send the patch out of the patch series. Thanks,
On Mon, 15 Nov 2010, Anthony PERARD wrote: > On Mon, 15 Nov 2010, Alexander Graf wrote: > > > > > On 21.10.2010, at 19:36, Anthony.Perard@citrix.com wrote: > > > > > From: Anthony PERARD <anthony.perard@citrix.com> > > > > > > This option gives the ability to switch one "accelerator" like kvm, xen > > > or the default one tcg. We can specify more than one accelerator by > > > separate them by a comma. QEMU will try each one and use the first whose > > > works. > > > > > > So, > > > > > > -accel xen,kvm,tcg > > > > > > which would try Xen support first, then KVM and finaly tcg if none of > > > the other works. > > > > > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > > --- > > > qemu-options.hx | 10 ++++++ > > > vl.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++------- > > > 2 files changed, 85 insertions(+), 11 deletions(-) > > > > > > diff --git a/vl.c b/vl.c > > > index df414ef..40a26ee 100644 > > > --- a/vl.c > > > +++ b/vl.c > > > @@ -1750,6 +1750,74 @@ static int debugcon_parse(const char *devname) > > > return 0; > > > } > > > > > > +static struct { > > > + const char *opt_name; > > > + const char *name; > > > + int (*available)(void); > > > + int (*init)(int smp_cpus); > > > + int *allowed; > > > +} accel_list[] = { > > > + { "tcg", "tcg", NULL, NULL, NULL }, > > > + { "kvm", "KVM", kvm_available, kvm_init, &kvm_allowed }, > > > > Thinking about this a bit more ... > > > > kvm_available is a function pointer that gets #defined to (0) when we don't have KVM available. I can imagine some compiler might throw a warning on us for this one day. > > > > Is there a valid reason not to do > > > > static inline int kvm_enabled() > > { > > #ifdef CONFIG_KVM > > return kvm_allowed; > > #else > > return 0; > > #endif > > } > > > > That should compile into the exact same code but be valid for function pointers. > > I will do this change, as well as for the two others patches. Actually, kvm_available is already a function, not a define. kvm_enable can be change from define to function, but not in this patch, because I don't use it.
On 15.11.2010, at 15:47, Anthony PERARD wrote: > On Mon, 15 Nov 2010, Anthony PERARD wrote: > >> On Mon, 15 Nov 2010, Alexander Graf wrote: >> >>> >>> On 21.10.2010, at 19:36, Anthony.Perard@citrix.com wrote: >>> >>>> From: Anthony PERARD <anthony.perard@citrix.com> >>>> >>>> This option gives the ability to switch one "accelerator" like kvm, xen >>>> or the default one tcg. We can specify more than one accelerator by >>>> separate them by a comma. QEMU will try each one and use the first whose >>>> works. >>>> >>>> So, >>>> >>>> -accel xen,kvm,tcg >>>> >>>> which would try Xen support first, then KVM and finaly tcg if none of >>>> the other works. >>>> >>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> >>>> --- >>>> qemu-options.hx | 10 ++++++ >>>> vl.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++------- >>>> 2 files changed, 85 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/vl.c b/vl.c >>>> index df414ef..40a26ee 100644 >>>> --- a/vl.c >>>> +++ b/vl.c >>>> @@ -1750,6 +1750,74 @@ static int debugcon_parse(const char *devname) >>>> return 0; >>>> } >>>> >>>> +static struct { >>>> + const char *opt_name; >>>> + const char *name; >>>> + int (*available)(void); >>>> + int (*init)(int smp_cpus); >>>> + int *allowed; >>>> +} accel_list[] = { >>>> + { "tcg", "tcg", NULL, NULL, NULL }, >>>> + { "kvm", "KVM", kvm_available, kvm_init, &kvm_allowed }, >>> >>> Thinking about this a bit more ... >>> >>> kvm_available is a function pointer that gets #defined to (0) when we don't have KVM available. I can imagine some compiler might throw a warning on us for this one day. >>> >>> Is there a valid reason not to do >>> >>> static inline int kvm_enabled() >>> { >>> #ifdef CONFIG_KVM >>> return kvm_allowed; >>> #else >>> return 0; >>> #endif >>> } >>> >>> That should compile into the exact same code but be valid for function pointers. >> >> I will do this change, as well as for the two others patches. > > Actually, kvm_available is already a function, not a define. > > kvm_enable can be change from define to function, but not in this patch, > because I don't use it. Oh no worries for stuff you don't use. As long as kvm_available is a function, just make sure that xen_available is one too :). The main incentive is to get rid of all those if (!x->available) parts, as we can guarantee there to always be a function behind. Treating tcg the same way would also help in the long term for non-tcg build of qemu which some people are interested in ;). Alex
diff --git a/qemu-options.hx b/qemu-options.hx index 718d47a..ba8385b 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1925,6 +1925,16 @@ Enable KVM full virtualization support. This option is only available if KVM support is enabled when compiling. ETEXI +DEF("accel", HAS_ARG, QEMU_OPTION_accel, \ + "-accel accel use an accelerator (kvm,xen,tcg), default is tcg\n", QEMU_ARCH_ALL) +STEXI +@item -accel @var{accel}[,@var{accel}[,...]] +@findex -accel +This is use to enable an accelerator, in kvm,xen,tcg. +By default, it use only tcg. If there a more than one accelerator +specified, the next one is used if the first don't work. +ETEXI + DEF("xen-domid", HAS_ARG, QEMU_OPTION_xen_domid, "-xen-domid id specify xen guest domain id\n", QEMU_ARCH_ALL) DEF("xen-create", 0, QEMU_OPTION_xen_create, diff --git a/vl.c b/vl.c index df414ef..40a26ee 100644 --- a/vl.c +++ b/vl.c @@ -1750,6 +1750,74 @@ static int debugcon_parse(const char *devname) return 0; } +static struct { + const char *opt_name; + const char *name; + int (*available)(void); + int (*init)(int smp_cpus); + int *allowed; +} accel_list[] = { + { "tcg", "tcg", NULL, NULL, NULL }, + { "kvm", "KVM", kvm_available, kvm_init, &kvm_allowed }, +}; + +static int accel_parse_init(const char *opts) +{ + const char *p = opts; + char buf[10]; + int i, ret; + bool accel_initalised = 0; + bool init_failed = 0; + + while (!accel_initalised && *p != '\0') { + if (*p == ',') { + p++; + } + p = get_opt_name(buf, sizeof (buf), p, ','); + for (i = 0; i < ARRAY_SIZE(accel_list); i++) { + if (strcmp(accel_list[i].opt_name, buf) == 0) { + if (accel_list[i].init) { + ret = accel_list[i].init(smp_cpus); + } else { + ret = 0; + } + if (ret < 0) { + init_failed = 1; + if (!accel_list[i].available()) { + printf("%s not supported for this target\n", + accel_list[i].name); + } else { + fprintf(stderr, "failed to initialize %s: %s\n", + accel_list[i].name, + strerror(-ret)); + } + } else { + accel_initalised = 1; + if (accel_list[i].allowed) { + *(accel_list[i].allowed) = 1; + } + } + break; + } + } + if (i == ARRAY_SIZE(accel_list) + 1) { + fprintf(stderr, "\"%s\" accelerator does not exist.\n", buf); + exit(1); + } + } + + if (!accel_initalised) { + fprintf(stderr, "No accelerator found!\n"); + exit(1); + } + + if (init_failed) { + fprintf(stderr, "Back to %s accelerator.\n", accel_list[i].name); + } + + return !accel_initalised; +} + void qemu_add_exit_notifier(Notifier *notify) { notifier_list_add(&exit_notifiers, notify); @@ -1829,6 +1897,7 @@ int main(int argc, char **argv, char **envp) const char *incoming = NULL; int show_vnc_port = 0; int defconfig = 1; + const char *accel_list_opts = "tcg"; #ifdef CONFIG_SIMPLE_TRACE const char *trace_file = NULL; @@ -2444,7 +2513,10 @@ int main(int argc, char **argv, char **envp) do_smbios_option(optarg); break; case QEMU_OPTION_enable_kvm: - kvm_allowed = 1; + accel_list_opts = "kvm"; + break; + case QEMU_OPTION_accel: + accel_list_opts = optarg; break; case QEMU_OPTION_usb: usb_enabled = 1; @@ -2754,16 +2826,8 @@ int main(int argc, char **argv, char **envp) exit(1); } - if (kvm_allowed) { - int ret = kvm_init(smp_cpus); - if (ret < 0) { - if (!kvm_available()) { - printf("KVM not supported for this target\n"); - } else { - fprintf(stderr, "failed to initialize KVM: %s\n", strerror(-ret)); - } - exit(1); - } + if (accel_list_opts) { + accel_parse_init(accel_list_opts); } if (qemu_init_main_loop()) {