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
