| Message ID | 1283858843-21330-2-git-send-email-joerg.roedel@amd.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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?
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
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?
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
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.
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.
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
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
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
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
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.
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
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
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
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
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.
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; }
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(-)