jacgoudsmit / Propeddle

Software-Defined 6502 Computer
http://www.propeddle.com
19 stars 4 forks source link

issue with read-only mode in PropeddleHub.spin #1

Closed konimaru closed 9 years ago

konimaru commented 9 years ago

Line 181 states that carry is set. This is irrelevant for r/w mode as RangeTestIns1 unconditionally re/sets carry. For r/o mode this becomes an issue because the insn setting the carry flag:

                        jmp     #AccessLoop wc   ' Set the C flag

does in fact clear it. The jump insn does an unsigned overflow test, in this case between the value stored at location 0 (default jmp dst value) and a 9bit literal. The value at location 0 is a move insn which is always greater.

Seeing that shadow[par] isn't used you could easily fix this by using:

                        jmpret  par, #AccessLoop nr,wc   ' Set the C flag
jacgoudsmit commented 9 years ago

Hi Marko, and thanks for your remark!

On page 298 of the Propeller Manual (v1.2), it states that when you execute a JMP instruction [with the WC bit set], the Carry flag is set to 1. There's a footnote that says this always happens unless you somehow manage to execute the instruction from address $1FF which is definitely not happening in PropeddleHub.spin.

Unless the manual is wrong, it's not necessary to change the instruction to always set the C flag to 1, all that's necessary is to execute the instruction from another location than $1FF (and use the WC flag).

Do you have any indication that the current code doesn't work? This would manifest itself by allowing the 6502 to overwrite a hub memory area that should normally be read-only.

===Jac

konimaru commented 9 years ago

The manual is wrong in that respect. All my code (where it counts) uses the unsigned carry behaviour (dst vs src). As for Z, it's only ever set if the return address would be 0 (e.g. insn at $1FF). Unfortunately any code at that location is cancelled (nop) so Z is always cleared provided the jump is taken.

I don't have proof for any failure yet, I just had a few spare minutes at work so I sometimes do code reviews :)

konimaru commented 9 years ago

Note: When I say the manual is wrong I simply mean that I checked on real h/w which acts differently. The datasheet (preferred source) has many empty spots in the insn table so I figured out the missing bits myself.

Stuff like this adds up, e.g.

    * counter FFs (A1/B1) and the frame counter are not reset and hold random
      values after power-up

    * wrxxxx: Z flag
      Z is set when the intrinsic value (long/word/byte) is 0, i.e. writing a
      value of $FF000000 with wrlong will result in Z = 0 while wrword and wrbyte
      (using the same value) result in Z = 1.

Drop me a note if you want a copy (email preferred, nothing here seems to accept ZIP & Co).

jacgoudsmit commented 9 years ago

You're right, the manual is wrong!

I wrote a small test program that can be run on a QuickStart, to prove that the JMP/JMPRET instruction compares the source field with the value at the return location:

PUB main | i
    ' Just turning on one LED to show that program is running
    dira[16]:=1
    outa[16]:=1

    cognew(@toggle, 0)

    repeat while true
      i := 0

DAT
              org 0
toggle
              mov dira, ffffffff
              mov outa, #0 wc                   ' clear CF        

              ' Note: $+1 is 3
              jmpret elsewhere, #$+1 wc,nr

              muxc outa, ffffffff

              ' Infinite loop
              jmp #$              

ffffffff      long $FFFFFFFF

elsewhere
              long 0                            ' Any value 3 or higher clears CF in jmpret                                    

When you store a value of 3 or higher at "elsewhere" (which is what is used as the return address for the jmpret instruction), the jmpret instruction resets the Carry. When you use a lower value, the carry is set. Obviously for jmpret instructions executed from different locations, the value changes correspondingly.

So, generally speaking: the carry is set by a JMPRET instruction if the value that's stored at the return address (destination field) is less (unsigned) than the jump address (source field, immediate or indirect).

When you use JMPRET or CALL to call a subroutine, the RET instruction is really a JMPRET instruction too. That means that the Carry flag is pretty much always cleared (not set) when you do a CALL or JMPRET into a normally coded subroutine.

To accomplish a JMP that always sets the Carry flag (as advertised in the manual), you need to make sure that whatever is stored at location 0 (the return address that's used when you code the JMPRET as JMP) is less than the address you're jumping to. The safest way to do this is to do a MOV 0, #0 before the JMP ... WC. That's probably how I'll change my code.

This is probably a bug in the Propeller that might be put to good use: with carefully crafted machine language (which has to be stored in the exact right place in a cog), a JMP will give you a free partial compare operation.