CPU disassembler-memory accessor

CPU disassembler-memory accessor

[Ben Elliston]

Hi.

The following patch adds a third accessor to CPUs for the purpose of
providing the disassembler with access to the instruction memory
without causing undesirable side effects in the memory hierarchy.
Without this patch, the disassembler may produce unwanted side effects
such as priming cache lines as it groks around the instruction memory.
Since this inspection tool should not modify the system it is
inspecting, the new accessor provides an opportunity to wire the
disassembler directly to the instruction memory, bypassing caches.

If the disassembler-memory accessor is not used, the CPU will default
to using the insn-mem accessor so this change is backward compatible
with existing ports.  Any comments?

Ben

2002-05-28  Ben Elliston  <bje@redhat.com>
 
	* compCGEN.cxx (cgen_bi_endian_cpu::cgen_read_memory): Don't
	bother fiddling the latency values.  Catch exceptions raised by
	read_disasm_memory_1 and return 1 if so (as required by GNU
	opcodes read_memory functions).
	* common-xml/interface.xml: Document disassembler-memory accessor.
	* common-xml/behavior.xml (execution): Likewise.

2002-05-28  Ben Elliston  <bje@redhat.com>

	* sidcpuutil.h (basic_cpu::disassembler_bus): New accessor.
	(basic_cpu::basic_cpu): Initialise; register disassembler-memory".
	(basic_cpu::read_disasm_memory): New function template.
	(basic_big_endian_cpu::read_disasm_memory_{1,2,4,8}): New members.
	(basic_little_endian_cpu::read_disasm_memory_{1,2,4,8}): Likewise.
	(basic_bi_endian_cpu::read_disasm_memory_{1,2,4,8}): Likewise.

Index: component/cgen-cpu/compCGEN.cxx
===================================================================
RCS file: /cvs/src/src/sid/component/cgen-cpu/compCGEN.cxx,v
retrieving revision 1.8
diff -u -p -r1.8 compCGEN.cxx
--- component/cgen-cpu/compCGEN.cxx	7 Feb 2002 17:32:28 -0000	1.8
+++ component/cgen-cpu/compCGEN.cxx	29 May 2002 14:39:23 -0000
@@ -178,10 +178,6 @@ cgen_bi_endian_cpu::cgen_read_memory(bfd
 {
   cgen_bi_endian_cpu *thisp = static_cast<cgen_bi_endian_cpu *>(info->application_data);
 
-  // We don't want to penalize the disassembler with memory latency counts, so we
-  // store it away here ...
-  host_int_8 prev_latency = thisp->total_latency;
-
   switch (length) {
 #if 0 // XXX not sure if this has byte order dependancies or not
   case 1:
@@ -198,13 +194,16 @@ cgen_bi_endian_cpu::cgen_read_memory(bfd
     break;
 #endif
   default:
-    for (int i = 0; i < length; i++)
-      *(myaddr + i) = thisp->read_insn_memory_1(0, memaddr + i);
+    try {
+	for (int i = 0; i < length; i++)
+	  *(myaddr + i) = thisp->read_disasm_memory_1 (0, memaddr + i);
+    }
+    catch (cpu_memory_fault& f) {
+      // The GNU disassembler interface is not expecting an exception.
+      // Return a non-zero value as prescribed by the interface.
+      return 1;
+    }
   }
-
-  // ... and restore it here.
-  thisp->total_latency = prev_latency;
-
   return 0;
 }
 
Index: component/cgen-cpu/common-xml/behavior.xml
===================================================================
RCS file: /cvs/src/src/sid/component/cgen-cpu/common-xml/behavior.xml,v
retrieving revision 1.2
diff -u -p -r1.2 behavior.xml
--- component/cgen-cpu/common-xml/behavior.xml	14 Mar 2002 00:16:28 -0000	1.2
+++ component/cgen-cpu/common-xml/behavior.xml	29 May 2002 14:39:23 -0000
@@ -32,9 +32,12 @@
       <p>Each instruction is first fetched from memory via the
       <accessor>insn-memory</accessor> accessor, and its decoding
       traced if the <attribute>trace-extract?</attribute> attribute is
-      set to a true value.  The decoded form may be cached
-      indefinitely afterwards, although this cache is flushed when the
-      <pin>flush-icache</pin> pin is driven.</p>
+      set to a true value.  To prevent unwanted cache side effects,
+      the <accessor>disassembler-memory</accessor> accessor can be
+      used and connected directly to main memory, bypassing any memory
+      caches.  The decoded form may be cached indefinitely afterwards,
+      although this cache is flushed when the <pin>flush-icache</pin>
+      pin is driven.</p>
 
       <p>The <attribute>engine-type</attribute> attribute specifies
       whether the "scache" ("semantic cache") or "pbb" ("pseudo basic
Index: component/cgen-cpu/common-xml/interface.xml
===================================================================
RCS file: /cvs/src/src/sid/component/cgen-cpu/common-xml/interface.xml,v
retrieving revision 1.3
diff -u -p -r1.3 interface.xml
--- component/cgen-cpu/common-xml/interface.xml	8 May 2002 19:33:13 -0000	1.3
+++ component/cgen-cpu/common-xml/interface.xml	29 May 2002 14:39:23 -0000
@@ -16,6 +16,7 @@
     <!-- accessors -->
     <defaccessor name="data-memory" accesses="any" behaviors="execution" />
     <defaccessor name="insn-memory" accesses="typically 4-byte accesses" behaviors="execution" />
+    <defaccessor name="disassembler-memory" accesses="any" behaviors="execution" />
 
 
     <!-- buses -->
Index: include/sidcpuutil.h
===================================================================
RCS file: /cvs/src/src/sid/include/sidcpuutil.h,v
retrieving revision 1.19
diff -u -p -r1.19 sidcpuutil.h
--- include/sidcpuutil.h	23 Apr 2002 21:00:33 -0000	1.19
+++ include/sidcpuutil.h	29 May 2002 14:39:24 -0000
@@ -496,6 +496,7 @@ namespace sidutil
   protected:
     sid::bus* data_bus;
     sid::bus* insn_bus;
+    sid::bus* disassembler_bus;
 
   protected:
     template <typename BigOrLittleInt>
@@ -506,6 +507,8 @@ namespace sidutil
     BigOrLittleInt read_data_memory (sid::host_int_4 pc, sid::host_int_4 address, BigOrLittleInt) const;
     template <typename BigOrLittleInt>
     BigOrLittleInt write_data_memory (sid::host_int_4 pc, sid::host_int_4 address, BigOrLittleInt value) const;
+    template <typename BigOrLittleInt>
+    BigOrLittleInt read_disasm_memory (sid::host_int_4 pc, sid::host_int_4 address, BigOrLittleInt) const;
 
     // ------------------------------------------------------------------------
     
@@ -530,6 +533,8 @@ public:
 	add_accessor ("data-memory", & this->data_bus);
 	this->insn_bus = 0;
 	add_accessor ("insn-memory", & this->insn_bus);
+	this->disassembler_bus = 0;
+	add_accessor ("disassembler-memory", & this->disassembler_bus);
 	add_bus ("debugger-bus", & this->debugger_bus);
 
 	// pins
@@ -599,7 +604,25 @@ public:
 	dynamic_cast <ofstream&> (s) << t;
       return s;
     }
-  
+
+    template <typename BigOrLittleInt>
+    BigOrLittleInt basic_cpu::read_disasm_memory (sid::host_int_4 pc, sid::host_int_4 address, BigOrLittleInt) const
+      {
+	BigOrLittleInt value;
+	sid::bus::status s;
+	    
+	if (UNLIKELY (this->disassembler_bus))
+	  {
+	    s = this->disassembler_bus->read (address, value);
+	    if (LIKELY (s == sid::bus::ok))
+	      return value;
+	  }
+	else
+	  return read_insn_memory (pc, address, BigOrLittleInt());
+
+	throw cpu_memory_fault (pc, address, s, "insn disasm read");
+      }
+
     template <typename BigOrLittleInt>
     BigOrLittleInt basic_cpu::read_insn_memory (sid::host_int_4 pc, sid::host_int_4 address, BigOrLittleInt) const
       {
@@ -679,6 +702,26 @@ public:
       }
     ~basic_big_endian_cpu () throw() {}
 
+    sid::host_int_1 read_disasm_memory_1 (sid::host_int_4 pc, sid::host_int_4 address) const
+      {
+	return this->read_disasm_memory (pc, address, sid::big_int_1());
+      }
+
+    sid::host_int_2 read_disasm_memory_2 (sid::host_int_4 pc, sid::host_int_4 address) const
+      {
+	return this->read_disasm_memory (pc, address, sid::big_int_2());
+      }
+
+    sid::host_int_4 read_disasm_memory_4 (sid::host_int_4 pc, sid::host_int_4 address) const
+      {
+	return this->read_disasm_memory (pc, address, sid::big_int_4());
+      }
+
+    sid::host_int_8 read_disasm_memory_8 (sid::host_int_4 pc, sid::host_int_8 address) const
+      {
+	return this->read_disasm_memory (pc, address, sid::big_int_8());
+      }
+
     sid::host_int_1 read_insn_memory_1 (sid::host_int_4 pc, sid::host_int_4 address) const
       {
 	return this->read_insn_memory (pc, address, sid::big_int_1());
@@ -774,6 +817,26 @@ public:
       }
     ~basic_little_endian_cpu () throw() {}
 
+    sid::host_int_1 read_disasm_memory_1 (sid::host_int_4 pc, sid::host_int_4 address) const
+      {
+	return this->read_disasm_memory (pc, address, sid::little_int_1());
+      }
+
+    sid::host_int_2 read_disasm_memory_2 (sid::host_int_4 pc, sid::host_int_4 address) const
+      {
+	return this->read_disasm_memory (pc, address, sid::little_int_2());
+      }
+
+    sid::host_int_4 read_disasm_memory_4 (sid::host_int_4 pc, sid::host_int_4 address) const
+      {
+	return this->read_disasm_memory (pc, address, sid::little_int_4());
+      }
+
+    sid::host_int_8 read_disasm_memory_8 (sid::host_int_4 pc, sid::host_int_4 address) const
+      {
+	return this->read_disasm_memory (pc, address, sid::little_int_8());
+      }
+
     sid::host_int_1 read_insn_memory_1 (sid::host_int_4 pc, sid::host_int_4 address) const
       {
 	return this->read_insn_memory (pc, address, sid::little_int_1());
@@ -901,7 +964,38 @@ public:
 	i >> this->_current_endianness;
       }
 
+    sid::host_int_1 read_disasm_memory_1 (sid::host_int_4 pc, sid::host_int_4 address) const
+      {
+	if (this->_current_endianness == endian_little)
+	  return this->read_disasm_memory (pc, address, sid::little_int_1());
+	else // endian_big or endian_unknown
+	  return this->read_disasm_memory (pc, address, sid::big_int_1());
+      }
+
+    sid::host_int_2 read_disasm_memory_2 (sid::host_int_4 pc, sid::host_int_4 address) const
+      {
+	if (this->_current_endianness == endian_little)
+	  return this->read_disasm_memory (pc, address, sid::little_int_2());
+	else // endian_big or endian_unknown
+	  return this->read_disasm_memory (pc, address, sid::big_int_2());
+      }
 
+    sid::host_int_4 read_disasm_memory_4 (sid::host_int_4 pc, sid::host_int_4 address) const
+      {
+	if (this->_current_endianness == endian_little)
+	  return this->read_disasm_memory (pc, address, sid::little_int_4());
+	else // endian_big or endian_unknown
+	  return this->read_disasm_memory (pc, address, sid::big_int_4());
+      }
+
+    sid::host_int_8 read_disasm_memory_8 (sid::host_int_4 pc, sid::host_int_4 address) const
+      {
+	if (this->_current_endianness == endian_little)
+	  return this->read_disasm_memory (pc, address, sid::little_int_8());
+	else // endian_big or endian_unknown
+	  return this->read_disasm_memory (pc, address, sid::big_int_8());
+      }
+    
     sid::host_int_1 read_insn_memory_1 (sid::host_int_4 pc, sid::host_int_4 address) const
       {
 	if (this->_current_endianness == endian_little)

Re: CPU disassembler-memory accessor

[Frank Ch. Eigler]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

Hi -

bje wrote:
> [...]
> The following patch adds a third accessor to CPUs for the purpose of
> providing the disassembler with access to the instruction memory
> without causing undesirable side effects in the memory hierarchy.
> [...]

Thanks.

> [...] Any comments?

Yes, just one.  The read_disasm_memory_1 function is methinks
overengineered.  It doesn't need to throw exceptions, nor handle
accesses of other sizes.  Actually, why not just do a direct
sid::bus-level read() on the accessor and do away with the
read_disasm_memory* thingie altogether?


- FChE

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: CPU disassembler-memory accessor

[Ben Elliston]

>>>>> "FChE" == Frank Ch Eigler <fche@redhat.com> writes:

  FChE> Yes, just one.  The read_disasm_memory_1 function is methinks
  FChE> overengineered.  It doesn't need to throw exceptions, nor handle
  FChE> accesses of other sizes.  Actually, why not just do a direct
  FChE> sid::bus-level read() on the accessor and do away with the
  FChE> read_disasm_memory* thingie altogether?

Thanks for the feedback.  How's this instead?

Index: include/sidcpuutil.h
===================================================================
RCS file: /cvs/cvsfiles/devo/sid/include/sidcpuutil.h,v
retrieving revision 1.28.2.4
diff -u -p -r1.28.2.4 sidcpuutil.h
--- include/sidcpuutil.h	2002/04/23 21:07:51	1.28.2.4
+++ include/sidcpuutil.h	2002/05/31 13:15:37
@@ -495,6 +495,7 @@ namespace sidutil
   protected:
     sid::bus* data_bus;
     sid::bus* insn_bus;
+    sid::bus* disassembler_bus;
 
   protected:
     template <typename BigOrLittleInt>
@@ -506,6 +507,8 @@ namespace sidutil
     template <typename BigOrLittleInt>
     BigOrLittleInt write_data_memory (sid::host_int_4 pc, sid::host_int_4 address, BigOrLittleInt value) const;
 
+    bool read_disasm_memory (sid::host_int_4 address, sid::big_int_1& value) const;
+
     // ------------------------------------------------------------------------
     
 public:
@@ -529,6 +532,8 @@ public:
 	add_accessor ("data-memory", & this->data_bus);
 	this->insn_bus = 0;
 	add_accessor ("insn-memory", & this->insn_bus);
+	this->disassembler_bus = 0;
+	add_accessor ("disassembler-memory", & this->disassembler_bus);
 	add_bus ("debugger-bus", & this->debugger_bus);
 
 	// pins
@@ -597,6 +602,23 @@ public:
       else
 	dynamic_cast <ofstream&> (s) << t;
       return s;
+    }
+
+    inline bool
+    basic_cpu::read_disasm_memory (sid::host_int_4 address, sid::big_int_1& value) const
+    {
+      sid::bus* bus;
+      bus = (this->disassembler_bus) ? this->disassembler_bus : this->insn_bus;
+      
+      try
+	{
+	  if (LIKELY (bus->read (address, value) == sid::bus::ok))
+	    return true;
+	}
+      catch (cpu_memory_fault& f)
+	{
+	  return false;
+	}
     }
   
     template <typename BigOrLittleInt>

Index: component/cgen-cpu/compCGEN.cxx
===================================================================
RCS file: /cvs/cvsfiles/devo/sid/component/cgen-cpu/compCGEN.cxx,v
retrieving revision 1.66.2.5
diff -u -p -r1.66.2.5 compCGEN.cxx
--- component/cgen-cpu/compCGEN.cxx	2002/04/03 18:43:36	1.66.2.5
+++ component/cgen-cpu/compCGEN.cxx	2002/05/31 13:15:35
@@ -184,10 +184,6 @@ cgen_bi_endian_cpu::cgen_read_memory(bfd
 {
   cgen_bi_endian_cpu *thisp = static_cast<cgen_bi_endian_cpu *>(info->application_data);
 
-  // We don't want to penalize the disassembler with memory latency counts, so we
-  // store it away here ...
-  host_int_8 prev_latency = thisp->total_latency;
-
   switch (length) {
 #if 0 // XXX not sure if this has byte order dependancies or not
   case 1:
@@ -204,13 +200,17 @@ cgen_bi_endian_cpu::cgen_read_memory(bfd
     break;
 #endif
   default:
-    for (int i = 0; i < length; i++)
-      *(myaddr + i) = thisp->read_insn_memory_1(0, memaddr + i);
+    {
+      big_int_1 value;
+      for (int i = 0; i < length; i++)
+	{
+	  if (! thisp->read_disasm_memory (memaddr + i, value))
+	    return 1;
+	  else
+	    *(myaddr + i) = value;
+	}
+    }
   }
-
-  // ... and restore it here.
-  thisp->total_latency = prev_latency;
-
   return 0;
 }
 
Index: component/cgen-cpu/common-xml/behavior.xml
===================================================================
RCS file: /cvs/cvsfiles/devo/sid/component/cgen-cpu/common-xml/behavior.xml,v
retrieving revision 1.1.10.1
diff -u -p -r1.1.10.1 behavior.xml
--- component/cgen-cpu/common-xml/behavior.xml	2002/03/14 01:04:39	1.1.10.1
+++ component/cgen-cpu/common-xml/behavior.xml	2002/05/31 13:15:35
@@ -32,9 +32,12 @@
       <p>Each instruction is first fetched from memory via the
       <accessor>insn-memory</accessor> accessor, and its decoding
       traced if the <attribute>trace-extract?</attribute> attribute is
-      set to a true value.  The decoded form may be cached
-      indefinitely afterwards, although this cache is flushed when the
-      <pin>flush-icache</pin> pin is driven.</p>
+      set to a true value.  To prevent unwanted cache side effects,
+      the <accessor>disassembler-memory</accessor> accessor can be
+      used and connected directly to main memory, bypassing any memory
+      caches.  The decoded form may be cached indefinitely afterwards,
+      although this cache is flushed when the <pin>flush-icache</pin>
+      pin is driven.</p>
 
       <p>The <attribute>engine-type</attribute> attribute specifies
       whether the "scache" ("semantic cache") or "pbb" ("pseudo basic
Index: component/cgen-cpu/common-xml/interface.xml
===================================================================
RCS file: /cvs/cvsfiles/devo/sid/component/cgen-cpu/common-xml/interface.xml,v
retrieving revision 1.2.10.1
diff -u -p -r1.2.10.1 interface.xml
--- component/cgen-cpu/common-xml/interface.xml	2002/03/14 01:04:39	1.2.10.1
+++ component/cgen-cpu/common-xml/interface.xml	2002/05/31 13:15:35
@@ -16,6 +16,7 @@
     <!-- accessors -->
     <defaccessor name="data-memory" accesses="any" behaviors="execution" />
     <defaccessor name="insn-memory" accesses="typically 4-byte accesses" behaviors="execution" />
+    <defaccessor name="disassembler-memory" accesses="any" behaviors="execution" />
 
 
     <!-- buses -->

Re: CPU disassembler-memory accessor

[Frank Ch. Eigler]

[-- Attachment #1: Type: text/plain, Size: 748 bytes --]

Hi -

> Thanks for the feedback.  How's this instead?
> [...]

Almost perfect:


> +    inline bool
> +    basic_cpu::read_disasm_memory (sid::host_int_4 address, sid::big_int_1& value) const
> +    {
> +      sid::bus* bus;
> +      bus = (this->disassembler_bus) ? this->disassembler_bus : this->insn_bus;
> +      
> +      try
> +	{
> +	  if (LIKELY (bus->read (address, value) == sid::bus::ok))
> +	    return true;
> +	}
> +      catch (cpu_memory_fault& f)
> +	{
> +	  return false;
> +	}
>      }

This function is IMO still unnecessary.  For one, sid::bus::read can never
throw an exception (it's declared throw()).
Why not just inline " ... bus->read (address, value) ... " into
cgen_read_memory?


- FChE

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]