Re: [PATCH V6 2/5] perf/x86/kvm: Avoid unnecessary work in guest filtering

From: Liang, Kan
Date: Mon Feb 04 2019 - 13:57:56 EST




On 2/4/2019 1:15 PM, Borislav Petkov wrote:
On Mon, Feb 04, 2019 at 11:55:27AM -0500, Liang, Kan wrote:

We cannot apply X86_STEPPING_ANY to ignore the stepping. There will be
problems for 0-8 stepping for KABYLAKE_MOBILE.

So why are we even doing this new "interface"
x86_cpu_has_min_microcode_rev() if even at the conversion stage it shows
that it is inadequate?

I think what we need is x86_match_cpu_with_stepping_range().
But I don't think it is worth enabling it just for this rare case.

Sounds to me like you wanna go back to the drawing board after having
evaluated all the use cases.

And yes, I can imagine:

+struct x86_cpu_desc {
+ __u8 x86; /* CPU family */
+ __u8 x86_vendor; /* CPU vendor */
+ __u8 x86_model;
+ __u8 x86_min_stepping;
+ __u8 x86_max_stepping;
+ __u32 x86_microcode_rev;
+};

along with the usage:

INTEL_CPU_DESC(mod, min_step, max_step, rev)

to make it more elegant.

Question is, can you have a given microcode revision X applying to
multiple revisions?

If yes, the above should work...


You mean a given microcode revision X applying to multiple stepping,
right?
I don't think so. I still think the KABYLAKE case is an uncommon case.


Can we do something as below just for this case?

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 3eacdf6..d5fba67 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3745,6 +3745,12 @@ static __init void intel_clovertown_quirk(void)
x86_pmu.pebs_constraints = NULL;
}

+#define INTEL_CPU_DESC_FOUR(mod, start, rev) \
+ INTEL_CPU_DESC(mod, start, rev), \
+ INTEL_CPU_DESC(mod, start + 1, rev), \
+ INTEL_CPU_DESC(mod, start + 2, rev), \
+ INTEL_CPU_DESC(mod, start + 3, rev)
+
static const struct x86_cpu_desc isolation_ucodes[] = {
INTEL_CPU_DESC(INTEL_FAM6_HASWELL_CORE, 3, 0x0000001f),
INTEL_CPU_DESC(INTEL_FAM6_HASWELL_ULT, 1, 0x0000001e),
@@ -3763,15 +3769,8 @@ static const struct x86_cpu_desc isolation_ucodes[] = {
INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_MOBILE, 3, 0x0000007c),
INTEL_CPU_DESC(INTEL_FAM6_SKYLAKE_DESKTOP, 3, 0x0000007c),
INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP, 9, 0x0000004e),
- INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE, 9, 0x0000004e),
- INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE, 10, 0x0000004e),
- INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE, 11, 0x0000004e),
- INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_MOBILE, 12, 0x0000004e),
- INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP, 10, 0x0000004e),
- INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP, 11, 0x0000004e),
- INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP, 12, 0x0000004e),
- INTEL_CPU_DESC(INTEL_FAM6_KABYLAKE_DESKTOP, 13, 0x0000004e),
- INTEL_CPU_DESC(INTEL_FAM6_CANNONLAKE_MOBILE, 3, 0x00000000),
+ INTEL_CPU_DESC_FOUR(INTEL_FAM6_KABYLAKE_MOBILE, 9, 0x0000004e),
+ INTEL_CPU_DESC_FOUR(INTEL_FAM6_KABYLAKE_DESKTOP,10, 0x0000004e),
{}
};

Thanks,
Kan