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 <address@hidden>
>>> ---
>>> 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