lkml.org 
[lkml]   [2019]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 04/10] KVM/x86: intel_pmu_lbr_enable
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


\
 
 \ /
  Last update: 2019-01-05 11:05    [W:0.073 / U:1.392 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site