Re: [PATCH] qemu-kvm: Switch to upstream -enable-kvm semantics

[Anthony Liguori <anthony@codemonkey.ws>]

On 12/15/2010 09:50 AM, Markus Armbruster wrote:
> We currently enable KVM by default, and when it's not available, we
> print a message and fall back to TCG.  Option -enable-kvm is ignored.
> Option -no-kvm suppresses KVM.
>
> Upstream works differently: KVM is off by default, -enable-kvm
> switches it on.  -enable-kvm terminates the process unsuccessfully if
> KVM is not available.
>
> upstream qemu   |  default  |-enable-kvm
> ----------------+-----------+-----------
> KVM available   | disabled  |  enabled
> KVM unavailable | disabled  |    fail
>
> qemu-kvm        |  default  |-enable-kvm
> ----------------+-----------+-----------
> KVM available   |  enabled* |  enabled
> KVM unavailable | disabled  | disabled*
>
> * differs from upstream
>
> Users of qemu and qemu-kvm need to be aware of these differences to
> enable / disable use of KVM reliably.  This is bothersome.
>
> Consider -enable-kvm when KVM is unavailable: If the user expects
> qemu-kvm behavior (fall back), but qemu fails, he'll likely be
> surprised and unhappy.  If the user expects upstream behavior (fail),
> but qemu-kvm falls back to TCG, the guest runs slow as molasses, and
> the user will likely be confused and unhappy (unless he spots and
> understands the "disable KVM" message).
>
> Switch to upstream semantics: KVM off by default, -enable-kvm switches
> it on, and when it can't, it's fatal.
>
> Having to enable KVM explicitly is annoying, but the proper place to
> address that is upstream.
>
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>    

Backwards compatibility is going to kill us if we try to make this change.

Current qemu-kvm behavior:

default: -accel kvm,tcg
-no-kvm: -accel tcg
-enable-kvm: -accel kvm,tcg

Current upstream behavior

default: -accel tcg
-enable-kvm: -accel kvm

I think we should tie `-accel' to the machine type.  For qemu-kvm, a 
different default machine type should be used than upstream qemu (it 
really should be a configure switch).

For `pc', the default `-accel' behavior should remain 'tcg'.  For 
`kvmpc', the default `-accel' behavior should be 'kvm,tcg'.

-no-kvm should be deprecated.  -enable-kvm should also be deprecated in 
favor of the `-accel' option.

In the short term, it would be a good idea to modify qemu-kvm to switch 
the -enable-kvm semantics to match upstream (fail if KVM isn't 
available).  Adding an alias for 'kvmpc' upstream and qemu-kvm and 
making qemu-kvm default to 'kvmpc' would be helpful for management tools 
too.

Regards,

Anthony Liguori

> ---
>   vl.c |   10 +---------
>   1 files changed, 1 insertions(+), 9 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index e3c8919..87e88c2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -247,7 +247,7 @@ static void *boot_set_opaque;
>   static NotifierList exit_notifiers =
>       NOTIFIER_LIST_INITIALIZER(exit_notifiers);
>
> -int kvm_allowed = 1;
> +int kvm_allowed = 0;
>   uint32_t xen_domid;
>   enum xen_mode xen_mode = XEN_EMULATE;
>
> @@ -2436,10 +2436,8 @@ int main(int argc, char **argv, char **envp)
>               case QEMU_OPTION_smbios:
>                   do_smbios_option(optarg);
>                   break;
> -#ifdef OBSOLETE_KVM_IMPL
>               case QEMU_OPTION_enable_kvm:
>                   kvm_allowed = 1;
> -#endif
>                   break;
>   	    case QEMU_OPTION_no_kvm:
>   		kvm_allowed = 0;
> @@ -2789,18 +2787,12 @@ int main(int argc, char **argv, char **envp)
>       if (kvm_allowed) {
>           int ret = kvm_init(smp_cpus);
>           if (ret<  0) {
> -#if defined(OBSOLETE_KVM_IMPL) || defined(CONFIG_NO_CPU_EMULATION)
>               if (!kvm_available()) {
>                   printf("KVM not supported for this target\n");
>               } else {
>                   fprintf(stderr, "failed to initialize KVM: %s\n", strerror(-ret));
>               }
>               exit(1);
> -#endif
> -#ifdef CONFIG_KVM
> -            fprintf(stderr, "Could not initialize KVM, will disable KVM support\n");
> -            kvm_allowed = 0;
> -#endif
>           }
>       }
>
>