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

[Alexander Graf]

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

> On Sat, Sep 11, 2010 at 03:43:02PM +0200, Alexander Graf wrote:
>>> @@ -305,6 +322,8 @@ static x86_def_t builtin_x86_defs[] = {
>>>                    CPUID_EXT3_OSVW, CPUID_EXT3_IBS */
>>>        .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
>>>            CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
>>> +        .svm_features = CPUID_SVM_NPT | CPUID_SVM_LBRV | 
>>> CPUID_SVM_NRIPSAVE |
>>> +            CPUID_SVM_VMCBCLEAN,
>> 
>> Does that phenom already do all those? It does NPT, but I'm not sure
>> about NRIPSAVE for example.
> 
> Depends on which Phenom you have. A Phenom II has NRIPSAVE but the old
> Phenoms don't have it. For the SVM features it is not that important
> what the host hardware supports but what KVM can emulate. VMCBCLEAN can
> be emulated without supporting it in the host for example.

That particular one was my workstation - a Phenom 9550 which is one of the 
early 4-core ones.

> 
>>> +    /*
>>> +     * Every SVM feature requires emulation support in KVM - so we can't 
>>> just
>>> +     * read the host features here. KVM might even support SVM features not
>>> +     * available on the host hardware
>>> +     */
>>> +    x86_cpu_def->svm_features = CPUID_SVM_NPT | CPUID_SVM_LBRV |
>>> +                                CPUID_SVM_NRIPSAVE | CPUID_SVM_VMCBCLEAN;
>> 
>> Hrm. Wouldn't it make more sense to declare this to -1? This will
>> still go through the kernel space matcher which tells us which
>> features are available anyways, right?
> 
> Yeah, that would make sense. I thought about it while porting the
> patches but could not actually made me do it because I am not entirely
> sure that this is a good idea. But I may revisit that, especially after
> your question :-)
> 
>>> -
>>> -        if (kvm_enabled()) {
>>> -            /* Nested SVM not yet supported in upstream QEMU */
>>> -            *ecx &= ~CPUID_EXT3_SVM;
>>> -        }
>> 
>> Have you made sure that the default cpu type doesn't enable the SVM
>> bit? I couldn't find any trace of an override to "kvm64" as default
>> type when KVM is used.
> 
> No, the default CPU type has SVM still enabled by default. I thought
> about removing the SVM flag from the qemu64 cpu definition but that
> breaks on TCG where SVM is emulated too.
> What I implemented in this patch is to enable SVM by default and mask it
> out if KVM does not support it on the given machine. Problem here is
> that KVM is currently buggy because it always reports support for SVM,
> even on Intel machines. I fixed that with patch 29 of my npt-virt
> patch-set. The patch will hopefully make it into the various stable
> trees and then we have a clean solution.

It still won't be clean as it breaks cross vendor migration :(. The real fix 
would be to set the default machine to "kvm64" instead of "qemu64" in pc.c when 
kvm_enabled().


Alex