Clean up debug module options. (#299)

* Clean up debug module options.

1. Instead of passing each one a few levels deep, create
debug_module_config_t which contains them all.
2. Rename all those command line options so they start with --dm for
debug module.
3. Add --dm-no-halt-groups to disable halt group support.

* Update changelog.

ChangeLog.md

@@ -1,7 +1,13 @@
 Version 1.0.1-dev
 -----------------
 - Added `hasel` debug feature.
-- Added `--debug-no-abstract-csr` command-line option.
+- Added `--dm-no-abstract-csr` command-line option.
+- Renamed `--progsize` to `--dm-progsize`.
+- Renamed `--debug-sba` to `--dm-sba`.
+- Renamed `--debug-auth` to `--dm-auth`.
+- Renamed `--abstract-rti` to `--dm-abstract-rti`.
+- Renamed `--without-hasel` to `--dm-no-hasel`.
+- Added `--dm-no-halt-groups` command line option.
 
 Version 1.0.0 (2019-03-30)
 --------------------------

riscv/debug_module.cc

@@ -31,16 +31,10 @@ static unsigned field_width(unsigned n)
 
 ///////////////////////// debug_module_t
 
-debug_module_t::debug_module_t(sim_t *sim, unsigned progbufsize, unsigned max_bus_master_bits,
-    bool require_authentication, unsigned abstract_rti, bool support_hasel,
-    bool support_abstract_csr_access) :
+debug_module_t::debug_module_t(sim_t *sim, const debug_module_config_t &config) :
   nprocs(sim->nprocs()),
-  progbufsize(progbufsize),
-  program_buffer_bytes(4 + 4*progbufsize),
-  max_bus_master_bits(max_bus_master_bits),
-  require_authentication(require_authentication),
-  abstract_rti(abstract_rti),
-  support_abstract_csr_access(support_abstract_csr_access),
+  config(config),
+  program_buffer_bytes(4 + 4*config.progbufsize),
   debug_progbuf_start(debug_data_start - program_buffer_bytes),
   debug_abstract_start(debug_progbuf_start - debug_abstract_size*4),
   custom_base(0),
@@ -49,8 +43,7 @@ debug_module_t::debug_module_t(sim_t *sim, unsigned progbufsize, unsigned max_bu
   // The spec lets a debugger select nonexistent harts. Create hart_state for
   // them because I'm too lazy to add the code to just ignore accesses.
   hart_state(1 << field_width(sim->nprocs())),
-  hart_array_mask(sim->nprocs()),
-  support_hasel(support_hasel)
+  hart_array_mask(sim->nprocs())
 {
   D(fprintf(stderr, "debug_data_start=0x%x\n", debug_data_start));
   D(fprintf(stderr, "debug_progbuf_start=0x%x\n", debug_progbuf_start));
@@ -62,10 +55,10 @@ debug_module_t::debug_module_t(sim_t *sim, unsigned progbufsize, unsigned max_bu
 
   memset(debug_rom_flags, 0, sizeof(debug_rom_flags));
   memset(program_buffer, 0, program_buffer_bytes);
-  program_buffer[4*progbufsize] = ebreak();
-  program_buffer[4*progbufsize+1] = ebreak() >> 8;
-  program_buffer[4*progbufsize+2] = ebreak() >> 16;
-  program_buffer[4*progbufsize+3] = ebreak() >> 24;
+  program_buffer[4*config.progbufsize] = ebreak();
+  program_buffer[4*config.progbufsize+1] = ebreak() >> 8;
+  program_buffer[4*config.progbufsize+2] = ebreak() >> 16;
+  program_buffer[4*config.progbufsize+3] = ebreak() >> 24;
   memset(dmdata, 0, sizeof(dmdata));
 
   write32(debug_rom_whereto, 0,
@@ -93,27 +86,27 @@ void debug_module_t::reset()
 
   dmstatus = {0};
   dmstatus.impebreak = true;
-  dmstatus.authenticated = !require_authentication;
+  dmstatus.authenticated = !config.require_authentication;
   dmstatus.version = 2;
 
   abstractcs = {0};
   abstractcs.datacount = sizeof(dmdata) / 4;
-  abstractcs.progbufsize = progbufsize;
+  abstractcs.progbufsize = config.progbufsize;
 
   abstractauto = {0};
 
   sbcs = {0};
-  if (max_bus_master_bits > 0) {
+  if (config.max_bus_master_bits > 0) {
     sbcs.version = 1;
     sbcs.asize = sizeof(reg_t) * 8;
   }
-  if (max_bus_master_bits >= 64)
+  if (config.max_bus_master_bits >= 64)
     sbcs.access64 = true;
-  if (max_bus_master_bits >= 32)
+  if (config.max_bus_master_bits >= 32)
     sbcs.access32 = true;
-  if (max_bus_master_bits >= 16)
+  if (config.max_bus_master_bits >= 16)
     sbcs.access16 = true;
-  if (max_bus_master_bits >= 8)
+  if (config.max_bus_master_bits >= 8)
     sbcs.access8 = true;
 
   challenge = random();
@@ -297,7 +290,7 @@ unsigned debug_module_t::sb_access_bits()
 
 void debug_module_t::sb_autoincrement()
 {
-  if (!sbcs.autoincrement || !max_bus_master_bits)
+  if (!sbcs.autoincrement || !config.max_bus_master_bits)
     return;
 
   uint64_t value = sbaddress[0] + sb_access_bits() / 8;
@@ -319,13 +312,13 @@ void debug_module_t::sb_read()
 {
   reg_t address = ((uint64_t) sbaddress[1] << 32) | sbaddress[0];
   try {
-    if (sbcs.sbaccess == 0 && max_bus_master_bits >= 8) {
+    if (sbcs.sbaccess == 0 && config.max_bus_master_bits >= 8) {
       sbdata[0] = sim->debug_mmu->load_uint8(address);
-    } else if (sbcs.sbaccess == 1 && max_bus_master_bits >= 16) {
+    } else if (sbcs.sbaccess == 1 && config.max_bus_master_bits >= 16) {
       sbdata[0] = sim->debug_mmu->load_uint16(address);
-    } else if (sbcs.sbaccess == 2 && max_bus_master_bits >= 32) {
+    } else if (sbcs.sbaccess == 2 && config.max_bus_master_bits >= 32) {
       sbdata[0] = sim->debug_mmu->load_uint32(address);
-    } else if (sbcs.sbaccess == 3 && max_bus_master_bits >= 64) {
+    } else if (sbcs.sbaccess == 3 && config.max_bus_master_bits >= 64) {
       uint64_t value = sim->debug_mmu->load_uint64(address);
       sbdata[0] = value;
       sbdata[1] = value >> 32;
@@ -341,13 +334,13 @@ void debug_module_t::sb_write()
 {
   reg_t address = ((uint64_t) sbaddress[1] << 32) | sbaddress[0];
   D(fprintf(stderr, "sb_write() 0x%x @ 0x%lx\n", sbdata[0], address));
-  if (sbcs.sbaccess == 0 && max_bus_master_bits >= 8) {
+  if (sbcs.sbaccess == 0 && config.max_bus_master_bits >= 8) {
     sim->debug_mmu->store_uint8(address, sbdata[0]);
-  } else if (sbcs.sbaccess == 1 && max_bus_master_bits >= 16) {
+  } else if (sbcs.sbaccess == 1 && config.max_bus_master_bits >= 16) {
     sim->debug_mmu->store_uint16(address, sbdata[0]);
-  } else if (sbcs.sbaccess == 2 && max_bus_master_bits >= 32) {
+  } else if (sbcs.sbaccess == 2 && config.max_bus_master_bits >= 32) {
     sim->debug_mmu->store_uint32(address, sbdata[0]);
-  } else if (sbcs.sbaccess == 3 && max_bus_master_bits >= 64) {
+  } else if (sbcs.sbaccess == 3 && config.max_bus_master_bits >= 64) {
     sim->debug_mmu->store_uint64(address,
         (((uint64_t) sbdata[1]) << 32) | sbdata[0]);
   } else {
@@ -374,7 +367,7 @@ bool debug_module_t::dmi_read(unsigned address, uint32_t *value)
     if (!abstractcs.busy && ((abstractauto.autoexecdata >> i) & 1)) {
       perform_abstract_command();
     }
-  } else if (address >= DMI_PROGBUF0 && address < DMI_PROGBUF0 + progbufsize) {
+  } else if (address >= DMI_PROGBUF0 && address < DMI_PROGBUF0 + config.progbufsize) {
     unsigned i = address - DMI_PROGBUF0;
     result = read32(program_buffer, i);
     if (abstractcs.busy) {
@@ -587,7 +580,7 @@ bool debug_module_t::perform_abstract_command()
     unsigned i = 0;
     if (get_field(command, AC_ACCESS_REGISTER_TRANSFER)) {
 
-      if (regno < 0x1000 && support_abstract_csr_access) {
+      if (regno < 0x1000 && config.support_abstract_csr_access) {
         write32(debug_abstract, i++, csrw(S0, CSR_DSCRATCH));
 
         if (write) {
@@ -700,7 +693,7 @@ bool debug_module_t::perform_abstract_command()
     }
 
     debug_rom_flags[dmcontrol.hartsel] |= 1 << DEBUG_ROM_FLAG_GO;
-    rti_remaining = abstract_rti;
+    rti_remaining = config.abstract_rti;
     abstract_command_completed = false;
 
     abstractcs.busy = true;
@@ -732,7 +725,7 @@ bool debug_module_t::dmi_write(unsigned address, uint32_t value)
     }
     return true;
 
-  } else if (address >= DMI_PROGBUF0 && address < DMI_PROGBUF0 + progbufsize) {
+  } else if (address >= DMI_PROGBUF0 && address < DMI_PROGBUF0 + config.progbufsize) {
     unsigned i = address - DMI_PROGBUF0;
 
     if (!abstractcs.busy)
@@ -757,7 +750,7 @@ bool debug_module_t::dmi_write(unsigned address, uint32_t value)
           dmcontrol.resumereq = get_field(value, DMI_DMCONTROL_RESUMEREQ);
           dmcontrol.hartreset = get_field(value, DMI_DMCONTROL_HARTRESET);
           dmcontrol.ndmreset = get_field(value, DMI_DMCONTROL_NDMRESET);
-          if (support_hasel)
+          if (config.support_hasel)
             dmcontrol.hasel = get_field(value, DMI_DMCONTROL_HASEL);
           else
             dmcontrol.hasel = 0;
@@ -871,7 +864,7 @@ bool debug_module_t::dmi_write(unsigned address, uint32_t value)
       case DMI_AUTHDATA:
         D(fprintf(stderr, "debug authentication: got 0x%x; 0x%x unlocks\n", value,
             challenge + secret));
-        if (require_authentication) {
+        if (config.require_authentication) {
           if (value == challenge + secret) {
             dmstatus.authenticated = true;
           } else {
@@ -881,7 +874,7 @@ bool debug_module_t::dmi_write(unsigned address, uint32_t value)
         }
         return true;
       case DMI_DMCS2:
-        if (get_field(value, DMI_DMCS2_HGWRITE)) {
+        if (config.support_haltgroups && get_field(value, DMI_DMCS2_HGWRITE)) {
           hart_state[dmcontrol.hartsel].haltgroup = get_field(value,
               DMI_DMCS2_HALTGROUP);
         }

riscv/debug_module.h

@@ -8,6 +8,18 @@
 
 class sim_t;
 
+typedef struct {
+    // Size of program_buffer in 32-bit words, as exposed to the rest of the
+    // world.
+    unsigned progbufsize;
+    unsigned max_bus_master_bits;
+    bool require_authentication;
+    unsigned abstract_rti;
+    bool support_hasel;
+    bool support_abstract_csr_access;
+    bool support_haltgroups;
+} debug_module_config_t;
+
 typedef struct {
   bool haltreq;
   bool resumereq;
@@ -93,10 +105,7 @@ class debug_module_t : public abstract_device_t
      * abstract_rti is extra run-test/idle cycles that each abstract command
      * takes to execute. Useful for testing OpenOCD.
      */
-    debug_module_t(sim_t *sim, unsigned progbufsize,
-        unsigned max_bus_master_bits, bool require_authentication,
-        unsigned abstract_rti, bool support_hasel,
-        bool support_abstract_csr_access);
+    debug_module_t(sim_t *sim, const debug_module_config_t &config);
     ~debug_module_t();
 
     void add_device(bus_t *bus);
@@ -119,16 +128,10 @@ class debug_module_t : public abstract_device_t
   private:
     static const unsigned datasize = 2;
     unsigned nprocs;
-    // Size of program_buffer in 32-bit words, as exposed to the rest of the
-    // world.
-    unsigned progbufsize;
+    debug_module_config_t config;
     // Actual size of the program buffer, which is 1 word bigger than we let on
     // to implement the implicit ebreak at the end.
     unsigned program_buffer_bytes;
-    unsigned max_bus_master_bits;
-    bool require_authentication;
-    unsigned abstract_rti;
-    bool support_abstract_csr_access;
     static const unsigned debug_data_start = 0x380;
     unsigned debug_progbuf_start;
 
@@ -182,7 +185,6 @@ class debug_module_t : public abstract_device_t
 
     bool abstract_command_completed;
     unsigned rti_remaining;
-    bool support_hasel;
 };
 
 #endif

riscv/sim.cc

@@ -27,16 +27,12 @@ static void handle_signal(int sig)
 sim_t::sim_t(const char* isa, size_t nprocs, bool halted, reg_t start_pc,
              std::vector<std::pair<reg_t, mem_t*>> mems,
              const std::vector<std::string>& args,
-             std::vector<int> const hartids, unsigned progsize,
-             unsigned max_bus_master_bits, bool require_authentication,
-             suseconds_t abstract_delay_usec, bool support_hasel,
-             bool support_abstract_csr_access)
+             std::vector<int> const hartids,
+             const debug_module_config_t &dm_config)
   : htif_t(args), mems(mems), procs(std::max(nprocs, size_t(1))),
     start_pc(start_pc), current_step(0), current_proc(0), debug(false),
     histogram_enabled(false), dtb_enabled(true), remote_bitbang(NULL),
-    debug_module(this, progsize, max_bus_master_bits, require_authentication,
-        abstract_delay_usec, support_hasel,
-        support_abstract_csr_access)
+    debug_module(this, dm_config)
 {
   signal(SIGINT, &handle_signal);
 

riscv/sim.h

@@ -24,9 +24,7 @@ public:
   sim_t(const char* isa, size_t _nprocs,  bool halted, reg_t start_pc,
         std::vector<std::pair<reg_t, mem_t*>> mems,
         const std::vector<std::string>& args, const std::vector<int> hartids,
-        unsigned progsize, unsigned max_bus_master_bits,
-        bool require_authentication, suseconds_t abstract_delay_usec,
-        bool support_hasel, bool support_abstract_csr_access);
+        const debug_module_config_t &dm_config);
   ~sim_t();
 
   // run the simulation to completion

spike_main/spike.cc

@@ -40,16 +40,17 @@ static void help(int exit_code = 1)
   fprintf(stderr, "  --rbb-port=<port>     Listen on <port> for remote bitbang connection\n");
   fprintf(stderr, "  --dump-dts            Print device tree string and exit\n");
   fprintf(stderr, "  --disable-dtb         Don't write the device tree blob into memory\n");
-  fprintf(stderr, "  --progsize=<words>    Progsize for the debug module [default 2]\n");
-  fprintf(stderr, "  --debug-sba=<bits>    Debug bus master supports up to "
+  fprintf(stderr, "  --dm-progsize=<words> Progsize for the debug module [default 2]\n");
+  fprintf(stderr, "  --dm-sba=<bits>       Debug bus master supports up to "
       "<bits> wide accesses [default 0]\n");
-  fprintf(stderr, "  --debug-auth          Debug module requires debugger to authenticate\n");
+  fprintf(stderr, "  --dm-auth             Debug module requires debugger to authenticate\n");
   fprintf(stderr, "  --dmi-rti=<n>         Number of Run-Test/Idle cycles "
       "required for a DMI access [default 0]\n");
-  fprintf(stderr, "  --abstract-rti=<n>    Number of Run-Test/Idle cycles "
+  fprintf(stderr, "  --dm-abstract-rti=<n> Number of Run-Test/Idle cycles "
       "required for an abstract command to execute [default 0]\n");
-  fprintf(stderr, "  --without-hasel       Debug module supports hasel\n");
-  fprintf(stderr, "  --debug-no-abstract-csr  Debug module won't support abstract to authenticate\n");
+  fprintf(stderr, "  --dm-no-hasel         Debug module supports hasel\n");
+  fprintf(stderr, "  --dm-no-abstract-csr  Debug module won't support abstract to authenticate\n");
+  fprintf(stderr, "  --dm-no-halt-groups   Debug module won't support halt groups\n");
 
   exit(exit_code);
 }
@@ -110,13 +111,16 @@ int main(int argc, char** argv)
   const char* isa = DEFAULT_ISA;
   uint16_t rbb_port = 0;
   bool use_rbb = false;
-  unsigned progsize = 2;
-  unsigned max_bus_master_bits = 0;
-  bool require_authentication = false;
   unsigned dmi_rti = 0;
-  unsigned abstract_rti = 0;
-  bool support_hasel = true;
-  bool support_abstract_csr_access = true;
+  debug_module_config_t dm_config = {
+    .progbufsize = 2,
+    .max_bus_master_bits = 0,
+    .require_authentication = false,
+    .abstract_rti = 0,
+    .support_hasel = true,
+    .support_abstract_csr_access = true,
+    .support_haltgroups = true
+  };
   std::vector<int> hartids;
 
   auto const hartids_parser = [&](const char *s) {
@@ -159,19 +163,22 @@ int main(int argc, char** argv)
       exit(-1);
     }
   });
-  parser.option(0, "progsize", 1, [&](const char* s){progsize = atoi(s);});
-  parser.option(0, "debug-sba", 1,
-      [&](const char* s){max_bus_master_bits = atoi(s);});
-  parser.option(0, "debug-auth", 0,
-      [&](const char* s){require_authentication = true;});
+  parser.option(0, "dm-progsize", 1,
+      [&](const char* s){dm_config.progbufsize = atoi(s);});
+  parser.option(0, "dm-sba", 1,
+      [&](const char* s){dm_config.max_bus_master_bits = atoi(s);});
+  parser.option(0, "dm-auth", 0,
+      [&](const char* s){dm_config.require_authentication = true;});
   parser.option(0, "dmi-rti", 1,
       [&](const char* s){dmi_rti = atoi(s);});
-  parser.option(0, "abstract-rti", 1,
-      [&](const char* s){abstract_rti = atoi(s);});
-  parser.option(0, "without-hasel", 0,
-      [&](const char* s){support_hasel = false;});
-  parser.option(0, "debug-no-abstract-csr", 0,
-      [&](const char* s){support_abstract_csr_access = false;});
+  parser.option(0, "dm-abstract-rti", 1,
+      [&](const char* s){dm_config.abstract_rti = atoi(s);});
+  parser.option(0, "dm-no-hasel", 0,
+      [&](const char* s){dm_config.support_hasel = false;});
+  parser.option(0, "dm-no-abstract-csr", 0,
+      [&](const char* s){dm_config.support_abstract_csr_access = false;});
+  parser.option(0, "dm-no-halt-groups", 0,
+      [&](const char* s){dm_config.support_haltgroups = false;});
 
   auto argv1 = parser.parse(argv);
   std::vector<std::string> htif_args(argv1, (const char*const*)argv + argc);
@@ -182,8 +189,7 @@ int main(int argc, char** argv)
     help();
 
   sim_t s(isa, nprocs, halted, start_pc, mems, htif_args, std::move(hartids),
-      progsize, max_bus_master_bits, require_authentication,
-      abstract_rti, support_hasel, support_abstract_csr_access);
+      dm_config);
   std::unique_ptr<remote_bitbang_t> remote_bitbang((remote_bitbang_t *) NULL);
   std::unique_ptr<jtag_dtm_t> jtag_dtm(
       new jtag_dtm_t(&s.debug_module, dmi_rti));