MIPS simulator initializes LSI pmon vector table with code

MIPS simulator initializes LSI pmon vector table with code

[Alexandre Oliva]

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

There's a chunk of memory in LSI pmon that is supposed to contain
pointers to `system call' handlers.  However, sim_open() will
clobber the valid addresses it stores in this table with code bare
code (as opposed to pointers to code) that was meant to catch invalid
accesses.  This is obviously wrong.  I'm checking this in, since Frank
Ch. Eigler and Eric Christopher agreed it was a reasonable thing to
do.  I'll leave the complete fix for someone who has a real
understanding of what was supposed to be done there.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sim-mips-no-code-in-vtable.patch --]
[-- Type: text/x-patch, Size: 1083 bytes --]

Index: sim/mips/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* interp.c (sim_open): Disable chunk of code that wrote code in
	vector table entries.

Index: sim/mips/interp.c
===================================================================
RCS file: /cvs/src/src/sim/mips/interp.c,v
retrieving revision 1.9
diff -u -p -r1.9 interp.c
--- sim/mips/interp.c 20 Mar 2002 01:35:13 -0000 1.9
+++ sim/mips/interp.c 18 Apr 2002 19:41:48 -0000
@@ -698,10 +698,14 @@ sim_open (kind, cb, abfd, argv)
       sim_write (sd, 0x80000000, (char *) halt, sizeof (halt));
       sim_write (sd, 0x80000180, (char *) halt, sizeof (halt));
       sim_write (sd, 0x80000200, (char *) halt, sizeof (halt));
+#if 0
+      /* This is wrong.  We're not supposed to write code to the
+	 vector tables, but rather pointers to code.  */
       /* XXX: Write here unconditionally? */
       sim_write (sd, 0xBFC00200, (char *) halt, sizeof (halt));
       sim_write (sd, 0xBFC00380, (char *) halt, sizeof (halt));
       sim_write (sd, 0xBFC00400, (char *) halt, sizeof (halt));
+#endif
     }
   }
 

[-- Attachment #3: Type: text/plain, Size: 289 bytes --]


-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Red Hat GCC Developer                  aoliva@{cygnus.com, redhat.com}
CS PhD student at IC-Unicamp        oliva@{lsd.ic.unicamp.br, gnu.org}
Free Software Evangelist                Professional serial bug killer

Re: MIPS simulator initializes LSI pmon vector table with code

[Andrew Cagney]

FYI,

The MIPS SIM maintainers are:

mips            Andrew Cagney <ac131313@redhat.com>
mips            Chris Demetriou <cgd@broadcom.com>

you may want to consult one of them before committing this.

Andrew

Re: MIPS simulator initializes LSI pmon vector table with code

[Alexandre Oliva]

On Apr 18, 2002, Andrew Cagney <ac131313@cygnus.com> wrote:

> FYI,
> The MIPS SIM maintainers are:

> mips            Andrew Cagney <ac131313@redhat.com>
> mips            Chris Demetriou <cgd@broadcom.com>

> you may want to consult one of them before committing this.

Apologies.  I had blindly assumed that Frank was the maintainer, just
because he owned the ChangeLog entries corresponding to that chunk of
code.  Stupid me.  I'll try to be more careful with such assumptions
next time.

-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Red Hat GCC Developer                  aoliva@{cygnus.com, redhat.com}
CS PhD student at IC-Unicamp        oliva@{lsd.ic.unicamp.br, gnu.org}
Free Software Evangelist                Professional serial bug killer

Re: MIPS simulator initializes LSI pmon vector table with code

[cgd]

At Thu, 18 Apr 2002 19:47:04 +0000 (UTC), "Alexandre Oliva" wrote:
> There's a chunk of memory in LSI pmon that is supposed to contain
> pointers to `system call' handlers.

What Andrew said, and...

Actually, your patch seems incorrect.

The code at 0xbfc00200, etc., _ARE_ supposed to be code; in MIPS
they're various exception entry points.  If they are data which is
invalid code, the wrong thing will almost certainly happen.

I cannot believe that any monitor does what you describe (puts a table
of addresses here), as doing so (instead of putting vectors there)
would be fundamentally incompatible with the MIPS architecture.


> However, sim_open() will
> clobber the valid addresses it stores in this table with code bare
> code (as opposed to pointers to code) that was meant to catch invalid
> accesses.  This is obviously wrong.  I'm checking this in, since Frank
> Ch. Eigler and Eric Christopher agreed it was a reasonable thing to
> do.  I'll leave the complete fix for someone who has a real
> understanding of what was supposed to be done there.

Please justify this further, preferably with a pointer to
code that is broken by the old behaviour or to documentation.

Otherwise, I think this should almost certainly be backed out...



cgd

Re: MIPS simulator initializes LSI pmon vector table with code

[Eric Christopher]

> > LSI TinyRisc.
> > 
> > I'll try to find the documentation - it'll be quite old though.
> 
> It's fine to support quite old things, IMO.  But, it shouldn't break
> the other support.
> 

This was support that worked a while back and the change that Alex is
reverting broke the LSI support. We aren't supporting something new,
we're fixing something that someone else broke :)


> At the very least, if it's going to be there at all, it should be
> conditional on the CPU being simulated being the CPU you're trying to
> support w/ the change.

This I can agree with.

-eric

-- 
Written using state-of-the-rat
technology.

Re: MIPS simulator initializes LSI pmon vector table with code

[cgd]

At 19 Apr 2002 11:34:58 -0700, Eric Christopher wrote:
> This was support that worked a while back and the change that Alex is
> reverting broke the LSI support. We aren't supporting something new,
> we're fixing something that someone else broke :)

well, at some point in time perhaps.

it looks like this code has been in the public cvs tree in some form
forever.

rev 1.1 did code like this, but it was moved in rev 1.2 (by fche) to
later in the function and made conditional on perhaps the wrong
variables.


"change that Alex is reverting" isn't a good description: if he
actually had reverted the change that put those there, he would have
put those calls back in their original locations and let the
subsequent vector-setting code do its work afterward, rather than just
commenting them out.


I'm still not seeing anything that convinces me that this change is
correct, or even more correct (using the metric of "number of
platforms broken") than what was there before.


cgd

Re: MIPS simulator initializes LSI pmon vector table with code

[Eric Christopher]

On Fri, 2002-04-19 at 12:06, cgd@broadcom.com wrote:
> At 19 Apr 2002 11:34:58 -0700, Eric Christopher wrote:
> > This was support that worked a while back and the change that Alex is
> > reverting broke the LSI support. We aren't supporting something new,
> > we're fixing something that someone else broke :)
> 
> well, at some point in time perhaps.
> 
> it looks like this code has been in the public cvs tree in some form
> forever.
> 

Forever is a very long time. mips16 didn't work in net gcc for more than
a year and a half until somebody (me) noticed :)

> rev 1.1 did code like this, but it was moved in rev 1.2 (by fche) to
> later in the function and made conditional on perhaps the wrong
> variables.
> 

OK.

> 
> "change that Alex is reverting" isn't a good description: if he
> actually had reverted the change that put those there, he would have
> put those calls back in their original locations and let the
> subsequent vector-setting code do its work afterward, rather than just
> commenting them out.
> 

True.

> 
> I'm still not seeing anything that convinces me that this change is
> correct, or even more correct (using the metric of "number of
> platforms broken") than what was there before.
> 

OK. I'll let Alex explain the sequence of events that shows that the
current code isn't correct, or at least breaks what used to be the
"existing" lsi support :)

-eric

-- 
Written using state-of-the-rat
technology.

Re: MIPS simulator initializes LSI pmon vector table with code

[Alexandre Oliva]

On Apr 18, 2002, cgd@broadcom.com wrote:

> The code at 0xbfc00200, etc., _ARE_ supposed to be code; in MIPS
> they're various exception entry points.  If they are data which is
> invalid code, the wrong thing will almost certainly happen.

Ok, so could you please explain to me how these two chunks of code can
possibly be correct:

	if (lsipmon_monitor_base != 0)
	  {
	    address_word vaddr = (lsipmon_monitor_base + (loop * 4));
	    sim_write (sd, vaddr, (char *)&value, sizeof (value));
	  }

(where lsipmon_monitor_base is 0xBFC00200, and loop varies from 0 to
23.  I read that as writing pointers into lsipmon_monitor_base)

      sim_write (sd, 0xBFC00200, (char *) halt, sizeof (halt));
      sim_write (sd, 0xBFC00380, (char *) halt, sizeof (halt));
      sim_write (sd, 0xBFC00400, (char *) halt, sizeof (halt));

(where halt is 64-byte long chunk of code, which means that one of
these halts overwrites not only its own entry in lsipmon_monitor_base,
but also the following entry.)

> I cannot believe that any monitor does what you describe (puts a table
> of addresses here), as doing so (instead of putting vectors there)
> would be fundamentally incompatible with the MIPS architecture.

I stand corrected.  It doesn't write pointers into chose chunks.  I
misremembered what I had seen.  The values of `value' that are written
in the first chunk of code as numbers that range from 6 to 28, except
for the printf service.  However, I don't see how a code sequence that
halts the machine could possibly fit there as an entry in the vector
table.

Enlightenment would be appreciated.

Thanks,

-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Red Hat GCC Developer                  aoliva@{cygnus.com, redhat.com}
CS PhD student at IC-Unicamp        oliva@{lsd.ic.unicamp.br, gnu.org}
Free Software Evangelist                Professional serial bug killer

Re: MIPS simulator initializes LSI pmon vector table with code

[cgd]

At 19 Apr 2002 18:06:34 -0300, Alexandre Oliva wrote:
> Ok, so could you please explain to me how these two chunks of code can
> possibly be correct:
> 
> 	if (lsipmon_monitor_base != 0)
> 	  {
> 	    address_word vaddr = (lsipmon_monitor_base + (loop * 4));
> 	    sim_write (sd, vaddr, (char *)&value, sizeof (value));
> 	  }
> 
> (where lsipmon_monitor_base is 0xBFC00200, and loop varies from 0 to
> 23.  I read that as writing pointers into lsipmon_monitor_base)
> 
>       sim_write (sd, 0xBFC00200, (char *) halt, sizeof (halt));
>       sim_write (sd, 0xBFC00380, (char *) halt, sizeof (halt));
>       sim_write (sd, 0xBFC00400, (char *) halt, sizeof (halt));
> 
> (where halt is 64-byte long chunk of code, which means that one of
> these halts overwrites not only its own entry in lsipmon_monitor_base,
> but also the following entry.)

Together, you mean?  And I assume you mean 64-bit rather than
'64-byte'?

(64 bit or 64-byte, I don't see what you mean by 'overwrites its own
entry...')

I think it's clear -- otherwise you wouldn't have had a problem to
begin with -- that the current situation doesn't do what's intended
for lsipmon.  8-)  More towards the bottom...


Well, note that I'm not the author of this code, nor was I a
maintainer when it got into its current state.  I suggest you look at
the diffs between rev 1.1 and 1.2 -- and then consult fche, who made
the change -- to find out why those changes were made.

If you go back to the original code in public rev 1.1, what you'll
find is that the writes of 'halt' to the BFC addrs above was done
_before_ the pmon/lsipmon loop to write the values.


> > I cannot believe that any monitor does what you describe (puts a table
> > of addresses here), as doing so (instead of putting vectors there)
> > would be fundamentally incompatible with the MIPS architecture.
> 
> I stand corrected.  It doesn't write pointers into chose chunks.  I
> misremembered what I had seen.  The values of `value' that are written
> in the first chunk of code as numbers that range from 6 to 28, except
> for the printf service.  However, I don't see how a code sequence that
> halts the machine could possibly fit there as an entry in the vector
> table.

So, you get to these vectors via exceptions (e.g., TLB exceptions,
address errors, etc.)

The action that the 'halt'-writing code accomplishes is that it
arranges to halt the simulator when you get one of these exceptions.
That seems like a reasonable thing to do.


What I believe you want to do here is:

* do the writes to the vector table first, iff you're using any of the
  monitors.

* then do the additional writes as-is if you're using pmon or lsipmon
  as the monitor.

This has the somewhat odd effect that if you're using lsipmon, the BEV
TLB Miss vector doesn't halt the simulator... but for all I know, the
CPUs you're talking about & for which you'd use 'lsipmon' had no TLBs
or something so that point would be moot.  8-)



chris

Re: MIPS simulator initializes LSI pmon vector table with code

[Alexandre Oliva]

On Apr 19, 2002, cgd@broadcom.com wrote:

>> (where halt is 64-byte long chunk of code, which means that one of
>> these halts overwrites not only its own entry in lsipmon_monitor_base,
>> but also the following entry.)

> Together, you mean?  And I assume you mean 64-bit rather than
> '64-byte'?

Yes, sorry.

> (64 bit or 64-byte, I don't see what you mean by 'overwrites its own
> entry...')

I mean that the entry is supposed to contain a vector number or
something, like what we write in there in the loop above, but the way
the code is written, we get code into it, that doesn't even fit in a
single entry.  This is blatantly, obviously wrong.

> Well, note that I'm not the author of this code, nor was I a
> maintainer when it got into its current state.  I suggest you look at
> the diffs between rev 1.1 and 1.2 -- and then consult fche, who made
> the change -- to find out why those changes were made.

I asked fche, and he agreed my fix was reasonable, so I checked it in,
just to have it reverted by one of the maintainers, and then have the
other maintainers say I should have done exactly what I did :-(

> If you go back to the original code in public rev 1.1, what you'll
> find is that the writes of 'halt' to the BFC addrs above was done
> _before_ the pmon/lsipmon loop to write the values.

Except that there was no pmon/lsipmon in rev 1.1.  Only idtmon.

According to a comment in the code, that was already in rev 1.1:

      /* The PMON monitor uses the same address space, but rather than
         branching into it the address of a routine is loaded. We can
         cheat for the moment, and direct the PMON routine to IDT style
         instructions within the monitor space. This relies on the IDT
         monitor not using the locations from 0xBFC00500 onwards as its
         entry points.*/

Clearly, if pmon (and lsipmon) expect the address of the handler to be
written into that space, writing a HALT instruction will get you the
wrong effect.  Since AFAICT that range is only used by the lsipmon, I
propose that, instead of just `#if'ing out that code, we actually
remove it.

> So, you get to these vectors via exceptions (e.g., TLB exceptions,
> address errors, etc.)

Except that the meaning of the vector varies from one monitor to the
other.  While some expect to find code there, others expect
addresses.

> The action that the 'halt'-writing code accomplishes is that it
> arranges to halt the simulator when you get one of these exceptions.

Nope, it arranges for an invalid instruction to be executed, or a
branch to an invalid address to be taken, if you get the monitor
conventions wrong.  In the case of lsipmon, not only does it change
the entry corresponding to an uninitialized service, but also the
following one, that happens to be a useful one, and one that had been
initialized correctly, just to have the correct value overwritten by
the second half of a halt sequence.

> * do the writes to the vector table first, iff you're using any of the
>   monitors.

> * then do the additional writes as-is if you're using pmon or lsipmon
>   as the monitor.

> This has the somewhat odd effect that if you're using lsipmon, the BEV
> TLB Miss vector doesn't halt the simulator... but for all I know, the
> CPUs you're talking about & for which you'd use 'lsipmon' had no TLBs
> or something so that point would be moot.  8-)

The right thing to do is to not write code into the table if it's not
supposed to contain code.

If anything, one might want to get it to somehow point at code
elsewhere in the monitor address space that would halt the machine.
But writing code there on all monitors can't be right if some monitors
don't take code in there.

-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Red Hat GCC Developer                  aoliva@{cygnus.com, redhat.com}
CS PhD student at IC-Unicamp        oliva@{lsd.ic.unicamp.br, gnu.org}
Free Software Evangelist                Professional serial bug killer

Re: MIPS simulator initializes LSI pmon vector table with code

[cgd]

At 22 Apr 2002 16:08:54 -0300, Alexandre Oliva wrote:
> I asked fche, and he agreed my fix was reasonable, so I checked it in,
> just to have it reverted by one of the maintainers, and then have the
> other maintainers say I should have done exactly what I did :-(

Well, what I was suggesting was meant to be a prefix for "and then
submit it via normal channels" which you didn't do.  8-)


> > If you go back to the original code in public rev 1.1, what you'll
> > find is that the writes of 'halt' to the BFC addrs above was done
> > _before_ the pmon/lsipmon loop to write the values.
> 
> Except that there was no pmon/lsipmon in rev 1.1.  Only idtmon.

Excuse me?

There were no variables to control use of the blocks of code, but the
blocks of code were there and run unconditionally, unless i'm going
blind...

lines 529 - 605 of rev 1.1, or thereabouts.


> According to a comment in the code, that was already in rev 1.1:
> 
>       /* The PMON monitor uses the same address space, but rather than
>          branching into it the address of a routine is loaded. We can
>          cheat for the moment, and direct the PMON routine to IDT style
>          instructions within the monitor space. This relies on the IDT
>          monitor not using the locations from 0xBFC00500 onwards as its
>          entry points.*/
> 
> Clearly, if pmon (and lsipmon) expect the address of the handler to be
> written into that space, writing a HALT instruction will get you the
> wrong effect.  Since AFAICT that range is only used by the lsipmon, I
> propose that, instead of just `#if'ing out that code, we actually
> remove it.

So, go through and look at what the code does:

* the PMON support _does not_ uses the memory at 0xbfc00200, except for
the halt instructions.  its handler address ptrs go at 0xbfc00500.

* lsipmon puts the handler addresses at 0xbfc00200.  If the handler
address setup were run after the halt code init, I believe that would
have the right effect.


I still don't believe you explained if there's anything special about
the LSI MiniRISC parts that makes that 'OK' or 'normal'.  On most MIPS
parts, like I said, those will be the TLB handlers.  (You're
apparently using it, which is why you care about this patch; surely
you can say what it's trying to do!  8-)

If the MiniRISC doesn't have a TLB, that would probably explain it
(but i'd like to know for sure, and it should probably be mentioned in
a comment).

Regardless, if that's what the monitor does, that's what the monitor
does, and that should be supported, but not at the expense of ripping
the 'halt's out for other monitors (which is what you did).


> > The action that the 'halt'-writing code accomplishes is that it
> > arranges to halt the simulator when you get one of these exceptions.
> 
> Nope, it arranges for an invalid instruction to be executed, or a
> branch to an invalid address to be taken, if you get the monitor
> conventions wrong.

That's what the _bug_, for your particular monitor, causes.  I believe
what I stated _is_ the goal of the halt-writing code.  8-)


> The right thing to do is to not write code into the table if it's not
> supposed to contain code.

So,

(1) It's fairly obvious that for other monitors that location _should_
    contain code.

(2) you've not explained why the lsipmon monitor _doesn't_ contain
    code there, at a location where the MIPS architecture says
    _should_ contain executable code.

(3) If you do the halt-writing first, followed by the pmon/lsipmon
    vector init (as revision 1.1 of the file did), I _believe_ all
    would be OK.


I think you have more than enough information to know how fix the
problem properly, at this point.

(I'd still like to know why lsipmon thinks it's OK to put
other-than-code at that vector.)


chris

Re: MIPS simulator initializes LSI pmon vector table with code

[Alexandre Oliva]

On Apr 22, 2002, cgd@broadcom.com wrote:

> Well, what I was suggesting was meant to be a prefix for "and then
> submit it via normal channels" which you didn't do.  8-)

Point taken; I think I have already apologized for crossing the line.
If I didn't, please accept my apologies.  If I did, please accept them
again :-)

>> > If you go back to the original code in public rev 1.1, what you'll
>> > find is that the writes of 'halt' to the BFC addrs above was done
>> > _before_ the pmon/lsipmon loop to write the values.

>> Except that there was no pmon/lsipmon in rev 1.1.  Only idtmon.

> Excuse me?

> There were no variables to control use of the blocks of code, but the
> blocks of code were there and run unconditionally, unless i'm going
> blind...

Sorry, I looked only at the diff between 1.1 and 1.2, and I admit I
must be confused.

> * lsipmon puts the handler addresses at 0xbfc00200.  If the handler
> address setup were run after the halt code init, I believe that would
> have the right effect.

Except that some handlers would be incorrectly set with remnants of
half halt sequences.

> I still don't believe you explained if there's anything special about
> the LSI MiniRISC parts that makes that 'OK' or 'normal'.  On most MIPS
> parts, like I said, those will be the TLB handlers.  (You're
> apparently using it, which is why you care about this patch; surely
> you can say what it's trying to do!  8-)

Now I have to confess that I have absolutely no idea of what's going
on in there, and that I had never heard of these monitors before.  All
I know is that I was debugging some totally unrelated code for a
lsipmon toolchain, and it was crashing at the end.  I eventually
figured out that statement was overwriting the entry for the `write'
service that had been put there in the previous loop, and figured the
`???' comment just before the code meant it was wrong.

In my book, failing to halt on invocation of unregistered services in
certain configurations is much better than failing to run any program
at all on other configurations, so I came up with this patch, ran it
through Frank that agreed it looked reasonable and put it in as, well,
obvious (having no background on the issue and seeing how it was
obviously doing the wrong thing, it didn't occur to me that there
might be more to it that it seemed at first).

Anyway, I don't have sufficient knowledge nor interest in these
matters to remain in this discussion (in fact, my slow response time
can be attributed to the fact that I don't read gdb-patches very
often, I'm significantly behind on it, and I'm not sure when I'm going
to have time to even think about catching up on it :-)

I suggest the maintainers of this simulator to build a mips-lsi-elf
toolchain and attempt to run `make check-gcc' with the simulator to
duplicate the problem I had run into, and investigate more closely
what the correct fix would be.

Thanks,

-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Red Hat GCC Developer                  aoliva@{cygnus.com, redhat.com}
CS PhD student at IC-Unicamp        oliva@{lsd.ic.unicamp.br, gnu.org}
Free Software Evangelist                Professional serial bug killer

Re: MIPS simulator initializes LSI pmon vector table with code

[cgd]

At Wed, 24 Apr 2002 01:24:20 +0000 (UTC), "Alexandre Oliva" wrote:
> > * lsipmon puts the handler addresses at 0xbfc00200.  If the handler
> > address setup were run after the halt code init, I believe that would
> > have the right effect.
> 
> Except that some handlers would be incorrectly set with remnants of
> half halt sequences.

Uh, not true AFAICT.

The LSIPMON handler pointers for which this might be a problem are the
ones when loop == 0 and loop == 1, right?  Since both of those are
written, really, there would be no "half halt sequences" remaining.


> Now I have to confess that I have absolutely no idea of what's going
> on in there, and that I had never heard of these monitors before.

"hmm."


> Anyway, I don't have sufficient knowledge nor interest in these
> matters to remain in this discussion (in fact, my slow response time
> can be attributed to the fact that I don't read gdb-patches very
> often, I'm significantly behind on it, and I'm not sure when I'm going
> to have time to even think about catching up on it :-)
> 
> I suggest the maintainers of this simulator to build a mips-lsi-elf
> toolchain and attempt to run `make check-gcc' with the simulator to
> duplicate the problem I had run into, and investigate more closely
> what the correct fix would be.

That's unfortunate.

I'd certainly be willing, as co-maintainer, to help look into the
problem.  However, my time is rather limited.

The first thing to do there is IMO probably _not_ to run the GCC
tests; you've already verified that they're borken.  It's to find out
exactly what LSIPMON is _supposed_ to be doing, and why.  (The theory
being that it's important to understand what you're trying to change,
before you go and change it.  8-) I think that's the most sane path to
a fix that's likely to work in the longer term.

Do (any of) you have pointers to documentation which will help
elucidate LSIPMON's use of 0xbfc00200?



cgd

Re: MIPS simulator initializes LSI pmon vector table with code

[Andrew Cagney]

> Index: sim/mips/ChangeLog
> from  Alexandre Oliva  <aoliva@redhat.com>
> 
> * interp.c (sim_open): Disable chunk of code that wrote code in
> 	vector table entries.
> 

I've reverted this.  So far I've managed to figure out that the change 
is not ``obvious'' :-)

Andrew