[Qemu-devel] [PATCH] Introduce -accel command option.

[Qemu-devel] [PATCH] Introduce -accel command option.

[anthony.perard]

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>
---
 arch_init.c     |    5 +++
 arch_init.h     |    1 +
 qemu-options.hx |   10 ++++++
 vl.c            |   85 +++++++++++++++++++++++++++++++++++++++++++++++-------
 4 files changed, 90 insertions(+), 11 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 4486925..e0d7a4c 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -639,6 +639,11 @@ int audio_available(void)
 #endif
 }
 
+int tcg_available(void)
+{
+    return 1;
+}
+
 int kvm_available(void)
 {
 #ifdef CONFIG_KVM
diff --git a/arch_init.h b/arch_init.h
index 682890c..f0fb6a0 100644
--- a/arch_init.h
+++ b/arch_init.h
@@ -27,6 +27,7 @@ void do_acpitable_option(const char *optarg);
 void do_smbios_option(const char *optarg);
 void cpudef_init(void);
 int audio_available(void);
+int tcg_available(void);
 int kvm_available(void);
 int xen_available(void);
 
diff --git a/qemu-options.hx b/qemu-options.hx
index 4d99a58..958d126 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1975,6 +1975,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 c58583d..2917e32 100644
--- a/vl.c
+++ b/vl.c
@@ -242,6 +242,7 @@ static void *boot_set_opaque;
 static NotifierList exit_notifiers =
     NOTIFIER_LIST_INITIALIZER(exit_notifiers);
 
+static int tcg_allowed = 1;
 int kvm_allowed = 0;
 uint32_t xen_domid;
 enum xen_mode xen_mode = XEN_EMULATE;
@@ -1723,6 +1724,72 @@ static int debugcon_parse(const char *devname)
     return 0;
 }
 
+static int tcg_init(int smp_cpus)
+{
+    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", tcg_available, tcg_init, &tcg_allowed },
+    { "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) {
+                ret = accel_list[i].init(smp_cpus);
+                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;
+                    *(accel_list[i].allowed) = 1;
+                }
+                break;
+            }
+        }
+        if (i == ARRAY_SIZE(accel_list)) {
+            fprintf(stderr, "\"%s\" accelerator does not exist.\n", buf);
+        }
+    }
+
+    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);
@@ -1802,6 +1869,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;
@@ -2409,7 +2477,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;
@@ -2719,16 +2790,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()) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH] Introduce -accel command option.

[Anthony Liguori]

On 11/15/2010 09:45 AM, 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.
>    

Should use QemuOpts instead of parsing by hand.  I'd rather it be 
presented as a -machine option too with accel=xen:kvm:tcg to specify order.

Regards,

Anthony Liguori

> Signed-off-by: Anthony PERARD<anthony.perard@citrix.com>
> ---
>   arch_init.c     |    5 +++
>   arch_init.h     |    1 +
>   qemu-options.hx |   10 ++++++
>   vl.c            |   85 +++++++++++++++++++++++++++++++++++++++++++++++-------
>   4 files changed, 90 insertions(+), 11 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 4486925..e0d7a4c 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -639,6 +639,11 @@ int audio_available(void)
>   #endif
>   }
>
> +int tcg_available(void)
> +{
> +    return 1;
> +}
> +
>   int kvm_available(void)
>   {
>   #ifdef CONFIG_KVM
> diff --git a/arch_init.h b/arch_init.h
> index 682890c..f0fb6a0 100644
> --- a/arch_init.h
> +++ b/arch_init.h
> @@ -27,6 +27,7 @@ void do_acpitable_option(const char *optarg);
>   void do_smbios_option(const char *optarg);
>   void cpudef_init(void);
>   int audio_available(void);
> +int tcg_available(void);
>   int kvm_available(void);
>   int xen_available(void);
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 4d99a58..958d126 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1975,6 +1975,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 c58583d..2917e32 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -242,6 +242,7 @@ static void *boot_set_opaque;
>   static NotifierList exit_notifiers =
>       NOTIFIER_LIST_INITIALIZER(exit_notifiers);
>
> +static int tcg_allowed = 1;
>   int kvm_allowed = 0;
>   uint32_t xen_domid;
>   enum xen_mode xen_mode = XEN_EMULATE;
> @@ -1723,6 +1724,72 @@ static int debugcon_parse(const char *devname)
>       return 0;
>   }
>
> +static int tcg_init(int smp_cpus)
> +{
> +    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", tcg_available, tcg_init,&tcg_allowed },
> +    { "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) {
> +                ret = accel_list[i].init(smp_cpus);
> +                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;
> +                    *(accel_list[i].allowed) = 1;
> +                }
> +                break;
> +            }
> +        }
> +        if (i == ARRAY_SIZE(accel_list)) {
> +            fprintf(stderr, "\"%s\" accelerator does not exist.\n", buf);
> +        }
> +    }
> +
> +    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);
> @@ -1802,6 +1869,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;
> @@ -2409,7 +2477,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;
> @@ -2719,16 +2790,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()) {
>    

Re: [Qemu-devel] [PATCH] Introduce -accel command option.

[Anthony PERARD]

On Mon, 15 Nov 2010, Anthony Liguori wrote:

> On 11/15/2010 09:45 AM, 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.
> >
>
> Should use QemuOpts instead of parsing by hand.

Ok for that.

> I'd rather it be
> presented as a -machine option too with accel=xen:kvm:tcg to specify order.

This is not clear to me, did you mean that you prefer to have both
"-accel accels" and "-machine accel=accels" options?

For the -machine options, I can put it in qemu-config.c and it will be
saved.

Regards,

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH] Introduce -accel command option.

[Anthony Liguori]

On 11/16/2010 10:10 AM, Anthony PERARD wrote:
> On Mon, 15 Nov 2010, Anthony Liguori wrote:
>
>    
>> On 11/15/2010 09:45 AM, 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.
>>>
>>>        
>> Should use QemuOpts instead of parsing by hand.
>>      
> Ok for that.
>
>    
>> I'd rather it be
>> presented as a -machine option too with accel=xen:kvm:tcg to specify order.
>>      
> This is not clear to me, did you mean that you prefer to have both
> "-accel accels" and "-machine accel=accels" options?
>    

Just -machine accel=accels.

Part of my rational is that accelerator is a machine property.  If you 
do -M xenpv it ought to imply -machine accel=xen.

Regards,

Anthony Liguori

> For the -machine options, I can put it in qemu-config.c and it will be
> saved.
>
> Regards,
>
>    

Re: [Qemu-devel] [PATCH] Introduce -accel command option.

[Daniel P. Berrange]

On Tue, Nov 16, 2010 at 10:41:25AM -0600, Anthony Liguori wrote:
> On 11/16/2010 10:10 AM, Anthony PERARD wrote:
> >On Mon, 15 Nov 2010, Anthony Liguori wrote:
> >
> >   
> >>On 11/15/2010 09:45 AM, 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.
> >>>
> >>>       
> >>Should use QemuOpts instead of parsing by hand.
> >>     
> >Ok for that.
> >
> >   
> >>I'd rather it be
> >>presented as a -machine option too with accel=xen:kvm:tcg to specify 
> >>order.
> >>     
> >This is not clear to me, did you mean that you prefer to have both
> >"-accel accels" and "-machine accel=accels" options?
> >   
> 
> Just -machine accel=accels.
> 
> Part of my rational is that accelerator is a machine property.  If you 
> do -M xenpv it ought to imply -machine accel=xen.

Surely, only if it is running on a Xen Dom0. If you use -M xenpv on a KVM
host, then -M xenpv should imply -machine accel=kvm (ie it would be using
xenner)

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH] Introduce -accel command option.

[Alexander Graf]

On 16.11.2010, at 17:55, Daniel P. Berrange wrote:

> On Tue, Nov 16, 2010 at 10:41:25AM -0600, Anthony Liguori wrote:
>> On 11/16/2010 10:10 AM, Anthony PERARD wrote:
>>> On Mon, 15 Nov 2010, Anthony Liguori wrote:
>>> 
>>> 
>>>> On 11/15/2010 09:45 AM, 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.
>>>>> 
>>>>> 
>>>> Should use QemuOpts instead of parsing by hand.
>>>> 
>>> Ok for that.
>>> 
>>> 
>>>> I'd rather it be
>>>> presented as a -machine option too with accel=xen:kvm:tcg to specify 
>>>> order.
>>>> 
>>> This is not clear to me, did you mean that you prefer to have both
>>> "-accel accels" and "-machine accel=accels" options?
>>> 
>> 
>> Just -machine accel=accels.
>> 
>> Part of my rational is that accelerator is a machine property.  If you 
>> do -M xenpv it ought to imply -machine accel=xen.
> 
> Surely, only if it is running on a Xen Dom0. If you use -M xenpv on a KVM
> host, then -M xenpv should imply -machine accel=kvm (ie it would be using
> xenner)

Actually, it should imply -machine accel=kvm,tcg :). Accelerators really are not a machine property. In an ideal world, -M pc would just work with xen hvm if -accel xen is given.


Alex

Re: [Qemu-devel] [PATCH] Introduce -accel command option.

[Anthony Liguori]

On 11/16/2010 10:59 AM, Alexander Graf wrote:
>
>> Surely, only if it is running on a Xen Dom0. If you use -M xenpv on a KVM
>> host, then -M xenpv should imply -machine accel=kvm (ie it would be using
>> xenner)
>>      
> Actually, it should imply -machine accel=kvm,tcg :). Accelerators really are not a machine property. In an ideal world, -M pc would just work with xen hvm if -accel xen is given.
>    

No, an accelerator is both a CPU selection and a machine 
characteristic.  For KVM, we overload -cpu to modify both the KVM CPU 
and the TCG CPU both this won't work with accel=xen.  We probably 
shouldn't do this with KVM either because there's a significant 
different between trying to do cpuid masking with KVM and modifying the 
TCG cpu emulation support.

Both KVM and Xen have other impacts on the platform devices though.  KVM 
does not support SMM so it disables that in the i440fx.  KVM prefers to 
use it's own in-kernel local APIC (and IOAPIC).  That makes it a 
property of the machine.

Regards,

Anthony Liguori

> Alex
>
>    

Re: [Qemu-devel] [PATCH] Introduce -accel command option.

[Alexander Graf]

On 16.11.2010, at 18:20, Anthony Liguori wrote:

> On 11/16/2010 10:59 AM, Alexander Graf wrote:
>> 
>>> Surely, only if it is running on a Xen Dom0. If you use -M xenpv on a KVM
>>> host, then -M xenpv should imply -machine accel=kvm (ie it would be using
>>> xenner)
>>>     
>> Actually, it should imply -machine accel=kvm,tcg :). Accelerators really are not a machine property. In an ideal world, -M pc would just work with xen hvm if -accel xen is given.
>>   
> 
> No, an accelerator is both a CPU selection and a machine characteristic.  For KVM, we overload -cpu to modify both the KVM CPU and the TCG CPU both this won't work with accel=xen.  We probably shouldn't do this with KVM either because there's a significant different between trying to do cpuid masking with KVM and modifying the TCG cpu emulation support.
> 
> Both KVM and Xen have other impacts on the platform devices though.  KVM does not support SMM so it disables that in the i440fx.  KVM prefers to use it's own in-kernel local APIC (and IOAPIC).  That makes it a property of the machine.

So you're saying the machine should define an accel mask of accels it supports?

However all this ends up internally, giving the user an easy option to choose accels would still be nice. Users don't use -device or -machine. They want shortcuts :).


Alex

Re: [Qemu-devel] [PATCH] Introduce -accel command option.

[Anthony Liguori]

On 11/16/2010 11:24 AM, Alexander Graf wrote:
> On 16.11.2010, at 18:20, Anthony Liguori wrote:
>
>    
>> On 11/16/2010 10:59 AM, Alexander Graf wrote:
>>      
>>>        
>>>> Surely, only if it is running on a Xen Dom0. If you use -M xenpv on a KVM
>>>> host, then -M xenpv should imply -machine accel=kvm (ie it would be using
>>>> xenner)
>>>>
>>>>          
>>> Actually, it should imply -machine accel=kvm,tcg :). Accelerators really are not a machine property. In an ideal world, -M pc would just work with xen hvm if -accel xen is given.
>>>
>>>        
>> No, an accelerator is both a CPU selection and a machine characteristic.  For KVM, we overload -cpu to modify both the KVM CPU and the TCG CPU both this won't work with accel=xen.  We probably shouldn't do this with KVM either because there's a significant different between trying to do cpuid masking with KVM and modifying the TCG cpu emulation support.
>>
>> Both KVM and Xen have other impacts on the platform devices though.  KVM does not support SMM so it disables that in the i440fx.  KVM prefers to use it's own in-kernel local APIC (and IOAPIC).  That makes it a property of the machine.
>>      
> So you're saying the machine should define an accel mask of accels it supports?
>
> However all this ends up internally, giving the user an easy option to choose accels would still be nice. Users don't use -device or -machine. They want shortcuts :).
>    

User's want things to just work.

That's why -M xenpv should imply -machine accel=xen.

A user should never have to specify and accelerator option IMHO.

Regards,

Anthony Liguori

> Alex
>
>    

Re: [Qemu-devel] [PATCH] Introduce -accel command option.

[Alexander Graf]

On 16.11.2010, at 18:27, Anthony Liguori wrote:

> On 11/16/2010 11:24 AM, Alexander Graf wrote:
>> On 16.11.2010, at 18:20, Anthony Liguori wrote:
>> 
>>   
>>> On 11/16/2010 10:59 AM, Alexander Graf wrote:
>>>     
>>>>       
>>>>> Surely, only if it is running on a Xen Dom0. If you use -M xenpv on a KVM
>>>>> host, then -M xenpv should imply -machine accel=kvm (ie it would be using
>>>>> xenner)
>>>>> 
>>>>>         
>>>> Actually, it should imply -machine accel=kvm,tcg :). Accelerators really are not a machine property. In an ideal world, -M pc would just work with xen hvm if -accel xen is given.
>>>> 
>>>>       
>>> No, an accelerator is both a CPU selection and a machine characteristic.  For KVM, we overload -cpu to modify both the KVM CPU and the TCG CPU both this won't work with accel=xen.  We probably shouldn't do this with KVM either because there's a significant different between trying to do cpuid masking with KVM and modifying the TCG cpu emulation support.
>>> 
>>> Both KVM and Xen have other impacts on the platform devices though.  KVM does not support SMM so it disables that in the i440fx.  KVM prefers to use it's own in-kernel local APIC (and IOAPIC).  That makes it a property of the machine.
>>>     
>> So you're saying the machine should define an accel mask of accels it supports?
>> 
>> However all this ends up internally, giving the user an easy option to choose accels would still be nice. Users don't use -device or -machine. They want shortcuts :).
>>   
> 
> User's want things to just work.
> 
> That's why -M xenpv should imply -machine accel=xen.

No, it should imply -machine accel=xen,kvm,tcg. Which should be the default anyways. For machines that don't work with specific accels, I'd agree that a machine should be able to mask those out. But in general, we'll always get back to defaulting to xen,kvm,tcg.

> A user should never have to specify and accelerator option IMHO.

If things don't work out, he should be able to do things like -no-kvm for bug hunting. In normal use cases, I tend to agree. Things should just work automatically.


Alex

Re: [Qemu-devel] [PATCH] Introduce -accel command option.

[Anthony PERARD]

On Tue, 16 Nov 2010, Anthony Liguori wrote:

> On 11/16/2010 11:24 AM, Alexander Graf wrote:
> > On 16.11.2010, at 18:20, Anthony Liguori wrote:
> >
> >
> >> On 11/16/2010 10:59 AM, Alexander Graf wrote:
> >>
> >>>
> >>>> Surely, only if it is running on a Xen Dom0. If you use -M xenpv on a KVM
> >>>> host, then -M xenpv should imply -machine accel=kvm (ie it would be using
> >>>> xenner)
> >>>>
> >>>>
> >>> Actually, it should imply -machine accel=kvm,tcg :). Accelerators really are not a machine property. In an ideal world, -M pc would just work with xen hvm if -accel xen is given.
> >>>
> >>>
> >> No, an accelerator is both a CPU selection and a machine characteristic.  For KVM, we overload -cpu to modify both the KVM CPU and the TCG CPU both this won't work with accel=xen.  We probably shouldn't do this with KVM either because there's a significant different between trying to do cpuid masking with KVM and modifying the TCG cpu emulation support.
> >>
> >> Both KVM and Xen have other impacts on the platform devices though.  KVM does not support SMM so it disables that in the i440fx.  KVM prefers to use it's own in-kernel local APIC (and IOAPIC).  That makes it a property of the machine.
> >>
> > So you're saying the machine should define an accel mask of accels it supports?
> >
> > However all this ends up internally, giving the user an easy option to choose accels would still be nice. Users don't use -device or -machine. They want shortcuts :).
> >
>
> User's want things to just work.
>
> That's why -M xenpv should imply -machine accel=xen.

Actually, it works like that for qemu-xen, we just specify -M xenpv, and
we don't have any --enable-xen or other -accel xen.

> A user should never have to specify and accelerator option IMHO.

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH] Introduce -accel command option.

[Alexander Graf]

On 16.11.2010, at 19:22, Anthony PERARD wrote:

> On Tue, 16 Nov 2010, Anthony Liguori wrote:
> 
>> On 11/16/2010 11:24 AM, Alexander Graf wrote:
>>> On 16.11.2010, at 18:20, Anthony Liguori wrote:
>>> 
>>> 
>>>> On 11/16/2010 10:59 AM, Alexander Graf wrote:
>>>> 
>>>>> 
>>>>>> Surely, only if it is running on a Xen Dom0. If you use -M xenpv on a KVM
>>>>>> host, then -M xenpv should imply -machine accel=kvm (ie it would be using
>>>>>> xenner)
>>>>>> 
>>>>>> 
>>>>> Actually, it should imply -machine accel=kvm,tcg :). Accelerators really are not a machine property. In an ideal world, -M pc would just work with xen hvm if -accel xen is given.
>>>>> 
>>>>> 
>>>> No, an accelerator is both a CPU selection and a machine characteristic.  For KVM, we overload -cpu to modify both the KVM CPU and the TCG CPU both this won't work with accel=xen.  We probably shouldn't do this with KVM either because there's a significant different between trying to do cpuid masking with KVM and modifying the TCG cpu emulation support.
>>>> 
>>>> Both KVM and Xen have other impacts on the platform devices though.  KVM does not support SMM so it disables that in the i440fx.  KVM prefers to use it's own in-kernel local APIC (and IOAPIC).  That makes it a property of the machine.
>>>> 
>>> So you're saying the machine should define an accel mask of accels it supports?
>>> 
>>> However all this ends up internally, giving the user an easy option to choose accels would still be nice. Users don't use -device or -machine. They want shortcuts :).
>>> 
>> 
>> User's want things to just work.
>> 
>> That's why -M xenpv should imply -machine accel=xen.
> 
> Actually, it works like that for qemu-xen, we just specify -M xenpv, and
> we don't have any --enable-xen or other -accel xen.

Yes, and that's exactly the behavior you'd get :). -accel would default to xen,kvm,tcg and thus -M xenpv on xen defaults to real xen pv code, -M xenpv on kvm would default to xenner w/ kvm and -M xenpv without any accelerator available would just fall back to xenner with tcg. Everyone is happy :).


Alex

Re: [Qemu-devel] [PATCH] Introduce -accel command option.

[Anthony Liguori]

On 11/16/2010 12:49 PM, Alexander Graf wrote:
> On 16.11.2010, at 19:22, Anthony PERARD wrote:
>
>    
>> On Tue, 16 Nov 2010, Anthony Liguori wrote:
>>
>>      
>>> On 11/16/2010 11:24 AM, Alexander Graf wrote:
>>>        
>>>> On 16.11.2010, at 18:20, Anthony Liguori wrote:
>>>>
>>>>
>>>>          
>>>>> On 11/16/2010 10:59 AM, Alexander Graf wrote:
>>>>>
>>>>>            
>>>>>>              
>>>>>>> Surely, only if it is running on a Xen Dom0. If you use -M xenpv on a KVM
>>>>>>> host, then -M xenpv should imply -machine accel=kvm (ie it would be using
>>>>>>> xenner)
>>>>>>>
>>>>>>>
>>>>>>>                
>>>>>> Actually, it should imply -machine accel=kvm,tcg :). Accelerators really are not a machine property. In an ideal world, -M pc would just work with xen hvm if -accel xen is given.
>>>>>>
>>>>>>
>>>>>>              
>>>>> No, an accelerator is both a CPU selection and a machine characteristic.  For KVM, we overload -cpu to modify both the KVM CPU and the TCG CPU both this won't work with accel=xen.  We probably shouldn't do this with KVM either because there's a significant different between trying to do cpuid masking with KVM and modifying the TCG cpu emulation support.
>>>>>
>>>>> Both KVM and Xen have other impacts on the platform devices though.  KVM does not support SMM so it disables that in the i440fx.  KVM prefers to use it's own in-kernel local APIC (and IOAPIC).  That makes it a property of the machine.
>>>>>
>>>>>            
>>>> So you're saying the machine should define an accel mask of accels it supports?
>>>>
>>>> However all this ends up internally, giving the user an easy option to choose accels would still be nice. Users don't use -device or -machine. They want shortcuts :).
>>>>
>>>>          
>>> User's want things to just work.
>>>
>>> That's why -M xenpv should imply -machine accel=xen.
>>>        
>> Actually, it works like that for qemu-xen, we just specify -M xenpv, and
>> we don't have any --enable-xen or other -accel xen.
>>      
> Yes, and that's exactly the behavior you'd get :). -accel would default to xen,kvm,tcg

No, that's the wrong default.  The default should be -accel 
tcg,[xen|kvm] if anything.

That said, if tied into QemuOpts, the default can be specified in the 
global config file which will make everyone happy.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] Introduce -accel command option.

[Anthony Liguori]

On 11/16/2010 10:55 AM, Daniel P. Berrange wrote:
>> Just -machine accel=accels.
>>
>> Part of my rational is that accelerator is a machine property.  If you
>> do -M xenpv it ought to imply -machine accel=xen.
>>      
> Surely, only if it is running on a Xen Dom0. If you use -M xenpv on a KVM
> host, then -M xenpv should imply -machine accel=kvm (ie it would be using
> xenner)
>    

Xenner is very different than Xen so I'd rather there be a separate 
machine type.

Too much magic makes debugging difficult because it's wildly different 
code paths even given the same invocation depending on whether you're on 
a dom0, a domU, or a KVM-enabled normal Linux.

Regards,

Anthony Liguori

> Regards,
> Daniel
>