[gem5-dev] [M] Change in gem5/gem5[develop]: mem: Fix SHM server path cleanup logic
Jui-min Lee has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/65153?usp=email )
Change subject: mem: Fix SHM server path cleanup logic
..
mem: Fix SHM server path cleanup logic
Previously, shared memory server remove old socket *before* filling the
target path into API's data structure. However, the target path might
get truncated hence the path we check against might not be the one we
will be using in the end.
In a case where the path specified by user is free while the truncated
path is in used, gem5 will get a mysterious EADDRINUSE.
We swap the two steps in the CL, so we'll be checking against the actual
path we use, instead of the path user request to use.
Change-Id: Ib34f8b00ea1d2f15dcd4e7b6d2d4a6d6ddc4e411
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/65153
Reviewed-by: Gabe Black
Tested-by: kokoro
Maintainer: Gabe Black
---
M src/mem/shared_memory_server.cc
1 file changed, 59 insertions(+), 35 deletions(-)
Approvals:
Gabe Black: Looks good to me, approved; Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/mem/shared_memory_server.cc
b/src/mem/shared_memory_server.cc
index ae5043f..bee663b 100644
--- a/src/mem/shared_memory_server.cc
+++ b/src/mem/shared_memory_server.cc
@@ -56,47 +56,50 @@
system(params.system), serverFd(-1)
{
fatal_if(system == nullptr, "Requires a system to share memory from!");
-// Ensure the unix socket path to use is not occupied. Also, if there's
-// actually anything to be removed, warn the user something might be
off.
-if (unlink(unixSocketPath.c_str()) == 0) {
-warn(
-"The server path %s was occupied and will be replaced. Please "
-"make sure there is no other server using the same path.",
-unixSocketPath.c_str());
-}
// Create a new unix socket.
serverFd = ListenSocket::socketCloexec(AF_UNIX, SOCK_STREAM, 0);
-panic_if(serverFd < 0, "%s: cannot create unix socket: %s",
name().c_str(),
+panic_if(serverFd < 0, "%s: cannot create unix socket: %s", name(),
strerror(errno));
// Bind to the specified path.
sockaddr_un serv_addr = {};
serv_addr.sun_family = AF_UNIX;
strncpy(serv_addr.sun_path, unixSocketPath.c_str(),
sizeof(serv_addr.sun_path) - 1);
-warn_if(strlen(serv_addr.sun_path) != unixSocketPath.size(),
-"%s: unix socket path truncated, expect '%s' but get '%s'",
-name().c_str(), unixSocketPath.c_str(), serv_addr.sun_path);
+// If the target path is truncated, warn the user that the actual path
is
+// different and update the target path.
+if (strlen(serv_addr.sun_path) != unixSocketPath.size()) {
+warn("%s: unix socket path truncated, expect '%s' but get '%s'",
+ name(), unixSocketPath, serv_addr.sun_path);
+unixSocketPath = serv_addr.sun_path;
+}
+// Ensure the unix socket path to use is not occupied. Also, if there's
+// actually anything to be removed, warn the user something might be
off.
+bool old_sock_removed = unlink(unixSocketPath.c_str()) == 0;
+warn_if(old_sock_removed,
+"%s: the server path %s was occupied and will be replaced.
Please "
+"make sure there is no other server using the same path.",
+name(), unixSocketPath);
int bind_retv = bind(serverFd, reinterpret_cast(_addr),
sizeof(serv_addr));
-fatal_if(bind_retv != 0, "%s: cannot bind unix socket: %s",
name().c_str(),
+fatal_if(bind_retv != 0, "%s: cannot bind unix socket: %s", name(),
strerror(errno));
// Start listening.
int listen_retv = listen(serverFd, 1);
-fatal_if(listen_retv != 0, "%s: listen failed: %s", name().c_str(),
+fatal_if(listen_retv != 0, "%s: listen failed: %s", name(),
strerror(errno));
listenSocketEvent.reset(new ListenSocketEvent(serverFd, this));
pollQueue.schedule(listenSocketEvent.get());
-inform("%s: listening at %s", name().c_str(), unixSocketPath.c_str());
+inform("%s: listening at %s", name(), unixSocketPath);
}
SharedMemoryServer::~SharedMemoryServer()
{
int unlink_retv = unlink(unixSocketPath.c_str());
-warn_if(unlink_retv != 0, "%s: cannot unlink unix socket: %s",
-name().c_str(), strerror(errno));
+warn_if(unlink_retv != 0, "%s: cannot unlink unix socket: %s", name(),
+strerror(errno));
int close_retv = close(serverFd);
-warn_if(close_retv != 0, "%s: cannot close unix socket: %s",
-name().c_str(), strerror(errno));
+warn_if(close_retv != 0, "%s: cannot close unix socket: %s", name(),
+strerror(errno));
}
SharedMemoryServer::BaseShmPollEvent::BaseShmPollEvent(
@@ -121,7 +124,7 @@
if (retv >= 0) {
offset += retv;
} else if (errno
[gem5-dev] [S] Change in gem5/gem5[develop]: mem: Fix SHM server path cleanup logic
Jui-min Lee has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/65153?usp=email )
Change subject: mem: Fix SHM server path cleanup logic
..
mem: Fix SHM server path cleanup logic
Previously, shared memory server remove old socket *before* filling the
target path into API's data structure. However, the target path might
get truncated hence the path we check against might not be the one we
will be using in the end.
In a case where the path specified by user is free while the truncated
path is in used, gem5 will get a mysterious EADDRINUSE.
We swap the two steps in the CL, so we'll be checking against the actual
path we use, instead of the path user request to use.
Change-Id: Ib34f8b00ea1d2f15dcd4e7b6d2d4a6d6ddc4e411
---
M src/mem/shared_memory_server.cc
1 file changed, 35 insertions(+), 11 deletions(-)
diff --git a/src/mem/shared_memory_server.cc
b/src/mem/shared_memory_server.cc
index ae5043f..52f599f 100644
--- a/src/mem/shared_memory_server.cc
+++ b/src/mem/shared_memory_server.cc
@@ -56,14 +56,6 @@
system(params.system), serverFd(-1)
{
fatal_if(system == nullptr, "Requires a system to share memory from!");
-// Ensure the unix socket path to use is not occupied. Also, if there's
-// actually anything to be removed, warn the user something might be
off.
-if (unlink(unixSocketPath.c_str()) == 0) {
-warn(
-"The server path %s was occupied and will be replaced. Please "
-"make sure there is no other server using the same path.",
-unixSocketPath.c_str());
-}
// Create a new unix socket.
serverFd = ListenSocket::socketCloexec(AF_UNIX, SOCK_STREAM, 0);
panic_if(serverFd < 0, "%s: cannot create unix socket: %s",
name().c_str(),
@@ -73,9 +65,21 @@
serv_addr.sun_family = AF_UNIX;
strncpy(serv_addr.sun_path, unixSocketPath.c_str(),
sizeof(serv_addr.sun_path) - 1);
-warn_if(strlen(serv_addr.sun_path) != unixSocketPath.size(),
-"%s: unix socket path truncated, expect '%s' but get '%s'",
-name().c_str(), unixSocketPath.c_str(), serv_addr.sun_path);
+// If the target path is truncated, warn the user that the actual path
is
+// different and update the target path.
+if (strlen(serv_addr.sun_path) != unixSocketPath.size()) {
+warn("%s: unix socket path truncated, expect '%s' but get '%s'",
+ name().c_str(), unixSocketPath.c_str(), serv_addr.sun_path);
+unixSocketPath = serv_addr.sun_path;
+}
+// Ensure the unix socket path to use is not occupied. Also, if there's
+// actually anything to be removed, warn the user something might be
off.
+if (unlink(unixSocketPath.c_str()) == 0) {
+warn(
+"The server path %s was occupied and will be replaced. Please "
+"make sure there is no other server using the same path.",
+unixSocketPath.c_str());
+}
int bind_retv = bind(serverFd, reinterpret_cast(_addr),
sizeof(serv_addr));
fatal_if(bind_retv != 0, "%s: cannot bind unix socket: %s",
name().c_str(),
--
To view, visit
https://gem5-review.googlesource.com/c/public/gem5/+/65153?usp=email
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: Ib34f8b00ea1d2f15dcd4e7b6d2d4a6d6ddc4e411
Gerrit-Change-Number: 65153
Gerrit-PatchSet: 1
Gerrit-Owner: Jui-min Lee
Gerrit-MessageType: newchange
___
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
[gem5-dev] Change in gem5/gem5[develop]: mem: Align mmap offset to page boundary
Jui-min Lee has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/58289 )
Change subject: mem: Align mmap offset to page boundary
..
mem: Align mmap offset to page boundary
If we create abstract memories with a sub-page size on a system with
shared backstore, the offset of next mmap might become non-page-align
and cause an invalid argument error.
In this CL, we always upscale the range size to multiple of page before
updating the offset, so the offset is always on page boundary.
Change-Id: I3a6adf312f2cb5a09ee6a24a87adc62b630eac66
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/58289
Reviewed-by: Gabe Black
Maintainer: Gabe Black
Reviewed-by: Boris Shingarov
Tested-by: kokoro
---
M src/mem/physical.cc
M src/mem/physical.hh
2 files changed, 29 insertions(+), 2 deletions(-)
Approvals:
Boris Shingarov: Looks good to me, approved
Gabe Black: Looks good to me, but someone else must approve; Looks good
to me, approved
kokoro: Regressions pass
diff --git a/src/mem/physical.cc b/src/mem/physical.cc
index cae37a0..06f2cdc 100644
--- a/src/mem/physical.cc
+++ b/src/mem/physical.cc
@@ -50,6 +50,7 @@
#include
#include
+#include "base/intmath.hh"
#include "base/trace.hh"
#include "debug/AddrRanges.hh"
#include "debug/Checkpoint.hh"
@@ -81,7 +82,8 @@
const std::string& shared_backstore,
bool auto_unlink_shared_backstore) :
_name(_name), size(0), mmapUsingNoReserve(mmap_using_noreserve),
-sharedBackstore(shared_backstore), sharedBackstoreSize(0)
+sharedBackstore(shared_backstore), sharedBackstoreSize(0),
+pageSize(sysconf(_SC_PAGE_SIZE))
{
// Register cleanup callback if requested.
if (auto_unlink_shared_backstore && !sharedBackstore.empty()) {
@@ -217,7 +219,9 @@
} else {
// Newly create backstore will be located after previous one.
map_offset = sharedBackstoreSize;
-sharedBackstoreSize += range.size();
+// mmap requires the offset to be multiple of page, so we need to
+// upscale the range size.
+sharedBackstoreSize += roundUp(range.size(), pageSize);
DPRINTF(AddrRanges, "Sharing backing store as %s at offset %llu\n",
sharedBackstore.c_str(), (uint64_t)map_offset);
shm_fd = shm_open(sharedBackstore.c_str(), O_CREAT | O_RDWR, 0666);
diff --git a/src/mem/physical.hh b/src/mem/physical.hh
index 3a976ed..4997d80 100644
--- a/src/mem/physical.hh
+++ b/src/mem/physical.hh
@@ -156,6 +156,8 @@
const std::string sharedBackstore;
uint64_t sharedBackstoreSize;
+long pageSize;
+
// The physical memory used to provide the memory in the simulated
// system
std::vector backingStore;
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/58289
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: I3a6adf312f2cb5a09ee6a24a87adc62b630eac66
Gerrit-Change-Number: 58289
Gerrit-PatchSet: 3
Gerrit-Owner: Jui-min Lee
Gerrit-Reviewer: Boris Shingarov
Gerrit-Reviewer: Earl Ou
Gerrit-Reviewer: Gabe Black
Gerrit-Reviewer: Jui-min Lee
Gerrit-Reviewer: Nikos Nikoleris
Gerrit-Reviewer: Yu-hsin Wang
Gerrit-Reviewer: kokoro
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
[gem5-dev] Change in gem5/gem5[develop]: mem: Align mmap offset to page boundary
Jui-min Lee has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/58289 )
Change subject: mem: Align mmap offset to page boundary
..
mem: Align mmap offset to page boundary
If we create abstract memories with a sub-page size on a system with
shared backstore, the offset of next mmap might become non-page-align
and cause an invalid argument error.
In this CL, we always upscale the range size to multiple of page before
updating the offset, so the offset is always on page boundary.
Change-Id: I3a6adf312f2cb5a09ee6a24a87adc62b630eac66
---
M src/mem/physical.cc
M src/mem/physical.hh
2 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/src/mem/physical.cc b/src/mem/physical.cc
index cae37a0..94438db 100644
--- a/src/mem/physical.cc
+++ b/src/mem/physical.cc
@@ -50,6 +50,7 @@
#include
#include
+#include "base/intmath.hh"
#include "base/trace.hh"
#include "debug/AddrRanges.hh"
#include "debug/Checkpoint.hh"
@@ -81,7 +82,8 @@
const std::string& shared_backstore,
bool auto_unlink_shared_backstore) :
_name(_name), size(0), mmapUsingNoReserve(mmap_using_noreserve),
-sharedBackstore(shared_backstore), sharedBackstoreSize(0)
+sharedBackstore(shared_backstore), sharedBackstoreSize(0),
+pageSize(sysconf(_SC_PAGE_SIZE))
{
// Register cleanup callback if requested.
if (auto_unlink_shared_backstore && !sharedBackstore.empty()) {
@@ -217,7 +219,9 @@
} else {
// Newly create backstore will be located after previous one.
map_offset = sharedBackstoreSize;
-sharedBackstoreSize += range.size();
+// mmap requires the offset to be multiple of page, so we need to
+// upscale the range size.
+sharedBackstoreSize += divCeil(range.size(), pageSize) * pageSize;
DPRINTF(AddrRanges, "Sharing backing store as %s at offset %llu\n",
sharedBackstore.c_str(), (uint64_t)map_offset);
shm_fd = shm_open(sharedBackstore.c_str(), O_CREAT | O_RDWR, 0666);
diff --git a/src/mem/physical.hh b/src/mem/physical.hh
index 3a976ed..4997d80 100644
--- a/src/mem/physical.hh
+++ b/src/mem/physical.hh
@@ -156,6 +156,8 @@
const std::string sharedBackstore;
uint64_t sharedBackstoreSize;
+long pageSize;
+
// The physical memory used to provide the memory in the simulated
// system
std::vector backingStore;
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/58289
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: I3a6adf312f2cb5a09ee6a24a87adc62b630eac66
Gerrit-Change-Number: 58289
Gerrit-PatchSet: 1
Gerrit-Owner: Jui-min Lee
Gerrit-MessageType: newchange
___
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
[gem5-dev] Change in gem5/gem5[develop]: mem: Add SharedMemoryServer
Jui-min Lee has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/57729 )
Change subject: mem: Add SharedMemoryServer
..
mem: Add SharedMemoryServer
Add an utility class that provides a service for another process
query and get the fd of the corresponding region in gem5's physmem.
Basically, the service works in this way:
1. client connect to the unix socket created by a SharedMemoryServer
2. client send a request {start, end} to gem5
3. the server locates the corresponding shared memory
4. gem5 response {offset} and pass {fd} in ancillary data
mmap fd at offset will provide the client the view into the physical
memory of the request range.
Change-Id: I9d42fd8a41fc28dcfebb45dec10bc9ebb8e21d11
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/57729
Reviewed-by: Gabe Black
Maintainer: Gabe Black
Reviewed-by: Boris Shingarov
Tested-by: kokoro
---
M src/base/socket.hh
M src/mem/SConscript
A src/mem/SharedMemoryServer.py
M src/mem/physical.cc
M src/mem/physical.hh
A src/mem/shared_memory_server.cc
A src/mem/shared_memory_server.hh
7 files changed, 410 insertions(+), 11 deletions(-)
Approvals:
Boris Shingarov: Looks good to me, approved
Gabe Black: Looks good to me, approved; Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/base/socket.hh b/src/base/socket.hh
index 4ed6185..3375ccc 100644
--- a/src/base/socket.hh
+++ b/src/base/socket.hh
@@ -62,14 +62,6 @@
*/
static void cleanup();
- private:
-/* Create a socket, adding SOCK_CLOEXEC if available. */
-static int socketCloexec(int domain, int type, int protocol);
-/* Accept a connection, adding SOCK_CLOEXEC if available. */
-static int acceptCloexec(int sockfd, struct sockaddr *addr,
- socklen_t *addrlen);
-
-
public:
/**
* @ingroup api_socket
@@ -84,6 +76,12 @@
int getfd() const { return fd; }
bool islistening() const { return listening; }
+
+/* Create a socket, adding SOCK_CLOEXEC if available. */
+static int socketCloexec(int domain, int type, int protocol);
+/* Accept a connection, adding SOCK_CLOEXEC if available. */
+static int acceptCloexec(int sockfd, struct sockaddr *addr,
+ socklen_t *addrlen);
/** @} */ // end of api_socket
};
diff --git a/src/mem/SConscript b/src/mem/SConscript
index 7790e1d..e07942e 100644
--- a/src/mem/SConscript
+++ b/src/mem/SConscript
@@ -56,6 +56,7 @@
SimObject('ExternalMaster.py', sim_objects=['ExternalMaster'])
SimObject('ExternalSlave.py', sim_objects=['ExternalSlave'])
SimObject('CfiMemory.py', sim_objects=['CfiMemory'])
+SimObject('SharedMemoryServer.py', sim_objects=['SharedMemoryServer'])
SimObject('SimpleMemory.py', sim_objects=['SimpleMemory'])
SimObject('XBar.py', sim_objects=[
'BaseXBar', 'NoncoherentXBar', 'CoherentXBar', 'SnoopFilter'])
@@ -80,6 +81,7 @@
Source('packet_queue.cc')
Source('port_proxy.cc')
Source('physical.cc')
+Source('shared_memory_server.cc')
Source('simple_mem.cc')
Source('snoop_filter.cc')
Source('stack_dist_calc.cc')
diff --git a/src/mem/SharedMemoryServer.py b/src/mem/SharedMemoryServer.py
new file mode 100644
index 000..3a63f45
--- /dev/null
+++ b/src/mem/SharedMemoryServer.py
@@ -0,0 +1,15 @@
+from m5.SimObject import SimObject
+from m5.params import Param
+from m5.proxy import Parent
+
+
+class SharedMemoryServer(SimObject):
+type = "SharedMemoryServer"
+cxx_header = "mem/shared_memory_server.hh"
+cxx_class = "gem5::memory::SharedMemoryServer"
+
+system = Param.System(
+Parent.any,
+"The system where the target shared memory is actually stored.")
+server_path = Param.String(
+"The unix socket path where the server should be running upon.")
diff --git a/src/mem/physical.cc b/src/mem/physical.cc
index 08707eb..cae37a0 100644
--- a/src/mem/physical.cc
+++ b/src/mem/physical.cc
@@ -247,7 +247,8 @@
// remember this backing store so we can checkpoint it and unmap
// it appropriately
backingStore.emplace_back(range, pmem,
- conf_table_reported, in_addr_map, kvm_map);
+ conf_table_reported, in_addr_map, kvm_map,
+ shm_fd, map_offset);
// point the memories to their backing store
for (const auto& m : _memories) {
diff --git a/src/mem/physical.hh b/src/mem/physical.hh
index ff0dc61..3a976ed 100644
--- a/src/mem/physical.hh
+++ b/src/mem/physical.hh
@@ -70,9 +70,11 @@
* pointers, because PhysicalMemory is responsible for that.
*/
BackingStoreEntry(AddrRange range, uint8_t* pmem,
- bool conf_table_reported, bool in_addr_map, bool
kvm_map)
+ bool conf_table_reported, bool in_addr_map, bool
kvm_map,
+ int shm_fd=-1, off_t shm_offset=0)
: range(range),
[gem5-dev] Change in gem5/gem5[develop]: mem: Add option to remove shared memory at the end
Jui-min Lee has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/57469 )
Change subject: mem: Add option to remove shared memory at the end
..
mem: Add option to remove shared memory at the end
Add a new option `auto_unlink_shared_backstore` to System so it will
remove the shared backstore used in physical memories when the System is
getting destructed. This will prevent unintended memory leak.
If the shared memory is designed to live through multiple round of
simulations, you may set the option to false to prevent the removal.
Test: Run a simulation with shared_backstore set, and see whether there
is anything left in /dev/shm/ after simulation ends.
Change-Id: I0267b643bd24e62cb7571674fe98f831c13a586d
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/57469
Reviewed-by: Daniel Carvalho
Maintainer: Daniel Carvalho
Tested-by: kokoro
---
M src/mem/physical.cc
M src/mem/physical.hh
M src/sim/System.py
M src/sim/system.cc
4 files changed, 36 insertions(+), 3 deletions(-)
Approvals:
Daniel Carvalho: Looks good to me, approved; Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/mem/physical.cc b/src/mem/physical.cc
index a7e261f..08707eb 100644
--- a/src/mem/physical.cc
+++ b/src/mem/physical.cc
@@ -55,6 +55,7 @@
#include "debug/Checkpoint.hh"
#include "mem/abstract_mem.hh"
#include "sim/serialize.hh"
+#include "sim/sim_exit.hh"
/**
* On Linux, MAP_NORESERVE allow us to simulate a very large memory
@@ -77,10 +78,16 @@
PhysicalMemory::PhysicalMemory(const std::string& _name,
const std::vector&
_memories,
bool mmap_using_noreserve,
- const std::string& shared_backstore) :
+ const std::string& shared_backstore,
+ bool auto_unlink_shared_backstore) :
_name(_name), size(0), mmapUsingNoReserve(mmap_using_noreserve),
sharedBackstore(shared_backstore), sharedBackstoreSize(0)
{
+// Register cleanup callback if requested.
+if (auto_unlink_shared_backstore && !sharedBackstore.empty()) {
+registerExitCallback([=]() { shm_unlink(shared_backstore.c_str());
});
+}
+
if (mmap_using_noreserve)
warn("Not reserving swap space. May cause SIGSEGV on actual
usage\n");
diff --git a/src/mem/physical.hh b/src/mem/physical.hh
index bb90664..ff0dc61 100644
--- a/src/mem/physical.hh
+++ b/src/mem/physical.hh
@@ -174,7 +174,8 @@
PhysicalMemory(const std::string& _name,
const std::vector& _memories,
bool mmap_using_noreserve,
- const std::string& shared_backstore);
+ const std::string& shared_backstore,
+ bool auto_unlink_shared_backstore);
/**
* Unmap all the backing store we have used.
diff --git a/src/sim/System.py b/src/sim/System.py
index 499cf9b..b5bd5df 100644
--- a/src/sim/System.py
+++ b/src/sim/System.py
@@ -87,6 +87,9 @@
shared_backstore = Param.String("", "backstore's shmem segment
filename, "
"use to directly address the backstore from another host-OS
process. "
"Leave this empty to unset the MAP_SHARED flag.")
+auto_unlink_shared_backstore = Param.Bool(False, "Automatically remove
the "
+"shmem segment file upon destruction. This is used only if "
+"shared_backstore is non-empty.")
cache_line_size = Param.Unsigned(64, "Cache line size in bytes")
diff --git a/src/sim/system.cc b/src/sim/system.cc
index d5e5e3f..b7fba8a 100644
--- a/src/sim/system.cc
+++ b/src/sim/system.cc
@@ -182,7 +182,7 @@
physProxy(_systemPort, p.cache_line_size),
workload(p.workload),
physmem(name() + ".physmem", p.memories, p.mmap_using_noreserve,
- p.shared_backstore),
+ p.shared_backstore, p.auto_unlink_shared_backstore),
ShadowRomRanges(p.shadow_rom_ranges.begin(),
p.shadow_rom_ranges.end()),
memoryMode(p.mem_mode),
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/57469
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: I0267b643bd24e62cb7571674fe98f831c13a586d
Gerrit-Change-Number: 57469
Gerrit-PatchSet: 8
Gerrit-Owner: Jui-min Lee
Gerrit-Reviewer: Daniel Carvalho
Gerrit-Reviewer: Gabe Black
Gerrit-Reviewer: Jui-min Lee
Gerrit-Reviewer: Nikos Nikoleris
Gerrit-Reviewer: kokoro
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
[gem5-dev] Change in gem5/gem5[develop]: mem: Add SharedMemoryServer
Jui-min Lee has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/57729 )
Change subject: mem: Add SharedMemoryServer
..
mem: Add SharedMemoryServer
Add an utility class that provides a service for another process
query and get the fd of the corresponding region in gem5's physmem.
We also add a new option in System.py so people can specify the path of
the unix socket providing such service.
Basically, the service works in this way:
1. client connect to the unix socket created by a gem5 system
2. client send a request {start, end} to gem5
3. the system locates the corresponding shared memory
4. gem5 response {offset} and pass {fd} in ancillary data
mmap fd at offset will provide the client the view into the physical
memory of the request range.
Bug: 222145732
Test: None
Change-Id: I9d42fd8a41fc28dcfebb45dec10bc9ebb8e21d11
---
M src/base/socket.hh
M src/mem/SConscript
M src/mem/physical.cc
M src/mem/physical.hh
A src/mem/shared_memory_server.cc
A src/mem/shared_memory_server.hh
M src/sim/System.py
M src/sim/system.cc
M src/sim/system.hh
9 files changed, 356 insertions(+), 11 deletions(-)
diff --git a/src/base/socket.hh b/src/base/socket.hh
index 4ed6185..3375ccc 100644
--- a/src/base/socket.hh
+++ b/src/base/socket.hh
@@ -62,14 +62,6 @@
*/
static void cleanup();
- private:
-/* Create a socket, adding SOCK_CLOEXEC if available. */
-static int socketCloexec(int domain, int type, int protocol);
-/* Accept a connection, adding SOCK_CLOEXEC if available. */
-static int acceptCloexec(int sockfd, struct sockaddr *addr,
- socklen_t *addrlen);
-
-
public:
/**
* @ingroup api_socket
@@ -84,6 +76,12 @@
int getfd() const { return fd; }
bool islistening() const { return listening; }
+
+/* Create a socket, adding SOCK_CLOEXEC if available. */
+static int socketCloexec(int domain, int type, int protocol);
+/* Accept a connection, adding SOCK_CLOEXEC if available. */
+static int acceptCloexec(int sockfd, struct sockaddr *addr,
+ socklen_t *addrlen);
/** @} */ // end of api_socket
};
diff --git a/src/mem/SConscript b/src/mem/SConscript
index 7790e1d..c0cc4d7 100644
--- a/src/mem/SConscript
+++ b/src/mem/SConscript
@@ -80,6 +80,7 @@
Source('packet_queue.cc')
Source('port_proxy.cc')
Source('physical.cc')
+Source('shared_memory_server.cc')
Source('simple_mem.cc')
Source('snoop_filter.cc')
Source('stack_dist_calc.cc')
diff --git a/src/mem/physical.cc b/src/mem/physical.cc
index ebc9ca8..c012ef7 100644
--- a/src/mem/physical.cc
+++ b/src/mem/physical.cc
@@ -248,7 +248,8 @@
// remember this backing store so we can checkpoint it and unmap
// it appropriately
backingStore.emplace_back(range, pmem,
- conf_table_reported, in_addr_map, kvm_map);
+ conf_table_reported, in_addr_map, kvm_map,
+ shm_fd, map_offset);
// point the memories to their backing store
for (const auto& m : _memories) {
diff --git a/src/mem/physical.hh b/src/mem/physical.hh
index ff0dc61..37bcd46 100644
--- a/src/mem/physical.hh
+++ b/src/mem/physical.hh
@@ -70,9 +70,11 @@
* pointers, because PhysicalMemory is responsible for that.
*/
BackingStoreEntry(AddrRange range, uint8_t* pmem,
- bool conf_table_reported, bool in_addr_map, bool
kvm_map)
+ bool conf_table_reported, bool in_addr_map, bool
kvm_map,
+ int shm_fd, off_t shm_offset)
: range(range), pmem(pmem), confTableReported(conf_table_reported),
- inAddrMap(in_addr_map), kvmMap(kvm_map)
+ inAddrMap(in_addr_map), kvmMap(kvm_map), shmFd(shm_fd),
+ shmOffset(shm_offset)
{}
/**
@@ -101,6 +103,19 @@
* acceleration.
*/
bool kvmMap;
+
+ /**
+ * If this backing store is based on a shared memory, this is the fd
to
+ * the shared memory. Otherwise, it should be -1.
+ */
+ int shmFd;
+
+ /**
+ * If this backing store is based on a shared memory, this is the
offset
+ * of this backing store in the share memory. Otherwise, the value is
+ * unspecified and should not be rely upon.
+ */
+ off_t shmOffset;
};
/**
diff --git a/src/mem/shared_memory_server.cc
b/src/mem/shared_memory_server.cc
new file mode 100644
index 000..3b78527
--- /dev/null
+++ b/src/mem/shared_memory_server.cc
@@ -0,0 +1,206 @@
+/*
+ * Copyright 2022 Google, Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met: redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer;
+ * redistributions in
[gem5-dev] Change in gem5/gem5[develop]: mem: Add option to remove shared memory at the end
Jui-min Lee has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/57469 )
Change subject: mem: Add option to remove shared memory at the end
..
mem: Add option to remove shared memory at the end
Add a new option `auto_unlink_shared_backstore` to System so it will
remove the shared backstore used in physical memories when the System is
getting destructed. This will prevent unintended memory leak.
If the shared memory is designed to live through multiple round of
simulations, you may set the option to false to prevent the removal.
Test: Run a simulation with shared_backstore set, and see whether there
is anything left in /dev/shm/ after simulation ends.
Change-Id: I0267b643bd24e62cb7571674fe98f831c13a586d
---
M src/mem/physical.cc
M src/mem/physical.hh
M src/sim/System.py
M src/sim/system.cc
4 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/src/mem/physical.cc b/src/mem/physical.cc
index a7e261f..4e0cfbc 100644
--- a/src/mem/physical.cc
+++ b/src/mem/physical.cc
@@ -77,9 +77,12 @@
PhysicalMemory::PhysicalMemory(const std::string& _name,
const std::vector&
_memories,
bool mmap_using_noreserve,
- const std::string& shared_backstore) :
+ const std::string& shared_backstore,
+ bool auto_unlink_shared_backstore) :
_name(_name), size(0), mmapUsingNoReserve(mmap_using_noreserve),
-sharedBackstore(shared_backstore), sharedBackstoreSize(0)
+sharedBackstore(shared_backstore),
+autoUnlinkSharedBackstore(auto_unlink_shared_backstore),
+sharedBackstoreSize(0)
{
if (mmap_using_noreserve)
warn("Not reserving swap space. May cause SIGSEGV on actual
usage\n");
@@ -255,6 +258,8 @@
// unmap the backing store
for (auto& s : backingStore)
munmap((char*)s.pmem, s.range.size());
+if (autoUnlinkSharedBackstore && !sharedBackstore.empty())
+shm_unlink(sharedBackstore.c_str());
}
bool
diff --git a/src/mem/physical.hh b/src/mem/physical.hh
index bb90664..3e7d031 100644
--- a/src/mem/physical.hh
+++ b/src/mem/physical.hh
@@ -140,6 +140,7 @@
const bool mmapUsingNoReserve;
const std::string sharedBackstore;
+const bool autoUnlinkSharedBackstore;
uint64_t sharedBackstoreSize;
// The physical memory used to provide the memory in the simulated
diff --git a/src/sim/System.py b/src/sim/System.py
index 499cf9b..a1bff12 100644
--- a/src/sim/System.py
+++ b/src/sim/System.py
@@ -87,6 +87,9 @@
shared_backstore = Param.String("", "backstore's shmem segment
filename, "
"use to directly address the backstore from another host-OS
process. "
"Leave this empty to unset the MAP_SHARED flag.")
+auto_unlink_shared_backstore = Param.Bool(True, "Automatically remove
the "
+"shmem segment file upon destruction. This is used only if "
+"shared_backstore is non-empty.")
cache_line_size = Param.Unsigned(64, "Cache line size in bytes")
diff --git a/src/sim/system.cc b/src/sim/system.cc
index d5e5e3f..b7fba8a 100644
--- a/src/sim/system.cc
+++ b/src/sim/system.cc
@@ -182,7 +182,7 @@
physProxy(_systemPort, p.cache_line_size),
workload(p.workload),
physmem(name() + ".physmem", p.memories, p.mmap_using_noreserve,
- p.shared_backstore),
+ p.shared_backstore, p.auto_unlink_shared_backstore),
ShadowRomRanges(p.shadow_rom_ranges.begin(),
p.shadow_rom_ranges.end()),
memoryMode(p.mem_mode),
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/57469
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: I0267b643bd24e62cb7571674fe98f831c13a586d
Gerrit-Change-Number: 57469
Gerrit-PatchSet: 1
Gerrit-Owner: Jui-min Lee
Gerrit-MessageType: newchange
___
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
[gem5-dev] Change in gem5/gem5[develop]: mem: Fix phy mem with shm and multiple abstr mem
Jui-min Lee has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/57369 )
Change subject: mem: Fix phy mem with shm and multiple abstr mem
..
mem: Fix phy mem with shm and multiple abstr mem
Previously, all abstract memory backed by the same physical memory will
use the exact same chunk of shared memory if sharedBackstore is set. It
means that all abstract memories, despite setting to a different range,
will still be map to the same chunk of memory.
As a result, setting the sharedBackstore not only allows our host system
to share gem5 memory, it also enforces multiple gem5 memories to share
the same content. Which will significantly affect the simulation result.
Furthermore, the actual size of the shared memory will be determined by
the last backingStore created. If the last one is unfortunately smaller
than any previous backingStore, this may invalid previous mapped region
and cause a SIGBUS upon access (on linux).
In this CL, we put all backingStores of those abstract memories side by
side instead of stacking them all together. So the behavior of abstract
memories will be kept consistent whether the sharedBackstore is set or
not, yet presist the ability to access those memories from host.
Change-Id: Ic4ec25c99fe72744afaa2dfbb48cd0d65230e9a8
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/57369
Reviewed-by: Yu-hsin Wang
Reviewed-by: Gabe Black
Maintainer: Gabe Black
Tested-by: kokoro
---
M src/mem/physical.cc
M src/mem/physical.hh
2 files changed, 44 insertions(+), 5 deletions(-)
Approvals:
Gabe Black: Looks good to me, approved; Looks good to me, approved
Yu-hsin Wang: Looks good to me, but someone else must approve
kokoro: Regressions pass
diff --git a/src/mem/physical.cc b/src/mem/physical.cc
index ae20fb6..a7e261f 100644
--- a/src/mem/physical.cc
+++ b/src/mem/physical.cc
@@ -79,7 +79,7 @@
bool mmap_using_noreserve,
const std::string& shared_backstore) :
_name(_name), size(0), mmapUsingNoReserve(mmap_using_noreserve),
-sharedBackstore(shared_backstore)
+sharedBackstore(shared_backstore), sharedBackstoreSize(0)
{
if (mmap_using_noreserve)
warn("Not reserving swap space. May cause SIGSEGV on actual
usage\n");
@@ -201,17 +201,22 @@
int shm_fd;
int map_flags;
+off_t map_offset;
if (sharedBackstore.empty()) {
shm_fd = -1;
map_flags = MAP_ANON | MAP_PRIVATE;
+map_offset = 0;
} else {
-DPRINTF(AddrRanges, "Sharing backing store as %s\n",
-sharedBackstore.c_str());
+// Newly create backstore will be located after previous one.
+map_offset = sharedBackstoreSize;
+sharedBackstoreSize += range.size();
+DPRINTF(AddrRanges, "Sharing backing store as %s at offset %llu\n",
+sharedBackstore.c_str(), (uint64_t)map_offset);
shm_fd = shm_open(sharedBackstore.c_str(), O_CREAT | O_RDWR, 0666);
if (shm_fd == -1)
panic("Shared memory failed");
-if (ftruncate(shm_fd, range.size()))
+if (ftruncate(shm_fd, sharedBackstoreSize))
panic("Setting size of shared memory failed");
map_flags = MAP_SHARED;
}
@@ -224,7 +229,7 @@
uint8_t* pmem = (uint8_t*) mmap(NULL, range.size(),
PROT_READ | PROT_WRITE,
-map_flags, shm_fd, 0);
+map_flags, shm_fd, map_offset);
if (pmem == (uint8_t*) MAP_FAILED) {
perror("mmap");
diff --git a/src/mem/physical.hh b/src/mem/physical.hh
index b7ec8eb..bb90664 100644
--- a/src/mem/physical.hh
+++ b/src/mem/physical.hh
@@ -140,6 +140,7 @@
const bool mmapUsingNoReserve;
const std::string sharedBackstore;
+uint64_t sharedBackstoreSize;
// The physical memory used to provide the memory in the simulated
// system
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/57369
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: Ic4ec25c99fe72744afaa2dfbb48cd0d65230e9a8
Gerrit-Change-Number: 57369
Gerrit-PatchSet: 2
Gerrit-Owner: Jui-min Lee
Gerrit-Reviewer: Boris Shingarov
Gerrit-Reviewer: Gabe Black
Gerrit-Reviewer: Jui-min Lee
Gerrit-Reviewer: Nikos Nikoleris
Gerrit-Reviewer: Yu-hsin Wang
Gerrit-Reviewer: kokoro
Gerrit-CC: Earl Ou
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
[gem5-dev] Change in gem5/gem5[develop]: systemc: Fix memory leak of sc_event_list
Jui-min Lee has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/57329 )
Change subject: systemc: Fix memory leak of sc_event_list
..
systemc: Fix memory leak of sc_event_list
Make SensitivityAnd/OrList class track the sc_event_and/or_list and
delete it when the Sensitivity object itself is deleted.
Bug: 222177290
Test: Run gem5 unittests, presubmits, and systemc unittests
Change-Id: Ib46f8dfb6727f77ad843ba33ce22c7e6d2645ff2
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/57329
Reviewed-by: Gabe Black
Maintainer: Gabe Black
Tested-by: kokoro
---
M src/systemc/core/sensitivity.cc
M src/systemc/core/sensitivity.hh
2 files changed, 50 insertions(+), 2 deletions(-)
Approvals:
Gabe Black: Looks good to me, approved; Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/systemc/core/sensitivity.cc
b/src/systemc/core/sensitivity.cc
index 8c4aa23..1367b29 100644
--- a/src/systemc/core/sensitivity.cc
+++ b/src/systemc/core/sensitivity.cc
@@ -214,9 +214,21 @@
DynamicSensitivityEventOrList::DynamicSensitivityEventOrList(
Process *p, const sc_core::sc_event_or_list *eol) :
-Sensitivity(p), DynamicSensitivity(p), SensitivityEvents(p,
eol->events)
+Sensitivity(p),
+DynamicSensitivity(p),
+SensitivityEvents(p, eol->events),
+list(eol)
{}
+DynamicSensitivityEventOrList::~DynamicSensitivityEventOrList()
+{
+if (list->autoDelete) {
+panic_if(list->busy,
+ "sc_event_or_list can never be busy in gem5
implementation");
+delete list;
+}
+}
+
bool
DynamicSensitivityEventOrList::notifyWork(Event *e)
{
@@ -233,9 +245,21 @@
DynamicSensitivityEventAndList::DynamicSensitivityEventAndList(
Process *p, const sc_core::sc_event_and_list *eal) :
-Sensitivity(p), DynamicSensitivity(p), SensitivityEvents(p,
eal->events)
+Sensitivity(p),
+DynamicSensitivity(p),
+SensitivityEvents(p, eal->events),
+list(eal)
{}
+DynamicSensitivityEventAndList::~DynamicSensitivityEventAndList()
+{
+if (list->autoDelete) {
+panic_if(list->busy,
+ "sc_event_and_list can never be busy in gem5
implementation");
+delete list;
+}
+}
+
bool
DynamicSensitivityEventAndList::notifyWork(Event *e)
{
diff --git a/src/systemc/core/sensitivity.hh
b/src/systemc/core/sensitivity.hh
index 9e76969..af4ce2d 100644
--- a/src/systemc/core/sensitivity.hh
+++ b/src/systemc/core/sensitivity.hh
@@ -274,8 +274,11 @@
DynamicSensitivityEventOrList(
Process *p, const sc_core::sc_event_or_list *eol);
+~DynamicSensitivityEventOrList();
bool notifyWork(Event *e) override;
+
+const sc_core::sc_event_or_list *list;
};
//XXX This sensitivity can't be reused. To reset it, it has to be deleted
and
@@ -290,8 +293,11 @@
DynamicSensitivityEventAndList(
Process *p, const sc_core::sc_event_and_list *eal);
+~DynamicSensitivityEventAndList();
bool notifyWork(Event *e) override;
+
+const sc_core::sc_event_and_list *list;
};
} // namespace sc_gem5
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/57329
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: Ib46f8dfb6727f77ad843ba33ce22c7e6d2645ff2
Gerrit-Change-Number: 57329
Gerrit-PatchSet: 3
Gerrit-Owner: Jui-min Lee
Gerrit-Reviewer: Gabe Black
Gerrit-Reviewer: Jui-min Lee
Gerrit-Reviewer: kokoro
Gerrit-CC: Earl Ou
Gerrit-CC: Yu-hsin Wang
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
[gem5-dev] Change in gem5/gem5[develop]: mem: Fix phy mem with shm and multiple abstr mem
Jui-min Lee has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/57369 )
Change subject: mem: Fix phy mem with shm and multiple abstr mem
..
mem: Fix phy mem with shm and multiple abstr mem
Previously, all abstract memory backed by the same physical memory will
use the exact same chunk of shared memory if sharedBackstore is set. It
means that all abstract memories, despite setting to a different range,
will still be map to the same chunk of memory.
As a result, setting the sharedBackstore not only allows our host system
to share gem5 memory, it also enforces multiple gem5 memories to share
the same content. Which will significantly affect the simulation result.
Furthermore, the actual size of the shared memory will be determined by
the last backingStore created. If the last one is unfortunately smaller
than any previous backingStore, this may invalid previous mapped region
and cause a SIGBUS upon access (on linux).
In this CL, we put all backingStores of those abstract memories side by
side instead of stacking them all together. So the behavior of abstract
memories will be kept consistent whether the sharedBackstore is set or
not, yet presist the ability to access those memories from host.
Change-Id: Ic4ec25c99fe72744afaa2dfbb48cd0d65230e9a8
---
M src/mem/physical.cc
M src/mem/physical.hh
2 files changed, 39 insertions(+), 5 deletions(-)
diff --git a/src/mem/physical.cc b/src/mem/physical.cc
index ae20fb6..a7e261f 100644
--- a/src/mem/physical.cc
+++ b/src/mem/physical.cc
@@ -79,7 +79,7 @@
bool mmap_using_noreserve,
const std::string& shared_backstore) :
_name(_name), size(0), mmapUsingNoReserve(mmap_using_noreserve),
-sharedBackstore(shared_backstore)
+sharedBackstore(shared_backstore), sharedBackstoreSize(0)
{
if (mmap_using_noreserve)
warn("Not reserving swap space. May cause SIGSEGV on actual
usage\n");
@@ -201,17 +201,22 @@
int shm_fd;
int map_flags;
+off_t map_offset;
if (sharedBackstore.empty()) {
shm_fd = -1;
map_flags = MAP_ANON | MAP_PRIVATE;
+map_offset = 0;
} else {
-DPRINTF(AddrRanges, "Sharing backing store as %s\n",
-sharedBackstore.c_str());
+// Newly create backstore will be located after previous one.
+map_offset = sharedBackstoreSize;
+sharedBackstoreSize += range.size();
+DPRINTF(AddrRanges, "Sharing backing store as %s at offset %llu\n",
+sharedBackstore.c_str(), (uint64_t)map_offset);
shm_fd = shm_open(sharedBackstore.c_str(), O_CREAT | O_RDWR, 0666);
if (shm_fd == -1)
panic("Shared memory failed");
-if (ftruncate(shm_fd, range.size()))
+if (ftruncate(shm_fd, sharedBackstoreSize))
panic("Setting size of shared memory failed");
map_flags = MAP_SHARED;
}
@@ -224,7 +229,7 @@
uint8_t* pmem = (uint8_t*) mmap(NULL, range.size(),
PROT_READ | PROT_WRITE,
-map_flags, shm_fd, 0);
+map_flags, shm_fd, map_offset);
if (pmem == (uint8_t*) MAP_FAILED) {
perror("mmap");
diff --git a/src/mem/physical.hh b/src/mem/physical.hh
index b7ec8eb..bb90664 100644
--- a/src/mem/physical.hh
+++ b/src/mem/physical.hh
@@ -140,6 +140,7 @@
const bool mmapUsingNoReserve;
const std::string sharedBackstore;
+uint64_t sharedBackstoreSize;
// The physical memory used to provide the memory in the simulated
// system
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/57369
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: Ic4ec25c99fe72744afaa2dfbb48cd0d65230e9a8
Gerrit-Change-Number: 57369
Gerrit-PatchSet: 1
Gerrit-Owner: Jui-min Lee
Gerrit-MessageType: newchange
___
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
[gem5-dev] Change in gem5/gem5[develop]: systemc: Fix memory leak of sc_event_list
Jui-min Lee has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/57329 )
Change subject: systemc: Fix memory leak of sc_event_list
..
systemc: Fix memory leak of sc_event_list
Make SensitivityAnd/OrList class track the sc_event_and/or_list and
delete it when the Sensitivity object itself is deleted.
Bug: 222177290
Test: Run gem5 unittests and our presubmits
Change-Id: Ib46f8dfb6727f77ad843ba33ce22c7e6d2645ff2
---
M src/systemc/core/sensitivity.cc
M src/systemc/core/sensitivity.hh
2 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/src/systemc/core/sensitivity.cc
b/src/systemc/core/sensitivity.cc
index 8c4aa23..1367b29 100644
--- a/src/systemc/core/sensitivity.cc
+++ b/src/systemc/core/sensitivity.cc
@@ -214,9 +214,21 @@
DynamicSensitivityEventOrList::DynamicSensitivityEventOrList(
Process *p, const sc_core::sc_event_or_list *eol) :
-Sensitivity(p), DynamicSensitivity(p), SensitivityEvents(p,
eol->events)
+Sensitivity(p),
+DynamicSensitivity(p),
+SensitivityEvents(p, eol->events),
+list(eol)
{}
+DynamicSensitivityEventOrList::~DynamicSensitivityEventOrList()
+{
+if (list->autoDelete) {
+panic_if(list->busy,
+ "sc_event_or_list can never be busy in gem5
implementation");
+delete list;
+}
+}
+
bool
DynamicSensitivityEventOrList::notifyWork(Event *e)
{
@@ -233,9 +245,21 @@
DynamicSensitivityEventAndList::DynamicSensitivityEventAndList(
Process *p, const sc_core::sc_event_and_list *eal) :
-Sensitivity(p), DynamicSensitivity(p), SensitivityEvents(p,
eal->events)
+Sensitivity(p),
+DynamicSensitivity(p),
+SensitivityEvents(p, eal->events),
+list(eal)
{}
+DynamicSensitivityEventAndList::~DynamicSensitivityEventAndList()
+{
+if (list->autoDelete) {
+panic_if(list->busy,
+ "sc_event_and_list can never be busy in gem5
implementation");
+delete list;
+}
+}
+
bool
DynamicSensitivityEventAndList::notifyWork(Event *e)
{
diff --git a/src/systemc/core/sensitivity.hh
b/src/systemc/core/sensitivity.hh
index 9e76969..af4ce2d 100644
--- a/src/systemc/core/sensitivity.hh
+++ b/src/systemc/core/sensitivity.hh
@@ -274,8 +274,11 @@
DynamicSensitivityEventOrList(
Process *p, const sc_core::sc_event_or_list *eol);
+~DynamicSensitivityEventOrList();
bool notifyWork(Event *e) override;
+
+const sc_core::sc_event_or_list *list;
};
//XXX This sensitivity can't be reused. To reset it, it has to be deleted
and
@@ -290,8 +293,11 @@
DynamicSensitivityEventAndList(
Process *p, const sc_core::sc_event_and_list *eal);
+~DynamicSensitivityEventAndList();
bool notifyWork(Event *e) override;
+
+const sc_core::sc_event_and_list *list;
};
} // namespace sc_gem5
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/57329
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: Ib46f8dfb6727f77ad843ba33ce22c7e6d2645ff2
Gerrit-Change-Number: 57329
Gerrit-PatchSet: 1
Gerrit-Owner: Jui-min Lee
Gerrit-MessageType: newchange
___
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
[gem5-dev] Change in gem5/gem5[develop]: base: Fix ListenSocket binding logic
Jui-min Lee has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/55943 )
Change subject: base: Fix ListenSocket binding logic
..
base: Fix ListenSocket binding logic
The original implementation does not cleanup the socket after it failed
to listen. However, the API doesn't give our user a way to bypass the
bind part and the next try will always break at the bind call.
Furthermore, the next failure will be EINVAL instead of EADDRINUSE so
gem5 will just abort without giving any meaningful message.
In this CL we cleanup the socket if we failed to invoke listen, so the
user can retry with a clean state and even retry on another port.
Test: Try to launch two gem5 that both bind to gdb port (7000) and
repeat it for 100 times.
Change-Id: I7272ea3c3b6ab56e4b904f3a3a45ed389d00dd05
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/55943
Reviewed-by: Yu-hsin Wang
Reviewed-by: Gabe Black
Maintainer: Gabe Black
Tested-by: kokoro
Reviewed-by: Nathanael Premillieu
---
M src/base/socket.cc
1 file changed, 32 insertions(+), 1 deletion(-)
Approvals:
Gabe Black: Looks good to me, but someone else must approve; Looks good
to me, approved
Yu-hsin Wang: Looks good to me, but someone else must approve
Nathanael Premillieu: Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/base/socket.cc b/src/base/socket.cc
index 860249a..5cf67fd 100644
--- a/src/base/socket.cc
+++ b/src/base/socket.cc
@@ -155,7 +155,12 @@
if (::listen(fd, 1) == -1) {
if (errno != EADDRINUSE)
panic("ListenSocket(listen): listen() failed!");
-
+// User may decide to retry with a different port later; however,
the
+// socket is already bound to a port and the next bind will surely
+// fail. We'll close the socket and reset fd to -1 so our user can
+// retry with a cleaner state.
+close(fd);
+fd = -1;
return false;
}
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/55943
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: I7272ea3c3b6ab56e4b904f3a3a45ed389d00dd05
Gerrit-Change-Number: 55943
Gerrit-PatchSet: 3
Gerrit-Owner: Jui-min Lee
Gerrit-Reviewer: Bobby Bruce
Gerrit-Reviewer: Daniel Carvalho
Gerrit-Reviewer: Earl Ou
Gerrit-Reviewer: Gabe Black
Gerrit-Reviewer: Gabe Black
Gerrit-Reviewer: Jui-min Lee
Gerrit-Reviewer: Nathanael Premillieu
Gerrit-Reviewer: Yu-hsin Wang
Gerrit-Reviewer: kokoro
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
[gem5-dev] Change in gem5/gem5[develop]: base: Fix ListenSocket binding logic
Jui-min Lee has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/55943 )
Change subject: base: Fix ListenSocket binding logic
..
base: Fix ListenSocket binding logic
The original implementation does not cleanup the socket after it failed
to listen. However, the API doesn't give our user a way to bypass the
bind part and the next try will always break at the bind call.
Furthermore, the next failure will be EINVAL instead of EADDRINUSE so
gem5 will just abort without giving any meaningful message.
In this CL we cleanup the socket if we failed to invoke listen, so the
user can retry with a clean state and even retry on another port.
Test: Try to launch two gem5 that both bind to gdb port (7000) and
repeat it for 100 times.
Change-Id: I7272ea3c3b6ab56e4b904f3a3a45ed389d00dd05
---
M src/base/socket.cc
1 file changed, 49 insertions(+), 21 deletions(-)
diff --git a/src/base/socket.cc b/src/base/socket.cc
index 860249a..7a3a721 100644
--- a/src/base/socket.cc
+++ b/src/base/socket.cc
@@ -138,30 +138,38 @@
panic("ListenSocket(listen): setsockopt() SO_REUSEADDR
failed!");
}
-struct sockaddr_in sockaddr;
-sockaddr.sin_family = PF_INET;
-sockaddr.sin_addr.s_addr =
-htobe(bindToLoopback ? INADDR_LOOPBACK : INADDR_ANY);
-sockaddr.sin_port = htons(port);
-// finally clear sin_zero
-std::memset(_zero, 0, sizeof(sockaddr.sin_zero));
-int ret = ::bind(fd, (struct sockaddr *), sizeof (sockaddr));
-if (ret != 0) {
-if (ret == -1 && errno != EADDRINUSE)
-panic("ListenSocket(listen): bind() failed!");
-return false;
-}
+do {
+struct sockaddr_in sockaddr;
+sockaddr.sin_family = PF_INET;
+sockaddr.sin_addr.s_addr =
+htobe(bindToLoopback ? INADDR_LOOPBACK :
INADDR_ANY);
+sockaddr.sin_port = htons(port);
+// finally clear sin_zero
+std::memset(_zero, 0, sizeof(sockaddr.sin_zero));
+int ret = ::bind(fd, (struct sockaddr *), sizeof
(sockaddr));
+if (ret != 0) {
+if (ret == -1 && errno != EADDRINUSE)
+panic("ListenSocket(listen): bind() failed!");
+break;
+}
-if (::listen(fd, 1) == -1) {
-if (errno != EADDRINUSE)
-panic("ListenSocket(listen): listen() failed!");
+if (::listen(fd, 1) == -1) {
+if (errno != EADDRINUSE)
+panic("ListenSocket(listen): listen() failed!");
-return false;
-}
+break;
+}
-listening = true;
-anyListening = true;
-return true;
+listening = true;
+anyListening = true;
+return true;
+} while(false);
+
+// If we reach here, something must have gone wrong while binding
socket.
+// We'll close the socket so our user can retry from a cleaner state.
+close(fd);
+fd = -1;
+return false;
}
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/55943
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: I7272ea3c3b6ab56e4b904f3a3a45ed389d00dd05
Gerrit-Change-Number: 55943
Gerrit-PatchSet: 1
Gerrit-Owner: Jui-min Lee
Gerrit-MessageType: newchange
___
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
[gem5-dev] Change in gem5/gem5[develop]: arch-riscv: Add NMI support
Jui-min Lee has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/52363 )
Change subject: arch-riscv: Add NMI support
..
arch-riscv: Add NMI support
NMI is platform dependent according to riscv spec, so we intentionally
propose a very minimal design that doesn't give user any new accessible
register to interact with the NMI mechanism.
In current design, the NMI reset vector is fixed to 0x0 and always set
mcause to zero. mret from NMI handler is still possible, but it's up to
the user to detect whether a M-mode trap handler is interrupted and to
recover from it (if at all possible).
1. Add new fault type to represent NMI fault
2. Add non-standard registers to save the status of NMI
a. nmivec[63:2] = NMI reset vector address
b. nmie[0:0] = is NMI enabled = not in NMI handler
c. nmip[0:0] = is NMI pending
3. Add new function in RiscvInterrupts to raise/clear NMI
Bug: 200169094
Test: None
Change-Id: Ia81e1c9589bc02f0690d094fff5f13412846acbe
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/52363
Reviewed-by: Yu-hsin Wang
Reviewed-by: Jason Lowe-Power
Maintainer: Jason Lowe-Power
Tested-by: kokoro
---
M src/arch/riscv/isa.cc
M src/arch/riscv/regs/misc.hh
M src/arch/riscv/isa/decoder.isa
M src/arch/riscv/interrupts.hh
M src/arch/riscv/faults.cc
M src/arch/riscv/faults.hh
6 files changed, 111 insertions(+), 12 deletions(-)
Approvals:
Jason Lowe-Power: Looks good to me, approved; Looks good to me, approved
Yu-hsin Wang: Looks good to me, but someone else must approve
kokoro: Regressions pass
diff --git a/src/arch/riscv/faults.cc b/src/arch/riscv/faults.cc
index 36e2ce9..01f6827 100644
--- a/src/arch/riscv/faults.cc
+++ b/src/arch/riscv/faults.cc
@@ -67,8 +67,17 @@
PrivilegeMode prv = PRV_M;
STATUS status = tc->readMiscReg(MISCREG_STATUS);
+// According to riscv-privileged-v1.11, if a NMI occurs at the
middle
+// of a M-mode trap handler, the state (epc/cause) will be
overwritten
+// and is not necessary recoverable. There's nothing we can do
here so
+// we'll just warn our user that the CPU state might be broken.
+warn_if(isNonMaskableInterrupt() && pp == PRV_M && status.mie == 0,
+"NMI overwriting M-mode trap handler state");
+
// Set fault handler privilege mode
-if (isInterrupt()) {
+if (isNonMaskableInterrupt()) {
+prv = PRV_M;
+} else if (isInterrupt()) {
if (pp != PRV_M &&
bits(tc->readMiscReg(MISCREG_MIDELEG), _code) != 0) {
prv = PRV_S;
@@ -113,7 +122,7 @@
case PRV_M:
cause = MISCREG_MCAUSE;
epc = MISCREG_MEPC;
-tvec = MISCREG_MTVEC;
+tvec = isNonMaskableInterrupt() ? MISCREG_NMIVEC :
MISCREG_MTVEC;
tval = MISCREG_MTVAL;
status.mpp = pp;
@@ -136,6 +145,12 @@
tc->setMiscReg(tval, trap_value());
tc->setMiscReg(MISCREG_PRV, prv);
tc->setMiscReg(MISCREG_STATUS, status);
+// Temporarily mask NMI while we're in NMI handler. Otherweise, the
+// checkNonMaskableInterrupt will always return true and we'll be
+// stucked in an infinite loop.
+if (isNonMaskableInterrupt()) {
+tc->setMiscReg(MISCREG_NMIE, 0);
+}
// Set PC to fault handler address
Addr addr = mbits(tc->readMiscReg(tvec), 63, 2);
diff --git a/src/arch/riscv/faults.hh b/src/arch/riscv/faults.hh
index a8df3f5..1ea8f70 100644
--- a/src/arch/riscv/faults.hh
+++ b/src/arch/riscv/faults.hh
@@ -96,19 +96,30 @@
NumInterruptTypes
};
+enum class FaultType
+{
+INTERRUPT,
+NON_MASKABLE_INTERRUPT,
+OTHERS,
+};
+
class RiscvFault : public FaultBase
{
protected:
const FaultName _name;
-const bool _interrupt;
+const FaultType _fault_type;
ExceptionCode _code;
-RiscvFault(FaultName n, bool i, ExceptionCode c)
-: _name(n), _interrupt(i), _code(c)
+RiscvFault(FaultName n, FaultType ft, ExceptionCode c)
+: _name(n), _fault_type(ft), _code(c)
{}
FaultName name() const override { return _name; }
-bool isInterrupt() const { return _interrupt; }
+bool isInterrupt() const { return _fault_type == FaultType::INTERRUPT;
}
+bool isNonMaskableInterrupt() const
+{
+return _fault_type == FaultType::NON_MASKABLE_INTERRUPT;
+}
ExceptionCode exception() const { return _code; }
virtual RegVal trap_value() const { return 0; }
@@ -132,10 +143,22 @@
class InterruptFault : public RiscvFault
{
public:
-InterruptFault(ExceptionCode c) : RiscvFault("interrupt", true, c) {}
+InterruptFault(ExceptionCode c)
+: RiscvFault("interrupt", FaultType::INTERRUPT, c)
+{}
InterruptFault(int c) : InterruptFault(static_cast(c))
{}
};
+class
[gem5-dev] Change in gem5/gem5[develop]: arch-riscv: Add NMI support
Jui-min Lee has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/52363 )
Change subject: arch-riscv: Add NMI support
..
arch-riscv: Add NMI support
NMI is platform dependent according to riscv spec, so we intentionally
propose a very minimal design that doesn't give user any new accessible
register to interact with the NMI mechanism.
In current design, the NMI reset vector is fixed to 0x0 and always set
mcause to zero. mret from NMI handler is still possible, but it's up to
the user to detect whether an M-mode trap handler is interrupted and to
recover from it (if possible at all).
1. Add new fault type to represent NMI fault
2. Add non-standard registers to save the status of NMI
a. nmivec[63:2] = NMI reset vector address
b. nmie[0:0] = is NMI enabled = not in NMI handler
c. nmip[0:0] = is NMI pending
3. Add new function in RiscvInterrupts to raise/clear NMI
Bug: 200169094
Test: None
Change-Id: Ia81e1c9589bc02f0690d094fff5f13412846acbe
---
M src/arch/riscv/isa.cc
M src/arch/riscv/regs/misc.hh
M src/arch/riscv/isa/decoder.isa
M src/arch/riscv/interrupts.hh
M src/arch/riscv/faults.cc
M src/arch/riscv/faults.hh
6 files changed, 100 insertions(+), 12 deletions(-)
diff --git a/src/arch/riscv/faults.cc b/src/arch/riscv/faults.cc
index b7fb509..085e113 100644
--- a/src/arch/riscv/faults.cc
+++ b/src/arch/riscv/faults.cc
@@ -67,8 +67,17 @@
PrivilegeMode prv = PRV_M;
STATUS status = tc->readMiscReg(MISCREG_STATUS);
+// According to riscv-privileged-v1.11, if a NMI occurs at the
middle
+// of a M-mode trap handler, the state (epc/cause) will be
overwritten
+// and is not necessary recoverable. There's nothing we can do
here so
+// we'll just warn our user that the CPU state might be broken.
+warn_if(isNonMaskableInterrupt() && pp == PRV_M && status.mie == 0,
+"NMI overwriting M-mode trap handler state");
+
// Set fault handler privilege mode
-if (isInterrupt()) {
+if (isNonMaskableInterrupt()) {
+prv = PRV_M;
+} else if (isInterrupt()) {
if (pp != PRV_M &&
bits(tc->readMiscReg(MISCREG_MIDELEG), _code) != 0) {
prv = PRV_S;
@@ -113,7 +122,7 @@
case PRV_M:
cause = MISCREG_MCAUSE;
epc = MISCREG_MEPC;
-tvec = MISCREG_MTVEC;
+tvec = isNonMaskableInterrupt() ? MISCREG_NMIVEC :
MISCREG_MTVEC;
tval = MISCREG_MTVAL;
status.mpp = pp;
@@ -136,6 +145,12 @@
tc->setMiscReg(tval, trap_value());
tc->setMiscReg(MISCREG_PRV, prv);
tc->setMiscReg(MISCREG_STATUS, status);
+// Temporarily mask NMI while we're in NMI handler. Otherweise, the
+// checkNonMaskableInterrupt will always return true and we'll be
+// stucked in an infinite loop.
+if (isNonMaskableInterrupt()) {
+tc->setMiscReg(MISCREG_NMIE, 0);
+}
// Set PC to fault handler address
Addr addr = mbits(tc->readMiscReg(tvec), 63, 2);
diff --git a/src/arch/riscv/faults.hh b/src/arch/riscv/faults.hh
index a8df3f5..1ea8f70 100644
--- a/src/arch/riscv/faults.hh
+++ b/src/arch/riscv/faults.hh
@@ -96,19 +96,30 @@
NumInterruptTypes
};
+enum class FaultType
+{
+INTERRUPT,
+NON_MASKABLE_INTERRUPT,
+OTHERS,
+};
+
class RiscvFault : public FaultBase
{
protected:
const FaultName _name;
-const bool _interrupt;
+const FaultType _fault_type;
ExceptionCode _code;
-RiscvFault(FaultName n, bool i, ExceptionCode c)
-: _name(n), _interrupt(i), _code(c)
+RiscvFault(FaultName n, FaultType ft, ExceptionCode c)
+: _name(n), _fault_type(ft), _code(c)
{}
FaultName name() const override { return _name; }
-bool isInterrupt() const { return _interrupt; }
+bool isInterrupt() const { return _fault_type == FaultType::INTERRUPT;
}
+bool isNonMaskableInterrupt() const
+{
+return _fault_type == FaultType::NON_MASKABLE_INTERRUPT;
+}
ExceptionCode exception() const { return _code; }
virtual RegVal trap_value() const { return 0; }
@@ -132,10 +143,22 @@
class InterruptFault : public RiscvFault
{
public:
-InterruptFault(ExceptionCode c) : RiscvFault("interrupt", true, c) {}
+InterruptFault(ExceptionCode c)
+: RiscvFault("interrupt", FaultType::INTERRUPT, c)
+{}
InterruptFault(int c) : InterruptFault(static_cast(c))
{}
};
+class NonMaskableInterruptFault : public RiscvFault
+{
+ public:
+NonMaskableInterruptFault()
+: RiscvFault("non_maskable_interrupt",
+ FaultType::NON_MASKABLE_INTERRUPT,
+ static_cast(0))
+{}
+};
+
class InstFault : public RiscvFault
{
protected:
@@ -143,7 +166,7 @@
public:
InstFault(FaultName n,
[gem5-dev] Change in gem5/gem5[develop]: base: Construct loggers on the heap
Jui-min Lee has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/46079 )
Change subject: base: Construct loggers on the heap
..
base: Construct loggers on the heap
Loggers was previously declared as global variables, hence are unsafe to
be used inside other global objects' destructor (e.g. scMainFiber). This
CL makes them heap allocated objects hold by function static variables.
As a result:
1. The loggers never get destructed at the end of program, which makes
them safe to be used in global objects' destructor.
2. The loggers are constructed ondemand instead of relying on linker's
unknown way of ordering, which makes them safe to be used in global
objects' constructor.
Change-Id: Ieb499d2fa4c5c1c015324cb72b055115b0933ab8
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/46079
Reviewed-by: Daniel Carvalho
Reviewed-by: Jason Lowe-Power
Maintainer: Jason Lowe-Power
Tested-by: kokoro
---
M src/base/gtest/logging_mock.cc
M src/base/logging.cc
2 files changed, 70 insertions(+), 22 deletions(-)
Approvals:
Jason Lowe-Power: Looks good to me, approved; Looks good to me, approved
Daniel Carvalho: Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/base/gtest/logging_mock.cc b/src/base/gtest/logging_mock.cc
index e09a144..572d057 100644
--- a/src/base/gtest/logging_mock.cc
+++ b/src/base/gtest/logging_mock.cc
@@ -71,16 +71,40 @@
void exit() override { throw GTestException(); }
};
-GTestExitLogger panicLogger("panic: ");
-GTestExitLogger fatalLogger("fatal: ");
-GTestLogger warnLogger("warn: ");
-GTestLogger infoLogger("info: ");
-GTestLogger hackLogger("hack: ");
-
} // anonymous namespace
-Logger ::getPanic() { return panicLogger; }
-Logger ::getFatal() { return fatalLogger; }
-Logger ::getWarn() { return warnLogger; }
-Logger ::getInfo() { return infoLogger; }
-Logger ::getHack() { return hackLogger; }
+// We intentionally put all the loggers on the heap to prevent them from
being
+// destructed at the end of the program. This make them safe to be used
inside
+// destructor of other global objects. Also, we make them function static
+// veriables to ensure they are initialized ondemand, so it is also safe
to use
+// them inside constructor of other global objects.
+
+Logger&
+Logger::getPanic() {
+static GTestExitLogger* panic_logger = new GTestExitLogger("panic: ");
+return *panic_logger;
+}
+
+Logger&
+Logger::getFatal() {
+static GTestExitLogger* fatal_logger = new GTestExitLogger("fatal: ");
+return *fatal_logger;
+}
+
+Logger&
+Logger::getWarn() {
+static GTestLogger* warn_logger = new GTestLogger("warn: ");
+return *warn_logger;
+}
+
+Logger&
+Logger::getInfo() {
+static GTestLogger* info_logger = new GTestLogger("info: ");
+return *info_logger;
+}
+
+Logger&
+Logger::getHack() {
+static GTestLogger* hack_logger = new GTestLogger("hack: ");
+return *hack_logger;
+}
diff --git a/src/base/logging.cc b/src/base/logging.cc
index 1290455..f96d101 100644
--- a/src/base/logging.cc
+++ b/src/base/logging.cc
@@ -70,16 +70,40 @@
void exit() override { ::exit(1); }
};
-ExitLogger panicLogger("panic: ");
-FatalLogger fatalLogger("fatal: ");
-Logger warnLogger("warn: ");
-Logger infoLogger("info: ");
-Logger hackLogger("hack: ");
-
} // anonymous namespace
-Logger ::getPanic() { return panicLogger; }
-Logger ::getFatal() { return fatalLogger; }
-Logger ::getWarn() { return warnLogger; }
-Logger ::getInfo() { return infoLogger; }
-Logger ::getHack() { return hackLogger; }
+// We intentionally put all the loggers on the heap to prevent them from
being
+// destructed at the end of the program. This make them safe to be used
inside
+// destructor of other global objects. Also, we make them function static
+// veriables to ensure they are initialized ondemand, so it is also safe
to use
+// them inside constructor of other global objects.
+
+Logger&
+Logger::getPanic() {
+static ExitLogger* panic_logger = new ExitLogger("panic: ");
+return *panic_logger;
+}
+
+Logger&
+Logger::getFatal() {
+static FatalLogger* fatal_logger = new FatalLogger("fatal: ");
+return *fatal_logger;
+}
+
+Logger&
+Logger::getWarn() {
+static Logger* warn_logger = new Logger("warn: ");
+return *warn_logger;
+}
+
+Logger&
+Logger::getInfo() {
+static Logger* info_logger = new Logger("info: ");
+return *info_logger;
+}
+
+Logger&
+Logger::getHack() {
+static Logger* hack_logger = new Logger("hack: ");
+return *hack_logger;
+}
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/46079
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: Ieb499d2fa4c5c1c015324cb72b055115b0933ab8
Gerrit-Change-Number: 46079
Gerrit-PatchSet: 4
Gerrit-Owner: Jui-min
[gem5-dev] Change in gem5/gem5[develop]: base: Construct loggers on the heap
Jui-min Lee has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/46079 )
Change subject: base: Construct loggers on the heap
..
base: Construct loggers on the heap
Loggers was previously declared as global variables, hence are unsafe to
be used inside other global objects' destructor (e.g. scMainFiber). This
CL makes them heap allocated objects hold by function static variables.
As a result:
1. The loggers never get destructed at the end of program, which makes
them safe to be used in global objects' destructor.
2. The loggers are constructed ondemand instead of relying on linker's
unknown way of ordering, which makes them safe to be used in global
objects' constructor.
Change-Id: Ieb499d2fa4c5c1c015324cb72b055115b0933ab8
---
M src/base/logging.cc
1 file changed, 29 insertions(+), 11 deletions(-)
diff --git a/src/base/logging.cc b/src/base/logging.cc
index 1290455..11efc83 100644
--- a/src/base/logging.cc
+++ b/src/base/logging.cc
@@ -70,16 +70,34 @@
void exit() override { ::exit(1); }
};
-ExitLogger panicLogger("panic: ");
-FatalLogger fatalLogger("fatal: ");
-Logger warnLogger("warn: ");
-Logger infoLogger("info: ");
-Logger hackLogger("hack: ");
-
} // anonymous namespace
-Logger ::getPanic() { return panicLogger; }
-Logger ::getFatal() { return fatalLogger; }
-Logger ::getWarn() { return warnLogger; }
-Logger ::getInfo() { return infoLogger; }
-Logger ::getHack() { return hackLogger; }
+Logger&
+Logger::getPanic() {
+static ExitLogger* panicLogger = new ExitLogger("panic: ");
+return *panicLogger;
+}
+
+Logger&
+Logger::getFatal() {
+static FatalLogger* fatalLogger = new FatalLogger("fatal: ");
+return *fatalLogger;
+}
+
+Logger&
+Logger::getWarn() {
+static NormalLogger* warnLogger = new NormalLogger("warn: ");
+return *warnLogger;
+}
+
+Logger&
+Logger::getInfo() {
+static NormalLogger* infoLogger = new NormalLogger("info: ");
+return *infoLogger;
+}
+
+Logger&
+Logger::getHack() {
+static NormalLogger* hackLogger = new NormalLogger("hack: ");
+return *hackLogger;
+}
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/46079
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: Ieb499d2fa4c5c1c015324cb72b055115b0933ab8
Gerrit-Change-Number: 46079
Gerrit-PatchSet: 1
Gerrit-Owner: Jui-min Lee
Gerrit-MessageType: newchange
___
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
[gem5-dev] Change in gem5/gem5[develop]: base: Construct debug flags on the heap
Jui-min Lee has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/45582 )
Change subject: base: Construct debug flags on the heap
..
base: Construct debug flags on the heap
Debug flags were directly declared as global objects; however, the
destruction order of global objects in different translation units are
not defined in c++, so the destructor of other objects cannot safely
utilize debug flags to output extra information.
We now define those flags as references to objects allocated on the heap
so our flags will never get destructed. Note that this won't really
introduce any memory leak, as the program is getting terminated anyway.
Change-Id: I21f84ae17ac20653636ca67dc0c7855c0149
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/45582
Maintainer: Bobby R. Bruce
Tested-by: kokoro
Reviewed-by: Gabe Black
---
M src/SConscript
1 file changed, 21 insertions(+), 7 deletions(-)
Approvals:
Gabe Black: Looks good to me, approved
Bobby R. Bruce: Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/SConscript b/src/SConscript
index 7a6f9c1..cc51b9f 100644
--- a/src/SConscript
+++ b/src/SConscript
@@ -1177,18 +1177,32 @@
n, compound, desc, fmt = flag
assert n == name
+# We intentionally make flag a reference to a heap allocated
object so
+# (1) It has a similar interface to a global object like before
+# (2) It does not get destructed at the end of simulation
+# The second property is desirable as global objects from different
+# translation units do not have a defined destruction order, so
it'll
+# be unsafe to access debug flags in their destructor in such
cases.
if not compound:
+code('SimpleFlag& $name = *(')
+code.indent()
if fmt:
-code('SimpleFlag $name("$name", "$desc", true);')
+code('new SimpleFlag("$name", "$desc", true)')
else:
-code('SimpleFlag $name("$name", "$desc", false);')
+code('new SimpleFlag("$name", "$desc", false)')
+code.dedent()
+code(');')
else:
-comp_code('CompoundFlag $name("$name", "$desc", {')
+comp_code('CompoundFlag& $name = *(')
+comp_code.indent()
+comp_code('new CompoundFlag("$name", "$desc", {')
comp_code.indent()
for flag in compound:
comp_code('&$flag,')
comp_code.dedent()
-comp_code('});')
+comp_code('})')
+comp_code.dedent()
+comp_code(');')
code.append(comp_code)
code()
@@ -1221,11 +1235,11 @@
code('class SimpleFlag;')
if compound:
-code('extern CompoundFlag $name;')
+code('extern CompoundFlag& $name;')
for flag in compound:
-code('extern SimpleFlag $flag;')
+code('extern SimpleFlag& $flag;')
else:
-code('extern SimpleFlag $name;')
+code('extern SimpleFlag& $name;')
code('''
}
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/45582
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: I21f84ae17ac20653636ca67dc0c7855c0149
Gerrit-Change-Number: 45582
Gerrit-PatchSet: 7
Gerrit-Owner: Jui-min Lee
Gerrit-Reviewer: Andreas Sandberg
Gerrit-Reviewer: Bobby R. Bruce
Gerrit-Reviewer: Daniel Carvalho
Gerrit-Reviewer: Gabe Black
Gerrit-Reviewer: Jui-min Lee
Gerrit-Reviewer: kokoro
Gerrit-CC: Jason Lowe-Power
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
[gem5-dev] Change in gem5/gem5[develop]: base: Make debug flags reference
Jui-min Lee has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/45582 )
Change subject: base: Make debug flags reference
..
base: Make debug flags reference
Debug flags were directly declared as global objects; however, the
destruction order of global objects in different translation unit is not
defined in c++, so destructor of other objects cannot safely utilize
debug flags to output extra information.
We now define those flags as reference to objects allocated on heap so
our flags will never really get destructed. Note that this won't really
introduce any memory leak, as the program is getting terminated anyway.
Change-Id: I21f84ae17ac20653636ca67dc0c7855c0149
---
M src/SConscript
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/src/SConscript b/src/SConscript
index 7a6f9c1..947791c 100644
--- a/src/SConscript
+++ b/src/SConscript
@@ -1178,17 +1178,25 @@
assert n == name
if not compound:
+code('SimpleFlag& $name = *(')
+code.indent()
if fmt:
-code('SimpleFlag $name("$name", "$desc", true);')
+code('new SimpleFlag("$name", "$desc", true)')
else:
-code('SimpleFlag $name("$name", "$desc", false);')
+code('new SimpleFlag("$name", "$desc", false)')
+code.dedent()
+code(');')
else:
-comp_code('CompoundFlag $name("$name", "$desc", {')
+comp_code('CompoundFlag& $name = *(')
+comp_code.indent()
+comp_code('new CompoundFlag("$name", "$desc", {')
comp_code.indent()
for flag in compound:
comp_code('&$flag,')
comp_code.dedent()
-comp_code('});')
+comp_code('})')
+comp_code.dedent()
+comp_code(');')
code.append(comp_code)
code()
@@ -1221,11 +1229,11 @@
code('class SimpleFlag;')
if compound:
-code('extern CompoundFlag $name;')
+code('extern CompoundFlag& $name;')
for flag in compound:
-code('extern SimpleFlag $flag;')
+code('extern SimpleFlag& $flag;')
else:
-code('extern SimpleFlag $name;')
+code('extern SimpleFlag& $name;')
code('''
}
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/45582
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: I21f84ae17ac20653636ca67dc0c7855c0149
Gerrit-Change-Number: 45582
Gerrit-PatchSet: 1
Gerrit-Owner: Jui-min Lee
Gerrit-MessageType: newchange
___
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
[gem5-dev] Change in gem5/gem5[develop]: systemc: Fix tlm phase ordering in Gem5ToTlmBridge
Jui-min Lee has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/44305 )
Change subject: systemc: Fix tlm phase ordering in Gem5ToTlmBridge
..
systemc: Fix tlm phase ordering in Gem5ToTlmBridge
Previously we only rely on timestamp to schedule all phase change events
of tlm transactions in Gem5ToTlmBridge. However, it is valid to have
END_REQ and BEGIN_RESP of a transaction to be scheduled at the same
timestamp and the gem5 scheduler will not necessary order them in a
correct way.
This unfortunately breaks the code as sending a response before
accepting a request doesn't make sense at all and will trigger an
assertion failure.
In this CL we slightly increase the priority of END_REQ event so we can
always process phase change events in a correct order.
Change-Id: Ic33a92162c8c53af3887c7b04090115a38f96866
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/44305
Reviewed-by: Gabe Black
Maintainer: Gabe Black
Tested-by: kokoro
---
M src/systemc/tlm_bridge/gem5_to_tlm.cc
1 file changed, 25 insertions(+), 4 deletions(-)
Approvals:
Gabe Black: Looks good to me, approved; Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/systemc/tlm_bridge/gem5_to_tlm.cc
b/src/systemc/tlm_bridge/gem5_to_tlm.cc
index b80a083..e1cc087 100644
--- a/src/systemc/tlm_bridge/gem5_to_tlm.cc
+++ b/src/systemc/tlm_bridge/gem5_to_tlm.cc
@@ -62,6 +62,7 @@
#include "params/Gem5ToTlmBridge32.hh"
#include "params/Gem5ToTlmBridge64.hh"
+#include "sim/eventq.hh"
#include "sim/system.hh"
#include "systemc/tlm_bridge/sc_ext.hh"
#include "systemc/tlm_bridge/sc_mm.hh"
@@ -70,6 +71,24 @@
{
/**
+ * Helper function to help set priority of phase change events of tlm
+ * transactions. This is to workaround the uncertainty of gem5 eventq if
+ * multiple events are scheduled at the same timestamp.
+ */
+static EventBase::Priority
+getPriorityOfTlmPhase(const tlm::tlm_phase& phase)
+{
+// In theory, for all phase change events of a specific TLM base
protocol
+// transaction, only tlm::END_REQ and tlm::BEGIN_RESP would be
scheduled at
+// the same time in the same queue. So we only need to ensure END_REQ
has a
+// higher priority (less in pri value) than BEGIN_RESP.
+if (phase == tlm::END_REQ) {
+return EventBase::Default_Pri - 1;
+}
+return EventBase::Default_Pri;
+}
+
+/**
* Instantiate a tlm memory manager that takes care about all the
* tlm transactions in the system.
*/
@@ -367,8 +386,9 @@
// Accepted but is now blocking until END_REQ (exclusion rule).
blockingRequest = trans;
auto cb = [this, trans, phase]() { pec(*trans, phase); };
-system->schedule(new EventFunctionWrapper(cb, "pec", true),
- curTick() + delay.value());
+auto event = new EventFunctionWrapper(
+cb, "pec", true, getPriorityOfTlmPhase(phase));
+system->schedule(event, curTick() + delay.value());
} else if (status == tlm::TLM_COMPLETED) {
// Transaction is over nothing has do be done.
sc_assert(phase == tlm::END_RESP);
@@ -442,8 +462,9 @@
tlm::tlm_phase , sc_core::sc_time )
{
auto cb = [this, , phase]() { pec(trans, phase); };
-system->schedule(new EventFunctionWrapper(cb, "pec", true),
- curTick() + delay.value());
+auto event = new EventFunctionWrapper(
+cb, "pec", true, getPriorityOfTlmPhase(phase));
+system->schedule(event, curTick() + delay.value());
return tlm::TLM_ACCEPTED;
}
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/44305
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: Ic33a92162c8c53af3887c7b04090115a38f96866
Gerrit-Change-Number: 44305
Gerrit-PatchSet: 5
Gerrit-Owner: Jui-min Lee
Gerrit-Reviewer: Earl Ou
Gerrit-Reviewer: Gabe Black
Gerrit-Reviewer: Gabe Black
Gerrit-Reviewer: Jui-min Lee
Gerrit-Reviewer: Yu-hsin Wang
Gerrit-Reviewer: kokoro
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
[gem5-dev] Change in gem5/gem5[develop]: systemc: Use peq in Gem5ToTlmBridge
Jui-min Lee has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/44305 )
Change subject: systemc: Use peq in Gem5ToTlmBridge
..
systemc: Use peq in Gem5ToTlmBridge
Old implementation utilize gem5's scheduling function to handle tlm2
transaction delay; however, the behavior of gem5 event queue is slightly
different from the SystemC peq and will cause error in extreme case.
i.e. when END_REQ and BEGIN_RESP occurs at the same time.
In this CL we add a peq into the bridge and use it to properly handle
the phase change and delay of tlm2 transaction.
Change-Id: Ic33a92162c8c53af3887c7b04090115a38f96866
---
M src/systemc/tlm_bridge/gem5_to_tlm.cc
M src/systemc/tlm_bridge/gem5_to_tlm.hh
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/systemc/tlm_bridge/gem5_to_tlm.cc
b/src/systemc/tlm_bridge/gem5_to_tlm.cc
index b80a083..e66534e 100644
--- a/src/systemc/tlm_bridge/gem5_to_tlm.cc
+++ b/src/systemc/tlm_bridge/gem5_to_tlm.cc
@@ -366,9 +366,7 @@
sc_assert(phase == tlm::END_REQ || phase == tlm::BEGIN_RESP);
// Accepted but is now blocking until END_REQ (exclusion rule).
blockingRequest = trans;
-auto cb = [this, trans, phase]() { pec(*trans, phase); };
-system->schedule(new EventFunctionWrapper(cb, "pec", true),
- curTick() + delay.value());
+peq.notify(*trans, phase, delay);
} else if (status == tlm::TLM_COMPLETED) {
// Transaction is over nothing has do be done.
sc_assert(phase == tlm::END_RESP);
@@ -441,9 +439,7 @@
Gem5ToTlmBridge::nb_transport_bw(tlm::tlm_generic_payload ,
tlm::tlm_phase , sc_core::sc_time )
{
-auto cb = [this, , phase]() { pec(trans, phase); };
-system->schedule(new EventFunctionWrapper(cb, "pec", true),
- curTick() + delay.value());
+peq.notify(trans, phase, delay);
return tlm::TLM_ACCEPTED;
}
@@ -472,7 +468,9 @@
bridgeResponsePort(std::string(name()) + ".gem5", *this),
socket("tlm_socket"),
wrapper(socket, std::string(name()) + ".tlm", InvalidPortID),
-system(params.system), blockingRequest(nullptr),
+system(params.system),
+peq(this, ::pec),
+blockingRequest(nullptr),
needToSendRequestRetry(false), blockingResponse(nullptr),
addrRanges(params.addr_ranges.begin(), params.addr_ranges.end())
{
diff --git a/src/systemc/tlm_bridge/gem5_to_tlm.hh
b/src/systemc/tlm_bridge/gem5_to_tlm.hh
index d29bfd7..10a7253 100644
--- a/src/systemc/tlm_bridge/gem5_to_tlm.hh
+++ b/src/systemc/tlm_bridge/gem5_to_tlm.hh
@@ -68,6 +68,7 @@
#include "systemc/ext/core/sc_module.hh"
#include "systemc/ext/core/sc_module_name.hh"
#include "systemc/ext/tlm_core/2/generic_payload/gp.hh"
+#include "systemc/ext/tlm_utils/peq_with_cb_and_phase.h"
#include "systemc/ext/tlm_utils/simple_initiator_socket.h"
#include "systemc/tlm_port_wrapper.hh"
@@ -146,6 +147,7 @@
sc_gem5::TlmInitiatorWrapper wrapper;
System *system;
+tlm_utils::peq_with_cb_and_phase> peq;
/**
* A transaction after BEGIN_REQ has been sent but before END_REQ,
which
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/44305
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: Ic33a92162c8c53af3887c7b04090115a38f96866
Gerrit-Change-Number: 44305
Gerrit-PatchSet: 1
Gerrit-Owner: Jui-min Lee
Gerrit-MessageType: newchange
___
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
[gem5-dev] Change in gem5/gem5[develop]: systemc: Make tlm/gem5 packet conversion flexible
Jui-min Lee has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/37075 )
Change subject: systemc: Make tlm/gem5 packet conversion flexible
..
systemc: Make tlm/gem5 packet conversion flexible
We used to have a hard-coded packet2payload and payload2packet in the
tlm_bridge implementation. However, as the conversion is operated on
generic tlm payload, we're not able to handle information stored in any
user defined SystemC extensions.
In this CL, we add a pair of function to register extra conversion steps
between tlm payload and gem5 packet. This decouples the exact conversion
logic and enables SystemC users to register any necessary steps for
their extensions.
Change-Id: I70b3405395fed0f757f0fb7e19136f47d84ac115
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/37075
Reviewed-by: Gabe Black
Maintainer: Gabe Black
Tested-by: kokoro
---
M src/systemc/tlm_bridge/gem5_to_tlm.cc
M src/systemc/tlm_bridge/gem5_to_tlm.hh
M src/systemc/tlm_bridge/tlm_to_gem5.cc
M src/systemc/tlm_bridge/tlm_to_gem5.hh
4 files changed, 71 insertions(+), 2 deletions(-)
Approvals:
Gabe Black: Looks good to me, approved; Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/systemc/tlm_bridge/gem5_to_tlm.cc
b/src/systemc/tlm_bridge/gem5_to_tlm.cc
index ba8f121..f03548c 100644
--- a/src/systemc/tlm_bridge/gem5_to_tlm.cc
+++ b/src/systemc/tlm_bridge/gem5_to_tlm.cc
@@ -58,6 +58,8 @@
#include "systemc/tlm_bridge/gem5_to_tlm.hh"
+#include
+
#include "params/Gem5ToTlmBridge32.hh"
#include "params/Gem5ToTlmBridge64.hh"
#include "sim/system.hh"
@@ -73,6 +75,27 @@
*/
Gem5SystemC::MemoryManager mm;
+namespace
+{
+/**
+ * Hold all the callbacks necessary to convert a gem5 packet to tlm
payload.
+ */
+std::vector extraPacketToPayloadSteps;
+} // namespace
+
+/**
+ * Notify the Gem5ToTlm bridge that we need an extra step to properly
convert a
+ * gem5 packet to tlm payload. This can be useful when there exists a
SystemC
+ * extension that requires information in gem5 packet. For example, if a
user
+ * defined a SystemC extension the carries stream_id, the user may add a
step
+ * here to read stream_id out and set the extension properly.
+ */
+void
+addPacketToPayloadConversionStep(PacketToPayloadConversionStep step)
+{
+extraPacketToPayloadSteps.push_back(std::move(step));
+}
+
/**
* Convert a gem5 packet to a TLM payload by copying all the relevant
* information to new tlm payload.
@@ -110,6 +133,11 @@
auto *extension = new Gem5SystemC::Gem5Extension(packet);
trans->set_auto_extension(extension);
+// Apply all conversion steps necessary in this specific setup.
+for (auto : extraPacketToPayloadSteps) {
+step(packet, *trans);
+}
+
return trans;
}
diff --git a/src/systemc/tlm_bridge/gem5_to_tlm.hh
b/src/systemc/tlm_bridge/gem5_to_tlm.hh
index 2572027..d29bfd7 100644
--- a/src/systemc/tlm_bridge/gem5_to_tlm.hh
+++ b/src/systemc/tlm_bridge/gem5_to_tlm.hh
@@ -59,6 +59,7 @@
#ifndef __SYSTEMC_TLM_BRIDGE_GEM5_TO_TLM_HH__
#define __SYSTEMC_TLM_BRIDGE_GEM5_TO_TLM_HH__
+#include
#include
#include "mem/port.hh"
@@ -73,6 +74,11 @@
namespace sc_gem5
{
+using PacketToPayloadConversionStep =
+std::function;
+
+void addPacketToPayloadConversionStep(PacketToPayloadConversionStep step);
+
tlm::tlm_generic_payload *packet2payload(PacketPtr packet);
class Gem5ToTlmBridgeBase : public sc_core::sc_module
diff --git a/src/systemc/tlm_bridge/tlm_to_gem5.cc
b/src/systemc/tlm_bridge/tlm_to_gem5.cc
index 0cc0d7f..a1a8382 100644
--- a/src/systemc/tlm_bridge/tlm_to_gem5.cc
+++ b/src/systemc/tlm_bridge/tlm_to_gem5.cc
@@ -57,6 +57,8 @@
#include "systemc/tlm_bridge/tlm_to_gem5.hh"
+#include
+
#include "params/TlmToGem5Bridge32.hh"
#include "params/TlmToGem5Bridge64.hh"
#include "sim/system.hh"
@@ -66,6 +68,27 @@
namespace sc_gem5
{
+namespace
+{
+/**
+ * Hold all the callbacks necessary to convert a tlm payload to gem5
packet.
+ */
+std::vector extraPayloadToPacketSteps;
+} // namespace
+
+/**
+ * Notify the Tlm2Gem5 bridge that we need an extra step to properly
convert a
+ * tlm payload to gem5 packet. This can be useful when there exists a
SystemC
+ * extension that carries extra information. For example, SystemC user
might
+ * define an extension to store stream_id, the user may then add an extra
step
+ * to set the generated request's stream_id accordingly.
+ */
+void
+addPayloadToPacketConversionStep(PayloadToPacketConversionStep step)
+{
+extraPayloadToPacketSteps.push_back(std::move(step));
+}
+
PacketPtr
payload2packet(RequestorID _id, tlm::tlm_generic_payload )
{
@@ -96,6 +119,11 @@
auto pkt = new Packet(req, cmd);
pkt->dataStatic(trans.get_data_ptr());
+// Apply all conversion steps necessary in this specific setup.
+for (auto : extraPayloadToPacketSteps) {
+step(pkt,
[gem5-dev] Change in gem5/gem5[develop]: systemc: Make tlm/gem5 packet conversion flexible
Jui-min Lee has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/37075 )
Change subject: systemc: Make tlm/gem5 packet conversion flexible
..
systemc: Make tlm/gem5 packet conversion flexible
We used to have a hard-coded packet2payload and payload2packet in the
tlm_bridge implementation. However, as the conversion is operated on
generic tlm payload, we're not able to handle information stored in any
user defined SystemC extensions.
In this CL, we add a function to Tlm2Gem5BridgeBase/Gem52TlmBridgeBase
so we're able to register extra conversion steps between tlm payload and
gem5 packet. This decoupled the exact conversion logic and enable
SystemC users to register any necessary steps for their extensions.
Change-Id: I70b3405395fed0f757f0fb7e19136f47d84ac115
---
M src/systemc/tlm_bridge/gem5_to_tlm.cc
M src/systemc/tlm_bridge/gem5_to_tlm.hh
M src/systemc/tlm_bridge/tlm_to_gem5.cc
M src/systemc/tlm_bridge/tlm_to_gem5.hh
4 files changed, 65 insertions(+), 6 deletions(-)
diff --git a/src/systemc/tlm_bridge/gem5_to_tlm.cc
b/src/systemc/tlm_bridge/gem5_to_tlm.cc
index ba8f121..511cd0d 100644
--- a/src/systemc/tlm_bridge/gem5_to_tlm.cc
+++ b/src/systemc/tlm_bridge/gem5_to_tlm.cc
@@ -74,11 +74,28 @@
Gem5SystemC::MemoryManager mm;
/**
+ * Hold all the callbacks necessary to convert a gem5 packet to tlm
payload.
+ */
+std::vector extra_gem5_to_tlm_steps;
+
+/**
+ * Notify the Gem5ToTlm bridge that we need an extra step to properly
convert a
+ * gem5 packet to tlm payload. This can be useful when there exists a
SystemC
+ * extension that requires information in gem5 packet. For example, if a
user
+ * defined a SystemC extension the carries stream_id, the user may add a
step
+ * here to read stream_id out and set the extension properly.
+ */
+void Gem5ToTlmBridgeBase::addConversionStep(ConversionStep step)
+{
+extra_gem5_to_tlm_steps.push_back(step);
+}
+
+/**
* Convert a gem5 packet to a TLM payload by copying all the relevant
* information to new tlm payload.
*/
tlm::tlm_generic_payload *
-packet2payload(PacketPtr packet)
+Gem5ToTlmBridgeBase::packet2payload(PacketPtr packet)
{
tlm::tlm_generic_payload *trans = mm.allocate();
trans->acquire();
@@ -110,6 +127,11 @@
auto *extension = new Gem5SystemC::Gem5Extension(packet);
trans->set_auto_extension(extension);
+// Apply all conversion steps necessary in this specific setup.
+for (auto& step : extra_gem5_to_tlm_steps) {
+step(packet, *trans);
+}
+
return trans;
}
diff --git a/src/systemc/tlm_bridge/gem5_to_tlm.hh
b/src/systemc/tlm_bridge/gem5_to_tlm.hh
index 2572027..502931f 100644
--- a/src/systemc/tlm_bridge/gem5_to_tlm.hh
+++ b/src/systemc/tlm_bridge/gem5_to_tlm.hh
@@ -59,6 +59,7 @@
#ifndef __SYSTEMC_TLM_BRIDGE_GEM5_TO_TLM_HH__
#define __SYSTEMC_TLM_BRIDGE_GEM5_TO_TLM_HH__
+#include
#include
#include "mem/port.hh"
@@ -73,12 +74,18 @@
namespace sc_gem5
{
-tlm::tlm_generic_payload *packet2payload(PacketPtr packet);
-
class Gem5ToTlmBridgeBase : public sc_core::sc_module
{
protected:
using sc_core::sc_module::sc_module;
+
+ public:
+using ConversionStep =
+std::function)>;
+
+static void addConversionStep(ConversionStep step);
+
+static tlm::tlm_generic_payload *packet2payload(PacketPtr packet);
};
template
diff --git a/src/systemc/tlm_bridge/tlm_to_gem5.cc
b/src/systemc/tlm_bridge/tlm_to_gem5.cc
index 0cc0d7f..330d5c9 100644
--- a/src/systemc/tlm_bridge/tlm_to_gem5.cc
+++ b/src/systemc/tlm_bridge/tlm_to_gem5.cc
@@ -66,8 +66,25 @@
namespace sc_gem5
{
+/**
+ * Hold all the callbacks necessary to convert a tlm payload to gem5
packet.
+ */
+std::vector extra_tlm_to_gem5_steps;
+
+/**
+ * Notify the Tlm2Gem5 bridge that we need an extra step to properly
convert a
+ * tlm payload to gem5 packet. This can be useful when there exists a
SystemC
+ * extension that carries extra information. For example, SystemC user
might
+ * define an extension to store stream_id, the user may then add an extra
step
+ * to set the generated request's stream_id accordingly.
+ */
+void TlmToGem5BridgeBase::addConversionStep(ConversionStep step)
+{
+extra_tlm_to_gem5_steps.push_back(step);
+}
+
PacketPtr
-payload2packet(RequestorID _id, tlm::tlm_generic_payload )
+TlmToGem5BridgeBase::payload2packet(RequestorID _id,
tlm::tlm_generic_payload )
{
MemCmd cmd;
@@ -96,6 +113,11 @@
auto pkt = new Packet(req, cmd);
pkt->dataStatic(trans.get_data_ptr());
+// Apply all conversion steps necessary in this specific setup.
+for (auto& step : extra_tlm_to_gem5_steps) {
+step(pkt, trans);
+}
+
return pkt;
}
diff --git a/src/systemc/tlm_bridge/tlm_to_gem5.hh
b/src/systemc/tlm_bridge/tlm_to_gem5.hh
index 279f76d..b280515 100644
--- a/src/systemc/tlm_bridge/tlm_to_gem5.hh
+++
