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

["Liang, Kan" <kan.liang@linux.intel.com>]

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