Re: [PATCH v2 2/8] KVM: x86: Use a new flag for branch instructions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 8/16/2023 6:51 AM, Sean Christopherson wrote:
Branch *targets*, not branch instructions.

On Wed, Jul 19, 2023, Zeng Guang wrote:
From: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx>

Use the new flag X86EMUL_F_BRANCH instead of X86EMUL_F_FETCH in
assign_eip(), since strictly speaking it is not behavior of instruction
fetch.
Eh, I'd just drop this paragraph, as evidenced by this code existing as-is for
years, we wouldn't introduce X86EMUL_F_BRANCH just because resolving a branch
target isn't strictly an instruction fetch.

Another reason is to distinguish instruction fetch and execution of branch
instruction for feature(s) that handle differently on them.
Similar to the shortlog, it's about computing the branch target, not executing a
branch instruction.  That distinction matters, e.g. a Jcc that is not taken will
*not* follow the branch target, but the instruction is still *executed*.  And there
exist instructions that compute branch targets, but aren't what most people would
typically consider a branch instruction, e.g. XBEGIN.

Branch instruction is not data access instruction, so skip checking against
execute-only code segment as instruction fetch.
Rather than call out individual use case, I would simply state that as of this
patch, X86EMUL_F_BRANCH and X86EMUL_F_FETCH are identical as far as KVM is
concernered.  That let's the reader know that (a) there's no intended change in
behavior and (b) that the intent is to effectively split all consumption of
X86EMUL_F_FETCH into (X86EMUL_F_FETCH | X86EMUL_F_BRANCH).

How about this:

    KVM: x86: Use a new flag for branch targets

    Use the new flag X86EMUL_F_BRANCH instead of X86EMUL_F_FETCH in assign_eip()     to distinguish instruction fetch and branch target computation for feature(s)
    that handle differently on them.

    As of this patch, X86EMUL_F_BRANCH and X86EMUL_F_FETCH are identical as far as
    KVM is concernered.

    No functional change intended.


Signed-off-by: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx>
Signed-off-by: Zeng Guang <guang.zeng@xxxxxxxxx>
---
  arch/x86/kvm/emulate.c     | 5 +++--
  arch/x86/kvm/kvm_emulate.h | 1 +
  2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 3ddfbc99fa4f..8e706d19ae45 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -721,7 +721,8 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt,
  		    (flags & X86EMUL_F_WRITE))
  			goto bad;
  		/* unreadable code segment */
-		if (!(flags & X86EMUL_F_FETCH) && (desc.type & 8) && !(desc.type & 2))
+		if (!(flags & (X86EMUL_F_FETCH | X86EMUL_F_BRANCH))
+			&& (desc.type & 8) && !(desc.type & 2))
Put the && on the first line, and align indendation.
I should have been more careful on the alignment & indentation.
Will update it. Thanks.


		/* unreadable code segment */
		if (!(flags & (X86EMUL_F_FETCH | X86EMUL_F_BRANCH)) &&
		    (desc.type & 8) && !(desc.type & 2))
			goto bad;




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux