linux-kernel - Re: [PATCH] KVM: x86: Don't treat ENTER and LEAVE as branches, because they aren't

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aNECoDGVRjy1OMOn@intel.com>
Date: Mon, 22 Sep 2025 16:02:40 +0800
From: Chao Gao <chao.gao@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: Paolo Bonzini <pbonzini@...hat.com>, <kvm@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Jim Mattson <jmattson@...gle.com>
Subject: Re: [PATCH] KVM: x86: Don't treat ENTER and LEAVE as branches,
 because they aren't

On Thu, Sep 18, 2025 at 05:46:39PM -0700, Sean Christopherson wrote:
>Remove the IsBranch flag from ENTER and LEAVE in KVM's emulator, as ENTER
>and LEAVE are stack operations, not branches.  Add forced emulation of
>said instructions to the PMU counters test to prove that KVM diverges from
>hardware, and to guard against regressions.
>
>Fixes: 018d70ffcfec ("KVM: x86: Update vPMCs when retiring branch instructions")
>Cc: Jim Mattson <jmattson@...gle.com>
>Signed-off-by: Sean Christopherson <seanjc@...gle.com>

Reviewed-by: Chao Gao <chao.gao@...el.com>

one nit below:

>---
> arch/x86/kvm/emulate.c                              | 4 ++--
> tools/testing/selftests/kvm/x86/pmu_counters_test.c | 8 +++++---
> 2 files changed, 7 insertions(+), 5 deletions(-)
>
>diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>index 542d3664afa3..23929151a5b8 100644
>--- a/arch/x86/kvm/emulate.c
>+++ b/arch/x86/kvm/emulate.c
>@@ -4330,8 +4330,8 @@ static const struct opcode opcode_table[256] = {
> 	I(DstReg | SrcMemFAddr | ModRM | No64 | Src2DS, em_lseg),
> 	G(ByteOp, group11), G(0, group11),
> 	/* 0xC8 - 0xCF */
>-	I(Stack | SrcImmU16 | Src2ImmByte | IsBranch, em_enter),
>-	I(Stack | IsBranch, em_leave),
>+	I(Stack | SrcImmU16 | Src2ImmByte, em_enter),
>+	I(Stack, em_leave),
> 	I(ImplicitOps | SrcImmU16 | IsBranch, em_ret_far_imm),
> 	I(ImplicitOps | IsBranch, em_ret_far),
> 	D(ImplicitOps | IsBranch), DI(SrcImmByte | IsBranch, intn),
>diff --git a/tools/testing/selftests/kvm/x86/pmu_counters_test.c b/tools/testing/selftests/kvm/x86/pmu_counters_test.c
>index 8aaaf25b6111..89c1e462cd1c 100644
>--- a/tools/testing/selftests/kvm/x86/pmu_counters_test.c
>+++ b/tools/testing/selftests/kvm/x86/pmu_counters_test.c
>@@ -14,10 +14,10 @@
> #define NUM_BRANCH_INSNS_RETIRED	(NUM_LOOPS)
> 
> /*
>- * Number of instructions in each loop. 1 CLFLUSH/CLFLUSHOPT/NOP, 1 MFENCE,
>- * 1 LOOP.
>+ * Number of instructions in each loop. 1 ENTER, 1 CLFLUSH/CLFLUSHOPT/NOP,
>+ * 1 MFENCE, 1 LEAVE, 1 LOOP.

	      ^ 1 MOV,

7803339fa929 ("Use data load to trigger LLC references/misses in Intel PMU")
forgot to update this comment. Otherwise it is a bit confusing that the comment
lists only 5 instructions while the macro is 6.


>  */
>-#define NUM_INSNS_PER_LOOP		4
>+#define NUM_INSNS_PER_LOOP		6
> 
> /*
>  * Number of "extra" instructions that will be counted, i.e. the number of
>@@ -210,9 +210,11 @@ do {										\
> 	__asm__ __volatile__("wrmsr\n\t"					\
> 			     " mov $" __stringify(NUM_LOOPS) ", %%ecx\n\t"	\
> 			     "1:\n\t"						\
>+			     FEP "enter $0, $0\n\t"				\
> 			     clflush "\n\t"					\
> 			     "mfence\n\t"					\
> 			     "mov %[m], %%eax\n\t"				\
>+			     FEP "leave\n\t"					\
> 			     FEP "loop 1b\n\t"					\
> 			     FEP "mov %%edi, %%ecx\n\t"				\
> 			     FEP "xor %%eax, %%eax\n\t"				\
>
>base-commit: c8fbf7ceb2ae3f64b0c377c8c21f6df577a13eb4
>-- 
>2.51.0.470.ga7dc726c21-goog
>
>