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

[Joerg Roedel]

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.

> > +    /*
> > +     * 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.

> The rest looks good :). Thanks a lot for this patch set!

Great, thanks :-)

        Joerg