Re: [PATCH v4 04/10] KVM/x86: intel_pmu_lbr_enable

[Wei Wang]

On 01/04/2019 11:57 PM, Liang, Kan wrote:
> On 1/4/2019 4:58 AM, Wei Wang wrote:
> > On 01/03/2019 12:33 AM, Liang, Kan wrote:
> > > On 12/26/2018 4:25 AM, Wei Wang wrote:
> > > > +
> > > > +    /*
> > > > +     * It could be possible that people have vcpus of old model 
> > > > run on
> > > > +     * physcal cpus of newer model, for example a BDW guest on a SKX
> > > > +     * machine (but not possible to be the other way around).
> > > > +     * The BDW guest may not get accurate results on a SKX machine 
> > > > as it
> > > > +     * only reads 16 entries of the lbr stack while there are 32 
> > > > entries
> > > > +     * of recordings. So we currently forbid the lbr enabling when 
> > > > the
> > > > +     * vcpu and physical cpu see different lbr stack entries.
> > >
> > > I think it's not enough to only check number of entries. The LBR 
> > > from/to MSRs may be different even the number of entries is the 
> > > same, e.g SLM and KNL.
> >
> > Yes, we could add the comparison of the FROM msrs.
> >
> > > > +     */
> > > > +    switch (vcpu_model) {
> > >
> > > That's a duplicate of intel_pmu_init(). I think it's better to 
> > > factor out the common part if you want to check LBR MSRs and 
> > > entries. Then we don't need to add the same codes in two different 
> > > places when enabling new platforms.
> >
> >
> > Yes, I thought about this, but intel_pmu_init() does a lot more 
> > things in each "Case xx". Any thought about how to factor them out?
>
> I think we may only move the "switch (boot_cpu_data.x86_model) { ... 
> }" to a new function, e.g. __intel_pmu_init(int model, struct x86_pmu 
> *x86_pmu)
>
> In __intel_pmu_init, if the model != boot_cpu_data.x86_model, you only 
> need to update x86_pmu.*. Just ignore global settings, e.g 
> hw_cache_event_ids, mem_attr, extra_attr etc.

Thanks for sharing. I understand the point of maintaining those models 
at one place,
but this factor-out doesn't seem very elegant to me, like below

__intel_pmu_init (int model, struct x86_pmu *x86_pmu)
{
...
switch (model)
case INTEL_FAM6_NEHALEM:
case INTEL_FAM6_NEHALEM_EP:
case INTEL_FAM6_NEHALEM_EX:
    intel_pmu_lbr_init(x86_pmu);
    if (model != boot_cpu_data.x86_model)
        return;

    /* Other a lot of things init like below..*/
    memcpy(hw_cache_event_ids, nehalem_hw_cache_event_ids,
                   sizeof(hw_cache_event_ids));
    memcpy(hw_cache_extra_regs, nehalem_hw_cache_extra_regs,
                   sizeof(hw_cache_extra_regs));
    x86_pmu.event_constraints = intel_nehalem_event_constraints;
                x86_pmu.pebs_constraints = 
intel_nehalem_pebs_event_constraints;
                x86_pmu.enable_all = intel_pmu_nhm_enable_all;
                x86_pmu.extra_regs = intel_nehalem_extra_regs;
 ...

Case...
}
We need insert "if (model != boot_cpu_data.x86_model)" in every "Case xx".

What would be the rationale that we only do lbr_init for "x86_pmu"
when model != boot_cpu_data.x86_model?
(It looks more like a workaround to factor-out the function and get what 
we want)

I would prefer having them separated as this patch for now - it is 
logically more clear to me.


> > > Actually, I think we may just support LBR for guest if it has the 
> > > identical CPU model as host. It should be good enough for now.
> >
> > I actually tried this in the first place but it failed to work with 
> > the existing QEMU.
> > For example, when we specify "Broadwell" cpu from qemu, then qemu 
> > uses Broadwell core model,
> > but the physical machine I have is Broadwell X. This patch will 
> > support this case.
>
> I mean is it good enough if we only support "-cpu host"?

Not really. AFAIK, people don't use this usually. It is more common to 
specify the CPU type.

Best,
Wei