Split mem layout computation in spike.cc (#957)

The motivation here is mostly to enable a refactoring where the memory layout (sans allocated memory) gets passed to DTS/DTB code before we ever allocate anything. But it turns out to make merge_overlapping_memory_regions a bit simpler, which is an added bonus.
4 years ago · 168b4ea6a5
2 changed files with 65 additions and 27 deletions
--- a/riscv/cfg.h
+++ b/riscv/cfg.h
@ -3,6 +3,8 @@
 #define _RISCV_CFG_H

 #include "decode.h"
+#include "mmu.h"
+#include <cassert>

 template <typename T>
 class cfg_arg_t {
@ -25,17 +27,41 @@ private:
  bool was_set;
 };

+// Configuration that describes a memory region
+class mem_cfg_t
+{
+public:
+  mem_cfg_t(reg_t base, reg_t size)
+    : base(base), size(size)
+  {
+    // The truth of these assertions should be ensured by whatever is creating
+    // the regions in the first place, but we have them here to make sure that
+    // we can't end up describing memory regions that don't make sense. They
+    // ask that the page size is a multiple of the minimum page size, that the
+    // page is aligned to the minimum page size, that the page is non-empty and
+    // that the top address is still representable in a reg_t.
+    assert((size % PGSIZE == 0) &&
+           (base % PGSIZE == 0) &&
+           (base + size > base));
+  }
+
+  reg_t base;
+  reg_t size;
+};
+
 class cfg_t
 {
 public:
  cfg_t(std::pair<reg_t, reg_t> default_initrd_bounds,
        const char *default_bootargs, size_t default_nprocs,
-        const char *default_isa, const char *default_priv)
+        const char *default_isa, const char *default_priv,
+        const std::vector<mem_cfg_t> &default_mem_layout)
    : initrd_bounds(default_initrd_bounds),
      bootargs(default_bootargs),
      nprocs(default_nprocs),
      isa(default_isa),
-      priv(default_priv)
+      priv(default_priv),
+      mem_layout(default_mem_layout)
  {}

  cfg_arg_t<std::pair<reg_t, reg_t>> initrd_bounds;
@ -43,6 +69,7 @@ public:
  cfg_arg_t<size_t>                  nprocs;
  cfg_arg_t<const char *>            isa;
  cfg_arg_t<const char *>            priv;
+  cfg_arg_t<std::vector<mem_cfg_t>>  mem_layout;
 };

 #endif
--- a/spike_main/spike.cc
+++ b/spike_main/spike.cc
@ -107,35 +107,33 @@ static void read_file_bytes(const char *filename,size_t fileoff,
  mem->store(memoff, read_sz, (uint8_t*)&read_buf[0]);
 }

-bool sort_mem_region(const std::pair<reg_t, mem_t*> &a,
-                       const std::pair<reg_t, mem_t*> &b)
+bool sort_mem_region(const mem_cfg_t &a, const mem_cfg_t &b)
 {
-  if (a.first == b.first)
-    return (a.second->size() < b.second->size());
+  if (a.base == b.base)
+    return (a.size < b.size);
  else
-    return (a.first < b.first);
+    return (a.base < b.base);
 }

-void merge_overlapping_memory_regions(std::vector<std::pair<reg_t, mem_t*>>& mems)
+void merge_overlapping_memory_regions(std::vector<mem_cfg_t> &mems)
 {
  // check the user specified memory regions and merge the overlapping or
  // eliminate the containing parts
-  std::sort(mems.begin(), mems.end(), sort_mem_region);
-  std::vector<std::pair<reg_t, mem_t*>>::iterator it = mems.begin() + 1;
+  assert(!mems.empty());

-  while (it != mems.end()) {
-    reg_t start = prev(it)->first;
-    reg_t end = prev(it)->first + prev(it)->second->size();
-    reg_t start2 = it->first;
-    reg_t end2 = it->first + it->second->size();
+  std::sort(mems.begin(), mems.end(), sort_mem_region);
+  for (auto it = mems.begin() + 1; it != mems.end(); ) {
+    reg_t start = prev(it)->base;
+    reg_t end = prev(it)->base + prev(it)->size;
+    reg_t start2 = it->base;
+    reg_t end2 = it->base + it->size;

    //contains -> remove
    if (start2 >= start && end2 <= end) {
      it = mems.erase(it);
-    //parital overlapped -> extend
+    //partial overlapped -> extend
    } else if (start2 >= start && start2 < end) {
-      delete prev(it)->second;
-      prev(it)->second = new mem_t(std::max(end, end2) - start);
+      prev(it)->size = std::max(end, end2) - start;
      it = mems.erase(it);
    // no overlapping -> keep it
    } else {
@ -144,8 +142,10 @@ void merge_overlapping_memory_regions(std::vector<std::pair<reg_t, mem_t*>>& mem
  }
 }

-static std::vector<std::pair<reg_t, mem_t*>> make_mems(const char* arg)
+static std::vector<mem_cfg_t> parse_mem_layout(const char* arg)
 {
+  std::vector<mem_cfg_t> res;
+
  // handle legacy mem argument
  char* p;
  auto mb = strtoull(arg, &p, 0);
@ -153,11 +153,11 @@ static std::vector<std::pair<reg_t, mem_t*>> make_mems(const char* arg)
    reg_t size = reg_t(mb) << 20;
    if (size != (size_t)size)
      throw std::runtime_error("Size would overflow size_t");
-    return std::vector<std::pair<reg_t, mem_t*>>(1, std::make_pair(reg_t(DRAM_BASE), new mem_t(size)));
+    res.push_back(mem_cfg_t(reg_t(DRAM_BASE), size));
+    return res;
  }

  // handle base/size tuples
-  std::vector<std::pair<reg_t, mem_t*>> res;
  while (true) {
    auto base = strtoull(arg, &p, 0);
    if (!*p || *p != ':')
@ -180,7 +180,7 @@ static std::vector<std::pair<reg_t, mem_t*>> make_mems(const char* arg)
              base0, base0 + size0 - 1, long(PGSIZE / 1024), base, base + size - 1);
    }

-    res.push_back(std::make_pair(reg_t(base), new mem_t(size)));
+    res.push_back(mem_cfg_t(reg_t(base), reg_t(size)));
    if (!*p)
      break;
    if (*p != ',')
@ -189,9 +189,20 @@ static std::vector<std::pair<reg_t, mem_t*>> make_mems(const char* arg)
  }

  merge_overlapping_memory_regions(res);
+
  return res;
 }

+static std::vector<std::pair<reg_t, mem_t*>> make_mems(const std::vector<mem_cfg_t> &layout)
+{
+  std::vector<std::pair<reg_t, mem_t*>> mems;
+  mems.reserve(layout.size());
+  for (const auto &cfg : layout) {
+    mems.push_back(std::make_pair(cfg.base, new mem_t(cfg.size)));
+  }
+  return mems;
+}
+
 static unsigned long atoul_safe(const char* s)
 {
  char* e;
@ -222,7 +233,6 @@ int main(int argc, char** argv)
  const char* kernel = NULL;
  reg_t kernel_offset, kernel_size;
  reg_t start_pc = reg_t(-1);
-  std::vector<std::pair<reg_t, mem_t*>> mems;
  std::vector<std::pair<reg_t, abstract_device_t*>> plugin_devices;
  std::unique_ptr<icache_sim_t> ic;
  std::unique_ptr<dcache_sim_t> dc;
@ -253,7 +263,8 @@ int main(int argc, char** argv)
            /*default_bootargs=*/nullptr,
            /*default_nprocs=*/1,
            /*default_isa=*/DEFAULT_ISA,
-            /*default_priv=*/DEFAULT_PRIV);
+            /*default_priv=*/DEFAULT_PRIV,
+            /*default_mem_layout=*/parse_mem_layout("2048"));

  auto const hartids_parser = [&](const char *s) {
    std::string const str(s);
@ -320,7 +331,7 @@ int main(int argc, char** argv)
  parser.option('s', 0, 0, [&](const char* s){socket = true;});
 #endif
  parser.option('p', 0, 1, [&](const char* s){cfg.nprocs = atoul_nonzero_safe(s);});
-  parser.option('m', 0, 1, [&](const char* s){mems = make_mems(s);});
+  parser.option('m', 0, 1, [&](const char* s){cfg.mem_layout = parse_mem_layout(s);});
  // I wanted to use --halted, but for some reason that doesn't work.
  parser.option('H', 0, 0, [&](const char* s){halted = true;});
  parser.option(0, "rbb-port", 1, [&](const char* s){use_rbb = true; rbb_port = atoul_safe(s);});
@ -388,12 +399,12 @@ int main(int argc, char** argv)

  auto argv1 = parser.parse(argv);
  std::vector<std::string> htif_args(argv1, (const char*const*)argv + argc);
-  if (mems.empty())
-    mems = make_mems("2048");

  if (!*argv1)
    help();

+  std::vector<std::pair<reg_t, mem_t*>> mems = make_mems(cfg.mem_layout());
+
  if (kernel && check_file_exists(kernel)) {
    const char *isa = cfg.isa();
    kernel_size = get_file_size(kernel);