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
