Re: [PATCH V2 09/11] target/i386: Refactor LBR format handling

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 1/29/2026 7:09 AM, Zide Chen wrote:
> Detach x86_cpu_pmu_realize() from x86_cpu_realizefn() to keep the latter
> focused and easier to follow.  Introduce a dedicated helper,
> x86_cpu_apply_lbr_pebs_fmt(), in preparation for adding PEBS format
> support without duplicating code.
>
> Convert PERF_CAP_LBR_FMT into separate mask and shift macros to allow
> x86_cpu_apply_lbr_pebs_fmt() to be shared with PEBS format handling.
>
> No functional change intended

Seems a period is missed for above line.

Others look good to me.

Reviewed-by: Dapeng Mi <dapeng1.mi@linux.intel.com>


>
> Signed-off-by: Zide Chen <zide.chen@intel.com>
> ---
> V2:
> - New patch.
>
>  target/i386/cpu.c | 94 +++++++++++++++++++++++++++++++----------------
>  target/i386/cpu.h |  3 +-
>  2 files changed, 65 insertions(+), 32 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 09180c718d58..54f04adb0b48 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -9781,6 +9781,66 @@ static bool x86_cpu_update_smp_cache_topo(MachineState *ms, X86CPU *cpu,
>  }
>  #endif
>  
> +static bool x86_cpu_apply_lbr_pebs_fmt(X86CPU *cpu, uint64_t host_perf_cap,
> +                                  uint64_t user_req, bool is_lbr_fmt,
> +                                  Error **errp)
> +{
> +    CPUX86State *env = &cpu->env;
> +    uint64_t mask;
> +    unsigned shift;
> +    unsigned user_fmt;
> +    const char *name;
> +
> +    if (is_lbr_fmt) {
> +        mask = PERF_CAP_LBR_FMT_MASK;
> +        shift = PERF_CAP_LBR_FMT_SHIFT;
> +        name = "lbr";
> +    } else {
> +        return false;
> +    }
> +
> +    if (user_req != -1) {
> +        env->features[FEAT_PERF_CAPABILITIES] &= ~(mask << shift);
> +        env->features[FEAT_PERF_CAPABILITIES] |= (user_req << shift);
> +    }
> +
> +    user_fmt = (env->features[FEAT_PERF_CAPABILITIES] >> shift) & mask;
> +
> +    if (user_fmt) {
> +        unsigned host_fmt = (host_perf_cap >> shift) & mask;
> +
> +        if (!cpu->enable_pmu) {
> +            error_setg(errp, "vPMU: %s is unsupported without pmu=on", name);
> +            return false;
> +        }
> +        if (user_fmt != host_fmt) {
> +            error_setg(errp, "vPMU: the %s-fmt value (0x%x) does not match "
> +                        "the host value (0x%x).",
> +                        name, user_fmt, host_fmt);
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
> +static int x86_cpu_pmu_realize(X86CPU *cpu, Error **errp)
> +{
> +    uint64_t host_perf_cap =
> +        x86_cpu_get_supported_feature_word(NULL, FEAT_PERF_CAPABILITIES);
> +
> +    /*
> +     * Override env->features[FEAT_PERF_CAPABILITIES].LBR_FMT
> +     * with user-provided setting.
> +     */
> +    if (!x86_cpu_apply_lbr_pebs_fmt(cpu, host_perf_cap,
> +                                    cpu->lbr_fmt, true, errp)) {
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
>  static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
> @@ -9788,7 +9848,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
>      CPUX86State *env = &cpu->env;
>      Error *local_err = NULL;
> -    unsigned guest_fmt;
>  
>      if (!kvm_enabled())
>          cpu->enable_pmu = false;
> @@ -9824,35 +9883,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>          goto out;
>      }
>  
> -    /*
> -     * Override env->features[FEAT_PERF_CAPABILITIES].LBR_FMT
> -     * with user-provided setting.
> -     */
> -    if (cpu->lbr_fmt != -1) {
> -        env->features[FEAT_PERF_CAPABILITIES] &= ~PERF_CAP_LBR_FMT;
> -        env->features[FEAT_PERF_CAPABILITIES] |= cpu->lbr_fmt;
> -    }
> -
> -    /*
> -     * vPMU LBR is supported when 1) KVM is enabled 2) Option pmu=on and
> -     * 3)vPMU LBR format matches that of host setting.
> -     */
> -    guest_fmt = env->features[FEAT_PERF_CAPABILITIES] & PERF_CAP_LBR_FMT;
> -    if (guest_fmt) {
> -        uint64_t host_perf_cap =
> -            x86_cpu_get_supported_feature_word(NULL, FEAT_PERF_CAPABILITIES);
> -        unsigned host_lbr_fmt = host_perf_cap & PERF_CAP_LBR_FMT;
> -
> -        if (!cpu->enable_pmu) {
> -            error_setg(errp, "vPMU: LBR is unsupported without pmu=on");
> -            return;
> -        }
> -        if (guest_fmt != host_lbr_fmt) {
> -            error_setg(errp, "vPMU: the lbr-fmt value (0x%x) does not match "
> -                        "the host value (0x%x).",
> -                        guest_fmt, host_lbr_fmt);
> -            return;
> -        }
> +    if (x86_cpu_pmu_realize(cpu, errp)) {
> +        return;
>      }
>  
>      if (x86_cpu_filter_features(cpu, cpu->check_cpuid || cpu->enforce_cpuid)) {
> @@ -10445,7 +10477,7 @@ static const Property x86_cpu_properties[] = {
>  #endif
>      DEFINE_PROP_INT32("node-id", X86CPU, node_id, CPU_UNSET_NUMA_NODE_ID),
>      DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
> -    DEFINE_PROP_UINT64_CHECKMASK("lbr-fmt", X86CPU, lbr_fmt, PERF_CAP_LBR_FMT),
> +    DEFINE_PROP_UINT64_CHECKMASK("lbr-fmt", X86CPU, lbr_fmt, PERF_CAP_LBR_FMT_MASK),
>  
>      DEFINE_PROP_UINT32("hv-spinlocks", X86CPU, hyperv_spinlock_attempts,
>                         HYPERV_SPINLOCK_NEVER_NOTIFY),
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 3e2222e105bc..aa3c24e0ba13 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -420,7 +420,8 @@ typedef enum X86Seg {
>  #define ARCH_CAP_TSX_CTRL_MSR		(1<<7)
>  
>  #define MSR_IA32_PERF_CAPABILITIES      0x345
> -#define PERF_CAP_LBR_FMT                0x3f
> +#define PERF_CAP_LBR_FMT_MASK           0x3f
> +#define PERF_CAP_LBR_FMT_SHIFT          0x0
>  #define PERF_CAP_FULL_WRITE             (1U << 13)
>  #define PERF_CAP_PEBS_BASELINE          (1U << 14)
>