Re: [Qemu-devel] [PATCH v4 07/18] target-i386: Filter FEAT_7_0_EBX TCG features too

[Eduardo Habkost]

On Thu, May 15, 2014 at 08:10:16PM +0200, Andreas Färber wrote:
> Am 30.04.2014 18:48, schrieb Eduardo Habkost:
> > The TCG_7_0_EBX_FEATURES macro was defined but never used (it even had a
> > typo that was never noticed). Make the existing TCG feature filtering
> > code use it.
> > 
> > Change the TCG feature flag filtering code to use it.
> 
> Sentence seems duplicate - which one to keep?

Oops. Please removed the second paragraph (I removed it on the v4
resend).

> 
> Should we CC this commit for -stable? (Affects -cpu Haswell probably?)

It affects Haswell in a guest-visible way, yes. I don't know how well
guests behave when very recent CPU models run in TCG mode (having lots
of features removed), so I don't know if it makes sense for -stable or
not.

> If not, should we make this conditional on the machine version?

There would be no point, as there's no ABI stability guarantee in TCG
mode at all.

One day somebody may want to implement it, and it should be possible now
that we are making the enforce/check/filtered-features code work
properly with TCG. But it doesn't exist today.

> 
> One off-topic question below...
> 
> > 
> > Reviewed-by: Richard Henderson <address@hidden>
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> > Changes v1 -> v2:
> >  * Trivial rebase to latest qom-cpu (commit 90c5d39c)
> >    (Reviewed-by line kept)
> > Changes v2 -> v3:
> >  * Trivial rebase after QEMU 2.0 (onto commit 2d03b49)
> >    (Reviewed-by line kept)
> > ---
> >  target-i386/cpu.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index bbac5fc..714d121 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -588,7 +588,7 @@ struct X86CPUDefinition {
> >  #define TCG_EXT3_FEATURES (CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM | \
> >            CPUID_EXT3_CR8LEG | CPUID_EXT3_ABM | CPUID_EXT3_SSE4A)
> >  #define TCG_SVM_FEATURES 0
> > -#define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP \
> > +#define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP | \
> >            CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ADX)
> >            /* missing:
> >            CPUID_7_0_EBX_FSGSBASE, CPUID_7_0_EBX_HLE, CPUID_7_0_EBX_AVX2,
> > @@ -2596,6 +2596,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
> > **errp)
> >      if (!kvm_enabled()) {
> 
> Is there a patch or should I follow-up with one to make TCG filtering
> conditional to if (tcg_enabled())? (Xen, QTest)

There's no patch for that yet. I am not sure what would be appropriate
to do on those cases. Does it even make sense to set up any CPUID data
with Xen or QTest?

> 
> Regards,
> Andreas
> 
> >          env->features[FEAT_1_EDX] &= TCG_FEATURES;
> >          env->features[FEAT_1_ECX] &= TCG_EXT_FEATURES;
> > +        env->features[FEAT_7_0_EBX] &= TCG_7_0_EBX_FEATURES;
> >          env->features[FEAT_8000_0001_EDX] &= TCG_EXT2_FEATURES;
> >          env->features[FEAT_8000_0001_ECX] &= TCG_EXT3_FEATURES;
> >          env->features[FEAT_SVM] &= TCG_SVM_FEATURES;
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo