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

[Liang, Kan]

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.

You may also need to introduce another new function to check if the LBR 
is compatible with guest in lbr.c, e.g. bool 
is_lbr_compatible_with_guest(int model).

bool is_lbr_compatible_with_guest(int model) {
	struct x86_pmu fake_x86_pmu;

	if (boot_cpu_data.x86_model == model)
		return true;

	__intel_pmu_init(model, &fake_x86_pmu);

	if ((x86_pmu.lbr_nr == fake_x86_pmu.lbr_nr) &&
	    (x86_pmu.lbr_tos == fake_x86_pmu.lbr_tos) &&
	    (x86_pmu.lbr_from == fake_x86_pmu.lbr_from))
		return true;

	return false;
}

> > 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"?

Thanks,
Kan