lkml.org 
[lkml]   [2019]   [Jan]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4 04/10] KVM/x86: intel_pmu_lbr_enable
From
Date


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

\
 
 \ /
  Last update: 2019-01-04 16:58    [W:0.117 / U:2.548 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site