From 09982dcbe9e2ff730bbf4fe9408b576c3f218259 Mon Sep 17 00:00:00 2001 From: Gabe Black Date: Sun, 13 Dec 2020 01:09:30 -0800 Subject: [PATCH] x86,sim: Remove special handling for KVM in the clone syscall. When a gem5 op is triggered using a KVM MMIO exit event, the PC has already been advanced beyond the offending instruction. Normally when a system call or gem5 op is triggered, the PC has not advanced because the instruction hasn't actually finished executing. This means that if a gem5 op, and by extension a system call in SE mode, want to advance the PC to the instruction after the gem5 op, they have to check whether they were triggered from KVM. To avoid having to special case these sorts of situations (currently only in the clone system call), we can have the code which dispatches to gem5 ops from KVM adjust the next PC so that it points to what the current PC is. That way the PC can be advanced unconditionally, and will point to the instruction after the one that triggered the call. To be fully consistent, we would also need to adjust the current PC. That would be non-trivial since we'd have to figure out where the current instruction started, and that may not even be possible to unambiguously determine given x86's instruction structure. Then we would also need to restore the original PC to avoid confusing KVM. Change-Id: I9ef90b2df8e27334dedc25c59eb45757f7220eea Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/38486 Reviewed-by: Andreas Sandberg Maintainer: Andreas Sandberg Tested-by: kokoro --- src/cpu/kvm/base.cc | 9 +++++++++ src/sim/syscall_emul.hh | 3 +-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/cpu/kvm/base.cc b/src/cpu/kvm/base.cc index b364d4f29..aac6aaf7d 100644 --- a/src/cpu/kvm/base.cc +++ b/src/cpu/kvm/base.cc @@ -1091,6 +1091,15 @@ BaseKvmCPU::doMMIOAccess(Addr paddr, void *data, int size, bool write) pkt->dataStatic(data); if (mmio_req->isLocalAccess()) { + // Since the PC has already been advanced by KVM, set the next + // PC to the current PC. KVM doesn't use that value, and that + // way any gem5 op or syscall which needs to know what the next + // PC is will be able to get a reasonable value. + // + // We won't be able to rewind the current PC to the "correct" + // value without figuring out how big the current instruction + // is, and that's probably not worth the effort. + tc->setNPC(tc->instAddr()); // We currently assume that there is no need to migrate to a // different event queue when doing local accesses. Currently, they // are only used for m5ops, so it should be a valid assumption. diff --git a/src/sim/syscall_emul.hh b/src/sim/syscall_emul.hh index 491a309b4..79cd35a6d 100644 --- a/src/sim/syscall_emul.hh +++ b/src/sim/syscall_emul.hh @@ -1509,8 +1509,7 @@ cloneFunc(SyscallDesc *desc, ThreadContext *tc, RegVal flags, RegVal newStack, desc->returnInto(ctc, 0); TheISA::PCState cpc = tc->pcState(); - if (!p->kvmInSE) - cpc.advance(); + cpc.advance(); ctc->pcState(cpc); ctc->activate(); -- 2.30.2