[Qemu-devel] [PATCH 0/3] Add svm cpuid features to qemu-kvm

[Qemu-devel] [PATCH 0/3] Add svm cpuid features to qemu-kvm

[Joerg Roedel]

Hi,

this patch-set adds the svm cpuid feature flags to qemu-kvm. With this
patch-set the npt feature-flag can be presented to the guest so that it will
use nested-paging when emulated by the host kvm. Please review, comment and/or
apply these patches :-)

Thanks,

	Joerg

[Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Joerg Roedel]

This patch changes the setting logic for the svm bit in
qemu-kvm. The bit is now explicitly set on -enable-nesting
instead of masked out if the parameter is not supplied.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 target-i386/cpuid.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index d63fdcb..5fa0dd0 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -276,8 +276,8 @@ static x86_def_t builtin_x86_defs[] = {
         .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_CX16 | CPUID_EXT_POPCNT,
         .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) |
             CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
-        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
-            CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
+        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_ABM |
+                         CPUID_EXT3_SSE4A,
         .xlevel = 0x8000000A,
         .model_id = "QEMU Virtual CPU version " QEMU_VERSION,
     },
@@ -303,8 +303,8 @@ static x86_def_t builtin_x86_defs[] = {
                     CPUID_EXT3_CR8LEG,
                     CPUID_EXT3_MISALIGNSSE, CPUID_EXT3_3DNOWPREFETCH,
                     CPUID_EXT3_OSVW, CPUID_EXT3_IBS */
-        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
-            CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
+        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_ABM |
+                         CPUID_EXT3_SSE4A,
         .xlevel = 0x8000001A,
         .model_id = "AMD Phenom(tm) 9550 Quad-Core Processor"
     },
@@ -1154,8 +1154,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             /* disable CPU features that KVM cannot support */
 
             /* svm */
-            if (!kvm_nested)
-                *ecx &= ~CPUID_EXT3_SVM;
+            if (kvm_nested)
+                *ecx |= CPUID_EXT3_SVM;
             /* 3dnow */
             *edx &= ~0xc0000000;
         }
-- 
1.7.0.4

[Qemu-devel] [PATCH 2/3] qemu-kvm: Set cpuid definition to 0 before initializing it

[Joerg Roedel]

This patch cleans the (stack-allocated) cpuid definition to
0 before actually initializing it.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 target-i386/cpuid.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index 5fa0dd0..5735795 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -788,6 +788,8 @@ int cpu_x86_register (CPUX86State *env, const char *cpu_model)
 {
     x86_def_t def1, *def = &def1;
 
+    memset(def, 0, sizeof(*def));
+
     if (cpu_x86_find_by_name(def, cpu_model) < 0)
         return -1;
     if (def->vendor1) {
-- 
1.7.0.4

[Qemu-devel] [PATCH 3/3] qemu-kvm: Add svm cpuid features

[Joerg Roedel]

This patch adds the svm cpuid feature flags to the qemu
intialization path.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 target-i386/cpu.h   |    1 +
 target-i386/cpuid.c |   50 +++++++++++++++++++++++++++++++++++++++++---------
 target-i386/kvm.c   |    3 +++
 3 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 8b6efed..d84804e 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -718,6 +718,7 @@ typedef struct CPUX86State {
     uint8_t has_error_code;
     uint32_t sipi_vector;
     uint32_t cpuid_kvm_features;
+    uint32_t cpuid_svm_features;
     
     /* in order to simplify APIC support, we leave this pointer to the
        user */
diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index 5735795..30e3c09 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -79,6 +79,17 @@ static const char *kvm_feature_name[] = {
     NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
 };
 
+static const char *svm_feature_name[] = {
+    "npt", "lbrv", "svm_lock", "nrip_save",
+    "tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
+    NULL, NULL, "pause_filter", NULL,
+    "pfthreshold", NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+};
+
 /* collects per-function cpuid data
  */
 typedef struct model_features_t {
@@ -192,13 +203,15 @@ static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
                                     uint32_t *ext_features,
                                     uint32_t *ext2_features,
                                     uint32_t *ext3_features,
-                                    uint32_t *kvm_features)
+                                    uint32_t *kvm_features,
+                                    uint32_t *svm_features)
 {
     if (!lookup_feature(features, flagname, NULL, feature_name) &&
         !lookup_feature(ext_features, flagname, NULL, ext_feature_name) &&
         !lookup_feature(ext2_features, flagname, NULL, ext2_feature_name) &&
         !lookup_feature(ext3_features, flagname, NULL, ext3_feature_name) &&
-        !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name))
+        !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name) &&
+        !lookup_feature(svm_features, flagname, NULL, svm_feature_name))
             fprintf(stderr, "CPU feature %s not found\n", flagname);
 }
 
@@ -210,7 +223,8 @@ typedef struct x86_def_t {
     int family;
     int model;
     int stepping;
-    uint32_t features, ext_features, ext2_features, ext3_features, kvm_features;
+    uint32_t features, ext_features, ext2_features, ext3_features;
+    uint32_t kvm_features, svm_features;
     uint32_t xlevel;
     char model_id[48];
     int vendor_override;
@@ -254,6 +268,8 @@ typedef struct x86_def_t {
 #define TCG_EXT3_FEATURES (CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM | \
           CPUID_EXT3_CR8LEG | CPUID_EXT3_ABM | CPUID_EXT3_SSE4A)
 
+#define TCG_SVM_FEATURES 0
+
 /* maintains list of cpu model definitions
  */
 static x86_def_t *x86_defs = {NULL};
@@ -560,8 +576,14 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
 
     char *s = strdup(cpu_model);
     char *featurestr, *name = strtok(s, ",");
-    uint32_t plus_features = 0, plus_ext_features = 0, plus_ext2_features = 0, plus_ext3_features = 0, plus_kvm_features = 0;
-    uint32_t minus_features = 0, minus_ext_features = 0, minus_ext2_features = 0, minus_ext3_features = 0, minus_kvm_features = 0;
+    /* Features to be added*/
+    uint32_t plus_features = 0, plus_ext_features = 0;
+    uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
+    uint32_t plus_kvm_features = 0, plus_svm_features = 0;
+    /* Features to be removed */
+    uint32_t minus_features = 0, minus_ext_features = 0;
+    uint32_t minus_ext2_features = 0, minus_ext3_features = 0;
+    uint32_t minus_kvm_features = 0, minus_svm_features = 0;
     uint32_t numvalue;
 
     for (def = x86_defs; def; def = def->next)
@@ -579,16 +601,22 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
 
     add_flagname_to_bitmaps("hypervisor", &plus_features,
         &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
-        &plus_kvm_features);
+        &plus_kvm_features, &plus_svm_features);
 
     featurestr = strtok(NULL, ",");
 
     while (featurestr) {
         char *val;
         if (featurestr[0] == '+') {
-            add_flagname_to_bitmaps(featurestr + 1, &plus_features, &plus_ext_features, &plus_ext2_features, &plus_ext3_features, &plus_kvm_features);
+            add_flagname_to_bitmaps(featurestr + 1, &plus_features,
+                            &plus_ext_features, &plus_ext2_features,
+                            &plus_ext3_features, &plus_kvm_features,
+                            &plus_svm_features);
         } else if (featurestr[0] == '-') {
-            add_flagname_to_bitmaps(featurestr + 1, &minus_features, &minus_ext_features, &minus_ext2_features, &minus_ext3_features, &minus_kvm_features);
+            add_flagname_to_bitmaps(featurestr + 1, &minus_features,
+                            &minus_ext_features, &minus_ext2_features,
+                            &minus_ext3_features, &minus_kvm_features,
+                            &minus_svm_features);
         } else if ((val = strchr(featurestr, '='))) {
             *val = 0; val++;
             if (!strcmp(featurestr, "family")) {
@@ -670,11 +698,13 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
     x86_cpu_def->ext2_features |= plus_ext2_features;
     x86_cpu_def->ext3_features |= plus_ext3_features;
     x86_cpu_def->kvm_features |= plus_kvm_features;
+    x86_cpu_def->svm_features |= plus_svm_features;
     x86_cpu_def->features &= ~minus_features;
     x86_cpu_def->ext_features &= ~minus_ext_features;
     x86_cpu_def->ext2_features &= ~minus_ext2_features;
     x86_cpu_def->ext3_features &= ~minus_ext3_features;
     x86_cpu_def->kvm_features &= ~minus_kvm_features;
+    x86_cpu_def->svm_features &= ~minus_svm_features;
     if (check_cpuid) {
         if (check_features_against_host(x86_cpu_def) && enforce_cpuid)
             goto error;
@@ -816,6 +846,7 @@ int cpu_x86_register (CPUX86State *env, const char *cpu_model)
     env->cpuid_ext3_features = def->ext3_features;
     env->cpuid_xlevel = def->xlevel;
     env->cpuid_kvm_features = def->kvm_features;
+    env->cpuid_svm_features = def->svm_features;
     if (!kvm_enabled()) {
         env->cpuid_features &= TCG_FEATURES;
         env->cpuid_ext_features &= TCG_EXT_FEATURES;
@@ -825,6 +856,7 @@ int cpu_x86_register (CPUX86State *env, const char *cpu_model)
 #endif
             );
         env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
+	env->cpuid_svm_features &= TCG_SVM_FEATURES;
     }
     {
         const char *model_id = def->model_id;
@@ -1208,7 +1240,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         *eax = 0x00000001; /* SVM Revision */
         *ebx = 0x00000010; /* nr of ASIDs */
         *ecx = 0;
-        *edx = 0; /* optional features */
+        *edx = env->cpuid_svm_features; /* optional features */
         break;
     default:
         /* reserved values: zero */
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index b00e80d..a547652 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -204,6 +204,9 @@ int kvm_arch_init_vcpu(CPUState *env)
                                                              0, R_EDX);
     env->cpuid_ext3_features &= kvm_arch_get_supported_cpuid(env, 0x80000001,
                                                              0, R_ECX);
+    env->cpuid_kvm_features  &= kvm_arch_get_supported_cpuid(env, 0x8000000A,
+                                                             0, R_EDX);
+
 
     cpuid_i = 0;
 
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Alexander Graf]

On 07.09.2010, at 13:27, Joerg Roedel wrote:

> This patch changes the setting logic for the svm bit in
> qemu-kvm. The bit is now explicitly set on -enable-nesting
> instead of masked out if the parameter is not supplied.
> 
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
> target-i386/cpuid.c |   12 ++++++------
> 1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
> index d63fdcb..5fa0dd0 100644
> --- a/target-i386/cpuid.c
> +++ b/target-i386/cpuid.c
> @@ -276,8 +276,8 @@ static x86_def_t builtin_x86_defs[] = {
>         .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_CX16 | CPUID_EXT_POPCNT,
>         .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) |
>             CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
> -        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
> -            CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
> +        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_ABM |
> +                         CPUID_EXT3_SSE4A,
>         .xlevel = 0x8000000A,
>         .model_id = "QEMU Virtual CPU version " QEMU_VERSION,
>     },
> @@ -303,8 +303,8 @@ static x86_def_t builtin_x86_defs[] = {
>                     CPUID_EXT3_CR8LEG,
>                     CPUID_EXT3_MISALIGNSSE, CPUID_EXT3_3DNOWPREFETCH,
>                     CPUID_EXT3_OSVW, CPUID_EXT3_IBS */
> -        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
> -            CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
> +        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_ABM |
> +                         CPUID_EXT3_SSE4A,
>         .xlevel = 0x8000001A,
>         .model_id = "AMD Phenom(tm) 9550 Quad-Core Processor"
>     },
> @@ -1154,8 +1154,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>             /* disable CPU features that KVM cannot support */
> 
>             /* svm */
> -            if (!kvm_nested)
> -                *ecx &= ~CPUID_EXT3_SVM;
> +            if (kvm_nested)

I think we should get rid of kvm_nested and -enable-nesting. Instead, we should enable the SVM bit in the "host" and "qemu64" cpu types, but not in "kvm64". This way users are safe to not use nested svm, but can choose to do so if they like.

Also, it should be possible to do something like -cpu kvm64,flags=+svm. Without having to mess with -enable-nesting.


Alex

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Daniel P. Berrange]

On Tue, Sep 07, 2010 at 02:23:55PM +0200, Alexander Graf wrote:
> 
> On 07.09.2010, at 13:27, Joerg Roedel wrote:
> 
> > This patch changes the setting logic for the svm bit in
> > qemu-kvm. The bit is now explicitly set on -enable-nesting
> > instead of masked out if the parameter is not supplied.
> > 
> > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> > ---
> > target-i386/cpuid.c |   12 ++++++------
> > 1 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
> > index d63fdcb..5fa0dd0 100644
> > --- a/target-i386/cpuid.c
> > +++ b/target-i386/cpuid.c
> > @@ -276,8 +276,8 @@ static x86_def_t builtin_x86_defs[] = {
> >         .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_CX16 | CPUID_EXT_POPCNT,
> >         .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) |
> >             CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
> > -        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
> > -            CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
> > +        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_ABM |
> > +                         CPUID_EXT3_SSE4A,
> >         .xlevel = 0x8000000A,
> >         .model_id = "QEMU Virtual CPU version " QEMU_VERSION,
> >     },
> > @@ -303,8 +303,8 @@ static x86_def_t builtin_x86_defs[] = {
> >                     CPUID_EXT3_CR8LEG,
> >                     CPUID_EXT3_MISALIGNSSE, CPUID_EXT3_3DNOWPREFETCH,
> >                     CPUID_EXT3_OSVW, CPUID_EXT3_IBS */
> > -        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
> > -            CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
> > +        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_ABM |
> > +                         CPUID_EXT3_SSE4A,
> >         .xlevel = 0x8000001A,
> >         .model_id = "AMD Phenom(tm) 9550 Quad-Core Processor"
> >     },
> > @@ -1154,8 +1154,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >             /* disable CPU features that KVM cannot support */
> > 
> >             /* svm */
> > -            if (!kvm_nested)
> > -                *ecx &= ~CPUID_EXT3_SVM;
> > +            if (kvm_nested)
> 
> I think we should get rid of kvm_nested and -enable-nesting. Instead, we should enable the SVM bit in the "host" and "qemu64" cpu types, but not in "kvm64". This way users are safe to not use nested svm, but can choose to do so if they like.
> 
> Also, it should be possible to do something like -cpu kvm64,flags=+svm. Without having to mess with -enable-nesting.

I agree that its much nicer for mgmt tools if svm/vmx CPU flags can just be
toggled in the normal manner. Might we also want to have a enable/disable
nesting flag, so we can mirror the way real hardware lets you disable virt
in the BIOS even when the CPU(s) have the vmx/svm flags set ?

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 1/3] qemu-kvm: Invert svm-flag setting logic

[Alexander Graf]

On 07.09.2010, at 14:30, Daniel P. Berrange wrote:

> On Tue, Sep 07, 2010 at 02:23:55PM +0200, Alexander Graf wrote:
>> 
>> On 07.09.2010, at 13:27, Joerg Roedel wrote:
>> 
>>> This patch changes the setting logic for the svm bit in
>>> qemu-kvm. The bit is now explicitly set on -enable-nesting
>>> instead of masked out if the parameter is not supplied.
>>> 
>>> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>>> ---
>>> target-i386/cpuid.c |   12 ++++++------
>>> 1 files changed, 6 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
>>> index d63fdcb..5fa0dd0 100644
>>> --- a/target-i386/cpuid.c
>>> +++ b/target-i386/cpuid.c
>>> @@ -276,8 +276,8 @@ static x86_def_t builtin_x86_defs[] = {
>>>        .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_CX16 | CPUID_EXT_POPCNT,
>>>        .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) |
>>>            CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX,
>>> -        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
>>> -            CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
>>> +        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_ABM |
>>> +                         CPUID_EXT3_SSE4A,
>>>        .xlevel = 0x8000000A,
>>>        .model_id = "QEMU Virtual CPU version " QEMU_VERSION,
>>>    },
>>> @@ -303,8 +303,8 @@ static x86_def_t builtin_x86_defs[] = {
>>>                    CPUID_EXT3_CR8LEG,
>>>                    CPUID_EXT3_MISALIGNSSE, CPUID_EXT3_3DNOWPREFETCH,
>>>                    CPUID_EXT3_OSVW, CPUID_EXT3_IBS */
>>> -        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
>>> -            CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
>>> +        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_ABM |
>>> +                         CPUID_EXT3_SSE4A,
>>>        .xlevel = 0x8000001A,
>>>        .model_id = "AMD Phenom(tm) 9550 Quad-Core Processor"
>>>    },
>>> @@ -1154,8 +1154,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>>>            /* disable CPU features that KVM cannot support */
>>> 
>>>            /* svm */
>>> -            if (!kvm_nested)
>>> -                *ecx &= ~CPUID_EXT3_SVM;
>>> +            if (kvm_nested)
>> 
>> I think we should get rid of kvm_nested and -enable-nesting. Instead, we should enable the SVM bit in the "host" and "qemu64" cpu types, but not in "kvm64". This way users are safe to not use nested svm, but can choose to do so if they like.
>> 
>> Also, it should be possible to do something like -cpu kvm64,flags=+svm. Without having to mess with -enable-nesting.
> 
> I agree that its much nicer for mgmt tools if svm/vmx CPU flags can just be
> toggled in the normal manner. Might we also want to have a enable/disable
> nesting flag, so we can mirror the way real hardware lets you disable virt
> in the BIOS even when the CPU(s) have the vmx/svm flags set ?

I don't see how that's useful. We don't model anything like this in the kvm kernel module, so whatever we expose in user space would not be useful for detection testing. Also I'm fairly sure you either want nesting or you don't :). And if you want nesting, you probably want -cpu host too, since you lose migration anyways.


Alex

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Avi Kivity]

  On 09/07/2010 03:33 PM, Alexander Graf wrote:
>
>> I agree that its much nicer for mgmt tools if svm/vmx CPU flags can just be
>> toggled in the normal manner. Might we also want to have a enable/disable
>> nesting flag, so we can mirror the way real hardware lets you disable virt
>> in the BIOS even when the CPU(s) have the vmx/svm flags set ?
> I don't see how that's useful. We don't model anything like this in the kvm kernel module, so whatever we expose in user space would not be useful for detection testing. Also I'm fairly sure you either want nesting or you don't :). And if you want nesting, you probably want -cpu host too, since you lose migration anyways.

Why do you lost migration?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Alexander Graf]

On 07.09.2010, at 14:35, Avi Kivity wrote:

> On 09/07/2010 03:33 PM, Alexander Graf wrote:
>> 
>>> I agree that its much nicer for mgmt tools if svm/vmx CPU flags can just be
>>> toggled in the normal manner. Might we also want to have a enable/disable
>>> nesting flag, so we can mirror the way real hardware lets you disable virt
>>> in the BIOS even when the CPU(s) have the vmx/svm flags set ?
>> I don't see how that's useful. We don't model anything like this in the kvm kernel module, so whatever we expose in user space would not be useful for detection testing. Also I'm fairly sure you either want nesting or you don't :). And if you want nesting, you probably want -cpu host too, since you lose migration anyways.
> 
> Why do you lost migration?

Is that part fixed already? Joerg worked so hard on so many things that I lost track of what works and what doesn't :).


Alex

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Avi Kivity]

  On 09/07/2010 03:37 PM, Alexander Graf wrote:
>
>> Why do you lost migration?
> Is that part fixed already? Joerg worked so hard on so many things that I lost track of what works and what doesn't :).

Was it broken?  How?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Alexander Graf]

On 07.09.2010, at 14:43, Avi Kivity wrote:

> On 09/07/2010 03:37 PM, Alexander Graf wrote:
>> 
>>> Why do you lost migration?
>> Is that part fixed already? Joerg worked so hard on so many things that I lost track of what works and what doesn't :).
> 
> Was it broken?  How?

When migrating inside l2 context, we're missing information. My idea back then was to force the l1 guest out of l2 context every time we want to migrate, but I'm not sure this has happened. Even then I'm not sure we're transferring the GIF.


Alex

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Avi Kivity]

  On 09/07/2010 03:55 PM, Alexander Graf wrote:
>
>> Was it broken?  How?
> When migrating inside l2 context, we're missing information. My idea back then was to force the l1 guest out of l2 context every time we want to migrate, but I'm not sure this has happened.

I thought that was implemented, but not sure (though it isn't clean 
architecturally).

> Even then I'm not sure we're transferring the GIF.

Argh, we aren't.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Avi Kivity]

  On 09/07/2010 03:23 PM, Alexander Graf wrote:
>
> I think we should get rid of kvm_nested and -enable-nesting. Instead, we should enable the SVM bit in the "host" and "qemu64" cpu types, but not in "kvm64". This way users are safe to not use nested svm, but can choose to do so if they like.
>
> Also, it should be possible to do something like -cpu kvm64,flags=+svm. Without having to mess with -enable-nesting.

I agree, -enable-nesting is redundant with -cpu ,+svm.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Roedel, Joerg]

On Tue, Sep 07, 2010 at 09:51:33AM -0400, Avi Kivity wrote:
>   On 09/07/2010 03:23 PM, Alexander Graf wrote:
> >
> > I think we should get rid of kvm_nested and -enable-nesting. Instead, we should enable the SVM bit in the "host" and "qemu64" cpu types, but not in "kvm64". This way users are safe to not use nested svm, but can choose to do so if they like.
> >
> > Also, it should be possible to do something like -cpu kvm64,flags=+svm. Without having to mess with -enable-nesting.
> 
> I agree, -enable-nesting is redundant with -cpu ,+svm.

This patchset makes -enable-nesting pratically a synonym for -cpu ,+svm.
So we can safely remove -enable-nesting in the future.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Roedel, Joerg]

On Tue, Sep 07, 2010 at 08:59:52AM -0400, Avi Kivity wrote:
>   On 09/07/2010 03:55 PM, Alexander Graf wrote:
> >
> >> Was it broken?  How?
> > When migrating inside l2 context, we're missing information. My idea back then was to force the l1 guest out of l2 context every time we want to migrate, but I'm not sure this has happened.
> 
> I thought that was implemented, but not sure (though it isn't clean 
> architecturally).
> 
> > Even then I'm not sure we're transferring the GIF.
> 
> Argh, we aren't.

Migration with nested-svm doesn't work reliably today. But that is on my
list.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Alexander Graf]

On 07.09.2010, at 16:12, Roedel, Joerg wrote:

> On Tue, Sep 07, 2010 at 09:51:33AM -0400, Avi Kivity wrote:
>>  On 09/07/2010 03:23 PM, Alexander Graf wrote:
>>> 
>>> I think we should get rid of kvm_nested and -enable-nesting. Instead, we should enable the SVM bit in the "host" and "qemu64" cpu types, but not in "kvm64". This way users are safe to not use nested svm, but can choose to do so if they like.
>>> 
>>> Also, it should be possible to do something like -cpu kvm64,flags=+svm. Without having to mess with -enable-nesting.
>> 
>> I agree, -enable-nesting is redundant with -cpu ,+svm.
> 
> This patchset makes -enable-nesting pratically a synonym for -cpu ,+svm.
> So we can safely remove -enable-nesting in the future.

Why not just not introduce it? :) It's always hard to get rid of legacy.


Alex

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Alexander Graf]

On 07.09.2010, at 16:12, Roedel, Joerg wrote:

> On Tue, Sep 07, 2010 at 09:51:33AM -0400, Avi Kivity wrote:
>>  On 09/07/2010 03:23 PM, Alexander Graf wrote:
>>> 
>>> I think we should get rid of kvm_nested and -enable-nesting. Instead, we should enable the SVM bit in the "host" and "qemu64" cpu types, but not in "kvm64". This way users are safe to not use nested svm, but can choose to do so if they like.
>>> 
>>> Also, it should be possible to do something like -cpu kvm64,flags=+svm. Without having to mess with -enable-nesting.
>> 
>> I agree, -enable-nesting is redundant with -cpu ,+svm.
> 
> This patchset makes -enable-nesting pratically a synonym for -cpu ,+svm.
> So we can safely remove -enable-nesting in the future.

Oh, because this is for qemu-kvm. Uh - how about a nice little patch that makes things work in qemu.git, leave out the -enable-nesting piece and just keep the -enable-nesting backwards compat patch in qemu-kvm.git - or maybe even remove it there.


Alex

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Roedel, Joerg]

On Tue, Sep 07, 2010 at 10:14:46AM -0400, Alexander Graf wrote:
> 
> On 07.09.2010, at 16:12, Roedel, Joerg wrote:
> 
> > On Tue, Sep 07, 2010 at 09:51:33AM -0400, Avi Kivity wrote:
> >>  On 09/07/2010 03:23 PM, Alexander Graf wrote:
> >>> 
> >>> I think we should get rid of kvm_nested and -enable-nesting. Instead, we should enable the SVM bit in the "host" and "qemu64" cpu types, but not in "kvm64". This way users are safe to not use nested svm, but can choose to do so if they like.
> >>> 
> >>> Also, it should be possible to do something like -cpu kvm64,flags=+svm. Without having to mess with -enable-nesting.
> >> 
> >> I agree, -enable-nesting is redundant with -cpu ,+svm.
> > 
> > This patchset makes -enable-nesting pratically a synonym for -cpu ,+svm.
> > So we can safely remove -enable-nesting in the future.
> 
> Why not just not introduce it? :) It's always hard to get rid of legacy.

Because its already there? The patches are against qemu-kvm.

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Roedel, Joerg]

On Tue, Sep 07, 2010 at 10:16:10AM -0400, Alexander Graf wrote:
> Oh, because this is for qemu-kvm. Uh - how about a nice little patch
> that makes things work in qemu.git, leave out the -enable-nesting
> piece and just keep the -enable-nesting backwards compat patch in
> qemu-kvm.git - or maybe even remove it there.

I can do this. Is it better to merge all this stuff into qemu.git
instead of qemu-kvm.git? Then I target again for qemu.git.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Alexander Graf]

On 07.09.2010, at 16:20, Roedel, Joerg wrote:

> On Tue, Sep 07, 2010 at 10:16:10AM -0400, Alexander Graf wrote:
>> Oh, because this is for qemu-kvm. Uh - how about a nice little patch
>> that makes things work in qemu.git, leave out the -enable-nesting
>> piece and just keep the -enable-nesting backwards compat patch in
>> qemu-kvm.git - or maybe even remove it there.
> 
> I can do this. Is it better to merge all this stuff into qemu.git
> instead of qemu-kvm.git? Then I target again for qemu.git.

Yes, please. We're finally moving towards getting rid of qemu-kvm.git :). Or at least degrading it to a staging tree.


Alex

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Roedel, Joerg]

On Tue, Sep 07, 2010 at 10:22:21AM -0400, Alexander Graf wrote:
> 
> On 07.09.2010, at 16:20, Roedel, Joerg wrote:
> 
> > On Tue, Sep 07, 2010 at 10:16:10AM -0400, Alexander Graf wrote:
> >> Oh, because this is for qemu-kvm. Uh - how about a nice little patch
> >> that makes things work in qemu.git, leave out the -enable-nesting
> >> piece and just keep the -enable-nesting backwards compat patch in
> >> qemu-kvm.git - or maybe even remove it there.
> > 
> > I can do this. Is it better to merge all this stuff into qemu.git
> > instead of qemu-kvm.git? Then I target again for qemu.git.
> 
> Yes, please. We're finally moving towards getting rid of qemu-kvm.git
> :). Or at least degrading it to a staging tree.

Who are the maintainers to talk with? Anthony? Anybody else?

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Alexander Graf]

On 07.09.2010, at 16:33, Roedel, Joerg wrote:

> On Tue, Sep 07, 2010 at 10:22:21AM -0400, Alexander Graf wrote:
>> 
>> On 07.09.2010, at 16:20, Roedel, Joerg wrote:
>> 
>>> On Tue, Sep 07, 2010 at 10:16:10AM -0400, Alexander Graf wrote:
>>>> Oh, because this is for qemu-kvm. Uh - how about a nice little patch
>>>> that makes things work in qemu.git, leave out the -enable-nesting
>>>> piece and just keep the -enable-nesting backwards compat patch in
>>>> qemu-kvm.git - or maybe even remove it there.
>>> 
>>> I can do this. Is it better to merge all this stuff into qemu.git
>>> instead of qemu-kvm.git? Then I target again for qemu.git.
>> 
>> Yes, please. We're finally moving towards getting rid of qemu-kvm.git
>> :). Or at least degrading it to a staging tree.
> 
> Who are the maintainers to talk with? Anthony? Anybody else?

Anthony, Avi, Marcelo.


Alex

Re: [Qemu-devel] [PATCH 1/3] qemu-kvm: Invert svm-flag setting logic

[Avi Kivity]

  On 09/07/2010 05:36 PM, Alexander Graf wrote:
>
>> Who are the maintainers to talk with? Anthony? Anybody else?
> Anthony, Avi, Marcelo.

Post the patches against qemu.git, Marcelo or myself will apply them 
against a staging branch (uq/master, for "upstream queue for qemu.git 
master") and ask Anthony to pull.

-- 
error compiling committee.c: too many arguments to function