Re: [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP, FDS and FDP

[Eduardo Habkost <ehabkost@redhat.com>]

On Tue, May 18, 2021 at 11:06:56AM +0800, Ziqiao Kong wrote:
[...]
> > > +    /*
> > > +     * If CR0.PE = 1, each instruction saves FCS and FDS into memory. If
> > > +     * CPUID.(EAX=07H,ECX=0H):EBX[bit 13] = 1, the processor deprecates
> > > +     * FCS and FDS; it saves each as 0000H.
> > > +     */
> > > +    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_FCS_FDS)
> > > +        && (env->cr[0] & CR0_PE_MASK)) {
> > > +        fpcs = env->fpcs;
> > > +        fpds = env->fpds;
> >
> >
> > If you want to start supporting this feature flag, I suggest
> > moving this to a separate patch.  The description of this patch
> > seems to imply it is just a bug fix, not the implementation of a
> > new feature flag.
> >
> > When adding support for a new feature flag in TCG code, you need
> > to extend TCG_*_FEATURES in target/i386/cpu.c, otherwise the
> > feature flag will never be enabled by the CPU configuration code.
> 
> Yes, currently this feature flag is never enabled for all CPU types.
> How about removing this
> feature flag in this patch and leaving a TODO in the comment? Thus I
> can issue another patch
> to complete the feature later.

Sounds better to me.  Otherwise you'll be just adding dead code
(because the flag will is impossible to be enabled if it's not
present in TCG_*_FEATURES).

-- 
Eduardo