Gabe Black has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/53983 )
Change subject: arch,sim-se: Update the PC before emulating a system call.
......................................................................
arch,sim-se: Update the PC before emulating a system call.
For most system calls, it doesn't matter if the PC is advanced to the
instruction after the system call instruction before or after the system
call itself is invoked, because the system call itself doesn't interact
with it.
For some system calls however, specifically "clone" and "execve",
advancing the PC *after* the system call complicates things, because it
means the system call needs to set the PC to something that will equal
the desired value only *after* it's advanced.
By setting the PC *before* the system call, the system call can set the
PC to whatever it needs to. This means the new cloned context doesn't
need to advance the PC because it's already advanced, and execve doesn't
need to set NPC, it can leave the PC set to the correct value from the
entry point set during Process initialization.
Change-Id: I830607c2e9adcc22e738178fd3663417512e2e56
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/53983
Reviewed-by: Jason Lowe-Power <power...@gmail.com>
Maintainer: Jason Lowe-Power <power...@gmail.com>
Tested-by: kokoro <noreply+kok...@google.com>
Reviewed-by: Matt Sinclair <mattdsincl...@gmail.com>
Maintainer: Matt Sinclair <mattdsincl...@gmail.com>
Reviewed-by: Kyle Roarty <kyleroarty1...@gmail.com>
---
M src/arch/x86/linux/se_workload.cc
M src/sim/faults.cc
M src/arch/arm/faults.cc
M src/sim/syscall_emul.hh
M src/arch/riscv/faults.cc
5 files changed, 42 insertions(+), 14 deletions(-)
Approvals:
Jason Lowe-Power: Looks good to me, but someone else must approve; Looks
good to me, approved
Matt Sinclair: Looks good to me, but someone else must approve; Looks
good to me, approved
Kyle Roarty: Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/arch/arm/faults.cc b/src/arch/arm/faults.cc
index e340d07..ba5bcc9 100644
--- a/src/arch/arm/faults.cc
+++ b/src/arch/arm/faults.cc
@@ -881,15 +881,15 @@
return;
}
- // As of now, there isn't a 32 bit thumb version of this instruction.
- assert(!machInst.bigThumb);
- tc->getSystemPtr()->workload->syscall(tc);
-
// Advance the PC since that won't happen automatically.
PCState pc = tc->pcState().as<PCState>();
assert(inst);
inst->advancePC(pc);
tc->pcState(pc);
+
+ // As of now, there isn't a 32 bit thumb version of this instruction.
+ assert(!machInst.bigThumb);
+ tc->getSystemPtr()->workload->syscall(tc);
}
bool
diff --git a/src/arch/riscv/faults.cc b/src/arch/riscv/faults.cc
index 129e767..eea42fe 100644
--- a/src/arch/riscv/faults.cc
+++ b/src/arch/riscv/faults.cc
@@ -157,11 +157,12 @@
if (isInterrupt() && bits(tc->readMiscReg(tvec), 1, 0) == 1)
addr += 4 * _code;
pc_state.set(addr);
+ tc->pcState(pc_state);
} else {
- invokeSE(tc, inst);
inst->advancePC(pc_state);
+ tc->pcState(pc_state);
+ invokeSE(tc, inst);
}
- tc->pcState(pc_state);
}
void
diff --git a/src/arch/x86/linux/se_workload.cc
b/src/arch/x86/linux/se_workload.cc
index f5fa519..d280c7c 100644
--- a/src/arch/x86/linux/se_workload.cc
+++ b/src/arch/x86/linux/se_workload.cc
@@ -120,7 +120,7 @@
Addr eip = pc.pc();
const auto &vsyscall = proc32->getVSyscallPage();
if (eip >= vsyscall.base && eip < vsyscall.base + vsyscall.size) {
- pc.npc(vsyscall.base + vsyscall.vsysexitOffset);
+ pc.set(vsyscall.base + vsyscall.vsysexitOffset);
tc->pcState(pc);
}
syscallDescs32.get(rax)->doSyscall(tc);
diff --git a/src/sim/faults.cc b/src/sim/faults.cc
index 98778f2..115c0ed 100644
--- a/src/sim/faults.cc
+++ b/src/sim/faults.cc
@@ -71,11 +71,12 @@
void
SESyscallFault::invoke(ThreadContext *tc, const StaticInstPtr &inst)
{
- tc->getSystemPtr()->workload->syscall(tc);
// Move the PC forward since that doesn't happen automatically.
std::unique_ptr<PCStateBase> pc(tc->pcState().clone());
inst->advancePC(*pc);
tc->pcState(*pc);
+
+ tc->getSystemPtr()->workload->syscall(tc);
}
void
diff --git a/src/sim/syscall_emul.hh b/src/sim/syscall_emul.hh
index a74aabf..546ae75 100644
--- a/src/sim/syscall_emul.hh
+++ b/src/sim/syscall_emul.hh
@@ -1703,9 +1703,6 @@
desc->returnInto(ctc, 0);
- std::unique_ptr<PCStateBase> cpc(tc->pcState().clone());
- cpc->advance();
- ctc->pcState(*cpc);
ctc->activate();
if (flags & OS::TGT_CLONE_VFORK) {
@@ -2267,9 +2264,6 @@
new_p->init();
new_p->initState();
tc->activate();
- std::unique_ptr<PCStateBase> pc_state(tc->pcState().clone());
- pc_state->advance();
- tc->pcState(*pc_state);
return SyscallReturn();
}
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/53983
To unsubscribe, or for help writing mail filters, visit
https://gem5-review.googlesource.com/settings
Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I830607c2e9adcc22e738178fd3663417512e2e56
Gerrit-Change-Number: 53983
Gerrit-PatchSet: 2
Gerrit-Owner: Gabe Black <gabe.bl...@gmail.com>
Gerrit-Reviewer: Alexandru Duțu <alexandru.d...@amd.com>
Gerrit-Reviewer: Bobby Bruce <bbr...@ucdavis.edu>
Gerrit-Reviewer: Bradford Beckmann <bradford.beckm...@gmail.com>
Gerrit-Reviewer: Gabe Black <gabe.bl...@gmail.com>
Gerrit-Reviewer: Jason Lowe-Power <ja...@lowepower.com>
Gerrit-Reviewer: Jason Lowe-Power <power...@gmail.com>
Gerrit-Reviewer: Kyle Roarty <kyleroarty1...@gmail.com>
Gerrit-Reviewer: Matt Poremba <che...@gmail.com>
Gerrit-Reviewer: Matt Sinclair <mattdsincl...@gmail.com>
Gerrit-Reviewer: kokoro <noreply+kok...@google.com>
Gerrit-MessageType: merged
_______________________________________________
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
