Re: [PATCH v3 4/6] i386: Mask and report unavailable multi-bit feature values

[Xiaoyao Li]

On 2/9/2023 2:15 PM, Wang, Lei wrote:
> On 2/9/2023 1:59 PM, Xiaoyao Li wrote:
> > On 2/9/2023 12:21 PM, Wang, Lei wrote:
> > > > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > > > > > index 88aa780566..e638a31d34 100644
> > > > > > > --- a/target/i386/cpu.c
> > > > > > > +++ b/target/i386/cpu.c
> > > > > > > @@ -4377,6 +4377,28 @@ static bool x86_cpu_have_filtered_features(X86CPU
> > > > > > > *cpu)
> > > > > > >         return false;
> > > > > > >     }
> > > > > > >
> > > > > > > +static void mark_unavailable_multi_bit_default(X86CPU *cpu, FeatureWord w,
> > > > > > > +                                               int index,
> > > > > > > +                                               const char *verbose_prefix)
> > > > > > > +{
> > > > > > > +    FeatureWordInfo *f = &feature_word_info[w];
> > > > > > > +    g_autofree char *feat_word_str = feature_word_description(f);
> > > > > > > +    uint64_t host_feat = x86_cpu_get_supported_feature_word(w, false);
> > > > > > > +    MultiBitFeatureInfo mf = f->multi_bit_features[index];
> > > > > > > +
> > > > > > > +    if ((cpu->env.features[w] & mf.mask) &&
> > > > > > With this checking bits are all 0 but covered by mf.mask's range are skippped,
> > > > > > even if they're different from the host_feat, please check whether it's
> > > > > > desried
> > > > > > behavior.
> > > > > This is the intended design because there are quite a number of multi-bit
> > > > > CPUIDs
> > > > > should support passing all 0 to them.
> > > > you didn't answer the question. The question is why the handling can be skipped
> > > > when the value of multi-bit feature is 0.
> > > I think the default function should handle the most common case, which is,
> > > passing all 0 multi-bits to KVM is valid and shouldn't be warned. E.g, when we
> > > are using some earlier CPU models which doesn't support AMX, we shouldn't be
> > > warned that all 0 values don't match the CPUID reported by KVM.
> >
> > passing value 0 to KVM is valid, is not the reason why the handling can be 
> > skipped.
> >
> > The correct reason is that when value is 0, it means the multi-bit feature is
> > not enabled/requested. It's always legal and doesn't need any handling. So it
> > can be just skipped.
>
> Currently, we can say yes, but I doubt if there will be a multi-bit field in the
> future that only accepts the non-zero value specified.

If so, then the multi-bit field cannot fall into the default handling 
category and it requires its own special handling callback.

It's also the reason I asked "What's the behavior of default ? you need 
to call out it clearly."

This patch should define well the behavior of default handler and 
describe it in the commit message. Moreover, it's better to comment it 
as well above the implementation of mark_unavailable_multi_bit_default()