Re: [PATCH] KVM: SVM: fix trashing of MSR_TSC_AUX

[Eduardo Habkost]

On Thu, Jul 07, 2016 at 07:04:42PM +0200, Borislav Petkov wrote:
> On Thu, Jul 07, 2016 at 01:27:55PM -0300, Eduardo Habkost wrote:
> > You mean KVM kernel patches?
> 
> No, other ones. Here's one example:
> 
> https://lkml.kernel.org/r/1467633035-32080-2-git-send-email-Yazen.Ghannam@xxxxxxx
> 
> > I assume the features require additional KVM code to support them
> > in guests. In that case, why wouldn't the kernel return them in
> > GET_SUPPORTED_CPUID? Then you won't need filter=off.
> 
> Yeah, so in most cases they will need additional KVM code to enable
> them. More often than not, this is not always at the top of the TODO
> list of people so ...
> 
> That's why I did the quick thing of smoke-testing them by enabling only
> CPUID bits and the filter=off thing.
> 
> Would it be nicer to see them actually implemented in qemu/kvm?
> Definitely.
> 
> > About filter=off: not sure. Do we really have valid use cases to
> > enable a feature even if the kernel reports it as unsupported in
> > GET_SUPPORTED_CPUID?
> 
> Yeah, as said above, the filter=off thing was a dirty hack just to stop
> x86_cpu_filter_features() from checking whether the host supports them
> or not.

I see. If you have an useful use case for it, we may consider
that. But first I would like to see an actual case where a
feature was not added to GET_SUPPORTED_CPUID yet, but would not
crash and burn if forcibly enabled by QEMU.

> 
> > Do those features introduce additional state that need migration
> > support? If they do, you need to add them to
> > feature_word_info[FEAT_8000_0007_EBX].unmigratable_flags until
> > migration support is implemented.
> 
> I'm afraid you'd need to explain migration support to me: is the
> question whether migrating the guest to an Intel platform and whether
> the features would still work?
> 
> Because those three above are AMD-only and they won't work on an Intel
> platform.
> 
> And if so, I'm guessing they should always remain unmigratable.
> 
> Which is not a problem as there are Intel features which are not present
> on AMD so...

I mean live migration to a different host (that normally has the
same CPU vendor). When you live-migrate or use savevm, you need
to send the machine state to the other host. This is implemented
using VMStateDescription structs describing the data to be
migrated. See vmstate_x86_cpu in target-i386/machine.c, for
example.

You need additional migration sections if the feature introduces
additional state (e.g. CPU registers) that need to be migrated
too, to keep the feature working. If there's new state but no
migration support is implemented yet, you need to add the feature
to unmigratable_flags.

For an example where no additional state is introduced by new
features, see:

Author: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
Date:   Thu Oct 29 15:31:39 2015 +0800

    target-i386: Enable clflushopt/clwb/pcommit instructions
    
    These instructions are used by NVDIMM drivers and the specification is
    located at:
    https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf
    
    There instructions are available on Skylake Server.
    
    Signed-off-by: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
    Reviewed-by: Richard Henderson <rth@xxxxxxxxxxx>
    Signed-off-by: Eduardo Habkost <ehabkost@xxxxxxxxxx>

For an example where additional state is introduced by a CPU
feature and migration support was implemented, see:

commit f74eefe0b98cd7e13825de8e8d9f32e22aed102c
Author: Huaitong Han <huaitong.han@xxxxxxxxx>
Date:   Wed Nov 18 10:20:15 2015 +0800

    target-i386: Add PKU and and OSPKE support
    
    Add PKU and OSPKE CPUID features, including xsave state and
    migration support.
    
    Signed-off-by: Huaitong Han <huaitong.han@xxxxxxxxx>
    Reviewed-by: Eduardo Habkost <ehabkost@xxxxxxxxxx>
    [ehabkost: squashed 3 patches together, edited patch description]
    Signed-off-by: Eduardo Habkost <ehabkost@xxxxxxxxxx>

For an example where a feature was added without required
migration code and was added to unmigratable_flags, see:

commit 0bb0b2d2fe7f645ddaf1f0ff40ac669c9feb4aa1
Author: Paolo Bonzini <pbonzini@xxxxxxxxxx>
Date:   Mon Nov 24 15:54:43 2014 +0100

    target-i386: add feature flags for CPUID[EAX=0xd,ECX=1]
    
    These represent xsave-related capabilities of the processor, and KVM may
    or may not support them.
    
    Add feature bits so that they are considered by "-cpu ...,enforce", and use
    the new feature work instead of calling kvm_arch_get_supported_cpuid.
    
    Bit 3 (XSAVES) is not migratables because it requires saving MSR_IA32_XSS.
    Neither KVM nor any commonly available hardware supports it anyway.
    
    Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>



-- 
Eduardo