Re: [PATCH v2] target/i386: add "-cpu, lbr-fmt=*" support to enable guest LBR

[Eduardo Habkost]

On Fri, Apr 30, 2021 at 11:20:08AM +0800, Like Xu wrote:
[...]
> > > +    if (cpu->lbr_fmt) {
> > > +        if (!cpu->enable_pmu) {
> > > +            error_setg(errp, "LBR is unsupported since guest PMU is 
> > > disabled.");
> > > +            return;
> > > +        }
> > > +        env->features[FEAT_PERF_CAPABILITIES] |= cpu->lbr_fmt;
> > 
> > This doesn't seem right, as we should still do what the user
> > asked for if "lbr-fmt=0" is used.
> > 
> > You need to differentiate "lbr-fmt=0" from "lbr-fmt not set"
> > somehow.  I suggest initializing lbr_fmt to 0xFF by default,
> > so you can override env->features[FEAT_PERF_CAPABILITIES]
> > when lbr_fmt != 0xFF (even if lbr_fmt=0).
> 
> 
> > 
> > Something like this:
> > 
> >    #define LBR_FMT_UNSET 0xff
> >    ...
> >    DEFINE_PROP_UINT8("lbr-fmt", X86CPU, lbr_fmt, LBR_FMT_UNSET)
> >    ...
> > 
> >    void x86_cpu_realizefn(...)
> >    {
> >        ...
> >        if (cpu->lbr_fmt != LBR_FMT_UNSET) {
> >            if ((cpu->lbr_fmt & LBR_FMT_FMT) != cpu->lbr_fmt) {
> >                error_setg(errp, "invalid lbr-fmt" ...);
> >                return;
> >            }
> >            env->features[FEAT_PERF_CAPABILITIES] &= ~PERF_CAP_LBR_FMT;
> >            env->features[FEAT_PERF_CAPABILITIES] |= cpu->lbr_fmt;
> >        }
> >        /* If lbr_fmt == LBR_FMT_UNSET, the default value of env->features[]
> >         * will be used as is (and it may depend on the "migratable" flag)
> >         */
> 
> How about the user use "-cpu host,lbr-fmt=0xff" ?
> 
> The proposed code does nothing about warning or error,
> but implicitly uses the the default value of env->features[]
> 
> Users may have an illusion that the "lbr-fmt=0xff" is a "valid" lbr-fmt
> and it may enable guest LBR (depend on the "migratable" flag).

You are correct, lbr-fmt=0xff will be synonymous to "use default
lbr-fmt", but this won't be obvious.

You can avoid this by validating the user-provided value in a
property setter.  Or you could just document that 0xff has a
special meaning, to avoid a custom setter.  I believe custom
setters are more likely to cause us problems in the future than
users fiddling with obviously an invalid lbr-fmt value.

-- 
Eduardo