Patchwork [V6,04/15] Introduce -accel command option.

login
register
mail settings
Submitter Anthony PERARD
Date Oct. 21, 2010, 5:36 p.m.
Message ID <1287682587-18642-5-git-send-email-anthony.perard@citrix.com>
Download mbox | patch
Permalink /patch/68749/
State New
Headers show

Comments

Anthony PERARD - Oct. 21, 2010, 5:36 p.m.
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(-)
Alexander Graf - Nov. 15, 2010, 10:38 a.m.
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>
Alexander Graf - Nov. 15, 2010, 10:46 a.m.
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
Paolo Bonzini - Nov. 15, 2010, 12:56 p.m.
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
Anthony PERARD - Nov. 15, 2010, 2:27 p.m.
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,
Anthony PERARD - Nov. 15, 2010, 2:47 p.m.
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.
Alexander Graf - Nov. 15, 2010, 2:51 p.m.
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

Patch

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()) {