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

[Eduardo Habkost]

On Thu, Jul 07, 2016 at 06:01:46PM +0200, Borislav Petkov wrote:
> On Thu, Jul 07, 2016 at 03:16:21PM +0200, Paolo Bonzini wrote:
> > Eduardo is the one to answer, but usually we add features to QEMU 
> > before the processors are released (typically as soon as KVM supports 
> > them).  So with a new enough QEMU this in theory should not be 
> > necessary.
> > 
> > Adding a new feature that's not in a CPU model and that's not 
> > associated to new state is really trivial:
> 
> Cool.
> 
> Btw, how about something like this?
> 
> Specifically, I'd like to test RAS features on the new upcoming AMD
> Zen CPU and I've defined one from the stuff we know so far from kernel
> patches.

You mean KVM kernel patches? 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.

> 
> The "filter=off" thing I've added in case I want to disable
> x86_cpu_filter_features() but it works just fine without it when I boot
> with -cpu Zen. So I can remove it too.
> 
> Would something like that be acceptable?

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?

Specifically about the feature names, I have some question below:

> 
> We can continue improving on this as features become known and even
> implement some functionality in qemu/kvm as time allows.
> 
> ---
> From: Borislav Petkov <bp@xxxxxxx>
> Date: Tue, 5 Jul 2016 16:12:18 +0200
> Subject: [PATCH] Zen emu: first working version
> 
> Boot with "-c Zen,filter=off" to disable CPUID bits filtering.
> 
> Signed-off-by: Borislav Petkov <bp@xxxxxxx>
> ---
>  target-i386/cpu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  target-i386/cpu.h |  7 +++++++
>  2 files changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3bd3cfc3ad16..cc9c97457387 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -307,6 +307,17 @@ static const char *cpuid_6_feature_name[] = {
>      NULL, NULL, NULL, NULL,
>  };
>  
> +static const char *smca_feature_name[] = {
> +    "overflow_recov", "succor", NULL, "smca",

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.


> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +};
> +
>  #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE)
>  #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \
>            CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX | CPUID_APIC)
> @@ -449,6 +460,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>          .cpuid_eax = 6, .cpuid_reg = R_EAX,
>          .tcg_features = TCG_6_EAX_FEATURES,
>      },
> +    [FEAT_8000_0007_EBX] = {
> +	.feat_names = smca_feature_name,
> +	.cpuid_eax = 0x80000007,
> +	.cpuid_reg = R_EBX,
> +    },
>  };
>  
>  typedef struct X86RegisterInfo32 {
[...]

-- 
Eduardo