[Qemu-devel] [PATCH] add --accel option

[Qemu-devel] [PATCH] add --accel option

[Glauber Costa]

Since libvirt dudes seem to prefer it, add a --accel option. For now,
it only does the same as --enable-kvm, but it can easily be extended
in the future.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu-options.hx |    7 +++++++
 vl.c            |    7 ++++++-
 2 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 1b420a3..990513f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1429,6 +1429,13 @@ Enable KQEMU kernel module usage. KQEMU options are only available if
 KQEMU support is enabled when compiling.
 ETEXI
 
+DEF("accel", HAS_ARG, QEMU_OPTION_accel, \
+    "-accel name	enable a given accelerator module\n")
+STEXI
+@item -accel @var{name}
+Enable a given accelerator module, for instance, KVM.
+ETEXI
+
 #ifdef CONFIG_KVM
 DEF("enable-kvm", 0, QEMU_OPTION_enable_kvm, \
     "-enable-kvm     enable KVM full virtualization support\n")
diff --git a/vl.c b/vl.c
index bb56644..e03f3af 100644
--- a/vl.c
+++ b/vl.c
@@ -3091,7 +3091,6 @@ static int ram_save_live(QEMUFile *f, int stage, void *opaque)
         while (ram_save_block(f) != 0) {
             bytes_transferred += TARGET_PAGE_SIZE;
         }
-        cpu_physical_memory_set_dirty_tracking(0);
     }
 
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
@@ -5431,6 +5430,12 @@ int main(int argc, char **argv, char **envp)
                 kqemu_allowed = 2;
                 break;
 #endif
+            case QEMU_OPTION_accel:
+                if (strstart(optarg, "kvm", NULL)) {
+                    kvm_allowed = 1;
+                    kqemu_allowed = 0;
+                }
+                break;
 #ifdef CONFIG_KVM
             case QEMU_OPTION_enable_kvm:
                 kvm_allowed = 1;
-- 
1.6.2.2

[Qemu-devel] Re: [PATCH] add --accel option

[Anthony Liguori]

Glauber Costa wrote:
> Since libvirt dudes seem to prefer it, add a --accel option. For now,
> it only does the same as --enable-kvm, but it can easily be extended
> in the future.
>
> Signed-off-by: Glauber Costa <glommer@redhat.com>
>   

To be useful, it should also include kqemu and a none option.

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] add --accel option

[Glauber Costa]

On Tue, Jul 28, 2009 at 03:27:19PM -0500, Anthony Liguori wrote:
> Glauber Costa wrote:
>> Since libvirt dudes seem to prefer it, add a --accel option. For now,
>> it only does the same as --enable-kvm, but it can easily be extended
>> in the future.
>>
>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>   
>
> To be useful, it should also include kqemu and a none option.
yeah, agree. This can be added later (even now!). I just wanted to signal it
out that it will happen, because it will allow us to change qemu-kvm semantics in a
positive way. I can redo the patch or send an incremental, whatever you prefer

[Qemu-devel] [PATCH] add --accel option

[Glauber Costa]

Since libvirt dudes seem to prefer it, add a --accel option. For now,
it only does the same as --enable-kvm, but it can easily be extended
in the future. It also accepts "none" and "kqemu" options.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu-options.hx |    7 +++++++
 vl.c            |   26 +++++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 1b420a3..990513f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1429,6 +1429,13 @@ Enable KQEMU kernel module usage. KQEMU options are only available if
 KQEMU support is enabled when compiling.
 ETEXI
 
+DEF("accel", HAS_ARG, QEMU_OPTION_accel, \
+    "-accel name	enable a given accelerator module\n")
+STEXI
+@item -accel @var{name}
+Enable a given accelerator module, for instance, KVM.
+ETEXI
+
 #ifdef CONFIG_KVM
 DEF("enable-kvm", 0, QEMU_OPTION_enable_kvm, \
     "-enable-kvm     enable KVM full virtualization support\n")
diff --git a/vl.c b/vl.c
index bb56644..5728715 100644
--- a/vl.c
+++ b/vl.c
@@ -3091,7 +3091,6 @@ static int ram_save_live(QEMUFile *f, int stage, void *opaque)
         while (ram_save_block(f) != 0) {
             bytes_transferred += TARGET_PAGE_SIZE;
         }
-        cpu_physical_memory_set_dirty_tracking(0);
     }
 
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
@@ -5431,6 +5430,31 @@ int main(int argc, char **argv, char **envp)
                 kqemu_allowed = 2;
                 break;
 #endif
+            case QEMU_OPTION_accel:
+            {
+                const char *kqemu_opt;
+                if (strstart(optarg, "kvm", NULL)) {
+                    kvm_allowed = 1;
+                    kqemu_allowed = 0;
+                }
+                if (strstart(optarg, "none", NULL)) {
+                    kvm_allowed = 0;
+                    kqemu_allowed = 0;
+                }
+
+                if (strstart(optarg, "kqemu", &kqemu_opt)) {
+                    kvm_allowed = 0;
+                    if (*kqemu_opt == ',' && strstart(kqemu_opt,",mode=kernel", NULL)) {
+                        kqemu_allowed = 2;
+                    } else if (*kqemu_opt != ',' || strstart(kqemu_opt, ",mode=user", NULL)) {
+                        kqemu_allowed = 1;
+                    } else {
+                        fprintf(stderr, "invalid kqemu mode. use mode=user or mode=kernel\n");
+                        exit(1);
+                    }
+                }
+                break;
+            }
 #ifdef CONFIG_KVM
             case QEMU_OPTION_enable_kvm:
                 kvm_allowed = 1;
-- 
1.6.2.2

Re: [Qemu-devel] [PATCH] add --accel option

[Filip Navara]

On Tue, Jul 28, 2009 at 10:48 PM, Glauber Costa<glommer@redhat.com> wrote:
[snip]
> @@ -3091,7 +3091,6 @@ static int ram_save_live(QEMUFile *f, int stage, void *opaque)
>         while (ram_save_block(f) != 0) {
>             bytes_transferred += TARGET_PAGE_SIZE;
>         }
> -        cpu_physical_memory_set_dirty_tracking(0);
>     }
>
>     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);

This change shouldn't be part of the patch. Otherwise it looks ok.

Best regards,
Filip Navara

Re: [Qemu-devel] [PATCH] add --accel option

[Kevin Wolf]

Glauber Costa schrieb:
> Since libvirt dudes seem to prefer it, add a --accel option. For now,
> it only does the same as --enable-kvm, but it can easily be extended
> in the future. It also accepts "none" and "kqemu" options.

The documentation should include which values are valid for the option.
Guessing that the syntax is -accel kvm is probably possible, but for
-accel kqemu,mode=kernel it's definitely hard.

Kevin

Re: [Qemu-devel] [PATCH] add --accel option

[Glauber Costa]

On Wed, Jul 29, 2009 at 10:16:28AM +0200, Kevin Wolf wrote:
> Glauber Costa schrieb:
> > Since libvirt dudes seem to prefer it, add a --accel option. For now,
> > it only does the same as --enable-kvm, but it can easily be extended
> > in the future. It also accepts "none" and "kqemu" options.
> 
> The documentation should include which values are valid for the option.
> Guessing that the syntax is -accel kvm is probably possible, but for
> -accel kqemu,mode=kernel it's definitely hard.
right now kqemu needs an extra argument.

But very soon kvm will need it too. Like for example controlling knobs
like kvm-irqchip and kvm-pit. So maybe we want a standard way of passing
arguments for all accels?

How about --accel foo,args=bar,bar,bar with the args part being optional ?

An exampel would be --accel kvm,args=userpit,userirqchip (assuming we want the
default to be in the kernel)

Otherwise we're stuck with an evergrowing combination to support

Re: [Qemu-devel] [PATCH] add --accel option

[Kevin Wolf]

Glauber Costa schrieb:
> On Wed, Jul 29, 2009 at 10:16:28AM +0200, Kevin Wolf wrote:
>> Glauber Costa schrieb:
>>> Since libvirt dudes seem to prefer it, add a --accel option. For now,
>>> it only does the same as --enable-kvm, but it can easily be extended
>>> in the future. It also accepts "none" and "kqemu" options.
>> The documentation should include which values are valid for the option.
>> Guessing that the syntax is -accel kvm is probably possible, but for
>> -accel kqemu,mode=kernel it's definitely hard.
> right now kqemu needs an extra argument.
> 
> But very soon kvm will need it too. Like for example controlling knobs
> like kvm-irqchip and kvm-pit. So maybe we want a standard way of passing
> arguments for all accels?
> 
> How about --accel foo,args=bar,bar,bar with the args part being optional ?
> 
> An exampel would be --accel kvm,args=userpit,userirqchip (assuming we want the
> default to be in the kernel)

Why not the usual -accel kvm,userpit=on and so on? When having such
parameters you probably should be using QemuOpts/qemu-option in the end
anyway.

But actually I was just arguing that the parameters need to be
documented, whatever they look like.

Kevin

Re: [Qemu-devel] [PATCH] add --accel option

[Paul Brook]

On Tuesday 28 July 2009, Glauber Costa wrote:
> Since libvirt dudes seem to prefer it, add a --accel option. For now,
> it only does the same as --enable-kvm, but it can easily be extended
> in the future. It also accepts "none" and "kqemu" options.

Having two options do the same thing seems wrong. IMO at least one of them 
should be deprecated and documented as a backward compatibility alias.

Paul

Re: [Qemu-devel] [PATCH] add --accel option

[Anthony Liguori]

Paul Brook wrote:
> On Tuesday 28 July 2009, Glauber Costa wrote:
>   
>> Since libvirt dudes seem to prefer it, add a --accel option. For now,
>> it only does the same as --enable-kvm, but it can easily be extended
>> in the future. It also accepts "none" and "kqemu" options.
>>     
>
> Having two options do the same thing seems wrong. IMO at least one of them 
> should be deprecated and documented as a backward compatibility alias.
>   

Yup.  --enable-kvm/--enable-kqemu/--kernel-kqemu should all be 
deprecated and documented as such.

-- 
Regards,

Anthony Liguori