XOR 0, R9 bug in shell.asm?

[sy2002]

@MJoergen found this:

I answered this:

We are talking about this:

https://github.com/sy2002/MiSTer2MEGA65/blob/V2.0.0-release/M2M/rom/shell.asm#L1047

[Rhialto]

Ah, I noticed as well that the write enable for the vdrive track buffer remains on all the time, but even the 4040/8250 I'm importing right now ANDs this with the ACK signal.
https://github.com/eriks5/CBM-II_MiSTer/blob/master/rtl/ieee_drive/ieeedrv_drv.sv#L290
But it would be nicer to turn off the write enable after the writing is done.

[Rhialto]

Further development: I think that some of the bugs I ran into (in the 4040/8250 disk drive I imported from the CBM-II Mister core) are actually caused by the fact that the write enable remains on!

What I see is that the drive wants to save its data, so the qnice code obligingly iterates over the track buffer to fetch all the data.
However when it is finished, and the address is pointing at the last byte of the track buffer, it strobes the ACK signal. And since the write enable is still on, this actually activates a write into the track buffer... corrupting it.

Besides, I had the impression that ACK is supposed to be set first, then all bytes processed, then ACK is cleared. The same for reading and writing. But the qnice code treats reading from the disk buffer differently: it just briefly strobes ACK after reading is basically finished. This seems to work but looking at the 8250's dive code this isn't how things are expected.

Edit: I applied this patch locally and it seems to have solved the remaining issues from eriks5/CBM-II_MiSTer#3

Author: Olaf Seibert <rhialto@falu.nl>
Date:   Mon Jun 30 22:42:05 2025 +0200

    PATCH M2M for bug 52.
    
    https://github.com/sy2002/MiSTer2MEGA65/issues/52
    
    What I see is that the drive wants to save its data, so the qnice code
    iterates over the track buffer to fetch all the data.
    However when it is finished, and the address is pointing at the last
    byte of the track buffer, it strobes the ACK signal. And since the write
    enable is still on, this actually activates a write into the track
    buffer... corrupting it.
    
    Fix: explicitly reset write enable VD_B_WREN (sd_buff_wr in the drive).
    Leave the XOR bug as it is, so that write enable remains on all the time
    during writing into the disk's buffer.

diff --git a/M2M/rom/shell.asm b/M2M/rom/shell.asm
index 2f4d83a..14e333a 100644
--- a/M2M/rom/shell.asm
+++ b/M2M/rom/shell.asm
@@ -1061,6 +1061,12 @@ _HDR_SEND_DONE  MOVE    R11, R8                 ; virtual drive ID
                 XOR     R10, R10
                 RSUB    VD_DRV_WRITE, 1
 
+                ; unassert write enable.                                    ; [Rhialto]
+                ; we don't want it active when reading from the disk later! ; [Rhialto]
+                MOVE    VD_B_WREN, R8           ; [Rhialto] unassert write enable
+                MOVE    0, R9                   ; [Rhialto]
+                RSUB    VD_CAD_WRITE, 1         ; [Rhialto]
+
                 SYSCALL(leave, 1)
                 RET

[sy2002]

Sounds great @Rhialto - I will apply this to the next M2M iteration/release. Can you make a PR?

[Rhialto]

Of course! For the develop branch I expect? And I'll do it with XOR R9,R9 to make it a bit shorter. Edit: #61 (and I just did a copy/paste, and didn't retest. It is too warm today...)

I can think of some further improvements, from simple to complicated:

1. Remove the in-effective XOR 0, R9 / RSUB VD_CAD_WRITE, 1 from the loop to make iterations slightly faster.
2. Remove the code to turn on the write enable to before the loop, again to make iterations slightly faster. It would also need to set up the first address and data correctly, to prevent more messy garbage writes
3. Create some vhdl support for these loops, to make it even super-fast.

The Mister is apparently a lot faster than we are, in servicing the "sd card" requests. Certainly in the case of my 8250, where I need to access hyperram. This long time caused bugs in the drive which wasn't expecting this. So I will take any easy speed-up here.

[sy2002]

@Rhialto Yes, develop branch, please.

In general: The slow speed in our SD card handling is due to a very sub-optimal way how the underlying QNICE SoC handles SD cards in terms of how it buffers things and also when it comes to the actual SD card controller that QNICE uses. So please do not expect any noticable speedup by the optimizations you propose above. When preparing for an upcoming V6 release of the C64 core, we should refactor and optimize a lot of this underlying infrastructure. Otherwise I also see it coming, that for example for an Amiga or Atari ST core, the VDrive system will simply be too slow.

But still, we are doing this to fix a bug. So thank you for the patch :-)

We might also want to think about removing the in-effective "strobing" - or - do a real strobing, i.e. fixing the XOR bug. This code is rather old, so I need to wrap my head around everything in due time.

[Rhialto]

A further thought: perhaps we should test if this bug affects the 1541 in the C64 core. Or if it doesn't, what makes it immune.

A test scenario would probably look something like:

• using a C64 program, write into some sector of some track
• wait long enough that the sector buffer gets copied into the disk image, triggering the corruption
• the program writes another sector of the same track (not the last one)
• then the corrupted track buffer gets saved into the disk image.

That the bug manifested itself more clearly in the 8250 could be called "luck" :-)

[sy2002]

@paich64 can you please work with Rhialto to write a small Basic program and test this?